[PATCH v2] USB: serial: cp210x: Implement GPIO support for CP2102N

[PATCH v2] USB: serial: cp210x: Implement GPIO support for CP2102N

[Karoly Pados]

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 patch is based on the latest patch series by Johan just
submitted today.

Signed-off-by: Karoly Pados <pados@pados.hu>
---
 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;
 #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);
+}
+
 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");
+		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;
 	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)) {
+		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");
 }
 
 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)) {
+		/* 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");
+			return -EPERM;
+		}
+
+		/* 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 */
+	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;
+		}
 
-		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;
+	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
+	 */
+	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;
+	}
+
+	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;
+
+	if (result < 0) {
+		dev_err(&serial->interface->dev,
+			"GPIO initialisation failed, continuing without GPIO support\n");
+		return result;
+	}
+
 	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;
 }
-- 
2.17.1

[v2] USB: serial: cp210x: Implement GPIO support for CP2102N

[Karoly Pados]

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 patch is based on the latest patch series by Johan just
submitted today.

Signed-off-by: Karoly Pados <pados@pados.hu>
---
 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;
 #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);
+}
+
 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");
+		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;
 	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)) {
+		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");
 }
 
 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)) {
+		/* 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");
+			return -EPERM;
+		}
+
+		/* 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 */
+	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;
+		}
 
-		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;
+	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
+	 */
+	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;
+	}
+
+	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;
+
+	if (result < 0) {
+		dev_err(&serial->interface->dev,
+			"GPIO initialisation failed, continuing without GPIO support\n");
+		return result;
+	}
+
 	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;
 }

Re: [PATCH v2] USB: serial: cp210x: Implement GPIO support for CP2102N

[Karoly Pados]

Hello Johan,

I hope maybe after you come back from your vacation you can still
merge this into 4.19, I guess the merge window will still be open.

Given that there are
about another 5 weeks until 4.19rc1 merge window closes,
I didn't think I'd be late with this patch, I am sorry.
However, as already explained to you, it'd be very important
for me to get this patch at least into 4.19, as otherwise the
time I loose is much greater than a single release cycle (due
to distro cycles), and I have actually a crowdfunding product
depending on this.

Thank you for your understanding,
Karoly

July 18, 2018 11:20 PM, "Karoly Pados" <pados@pados.hu> 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 patch is based on the latest patch series by Johan just
> submitted today.
> 
> Signed-off-by: Karoly Pados <pados@pados.hu>
> ---
> 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;
> #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);
> +}
> +
> 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");
> + 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;
> 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)) {
> + 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");
> }
> 
> 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)) {
> + /* 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");
> + return -EPERM;
> + }
> +
> + /* 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 */
> + 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;
> + }
> 
> - 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;
> + 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-xs
> a
> + */
> + 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;
> + }
> +
> + 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;
> +
> + if (result < 0) {
> + dev_err(&serial->interface->dev,
> + "GPIO initialisation failed, continuing without GPIO support\n");
> + return result;
> + }
> +
> 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;
> }
> -- 
> 2.17.1

[v2] USB: serial: cp210x: Implement GPIO support for CP2102N

[Karoly Pados]

Hello Johan,

I hope maybe after you come back from your vacation you can still
merge this into 4.19, I guess the merge window will still be open.

Given that there are
about another 5 weeks until 4.19rc1 merge window closes,
I didn't think I'd be late with this patch, I am sorry.
However, as already explained to you, it'd be very important
for me to get this patch at least into 4.19, as otherwise the
time I loose is much greater than a single release cycle (due
to distro cycles), and I have actually a crowdfunding product
depending on this.

Thank you for your understanding,
Karoly

July 18, 2018 11:20 PM, "Karoly Pados" <pados@pados.hu> 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 patch is based on the latest patch series by Johan just
> submitted today.
> 
> Signed-off-by: Karoly Pados <pados@pados.hu>
> ---
> 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;
> #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);
> +}
> +
> 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");
> + 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;
> 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)) {
> + 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");
> }
> 
> 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)) {
> + /* 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");
> + return -EPERM;
> + }
> +
> + /* 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 */
> + 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;
> + }
> 
> - 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;
> + 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-xs
> a
> + */
> + 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;
> + }
> +
> + 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;
> +
> + if (result < 0) {
> + dev_err(&serial->interface->dev,
> + "GPIO initialisation failed, continuing without GPIO support\n");
> + return result;
> + }
> +
> 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;
> }
> -- 
> 2.17.1
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] USB: serial: cp210x: Implement GPIO support for CP2102N

[Johan Hovold]

On Wed, Jul 18, 2018 at 09:38:12PM +0000, Karoly Pados wrote:
> Hello Johan,
> 
> I hope maybe after you come back from your vacation you can still
> merge this into 4.19, I guess the merge window will still be open.

You shouldn't make assumptions about peoples vacation plans. ;)

> Given that there are
> about another 5 weeks until 4.19rc1 merge window closes,
> I didn't think I'd be late with this patch, I am sorry.

New code must be ready and reviewed well in time before the merge window
*opens*. So generally you cannot expect anything you post this week to
be included in 4.19.

> However, as already explained to you, it'd be very important
> for me to get this patch at least into 4.19, as otherwise the
> time I loose is much greater than a single release cycle (due
> to distro cycles), and I have actually a crowdfunding product
> depending on this.

I know, and I already cut some corners for you with the baud-rate
handling changes even though this is strictly not a valid reason to
change the normal procedures.

That being said, I took a look at your gpio patch today and as it looks
like you've addressed the major concerns with v1, and done so in a clean
way, I'll make an exception.

I'll post some review comments, but I've already fixed up the patch
myself. It's mostly style issues, but I did also find two bugs.

I do need you to respin the commit message, though; a commit message
should be self-contained and not rely on external discussions to make
sense (you can of course still refer/link to it). Patch-revision info
should generally go below the cut-off line, unless it is useful to have
it recorded.

Can you post a reworked commit message today so I can include this in my
4.19 updates?

Thanks,
Johan

[v2] USB: serial: cp210x: Implement GPIO support for CP2102N

[Johan Hovold]

On Wed, Jul 18, 2018 at 09:38:12PM +0000, Karoly Pados wrote:
> Hello Johan,
> 
> I hope maybe after you come back from your vacation you can still
> merge this into 4.19, I guess the merge window will still be open.

You shouldn't make assumptions about peoples vacation plans. ;)

> Given that there are
> about another 5 weeks until 4.19rc1 merge window closes,
> I didn't think I'd be late with this patch, I am sorry.

New code must be ready and reviewed well in time before the merge window
*opens*. So generally you cannot expect anything you post this week to
be included in 4.19.

> However, as already explained to you, it'd be very important
> for me to get this patch at least into 4.19, as otherwise the
> time I loose is much greater than a single release cycle (due
> to distro cycles), and I have actually a crowdfunding product
> depending on this.

I know, and I already cut some corners for you with the baud-rate
handling changes even though this is strictly not a valid reason to
change the normal procedures.

That being said, I took a look at your gpio patch today and as it looks
like you've addressed the major concerns with v1, and done so in a clean
way, I'll make an exception.

I'll post some review comments, but I've already fixed up the patch
myself. It's mostly style issues, but I did also find two bugs.

I do need you to respin the commit message, though; a commit message
should be self-contained and not rely on external discussions to make
sense (you can of course still refer/link to it). Patch-revision info
should generally go below the cut-off line, unless it is useful to have
it recorded.

Can you post a reworked commit message today so I can include this in my
4.19 updates?

Thanks,
Johan
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] USB: serial: cp210x: Implement GPIO support for CP2102N

[Johan Hovold]

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@pados.hu>

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

[v2] USB: serial: cp210x: Implement GPIO support for CP2102N

[Johan Hovold]

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@pados.hu>

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] USB: serial: cp210x: Implement GPIO support for CP2102N

[Karoly Pados]

Thank you very much for squeezing this into your schedule. 
I've also become a lot more experienced now in contributing to the
kernel, concerning both planning and code. Next time things should
work out even smoother.

Wish you a great and rewarding vacation!

Karoly

July 20, 2018 12:16 PM, "Johan Hovold" <johan@kernel.org> wrote:

> On Wed, Jul 18, 2018 at 09:38:12PM +0000, Karoly Pados wrote:
> 
>> Hello Johan,
>> 
>> I hope maybe after you come back from your vacation you can still
>> merge this into 4.19, I guess the merge window will still be open.
> 
> You shouldn't make assumptions about peoples vacation plans. ;)
> 
>> Given that there are
>> about another 5 weeks until 4.19rc1 merge window closes,
>> I didn't think I'd be late with this patch, I am sorry.
> 
> New code must be ready and reviewed well in time before the merge window
> *opens*. So generally you cannot expect anything you post this week to
> be included in 4.19.
> 
>> However, as already explained to you, it'd be very important
>> for me to get this patch at least into 4.19, as otherwise the
>> time I loose is much greater than a single release cycle (due
>> to distro cycles), and I have actually a crowdfunding product
>> depending on this.
> 
> I know, and I already cut some corners for you with the baud-rate
> handling changes even though this is strictly not a valid reason to
> change the normal procedures.
> 
> That being said, I took a look at your gpio patch today and as it looks
> like you've addressed the major concerns with v1, and done so in a clean
> way, I'll make an exception.
> 
> I'll post some review comments, but I've already fixed up the patch
> myself. It's mostly style issues, but I did also find two bugs.
> 
> I do need you to respin the commit message, though; a commit message
> should be self-contained and not rely on external discussions to make
> sense (you can of course still refer/link to it). Patch-revision info
> should generally go below the cut-off line, unless it is useful to have
> it recorded.
> 
> Can you post a reworked commit message today so I can include this in my
> 4.19 updates?
> 
> Thanks,
> Johan

[v2] USB: serial: cp210x: Implement GPIO support for CP2102N

[Karoly Pados]

Thank you very much for squeezing this into your schedule. 
I've also become a lot more experienced now in contributing to the
kernel, concerning both planning and code. Next time things should
work out even smoother.

Wish you a great and rewarding vacation!

Karoly

July 20, 2018 12:16 PM, "Johan Hovold" <johan@kernel.org> wrote:

> On Wed, Jul 18, 2018 at 09:38:12PM +0000, Karoly Pados wrote:
> 
>> Hello Johan,
>> 
>> I hope maybe after you come back from your vacation you can still
>> merge this into 4.19, I guess the merge window will still be open.
> 
> You shouldn't make assumptions about peoples vacation plans. ;)
> 
>> Given that there are
>> about another 5 weeks until 4.19rc1 merge window closes,
>> I didn't think I'd be late with this patch, I am sorry.
> 
> New code must be ready and reviewed well in time before the merge window
> *opens*. So generally you cannot expect anything you post this week to
> be included in 4.19.
> 
>> However, as already explained to you, it'd be very important
>> for me to get this patch at least into 4.19, as otherwise the
>> time I loose is much greater than a single release cycle (due
>> to distro cycles), and I have actually a crowdfunding product
>> depending on this.
> 
> I know, and I already cut some corners for you with the baud-rate
> handling changes even though this is strictly not a valid reason to
> change the normal procedures.
> 
> That being said, I took a look at your gpio patch today and as it looks
> like you've addressed the major concerns with v1, and done so in a clean
> way, I'll make an exception.
> 
> I'll post some review comments, but I've already fixed up the patch
> myself. It's mostly style issues, but I did also find two bugs.
> 
> I do need you to respin the commit message, though; a commit message
> should be self-contained and not rely on external discussions to make
> sense (you can of course still refer/link to it). Patch-revision info
> should generally go below the cut-off line, unless it is useful to have
> it recorded.
> 
> Can you post a reworked commit message today so I can include this in my
> 4.19 updates?
> 
> Thanks,
> Johan
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] USB: serial: cp210x: Implement GPIO support for CP2102N

[Johan Hovold]

On Fri, Jul 20, 2018 at 12:43:09PM +0000, Karoly Pados wrote:
> Thank you very much for squeezing this into your schedule. 
> I've also become a lot more experienced now in contributing to the
> kernel, concerning both planning and code. Next time things should
> work out even smoother.

Sounds good!

> Wish you a great and rewarding vacation!

Thanks!

Johan

[v2] USB: serial: cp210x: Implement GPIO support for CP2102N

[Johan Hovold]

On Fri, Jul 20, 2018 at 12:43:09PM +0000, Karoly Pados wrote:
> Thank you very much for squeezing this into your schedule. 
> I've also become a lot more experienced now in contributing to the
> kernel, concerning both planning and code. Next time things should
> work out even smoother.

Sounds good!

> Wish you a great and rewarding vacation!

Thanks!

Johan
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html