On Wed, Jul 18, 2018 at 11:20:04PM +0200, Karoly Pados wrote: > This is v2 of the patch and addresses all feedback previously > received from the maintainer. New input/output support stayed > as discussed on the e-mail lists separately. CP2105 is also > using the new code structure, but due to missing way to get > default pin states after reset from the device, support > for this device is basically still output-only as before, at > least in name. But CP2105 and CP2102N paths are unified. This reads like a patch changelog rather than a commit message. Please try to rephrase this so that it's self-contained and not relying on having read the mail thread for v1. In the future you should include a changelog from what changed from one revision to another below the cut-off line (---) instead. > This patch is based on the latest patch series by Johan just > submitted today. Should also go below the cut-off line. > Signed-off-by: Karoly Pados <pados@xxxxxxxx> Oh, and you should have included Martyn who contributed to the discussion about how to handle input mode on CC. > --- > drivers/usb/serial/cp210x.c | 274 ++++++++++++++++++++++++++++++------ > 1 file changed, 232 insertions(+), 42 deletions(-) > > diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c > index 4a118eb13590..81f9d3e183c6 100644 > --- a/drivers/usb/serial/cp210x.c > +++ b/drivers/usb/serial/cp210x.c > @@ -224,9 +224,19 @@ MODULE_DEVICE_TABLE(usb, id_table); > struct cp210x_serial_private { > #ifdef CONFIG_GPIOLIB > struct gpio_chip gc; > - u8 config; > - u8 gpio_mode; > bool gpio_registered; > + > + /* > + * The following three fields are for devices that > + * emulate input/output pins using open-drain/pushpull > + * drive modes. > + */ > + /* One bit for each GPIO, 1 if pin is pushpull */ > + u8 gpio_pushpull; > + /* One bit for each GPIO, 1 if pin is not in GPIO mode */ > + u8 gpio_altfunc; > + /* One bit for each GPIO, 1 if pin direction is input */ > + u8 gpio_input; Your code is clean, but you're relying a bit too much on comments in my opinion. The code should be self-explanatory and not rely on in-function comments, unless you want to highlight something particularly important or clever. I've dropped some examples of this in my reworked version of the patch. > #endif > u8 partnum; > speed_t max_speed; > @@ -343,6 +353,7 @@ static struct usb_serial_driver * const serial_drivers[] = { > #define CONTROL_WRITE_RTS 0x0200 > > /* CP210X_VENDOR_SPECIFIC values */ > +#define CP210X_READ_2NCONFIG 0x000E > #define CP210X_READ_LATCH 0x00C2 > #define CP210X_GET_PARTNUM 0x370B > #define CP210X_GET_PORTCONFIG 0x370C > @@ -452,6 +463,12 @@ struct cp210x_config { > #define CP2105_GPIO1_RXLED_MODE BIT(1) > #define CP2105_GPIO1_RS485_MODE BIT(2) > > +/* CP2102N configuration array indices */ > +#define CP210X_2NCONFIG_CONFIG_VERSION_IDX 2 > +#define CP210X_2NCONFIG_GPIO_MODE_IDX 581 > +#define CP210X_2NCONFIG_GPIO_RSTLATCH_IDX 587 > +#define CP210X_2NCONFIG_GPIO_CONTROL_IDX 600 > + > /* CP210X_VENDOR_SPECIFIC, CP210X_WRITE_LATCH call writes these 0x2 bytes. */ > struct cp210x_gpio_write { > u8 mask; > @@ -1308,21 +1325,29 @@ static void cp210x_break_ctl(struct tty_struct *tty, int break_state) > } > > #ifdef CONFIG_GPIOLIB > + > +/* > + * Helper to determine if a specific serial device belongs to the cp2102n > + * family of devices. > + */ > +static bool cp210x_is_cp2102n(struct usb_serial *serial) > +{ > + struct cp210x_serial_private *priv = usb_get_serial_data(serial); > + > + return (priv->partnum == CP210X_PARTNUM_CP2102N_QFN28) || > + (priv->partnum == CP210X_PARTNUM_CP2102N_QFN24) || > + (priv->partnum == CP210X_PARTNUM_CP2102N_QFN20); > +} I also did away with this one (again). The cp2102n way of dealing with gpios appear to be the standard way; while cp2105 and cp2108 are the odd birds. > + > static int cp210x_gpio_request(struct gpio_chip *gc, unsigned int offset) > { > struct usb_serial *serial = gpiochip_get_data(gc); > struct cp210x_serial_private *priv = usb_get_serial_data(serial); > > - switch (offset) { > - case 0: > - if (priv->config & CP2105_GPIO0_TXLED_MODE) > - return -ENODEV; > - break; > - case 1: > - if (priv->config & (CP2105_GPIO1_RXLED_MODE | > - CP2105_GPIO1_RS485_MODE)) > - return -ENODEV; > - break; > + if (priv->gpio_altfunc & BIT(offset)) { > + dev_warn(&serial->interface->dev, > + "Cannot control GPIO with active alternate function.\n"); I dropped the warning, you're already returning an errno as before. > + return -ENODEV; > } > > return 0; > @@ -1331,10 +1356,15 @@ static int cp210x_gpio_request(struct gpio_chip *gc, unsigned int offset) > static int cp210x_gpio_get(struct gpio_chip *gc, unsigned int gpio) > { > struct usb_serial *serial = gpiochip_get_data(gc); > + struct cp210x_serial_private *priv = usb_get_serial_data(serial); > + u8 req_type = REQTYPE_DEVICE_TO_HOST; > int result; > u8 buf; > > - result = cp210x_read_vendor_block(serial, REQTYPE_INTERFACE_TO_HOST, > + if (priv->partnum == CP210X_PARTNUM_CP2105) > + req_type = REQTYPE_INTERFACE_TO_HOST; > + > + result = cp210x_read_vendor_block(serial, req_type, > CP210X_READ_LATCH, &buf, sizeof(buf)); > if (result < 0) > return result; > @@ -1345,34 +1375,82 @@ static int cp210x_gpio_get(struct gpio_chip *gc, unsigned int gpio) > static void cp210x_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value) > { > struct usb_serial *serial = gpiochip_get_data(gc); > + struct cp210x_serial_private *priv = usb_get_serial_data(serial); > struct cp210x_gpio_write buf; > + int result = 0; > > - if (value == 1) > - buf.state = BIT(gpio); > - else > - buf.state = 0; > - > + buf.state = (value == 1) ? BIT(gpio) : 0; No need to change this, and generally try to avoid the ternary operator which often just makes code harder to read. > buf.mask = BIT(gpio); > > - cp210x_write_vendor_block(serial, REQTYPE_HOST_TO_INTERFACE, > - CP210X_WRITE_LATCH, &buf, sizeof(buf)); > + if (priv->partnum == CP210X_PARTNUM_CP2105) { > + result = cp210x_write_vendor_block(serial, > + REQTYPE_HOST_TO_INTERFACE, > + CP210X_WRITE_LATCH, &buf, > + sizeof(buf)); > + } else if (cp210x_is_cp2102n(serial)) { So I just made this the default implementation by dropping the conditional. > + u16 wIndex = buf.state << 8 | buf.mask; > + > + result = usb_control_msg(serial->dev, > + usb_sndctrlpipe(serial->dev, 0), > + CP210X_VENDOR_SPECIFIC, > + REQTYPE_HOST_TO_DEVICE, > + CP210X_WRITE_LATCH, > + wIndex, > + NULL, 0, USB_CTRL_SET_TIMEOUT); > + } > + > + if (result < 0) > + dev_err(&serial->interface->dev, "Failed to set GPIO value.\n"); This could include the errno. > } > > static int cp210x_gpio_direction_get(struct gpio_chip *gc, unsigned int gpio) > { > - /* Hardware does not support an input mode */ > - return 0; > + struct usb_serial *serial = gpiochip_get_data(gc); > + struct cp210x_serial_private *priv = usb_get_serial_data(serial); > + > + return priv->gpio_input & BIT(gpio); > } > > static int cp210x_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio) > { > - /* Hardware does not support an input mode */ > - return -ENOTSUPP; > + struct usb_serial *serial = gpiochip_get_data(gc); > + struct cp210x_serial_private *priv = usb_get_serial_data(serial); > + > + if (priv->partnum == CP210X_PARTNUM_CP2105) { > + /* Hardware does not support an input mode */ > + return -ENOTSUPP; > + } else if (cp210x_is_cp2102n(serial)) { This should be the default implementation. > + /* Push-pull pins cannot be changed to be inputs */ > + if (priv->gpio_pushpull & BIT(gpio)) { > + dev_warn(&serial->interface->dev, > + "Cannot change direction of a push-pull GPIO to input.\n"); No need to warn, as you're returning an error. > + return -EPERM; And this isn't really a permission issue; -EINVAL is more appropriate. > + } > + > + /* Make sure to release pin if it is being driven low */ > + cp210x_gpio_set(gc, gpio, 1); > + > + /* Note pin direction to ourselves */ > + priv->gpio_input |= BIT(gpio); > + > + return 0; > + } > + > + return -EPERM; > } > > static int cp210x_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio, > int value) > { > + struct usb_serial *serial = gpiochip_get_data(gc); > + struct cp210x_serial_private *priv = usb_get_serial_data(serial); > + > + /* Note pin direction to ourselves */ > + priv->gpio_input &= ~BIT(gpio); > + > + /* Set requested initial output value */ I'd drop these comments for example. > + cp210x_gpio_set(gc, gpio, value); > + > return 0; > } > > @@ -1385,11 +1463,11 @@ static int cp210x_gpio_set_config(struct gpio_chip *gc, unsigned int gpio, > > /* Succeed only if in correct mode (this can't be set at runtime) */ > if ((param == PIN_CONFIG_DRIVE_PUSH_PULL) && > - (priv->gpio_mode & BIT(gpio))) > + (priv->gpio_pushpull & BIT(gpio))) > return 0; > > if ((param == PIN_CONFIG_DRIVE_OPEN_DRAIN) && > - !(priv->gpio_mode & BIT(gpio))) > + !(priv->gpio_pushpull & BIT(gpio))) > return 0; > > return -ENOTSUPP; > @@ -1402,13 +1480,14 @@ static int cp210x_gpio_set_config(struct gpio_chip *gc, unsigned int gpio, > * this driver that provide GPIO do so in a way that does not impact other > * signals and are thus expected to have very different initialisation. > */ > -static int cp2105_shared_gpio_init(struct usb_serial *serial) > +static int cp2105_gpioconf_init(struct usb_serial *serial) > { > struct cp210x_serial_private *priv = usb_get_serial_data(serial); > struct cp210x_pin_mode mode; > struct cp210x_config config; > u8 intf_num = cp210x_interface_num(serial); > int result; > + u8 iface_config; > > result = cp210x_read_vendor_block(serial, REQTYPE_DEVICE_TO_HOST, > CP210X_GET_DEVICEMODE, &mode, > @@ -1424,20 +1503,25 @@ static int cp2105_shared_gpio_init(struct usb_serial *serial) > > /* 2 banks of GPIO - One for the pins taken from each serial port */ > if (intf_num == 0) { > - if (mode.eci == CP210X_PIN_MODE_MODEM) > + if (mode.eci == CP210X_PIN_MODE_MODEM) { > + /* Mark all GPIOs of this interface as reserved */ > + priv->gpio_altfunc = 0xFF; > return 0; > + } > > - priv->config = config.eci_cfg; > - priv->gpio_mode = (u8)((le16_to_cpu(config.gpio_mode) & > + iface_config = config.eci_cfg; > + priv->gpio_pushpull = (u8)((le16_to_cpu(config.gpio_mode) & > CP210X_ECI_GPIO_MODE_MASK) >> > CP210X_ECI_GPIO_MODE_OFFSET); > priv->gc.ngpio = 2; > } else if (intf_num == 1) { > - if (mode.sci == CP210X_PIN_MODE_MODEM) > - return 0; > + if (mode.sci == CP210X_PIN_MODE_MODEM) { > + /* Mark all GPIOs of this interface as reserved */ > + priv->gpio_altfunc = 0xFF; Here the return 0 fell out, which could lead to the pins being considered available. > + } > > - priv->config = config.sci_cfg; > - priv->gpio_mode = (u8)((le16_to_cpu(config.gpio_mode) & > + iface_config = config.sci_cfg; > + priv->gpio_pushpull = (u8)((le16_to_cpu(config.gpio_mode) & > CP210X_SCI_GPIO_MODE_MASK) >> > CP210X_SCI_GPIO_MODE_OFFSET); > priv->gc.ngpio = 3; > @@ -1445,6 +1529,118 @@ static int cp2105_shared_gpio_init(struct usb_serial *serial) > return -ENODEV; > } > > + /* Mark all pins which are not in GPIO mode */ > + priv->gpio_altfunc = 0; > + if (iface_config & CP2105_GPIO0_TXLED_MODE) /* GPIO 0 */ > + priv->gpio_altfunc |= BIT(0); > + if (iface_config & (CP2105_GPIO1_RXLED_MODE | /* GPIO 1 */ > + CP2105_GPIO1_RS485_MODE)) > + priv->gpio_altfunc |= BIT(1); > + > + /* Driver implementation for CP2105 only supports outputs */ > + priv->gpio_input = 0; > + > + return 0; > +} > + > +static int cp2102n_gpioconf_init(struct usb_serial *serial) > +{ > + struct cp210x_serial_private *priv = usb_get_serial_data(serial); > + const u16 CONFIG_SIZE = 0x02A6; Lower case variable names, and I'd use lower-case for hex constants as well. > + u8 gpio_rst_latch; > + u8 config_version; > + u8 gpio_pushpull; > + u8 *config_buf; > + u8 gpio_latch; > + u8 gpio_ctrl; > + int result; > + u8 i; > + > + /* Retrieve device configuration from the device. > + * The array received contains all customization settings > + * done at the factory/manufacturer. > + * Format of the array is documented at the time of writing at > + * https://www.silabs.com/community/interface/knowledge-base.entry.html/2017/03/31/cp2102n_setconfig-xsfa > + */ Multi-line comment style is /* * blah... */ as I already mentioned in comments to v1. > + config_buf = kmalloc(CONFIG_SIZE, GFP_KERNEL); > + if (!config_buf) > + return -ENOMEM; > + > + result = cp210x_read_vendor_block(serial, > + REQTYPE_DEVICE_TO_HOST, > + CP210X_READ_2NCONFIG, > + config_buf, > + CONFIG_SIZE); > + if (result < 0) { > + kfree(config_buf); > + return -EIO; Return result. > + } > + > + config_version = config_buf[CP210X_2NCONFIG_CONFIG_VERSION_IDX]; > + gpio_pushpull = config_buf[CP210X_2NCONFIG_GPIO_MODE_IDX]; > + gpio_ctrl = config_buf[CP210X_2NCONFIG_GPIO_CONTROL_IDX]; > + gpio_rst_latch = config_buf[CP210X_2NCONFIG_GPIO_RSTLATCH_IDX]; > + > + kfree(config_buf); > + > + /* Make sure this is a config format we understand */ > + if (config_version != 0x01) > + return -ENOTSUPP; > + > + /* We only support 4 GPIOs even on the QFN28 package, because > + * config locations of GPIOs 4-6 determined using reverse > + * engineering revealed conflicting offsets with other > + * documented functions. So we'll just play it safe for now. > + */ > + priv->gc.ngpio = 4; > + > + /* Get default pin states after reset. Needed so we can determine > + * the direction of an open-drain pin. > + */ > + gpio_latch = (gpio_rst_latch >> 3) & 0x0F; > + > + /* 0 indicates open-drain mode, 1 is push-pull */ > + priv->gpio_pushpull = (gpio_pushpull >> 3) & 0x0F; > + > + /* 0 indicates GPIO mode, 1 is alternate function */ > + priv->gpio_altfunc = (gpio_ctrl >> 2) & 0x0F; > + > + /* The CP2102N does not strictly has input and output pin modes, > + * it only knows open-drain and push-pull modes which is set at > + * factory. An open-drain pin can function both as an > + * input or an output. We emulate input mode for open-drain pins > + * by making sure they are not driven low, and we do not allow > + * push-pull pins to be set as an input. > + */ > + for (i = 0; i < priv->gc.ngpio; ++i) { > + /* Set direction to "input" iff > + * pin is open-drain and reset value is 1 > + */ > + if (!(priv->gpio_pushpull & BIT(i)) && (gpio_latch & BIT(i))) > + priv->gpio_input |= BIT(i); > + } > + > + return 0; > +} > + > +static int cp210x_gpio_init(struct usb_serial *serial) > +{ > + struct cp210x_serial_private *priv = usb_get_serial_data(serial); > + int result = 0; > + > + if (cp210x_is_cp2102n(serial)) > + result = cp2102n_gpioconf_init(serial); > + else if (priv->partnum == CP210X_PARTNUM_CP2105) > + result = cp2105_gpioconf_init(serial); > + else > + return 0; This could be a switch-statement. > + > + if (result < 0) { > + dev_err(&serial->interface->dev, > + "GPIO initialisation failed, continuing without GPIO support\n"); > + return result; > + } By moving the error message into cp210x_gpio_init, we'd no longer log any registration errors below. > + > priv->gc.label = "cp210x"; > priv->gc.request = cp210x_gpio_request; > priv->gc.get_direction = cp210x_gpio_direction_get; > @@ -1477,7 +1673,7 @@ static void cp210x_gpio_remove(struct usb_serial *serial) > > #else > > -static int cp2105_shared_gpio_init(struct usb_serial *serial) > +static int cp210x_gpio_init(struct usb_serial *serial) > { > return 0; > } > @@ -1588,13 +1784,7 @@ static int cp210x_attach(struct usb_serial *serial) > > cp210x_init_max_speed(serial); > > - if (priv->partnum == CP210X_PARTNUM_CP2105) { > - result = cp2105_shared_gpio_init(serial); > - if (result < 0) { > - dev_err(&serial->interface->dev, > - "GPIO initialisation failed, continuing without GPIO support\n"); > - } > - } > + cp210x_gpio_init(serial); > > return 0; > } So, mostly minor things that I've now fixed up. Nice and clean job overall. Post a new commit message, and we'll get this included in 4.19. Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html