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

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

[Karoly Pados]

Pretty much what the title says. No other functions/devices
touched. Tested on CP2102N-QFN28.

Limitation: Even though the QFN28 package has 7 GPIOs,
this patch allows to control only the first 4.
What I've found using reverse engineering regarding the
other 3 pins collides both with reality and with other
documented functions, so I did not dare to just use my
findings because I cannot test on other packages.
Instead, I decided to play it safe and only support 4 GPIOs.

Signed-off-by: Karoly Pados <pados@pados.hu>
---
 drivers/usb/serial/cp210x.c | 259 +++++++++++++++++++++++++++++++++++-
 1 file changed, 257 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 793b86252c46..adb450185ce8 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -6,7 +6,8 @@
  *
  * Support to set flow control line levels using TIOCMGET and TIOCMSET
  * thanks to Karl Hiramoto karl@hiramoto.org. RTSCTS hardware flow
- * control thanks to Munir Nassar nassarmu@real-time.com
+ * control thanks to Munir Nassar nassarmu@real-time.com.
+ * GPIO support for CP2102N thanks to Karoly Pados.
  *
  */
 
@@ -229,11 +230,16 @@ struct cp210x_serial_private {
 	struct gpio_chip	gc;
 	u8			config;
 	u8			gpio_mode;
+	u8			gpio_control;
+	u8			gpio_dir;
 	bool			gpio_registered;
 #endif
 	u8			partnum;
 };
 
+#define CP2102N_PIN_OPENDRAIN(var, pin)	(!((var) & BIT(pin)))
+#define CP2102N_PIN_GPIO(var, pin)	(!((var) & BIT(pin)))
+
 struct cp210x_port_private {
 	__u8			bInterfaceNumber;
 	bool			has_swapped_line_ctl;
@@ -349,6 +355,7 @@ static struct usb_serial_driver * const serial_drivers[] = {
 #define CP210X_GET_PORTCONFIG	0x370C
 #define CP210X_GET_DEVICEMODE	0x3711
 #define CP210X_WRITE_LATCH	0x37E1
+#define CP210X_READ_2NCONFIG	0x000E
 
 /* Part number definitions */
 #define CP210X_PARTNUM_CP2101	0x01
@@ -362,6 +369,14 @@ static struct usb_serial_driver * const serial_drivers[] = {
 #define CP210X_PARTNUM_CP2102N_QFN20	0x22
 #define CP210X_PARTNUM_UNKNOWN	0xFF
 
+#define	IS_CP2102N(partnum)   (((partnum) == CP210X_PARTNUM_CP2102N_QFN28) || \
+			       ((partnum) == CP210X_PARTNUM_CP2102N_QFN24) || \
+			       ((partnum) == CP210X_PARTNUM_CP2102N_QFN20))
+
+#define CP210X_2NCONFIG_GPIO_CONTROL_IDX	600
+#define CP210X_2NCONFIG_GPIO_MODE_IDX		581
+#define CP210X_2NCONFIG_GPIO_RSTLATCH_IDX	587
+
 /* CP210X_GET_COMM_STATUS returns these 0x13 bytes */
 struct cp210x_comm_status {
 	__le32   ulErrors;
@@ -1455,6 +1470,235 @@ static void cp210x_gpio_remove(struct usb_serial *serial)
 	}
 }
 
+static int cp2102n_gpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct usb_serial *serial = gpiochip_get_data(gc);
+	int result;
+	u8 buf;
+
+	result = cp210x_read_vendor_block(serial, REQTYPE_DEVICE_TO_HOST,
+					  CP210X_READ_LATCH, &buf, sizeof(buf));
+	if (result < 0)
+		return result;
+
+	return !!(buf & BIT(gpio));
+}
+
+static void cp2102n_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
+{
+	int result;
+	struct cp210x_gpio_write buf;
+	struct usb_serial *serial = gpiochip_get_data(gc);
+	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
+
+	/* Ignore request if pin is not in our control */
+	if (!CP2102N_PIN_GPIO(priv->gpio_control, gpio)) {
+		dev_warn(&serial->interface->dev,
+			 "Cannot control GPIO with active alternate function.\n");
+		return;
+	}
+
+	buf.state = (value == 1) ? BIT(gpio) : 0;
+	buf.mask = BIT(gpio);
+
+	result = usb_control_msg(serial->dev,
+				 usb_sndctrlpipe(serial->dev, 0),
+				 CP210X_VENDOR_SPECIFIC,
+				 REQTYPE_HOST_TO_DEVICE,
+				 CP210X_WRITE_LATCH,
+				 *(u16 *)&buf,
+				 NULL, 0, USB_CTRL_SET_TIMEOUT);
+
+	if (result < 0) {
+		dev_err(&serial->interface->dev,
+			"Failed to set GPIO value.\n");
+	}
+}
+
+static int cp2102n_gpio_direction_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);
+
+	return priv->gpio_dir & BIT(gpio);
+}
+
+static int cp2102n_gpio_direction_input(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);
+
+	/* Return an error if pin is not in our control */
+	if (!CP2102N_PIN_GPIO(priv->gpio_control, gpio)) {
+		dev_warn(&serial->interface->dev,
+			 "Cannot control GPIO with active alternate function.\n");
+		return -EPERM;
+	}
+
+	/* Push-pull pins cannot be changed to be inputs */
+	if (!CP2102N_PIN_OPENDRAIN(priv->gpio_mode, 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 */
+	cp2102n_gpio_set(gc, gpio, 1);
+
+	/* Note pin direction to ourselves */
+	priv->gpio_dir |= BIT(gpio);
+
+	return 0;
+}
+
+static int cp2102n_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);
+
+	/* Return an error if pin is not in our control */
+	if (!CP2102N_PIN_GPIO(priv->gpio_control, gpio)) {
+		dev_warn(&serial->interface->dev,
+			 "Cannot control GPIO with active alternate function.\n");
+		return -EPERM;
+	}
+
+	/* Note pin direction to ourselves */
+	priv->gpio_dir &= ~BIT(gpio);
+
+	/* Set requested initial output value */
+	cp2102n_gpio_set(gc, gpio, value);
+
+	return 0;
+}
+
+static u16 fletcher16(u8 *data, u16 bytes)
+{
+	u16 sum1 = 0xff, sum2 = 0xff;
+	u16 tlen;
+
+	while (bytes) {
+		tlen = bytes >= 20 ? 20 : bytes;
+		bytes -= tlen;
+		do {
+			sum2 += sum1 += *data++;
+		} while (--tlen);
+		sum1 = (sum1 & 0xff) + (sum1 >> 8);
+		sum2 = (sum2 & 0xff) + (sum2 >> 8);
+	}
+	/* Second reduction step to reduce sums to 8 bits */
+	sum1 = (sum1 & 0xff) + (sum1 >> 8);
+	sum2 = (sum2 & 0xff) + (sum2 >> 8);
+	return sum2 << 8 | sum1;
+}
+
+static int cp2102n_gpio_init(struct usb_serial *serial)
+{
+	const u16 CONFIG_SIZE = 0x02A6;
+	int result;
+	u16 config_csum;
+	u8 *config_buf;
+	u8 gpio_ctrl;
+	u8 gpio_mode;
+	u8 gpio_latch;
+	u8 gpio_rst_latch;
+	u8 i;
+	bool config_valid = true;
+	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
+
+	/* 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)
+		return result;
+
+	/* Check configuration for validity.
+	 * The last two bytes of the array contain a fletcher16
+	 * checksum of all the bytes before, which we can validate.
+	 */
+	config_csum = fletcher16(config_buf, CONFIG_SIZE - 2);
+	if (((config_csum & 0xFF) != config_buf[CONFIG_SIZE - 1]) ||
+	    ((config_csum >> 8)   != config_buf[CONFIG_SIZE - 2])) {
+		config_valid = false;
+		dev_err(&serial->interface->dev,
+			"Corrupted device configuration received\n");
+	}
+
+	gpio_mode = 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);
+
+	if (!config_valid)
+		return -EIO;
+
+	/* We only support 4 GPIOs even on the QFN28 package because
+	 * documentation about the CP2102N GetConfig Array
+	 * does not seem to correspond to reality on this device.
+	 */
+	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_mode = (gpio_mode >> 3) & 0x0F;
+
+	/* 0 indicates GPIO mode, 1 is alternate function */
+	priv->gpio_control = (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 (CP2102N_PIN_OPENDRAIN(priv->gpio_mode, i) &&
+		    (gpio_latch & BIT(i))) {
+			priv->gpio_dir |= BIT(i);
+		}
+	}
+
+	priv->gc.label = "cp2102n";
+	priv->gc.get_direction = cp2102n_gpio_direction_get;
+	priv->gc.direction_input = cp2102n_gpio_direction_input;
+	priv->gc.direction_output = cp2102n_gpio_direction_output;
+	priv->gc.get = cp2102n_gpio_get;
+	priv->gc.set = cp2102n_gpio_set;
+	priv->gc.owner = THIS_MODULE;
+	priv->gc.parent = &serial->interface->dev;
+	priv->gc.base = -1;
+	priv->gc.can_sleep = true;
+
+	result = gpiochip_add_data(&priv->gc, serial);
+	if (!result)
+		priv->gpio_registered = true;
+
+	return result;
+}
+
 #else
 
 static int cp2105_shared_gpio_init(struct usb_serial *serial)
@@ -1462,6 +1706,11 @@ static int cp2105_shared_gpio_init(struct usb_serial *serial)
 	return 0;
 }
 
+static int cp2102n_gpio_init(struct usb_serial *serial)
+{
+	return 0;
+}
+
 static void cp210x_gpio_remove(struct usb_serial *serial)
 {
 	/* Nothing to do */
@@ -1522,7 +1771,13 @@ static int cp210x_attach(struct usb_serial *serial)
 
 	usb_set_serial_data(serial, priv);
 
-	if (priv->partnum == CP210X_PARTNUM_CP2105) {
+	if (IS_CP2102N(priv->partnum)) {
+		result = cp2102n_gpio_init(serial);
+		if (result < 0) {
+			dev_err(&serial->interface->dev,
+				"GPIO initialisation failed, continuing without GPIO support\n");
+		}
+	} else if (priv->partnum == CP210X_PARTNUM_CP2105) {
 		result = cp2105_shared_gpio_init(serial);
 		if (result < 0) {
 			dev_err(&serial->interface->dev,
-- 
2.17.1

USB: serial: cp210x: Implement GPIO support for CP2102N

[Karoly Pados]

Pretty much what the title says. No other functions/devices
touched. Tested on CP2102N-QFN28.

Limitation: Even though the QFN28 package has 7 GPIOs,
this patch allows to control only the first 4.
What I've found using reverse engineering regarding the
other 3 pins collides both with reality and with other
documented functions, so I did not dare to just use my
findings because I cannot test on other packages.
Instead, I decided to play it safe and only support 4 GPIOs.

Signed-off-by: Karoly Pados <pados@pados.hu>
---
 drivers/usb/serial/cp210x.c | 259 +++++++++++++++++++++++++++++++++++-
 1 file changed, 257 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 793b86252c46..adb450185ce8 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -6,7 +6,8 @@
  *
  * Support to set flow control line levels using TIOCMGET and TIOCMSET
  * thanks to Karl Hiramoto karl@hiramoto.org. RTSCTS hardware flow
- * control thanks to Munir Nassar nassarmu@real-time.com
+ * control thanks to Munir Nassar nassarmu@real-time.com.
+ * GPIO support for CP2102N thanks to Karoly Pados.
  *
  */
 
@@ -229,11 +230,16 @@ struct cp210x_serial_private {
 	struct gpio_chip	gc;
 	u8			config;
 	u8			gpio_mode;
+	u8			gpio_control;
+	u8			gpio_dir;
 	bool			gpio_registered;
 #endif
 	u8			partnum;
 };
 
+#define CP2102N_PIN_OPENDRAIN(var, pin)	(!((var) & BIT(pin)))
+#define CP2102N_PIN_GPIO(var, pin)	(!((var) & BIT(pin)))
+
 struct cp210x_port_private {
 	__u8			bInterfaceNumber;
 	bool			has_swapped_line_ctl;
@@ -349,6 +355,7 @@ static struct usb_serial_driver * const serial_drivers[] = {
 #define CP210X_GET_PORTCONFIG	0x370C
 #define CP210X_GET_DEVICEMODE	0x3711
 #define CP210X_WRITE_LATCH	0x37E1
+#define CP210X_READ_2NCONFIG	0x000E
 
 /* Part number definitions */
 #define CP210X_PARTNUM_CP2101	0x01
@@ -362,6 +369,14 @@ static struct usb_serial_driver * const serial_drivers[] = {
 #define CP210X_PARTNUM_CP2102N_QFN20	0x22
 #define CP210X_PARTNUM_UNKNOWN	0xFF
 
+#define	IS_CP2102N(partnum)   (((partnum) == CP210X_PARTNUM_CP2102N_QFN28) || \
+			       ((partnum) == CP210X_PARTNUM_CP2102N_QFN24) || \
+			       ((partnum) == CP210X_PARTNUM_CP2102N_QFN20))
+
+#define CP210X_2NCONFIG_GPIO_CONTROL_IDX	600
+#define CP210X_2NCONFIG_GPIO_MODE_IDX		581
+#define CP210X_2NCONFIG_GPIO_RSTLATCH_IDX	587
+
 /* CP210X_GET_COMM_STATUS returns these 0x13 bytes */
 struct cp210x_comm_status {
 	__le32   ulErrors;
@@ -1455,6 +1470,235 @@ static void cp210x_gpio_remove(struct usb_serial *serial)
 	}
 }
 
+static int cp2102n_gpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct usb_serial *serial = gpiochip_get_data(gc);
+	int result;
+	u8 buf;
+
+	result = cp210x_read_vendor_block(serial, REQTYPE_DEVICE_TO_HOST,
+					  CP210X_READ_LATCH, &buf, sizeof(buf));
+	if (result < 0)
+		return result;
+
+	return !!(buf & BIT(gpio));
+}
+
+static void cp2102n_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
+{
+	int result;
+	struct cp210x_gpio_write buf;
+	struct usb_serial *serial = gpiochip_get_data(gc);
+	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
+
+	/* Ignore request if pin is not in our control */
+	if (!CP2102N_PIN_GPIO(priv->gpio_control, gpio)) {
+		dev_warn(&serial->interface->dev,
+			 "Cannot control GPIO with active alternate function.\n");
+		return;
+	}
+
+	buf.state = (value == 1) ? BIT(gpio) : 0;
+	buf.mask = BIT(gpio);
+
+	result = usb_control_msg(serial->dev,
+				 usb_sndctrlpipe(serial->dev, 0),
+				 CP210X_VENDOR_SPECIFIC,
+				 REQTYPE_HOST_TO_DEVICE,
+				 CP210X_WRITE_LATCH,
+				 *(u16 *)&buf,
+				 NULL, 0, USB_CTRL_SET_TIMEOUT);
+
+	if (result < 0) {
+		dev_err(&serial->interface->dev,
+			"Failed to set GPIO value.\n");
+	}
+}
+
+static int cp2102n_gpio_direction_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);
+
+	return priv->gpio_dir & BIT(gpio);
+}
+
+static int cp2102n_gpio_direction_input(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);
+
+	/* Return an error if pin is not in our control */
+	if (!CP2102N_PIN_GPIO(priv->gpio_control, gpio)) {
+		dev_warn(&serial->interface->dev,
+			 "Cannot control GPIO with active alternate function.\n");
+		return -EPERM;
+	}
+
+	/* Push-pull pins cannot be changed to be inputs */
+	if (!CP2102N_PIN_OPENDRAIN(priv->gpio_mode, 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 */
+	cp2102n_gpio_set(gc, gpio, 1);
+
+	/* Note pin direction to ourselves */
+	priv->gpio_dir |= BIT(gpio);
+
+	return 0;
+}
+
+static int cp2102n_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);
+
+	/* Return an error if pin is not in our control */
+	if (!CP2102N_PIN_GPIO(priv->gpio_control, gpio)) {
+		dev_warn(&serial->interface->dev,
+			 "Cannot control GPIO with active alternate function.\n");
+		return -EPERM;
+	}
+
+	/* Note pin direction to ourselves */
+	priv->gpio_dir &= ~BIT(gpio);
+
+	/* Set requested initial output value */
+	cp2102n_gpio_set(gc, gpio, value);
+
+	return 0;
+}
+
+static u16 fletcher16(u8 *data, u16 bytes)
+{
+	u16 sum1 = 0xff, sum2 = 0xff;
+	u16 tlen;
+
+	while (bytes) {
+		tlen = bytes >= 20 ? 20 : bytes;
+		bytes -= tlen;
+		do {
+			sum2 += sum1 += *data++;
+		} while (--tlen);
+		sum1 = (sum1 & 0xff) + (sum1 >> 8);
+		sum2 = (sum2 & 0xff) + (sum2 >> 8);
+	}
+	/* Second reduction step to reduce sums to 8 bits */
+	sum1 = (sum1 & 0xff) + (sum1 >> 8);
+	sum2 = (sum2 & 0xff) + (sum2 >> 8);
+	return sum2 << 8 | sum1;
+}
+
+static int cp2102n_gpio_init(struct usb_serial *serial)
+{
+	const u16 CONFIG_SIZE = 0x02A6;
+	int result;
+	u16 config_csum;
+	u8 *config_buf;
+	u8 gpio_ctrl;
+	u8 gpio_mode;
+	u8 gpio_latch;
+	u8 gpio_rst_latch;
+	u8 i;
+	bool config_valid = true;
+	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
+
+	/* 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)
+		return result;
+
+	/* Check configuration for validity.
+	 * The last two bytes of the array contain a fletcher16
+	 * checksum of all the bytes before, which we can validate.
+	 */
+	config_csum = fletcher16(config_buf, CONFIG_SIZE - 2);
+	if (((config_csum & 0xFF) != config_buf[CONFIG_SIZE - 1]) ||
+	    ((config_csum >> 8)   != config_buf[CONFIG_SIZE - 2])) {
+		config_valid = false;
+		dev_err(&serial->interface->dev,
+			"Corrupted device configuration received\n");
+	}
+
+	gpio_mode = 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);
+
+	if (!config_valid)
+		return -EIO;
+
+	/* We only support 4 GPIOs even on the QFN28 package because
+	 * documentation about the CP2102N GetConfig Array
+	 * does not seem to correspond to reality on this device.
+	 */
+	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_mode = (gpio_mode >> 3) & 0x0F;
+
+	/* 0 indicates GPIO mode, 1 is alternate function */
+	priv->gpio_control = (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 (CP2102N_PIN_OPENDRAIN(priv->gpio_mode, i) &&
+		    (gpio_latch & BIT(i))) {
+			priv->gpio_dir |= BIT(i);
+		}
+	}
+
+	priv->gc.label = "cp2102n";
+	priv->gc.get_direction = cp2102n_gpio_direction_get;
+	priv->gc.direction_input = cp2102n_gpio_direction_input;
+	priv->gc.direction_output = cp2102n_gpio_direction_output;
+	priv->gc.get = cp2102n_gpio_get;
+	priv->gc.set = cp2102n_gpio_set;
+	priv->gc.owner = THIS_MODULE;
+	priv->gc.parent = &serial->interface->dev;
+	priv->gc.base = -1;
+	priv->gc.can_sleep = true;
+
+	result = gpiochip_add_data(&priv->gc, serial);
+	if (!result)
+		priv->gpio_registered = true;
+
+	return result;
+}
+
 #else
 
 static int cp2105_shared_gpio_init(struct usb_serial *serial)
@@ -1462,6 +1706,11 @@ static int cp2105_shared_gpio_init(struct usb_serial *serial)
 	return 0;
 }
 
+static int cp2102n_gpio_init(struct usb_serial *serial)
+{
+	return 0;
+}
+
 static void cp210x_gpio_remove(struct usb_serial *serial)
 {
 	/* Nothing to do */
@@ -1522,7 +1771,13 @@ static int cp210x_attach(struct usb_serial *serial)
 
 	usb_set_serial_data(serial, priv);
 
-	if (priv->partnum == CP210X_PARTNUM_CP2105) {
+	if (IS_CP2102N(priv->partnum)) {
+		result = cp2102n_gpio_init(serial);
+		if (result < 0) {
+			dev_err(&serial->interface->dev,
+				"GPIO initialisation failed, continuing without GPIO support\n");
+		}
+	} else if (priv->partnum == CP210X_PARTNUM_CP2105) {
 		result = cp2105_shared_gpio_init(serial);
 		if (result < 0) {
 			dev_err(&serial->interface->dev,

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

[Johan Hovold]

On Sun, Jun 17, 2018 at 08:25:03PM +0200, Karoly Pados wrote:
> Pretty much what the title says. No other functions/devices
> touched. Tested on CP2102N-QFN28.
> 
> Limitation: Even though the QFN28 package has 7 GPIOs,
> this patch allows to control only the first 4.
> What I've found using reverse engineering regarding the
> other 3 pins collides both with reality and with other
> documented functions, so I did not dare to just use my
> findings because I cannot test on other packages.
> Instead, I decided to play it safe and only support 4 GPIOs.

Thanks for the patch. Nice to see this finally be implemented for
further device types.

You're gonna need to unify a lot of this with the current implementation
for cp2105 however.

> Signed-off-by: Karoly Pados <pados@pados.hu>
> ---
>  drivers/usb/serial/cp210x.c | 259 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 257 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index 793b86252c46..adb450185ce8 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -6,7 +6,8 @@
>   *
>   * Support to set flow control line levels using TIOCMGET and TIOCMSET
>   * thanks to Karl Hiramoto karl@hiramoto.org. RTSCTS hardware flow
> - * control thanks to Munir Nassar nassarmu@real-time.com
> + * control thanks to Munir Nassar nassarmu@real-time.com.
> + * GPIO support for CP2102N thanks to Karoly Pados.

No need to thank yourself here. This was added by the original author
(who appears to have partly been developing this driver out-of-tree) to
credit contributers.

The git logs will keep a record of your contributions.

>   *
>   */
>  
> @@ -229,11 +230,16 @@ struct cp210x_serial_private {
>  	struct gpio_chip	gc;
>  	u8			config;
>  	u8			gpio_mode;
> +	u8			gpio_control;
> +	u8			gpio_dir;
>  	bool			gpio_registered;
>  #endif
>  	u8			partnum;
>  };
>  
> +#define CP2102N_PIN_OPENDRAIN(var, pin)	(!((var) & BIT(pin)))
> +#define CP2102N_PIN_GPIO(var, pin)	(!((var) & BIT(pin)))

These are in fact not cp2102n specific. If they are at all needed in the
end they should be implemented as generic static functions.

> +
>  struct cp210x_port_private {
>  	__u8			bInterfaceNumber;
>  	bool			has_swapped_line_ctl;
> @@ -349,6 +355,7 @@ static struct usb_serial_driver * const serial_drivers[] = {
>  #define CP210X_GET_PORTCONFIG	0x370C
>  #define CP210X_GET_DEVICEMODE	0x3711
>  #define CP210X_WRITE_LATCH	0x37E1
> +#define CP210X_READ_2NCONFIG	0x000E

Keep the defines sorted by value.

>  /* Part number definitions */
>  #define CP210X_PARTNUM_CP2101	0x01
> @@ -362,6 +369,14 @@ static struct usb_serial_driver * const serial_drivers[] = {
>  #define CP210X_PARTNUM_CP2102N_QFN20	0x22
>  #define CP210X_PARTNUM_UNKNOWN	0xFF
>  
> +#define	IS_CP2102N(partnum)   (((partnum) == CP210X_PARTNUM_CP2102N_QFN28) || \
> +			       ((partnum) == CP210X_PARTNUM_CP2102N_QFN24) || \
> +			       ((partnum) == CP210X_PARTNUM_CP2102N_QFN20))

So reuse the static helper I mentioned in my comments to the baudrate
patch.

> +
> +#define CP210X_2NCONFIG_GPIO_CONTROL_IDX	600
> +#define CP210X_2NCONFIG_GPIO_MODE_IDX		581
> +#define CP210X_2NCONFIG_GPIO_RSTLATCH_IDX	587

Sort by value.

> +
>  /* CP210X_GET_COMM_STATUS returns these 0x13 bytes */
>  struct cp210x_comm_status {
>  	__le32   ulErrors;
> @@ -1455,6 +1470,235 @@ static void cp210x_gpio_remove(struct usb_serial *serial)
>  	}
>  }
>  
> +static int cp2102n_gpio_get(struct gpio_chip *gc, unsigned int gpio)
> +{
> +	struct usb_serial *serial = gpiochip_get_data(gc);
> +	int result;
> +	u8 buf;
> +
> +	result = cp210x_read_vendor_block(serial, REQTYPE_DEVICE_TO_HOST,
> +					  CP210X_READ_LATCH, &buf, sizeof(buf));
> +	if (result < 0)
> +		return result;
> +
> +	return !!(buf & BIT(gpio));
> +}

This should be handled in cp210x_gpio_get() by making the control
request type depend on the device type.

Make the cp2102n behaviour the default as it is used also by some legacy
devices, and explicitly check for the cp2105 odd bird. We'll deal with
cp2108, which 16 gpios and hence uses 16-bit mask, when we get there.

> +
> +static void cp2102n_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
> +{
> +	int result;
> +	struct cp210x_gpio_write buf;
> +	struct usb_serial *serial = gpiochip_get_data(gc);
> +	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
> +
> +	/* Ignore request if pin is not in our control */
> +	if (!CP2102N_PIN_GPIO(priv->gpio_control, gpio)) {
> +		dev_warn(&serial->interface->dev,
> +			 "Cannot control GPIO with active alternate function.\n");
> +		return;
> +	}

No need to check for this in every callback; move to
cp210x_gpio_request().

> +
> +	buf.state = (value == 1) ? BIT(gpio) : 0;
> +	buf.mask = BIT(gpio);
> +
> +	result = usb_control_msg(serial->dev,
> +				 usb_sndctrlpipe(serial->dev, 0),
> +				 CP210X_VENDOR_SPECIFIC,
> +				 REQTYPE_HOST_TO_DEVICE,
> +				 CP210X_WRITE_LATCH,
> +				 *(u16 *)&buf,

AFAICT this will work also on big-endian machines, but the implicit
endian conversions involved makes my head spin. ;)

It may be better to just encode the masks explicitly for the default
(non-cp2105) case that use wIndex:

	buf.state << 8 | buf.mask

when merging this with cp210x_gpio_set.

> +				 NULL, 0, USB_CTRL_SET_TIMEOUT);
> +
> +	if (result < 0) {
> +		dev_err(&serial->interface->dev,
> +			"Failed to set GPIO value.\n");
> +	}
> +}
> +
> +static int cp2102n_gpio_direction_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);
> +
> +	return priv->gpio_dir & BIT(gpio);
> +}

So for cp2105 we decided against implementing an input mode. We at least
need to be consistent here, so I suggest starting with dropping those
bits of the implementation and we can have that discussion again and,
depending on the outcome, possibly enable it for all cp210x devices
later.

> +static u16 fletcher16(u8 *data, u16 bytes)
> +{
> +	u16 sum1 = 0xff, sum2 = 0xff;
> +	u16 tlen;
> +
> +	while (bytes) {
> +		tlen = bytes >= 20 ? 20 : bytes;
> +		bytes -= tlen;
> +		do {
> +			sum2 += sum1 += *data++;
> +		} while (--tlen);
> +		sum1 = (sum1 & 0xff) + (sum1 >> 8);
> +		sum2 = (sum2 & 0xff) + (sum2 >> 8);
> +	}
> +	/* Second reduction step to reduce sums to 8 bits */
> +	sum1 = (sum1 & 0xff) + (sum1 >> 8);
> +	sum2 = (sum2 & 0xff) + (sum2 >> 8);
> +	return sum2 << 8 | sum1;
> +}

I was going to comment on the odd coding style above, when I noticed
that you've copied this implementation from a silabs forum post. Not
good at all (as there was no indication of any license).

Please remove.

Fortunately, the below checksum is redundant so no need for you to
reimplement this yourself.

> +
> +static int cp2102n_gpio_init(struct usb_serial *serial)
> +{
> +	const u16 CONFIG_SIZE = 0x02A6;
> +	int result;
> +	u16 config_csum;
> +	u8 *config_buf;
> +	u8 gpio_ctrl;
> +	u8 gpio_mode;
> +	u8 gpio_latch;
> +	u8 gpio_rst_latch;
> +	u8 i;
> +	bool config_valid = true;
> +	struct cp210x_serial_private *priv = usb_get_serial_data(serial);

Style nit: Move longer declarations towards the top of the function
(reverse xmas style).

> +
> +	/* 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 comments should use the following format

	/*
	 * blah...
	 */

> +	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)
> +		return result;
> +
> +	/* Check configuration for validity.
> +	 * The last two bytes of the array contain a fletcher16
> +	 * checksum of all the bytes before, which we can validate.
> +	 */
> +	config_csum = fletcher16(config_buf, CONFIG_SIZE - 2);
> +	if (((config_csum & 0xFF) != config_buf[CONFIG_SIZE - 1]) ||
> +	    ((config_csum >> 8)   != config_buf[CONFIG_SIZE - 2])) {
> +		config_valid = false;
> +		dev_err(&serial->interface->dev,
> +			"Corrupted device configuration received\n");
> +	}

So just drop this for now.

> +
> +	gpio_mode = 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);
> +
> +	if (!config_valid)
> +		return -EIO;
> +
> +	/* We only support 4 GPIOs even on the QFN28 package because
> +	 * documentation about the CP2102N GetConfig Array
> +	 * does not seem to correspond to reality on this device.
> +	 */

Please be more specific here; what do you mean by not corresponding to
reality.

> +	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_mode = (gpio_mode >> 3) & 0x0F;
> +
> +	/* 0 indicates GPIO mode, 1 is alternate function */
> +	priv->gpio_control = (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 (CP2102N_PIN_OPENDRAIN(priv->gpio_mode, i) &&
> +		    (gpio_latch & BIT(i))) {
> +			priv->gpio_dir |= BIT(i);
> +		}
> +	}

So this can be simplified a bit by deferring implementing an input mode.

> +
> +	priv->gc.label = "cp2102n";
> +	priv->gc.get_direction = cp2102n_gpio_direction_get;
> +	priv->gc.direction_input = cp2102n_gpio_direction_input;
> +	priv->gc.direction_output = cp2102n_gpio_direction_output;
> +	priv->gc.get = cp2102n_gpio_get;
> +	priv->gc.set = cp2102n_gpio_set;
> +	priv->gc.owner = THIS_MODULE;
> +	priv->gc.parent = &serial->interface->dev;
> +	priv->gc.base = -1;
> +	priv->gc.can_sleep = true;
> +
> +	result = gpiochip_add_data(&priv->gc, serial);
> +	if (!result)
> +		priv->gpio_registered = true;

And this whole function should be merged with the cp2105 implementation.
Retrieving and parsing the configuration is obviously going to remain
type specific (and live in their own functions), but the above
initialisation and registration can be shared.

> +
> +	return result;
> +}
> +
>  #else
>  
>  static int cp2105_shared_gpio_init(struct usb_serial *serial)
> @@ -1462,6 +1706,11 @@ static int cp2105_shared_gpio_init(struct usb_serial *serial)
>  	return 0;
>  }
>  
> +static int cp2102n_gpio_init(struct usb_serial *serial)
> +{
> +	return 0;
> +}
> +
>  static void cp210x_gpio_remove(struct usb_serial *serial)
>  {
>  	/* Nothing to do */
> @@ -1522,7 +1771,13 @@ static int cp210x_attach(struct usb_serial *serial)
>  
>  	usb_set_serial_data(serial, priv);
>  
> -	if (priv->partnum == CP210X_PARTNUM_CP2105) {
> +	if (IS_CP2102N(priv->partnum)) {
> +		result = cp2102n_gpio_init(serial);
> +		if (result < 0) {
> +			dev_err(&serial->interface->dev,
> +				"GPIO initialisation failed, continuing without GPIO support\n");
> +		}
> +	} else if (priv->partnum == CP210X_PARTNUM_CP2105) {
>  		result = cp2105_shared_gpio_init(serial);
>  		if (result < 0) {
>  			dev_err(&serial->interface->dev,

And this all should be hidden away in a common cp210x_gpio_init()
function.

Thanks,
Johan

USB: serial: cp210x: Implement GPIO support for CP2102N

[Johan Hovold]

On Sun, Jun 17, 2018 at 08:25:03PM +0200, Karoly Pados wrote:
> Pretty much what the title says. No other functions/devices
> touched. Tested on CP2102N-QFN28.
> 
> Limitation: Even though the QFN28 package has 7 GPIOs,
> this patch allows to control only the first 4.
> What I've found using reverse engineering regarding the
> other 3 pins collides both with reality and with other
> documented functions, so I did not dare to just use my
> findings because I cannot test on other packages.
> Instead, I decided to play it safe and only support 4 GPIOs.

Thanks for the patch. Nice to see this finally be implemented for
further device types.

You're gonna need to unify a lot of this with the current implementation
for cp2105 however.

> Signed-off-by: Karoly Pados <pados@pados.hu>
> ---
>  drivers/usb/serial/cp210x.c | 259 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 257 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index 793b86252c46..adb450185ce8 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -6,7 +6,8 @@
>   *
>   * Support to set flow control line levels using TIOCMGET and TIOCMSET
>   * thanks to Karl Hiramoto karl@hiramoto.org. RTSCTS hardware flow
> - * control thanks to Munir Nassar nassarmu@real-time.com
> + * control thanks to Munir Nassar nassarmu@real-time.com.
> + * GPIO support for CP2102N thanks to Karoly Pados.

No need to thank yourself here. This was added by the original author
(who appears to have partly been developing this driver out-of-tree) to
credit contributers.

The git logs will keep a record of your contributions.

>   *
>   */
>  
> @@ -229,11 +230,16 @@ struct cp210x_serial_private {
>  	struct gpio_chip	gc;
>  	u8			config;
>  	u8			gpio_mode;
> +	u8			gpio_control;
> +	u8			gpio_dir;
>  	bool			gpio_registered;
>  #endif
>  	u8			partnum;
>  };
>  
> +#define CP2102N_PIN_OPENDRAIN(var, pin)	(!((var) & BIT(pin)))
> +#define CP2102N_PIN_GPIO(var, pin)	(!((var) & BIT(pin)))

These are in fact not cp2102n specific. If they are at all needed in the
end they should be implemented as generic static functions.

> +
>  struct cp210x_port_private {
>  	__u8			bInterfaceNumber;
>  	bool			has_swapped_line_ctl;
> @@ -349,6 +355,7 @@ static struct usb_serial_driver * const serial_drivers[] = {
>  #define CP210X_GET_PORTCONFIG	0x370C
>  #define CP210X_GET_DEVICEMODE	0x3711
>  #define CP210X_WRITE_LATCH	0x37E1
> +#define CP210X_READ_2NCONFIG	0x000E

Keep the defines sorted by value.

>  /* Part number definitions */
>  #define CP210X_PARTNUM_CP2101	0x01
> @@ -362,6 +369,14 @@ static struct usb_serial_driver * const serial_drivers[] = {
>  #define CP210X_PARTNUM_CP2102N_QFN20	0x22
>  #define CP210X_PARTNUM_UNKNOWN	0xFF
>  
> +#define	IS_CP2102N(partnum)   (((partnum) == CP210X_PARTNUM_CP2102N_QFN28) || \
> +			       ((partnum) == CP210X_PARTNUM_CP2102N_QFN24) || \
> +			       ((partnum) == CP210X_PARTNUM_CP2102N_QFN20))

So reuse the static helper I mentioned in my comments to the baudrate
patch.

> +
> +#define CP210X_2NCONFIG_GPIO_CONTROL_IDX	600
> +#define CP210X_2NCONFIG_GPIO_MODE_IDX		581
> +#define CP210X_2NCONFIG_GPIO_RSTLATCH_IDX	587

Sort by value.

> +
>  /* CP210X_GET_COMM_STATUS returns these 0x13 bytes */
>  struct cp210x_comm_status {
>  	__le32   ulErrors;
> @@ -1455,6 +1470,235 @@ static void cp210x_gpio_remove(struct usb_serial *serial)
>  	}
>  }
>  
> +static int cp2102n_gpio_get(struct gpio_chip *gc, unsigned int gpio)
> +{
> +	struct usb_serial *serial = gpiochip_get_data(gc);
> +	int result;
> +	u8 buf;
> +
> +	result = cp210x_read_vendor_block(serial, REQTYPE_DEVICE_TO_HOST,
> +					  CP210X_READ_LATCH, &buf, sizeof(buf));
> +	if (result < 0)
> +		return result;
> +
> +	return !!(buf & BIT(gpio));
> +}

This should be handled in cp210x_gpio_get() by making the control
request type depend on the device type.

Make the cp2102n behaviour the default as it is used also by some legacy
devices, and explicitly check for the cp2105 odd bird. We'll deal with
cp2108, which 16 gpios and hence uses 16-bit mask, when we get there.

> +
> +static void cp2102n_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
> +{
> +	int result;
> +	struct cp210x_gpio_write buf;
> +	struct usb_serial *serial = gpiochip_get_data(gc);
> +	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
> +
> +	/* Ignore request if pin is not in our control */
> +	if (!CP2102N_PIN_GPIO(priv->gpio_control, gpio)) {
> +		dev_warn(&serial->interface->dev,
> +			 "Cannot control GPIO with active alternate function.\n");
> +		return;
> +	}

No need to check for this in every callback; move to
cp210x_gpio_request().

> +
> +	buf.state = (value == 1) ? BIT(gpio) : 0;
> +	buf.mask = BIT(gpio);
> +
> +	result = usb_control_msg(serial->dev,
> +				 usb_sndctrlpipe(serial->dev, 0),
> +				 CP210X_VENDOR_SPECIFIC,
> +				 REQTYPE_HOST_TO_DEVICE,
> +				 CP210X_WRITE_LATCH,
> +				 *(u16 *)&buf,

AFAICT this will work also on big-endian machines, but the implicit
endian conversions involved makes my head spin. ;)

It may be better to just encode the masks explicitly for the default
(non-cp2105) case that use wIndex:

	buf.state << 8 | buf.mask

when merging this with cp210x_gpio_set.

> +				 NULL, 0, USB_CTRL_SET_TIMEOUT);
> +
> +	if (result < 0) {
> +		dev_err(&serial->interface->dev,
> +			"Failed to set GPIO value.\n");
> +	}
> +}
> +
> +static int cp2102n_gpio_direction_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);
> +
> +	return priv->gpio_dir & BIT(gpio);
> +}

So for cp2105 we decided against implementing an input mode. We at least
need to be consistent here, so I suggest starting with dropping those
bits of the implementation and we can have that discussion again and,
depending on the outcome, possibly enable it for all cp210x devices
later.

> +static u16 fletcher16(u8 *data, u16 bytes)
> +{
> +	u16 sum1 = 0xff, sum2 = 0xff;
> +	u16 tlen;
> +
> +	while (bytes) {
> +		tlen = bytes >= 20 ? 20 : bytes;
> +		bytes -= tlen;
> +		do {
> +			sum2 += sum1 += *data++;
> +		} while (--tlen);
> +		sum1 = (sum1 & 0xff) + (sum1 >> 8);
> +		sum2 = (sum2 & 0xff) + (sum2 >> 8);
> +	}
> +	/* Second reduction step to reduce sums to 8 bits */
> +	sum1 = (sum1 & 0xff) + (sum1 >> 8);
> +	sum2 = (sum2 & 0xff) + (sum2 >> 8);
> +	return sum2 << 8 | sum1;
> +}

I was going to comment on the odd coding style above, when I noticed
that you've copied this implementation from a silabs forum post. Not
good at all (as there was no indication of any license).

Please remove.

Fortunately, the below checksum is redundant so no need for you to
reimplement this yourself.

> +
> +static int cp2102n_gpio_init(struct usb_serial *serial)
> +{
> +	const u16 CONFIG_SIZE = 0x02A6;
> +	int result;
> +	u16 config_csum;
> +	u8 *config_buf;
> +	u8 gpio_ctrl;
> +	u8 gpio_mode;
> +	u8 gpio_latch;
> +	u8 gpio_rst_latch;
> +	u8 i;
> +	bool config_valid = true;
> +	struct cp210x_serial_private *priv = usb_get_serial_data(serial);

Style nit: Move longer declarations towards the top of the function
(reverse xmas style).

> +
> +	/* 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 comments should use the following format

	/*
	 * blah...
	 */

> +	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)
> +		return result;
> +
> +	/* Check configuration for validity.
> +	 * The last two bytes of the array contain a fletcher16
> +	 * checksum of all the bytes before, which we can validate.
> +	 */
> +	config_csum = fletcher16(config_buf, CONFIG_SIZE - 2);
> +	if (((config_csum & 0xFF) != config_buf[CONFIG_SIZE - 1]) ||
> +	    ((config_csum >> 8)   != config_buf[CONFIG_SIZE - 2])) {
> +		config_valid = false;
> +		dev_err(&serial->interface->dev,
> +			"Corrupted device configuration received\n");
> +	}

So just drop this for now.

> +
> +	gpio_mode = 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);
> +
> +	if (!config_valid)
> +		return -EIO;
> +
> +	/* We only support 4 GPIOs even on the QFN28 package because
> +	 * documentation about the CP2102N GetConfig Array
> +	 * does not seem to correspond to reality on this device.
> +	 */

Please be more specific here; what do you mean by not corresponding to
reality.

> +	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_mode = (gpio_mode >> 3) & 0x0F;
> +
> +	/* 0 indicates GPIO mode, 1 is alternate function */
> +	priv->gpio_control = (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 (CP2102N_PIN_OPENDRAIN(priv->gpio_mode, i) &&
> +		    (gpio_latch & BIT(i))) {
> +			priv->gpio_dir |= BIT(i);
> +		}
> +	}

So this can be simplified a bit by deferring implementing an input mode.

> +
> +	priv->gc.label = "cp2102n";
> +	priv->gc.get_direction = cp2102n_gpio_direction_get;
> +	priv->gc.direction_input = cp2102n_gpio_direction_input;
> +	priv->gc.direction_output = cp2102n_gpio_direction_output;
> +	priv->gc.get = cp2102n_gpio_get;
> +	priv->gc.set = cp2102n_gpio_set;
> +	priv->gc.owner = THIS_MODULE;
> +	priv->gc.parent = &serial->interface->dev;
> +	priv->gc.base = -1;
> +	priv->gc.can_sleep = true;
> +
> +	result = gpiochip_add_data(&priv->gc, serial);
> +	if (!result)
> +		priv->gpio_registered = true;

And this whole function should be merged with the cp2105 implementation.
Retrieving and parsing the configuration is obviously going to remain
type specific (and live in their own functions), but the above
initialisation and registration can be shared.

> +
> +	return result;
> +}
> +
>  #else
>  
>  static int cp2105_shared_gpio_init(struct usb_serial *serial)
> @@ -1462,6 +1706,11 @@ static int cp2105_shared_gpio_init(struct usb_serial *serial)
>  	return 0;
>  }
>  
> +static int cp2102n_gpio_init(struct usb_serial *serial)
> +{
> +	return 0;
> +}
> +
>  static void cp210x_gpio_remove(struct usb_serial *serial)
>  {
>  	/* Nothing to do */
> @@ -1522,7 +1771,13 @@ static int cp210x_attach(struct usb_serial *serial)
>  
>  	usb_set_serial_data(serial, priv);
>  
> -	if (priv->partnum == CP210X_PARTNUM_CP2105) {
> +	if (IS_CP2102N(priv->partnum)) {
> +		result = cp2102n_gpio_init(serial);
> +		if (result < 0) {
> +			dev_err(&serial->interface->dev,
> +				"GPIO initialisation failed, continuing without GPIO support\n");
> +		}
> +	} else if (priv->partnum == CP210X_PARTNUM_CP2105) {
>  		result = cp2105_shared_gpio_init(serial);
>  		if (result < 0) {
>  			dev_err(&serial->interface->dev,

And this all should be hidden away in a common cp210x_gpio_init()
function.

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] USB: serial: cp210x: Implement GPIO support for CP2102N

[Karoly Pados]

OK, though if you allow me a few comments and explanations:


> You're gonna need to unify a lot of this with the current implementation
> for cp2105 however.
>
This also relates to your later points about unifying the cp2105 an cp2102n:
The implementation for the cp2102n ended up pretty different. Both in initializaiton
as well as normal operation, partially due to the better config/input/alt.function
support for the cp2102n. At this point I though it is much more readable if I just create
separate code hierarchies than having to pollute everything with ifs and switch-cases.

And maybe most importantly, I cannot test on the cp2105, so I cannot check if I've broken
anything by mistake, and since it is the odd bird anyway among most devices, 
I though it is pretty logical not to touch its code but instead create a new skeleton for
others.

Of course you're the boss and whatever you say I will do, but I wanted to explain my reasons
(or at least let you know my arguments).

> I was going to comment on the odd coding style above, when I noticed
> that you've copied this implementation from a silabs forum post. Not
> good at all (as there was no indication of any license).
> 
Not quite. It is not from a forum post, but from a SiLabs Knowledge Base article
(https://www.silabs.com/community/interface/knowledge-base.entry.html/2017/06/12/fletcher_checksumfo-TeDF)
That article states explicitly that the code was taken from Wikipedia, so it is CC-SA, which is to
the best of my knowledge 1) compatible with GPL, and 2) does not require attribution if the original material
is missing it, and it does. So AFAICT we are clear on the licensing front.

Do you still want me to remove the checksum check?

> So for cp2105 we decided against implementing an input mode. We at least
> need to be consistent here, so I suggest starting with dropping those
>
Again, you're the boss, but I do find this odd. These are different devices and thankfully now
there'd be better support for the cp2102n, and you're asking me to remove input support for
consistency reasons. To remove a feature for one device just because it wasn't correctly implemented
in another and wanting to stay consistent with the missing support is very odd for me tbh.
I know I'm in no position, but please let me ask you once to please reconsider this.

The way it was done for the cp2105 where we make everything and everyone on the system
think that a pin is "out" but in reality is an input is quirky to say the least, while
at the same time it also does not make sure that an input pin (which is of course not marked as such)
is really an input.  Why amputate the cp2102n like this if it has been already implemented correctly?
If we wanted to stay consistent with half-developed and incomplete drivers then there'd be no
improvements to Linux. 

> Please be more specific here; what do you mean by not corresponding to
> reality.
>
It means that I've found GPIO4-6 occupying bits which were documented doing completely other things,
and those other things occupying reserved bits even though they were documented occupying the places
where GPIOs 4-6 were.

> No need to check for this in every callback; move to
> cp210x_gpio_request().
>
Wouldn't this prevent the disabled GPIOs from being created in the first place?
I very much want to create them in every case, because otherwise it becomes a PITA 
for both the driver and consumers:
- For the driver a PITA, because the disabled GPIOs can be any and non-continuous,
so the module would need to dynamically and arbitrarily remap logical GPIOs to hardware GPIO numbers.
- For consumers a PITA, because it would mean GPIOs can change names depending on the current configuration
of the device. On one device GPIO.3 would be called GPIO.2, on another GPIO.1 etc. My intention
here is that applications should be able to use the real ID of the GPIO and expect that GPIO.3 (base+offset 3)
is actually GPIO.3 and not something else.

Best,
Karoly

June 20, 2018 10:25 AM, "Johan Hovold" <johan@kernel.org> wrote:

> On Sun, Jun 17, 2018 at 08:25:03PM +0200, Karoly Pados wrote:
> 
>> Pretty much what the title says. No other functions/devices
>> touched. Tested on CP2102N-QFN28.
>> 
>> Limitation: Even though the QFN28 package has 7 GPIOs,
>> this patch allows to control only the first 4.
>> What I've found using reverse engineering regarding the
>> other 3 pins collides both with reality and with other
>> documented functions, so I did not dare to just use my
>> findings because I cannot test on other packages.
>> Instead, I decided to play it safe and only support 4 GPIOs.
> 
> Thanks for the patch. Nice to see this finally be implemented for
> further device types.
> 
> You're gonna need to unify a lot of this with the current implementation
> for cp2105 however.
> 
>> Signed-off-by: Karoly Pados <pados@pados.hu>
>> ---
>> drivers/usb/serial/cp210x.c | 259 +++++++++++++++++++++++++++++++++++-
>> 1 file changed, 257 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
>> index 793b86252c46..adb450185ce8 100644
>> --- a/drivers/usb/serial/cp210x.c
>> +++ b/drivers/usb/serial/cp210x.c
>> @@ -6,7 +6,8 @@
>> *
>> * Support to set flow control line levels using TIOCMGET and TIOCMSET
>> * thanks to Karl Hiramoto karl@hiramoto.org. RTSCTS hardware flow
>> - * control thanks to Munir Nassar nassarmu@real-time.com
>> + * control thanks to Munir Nassar nassarmu@real-time.com.
>> + * GPIO support for CP2102N thanks to Karoly Pados.
> 
> No need to thank yourself here. This was added by the original author
> (who appears to have partly been developing this driver out-of-tree) to
> credit contributers.
> 
> The git logs will keep a record of your contributions.
> 
>> *
>> */
>> 
>> @@ -229,11 +230,16 @@ struct cp210x_serial_private {
>> struct gpio_chip gc;
>> u8 config;
>> u8 gpio_mode;
>> + u8 gpio_control;
>> + u8 gpio_dir;
>> bool gpio_registered;
>> #endif
>> u8 partnum;
>> };
>> 
>> +#define CP2102N_PIN_OPENDRAIN(var, pin) (!((var) & BIT(pin)))
>> +#define CP2102N_PIN_GPIO(var, pin) (!((var) & BIT(pin)))
> 
> These are in fact not cp2102n specific. If they are at all needed in the
> end they should be implemented as generic static functions.
> 
>> +
>> struct cp210x_port_private {
>> __u8 bInterfaceNumber;
>> bool has_swapped_line_ctl;
>> @@ -349,6 +355,7 @@ static struct usb_serial_driver * const serial_drivers[] = {
>> #define CP210X_GET_PORTCONFIG 0x370C
>> #define CP210X_GET_DEVICEMODE 0x3711
>> #define CP210X_WRITE_LATCH 0x37E1
>> +#define CP210X_READ_2NCONFIG 0x000E
> 
> Keep the defines sorted by value.
> 
>> /* Part number definitions */
>> #define CP210X_PARTNUM_CP2101 0x01
>> @@ -362,6 +369,14 @@ static struct usb_serial_driver * const serial_drivers[] = {
>> #define CP210X_PARTNUM_CP2102N_QFN20 0x22
>> #define CP210X_PARTNUM_UNKNOWN 0xFF
>> 
>> +#define IS_CP2102N(partnum) (((partnum) == CP210X_PARTNUM_CP2102N_QFN28) || \
>> + ((partnum) == CP210X_PARTNUM_CP2102N_QFN24) || \
>> + ((partnum) == CP210X_PARTNUM_CP2102N_QFN20))
> 
> So reuse the static helper I mentioned in my comments to the baudrate
> patch.
> 
>> +
>> +#define CP210X_2NCONFIG_GPIO_CONTROL_IDX 600
>> +#define CP210X_2NCONFIG_GPIO_MODE_IDX 581
>> +#define CP210X_2NCONFIG_GPIO_RSTLATCH_IDX 587
> 
> Sort by value.
> 
>> +
>> /* CP210X_GET_COMM_STATUS returns these 0x13 bytes */
>> struct cp210x_comm_status {
>> __le32 ulErrors;
>> @@ -1455,6 +1470,235 @@ static void cp210x_gpio_remove(struct usb_serial *serial)
>> }
>> }
>> 
>> +static int cp2102n_gpio_get(struct gpio_chip *gc, unsigned int gpio)
>> +{
>> + struct usb_serial *serial = gpiochip_get_data(gc);
>> + int result;
>> + u8 buf;
>> +
>> + result = cp210x_read_vendor_block(serial, REQTYPE_DEVICE_TO_HOST,
>> + CP210X_READ_LATCH, &buf, sizeof(buf));
>> + if (result < 0)
>> + return result;
>> +
>> + return !!(buf & BIT(gpio));
>> +}
> 
> This should be handled in cp210x_gpio_get() by making the control
> request type depend on the device type.
> 
> Make the cp2102n behaviour the default as it is used also by some legacy
> devices, and explicitly check for the cp2105 odd bird. We'll deal with
> cp2108, which 16 gpios and hence uses 16-bit mask, when we get there.
> 
>> +
>> +static void cp2102n_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
>> +{
>> + int result;
>> + struct cp210x_gpio_write buf;
>> + struct usb_serial *serial = gpiochip_get_data(gc);
>> + struct cp210x_serial_private *priv = usb_get_serial_data(serial);
>> +
>> + /* Ignore request if pin is not in our control */
>> + if (!CP2102N_PIN_GPIO(priv->gpio_control, gpio)) {
>> + dev_warn(&serial->interface->dev,
>> + "Cannot control GPIO with active alternate function.\n");
>> + return;
>> + }
> 
> No need to check for this in every callback; move to
> cp210x_gpio_request().
> 
>> +
>> + buf.state = (value == 1) ? BIT(gpio) : 0;
>> + buf.mask = BIT(gpio);
>> +
>> + result = usb_control_msg(serial->dev,
>> + usb_sndctrlpipe(serial->dev, 0),
>> + CP210X_VENDOR_SPECIFIC,
>> + REQTYPE_HOST_TO_DEVICE,
>> + CP210X_WRITE_LATCH,
>> + *(u16 *)&buf,
> 
> AFAICT this will work also on big-endian machines, but the implicit
> endian conversions involved makes my head spin. ;)
> 
> It may be better to just encode the masks explicitly for the default
> (non-cp2105) case that use wIndex:
> 
> buf.state << 8 | buf.mask
> 
> when merging this with cp210x_gpio_set.
> 
>> + NULL, 0, USB_CTRL_SET_TIMEOUT);
>> +
>> + if (result < 0) {
>> + dev_err(&serial->interface->dev,
>> + "Failed to set GPIO value.\n");
>> + }
>> +}
>> +
>> +static int cp2102n_gpio_direction_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);
>> +
>> + return priv->gpio_dir & BIT(gpio);
>> +}
> 
> So for cp2105 we decided against implementing an input mode. We at least
> need to be consistent here, so I suggest starting with dropping those
> bits of the implementation and we can have that discussion again and,
> depending on the outcome, possibly enable it for all cp210x devices
> later.
> 
>> +static u16 fletcher16(u8 *data, u16 bytes)
>> +{
>> + u16 sum1 = 0xff, sum2 = 0xff;
>> + u16 tlen;
>> +
>> + while (bytes) {
>> + tlen = bytes >= 20 ? 20 : bytes;
>> + bytes -= tlen;
>> + do {
>> + sum2 += sum1 += *data++;
>> + } while (--tlen);
>> + sum1 = (sum1 & 0xff) + (sum1 >> 8);
>> + sum2 = (sum2 & 0xff) + (sum2 >> 8);
>> + }
>> + /* Second reduction step to reduce sums to 8 bits */
>> + sum1 = (sum1 & 0xff) + (sum1 >> 8);
>> + sum2 = (sum2 & 0xff) + (sum2 >> 8);
>> + return sum2 << 8 | sum1;
>> +}
> 
> I was going to comment on the odd coding style above, when I noticed
> that you've copied this implementation from a silabs forum post. Not
> good at all (as there was no indication of any license).
> 
> Please remove.
> 
> Fortunately, the below checksum is redundant so no need for you to
> reimplement this yourself.
> 
>> +
>> +static int cp2102n_gpio_init(struct usb_serial *serial)
>> +{
>> + const u16 CONFIG_SIZE = 0x02A6;
>> + int result;
>> + u16 config_csum;
>> + u8 *config_buf;
>> + u8 gpio_ctrl;
>> + u8 gpio_mode;
>> + u8 gpio_latch;
>> + u8 gpio_rst_latch;
>> + u8 i;
>> + bool config_valid = true;
>> + struct cp210x_serial_private *priv = usb_get_serial_data(serial);
> 
> Style nit: Move longer declarations towards the top of the function
> (reverse xmas style).
> 
>> +
>> + /* 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
>> + */
> 
> Multi-line comments should use the following format
> 
> /*
> * blah...
> */
> 
>> + 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)
>> + return result;
>> +
>> + /* Check configuration for validity.
>> + * The last two bytes of the array contain a fletcher16
>> + * checksum of all the bytes before, which we can validate.
>> + */
>> + config_csum = fletcher16(config_buf, CONFIG_SIZE - 2);
>> + if (((config_csum & 0xFF) != config_buf[CONFIG_SIZE - 1]) ||
>> + ((config_csum >> 8) != config_buf[CONFIG_SIZE - 2])) {
>> + config_valid = false;
>> + dev_err(&serial->interface->dev,
>> + "Corrupted device configuration received\n");
>> + }
> 
> So just drop this for now.
> 
>> +
>> + gpio_mode = 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);
>> +
>> + if (!config_valid)
>> + return -EIO;
>> +
>> + /* We only support 4 GPIOs even on the QFN28 package because
>> + * documentation about the CP2102N GetConfig Array
>> + * does not seem to correspond to reality on this device.
>> + */
> 
> Please be more specific here; what do you mean by not corresponding to
> reality.
> 
>> + 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_mode = (gpio_mode >> 3) & 0x0F;
>> +
>> + /* 0 indicates GPIO mode, 1 is alternate function */
>> + priv->gpio_control = (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 (CP2102N_PIN_OPENDRAIN(priv->gpio_mode, i) &&
>> + (gpio_latch & BIT(i))) {
>> + priv->gpio_dir |= BIT(i);
>> + }
>> + }
> 
> So this can be simplified a bit by deferring implementing an input mode.
> 
>> +
>> + priv->gc.label = "cp2102n";
>> + priv->gc.get_direction = cp2102n_gpio_direction_get;
>> + priv->gc.direction_input = cp2102n_gpio_direction_input;
>> + priv->gc.direction_output = cp2102n_gpio_direction_output;
>> + priv->gc.get = cp2102n_gpio_get;
>> + priv->gc.set = cp2102n_gpio_set;
>> + priv->gc.owner = THIS_MODULE;
>> + priv->gc.parent = &serial->interface->dev;
>> + priv->gc.base = -1;
>> + priv->gc.can_sleep = true;
>> +
>> + result = gpiochip_add_data(&priv->gc, serial);
>> + if (!result)
>> + priv->gpio_registered = true;
> 
> And this whole function should be merged with the cp2105 implementation.
> Retrieving and parsing the configuration is obviously going to remain
> type specific (and live in their own functions), but the above
> initialisation and registration can be shared.
> 
>> +
>> + return result;
>> +}
>> +
>> #else
>> 
>> static int cp2105_shared_gpio_init(struct usb_serial *serial)
>> @@ -1462,6 +1706,11 @@ static int cp2105_shared_gpio_init(struct usb_serial *serial)
>> return 0;
>> }
>> 
>> +static int cp2102n_gpio_init(struct usb_serial *serial)
>> +{
>> + return 0;
>> +}
>> +
>> static void cp210x_gpio_remove(struct usb_serial *serial)
>> {
>> /* Nothing to do */
>> @@ -1522,7 +1771,13 @@ static int cp210x_attach(struct usb_serial *serial)
>> 
>> usb_set_serial_data(serial, priv);
>> 
>> - if (priv->partnum == CP210X_PARTNUM_CP2105) {
>> + if (IS_CP2102N(priv->partnum)) {
>> + result = cp2102n_gpio_init(serial);
>> + if (result < 0) {
>> + dev_err(&serial->interface->dev,
>> + "GPIO initialisation failed, continuing without GPIO support\n");
>> + }
>> + } else if (priv->partnum == CP210X_PARTNUM_CP2105) {
>> result = cp2105_shared_gpio_init(serial);
>> if (result < 0) {
>> dev_err(&serial->interface->dev,
> 
> And this all should be hidden away in a common cp210x_gpio_init()
> function.
> 
> Thanks,
> Johan

USB: serial: cp210x: Implement GPIO support for CP2102N

[Karoly Pados]

OK, though if you allow me a few comments and explanations:


> You're gonna need to unify a lot of this with the current implementation
> for cp2105 however.
>
This also relates to your later points about unifying the cp2105 an cp2102n:
The implementation for the cp2102n ended up pretty different. Both in initializaiton
as well as normal operation, partially due to the better config/input/alt.function
support for the cp2102n. At this point I though it is much more readable if I just create
separate code hierarchies than having to pollute everything with ifs and switch-cases.

And maybe most importantly, I cannot test on the cp2105, so I cannot check if I've broken
anything by mistake, and since it is the odd bird anyway among most devices, 
I though it is pretty logical not to touch its code but instead create a new skeleton for
others.

Of course you're the boss and whatever you say I will do, but I wanted to explain my reasons
(or at least let you know my arguments).

> I was going to comment on the odd coding style above, when I noticed
> that you've copied this implementation from a silabs forum post. Not
> good at all (as there was no indication of any license).
> 
Not quite. It is not from a forum post, but from a SiLabs Knowledge Base article
(https://www.silabs.com/community/interface/knowledge-base.entry.html/2017/06/12/fletcher_checksumfo-TeDF)
That article states explicitly that the code was taken from Wikipedia, so it is CC-SA, which is to
the best of my knowledge 1) compatible with GPL, and 2) does not require attribution if the original material
is missing it, and it does. So AFAICT we are clear on the licensing front.

Do you still want me to remove the checksum check?

> So for cp2105 we decided against implementing an input mode. We at least
> need to be consistent here, so I suggest starting with dropping those
>
Again, you're the boss, but I do find this odd. These are different devices and thankfully now
there'd be better support for the cp2102n, and you're asking me to remove input support for
consistency reasons. To remove a feature for one device just because it wasn't correctly implemented
in another and wanting to stay consistent with the missing support is very odd for me tbh.
I know I'm in no position, but please let me ask you once to please reconsider this.

The way it was done for the cp2105 where we make everything and everyone on the system
think that a pin is "out" but in reality is an input is quirky to say the least, while
at the same time it also does not make sure that an input pin (which is of course not marked as such)
is really an input.  Why amputate the cp2102n like this if it has been already implemented correctly?
If we wanted to stay consistent with half-developed and incomplete drivers then there'd be no
improvements to Linux. 

> Please be more specific here; what do you mean by not corresponding to
> reality.
>
It means that I've found GPIO4-6 occupying bits which were documented doing completely other things,
and those other things occupying reserved bits even though they were documented occupying the places
where GPIOs 4-6 were.

> No need to check for this in every callback; move to
> cp210x_gpio_request().
>
Wouldn't this prevent the disabled GPIOs from being created in the first place?
I very much want to create them in every case, because otherwise it becomes a PITA 
for both the driver and consumers:
- For the driver a PITA, because the disabled GPIOs can be any and non-continuous,
so the module would need to dynamically and arbitrarily remap logical GPIOs to hardware GPIO numbers.
- For consumers a PITA, because it would mean GPIOs can change names depending on the current configuration
of the device. On one device GPIO.3 would be called GPIO.2, on another GPIO.1 etc. My intention
here is that applications should be able to use the real ID of the GPIO and expect that GPIO.3 (base+offset 3)
is actually GPIO.3 and not something else.

Best,
Karoly

June 20, 2018 10:25 AM, "Johan Hovold" <johan@kernel.org> wrote:

> On Sun, Jun 17, 2018 at 08:25:03PM +0200, Karoly Pados wrote:
> 
>> Pretty much what the title says. No other functions/devices
>> touched. Tested on CP2102N-QFN28.
>> 
>> Limitation: Even though the QFN28 package has 7 GPIOs,
>> this patch allows to control only the first 4.
>> What I've found using reverse engineering regarding the
>> other 3 pins collides both with reality and with other
>> documented functions, so I did not dare to just use my
>> findings because I cannot test on other packages.
>> Instead, I decided to play it safe and only support 4 GPIOs.
> 
> Thanks for the patch. Nice to see this finally be implemented for
> further device types.
> 
> You're gonna need to unify a lot of this with the current implementation
> for cp2105 however.
> 
>> Signed-off-by: Karoly Pados <pados@pados.hu>
>> ---
>> drivers/usb/serial/cp210x.c | 259 +++++++++++++++++++++++++++++++++++-
>> 1 file changed, 257 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
>> index 793b86252c46..adb450185ce8 100644
>> --- a/drivers/usb/serial/cp210x.c
>> +++ b/drivers/usb/serial/cp210x.c
>> @@ -6,7 +6,8 @@
>> *
>> * Support to set flow control line levels using TIOCMGET and TIOCMSET
>> * thanks to Karl Hiramoto karl@hiramoto.org. RTSCTS hardware flow
>> - * control thanks to Munir Nassar nassarmu@real-time.com
>> + * control thanks to Munir Nassar nassarmu@real-time.com.
>> + * GPIO support for CP2102N thanks to Karoly Pados.
> 
> No need to thank yourself here. This was added by the original author
> (who appears to have partly been developing this driver out-of-tree) to
> credit contributers.
> 
> The git logs will keep a record of your contributions.
> 
>> *
>> */
>> 
>> @@ -229,11 +230,16 @@ struct cp210x_serial_private {
>> struct gpio_chip gc;
>> u8 config;
>> u8 gpio_mode;
>> + u8 gpio_control;
>> + u8 gpio_dir;
>> bool gpio_registered;
>> #endif
>> u8 partnum;
>> };
>> 
>> +#define CP2102N_PIN_OPENDRAIN(var, pin) (!((var) & BIT(pin)))
>> +#define CP2102N_PIN_GPIO(var, pin) (!((var) & BIT(pin)))
> 
> These are in fact not cp2102n specific. If they are at all needed in the
> end they should be implemented as generic static functions.
> 
>> +
>> struct cp210x_port_private {
>> __u8 bInterfaceNumber;
>> bool has_swapped_line_ctl;
>> @@ -349,6 +355,7 @@ static struct usb_serial_driver * const serial_drivers[] = {
>> #define CP210X_GET_PORTCONFIG 0x370C
>> #define CP210X_GET_DEVICEMODE 0x3711
>> #define CP210X_WRITE_LATCH 0x37E1
>> +#define CP210X_READ_2NCONFIG 0x000E
> 
> Keep the defines sorted by value.
> 
>> /* Part number definitions */
>> #define CP210X_PARTNUM_CP2101 0x01
>> @@ -362,6 +369,14 @@ static struct usb_serial_driver * const serial_drivers[] = {
>> #define CP210X_PARTNUM_CP2102N_QFN20 0x22
>> #define CP210X_PARTNUM_UNKNOWN 0xFF
>> 
>> +#define IS_CP2102N(partnum) (((partnum) == CP210X_PARTNUM_CP2102N_QFN28) || \
>> + ((partnum) == CP210X_PARTNUM_CP2102N_QFN24) || \
>> + ((partnum) == CP210X_PARTNUM_CP2102N_QFN20))
> 
> So reuse the static helper I mentioned in my comments to the baudrate
> patch.
> 
>> +
>> +#define CP210X_2NCONFIG_GPIO_CONTROL_IDX 600
>> +#define CP210X_2NCONFIG_GPIO_MODE_IDX 581
>> +#define CP210X_2NCONFIG_GPIO_RSTLATCH_IDX 587
> 
> Sort by value.
> 
>> +
>> /* CP210X_GET_COMM_STATUS returns these 0x13 bytes */
>> struct cp210x_comm_status {
>> __le32 ulErrors;
>> @@ -1455,6 +1470,235 @@ static void cp210x_gpio_remove(struct usb_serial *serial)
>> }
>> }
>> 
>> +static int cp2102n_gpio_get(struct gpio_chip *gc, unsigned int gpio)
>> +{
>> + struct usb_serial *serial = gpiochip_get_data(gc);
>> + int result;
>> + u8 buf;
>> +
>> + result = cp210x_read_vendor_block(serial, REQTYPE_DEVICE_TO_HOST,
>> + CP210X_READ_LATCH, &buf, sizeof(buf));
>> + if (result < 0)
>> + return result;
>> +
>> + return !!(buf & BIT(gpio));
>> +}
> 
> This should be handled in cp210x_gpio_get() by making the control
> request type depend on the device type.
> 
> Make the cp2102n behaviour the default as it is used also by some legacy
> devices, and explicitly check for the cp2105 odd bird. We'll deal with
> cp2108, which 16 gpios and hence uses 16-bit mask, when we get there.
> 
>> +
>> +static void cp2102n_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
>> +{
>> + int result;
>> + struct cp210x_gpio_write buf;
>> + struct usb_serial *serial = gpiochip_get_data(gc);
>> + struct cp210x_serial_private *priv = usb_get_serial_data(serial);
>> +
>> + /* Ignore request if pin is not in our control */
>> + if (!CP2102N_PIN_GPIO(priv->gpio_control, gpio)) {
>> + dev_warn(&serial->interface->dev,
>> + "Cannot control GPIO with active alternate function.\n");
>> + return;
>> + }
> 
> No need to check for this in every callback; move to
> cp210x_gpio_request().
> 
>> +
>> + buf.state = (value == 1) ? BIT(gpio) : 0;
>> + buf.mask = BIT(gpio);
>> +
>> + result = usb_control_msg(serial->dev,
>> + usb_sndctrlpipe(serial->dev, 0),
>> + CP210X_VENDOR_SPECIFIC,
>> + REQTYPE_HOST_TO_DEVICE,
>> + CP210X_WRITE_LATCH,
>> + *(u16 *)&buf,
> 
> AFAICT this will work also on big-endian machines, but the implicit
> endian conversions involved makes my head spin. ;)
> 
> It may be better to just encode the masks explicitly for the default
> (non-cp2105) case that use wIndex:
> 
> buf.state << 8 | buf.mask
> 
> when merging this with cp210x_gpio_set.
> 
>> + NULL, 0, USB_CTRL_SET_TIMEOUT);
>> +
>> + if (result < 0) {
>> + dev_err(&serial->interface->dev,
>> + "Failed to set GPIO value.\n");
>> + }
>> +}
>> +
>> +static int cp2102n_gpio_direction_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);
>> +
>> + return priv->gpio_dir & BIT(gpio);
>> +}
> 
> So for cp2105 we decided against implementing an input mode. We at least
> need to be consistent here, so I suggest starting with dropping those
> bits of the implementation and we can have that discussion again and,
> depending on the outcome, possibly enable it for all cp210x devices
> later.
> 
>> +static u16 fletcher16(u8 *data, u16 bytes)
>> +{
>> + u16 sum1 = 0xff, sum2 = 0xff;
>> + u16 tlen;
>> +
>> + while (bytes) {
>> + tlen = bytes >= 20 ? 20 : bytes;
>> + bytes -= tlen;
>> + do {
>> + sum2 += sum1 += *data++;
>> + } while (--tlen);
>> + sum1 = (sum1 & 0xff) + (sum1 >> 8);
>> + sum2 = (sum2 & 0xff) + (sum2 >> 8);
>> + }
>> + /* Second reduction step to reduce sums to 8 bits */
>> + sum1 = (sum1 & 0xff) + (sum1 >> 8);
>> + sum2 = (sum2 & 0xff) + (sum2 >> 8);
>> + return sum2 << 8 | sum1;
>> +}
> 
> I was going to comment on the odd coding style above, when I noticed
> that you've copied this implementation from a silabs forum post. Not
> good at all (as there was no indication of any license).
> 
> Please remove.
> 
> Fortunately, the below checksum is redundant so no need for you to
> reimplement this yourself.
> 
>> +
>> +static int cp2102n_gpio_init(struct usb_serial *serial)
>> +{
>> + const u16 CONFIG_SIZE = 0x02A6;
>> + int result;
>> + u16 config_csum;
>> + u8 *config_buf;
>> + u8 gpio_ctrl;
>> + u8 gpio_mode;
>> + u8 gpio_latch;
>> + u8 gpio_rst_latch;
>> + u8 i;
>> + bool config_valid = true;
>> + struct cp210x_serial_private *priv = usb_get_serial_data(serial);
> 
> Style nit: Move longer declarations towards the top of the function
> (reverse xmas style).
> 
>> +
>> + /* 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
>> + */
> 
> Multi-line comments should use the following format
> 
> /*
> * blah...
> */
> 
>> + 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)
>> + return result;
>> +
>> + /* Check configuration for validity.
>> + * The last two bytes of the array contain a fletcher16
>> + * checksum of all the bytes before, which we can validate.
>> + */
>> + config_csum = fletcher16(config_buf, CONFIG_SIZE - 2);
>> + if (((config_csum & 0xFF) != config_buf[CONFIG_SIZE - 1]) ||
>> + ((config_csum >> 8) != config_buf[CONFIG_SIZE - 2])) {
>> + config_valid = false;
>> + dev_err(&serial->interface->dev,
>> + "Corrupted device configuration received\n");
>> + }
> 
> So just drop this for now.
> 
>> +
>> + gpio_mode = 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);
>> +
>> + if (!config_valid)
>> + return -EIO;
>> +
>> + /* We only support 4 GPIOs even on the QFN28 package because
>> + * documentation about the CP2102N GetConfig Array
>> + * does not seem to correspond to reality on this device.
>> + */
> 
> Please be more specific here; what do you mean by not corresponding to
> reality.
> 
>> + 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_mode = (gpio_mode >> 3) & 0x0F;
>> +
>> + /* 0 indicates GPIO mode, 1 is alternate function */
>> + priv->gpio_control = (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 (CP2102N_PIN_OPENDRAIN(priv->gpio_mode, i) &&
>> + (gpio_latch & BIT(i))) {
>> + priv->gpio_dir |= BIT(i);
>> + }
>> + }
> 
> So this can be simplified a bit by deferring implementing an input mode.
> 
>> +
>> + priv->gc.label = "cp2102n";
>> + priv->gc.get_direction = cp2102n_gpio_direction_get;
>> + priv->gc.direction_input = cp2102n_gpio_direction_input;
>> + priv->gc.direction_output = cp2102n_gpio_direction_output;
>> + priv->gc.get = cp2102n_gpio_get;
>> + priv->gc.set = cp2102n_gpio_set;
>> + priv->gc.owner = THIS_MODULE;
>> + priv->gc.parent = &serial->interface->dev;
>> + priv->gc.base = -1;
>> + priv->gc.can_sleep = true;
>> +
>> + result = gpiochip_add_data(&priv->gc, serial);
>> + if (!result)
>> + priv->gpio_registered = true;
> 
> And this whole function should be merged with the cp2105 implementation.
> Retrieving and parsing the configuration is obviously going to remain
> type specific (and live in their own functions), but the above
> initialisation and registration can be shared.
> 
>> +
>> + return result;
>> +}
>> +
>> #else
>> 
>> static int cp2105_shared_gpio_init(struct usb_serial *serial)
>> @@ -1462,6 +1706,11 @@ static int cp2105_shared_gpio_init(struct usb_serial *serial)
>> return 0;
>> }
>> 
>> +static int cp2102n_gpio_init(struct usb_serial *serial)
>> +{
>> + return 0;
>> +}
>> +
>> static void cp210x_gpio_remove(struct usb_serial *serial)
>> {
>> /* Nothing to do */
>> @@ -1522,7 +1771,13 @@ static int cp210x_attach(struct usb_serial *serial)
>> 
>> usb_set_serial_data(serial, priv);
>> 
>> - if (priv->partnum == CP210X_PARTNUM_CP2105) {
>> + if (IS_CP2102N(priv->partnum)) {
>> + result = cp2102n_gpio_init(serial);
>> + if (result < 0) {
>> + dev_err(&serial->interface->dev,
>> + "GPIO initialisation failed, continuing without GPIO support\n");
>> + }
>> + } else if (priv->partnum == CP210X_PARTNUM_CP2105) {
>> result = cp2105_shared_gpio_init(serial);
>> if (result < 0) {
>> dev_err(&serial->interface->dev,
> 
> And this all should be hidden away in a common cp210x_gpio_init()
> function.
> 
> 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] USB: serial: cp210x: Implement GPIO support for CP2102N

[Johan Hovold]

[ Adding Martyn who implemented the cp2105 gpio support. ]

[ Karoly, please avoid top-posting and trim unneeded context instead of
  copying context to the top.
  
  Also set your mailer to wrap lines at 72 columns or so that I don't
  have to reflow your mail manually when responding. ]
  
On Wed, Jun 20, 2018 at 09:45:05AM +0000, Karoly Pados wrote:
> OK, though if you allow me a few comments and explanations:
> 
> > You're gonna need to unify a lot of this with the current
> > implementation for cp2105 however.
> >
> This also relates to your later points about unifying the cp2105 an
> cp2102n: The implementation for the cp2102n ended up pretty different.
> Both in initializaiton as well as normal operation, partially due to
> the better config/input/alt.function support for the cp2102n. At this
> point I though it is much more readable if I just create separate code
> hierarchies than having to pollute everything with ifs and
> switch-cases.
> 
> And maybe most importantly, I cannot test on the cp2105, so I cannot
> check if I've broken anything by mistake, and since it is the odd bird
> anyway among most devices, I though it is pretty logical not to touch
> its code but instead create a new skeleton for others.
>
> Of course you're the boss and whatever you say I will do, but I wanted
> to explain my reasons (or at least let you know my arguments).

If it really was all that different it may make sense, but judging from
vendor driver there are a lot of similarities (e.g. 2 byte mask + value
for set and 1 byte value for get, open-source or push-pull, alternate
function, ...).

As I already mentioned, parsing the configuration will need to be
different, but it seems like a lot of the implementation can be shared
(without much clutter).

> > I was going to comment on the odd coding style above, when I noticed
> > that you've copied this implementation from a silabs forum post. Not
> > good at all (as there was no indication of any license).
> > 
> Not quite. It is not from a forum post, but from a SiLabs Knowledge
> Base article
> (https://www.silabs.com/community/interface/knowledge-base.entry.html/2017/06/12/fletcher_checksumfo-TeDF)

Yeah, that's the one I was referring to.

> That article states explicitly that the code was taken from Wikipedia,
> so it is CC-SA, which is to the best of my knowledge 1) compatible
> with GPL, and 2) does not require attribution if the original material
> is missing it, and it does. So AFAICT we are clear on the licensing
> front.

First of all I can't seem to find that code snippet on the wiki page it
does refer to, so I'm still not convinced.

Second, this should have been high-lighted in your submission somehow.

> Do you still want me to remove the checksum check?

Yes.

> > So for cp2105 we decided against implementing an input mode. We at least
> > need to be consistent here, so I suggest starting with dropping those
> >
> Again, you're the boss, but I do find this odd. These are different
> devices and thankfully now there'd be better support for the cp2102n,
> and you're asking me to remove input support for consistency reasons.
> To remove a feature for one device just because it wasn't correctly
> implemented in another and wanting to stay consistent with the missing
> support is very odd for me tbh.
> I know I'm in no position, but please let me ask you once to please
> reconsider this.

We're not removing anything from the kernel since nothing has yet been
merged. I'm asking you to separate it into a follow up patch, which if
we find it useful and correct would allow the same functionality for
cp2105 as well.

The gpio pins on cp2102n and cp2105 share the same electrical
characteristics and there's no good reason why we should treat them
differently.

> The way it was done for the cp2105 where we make everything and
> everyone on the system think that a pin is "out" but in reality is an
> input is quirky to say the least, while at the same time it also does
> not make sure that an input pin (which is of course not marked as
> such) is really an input.  Why amputate the cp2102n like this if it
> has been already implemented correctly?  If we wanted to stay
> consistent with half-developed and incomplete drivers then there'd be
> no improvements to Linux.

We do not want to have two competing and incompatible implementations of
essentially the same thing.

> > Please be more specific here; what do you mean by not corresponding to
> > reality.
> >
> It means that I've found GPIO4-6 occupying bits which were documented
> doing completely other things, and those other things occupying
> reserved bits even though they were documented occupying the places
> where GPIOs 4-6 were.

Ok, but try to explain your findings regarding this in the commit
message and/or a comment instead of the current formulation. You can be
more specific in the comment, without getting into every detail too
("not corresponding to reality" is too vague).

But to clarify, this has to do with the configuration of the pins,
right? You'd still use the same mechanisms to set and retrieve the latch
register?

> > No need to check for this in every callback; move to
> > cp210x_gpio_request().
> >
> Wouldn't this prevent the disabled GPIOs from being created in the
> first place?  I very much want to create them in every case, because
> otherwise it becomes a PITA for both the driver and consumers:
>
> - For the driver a PITA, because the disabled GPIOs can be any and
>   non-continuous, so the module would need to dynamically and
>   arbitrarily remap logical GPIOs to hardware GPIO numbers.
>
> - For consumers a PITA, because it would mean GPIOs can change names
>   depending on the current configuration of the device. On one device
>   GPIO.3 would be called GPIO.2, on another GPIO.1 etc. My intention
>   here is that applications should be able to use the real ID of the
>   GPIO and expect that GPIO.3 (base+offset 3) is actually GPIO.3 and
>   not something else.

No, that shouldn't be problem (unless something has changed in gpiolib
recently). All pins would be registered, but we simply deny requests for
unavailable pins.

Thanks,
Johan

USB: serial: cp210x: Implement GPIO support for CP2102N

[Johan Hovold]

[ Adding Martyn who implemented the cp2105 gpio support. ]

[ Karoly, please avoid top-posting and trim unneeded context instead of
  copying context to the top.
  
  Also set your mailer to wrap lines at 72 columns or so that I don't
  have to reflow your mail manually when responding. ]
  
On Wed, Jun 20, 2018 at 09:45:05AM +0000, Karoly Pados wrote:
> OK, though if you allow me a few comments and explanations:
> 
> > You're gonna need to unify a lot of this with the current
> > implementation for cp2105 however.
> >
> This also relates to your later points about unifying the cp2105 an
> cp2102n: The implementation for the cp2102n ended up pretty different.
> Both in initializaiton as well as normal operation, partially due to
> the better config/input/alt.function support for the cp2102n. At this
> point I though it is much more readable if I just create separate code
> hierarchies than having to pollute everything with ifs and
> switch-cases.
> 
> And maybe most importantly, I cannot test on the cp2105, so I cannot
> check if I've broken anything by mistake, and since it is the odd bird
> anyway among most devices, I though it is pretty logical not to touch
> its code but instead create a new skeleton for others.
>
> Of course you're the boss and whatever you say I will do, but I wanted
> to explain my reasons (or at least let you know my arguments).

If it really was all that different it may make sense, but judging from
vendor driver there are a lot of similarities (e.g. 2 byte mask + value
for set and 1 byte value for get, open-source or push-pull, alternate
function, ...).

As I already mentioned, parsing the configuration will need to be
different, but it seems like a lot of the implementation can be shared
(without much clutter).

> > I was going to comment on the odd coding style above, when I noticed
> > that you've copied this implementation from a silabs forum post. Not
> > good at all (as there was no indication of any license).
> > 
> Not quite. It is not from a forum post, but from a SiLabs Knowledge
> Base article
> (https://www.silabs.com/community/interface/knowledge-base.entry.html/2017/06/12/fletcher_checksumfo-TeDF)

Yeah, that's the one I was referring to.

> That article states explicitly that the code was taken from Wikipedia,
> so it is CC-SA, which is to the best of my knowledge 1) compatible
> with GPL, and 2) does not require attribution if the original material
> is missing it, and it does. So AFAICT we are clear on the licensing
> front.

First of all I can't seem to find that code snippet on the wiki page it
does refer to, so I'm still not convinced.

Second, this should have been high-lighted in your submission somehow.

> Do you still want me to remove the checksum check?

Yes.

> > So for cp2105 we decided against implementing an input mode. We at least
> > need to be consistent here, so I suggest starting with dropping those
> >
> Again, you're the boss, but I do find this odd. These are different
> devices and thankfully now there'd be better support for the cp2102n,
> and you're asking me to remove input support for consistency reasons.
> To remove a feature for one device just because it wasn't correctly
> implemented in another and wanting to stay consistent with the missing
> support is very odd for me tbh.
> I know I'm in no position, but please let me ask you once to please
> reconsider this.

We're not removing anything from the kernel since nothing has yet been
merged. I'm asking you to separate it into a follow up patch, which if
we find it useful and correct would allow the same functionality for
cp2105 as well.

The gpio pins on cp2102n and cp2105 share the same electrical
characteristics and there's no good reason why we should treat them
differently.

> The way it was done for the cp2105 where we make everything and
> everyone on the system think that a pin is "out" but in reality is an
> input is quirky to say the least, while at the same time it also does
> not make sure that an input pin (which is of course not marked as
> such) is really an input.  Why amputate the cp2102n like this if it
> has been already implemented correctly?  If we wanted to stay
> consistent with half-developed and incomplete drivers then there'd be
> no improvements to Linux.

We do not want to have two competing and incompatible implementations of
essentially the same thing.

> > Please be more specific here; what do you mean by not corresponding to
> > reality.
> >
> It means that I've found GPIO4-6 occupying bits which were documented
> doing completely other things, and those other things occupying
> reserved bits even though they were documented occupying the places
> where GPIOs 4-6 were.

Ok, but try to explain your findings regarding this in the commit
message and/or a comment instead of the current formulation. You can be
more specific in the comment, without getting into every detail too
("not corresponding to reality" is too vague).

But to clarify, this has to do with the configuration of the pins,
right? You'd still use the same mechanisms to set and retrieve the latch
register?

> > No need to check for this in every callback; move to
> > cp210x_gpio_request().
> >
> Wouldn't this prevent the disabled GPIOs from being created in the
> first place?  I very much want to create them in every case, because
> otherwise it becomes a PITA for both the driver and consumers:
>
> - For the driver a PITA, because the disabled GPIOs can be any and
>   non-continuous, so the module would need to dynamically and
>   arbitrarily remap logical GPIOs to hardware GPIO numbers.
>
> - For consumers a PITA, because it would mean GPIOs can change names
>   depending on the current configuration of the device. On one device
>   GPIO.3 would be called GPIO.2, on another GPIO.1 etc. My intention
>   here is that applications should be able to use the real ID of the
>   GPIO and expect that GPIO.3 (base+offset 3) is actually GPIO.3 and
>   not something else.

No, that shouldn't be problem (unless something has changed in gpiolib
recently). All pins would be registered, but we simply deny requests for
unavailable pins.

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] USB: serial: cp210x: Implement GPIO support for CP2102N

[Martyn Welch]

On Wed, 2018-06-20 at 12:52 +0200, Johan Hovold wrote:
> > > [ Adding Martyn who implemented the cp2105 gpio support. ]
> > > So for cp2105 we decided against implementing an input mode. We
> > > at least
> > > need to be consistent here, so I suggest starting with dropping
> > > those
> > > 
> > 
> > Again, you're the boss, but I do find this odd. These are different
> > devices and thankfully now there'd be better support for the
> > cp2102n,
> > and you're asking me to remove input support for consistency
> > reasons.
> > To remove a feature for one device just because it wasn't correctly
> > implemented in another and wanting to stay consistent with the
> > missing
> > support is very odd for me tbh.
> > I know I'm in no position, but please let me ask you once to please
> > reconsider this.
> 
> We're not removing anything from the kernel since nothing has yet
> been
> merged. I'm asking you to separate it into a follow up patch, which
> if
> we find it useful and correct would allow the same functionality for
> cp2105 as well.
> 
> The gpio pins on cp2102n and cp2105 share the same electrical
> characteristics and there's no good reason why we should treat them
> differently.
> 
> > The way it was done for the cp2105 where we make everything and
> > everyone on the system think that a pin is "out" but in reality is
> > an
> > input is quirky to say the least, while at the same time it also
> > does
> > not make sure that an input pin (which is of course not marked as
> > such) is really an input.  Why amputate the cp2102n like this if it
> > has been already implemented correctly?  If we wanted to stay
> > consistent with half-developed and incomplete drivers then there'd
> > be
> > no improvements to Linux.
> 

The rationale for the pins being permanently configured as output pins
is that these pins (at least on the cp2105) do not appear to provide a
true input mode. They offer a "push-pull" mode (where the voltage is
pulled directly to ground or supply rail) and an "open-drain" mode
(where the pin is weakly pulled high by an internal resistor and can be
pulled to ground). Unless I missed something, there is no tristate/high
impedance mode typically associated with a pin being used as input.

Sure, you can use the open-drain mode as input as long as you
understand that the permanent pull up in the cp2015 might have an
impact on what you are reading. For example, if you have a signal that
is externally weakly pulled down, it's going to depend on the relative
resistances of the internal and external resistors as to what voltage
the line rests at and therefore what state the line is considered to be
in. This could stop things working if you naively think the cp2105 is
acting as a typical high-impedance input.

So, with that in mind, we came to the decision that as you *can* read
the state of the output when in open-drain mode, doing it that way may
avoid such potential issues. I didn't do an exhaustive search of other
GPIO devices at the time, so if it can be shown that the majority of
other GPIO drivers with such limitations can be placed into an input
state, I'd be happy for it to be changed so as to remain consistent.


Martyn

USB: serial: cp210x: Implement GPIO support for CP2102N

[Martyn Welch]

On Wed, 2018-06-20 at 12:52 +0200, Johan Hovold wrote:
> > > [ Adding Martyn who implemented the cp2105 gpio support. ]
> > > So for cp2105 we decided against implementing an input mode. We
> > > at least
> > > need to be consistent here, so I suggest starting with dropping
> > > those
> > > 
> > 
> > Again, you're the boss, but I do find this odd. These are different
> > devices and thankfully now there'd be better support for the
> > cp2102n,
> > and you're asking me to remove input support for consistency
> > reasons.
> > To remove a feature for one device just because it wasn't correctly
> > implemented in another and wanting to stay consistent with the
> > missing
> > support is very odd for me tbh.
> > I know I'm in no position, but please let me ask you once to please
> > reconsider this.
> 
> We're not removing anything from the kernel since nothing has yet
> been
> merged. I'm asking you to separate it into a follow up patch, which
> if
> we find it useful and correct would allow the same functionality for
> cp2105 as well.
> 
> The gpio pins on cp2102n and cp2105 share the same electrical
> characteristics and there's no good reason why we should treat them
> differently.
> 
> > The way it was done for the cp2105 where we make everything and
> > everyone on the system think that a pin is "out" but in reality is
> > an
> > input is quirky to say the least, while at the same time it also
> > does
> > not make sure that an input pin (which is of course not marked as
> > such) is really an input.  Why amputate the cp2102n like this if it
> > has been already implemented correctly?  If we wanted to stay
> > consistent with half-developed and incomplete drivers then there'd
> > be
> > no improvements to Linux.
> 

The rationale for the pins being permanently configured as output pins
is that these pins (at least on the cp2105) do not appear to provide a
true input mode. They offer a "push-pull" mode (where the voltage is
pulled directly to ground or supply rail) and an "open-drain" mode
(where the pin is weakly pulled high by an internal resistor and can be
pulled to ground). Unless I missed something, there is no tristate/high
impedance mode typically associated with a pin being used as input.

Sure, you can use the open-drain mode as input as long as you
understand that the permanent pull up in the cp2015 might have an
impact on what you are reading. For example, if you have a signal that
is externally weakly pulled down, it's going to depend on the relative
resistances of the internal and external resistors as to what voltage
the line rests at and therefore what state the line is considered to be
in. This could stop things working if you naively think the cp2105 is
acting as a typical high-impedance input.

So, with that in mind, we came to the decision that as you *can* read
the state of the output when in open-drain mode, doing it that way may
avoid such potential issues. I didn't do an exhaustive search of other
GPIO devices at the time, so if it can be shown that the majority of
other GPIO drivers with such limitations can be placed into an input
state, I'd be happy for it to be changed so as to remain consistent.


Martyn
---
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] USB: serial: cp210x: Implement GPIO support for CP2102N

[Karoly Pados]

Hi Martyn,

> The rationale for the pins being permanently configured as output pins
> is that these pins (at least on the cp2105) do not appear to provide a
> true input mode. They offer a "push-pull" mode (where the voltage is
> pulled directly to ground or supply rail) and an "open-drain" mode
> (where the pin is weakly pulled high by an internal resistor and can be
> pulled to ground). Unless I missed something, there is no tristate/high
> impedance mode typically associated with a pin being used as input.

You didn't miss anything. It is the same for many (maybe all) SiLabs
devices, in particular also for the cp2102n, which spawned this conversation.
I sent in patches for its GPIO support, but in contrast to the cp2105,
I try to emulate input mode by making sure that a pin is never pulled low
when it is set as an input, and that you cannot set push-pull pins as inputs.
The reason why you were invited is because we are trying to figure out which
approach should be taken.

> Sure, you can use the open-drain mode as input as long as you
> understand that the permanent pull up in the cp2015 might have an
> impact on what you are reading. For example, if you have a signal that
> is externally weakly pulled down, it's going to depend on the relative
> resistances of the internal and external resistors as to what voltage
> the line rests at and therefore what state the line is considered to be
> in. This could stop things working if you naively think the cp2105 is
> acting as a typical high-impedance input.

Here I argue the following multiple ways. First, I say that claiming that
a pin which is used as an input is actually an output is not only confusing,
but also much less correct than thinking of it as an input pin with a weak
pullup to prevent floating signals. Second, the pullups - while not 
explicitly listed in the datasheet - can be calculated from what is there, 
and for the cp2105 are typically 132k, for the cp2102n even higher around 
165k. These are pretty weak pullups, so weak that they won't matter for
the vast majority of applications as people rarely use pull-ups or pull-
downs higher than 100k (not never, but rarely). So claiming that it can
result in false expectation, while not completely wrong, is favoring the 
needs of a few instead of the much more common practice.

Lastly, and maybe most importantly, I argue that calling everything an 
"output" pin only in name does not actually avoid any design errors, as
the same circuit that would case a false reading in one case would also
cause the same false reading in the other, and the circuits are usually
developed before the software. So it'll be too late anyway by the time 
somebody realizes such a mistake. But on the contrary, it opens up more 
opportunities for errors, because now you are open to software bugs that
ignore a pin's direction because everything's an output either way even 
when it really isn't, and think that they can treat it as
as open-drain while for some reason it is in push-pull mode. Worse,
even if it is in open-drain mode, it will only work with a specific output
values - it must be high, which is not the default. With my proposal, 
setting a pin's direction to "input" will make sure it cannot be actively 
driven by the chip, avoiding such "misunderstandings" and errors, and 
similar measures are also in place for the push-pull pins.

The only problem I can see is if there isn't a way for the cp2105 to 
query the reset values of the pins (maybe there is, I just haven't 
looked into it). Then I don't know how the direction could be 
determined for an open-drain pin during initialization. But this is 
solved for the cp2102n, and then it is a device-specific issue for 
the cp2105, which shouldn't be forced onto other devices if we otherwise 
decide the approach to be inferior.

Karoly

USB: serial: cp210x: Implement GPIO support for CP2102N

[Karoly Pados]

Hi Martyn,

> The rationale for the pins being permanently configured as output pins
> is that these pins (at least on the cp2105) do not appear to provide a
> true input mode. They offer a "push-pull" mode (where the voltage is
> pulled directly to ground or supply rail) and an "open-drain" mode
> (where the pin is weakly pulled high by an internal resistor and can be
> pulled to ground). Unless I missed something, there is no tristate/high
> impedance mode typically associated with a pin being used as input.

You didn't miss anything. It is the same for many (maybe all) SiLabs
devices, in particular also for the cp2102n, which spawned this conversation.
I sent in patches for its GPIO support, but in contrast to the cp2105,
I try to emulate input mode by making sure that a pin is never pulled low
when it is set as an input, and that you cannot set push-pull pins as inputs.
The reason why you were invited is because we are trying to figure out which
approach should be taken.

> Sure, you can use the open-drain mode as input as long as you
> understand that the permanent pull up in the cp2015 might have an
> impact on what you are reading. For example, if you have a signal that
> is externally weakly pulled down, it's going to depend on the relative
> resistances of the internal and external resistors as to what voltage
> the line rests at and therefore what state the line is considered to be
> in. This could stop things working if you naively think the cp2105 is
> acting as a typical high-impedance input.

Here I argue the following multiple ways. First, I say that claiming that
a pin which is used as an input is actually an output is not only confusing,
but also much less correct than thinking of it as an input pin with a weak
pullup to prevent floating signals. Second, the pullups - while not 
explicitly listed in the datasheet - can be calculated from what is there, 
and for the cp2105 are typically 132k, for the cp2102n even higher around 
165k. These are pretty weak pullups, so weak that they won't matter for
the vast majority of applications as people rarely use pull-ups or pull-
downs higher than 100k (not never, but rarely). So claiming that it can
result in false expectation, while not completely wrong, is favoring the 
needs of a few instead of the much more common practice.

Lastly, and maybe most importantly, I argue that calling everything an 
"output" pin only in name does not actually avoid any design errors, as
the same circuit that would case a false reading in one case would also
cause the same false reading in the other, and the circuits are usually
developed before the software. So it'll be too late anyway by the time 
somebody realizes such a mistake. But on the contrary, it opens up more 
opportunities for errors, because now you are open to software bugs that
ignore a pin's direction because everything's an output either way even 
when it really isn't, and think that they can treat it as
as open-drain while for some reason it is in push-pull mode. Worse,
even if it is in open-drain mode, it will only work with a specific output
values - it must be high, which is not the default. With my proposal, 
setting a pin's direction to "input" will make sure it cannot be actively 
driven by the chip, avoiding such "misunderstandings" and errors, and 
similar measures are also in place for the push-pull pins.

The only problem I can see is if there isn't a way for the cp2105 to 
query the reset values of the pins (maybe there is, I just haven't 
looked into it). Then I don't know how the direction could be 
determined for an open-drain pin during initialization. But this is 
solved for the cp2102n, and then it is a device-specific issue for 
the cp2105, which shouldn't be forced onto other devices if we otherwise 
decide the approach to be inferior.

Karoly
---
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] USB: serial: cp210x: Implement GPIO support for CP2102N

[Martyn Welch]

On Wed, 2018-06-20 at 19:41 +0000, Karoly Pados wrote:
> Hi Martyn,
> 
> > The rationale for the pins being permanently configured as output
> > pins
> > is that these pins (at least on the cp2105) do not appear to
> > provide a
> > true input mode. They offer a "push-pull" mode (where the voltage
> > is
> > pulled directly to ground or supply rail) and an "open-drain" mode
> > (where the pin is weakly pulled high by an internal resistor and
> > can be
> > pulled to ground). Unless I missed something, there is no
> > tristate/high
> > impedance mode typically associated with a pin being used as input.
> 
> You didn't miss anything. It is the same for many (maybe all) SiLabs
> devices, in particular also for the cp2102n, which spawned this
> conversation.
> I sent in patches for its GPIO support, but in contrast to the
> cp2105,
> I try to emulate input mode by making sure that a pin is never pulled
> low
> when it is set as an input, and that you cannot set push-pull pins as
> inputs.
> The reason why you were invited is because we are trying to figure
> out which
> approach should be taken.
> 
> > Sure, you can use the open-drain mode as input as long as you
> > understand that the permanent pull up in the cp2015 might have an
> > impact on what you are reading. For example, if you have a signal
> > that
> > is externally weakly pulled down, it's going to depend on the
> > relative
> > resistances of the internal and external resistors as to what
> > voltage
> > the line rests at and therefore what state the line is considered
> > to be
> > in. This could stop things working if you naively think the cp2105
> > is
> > acting as a typical high-impedance input.
> 
> Here I argue the following multiple ways. First, I say that claiming
> that
> a pin which is used as an input is actually an output is not only
> confusing,
> but also much less correct than thinking of it as an input pin with a
> weak
> pullup to prevent floating signals. Second, the pullups - while not 
> explicitly listed in the datasheet - can be calculated from what is
> there, 
> and for the cp2105 are typically 132k, for the cp2102n even higher
> around 
> 165k. These are pretty weak pullups, so weak that they won't matter
> for
> the vast majority of applications as people rarely use pull-ups or
> pull-
> downs higher than 100k (not never, but rarely). So claiming that it
> can
> result in false expectation, while not completely wrong, is favoring
> the 
> needs of a few instead of the much more common practice.
> 
> Lastly, and maybe most importantly, I argue that calling everything
> an 
> "output" pin only in name does not actually avoid any design errors,
> as
> the same circuit that would case a false reading in one case would
> also
> cause the same false reading in the other, and the circuits are
> usually
> developed before the software. So it'll be too late anyway by the
> time 
> somebody realizes such a mistake. But on the contrary, it opens up
> more 
> opportunities for errors, because now you are open to software bugs
> that
> ignore a pin's direction because everything's an output either way
> even 
> when it really isn't, and think that they can treat it as
> as open-drain while for some reason it is in push-pull mode. Worse,
> even if it is in open-drain mode, it will only work with a specific
> output
> values - it must be high, which is not the default. With my
> proposal, 
> setting a pin's direction to "input" will make sure it cannot be
> actively 
> driven by the chip, avoiding such "misunderstandings" and errors,
> and 
> similar measures are also in place for the push-pull pins.
> 

Yeah, I'll go with that. :-)

> The only problem I can see is if there isn't a way for the cp2105 to 
> query the reset values of the pins (maybe there is, I just haven't 
> looked into it). Then I don't know how the direction could be 
> determined for an open-drain pin during initialization. But this is 
> solved for the cp2102n, and then it is a device-specific issue for 
> the cp2105, which shouldn't be forced onto other devices if we
> otherwise 
> decide the approach to be inferior.
> 

I'm pretty sure there is a way to determine the pin state, though
unfortunately I no longer have access to the HW to be able to test...

Martyn

> Karoly

USB: serial: cp210x: Implement GPIO support for CP2102N

[Martyn Welch]

On Wed, 2018-06-20 at 19:41 +0000, Karoly Pados wrote:
> Hi Martyn,
> 
> > The rationale for the pins being permanently configured as output
> > pins
> > is that these pins (at least on the cp2105) do not appear to
> > provide a
> > true input mode. They offer a "push-pull" mode (where the voltage
> > is
> > pulled directly to ground or supply rail) and an "open-drain" mode
> > (where the pin is weakly pulled high by an internal resistor and
> > can be
> > pulled to ground). Unless I missed something, there is no
> > tristate/high
> > impedance mode typically associated with a pin being used as input.
> 
> You didn't miss anything. It is the same for many (maybe all) SiLabs
> devices, in particular also for the cp2102n, which spawned this
> conversation.
> I sent in patches for its GPIO support, but in contrast to the
> cp2105,
> I try to emulate input mode by making sure that a pin is never pulled
> low
> when it is set as an input, and that you cannot set push-pull pins as
> inputs.
> The reason why you were invited is because we are trying to figure
> out which
> approach should be taken.
> 
> > Sure, you can use the open-drain mode as input as long as you
> > understand that the permanent pull up in the cp2015 might have an
> > impact on what you are reading. For example, if you have a signal
> > that
> > is externally weakly pulled down, it's going to depend on the
> > relative
> > resistances of the internal and external resistors as to what
> > voltage
> > the line rests at and therefore what state the line is considered
> > to be
> > in. This could stop things working if you naively think the cp2105
> > is
> > acting as a typical high-impedance input.
> 
> Here I argue the following multiple ways. First, I say that claiming
> that
> a pin which is used as an input is actually an output is not only
> confusing,
> but also much less correct than thinking of it as an input pin with a
> weak
> pullup to prevent floating signals. Second, the pullups - while not 
> explicitly listed in the datasheet - can be calculated from what is
> there, 
> and for the cp2105 are typically 132k, for the cp2102n even higher
> around 
> 165k. These are pretty weak pullups, so weak that they won't matter
> for
> the vast majority of applications as people rarely use pull-ups or
> pull-
> downs higher than 100k (not never, but rarely). So claiming that it
> can
> result in false expectation, while not completely wrong, is favoring
> the 
> needs of a few instead of the much more common practice.
> 
> Lastly, and maybe most importantly, I argue that calling everything
> an 
> "output" pin only in name does not actually avoid any design errors,
> as
> the same circuit that would case a false reading in one case would
> also
> cause the same false reading in the other, and the circuits are
> usually
> developed before the software. So it'll be too late anyway by the
> time 
> somebody realizes such a mistake. But on the contrary, it opens up
> more 
> opportunities for errors, because now you are open to software bugs
> that
> ignore a pin's direction because everything's an output either way
> even 
> when it really isn't, and think that they can treat it as
> as open-drain while for some reason it is in push-pull mode. Worse,
> even if it is in open-drain mode, it will only work with a specific
> output
> values - it must be high, which is not the default. With my
> proposal, 
> setting a pin's direction to "input" will make sure it cannot be
> actively 
> driven by the chip, avoiding such "misunderstandings" and errors,
> and 
> similar measures are also in place for the push-pull pins.
> 

Yeah, I'll go with that. :-)

> The only problem I can see is if there isn't a way for the cp2105 to 
> query the reset values of the pins (maybe there is, I just haven't 
> looked into it). Then I don't know how the direction could be 
> determined for an open-drain pin during initialization. But this is 
> solved for the cp2102n, and then it is a device-specific issue for 
> the cp2105, which shouldn't be forced onto other devices if we
> otherwise 
> decide the approach to be inferior.
> 

I'm pretty sure there is a way to determine the pin state, though
unfortunately I no longer have access to the HW to be able to test...

Martyn

> Karoly
---
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] USB: serial: cp210x: Implement GPIO support for CP2102N

[Bjørn Mork]

Johan Hovold <johan@kernel.org> writes:

>> Not quite. It is not from a forum post, but from a SiLabs Knowledge
>> Base article
>> (https://www.silabs.com/community/interface/knowledge-base.entry.html/2017/06/12/fletcher_checksumfo-TeDF)
>
> Yeah, that's the one I was referring to.
>
>> That article states explicitly that the code was taken from Wikipedia,
>> so it is CC-SA, which is to the best of my knowledge 1) compatible
>> with GPL, and 2) does not require attribution if the original material
>> is missing it, and it does. So AFAICT we are clear on the licensing
>> front.
>
> First of all I can't seem to find that code snippet on the wiki page it
> does refer to, so I'm still not convinced.

It was there in older versions of the article.  See for example:
https://en.wikipedia.org/w/index.php?title=Fletcher%27s_checksum&oldid=730327006

> Second, this should have been high-lighted in your submission somehow.

Definitely. All code has an original author who deserves credit.  And if
you cannot find the original author, then there is always a risk than
someone along the line stole the code...  Maybe long before it ended up
in Wikipedia.  But that doesn't matter.

Doesn't seem worth the risk for a simple checksum algorithm which
probably has lots of GPL implementations.


Bjørn

USB: serial: cp210x: Implement GPIO support for CP2102N

[Bjørn Mork]

Johan Hovold <johan@kernel.org> writes:

>> Not quite. It is not from a forum post, but from a SiLabs Knowledge
>> Base article
>> (https://www.silabs.com/community/interface/knowledge-base.entry.html/2017/06/12/fletcher_checksumfo-TeDF)
>
> Yeah, that's the one I was referring to.
>
>> That article states explicitly that the code was taken from Wikipedia,
>> so it is CC-SA, which is to the best of my knowledge 1) compatible
>> with GPL, and 2) does not require attribution if the original material
>> is missing it, and it does. So AFAICT we are clear on the licensing
>> front.
>
> First of all I can't seem to find that code snippet on the wiki page it
> does refer to, so I'm still not convinced.

It was there in older versions of the article.  See for example:
https://en.wikipedia.org/w/index.php?title=Fletcher%27s_checksum&oldid=730327006

> Second, this should have been high-lighted in your submission somehow.

Definitely. All code has an original author who deserves credit.  And if
you cannot find the original author, then there is always a risk than
someone along the line stole the code...  Maybe long before it ended up
in Wikipedia.  But that doesn't matter.

Doesn't seem worth the risk for a simple checksum algorithm which
probably has lots of GPL implementations.


Bjørn
---
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] USB: serial: cp210x: Implement GPIO support for CP2102N

[Karoly Pados]

>> First of all I can't seem to find that code snippet on the wiki page it
>> does refer to, so I'm still not convinced.
> 
> It was there in older versions of the article. See for example:
> https://en.wikipedia.org/w/index.php?title=Fletcher's_checksum&oldid=730327006
> 
>> Second, this should have been high-lighted in your submission somehow.
> 
> Definitely. All code has an original author who deserves credit. And if
> you cannot find the original author, then there is always a risk than
> someone along the line stole the code... Maybe long before it ended up
> in Wikipedia. But that doesn't matter.
> 
> Doesn't seem worth the risk for a simple checksum algorithm which
> probably has lots of GPL implementations.
> 

Sorry about not explicitly mentioning the source, I only cared about making
sure that there is no violation and I thought that'd be enough.
Next time something like this happens, I'll explicitly name the sources
even if the license does not require it.

And just to clear things up further, rest assured that fletcher function was 
the only one copied from the Cypress knowledge base or from anywhere else.
Rest is by me, so there should be no other unclear origins.

On a different note, v2 patches are coming soon with the feedback from Johan
incorporated. Did not forget and I still intend to polish everything up
as requested to get these features into mainline.

Karoly

USB: serial: cp210x: Implement GPIO support for CP2102N

[Karoly Pados]

>> First of all I can't seem to find that code snippet on the wiki page it
>> does refer to, so I'm still not convinced.
> 
> It was there in older versions of the article. See for example:
> https://en.wikipedia.org/w/index.php?title=Fletcher's_checksum&oldid=730327006
> 
>> Second, this should have been high-lighted in your submission somehow.
> 
> Definitely. All code has an original author who deserves credit. And if
> you cannot find the original author, then there is always a risk than
> someone along the line stole the code... Maybe long before it ended up
> in Wikipedia. But that doesn't matter.
> 
> Doesn't seem worth the risk for a simple checksum algorithm which
> probably has lots of GPL implementations.
> 

Sorry about not explicitly mentioning the source, I only cared about making
sure that there is no violation and I thought that'd be enough.
Next time something like this happens, I'll explicitly name the sources
even if the license does not require it.

And just to clear things up further, rest assured that fletcher function was 
the only one copied from the Cypress knowledge base or from anywhere else.
Rest is by me, so there should be no other unclear origins.

On a different note, v2 patches are coming soon with the feedback from Johan
incorporated. Did not forget and I still intend to polish everything up
as requested to get these features into mainline.

Karoly
---
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] USB: serial: cp210x: Implement GPIO support for CP2102N

[Johan Hovold]

On Fri, Jun 22, 2018 at 04:10:21PM +0100, Martyn Welch wrote:
> On Wed, 2018-06-20 at 19:41 +0000, Karoly Pados wrote:

> > Here I argue the following multiple ways. First, I say that claiming
> > that a pin which is used as an input is actually an output is not
> > only confusing, but also much less correct than thinking of it as an
> > input pin with a weak pullup to prevent floating signals. Second,
> > the pullups - while not  explicitly listed in the datasheet - can be
> > calculated from what is there,  and for the cp2105 are typically
> > 132k, for the cp2102n even higher around  165k. These are pretty
> > weak pullups, so weak that they won't matter for the vast majority
> > of applications as people rarely use pull-ups or pull- downs higher
> > than 100k (not never, but rarely). So claiming that it can result in
> > false expectation, while not completely wrong, is favoring the 
> > needs of a few instead of the much more common practice.
> > 
> > Lastly, and maybe most importantly, I argue that calling everything
> > an  "output" pin only in name does not actually avoid any design
> > errors, as the same circuit that would case a false reading in one
> > case would also cause the same false reading in the other, and the
> > circuits are usually developed before the software. So it'll be too
> > late anyway by the time  somebody realizes such a mistake. But on
> > the contrary, it opens up more  opportunities for errors, because
> > now you are open to software bugs that ignore a pin's direction
> > because everything's an output either way even  when it really
> > isn't, and think that they can treat it as as open-drain while for
> > some reason it is in push-pull mode. Worse, even if it is in
> > open-drain mode, it will only work with a specific output values -
> > it must be high, which is not the default. With my proposal, 
> > setting a pin's direction to "input" will make sure it cannot be
> > actively  driven by the chip, avoiding such "misunderstandings" and
> > errors, and  similar measures are also in place for the push-pull
> > pins.
> 
> Yeah, I'll go with that. :-)

Sounds good to me too. Thanks to both of you for spelling this out.

> > The only problem I can see is if there isn't a way for the cp2105 to 
> > query the reset values of the pins (maybe there is, I just haven't 
> > looked into it). Then I don't know how the direction could be 
> > determined for an open-drain pin during initialization. But this is 
> > solved for the cp2102n, and then it is a device-specific issue for 
> > the cp2105, which shouldn't be forced onto other devices if we
> > otherwise decide the approach to be inferior.
> 
> I'm pretty sure there is a way to determine the pin state, though
> unfortunately I no longer have access to the HW to be able to test...

If this isn't (yet) possible, it's never wrong to continue treating the
cp2105 pins as (open-drain) outputs.

Thanks,
Johan

USB: serial: cp210x: Implement GPIO support for CP2102N

[Johan Hovold]

On Fri, Jun 22, 2018 at 04:10:21PM +0100, Martyn Welch wrote:
> On Wed, 2018-06-20 at 19:41 +0000, Karoly Pados wrote:

> > Here I argue the following multiple ways. First, I say that claiming
> > that a pin which is used as an input is actually an output is not
> > only confusing, but also much less correct than thinking of it as an
> > input pin with a weak pullup to prevent floating signals. Second,
> > the pullups - while not  explicitly listed in the datasheet - can be
> > calculated from what is there,  and for the cp2105 are typically
> > 132k, for the cp2102n even higher around  165k. These are pretty
> > weak pullups, so weak that they won't matter for the vast majority
> > of applications as people rarely use pull-ups or pull- downs higher
> > than 100k (not never, but rarely). So claiming that it can result in
> > false expectation, while not completely wrong, is favoring the 
> > needs of a few instead of the much more common practice.
> > 
> > Lastly, and maybe most importantly, I argue that calling everything
> > an  "output" pin only in name does not actually avoid any design
> > errors, as the same circuit that would case a false reading in one
> > case would also cause the same false reading in the other, and the
> > circuits are usually developed before the software. So it'll be too
> > late anyway by the time  somebody realizes such a mistake. But on
> > the contrary, it opens up more  opportunities for errors, because
> > now you are open to software bugs that ignore a pin's direction
> > because everything's an output either way even  when it really
> > isn't, and think that they can treat it as as open-drain while for
> > some reason it is in push-pull mode. Worse, even if it is in
> > open-drain mode, it will only work with a specific output values -
> > it must be high, which is not the default. With my proposal, 
> > setting a pin's direction to "input" will make sure it cannot be
> > actively  driven by the chip, avoiding such "misunderstandings" and
> > errors, and  similar measures are also in place for the push-pull
> > pins.
> 
> Yeah, I'll go with that. :-)

Sounds good to me too. Thanks to both of you for spelling this out.

> > The only problem I can see is if there isn't a way for the cp2105 to 
> > query the reset values of the pins (maybe there is, I just haven't 
> > looked into it). Then I don't know how the direction could be 
> > determined for an open-drain pin during initialization. But this is 
> > solved for the cp2102n, and then it is a device-specific issue for 
> > the cp2105, which shouldn't be forced onto other devices if we
> > otherwise decide the approach to be inferior.
> 
> I'm pretty sure there is a way to determine the pin state, though
> unfortunately I no longer have access to the HW to be able to test...

If this isn't (yet) possible, it's never wrong to continue treating the
cp2105 pins as (open-drain) outputs.

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] USB: serial: cp210x: Implement GPIO support for CP2102N

[Johan Hovold]

On Mon, Jul 02, 2018 at 08:04:32PM +0200, Bjørn Mork wrote:
> Johan Hovold <johan@kernel.org> writes:
> 
> >> Not quite. It is not from a forum post, but from a SiLabs Knowledge
> >> Base article
> >> (https://www.silabs.com/community/interface/knowledge-base.entry.html/2017/06/12/fletcher_checksumfo-TeDF)
> >
> > Yeah, that's the one I was referring to.
> >
> >> That article states explicitly that the code was taken from Wikipedia,
> >> so it is CC-SA, which is to the best of my knowledge 1) compatible
> >> with GPL, and 2) does not require attribution if the original material
> >> is missing it, and it does. So AFAICT we are clear on the licensing
> >> front.
> >
> > First of all I can't seem to find that code snippet on the wiki page it
> > does refer to, so I'm still not convinced.
> 
> It was there in older versions of the article.  See for example:
> https://en.wikipedia.org/w/index.php?title=Fletcher%27s_checksum&oldid=730327006

Ah, thanks for digging that out.

> > Second, this should have been high-lighted in your submission somehow.
> 
> Definitely. All code has an original author who deserves credit.  And if
> you cannot find the original author, then there is always a risk than
> someone along the line stole the code...  Maybe long before it ended up
> in Wikipedia.  But that doesn't matter.
> 
> Doesn't seem worth the risk for a simple checksum algorithm which
> probably has lots of GPL implementations.

Right.

Johan

USB: serial: cp210x: Implement GPIO support for CP2102N

[Johan Hovold]

On Mon, Jul 02, 2018 at 08:04:32PM +0200, Bjørn Mork wrote:
> Johan Hovold <johan@kernel.org> writes:
> 
> >> Not quite. It is not from a forum post, but from a SiLabs Knowledge
> >> Base article
> >> (https://www.silabs.com/community/interface/knowledge-base.entry.html/2017/06/12/fletcher_checksumfo-TeDF)
> >
> > Yeah, that's the one I was referring to.
> >
> >> That article states explicitly that the code was taken from Wikipedia,
> >> so it is CC-SA, which is to the best of my knowledge 1) compatible
> >> with GPL, and 2) does not require attribution if the original material
> >> is missing it, and it does. So AFAICT we are clear on the licensing
> >> front.
> >
> > First of all I can't seem to find that code snippet on the wiki page it
> > does refer to, so I'm still not convinced.
> 
> It was there in older versions of the article.  See for example:
> https://en.wikipedia.org/w/index.php?title=Fletcher%27s_checksum&oldid=730327006

Ah, thanks for digging that out.

> > Second, this should have been high-lighted in your submission somehow.
> 
> Definitely. All code has an original author who deserves credit.  And if
> you cannot find the original author, then there is always a risk than
> someone along the line stole the code...  Maybe long before it ended up
> in Wikipedia.  But that doesn't matter.
> 
> Doesn't seem worth the risk for a simple checksum algorithm which
> probably has lots of GPL implementations.

Right.

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] USB: serial: cp210x: Implement GPIO support for CP2102N

[Johan Hovold]

On Mon, Jul 02, 2018 at 07:17:33PM +0000, Karoly Pados wrote:

> On a different note, v2 patches are coming soon with the feedback from Johan
> incorporated. Did not forget and I still intend to polish everything up
> as requested to get these features into mainline.

Sounds good!

Johan

USB: serial: cp210x: Implement GPIO support for CP2102N

[Johan Hovold]

On Mon, Jul 02, 2018 at 07:17:33PM +0000, Karoly Pados wrote:

> On a different note, v2 patches are coming soon with the feedback from Johan
> incorporated. Did not forget and I still intend to polish everything up
> as requested to get these features into mainline.

Sounds good!

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