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

[Johan Hovold]

On Tue, Sep 04, 2018 at 05:53:14PM +0000, Karoly Pados wrote:

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

Yeah, I understand that point, but still not sold on the idea of having
potentially all four pins change state when you request one of them.

Going back and forth, seamlessly, between having the kernel or user
space manage a device generally just isn't a supported use case.

But there is another (aspect of your) argument for your approach, and
that would be that people may be relying on this behaviour already. That
is, due to lack of CBUS support in the kernel driver, you have libftdi
setup those pins and then bind the kernel driver. Would that even work
today (i.e. it's nothing that gets reset when binding the driver)? If
so, me may have to continue supporting it.

What is the default state of your devices after reset by the way?
All inputs?

Ok, you may have convinced me. :)

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

Actually those two functions don't take any direction argument, but
get_direction() does indeed return 1 for input. But you can find
examples of the opposite too (e.g. FLAG_IS_OUT in gpio_desc).

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

Thanks, I think inverting the direction mask will allow for some
easier-to-follow code in this case.

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

No worries, just an unfortunate coincidence. And the more eyes on this,
the better.

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

Yep, as you noticed in your follow up, some of those devices you refer
to above are not USB-UART bridges.

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

Great, thanks.

Johan