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

[Andreas Mohr]

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