usb:serial:pl2303: add GPIOs interface on PL2303
Subsystems (2)
PATCH v2 1/1 usb:serial:pl2303: add GPIOs interface on PL2303
PL2303HX has two GPIOs, this patch add interface for it.
---
Changes v1-v2:
1:drop gpio-pl2303.c and relation stuff
2:hang gpio stuff off of pl2303.c
drivers/usb/serial/Kconfig | 7 ++ drivers/usb/serial/pl2303.c | 153 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 160 insertions(+)
drivers/usb/serial/Kconfig +7 -0
@@ -516,6 +516,13 @@ 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 + help + Say Y here if you want to use the GPIOs on PL2303 USB Serial single port + adapter from Prolific. + config USB_SERIAL_OTI6858 tristate "USB Ours Technology Inc. OTi-6858 USB To RS232 Bridge Controller" help
drivers/usb/serial/pl2303.c +153 -0
186 lines not on this page · raw message
Signature · 1 line
1.8.5.5.dirty
2014-07-20 8:38 GMT+02:00 Wang YanQing:
Quoted 1 line
PL2303HX has two GPIOs, this patch add interface for it.
checkpatch.pl shows 2 errors:
ERROR: space required before the open parenthesis '(' #218: FILE: drivers/usb/serial/pl2303.c:309:
+ if(pl2303_vendor_read(gpio->serial, 0x0081, buf) < 1)
ERROR: that open brace { should be on the previous line #248: FILE: drivers/usb/serial/pl2303.c:381:
+ if (type == TYPE_HX)
+ {
it also shows 3 warnings
Quoted 4 lines
drivers/usb/serial/pl2303.c +4
-0+#ifdef CONFIG_USB_SERIAL_PL2303_GPIO +struct pl2303_gpio { +/* +* 0..3: unknown (zero)
drivers/usb/serial/pl2303.c struct pl2303_type_data { · line 162
Quoted 10 lines
drivers/usb/serial/pl2303.c +10
-0+* 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; +struct usb_serial *serial; + struct gpio_chip gpio_chip; +}; +#endif
it would be nice to state what happens if you read pin 6 and 7 when they are set for output
drivers/usb/serial/pl2303.c static int pl2303_startup(struct usb_serial *serial) · line 378
Quoted 3 lines
drivers/usb/serial/pl2303.c +1
-0@@ -262,6 +377,37 @@ static int pl2303_startup(struct usb_serial *serial) + gpio->gpio_chip.ngpio =2 ;
you set ngpio but you don't use it
you would save some code if each function that use "index" has if (index >= gpio->gpio_chip.ngpio)
return -EINVAL;
and then read the constants from 2 arrays in file scope:gpio_index_mask = {0x10, 0x20};gpio_value_mask = {0x40, 0x80};
so you can change this:
Quoted 5 lines
drivers/usb/serial/pl2303.c +5
-0+if (offset ==0 ) { + gpio->index |=0x10 ; +if (value) + gpio->index |=0x40 ; +else
drivers/usb/serial/pl2303.c static int pl2303_probe(struct usb_serial *serial, · line 276
Quoted 10 lines
drivers/usb/serial/pl2303.c +10
-0+ gpio->index &= ~0x40 ; + }else if (offset ==1 ) { + gpio->index |=0x20 ; +if (value) + gpio->index |=0x80 ; +else + gpio->index &= ~0x80 ; + }else { +return -EINVAL ; + }
to this:
if (index >= gpio->gpio_chip.ngpio)
return -EINVAL;
gpio->index |= gpio_index_mask[offset];
if (value)
gpio->index |= gpio_value_mask[offset];
else
gpio->index &= ~gpio_value_mask[offset];do you find it less readable or less efficient?
Signature · 1 line
Daniele Forsi
drivers/usb/serial/pl2303.c line 34
Quoted 10 lines
drivers/usb/serial/pl2303.c +3
-0@@ -28,6 +28,9 @@#include <linux/usb.h> #include <linux/usb/serial.h> #include <asm/unaligned.h> +#ifdef CONFIG_USB_SERIAL_PL2303_GPIO +#include <linux/gpio.h> +#endif #include "pl2303.h"
Just include the file anyway it does no harm
Quoted 10 lines
drivers/usb/serial/pl2303.c +4
-0@@ -143,9 +146,27 @@ struct pl2303_type_data {unsigned long quirks; }; +#ifdef CONFIG_USB_SERIAL_PL2303_GPIO +struct pl2303_gpio { +/* +* 0..3: unknown (zero)
drivers/usb/serial/pl2303.c struct pl2303_type_data { · line 162
Quoted 10 lines
drivers/usb/serial/pl2303.c +10
-0+* 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; +struct usb_serial *serial; + struct gpio_chip gpio_chip; +}; +#endif
Declaring the struct anyway does no harm
drivers/usb/serial/pl2303.c static void pl2303_release(struct usb_serial *serial) · line 419
Quoted 4 lines
drivers/usb/serial/pl2303.c +2
-0struct pl2303_serial_private *spriv =usb_get_serial_data (serial); +#ifdef CONFIG_USB_SERIAL_PL2303_GPIO +if (spriv && spriv->gpio) {
Can spriv ever be NULL - how would that occur?
drivers/usb/serial/pl2303.c static void pl2303_release(struct usb_serial *serial) · line 425
Quoted 6 lines
drivers/usb/serial/pl2303.c +5
-0+if (gpiochip_remove (&spriv->gpio->gpio_chip)) +dev_err (&serial->interface->dev,"unable to remove gpio_chip?\n" ); + kfree(spriv->gpio); + } +#endif kfree (spriv);
Only other question I have - if I have multiple PL2303HX adapters how will I work out which GPIO lines belong to which /dev/ttyUSB* interface ?
Do we need a way to actually ask the serial port for its GPIO range ?
Greg KH
maintainerQuoted 9 lines · One Thousand Gnomes
drivers/usb/serial/pl2303.c static void pl2303_release(struct usb_serial *serial) · line 425
Quoted 6 lines
drivers/usb/serial/pl2303.c +5
-0+if (gpiochip_remove (&spriv->gpio->gpio_chip)) +dev_err (&serial->interface->dev,"unable to remove gpio_chip?\n" ); + kfree(spriv->gpio); + } +#endif kfree (spriv);Only other question I have - if I have multiple PL2303HX adapters how will I work out which GPIO lines belong to which
/dev/ttyUSB*interface ?
sysfs _should_ show you this, as it should point to the "parent" device, which will be associated with the ttyUSB interface. Well, both the tty device and the gpio device will have the same parent, is that good enough to determine this, or should the gpio device have the tty device as its parent?
thanks,
greg k-h
Quoted 11 lines · On Wed, 23 Jul 2014 09:21:29 -0700 Greg KH <[email protected]> wrote:
Quoted 9 lines · One Thousand Gnomes
drivers/usb/serial/pl2303.c static void pl2303_release(struct usb_serial *serial) · line 425
Quoted 6 lines
drivers/usb/serial/pl2303.c +5
-0+if (gpiochip_remove (&spriv->gpio->gpio_chip)) +dev_err (&serial->interface->dev,"unable to remove gpio_chip?\n" ); + kfree(spriv->gpio); + } +#endif kfree (spriv);Only other question I have - if I have multiple PL2303HX adapters how will I work out which GPIO lines belong to which
/dev/ttyUSB*interface ?
Quoted 5 lines
sysfs _should_ show you this, as it should point to the "parent" device, which will be associated with the ttyUSB interface. Well, both the tty device and the gpio device will have the same parent, is that good enough to determine this, or should the gpio device have the tty device as its parent?
Good point - that's probably sufficient as is. The GPIO and tty lines are sometimes used together and sometimes not.
Alan