[PATCH 2/2] usb: serial: xr_serial: Add gpiochip support

[PATCH 2/2] usb: serial: xr_serial: Add gpiochip support

[mani]

From: Manivannan Sadhasivam <mani@kernel.org>

Add gpiochip support for Maxlinear/Exar USB to serial converter
for controlling the available gpios.

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 | 186 ++++++++++++++++++++++++++++++++-
 drivers/usb/serial/xr_serial.h |   7 ++
 2 files changed, 192 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
index ea4a0b167d3f..d86fd40839f8 100644
--- a/drivers/usb/serial/xr_serial.c
+++ b/drivers/usb/serial/xr_serial.c
@@ -476,6 +476,189 @@ static void xr_break_ctl(struct tty_struct *tty, int break_state)
 		   state);
 }
 
+#ifdef CONFIG_GPIOLIB
+
+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);
+
+	/* Check if the requested GPIO is occupied */
+	if (port_priv->gpio_altfunc & BIT(offset))
+		return -ENODEV;
+
+	return 0;
+}
+
+static int xr_gpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct usb_serial_port *port = gpiochip_get_data(gc);
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+	int ret;
+	u16 gpio_status;
+
+	ret = xr_get_reg(port, XR21V141X_UART_REG_BLOCK,
+			 port_priv->regs->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);
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+
+	if (val)
+		xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+			   port_priv->regs->gpio_set, BIT(gpio));
+	else
+		xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+			   port_priv->regs->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);
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+	int ret;
+	u16 gpio_dir;
+
+	ret = xr_get_reg(port, XR21V141X_UART_REG_BLOCK,
+			 port_priv->regs->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);
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+	int ret;
+	u16 gpio_dir;
+
+	ret = xr_get_reg(port, XR21V141X_UART_REG_BLOCK,
+			 port_priv->regs->gpio_dir, &gpio_dir);
+	if (ret)
+		return ret;
+
+	gpio_dir &= ~BIT(gpio);
+
+	return xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+			  port_priv->regs->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);
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+	int ret;
+	u16 gpio_dir;
+
+	ret = xr_get_reg(port, XR21V141X_UART_REG_BLOCK,
+			 port_priv->regs->gpio_dir, &gpio_dir);
+	if (ret)
+		return ret;
+
+	gpio_dir |= BIT(gpio);
+
+	ret = xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+			 port_priv->regs->gpio_dir, gpio_dir);
+	if (ret)
+		return ret;
+
+	xr_gpio_set(gc, gpio, val);
+
+	return 0;
+}
+
+static int xr21v141x_gpio_init(struct usb_serial_port *port)
+{
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+	int ret;
+	u16 gpio_mode;
+
+	port_priv->gc.ngpio = 6;
+
+	ret = xr_get_reg(port, XR21V141X_UART_REG_BLOCK,
+			 port_priv->regs->gpio_mode, &gpio_mode);
+	if (ret)
+		return ret;
+
+	/* Mark all pins which are not in GPIO mode */
+	if (gpio_mode & UART_MODE_RTS_CTS)
+		port_priv->gpio_altfunc |= (BIT(4) | BIT(5));
+	else if (gpio_mode & UART_MODE_DTR_DSR)
+		port_priv->gpio_altfunc |= (BIT(2) | BIT(3));
+	else if (gpio_mode & UART_MODE_RS485)
+		port_priv->gpio_altfunc |= BIT(5);
+	else if (gpio_mode & UART_MODE_RS485_ADDR)
+		port_priv->gpio_altfunc |= BIT(5);
+	else
+		port_priv->gpio_altfunc = 0; /* All GPIOs are available */
+
+	return ret;
+}
+
+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;
+
+	if (port_priv->idProduct == XR21V141X_ID)
+		ret = xr21v141x_gpio_init(port);
+
+	if (ret < 0)
+		return ret;
+
+	port_priv->gc.label = "xr_gpios";
+	port_priv->gc.request = xr_gpio_request;
+	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)
+{
+	/* Nothing to do */
+}
+
+#endif
+
 static int xr_port_probe(struct usb_serial_port *port)
 {
 	struct usb_serial *serial = port->serial;
@@ -495,13 +678,14 @@ static int xr_port_probe(struct usb_serial_port *port)
 
 	usb_set_serial_port_data(port, port_priv);
 
-	return 0;
+	return xr_gpio_init(port);
 }
 
 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;
diff --git a/drivers/usb/serial/xr_serial.h b/drivers/usb/serial/xr_serial.h
index d2977ef847a0..079098cf553a 100644
--- a/drivers/usb/serial/xr_serial.h
+++ b/drivers/usb/serial/xr_serial.h
@@ -3,6 +3,8 @@
 #ifndef __LINUX_USB_SERIAL_XR_SERIAL_H
 #define __LINUX_USB_SERIAL_XR_SERIAL_H
 
+#include <linux/gpio/driver.h>
+
 struct xr_uart_regs {
 	u8 enable;
 	u8 format;
@@ -21,6 +23,11 @@ struct xr_uart_regs {
 };
 
 struct xr_port_private {
+#ifdef CONFIG_GPIOLIB
+	struct gpio_chip gc;
+	bool gpio_registered;
+	u8 gpio_altfunc;
+#endif
 	const struct xr_uart_regs *regs;
 	u16 idProduct;
 	u8 reg_width;
-- 
2.17.1

Re: [PATCH 2/2] usb: serial: xr_serial: Add gpiochip support

[Linus Walleij]

On Tue, Apr 28, 2020 at 9:57 PM <mani@kernel.org> wrote:

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

That's a nice and clean GPIO driver.

I would change this:

   port_priv->gc.label = "xr_gpios";

to something that is device-unique, like "xr-gpios-<serial number>"
which makes it easy to locate the GPIOs on a specific serial converter
for lab use. However the USB serial maintainers know better what
to use here. Whatever makes a USB-to-serial unique from a TTY
point of view is probably fine with me too.

My idea is that people might want to know which USB cable
this is sitting on, so I have this USB cable and from this label
I can always figure out which GPIO device it is.

Either way, it is not a super-big issue so:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Is this a off-the-shelf product that can be bought or is it mainly
integrated on boards?

I'm asking because I'm looking for a neat USB-to-serial adapter
with some GPIOs (2 is enough) that can be used for reset and
power cycling of lab boards using one simple piece of equipment.

Yours,
Linus Walleij

Re: [PATCH 2/2] usb: serial: xr_serial: Add gpiochip support

[Manivannan Sadhasivam]

Hi Linus,

On Wed, Apr 29, 2020 at 02:12:24PM +0200, Linus Walleij wrote:
> On Tue, Apr 28, 2020 at 9:57 PM <mani@kernel.org> wrote:
> 
> > From: Manivannan Sadhasivam <mani@kernel.org>
> >
> > Add gpiochip support for Maxlinear/Exar USB to serial converter
> > for controlling the available gpios.
> >
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: linux-gpio@vger.kernel.org
> > Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> 
> That's a nice and clean GPIO driver.

Thanks for the compliments :)

> 
> I would change this:
> 
>    port_priv->gc.label = "xr_gpios";
> 
> to something that is device-unique, like "xr-gpios-<serial number>"
> which makes it easy to locate the GPIOs on a specific serial converter
> for lab use. However the USB serial maintainers know better what
> to use here. Whatever makes a USB-to-serial unique from a TTY
> point of view is probably fine with me too.
> 
> My idea is that people might want to know which USB cable
> this is sitting on, so I have this USB cable and from this label
> I can always figure out which GPIO device it is.
> 

Sounds reasonable. I can postfix the PID as below:

port_priv->gc.label = devm_kasprintf(port->dev, GFP_KERNEL, "XR%04x",
                                     port_priv->idProduct);

So this will become, "XR1410".

> Either way, it is not a super-big issue so:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Is this a off-the-shelf product that can be bought or is it mainly
> integrated on boards?
> 

Both I believe, though I have only used it integrated in dev boards. But
a quick googling gives me below,

https://www.digikey.in/product-detail/en/maxlinear-inc/XR21V1410IL-0C-EB/1016-1425-ND/2636664

Thanks,
Mani

> I'm asking because I'm looking for a neat USB-to-serial adapter
> with some GPIOs (2 is enough) that can be used for reset and
> power cycling of lab boards using one simple piece of equipment.
> 
> Yours,
> Linus Walleij

Re: [PATCH 2/2] usb: serial: xr_serial: Add gpiochip support

[Manivannan Sadhasivam]

Hi Greg,

On Wed, Apr 29, 2020 at 01:26:51AM +0530, mani@kernel.org wrote:
> From: Manivannan Sadhasivam <mani@kernel.org>
> 
> Add gpiochip support for Maxlinear/Exar USB to serial converter
> for controlling the available gpios.
> 
> 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 | 186 ++++++++++++++++++++++++++++++++-
>  drivers/usb/serial/xr_serial.h |   7 ++
>  2 files changed, 192 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
> index ea4a0b167d3f..d86fd40839f8 100644
> --- a/drivers/usb/serial/xr_serial.c
> +++ b/drivers/usb/serial/xr_serial.c
> @@ -476,6 +476,189 @@ static void xr_break_ctl(struct tty_struct *tty, int break_state)
>  		   state);
>  }
>  
> +#ifdef CONFIG_GPIOLIB
> +

[...]

> +
> +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;
> +
> +	if (port_priv->idProduct == XR21V141X_ID)
> +		ret = xr21v141x_gpio_init(port);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	port_priv->gc.label = "xr_gpios";
> +	port_priv->gc.request = xr_gpio_request;
> +	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)
> +{
> +	/* Nothing to do */
> +}
> +
> +#endif
> +
>  static int xr_port_probe(struct usb_serial_port *port)
>  {
>  	struct usb_serial *serial = port->serial;
> @@ -495,13 +678,14 @@ static int xr_port_probe(struct usb_serial_port *port)
>  
>  	usb_set_serial_port_data(port, port_priv);
>  
> -	return 0;
> +	return xr_gpio_init(port);

Just realised that the gpiochip is registered for 2 interfaces exposed by
this chip. This is due to the fact that this chip presents CDC-ACM model,
so there are 2 interfaces (interrupt and bulk IN/OUT).

We shouldn't need gpiochip for interface 0. So what is the recommended way
to filter that?

Thanks,
Mani

>  }
>  
>  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;
> diff --git a/drivers/usb/serial/xr_serial.h b/drivers/usb/serial/xr_serial.h
> index d2977ef847a0..079098cf553a 100644
> --- a/drivers/usb/serial/xr_serial.h
> +++ b/drivers/usb/serial/xr_serial.h
> @@ -3,6 +3,8 @@
>  #ifndef __LINUX_USB_SERIAL_XR_SERIAL_H
>  #define __LINUX_USB_SERIAL_XR_SERIAL_H
>  
> +#include <linux/gpio/driver.h>
> +
>  struct xr_uart_regs {
>  	u8 enable;
>  	u8 format;
> @@ -21,6 +23,11 @@ struct xr_uart_regs {
>  };
>  
>  struct xr_port_private {
> +#ifdef CONFIG_GPIOLIB
> +	struct gpio_chip gc;
> +	bool gpio_registered;
> +	u8 gpio_altfunc;
> +#endif
>  	const struct xr_uart_regs *regs;
>  	u16 idProduct;
>  	u8 reg_width;
> -- 
> 2.17.1
> 

Re: [PATCH 2/2] usb: serial: xr_serial: Add gpiochip support

[Greg KH]

On Wed, Apr 29, 2020 at 11:17:27PM +0530, Manivannan Sadhasivam wrote:
> Hi Greg,
> 
> On Wed, Apr 29, 2020 at 01:26:51AM +0530, mani@kernel.org wrote:
> > From: Manivannan Sadhasivam <mani@kernel.org>
> > 
> > Add gpiochip support for Maxlinear/Exar USB to serial converter
> > for controlling the available gpios.
> > 
> > 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 | 186 ++++++++++++++++++++++++++++++++-
> >  drivers/usb/serial/xr_serial.h |   7 ++
> >  2 files changed, 192 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
> > index ea4a0b167d3f..d86fd40839f8 100644
> > --- a/drivers/usb/serial/xr_serial.c
> > +++ b/drivers/usb/serial/xr_serial.c
> > @@ -476,6 +476,189 @@ static void xr_break_ctl(struct tty_struct *tty, int break_state)
> >  		   state);
> >  }
> >  
> > +#ifdef CONFIG_GPIOLIB
> > +
> 
> [...]
> 
> > +
> > +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;
> > +
> > +	if (port_priv->idProduct == XR21V141X_ID)
> > +		ret = xr21v141x_gpio_init(port);
> > +
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	port_priv->gc.label = "xr_gpios";
> > +	port_priv->gc.request = xr_gpio_request;
> > +	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)
> > +{
> > +	/* Nothing to do */
> > +}
> > +
> > +#endif
> > +
> >  static int xr_port_probe(struct usb_serial_port *port)
> >  {
> >  	struct usb_serial *serial = port->serial;
> > @@ -495,13 +678,14 @@ static int xr_port_probe(struct usb_serial_port *port)
> >  
> >  	usb_set_serial_port_data(port, port_priv);
> >  
> > -	return 0;
> > +	return xr_gpio_init(port);
> 
> Just realised that the gpiochip is registered for 2 interfaces exposed by
> this chip. This is due to the fact that this chip presents CDC-ACM model,
> so there are 2 interfaces (interrupt and bulk IN/OUT).
> 
> We shouldn't need gpiochip for interface 0. So what is the recommended way
> to filter that?

Not create the gpiochip for interface 0?  :)

I really don't know what else to say here, sorry.

greg k-h

Re: [PATCH 2/2] usb: serial: xr_serial: Add gpiochip support

[Johan Hovold]

On Wed, Apr 29, 2020 at 06:19:18PM +0530, Manivannan Sadhasivam wrote:

> On Wed, Apr 29, 2020 at 02:12:24PM +0200, Linus Walleij wrote:
> > On Tue, Apr 28, 2020 at 9:57 PM <mani@kernel.org> wrote:
> > 
> > > From: Manivannan Sadhasivam <mani@kernel.org>
> > >
> > > Add gpiochip support for Maxlinear/Exar USB to serial converter
> > > for controlling the available gpios.
> > >
> > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > Cc: linux-gpio@vger.kernel.org
> > > Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>

> > I would change this:
> > 
> >    port_priv->gc.label = "xr_gpios";
> > 
> > to something that is device-unique, like "xr-gpios-<serial number>"
> > which makes it easy to locate the GPIOs on a specific serial converter
> > for lab use. However the USB serial maintainers know better what
> > to use here. Whatever makes a USB-to-serial unique from a TTY
> > point of view is probably fine with me too.
> > 
> > My idea is that people might want to know which USB cable
> > this is sitting on, so I have this USB cable and from this label
> > I can always figure out which GPIO device it is.

I think we've had this discussion before. First, not every device has a
unique serial number. Second, we already have a universal way of
distinguishing devices namely by using the bus topology. That's
available through sysfs and shouldn't have to be be re-encoded by every
driver in the gpiochip name.

> Sounds reasonable. I can postfix the PID as below:
>
> port_priv->gc.label = devm_kasprintf(port->dev, GFP_KERNEL, "XR%04x",
>                                      port_priv->idProduct);
> 
> So this will become, "XR1410".

So this doesn't really buy us anything; what if you have two of these
devices?

Johan

Re: [PATCH 2/2] usb: serial: xr_serial: Add gpiochip support

[Johan Hovold]

On Wed, Apr 29, 2020 at 11:17:27PM +0530, Manivannan Sadhasivam wrote:

> On Wed, Apr 29, 2020 at 01:26:51AM +0530, mani@kernel.org wrote:
> > From: Manivannan Sadhasivam <mani@kernel.org>
> > 
> > Add gpiochip support for Maxlinear/Exar USB to serial converter
> > for controlling the available gpios.
> > 
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: linux-gpio@vger.kernel.org
> > Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>

> >  static int xr_port_probe(struct usb_serial_port *port)
> >  {
> >  	struct usb_serial *serial = port->serial;
> > @@ -495,13 +678,14 @@ static int xr_port_probe(struct usb_serial_port *port)
> >  
> >  	usb_set_serial_port_data(port, port_priv);
> >  
> > -	return 0;
> > +	return xr_gpio_init(port);
> 
> Just realised that the gpiochip is registered for 2 interfaces exposed by
> this chip. This is due to the fact that this chip presents CDC-ACM model,
> so there are 2 interfaces (interrupt and bulk IN/OUT).
> 
> We shouldn't need gpiochip for interface 0. So what is the recommended way
> to filter that?

Your driver should only bind to the data interface, but also claim the
control interface (i.e. the reverse of what cdc-acm is doing).

This CDC model doesn't really fit the assumptions of usb-serial core,
but it might be doable. Try returning 1 from the attach callback for the
control interface so that core claims it but doesn't register a tty
device.

Johan

Re: [PATCH 2/2] usb: serial: xr_serial: Add gpiochip support

[Linus Walleij]

On Tue, May 19, 2020 at 10:57 AM Johan Hovold <johan@kernel.org> wrote:
> > On Wed, Apr 29, 2020 at 02:12:24PM +0200, Linus Walleij wrote:

> > > to something that is device-unique, like "xr-gpios-<serial number>"
> > > which makes it easy to locate the GPIOs on a specific serial converter
> > > for lab use. However the USB serial maintainers know better what
> > > to use here. Whatever makes a USB-to-serial unique from a TTY
> > > point of view is probably fine with me too.
> > >
> > > My idea is that people might want to know which USB cable
> > > this is sitting on, so I have this USB cable and from this label
> > > I can always figure out which GPIO device it is.
>
> I think we've had this discussion before. First, not every device has a
> unique serial number. Second, we already have a universal way of
> distinguishing devices namely by using the bus topology. That's
> available through sysfs and shouldn't have to be be re-encoded by every
> driver in the gpiochip name.

I remember I even referred to this myself, but I've been waning a bit
on it recently, because it turns out that userspace/users aren't very
good at parsing sysfs for topology.

For userspace other than udev there seems to be a kind of agreement
gap. Dunno how best to bridge it though. Education maybe.

Yours,
Linus Walleij

Re: [PATCH 2/2] usb: serial: xr_serial: Add gpiochip support

[Greg KH]

On Mon, May 25, 2020 at 10:59:59AM +0200, Linus Walleij wrote:
> On Tue, May 19, 2020 at 10:57 AM Johan Hovold <johan@kernel.org> wrote:
> > > On Wed, Apr 29, 2020 at 02:12:24PM +0200, Linus Walleij wrote:
> 
> > > > to something that is device-unique, like "xr-gpios-<serial number>"
> > > > which makes it easy to locate the GPIOs on a specific serial converter
> > > > for lab use. However the USB serial maintainers know better what
> > > > to use here. Whatever makes a USB-to-serial unique from a TTY
> > > > point of view is probably fine with me too.
> > > >
> > > > My idea is that people might want to know which USB cable
> > > > this is sitting on, so I have this USB cable and from this label
> > > > I can always figure out which GPIO device it is.
> >
> > I think we've had this discussion before. First, not every device has a
> > unique serial number. Second, we already have a universal way of
> > distinguishing devices namely by using the bus topology. That's
> > available through sysfs and shouldn't have to be be re-encoded by every
> > driver in the gpiochip name.
> 
> I remember I even referred to this myself, but I've been waning a bit
> on it recently, because it turns out that userspace/users aren't very
> good at parsing sysfs for topology.

Which is why they could use libudev :)

thanks,

greg k-h

Re: [PATCH 2/2] usb: serial: xr_serial: Add gpiochip support

[Linus Walleij]

On Mon, May 25, 2020 at 1:12 PM Greg KH <gregkh@linuxfoundation.org> wrote:

> > I remember I even referred to this myself, but I've been waning a bit
> > on it recently, because it turns out that userspace/users aren't very
> > good at parsing sysfs for topology.
>
> Which is why they could use libudev :)

Yet they insist on using things like Busybox' mdev (e.g. OpenWrt)
or Android ... or is Android using libudev now? I'd be delighted
if they did.

Yours,
Linus Walleij

Re: [PATCH 2/2] usb: serial: xr_serial: Add gpiochip support

[Greg KH]

On Mon, May 25, 2020 at 03:02:15PM +0200, Linus Walleij wrote:
> On Mon, May 25, 2020 at 1:12 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > > I remember I even referred to this myself, but I've been waning a bit
> > > on it recently, because it turns out that userspace/users aren't very
> > > good at parsing sysfs for topology.
> >
> > Which is why they could use libudev :)
> 
> Yet they insist on using things like Busybox' mdev (e.g. OpenWrt)
> or Android ... or is Android using libudev now? I'd be delighted
> if they did.

No, Android is not using libudev yet, they seem to be reinventing the
same thing, slowly, over time :(

greg k-h