lkml.org 
[lkml]   [2014]   [Aug]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v7] usb:serial:pl2303: add GPIOs interface on PL2303
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.


\
 
 \ /
  Last update: 2014-08-17 03:21    [W:0.071 / U:2.300 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site