On Sat, Aug 04, 2018 at 12:17:15PM +0200, Loic Poulain wrote: > Some FTDI devices like FTX or FT232R support CBUS Bit Bang mode on CBUS > pins, allowing host to control them via simple USB control transfers. > To make use of a CBUS pin in Bit Bang mode, the pin must be configured > to I/O mode in the FTDI EEPROM. This mode perfectly coexists with regular > USB to Serial function. > > In this implementation, a GPIO controller is registered on FTDI probe > if at least one CBUS pin is configured for I/O mode. For now, only > FTX devices are supported. > > This patch is based on previous Stefan Agner implementation tentative > on LKML [1]. > > [1] Message-Id: 1434838377-8042-1-git-send-email-stefan@xxxxxxxx > > Signed-off-by: Loic Poulain <loic.poulain@xxxxxxxxxx> > --- > v2: Use message-id for LKML reference > Rework read_eeprom according to Andy's comment and return read count > Remove noisy messages > Comment style alignment > Add defines for magic values > Cannot use devm, because gpiochip is linked to the port not to the udev > v3: > Fix typo in CBUS mask description comment First, thanks for looking into this; seems like we're finally about to get support for the CBUS gpios. But now I get not one, but two, competing implementations for this posted in one month: https://lkml.kernel.org/r/20180825204744.2307-1-pados@xxxxxxxx And it looks like you both have been considering at least some of the earlier attempts, which is great. I've reviewed both patches and it seems to me that Karoly's version is closer to what I'd like to see as the end-result. Specifically this means having: - fixed offsets for the physical pins rather than having the offsets depend on eeprom configuration - better type abstraction (we want to add support for ft232r and ft232h eventually as well) The other patch is also more complete in that it: - considers the initial pin state - implements a request() callback - implements a get_direction() callback In contrast, with this implementation as it stands, we could end up with a driven pin being reported as an input (corner case, but still). Implementation-wise, the other patch is also closer to what I'd like to see (e.g. not using atomic bit ops, and getting the error handling right from the start). There are some issues that needs to be addressed in the other patch as well, and in doing so it would be wise to look at your patch for inspiration (e.g. naming issues and adding an eeprom helper to only read the couple of configuration words needed). In the end, going with the other patch means less work for me, even if you both (by unfortunate timing) have forced me to do more than twice the amount of review work already. Hopefully we can all work together on getting the missing bits, including further device support, in place. For completeness, I've included my review comments below. Thanks, Johan > drivers/usb/serial/Kconfig | 9 ++ > drivers/usb/serial/ftdi_sio.c | 238 ++++++++++++++++++++++++++++++++++++++++++ > drivers/usb/serial/ftdi_sio.h | 83 +++++++++++++++ > 3 files changed, 330 insertions(+) > > diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig > index 533f127..64c9f2e 100644 > --- a/drivers/usb/serial/Kconfig > +++ b/drivers/usb/serial/Kconfig > @@ -181,6 +181,15 @@ config USB_SERIAL_FTDI_SIO > To compile this driver as a module, choose M here: the > module will be called ftdi_sio. > > +config USB_SERIAL_FTDI_SIO_CBUS_GPIO > + bool "USB FDTI CBUS GPIO support" > + depends on USB_SERIAL_FTDI_SIO > + depends on GPIOLIB > + help > + Say yes here to add support for the CBUS bit-bang mode, allowing CBUS > + pins to act as GPIOs. Note that pins must first be configured for GPIO > + in the device's EEPROM. The FT232R and FT-X series support this mode. I understand you're referring to hardware, but this is ambiguous as you don't implement support for FT232R in the driver. > + > config USB_SERIAL_VISOR > tristate "USB Handspring Visor / Palm m50x / Sony Clie Driver" > help > diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c > index b5cef32..e90cae6 100644 > --- a/drivers/usb/serial/ftdi_sio.c > +++ b/drivers/usb/serial/ftdi_sio.c > @@ -33,6 +33,7 @@ > #include <linux/tty.h> > #include <linux/tty_driver.h> > #include <linux/tty_flip.h> > +#include <linux/gpio/driver.h> > #include <linux/module.h> > #include <linux/spinlock.h> > #include <linux/mutex.h> > @@ -40,12 +41,21 @@ > #include <linux/usb.h> > #include <linux/serial.h> > #include <linux/usb/serial.h> > + Unrelated change. > #include "ftdi_sio.h" > #include "ftdi_sio_ids.h" > > #define DRIVER_AUTHOR "Greg Kroah-Hartman <greg@xxxxxxxxx>, Bill Ryder <bryder@xxxxxxx>, Kuba Ober <kuba@xxxxxxxxxxxxxxx>, Andreas Mohr, Johan Hovold <jhovold@xxxxxxxxx>" > #define DRIVER_DESC "USB FTDI Serial Converters Driver" > > +#define FTDI_CBUS_PIN_COUNT 4 > + > +struct ftdi_gpiochip { > + struct gpio_chip gc; > + struct usb_serial_port *port; > + unsigned int cbus_map[FTDI_CBUS_PIN_COUNT]; Looks like u8 will do here since it's just an index, right? But see more about this mapping below first. > + unsigned long cbus_mask; > +}; > > struct ftdi_private { > enum ftdi_chip_type chip_type; > @@ -72,6 +82,8 @@ struct ftdi_private { > unsigned int latency; /* latency setting in use */ > unsigned short max_packet_size; > struct mutex cfg_lock; /* Avoid mess by parallel calls of config ioctl() and change_speed() */ > + > + struct ftdi_gpiochip *fgc; > }; > > /* struct ftdi_sio_quirk is used by devices requiring special attention. */ > @@ -1528,6 +1540,224 @@ static int get_lsr_info(struct usb_serial_port *port, > return 0; > } > > +#ifdef CONFIG_USB_SERIAL_FTDI_SIO_CBUS_GPIO > + > +#define FTX_CBUS_MUX_EEPROM_ADDR 0x1a > + > +static int ftdi_read_eeprom(struct usb_device *udev, unsigned int off, > + void *val, size_t bytes) > +{ > + unsigned int read = 0; > + > + if (bytes % 2) /* 16-bit word eeprom */ > + bytes--; Error? > + > + while (read < bytes) { > + int rv; > + > + rv = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), > + FTDI_SIO_READ_EEPROM_REQUEST, > + FTDI_SIO_READ_EEPROM_REQUEST_TYPE, > + 0, (off + read) / 2, val + read, 2, > + WDR_TIMEOUT); > + if (rv < 0) > + break; Must check for short transfers. And return an error instead of doing a partial transfer. > + > + read += 2; > + } > + > + return read; > +} > + > +static int ftdi_set_bitmode(struct usb_serial_port *port, u8 bitmask, > + u8 bitmode) > +{ > + struct ftdi_private *priv = usb_get_serial_port_data(port); > + __u16 urb_value = 0; u16, rename "value", no need to initialise. > + > + urb_value = bitmode << 8 | bitmask; > + > + if (usb_control_msg(port->serial->dev, Use intermediate res (or rv) variable for status, here and elsewhere. > + usb_sndctrlpipe(port->serial->dev, 0), > + FTDI_SIO_SET_BITMODE_REQUEST, > + FTDI_SIO_SET_BITMODE_REQUEST_TYPE, > + urb_value, priv->interface, NULL, 0, > + WDR_SHORT_TIMEOUT) < 0) { > + return -EIO; Just return actual error, here and elsewhere. > + } > + > + return 0; > +} > + > +static int ftdi_read_pins(struct usb_serial_port *port, u8 *val) > +{ > + struct ftdi_private *priv = usb_get_serial_port_data(port); > + unsigned char *buf; > + > + buf = kmalloc(1, GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + > + if (usb_control_msg(port->serial->dev, > + usb_rcvctrlpipe(port->serial->dev, 0), > + FTDI_SIO_READ_PINS_REQUEST, > + FTDI_SIO_READ_PINS_REQUEST_TYPE, > + 0, priv->interface, buf, 1, WDR_TIMEOUT) < 0) { Must check for short transfers (yes, zero-length). > + kfree(buf); > + return -EIO; > + } > + > + *val = buf[0]; > + kfree(buf); > + > + return 0; > +} > + > +static int ftdi_cbus_gpio_dir_out(struct gpio_chip *gc, unsigned gpio, int val) > +{ > + struct ftdi_gpiochip *fgc = gpiochip_get_data(gc); > + unsigned int cbus_idx = fgc->cbus_map[gpio]; > + > + /* > + * CBUS mask is an 8-bit register controlling the direction and value > + * of CBUS pins in Bit-Bang mode. cf application note AN_373/AN_232R-01. > + * | bit 7 | bit 6 | bit 5 | bit 4 | bit 3 | bit 2 | bit 1 | bit 0 | > + * | CBUS3 | CBUS2 | CBUS1 | CBUS0 | CBUS3 | CBUS2 | CBUS1 | CBUS0 | > + * | I/O | I/O | I/O | I/O | Hi/Lo | Hi/Lo | Hi/Lo | Hi/Lo | > + */ > + set_bit(cbus_idx + 4, &fgc->cbus_mask); /* direction */ > + set_bit(cbus_idx, &fgc->cbus_mask); /* value */ No need for atomic bit ops. > + > + return ftdi_set_bitmode(fgc->port, fgc->cbus_mask, > + FTDI_SIO_BITMODE_CBUS); > +} > + > +static int ftdi_cbus_gpio_dir_in(struct gpio_chip *gc, unsigned gpio) > +{ > + struct ftdi_gpiochip *fgc = gpiochip_get_data(gc); > + unsigned int cbus_idx = fgc->cbus_map[gpio]; > + > + clear_bit(cbus_idx + 4, &fgc->cbus_mask); > + > + return ftdi_set_bitmode(fgc->port, fgc->cbus_mask, > + FTDI_SIO_BITMODE_CBUS); > +} > + > +static int ftdi_cbus_gpio_get(struct gpio_chip *gc, unsigned gpio) > +{ > + struct ftdi_gpiochip *fgc = gpiochip_get_data(gc); > + unsigned int cbus_idx = fgc->cbus_map[gpio]; > + u8 val = 0; > + int rv; > + > + rv = ftdi_read_pins(fgc->port, &val); > + if (rv) > + return rv; > + > + return !!(val & BIT(cbus_idx)); > +} > + > +static void ftdi_cbus_gpio_set(struct gpio_chip *gc, unsigned gpio, int val) > +{ > + struct ftdi_gpiochip *fgc = gpiochip_get_data(gc); > + > + if (val) > + set_bit(fgc->cbus_map[gpio], &fgc->cbus_mask); > + else > + clear_bit(fgc->cbus_map[gpio], &fgc->cbus_mask); > + > + ftdi_set_bitmode(fgc->port, fgc->cbus_mask, FTDI_SIO_BITMODE_CBUS); Logging an error seems appropriate here. > +} > + > +static int ftdi_register_cbus_gpiochip(struct usb_serial_port *port) > +{ > + struct ftdi_private *priv = usb_get_serial_port_data(port); > + struct usb_device *udev = port->serial->dev; > + struct ftdi_gpiochip *fgc; > + int rv = 0; > + > + fgc = kzalloc(sizeof(*fgc), GFP_KERNEL); > + if (!fgc) > + return -ENOMEM; > + > + if (priv->chip_type == FTX) { > + unsigned char *cbus_mux; > + unsigned int i; > + > + /* > + * Each individual CBUS pin is controlled by a separate 8-bit > + * MUX control value stored in the EEPROM, starting at 0x1a: > + * | 0x1a CBUS0 | 0x1b CBUS1 | 0x1c CBUS2 | 0x1d CBUS3 | ... > + * cf application note AN_201. > + */ > + > + cbus_mux = kmalloc(FTDI_CBUS_PIN_COUNT, GFP_KERNEL); > + if (!cbus_mux) > + return -ENOMEM; You're leaking fgc here. > + > + rv = ftdi_read_eeprom(udev, FTX_CBUS_MUX_EEPROM_ADDR, cbus_mux, > + FTDI_CBUS_PIN_COUNT); > + if (rv < 0) { And here you're not checking for short transfers, despite currently implementing them (which you probably should not). > + dev_err(&udev->dev, "Unable to read CBUS config\n"); > + kfree(cbus_mux); > + goto exit_release; > + } > + > + for (i = 0; i < rv; i++) { > + if (cbus_mux[i] == FTX_CBUS_MUX_IO) > + fgc->cbus_map[fgc->gc.ngpio++] = i; > + } We don't want the gpio chip offsets to change depending on how the pins have been muxed in eeprom (i.e. CBUS4 should always be at offset 3). So the cbus_map isn't needed, and you need an request callback to deny requests of pins that are not available. > + > + rv = 0; > + kfree(cbus_mux); > + } > + > + if (!fgc->gc.ngpio) > + goto exit_release; > + > + fgc->gc.label = "ftdi-cbus-gpio"; "ftdi-cbus" should do. > + fgc->gc.direction_input = ftdi_cbus_gpio_dir_in; > + fgc->gc.direction_output = ftdi_cbus_gpio_dir_out; > + fgc->gc.set = ftdi_cbus_gpio_set; > + fgc->gc.get = ftdi_cbus_gpio_get; By not implementing get_direction, and considering the initial pin state, we could end up with a driven pin being reported as an input. Specifically, you're relying on gpiolib considering pins as inputs by default, but the pin direction and state (for all pins) will not get updated until the first direction or state change. > + fgc->gc.can_sleep = true; > + fgc->gc.parent = &udev->dev; The gpio chip should be a child of the USB interface which we are binding to and not its parent USB device (also consider multiport devices). > + fgc->gc.base = -1; > + fgc->gc.owner = THIS_MODULE; > + fgc->port = port; > + > + rv = gpiochip_add_data(&fgc->gc, fgc); > + if (rv) { > + dev_err(&udev->dev, "Unable to add gpiochip\n"); Including an errno is usually a good idea. > + goto exit_release; > + } > + > + priv->fgc = fgc; > + > + dev_info(&udev->dev, > + "FTDI USB GPIO controller Registered, base=%d, ngpio=%u\n", > + fgc->gc.base, fgc->gc.ngpio); Demote to KERN_DEBUG or remove. > + > + return 0; > + > +exit_release: > + kfree(fgc); > + return rv; > +} > + > +static void ftdi_unregister_cbus_gpiochip(struct usb_serial_port *port) > +{ > + struct ftdi_private *priv = usb_get_serial_port_data(port); > + struct ftdi_gpiochip *fgc = priv->fgc; > + > + if (!fgc) > + return; > + > + gpiochip_remove(&fgc->gc); > + kfree(fgc); > +} > + > +#endif /* CONFIG_USB_SERIAL_FTDI_SIO_CBUS_GPIO */ > > /* Determine type of FTDI chip based on USB config and descriptor. */ > static void ftdi_determine_type(struct usb_serial_port *port) > @@ -1813,6 +2043,10 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port) > priv->latency = 16; > write_latency_timer(port); > create_sysfs_attrs(port); > +#ifdef CONFIG_USB_SERIAL_FTDI_SIO_CBUS_GPIO > + ftdi_register_cbus_gpiochip(port); > +#endif No ifdefs in code like this; add dummy implementation instead. And what about errors? Should be logged at least. > + > return 0; > } > > @@ -1930,6 +2164,10 @@ static int ftdi_sio_port_remove(struct usb_serial_port *port) > { > struct ftdi_private *priv = usb_get_serial_port_data(port); > > +#ifdef CONFIG_USB_SERIAL_FTDI_SIO_CBUS_GPIO > + ftdi_unregister_cbus_gpiochip(port); > +#endif > + > remove_sysfs_attrs(port); > > kfree(priv); > diff --git a/drivers/usb/serial/ftdi_sio.h b/drivers/usb/serial/ftdi_sio.h > index dcd0b6e..3bd248f 100644 > --- a/drivers/usb/serial/ftdi_sio.h > +++ b/drivers/usb/serial/ftdi_sio.h > @@ -36,6 +36,10 @@ > #define FTDI_SIO_SET_ERROR_CHAR 7 /* Set the error character */ > #define FTDI_SIO_SET_LATENCY_TIMER 9 /* Set the latency timer */ > #define FTDI_SIO_GET_LATENCY_TIMER 10 /* Get the latency timer */ > +#define FTDI_SIO_SET_BITMODE 11 /* Set the bitmode */ > +#define FTDI_SIO_READ_PINS 12 /* Read pins in bitmode */ > +#define FTDI_SIO_READ_EEPROM 0x90 /* Read eeprom */ > +#define FTDI_SIO_WRITE_EEPROM 0x91 /* Write eeprom */ > > /* Interface indices for FT2232, FT2232H and FT4232H devices */ > #define INTERFACE_A 1 > @@ -400,6 +404,60 @@ enum ftdi_sio_baudrate { > * > */ > > + /* FTDI_SIO_READ_EEPROM */ > +#define FTDI_SIO_READ_EEPROM_REQUEST_TYPE 0xc0 > +#define FTDI_SIO_READ_EEPROM_REQUEST FTDI_SIO_READ_EEPROM > +/* > + * BmRequestType: 1100 0000b > + * bRequest: FTDI_SIO_READ_EEPROM > + * wValue: 0 > + * wIndex: Word Index > + * wLength: 2 > + * Data: return data (a word) > + * Remove empty comment line, here and elsewhere. > + */ > + > +/* FTDI_SIO_WRITE_EEPROM */ > +#define FTDI_SIO_WRITE_EEPROM_REQUEST_TYPE 0x40 > +#define FTDI_SIO_WRITE_EEPROM_REQUEST FTDI_SIO_WRITE_EEPROM > +/* > + * BmRequestType: 0100 0000b > + * bRequest: FTDI_SIO_WRITE_EEPROM > + * wValue: Data (word) > + * wIndex: Word Index > + * wLength: 0 > + * Data: None > + * > + */ > + > + /* FTDI_SIO_SET_BITMODE */ > +#define FTDI_SIO_SET_BITMODE_REQUEST_TYPE 0x40 > +#define FTDI_SIO_SET_BITMODE_REQUEST FTDI_SIO_SET_BITMODE > +#define FTDI_SIO_BITMODE_RESET 0x00 > +#define FTDI_SIO_BITMODE_CBUS 0x20 > +/* > + * BmRequestType: 0100 0000b > + * bRequest: FTDI_SIO_SET_BITMODE > + * wValue: [0-7] bitmask, [8-15] bitmode > + * wIndex: Port > + * wLength: 0 > + * Data: None > + * > + */ > + > +/* FTDI_SIO_READ_PINS */ > +#define FTDI_SIO_READ_PINS_REQUEST_TYPE 0xC0 > +#define FTDI_SIO_READ_PINS_REQUEST FTDI_SIO_READ_PINS > +/* > + * BmRequestType: 1100 0000b > + * bRequest: FTDI_SIO_READ_PINS > + * wValue: 0 > + * wIndex: Port > + * wLength: 2 > + * Data: return data (a word) > + * > + */ > + > /* FTDI_SIO_GET_MODEM_STATUS */ > /* Retrieve the current value of the modem status register */ > > @@ -563,3 +621,28 @@ enum ftdi_sio_baudrate { > * B2..7 Length of message - (not including Byte 0) > * > */ > + > +enum ftdi_ftx_cbus_mux { > + FTX_CBUS_MUX_TRISTATE, > + FTX_CBUS_MUX_RXLED, > + FTX_CBUS_MUX_TXLED, > + FTX_CBUS_MUX_TXRXLED, > + FTX_CBUS_MUX_PWREN, > + FTX_CBUS_MUX_SLEEP, > + FTX_CBUS_MUX_DRIVE0, > + FTX_CBUS_MUX_DRIVE1, > + FTX_CBUS_MUX_IO, > + FTX_CBUS_MUX_TXDEN, > + FTX_CBUS_MUX_CLK24, > + FTX_CBUS_MUX_CLK12, > + FTX_CBUS_MUX_CLK6, > + FTX_CBUS_MUX_BCD_CHARGER, > + FTX_CBUS_MUX_BCD_CHARGER_N, > + FTX_CBUS_MUX_I2C_TXE, > + FTX_CBUS_MUX_I2C_RXF, > + FTX_CBUS_MUX_VBUS_SENSE, > + FTX_CBUS_MUX_BITBANG_WR, > + FTX_CBUS_MUX_BITBANG_RD, > + FTX_CBUS_MUX_TIMESTAMP, > + FTX_CBUS_MUX_KEEP_AWAKE > +}; Since the values are not arbitrary, they need to be initialised. Might be better to just use defines. And we will probably only ever need FTX_CBUS_MUX_IO, so just adding that one should do for now.
