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

[Wang YanQing]

On Tue, Aug 12, 2014 at 04:46:25PM +0200, Johan Hovold wrote:
> On Sat, Aug 09, 2014 at 01:28:28PM +0800, Wang YanQing wrote:
> > 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)."

> 
> >  enum pl2303_type {
> >  	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?.

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

> 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.
> 
> >  };
> >  
> >  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.

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

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

> > +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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/