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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 5 Sep 2018 12:36:50 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Karoly Pados <pados@...os.hu>
Cc:     Johan Hovold <johan@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        Loic Poulain <loic.poulain@...aro.org>,
        andy.shevchenko@...il.com, ajaykuee@...il.com,
        daniel.thompson@...aro.org
Subject: Re: [PATCH] USB: serial: ftdi_sio: implement GPIO support for FT230X

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