From: Pho Tran <[email protected]>
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 <[email protected]>
Co-developed-by: Tung Pham <[email protected]>
Signed-off-by: Tung Pham <[email protected]>
---
04/26/2021: Patch v11 Add Signed-off-by Tung Pham
04/26/2021: Patch v10 Add Co-developed-by Tung Pham
04/23/2021: Patch v9 Modified code according to comment of Johan:
1. Remove quad-port-config comment.
2. Move defice EF_IFC_GPIOs go after the quad-port-config
structure with the other port config defines, add CP2108_ prefix.
3. Drop CP2108 Quad Port State structure sentence comment;
clear from the code.
4. remove Double new line.
5. Use lowercase pb for consistency.
6. Avoid // comments. CP2108 (uppercase P).
7. remove Double new line.
8. Lowercase ifc.
9. Just keep the current struct as is and add a second one for 16 bits
(e.g. struct cp210x_gpio_write16).
10. don't need both u16 buf, but it may be cleaner to use u8 buf[2]
for the transfer buffer.
11. Add missing indentation.
12. Try to be consistent with capitalisation of CP210x.
13. Just do the read after the switch into a common u8 buf[2] and
use the full length only for CP2108.
14. Add leak the PM counter reference here.
15. Change buf = le16_to_cpu(wbuf); to le16_to_cpup((__le16 *)buf).
16. Keep the original struct as is cp210x_gpio_write.
17. Add another one for cp2108 with a suitable suffix.
18. No need to do open-parenthesis alignment; two tabs is enough.
19. Random whitespace changes.
20. Change check enhancedfxn_ifc.
21. Remove case EF_IFC_GPIO_RS485_LOGIC.
22. Remove case EF_IFC_DYNAMIC_SUSPEND.
23. Correct typo: only.
04/08/2021: Patch v8 Fixed build warning reported by kernel test robot
with ARCH=i386
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
with ARCH=x86_64
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.
5. Changed how the altfunctions were detected and fixed the gpio_init
error handling.
03/12/2021: Patch v4 used git send-mail instead of send patch by manual
follow the instructions of Johan Hovold <[email protected]>.
03/05/2021: Patch v3 modified format and contents of changelog follow feedback
from Johan Hovold <[email protected]>.
03/04/2021: Patch v2 modified format patch as comment from
Johan Hovold <[email protected]>:
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 | 211 +++++++++++++++++++++++++++++++-----
1 file changed, 182 insertions(+), 29 deletions(-)
diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 7bec1e730b20..24cdf86ed23c 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;
@@ -500,6 +500,50 @@ 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.
+ */
+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;
+};
+
+#define CP2108_EF_IFC_GPIO_TXLED 0x01
+#define CP2108_EF_IFC_GPIO_RXLED 0x02
+#define CP2108_EF_IFC_GPIO_RS485 0x04
+#define CP2108_EF_IFC_GPIO_RS485_LOGIC 0x08
+#define CP2108_EF_IFC_GPIO_CLOCK 0x10
+#define CP2108_EF_IFC_DYNAMIC_SUSPEND 0x40
+
+/* 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 +554,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,7 +573,19 @@ 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. */
+/*
+ * CP210X_VENDOR_SPECIFIC, CP210X_WRITE_LATCH call writes these
+ * 0x04 bytes on CP2108.
+ */
+struct cp210x_gpio_write16 {
+ __le16 mask;
+ __le16 state;
+};
+
+/*
+ * CP210X_VENDOR_SPECIFIC, CP210X_WRITE_LATCH call writes these
+ * 0x02 bytes on CP2102N, Cp2103, Cp2104 and CP2105.
+ */
struct cp210x_gpio_write {
u8 mask;
u8 state;
@@ -1298,22 +1357,39 @@ 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;
+ u8 buf[2];
result = usb_autopm_get_interface(serial->interface);
if (result)
return result;
+ /*
+ * 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
+ */
+ if ((priv->partnum == CP210X_PARTNUM_CP2108) || (priv->partnum == CP210X_PARTNUM_CP2105)) {
+ /*
+ * Request type to Read_Latch of CP2105 and CP2108
+ * is 0xc1 <REQTYPE_INTERFACE_TO_HOST>
+ */
+ req_type = REQTYPE_INTERFACE_TO_HOST;
+ }
result = cp210x_read_vendor_block(serial, req_type,
- CP210X_READ_LATCH, &buf, sizeof(buf));
- usb_autopm_put_interface(serial->interface);
- if (result < 0)
- return result;
+ CP210X_READ_LATCH, buf, 2);
- return !!(buf & BIT(gpio));
+ if (result < 0){
+ usb_autopm_put_interface(serial->interface);
+ return result;
+ }
+ result = le16_to_cpup((__le16 *)buf);
+ usb_autopm_put_interface(serial->interface);
+ return !!(result & BIT(gpio));
}
static void cp210x_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
@@ -1321,37 +1397,51 @@ 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;
+ struct cp210x_gpio_write16 buf16;
+ u16 wIndex;
int result;
- if (value == 1)
+ if (value == 1) {
buf.state = BIT(gpio);
- else
+ buf16.state = cpu_to_le16(BIT(gpio));
+ } else {
buf.state = 0;
-
+ buf16.state = 0;
+ }
buf.mask = BIT(gpio);
+ buf16.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, &buf16,
+ sizeof(buf16));
+ break;
+ case CP210X_PARTNUM_CP2105:
+ result = cp210x_write_vendor_block(serial,
+ REQTYPE_HOST_TO_INTERFACE,
+ CP210X_WRITE_LATCH, &buf,
+ sizeof(buf));
+ break;
+ default:
+ wIndex = buf.state << 8 | buf.mask;
result = usb_control_msg(serial->dev,
- usb_sndctrlpipe(serial->dev, 0),
- CP210X_VENDOR_SPECIFIC,
- REQTYPE_HOST_TO_DEVICE,
- CP210X_WRITE_LATCH,
- wIndex,
- NULL, 0, USB_CTRL_SET_TIMEOUT);
+ 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 +1510,60 @@ 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++) {
+ if(config.enhancedfxn_ifc[i] & CP2108_EF_IFC_GPIO_TXLED)
+ priv->gpio_altfunc |= BIT(i * 4);
+ if(config.enhancedfxn_ifc[i] & CP2108_EF_IFC_GPIO_RXLED)
+ priv->gpio_altfunc |= BIT((i * 4) + 1);
+ if(config.enhancedfxn_ifc[i] & CP2108_EF_IFC_GPIO_RS485)
+ priv->gpio_altfunc |= BIT((i * 4) + 2);
+ if(config.enhancedfxn_ifc[i] & CP2108_EF_IFC_GPIO_CLOCK)
+ priv->gpio_altfunc |= BIT((i * 4) + 3);
+ }
+ /*
+ * 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 +1793,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 only 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
Dear Johan Hovold.
Do you agree and approve with this path?.
Thank you,
On Tue, May 18, 2021 at 03:18:06AM +0000, Tung Pham wrote:
> Dear Johan Hovold.
> Do you agree and approve with this path?.
I'm still waiting for you to confirm that you have tested the patch with
different pin configurations in eeprom. The first few iterations clearly
weren't tested and I don't want to waste more time reviewing it before
it's tested as I believe I mentioned in my last mail.
Johan
-----Original Message-----
From: Johan Hovold <[email protected]>
Sent: Tuesday, May 18, 2021 2:05 PM
To: Tung Pham <[email protected]>
Cc: Pho Tran <[email protected]>; [email protected]; [email protected]; [email protected]; Hung Nguyen <[email protected]>; Pho Tran <[email protected]>
Subject: Re: [PATCH v12] USB: serial: cp210x: Add support for GPIOs on CP2108
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
On Tue, May 18, 2021 at 03:18:06AM +0000, Tung Pham wrote:
> Dear Johan Hovold.
> Do you agree and approve with this path?.
> I'm still waiting for you to confirm that you have tested the patch with different pin configurations in eeprom. The first few iterations > > clearly weren't tested and I don't want to waste more time reviewing it before it's tested as I believe I mentioned in my last mail.
> Johan
-> we have tested pass the GPIO of cp2108:
Case1: all 16 GPIO are configured as GPIO function.
Case2: all 16 GPIO are configured as alternative functions (not GPIO).
Please see attached file for more detail.
Thanks.
Tung Pham.
On Thu, May 20, 2021 at 03:11:54PM +0000, Tung Pham wrote:
> On Tue, May 18, 2021 at 03:18:06AM +0000, Tung Pham wrote:
> > Do you agree and approve with this path?.
>
> > I'm still waiting for you to confirm that you have tested the patch
> > with different pin configurations in eeprom.
> -> we have tested pass the GPIO of cp2108:
> Case1: all 16 GPIO are configured as GPIO function.
> Case2: all 16 GPIO are configured as alternative functions (not GPIO).
> Please see attached file for more detail.
Excellent, thanks for confirming. I'll try to review the latest revision
shortly.
Johan
On Wed, Jun 09, 2021 at 03:06:16AM +0000, Tung Pham wrote:
> Thanks for your effort to reviewing our code.
> What about status of this patch?.
It's still on my list. I'm a bit short on time at the moment and had to
prioritise the CP2102N regression. I'm still hoping to get to this patch
this week.
Johan
> On Tue, May 18, 2021 at 03:18:06AM +0000, Tung Pham wrote:
> > Do you agree and approve with this path?.
>
> > I'm still waiting for you to confirm that you have tested the patch
> > with different pin configurations in eeprom.
> -> we have tested pass the GPIO of cp2108:
> Case1: all 16 GPIO are configured as GPIO function.
> Case2: all 16 GPIO are configured as alternative functions (not GPIO).
> Please see attached file for more detail.
> Excellent, thanks for confirming. I'll try to review the latest revision shortly.
> Johan
Thanks for your effort to reviewing our code.
What about status of this patch?.
Thanks.
On Mon, Apr 26, 2021 at 04:12:44PM +0700, tu pham wrote:
> From: Pho Tran <[email protected]>
>
> 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 <[email protected]>
> Co-developed-by: Tung Pham <[email protected]>
> Signed-off-by: Tung Pham <[email protected]>
> ---
>
> 04/26/2021: Patch v11 Add Signed-off-by Tung Pham
> 04/26/2021: Patch v10 Add Co-developed-by Tung Pham
> 04/23/2021: Patch v9 Modified code according to comment of Johan:
> 1. Remove quad-port-config comment.
> 2. Move defice EF_IFC_GPIOs go after the quad-port-config
> structure with the other port config defines, add CP2108_ prefix.
> 3. Drop CP2108 Quad Port State structure sentence comment;
> clear from the code.
> 4. remove Double new line.
> 5. Use lowercase pb for consistency.
> 6. Avoid // comments. CP2108 (uppercase P).
> 7. remove Double new line.
> 8. Lowercase ifc.
> 9. Just keep the current struct as is and add a second one for 16 bits
> (e.g. struct cp210x_gpio_write16).
> 10. don't need both u16 buf, but it may be cleaner to use u8 buf[2]
> for the transfer buffer.
> 11. Add missing indentation.
> 12. Try to be consistent with capitalisation of CP210x.
> 13. Just do the read after the switch into a common u8 buf[2] and
> use the full length only for CP2108.
> 14. Add leak the PM counter reference here.
> 15. Change buf = le16_to_cpu(wbuf); to le16_to_cpup((__le16 *)buf).
> 16. Keep the original struct as is cp210x_gpio_write.
> 17. Add another one for cp2108 with a suitable suffix.
> 18. No need to do open-parenthesis alignment; two tabs is enough.
> 19. Random whitespace changes.
> 20. Change check enhancedfxn_ifc.
> 21. Remove case EF_IFC_GPIO_RS485_LOGIC.
> 22. Remove case EF_IFC_DYNAMIC_SUSPEND.
> 23. Correct typo: only.
> 04/08/2021: Patch v8 Fixed build warning reported by kernel test robot
> with ARCH=i386
> 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
> with ARCH=x86_64
> 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.
> 5. Changed how the altfunctions were detected and fixed the gpio_init
> error handling.
> 03/12/2021: Patch v4 used git send-mail instead of send patch by manual
> follow the instructions of Johan Hovold <[email protected]>.
> 03/05/2021: Patch v3 modified format and contents of changelog follow feedback
> from Johan Hovold <[email protected]>.
> 03/04/2021: Patch v2 modified format patch as comment from
> Johan Hovold <[email protected]>:
> 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.".
Checkpatch now complains about five white-space issues.
Normally, I'd just ask you to fix up and resend, but I fear this will
take all summer to get into shape so I'll fix up the warnings and a
number of issues that I point out below *this* time.
I'm really quite annoyed to find that you have again ignored a number of
my comments on v9.
Next time, do some proper internal review before posting and make sure
you have addressed all review comments (by following the suggestions or
arguing why you will not).
> drivers/usb/serial/cp210x.c | 211 +++++++++++++++++++++++++++++++-----
> 1 file changed, 182 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index 7bec1e730b20..24cdf86ed23c 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;
> @@ -500,6 +500,50 @@ 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.
> + */
At least the last part of this comment belongs above the config
structure itself.
> +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;
> +};
> +
> +#define CP2108_EF_IFC_GPIO_TXLED 0x01
> +#define CP2108_EF_IFC_GPIO_RXLED 0x02
> +#define CP2108_EF_IFC_GPIO_RS485 0x04
> +#define CP2108_EF_IFC_GPIO_RS485_LOGIC 0x08
> +#define CP2108_EF_IFC_GPIO_CLOCK 0x10
> +#define CP2108_EF_IFC_DYNAMIC_SUSPEND 0x40
Why aren't all the values aligned?
> +
> +/* 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 +554,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,7 +573,19 @@ 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. */
> +/*
> + * CP210X_VENDOR_SPECIFIC, CP210X_WRITE_LATCH call writes these
> + * 0x04 bytes on CP2108.
> + */
> +struct cp210x_gpio_write16 {
> + __le16 mask;
> + __le16 state;
> +};
> +
> +/*
> + * CP210X_VENDOR_SPECIFIC, CP210X_WRITE_LATCH call writes these
> + * 0x02 bytes on CP2102N, Cp2103, Cp2104 and CP2105.
You're still using lower-case 'p' for some of the models above.
> + */
> struct cp210x_gpio_write {
> u8 mask;
> u8 state;
> @@ -1298,22 +1357,39 @@ 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;
> + u8 buf[2];
>
> result = usb_autopm_get_interface(serial->interface);
> if (result)
> return result;
> + /*
> + * 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
> + */
Typo: "amd".
But as I mentioned earlier, I'm not sure it's worth adding this comment.
The code should be made obvious enough that it isn't needed.
> + if ((priv->partnum == CP210X_PARTNUM_CP2108) || (priv->partnum == CP210X_PARTNUM_CP2105)) {
> + /*
> + * Request type to Read_Latch of CP2105 and CP2108
> + * is 0xc1 <REQTYPE_INTERFACE_TO_HOST>
> + */
This comment is not indented correctly. It also not needed since what it
says is obvious from the code as I mentioned before.
> + req_type = REQTYPE_INTERFACE_TO_HOST;
> + }
The above should be a switch that also sets a length variable.
>
> result = cp210x_read_vendor_block(serial, req_type,
> - CP210X_READ_LATCH, &buf, sizeof(buf));
> - usb_autopm_put_interface(serial->interface);
Why are you changing the autopm call?
> - if (result < 0)
> - return result;
> + CP210X_READ_LATCH, buf, 2);
And no need to read 2 bytes for any device types but CP2108 as I also
mentioned earlier. Does that even work?
>
> - return !!(buf & BIT(gpio));
> + if (result < 0){
Missing space before {.
> + usb_autopm_put_interface(serial->interface);
> + return result;
> + }
> + result = le16_to_cpup((__le16 *)buf);
> + usb_autopm_put_interface(serial->interface);
> + return !!(result & BIT(gpio));
> }
When cleaning up the above, I switched to reading into a u16 mask
directly instead, which makes the code a bit more readable still.
> static void cp210x_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
> @@ -1321,37 +1397,51 @@ 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;
> + struct cp210x_gpio_write16 buf16;
> + u16 wIndex;
> int result;
>
> - if (value == 1)
> + if (value == 1) {
> buf.state = BIT(gpio);
> - else
> + buf16.state = cpu_to_le16(BIT(gpio));
> + } else {
> buf.state = 0;
> -
> + buf16.state = 0;
> + }
> buf.mask = BIT(gpio);
> + buf16.mask = cpu_to_le16(BIT(gpio));
Here I asked you to use two u16 variables for mask and state, which you
then use to set up the arguments below.
> 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, &buf16,
> + sizeof(buf16));
> + break;
> + case CP210X_PARTNUM_CP2105:
> + result = cp210x_write_vendor_block(serial,
> + REQTYPE_HOST_TO_INTERFACE,
> + CP210X_WRITE_LATCH, &buf,
> + sizeof(buf));
> + break;
> + default:
> + wIndex = buf.state << 8 | buf.mask;
> result = usb_control_msg(serial->dev,
> - usb_sndctrlpipe(serial->dev, 0),
> - CP210X_VENDOR_SPECIFIC,
> - REQTYPE_HOST_TO_DEVICE,
> - CP210X_WRITE_LATCH,
> - wIndex,
> - NULL, 0, USB_CTRL_SET_TIMEOUT);
> + usb_sndctrlpipe(serial->dev, 0),
> + CP210X_VENDOR_SPECIFIC,
> + REQTYPE_HOST_TO_DEVICE,
> + CP210X_WRITE_LATCH,
> + wIndex,
> + NULL, 0, USB_CTRL_SET_TIMEOUT);
> + break;
No need to be shuffling all the existing code around.
> }
>
> usb_autopm_put_interface(serial->interface);
> +
Random whitespace change.
> out:
> if (result < 0) {
> dev_err(&serial->interface->dev, "failed to set GPIO value: %d\n",
> @@ -1420,6 +1510,60 @@ 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;
As I mentioned earlier, the temporaries do not buy you anything since
the masks are 16 bits just like the structure fields, and the shifting
and masking is pointless.
I'll reply to this mail with an updated v13 addressing the above.
Johan
From: Pho Tran <[email protected]>
Similar to some other CP210x device types, CP2108 has a number of GPIO
pins that can be exposed through gpiolib.
CP2108 has four serial interfaces but only one set of GPIO pins, which
is modelled as a single gpio chip and registered as a child of the first
interface.
CP2108 has 16 GPIOs so the width of the state variables needs to be
extended to 16 bits and this is also reflected in the control requests.
Like CP2104, CP2108 have GPIO pins with configurable alternate
functions and pins unavailable for GPIO use are determined and reported
to gpiolib at probe.
Signed-off-by: Pho Tran <[email protected]>
Co-developed-by: Tung Pham <[email protected]>
Signed-off-by: Tung Pham <[email protected]>
[ johan: rewrite gpio get() and set(); misc cleanups; amend commit
message ]
Signed-off-by: Johan Hovold <[email protected]>
---
Tested using CP2108 and CP2102N.
Changes in v13
- rewrite cp210x_gpio_get() using a shared 16 bit mask and
le16_to_cpus()
- rewrite cp210x_gpio_set() using shared 16-bit mask and state
variables
- drop pointless no-op shift and mask operations during initialisation
- reorder defines
- reword some comments
- fix some style issues
- amend commit message
v12 can be found here:
- https://lore.kernel.org/r/[email protected]
drivers/usb/serial/cp210x.c | 189 ++++++++++++++++++++++++++++++++----
1 file changed, 170 insertions(+), 19 deletions(-)
diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index ee595d1bea0a..8424ad9f0955 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -247,9 +247,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;
@@ -531,18 +531,72 @@ struct cp210x_single_port_config {
#define CP2104_GPIO1_RXLED_MODE BIT(1)
#define CP2104_GPIO2_RS485_MODE BIT(2)
+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;
+};
+
+/*
+ * CP210X_VENDOR_SPECIFIC, CP210X_GET_PORTCONFIG call reads these 0x49 bytes
+ * on a CP2108 chip.
+ *
+ * See https://www.silabs.com/documents/public/application-notes/an978-cp210x-usb-to-uart-api-specification.pdf
+ */
+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;
+
+#define CP2108_EF_IFC_GPIO_TXLED 0x01
+#define CP2108_EF_IFC_GPIO_RXLED 0x02
+#define CP2108_EF_IFC_GPIO_RS485 0x04
+#define CP2108_EF_IFC_GPIO_RS485_LOGIC 0x08
+#define CP2108_EF_IFC_GPIO_CLOCK 0x10
+#define CP2108_EF_IFC_DYNAMIC_SUSPEND 0x40
+
/* CP2102N configuration array indices */
#define CP210X_2NCONFIG_CONFIG_VERSION_IDX 2
#define CP210X_2NCONFIG_GPIO_MODE_IDX 581
#define CP210X_2NCONFIG_GPIO_RSTLATCH_IDX 587
#define CP210X_2NCONFIG_GPIO_CONTROL_IDX 600
-/* CP210X_VENDOR_SPECIFIC, CP210X_WRITE_LATCH call writes these 0x2 bytes. */
+/*
+ * CP210X_VENDOR_SPECIFIC, CP210X_WRITE_LATCH call writes these 0x02 bytes
+ * for CP2102N, CP2103, CP2104 and CP2105.
+ */
struct cp210x_gpio_write {
u8 mask;
u8 state;
};
+/*
+ * CP210X_VENDOR_SPECIFIC, CP210X_WRITE_LATCH call writes these 0x04 bytes
+ * for CP2108.
+ */
+struct cp210x_gpio_write16 {
+ __le16 mask;
+ __le16 state;
+};
+
/*
* Helper to get interface number when we only have struct usb_serial.
*/
@@ -1414,52 +1468,84 @@ 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;
+ u8 req_type;
+ u16 mask;
int result;
- u8 buf;
-
- if (priv->partnum == CP210X_PARTNUM_CP2105)
- req_type = REQTYPE_INTERFACE_TO_HOST;
+ int len;
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));
+ switch (priv->partnum) {
+ case CP210X_PARTNUM_CP2105:
+ req_type = REQTYPE_INTERFACE_TO_HOST;
+ len = 1;
+ break;
+ case CP210X_PARTNUM_CP2108:
+ req_type = REQTYPE_INTERFACE_TO_HOST;
+ len = 2;
+ break;
+ default:
+ req_type = REQTYPE_DEVICE_TO_HOST;
+ len = 1;
+ break;
+ }
+
+ mask = 0;
+ result = cp210x_read_vendor_block(serial, req_type, CP210X_READ_LATCH,
+ &mask, len);
+
usb_autopm_put_interface(serial->interface);
+
if (result < 0)
return result;
- return !!(buf & BIT(gpio));
+ le16_to_cpus((__le16 *)&mask);
+
+ return !!(mask & 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_write16 buf16;
struct cp210x_gpio_write buf;
+ u16 mask, state;
+ u16 wIndex;
int result;
if (value == 1)
- buf.state = BIT(gpio);
+ state = BIT(gpio);
else
- buf.state = 0;
+ state = 0;
- buf.mask = BIT(gpio);
+ mask = 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_CP2105:
+ buf.mask = (u8)mask;
+ buf.state = (u8)state;
result = cp210x_write_vendor_block(serial,
REQTYPE_HOST_TO_INTERFACE,
CP210X_WRITE_LATCH, &buf,
sizeof(buf));
- } else {
- u16 wIndex = buf.state << 8 | buf.mask;
-
+ break;
+ case CP210X_PARTNUM_CP2108:
+ buf16.mask = cpu_to_le16(mask);
+ buf16.state = cpu_to_le16(state);
+ result = cp210x_write_vendor_block(serial,
+ REQTYPE_HOST_TO_INTERFACE,
+ CP210X_WRITE_LATCH, &buf16,
+ sizeof(buf16));
+ break;
+ default:
+ wIndex = state << 8 | mask;
result = usb_control_msg(serial->dev,
usb_sndctrlpipe(serial->dev, 0),
CP210X_VENDOR_SPECIFIC,
@@ -1467,6 +1553,7 @@ static void cp210x_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
CP210X_WRITE_LATCH,
wIndex,
NULL, 0, USB_CTRL_SET_TIMEOUT);
+ break;
}
usb_autopm_put_interface(serial->interface);
@@ -1676,6 +1763,61 @@ static int cp2104_gpioconf_init(struct usb_serial *serial)
return 0;
}
+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;
+ 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;
+ priv->gpio_pushpull = le16_to_cpu(config.reset_state.gpio_mode_pb1);
+ gpio_latch = le16_to_cpu(config.reset_state.gpio_latch_pb1);
+
+ /*
+ * Mark all pins which are not in GPIO mode.
+ *
+ * Refer to table 9.1 "GPIO Mode alternate Functions" in the datasheet:
+ * https://www.silabs.com/documents/public/data-sheets/cp2108-datasheet.pdf
+ *
+ * Alternate functions of GPIO0 to GPIO3 are determine by enhancedfxn_ifc[0]
+ * and the similarly for the 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++) {
+ if (config.enhancedfxn_ifc[i] & CP2108_EF_IFC_GPIO_TXLED)
+ priv->gpio_altfunc |= BIT(i * 4);
+ if (config.enhancedfxn_ifc[i] & CP2108_EF_IFC_GPIO_RXLED)
+ priv->gpio_altfunc |= BIT((i * 4) + 1);
+ if (config.enhancedfxn_ifc[i] & CP2108_EF_IFC_GPIO_RS485)
+ priv->gpio_altfunc |= BIT((i * 4) + 2);
+ if (config.enhancedfxn_ifc[i] & CP2108_EF_IFC_GPIO_CLOCK)
+ priv->gpio_altfunc |= BIT((i * 4) + 3);
+ }
+
+ /*
+ * 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;
+}
+
static int cp2102n_gpioconf_init(struct usb_serial *serial)
{
struct cp210x_serial_private *priv = usb_get_serial_data(serial);
@@ -1780,6 +1922,15 @@ static int cp210x_gpio_init(struct usb_serial *serial)
case CP210X_PARTNUM_CP2105:
result = cp2105_gpioconf_init(serial);
break;
+ case CP210X_PARTNUM_CP2108:
+ /*
+ * The GPIOs are not tied to any specific port so only register
+ * once for interface 0.
+ */
+ if (cp210x_interface_num(serial) != 0)
+ return 0;
+ result = cp2108_gpio_init(serial);
+ break;
case CP210X_PARTNUM_CP2102N_QFN28:
case CP210X_PARTNUM_CP2102N_QFN24:
case CP210X_PARTNUM_CP2102N_QFN20:
--
2.31.1
Dear Johan.
I 'm sorry about my less of experience about committing patch to Linux kernel, causing you take so much time to review.
I'll document your comment and last commit by you as a reference for my next commit if I have in the future.
Thanks you for your effort.
On Sun, Jun 13, 2021 at 09:45:47AM +0000, Tung Pham wrote:
> Dear Johan.
> I 'm sorry about my less of experience about committing patch to Linux
> kernel, causing you take so much time to review.
> I'll document your comment and last commit by you as a reference for
> my next commit if I have in the future.
Sounds good.
> Thanks you for your effort.
You're welcome.
Johan
On Thu, Jun 10, 2021 at 03:28:44PM +0200, Johan Hovold wrote:
> From: Pho Tran <[email protected]>
>
> Similar to some other CP210x device types, CP2108 has a number of GPIO
> pins that can be exposed through gpiolib.
>
> CP2108 has four serial interfaces but only one set of GPIO pins, which
> is modelled as a single gpio chip and registered as a child of the first
> interface.
>
> CP2108 has 16 GPIOs so the width of the state variables needs to be
> extended to 16 bits and this is also reflected in the control requests.
>
> Like CP2104, CP2108 have GPIO pins with configurable alternate
> functions and pins unavailable for GPIO use are determined and reported
> to gpiolib at probe.
>
> Signed-off-by: Pho Tran <[email protected]>
> Co-developed-by: Tung Pham <[email protected]>
> Signed-off-by: Tung Pham <[email protected]>
> [ johan: rewrite gpio get() and set(); misc cleanups; amend commit
> message ]
> Signed-off-by: Johan Hovold <[email protected]>
> ---
>
> Tested using CP2108 and CP2102N.
>
> Changes in v13
> - rewrite cp210x_gpio_get() using a shared 16 bit mask and
> le16_to_cpus()
> - rewrite cp210x_gpio_set() using shared 16-bit mask and state
> variables
> - drop pointless no-op shift and mask operations during initialisation
> - reorder defines
> - reword some comments
> - fix some style issues
> - amend commit message
>
> v12 can be found here:
> - https://lore.kernel.org/r/[email protected]
>
>
> drivers/usb/serial/cp210x.c | 189 ++++++++++++++++++++++++++++++++----
> 1 file changed, 170 insertions(+), 19 deletions(-)
> @@ -1414,52 +1468,84 @@ 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;
> + u8 req_type;
> + u16 mask;
> int result;
> - u8 buf;
> -
> - if (priv->partnum == CP210X_PARTNUM_CP2105)
> - req_type = REQTYPE_INTERFACE_TO_HOST;
> + int len;
>
> 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));
> + switch (priv->partnum) {
> + case CP210X_PARTNUM_CP2105:
> + req_type = REQTYPE_INTERFACE_TO_HOST;
> + len = 1;
> + break;
> + case CP210X_PARTNUM_CP2108:
> + req_type = REQTYPE_INTERFACE_TO_HOST;
> + len = 2;
> + break;
> + default:
> + req_type = REQTYPE_DEVICE_TO_HOST;
> + len = 1;
> + break;
> + }
> +
> + mask = 0;
> + result = cp210x_read_vendor_block(serial, req_type, CP210X_READ_LATCH,
> + &mask, len);
> +
> usb_autopm_put_interface(serial->interface);
> +
> if (result < 0)
> return result;
>
> - return !!(buf & BIT(gpio));
> + le16_to_cpus((__le16 *)&mask);
Now applied after removing the (__le16 *) cast here.
> +
> + return !!(mask & BIT(gpio));
> }
Johan