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:

drivers/usb/serial/pl2303.c static int pl2303_probe(struct usb_serial *serial, · line 317

Quoted 9 lines

drivers/usb/serial/pl2303.c +9 -0

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

drivers/usb/serial/pl2303.c struct pl2303_type_data { · line 160

Quoted 9 lines

drivers/usb/serial/pl2303.c +9 -0

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

drivers/usb/serial/pl2303.c static int pl2303_probe(struct usb_serial *serial, · line 246

Quoted 5 lines

+static inline struct pl2303_gpio *to_pl2303_gpio(struct gpio_chip *chip)

drivers/usb/serial/pl2303.c +3 -0

+{
+       return container_of(chip, struct pl2303_gpio, gpio_chip);
+}

Nicely cast-avoiding helper :)

drivers/usb/serial/pl2303.c static int pl2303_startup(struct usb_serial *serial) · line 387

Quoted 8 lines

drivers/usb/serial/pl2303.c +8 -0

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

PL2303HX has two GPIOs, this patch add interface for it.

Signed-off-by Wang YanQing <[email protected]>

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