[PATCH v2] usb:serial:pl2303: add GPIOs interface on PL2303

[PATCH v2] usb:serial:pl2303: add GPIOs interface on PL2303

[Wang YanQing]

PL2303HX has two GPIOs, this patch add interface for it.

Signed-off-by: Wang YanQing <udknight@gmail.com>
---
 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(+)

diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
index 3ce5c74..099ff05 100644
--- a/drivers/usb/serial/Kconfig
+++ b/drivers/usb/serial/Kconfig
@@ -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
diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index b3d5a35..1fbd338 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -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"
 
 
@@ -143,9 +146,27 @@ struct pl2303_type_data {
 	unsigned long quirks;
 };
 
+#ifdef CONFIG_USB_SERIAL_PL2303_GPIO
+struct pl2303_gpio {
+	/*
+	 * 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;
+	struct usb_serial *serial;
+	struct gpio_chip gpio_chip;
+};
+#endif
+
 struct pl2303_serial_private {
 	const struct pl2303_type_data *type;
 	unsigned long quirks;
+#ifdef CONFIG_USB_SERIAL_PL2303_GPIO
+	struct pl2303_gpio *gpio;
+#endif
 };
 
 struct pl2303_private {
@@ -213,6 +234,100 @@ static int pl2303_probe(struct usb_serial *serial,
 	return 0;
 }
 
+#ifdef CONFIG_USB_SERIAL_PL2303_GPIO
+static inline struct pl2303_gpio *to_pl2303_gpio(struct gpio_chip *chip)
+{
+	return container_of(chip, struct pl2303_gpio, gpio_chip);
+}
+
+static int pl2303_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
+{
+	struct pl2303_gpio *gpio = to_pl2303_gpio(chip);
+
+	if (offset == 0)
+		gpio->index &= ~0x10;
+	else if (offset == 1)
+		gpio->index &= ~0x20;
+	else
+		return -EINVAL;
+
+	pl2303_vendor_write(gpio->serial, 1, gpio->index);
+	return 0;
+}
+
+static int pl2303_gpio_direction_out(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct pl2303_gpio *gpio = to_pl2303_gpio(chip);
+
+	if (offset == 0) {
+		gpio->index |= 0x10;
+		if (value)
+			gpio->index |= 0x40;
+		else
+			gpio->index &= ~0x40;
+	} else if (offset == 1) {
+		gpio->index |= 0x20;
+		if (value)
+			gpio->index |= 0x80;
+		else
+			gpio->index &= ~0x80;
+	} else {
+		return -EINVAL;
+	}
+
+	pl2303_vendor_write(gpio->serial, 1, gpio->index);
+	return 0;
+}
+
+static void pl2303_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct pl2303_gpio *gpio = to_pl2303_gpio(chip);
+
+	if (offset == 0) {
+		if (value)
+			gpio->index |= 0x40;
+		else
+			gpio->index &= ~0x40;
+	} else if (offset == 1) {
+		if (value)
+			gpio->index |= 0x80;
+		else
+			gpio->index &= ~0x80;
+	} else {
+		return;
+	}
+
+	pl2303_vendor_write(gpio->serial, 1, gpio->index);
+}
+
+static int pl2303_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+	struct pl2303_gpio *gpio = to_pl2303_gpio(chip);
+	unsigned char buf[1];
+	int value = 0;
+
+	if(pl2303_vendor_read(gpio->serial, 0x0081, buf) < 1)
+		return -EIO;
+	if (offset == 0)
+		value = buf[0] & 0x40;
+	else if (offset == 1)
+		value = buf[0] & 0x80;
+	else
+		value = -EINVAL;
+	return value;
+}
+
+static struct gpio_chip template_chip = {
+	.label                  = "pl2303-gpio",
+	.owner                  = THIS_MODULE,
+	.direction_input        = pl2303_gpio_direction_in,
+	.get                    = pl2303_gpio_get,
+	.direction_output       = pl2303_gpio_direction_out,
+	.set                    = pl2303_gpio_set,
+	.can_sleep              = 1,
+};
+#endif
+
 static int pl2303_startup(struct usb_serial *serial)
 {
 	struct pl2303_serial_private *spriv;
@@ -262,6 +377,37 @@ static int pl2303_startup(struct usb_serial *serial)
 
 	kfree(buf);
 
+#ifdef CONFIG_USB_SERIAL_PL2303_GPIO
+	if (type == TYPE_HX)
+	{
+		struct pl2303_gpio *gpio;
+
+		gpio = kzalloc(sizeof(struct pl2303_gpio), GFP_KERNEL);
+		if (gpio == NULL) {
+			dev_err(&serial->interface->dev, "Failed to allocate pl2303_gpio\n");
+		} else {
+			int ret;
+
+			gpio->index = 0x00;
+			gpio->serial = serial;
+			gpio->gpio_chip = template_chip;
+			gpio->gpio_chip.ngpio = 2;
+			gpio->gpio_chip.base = -1;
+			gpio->gpio_chip.dev = &serial->interface->dev;
+			/* initialize GPIOs, input mode as default */
+			pl2303_vendor_write(gpio->serial, 1, gpio->index);
+
+			spriv->gpio = gpio;
+			ret = gpiochip_add(&spriv->gpio->gpio_chip);
+			if (ret < 0) {
+				dev_err(&serial->interface->dev,
+					"Could not register gpiochip, %d\n", ret);
+				kfree(spriv->gpio);
+				spriv->gpio = NULL;
+			}
+		}
+	}
+#endif
 	return 0;
 }
 
@@ -269,6 +415,13 @@ static void pl2303_release(struct usb_serial *serial)
 {
 	struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
 
+#ifdef CONFIG_USB_SERIAL_PL2303_GPIO
+	if (spriv && spriv->gpio) {
+		if (gpiochip_remove(&spriv->gpio->gpio_chip))
+			dev_err(&serial->interface->dev, "unable to remove gpio_chip?\n");
+		kfree(spriv->gpio);
+	}
+#endif
 	kfree(spriv);
 }
 
-- 
1.8.5.5.dirty

Re: [PATCH v2] usb:serial:pl2303: add GPIOs interface on PL2303

[Daniele Forsi]

2014-07-20 8:38 GMT+02:00 Wang YanQing:

> 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

> +#ifdef CONFIG_USB_SERIAL_PL2303_GPIO
> +struct pl2303_gpio {
> +       /*
> +        * 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;
> +       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

> @@ -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:
> +       if (offset == 0) {
> +               gpio->index |= 0x10;
> +               if (value)
> +                       gpio->index |= 0x40;
> +               else
> +                       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?
-- 
Daniele Forsi

Re: [PATCH v2] usb:serial:pl2303: add GPIOs interface on PL2303

[One Thousand Gnomes]

> --- a/drivers/usb/serial/pl2303.c
> +++ b/drivers/usb/serial/pl2303.c
> @@ -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

>  
>  
> @@ -143,9 +146,27 @@ struct pl2303_type_data {
>  	unsigned long quirks;
>  };
>  
> +#ifdef CONFIG_USB_SERIAL_PL2303_GPIO
> +struct pl2303_gpio {
> +	/*
> +	 * 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;
> +	struct usb_serial *serial;
> +	struct gpio_chip gpio_chip;
> +};
> +#endif

Declaring the struct anyway does no harm


>  	struct 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?


> +		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 ?

Re: [PATCH v2] usb:serial:pl2303: add GPIOs interface on PL2303

[Greg KH]

On Wed, Jul 23, 2014 at 05:03:14PM +0100, One Thousand Gnomes wrote:
> > +		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

Re: [PATCH v2] usb:serial:pl2303: add GPIOs interface on PL2303

[One Thousand Gnomes]

On Wed, 23 Jul 2014 09:21:29 -0700
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Wed, Jul 23, 2014 at 05:03:14PM +0100, One Thousand Gnomes wrote:
> > > +		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?

Good point - that's probably sufficient as is. The GPIO and tty lines are
sometimes used together and sometimes not.

Alan