linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v8] USB: serial: cp210x: Add support for GPIOs on CP2108
@ 2021-04-06 10:18 Pho Tran
  2021-04-06 13:10 ` kernel test robot
       [not found] ` <CAHp75Vf8jwhLkaHL2D6FvRJpmbBqpTzePpNqVAFVt8EhSCgxnw@mail.gmail.com>
  0 siblings, 2 replies; 4+ messages in thread
From: Pho Tran @ 2021-04-06 10:18 UTC (permalink / raw)
  To: johan, gregkh; +Cc: linux-usb, linux-kernel, Hung.Nguyen, Tung.Pham, Pho Tran

From: Pho Tran <pho.tran@silabs.com>

Similar to other CP210x devices, GPIO interfaces (gpiochip) should be
supported for CP2108.

CP2108 has 4 serial interfaces but only 1 set of GPIO pins are shared
to all of those interfaces. So, just need to initialize GPIOs of CP2108
with only one interface (I use interface 0). It means just only 1 gpiochip
device file will be created for CP2108.

CP2108 has 16 GPIOs, So data types of several variables need to be is u16
instead of u8(in struct cp210x_serial_private). This doesn't affect other
CP210x devices.

Because CP2108 has 16 GPIO pins, the parameter passed by cp210x functions
will be different from other CP210x devices. So need to check part number
of the device to use correct data format  before sending commands to
devices.

Like CP2104, CP2108 have GPIO pins with configurable options. Therefore,
should be mask all pins which are not in GPIO mode in cp2108_gpio_init()
function.

Signed-off-by: Pho Tran <pho.tran@silabs.com>
---

04/05/2021: Patch v7 Modified commit message follow Greg's comment.
04/05/2021: Patch v6 Fixed build warning reported by kernel test robot.
03/15/2021: Patch v5 Modified code according to comment of Johan:
	1. Unified the handling of CP2108 and other types and
	take care about endianness.
	2. Used suitable types data for variable.
	3. Fixed cp2108_gpio_init and add more detail on
	commit message and comment.
	4. Dropped some of the ones that don't add any value.
03/12/2021: Patch v4 used git send-mail instead of send patch by manual
follow the instructions of Johan Hovold <johan@kernel.org>.
03/05/2021: Patch v3 modified format and contents of changelog follow feedback
from Johan Hovold <johan@kernel.org>.
03/04/2021: Patch v2 modified format patch as comment from
Johan Hovold <johan@kernel.org>:
	1. Break commit message lines at 80 cols
	2. Use kernel u8 and u16 instead of the c99 ones.
03/01/2021: Initialed submission of patch "Make the CP210x driver work with
GPIOs of CP2108.".

 drivers/usb/serial/cp210x.c | 253 +++++++++++++++++++++++++++++++-----
 1 file changed, 219 insertions(+), 34 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 7bec1e730b20..300e4b77c26c 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -245,9 +245,9 @@ struct cp210x_serial_private {
 #ifdef CONFIG_GPIOLIB
 	struct gpio_chip	gc;
 	bool			gpio_registered;
-	u8			gpio_pushpull;
-	u8			gpio_altfunc;
-	u8			gpio_input;
+	u16			gpio_pushpull;
+	u16			gpio_altfunc;
+	u16			gpio_input;
 #endif
 	u8			partnum;
 	speed_t			min_speed;
@@ -399,6 +399,18 @@ static struct usb_serial_driver * const serial_drivers[] = {
 #define CP210X_PARTNUM_CP2102N_QFN20	0x22
 #define CP210X_PARTNUM_UNKNOWN	0xFF
 
+/*
+ * CP2108 Define bit locations for EnhancedFxn_IFCx
+ * Refer to https://www.silabs.com/documents/public/application-notes/an978-cp210x-usb-to-uart-api-specification.pdf
+ * for more information.
+ */
+#define EF_IFC_GPIO_TXLED		0x01
+#define EF_IFC_GPIO_RXLED		0x02
+#define EF_IFC_GPIO_RS485		0x04
+#define EF_IFC_GPIO_RS485_LOGIC 0x08
+#define EF_IFC_GPIO_CLOCK		0x10
+#define EF_IFC_DYNAMIC_SUSPEND	0x40
+
 /* CP210X_GET_COMM_STATUS returns these 0x13 bytes */
 struct cp210x_comm_status {
 	__le32   ulErrors;
@@ -500,6 +512,45 @@ struct cp210x_single_port_config {
 	u8	device_cfg;
 } __packed;
 
+/*
+ * Quad Port Config definitions
+ * Refer to https://www.silabs.com/documents/public/application-notes/an978-cp210x-usb-to-uart-api-specification.pdf
+ * for more information.
+ * CP210X_VENDOR_SPECIFIC, CP210X_GET_PORTCONFIG call reads these 0x49 bytes
+ * on a CP2108 chip.
+ * CP2108 Quad Port State structure(used in Quad Port Config structure)
+ */
+struct cp210x_quad_port_state {
+	__le16 gpio_mode_PB0;
+	__le16 gpio_mode_PB1;
+	__le16 gpio_mode_PB2;
+	__le16 gpio_mode_PB3;
+	__le16 gpio_mode_PB4;
+
+
+	__le16 gpio_lowpower_PB0;
+	__le16 gpio_lowpower_PB1;
+	__le16 gpio_lowpower_PB2;
+	__le16 gpio_lowpower_PB3;
+	__le16 gpio_lowpower_PB4;
+
+	__le16 gpio_latch_PB0;
+	__le16 gpio_latch_PB1;
+	__le16 gpio_latch_PB2;
+	__le16 gpio_latch_PB3;
+	__le16 gpio_latch_PB4;
+};
+
+// Cp2108 Quad Port Config structure
+struct cp210x_quad_port_config {
+	struct cp210x_quad_port_state reset_state;
+	struct cp210x_quad_port_state suspend_state;
+	u8 ipdelay_IFC[4];
+	u8 enhancedfxn_IFC[4];
+	u8 enhancedfxn_device;
+	u8 extclkfreq[4];
+} __packed;
+
 /* GPIO modes */
 #define CP210X_SCI_GPIO_MODE_OFFSET	9
 #define CP210X_SCI_GPIO_MODE_MASK	GENMASK(11, 9)
@@ -510,6 +561,9 @@ struct cp210x_single_port_config {
 #define CP210X_GPIO_MODE_OFFSET		8
 #define CP210X_GPIO_MODE_MASK		GENMASK(11, 8)
 
+#define CP2108_GPIO_MODE_OFFSET		0
+#define CP2108_GPIO_MODE_MASK		GENMASK(15, 0)
+
 /* CP2105 port configuration values */
 #define CP2105_GPIO0_TXLED_MODE		BIT(0)
 #define CP2105_GPIO1_RXLED_MODE		BIT(1)
@@ -526,12 +580,31 @@ struct cp210x_single_port_config {
 #define CP210X_2NCONFIG_GPIO_RSTLATCH_IDX	587
 #define CP210X_2NCONFIG_GPIO_CONTROL_IDX	600
 
-/* CP210X_VENDOR_SPECIFIC, CP210X_WRITE_LATCH call writes these 0x2 bytes. */
-struct cp210x_gpio_write {
+/*
+ * CP210X_VENDOR_SPECIFIC, CP210X_WRITE_LATCH call writes these
+ * 0x04 bytes on CP2108.
+ */
+struct cp210x_16gpios_write {
+	__le16	mask;
+	__le16	state;
+};
+
+/*
+ * CP210X_VENDOR_SPECIFIC, CP210X_WRITE_LATCH call writes these
+ * 0x02 bytes on CP2102N, Cp2103, Cp2104 and CP2105.
+ */
+struct cp210x_8gpios_write {
 	u8	mask;
 	u8	state;
 };
 
+//Struct cp210x_gpio_write include devices have both of 8 gpios and 16 gpios.
+struct cp210x_gpio_write {
+	struct cp210x_8gpios_write cp210x_8gpios;
+	struct cp210x_16gpios_write cp210x_16gpios;
+};
+
+
 /*
  * Helper to get interface number when we only have struct usb_serial.
  */
@@ -1298,21 +1371,45 @@ static int cp210x_gpio_get(struct gpio_chip *gc, unsigned int gpio)
 	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)
-		req_type = REQTYPE_INTERFACE_TO_HOST;
+	u16 buf;
 
 	result = usb_autopm_get_interface(serial->interface);
 	if (result)
 		return result;
-
-	result = cp210x_read_vendor_block(serial, req_type,
-					  CP210X_READ_LATCH, &buf, sizeof(buf));
-	usb_autopm_put_interface(serial->interface);
+/*
+ * This function will be read latch value of gpio and storage to buf(16bit)
+ * where bit 0 is GPIO0, bit 1 is GPIO1, etc. Up to GPIOn where n is
+ * total number of GPIO pins the interface supports.
+ * Interfaces on CP2102N supports 7 GPIOs
+ * Interfaces on CP2103 amd CP2104 supports 4 GPIOs
+ * Enhanced interfaces on CP2105 support 3 GPIOs
+ * Standard interfaces on CP2105 support 4 GPIOs
+ * Interfaces on CP2108 supports 16 GPIOs
+ */
+	switch (priv->partnum) {
+	/*
+	 * Request type to Read_Latch of CP2105 and Cp2108
+	 * is 0xc1 <REQTYPE_INTERFACE_TO_HOST>
+	 */
+	case CP210X_PARTNUM_CP2108:
+		req_type = REQTYPE_INTERFACE_TO_HOST;
+		result = cp210x_read_vendor_block(serial, req_type,
+						CP210X_READ_LATCH, &buf, sizeof(__le16));
+		break;
+	case CP210X_PARTNUM_CP2105:
+		req_type = REQTYPE_INTERFACE_TO_HOST;
+		result = cp210x_read_vendor_block(serial, req_type,
+						CP210X_READ_LATCH, &buf, sizeof(u8));
+		break;
+	default:
+		result = cp210x_read_vendor_block(serial, req_type,
+						CP210X_READ_LATCH, &buf, sizeof(u8));
+		break;
+	}
 	if (result < 0)
 		return result;
-
+	buf = le16_to_cpu(buf);
+	usb_autopm_put_interface(serial->interface);
 	return !!(buf & BIT(gpio));
 }
 
@@ -1321,37 +1418,49 @@ 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;
+	u16 wIndex;
 	int result;
 
-	if (value == 1)
-		buf.state = BIT(gpio);
-	else
-		buf.state = 0;
-
-	buf.mask = BIT(gpio);
+	if (value == 1) {
+		buf.cp210x_8gpios.state = BIT(gpio);
+		buf.cp210x_16gpios.state = cpu_to_le16(BIT(gpio));
+	} else {
+		buf.cp210x_8gpios.state = 0;
+		buf.cp210x_16gpios.state = 0;
+	}
+	buf.cp210x_8gpios.mask = BIT(gpio);
+	buf.cp210x_16gpios.mask = cpu_to_le16(BIT(gpio));
 
 	result = usb_autopm_get_interface(serial->interface);
 	if (result)
 		goto out;
 
-	if (priv->partnum == CP210X_PARTNUM_CP2105) {
+	switch (priv->partnum) {
+	case CP210X_PARTNUM_CP2108:
 		result = cp210x_write_vendor_block(serial,
-						   REQTYPE_HOST_TO_INTERFACE,
-						   CP210X_WRITE_LATCH, &buf,
-						   sizeof(buf));
-	} else {
-		u16 wIndex = buf.state << 8 | buf.mask;
-
+							REQTYPE_HOST_TO_INTERFACE,
+							CP210X_WRITE_LATCH, &buf.cp210x_16gpios,
+							sizeof(buf.cp210x_16gpios));
+		break;
+	case CP210X_PARTNUM_CP2105:
+		result = cp210x_write_vendor_block(serial,
+							REQTYPE_HOST_TO_INTERFACE,
+							CP210X_WRITE_LATCH, &buf.cp210x_8gpios,
+							sizeof(buf.cp210x_8gpios));
+		break;
+	default:
+		wIndex = buf.cp210x_8gpios.state << 8 | buf.cp210x_8gpios.mask;
 		result = usb_control_msg(serial->dev,
-					 usb_sndctrlpipe(serial->dev, 0),
-					 CP210X_VENDOR_SPECIFIC,
-					 REQTYPE_HOST_TO_DEVICE,
-					 CP210X_WRITE_LATCH,
-					 wIndex,
-					 NULL, 0, USB_CTRL_SET_TIMEOUT);
+							usb_sndctrlpipe(serial->dev, 0),
+							CP210X_VENDOR_SPECIFIC,
+							REQTYPE_HOST_TO_DEVICE,
+							CP210X_WRITE_LATCH,
+							wIndex,
+							NULL, 0, USB_CTRL_SET_TIMEOUT);
+		break;
 	}
-
 	usb_autopm_put_interface(serial->interface);
+
 out:
 	if (result < 0) {
 		dev_err(&serial->interface->dev, "failed to set GPIO value: %d\n",
@@ -1420,6 +1529,73 @@ static int cp210x_gpio_set_config(struct gpio_chip *gc, unsigned int gpio,
 	return -ENOTSUPP;
 }
 
+static int cp2108_gpio_init(struct usb_serial *serial)
+{
+	struct cp210x_serial_private *priv = usb_get_serial_data(serial);
+	struct cp210x_quad_port_config config;
+	u16 gpio_latch;
+	u16 temp;
+	int result;
+	u8 i;
+
+	result = cp210x_read_vendor_block(serial, REQTYPE_DEVICE_TO_HOST,
+					  CP210X_GET_PORTCONFIG, &config,
+					  sizeof(config));
+	if (result < 0)
+		return result;
+	priv->gc.ngpio = 16;
+	temp = le16_to_cpu(config.reset_state.gpio_mode_PB1);
+	priv->gpio_pushpull = (temp & CP2108_GPIO_MODE_MASK) >> CP2108_GPIO_MODE_OFFSET;
+	temp = le16_to_cpu(config.reset_state.gpio_latch_PB1);
+	gpio_latch = (temp & CP2108_GPIO_MODE_MASK) >> CP2108_GPIO_MODE_OFFSET;
+	/*
+	 * Mark all pins which are not in GPIO mode
+	 * Refer to table 9.1: GPIO Mode alternate Functions on CP2108 datasheet:
+	 * https://www.silabs.com/documents/public/data-sheets/cp2108-datasheet.pdf
+	 * Alternate Functions of GPIO0 to GPIO3 is determine by enhancedfxn_IFC[0]
+	 * and the same for other pins, enhancedfxn_IFC[1]: GPIO4 to GPIO7,
+	 * enhancedfxn_IFC[2]: GPIO8 to GPIO11, enhancedfxn_IFC[3]: GPIO12 to GPIO15.
+	 */
+	for (i = 0; i < 4; i++) {
+		switch (config.enhancedfxn_IFC[i]) {
+		case EF_IFC_GPIO_TXLED:
+			priv->gpio_altfunc |= BIT(i * 4);
+			break;
+		case EF_IFC_GPIO_RXLED:
+			priv->gpio_altfunc |= BIT((i * 4) + 1);
+			break;
+		case EF_IFC_GPIO_RS485_LOGIC:
+		case EF_IFC_GPIO_RS485:
+			priv->gpio_altfunc |= BIT((i * 4) + 2);
+			break;
+		case EF_IFC_GPIO_CLOCK:
+			priv->gpio_altfunc |= BIT((i * 4) + 3);
+			break;
+		case EF_IFC_DYNAMIC_SUSPEND:
+			priv->gpio_altfunc |= BIT(i * 4);
+			priv->gpio_altfunc |= BIT((i * 4) + 1);
+			priv->gpio_altfunc |= BIT((i * 4) + 2);
+			priv->gpio_altfunc |= BIT((i * 4) + 3);
+			break;
+		}
+	}
+	/*
+	 * Like CP2102N, CP2108 has also no strict input and output pin
+	 * modes.
+	 * Do the same input mode emulation as CP2102N.
+	 */
+	for (i = 0; i < priv->gc.ngpio; ++i) {
+		/*
+		 * Set direction to "input" iff pin is open-drain and reset
+		 * value is 1.
+		 */
+		if (!(priv->gpio_pushpull & BIT(i)) && (gpio_latch & BIT(i)))
+			priv->gpio_input |= BIT(i);
+	}
+
+	return 0;
+}
+
 /*
  * 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
@@ -1649,6 +1825,15 @@ static int cp210x_gpio_init(struct usb_serial *serial)
 	case CP210X_PARTNUM_CP2102N_QFN20:
 		result = cp2102n_gpioconf_init(serial);
 		break;
+	case CP210X_PARTNUM_CP2108:
+		/*
+		 * The GPIOs are not tied to any specific port so onlu register
+		 * once for interface 0.
+		 */
+		if (cp210x_interface_num(serial) != 0)
+			return 0;
+		result = cp2108_gpio_init(serial);
+		break;
 	default:
 		return 0;
 	}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v8] USB: serial: cp210x: Add support for GPIOs on CP2108
  2021-04-06 10:18 [PATCH v8] USB: serial: cp210x: Add support for GPIOs on CP2108 Pho Tran
@ 2021-04-06 13:10 ` kernel test robot
       [not found] ` <CAHp75Vf8jwhLkaHL2D6FvRJpmbBqpTzePpNqVAFVt8EhSCgxnw@mail.gmail.com>
  1 sibling, 0 replies; 4+ messages in thread
From: kernel test robot @ 2021-04-06 13:10 UTC (permalink / raw)
  To: Pho Tran, johan, gregkh
  Cc: kbuild-all, linux-usb, linux-kernel, Hung.Nguyen, Tung.Pham, Pho Tran

[-- Attachment #1: Type: text/plain, Size: 3514 bytes --]

Hi Pho,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on usb-serial/usb-next]
[also build test WARNING on usb/usb-testing tty/tty-testing v5.12-rc6 next-20210401]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Pho-Tran/USB-serial-cp210x-Add-support-for-GPIOs-on-CP2108/20210406-182022
base:   https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git usb-next
config: i386-randconfig-s001-20210406 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-279-g6d5d9b42-dirty
        # https://github.com/0day-ci/linux/commit/850a69bacbd236b7bbdadfa4007e7f13b3c79471
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Pho-Tran/USB-serial-cp210x-Add-support-for-GPIOs-on-CP2108/20210406-182022
        git checkout 850a69bacbd236b7bbdadfa4007e7f13b3c79471
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/usb/serial/cp210x.c:1540:15: sparse: sparse: cast to restricted __le16

vim +1540 drivers/usb/serial/cp210x.c

  1496	
  1497	static int cp210x_gpio_get(struct gpio_chip *gc, unsigned int gpio)
  1498	{
  1499		struct usb_serial *serial = gpiochip_get_data(gc);
  1500		struct cp210x_serial_private *priv = usb_get_serial_data(serial);
  1501		u8 req_type = REQTYPE_DEVICE_TO_HOST;
  1502		int result;
  1503		u16 buf;
  1504	
  1505		result = usb_autopm_get_interface(serial->interface);
  1506		if (result)
  1507			return result;
  1508	/*
  1509	 * This function will be read latch value of gpio and storage to buf(16bit)
  1510	 * where bit 0 is GPIO0, bit 1 is GPIO1, etc. Up to GPIOn where n is
  1511	 * total number of GPIO pins the interface supports.
  1512	 * Interfaces on CP2102N supports 7 GPIOs
  1513	 * Interfaces on CP2103 amd CP2104 supports 4 GPIOs
  1514	 * Enhanced interfaces on CP2105 support 3 GPIOs
  1515	 * Standard interfaces on CP2105 support 4 GPIOs
  1516	 * Interfaces on CP2108 supports 16 GPIOs
  1517	 */
  1518		switch (priv->partnum) {
  1519		/*
  1520		 * Request type to Read_Latch of CP2105 and Cp2108
  1521		 * is 0xc1 <REQTYPE_INTERFACE_TO_HOST>
  1522		 */
  1523		case CP210X_PARTNUM_CP2108:
  1524			req_type = REQTYPE_INTERFACE_TO_HOST;
  1525			result = cp210x_read_vendor_block(serial, req_type,
  1526							CP210X_READ_LATCH, &buf, sizeof(__le16));
  1527			break;
  1528		case CP210X_PARTNUM_CP2105:
  1529			req_type = REQTYPE_INTERFACE_TO_HOST;
  1530			result = cp210x_read_vendor_block(serial, req_type,
  1531							CP210X_READ_LATCH, &buf, sizeof(u8));
  1532			break;
  1533		default:
  1534			result = cp210x_read_vendor_block(serial, req_type,
  1535							CP210X_READ_LATCH, &buf, sizeof(u8));
  1536			break;
  1537		}
  1538		if (result < 0)
  1539			return result;
> 1540		buf = le16_to_cpu(buf);
  1541		usb_autopm_put_interface(serial->interface);
  1542		return !!(buf & BIT(gpio));
  1543	}
  1544	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35508 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v8] USB: serial: cp210x: Add support for GPIOs on CP2108
       [not found]   ` <CAG-T6mP98t6JuQ68bi0aD9PDEpLbkAVwrf-Gp7H7GLQV5rW_GQ@mail.gmail.com>
@ 2021-04-07 12:38     ` Andy Shevchenko
  0 siblings, 0 replies; 4+ messages in thread
From: Andy Shevchenko @ 2021-04-07 12:38 UTC (permalink / raw)
  To: Tran Van Pho
  Cc: johan, gregkh, linux-usb, linux-kernel, Hung.Nguyen, Tung.Pham, Pho Tran

On Wed, Apr 7, 2021 at 6:21 AM Tran Van Pho <photranvan0712@gmail.com> wrote:
> On Wed, Apr 7, 2021 at 5:25 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> On Tuesday, April 6, 2021, Pho Tran <photranvan0712@gmail.com> wrote:

...

>>> CP2108 has 16 GPIOs, So data types of several variables need to be is u16
>>> instead of u8(in struct cp210x_serial_private). This doesn't affect other
>>> CP210x devices.
>>
>> Here I am wondering if you should use 4 pins per interface. Is any pointer to data sheet?

> In datasheet, Using 4 pins per interface isn't specified. And when setting or getting Latch
> of GPIO, all of 16 gpios will be done on the same command. So there no difference between
> 4 interfaces of CP2108 in terms of GPIOs.

Okay, so we may have different configurations:
 1 interface + 1 GPIO (N or M pins)
 4 interfaces + 1 GPIO (N or M pins)

...

>>> Because CP2108 has 16 GPIO pins, the parameter passed by cp210x functions
>>> will be different from other CP210x devices. So need to check part number
>>> of the device to use correct data format  before sending commands to
>>> devices.
>>>
>>> Like CP2104, CP2108 have GPIO pins with configurable options. Therefore,
>>> should be mask all pins which are not in GPIO mode in cp2108_gpio_init()
>>> function.
>>
>> This I didn’t get. If you are talking about usage pin as GPIO, perhaps you should use valid_mask in GPIO chip structure. Otherwise you probably need to implement a proper pinmux ops for this (and register a pin controller which the code below also suggests).
>
>
> This message means 16 pins on  CP2108 can have many options like GPIO, TX toggle, RX toggle, clock output for example.
> If there are any pins set in the mode that aren't GPIO mode, we should mask these pins at gpio_altfunc in struct cp210x_serial_private.

Can those functions be changed at run time? Or is it simply one time setting?

> I am not clear  with what you said about pinmux, But I think "gpio_altfunc in struct cp210x_serial_private" has the same meaning as your idea.

Yeah, but it's custom grown (I believe due to historical reasons, i.e.
there was no pin controller framework at that time) approach.
If pin functions may be changed at run time, it's better to have them
described as pinmux.

...

>>> +       __le16 gpio_lowpower_PB0;
>>> +       __le16 gpio_lowpower_PB1;
>>> +       __le16 gpio_lowpower_PB2;
>>> +       __le16 gpio_lowpower_PB3;
>>> +       __le16 gpio_lowpower_PB4;
>>> +
>>> +       __le16 gpio_latch_PB0;
>>> +       __le16 gpio_latch_PB1;
>>> +       __le16 gpio_latch_PB2;
>>> +       __le16 gpio_latch_PB3;
>>> +       __le16 gpio_latch_PB4;
>>
>>
>> Sounds to me like pin controller functions rather than GPIO.
>
> Oh yes, This is related to the functions of pins. But we need to define it to get the state of the pins to
> check whether It's  GPIO mode or other function.

Yep, and this is pinmux functionality, no?

> It's also included in vendor specified USB packet structure.
> You can refer to https://www.silabs.com/documents/public/application-notes/an978-cp210x-usb-to-uart-api-specification.pdf
> at page34 to see struct _QUAD_PORT_CONFIG.
>
> I have  a question, Should I need to resolve the build warning on i386 reported by the kernel test robot?

If you get a warning, yes, it's better to resolve it.

>>> +       struct cp210x_quad_port_state reset_state;
>>> +       struct cp210x_quad_port_state suspend_state;
>>> +       u8 ipdelay_IFC[4];
>>> +       u8 enhancedfxn_IFC[4];
>>> +       u8 enhancedfxn_device;
>>> +       u8 extclkfreq[4];
>>> +} __packed;

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v8] USB: serial: cp210x: Add support for GPIOs on CP2108
       [not found] ` <CAHp75Vf8jwhLkaHL2D6FvRJpmbBqpTzePpNqVAFVt8EhSCgxnw@mail.gmail.com>
       [not found]   ` <CAG-T6mP98t6JuQ68bi0aD9PDEpLbkAVwrf-Gp7H7GLQV5rW_GQ@mail.gmail.com>
@ 2021-04-09 15:50   ` Johan Hovold
  1 sibling, 0 replies; 4+ messages in thread
From: Johan Hovold @ 2021-04-09 15:50 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Pho Tran, gregkh, linux-usb, linux-kernel, Hung.Nguyen,
	Tung.Pham, Pho Tran

On Wed, Apr 07, 2021 at 01:25:00AM +0300, Andy Shevchenko wrote:
> On Tuesday, April 6, 2021, Pho Tran <photranvan0712@gmail.com> wrote:

> > Because CP2108 has 16 GPIO pins, the parameter passed by cp210x functions
> > will be different from other CP210x devices. So need to check part number
> > of the device to use correct data format  before sending commands to
> > devices.
> >
> > Like CP2104, CP2108 have GPIO pins with configurable options. Therefore,
> > should be mask all pins which are not in GPIO mode in cp2108_gpio_init()
> > function.

> This I didn’t get. If you are talking about usage pin as GPIO, perhaps you
> should use valid_mask in GPIO chip structure. Otherwise you probably need
> to implement a proper pinmux ops for this (and register a pin controller
> which the code below also suggests).

Neither is needed here.

Using a valid mask is a new feature and isn't a prerequisite for adding
support for the GPIOs on cp2108. I've been meaning to implement that
since we started using it for ftdi_sio, and I'll be posting that
shortly.

The cp2108 pin configuration can't be changed at runtime, but even if it
was it's not clear what the pinctrl subsystem would buy us for a
hotpluggable USB device currently (even if devicetree could be used for
static topologies).

I'll look at the rest of this thread and the latest version of this
patch next week.

Johan

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-04-09 15:50 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-06 10:18 [PATCH v8] USB: serial: cp210x: Add support for GPIOs on CP2108 Pho Tran
2021-04-06 13:10 ` kernel test robot
     [not found] ` <CAHp75Vf8jwhLkaHL2D6FvRJpmbBqpTzePpNqVAFVt8EhSCgxnw@mail.gmail.com>
     [not found]   ` <CAG-T6mP98t6JuQ68bi0aD9PDEpLbkAVwrf-Gp7H7GLQV5rW_GQ@mail.gmail.com>
2021-04-07 12:38     ` Andy Shevchenko
2021-04-09 15:50   ` Johan Hovold

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).