Skip to contentSkip to conversation

usb:serial:pl2303: add GPIOs interface on PL2303

PATCH Superseded Wang YanQing 4 participants 11 messages Aug 9, 2014 – Aug 29, 2014

Subsystems (2)
USB USB serial

v7 of 8 Superseded Latest is v8 →

lore
Share & tools
lore

Wang YanQing

author
#0 lore raw

PATCH v7 1/1 usb:serial:pl2303: add GPIOs interface on PL2303

PL2303 USB Serial devices always has GPIOs, this patch add generic gpio support interfaces for pl2303 USB Serial devices in pl2303_type_data and pl2303_serial_private, then use them to add GPIOs support for one type of them, PL2303HXA.

Known issue:
If gpios are in use(export to userspace through sysfs interface, etc), then call pl2303_release(unplug usb-serial convertor, modprobe -r, etc), will cause trouble, so we need to make sure there is no gpio user before call pl2303_release.

Signed-off-by Wang YanQing <[email protected]>

---
Changes
v6-v7:
1: add generic gpio support interfaces for pl2303 USB Serial devices
in pl2303_type_data and pl2303_serial_private suggested by Andreas Mohr.
2: drop different label names for different pl2303 instance suggested by Johan Hovold. 3: fix missing lock corrected by Johan Hovold.
4: use prefix pl2303hx_gpio instead pl2303_gpio, pl2303_gpio is over generic for current code,
and now we move gpio_startup|gpio_release into type-specified data structure, so pl2303hx_gpio is a better prefix.
5: make words in Kconfig a little more useful and clarified.
6: many misc code quality enhancement suggested by Johan Hovold.

v5-v6:
1: fix typo error in Kconfig reported by Andreas Mohr
2: add const qulification to gpio_dir_mask and gpio_value_mask suggested by Andreas Mohr

v4-v5:
1: fix gpio_chip.lable been overwrited by template_chip.
2: use idr to manage minor instead of crude monotonous atomic increment.

v3-v4:
1: fix missing static for gpio_dir_mask and gpio_value_mask
2: reduce unneeded compile macro defined suggested by [email protected] 3: use true instead of 1 corrected by Linus Walleij
4: ignore return value of gpiochip_remove suggested by Linus Walleij 5: fix multi gpio_chips registered by pl2303 can't be distinguished in kernel space.

v2-v3:
1: fix errors and warnings reported by Daniele Forsi checked with checkpatch.pl 2: fix missing GPIOLIB dependence in Kconfig
3: fix pl2303_gpio_get can't work

v1-v2:
1:drop gpio-pl2303.c and relation stuff
2:hang gpio stuff off of pl2303.c

 drivers/usb/serial/Kconfig  |  10 +++
 drivers/usb/serial/pl2303.c | 202 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 212 insertions(+)
drivers/usb/serial/Kconfig +10 -0
index 3ce5c74..008ec57 100644
@@ -516,6 +516,16 @@ config USB_SERIAL_PL2303
 	  To compile this driver as a module, choose M here: the
 	  module will be called pl2303.
 
+config USB_SERIAL_PL2303_GPIO
+	bool "USB Prolific 2303 Single Port GPIOs support"
+	depends on USB_SERIAL_PL2303 && GPIOLIB
+	---help---
+	  Say Y here if you want to use GPIOs on PL2303 USB Serial single port
+	  adapter from Prolific.
+
+	  It supports 2 dedicated GPIOs on PL2303HXA only currently.
+	  If unsure, say N.
+
 config USB_SERIAL_OTI6858
 	tristate "USB Ours Technology Inc. OTi-6858 USB To RS232 Bridge Controller"
 	help
drivers/usb/serial/pl2303.c +202 -0

254 lines not on this page · raw message

Signature · 1 line
1.8.5.5.dirty
maintainer
#1 lore raw
Quoted 1 line · Wang YanQing

PL2303 USB Serial devices always has GPIOs,

Always? Are you sure? It's probably better to write "might have" as they are unlikely to be accessible even if the pins exist.

Quoted 3 lines

this patch add
generic gpio support interfaces for pl2303 USB Serial devices in pl2303_type_data and pl2303_serial_private, then use them

No need to mention these implementation details.

Quoted 7 lines

to add GPIOs support for one type of them, PL2303HXA.

Known issue:
If gpios are in use(export to userspace through sysfs interface, etc), then call pl2303_release(unplug usb-serial convertor, modprobe -r, etc), will cause trouble, so we need to make sure there is no gpio user before call pl2303_release.

This is a real problem that we need to address. gpiolib isn't really able to handle devices that just disappear. In fact, it's API claims that we must not call gpiochip_remove with requested gpios and this is exactly what you might do in pl2303hx_gpio_release below.

As I mentioned earlier, this crashes the kernel when a new gpiochip is later added (the gpiochip data structures are likely corrupted and we get a NULL pointer deref in gpiochip_find_base).

Linus, any thoughts on this?

Quoted 11 lines
Signed-off-by Wang YanQing <[email protected]>

---
Changes
v6-v7:
1: add generic gpio support interfaces for pl2303 USB Serial devices
in pl2303_type_data and pl2303_serial_private suggested by Andreas Mohr.
2: drop different label names for different pl2303 instance suggested by Johan Hovold. 3: fix missing lock corrected by Johan Hovold.
4: use prefix pl2303hx_gpio instead pl2303_gpio, pl2303_gpio is over generic for current code,
and now we move gpio_startup|gpio_release into type-specified data structure, so pl2303hx_gpio is a better prefix.

I'm not sure that was such a good idea. More below.

Quoted 75 lines

5: make words in Kconfig a little more useful and clarified. 6: many misc code quality enhancement suggested by Johan Hovold.

v5-v6:
1: fix typo error in Kconfig reported by Andreas Mohr
2: add const qulification to gpio_dir_mask and gpio_value_mask suggested by Andreas Mohr

v4-v5:
1: fix gpio_chip.lable been overwrited by template_chip.
2: use idr to manage minor instead of crude monotonous atomic increment.

v3-v4:
1: fix missing static for gpio_dir_mask and gpio_value_mask
2: reduce unneeded compile macro defined suggested by [email protected] 3: use true instead of 1 corrected by Linus Walleij
4: ignore return value of gpiochip_remove suggested by Linus Walleij 5: fix multi gpio_chips registered by pl2303 can't be distinguished in kernel space.

v2-v3:
1: fix errors and warnings reported by Daniele Forsi checked with checkpatch.pl 2: fix missing GPIOLIB dependence in Kconfig
3: fix pl2303_gpio_get can't work

v1-v2:
1:drop gpio-pl2303.c and relation stuff
2:hang gpio stuff off of pl2303.c

 drivers/usb/serial/Kconfig  |  10 +++
 drivers/usb/serial/pl2303.c | 202 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 212 insertions(+)
drivers/usb/serial/Kconfig +10 -0
index 3ce5c74..008ec57 100644
@@ -516,6 +516,16 @@ config USB_SERIAL_PL2303
 	  To compile this driver as a module, choose M here: the
 	  module will be called pl2303.
 
+config USB_SERIAL_PL2303_GPIO
+	bool "USB Prolific 2303 Single Port GPIOs support"
+	depends on USB_SERIAL_PL2303 && GPIOLIB
+	---help---
+	  Say Y here if you want to use GPIOs on PL2303 USB Serial single port
+	  adapter from Prolific.
+
+	  It supports 2 dedicated GPIOs on PL2303HXA only currently.
+	  If unsure, say N.
+
 config USB_SERIAL_OTI6858
 	tristate "USB Ours Technology Inc. OTi-6858 USB To RS232 Bridge Controller"
 	help
drivers/usb/serial/pl2303.c +4 -0
index b3d5a35..edbe634 100644
@@ -28,6 +28,8 @@
 #include <linux/usb.h>
 #include <linux/usb/serial.h>
 #include <asm/unaligned.h>
+#include <linux/gpio.h>
+#include <linux/mutex.h>
 #include "pl2303.h"
 
 

@@ -131,6 +133,8 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define UART_OVERRUN_ERROR		0x40
 #define UART_CTS			0x80
 
+#define PL2303HX_REG_GPIO_CONTROL	0x01
+#define PL2303HX_REG_GPIO_STATE	0x81
 
 enum pl2303_type {
 	TYPE_01,	/* Type 0 and 1 (difference unknown) */

drivers/usb/serial/pl2303.c enum pl2303_type { · line 148

Quoted 4 lines
drivers/usb/serial/pl2303.c +1 -0
@@ -141,11 +145,15 @@ enum pl2303_type {
 struct pl2303_type_data {
 	speed_t max_baud_rate;
 	unsigned long quirks;
+	u16 ngpio;

u8 should be enough.

drivers/usb/serial/pl2303.c enum pl2303_type { · line 150

Quoted 2 lines
drivers/usb/serial/pl2303.c +2 -0
+	int (*gpio_startup)(struct usb_serial *serial);
+	void (*gpio_release)(struct usb_serial *serial);

This isn't the right place for this abstraction. Most of the setup code would be common for any device type with GPIOs.

Just keep the generic gpio_startup and release from v6, and verify that ngpio > 0. Any further abstraction should only be added once we know how the other types handles GPIOs.

drivers/usb/serial/pl2303.c enum pl2303_type { · line 156

Quoted 6 lines
drivers/usb/serial/pl2303.c +1 -0
 };
 
 struct pl2303_serial_private {
 	const struct pl2303_type_data *type;
 	unsigned long quirks;
+	void *gpio;

No void pointers, please.

Quoted 4 lines
drivers/usb/serial/pl2303.c +0 -0
 };
 
 struct pl2303_private {

drivers/usb/serial/pl2303.c struct pl2303_private { · line 173

Quoted 10 lines
drivers/usb/serial/pl2303.c +7 -0
@@ -156,11 +164,24 @@ struct pl2303_private {
 	u8 line_settings[7];
 };
 
+#ifdef CONFIG_USB_SERIAL_PL2303_GPIO
+static int pl2303hx_gpio_startup(struct usb_serial *serial);
+static void pl2303hx_gpio_release(struct usb_serial *serial);
+#else
+static inline int pl2303hx_gpio_startup(struct usb_serial *serial) { return 0; }
+static inline void pl2303hx_gpio_release(struct usb_serial *serial) {}
+#endif

So rename these back to pl2303_gpio_startup/release again.

Drop the hx suffix you just added from all functions until we have a need to differentiate the types.

Quoted 18 lines
drivers/usb/serial/pl2303.c +6 -0
+
 static const struct pl2303_type_data pl2303_type_data[TYPE_COUNT] = {
 	[TYPE_01] = {
 		.max_baud_rate =	1228800,
 		.quirks =		PL2303_QUIRK_LEGACY,
 	},
+	[TYPE_HX] = {
+		.ngpio = 2,
+		.gpio_startup = pl2303hx_gpio_startup,
+		.gpio_release = pl2303hx_gpio_release,
+	}
 };
 
 static int pl2303_vendor_read(struct usb_serial *serial, u16 value,

@@ -213,6 +234,176 @@ static int pl2303_probe(struct usb_serial *serial,
 	return 0;
 }

drivers/usb/serial/pl2303.c static int pl2303_probe(struct usb_serial *serial, · line 246

Quoted 10 lines
drivers/usb/serial/pl2303.c +10 -0
+#ifdef CONFIG_USB_SERIAL_PL2303_GPIO
+struct pl2303hx_gpio_data {
+	/*
+	 * 0..3: unknown (zero)
+	 * 4: gp0 output enable (1: gp0 pin is output, 0: gp0 pin is input)
+	 * 5: gp1 output enable (1: gp1 pin is output, 0: gp1 pin is input)
+	 * 6: gp0 pin value
+	 * 7: gp1 pin value
+	 */
+	u8 index;

Perhaps index isn't the most descriptive name of this shadow register. Why not call it ctrl_reg, or similar.

Quoted 27 lines
drivers/usb/serial/pl2303.c +27 -0
+	struct mutex lock;
+	struct usb_serial *serial;
+	struct gpio_chip gpio_chip;
+};
+
+static inline struct pl2303hx_gpio_data *to_pl2303hx_gpio_data(struct gpio_chip *chip)
+{
+	return container_of(chip, struct pl2303hx_gpio_data, gpio_chip);
+}
+
+static u8 pl2303hx_gpio_dir_mask(unsigned offset)
+{
+	switch(offset)
+	{
+	case 0:
+		return 0x10;
+	case 1:
+		return 0x20;
+	default:
+		break;
+	};
+	return 0;
+}
+
+static u8 pl2303hx_gpio_value_mask(unsigned offset)
+{
+	switch(offset)

drivers/usb/serial/pl2303.c static int pl2303_probe(struct usb_serial *serial, · line 283

Quoted 10 lines
drivers/usb/serial/pl2303.c +10 -0
+	{
+	case 0:
+		return 0x40;
+	case 1:
+		return 0x80;
+	default:
+		break;
+	};
+	return 0;
+}

Use bitshifts rather than switch statements. Perhaps you don't even need the helpers if you add temporary mask variables.

drivers/usb/serial/pl2303.c static int pl2303_probe(struct usb_serial *serial, · line 292

Quoted 9 lines
drivers/usb/serial/pl2303.c +9 -0
+
+static int pl2303hx_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
+{
+	struct pl2303hx_gpio_data *gpio = to_pl2303hx_gpio_data(chip);
+	int ret;
+
+	mutex_lock(&gpio->lock);
+	gpio->index &= ~pl2303hx_gpio_dir_mask(offset);
+	ret = pl2303_vendor_write(gpio->serial, PL2303HX_REG_GPIO_CONTROL, gpio->index);

This line is too long.

Apparently you did not run checkpatch on v7 because there are a whole bunch of warning and errors now.

Quoted 33 lines
drivers/usb/serial/pl2303.c +33 -0
+	mutex_unlock(&gpio->lock);
+
+	return ret;
+}
+
+static int pl2303hx_gpio_direction_out(struct gpio_chip *chip,
+				unsigned offset, int value)
+{
+	struct pl2303hx_gpio_data *gpio = to_pl2303hx_gpio_data(chip);
+	int ret;
+
+	mutex_lock(&gpio->lock);
+	gpio->index |= pl2303hx_gpio_dir_mask(offset);
+	if (value)
+		gpio->index |= pl2303hx_gpio_value_mask(offset);
+	else
+		gpio->index &= ~pl2303hx_gpio_value_mask(offset);
+
+	ret = pl2303_vendor_write(gpio->serial, PL2303HX_REG_GPIO_CONTROL, gpio->index);
+	mutex_unlock(&gpio->lock);
+
+	return ret;
+}
+
+static void pl2303hx_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct pl2303hx_gpio_data *gpio = to_pl2303hx_gpio_data(chip);
+
+	mutex_lock(&gpio->lock);
+	if (value)
+		gpio->index |= pl2303hx_gpio_value_mask(offset);
+	else
+		gpio->index &= ~pl2303hx_gpio_value_mask(offset);

drivers/usb/serial/pl2303.c static int pl2303_probe(struct usb_serial *serial, · line 335

Quoted 10 lines
drivers/usb/serial/pl2303.c +10 -0
+
+	pl2303_vendor_write(gpio->serial, PL2303HX_REG_GPIO_CONTROL, gpio->index);
+	mutex_unlock(&gpio->lock);
+}
+
+static int pl2303hx_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+	struct pl2303hx_gpio_data *gpio = to_pl2303hx_gpio_data(chip);
+	unsigned char *buf;
+	int value = 0;

No need to initialise.

drivers/usb/serial/pl2303.c static int pl2303_probe(struct usb_serial *serial, · line 340

Quoted 5 lines
drivers/usb/serial/pl2303.c +5 -0
+
+	buf = kmalloc(1, GFP_KERNEL);
+	if (!buf) {
+		return -ENOMEM;
+	}

No braces.

drivers/usb/serial/pl2303.c static int pl2303_probe(struct usb_serial *serial, · line 343

Quoted 3 lines
drivers/usb/serial/pl2303.c +3 -0
+
+	mutex_lock(&gpio->lock);
+	if (pl2303_vendor_read(gpio->serial, PL2303HX_REG_GPIO_STATE, buf)) {

Please use a temporary variable for the returned status (e.g. int res) rather than calling vendor read from within the if construct.

Quoted 11 lines
drivers/usb/serial/pl2303.c +11 -0
+		mutex_unlock(&gpio->lock);
+		return -EIO;
+	}
+
+	value = buf[0] & pl2303hx_gpio_value_mask(offset);
+	mutex_unlock(&gpio->lock);
+	kfree(buf);
+
+	return value;
+}
+

drivers/usb/serial/pl2303.c static int pl2303_probe(struct usb_serial *serial, · line 364

Quoted 10 lines
drivers/usb/serial/pl2303.c +10 -0
+static int pl2303hx_gpio_startup(struct usb_serial *serial)
+{
+	struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
+	struct pl2303hx_gpio_data *gpio;
+	int ret;
+
+	gpio = kzalloc(sizeof(struct pl2303hx_gpio_data), GFP_KERNEL);
+	if (!gpio) {
+		dev_err(&serial->interface->dev,
+			"Failed to allocate pl2303_gpio_data\n");

No need to print an error message on failed allocations as this has already been taken care of.

Quoted 4 lines
drivers/usb/serial/pl2303.c +4 -0
+		return -ENOMEM;
+	}
+
+	gpio->index = 0x00;

drivers/usb/serial/pl2303.c static int pl2303_probe(struct usb_serial *serial, · line 378

Quoted 10 lines
drivers/usb/serial/pl2303.c +10 -0
+	gpio->serial = serial;
+	mutex_init(&gpio->lock);
+
+	gpio->gpio_chip.label = "pl2303";
+	gpio->gpio_chip.owner = THIS_MODULE;
+	gpio->gpio_chip.direction_input = pl2303hx_gpio_direction_in;
+	gpio->gpio_chip.get = pl2303hx_gpio_get;
+	gpio->gpio_chip.direction_output = pl2303hx_gpio_direction_out;
+	gpio->gpio_chip.set = pl2303hx_gpio_set;
+	gpio->gpio_chip.can_sleep  = true;

                                  ^

Runaway whitespace.

Quoted 14 lines
drivers/usb/serial/pl2303.c +14 -0
+	gpio->gpio_chip.ngpio = spriv->type->ngpio;
+	gpio->gpio_chip.base = -1;
+	gpio->gpio_chip.dev = &serial->interface->dev;
+
+	/* initialize GPIOs, input mode as default */
+	pl2303_vendor_write(gpio->serial, PL2303HX_REG_GPIO_CONTROL, gpio->index);
+
+	ret = gpiochip_add(&gpio->gpio_chip);
+	if (ret < 0) {
+		dev_err(&serial->interface->dev,
+			"Could not register gpiochip, %d\n", ret);
+		kfree(gpio);
+		return ret;
+	}

drivers/usb/serial/pl2303.c static int pl2303_probe(struct usb_serial *serial, · line 402

Quoted 10 lines
drivers/usb/serial/pl2303.c +10 -0
+	spriv->gpio = gpio;
+	return 0;
+}
+
+static void pl2303hx_gpio_release(struct usb_serial *serial)
+{
+	struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
+	struct pl2303hx_gpio_data *gpio = (struct pl2303hx_gpio_data *)spriv->gpio;
+
+	gpiochip_remove(&gpio->gpio_chip);

So this will corrupt the gpiolib structures if there are requested / exported gpios.

drivers/usb/serial/pl2303.c static int pl2303_startup(struct usb_serial *serial) · line 457

Quoted 13 lines
drivers/usb/serial/pl2303.c +6 -0
+	kfree(gpio);
+}
+#endif
+
 static int pl2303_startup(struct usb_serial *serial)
 {
 	struct pl2303_serial_private *spriv;

@@ -262,6 +453,15 @@ static int pl2303_startup(struct usb_serial *serial)
 
 	kfree(buf);
 
+	if (spriv->type->ngpio > 0) {
+		int ret;

Please declare all variables at the start of the function.

drivers/usb/serial/pl2303.c static int pl2303_startup(struct usb_serial *serial) · line 459

Quoted 2 lines
drivers/usb/serial/pl2303.c +2 -0
+
+		ret = spriv->type->gpio_startup(serial);

So call pl2303_gpio_startup() here and check ngpio in that function.

Quoted 9 lines
drivers/usb/serial/pl2303.c +5 -0
+		if (ret) {
+			kfree(spriv);
+			return ret;
+		}
+	}
 	return 0;
 }

drivers/usb/serial/pl2303.c static void pl2303_release(struct usb_serial *serial) · line 473

Quoted 5 lines
drivers/usb/serial/pl2303.c +2 -0
@@ -269,6 +469,8 @@ static void pl2303_release(struct usb_serial *serial)
 {
 	struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
 
+	if (spriv->type->ngpio > 0)
+		spriv->type->gpio_release(serial);

Call pl2303_gpio_release() and check ngpio there.

drivers/usb/serial/pl2303.c static void pl2303_release(struct usb_serial *serial) · line 475

Quoted 2 lines
drivers/usb/serial/pl2303.c +0 -0
 	kfree(spriv);
 }

Thanks,
Johan

#2 lore raw
Quoted 4 lines
Quoted 4 lines

Known issue:
If gpios are in use(export to userspace through sysfs interface, etc), then call pl2303_release(unplug usb-serial convertor, modprobe -r,

etc),

Quoted 1 line
Quoted 1 line

will cause trouble, so we need to make sure there is no gpio user

before

Quoted 4 lines
Quoted 1 line

call pl2303_release.

This is a real problem that we need to address. gpiolib isn't really able to handle devices that just disappear. In fact, it's API claims

that

Quoted 8 lines

we must not call gpiochip_remove with requested gpios and this is exactly what you might do in pl2303hx_gpio_release below.

As I mentioned earlier, this crashes the kernel when a new gpiochip is later added (the gpiochip data structures are likely corrupted and we get a NULL pointer deref in gpiochip_find_base).

Linus, any thoughts on this?

Hi,
there are several USB to I2C bus adapters and I2C IO-Expanders, how is this handled there ?

Maybe we you can take some idea from there.

best regards,
Hannes

maintainer
#3 lore raw
Quoted 19 lines · Hannes Petermaier
Quoted 4 lines
Quoted 4 lines

Known issue:
If gpios are in use(export to userspace through sysfs interface, etc), then call pl2303_release(unplug usb-serial convertor, modprobe -r,

etc),

Quoted 1 line
Quoted 1 line

will cause trouble, so we need to make sure there is no gpio user

before

Quoted 4 lines
Quoted 1 line

call pl2303_release.

This is a real problem that we need to address. gpiolib isn't really able to handle devices that just disappear. In fact, it's API claims

that

Quoted 7 lines

we must not call gpiochip_remove with requested gpios and this is exactly what you might do in pl2303hx_gpio_release below.

As I mentioned earlier, this crashes the kernel when a new gpiochip is later added (the gpiochip data structures are likely corrupted and we get a NULL pointer deref in gpiochip_find_base).

Quoted 5 lines
Quoted 1 line

Linus, any thoughts on this?

Hi,
there are several USB to I2C bus adapters and I2C IO-Expanders, how is this handled there ?

The short answer is: it isn't.

A few i2c-gpio-expander drivers have teardown callbacks that can be used from board files to release any gpios requested there, but this neither translates to device tree or is of any help when gpios have been exported to user space.

Thanks,
Johan

maintainer
#4 lore raw
Quoted 26 lines · Johan Hovold
Quoted 19 lines · Hannes Petermaier
Quoted 4 lines
Quoted 4 lines

Known issue:
If gpios are in use(export to userspace through sysfs interface, etc), then call pl2303_release(unplug usb-serial convertor, modprobe -r,

etc),

Quoted 1 line
Quoted 1 line

will cause trouble, so we need to make sure there is no gpio user

before

Quoted 4 lines
Quoted 1 line

call pl2303_release.

This is a real problem that we need to address. gpiolib isn't really able to handle devices that just disappear. In fact, it's API claims

that

Quoted 7 lines

we must not call gpiochip_remove with requested gpios and this is exactly what you might do in pl2303hx_gpio_release below.

As I mentioned earlier, this crashes the kernel when a new gpiochip is later added (the gpiochip data structures are likely corrupted and we get a NULL pointer deref in gpiochip_find_base).

Quoted 5 lines
Quoted 1 line

Linus, any thoughts on this?

Hi,
there are several USB to I2C bus adapters and I2C IO-Expanders, how is this handled there ?

Quoted 6 lines

The short answer is: it isn't.

A few i2c-gpio-expander drivers have teardown callbacks that can be used from board files to release any gpios requested there, but this neither translates to device tree or is of any help when gpios have been exported to user space.

For some reason I thought I saw some patches recently that was trying to resolve this problem. So it might get fixed for 3.18...

thanks,

greg k-h

author
#5 lore raw
In reply to Johan Hovold
Quoted 5 lines · Johan Hovold
Quoted 1 line · Wang YanQing

PL2303 USB Serial devices always has GPIOs,

Always? Are you sure? It's probably better to write "might have" as they are unlikely to be accessible even if the pins exist.

http://prolificusa.com/docs/2303/all/an_PL2303_GPIO_LED_Indicator_v1.0.pdf

"
All of the PL2303
chips aside from PL2303HXD have two dedicated GPIO pins (GP0 and GP1) while PL2303HXD have four GPIO pins (GP0, GP1, GP2, GP3)."

Quoted 10 lines

drivers/usb/serial/pl2303.c enum pl2303_type { · line 148

Quoted 8 lines

enum pl2303_type {

drivers/usb/serial/pl2303.c +1 -0
 	TYPE_01,	/* Type 0 and 1 (difference unknown) */

@@ -141,11 +145,15 @@ enum pl2303_type {
 struct pl2303_type_data {
 	speed_t max_baud_rate;
 	unsigned long quirks;
+	u16 ngpio;

u8 should be enough.

Yes, but struct gpio_chip use u16, so maybe u16 is better?.

Quoted 6 lines

drivers/usb/serial/pl2303.c enum pl2303_type { · line 150

Quoted 3 lines
drivers/usb/serial/pl2303.c +2 -0
+	int (*gpio_startup)(struct usb_serial *serial);
+	void (*gpio_release)(struct usb_serial *serial);

This isn't the right place for this abstraction. Most of the setup code would be common for any device type with GPIOs.

For any pl2303 variant instead of any device type ?

Quoted 4 lines

Just keep the generic gpio_startup and release from v6, and verify that ngpio > 0. Any further abstraction should only be added once we know how the other types handles GPIOs.

Quoted 8 lines

drivers/usb/serial/pl2303.c enum pl2303_type { · line 156

Quoted 6 lines
drivers/usb/serial/pl2303.c +1 -0
 };
 
 struct pl2303_serial_private {
 	const struct pl2303_type_data *type;
 	unsigned long quirks;
+	void *gpio;

No void pointers, please.

void pointer is abstraction for different pl2303 gpio_data struct, just like driver core or subsystem core does. I mean void pointer is right thing when we need abstraction, and we don't need type-specified startup|release, we don't need this void pointer.

Quoted 12 lines

drivers/usb/serial/pl2303.c static int pl2303_probe(struct usb_serial *serial, · line 292

Quoted 9 lines
drivers/usb/serial/pl2303.c +9 -0
+
+static int pl2303hx_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
+{
+	struct pl2303hx_gpio_data *gpio = to_pl2303hx_gpio_data(chip);
+	int ret;
+
+	mutex_lock(&gpio->lock);
+	gpio->index &= ~pl2303hx_gpio_dir_mask(offset);
+	ret = pl2303_vendor_write(gpio->serial, PL2303HX_REG_GPIO_CONTROL, gpio->index);

This line is too long.

Quoted 2 lines

Apparently you did not run checkpatch on v7 because there are a whole bunch of warning and errors now.

Yes, I haven't run checkpatch on v7, I will do it on v8 if there is one.

Quoted 9 lines

drivers/usb/serial/pl2303.c static int pl2303_probe(struct usb_serial *serial, · line 402

Quoted 6 lines
drivers/usb/serial/pl2303.c +6 -0
+static void pl2303hx_gpio_release(struct usb_serial *serial)
+{
+	struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
+	struct pl2303hx_gpio_data *gpio = (struct pl2303hx_gpio_data *)spriv->gpio;
+
+	gpiochip_remove(&gpio->gpio_chip);

So this will corrupt the gpiolib structures if there are requested / exported gpios.

We can do nothing here, indeed we must call gpiochip_remove to notify gpio layer our leave, and hope gpio layer could handle it.

Thanks.

maintainer
#6 lore raw
Quoted 11 lines · Wang YanQing
Quoted 5 lines · Johan Hovold
Quoted 1 line · Wang YanQing

PL2303 USB Serial devices always has GPIOs,

Always? Are you sure? It's probably better to write "might have" as they are unlikely to be accessible even if the pins exist.

http://prolificusa.com/docs/2303/all/an_PL2303_GPIO_LED_Indicator_v1.0.pdf

"All of the PL2303 chips aside from PL2303HXD have two dedicated GPIO pins (GP0 and GP1) while PL2303HXD have four GPIO pins (GP0, GP1, GP2, GP3)."

That's true for the HX family (when the document was written), but for example the PL2303SA has none.

Quoted 11 lines
Quoted 9 lines

drivers/usb/serial/pl2303.c enum pl2303_type { · line 148

Quoted 7 lines

enum pl2303_type {

drivers/usb/serial/pl2303.c +1 -0
 	TYPE_01,	/* Type 0 and 1 (difference unknown) */

@@ -141,11 +145,15 @@ enum pl2303_type {
 struct pl2303_type_data {
 	speed_t max_baud_rate;
 	unsigned long quirks;
+	u16 ngpio;

u8 should be enough.

Yes, but struct gpio_chip use u16, so maybe u16 is better?.

Not really, unless you foresee pl2303 device with more than 256 GPIOs. ;) And even then the type could have been changed later. It's just static type data so it really does not make much difference, but still.

Quoted 7 lines
Quoted 5 lines

drivers/usb/serial/pl2303.c enum pl2303_type { · line 150

Quoted 2 lines
drivers/usb/serial/pl2303.c +2 -0
+	int (*gpio_startup)(struct usb_serial *serial);
+	void (*gpio_release)(struct usb_serial *serial);

This isn't the right place for this abstraction. Most of the setup code would be common for any device type with GPIOs.

For any pl2303 variant instead of any device type ?

Yes, I meant pl2303 device type.

Quoted 13 lines
Quoted 4 lines

Just keep the generic gpio_startup and release from v6, and verify that ngpio > 0. Any further abstraction should only be added once we know how the other types handles GPIOs.

Quoted 8 lines

drivers/usb/serial/pl2303.c enum pl2303_type { · line 156

Quoted 6 lines
drivers/usb/serial/pl2303.c +1 -0
 };
 
 struct pl2303_serial_private {
 	const struct pl2303_type_data *type;
 	unsigned long quirks;
+	void *gpio;

No void pointers, please.

Quoted 4 lines

void pointer is abstraction for different pl2303 gpio_data struct, just like driver core or subsystem core does. I mean void pointer is right thing when we need abstraction, and we don't need type-specified startup|release, we don't need this void pointer.

You gonna have to store the gpio state somewhere. And it should be done in a struct of some specific sort just as in your previous versions. Use forward declarations, or compile guards, if that's what you need.

Quoted 27 lines
Quoted 12 lines

drivers/usb/serial/pl2303.c static int pl2303_probe(struct usb_serial *serial, · line 292

Quoted 9 lines
drivers/usb/serial/pl2303.c +9 -0
+
+static int pl2303hx_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
+{
+	struct pl2303hx_gpio_data *gpio = to_pl2303hx_gpio_data(chip);
+	int ret;
+
+	mutex_lock(&gpio->lock);
+	gpio->index &= ~pl2303hx_gpio_dir_mask(offset);
+	ret = pl2303_vendor_write(gpio->serial, PL2303HX_REG_GPIO_CONTROL, gpio->index);

This line is too long.

Quoted 2 lines

Apparently you did not run checkpatch on v7 because there are a whole bunch of warning and errors now.

Yes, I haven't run checkpatch on v7, I will do it on v8 if there is one.

Quoted 9 lines

drivers/usb/serial/pl2303.c static int pl2303_probe(struct usb_serial *serial, · line 402

Quoted 6 lines
drivers/usb/serial/pl2303.c +6 -0
+static void pl2303hx_gpio_release(struct usb_serial *serial)
+{
+	struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
+	struct pl2303hx_gpio_data *gpio = (struct pl2303hx_gpio_data *)spriv->gpio;
+
+	gpiochip_remove(&gpio->gpio_chip);

So this will corrupt the gpiolib structures if there are requested / exported gpios.

Quoted 2 lines

We can do nothing here, indeed we must call gpiochip_remove to notify gpio layer our leave, and hope gpio layer could handle it.

I'm not saying that you should do something different here. But I'm hesitant of merging this functionality before gpiolib is fixed.

Johan

author
#7 lore raw
In reply to Johan Hovold

Hi Johan Hovold.

Another two questions.

Quoted 6 lines · Johan Hovold

drivers/usb/serial/pl2303.c enum pl2303_type { · line 150

Quoted 3 lines
drivers/usb/serial/pl2303.c +2 -0
+	int (*gpio_startup)(struct usb_serial *serial);
+	void (*gpio_release)(struct usb_serial *serial);

This isn't the right place for this abstraction. Most of the setup code would be common for any device type with GPIOs.

I assume you mean any pl2303 variant, not any device type, because no device in drivers/gpio has common setup code except many of them use struct gpio_chip.

Quoted 4 lines

Just keep the generic gpio_startup and release from v6, and verify that ngpio > 0. Any further abstraction should only be added once we know how the other types handles GPIOs.

Instead of assume code works for all situation firstly, test and write code for only one type device, and make code become generic when we find it works for others' type.

I don't known which way is better.

Thanks very much for your patient review !!

maintainer
#8 lore raw
Quoted 12 lines · Wang YanQing

Hi Johan Hovold.

Another two questions.

Quoted 6 lines · Johan Hovold

drivers/usb/serial/pl2303.c enum pl2303_type { · line 150

Quoted 3 lines
drivers/usb/serial/pl2303.c +2 -0
+	int (*gpio_startup)(struct usb_serial *serial);
+	void (*gpio_release)(struct usb_serial *serial);

This isn't the right place for this abstraction. Most of the setup code would be common for any device type with GPIOs.

Quoted 3 lines

I assume you mean any pl2303 variant, not any device type, because no device in drivers/gpio has common setup code except many of them use struct gpio_chip.

Yes, pl2303 type/variant. Specifically, much of the setup code will be identical even if say the number of gpio differ (2 or 4) depending on type.

Quoted 9 lines
Quoted 3 lines

Just keep the generic gpio_startup and release from v6, and verify that ngpio > 0. Any further abstraction should only be added once we know how the other types handles GPIOs.

Instead of assume code works for all situation firstly, test and write code for only one type device, and make code become generic when we find it works for others' type.

I don't known which way is better.

Add support for your device type (HXA). We should verify that it also works for the non-end-of-life HXD, or we might need to find a way to detect those two different types of HX and limit it to HXA for now.

If and when someone reverse-engineers a device type with a different protocol for dealing with GPIOs, that's when you add further abstraction.

Quoted 1 line

Thanks very much for your patient review !!

You're welcome.

Johan

author
#9 lore raw
Quoted 13 lines · Johan Hovold
Quoted 12 lines · Wang YanQing

Hi Johan Hovold.

Another two questions.

Quoted 6 lines · Johan Hovold

drivers/usb/serial/pl2303.c enum pl2303_type { · line 150

Quoted 3 lines
drivers/usb/serial/pl2303.c +2 -0
+	int (*gpio_startup)(struct usb_serial *serial);
+	void (*gpio_release)(struct usb_serial *serial);

This isn't the right place for this abstraction. Most of the setup code would be common for any device type with GPIOs.

Quoted 7 lines
Quoted 3 lines

I assume you mean any pl2303 variant, not any device type, because no device in drivers/gpio has common setup code except many of them use struct gpio_chip.

Yes, pl2303 type/variant. Specifically, much of the setup code will be identical even if say the number of gpio differ (2 or 4) depending on type.

Yes, indeed unless I know how to make kernel could distinguish those two types, this patch willn't be fit for mergence.

And I don't have free time recent, I need more time to prepare this patch, or could you figure out how to distinguish them?

Thanks.

maintainer
#10 lore raw
Quoted 18 lines · Wang YanQing
Quoted 17 lines · Johan Hovold
Quoted 15 lines · Wang YanQing

Hi Johan Hovold.

Another two questions.

Quoted 6 lines · Johan Hovold

drivers/usb/serial/pl2303.c enum pl2303_type { · line 150

Quoted 3 lines
drivers/usb/serial/pl2303.c +2 -0
+	int (*gpio_startup)(struct usb_serial *serial);
+	void (*gpio_release)(struct usb_serial *serial);

This isn't the right place for this abstraction. Most of the setup code would be common for any device type with GPIOs.

I assume you mean any pl2303 variant, not any device type, because no device in drivers/gpio has common setup code except many of them use struct gpio_chip.

Quoted 6 lines
Quoted 3 lines

Yes, pl2303 type/variant. Specifically, much of the setup code will be identical even if say the number of gpio differ (2 or 4) depending on type.

Yes, indeed unless I know how to make kernel could distinguish those two types, this patch willn't be fit for mergence.

It might be possible to do based on the device descriptor (e.g. bcdDevice). Someone sniffed the official drivers and found some requests that appeared to be device specific as well.

Unfortunately official documentation from Prolific on this is lacking.

Johan