USB: serial: cp210x: provide gpio valid mask

PATCH In review Johan Hovold 2 participants 7 messages Apr 9, 2021 – Apr 12, 2021

Subsystems (2)

USB USB serial

lore

Share & tools

lore

Johan Hovold

author maintainer

2021-04-09 15:52 UTC #0 lore raw

cover USB: serial: cp210x: provide gpio valid mask

Use the new GPIO valid-mask feature to inform gpiolib which pins are available for use instead of handling that in a request callback.

This also allows user space to figure out which pins are available through the chardev interface without having to request each pin in turn.

Johan

Johan Hovold (2):
USB: serial: cp210x: provide gpio valid mask
USB: serial: cp210x: add gpio-configuration debug printk

 drivers/usb/serial/cp210x.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

Signature · 1 line

2.26.3

Johan Hovold

author maintainer

2021-04-09 15:52 UTC #1 lore raw

PATCH 1/2 USB: serial: cp210x: provide gpio valid mask

Use the new GPIO valid-mask feature to inform gpiolib which pins are available for use instead of handling that in a request callback.

This also allows user space to figure out which pins are available through the chardev interface without having to request each pin in turn.

Note that the return value when requesting an unavailable pin will now be -EINVAL instead of -ENODEV.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/serial/cp210x.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

drivers/usb/serial/cp210x.c +13 ~~-12~~

index a373cd63b3a4..ceb3a656a075 100644
@@ -1410,17 +1410,6 @@ static void cp210x_break_ctl(struct tty_struct *tty, int break_state)
 }
 
 #ifdef CONFIG_GPIOLIB
-static int cp210x_gpio_request(struct gpio_chip *gc, unsigned int offset)
-{
-	struct usb_serial *serial = gpiochip_get_data(gc);
-	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
-
-	if (priv->gpio_altfunc & BIT(offset))
-		return -ENODEV;
-
-	return 0;
-}
-
 static int cp210x_gpio_get(struct gpio_chip *gc, unsigned int gpio)
 {
 	struct usb_serial *serial = gpiochip_get_data(gc);

@@ -1549,6 +1538,18 @@ static int cp210x_gpio_set_config(struct gpio_chip *gc, unsigned int gpio,
 	return -ENOTSUPP;
 }
 
+static int cp210x_gpio_init_valid_mask(struct gpio_chip *gc,
+		unsigned long *valid_mask, unsigned int ngpios)
+{
+	struct usb_serial *serial = gpiochip_get_data(gc);
+	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
+	unsigned long altfunc_mask = priv->gpio_altfunc;
+
+	bitmap_complement(valid_mask, &altfunc_mask, ngpios);
+
+	return 0;
+}
+
 /*
  * This function is for configuring GPIO using shared pins, where other signals
  * are made unavailable by configuring the use of GPIO. This is believed to be

@@ -1786,13 +1787,13 @@ static int cp210x_gpio_init(struct usb_serial *serial)
 		return result;
 
 	priv->gc.label = "cp210x";
-	priv->gc.request = cp210x_gpio_request;
 	priv->gc.get_direction = cp210x_gpio_direction_get;
 	priv->gc.direction_input = cp210x_gpio_direction_input;
 	priv->gc.direction_output = cp210x_gpio_direction_output;
 	priv->gc.get = cp210x_gpio_get;
 	priv->gc.set = cp210x_gpio_set;
 	priv->gc.set_config = cp210x_gpio_set_config;
+	priv->gc.init_valid_mask = cp210x_gpio_init_valid_mask;
 	priv->gc.owner = THIS_MODULE;
 	priv->gc.parent = &serial->interface->dev;
 	priv->gc.base = -1;

Signature · 1 line

2.26.3

Johan Hovold

author maintainer

2021-04-09 15:52 UTC #2 lore raw

PATCH 2/2 USB: serial: cp210x: add gpio-configuration debug printk

In reply to Johan Hovold

Add a debug printk to dump the GPIO configuration stored in EEPROM during probe.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/serial/cp210x.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/usb/serial/cp210x.c +6 -0

index ceb3a656a075..ee595d1bea0a 100644
@@ -1543,10 +1543,16 @@ static int cp210x_gpio_init_valid_mask(struct gpio_chip *gc,
 {
 	struct usb_serial *serial = gpiochip_get_data(gc);
 	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
+	struct device *dev = &serial->interface->dev;
 	unsigned long altfunc_mask = priv->gpio_altfunc;
 
 	bitmap_complement(valid_mask, &altfunc_mask, ngpios);
 
+	if (bitmap_empty(valid_mask, ngpios))
+		dev_dbg(dev, "no pin configured for GPIO\n");
+	else
+		dev_dbg(dev, "GPIO.%*pbl configured for GPIO\n", ngpios,
+				valid_mask);
 	return 0;
 }

Signature · 1 line

2.26.3

Andy Shevchenko

2021-04-09 16:22 UTC #3 lore raw

Re: [PATCH 2/2] USB: serial: cp210x: add gpio-configuration debug printk

Quoted 14 lines · Johan Hovold

Add a debug printk to dump the GPIO configuration stored in EEPROM during probe.
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/cp210x.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/usb/serial/cp210x.c +0 -0
index ceb3a656a075..ee595d1bea0a 100644

drivers/usb/serial/cp210x.c static int cp210x_gpio_init_valid_mask(struct gpio_chip *gc, · line 1552

Quoted 10 lines

drivers/usb/serial/cp210x.c +3 -0

@@ -1543,10 +1543,16 @@ static int cp210x_gpio_init_valid_mask(struct gpio_chip *gc,
 {
        struct usb_serial *serial = gpiochip_get_data(gc);
        struct cp210x_serial_private *priv = usb_get_serial_data(serial);
+       struct device *dev = &serial->interface->dev;
        unsigned long altfunc_mask = priv->gpio_altfunc;
 
        bitmap_complement(valid_mask, &altfunc_mask, ngpios);
 
+       if (bitmap_empty(valid_mask, ngpios))
+               dev_dbg(dev, "no pin configured for GPIO\n");

Shouldn't we drop the GPIO device completely in such a case? Bart, wouldn't it be a good idea for GPIO library to do something like this on driver's behalf?

drivers/usb/serial/cp210x.c static int cp210x_gpio_init_valid_mask(struct gpio_chip *gc, · line 1555

Quoted 3 lines

drivers/usb/serial/cp210x.c +3 -0

+       else
+               dev_dbg(dev, "GPIO.%*pbl configured for GPIO\n", ngpios,
+                               valid_mask);

A nit-pick:
I would change GPIO -> pin in the second message in the first occurrence.

Quoted 6 lines

drivers/usb/serial/cp210x.c +0 -1
        return 0;
 }
 
--
2.26.3

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Johan Hovold

author maintainer

2021-04-09 16:32 UTC #4 lore raw

Re: [PATCH 2/2] USB: serial: cp210x: add gpio-configuration debug printk

Quoted 21 lines · Andy Shevchenko

Quoted 20 lines · Johan Hovold

Add a debug printk to dump the GPIO configuration stored in EEPROM during probe.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/serial/cp210x.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/usb/serial/cp210x.c +1 -0

index ceb3a656a075..ee595d1bea0a 100644
@@ -1543,10 +1543,16 @@ static int cp210x_gpio_init_valid_mask(struct gpio_chip *gc,
 {
        struct usb_serial *serial = gpiochip_get_data(gc);
        struct cp210x_serial_private *priv = usb_get_serial_data(serial);
+       struct device *dev = &serial->interface->dev;
        unsigned long altfunc_mask = priv->gpio_altfunc;

Quoted 6 lines

drivers/usb/serial/cp210x.c static int cp210x_gpio_init_valid_mask(struct gpio_chip *gc, · line 1552
Quoted 4 lines
drivers/usb/serial/cp210x.c +2 -0
        bitmap_complement(valid_mask, &altfunc_mask, ngpios);
 
+       if (bitmap_empty(valid_mask, ngpios))
+               dev_dbg(dev, "no pin configured for GPIO\n");
Shouldn't we drop the GPIO device completely in such a case?

I considered it when we first added support for GPIOs to this driver but decided not to. The reason being that we want to to tell user-space that the device has gpio capability even if the GPIOs are currently muxed (in EEPROM) for other functionality.

Quoted 2 lines

Bart, wouldn't it be a good idea for GPIO library to do something like this on driver's behalf?

I'd say this is mostly useful for hotpluggable devices with EEPROM configuration and is probably best handled by the drivers.

Quoted 6 lines

drivers/usb/serial/cp210x.c static int cp210x_gpio_init_valid_mask(struct gpio_chip *gc, · line 1555
Quoted 3 lines
drivers/usb/serial/cp210x.c +3 -0
+       else
+               dev_dbg(dev, "GPIO.%*pbl configured for GPIO\n", ngpios,
+                               valid_mask);
A nit-pick:
I would change GPIO -> pin in the second message in the first occurrence.

"GPIO.n" are the actual pin names from the datasheet (cf. ftdi_sio which use "CBUSn" here). It's just a debug statement anyway.

Johan

Andy Shevchenko

Reviewed-by on the v1 series

2021-04-09 16:23 UTC #5 lore raw

In reply to Johan Hovold

Quoted 7 lines · Johan Hovold

Use the new GPIO valid-mask feature to inform gpiolib which pins are available for use instead of handling that in a request callback.
This also allows user space to figure out which pins are available through the chardev interface without having to request each pin in turn.

Thanks! I like the series.
Independently on reaction on my comments:

Reviewed-by Andy Shevchenko <[email protected]>

Quoted 13 lines

Johan
Johan Hovold (2):
USB: serial: cp210x: provide gpio valid mask
USB: serial: cp210x: add gpio-configuration debug printk
 drivers/usb/serial/cp210x.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)
--
2.26.3

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Johan Hovold

author maintainer

2021-04-12 10:03 UTC #6 lore raw

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · Johan Hovold
Use the new GPIO valid-mask feature to inform gpiolib which pins are available for use instead of handling that in a request callback.

Quoted 7 lines

Quoted 3 lines
This also allows user space to figure out which pins are available through the chardev interface without having to request each pin in turn.
Thanks! I like the series.
Independently on reaction on my comments:
Reviewed-by Andy Shevchenko <[email protected]>

Thanks for reviewing. Now applied.

Johan