diff mbox series

[v5,2/3] usb: serial: xr_serial: Add gpiochip support

Message ID 20201122170822.21715-3-mani@kernel.org
State New
Headers show
Series Add support for MaxLinear/Exar USB to serial converters | expand

Commit Message

Manivannan Sadhasivam Nov. 22, 2020, 5:08 p.m. UTC
Add gpiochip support for Maxlinear/Exar USB to serial converter
for controlling the available gpios.

Inspired from cp210x usb to serial converter driver.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: linux-gpio@vger.kernel.org
Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
---
 drivers/usb/serial/xr_serial.c | 267 ++++++++++++++++++++++++++++++++-
 1 file changed, 261 insertions(+), 6 deletions(-)

Comments

Linus Walleij Dec. 1, 2020, 2:37 p.m. UTC | #1
On Sun, Nov 22, 2020 at 6:08 PM Manivannan Sadhasivam <mani@kernel.org> wrote:

> Add gpiochip support for Maxlinear/Exar USB to serial converter
> for controlling the available gpios.
>
> Inspired from cp210x usb to serial converter driver.
>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: linux-gpio@vger.kernel.org
> Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>

This looks good to me overall, provided that it plays well with the
serial port.

One minor notice:

> +enum gpio_pins {
> +       GPIO_RI = 0,
> +       GPIO_CD,
> +       GPIO_DSR,
> +       GPIO_DTR,
> +       GPIO_CTS,
> +       GPIO_RTS,
> +       GPIO_MAX,
> +};

You know the names of the pins...

> +       port_priv->gc.ngpio = 6;
> +       port_priv->gc.label = "xr_gpios";
> +       port_priv->gc.request = xr_gpio_request;
> +       port_priv->gc.free = xr_gpio_free;
> +       port_priv->gc.get_direction = xr_gpio_direction_get;
> +       port_priv->gc.direction_input = xr_gpio_direction_input;
> +       port_priv->gc.direction_output = xr_gpio_direction_output;
> +       port_priv->gc.get = xr_gpio_get;
> +       port_priv->gc.set = xr_gpio_set;
> +       port_priv->gc.owner = THIS_MODULE;
> +       port_priv->gc.parent = &port->dev;
> +       port_priv->gc.base = -1;
> +       port_priv->gc.can_sleep = true;

So assign port_priv->gc.names here as well with an array
of strings with the names ("RI", "CD", ... etc).
This makes it look really nice in userspace if you do
e.g. "lsgpio".

With that:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
Johan Hovold Dec. 1, 2020, 3:51 p.m. UTC | #2
On Tue, Dec 01, 2020 at 03:37:38PM +0100, Linus Walleij wrote:
> On Sun, Nov 22, 2020 at 6:08 PM Manivannan Sadhasivam <mani@kernel.org> wrote:
> 
> > Add gpiochip support for Maxlinear/Exar USB to serial converter
> > for controlling the available gpios.

> One minor notice:
> 
> > +enum gpio_pins {
> > +       GPIO_RI = 0,
> > +       GPIO_CD,
> > +       GPIO_DSR,
> > +       GPIO_DTR,
> > +       GPIO_CTS,
> > +       GPIO_RTS,
> > +       GPIO_MAX,
> > +};
> 
> You know the names of the pins...
> 
> > +       port_priv->gc.ngpio = 6;
> > +       port_priv->gc.label = "xr_gpios";
> > +       port_priv->gc.request = xr_gpio_request;
> > +       port_priv->gc.free = xr_gpio_free;
> > +       port_priv->gc.get_direction = xr_gpio_direction_get;
> > +       port_priv->gc.direction_input = xr_gpio_direction_input;
> > +       port_priv->gc.direction_output = xr_gpio_direction_output;
> > +       port_priv->gc.get = xr_gpio_get;
> > +       port_priv->gc.set = xr_gpio_set;
> > +       port_priv->gc.owner = THIS_MODULE;
> > +       port_priv->gc.parent = &port->dev;
> > +       port_priv->gc.base = -1;
> > +       port_priv->gc.can_sleep = true;
> 
> So assign port_priv->gc.names here as well with an array
> of strings with the names ("RI", "CD", ... etc).
> This makes it look really nice in userspace if you do
> e.g. "lsgpio".

Last time we tried that gpiolib still used a flat namespace so that you
can't have have more than one device using the same names. Unless that
has changed this is a no-go. See

	https://lore.kernel.org/r/20180930122703.7115-1-johan@kernel.org

for our previous discussion about this.

Johan
Manivannan Sadhasivam Dec. 1, 2020, 6 p.m. UTC | #3
Hi Linus,

On Tue, Dec 01, 2020 at 03:37:38PM +0100, Linus Walleij wrote:
> On Sun, Nov 22, 2020 at 6:08 PM Manivannan Sadhasivam <mani@kernel.org> wrote:

> 

> > Add gpiochip support for Maxlinear/Exar USB to serial converter

> > for controlling the available gpios.

> >

> > Inspired from cp210x usb to serial converter driver.

> >

> > Cc: Linus Walleij <linus.walleij@linaro.org>

> > Cc: linux-gpio@vger.kernel.org

> > Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>

> 

> This looks good to me overall, provided that it plays well with the

> serial port.

> 

> One minor notice:

> 

> > +enum gpio_pins {

> > +       GPIO_RI = 0,

> > +       GPIO_CD,

> > +       GPIO_DSR,

> > +       GPIO_DTR,

> > +       GPIO_CTS,

> > +       GPIO_RTS,

> > +       GPIO_MAX,

> > +};

> 

> You know the names of the pins...

> 

> > +       port_priv->gc.ngpio = 6;

> > +       port_priv->gc.label = "xr_gpios";

> > +       port_priv->gc.request = xr_gpio_request;

> > +       port_priv->gc.free = xr_gpio_free;

> > +       port_priv->gc.get_direction = xr_gpio_direction_get;

> > +       port_priv->gc.direction_input = xr_gpio_direction_input;

> > +       port_priv->gc.direction_output = xr_gpio_direction_output;

> > +       port_priv->gc.get = xr_gpio_get;

> > +       port_priv->gc.set = xr_gpio_set;

> > +       port_priv->gc.owner = THIS_MODULE;

> > +       port_priv->gc.parent = &port->dev;

> > +       port_priv->gc.base = -1;

> > +       port_priv->gc.can_sleep = true;

> 

> So assign port_priv->gc.names here as well with an array

> of strings with the names ("RI", "CD", ... etc).

> This makes it look really nice in userspace if you do

> e.g. "lsgpio".

> 


As Johan stated, this doesn't work with multiple devices attached to the system.
That's the reason for not adding the line names.

This gives me the motivation to get my hands dirty with gpiolib (but I fear of
breaking the ABI)...

> With that:

> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

> 


Thanks for the review!

Regards,
Mani

> Yours,

> Linus Walleij
Linus Walleij Dec. 5, 2020, 10:21 p.m. UTC | #4
On Tue, Dec 1, 2020 at 4:50 PM Johan Hovold <johan@kernel.org> wrote:
> On Tue, Dec 01, 2020 at 03:37:38PM +0100, Linus Walleij wrote:
> > On Sun, Nov 22, 2020 at 6:08 PM Manivannan Sadhasivam <mani@kernel.org> wrote:

> > You know the names of the pins...
> >
> > > +       port_priv->gc.ngpio = 6;
> > > +       port_priv->gc.label = "xr_gpios";
> > > +       port_priv->gc.request = xr_gpio_request;
> > > +       port_priv->gc.free = xr_gpio_free;
> > > +       port_priv->gc.get_direction = xr_gpio_direction_get;
> > > +       port_priv->gc.direction_input = xr_gpio_direction_input;
> > > +       port_priv->gc.direction_output = xr_gpio_direction_output;
> > > +       port_priv->gc.get = xr_gpio_get;
> > > +       port_priv->gc.set = xr_gpio_set;
> > > +       port_priv->gc.owner = THIS_MODULE;
> > > +       port_priv->gc.parent = &port->dev;
> > > +       port_priv->gc.base = -1;
> > > +       port_priv->gc.can_sleep = true;
> >
> > So assign port_priv->gc.names here as well with an array
> > of strings with the names ("RI", "CD", ... etc).
> > This makes it look really nice in userspace if you do
> > e.g. "lsgpio".
>
> Last time we tried that gpiolib still used a flat namespace so that you
> can't have have more than one device using the same names. Unless that
> has changed this is a no-go. See
>
>         https://lore.kernel.org/r/20180930122703.7115-1-johan@kernel.org
>
> for our previous discussion about this.

Hm hm yeah we actually put in a nasty warning there since:

                gpio = gpio_name_to_desc(gc->names[i]);
                if (gpio)
                        dev_warn(&gdev->dev,
                                 "Detected name collision for GPIO name '%s'\n",
                                 gc->names[i]);


A better approach might be to create an array of names
prepended with something device-unique like the USB
bus topology? Or do we need a helper to help naming the
GPIOs? What would be helpful here?

name = kasprintf(GFP_KERNEL, "%s-NAME", topology_str);

Yours,
Linus Walleij
Linus Walleij Dec. 8, 2020, 12:41 p.m. UTC | #5
On Tue, Dec 8, 2020 at 10:57 AM Johan Hovold <johan@kernel.org> wrote:
> [Me]


> > A better approach might be to create an array of names

> > prepended with something device-unique like the USB

> > bus topology? Or do we need a helper to help naming the

> > GPIOs? What would be helpful here?

> >

> > name = kasprintf(GFP_KERNEL, "%s-NAME", topology_str);

>

> Well we started discussing this back when we only had the sysfs

> interface which suffered from the same problem. I thought the chardev

> interface was supposed to get rid of the assumption of a flat name

> space? Perhaps in v3 of the ABI. ;P


It's "mostly true" that the line names are unique per-chip actually,
because people don't like the nasty warning message. I wonder
if anything would really break if I go in and make a patch to
enforce it, since all drivers passing ->names in the gpiochip
are in the kernel we can check them all.

If the names are unique-per-chip, we can add a restriction like this
with the requirement:

depends on !GPIO_SYSFS

so it can't even be compiled in if someone is using the sysfs.

That should solve the situation where people are (ab)using
the sysfs and getting name collisions as a result.

Then it should be fine for any driver to provide a names array
provided all the names are unique on that gpiochip.

I doubt it would break anything, but let's see what Geert says.
He has some special usecases in the gpio-aggregator driver
which will incidentally look for just linenames when
aggregating gpios, but I feel it is a bit thick for it to work
with multiple hot-pluggable GPIO chips as well, I don't think
that is its usecase. (We all want to be perfect but...)

> But what about any other non-pluggable

> IC, which provides a few named GPIO lines and of which there could be

> more than one in a system?


I think if there are such, and the lines are unique per-chip
we should make the drivers depend on !GPIO_SYSFS.

> The topology is already encoded in sysfs and it seems backwards to have

> each and every gpio driver reconstruct it.


I agree.

I think if this driver already has unique line-names per-gpiochip
we could actually make it depend on !GPIO_SYSFS and
just add the names.

Yours,
Linus Walleij
Manivannan Sadhasivam Dec. 8, 2020, 12:52 p.m. UTC | #6
On Tue, Dec 08, 2020 at 01:41:52PM +0100, Linus Walleij wrote:
> On Tue, Dec 8, 2020 at 10:57 AM Johan Hovold <johan@kernel.org> wrote:
> > [Me]
> 
> > > A better approach might be to create an array of names
> > > prepended with something device-unique like the USB
> > > bus topology? Or do we need a helper to help naming the
> > > GPIOs? What would be helpful here?
> > >
> > > name = kasprintf(GFP_KERNEL, "%s-NAME", topology_str);
> >
> > Well we started discussing this back when we only had the sysfs
> > interface which suffered from the same problem. I thought the chardev
> > interface was supposed to get rid of the assumption of a flat name
> > space? Perhaps in v3 of the ABI. ;P
> 
> It's "mostly true" that the line names are unique per-chip actually,
> because people don't like the nasty warning message. I wonder
> if anything would really break if I go in and make a patch to
> enforce it, since all drivers passing ->names in the gpiochip
> are in the kernel we can check them all.
> 
> If the names are unique-per-chip, we can add a restriction like this
> with the requirement:
> 
> depends on !GPIO_SYSFS
> 

This sounds reasonable to me.

> so it can't even be compiled in if someone is using the sysfs.
> 
> That should solve the situation where people are (ab)using
> the sysfs and getting name collisions as a result.
> 
> Then it should be fine for any driver to provide a names array
> provided all the names are unique on that gpiochip.
> 
> I doubt it would break anything, but let's see what Geert says.
> He has some special usecases in the gpio-aggregator driver
> which will incidentally look for just linenames when
> aggregating gpios, but I feel it is a bit thick for it to work
> with multiple hot-pluggable GPIO chips as well, I don't think
> that is its usecase. (We all want to be perfect but...)
> 
> > But what about any other non-pluggable
> > IC, which provides a few named GPIO lines and of which there could be
> > more than one in a system?
> 
> I think if there are such, and the lines are unique per-chip
> we should make the drivers depend on !GPIO_SYSFS.
> 
> > The topology is already encoded in sysfs and it seems backwards to have
> > each and every gpio driver reconstruct it.
> 
> I agree.
> 
> I think if this driver already has unique line-names per-gpiochip
> we could actually make it depend on !GPIO_SYSFS and
> just add the names.
> 

Sure thing.

Johan, if you are okay with this I can resubmit incorporating Linus's
suggestion.

Thanks,
Mani

> Yours,
> Linus Walleij
Johan Hovold Dec. 9, 2020, 3:25 p.m. UTC | #7
On Tue, Dec 08, 2020 at 01:41:52PM +0100, Linus Walleij wrote:
> On Tue, Dec 8, 2020 at 10:57 AM Johan Hovold <johan@kernel.org> wrote:


> > Well we started discussing this back when we only had the sysfs

> > interface which suffered from the same problem. I thought the chardev

> > interface was supposed to get rid of the assumption of a flat name

> > space? Perhaps in v3 of the ABI. ;P

> 

> It's "mostly true" that the line names are unique per-chip actually,

> because people don't like the nasty warning message. I wonder

> if anything would really break if I go in and make a patch to

> enforce it, since all drivers passing ->names in the gpiochip

> are in the kernel we can check them all.

> 

> If the names are unique-per-chip, we can add a restriction like this

> with the requirement:

> 

> depends on !GPIO_SYSFS

> 

> so it can't even be compiled in if someone is using the sysfs.

>

> That should solve the situation where people are (ab)using

> the sysfs and getting name collisions as a result.


Would it possible to set a flag to suppress just the sysfs entry
renaming instead?

Despite its flaws the sysfs interface is still very convenient and I'd
prefer not to disable it just because of the line names.

> Then it should be fine for any driver to provide a names array

> provided all the names are unique on that gpiochip.


So it sounds like there's nothing preventing per-chip-unique names in
the rest of gpiolib and the new chardev interface then? Are the
user-space libraries able to cope with it, etc?

> I doubt it would break anything, but let's see what Geert says.

> He has some special usecases in the gpio-aggregator driver

> which will incidentally look for just linenames when

> aggregating gpios, but I feel it is a bit thick for it to work

> with multiple hot-pluggable GPIO chips as well, I don't think

> that is its usecase. (We all want to be perfect but...)


Ok.

> > But what about any other non-pluggable

> > IC, which provides a few named GPIO lines and of which there could be

> > more than one in a system?

> 

> I think if there are such, and the lines are unique per-chip

> we should make the drivers depend on !GPIO_SYSFS.


Or just suppress the sysfs-entry renaming if that's the only thing
that's blocking this.

Johan
Johan Hovold Dec. 9, 2020, 3:31 p.m. UTC | #8
On Tue, Dec 08, 2020 at 06:22:50PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Dec 08, 2020 at 01:41:52PM +0100, Linus Walleij wrote:
> > On Tue, Dec 8, 2020 at 10:57 AM Johan Hovold <johan@kernel.org> wrote:

> > I think if this driver already has unique line-names per-gpiochip
> > we could actually make it depend on !GPIO_SYSFS and
> > just add the names.
> 
> Sure thing.
> 
> Johan, if you are okay with this I can resubmit incorporating Linus's
> suggestion.

Let's wait a bit with adding the names.

That can possibly be done as a follow-up too even if removing GPIO_SYSFS
support later is not ideal in case that's the path chosen (we'd have a
similar problem with the existing USB-serial GPIO implementations though).

Johan
Linus Walleij Dec. 9, 2020, 4:25 p.m. UTC | #9
On Wed, Dec 9, 2020 at 4:24 PM Johan Hovold <johan@kernel.org> wrote:
> On Tue, Dec 08, 2020 at 01:41:52PM +0100, Linus Walleij wrote:


> > depends on !GPIO_SYSFS

> >

> > so it can't even be compiled in if someone is using the sysfs.

> >

> > That should solve the situation where people are (ab)using

> > the sysfs and getting name collisions as a result.

>

> Would it possible to set a flag to suppress just the sysfs entry

> renaming instead?


Hm you mean that when a GPIO is "exported" in sysfs
it should not get a symbolic name from the names but instead
just the number?

I bet someone has written their scripts to take advantage of
the symbolic names so I suspect the task becomes bigger
like suppress the sysfs entry renaming if and only if there is
a namespace collision.

But I think we can do that, doesn't seem too hard?

I just hacked up this:
https://lore.kernel.org/linux-gpio/20201209161821.92931-1-linus.walleij@linaro.org/T/#u

> Despite its flaws the sysfs interface is still very convenient and I'd

> prefer not to disable it just because of the line names.


Would these conveniences be identical to those listed
in my recent TODO entry?
https://lore.kernel.org/linux-gpio/20201204083533.65830-1-linus.walleij@linaro.org/

There are several other issues with the sysfs, so making it conflict
with other drivers is almost  plus in the direction of discouragement
from the GPIO submaintainer point of view, but I do see that
people like it for the reasons in the TODO. :/

I am strongly encouraging any developer with a few spare cycles
on their hands to go and implement the debugfs facility because
we can make it so much better than the sysfs, easier and
more convenient for testing etc.

> > Then it should be fine for any driver to provide a names array

> > provided all the names are unique on that gpiochip.

>

> So it sounds like there's nothing preventing per-chip-unique names in

> the rest of gpiolib and the new chardev interface then? Are the

> user-space libraries able to cope with it, etc?


Yes the documentation refers to libgpiod a very well maintained
library:
https://www.kernel.org/doc/html/latest/driver-api/gpio/using-gpio.html
https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/

Then there are the the example tools included with the kernel
that provide a second implementation for the same interfaces
using just the C standard library:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/gpio

I usually use the tools myself.

Yours,
Linus Walleij
Johan Hovold Dec. 10, 2020, 8:53 a.m. UTC | #10
On Wed, Dec 09, 2020 at 05:25:32PM +0100, Linus Walleij wrote:
> On Wed, Dec 9, 2020 at 4:24 PM Johan Hovold <johan@kernel.org> wrote:

> > On Tue, Dec 08, 2020 at 01:41:52PM +0100, Linus Walleij wrote:

> 

> > > depends on !GPIO_SYSFS

> > >

> > > so it can't even be compiled in if someone is using the sysfs.

> > >

> > > That should solve the situation where people are (ab)using

> > > the sysfs and getting name collisions as a result.

> >

> > Would it possible to set a flag to suppress just the sysfs entry

> > renaming instead?

> 

> Hm you mean that when a GPIO is "exported" in sysfs

> it should not get a symbolic name from the names but instead

> just the number?


Right.

> I bet someone has written their scripts to take advantage of

> the symbolic names so I suspect the task becomes bigger

> like suppress the sysfs entry renaming if and only if there is

> a namespace collision.

> 

> But I think we can do that, doesn't seem too hard?

> 

> I just hacked up this:

> https://lore.kernel.org/linux-gpio/20201209161821.92931-1-linus.walleij@linaro.org/T/#u


I just replied to that thread, but to summarize, you can't rely on
having the sysfs code detect collisions since that will trigger a bunch
of nasty warnings and backtraces. We also don't want the sysfs interface
for a specific USB device to depend on probe order (only the first one
plugged in gets to use the line names). And adding line names now could
in fact be what breaks currently working scripts.

> > Despite its flaws the sysfs interface is still very convenient and I'd

> > prefer not to disable it just because of the line names.

> 

> Would these conveniences be identical to those listed

> in my recent TODO entry?

> https://lore.kernel.org/linux-gpio/20201204083533.65830-1-linus.walleij@linaro.org/


Indeed.

> There are several other issues with the sysfs, so making it conflict

> with other drivers is almost  plus in the direction of discouragement

> from the GPIO submaintainer point of view, but I do see that

> people like it for the reasons in the TODO. :/

> 

> I am strongly encouraging any developer with a few spare cycles

> on their hands to go and implement the debugfs facility because

> we can make it so much better than the sysfs, easier and

> more convenient for testing etc.


Don't you run the risk of having people enable debugfs in production
systems now just so they can use the old-style interface?

Side note: if you skip the "export" part of the interface, how would you
indicate that a line is already in use or not available (e.g.
gpio-range-reserved)?

> > > Then it should be fine for any driver to provide a names array

> > > provided all the names are unique on that gpiochip.

> >

> > So it sounds like there's nothing preventing per-chip-unique names in

> > the rest of gpiolib and the new chardev interface then? Are the

> > user-space libraries able to cope with it, etc?

> 

> Yes the documentation refers to libgpiod a very well maintained

> library:

> https://www.kernel.org/doc/html/latest/driver-api/gpio/using-gpio.html

> https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/


Just did a super quick check and it seems libgpiod still assumes a flat
name space. For example, gpiod_chip_find_line() returns only the first
line found that matches a name. Shouldn't be impossible to extend, but
just want to make sure this flat namespace assumption hasn't been to
heavily relied upon.

Johan
Johan Hovold Dec. 10, 2020, 9:04 a.m. UTC | #11
On Thu, Dec 10, 2020 at 09:53:45AM +0100, Johan Hovold wrote:
> On Wed, Dec 09, 2020 at 05:25:32PM +0100, Linus Walleij wrote:
> > On Wed, Dec 9, 2020 at 4:24 PM Johan Hovold <johan@kernel.org> wrote:

> > > So it sounds like there's nothing preventing per-chip-unique names in
> > > the rest of gpiolib and the new chardev interface then? Are the
> > > user-space libraries able to cope with it, etc?
> > 
> > Yes the documentation refers to libgpiod a very well maintained
> > library:
> > https://www.kernel.org/doc/html/latest/driver-api/gpio/using-gpio.html
> > https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/
> 
> Just did a super quick check and it seems libgpiod still assumes a flat
> name space. For example, gpiod_chip_find_line() returns only the first
> line found that matches a name. Shouldn't be impossible to extend, but
> just want to make sure this flat namespace assumption hasn't been to
> heavily relied upon.

That should have been gpiod_line_find() (which in turn uses the above
mentioned helper for per-chip lookups).

Johan
Johan Hovold Dec. 14, 2020, 8:58 a.m. UTC | #12
On Sat, Dec 12, 2020 at 01:03:32AM +0100, Linus Walleij wrote:
> On Thu, Dec 10, 2020 at 9:53 AM Johan Hovold <johan@kernel.org> wrote:

> > On Wed, Dec 09, 2020 at 05:25:32PM +0100, Linus Walleij wrote:

> 

> > I just replied to that thread, but to summarize, you can't rely on

> > having the sysfs code detect collisions since that will trigger a bunch

> > of nasty warnings and backtraces. We also don't want the sysfs interface

> > for a specific USB device to depend on probe order (only the first one

> > plugged in gets to use the line names). And adding line names now could

> > in fact be what breaks currently working scripts.

> 

> Yes the sysfs ABI is very volatile and easy to break.

> 

> As pointed out in the other reply, sysfs base GPIO number is all

> wibbly-wobbly on anything hot-pluggable so in a way I feel it

> is the right thing to disallow sysfs altogether on hotpluggable

> devices.


No, the gpio numbers will of course vary, but since gpiolib exports the
base number for the chip, a scripts can easily determine the right gpio
number as base + offset.

Having probe order break that by sometimes exporting the line using it's
name is what would be a problem.

> > Just did a super quick check and it seems libgpiod still assumes a flat

> > name space. For example, gpiod_chip_find_line() returns only the first

> > line found that matches a name. Shouldn't be impossible to extend, but

> > just want to make sure this flat namespace assumption hasn't been to

> > heavily relied upon.

> 

> The unique way to identify a GPIO is gpiochip instance (with

> topology from sysfs) and then a line number on that chip.

> This is done e.g. in the example tool

> tools/gpio/gpio-hammer.c

> 

> As you can see the tool doesn't use these line names.

>

> The line names are really like symbolic links or something.

> But they are indeed in a flat namespace so we should try to

> at least make them unique if it turns out people love to use

> these.


Not necessarily, they could be unique per chip as we're discussing here
with respect to hotpluggable controllers. We just can't have it both
ways.

> As it is now system designers mostly use device tree to assign

> line names and they try to make these unique because they don't

> like the nasty warnings from gpiolib.

>

> If I google for the phrase "Detected name collision for GPIO name"

> I just find the code, our discussions and some USB serial devices

> warning about this so far.

> 

> Maybe we should just make a patch to disallow it?


That would make it impossible to provide name lines on hotpluggable
controllers, which would be nice to support.

Johan
Linus Walleij Dec. 14, 2020, 9:19 a.m. UTC | #13
On Mon, Dec 14, 2020 at 9:58 AM Johan Hovold <johan@kernel.org> wrote:
> On Sat, Dec 12, 2020 at 01:03:32AM +0100, Linus Walleij wrote:

> > If I google for the phrase "Detected name collision for GPIO name"
> > I just find the code, our discussions and some USB serial devices
> > warning about this so far.
> >
> > Maybe we should just make a patch to disallow it?
>
> That would make it impossible to provide name lines on hotpluggable
> controllers, which would be nice to support.

I merged a patch for this now, let's tighten this loose end up.

Also: thanks for poking me about this, I should have looked into
this ages ago :/ focus you know...

Yours,
Linus Walleij
Johan Hovold Dec. 14, 2020, 9:31 a.m. UTC | #14
On Mon, Dec 14, 2020 at 10:19:07AM +0100, Linus Walleij wrote:
> On Mon, Dec 14, 2020 at 9:58 AM Johan Hovold <johan@kernel.org> wrote:

> > On Sat, Dec 12, 2020 at 01:03:32AM +0100, Linus Walleij wrote:

> 

> > > If I google for the phrase "Detected name collision for GPIO name"

> > > I just find the code, our discussions and some USB serial devices

> > > warning about this so far.

> > >

> > > Maybe we should just make a patch to disallow it?

> >

> > That would make it impossible to provide name lines on hotpluggable

> > controllers, which would be nice to support.

> 

> I merged a patch for this now, let's tighten this loose end up.

> 

> Also: thanks for poking me about this, I should have looked into

> this ages ago :/ focus you know...


Yeah, tell me about it. :/

Johan
Johan Hovold Jan. 21, 2021, 11:06 a.m. UTC | #15
On Sun, Nov 22, 2020 at 10:38:21PM +0530, Manivannan Sadhasivam wrote:
> Add gpiochip support for Maxlinear/Exar USB to serial converter

> for controlling the available gpios.

> 

> Inspired from cp210x usb to serial converter driver.

> 

> Cc: Linus Walleij <linus.walleij@linaro.org>

> Cc: linux-gpio@vger.kernel.org

> Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>

> ---

>  drivers/usb/serial/xr_serial.c | 267 ++++++++++++++++++++++++++++++++-

>  1 file changed, 261 insertions(+), 6 deletions(-)

> 

> diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c

> index e166916554d6..ca63527a5310 100644

> --- a/drivers/usb/serial/xr_serial.c

> +++ b/drivers/usb/serial/xr_serial.c

> @@ -9,6 +9,7 @@

>   * Copyright (c) 2020 Manivannan Sadhasivam <mani@kernel.org>

>   */

>  

> +#include <linux/gpio/driver.h>

>  #include <linux/kernel.h>

>  #include <linux/module.h>

>  #include <linux/slab.h>

> @@ -16,6 +17,28 @@

>  #include <linux/usb.h>

>  #include <linux/usb/serial.h>

>  

> +#ifdef CONFIG_GPIOLIB

> +enum gpio_pins {

> +	GPIO_RI = 0,

> +	GPIO_CD,

> +	GPIO_DSR,

> +	GPIO_DTR,

> +	GPIO_CTS,

> +	GPIO_RTS,

> +	GPIO_MAX,

> +};

> +#endif

> +

> +struct xr_port_private {

> +#ifdef CONFIG_GPIOLIB

> +	struct gpio_chip gc;

> +	bool gpio_registered;

> +	enum gpio_pins pin_status[GPIO_MAX];


This isn't an array of enum gpio_pins, rather you use the enum as an
index into the array which only stores a boolean value per pin.

Please rename the array and fix the type (e.g. bool) so that it is clear
how from the name how it is being used, for example, "gpio_requested" or
"gpio_in_use".

> +#endif

> +	struct mutex gpio_lock;	/* protects GPIO state */

> +	bool port_open;

> +};

> +

>  struct xr_txrx_clk_mask {

>  	u16 tx;

>  	u16 rx0;

> @@ -106,6 +129,8 @@ struct xr_txrx_clk_mask {

>  #define XR21V141X_REG_GPIO_CLR		0x1e

>  #define XR21V141X_REG_GPIO_STATUS	0x1f

>  

> +static int xr_cts_rts_check(struct xr_port_private *port_priv);

> +

>  static int xr_set_reg(struct usb_serial_port *port, u8 block, u8 reg,

>  		      u8 val)

>  {

> @@ -309,6 +334,7 @@ static int xr_uart_disable(struct usb_serial_port *port)

>  static void xr_set_flow_mode(struct tty_struct *tty,

>  			     struct usb_serial_port *port)

>  {

> +	struct xr_port_private *port_priv = usb_get_serial_port_data(port);

>  	unsigned int cflag = tty->termios.c_cflag;

>  	int ret;

>  	u8 flow, gpio_mode;

> @@ -318,6 +344,17 @@ static void xr_set_flow_mode(struct tty_struct *tty,

>  		return;

>  

>  	if (cflag & CRTSCTS) {

> +		/*

> +		 * Check if the CTS/RTS pins are occupied by GPIOLIB. If yes,


GPIOLIB doesn't "occupy" anything. Use something like "claimed by"
instead.

> +		 * then use no flow control.

> +		 */

> +		if (xr_cts_rts_check(port_priv)) {

> +			dev_dbg(&port->dev,

> +				"CTS/RTS pins are occupied, so disabling flow control\n");


Ditto.

And there's no need to ignore a request for sw flow control if the pins
are in use. Just move the check above the conditional and make the
first branch depend on it.

You also need to clear CRTSCTS in termios if it cannot be set.

> +			flow = XR21V141X_UART_FLOW_MODE_NONE;

> +			goto exit;

> +		}

> +

>  		dev_dbg(&port->dev, "Enabling hardware flow ctrl\n");

>  

>  		/*

> @@ -341,6 +378,7 @@ static void xr_set_flow_mode(struct tty_struct *tty,

>  		flow = XR21V141X_UART_FLOW_MODE_NONE;

>  	}

>  

> +exit:

>  	/*

>  	 * As per the datasheet, UART needs to be disabled while writing to

>  	 * FLOW_CONTROL register.

> @@ -355,18 +393,22 @@ static void xr_set_flow_mode(struct tty_struct *tty,

>  static int xr_tiocmset_port(struct usb_serial_port *port,

>  			    unsigned int set, unsigned int clear)

>  {

> +	struct xr_port_private *port_priv = usb_get_serial_port_data(port);

>  	u8 gpio_set = 0;

>  	u8 gpio_clr = 0;

>  	int ret = 0;

>  

> -	/* Modem control pins are active low, so set & clr are swapped */

> -	if (set & TIOCM_RTS)

> +	/*

> +	 * Modem control pins are active low, so set & clr are swapped. And

> +	 * ignore the pins that are occupied by GPIOLIB.

> +	 */

> +	if ((set & TIOCM_RTS) && !(port_priv->pin_status[GPIO_RTS]))

>  		gpio_clr |= XR21V141X_UART_MODE_RTS;

> -	if (set & TIOCM_DTR)

> +	if ((set & TIOCM_DTR) && !(port_priv->pin_status[GPIO_DTR]))

>  		gpio_clr |= XR21V141X_UART_MODE_DTR;

> -	if (clear & TIOCM_RTS)

> +	if ((clear & TIOCM_RTS) && !(port_priv->pin_status[GPIO_RTS]))

>  		gpio_set |= XR21V141X_UART_MODE_RTS;

> -	if (clear & TIOCM_DTR)

> +	if ((clear & TIOCM_DTR) && !(port_priv->pin_status[GPIO_DTR]))

>  		gpio_set |= XR21V141X_UART_MODE_DTR;

>  

>  	/* Writing '0' to gpio_{set/clr} bits has no effect, so no need to do */

> @@ -464,6 +506,7 @@ static void xr_set_termios(struct tty_struct *tty,

>  

>  static int xr_open(struct tty_struct *tty, struct usb_serial_port *port)

>  {

> +	struct xr_port_private *port_priv = usb_get_serial_port_data(port);

>  	int ret;

>  

>  	ret = xr_uart_enable(port);

> @@ -482,13 +525,23 @@ static int xr_open(struct tty_struct *tty, struct usb_serial_port *port)

>  		return ret;

>  	}

>  

> +	mutex_lock(&port_priv->gpio_lock);

> +	port_priv->port_open = true;

> +	mutex_unlock(&port_priv->gpio_lock);


This needs to be done before calling set_termios() above.

> +

>  	return 0;

>  }

>  

>  static void xr_close(struct usb_serial_port *port)

>  {

> +	struct xr_port_private *port_priv = usb_get_serial_port_data(port);

> +

>  	usb_serial_generic_close(port);

>  

> +	mutex_lock(&port_priv->gpio_lock);

> +	port_priv->port_open = false;

> +	mutex_unlock(&port_priv->gpio_lock);

> +

>  	xr_uart_disable(port);

>  }

>  

> @@ -553,13 +606,215 @@ static void xr_break_ctl(struct tty_struct *tty, int break_state)

>  	xr_set_reg_uart(port, XR21V141X_REG_TX_BREAK, state);

>  }

>  

> -static int xr_port_probe(struct usb_serial_port *port)

> +#ifdef CONFIG_GPIOLIB

> +

> +static int xr_cts_rts_check(struct xr_port_private *port_priv)

>  {

> +	if (port_priv->pin_status[GPIO_RTS] || port_priv->pin_status[GPIO_CTS])

> +		return -EBUSY;

> +

>  	return 0;

>  }

>  

> +static int xr_gpio_request(struct gpio_chip *gc, unsigned int offset)

> +{

> +	struct usb_serial_port *port = gpiochip_get_data(gc);

> +	struct xr_port_private *port_priv = usb_get_serial_port_data(port);

> +	int ret = 0;

> +

> +	mutex_lock(&port_priv->gpio_lock);

> +	/*

> +	 * Do not proceed if the port is open. This is done to avoid changing

> +	 * the GPIO configuration used by the serial driver.

> +	 */

> +	if (port_priv->port_open) {

> +		ret = -EBUSY;

> +		goto err_out;

> +	}

> +

> +	/* Mark the GPIO pin as busy */

> +	port_priv->pin_status[offset] = true;


What about GPIO5/RTS#? It may still be configured for flow control even
if the port is now closed. You need to switch to GPIO mode.

> +

> +err_out:

> +	mutex_unlock(&port_priv->gpio_lock);

> +

> +	return ret;

> +}

> +

> +static void xr_gpio_free(struct gpio_chip *gc, unsigned int offset)

> +{

> +	struct usb_serial_port *port = gpiochip_get_data(gc);

> +	struct xr_port_private *port_priv = usb_get_serial_port_data(port);

> +

> +	mutex_lock(&port_priv->gpio_lock);

> +	/*

> +	 * Do not proceed if the port is open. This is done to avoid toggling

> +	 * of pins suddenly when the serial port is in use.

> +	 */

> +	if (port_priv->port_open)

> +		goto err_out;

> +

> +	/* Mark the GPIO pin as free */

> +	port_priv->pin_status[offset] = false;


You cannot fail this even if the port is open since otherwise the pin
will remain claimed.

You may need to cache the valid pins at serial port open instead as I
already mentioned.

Also, you need to figure out how to coordinate the pin-configuration
with the serial driver. I suggested added a call to configure DTR/RTS at
outputs on open(), but perhaps this should only be done once in case
they are not really used for modem control and instead needs to be
reconfigured as inputs by the gpio driver (i.e. before opening the
port). 

I'm still not sure about how best to handle this remuxing at runtime in
order to avoid having incidentally setting up an incorrect pin
configuration.

Ideally, the muxing should be determined in EEPROM or be based on
VID/PID and not be allowed to change at runtime at all...

I'm not convinced that the current approach is a good idea yet.

> +

> +err_out:

> +	mutex_unlock(&port_priv->gpio_lock);

> +}


Johan
diff mbox series

Patch

diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
index e166916554d6..ca63527a5310 100644
--- a/drivers/usb/serial/xr_serial.c
+++ b/drivers/usb/serial/xr_serial.c
@@ -9,6 +9,7 @@ 
  * Copyright (c) 2020 Manivannan Sadhasivam <mani@kernel.org>
  */
 
+#include <linux/gpio/driver.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/slab.h>
@@ -16,6 +17,28 @@ 
 #include <linux/usb.h>
 #include <linux/usb/serial.h>
 
+#ifdef CONFIG_GPIOLIB
+enum gpio_pins {
+	GPIO_RI = 0,
+	GPIO_CD,
+	GPIO_DSR,
+	GPIO_DTR,
+	GPIO_CTS,
+	GPIO_RTS,
+	GPIO_MAX,
+};
+#endif
+
+struct xr_port_private {
+#ifdef CONFIG_GPIOLIB
+	struct gpio_chip gc;
+	bool gpio_registered;
+	enum gpio_pins pin_status[GPIO_MAX];
+#endif
+	struct mutex gpio_lock;	/* protects GPIO state */
+	bool port_open;
+};
+
 struct xr_txrx_clk_mask {
 	u16 tx;
 	u16 rx0;
@@ -106,6 +129,8 @@  struct xr_txrx_clk_mask {
 #define XR21V141X_REG_GPIO_CLR		0x1e
 #define XR21V141X_REG_GPIO_STATUS	0x1f
 
+static int xr_cts_rts_check(struct xr_port_private *port_priv);
+
 static int xr_set_reg(struct usb_serial_port *port, u8 block, u8 reg,
 		      u8 val)
 {
@@ -309,6 +334,7 @@  static int xr_uart_disable(struct usb_serial_port *port)
 static void xr_set_flow_mode(struct tty_struct *tty,
 			     struct usb_serial_port *port)
 {
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
 	unsigned int cflag = tty->termios.c_cflag;
 	int ret;
 	u8 flow, gpio_mode;
@@ -318,6 +344,17 @@  static void xr_set_flow_mode(struct tty_struct *tty,
 		return;
 
 	if (cflag & CRTSCTS) {
+		/*
+		 * Check if the CTS/RTS pins are occupied by GPIOLIB. If yes,
+		 * then use no flow control.
+		 */
+		if (xr_cts_rts_check(port_priv)) {
+			dev_dbg(&port->dev,
+				"CTS/RTS pins are occupied, so disabling flow control\n");
+			flow = XR21V141X_UART_FLOW_MODE_NONE;
+			goto exit;
+		}
+
 		dev_dbg(&port->dev, "Enabling hardware flow ctrl\n");
 
 		/*
@@ -341,6 +378,7 @@  static void xr_set_flow_mode(struct tty_struct *tty,
 		flow = XR21V141X_UART_FLOW_MODE_NONE;
 	}
 
+exit:
 	/*
 	 * As per the datasheet, UART needs to be disabled while writing to
 	 * FLOW_CONTROL register.
@@ -355,18 +393,22 @@  static void xr_set_flow_mode(struct tty_struct *tty,
 static int xr_tiocmset_port(struct usb_serial_port *port,
 			    unsigned int set, unsigned int clear)
 {
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
 	u8 gpio_set = 0;
 	u8 gpio_clr = 0;
 	int ret = 0;
 
-	/* Modem control pins are active low, so set & clr are swapped */
-	if (set & TIOCM_RTS)
+	/*
+	 * Modem control pins are active low, so set & clr are swapped. And
+	 * ignore the pins that are occupied by GPIOLIB.
+	 */
+	if ((set & TIOCM_RTS) && !(port_priv->pin_status[GPIO_RTS]))
 		gpio_clr |= XR21V141X_UART_MODE_RTS;
-	if (set & TIOCM_DTR)
+	if ((set & TIOCM_DTR) && !(port_priv->pin_status[GPIO_DTR]))
 		gpio_clr |= XR21V141X_UART_MODE_DTR;
-	if (clear & TIOCM_RTS)
+	if ((clear & TIOCM_RTS) && !(port_priv->pin_status[GPIO_RTS]))
 		gpio_set |= XR21V141X_UART_MODE_RTS;
-	if (clear & TIOCM_DTR)
+	if ((clear & TIOCM_DTR) && !(port_priv->pin_status[GPIO_DTR]))
 		gpio_set |= XR21V141X_UART_MODE_DTR;
 
 	/* Writing '0' to gpio_{set/clr} bits has no effect, so no need to do */
@@ -464,6 +506,7 @@  static void xr_set_termios(struct tty_struct *tty,
 
 static int xr_open(struct tty_struct *tty, struct usb_serial_port *port)
 {
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
 	int ret;
 
 	ret = xr_uart_enable(port);
@@ -482,13 +525,23 @@  static int xr_open(struct tty_struct *tty, struct usb_serial_port *port)
 		return ret;
 	}
 
+	mutex_lock(&port_priv->gpio_lock);
+	port_priv->port_open = true;
+	mutex_unlock(&port_priv->gpio_lock);
+
 	return 0;
 }
 
 static void xr_close(struct usb_serial_port *port)
 {
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+
 	usb_serial_generic_close(port);
 
+	mutex_lock(&port_priv->gpio_lock);
+	port_priv->port_open = false;
+	mutex_unlock(&port_priv->gpio_lock);
+
 	xr_uart_disable(port);
 }
 
@@ -553,13 +606,215 @@  static void xr_break_ctl(struct tty_struct *tty, int break_state)
 	xr_set_reg_uart(port, XR21V141X_REG_TX_BREAK, state);
 }
 
-static int xr_port_probe(struct usb_serial_port *port)
+#ifdef CONFIG_GPIOLIB
+
+static int xr_cts_rts_check(struct xr_port_private *port_priv)
 {
+	if (port_priv->pin_status[GPIO_RTS] || port_priv->pin_status[GPIO_CTS])
+		return -EBUSY;
+
 	return 0;
 }
 
+static int xr_gpio_request(struct gpio_chip *gc, unsigned int offset)
+{
+	struct usb_serial_port *port = gpiochip_get_data(gc);
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+	int ret = 0;
+
+	mutex_lock(&port_priv->gpio_lock);
+	/*
+	 * Do not proceed if the port is open. This is done to avoid changing
+	 * the GPIO configuration used by the serial driver.
+	 */
+	if (port_priv->port_open) {
+		ret = -EBUSY;
+		goto err_out;
+	}
+
+	/* Mark the GPIO pin as busy */
+	port_priv->pin_status[offset] = true;
+
+err_out:
+	mutex_unlock(&port_priv->gpio_lock);
+
+	return ret;
+}
+
+static void xr_gpio_free(struct gpio_chip *gc, unsigned int offset)
+{
+	struct usb_serial_port *port = gpiochip_get_data(gc);
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+
+	mutex_lock(&port_priv->gpio_lock);
+	/*
+	 * Do not proceed if the port is open. This is done to avoid toggling
+	 * of pins suddenly when the serial port is in use.
+	 */
+	if (port_priv->port_open)
+		goto err_out;
+
+	/* Mark the GPIO pin as free */
+	port_priv->pin_status[offset] = false;
+
+err_out:
+	mutex_unlock(&port_priv->gpio_lock);
+}
+
+static int xr_gpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct usb_serial_port *port = gpiochip_get_data(gc);
+	u8 gpio_status;
+	int ret;
+
+	ret = xr_get_reg_uart(port, XR21V141X_REG_GPIO_STATUS, &gpio_status);
+	if (ret)
+		return ret;
+
+	return !!(gpio_status & BIT(gpio));
+}
+
+static void xr_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+	struct usb_serial_port *port = gpiochip_get_data(gc);
+
+	if (val)
+		xr_set_reg_uart(port, XR21V141X_REG_GPIO_SET, BIT(gpio));
+	else
+		xr_set_reg_uart(port, XR21V141X_REG_GPIO_CLR, BIT(gpio));
+}
+
+static int xr_gpio_direction_get(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct usb_serial_port *port = gpiochip_get_data(gc);
+	u8 gpio_dir;
+	int ret;
+
+	ret = xr_get_reg_uart(port, XR21V141X_REG_GPIO_DIR, &gpio_dir);
+	if (ret)
+		return ret;
+
+	/* Logic 0 = input and Logic 1 = output */
+	return !(gpio_dir & BIT(gpio));
+}
+
+static int xr_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct usb_serial_port *port = gpiochip_get_data(gc);
+	u8 gpio_dir;
+	int ret;
+
+	ret = xr_get_reg_uart(port, XR21V141X_REG_GPIO_DIR, &gpio_dir);
+	if (ret)
+		return ret;
+
+	gpio_dir &= ~BIT(gpio);
+
+	return xr_set_reg_uart(port, XR21V141X_REG_GPIO_DIR, gpio_dir);
+}
+
+static int xr_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio,
+				    int val)
+{
+	struct usb_serial_port *port = gpiochip_get_data(gc);
+	u8 gpio_dir;
+	int ret;
+
+	xr_gpio_set(gc, gpio, val);
+
+	ret = xr_get_reg_uart(port, XR21V141X_REG_GPIO_DIR, &gpio_dir);
+	if (ret)
+		return ret;
+
+	gpio_dir |= BIT(gpio);
+
+	ret = xr_set_reg_uart(port, XR21V141X_REG_GPIO_DIR, gpio_dir);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int xr_gpio_init(struct usb_serial_port *port)
+{
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+	int ret = 0;
+
+	port_priv->gc.ngpio = 6;
+	port_priv->gc.label = "xr_gpios";
+	port_priv->gc.request = xr_gpio_request;
+	port_priv->gc.free = xr_gpio_free;
+	port_priv->gc.get_direction = xr_gpio_direction_get;
+	port_priv->gc.direction_input = xr_gpio_direction_input;
+	port_priv->gc.direction_output = xr_gpio_direction_output;
+	port_priv->gc.get = xr_gpio_get;
+	port_priv->gc.set = xr_gpio_set;
+	port_priv->gc.owner = THIS_MODULE;
+	port_priv->gc.parent = &port->dev;
+	port_priv->gc.base = -1;
+	port_priv->gc.can_sleep = true;
+
+	ret = gpiochip_add_data(&port_priv->gc, port);
+	if (!ret)
+		port_priv->gpio_registered = true;
+
+	return ret;
+}
+
+static void xr_gpio_remove(struct usb_serial_port *port)
+{
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+
+	if (port_priv->gpio_registered) {
+		gpiochip_remove(&port_priv->gc);
+		port_priv->gpio_registered = false;
+	}
+}
+
+#else
+
+static int xr_gpio_init(struct usb_serial_port *port)
+{
+	return 0;
+}
+
+static void xr_gpio_remove(struct usb_serial_port *port)
+{
+}
+
+static int xr_cts_rts_check(struct xr_port_private *port_priv)
+{
+	return 0;
+}
+
+#endif
+
+static int xr_port_probe(struct usb_serial_port *port)
+{
+	struct xr_port_private *port_priv;
+	int ret;
+
+	port_priv = kzalloc(sizeof(*port_priv), GFP_KERNEL);
+	if (!port_priv)
+		return -ENOMEM;
+
+	usb_set_serial_port_data(port, port_priv);
+	mutex_init(&port_priv->gpio_lock);
+
+	ret = xr_gpio_init(port);
+	if (ret)
+		kfree(port_priv);
+
+	return ret;
+}
+
 static int xr_port_remove(struct usb_serial_port *port)
 {
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+
+	xr_gpio_remove(port);
+	kfree(port_priv);
+
 	return 0;
 }