[PATCH] usb: cp210x: Add cp2108 GPIOs support

[PATCH] usb: cp210x: Add cp2108 GPIOs support

[Serge Semin]

Each chip from the cp210x series got GPIOs on board. This commit
provides the support for sixteen ones placed on the cp2108 four-ports
serial console controller. All of the GPIOs are equally distributed
to four USB interfaces in accordance with GPIOs alternative functions
attachment.

cp2108 GPIOs can be either in open-drain or push-pull modes setup once
after reset was cleared. In this matter it doesn't differ from the rest
of cp210x devices supported by the driver. So with minor alterations the
standard output/intput GPIO interface is implemented for cp2108.

Aside from traditional GPIO functions like setting/getting pins value,
each GPIO is also multiplexed with alternative functions: TX/RX LEDs, RS485
TX-enable and Clocks source. These functions can't be activated on-fly.
Instead the chips firmware should be properly setup, so they would be
enabled in the ports-config structure at the controller logic startup.
Needless to say, that when the alternative functions are activated,
the GPIOs can't be used. Thus we need to check the GPIO pin config in the
request callback and deny the request if GPIO standard function is
disabled.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/usb/serial/Kconfig  |   2 +-
 drivers/usb/serial/cp210x.c | 158 ++++++++++++++++++++++++++++++++----
 2 files changed, 143 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
index 7d031911d04e..20bd4c0632c7 100644
--- a/drivers/usb/serial/Kconfig
+++ b/drivers/usb/serial/Kconfig
@@ -138,7 +138,7 @@ config USB_SERIAL_DIGI_ACCELEPORT
 config USB_SERIAL_CP210X
 	tristate "USB CP210x family of UART Bridge Controllers"
 	help
-	  Say Y here if you want to use a CP2101/CP2102/CP2103 based USB
+	  Say Y here if you want to use a CP2101/2/3/4/5/8 based USB
 	  to RS232 converters.
 
 	  To compile this driver as a module, choose M here: the
diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 979bef9bfb6b..a97f04d9e99f 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -505,6 +505,56 @@ struct cp210x_gpio_write {
 	u8	state;
 } __packed;
 
+/* CP2108 interfaces, gpio (per interface), port-blocks number, GPIO block. */
+#define CP2108_IFACE_NUM		4
+#define CP2108_GPIO_NUM			4
+#define CP2108_PB_NUM			5
+#define CP2108_GPIO_PB			1
+
+/*
+ * CP2108 default pins state. There are five PBs. Each one is with its specific
+ * pins-set (see USB Express SDK sources or SDK-based smt application
+ * https://github.com/fancer/smt-cp210x for details).
+ */
+struct cp2108_state {
+	__le16	mode[CP2108_PB_NUM];	/* 0 - Open-Drain, 1 - Push-Pull */
+	__le16	low_power[CP2108_PB_NUM];
+	__le16	latch[CP2108_PB_NUM];	/* 0 - Logic Low, 1 - Logic High */
+} __packed;
+
+/*
+ * CP210X_VENDOR_SPECIFIC, CP210X_GET_PORTCONFIG call reads these 73 bytes.
+ * Reset/Suspend latches describe default states after reset/suspend of the
+ * pins. The rest are responsible for alternate functions settings of the
+ * chip pins (see USB Express SDK sources or SDK-based smt application
+ * https://github.com/fancer/smt-cp210x for details).
+ */
+struct cp2108_config {
+	struct cp2108_state reset_latch;
+	struct cp2108_state suspend_latch;
+	u8	ip_delay[CP2108_IFACE_NUM];
+	u8	enhanced_fxn[CP2108_IFACE_NUM];
+	u8	enhanced_fxn_dev;
+	u8	ext_clock_freq[CP2108_IFACE_NUM];
+} __packed;
+
+/* CP2108 port alternate functions fields */
+#define CP2108_GPIO_TXLED_MODE		BIT(0)
+#define CP2108_GPIO_RXLED_MODE		BIT(1)
+#define CP2108_GPIO_RS485_MODE		BIT(2)
+#define CP2108_GPIO_RS485_LOGIC		BIT(3)
+#define CP2108_GPIO_CLOCK_MODE		BIT(4)
+#define CP2108_DYNAMIC_SUSPEND		BIT(5)
+
+/*
+ * CP210X_VENDOR_SPECIFIC, CP210X_WRITE_LATCH call writes these 0x4 bytes
+ * to CP2108 controller.
+ */
+struct cp2108_gpio_write {
+	__le16	mask;
+	__le16	state;
+} __packed;
+
 /*
  * Helper to get interface number when we only have struct usb_serial.
  */
@@ -1366,10 +1416,15 @@ static int cp210x_gpio_get(struct gpio_chip *gc, unsigned int gpio)
 	struct usb_serial *serial = gpiochip_get_data(gc);
 	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
 	u8 req_type = REQTYPE_DEVICE_TO_HOST;
-	int result;
-	u8 buf;
-
-	if (priv->partnum == CP210X_PARTNUM_CP2105)
+	union {
+		u8 single;
+		__le16 dual;
+	} buf;
+	int result, bufsize = sizeof(buf.single);
+
+	if (priv->partnum == CP210X_PARTNUM_CP2108)
+		bufsize = sizeof(buf.dual);
+	else if (priv->partnum == CP210X_PARTNUM_CP2105)
 		req_type = REQTYPE_INTERFACE_TO_HOST;
 
 	result = usb_autopm_get_interface(serial->interface);
@@ -1377,39 +1432,51 @@ static int cp210x_gpio_get(struct gpio_chip *gc, unsigned int gpio)
 		return result;
 
 	result = cp210x_read_vendor_block(serial, req_type,
-					  CP210X_READ_LATCH, &buf, sizeof(buf));
+					  CP210X_READ_LATCH, &buf, bufsize);
 	usb_autopm_put_interface(serial->interface);
 	if (result < 0)
 		return result;
 
-	return !!(buf & BIT(gpio));
+	if (priv->partnum == CP210X_PARTNUM_CP2108)
+		result = !!(le16_to_cpu(buf.dual) & BIT(gpio));
+	else
+		result = !!(buf.single & BIT(gpio));
+
+	return result;
 }
 
 static void cp210x_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
 {
 	struct usb_serial *serial = gpiochip_get_data(gc);
 	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
-	struct cp210x_gpio_write buf;
 	int result;
 
-	if (value == 1)
-		buf.state = BIT(gpio);
-	else
-		buf.state = 0;
-
-	buf.mask = BIT(gpio);
-
 	result = usb_autopm_get_interface(serial->interface);
 	if (result)
 		goto out;
 
-	if (priv->partnum == CP210X_PARTNUM_CP2105) {
+	if (priv->partnum == CP210X_PARTNUM_CP2108) {
+		struct cp2108_gpio_write buf;
+
+		buf.state = ((value == 1) ? cpu_to_le16(BIT(gpio)) : 0);
+		buf.mask = cpu_to_le16(BIT(gpio));
+
+		result = cp210x_write_vendor_block(serial,
+						   REQTYPE_HOST_TO_DEVICE,
+						   CP210X_WRITE_LATCH, &buf,
+						   sizeof(buf));
+	} else if (priv->partnum == CP210X_PARTNUM_CP2105) {
+		struct cp210x_gpio_write buf;
+
+		buf.state = ((value == 1) ? BIT(gpio) : 0);
+		buf.mask = BIT(gpio);
+
 		result = cp210x_write_vendor_block(serial,
 						   REQTYPE_HOST_TO_INTERFACE,
 						   CP210X_WRITE_LATCH, &buf,
 						   sizeof(buf));
 	} else {
-		u16 wIndex = buf.state << 8 | buf.mask;
+		u16 wIndex = ((value == 1) ? BIT(gpio) : 0) << 8 | BIT(gpio);
 
 		result = usb_control_msg(serial->dev,
 					 usb_sndctrlpipe(serial->dev, 0),
@@ -1489,6 +1556,62 @@ static int cp210x_gpio_set_config(struct gpio_chip *gc, unsigned int gpio,
 	return -ENOTSUPP;
 }
 
+/*
+ * CP2108 got 16 GPIOs, each of which can be configured either as input, or
+ * as open-drain with weak pulling up to VIO or as push-pull with strong
+ * pulling up to VIO. Similar to the rest of devices the open-drain mode
+ * with latch set high is treated as input mode. All GPIOs are equally
+ * distributed between four interfaces. Thanks to the mask-state based
+ * write-latch control message we don't need to worry about possible races.
+ */
+static int cp2108_gpioconf_init(struct usb_serial *serial)
+{
+	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
+	struct cp2108_config config;
+	u16 mode, latch;
+	u8 intf_num;
+	int result;
+
+	result = cp210x_read_vendor_block(serial, REQTYPE_DEVICE_TO_HOST,
+					  CP210X_GET_PORTCONFIG, &config,
+					  sizeof(config));
+	if (result < 0)
+		return result;
+
+	/*
+	 * There are four interfaces with four GPIOs for each port. Here we
+	 * parse the device config data to comply with the driver interface.
+	 * Note that the mode can be changed only after reset, which cause
+	 * the driver reloading anyway. So we can safely read the config just
+	 * once at attach procedure.
+	 */
+	intf_num = cp210x_interface_num(serial);
+	mode = le16_to_cpu(config.reset_latch.mode[CP2108_GPIO_PB]);
+	latch = le16_to_cpu(config.reset_latch.latch[CP2108_GPIO_PB]);
+
+	priv->gpio_altfunc = config.enhanced_fxn[intf_num];
+	priv->gpio_pushpull = (mode >> (intf_num*CP2108_GPIO_NUM)) & 0x0f;
+	priv->gpio_input = (latch >> (intf_num*CP2108_GPIO_NUM)) & 0x0f;
+
+	/*
+	 * Move the GPIO clock alternative function bit value to the fourth bit
+	 * as the corresponding GPIO pin reside. It shall make the generic
+	 * cp210x GPIO request method being suitable for cp2108 as well.
+	 */
+	priv->gpio_altfunc &= ~BIT(3);
+	if (priv->gpio_altfunc & CP2108_GPIO_CLOCK_MODE)
+		priv->gpio_altfunc |= BIT(3);
+
+	/*
+	 * Open-drain mode in combination with a high latch value is used
+	 * to emulate the GPIO input pin.
+	 */
+	priv->gpio_input &= ~priv->gpio_pushpull;
+	priv->gc.ngpio = CP2108_GPIO_NUM;
+
+	return 0;
+}
+
 /*
  * This function is for configuring GPIO using shared pins, where other signals
  * are made unavailable by configuring the use of GPIO. This is believed to be
@@ -1713,6 +1836,9 @@ static int cp210x_gpio_init(struct usb_serial *serial)
 	case CP210X_PARTNUM_CP2105:
 		result = cp2105_gpioconf_init(serial);
 		break;
+	case CP210X_PARTNUM_CP2108:
+		result = cp2108_gpioconf_init(serial);
+		break;
 	case CP210X_PARTNUM_CP2102N_QFN28:
 	case CP210X_PARTNUM_CP2102N_QFN24:
 	case CP210X_PARTNUM_CP2102N_QFN20:
-- 
2.21.0

Re: [PATCH] usb: cp210x: Add cp2108 GPIOs support

[Johan Hovold]

On Tue, May 14, 2019 at 01:53:57PM +0300, Serge Semin wrote:
> Each chip from the cp210x series got GPIOs on board. This commit
> provides the support for sixteen ones placed on the cp2108 four-ports
> serial console controller. All of the GPIOs are equally distributed
> to four USB interfaces in accordance with GPIOs alternative functions
> attachment.
> 
> cp2108 GPIOs can be either in open-drain or push-pull modes setup once
> after reset was cleared. In this matter it doesn't differ from the rest
> of cp210x devices supported by the driver. So with minor alterations the
> standard output/intput GPIO interface is implemented for cp2108.
> 
> Aside from traditional GPIO functions like setting/getting pins value,
> each GPIO is also multiplexed with alternative functions: TX/RX LEDs, RS485
> TX-enable and Clocks source. These functions can't be activated on-fly.
> Instead the chips firmware should be properly setup, so they would be
> enabled in the ports-config structure at the controller logic startup.
> Needless to say, that when the alternative functions are activated,
> the GPIOs can't be used. Thus we need to check the GPIO pin config in the
> request callback and deny the request if GPIO standard function is
> disabled.

Thanks for the patch.

I'm a bit worried about the logic getting too convoluted when dealing
with the cp2108 and cp2105 differences. Please see if you can do
something about that.

Other than that, just some minor comments and request for
clarifications.

> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  drivers/usb/serial/Kconfig  |   2 +-
>  drivers/usb/serial/cp210x.c | 158 ++++++++++++++++++++++++++++++++----
>  2 files changed, 143 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
> index 7d031911d04e..20bd4c0632c7 100644
> --- a/drivers/usb/serial/Kconfig
> +++ b/drivers/usb/serial/Kconfig
> @@ -138,7 +138,7 @@ config USB_SERIAL_DIGI_ACCELEPORT
>  config USB_SERIAL_CP210X
>  	tristate "USB CP210x family of UART Bridge Controllers"
>  	help
> -	  Say Y here if you want to use a CP2101/CP2102/CP2103 based USB
> +	  Say Y here if you want to use a CP2101/2/3/4/5/8 based USB

Please drop this since it's a separate change. If anything we should
probably just change this to "CP210X" not have to worry about it getting
outdated again.

>  	  to RS232 converters.
>  
>  	  To compile this driver as a module, choose M here: the
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index 979bef9bfb6b..a97f04d9e99f 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -505,6 +505,56 @@ struct cp210x_gpio_write {
>  	u8	state;
>  } __packed;
>  
> +/* CP2108 interfaces, gpio (per interface), port-blocks number, GPIO block. */
> +#define CP2108_IFACE_NUM		4
> +#define CP2108_GPIO_NUM			4
> +#define CP2108_PB_NUM			5
> +#define CP2108_GPIO_PB			1

Please try to give these more descriptive names; I'd prefer COUNT over
NUM when used as a suffix.

Perhaps slap an INDEX suffix on CP2108_GPIO_PB etc.

> +/*
> + * CP2108 default pins state. There are five PBs. Each one is with its specific
> + * pins-set (see USB Express SDK sources or SDK-based smt application
> + * https://github.com/fancer/smt-cp210x for details).
> + */
> +struct cp2108_state {
> +	__le16	mode[CP2108_PB_NUM];	/* 0 - Open-Drain, 1 - Push-Pull */
> +	__le16	low_power[CP2108_PB_NUM];
> +	__le16	latch[CP2108_PB_NUM];	/* 0 - Logic Low, 1 - Logic High */
> +} __packed;

Ok, so you use mode[CP2108_GPIO_PB] to get the pin modes below;
what are the other "PB"s used for? Why is it a "port" block, if all 16
pins allocated to four different ports are accessible through one block?

Please try to make the comment self-contained (even if you leave some
details out). And perhaps we shouldn't refer to proprietary code in
here, and in any case the link may go away.

> +/*
> + * CP210X_VENDOR_SPECIFIC, CP210X_GET_PORTCONFIG call reads these 73 bytes.
> + * Reset/Suspend latches describe default states after reset/suspend of the
> + * pins. The rest are responsible for alternate functions settings of the
> + * chip pins (see USB Express SDK sources or SDK-based smt application
> + * https://github.com/fancer/smt-cp210x for details).
> + */
> +struct cp2108_config {
> +	struct cp2108_state reset_latch;
> +	struct cp2108_state suspend_latch;
> +	u8	ip_delay[CP2108_IFACE_NUM];
> +	u8	enhanced_fxn[CP2108_IFACE_NUM];
> +	u8	enhanced_fxn_dev;
> +	u8	ext_clock_freq[CP2108_IFACE_NUM];
> +} __packed;
> +
> +/* CP2108 port alternate functions fields */
> +#define CP2108_GPIO_TXLED_MODE		BIT(0)
> +#define CP2108_GPIO_RXLED_MODE		BIT(1)
> +#define CP2108_GPIO_RS485_MODE		BIT(2)
> +#define CP2108_GPIO_RS485_LOGIC		BIT(3)
> +#define CP2108_GPIO_CLOCK_MODE		BIT(4)
> +#define CP2108_DYNAMIC_SUSPEND		BIT(5)

No GPIO infix?

> +
> +/*
> + * CP210X_VENDOR_SPECIFIC, CP210X_WRITE_LATCH call writes these 0x4 bytes
> + * to CP2108 controller.
> + */
> +struct cp2108_gpio_write {
> +	__le16	mask;
> +	__le16	state;
> +} __packed;

No need for __packed (yes, I know there are other unnecessary __packed
in this file).

>  /*
>   * Helper to get interface number when we only have struct usb_serial.
>   */
> @@ -1366,10 +1416,15 @@ static int cp210x_gpio_get(struct gpio_chip *gc, unsigned int gpio)
>  	struct usb_serial *serial = gpiochip_get_data(gc);
>  	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
>  	u8 req_type = REQTYPE_DEVICE_TO_HOST;
> -	int result;
> -	u8 buf;
> -
> -	if (priv->partnum == CP210X_PARTNUM_CP2105)
> +	union {
> +		u8 single;
> +		__le16 dual;
> +	} buf;

I wonder if this would become more readable if you just use u16,
initialise to zero, read one byte unless cp2108, and then
unconditionally do le16_to_cpus() before applying the mask.

I want to keep the number of conditionals down, but if the logic is
getting to hard to follow because cp2105 and cp2108 doing things
differently we may need to add dedicated cp2108 (and maybe cp2105)
get/set callbacks instead.

> +	int result, bufsize = sizeof(buf.single);

One declaration per line please, especially when initialising.

> +
> +	if (priv->partnum == CP210X_PARTNUM_CP2108)
> +		bufsize = sizeof(buf.dual);
> +	else if (priv->partnum == CP210X_PARTNUM_CP2105)
>  		req_type = REQTYPE_INTERFACE_TO_HOST;
>  
>  	result = usb_autopm_get_interface(serial->interface);
> @@ -1377,39 +1432,51 @@ static int cp210x_gpio_get(struct gpio_chip *gc, unsigned int gpio)
>  		return result;
>  
>  	result = cp210x_read_vendor_block(serial, req_type,
> -					  CP210X_READ_LATCH, &buf, sizeof(buf));
> +					  CP210X_READ_LATCH, &buf, bufsize);
>  	usb_autopm_put_interface(serial->interface);
>  	if (result < 0)
>  		return result;
>  
> -	return !!(buf & BIT(gpio));
> +	if (priv->partnum == CP210X_PARTNUM_CP2108)
> +		result = !!(le16_to_cpu(buf.dual) & BIT(gpio));
> +	else
> +		result = !!(buf.single & BIT(gpio));
> +
> +	return result;
>  }
>  
>  static void cp210x_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
>  {
>  	struct usb_serial *serial = gpiochip_get_data(gc);
>  	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
> -	struct cp210x_gpio_write buf;
>  	int result;
>  
> -	if (value == 1)
> -		buf.state = BIT(gpio);
> -	else
> -		buf.state = 0;
> -
> -	buf.mask = BIT(gpio);
> -
>  	result = usb_autopm_get_interface(serial->interface);
>  	if (result)
>  		goto out;
>  
> -	if (priv->partnum == CP210X_PARTNUM_CP2105) {
> +	if (priv->partnum == CP210X_PARTNUM_CP2108) {
> +		struct cp2108_gpio_write buf;
> +
> +		buf.state = ((value == 1) ? cpu_to_le16(BIT(gpio)) : 0);
> +		buf.mask = cpu_to_le16(BIT(gpio));
> +
> +		result = cp210x_write_vendor_block(serial,
> +						   REQTYPE_HOST_TO_DEVICE,
> +						   CP210X_WRITE_LATCH, &buf,
> +						   sizeof(buf));
> +	} else if (priv->partnum == CP210X_PARTNUM_CP2105) {
> +		struct cp210x_gpio_write buf;
> +
> +		buf.state = ((value == 1) ? BIT(gpio) : 0);
> +		buf.mask = BIT(gpio);
> +
>  		result = cp210x_write_vendor_block(serial,
>  						   REQTYPE_HOST_TO_INTERFACE,
>  						   CP210X_WRITE_LATCH, &buf,
>  						   sizeof(buf));
>  	} else {
> -		u16 wIndex = buf.state << 8 | buf.mask;
> +		u16 wIndex = ((value == 1) ? BIT(gpio) : 0) << 8 | BIT(gpio);

This is way I try to avoid the ternary operator; too easy to write
overly compact code. Please untangle again.

>  		result = usb_control_msg(serial->dev,
>  					 usb_sndctrlpipe(serial->dev, 0),
> @@ -1489,6 +1556,62 @@ static int cp210x_gpio_set_config(struct gpio_chip *gc, unsigned int gpio,
>  	return -ENOTSUPP;
>  }
>  
> +/*
> + * CP2108 got 16 GPIOs, each of which can be configured either as input, or
> + * as open-drain with weak pulling up to VIO or as push-pull with strong
> + * pulling up to VIO. Similar to the rest of devices the open-drain mode
> + * with latch set high is treated as input mode. All GPIOs are equally
> + * distributed between four interfaces. Thanks to the mask-state based
> + * write-latch control message we don't need to worry about possible races.
> + */
> +static int cp2108_gpioconf_init(struct usb_serial *serial)
> +{
> +	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
> +	struct cp2108_config config;
> +	u16 mode, latch;
> +	u8 intf_num;
> +	int result;
> +
> +	result = cp210x_read_vendor_block(serial, REQTYPE_DEVICE_TO_HOST,
> +					  CP210X_GET_PORTCONFIG, &config,
> +					  sizeof(config));
> +	if (result < 0)
> +		return result;
> +
> +	/*
> +	 * There are four interfaces with four GPIOs for each port. Here we
> +	 * parse the device config data to comply with the driver interface.
> +	 * Note that the mode can be changed only after reset, which cause
> +	 * the driver reloading anyway. So we can safely read the config just
> +	 * once at attach procedure.
> +	 */
> +	intf_num = cp210x_interface_num(serial);
> +	mode = le16_to_cpu(config.reset_latch.mode[CP2108_GPIO_PB]);
> +	latch = le16_to_cpu(config.reset_latch.latch[CP2108_GPIO_PB]);
> +
> +	priv->gpio_altfunc = config.enhanced_fxn[intf_num];

Missing sanity check on intf_num.

> +	priv->gpio_pushpull = (mode >> (intf_num*CP2108_GPIO_NUM)) & 0x0f;
> +	priv->gpio_input = (latch >> (intf_num*CP2108_GPIO_NUM)) & 0x0f;

Missing spaces around multiplication operators.

> +
> +	/*
> +	 * Move the GPIO clock alternative function bit value to the fourth bit
> +	 * as the corresponding GPIO pin reside. It shall make the generic
> +	 * cp210x GPIO request method being suitable for cp2108 as well.
> +	 */

This isn't entirely clear, please try to rephrase.

Which functions are available on which pins? Do the function defines
need to be renamed to reflect the pin numbers as for CP2104?

> +	priv->gpio_altfunc &= ~BIT(3);
> +	if (priv->gpio_altfunc & CP2108_GPIO_CLOCK_MODE)
> +		priv->gpio_altfunc |= BIT(3);
> +
> +	/*
> +	 * Open-drain mode in combination with a high latch value is used
> +	 * to emulate the GPIO input pin.
> +	 */
> +	priv->gpio_input &= ~priv->gpio_pushpull;

Input mode should only be set when the reset latch value is 1 (see
cp2104).

> +	priv->gc.ngpio = CP2108_GPIO_NUM;

Probably better to just use 4 directly instead of this define in this
function (e.g. your mask above implicitly depends on it anyway).

> +	return 0;
> +}

Johan

Re: [PATCH] usb: cp210x: Add cp2108 GPIOs support

[Serge Semin]

Hello Johan

On Mon, Jun 03, 2019 at 02:52:05PM +0200, Johan Hovold wrote:
> On Tue, May 14, 2019 at 01:53:57PM +0300, Serge Semin wrote:
> > Each chip from the cp210x series got GPIOs on board. This commit
> > provides the support for sixteen ones placed on the cp2108 four-ports
> > serial console controller. All of the GPIOs are equally distributed
> > to four USB interfaces in accordance with GPIOs alternative functions
> > attachment.
> > 
> > cp2108 GPIOs can be either in open-drain or push-pull modes setup once
> > after reset was cleared. In this matter it doesn't differ from the rest
> > of cp210x devices supported by the driver. So with minor alterations the
> > standard output/intput GPIO interface is implemented for cp2108.
> > 
> > Aside from traditional GPIO functions like setting/getting pins value,
> > each GPIO is also multiplexed with alternative functions: TX/RX LEDs, RS485
> > TX-enable and Clocks source. These functions can't be activated on-fly.
> > Instead the chips firmware should be properly setup, so they would be
> > enabled in the ports-config structure at the controller logic startup.
> > Needless to say, that when the alternative functions are activated,
> > the GPIOs can't be used. Thus we need to check the GPIO pin config in the
> > request callback and deny the request if GPIO standard function is
> > disabled.
> 
> Thanks for the patch.
> 
> I'm a bit worried about the logic getting too convoluted when dealing
> with the cp2108 and cp2105 differences. Please see if you can do
> something about that.
> 
> Other than that, just some minor comments and request for
> clarifications.
> 
> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> > ---
> >  drivers/usb/serial/Kconfig  |   2 +-
> >  drivers/usb/serial/cp210x.c | 158 ++++++++++++++++++++++++++++++++----
> >  2 files changed, 143 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
> > index 7d031911d04e..20bd4c0632c7 100644
> > --- a/drivers/usb/serial/Kconfig
> > +++ b/drivers/usb/serial/Kconfig
> > @@ -138,7 +138,7 @@ config USB_SERIAL_DIGI_ACCELEPORT
> >  config USB_SERIAL_CP210X
> >  	tristate "USB CP210x family of UART Bridge Controllers"
> >  	help
> > -	  Say Y here if you want to use a CP2101/CP2102/CP2103 based USB
> > +	  Say Y here if you want to use a CP2101/2/3/4/5/8 based USB
> 
> Please drop this since it's a separate change. If anything we should
> probably just change this to "CP210X" not have to worry about it getting
> outdated again.
> 

Ok. Moved to a separate patch.

> >  	  to RS232 converters.
> >  
> >  	  To compile this driver as a module, choose M here: the
> > diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> > index 979bef9bfb6b..a97f04d9e99f 100644
> > --- a/drivers/usb/serial/cp210x.c
> > +++ b/drivers/usb/serial/cp210x.c
> > @@ -505,6 +505,56 @@ struct cp210x_gpio_write {
> >  	u8	state;
> >  } __packed;
> >  
> > +/* CP2108 interfaces, gpio (per interface), port-blocks number, GPIO block. */
> > +#define CP2108_IFACE_NUM		4
> > +#define CP2108_GPIO_NUM			4
> > +#define CP2108_PB_NUM			5
> > +#define CP2108_GPIO_PB			1
> 
> Please try to give these more descriptive names; I'd prefer COUNT over
> NUM when used as a suffix.
> 
> Perhaps slap an INDEX suffix on CP2108_GPIO_PB etc.
> 

Ok. Added CNT and IDX suffixes.

> > +/*
> > + * CP2108 default pins state. There are five PBs. Each one is with its specific
> > + * pins-set (see USB Express SDK sources or SDK-based smt application
> > + * https://github.com/fancer/smt-cp210x for details).
> > + */
> > +struct cp2108_state {
> > +	__le16	mode[CP2108_PB_NUM];	/* 0 - Open-Drain, 1 - Push-Pull */
> > +	__le16	low_power[CP2108_PB_NUM];
> > +	__le16	latch[CP2108_PB_NUM];	/* 0 - Logic Low, 1 - Logic High */
> > +} __packed;
> 
> Ok, so you use mode[CP2108_GPIO_PB] to get the pin modes below;
> what are the other "PB"s used for? Why is it a "port" block, if all 16
> pins allocated to four different ports are accessible through one block?
> 
> Please try to make the comment self-contained (even if you leave some
> details out). And perhaps we shouldn't refer to proprietary code in
> here, and in any case the link may go away.
> 

Ok added a bit more detailed information regarding the port blocks. But
I'd prefer to keep the link. First of all I can't provide the complete
description of the fields here because it would take too much space and
in fact is pointless since only a single port block with GPIOs is utilized.
So the link and the SDK info are a good reference to find some details
(especially due to lacking such an info in the chip datasheet). Secondly
even though the code is distributed under the Silicon Labs specific license
it is open-source. Finally the link may go away only if I removed the
application from my github account, which I don't intend to.

> > +/*
> > + * CP210X_VENDOR_SPECIFIC, CP210X_GET_PORTCONFIG call reads these 73 bytes.
> > + * Reset/Suspend latches describe default states after reset/suspend of the
> > + * pins. The rest are responsible for alternate functions settings of the
> > + * chip pins (see USB Express SDK sources or SDK-based smt application
> > + * https://github.com/fancer/smt-cp210x for details).
> > + */
> > +struct cp2108_config {
> > +	struct cp2108_state reset_latch;
> > +	struct cp2108_state suspend_latch;
> > +	u8	ip_delay[CP2108_IFACE_NUM];
> > +	u8	enhanced_fxn[CP2108_IFACE_NUM];
> > +	u8	enhanced_fxn_dev;
> > +	u8	ext_clock_freq[CP2108_IFACE_NUM];
> > +} __packed;
> > +
> > +/* CP2108 port alternate functions fields */
> > +#define CP2108_GPIO_TXLED_MODE		BIT(0)
> > +#define CP2108_GPIO_RXLED_MODE		BIT(1)
> > +#define CP2108_GPIO_RS485_MODE		BIT(2)
> > +#define CP2108_GPIO_RS485_LOGIC		BIT(3)
> > +#define CP2108_GPIO_CLOCK_MODE		BIT(4)
> > +#define CP2108_DYNAMIC_SUSPEND		BIT(5)
> 
> No GPIO infix?
> 

No, because this refers to all pins state when the chip being suspended
(whether the suspend_latch state is pushed to the pins at the suspend USB
state), while the rest of the macros are defined for the GPIOs alternative
functions. I'll add the MODE suffix though.

> > +
> > +/*
> > + * CP210X_VENDOR_SPECIFIC, CP210X_WRITE_LATCH call writes these 0x4 bytes
> > + * to CP2108 controller.
> > + */
> > +struct cp2108_gpio_write {
> > +	__le16	mask;
> > +	__le16	state;
> > +} __packed;
> 
> No need for __packed (yes, I know there are other unnecessary __packed
> in this file).
> 

Ok.

> >  /*
> >   * Helper to get interface number when we only have struct usb_serial.
> >   */
> > @@ -1366,10 +1416,15 @@ static int cp210x_gpio_get(struct gpio_chip *gc, unsigned int gpio)
> >  	struct usb_serial *serial = gpiochip_get_data(gc);
> >  	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
> >  	u8 req_type = REQTYPE_DEVICE_TO_HOST;
> > -	int result;
> > -	u8 buf;
> > -
> > -	if (priv->partnum == CP210X_PARTNUM_CP2105)
> > +	union {
> > +		u8 single;
> > +		__le16 dual;
> > +	} buf;
> 
> I wonder if this would become more readable if you just use u16,
> initialise to zero, read one byte unless cp2108, and then
> unconditionally do le16_to_cpus() before applying the mask.
> 
> I want to keep the number of conditionals down, but if the logic is
> getting to hard to follow because cp2105 and cp2108 doing things
> differently we may need to add dedicated cp2108 (and maybe cp2105)
> get/set callbacks instead.
> 

Ok. Replaced the union with u16 buffer.

> > +	int result, bufsize = sizeof(buf.single);
> 
> One declaration per line please, especially when initialising.
> 

Done. Though I can't remember this being requirement in the kernel
coding style. Could you give me a link to the corresponding statement?

> > +
> > +	if (priv->partnum == CP210X_PARTNUM_CP2108)
> > +		bufsize = sizeof(buf.dual);
> > +	else if (priv->partnum == CP210X_PARTNUM_CP2105)
> >  		req_type = REQTYPE_INTERFACE_TO_HOST;
> >  
> >  	result = usb_autopm_get_interface(serial->interface);
> > @@ -1377,39 +1432,51 @@ static int cp210x_gpio_get(struct gpio_chip *gc, unsigned int gpio)
> >  		return result;
> >  
> >  	result = cp210x_read_vendor_block(serial, req_type,
> > -					  CP210X_READ_LATCH, &buf, sizeof(buf));
> > +					  CP210X_READ_LATCH, &buf, bufsize);
> >  	usb_autopm_put_interface(serial->interface);
> >  	if (result < 0)
> >  		return result;
> >  
> > -	return !!(buf & BIT(gpio));
> > +	if (priv->partnum == CP210X_PARTNUM_CP2108)
> > +		result = !!(le16_to_cpu(buf.dual) & BIT(gpio));
> > +	else
> > +		result = !!(buf.single & BIT(gpio));
> > +
> > +	return result;
> >  }
> >  
> >  static void cp210x_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
> >  {
> >  	struct usb_serial *serial = gpiochip_get_data(gc);
> >  	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
> > -	struct cp210x_gpio_write buf;
> >  	int result;
> >  
> > -	if (value == 1)
> > -		buf.state = BIT(gpio);
> > -	else
> > -		buf.state = 0;
> > -
> > -	buf.mask = BIT(gpio);
> > -
> >  	result = usb_autopm_get_interface(serial->interface);
> >  	if (result)
> >  		goto out;
> >  
> > -	if (priv->partnum == CP210X_PARTNUM_CP2105) {
> > +	if (priv->partnum == CP210X_PARTNUM_CP2108) {
> > +		struct cp2108_gpio_write buf;
> > +
> > +		buf.state = ((value == 1) ? cpu_to_le16(BIT(gpio)) : 0);
> > +		buf.mask = cpu_to_le16(BIT(gpio));
> > +
> > +		result = cp210x_write_vendor_block(serial,
> > +						   REQTYPE_HOST_TO_DEVICE,
> > +						   CP210X_WRITE_LATCH, &buf,
> > +						   sizeof(buf));
> > +	} else if (priv->partnum == CP210X_PARTNUM_CP2105) {
> > +		struct cp210x_gpio_write buf;
> > +
> > +		buf.state = ((value == 1) ? BIT(gpio) : 0);
> > +		buf.mask = BIT(gpio);
> > +
> >  		result = cp210x_write_vendor_block(serial,
> >  						   REQTYPE_HOST_TO_INTERFACE,
> >  						   CP210X_WRITE_LATCH, &buf,
> >  						   sizeof(buf));
> >  	} else {
> > -		u16 wIndex = buf.state << 8 | buf.mask;
> > +		u16 wIndex = ((value == 1) ? BIT(gpio) : 0) << 8 | BIT(gpio);
> 
> This is way I try to avoid the ternary operator; too easy to write
> overly compact code. Please untangle again.
> 

Ok. Untangled all the ternary operations utilized in this function.

> >  		result = usb_control_msg(serial->dev,
> >  					 usb_sndctrlpipe(serial->dev, 0),
> > @@ -1489,6 +1556,62 @@ static int cp210x_gpio_set_config(struct gpio_chip *gc, unsigned int gpio,
> >  	return -ENOTSUPP;
> >  }
> >  
> > +/*
> > + * CP2108 got 16 GPIOs, each of which can be configured either as input, or
> > + * as open-drain with weak pulling up to VIO or as push-pull with strong
> > + * pulling up to VIO. Similar to the rest of devices the open-drain mode
> > + * with latch set high is treated as input mode. All GPIOs are equally
> > + * distributed between four interfaces. Thanks to the mask-state based
> > + * write-latch control message we don't need to worry about possible races.
> > + */
> > +static int cp2108_gpioconf_init(struct usb_serial *serial)
> > +{
> > +	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
> > +	struct cp2108_config config;
> > +	u16 mode, latch;
> > +	u8 intf_num;
> > +	int result;
> > +
> > +	result = cp210x_read_vendor_block(serial, REQTYPE_DEVICE_TO_HOST,
> > +					  CP210X_GET_PORTCONFIG, &config,
> > +					  sizeof(config));
> > +	if (result < 0)
> > +		return result;
> > +
> > +	/*
> > +	 * There are four interfaces with four GPIOs for each port. Here we
> > +	 * parse the device config data to comply with the driver interface.
> > +	 * Note that the mode can be changed only after reset, which cause
> > +	 * the driver reloading anyway. So we can safely read the config just
> > +	 * once at attach procedure.
> > +	 */
> > +	intf_num = cp210x_interface_num(serial);
> > +	mode = le16_to_cpu(config.reset_latch.mode[CP2108_GPIO_PB]);
> > +	latch = le16_to_cpu(config.reset_latch.latch[CP2108_GPIO_PB]);
> > +
> > +	priv->gpio_altfunc = config.enhanced_fxn[intf_num];
> 
> Missing sanity check on intf_num.
> 

Done.

> > +	priv->gpio_pushpull = (mode >> (intf_num*CP2108_GPIO_NUM)) & 0x0f;
> > +	priv->gpio_input = (latch >> (intf_num*CP2108_GPIO_NUM)) & 0x0f;
> 
> Missing spaces around multiplication operators.
> 

Done.

> > +
> > +	/*
> > +	 * Move the GPIO clock alternative function bit value to the fourth bit
> > +	 * as the corresponding GPIO pin reside. It shall make the generic
> > +	 * cp210x GPIO request method being suitable for cp2108 as well.
> > +	 */
> 
> This isn't entirely clear, please try to rephrase.
> 
> Which functions are available on which pins? Do the function defines
> need to be renamed to reflect the pin numbers as for CP2104?
> 

I wouldn't rename the macros'es, since they describe the functions of
enhanced_fxn bits. BTW CP2104 doesn't do any renaming anyway, while it
is doing the same thing as I am here. Instead I added a more descriptive
comment, so the code hackers would see the reason of the BITs copy-pasting.

> > +	priv->gpio_altfunc &= ~BIT(3);
> > +	if (priv->gpio_altfunc & CP2108_GPIO_CLOCK_MODE)
> > +		priv->gpio_altfunc |= BIT(3);
> > +
> > +	/*
> > +	 * Open-drain mode in combination with a high latch value is used
> > +	 * to emulate the GPIO input pin.
> > +	 */
> > +	priv->gpio_input &= ~priv->gpio_pushpull;
> 
> Input mode should only be set when the reset latch value is 1 (see
> cp2104).
> 

Yes, and this code does the same thing as the loop in cp2104, but in a single
line. BTW the cp2104 cp2104_gpioconf_init() method can be simplified in the
same way.stash@{0}stash@{0}

> > +	priv->gc.ngpio = CP2108_GPIO_NUM;
> 
> Probably better to just use 4 directly instead of this define in this
> function (e.g. your mask above implicitly depends on it anyway).
> 

Ok. Removed the macro from this function.

It should be also noted that in v2 I fixed a cp2108-related bug in the
cp210x_gpio_get()/cp210x_gpio_set() methods. So aside from the requested
alterations the logic of the cp2108 GPIOs setter/getter is bit different
now.

Regards,
-Sergey

> > +	return 0;
> > +}
> 
> Johan

[PATCH v2] usb: cp210x: Add cp2108 GPIOs support

[Serge Semin]

Each chip from the cp210x series got GPIOs on board. This commit
provides the support for sixteen ones placed on the cp2108 four-ports
serial console controller. Since all GPIOs are accessible via any
cp2108 USB interface we manually and equally distributed them between
all interfaces in accordance with GPIOs alternative functions attachment.
So the driver provides four GPIOs per each cp2108 USB interface.

cp2108 GPIOs can be either in open-drain or push-pull modes setup once
after reset was cleared. In this matter it doesn't differ from the rest
of cp210x devices supported by the driver. So with minor alterations the
standard output/intput GPIO interface is implemented for cp2108.

Aside from traditional GPIO functions like setting/getting pins value,
each GPIO is also multiplexed with alternative functions: TX/RX LEDs, RS485
TX-enable and Clocks source. These functions can't be activated on-fly.
Instead the chips firmware should be properly setup, so they would be
enabled in the ports-config structure at the controller logic startup.
Needless to say, that when the alternative functions are activated,
the GPIOs can't be used. Thus we need to check the GPIO pin config in the
request callback and deny the request if GPIO standard function is
disabled.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---

Changelog v2:
- Rebase onto kernel 5.2.0-rc4.
- Move USB_SERIAL_CP210X config descriptor modification to a separate patch.
- Add more descriptive info of cp2108 state and config structures.
- Replace _NUM suffix with _CNT.
- Add _IDX suffix to CP2108_GPIO_PB macro.
- Add _MODE suffix to CP2108_DYNAMIC_SUSPEND macro.
- Discard __packed attribute from struct cp2108_gpio_write.
- Discard CP2108_GPIO_CNT macro and use literal 4 instead.
- Simplify cp210x_gpio_get() method by replacing the union with a u16 buffer.
- Replace ternary operations of cp210x_gpio_set() with a conditional statement.
- Add more descriptive comments regarding the gpio_altfunc bits collection.
- Fix a bug in the GPIOs setter/getter methods of invalid bit being utilized
to set/get GPIO values.
---
 drivers/usb/serial/cp210x.c | 178 +++++++++++++++++++++++++++++++++---
 1 file changed, 164 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 979bef9bfb6b..32f0a4273abb 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -505,6 +505,60 @@ struct cp210x_gpio_write {
 	u8	state;
 } __packed;
 
+/*
+ * Number of CP2108 USB interfaces, port-blocks and GPIO port-block index,
+ * where port-blocks are the internal pins containers of the chip.
+ */
+#define CP2108_IFACE_CNT		4
+#define CP2108_PB_CNT			5
+#define CP2108_GPIO_PB_IDX		1
+
+/*
+ * CP2108 default pins state. There are five port-blocks (PB). Each one is with
+ * it' specific pins-set: Port 0 - UART 0 and 1, Port 1 - GPIOs, Port 2 - chip
+ * suspend and a part of UART 2 pins, Port 3 and 4 - UART 2 and 3 pins
+ * (for details see USB Express SDK sources or SDK-based smt application
+ * accessible here https://github.com/fancer/smt-cp210x).
+ */
+struct cp2108_state {
+	__le16	mode[CP2108_PB_CNT];	/* 0 - Open-Drain, 1 - Push-Pull */
+	__le16	low_power[CP2108_PB_CNT];
+	__le16	latch[CP2108_PB_CNT];	/* 0 - Logic Low, 1 - Logic High */
+} __packed;
+
+/*
+ * CP210X_VENDOR_SPECIFIC, CP210X_GET_PORTCONFIG call reads these 73 bytes.
+ * Reset/Suspend latches describe default states after reset/suspend of the
+ * pins. The rest are responsible for alternate functions settings of the
+ * chip pins (see USB Express SDK sources or SDK-based smt application
+ * https://github.com/fancer/smt-cp210x for details).
+ */
+struct cp2108_config {
+	struct cp2108_state reset_latch;
+	struct cp2108_state suspend_latch;
+	u8	ip_delay[CP2108_IFACE_CNT];
+	u8	enhanced_fxn[CP2108_IFACE_CNT];
+	u8	enhanced_fxn_dev;
+	u8	ext_clock_freq[CP2108_IFACE_CNT];
+} __packed;
+
+/* CP2108 port alternate functions fields. */
+#define CP2108_GPIO_TXLED_MODE		BIT(0)
+#define CP2108_GPIO_RXLED_MODE		BIT(1)
+#define CP2108_GPIO_RS485_MODE		BIT(2)
+#define CP2108_GPIO_RS485_LOGIC		BIT(3)
+#define CP2108_GPIO_CLOCK_MODE		BIT(4)
+#define CP2108_DYNAMIC_SUSPEND_MODE	BIT(5)
+
+/*
+ * CP210X_VENDOR_SPECIFIC, CP210X_WRITE_LATCH call writes these 0x4 bytes
+ * to CP2108 controller.
+ */
+struct cp2108_gpio_write {
+	__le16	mask;
+	__le16	state;
+};
+
 /*
  * Helper to get interface number when we only have struct usb_serial.
  */
@@ -1366,10 +1420,13 @@ static int cp210x_gpio_get(struct gpio_chip *gc, unsigned int gpio)
 	struct usb_serial *serial = gpiochip_get_data(gc);
 	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
 	u8 req_type = REQTYPE_DEVICE_TO_HOST;
+	int bufsize = 1;
 	int result;
-	u8 buf;
+	u16 buf;
 
-	if (priv->partnum == CP210X_PARTNUM_CP2105)
+	if (priv->partnum == CP210X_PARTNUM_CP2108)
+		bufsize = 2;
+	else if (priv->partnum == CP210X_PARTNUM_CP2105)
 		req_type = REQTYPE_INTERFACE_TO_HOST;
 
 	result = usb_autopm_get_interface(serial->interface);
@@ -1377,39 +1434,62 @@ static int cp210x_gpio_get(struct gpio_chip *gc, unsigned int gpio)
 		return result;
 
 	result = cp210x_read_vendor_block(serial, req_type,
-					  CP210X_READ_LATCH, &buf, sizeof(buf));
+					  CP210X_READ_LATCH, &buf, bufsize);
 	usb_autopm_put_interface(serial->interface);
 	if (result < 0)
 		return result;
 
-	return !!(buf & BIT(gpio));
+	if (priv->partnum == CP210X_PARTNUM_CP2108) {
+		u8 intf_num = cp210x_interface_num(serial);
+
+		gpio += intf_num * 4;
+	}
+
+	return !!(le16_to_cpu(buf) & BIT(gpio));
 }
 
 static void cp210x_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
 {
 	struct usb_serial *serial = gpiochip_get_data(gc);
 	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
-	struct cp210x_gpio_write buf;
 	int result;
 
-	if (value == 1)
-		buf.state = BIT(gpio);
-	else
-		buf.state = 0;
-
-	buf.mask = BIT(gpio);
-
 	result = usb_autopm_get_interface(serial->interface);
 	if (result)
 		goto out;
 
-	if (priv->partnum == CP210X_PARTNUM_CP2105) {
+	if (priv->partnum == CP210X_PARTNUM_CP2108) {
+		u8 intf_num = cp210x_interface_num(serial);
+		struct cp2108_gpio_write buf;
+
+		buf.mask = cpu_to_le16(BIT(intf_num * 4 + gpio));
+		if (value == 1)
+			buf.state = buf.mask;
+		else
+			buf.state = 0;
+
+		result = cp210x_write_vendor_block(serial,
+						   REQTYPE_HOST_TO_DEVICE,
+						   CP210X_WRITE_LATCH, &buf,
+						   sizeof(buf));
+	} else if (priv->partnum == CP210X_PARTNUM_CP2105) {
+		struct cp210x_gpio_write buf;
+
+		buf.mask = BIT(gpio);
+		if (value == 1)
+			buf.state = buf.mask;
+		else
+			buf.state = 0;
+
 		result = cp210x_write_vendor_block(serial,
 						   REQTYPE_HOST_TO_INTERFACE,
 						   CP210X_WRITE_LATCH, &buf,
 						   sizeof(buf));
 	} else {
-		u16 wIndex = buf.state << 8 | buf.mask;
+		u16 wIndex = BIT(gpio);
+
+		if (value == 1)
+			wIndex |= (BIT(gpio) << 8);
 
 		result = usb_control_msg(serial->dev,
 					 usb_sndctrlpipe(serial->dev, 0),
@@ -1489,6 +1569,73 @@ static int cp210x_gpio_set_config(struct gpio_chip *gc, unsigned int gpio,
 	return -ENOTSUPP;
 }
 
+/*
+ * CP2108 got 16 GPIOs, each of which can be configured either as input, or
+ * as open-drain with weak pulling up to VIO or as push-pull with strong
+ * pulling up to VIO. Similar to the rest of devices the open-drain mode
+ * with latch set high is treated as input mode. All GPIOs are equally
+ * distributed between four interfaces. Thanks to the mask-state based
+ * write-latch control message we don't need to worry about possible races.
+ */
+static int cp2108_gpioconf_init(struct usb_serial *serial)
+{
+	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
+	struct cp2108_config config;
+	u16 mode, latch;
+	u8 intf_num;
+	int result;
+
+	result = cp210x_read_vendor_block(serial, REQTYPE_DEVICE_TO_HOST,
+					  CP210X_GET_PORTCONFIG, &config,
+					  sizeof(config));
+	if (result < 0)
+		return result;
+
+	/*
+	 * There are four interfaces with four GPIOs manually distributed for
+	 * each of them. Here we parse the device config data to comply with
+	 * the driver interface. Note that the mode (push-pull, open-drain,
+	 * etc) can be changed only after reset, which causes the driver
+	 * reloading anyway. So we can safely read the config just once at the
+	 * device attachment procedure.
+	 */
+	intf_num = cp210x_interface_num(serial);
+	if (intf_num >= CP2108_IFACE_CNT)
+		return -ENODEV;
+	mode = le16_to_cpu(config.reset_latch.mode[CP2108_GPIO_PB_IDX]);
+	latch = le16_to_cpu(config.reset_latch.latch[CP2108_GPIO_PB_IDX]);
+
+	priv->gpio_altfunc = config.enhanced_fxn[intf_num];
+	priv->gpio_pushpull = (mode >> (intf_num * 4)) & 0x0f;
+	priv->gpio_input = (latch >> (intf_num * 4)) & 0x0f;
+	priv->gc.ngpio = 4;
+
+	/*
+	 * Each GPIO[i*4 + x] can have an alternative function enabled
+	 * (x = 0 - UART i TX toggle, x = 1 - UART i RX toggle, x = 2 - UART i
+	 * RS485 mode and x = 3  - clock output i), in which case the GPIO
+	 * functionality isn't available. If it is switched on we deny the GPIO
+	 * requests. GPIO alternative functions state resides the
+	 * enhanced_fxn[i] bitfield, where each bit linearly corresponds to
+	 * the specific GPIO, except GPIO[3] - clock output function, which
+	 * alas is placed at fourth bit, while the third bit is busy with RS485
+	 * logic mode flag. In order to make the generic cp210x GPIO request
+	 * method being suitable for cp2108, lets copy the GPIO[4] clock
+	 * alternative function state bit to the GPIO[3] RS485 logic mode bit.
+	 */
+	priv->gpio_altfunc &= ~CP2108_GPIO_RS485_LOGIC;
+	if (priv->gpio_altfunc & CP2108_GPIO_CLOCK_MODE)
+		priv->gpio_altfunc |= CP2108_GPIO_RS485_LOGIC;
+
+	/*
+	 * Open-drain mode in combination with a high latch value is used
+	 * to emulate the GPIO input pin.
+	 */
+	priv->gpio_input &= ~priv->gpio_pushpull;
+
+	return 0;
+}
+
 /*
  * This function is for configuring GPIO using shared pins, where other signals
  * are made unavailable by configuring the use of GPIO. This is believed to be
@@ -1713,6 +1860,9 @@ static int cp210x_gpio_init(struct usb_serial *serial)
 	case CP210X_PARTNUM_CP2105:
 		result = cp2105_gpioconf_init(serial);
 		break;
+	case CP210X_PARTNUM_CP2108:
+		result = cp2108_gpioconf_init(serial);
+		break;
 	case CP210X_PARTNUM_CP2102N_QFN28:
 	case CP210X_PARTNUM_CP2102N_QFN24:
 	case CP210X_PARTNUM_CP2102N_QFN20:
-- 
2.21.0

Re: [PATCH] usb: cp210x: Add cp2108 GPIOs support

[Johan Hovold]

On Sun, Jun 16, 2019 at 01:56:36AM +0300, Serge Semin wrote:

> > > +/* CP2108 interfaces, gpio (per interface), port-blocks number, GPIO block. */
> > > +#define CP2108_IFACE_NUM		4
> > > +#define CP2108_GPIO_NUM			4
> > > +#define CP2108_PB_NUM			5
> > > +#define CP2108_GPIO_PB			1
> > 
> > Please try to give these more descriptive names; I'd prefer COUNT over
> > NUM when used as a suffix.
> > 
> > Perhaps slap an INDEX suffix on CP2108_GPIO_PB etc.
> 
> Ok. Added CNT and IDX suffixes.

Please spell out COUNT, no need to be that terse here.

> > > +/*
> > > + * CP2108 default pins state. There are five PBs. Each one is with its specific
> > > + * pins-set (see USB Express SDK sources or SDK-based smt application
> > > + * https://github.com/fancer/smt-cp210x for details).
> > > + */
> > > +struct cp2108_state {
> > > +	__le16	mode[CP2108_PB_NUM];	/* 0 - Open-Drain, 1 - Push-Pull */
> > > +	__le16	low_power[CP2108_PB_NUM];
> > > +	__le16	latch[CP2108_PB_NUM];	/* 0 - Logic Low, 1 - Logic High */
> > > +} __packed;
> > 
> > Ok, so you use mode[CP2108_GPIO_PB] to get the pin modes below;
> > what are the other "PB"s used for? Why is it a "port" block, if all 16
> > pins allocated to four different ports are accessible through one block?
> > 
> > Please try to make the comment self-contained (even if you leave some
> > details out). And perhaps we shouldn't refer to proprietary code in
> > here, and in any case the link may go away.
> > 
> 
> Ok added a bit more detailed information regarding the port blocks. But
> I'd prefer to keep the link. First of all I can't provide the complete
> description of the fields here because it would take too much space and
> in fact is pointless since only a single port block with GPIOs is utilized.
> So the link and the SDK info are a good reference to find some details
> (especially due to lacking such an info in the chip datasheet).

I never said it had to be complete, just self-contained for what it's
used for here. I don't want to go browsing random vendor code to figure
out what the code is doing when reviewing or modifying kernel code.

> Secondly even though the code is distributed under the Silicon Labs
> specific license it is open-source.

But it's not necessarily GPL compatible, which is what we care about
here.

Then again, you've already used it as reference for the protocol, so
let's keep it in.

> Finally the link may go away only
> if I removed the application from my github account, which I don't
> intend to.

Ok, didn't notice it was your account.

> > > +/*
> > > + * CP210X_VENDOR_SPECIFIC, CP210X_GET_PORTCONFIG call reads these 73 bytes.
> > > + * Reset/Suspend latches describe default states after reset/suspend of the
> > > + * pins. The rest are responsible for alternate functions settings of the
> > > + * chip pins (see USB Express SDK sources or SDK-based smt application
> > > + * https://github.com/fancer/smt-cp210x for details).
> > > + */
> > > +struct cp2108_config {
> > > +	struct cp2108_state reset_latch;
> > > +	struct cp2108_state suspend_latch;
> > > +	u8	ip_delay[CP2108_IFACE_NUM];
> > > +	u8	enhanced_fxn[CP2108_IFACE_NUM];
> > > +	u8	enhanced_fxn_dev;
> > > +	u8	ext_clock_freq[CP2108_IFACE_NUM];
> > > +} __packed;
> > > +
> > > +/* CP2108 port alternate functions fields */

So it's really the enhanced_fxn field above right (and not about pin
alternate functions only).

> > > +#define CP2108_GPIO_TXLED_MODE		BIT(0)
> > > +#define CP2108_GPIO_RXLED_MODE		BIT(1)
> > > +#define CP2108_GPIO_RS485_MODE		BIT(2)
> > > +#define CP2108_GPIO_RS485_LOGIC		BIT(3)
> > > +#define CP2108_GPIO_CLOCK_MODE		BIT(4)
> > > +#define CP2108_DYNAMIC_SUSPEND		BIT(5)
> > 
> > No GPIO infix?
> 
> No, because this refers to all pins state when the chip being suspended
> (whether the suspend_latch state is pushed to the pins at the suspend USB
> state), while the rest of the macros are defined for the GPIOs alternative
> functions. I'll add the MODE suffix though.

Ok, does the pins maintain their state if DYNAMIC_SUSPEND is set or cleared?

> > > +	int result, bufsize = sizeof(buf.single);
> > 
> > One declaration per line please, especially when initialising.
> 
> Done. Though I can't remember this being requirement in the kernel
> coding style. Could you give me a link to the corresponding statement?

It's actually mentioned in passing in the coding style, but with a
different motivation.

The general idea is just to avoid unnecessarily terse code.

> > > +
> > > +	/*
> > > +	 * Move the GPIO clock alternative function bit value to the fourth bit
> > > +	 * as the corresponding GPIO pin reside. It shall make the generic
> > > +	 * cp210x GPIO request method being suitable for cp2108 as well.
> > > +	 */
> > 
> > This isn't entirely clear, please try to rephrase.
> > 
> > Which functions are available on which pins? Do the function defines
> > need to be renamed to reflect the pin numbers as for CP2104?
> 
> I wouldn't rename the macros'es, since they describe the functions of
> enhanced_fxn bits. BTW CP2104 doesn't do any renaming anyway, while it
> is doing the same thing as I am here. Instead I added a more descriptive
> comment, so the code hackers would see the reason of the BITs copy-pasting.

My point was that the corresponding macros for cp2104 includes the pin
number in the name (e.g. CP2104_GPIO1_RXLED_MODE), which makes it clear
on which pin each alternate function is available.

> > > +	priv->gpio_altfunc &= ~BIT(3);
> > > +	if (priv->gpio_altfunc & CP2108_GPIO_CLOCK_MODE)
> > > +		priv->gpio_altfunc |= BIT(3);
> > > +
> > > +	/*
> > > +	 * Open-drain mode in combination with a high latch value is used
> > > +	 * to emulate the GPIO input pin.
> > > +	 */
> > > +	priv->gpio_input &= ~priv->gpio_pushpull;
> > 
> > Input mode should only be set when the reset latch value is 1 (see
> > cp2104).
> 
> Yes, and this code does the same thing as the loop in cp2104, but in a single
> line. BTW the cp2104 cp2104_gpioconf_init() method can be simplified in the
> same way.stash@{0}stash@{0}

Not in a single line, you initialised gpio_input to the latch values
several lines above. Please keep the logic in one place, and use
temporaries where appropriate to make the code self-documenting.

I'd prefer a comment here along the lines of those in the other gpio
init functions (e.g. "set input mode iff open-drain and high latch
value").

> It should be also noted that in v2 I fixed a cp2108-related bug in the
> cp210x_gpio_get()/cp210x_gpio_set() methods. So aside from the requested
> alterations the logic of the cp2108 GPIOs setter/getter is bit different
> now.

Ah, guess you had only tested the first port's pins? Possibly another
reason for adding a dedicated cp2108 callback.

I'll comment on the patch itself.

Johan

Re: [PATCH v2] usb: cp210x: Add cp2108 GPIOs support

[Johan Hovold]

On Sun, Jun 16, 2019 at 02:02:15AM +0300, Serge Semin wrote:
> Each chip from the cp210x series got GPIOs on board. This commit
> provides the support for sixteen ones placed on the cp2108 four-ports
> serial console controller. Since all GPIOs are accessible via any
> cp2108 USB interface we manually and equally distributed them between
> all interfaces in accordance with GPIOs alternative functions attachment.
> So the driver provides four GPIOs per each cp2108 USB interface.
> 
> cp2108 GPIOs can be either in open-drain or push-pull modes setup once
> after reset was cleared. In this matter it doesn't differ from the rest
> of cp210x devices supported by the driver. So with minor alterations the
> standard output/intput GPIO interface is implemented for cp2108.
> 
> Aside from traditional GPIO functions like setting/getting pins value,
> each GPIO is also multiplexed with alternative functions: TX/RX LEDs, RS485
> TX-enable and Clocks source. These functions can't be activated on-fly.
> Instead the chips firmware should be properly setup, so they would be
> enabled in the ports-config structure at the controller logic startup.
> Needless to say, that when the alternative functions are activated,
> the GPIOs can't be used. Thus we need to check the GPIO pin config in the
> request callback and deny the request if GPIO standard function is
> disabled.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
> 
> Changelog v2:
> - Rebase onto kernel 5.2.0-rc4.
> - Move USB_SERIAL_CP210X config descriptor modification to a separate patch.
> - Add more descriptive info of cp2108 state and config structures.
> - Replace _NUM suffix with _CNT.
> - Add _IDX suffix to CP2108_GPIO_PB macro.
> - Add _MODE suffix to CP2108_DYNAMIC_SUSPEND macro.
> - Discard __packed attribute from struct cp2108_gpio_write.
> - Discard CP2108_GPIO_CNT macro and use literal 4 instead.
> - Simplify cp210x_gpio_get() method by replacing the union with a u16 buffer.
> - Replace ternary operations of cp210x_gpio_set() with a conditional statement.
> - Add more descriptive comments regarding the gpio_altfunc bits collection.
> - Fix a bug in the GPIOs setter/getter methods of invalid bit being utilized
> to set/get GPIO values.
> ---
>  drivers/usb/serial/cp210x.c | 178 +++++++++++++++++++++++++++++++++---
>  1 file changed, 164 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index 979bef9bfb6b..32f0a4273abb 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -505,6 +505,60 @@ struct cp210x_gpio_write {
>  	u8	state;
>  } __packed;
>  
> +/*
> + * Number of CP2108 USB interfaces, port-blocks and GPIO port-block index,
> + * where port-blocks are the internal pins containers of the chip.
> + */
> +#define CP2108_IFACE_CNT		4
> +#define CP2108_PB_CNT			5
> +#define CP2108_GPIO_PB_IDX		1

s/CNT/COUNT/

> +/*
> + * CP2108 default pins state. There are five port-blocks (PB). Each one is with

pin state?

> + * it' specific pins-set: Port 0 - UART 0 and 1, Port 1 - GPIOs, Port 2 - chip

typos: it's, pin-set

> + * suspend and a part of UART 2 pins, Port 3 and 4 - UART 2 and 3 pins

Probably more readable as a table (one line per port block).

> + * (for details see USB Express SDK sources or SDK-based smt application
> + * accessible here https://github.com/fancer/smt-cp210x).
> + */
> +struct cp2108_state {
> +	__le16	mode[CP2108_PB_CNT];	/* 0 - Open-Drain, 1 - Push-Pull */
> +	__le16	low_power[CP2108_PB_CNT];
> +	__le16	latch[CP2108_PB_CNT];	/* 0 - Logic Low, 1 - Logic High */
> +} __packed;

Still not clear to me how these affect one of the UARTs pins, or what
the "chip suspend" port-block is.

Do you think you can add something short about that?

> +/*
> + * CP210X_VENDOR_SPECIFIC, CP210X_GET_PORTCONFIG call reads these 73 bytes.
> + * Reset/Suspend latches describe default states after reset/suspend of the
> + * pins.

Don't we need to consider also the suspend state when declaring as
input?

By leaving the pin as an output if it will be driven high during
suspend, users would at least get an indication that things may not be
configured as expected.

>           The rest are responsible for alternate functions settings of the
> + * chip pins (see USB Express SDK sources or SDK-based smt application
> + * https://github.com/fancer/smt-cp210x for details).
> + */
> +struct cp2108_config {
> +	struct cp2108_state reset_latch;
> +	struct cp2108_state suspend_latch;
> +	u8	ip_delay[CP2108_IFACE_CNT];
> +	u8	enhanced_fxn[CP2108_IFACE_CNT];
> +	u8	enhanced_fxn_dev;
> +	u8	ext_clock_freq[CP2108_IFACE_CNT];
> +} __packed;
> +
> +/* CP2108 port alternate functions fields. */
> +#define CP2108_GPIO_TXLED_MODE		BIT(0)
> +#define CP2108_GPIO_RXLED_MODE		BIT(1)
> +#define CP2108_GPIO_RS485_MODE		BIT(2)
> +#define CP2108_GPIO_RS485_LOGIC		BIT(3)
> +#define CP2108_GPIO_CLOCK_MODE		BIT(4)
> +#define CP2108_DYNAMIC_SUSPEND_MODE	BIT(5)
> +
> +/*
> + * CP210X_VENDOR_SPECIFIC, CP210X_WRITE_LATCH call writes these 0x4 bytes
> + * to CP2108 controller.
> + */
> +struct cp2108_gpio_write {
> +	__le16	mask;
> +	__le16	state;
> +};
> +
>  /*
>   * Helper to get interface number when we only have struct usb_serial.
>   */
> @@ -1366,10 +1420,13 @@ static int cp210x_gpio_get(struct gpio_chip *gc, unsigned int gpio)
>  	struct usb_serial *serial = gpiochip_get_data(gc);
>  	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
>  	u8 req_type = REQTYPE_DEVICE_TO_HOST;
> +	int bufsize = 1;
>  	int result;
> -	u8 buf;
> +	u16 buf;
>  
> -	if (priv->partnum == CP210X_PARTNUM_CP2105)
> +	if (priv->partnum == CP210X_PARTNUM_CP2108)
> +		bufsize = 2;
> +	else if (priv->partnum == CP210X_PARTNUM_CP2105)
>  		req_type = REQTYPE_INTERFACE_TO_HOST;
>  
>  	result = usb_autopm_get_interface(serial->interface);
> @@ -1377,39 +1434,62 @@ static int cp210x_gpio_get(struct gpio_chip *gc, unsigned int gpio)
>  		return result;
>  
>  	result = cp210x_read_vendor_block(serial, req_type,
> -					  CP210X_READ_LATCH, &buf, sizeof(buf));
> +					  CP210X_READ_LATCH, &buf, bufsize);
>  	usb_autopm_put_interface(serial->interface);
>  	if (result < 0)
>  		return result;
>  
> -	return !!(buf & BIT(gpio));
> +	if (priv->partnum == CP210X_PARTNUM_CP2108) {
> +		u8 intf_num = cp210x_interface_num(serial);
> +
> +		gpio += intf_num * 4;
> +	}

So with the gpio-index bug fixed we still get a conditional here...

Did you try adding a dedicated cp2108x_gpio_get callback (I'm aware that
the generic init code needs to be updated for this)?

> +
> +	return !!(le16_to_cpu(buf) & BIT(gpio));
>  }
>  
>  static void cp210x_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
>  {
>  	struct usb_serial *serial = gpiochip_get_data(gc);
>  	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
> -	struct cp210x_gpio_write buf;
>  	int result;
>  
> -	if (value == 1)
> -		buf.state = BIT(gpio);
> -	else
> -		buf.state = 0;
> -
> -	buf.mask = BIT(gpio);
> -
>  	result = usb_autopm_get_interface(serial->interface);
>  	if (result)
>  		goto out;
>  
> -	if (priv->partnum == CP210X_PARTNUM_CP2105) {
> +	if (priv->partnum == CP210X_PARTNUM_CP2108) {
> +		u8 intf_num = cp210x_interface_num(serial);
> +		struct cp2108_gpio_write buf;
> +
> +		buf.mask = cpu_to_le16(BIT(intf_num * 4 + gpio));
> +		if (value == 1)
> +			buf.state = buf.mask;
> +		else
> +			buf.state = 0;
> +
> +		result = cp210x_write_vendor_block(serial,
> +						   REQTYPE_HOST_TO_DEVICE,
> +						   CP210X_WRITE_LATCH, &buf,
> +						   sizeof(buf));

And if adding a cp2108 get callback, you may as well break this out as
well.

> +	} else if (priv->partnum == CP210X_PARTNUM_CP2105) {
> +		struct cp210x_gpio_write buf;
> +
> +		buf.mask = BIT(gpio);
> +		if (value == 1)
> +			buf.state = buf.mask;
> +		else
> +			buf.state = 0;
> +
>  		result = cp210x_write_vendor_block(serial,
>  						   REQTYPE_HOST_TO_INTERFACE,
>  						   CP210X_WRITE_LATCH, &buf,
>  						   sizeof(buf));
>  	} else {
> -		u16 wIndex = buf.state << 8 | buf.mask;
> +		u16 wIndex = BIT(gpio);
> +
> +		if (value == 1)
> +			wIndex |= (BIT(gpio) << 8);
>  
>  		result = usb_control_msg(serial->dev,
>  					 usb_sndctrlpipe(serial->dev, 0),
> @@ -1489,6 +1569,73 @@ static int cp210x_gpio_set_config(struct gpio_chip *gc, unsigned int gpio,
>  	return -ENOTSUPP;
>  }
>  
> +/*
> + * CP2108 got 16 GPIOs, each of which can be configured either as input, or
> + * as open-drain with weak pulling up to VIO or as push-pull with strong
> + * pulling up to VIO. Similar to the rest of devices the open-drain mode
> + * with latch set high is treated as input mode. All GPIOs are equally
> + * distributed between four interfaces. Thanks to the mask-state based
> + * write-latch control message we don't need to worry about possible races.
> + */
> +static int cp2108_gpioconf_init(struct usb_serial *serial)
> +{
> +	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
> +	struct cp2108_config config;
> +	u16 mode, latch;
> +	u8 intf_num;
> +	int result;
> +
> +	result = cp210x_read_vendor_block(serial, REQTYPE_DEVICE_TO_HOST,
> +					  CP210X_GET_PORTCONFIG, &config,
> +					  sizeof(config));
> +	if (result < 0)
> +		return result;
> +
> +	/*
> +	 * There are four interfaces with four GPIOs manually distributed for
> +	 * each of them. Here we parse the device config data to comply with
> +	 * the driver interface. Note that the mode (push-pull, open-drain,
> +	 * etc) can be changed only after reset, which causes the driver
> +	 * reloading anyway. So we can safely read the config just once at the
> +	 * device attachment procedure.
> +	 */
> +	intf_num = cp210x_interface_num(serial);
> +	if (intf_num >= CP2108_IFACE_CNT)
> +		return -ENODEV;
> +	mode = le16_to_cpu(config.reset_latch.mode[CP2108_GPIO_PB_IDX]);
> +	latch = le16_to_cpu(config.reset_latch.latch[CP2108_GPIO_PB_IDX]);
> +
> +	priv->gpio_altfunc = config.enhanced_fxn[intf_num];

gpio_altfunc isn't the same as as enhanced_fxn; use a temporary or move
where you finally set gpio_altfunc (which is a per-pin bitmask).

> +	priv->gpio_pushpull = (mode >> (intf_num * 4)) & 0x0f;
> +	priv->gpio_input = (latch >> (intf_num * 4)) & 0x0f;

Add a temporary or reuse latch here; this isn't gpio_input which is set
below.

> +	priv->gc.ngpio = 4;
> +
> +	/*
> +	 * Each GPIO[i*4 + x] can have an alternative function enabled
> +	 * (x = 0 - UART i TX toggle, x = 1 - UART i RX toggle, x = 2 - UART i
> +	 * RS485 mode and x = 3  - clock output i), in which case the GPIO

A table would be more readable (i.e. one value per line).

> +	 * functionality isn't available. If it is switched on we deny the GPIO
> +	 * requests. GPIO alternative functions state resides the
> +	 * enhanced_fxn[i] bitfield, where each bit linearly corresponds to
> +	 * the specific GPIO, except GPIO[3] - clock output function, which
> +	 * alas is placed at fourth bit, while the third bit is busy with RS485
> +	 * logic mode flag. In order to make the generic cp210x GPIO request
> +	 * method being suitable for cp2108, lets copy the GPIO[4] clock
> +	 * alternative function state bit to the GPIO[3] RS485 logic mode bit.
> +	 */

So instead of this wall of text, why not use the pattern from the other
init functions, which is mostly self-explantory? That is, four
conditionals like:

	if (enhanced_fxn & CP2108_GPIO_CLOCK_MODE)
		priv->gpio_altfunc |= BIT(3);

> +	priv->gpio_altfunc &= ~CP2108_GPIO_RS485_LOGIC;
> +	if (priv->gpio_altfunc & CP2108_GPIO_CLOCK_MODE)
> +		priv->gpio_altfunc |= CP2108_GPIO_RS485_LOGIC;

This really doesn't make any sense without the comment and knowing the
values of the macros, which kind of defeats their purpose.

> +
> +	/*
> +	 * Open-drain mode in combination with a high latch value is used
> +	 * to emulate the GPIO input pin.
> +	 */

This only hints at how we implement input mode. Also add something about
the initial direction which is what the following code is setting, along
the lines of:

	Set direction to "input" iff pin is open-drain and reset
	value is 1.

> +	priv->gpio_input &= ~priv->gpio_pushpull;

	priv->gpio_input = latch & ~priv->gpio_pushpull

where latch has been shifted as required (here or above).

Johan