On Tue, Sep 04, 2018 at 02:49:24PM +0200, Johan Hovold wrote:
> On Sat, Aug 25, 2018 at 10:47:44PM +0200, Karoly Pados wrote:
Here are some more comments on the setup functions, after having
prepared a patch adding support to FT232R.
> > +static int ftx_gpioconf_init(struct usb_serial_port *port)
Perhaps rename rename this using an ftdi_ prefix as well for consistency
with the current device-specific functions.
> > +{
> > + struct ftdi_private *priv = usb_get_serial_port_data(port);
> > + struct usb_serial *serial = port->serial;
> > + const u16 config_size = 0x24;
> > + u8 *config_buf;
> > + int result;
> > + u8 i;
> > +
[...]
> > + /*
> > + * All FT-X devices have at least 1 GPIO, some have more.
> > + * Chip-type guessing logic based on libftdi.
> > + */
> > + priv->gc.ngpio = 1;
> > + if (serial->dev->descriptor.bcdDevice == 0x1000) /* FT230X */
>
> Missing le16_to_cpu().
>
> > + 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.
>
> > +
> > + /* Determine which pins are configured for CBUS bitbanging */
> > + priv->gpio_altfunc = 0xff;
> > + for (i = 0; i < priv->gc.ngpio; ++i) {
> > + if (config_buf[0x1A + i] == FTDI_SIO_CBUS_MUX_GPIO)
>
> 0x1a warrants a define; but you shouldn't be reading 0x24 bytes from
> offset 0 above when the pinconfig is stored in just a couple of words.
>
> > + priv->gpio_altfunc &= ~BIT(i);
> > + }
> > +
> > + kfree(config_buf);
> > +
> > + return 0;
> > +}
How about returning the number of gpios instead of having every
device-specific function mess with priv->gc?
> > +
> > +static int ftdi_sio_gpio_init(struct usb_serial_port *port)
> > +{
> > + struct ftdi_private *priv = usb_get_serial_port_data(port);
> > + struct usb_serial *serial = port->serial;
> > + int result;
> > +
> > + /* Device-specific initializations */
> > + switch (priv->chip_type) {
> > + case FTX:
> > + result = ftx_gpioconf_init(port);
> > + break;
> > + default:
> > + return 0;
> > + }
> > +
> > + if (result < 0)
> > + return result;
> > +
> > + /* Register GPIO chip to kernel */
> > + priv->gc.label = "ftdi_sio";
>
> Maybe we shouldn't use the legacy sio suffix here; "ftdi" or "ftdi-cbus"
> should do.
>
> > + priv->gc.request = ftdi_sio_gpio_request;
> > + priv->gc.get_direction = ftdi_sio_gpio_direction_get;
> > + priv->gc.direction_input = ftdi_sio_gpio_direction_input;
> > + priv->gc.direction_output = ftdi_sio_gpio_direction_output;
> > + priv->gc.get = ftdi_sio_gpio_get;
> > + priv->gc.set = ftdi_sio_gpio_set;
> > + priv->gc.owner = THIS_MODULE;
> > + priv->gc.parent = &serial->interface->dev;
> > + priv->gc.base = -1;
> > + priv->gc.can_sleep = true;
> > +
> > + result = gpiochip_add_data(&priv->gc, port);
> > + if (!result)
> > + priv->gpio_registered = true;
> > +
> > + return result;
> > +}
> > +#define FTDI_SIO_CBUS_MUX_GPIO 8
And this one needs to have an FTX infix as it's FTX specific (e.g.
rename as FTDI_FTX_CBUS_MUX_IOMODE, which I believe better matches with
the various docs).
Johan
