Re: [PATCH v3] usb:serial:pl2303: add GPIOs interface on PL2303
On Mon, Jul 21, 2014 at 4:46 AM, Wang YanQing udkni...@gmail.com wrote:
PL2303HX has two GPIOs, this patch add interface for it.
Signed-off-by: Wang YanQing udkni...@gmail.com
---
Changes 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
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.
The sysfs ABI is not sound, using it is a recipe for trouble.
IIRC it was merged at a time when drivers/gpio was unmaintained :-(
(...)
+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,
This is a bool so use = true,
+#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);
I'm getting rid of the return code from gpiochip_remove() and have removed
the __must_check tag in the gpio tree, so just call gpiochip_remove()
unconditionally and ignore any compile error messages for now.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] usb:serial:pl2303: add GPIOs interface on PL2303
On Mon, Jul 21, 2014 at 07:46:46AM +0200, Andreas Mohr wrote:
Hi,
Did some more review, sorry ;)
On Mon, Jul 21, 2014 at 10:46:24AM +0800, Wang YanQing wrote:
+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,
+};
Could this be made static const? (since it's for one-time copy purposes only)
OK, I will add const.
+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;
Most frequently accessed member at first position - good.
+static inline struct pl2303_gpio *to_pl2303_gpio(struct gpio_chip
*chip)
+{
+ return container_of(chip, struct pl2303_gpio, gpio_chip);
+}
Nicely cast-avoiding helper :)
+ spriv-gpio-index = 0x00;
+ spriv-gpio-serial = serial;
+ spriv-gpio-gpio_chip = template_chip;
+ spriv-gpio-gpio_chip.ngpio = GPIO_NUM;
+ spriv-gpio-gpio_chip.base = -1;
+ spriv-gpio-gpio_chip.dev = serial-interface-dev;
+ /* initialize GPIOs, input mode as default */
+ pl2303_vendor_write(spriv-gpio-serial, 1, spriv-gpio-index);
Perhaps the index line should better be moved
right before the pl2303_vendor_write() line,
to more obviously hint at why it's input mode
(but OTOH currently you're cleanly initializing things in correct member
order,
so it's probably better to keep it that way).
Also, it's perhaps a better idea to do this initial init call
via calls of actual GPIO API rather than implementation-specific call
(e.g. that way one could simple reuse the generic GPIO call for devices
which happen to implement GPIO via a different mechanism...).
(hmm, nope, from a layering POV it's not recommendable,
since we clearly are at hardware-specific init here,
where you want to firmly guarantee
that the hardware is directly and fully initialized).
Thanks very much for review :)
Since there are several repeated
pl2303_vendor_write(...serial, 1, ...index) calls,
it's possibly a good idea to wrap these calls in a convenience
pl2303_gpio_state_update(gpio)
This would also increase GPIO hardware abstraction,
by then simply having to provide an alternative for this single function
if needed.
Also, it may (depending on the number of callers,
which is on the low side here unfortunately)
reduce instruction cache footprint.
The reason I don't want to add more abstraction here are:
1: Abstraction don't reduce code lines, then reuse
pl2303_vendor_write|read is a better choice, i think.
2: pl2303_vendor_write|read is more generic than abstraction,
then someone maybe could use pl2303_vendor_write|read to support
another two Auxiliary GPIOs on PL2303HXD(I don't have one) directly
without play with abstraction.
Thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] usb:serial:pl2303: add GPIOs interface on PL2303
Hi,
Did some more review, sorry ;)
On Mon, Jul 21, 2014 at 10:46:24AM +0800, Wang YanQing wrote:
+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,
+};
Could this be made static const? (since it's for one-time copy purposes only)
+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;
Most frequently accessed member at first position - good.
+static inline struct pl2303_gpio *to_pl2303_gpio(struct gpio_chip
*chip)
+{
+ return container_of(chip, struct pl2303_gpio, gpio_chip);
+}
Nicely cast-avoiding helper :)
+ spriv-gpio-index = 0x00;
+ spriv-gpio-serial = serial;
+ spriv-gpio-gpio_chip = template_chip;
+ spriv-gpio-gpio_chip.ngpio = GPIO_NUM;
+ spriv-gpio-gpio_chip.base = -1;
+ spriv-gpio-gpio_chip.dev = serial-interface-dev;
+ /* initialize GPIOs, input mode as default */
+ pl2303_vendor_write(spriv-gpio-serial, 1, spriv-gpio-index);
Perhaps the index line should better be moved
right before the pl2303_vendor_write() line,
to more obviously hint at why it's input mode
(but OTOH currently you're cleanly initializing things in correct member order,
so it's probably better to keep it that way).
Also, it's perhaps a better idea to do this initial init call
via calls of actual GPIO API rather than implementation-specific call
(e.g. that way one could simple reuse the generic GPIO call for devices
which happen to implement GPIO via a different mechanism...).
(hmm, nope, from a layering POV it's not recommendable,
since we clearly are at hardware-specific init here,
where you want to firmly guarantee
that the hardware is directly and fully initialized).
Since there are several repeated
pl2303_vendor_write(...serial, 1, ...index) calls,
it's possibly a good idea to wrap these calls in a convenience
pl2303_gpio_state_update(gpio)
This would also increase GPIO hardware abstraction,
by then simply having to provide an alternative for this single function
if needed.
Also, it may (depending on the number of callers,
which is on the low side here unfortunately)
reduce instruction cache footprint.
So these are my additional thoughts about the implementation,
which you may or may not choose to adopt.
HTH,
Andreas Mohr
--
GNU/Linux. It's not the software that's free, it's you.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
