| Message ID | 20180825204744.2307-1-pados@pados.hu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | USB: serial: ftdi_sio: implement GPIO support for FT230X | expand |
Hi Karoly,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on usb-serial/usb-next]
[also build test ERROR on v4.19-rc1 next-20180827]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Karoly-Pados/USB-serial-ftdi_sio-implement-GPIO-support-for-FT230X/20180827-005902
base: https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git usb-next
config: mips-rm200_defconfig (attached as .config)
compiler: mipsel-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=mips
All errors (new ones prefixed by >>):
drivers/usb/serial/ftdi_sio.c: In function 'ftdi_sio_port_probe':
>> drivers/usb/serial/ftdi_sio.c:2090:30: error: passing argument 1 of 'ftdi_sio_gpio_init' from incompatible pointer type [-Werror=incompatible-pointer-types]
result = ftdi_sio_gpio_init(port);
^~~~
drivers/usb/serial/ftdi_sio.c:2030:12: note: expected 'struct usb_serial *' but argument is of type 'struct usb_serial_port *'
static int ftdi_sio_gpio_init(struct usb_serial *serial)
^~~~~~~~~~~~~~~~~~
drivers/usb/serial/ftdi_sio.c: In function 'ftdi_sio_port_remove':
>> drivers/usb/serial/ftdi_sio.c:2213:23: error: passing argument 1 of 'ftdi_sio_gpio_remove' from incompatible pointer type [-Werror=incompatible-pointer-types]
ftdi_sio_gpio_remove(port);
^~~~
drivers/usb/serial/ftdi_sio.c:2035:13: note: expected 'struct usb_serial *' but argument is of type 'struct usb_serial_port *'
static void ftdi_sio_gpio_remove(struct usb_serial *serial)
^~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/ftdi_sio_gpio_init +2090 drivers/usb/serial/ftdi_sio.c
2065
2066 static int ftdi_sio_port_probe(struct usb_serial_port *port)
2067 {
2068 struct ftdi_private *priv;
2069 const struct ftdi_sio_quirk *quirk = usb_get_serial_data(port->serial);
2070 int result;
2071
2072 priv = kzalloc(sizeof(struct ftdi_private), GFP_KERNEL);
2073 if (!priv)
2074 return -ENOMEM;
2075
2076 mutex_init(&priv->cfg_lock);
2077
2078 if (quirk && quirk->port_probe)
2079 quirk->port_probe(priv);
2080
2081 usb_set_serial_port_data(port, priv);
2082
2083 ftdi_determine_type(port);
2084 ftdi_set_max_packet_size(port);
2085 if (read_latency_timer(port) < 0)
2086 priv->latency = 16;
2087 write_latency_timer(port);
2088 create_sysfs_attrs(port);
2089
> 2090 result = ftdi_sio_gpio_init(port);
2091 if (result < 0)
2092 dev_err(&port->serial->interface->dev,
2093 "GPIO initialisation failed: %d\n",
2094 result);
2095
2096 return 0;
2097 }
2098
2099 /* Setup for the USB-UIRT device, which requires hardwired
2100 * baudrate (38400 gets mapped to 312500) */
2101 /* Called from usbserial:serial_probe */
2102 static void ftdi_USB_UIRT_setup(struct ftdi_private *priv)
2103 {
2104 priv->flags |= ASYNC_SPD_CUST;
2105 priv->custom_divisor = 77;
2106 priv->force_baud = 38400;
2107 }
2108
2109 /* Setup for the HE-TIRA1 device, which requires hardwired
2110 * baudrate (38400 gets mapped to 100000) and RTS-CTS enabled. */
2111
2112 static void ftdi_HE_TIRA1_setup(struct ftdi_private *priv)
2113 {
2114 priv->flags |= ASYNC_SPD_CUST;
2115 priv->custom_divisor = 240;
2116 priv->force_baud = 38400;
2117 priv->force_rtscts = 1;
2118 }
2119
2120 /*
2121 * Module parameter to control latency timer for NDI FTDI-based USB devices.
2122 * If this value is not set in /etc/modprobe.d/ its value will be set
2123 * to 1ms.
2124 */
2125 static int ndi_latency_timer = 1;
2126
2127 /* Setup for the NDI FTDI-based USB devices, which requires hardwired
2128 * baudrate (19200 gets mapped to 1200000).
2129 *
2130 * Called from usbserial:serial_probe.
2131 */
2132 static int ftdi_NDI_device_setup(struct usb_serial *serial)
2133 {
2134 struct usb_device *udev = serial->dev;
2135 int latency = ndi_latency_timer;
2136
2137 if (latency == 0)
2138 latency = 1;
2139 if (latency > 99)
2140 latency = 99;
2141
2142 dev_dbg(&udev->dev, "%s setting NDI device latency to %d\n", __func__, latency);
2143 dev_info(&udev->dev, "NDI device with a latency value of %d\n", latency);
2144
2145 /* FIXME: errors are not returned */
2146 usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
2147 FTDI_SIO_SET_LATENCY_TIMER_REQUEST,
2148 FTDI_SIO_SET_LATENCY_TIMER_REQUEST_TYPE,
2149 latency, 0, NULL, 0, WDR_TIMEOUT);
2150 return 0;
2151 }
2152
2153 /*
2154 * First port on JTAG adaptors such as Olimex arm-usb-ocd or the FIC/OpenMoko
2155 * Neo1973 Debug Board is reserved for JTAG interface and can be accessed from
2156 * userspace using openocd.
2157 */
2158 static int ftdi_jtag_probe(struct usb_serial *serial)
2159 {
2160 struct usb_device *udev = serial->dev;
2161 struct usb_interface *interface = serial->interface;
2162
2163 if (interface == udev->actconfig->interface[0]) {
2164 dev_info(&udev->dev,
2165 "Ignoring serial port reserved for JTAG\n");
2166 return -ENODEV;
2167 }
2168
2169 return 0;
2170 }
2171
2172 static int ftdi_8u2232c_probe(struct usb_serial *serial)
2173 {
2174 struct usb_device *udev = serial->dev;
2175
2176 if (udev->manufacturer && !strcmp(udev->manufacturer, "CALAO Systems"))
2177 return ftdi_jtag_probe(serial);
2178
2179 if (udev->product &&
2180 (!strcmp(udev->product, "Arrow USB Blaster") ||
2181 !strcmp(udev->product, "BeagleBone/XDS100V2") ||
2182 !strcmp(udev->product, "SNAP Connect E10")))
2183 return ftdi_jtag_probe(serial);
2184
2185 return 0;
2186 }
2187
2188 /*
2189 * First two ports on JTAG adaptors using an FT4232 such as STMicroelectronics's
2190 * ST Micro Connect Lite are reserved for JTAG or other non-UART interfaces and
2191 * can be accessed from userspace.
2192 * The next two ports are enabled as UARTs by default, where port 2 is
2193 * a conventional RS-232 UART.
2194 */
2195 static int ftdi_stmclite_probe(struct usb_serial *serial)
2196 {
2197 struct usb_device *udev = serial->dev;
2198 struct usb_interface *interface = serial->interface;
2199
2200 if (interface == udev->actconfig->interface[0] ||
2201 interface == udev->actconfig->interface[1]) {
2202 dev_info(&udev->dev, "Ignoring serial port reserved for JTAG\n");
2203 return -ENODEV;
2204 }
2205
2206 return 0;
2207 }
2208
2209 static int ftdi_sio_port_remove(struct usb_serial_port *port)
2210 {
2211 struct ftdi_private *priv = usb_get_serial_port_data(port);
2212
> 2213 ftdi_sio_gpio_remove(port);
2214
2215 remove_sysfs_attrs(port);
2216
2217 kfree(priv);
2218
2219 return 0;
2220 }
2221
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Sat, Aug 25, 2018 at 10:47:44PM +0200, Karoly Pados wrote: > This patch uses the CBUS bitbang mode of the device, so there is > no conflict between the GPIO and VCP functionality. Please make the commit message self-contained (e.g. don't rely on the summary, or mail Subject, even if you feel like you're repeating yourself.) And you can be a bit more verbose when describing your patch here. > Signed-off-by: Karoly Pados <pados@pados.hu> > --- > > Tested on an FT230X device. Though there is no copied code from other > sources, libftdi was used as a reference. > > I noticed there have been numerous historic attempts by other people at > adding GPIO support to ftdi_sio. One tried adding it in some very > application-specific way, another one tried to use the mfd framework, the > tried using a GPIO mode which is mutually exclusive to VCP, another > one did not implement the review results etc... I hope I learned from those > attempts and am planning to respond to reviews. Also, this patch follows > the rough structure of the GPIO support for cp210x devices that have > already been accepted, in the hope that it is a good starting point. > > This patch uses CBUS bitbanging mode, which works nicely in parallel with > the VCP function. The other modes do not, and so IMHO it does not make > sense to try adding them to this same module. > > For this device, whenever changing the state of any single pin, all the > others need to be written too. This means in order to change any pin, we > need to know the current state of all the others. So when using GPIO, > we need to have a known starting state for all pins, but there seems to > be no way to retrieve the existing GPIO configuration (directions and > output register values). The way I handle this in this patch is that when > requesting a GPIO for the first time, the module initializes all pins to > a known default state (to inputs). Input was chosen, because a potentially > floating pin is better than a potential driver conflict, because the latter > could result in hardware damage. However, if the user does not request a > GPIO, the CBUS pins are not initialized, avoiding unnecessarily changing > hardware state. I figured I cannot rely on the default power-on state of > the device for this as the user might have used libftdi before loading > our module. When the module is unloaded, CBUS bitbanging mode is exited > if it were us who entered it earlier. This paragraph seems like it belongs in the commit message in some form. But your approach seems reasonable, even if it means all pins may change state when requesting the first. 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. > checkpatch.pl emits a bunch of warnings, recommending const for structs. > I am unsure if this makes sense, existing code in this same and also > other modules does not adhere to this rule either. I fixed everything else. Really? Checkpatch doesn't complain here. Were you using the --strict option, perhaps? Don't do that... > drivers/usb/serial/ftdi_sio.c | 284 +++++++++++++++++++++++++++++++++- > drivers/usb/serial/ftdi_sio.h | 25 +++ > 2 files changed, 308 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c > index b5cef322826f..d1ff4a372b6e 100644 > --- a/drivers/usb/serial/ftdi_sio.c > +++ b/drivers/usb/serial/ftdi_sio.c > @@ -40,6 +40,9 @@ > #include <linux/usb.h> > #include <linux/serial.h> > #include <linux/usb/serial.h> > +#ifdef CONFIG_GPIOLIB > +#include <linux/gpio/driver.h> > +#endif No ifdef. > #include "ftdi_sio.h" > #include "ftdi_sio_ids.h" > > @@ -72,6 +75,14 @@ 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() */ > +#ifdef CONFIG_GPIOLIB > + struct gpio_chip gc; > + bool gpio_registered; /* is the gpiochip in kernel registered */ > + bool gpio_used; /* true if the user requested a gpio */ > + u8 gpio_altfunc; /* which pins are in gpio mode */ > + u8 gpio_input; /* pin directions cache */ > + u8 gpio_value; /* pin value for outputs */ > +#endif > }; > > /* struct ftdi_sio_quirk is used by devices requiring special attention. */ > @@ -1766,6 +1777,268 @@ static void remove_sysfs_attrs(struct usb_serial_port *port) > > } > > +#ifdef CONFIG_GPIOLIB > + > +static int ftdi_sio_set_cbus(struct usb_serial_port *port, Please drop the "_sio" infix from your functions, "ftdi_" as prefix is enough. > + u8 direction, > + u8 value, > + u8 mode) > +{ > + struct ftdi_private *priv = usb_get_serial_port_data(port); Since you have port and priv here anyway, you could consider not passing direction and value in every call. Even mode could go, if you add a separate helper for that, which would also make this function more appropriately named. > + u16 val; > + > + /* 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. > + direction = (~direction) & 0x0f; > + > + val = (mode << 8) | (direction << 4) | (value & 0x0f); > + return usb_control_msg(port->serial->dev, > + usb_sndctrlpipe(port->serial->dev, 0), > + FTDI_SIO_SET_BITMODE_REQUEST, > + FTDI_SIO_SET_BITMODE_REQUEST_TYPE, val, > + priv->interface, NULL, 0, WDR_TIMEOUT); I think you should log any errors here (even if that means two error messages for failed set()). > +} > + > +static int ftdi_sio_gpio_request(struct gpio_chip *gc, unsigned int offset) > +{ > + struct usb_serial_port *port = gpiochip_get_data(gc); > + struct ftdi_private *priv = usb_get_serial_port_data(port); > + int result = 0; No need to initialise. > + > + if (priv->gpio_altfunc & BIT(offset)) > + return -ENODEV; > + > + if (!priv->gpio_used) { > + /* Set default pin states, as we cannot get them from device */ > + priv->gpio_input = 0xff; > + priv->gpio_value = 0x00; > + result = ftdi_sio_set_cbus(port, > + priv->gpio_input, > + priv->gpio_value, > + FTDI_SIO_GPIO_MODE_CBUS); > + if (result) > + return result; > + > + priv->gpio_used = true; > + } So I think this initialisation should be done at probe instead. > + > + return 0; > +} > + > +static int ftdi_sio_gpio_get(struct gpio_chip *gc, unsigned int gpio) > +{ > + struct usb_serial_port *port = gpiochip_get_data(gc); > + struct ftdi_private *priv = usb_get_serial_port_data(port); > + struct usb_serial *serial = port->serial; > + unsigned char *rcvbuf; > + int result; > + > + rcvbuf = kmalloc(1, GFP_KERNEL); > + if (!rcvbuf) > + return -ENOMEM; > + > + result = usb_control_msg(serial->dev, > + usb_rcvctrlpipe(serial->dev, 0), > + FTDI_SIO_READ_PINS_REQUEST, > + FTDI_SIO_READ_PINS_REQUEST_TYPE, 0, > + priv->interface, rcvbuf, 1, WDR_TIMEOUT); > + No newline. > + if (result < 1) > + result = -EIO; > + else > + result = !!(rcvbuf[0] & BIT(gpio)); > + > + kfree(rcvbuf); > + > + return result; > +} > + > +static void ftdi_sio_gpio_set(struct gpio_chip *gc, > + unsigned int gpio, > + int value > +) Closing parentheses goes after value. > +{ > + struct usb_serial_port *port = gpiochip_get_data(gc); > + struct ftdi_private *priv = usb_get_serial_port_data(port); > + int result; > + > + if (value) > + priv->gpio_value |= BIT(gpio); > + else > + priv->gpio_value &= ~BIT(gpio); > + > + result = ftdi_sio_set_cbus(port, > + priv->gpio_input, > + priv->gpio_value, > + FTDI_SIO_GPIO_MODE_CBUS); > + No newline. > + if (result < 0) { > + dev_err(&port->serial->interface->dev, > + "failed to set GPIO value: %d\n", > + result); > + } > +} > + > +static int ftdi_sio_gpio_direction_get(struct gpio_chip *gc, > + unsigned int gpio) > +{ > + struct usb_serial_port *port = gpiochip_get_data(gc); > + struct ftdi_private *priv = usb_get_serial_port_data(port); > + > + return priv->gpio_input & BIT(gpio); > +} > + > +static int ftdi_sio_gpio_direction_input(struct gpio_chip *gc, > + unsigned int gpio) > +{ > + struct usb_serial_port *port = gpiochip_get_data(gc); > + struct ftdi_private *priv = usb_get_serial_port_data(port); > + > + priv->gpio_input |= BIT(gpio); > + > + return ftdi_sio_set_cbus(port, > + priv->gpio_input, > + priv->gpio_value, > + FTDI_SIO_GPIO_MODE_CBUS); > +} > + > +static int ftdi_sio_gpio_direction_output(struct gpio_chip *gc, > + unsigned int gpio, > + int value) > +{ > + struct usb_serial_port *port = gpiochip_get_data(gc); > + struct ftdi_private *priv = usb_get_serial_port_data(port); > + > + priv->gpio_input &= ~BIT(gpio); > + if (value) > + priv->gpio_value |= BIT(gpio); > + else > + priv->gpio_value &= ~BIT(gpio); > + > + return ftdi_sio_set_cbus(port, > + priv->gpio_input, > + priv->gpio_value, > + FTDI_SIO_GPIO_MODE_CBUS); > +} > + > +static int ftx_gpioconf_init(struct usb_serial_port *port) > +{ > + 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; > + > + /* Read part of device EEPROM */ > + config_buf = kmalloc(config_size, GFP_KERNEL); > + if (!config_buf) > + return -ENOMEM; > + > + for (i = 0; i < config_size / 2; i++) { > + result = usb_control_msg(serial->dev, > + usb_rcvctrlpipe(serial->dev, 0), > + FTDI_SIO_READ_EEPROM_REQUEST, > + FTDI_SIO_READ_EEPROM_REQUEST_TYPE, 0, > + i, config_buf + (i * 2), 2 * i, parenthesis not needed > + 2, WDR_TIMEOUT); > + if (result < 2) { > + result = -EIO; Return -EIO for short transfers, but just return result otherwise. > + break; > + } > + } > + > + if (result < 0) { > + kfree(config_buf); > + return result; > + } Factor this out to an eeprom helper that can be reused by other chip types. > + > + /* > + * 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; > +} > + > +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; > +} > + > +static void ftdi_sio_gpio_remove(struct usb_serial_port *port) > +{ > + struct ftdi_private *priv = usb_get_serial_port_data(port); > + > + if (priv->gpio_used) { > + ftdi_sio_set_cbus(port, 0, 0, FTDI_SIO_GPIO_MODE_RESET); IIRC this leaves the pins in the state that we set them, which is fine, but could be highlighted here. > + priv->gpio_used = false; > + } > + > + if (priv->gpio_registered) { > + gpiochip_remove(&priv->gc); > + priv->gpio_registered = false; > + } > +} > + > +#else > + > +static int ftdi_sio_gpio_init(struct usb_serial *serial) As the test robot reported, these should take a port as their argument. > +{ > + return 0; > +} > + > +static void ftdi_sio_gpio_remove(struct usb_serial *serial) > +{ > + /* Nothing to do */ Comment not really needed; you can even put the brackets after the closing parentheses ...* serial) { } as we sometimes do in headers, if you want. > +} > + > +#endif > + > /* > * *************************************************************************** > * FTDI driver specific functions > @@ -1794,7 +2067,7 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port) > { > struct ftdi_private *priv; > const struct ftdi_sio_quirk *quirk = usb_get_serial_data(port->serial); > - > + int result; > > priv = kzalloc(sizeof(struct ftdi_private), GFP_KERNEL); > if (!priv) > @@ -1813,6 +2086,13 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port) > priv->latency = 16; > write_latency_timer(port); > create_sysfs_attrs(port); > + > + result = ftdi_sio_gpio_init(port); > + if (result < 0) > + dev_err(&port->serial->interface->dev, > + "GPIO initialisation failed: %d\n", > + result); Please use brackets for conditionals with multi-line statements. > + > return 0; > } > > @@ -1930,6 +2210,8 @@ static int ftdi_sio_port_remove(struct usb_serial_port *port) > { > struct ftdi_private *priv = usb_get_serial_port_data(port); > > + ftdi_sio_gpio_remove(port); > + > remove_sysfs_attrs(port); > > kfree(priv); > diff --git a/drivers/usb/serial/ftdi_sio.h b/drivers/usb/serial/ftdi_sio.h > index dcd0b6e05baf..5046d02fe3ac 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 CBUS GPIO pin mode */ > +#define FTDI_SIO_READ_PINS 12 /* Read immediate value of pins */ > +#define FTDI_SIO_READ_EEPROM 144 /* Read EEPROM */ Please use hex notation. > + > > /* Interface indices for FT2232, FT2232H and FT4232H devices */ > #define INTERFACE_A 1 > @@ -433,6 +437,27 @@ enum ftdi_sio_baudrate { > * 1 = active > */ > > +/* FTDI_SIO_SET_BITMODE */ > +#define FTDI_SIO_SET_BITMODE_REQUEST_TYPE 0x40 > +#define FTDI_SIO_SET_BITMODE_REQUEST FTDI_SIO_SET_BITMODE This looks awfully redundant, but I realise you're just sticking to the current pattern (as you should). > + > +/* FTDI_SIO_READ_PINS */ > +#define FTDI_SIO_READ_PINS_REQUEST_TYPE 0xc0 > +#define FTDI_SIO_READ_PINS_REQUEST FTDI_SIO_READ_PINS > + > +/* > + * FTDI_SIO_READ_EEPROM > + * > + * EEPROM format found in FTDI AN_201, "FT-X MTP memory Configuration", > + * http://www.ftdichip.com/Support/Documents/AppNotes/AN_201_FT-X%20MTP%20Memory%20Configuration.pdf > + */ > +#define FTDI_SIO_READ_EEPROM_REQUEST_TYPE 0xc0 > +#define FTDI_SIO_READ_EEPROM_REQUEST FTDI_SIO_READ_EEPROM > + > +#define FTDI_SIO_GPIO_MODE_CBUS 0x20 > +#define FTDI_SIO_GPIO_MODE_RESET 0x00 Sort the mode defines by value here. And these belong with FTDI_SET_BITMODE above and should be renamed to reflect that. > + > +#define FTDI_SIO_CBUS_MUX_GPIO 8 > > > /* Descriptors returned by the device Thanks, Johan
Thanks for the review, I am on the road most of this week, but I will submit a new patch this weekend with all your feedback incorporated. Until then here are my inputs / answers to some of your points. > 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. >> checkpatch.pl emits a bunch of warnings, recommending const for structs. >> I am unsure if this makes sense, existing code in this same and also >> other modules does not adhere to this rule either. I fixed everything else. > > Really? Checkpatch doesn't complain here. Were you using the --strict > option, perhaps? Don't do that... > Yeah, that's what happened. I'll drop that flag. >> + /* 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. 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. >> + if (!priv->gpio_used) { >> + /* Set default pin states, as we cannot get them from device */ >> + priv->gpio_input = 0xff; >> + priv->gpio_value = 0x00; >> + result = ftdi_sio_set_cbus(port, >> + priv->gpio_input, >> + priv->gpio_value, >> + FTDI_SIO_GPIO_MODE_CBUS); >> + if (result) >> + return result; >> + >> + priv->gpio_used = true; >> + } > > So I think this initialisation should be done at probe instead. See my point further above. > 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. >> + 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? >> +#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. Thanks again, Karoly
>>> + 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? > Aaah, but those other devices are not UART bridges. I see what you mean now. The point for me when developing th original patch
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
On Tue, Sep 04, 2018 at 06:07:49PM +0000, Karoly Pados wrote: > >>> + 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? > > > > Aaah, but those other devices are not UART bridges. I see what you mean now. > The point for me when developing th original patch The last sentence looks incomplete; did you hit send too soon or too late? ;) Johan
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
diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c index b5cef322826f..d1ff4a372b6e 100644 --- a/drivers/usb/serial/ftdi_sio.c +++ b/drivers/usb/serial/ftdi_sio.c @@ -40,6 +40,9 @@ #include <linux/usb.h> #include <linux/serial.h> #include <linux/usb/serial.h> +#ifdef CONFIG_GPIOLIB +#include <linux/gpio/driver.h> +#endif #include "ftdi_sio.h" #include "ftdi_sio_ids.h" @@ -72,6 +75,14 @@ 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() */ +#ifdef CONFIG_GPIOLIB + struct gpio_chip gc; + bool gpio_registered; /* is the gpiochip in kernel registered */ + bool gpio_used; /* true if the user requested a gpio */ + u8 gpio_altfunc; /* which pins are in gpio mode */ + u8 gpio_input; /* pin directions cache */ + u8 gpio_value; /* pin value for outputs */ +#endif }; /* struct ftdi_sio_quirk is used by devices requiring special attention. */ @@ -1766,6 +1777,268 @@ static void remove_sysfs_attrs(struct usb_serial_port *port) } +#ifdef CONFIG_GPIOLIB + +static int ftdi_sio_set_cbus(struct usb_serial_port *port, + u8 direction, + u8 value, + u8 mode) +{ + struct ftdi_private *priv = usb_get_serial_port_data(port); + u16 val; + + /* device's direction polarity is different from kernel's */ + direction = (~direction) & 0x0f; + + val = (mode << 8) | (direction << 4) | (value & 0x0f); + return usb_control_msg(port->serial->dev, + usb_sndctrlpipe(port->serial->dev, 0), + FTDI_SIO_SET_BITMODE_REQUEST, + FTDI_SIO_SET_BITMODE_REQUEST_TYPE, val, + priv->interface, NULL, 0, WDR_TIMEOUT); +} + +static int ftdi_sio_gpio_request(struct gpio_chip *gc, unsigned int offset) +{ + struct usb_serial_port *port = gpiochip_get_data(gc); + struct ftdi_private *priv = usb_get_serial_port_data(port); + int result = 0; + + if (priv->gpio_altfunc & BIT(offset)) + return -ENODEV; + + if (!priv->gpio_used) { + /* Set default pin states, as we cannot get them from device */ + priv->gpio_input = 0xff; + priv->gpio_value = 0x00; + result = ftdi_sio_set_cbus(port, + priv->gpio_input, + priv->gpio_value, + FTDI_SIO_GPIO_MODE_CBUS); + if (result) + return result; + + priv->gpio_used = true; + } + + return 0; +} + +static int ftdi_sio_gpio_get(struct gpio_chip *gc, unsigned int gpio) +{ + struct usb_serial_port *port = gpiochip_get_data(gc); + struct ftdi_private *priv = usb_get_serial_port_data(port); + struct usb_serial *serial = port->serial; + unsigned char *rcvbuf; + int result; + + rcvbuf = kmalloc(1, GFP_KERNEL); + if (!rcvbuf) + return -ENOMEM; + + result = usb_control_msg(serial->dev, + usb_rcvctrlpipe(serial->dev, 0), + FTDI_SIO_READ_PINS_REQUEST, + FTDI_SIO_READ_PINS_REQUEST_TYPE, 0, + priv->interface, rcvbuf, 1, WDR_TIMEOUT); + + if (result < 1) + result = -EIO; + else + result = !!(rcvbuf[0] & BIT(gpio)); + + kfree(rcvbuf); + + return result; +} + +static void ftdi_sio_gpio_set(struct gpio_chip *gc, + unsigned int gpio, + int value +) +{ + struct usb_serial_port *port = gpiochip_get_data(gc); + struct ftdi_private *priv = usb_get_serial_port_data(port); + int result; + + if (value) + priv->gpio_value |= BIT(gpio); + else + priv->gpio_value &= ~BIT(gpio); + + result = ftdi_sio_set_cbus(port, + priv->gpio_input, + priv->gpio_value, + FTDI_SIO_GPIO_MODE_CBUS); + + if (result < 0) { + dev_err(&port->serial->interface->dev, + "failed to set GPIO value: %d\n", + result); + } +} + +static int ftdi_sio_gpio_direction_get(struct gpio_chip *gc, + unsigned int gpio) +{ + struct usb_serial_port *port = gpiochip_get_data(gc); + struct ftdi_private *priv = usb_get_serial_port_data(port); + + return priv->gpio_input & BIT(gpio); +} + +static int ftdi_sio_gpio_direction_input(struct gpio_chip *gc, + unsigned int gpio) +{ + struct usb_serial_port *port = gpiochip_get_data(gc); + struct ftdi_private *priv = usb_get_serial_port_data(port); + + priv->gpio_input |= BIT(gpio); + + return ftdi_sio_set_cbus(port, + priv->gpio_input, + priv->gpio_value, + FTDI_SIO_GPIO_MODE_CBUS); +} + +static int ftdi_sio_gpio_direction_output(struct gpio_chip *gc, + unsigned int gpio, + int value) +{ + struct usb_serial_port *port = gpiochip_get_data(gc); + struct ftdi_private *priv = usb_get_serial_port_data(port); + + priv->gpio_input &= ~BIT(gpio); + if (value) + priv->gpio_value |= BIT(gpio); + else + priv->gpio_value &= ~BIT(gpio); + + return ftdi_sio_set_cbus(port, + priv->gpio_input, + priv->gpio_value, + FTDI_SIO_GPIO_MODE_CBUS); +} + +static int ftx_gpioconf_init(struct usb_serial_port *port) +{ + 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; + + /* Read part of device EEPROM */ + config_buf = kmalloc(config_size, GFP_KERNEL); + if (!config_buf) + return -ENOMEM; + + for (i = 0; i < config_size / 2; i++) { + result = usb_control_msg(serial->dev, + usb_rcvctrlpipe(serial->dev, 0), + FTDI_SIO_READ_EEPROM_REQUEST, + FTDI_SIO_READ_EEPROM_REQUEST_TYPE, 0, + i, config_buf + (i * 2), + 2, WDR_TIMEOUT); + if (result < 2) { + result = -EIO; + break; + } + } + + if (result < 0) { + kfree(config_buf); + return result; + } + + /* + * 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 */ + priv->gc.ngpio = 4; + + /* 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) + priv->gpio_altfunc &= ~BIT(i); + } + + kfree(config_buf); + + return 0; +} + +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"; + 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; +} + +static void ftdi_sio_gpio_remove(struct usb_serial_port *port) +{ + struct ftdi_private *priv = usb_get_serial_port_data(port); + + if (priv->gpio_used) { + ftdi_sio_set_cbus(port, 0, 0, FTDI_SIO_GPIO_MODE_RESET); + priv->gpio_used = false; + } + + if (priv->gpio_registered) { + gpiochip_remove(&priv->gc); + priv->gpio_registered = false; + } +} + +#else + +static int ftdi_sio_gpio_init(struct usb_serial *serial) +{ + return 0; +} + +static void ftdi_sio_gpio_remove(struct usb_serial *serial) +{ + /* Nothing to do */ +} + +#endif + /* * *************************************************************************** * FTDI driver specific functions @@ -1794,7 +2067,7 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port) { struct ftdi_private *priv; const struct ftdi_sio_quirk *quirk = usb_get_serial_data(port->serial); - + int result; priv = kzalloc(sizeof(struct ftdi_private), GFP_KERNEL); if (!priv) @@ -1813,6 +2086,13 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port) priv->latency = 16; write_latency_timer(port); create_sysfs_attrs(port); + + result = ftdi_sio_gpio_init(port); + if (result < 0) + dev_err(&port->serial->interface->dev, + "GPIO initialisation failed: %d\n", + result); + return 0; } @@ -1930,6 +2210,8 @@ static int ftdi_sio_port_remove(struct usb_serial_port *port) { struct ftdi_private *priv = usb_get_serial_port_data(port); + ftdi_sio_gpio_remove(port); + remove_sysfs_attrs(port); kfree(priv); diff --git a/drivers/usb/serial/ftdi_sio.h b/drivers/usb/serial/ftdi_sio.h index dcd0b6e05baf..5046d02fe3ac 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 CBUS GPIO pin mode */ +#define FTDI_SIO_READ_PINS 12 /* Read immediate value of pins */ +#define FTDI_SIO_READ_EEPROM 144 /* Read EEPROM */ + /* Interface indices for FT2232, FT2232H and FT4232H devices */ #define INTERFACE_A 1 @@ -433,6 +437,27 @@ enum ftdi_sio_baudrate { * 1 = active */ +/* FTDI_SIO_SET_BITMODE */ +#define FTDI_SIO_SET_BITMODE_REQUEST_TYPE 0x40 +#define FTDI_SIO_SET_BITMODE_REQUEST FTDI_SIO_SET_BITMODE + +/* FTDI_SIO_READ_PINS */ +#define FTDI_SIO_READ_PINS_REQUEST_TYPE 0xc0 +#define FTDI_SIO_READ_PINS_REQUEST FTDI_SIO_READ_PINS + +/* + * FTDI_SIO_READ_EEPROM + * + * EEPROM format found in FTDI AN_201, "FT-X MTP memory Configuration", + * http://www.ftdichip.com/Support/Documents/AppNotes/AN_201_FT-X%20MTP%20Memory%20Configuration.pdf + */ +#define FTDI_SIO_READ_EEPROM_REQUEST_TYPE 0xc0 +#define FTDI_SIO_READ_EEPROM_REQUEST FTDI_SIO_READ_EEPROM + +#define FTDI_SIO_GPIO_MODE_CBUS 0x20 +#define FTDI_SIO_GPIO_MODE_RESET 0x00 + +#define FTDI_SIO_CBUS_MUX_GPIO 8 /* Descriptors returned by the device
This patch uses the CBUS bitbang mode of the device, so there is no conflict between the GPIO and VCP functionality. Signed-off-by: Karoly Pados <pados@pados.hu> --- Tested on an FT230X device. Though there is no copied code from other sources, libftdi was used as a reference. I noticed there have been numerous historic attempts by other people at adding GPIO support to ftdi_sio. One tried adding it in some very application-specific way, another one tried to use the mfd framework, the tried using a GPIO mode which is mutually exclusive to VCP, another one did not implement the review results etc... I hope I learned from those attempts and am planning to respond to reviews. Also, this patch follows the rough structure of the GPIO support for cp210x devices that have already been accepted, in the hope that it is a good starting point. This patch uses CBUS bitbanging mode, which works nicely in parallel with the VCP function. The other modes do not, and so IMHO it does not make sense to try adding them to this same module. For this device, whenever changing the state of any single pin, all the others need to be written too. This means in order to change any pin, we need to know the current state of all the others. So when using GPIO, we need to have a known starting state for all pins, but there seems to be no way to retrieve the existing GPIO configuration (directions and output register values). The way I handle this in this patch is that when requesting a GPIO for the first time, the module initializes all pins to a known default state (to inputs). Input was chosen, because a potentially floating pin is better than a potential driver conflict, because the latter could result in hardware damage. However, if the user does not request a GPIO, the CBUS pins are not initialized, avoiding unnecessarily changing hardware state. I figured I cannot rely on the default power-on state of the device for this as the user might have used libftdi before loading our module. When the module is unloaded, CBUS bitbanging mode is exited if it were us who entered it earlier. checkpatch.pl emits a bunch of warnings, recommending const for structs. I am unsure if this makes sense, existing code in this same and also other modules does not adhere to this rule either. I fixed everything else. drivers/usb/serial/ftdi_sio.c | 284 +++++++++++++++++++++++++++++++++- drivers/usb/serial/ftdi_sio.h | 25 +++ 2 files changed, 308 insertions(+), 1 deletion(-)