usb:serial:pl2303: add GPIOs interface on PL2303
Subsystems (2)
PATCH v5 1/1 usb:serial:pl2303: add GPIOs interface on PL2303
PL2303HX has two GPIOs, this patch add interface for it.
---
Changes v4-v5:
1: fix gpio_chip.lable been overwrited by template_chip.
2: use idr to manage minor instead of crude monotonous atomic increment.
drivers/usb/serial/Kconfig | 10 +++ drivers/usb/serial/pl2303.c | 189 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 199 insertions(+)
drivers/usb/serial/Kconfig +10 -0
@@ -516,6 +516,16 @@ 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 the GPIOs on PL2303 USB Serial single port + adapter from Prolific. + + It support 2 GPIOs on PL2303HX currently, + if unsure, say N. + config USB_SERIAL_OTI6858 tristate "USB Ours Technology Inc. OTi-6858 USB To RS232 Bridge Controller" help
drivers/usb/serial/pl2303.c +189 -0
222 lines not on this page · raw message
Signature · 1 line
1.8.5.5.dirty
Hi,
Quoted 1 line · Wang YanQing
+ It support 2 GPIOs on PL2303HX currently,
"supports"
drivers/usb/serial/pl2303.c static int pl2303_probe(struct usb_serial *serial, · line 238
Quoted 2 lines
drivers/usb/serial/pl2303.c +2
-0+static u8 gpio_dir_mask[GPIO_NUM ] = {0x10 ,0x20 }; +static u8 gpio_value_mask[GPIO_NUM ] = {0x40 ,0x80 };
Those two should better be static const, too (sorry). You're at v5 already (wow, what endurance!), but if there ever will be a v6... :)
drivers/usb/serial/pl2303.c static int pl2303_probe(struct usb_serial *serial, · line 325
Quoted 9 lines
drivers/usb/serial/pl2303.c +9
-0+static int pl2303_gpio_startup (struct usb_serial *serial) +{ +struct pl2303_serial_private *spriv =usb_get_serial_data (serial); +char *label; +int ret; +int minor; + +if (spriv->type != &pl2303_type_data[TYPE_HX ]) +return 0 ;
Hmm, that's some structurally inverted check code.
The pl2303_gpio_startup() function in its entirety
is specific to the GPIO-supporting type TYPE_HX,
thus we shouldn't even *call* a type-specific sub handler
if we know that we're a different chip type.
And in fact pl2303_startup() already has everything in place for a direct type check.
Yeah, that might be "less reliable" than a type check planted within pl2303_gpio_startup() (someone might bogusly call pl2303_gpio_startup() for a different-type chip),
but such an unrelated (external!) type check dependency shouldn't be interwoven with a type-specific setup helper which is to be concerned with inner-layer setup handling only.
A probably(!) even better idea here might be
to add some function pointers to spriv->type struct def,
to be able to do != NULL ptr checks and in such cases
call such chip-specific setup functions (i.e., call the HX type helper which internally knows that it needs to do GPIO setup).
Such a change might be able to get rid of several #ifdef:s, too... (plus, provide long-lasting generic infrastructure for future chip types).
Andreas Mohr
Quoted 7 lines · Andreas Mohr
Hi,
Quoted 1 line · Wang YanQing
+ It support 2 GPIOs on PL2303HX currently,
"supports"
Right!
Quoted 7 lines
drivers/usb/serial/pl2303.c static int pl2303_probe(struct usb_serial *serial, · line 238
Quoted 2 lines
drivers/usb/serial/pl2303.c +2
-0+static u8 gpio_dir_mask[GPIO_NUM ] = {0x10 ,0x20 }; +static u8 gpio_value_mask[GPIO_NUM ] = {0x40 ,0x80 };Those two should better be static const, too (sorry). You're at v5 already (wow, what endurance!), but if there ever will be a v6... :)
OK! there will be v6.
Quoted 12 lines
drivers/usb/serial/pl2303.c static int pl2303_probe(struct usb_serial *serial, · line 325
Quoted 9 lines
drivers/usb/serial/pl2303.c +9
-0+static int pl2303_gpio_startup (struct usb_serial *serial) +{ +struct pl2303_serial_private *spriv =usb_get_serial_data (serial); +char *label; +int ret; +int minor; + +if (spriv->type != &pl2303_type_data[TYPE_HX ]) +return 0 ;Hmm, that's some structurally inverted check code. The
pl2303_gpio_startup()function in its entirety
Quoted 8 lines
is specific to the GPIO-supporting type
TYPE_HX,
thus we shouldn't even *call* a type-specific sub handler
if we know that we're a different chip type.
And in factpl2303_startup()already has everything in place for a direct type check.
Yeah, that might be "less reliable" than a type check planted withinpl2303_gpio_startup()(someone might bogusly callpl2303_gpio_startup()for a different-type chip),
This is just caller vs callee check, and ether of them has itself advantage in fit situation. I don't think it is wrong to reuse current pl2303_type_data for type check in pl2303_gpio_startup just like I don't think it is wrong to do the check prior to call pl2303_gpio_starutp in you suggestion.
Quoted 3 lines
but such an unrelated (external!) type check dependency shouldn't be interwoven with a type-specific setup helper which is to be concerned with inner-layer setup handling only.
It is not unrelated type check, type-specific setup helper has right to check whether it is the "really" specified type.:)
Quoted 5 lines
A probably(!) even better idea here might be
to add some function pointers tospriv->typestruct def,
to be able to do != NULL ptr checks and in such cases
call such chip-specific setup functions (i.e., call the HX type helper which internally knows that it needs to do GPIO setup).
I don't think pl2303_type_data is right place to hold it, pl2303_type_data is right place for serial concerning function, not for gpio.
Quoted 1 line
Such a change might be able to get rid of several #ifdef:s, too...
#ifdef is still necessary, most people don't need this feature, it bring no benefit to make kernel image bigger.
Quoted 1 line
(plus, provide long-lasting generic infrastructure for future chip types).
A simple and specified device driver, I doubt the need for so "generic infrastructure", and current code could just work for future chip types, just looks like below:
if (type == HX ||
type == "future type"||
more future type)pl2303_gpio_startup();
Thanks for review, and sorry for my delay reply.