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

[Johan Hovold]

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