Re: [PATCH] USB: serial: ftdi_sio: implement GPIO support for FT230X

["Karoly Pados" <pados@pados.hu>]

Thanks for the review, I am on the road most of this week, but I will submit
a new patch this weekend with all your feedback incorporated. Until then here
are my inputs / answers to some of your points. 

> Actually, after thinking about this some more, it may be better to just
> configure all pins during probe. The device is managed by the kernel,
> and we can't really consider what user space may have done before, at
> least if we can't query the device. Better to be in a consistent state
> while the driver is bound.

The CBUS pins do not affect the UART communication in any way, so if we're
not using the GPIO, I don't see any reason why we should destroy the CBUS
state just for the sake of knowing what value they have, even though we don't
set or use or need them and there is also no interaction. But if you wish so,
I'll set them during probe, I just think we are disabling a possible use-case
with no added gain.

>> checkpatch.pl emits a bunch of warnings, recommending const for structs.
>> I am unsure if this makes sense, existing code in this same and also
>> other modules does not adhere to this rule either. I fixed everything else.
> 
> Really? Checkpatch doesn't complain here. Were you using the --strict
> option, perhaps? Don't do that...
> 

Yeah, that's what happened. I'll drop that flag.

>> + /* device's direction polarity is different from kernel's */
> 
> Why so? You could just replace gpio_input with gpio_output. I think that
> may be preferred.

That doesn't change the fact that for the kernel (for example in
ftdi_gpio_direction_input or ftdi_gpio_direction_output) '1' is still input.
Yes I know we can do the inversion in those functions "for free" by setting
instead of clearing and vice-versa, I just thought it is better if I choose
to stay with the kernel convention and turn it device-specific at the deepest
level possible. Not arguing, just explaining what my motivation was. I'll
change it as you requested.

>> + if (!priv->gpio_used) {
>> + /* Set default pin states, as we cannot get them from device */
>> + priv->gpio_input = 0xff;
>> + priv->gpio_value = 0x00;
>> + result = ftdi_sio_set_cbus(port,
>> + priv->gpio_input,
>> + priv->gpio_value,
>> + FTDI_SIO_GPIO_MODE_CBUS);
>> + if (result)
>> + return result;
>> +
>> + priv->gpio_used = true;
>> + }
> 
> So I think this initialisation should be done at probe instead.

See my point further above.

> Factor this out to an eeprom helper that can be reused by other chip
> types.

Yep, I'll consult the other gpio patch regarding this as you suggested
in the other's review. By the way, sorry for the parallel submission,
I wasn't aware of Poulain's patch in this same month.

>> + priv->gc.ngpio = 4;
> 
> Shouldn't this be handled the other way round? IIRC there are two FTX
> device types with four pins, and one type where only one pin is
> accessible.
> 

There are 4 devices with 1 GPIO, 1 device with 2 GPIOs, 2 devices with 4 GPIOs,
and 1 device with 6 GPIOs. Source: http://www.ftdichip.com/FT-X.htm (2nd table)
Do you still want me turn this over?

>> +#else
>> +
>> +static int ftdi_sio_gpio_init(struct usb_serial *serial)
> 
> As the test robot reported, these should take a port as their argument.

Yes, I already sent in a v2 for that. So my patch incorporating your feedback
will be v3.

Thanks again,
Karoly