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

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

[Wang YanQing]

On Thu, Sep 04, 2014 at 06:44:31PM +0200, Benjamin Henrion wrote:
> On Thu, Sep 4, 2014 at 6:14 PM, Benjamin Henrion <zoobab@gmail.com> wrote:
> > I have subscribed to the lkml.
> >
> > Can you make me a favour, send me your email as you posted on the LKML
> > in mbox format attached to an email?
> >
> > I am trying to reply to it, but I cannot find an mbox archive, outside
> > of gmane which rewrites all the email addresses.
> >
> > Otherwise, I made a page here:
> >
> > http://www.zoobab.com/pl2303hxd-gpio
> >
> > I will also try with other adapters to see if the 2 GPIOs show up.
> >
> > Any idea how to test the GPIO sysfs speed?
> 
> Also tested with an HXA laying around, I could export the 2 GPIOS in
> /sys/class/gpio, but I could not change the status of the pins via
> echo.
> 
> Any idea why?

It is strange, I have tested again with my two HXA adapters, they work well.
How do you test? check cat value or check voltage change? I check cat value,
and they work good.


> 
> -- 
> Benjamin Henrion <bhenrion at ffii.org>
> FFII Brussels - +32-484-566109 - +32-2-4148403
> "In July 2005, after several failed attempts to legalise software
> patents in Europe, the patent establishment changed its strategy.
> Instead of explicitly seeking to sanction the patentability of
> software, they are now seeking to create a central European patent
> court, which would establish and enforce patentability rules in their
> favor, without any possibility of correction by competing courts or
> democratically elected legislators."

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

[Johan Hovold]

On Fri, Sep 05, 2014 at 09:30:11AM +0800, Wang YanQing wrote:
> On Thu, Sep 04, 2014 at 06:44:31PM +0200, Benjamin Henrion wrote:
> > On Thu, Sep 4, 2014 at 6:14 PM, Benjamin Henrion <zoobab@gmail.com> wrote:
> > > I have subscribed to the lkml.
> > >
> > > Can you make me a favour, send me your email as you posted on the LKML
> > > in mbox format attached to an email?
> > >
> > > I am trying to reply to it, but I cannot find an mbox archive, outside
> > > of gmane which rewrites all the email addresses.
> > >
> > > Otherwise, I made a page here:
> > >
> > > http://www.zoobab.com/pl2303hxd-gpio
> > >
> > > I will also try with other adapters to see if the 2 GPIOs show up.
> > >
> > > Any idea how to test the GPIO sysfs speed?
> > 
> > Also tested with an HXA laying around, I could export the 2 GPIOS in
> > /sys/class/gpio, but I could not change the status of the pins via
> > echo.
> > 
> > Any idea why?
> 
> It is strange, I have tested again with my two HXA adapters, they work well.
> How do you test? check cat value or check voltage change? I check cat value,
> and they work good.

So you haven't verified that the voltage actually changes?

Johan

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

[Wang YanQing]

On Tue, Sep 09, 2014 at 12:08:56PM +0200, Johan Hovold wrote:
> On Fri, Sep 05, 2014 at 09:30:11AM +0800, Wang YanQing wrote:
> > On Thu, Sep 04, 2014 at 06:44:31PM +0200, Benjamin Henrion wrote:
> > > On Thu, Sep 4, 2014 at 6:14 PM, Benjamin Henrion <zoobab@gmail.com> wrote:
> > > > I have subscribed to the lkml.
> > > >
> > > > Can you make me a favour, send me your email as you posted on the LKML
> > > > in mbox format attached to an email?
> > > >
> > > > I am trying to reply to it, but I cannot find an mbox archive, outside
> > > > of gmane which rewrites all the email addresses.
> > > >
> > > > Otherwise, I made a page here:
> > > >
> > > > http://www.zoobab.com/pl2303hxd-gpio
> > > >
> > > > I will also try with other adapters to see if the 2 GPIOs show up.
> > > >
> > > > Any idea how to test the GPIO sysfs speed?
> > > 
> > > Also tested with an HXA laying around, I could export the 2 GPIOS in
> > > /sys/class/gpio, but I could not change the status of the pins via
> > > echo.
> > > 
> > > Any idea why?
> > 
> > It is strange, I have tested again with my two HXA adapters, they work well.
> > How do you test? check cat value or check voltage change? I check cat value,
> > and they work good.
> 
> So you haven't verified that the voltage actually changes?
> 
> Johan

I have verified the voltage actually changes in previous patch version,
but I haven't verified that with v8.

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

[Johan Hovold]

On Tue, Sep 09, 2014 at 06:21:31PM +0800, Wang YanQing wrote:
> On Tue, Sep 09, 2014 at 12:08:56PM +0200, Johan Hovold wrote:
> > On Fri, Sep 05, 2014 at 09:30:11AM +0800, Wang YanQing wrote:
> > > On Thu, Sep 04, 2014 at 06:44:31PM +0200, Benjamin Henrion wrote:
> > > > On Thu, Sep 4, 2014 at 6:14 PM, Benjamin Henrion <zoobab@gmail.com> wrote:
> > > > > I have subscribed to the lkml.
> > > > >
> > > > > Can you make me a favour, send me your email as you posted on the LKML
> > > > > in mbox format attached to an email?
> > > > >
> > > > > I am trying to reply to it, but I cannot find an mbox archive, outside
> > > > > of gmane which rewrites all the email addresses.
> > > > >
> > > > > Otherwise, I made a page here:
> > > > >
> > > > > http://www.zoobab.com/pl2303hxd-gpio
> > > > >
> > > > > I will also try with other adapters to see if the 2 GPIOs show up.
> > > > >
> > > > > Any idea how to test the GPIO sysfs speed?
> > > > 
> > > > Also tested with an HXA laying around, I could export the 2 GPIOS in
> > > > /sys/class/gpio, but I could not change the status of the pins via
> > > > echo.
> > > > 
> > > > Any idea why?
> > > 
> > > It is strange, I have tested again with my two HXA adapters, they work well.
> > > How do you test? check cat value or check voltage change? I check cat value,
> > > and they work good.
> > 
> > So you haven't verified that the voltage actually changes?
> > 
> > Johan
> 
> I have verified the voltage actually changes in previous patch version,
> but I haven't verified that with v8.

Ok, good. I'll get back to you with some comments on v8 shortly.

Thanks,
Johan

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

[Johan Hovold]

On Tue, Sep 09, 2014 at 12:43:56PM +0200, Johan Hovold wrote:
> On Tue, Sep 09, 2014 at 06:21:31PM +0800, Wang YanQing wrote:
> > On Tue, Sep 09, 2014 at 12:08:56PM +0200, Johan Hovold wrote:
> > > On Fri, Sep 05, 2014 at 09:30:11AM +0800, Wang YanQing wrote:
> > > > On Thu, Sep 04, 2014 at 06:44:31PM +0200, Benjamin Henrion wrote:
> > > > > On Thu, Sep 4, 2014 at 6:14 PM, Benjamin Henrion <zoobab@gmail.com> wrote:
> > > > > > I have subscribed to the lkml.
> > > > > >
> > > > > > Can you make me a favour, send me your email as you posted on the LKML
> > > > > > in mbox format attached to an email?
> > > > > >
> > > > > > I am trying to reply to it, but I cannot find an mbox archive, outside
> > > > > > of gmane which rewrites all the email addresses.
> > > > > >
> > > > > > Otherwise, I made a page here:
> > > > > >
> > > > > > http://www.zoobab.com/pl2303hxd-gpio
> > > > > >
> > > > > > I will also try with other adapters to see if the 2 GPIOs show up.
> > > > > >
> > > > > > Any idea how to test the GPIO sysfs speed?
> > > > > 
> > > > > Also tested with an HXA laying around, I could export the 2 GPIOS in
> > > > > /sys/class/gpio, but I could not change the status of the pins via
> > > > > echo.
> > > > > 
> > > > > Any idea why?
> > > > 
> > > > It is strange, I have tested again with my two HXA adapters, they work well.
> > > > How do you test? check cat value or check voltage change? I check cat value,
> > > > and they work good.
> > > 
> > > So you haven't verified that the voltage actually changes?
> > > 
> > > Johan
> > 
> > I have verified the voltage actually changes in previous patch version,
> > but I haven't verified that with v8.
> 
> Ok, good. I'll get back to you with some comments on v8 shortly.

Wait a minute. You restructured the driver, added support for two new
gpios as well as another device type, so you need to verify the voltage
on the pins (on both device types) as well.

Thanks,
Johan

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

[Wang YanQing]

On Tue, Sep 09, 2014 at 04:02:04PM +0200, Johan Hovold wrote:
> On Tue, Sep 09, 2014 at 12:43:56PM +0200, Johan Hovold wrote:
> > On Tue, Sep 09, 2014 at 06:21:31PM +0800, Wang YanQing wrote:
> > > On Tue, Sep 09, 2014 at 12:08:56PM +0200, Johan Hovold wrote:
> > > > On Fri, Sep 05, 2014 at 09:30:11AM +0800, Wang YanQing wrote:
> > > > > On Thu, Sep 04, 2014 at 06:44:31PM +0200, Benjamin Henrion wrote:
> > > > > > On Thu, Sep 4, 2014 at 6:14 PM, Benjamin Henrion <zoobab@gmail.com> wrote:
> > > > > > > I have subscribed to the lkml.
> > > > > > >
> > > > > > > Can you make me a favour, send me your email as you posted on the LKML
> > > > > > > in mbox format attached to an email?
> > > > > > >
> > > > > > > I am trying to reply to it, but I cannot find an mbox archive, outside
> > > > > > > of gmane which rewrites all the email addresses.
> > > > > > >
> > > > > > > Otherwise, I made a page here:
> > > > > > >
> > > > > > > http://www.zoobab.com/pl2303hxd-gpio
> > > > > > >
> > > > > > > I will also try with other adapters to see if the 2 GPIOs show up.
> > > > > > >
> > > > > > > Any idea how to test the GPIO sysfs speed?
> > > > > > 
> > > > > > Also tested with an HXA laying around, I could export the 2 GPIOS in
> > > > > > /sys/class/gpio, but I could not change the status of the pins via
> > > > > > echo.
> > > > > > 
> > > > > > Any idea why?
> > > > > 
> > > > > It is strange, I have tested again with my two HXA adapters, they work well.
> > > > > How do you test? check cat value or check voltage change? I check cat value,
> > > > > and they work good.
> > > > 
> > > > So you haven't verified that the voltage actually changes?
> > > > 
> > > > Johan
> > > 
> > > I have verified the voltage actually changes in previous patch version,
> > > but I haven't verified that with v8.
> > 
> > Ok, good. I'll get back to you with some comments on v8 shortly.
> 
> Wait a minute. You restructured the driver, added support for two new
> gpios as well as another device type, so you need to verify the voltage
> on the pins (on both device types) as well.

Benjamin Henrion had verified voltage on pins of HXD, see

http://www.zoobab.com/pl2303hxd-gpio

Sorry I don't have pinout version of PL2303 RA, so I can't verified voltage on it,
I only can test it by echo & cat.

Thanks.

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

[Wang YanQing]

On Tue, Sep 09, 2014 at 04:02:04PM +0200, Johan Hovold wrote:
> On Tue, Sep 09, 2014 at 12:43:56PM +0200, Johan Hovold wrote:
> > On Tue, Sep 09, 2014 at 06:21:31PM +0800, Wang YanQing wrote:
> > > On Tue, Sep 09, 2014 at 12:08:56PM +0200, Johan Hovold wrote:
> > > > On Fri, Sep 05, 2014 at 09:30:11AM +0800, Wang YanQing wrote:
> > > > > On Thu, Sep 04, 2014 at 06:44:31PM +0200, Benjamin Henrion wrote:
> > > > > > On Thu, Sep 4, 2014 at 6:14 PM, Benjamin Henrion <zoobab@gmail.com> wrote:
> > > > > > > I have subscribed to the lkml.
> > > > > > >
> > > > > > > Can you make me a favour, send me your email as you posted on the LKML
> > > > > > > in mbox format attached to an email?
> > > > > > >
> > > > > > > I am trying to reply to it, but I cannot find an mbox archive, outside
> > > > > > > of gmane which rewrites all the email addresses.
> > > > > > >
> > > > > > > Otherwise, I made a page here:
> > > > > > >
> > > > > > > http://www.zoobab.com/pl2303hxd-gpio
> > > > > > >
> > > > > > > I will also try with other adapters to see if the 2 GPIOs show up.
> > > > > > >
> > > > > > > Any idea how to test the GPIO sysfs speed?
> > > > > > 
> > > > > > Also tested with an HXA laying around, I could export the 2 GPIOS in
> > > > > > /sys/class/gpio, but I could not change the status of the pins via
> > > > > > echo.
> > > > > > 
> > > > > > Any idea why?
> > > > > 
> > > > > It is strange, I have tested again with my two HXA adapters, they work well.
> > > > > How do you test? check cat value or check voltage change? I check cat value,
> > > > > and they work good.
> > > > 
> > > > So you haven't verified that the voltage actually changes?
> > > > 
> > > > Johan
> > > 
> > > I have verified the voltage actually changes in previous patch version,
> > > but I haven't verified that with v8.
> > 
> > Ok, good. I'll get back to you with some comments on v8 shortly.
> 
> Wait a minute. You restructured the driver, added support for two new
> gpios as well as another device type, so you need to verify the voltage
> on the pins (on both device types) as well.

Benjamin Henrion had verified voltage on pins of HXD, see

http://www.zoobab.com/pl2303hxd-gpio

Sorry I don't have pinout version of PL2303 RA, so I can't verified voltage on it,
I only can test it by echo & cat.

Thanks.

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

[Johan Hovold]

On Sun, Aug 31, 2014 at 11:24:56PM +0800, Wang YanQing wrote:
> PL2303 USB Serial devices may has GPIOs, this patch add
> basic PL2303 gpio support.
> 
> 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 <udknight@gmail.com>
> ---
>  Note: I sniffed office HXD gpio test program to get 
>        gpios control protocol with PL2303 RA, so I only
>        test it with PL2303 RA and HXA, I don't have HXD,
>        but because it is *office* HXD gpio test program,
>        so if it doesn't work with HXD, I will feel surprise.

Nice.

That should be ok as long as you can test it with RA and the official
driver treats them the same (and perhaps someone else could do the HXD
testing?).

> 
>  Changes
>  v7-v8:
>  1: add support for four dedicated gpios on HXD and RA
>  2: fix problem reported by Johan Hovold in v7
>  3: fix checkpatch.pl's warning
> 
>  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 gnomes@lxorguk.ukuu.org.uk 
>  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  |  11 ++
>  drivers/usb/serial/pl2303.c | 250 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 261 insertions(+)
> 
> diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
> index 3ce5c74..b4e0cf9 100644
> --- a/drivers/usb/serial/Kconfig
> +++ b/drivers/usb/serial/Kconfig
> @@ -516,6 +516,17 @@ 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, 4 dedicated GPIOs on
> +	  PL2303HXD and PL2303RA currently.
> +	  If unsure, say N.
> +
>  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 e9bad92..a3f8411 100644
> --- a/drivers/usb/serial/pl2303.c
> +++ b/drivers/usb/serial/pl2303.c
> @@ -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"
>  
>  
> @@ -132,6 +134,22 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  #define UART_OVERRUN_ERROR		0x40
>  #define UART_CTS			0x80
>  
> +#define HXD_GPIO_01_CTRL		0x0001
> +#define HXD_GPIO_01_VALUE		0x0081
> +#define HXD_GPIO_23_DIR_CTRL		0x0c0c
> +#define HXD_GPIO_23_VALUE_CTRL		0x0d0d
> +#define HXD_GPIO_23_VALUE		0x8d8d

These three new wValues look odd. Perhaps the most-significant byte is
ignored? Doesn't matter. :)

I would also suggest naming these registers differently, for example:

	PL2303_GPIO_01_CTRL
	PL2303_GPIO_01_STATUS
	PL2303_GPIO_23_OUTPUT_ENABLE
	PL2303_GPIO_23_OUTPUT
	PL2303_GPIO_23_STATUS

in order to reflect the different semantics for the two sets of
registers (and to use more standardised names).

> +
> +#define HXD_GPIO0_DIR_MASK		0x10
> +#define HXD_GPIO1_DIR_MASK		0x20
> +#define HXD_GPIO2_DIR_MASK		0x03
> +#define HXD_GPIO3_DIR_MASK		0x0c

Do you really need two-bit wide masks?

> +
> +#define HXD_GPIO0_VALUE_MASK		0x40
> +#define HXD_GPIO1_VALUE_MASK		0x80
> +#define HXD_GPIO2_VALUE_MASK		0x01
> +#define HXD_GPIO3_VALUE_MASK		0x02
> +
>  
>  enum pl2303_type {
>  	TYPE_01,	/* Type 0 and 1 (difference unknown) */
> @@ -144,9 +162,12 @@ struct pl2303_type_data {
>  	unsigned long quirks;
>  };
>  
> +struct pl2303_gpio;
>  struct pl2303_serial_private {
>  	const struct pl2303_type_data *type;
>  	unsigned long quirks;
> +	u8 ngpio;

Keep this in the type data as in the last version (more below).

> +	struct pl2303_gpio *gpio;
>  };
>  
>  struct pl2303_private {
> @@ -157,6 +178,14 @@ struct pl2303_private {
>  	u8 line_settings[7];
>  };
>  
> +#ifdef CONFIG_USB_SERIAL_PL2303_GPIO
> +static int pl2303_gpio_startup(struct usb_serial *serial);
> +static void pl2303_gpio_release(struct usb_serial *serial);
> +#else
> +static inline int pl2303_gpio_startup(struct usb_serial *serial) { return 0; }
> +static inline void pl2303_gpio_release(struct usb_serial *serial) {}
> +#endif
> +
>  static const struct pl2303_type_data pl2303_type_data[TYPE_COUNT] = {
>  	[TYPE_01] = {
>  		.max_baud_rate =	1228800,
> @@ -214,11 +243,213 @@ static int pl2303_probe(struct usb_serial *serial,
>  	return 0;
>  }
>  
> +#ifdef CONFIG_USB_SERIAL_PL2303_GPIO
> +struct pl2303_gpio_desc {
> +	u8 dir_offset;
> +	u8 dir_mask;
> +	u16 dir_ctrl;
> +	u8 value_offset;
> +	u8 value_mask;
> +	u16 value_ctrl;
> +	u16 read_ctrl;
> +};
> +
> +struct pl2303_gpio {
> +	struct pl2303_gpio_desc *descs;
> +	u8 data[3];

I see what you're trying to achieve, but I think this abstraction is
much to opaque. Especially with this data[3].

Keep the three shadow registers ctrl_01, oe_23 and output_23 in the gpio
struct explicitly instead and skip the pl2303_gpio_descs.

All devices we currently know of use the same two sets of registers,
but some only have the first (singleton) set (i.e. HX with two gpios).
Handle this in the gpio callbacks instead for now (e.g. re-introduce the
mask helper functions, but implement them using bit shifts as already
suggested).

> +	struct mutex lock;
> +	struct usb_serial *serial;
> +	struct gpio_chip gpio_chip;
> +};
> +
> +static inline struct pl2303_gpio *to_pl2303_gpio(struct gpio_chip *chip)
> +{
> +	return container_of(chip, struct pl2303_gpio, gpio_chip);
> +}
> +
> +static void pl2303_gpio_add(struct usb_serial *serial, u8 num, u16 read_ctrl,
> +			u8 dir_offset, u8 dir_mask, u16 dir_ctrl,
> +			u8 value_offset, u8 value_mask, u16 value_ctrl)
> +{
> +	struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
> +	struct pl2303_gpio *gpio = spriv->gpio;
> +
> +	BUG_ON(num >= spriv->ngpio);

Never use BUG_ON in driver code. Handle the error, or make sure it does
not happen.

> +	gpio->descs[num].dir_offset = dir_offset;
> +	gpio->descs[num].dir_mask = dir_mask;
> +	gpio->descs[num].dir_ctrl = dir_ctrl;
> +
> +	gpio->descs[num].value_offset = value_offset;
> +	gpio->descs[num].value_mask = value_mask;
> +	gpio->descs[num].value_ctrl = value_ctrl;
> +
> +	gpio->descs[num].read_ctrl = read_ctrl;
> +}
> +
> +static int pl2303_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
> +{
> +	struct pl2303_gpio *gpio = to_pl2303_gpio(chip);
> +	struct pl2303_gpio_desc *desc;
> +	int ret;
> +
> +	mutex_lock(&gpio->lock);
> +	desc = gpio->descs+offset;
> +	gpio->data[desc->dir_offset] &= ~desc->dir_mask;
> +	ret = pl2303_vendor_write(gpio->serial, desc->dir_ctrl,
> +				gpio->data[desc->dir_offset]);
> +	mutex_unlock(&gpio->lock);
> +
> +	return ret;
> +}
> +
> +static int pl2303_gpio_direction_out(struct gpio_chip *chip,
> +				unsigned offset, int value)
> +{
> +	struct pl2303_gpio *gpio = to_pl2303_gpio(chip);
> +	struct pl2303_gpio_desc *desc;
> +	int ret;
> +
> +	mutex_lock(&gpio->lock);
> +	desc = gpio->descs+offset;
> +	gpio->data[desc->dir_offset] |= desc->dir_mask;
> +	ret = pl2303_vendor_write(gpio->serial, desc->dir_ctrl,
> +				gpio->data[desc->dir_offset]);
> +	if (ret)
> +		goto error;
> +
> +	if (value)
> +		gpio->data[desc->value_offset] |= desc->value_mask;
> +	else
> +		gpio->data[desc->value_offset] &= ~desc->value_mask;
> +
> +	ret = pl2303_vendor_write(gpio->serial, desc->value_ctrl,
> +				gpio->data[desc->value_offset]);
> +error:
> +	mutex_unlock(&gpio->lock);
> +
> +	return ret;
> +}
> +
> +static void pl2303_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> +	struct pl2303_gpio *gpio = to_pl2303_gpio(chip);
> +	struct pl2303_gpio_desc *desc;
> +
> +	mutex_lock(&gpio->lock);
> +	desc = gpio->descs+offset;
> +	if (value)
> +		gpio->data[desc->value_offset] |= desc->value_mask;
> +	else
> +		gpio->data[desc->value_offset] &= ~desc->value_mask;
> +
> +	pl2303_vendor_write(gpio->serial, desc->value_ctrl,
> +			gpio->data[desc->value_offset]);
> +	mutex_unlock(&gpio->lock);
> +}
> +
> +static int pl2303_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> +	struct pl2303_gpio *gpio = to_pl2303_gpio(chip);
> +	struct pl2303_gpio_desc *desc;
> +	unsigned char *buf;
> +	int value;
> +
> +	buf = kzalloc(1, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	mutex_lock(&gpio->lock);
> +	desc = gpio->descs+offset;
> +
> +	if (pl2303_vendor_read(gpio->serial, desc->read_ctrl, buf)) {
> +		mutex_unlock(&gpio->lock);
> +		return -EIO;
> +	}
> +
> +	value = buf[0] & desc->value_mask;
> +	mutex_unlock(&gpio->lock);
> +	kfree(buf);
> +
> +	return value;
> +}
> +
> +static int pl2303_gpio_startup(struct usb_serial *serial)
> +{
> +	struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
> +	struct pl2303_gpio *gpio;
> +	int ret;
> +
> +	gpio = kzalloc(sizeof(struct pl2303_gpio), GFP_KERNEL);
> +	if (!gpio)
> +		return -ENOMEM;
> +
> +	gpio->descs = kcalloc(spriv->ngpio, sizeof(struct pl2303_gpio_desc),
> +			GFP_KERNEL);
> +	if (!gpio->descs) {
> +		kfree(gpio);
> +		return -ENOMEM;
> +	}
> +	spriv->gpio = gpio;
> +
> +	pl2303_gpio_add(serial, 0, HXD_GPIO_01_VALUE,
> +			0, HXD_GPIO0_DIR_MASK, HXD_GPIO_01_CTRL,
> +			0, HXD_GPIO0_VALUE_MASK, HXD_GPIO_01_CTRL);
> +	pl2303_gpio_add(serial, 1, HXD_GPIO_01_VALUE,
> +			0, HXD_GPIO1_DIR_MASK, HXD_GPIO_01_CTRL,
> +			0, HXD_GPIO1_VALUE_MASK, HXD_GPIO_01_CTRL);
> +
> +	if (spriv->ngpio == 4) {
> +		pl2303_gpio_add(serial, 2, HXD_GPIO_23_VALUE,
> +				1, HXD_GPIO2_DIR_MASK, HXD_GPIO_23_DIR_CTRL,
> +				2, HXD_GPIO2_VALUE_MASK,
> +				HXD_GPIO_23_VALUE_CTRL);
> +		pl2303_gpio_add(serial, 3, HXD_GPIO_23_VALUE,
> +				1, HXD_GPIO3_DIR_MASK, HXD_GPIO_23_DIR_CTRL,
> +				2, HXD_GPIO3_VALUE_MASK,
> +				HXD_GPIO_23_VALUE_CTRL);
> +	}
> +
> +	gpio->serial = serial;
> +	mutex_init(&gpio->lock);
> +
> +	gpio->gpio_chip.label = "pl2303";
> +	gpio->gpio_chip.owner = THIS_MODULE;
> +	gpio->gpio_chip.direction_input = pl2303_gpio_direction_in;
> +	gpio->gpio_chip.get = pl2303_gpio_get;
> +	gpio->gpio_chip.direction_output = pl2303_gpio_direction_out;

Why not keep direction_{in,out} together?

> +	gpio->gpio_chip.set = pl2303_gpio_set;
> +	gpio->gpio_chip.can_sleep  = true;

I already mentioned the stray     ^ whitespace here I believe.

> +	gpio->gpio_chip.ngpio = spriv->ngpio;
> +	gpio->gpio_chip.base = -1;
> +	gpio->gpio_chip.dev = &serial->interface->dev;
> +
> +	ret = gpiochip_add(&gpio->gpio_chip);
> +	if (ret < 0) {
> +		kfree(gpio);
> +		return ret;
> +	}
> +	return 0;
> +}
> +
> +static void pl2303_gpio_release(struct usb_serial *serial)
> +{
> +	struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
> +	struct pl2303_gpio *gpio = (struct pl2303_gpio *)spriv->gpio;

No need to cast.

> +
> +	gpiochip_remove(&gpio->gpio_chip);
> +	kfree(gpio->descs);
> +	kfree(gpio);
> +}
> +#endif
> +
>  static int pl2303_startup(struct usb_serial *serial)
>  {
>  	struct pl2303_serial_private *spriv;
>  	enum pl2303_type type = TYPE_01;
>  	unsigned char *buf;
> +	u16 bcdDevice;
> +	u8 major_revision;
> +	int ret;
>  
>  	spriv = kzalloc(sizeof(*spriv), GFP_KERNEL);
>  	if (!spriv)
> @@ -244,6 +475,16 @@ static int pl2303_startup(struct usb_serial *serial)
>  	spriv->quirks = (unsigned long)usb_get_serial_data(serial);
>  	spriv->quirks |= spriv->type->quirks;
>  
> +	spriv->ngpio = 0;
> +	if (type == TYPE_HX) {
> +		bcdDevice = le16_to_cpu(serial->dev->descriptor.bcdDevice);
> +		major_revision = bcdDevice = bcdDevice >> 8;
> +		if (major_revision == 3)
> +			spriv->ngpio = 2;
> +		else if (major_revision == 4)
> +			spriv->ngpio = 4;
> +	}

This should be implemented as a new device type. Split the current
TYPE_HX into TYPE_HX and TYPE_HXD and store the number of gpios along
with the type data as in previous versions of the patch.

And please do this on top of the usb-next branch of my tree

	git://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git

as there have been some updates related to the TYPE_HX.

> +
>  	usb_set_serial_data(serial, spriv);
>  
>  	pl2303_vendor_read(serial, 0x8484, buf);
> @@ -263,6 +504,13 @@ static int pl2303_startup(struct usb_serial *serial)
>  
>  	kfree(buf);
>  
> +	if (spriv->ngpio > 0) {

Move the ngpio check to pl2303_gpio_startup as most devices will have
gpios (or it's compiled out).

> +		ret = pl2303_gpio_startup(serial);
> +		if (ret) {
> +			kfree(spriv);
> +			return ret;
> +		}
> +	}
>  	return 0;
>  }
>  
> @@ -270,6 +518,8 @@ static void pl2303_release(struct usb_serial *serial)
>  {
>  	struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
>  
> +	if (spriv->ngpio > 0)

Same here.

> +		pl2303_gpio_release(serial);
>  	kfree(spriv);
>  }

Thanks,
Johan

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

[Linus Walleij]

On Thu, Sep 4, 2014 at 11:57 PM, Benjamin Henrion <zoobab@gmail.com> wrote:
> On Thu, Sep 4, 2014 at 7:35 PM, Wang YanQing <udknight@gmail.com> wrote:
>> On Sun, Aug 31, 2014 at 11:24:56PM +0800, Wang YanQing wrote:
>>> PL2303 USB Serial devices may has GPIOs, this patch add
>>> basic PL2303 gpio support.
>>>
>>> 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 <udknight@gmail.com>
>>> ---
>>>  Note: I sniffed office HXD gpio test program to get
>>>        gpios control protocol with PL2303 RA, so I only
>>>        test it with PL2303 RA and HXA, I don't have HXD,
>>>        but because it is *office* HXD gpio test program,
>>>        so if it doesn't work with HXD, I will feel surprise.
>
> I can confirm your patch is working fine on the HXD:
>
> http://www.zoobab.com/pl2303hxd-gpio
>
> I am still hunting for a simple C program to test the speed of the
> GPIOs and sysfs, if you have a good reference.

The exercise of the GPIO sysfs interface should be discouraged.
I would put emphasis on in-kernel tests and working on a better
userspace GPIO interface than the sysfs thing.

Yours,
Linus Walleij

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

[Benjamin Henrion]

On Thu, Sep 4, 2014 at 7:35 PM, Wang YanQing <udknight@gmail.com> wrote:
> On Sun, Aug 31, 2014 at 11:24:56PM +0800, Wang YanQing wrote:
>> PL2303 USB Serial devices may has GPIOs, this patch add
>> basic PL2303 gpio support.
>>
>> 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 <udknight@gmail.com>
>> ---
>>  Note: I sniffed office HXD gpio test program to get
>>        gpios control protocol with PL2303 RA, so I only
>>        test it with PL2303 RA and HXA, I don't have HXD,
>>        but because it is *office* HXD gpio test program,
>>        so if it doesn't work with HXD, I will feel surprise.

I can confirm your patch is working fine on the HXD:

http://www.zoobab.com/pl2303hxd-gpio

I am still hunting for a simple C program to test the speed of the
GPIOs and sysfs, if you have a good reference.

Best,

-- 
Benjamin Henrion <bhenrion at ffii.org>
FFII Brussels - +32-484-566109 - +32-2-4148403
"In July 2005, after several failed attempts to legalise software
patents in Europe, the patent establishment changed its strategy.
Instead of explicitly seeking to sanction the patentability of
software, they are now seeking to create a central European patent
court, which would establish and enforce patentability rules in their
favor, without any possibility of correction by competing courts or
democratically elected legislators."

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

[Wang YanQing]

On Sun, Aug 31, 2014 at 11:24:56PM +0800, Wang YanQing wrote:
> PL2303 USB Serial devices may has GPIOs, this patch add
> basic PL2303 gpio support.
> 
> 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 <udknight@gmail.com>
> ---
>  Note: I sniffed office HXD gpio test program to get 
>        gpios control protocol with PL2303 RA, so I only
>        test it with PL2303 RA and HXA, I don't have HXD,
>        but because it is *office* HXD gpio test program,
>        so if it doesn't work with HXD, I will feel surprise.
> 
>  Changes
>  v7-v8:
>  1: add support for four dedicated gpios on HXD and RA
>  2: fix problem reported by Johan Hovold in v7
>  3: fix checkpatch.pl's warning
> 
>  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 gnomes@lxorguk.ukuu.org.uk 
>  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  |  11 ++
>  drivers/usb/serial/pl2303.c | 250 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 261 insertions(+)
> 
> diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
> index 3ce5c74..b4e0cf9 100644
> --- a/drivers/usb/serial/Kconfig
> +++ b/drivers/usb/serial/Kconfig
> @@ -516,6 +516,17 @@ 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, 4 dedicated GPIOs on
> +	  PL2303HXD and PL2303RA currently.
> +	  If unsure, say N.
> +
>  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 e9bad92..a3f8411 100644
> --- a/drivers/usb/serial/pl2303.c
> +++ b/drivers/usb/serial/pl2303.c
> @@ -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"
>  
>  
> @@ -132,6 +134,22 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  #define UART_OVERRUN_ERROR		0x40
>  #define UART_CTS			0x80
>  
> +#define HXD_GPIO_01_CTRL		0x0001
> +#define HXD_GPIO_01_VALUE		0x0081
> +#define HXD_GPIO_23_DIR_CTRL		0x0c0c
> +#define HXD_GPIO_23_VALUE_CTRL		0x0d0d
> +#define HXD_GPIO_23_VALUE		0x8d8d
> +
> +#define HXD_GPIO0_DIR_MASK		0x10
> +#define HXD_GPIO1_DIR_MASK		0x20
> +#define HXD_GPIO2_DIR_MASK		0x03
> +#define HXD_GPIO3_DIR_MASK		0x0c
> +
> +#define HXD_GPIO0_VALUE_MASK		0x40
> +#define HXD_GPIO1_VALUE_MASK		0x80
> +#define HXD_GPIO2_VALUE_MASK		0x01
> +#define HXD_GPIO3_VALUE_MASK		0x02
> +
>  
>  enum pl2303_type {
>  	TYPE_01,	/* Type 0 and 1 (difference unknown) */
> @@ -144,9 +162,12 @@ struct pl2303_type_data {
>  	unsigned long quirks;
>  };
>  
> +struct pl2303_gpio;
>  struct pl2303_serial_private {
>  	const struct pl2303_type_data *type;
>  	unsigned long quirks;
> +	u8 ngpio;
> +	struct pl2303_gpio *gpio;
>  };
>  
>  struct pl2303_private {
> @@ -157,6 +178,14 @@ struct pl2303_private {
>  	u8 line_settings[7];
>  };
>  
> +#ifdef CONFIG_USB_SERIAL_PL2303_GPIO
> +static int pl2303_gpio_startup(struct usb_serial *serial);
> +static void pl2303_gpio_release(struct usb_serial *serial);
> +#else
> +static inline int pl2303_gpio_startup(struct usb_serial *serial) { return 0; }
> +static inline void pl2303_gpio_release(struct usb_serial *serial) {}
> +#endif
> +
>  static const struct pl2303_type_data pl2303_type_data[TYPE_COUNT] = {
>  	[TYPE_01] = {
>  		.max_baud_rate =	1228800,
> @@ -214,11 +243,213 @@ static int pl2303_probe(struct usb_serial *serial,
>  	return 0;
>  }
>  
> +#ifdef CONFIG_USB_SERIAL_PL2303_GPIO
> +struct pl2303_gpio_desc {
> +	u8 dir_offset;
> +	u8 dir_mask;
> +	u16 dir_ctrl;
> +	u8 value_offset;
> +	u8 value_mask;
> +	u16 value_ctrl;
> +	u16 read_ctrl;
> +};
> +
> +struct pl2303_gpio {
> +	struct pl2303_gpio_desc *descs;
> +	u8 data[3];
> +	struct mutex lock;
> +	struct usb_serial *serial;
> +	struct gpio_chip gpio_chip;
> +};
> +
> +static inline struct pl2303_gpio *to_pl2303_gpio(struct gpio_chip *chip)
> +{
> +	return container_of(chip, struct pl2303_gpio, gpio_chip);
> +}
> +
> +static void pl2303_gpio_add(struct usb_serial *serial, u8 num, u16 read_ctrl,
> +			u8 dir_offset, u8 dir_mask, u16 dir_ctrl,
> +			u8 value_offset, u8 value_mask, u16 value_ctrl)
> +{
> +	struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
> +	struct pl2303_gpio *gpio = spriv->gpio;
> +
> +	BUG_ON(num >= spriv->ngpio);
> +	gpio->descs[num].dir_offset = dir_offset;
> +	gpio->descs[num].dir_mask = dir_mask;
> +	gpio->descs[num].dir_ctrl = dir_ctrl;
> +
> +	gpio->descs[num].value_offset = value_offset;
> +	gpio->descs[num].value_mask = value_mask;
> +	gpio->descs[num].value_ctrl = value_ctrl;
> +
> +	gpio->descs[num].read_ctrl = read_ctrl;
> +}
> +
> +static int pl2303_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
> +{
> +	struct pl2303_gpio *gpio = to_pl2303_gpio(chip);
> +	struct pl2303_gpio_desc *desc;
> +	int ret;
> +
> +	mutex_lock(&gpio->lock);
> +	desc = gpio->descs+offset;
> +	gpio->data[desc->dir_offset] &= ~desc->dir_mask;
> +	ret = pl2303_vendor_write(gpio->serial, desc->dir_ctrl,
> +				gpio->data[desc->dir_offset]);
> +	mutex_unlock(&gpio->lock);
> +
> +	return ret;
> +}
> +
> +static int pl2303_gpio_direction_out(struct gpio_chip *chip,
> +				unsigned offset, int value)
> +{
> +	struct pl2303_gpio *gpio = to_pl2303_gpio(chip);
> +	struct pl2303_gpio_desc *desc;
> +	int ret;
> +
> +	mutex_lock(&gpio->lock);
> +	desc = gpio->descs+offset;
> +	gpio->data[desc->dir_offset] |= desc->dir_mask;
> +	ret = pl2303_vendor_write(gpio->serial, desc->dir_ctrl,
> +				gpio->data[desc->dir_offset]);
> +	if (ret)
> +		goto error;
> +
> +	if (value)
> +		gpio->data[desc->value_offset] |= desc->value_mask;
> +	else
> +		gpio->data[desc->value_offset] &= ~desc->value_mask;
> +
> +	ret = pl2303_vendor_write(gpio->serial, desc->value_ctrl,
> +				gpio->data[desc->value_offset]);
> +error:
> +	mutex_unlock(&gpio->lock);
> +
> +	return ret;
> +}
> +
> +static void pl2303_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> +	struct pl2303_gpio *gpio = to_pl2303_gpio(chip);
> +	struct pl2303_gpio_desc *desc;
> +
> +	mutex_lock(&gpio->lock);
> +	desc = gpio->descs+offset;
> +	if (value)
> +		gpio->data[desc->value_offset] |= desc->value_mask;
> +	else
> +		gpio->data[desc->value_offset] &= ~desc->value_mask;
> +
> +	pl2303_vendor_write(gpio->serial, desc->value_ctrl,
> +			gpio->data[desc->value_offset]);
> +	mutex_unlock(&gpio->lock);
> +}
> +
> +static int pl2303_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> +	struct pl2303_gpio *gpio = to_pl2303_gpio(chip);
> +	struct pl2303_gpio_desc *desc;
> +	unsigned char *buf;
> +	int value;
> +
> +	buf = kzalloc(1, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	mutex_lock(&gpio->lock);
> +	desc = gpio->descs+offset;
> +
> +	if (pl2303_vendor_read(gpio->serial, desc->read_ctrl, buf)) {
> +		mutex_unlock(&gpio->lock);
> +		return -EIO;
> +	}
> +
> +	value = buf[0] & desc->value_mask;
> +	mutex_unlock(&gpio->lock);
> +	kfree(buf);
> +
> +	return value;
> +}
> +
> +static int pl2303_gpio_startup(struct usb_serial *serial)
> +{
> +	struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
> +	struct pl2303_gpio *gpio;
> +	int ret;
> +
> +	gpio = kzalloc(sizeof(struct pl2303_gpio), GFP_KERNEL);
> +	if (!gpio)
> +		return -ENOMEM;
> +
> +	gpio->descs = kcalloc(spriv->ngpio, sizeof(struct pl2303_gpio_desc),
> +			GFP_KERNEL);
> +	if (!gpio->descs) {
> +		kfree(gpio);
> +		return -ENOMEM;
> +	}
> +	spriv->gpio = gpio;
> +
> +	pl2303_gpio_add(serial, 0, HXD_GPIO_01_VALUE,
> +			0, HXD_GPIO0_DIR_MASK, HXD_GPIO_01_CTRL,
> +			0, HXD_GPIO0_VALUE_MASK, HXD_GPIO_01_CTRL);
> +	pl2303_gpio_add(serial, 1, HXD_GPIO_01_VALUE,
> +			0, HXD_GPIO1_DIR_MASK, HXD_GPIO_01_CTRL,
> +			0, HXD_GPIO1_VALUE_MASK, HXD_GPIO_01_CTRL);
> +
> +	if (spriv->ngpio == 4) {
> +		pl2303_gpio_add(serial, 2, HXD_GPIO_23_VALUE,
> +				1, HXD_GPIO2_DIR_MASK, HXD_GPIO_23_DIR_CTRL,
> +				2, HXD_GPIO2_VALUE_MASK,
> +				HXD_GPIO_23_VALUE_CTRL);
> +		pl2303_gpio_add(serial, 3, HXD_GPIO_23_VALUE,
> +				1, HXD_GPIO3_DIR_MASK, HXD_GPIO_23_DIR_CTRL,
> +				2, HXD_GPIO3_VALUE_MASK,
> +				HXD_GPIO_23_VALUE_CTRL);
> +	}
> +
> +	gpio->serial = serial;
> +	mutex_init(&gpio->lock);
> +
> +	gpio->gpio_chip.label = "pl2303";
> +	gpio->gpio_chip.owner = THIS_MODULE;
> +	gpio->gpio_chip.direction_input = pl2303_gpio_direction_in;
> +	gpio->gpio_chip.get = pl2303_gpio_get;
> +	gpio->gpio_chip.direction_output = pl2303_gpio_direction_out;
> +	gpio->gpio_chip.set = pl2303_gpio_set;
> +	gpio->gpio_chip.can_sleep  = true;
> +	gpio->gpio_chip.ngpio = spriv->ngpio;
> +	gpio->gpio_chip.base = -1;
> +	gpio->gpio_chip.dev = &serial->interface->dev;
> +
> +	ret = gpiochip_add(&gpio->gpio_chip);
> +	if (ret < 0) {
> +		kfree(gpio);
> +		return ret;
> +	}
> +	return 0;
> +}
> +
> +static void pl2303_gpio_release(struct usb_serial *serial)
> +{
> +	struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
> +	struct pl2303_gpio *gpio = (struct pl2303_gpio *)spriv->gpio;
> +
> +	gpiochip_remove(&gpio->gpio_chip);
> +	kfree(gpio->descs);
> +	kfree(gpio);
> +}
> +#endif
> +
>  static int pl2303_startup(struct usb_serial *serial)
>  {
>  	struct pl2303_serial_private *spriv;
>  	enum pl2303_type type = TYPE_01;
>  	unsigned char *buf;
> +	u16 bcdDevice;
> +	u8 major_revision;
> +	int ret;
>  
>  	spriv = kzalloc(sizeof(*spriv), GFP_KERNEL);
>  	if (!spriv)
> @@ -244,6 +475,16 @@ static int pl2303_startup(struct usb_serial *serial)
>  	spriv->quirks = (unsigned long)usb_get_serial_data(serial);
>  	spriv->quirks |= spriv->type->quirks;
>  
> +	spriv->ngpio = 0;
> +	if (type == TYPE_HX) {
> +		bcdDevice = le16_to_cpu(serial->dev->descriptor.bcdDevice);
> +		major_revision = bcdDevice = bcdDevice >> 8;
> +		if (major_revision == 3)
> +			spriv->ngpio = 2;
> +		else if (major_revision == 4)
> +			spriv->ngpio = 4;
> +	}
> +
>  	usb_set_serial_data(serial, spriv);
>  
>  	pl2303_vendor_read(serial, 0x8484, buf);
> @@ -263,6 +504,13 @@ static int pl2303_startup(struct usb_serial *serial)
>  
>  	kfree(buf);
>  
> +	if (spriv->ngpio > 0) {
> +		ret = pl2303_gpio_startup(serial);
> +		if (ret) {
> +			kfree(spriv);
> +			return ret;
> +		}
> +	}
>  	return 0;
>  }
>  
> @@ -270,6 +518,8 @@ static void pl2303_release(struct usb_serial *serial)
>  {
>  	struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
>  
> +	if (spriv->ngpio > 0)
> +		pl2303_gpio_release(serial);
>  	kfree(spriv);
>  }
>  
> -- 
> 1.8.5.5.dirty

Add Benjamin Henrion <zoobab@gmail.com> to CC list, thanks very much for his test work with this patch.

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

[Linus Walleij]

On Sun, Aug 31, 2014 at 5:24 PM, Wang YanQing <udknight@gmail.com> wrote:

> PL2303 USB Serial devices may has GPIOs, this patch add
> basic PL2303 gpio support.
>
> 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 <udknight@gmail.com>
> ---
>  Note: I sniffed office HXD gpio test program to get
>        gpios control protocol with PL2303 RA, so I only
>        test it with PL2303 RA and HXA, I don't have HXD,
>        but because it is *office* HXD gpio test program,
>        so if it doesn't work with HXD, I will feel surprise.
>
>  Changes
>  v7-v8:
>  1: add support for four dedicated gpios on HXD and RA
>  2: fix problem reported by Johan Hovold in v7
>  3: fix checkpatch.pl's warning

This has been looking nice from the GPIO side of things
for a while so:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

[Wang YanQing]

PL2303 USB Serial devices may has GPIOs, this patch add
basic PL2303 gpio support.

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 <udknight@gmail.com>
---
 Note: I sniffed office HXD gpio test program to get 
       gpios control protocol with PL2303 RA, so I only
       test it with PL2303 RA and HXA, I don't have HXD,
       but because it is *office* HXD gpio test program,
       so if it doesn't work with HXD, I will feel surprise.

 Changes
 v7-v8:
 1: add support for four dedicated gpios on HXD and RA
 2: fix problem reported by Johan Hovold in v7
 3: fix checkpatch.pl's warning

 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 gnomes@lxorguk.ukuu.org.uk 
 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  |  11 ++
 drivers/usb/serial/pl2303.c | 250 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 261 insertions(+)

diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
index 3ce5c74..b4e0cf9 100644
--- a/drivers/usb/serial/Kconfig
+++ b/drivers/usb/serial/Kconfig
@@ -516,6 +516,17 @@ 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, 4 dedicated GPIOs on
+	  PL2303HXD and PL2303RA currently.
+	  If unsure, say N.
+
 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 e9bad92..a3f8411 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -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"
 
 
@@ -132,6 +134,22 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define UART_OVERRUN_ERROR		0x40
 #define UART_CTS			0x80
 
+#define HXD_GPIO_01_CTRL		0x0001
+#define HXD_GPIO_01_VALUE		0x0081
+#define HXD_GPIO_23_DIR_CTRL		0x0c0c
+#define HXD_GPIO_23_VALUE_CTRL		0x0d0d
+#define HXD_GPIO_23_VALUE		0x8d8d
+
+#define HXD_GPIO0_DIR_MASK		0x10
+#define HXD_GPIO1_DIR_MASK		0x20
+#define HXD_GPIO2_DIR_MASK		0x03
+#define HXD_GPIO3_DIR_MASK		0x0c
+
+#define HXD_GPIO0_VALUE_MASK		0x40
+#define HXD_GPIO1_VALUE_MASK		0x80
+#define HXD_GPIO2_VALUE_MASK		0x01
+#define HXD_GPIO3_VALUE_MASK		0x02
+
 
 enum pl2303_type {
 	TYPE_01,	/* Type 0 and 1 (difference unknown) */
@@ -144,9 +162,12 @@ struct pl2303_type_data {
 	unsigned long quirks;
 };
 
+struct pl2303_gpio;
 struct pl2303_serial_private {
 	const struct pl2303_type_data *type;
 	unsigned long quirks;
+	u8 ngpio;
+	struct pl2303_gpio *gpio;
 };
 
 struct pl2303_private {
@@ -157,6 +178,14 @@ struct pl2303_private {
 	u8 line_settings[7];
 };
 
+#ifdef CONFIG_USB_SERIAL_PL2303_GPIO
+static int pl2303_gpio_startup(struct usb_serial *serial);
+static void pl2303_gpio_release(struct usb_serial *serial);
+#else
+static inline int pl2303_gpio_startup(struct usb_serial *serial) { return 0; }
+static inline void pl2303_gpio_release(struct usb_serial *serial) {}
+#endif
+
 static const struct pl2303_type_data pl2303_type_data[TYPE_COUNT] = {
 	[TYPE_01] = {
 		.max_baud_rate =	1228800,
@@ -214,11 +243,213 @@ static int pl2303_probe(struct usb_serial *serial,
 	return 0;
 }
 
+#ifdef CONFIG_USB_SERIAL_PL2303_GPIO
+struct pl2303_gpio_desc {
+	u8 dir_offset;
+	u8 dir_mask;
+	u16 dir_ctrl;
+	u8 value_offset;
+	u8 value_mask;
+	u16 value_ctrl;
+	u16 read_ctrl;
+};
+
+struct pl2303_gpio {
+	struct pl2303_gpio_desc *descs;
+	u8 data[3];
+	struct mutex lock;
+	struct usb_serial *serial;
+	struct gpio_chip gpio_chip;
+};
+
+static inline struct pl2303_gpio *to_pl2303_gpio(struct gpio_chip *chip)
+{
+	return container_of(chip, struct pl2303_gpio, gpio_chip);
+}
+
+static void pl2303_gpio_add(struct usb_serial *serial, u8 num, u16 read_ctrl,
+			u8 dir_offset, u8 dir_mask, u16 dir_ctrl,
+			u8 value_offset, u8 value_mask, u16 value_ctrl)
+{
+	struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
+	struct pl2303_gpio *gpio = spriv->gpio;
+
+	BUG_ON(num >= spriv->ngpio);
+	gpio->descs[num].dir_offset = dir_offset;
+	gpio->descs[num].dir_mask = dir_mask;
+	gpio->descs[num].dir_ctrl = dir_ctrl;
+
+	gpio->descs[num].value_offset = value_offset;
+	gpio->descs[num].value_mask = value_mask;
+	gpio->descs[num].value_ctrl = value_ctrl;
+
+	gpio->descs[num].read_ctrl = read_ctrl;
+}
+
+static int pl2303_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
+{
+	struct pl2303_gpio *gpio = to_pl2303_gpio(chip);
+	struct pl2303_gpio_desc *desc;
+	int ret;
+
+	mutex_lock(&gpio->lock);
+	desc = gpio->descs+offset;
+	gpio->data[desc->dir_offset] &= ~desc->dir_mask;
+	ret = pl2303_vendor_write(gpio->serial, desc->dir_ctrl,
+				gpio->data[desc->dir_offset]);
+	mutex_unlock(&gpio->lock);
+
+	return ret;
+}
+
+static int pl2303_gpio_direction_out(struct gpio_chip *chip,
+				unsigned offset, int value)
+{
+	struct pl2303_gpio *gpio = to_pl2303_gpio(chip);
+	struct pl2303_gpio_desc *desc;
+	int ret;
+
+	mutex_lock(&gpio->lock);
+	desc = gpio->descs+offset;
+	gpio->data[desc->dir_offset] |= desc->dir_mask;
+	ret = pl2303_vendor_write(gpio->serial, desc->dir_ctrl,
+				gpio->data[desc->dir_offset]);
+	if (ret)
+		goto error;
+
+	if (value)
+		gpio->data[desc->value_offset] |= desc->value_mask;
+	else
+		gpio->data[desc->value_offset] &= ~desc->value_mask;
+
+	ret = pl2303_vendor_write(gpio->serial, desc->value_ctrl,
+				gpio->data[desc->value_offset]);
+error:
+	mutex_unlock(&gpio->lock);
+
+	return ret;
+}
+
+static void pl2303_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct pl2303_gpio *gpio = to_pl2303_gpio(chip);
+	struct pl2303_gpio_desc *desc;
+
+	mutex_lock(&gpio->lock);
+	desc = gpio->descs+offset;
+	if (value)
+		gpio->data[desc->value_offset] |= desc->value_mask;
+	else
+		gpio->data[desc->value_offset] &= ~desc->value_mask;
+
+	pl2303_vendor_write(gpio->serial, desc->value_ctrl,
+			gpio->data[desc->value_offset]);
+	mutex_unlock(&gpio->lock);
+}
+
+static int pl2303_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+	struct pl2303_gpio *gpio = to_pl2303_gpio(chip);
+	struct pl2303_gpio_desc *desc;
+	unsigned char *buf;
+	int value;
+
+	buf = kzalloc(1, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	mutex_lock(&gpio->lock);
+	desc = gpio->descs+offset;
+
+	if (pl2303_vendor_read(gpio->serial, desc->read_ctrl, buf)) {
+		mutex_unlock(&gpio->lock);
+		return -EIO;
+	}
+
+	value = buf[0] & desc->value_mask;
+	mutex_unlock(&gpio->lock);
+	kfree(buf);
+
+	return value;
+}
+
+static int pl2303_gpio_startup(struct usb_serial *serial)
+{
+	struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
+	struct pl2303_gpio *gpio;
+	int ret;
+
+	gpio = kzalloc(sizeof(struct pl2303_gpio), GFP_KERNEL);
+	if (!gpio)
+		return -ENOMEM;
+
+	gpio->descs = kcalloc(spriv->ngpio, sizeof(struct pl2303_gpio_desc),
+			GFP_KERNEL);
+	if (!gpio->descs) {
+		kfree(gpio);
+		return -ENOMEM;
+	}
+	spriv->gpio = gpio;
+
+	pl2303_gpio_add(serial, 0, HXD_GPIO_01_VALUE,
+			0, HXD_GPIO0_DIR_MASK, HXD_GPIO_01_CTRL,
+			0, HXD_GPIO0_VALUE_MASK, HXD_GPIO_01_CTRL);
+	pl2303_gpio_add(serial, 1, HXD_GPIO_01_VALUE,
+			0, HXD_GPIO1_DIR_MASK, HXD_GPIO_01_CTRL,
+			0, HXD_GPIO1_VALUE_MASK, HXD_GPIO_01_CTRL);
+
+	if (spriv->ngpio == 4) {
+		pl2303_gpio_add(serial, 2, HXD_GPIO_23_VALUE,
+				1, HXD_GPIO2_DIR_MASK, HXD_GPIO_23_DIR_CTRL,
+				2, HXD_GPIO2_VALUE_MASK,
+				HXD_GPIO_23_VALUE_CTRL);
+		pl2303_gpio_add(serial, 3, HXD_GPIO_23_VALUE,
+				1, HXD_GPIO3_DIR_MASK, HXD_GPIO_23_DIR_CTRL,
+				2, HXD_GPIO3_VALUE_MASK,
+				HXD_GPIO_23_VALUE_CTRL);
+	}
+
+	gpio->serial = serial;
+	mutex_init(&gpio->lock);
+
+	gpio->gpio_chip.label = "pl2303";
+	gpio->gpio_chip.owner = THIS_MODULE;
+	gpio->gpio_chip.direction_input = pl2303_gpio_direction_in;
+	gpio->gpio_chip.get = pl2303_gpio_get;
+	gpio->gpio_chip.direction_output = pl2303_gpio_direction_out;
+	gpio->gpio_chip.set = pl2303_gpio_set;
+	gpio->gpio_chip.can_sleep  = true;
+	gpio->gpio_chip.ngpio = spriv->ngpio;
+	gpio->gpio_chip.base = -1;
+	gpio->gpio_chip.dev = &serial->interface->dev;
+
+	ret = gpiochip_add(&gpio->gpio_chip);
+	if (ret < 0) {
+		kfree(gpio);
+		return ret;
+	}
+	return 0;
+}
+
+static void pl2303_gpio_release(struct usb_serial *serial)
+{
+	struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
+	struct pl2303_gpio *gpio = (struct pl2303_gpio *)spriv->gpio;
+
+	gpiochip_remove(&gpio->gpio_chip);
+	kfree(gpio->descs);
+	kfree(gpio);
+}
+#endif
+
 static int pl2303_startup(struct usb_serial *serial)
 {
 	struct pl2303_serial_private *spriv;
 	enum pl2303_type type = TYPE_01;
 	unsigned char *buf;
+	u16 bcdDevice;
+	u8 major_revision;
+	int ret;
 
 	spriv = kzalloc(sizeof(*spriv), GFP_KERNEL);
 	if (!spriv)
@@ -244,6 +475,16 @@ static int pl2303_startup(struct usb_serial *serial)
 	spriv->quirks = (unsigned long)usb_get_serial_data(serial);
 	spriv->quirks |= spriv->type->quirks;
 
+	spriv->ngpio = 0;
+	if (type == TYPE_HX) {
+		bcdDevice = le16_to_cpu(serial->dev->descriptor.bcdDevice);
+		major_revision = bcdDevice = bcdDevice >> 8;
+		if (major_revision == 3)
+			spriv->ngpio = 2;
+		else if (major_revision == 4)
+			spriv->ngpio = 4;
+	}
+
 	usb_set_serial_data(serial, spriv);
 
 	pl2303_vendor_read(serial, 0x8484, buf);
@@ -263,6 +504,13 @@ static int pl2303_startup(struct usb_serial *serial)
 
 	kfree(buf);
 
+	if (spriv->ngpio > 0) {
+		ret = pl2303_gpio_startup(serial);
+		if (ret) {
+			kfree(spriv);
+			return ret;
+		}
+	}
 	return 0;
 }
 
@@ -270,6 +518,8 @@ static void pl2303_release(struct usb_serial *serial)
 {
 	struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
 
+	if (spriv->ngpio > 0)
+		pl2303_gpio_release(serial);
 	kfree(spriv);
 }
 
-- 
1.8.5.5.dirty