Re: [PATCH v2] USB: serial: cp210x: Adding GPIO support for CP2105

Re: [PATCH v2] USB: serial: cp210x: Adding GPIO support for CP2105

[kbuild test robot]

Hi Martyn,

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on next-20160113]
[cannot apply to v4.4]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Martyn-Welch/USB-serial-cp210x-Adding-GPIO-support-for-CP2105/20160113-203435
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing


coccinelle warnings: (new ones prefixed by >>)

>> drivers/usb/serial/cp210x.c:1132:15-21: ERROR: application of sizeof to pointer

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[PATCH v2] USB: serial: cp210x: Adding GPIO support for CP2105

[Martyn Welch]

This patch adds support for the GPIO found on the CP2105. Unlike the GPIO
provided by some of the other devices supported by the cp210x driver, the
GPIO on the CP2015 is muxed on pins otherwise used for serial control
lines. The GPIO have been configured in 2 separate banks as the choice to
configure the pins for GPIO is made separately for pins shared with each
of the 2 serial ports this device provides, though the choice is made for
all pins associated with that port in one go. The choice of whether to use
the pins for GPIO or serial is made by adding configuration to a one-time
programable PROM in the chip and can not be changed at runtime. The device
defaults to GPIO.

This device supports either push-pull or open-drain modes, it doesn't
provide an explicit input mode, though the state of the GPIO can be read
when used in open-drain mode. Like with pin use, the mode is configured in
the one-time programable PROM and can't be changed at runtime.

Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
---

This patch applies to usb-next, which has a number of changes to the cp210x
driver in it, so I assumed that would be the correct tree to base this on.

V2: Doesn't break build when gpiolib isn't selected.

 drivers/usb/serial/cp210x.c | 223 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 218 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 9b90ad7..73e2a12 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -23,6 +23,8 @@
 #include <linux/usb.h>
 #include <linux/uaccess.h>
 #include <linux/usb/serial.h>
+#include <linux/gpio/driver.h>
+#include <linux/bitops.h>
 
 #define DRIVER_DESC "Silicon Labs CP210x RS232 serial adaptor driver"
 
@@ -44,10 +46,13 @@ static int cp210x_tiocmset(struct tty_struct *, unsigned int, unsigned int);
 static int cp210x_tiocmset_port(struct usb_serial_port *port,
 		unsigned int, unsigned int);
 static void cp210x_break_ctl(struct tty_struct *, int);
+static int cp210x_probe(struct usb_serial *, const struct usb_device_id *);
 static int cp210x_port_probe(struct usb_serial_port *);
 static int cp210x_port_remove(struct usb_serial_port *);
 static void cp210x_dtr_rts(struct usb_serial_port *p, int on);
 
+#define CP210X_FEATURE_HAS_SHARED_GPIO		BIT(0)
+
 static const struct usb_device_id id_table[] = {
 	{ USB_DEVICE(0x045B, 0x0053) }, /* Renesas RX610 RX-Stick */
 	{ USB_DEVICE(0x0471, 0x066A) }, /* AKTAKOM ACE-1001 cable */
@@ -132,7 +137,8 @@ static const struct usb_device_id id_table[] = {
 	{ USB_DEVICE(0x10C4, 0x8A2A) }, /* HubZ dual ZigBee and Z-Wave dongle */
 	{ USB_DEVICE(0x10C4, 0xEA60) }, /* Silicon Labs factory default */
 	{ USB_DEVICE(0x10C4, 0xEA61) }, /* Silicon Labs factory default */
-	{ USB_DEVICE(0x10C4, 0xEA70) }, /* Silicon Labs factory default */
+	{ USB_DEVICE(0x10C4, 0xEA70),	/* Silicon Labs factory default */
+	  .driver_info = CP210X_FEATURE_HAS_SHARED_GPIO },
 	{ USB_DEVICE(0x10C4, 0xEA71) }, /* Infinity GPS-MIC-1 Radio Monophone */
 	{ USB_DEVICE(0x10C4, 0xF001) }, /* Elan Digital Systems USBscope50 */
 	{ USB_DEVICE(0x10C4, 0xF002) }, /* Elan Digital Systems USBwave12 */
@@ -201,6 +207,10 @@ MODULE_DEVICE_TABLE(usb, id_table);
 struct cp210x_port_private {
 	__u8			bInterfaceNumber;
 	bool			has_swapped_line_ctl;
+#ifdef CONFIG_GPIOLIB
+	struct usb_serial	*serial;
+	struct gpio_chip	gc;
+#endif
 };
 
 static struct usb_serial_driver cp210x_device = {
@@ -219,6 +229,7 @@ static struct usb_serial_driver cp210x_device = {
 	.tx_empty		= cp210x_tx_empty,
 	.tiocmget		= cp210x_tiocmget,
 	.tiocmset		= cp210x_tiocmset,
+	.probe			= cp210x_probe,
 	.port_probe		= cp210x_port_probe,
 	.port_remove		= cp210x_port_remove,
 	.dtr_rts		= cp210x_dtr_rts
@@ -261,6 +272,7 @@ static struct usb_serial_driver * const serial_drivers[] = {
 #define CP210X_SET_CHARS	0x19
 #define CP210X_GET_BAUDRATE	0x1D
 #define CP210X_SET_BAUDRATE	0x1E
+#define CP210X_VENDOR_SPECIFIC	0xFF
 
 /* CP210X_IFC_ENABLE */
 #define UART_ENABLE		0x0001
@@ -303,6 +315,11 @@ static struct usb_serial_driver * const serial_drivers[] = {
 #define CONTROL_WRITE_DTR	0x0100
 #define CONTROL_WRITE_RTS	0x0200
 
+/* CP210X_VENDOR_SPECIFIC values */
+#define CP210X_GET_DEVICEMODE	0x3711
+#define CP210X_WRITE_LATCH	0x37E1
+#define CP210X_READ_LATCH	0x00C2
+
 /* CP210X_GET_COMM_STATUS returns these 0x13 bytes */
 struct cp210x_comm_status {
 	__le32   ulErrors;
@@ -991,6 +1008,179 @@ static void cp210x_break_ctl(struct tty_struct *tty, int break_state)
 	cp210x_set_config(port, CP210X_SET_BREAK, &state, 2);
 }
 
+#ifdef CONFIG_GPIOLIB
+static int cp210x_gpio_direction_get(struct gpio_chip *gc, unsigned gpio)
+{
+	return 0;
+}
+
+static int cp210x_gpio_get(struct gpio_chip *gc, unsigned gpio)
+{
+	struct cp210x_port_private *port_priv =
+			 container_of(gc, struct cp210x_port_private, gc);
+	struct usb_serial *serial = port_priv->serial;
+	__le32 *buf;
+	int result, i, length;
+	int size = 1;
+	unsigned int data[size];
+
+	/* Number of integers required to contain the array */
+	length = (((size - 1) | 3) + 1) / 4;
+
+	buf = kcalloc(length, sizeof(__le32), GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	/* Issue the request, attempting to read 'size' bytes */
+	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
+				 CP210X_VENDOR_SPECIFIC,
+				 REQTYPE_INTERFACE_TO_HOST, CP210X_READ_LATCH,
+				 port_priv->bInterfaceNumber, buf, size,
+				 USB_CTRL_GET_TIMEOUT);
+	if (result != size) {
+		result = 0;
+		goto err;
+	}
+
+	/* Convert data into an array of integers */
+	for (i = 0; i < length; i++)
+		data[i] = le32_to_cpu(buf[i]);
+
+	result = (data[0] >> gpio) & 0x1;
+
+	kfree(buf);
+err:
+	return result;
+}
+
+static void cp210x_gpio_set(struct gpio_chip *gc, unsigned gpio, int value)
+{
+	struct cp210x_port_private *port_priv =
+			 container_of(gc, struct cp210x_port_private, gc);
+	struct usb_serial *serial = port_priv->serial;
+
+	int result, i, length;
+	unsigned int data[1];
+	__le32 *buf;
+	int size = 2;
+
+	/* Number of integers required to contain the array */
+	length = (((size - 1) | 3) + 1) / 4;
+
+	buf = kcalloc(length, sizeof(__le32), GFP_KERNEL);
+	if (!buf) {
+		WARN_ON("Unable to alloc memory to perform set");
+		return;
+	}
+
+	/* Issue the request, attempting to read 'size' bytes */
+	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
+				 CP210X_VENDOR_SPECIFIC,
+				 REQTYPE_INTERFACE_TO_HOST, CP210X_READ_LATCH,
+				 port_priv->bInterfaceNumber, buf, 1,
+				 USB_CTRL_GET_TIMEOUT);
+	if (result != 1) {
+		WARN_ON("Failed to read data over usb");
+		goto err;
+	}
+
+	/* Convert data into an array of integers */
+	for (i = 0; i < length; i++)
+		data[i] = le32_to_cpu(buf[i]);
+
+	if (value == 0)
+		data[0] &= ~BIT(gpio);
+	else
+		data[0] |= BIT(gpio);
+
+	data[0] = data[0] << 8;
+
+	data[0] |= 0x00FF;
+
+	/* Array of integers into bytes */
+	for (i = 0; i < length; i++)
+		buf[i] = cpu_to_le32(data[i]);
+
+	result = usb_control_msg(serial->dev,
+				 usb_sndctrlpipe(serial->dev, 0),
+				 CP210X_VENDOR_SPECIFIC,
+				 REQTYPE_HOST_TO_INTERFACE, CP210X_WRITE_LATCH,
+				 port_priv->bInterfaceNumber, buf, size,
+				 USB_CTRL_SET_TIMEOUT);
+
+	if (result != size) {
+		WARN_ON("Failed to read data over usb");
+		goto err;
+	}
+err:
+	kfree(buf);
+}
+
+static int cp210x_shared_gpio_init(struct usb_serial_port *port)
+{
+	struct usb_serial *serial = port->serial;
+	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
+	__le32 *buf;
+	unsigned long tmp;
+	int result, mode;
+
+	tmp = (unsigned long)usb_get_serial_data(serial);
+
+	if ((tmp & CP210X_FEATURE_HAS_SHARED_GPIO) == 0)
+		return 0;
+
+	buf = kzalloc(sizeof(buf), GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	/* Issue the request, attempting to read 'size' bytes */
+	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
+				 CP210X_VENDOR_SPECIFIC, REQTYPE_DEVICE_TO_HOST,
+				 CP210X_GET_DEVICEMODE,
+				 port_priv->bInterfaceNumber, buf,
+				 2, USB_CTRL_GET_TIMEOUT);
+
+	if (result != 2) {
+		dev_err(&port->dev, "failed to get device mode: %d\n", result);
+		if (result > 0)
+			result = -EPROTO;
+
+		goto err;
+	}
+
+	mode = le32_to_cpu(*buf);
+
+	/*  2 banks of GPIO - One for the pins taken from each serial port */
+	if (port_priv->bInterfaceNumber == 0 && (mode & 0xFF) != 0) {
+		port_priv->gc.label = "cp210x_eci";
+		port_priv->gc.ngpio = 2;
+	} else if (port_priv->bInterfaceNumber == 1 &&
+		   ((mode >> 8) & 0xFF) != 0) {
+		port_priv->gc.label = "cp210x_sci";
+		port_priv->gc.ngpio = 3;
+	} else {
+		result = 0;
+		goto err;
+	}
+
+	port_priv->gc.get_direction = cp210x_gpio_direction_get;
+	port_priv->gc.get = cp210x_gpio_get;
+	port_priv->gc.set = cp210x_gpio_set;
+	port_priv->gc.owner = THIS_MODULE;
+	port_priv->gc.dev = &serial->dev->dev;
+	port_priv->gc.base = -1;
+
+	port_priv->serial = serial;
+
+	result = gpiochip_add(&port_priv->gc);
+
+err:
+	kfree(buf);
+
+	return result;
+}
+#endif
+
 static int cp210x_port_probe(struct usb_serial_port *port)
 {
 	struct usb_serial *serial = port->serial;
@@ -1008,12 +1198,21 @@ static int cp210x_port_probe(struct usb_serial_port *port)
 	usb_set_serial_port_data(port, port_priv);
 
 	ret = cp210x_detect_swapped_line_ctl(port);
-	if (ret) {
-		kfree(port_priv);
-		return ret;
-	}
+	if (ret)
+		goto err_ctl;
+
+#ifdef CONFIG_GPIOLIB
+	ret = cp210x_shared_gpio_init(port);
+	if (ret < 0)
+		goto err_ctl;
+#endif
 
 	return 0;
+
+err_ctl:
+	kfree(port_priv);
+
+	return ret;
 }
 
 static int cp210x_port_remove(struct usb_serial_port *port)
@@ -1021,11 +1220,25 @@ static int cp210x_port_remove(struct usb_serial_port *port)
 	struct cp210x_port_private *port_priv;
 
 	port_priv = usb_get_serial_port_data(port);
+
+#ifdef CONFIG_GPIOLIB
+	if (port_priv->gc.label)
+		gpiochip_remove(&port_priv->gc);
+#endif
+
 	kfree(port_priv);
 
 	return 0;
 }
 
+static int cp210x_probe(struct usb_serial *serial,
+			const struct usb_device_id *id)
+{
+	usb_set_serial_data(serial, (void *)id->driver_info);
+
+	return 0;
+}
+
 module_usb_serial_driver(serial_drivers, id_table);
 
 MODULE_DESCRIPTION(DRIVER_DESC);
-- 
2.1.4

Re: [PATCH v2] USB: serial: cp210x: Adding GPIO support for CP2105

[Martyn Welch]

On 13/01/16 12:30, Martyn Welch wrote:
> This patch adds support for the GPIO found on the CP2105. Unlike the GPIO
> provided by some of the other devices supported by the cp210x driver, the
> GPIO on the CP2015 is muxed on pins otherwise used for serial control
> lines. The GPIO have been configured in 2 separate banks as the choice to
> configure the pins for GPIO is made separately for pins shared with each
> of the 2 serial ports this device provides, though the choice is made for
> all pins associated with that port in one go. The choice of whether to use
> the pins for GPIO or serial is made by adding configuration to a one-time
> programable PROM in the chip and can not be changed at runtime. The device
> defaults to GPIO.
>
> This device supports either push-pull or open-drain modes, it doesn't
> provide an explicit input mode, though the state of the GPIO can be read
> when used in open-drain mode. Like with pin use, the mode is configured in
> the one-time programable PROM and can't be changed at runtime.
>
> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
> ---
>
> This patch applies to usb-next, which has a number of changes to the cp210x
> driver in it, so I assumed that would be the correct tree to base this on.
>
> V2: Doesn't break build when gpiolib isn't selected.
>
>   drivers/usb/serial/cp210x.c | 223 +++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 218 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index 9b90ad7..73e2a12 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -23,6 +23,8 @@
>   #include <linux/usb.h>
>   #include <linux/uaccess.h>
>   #include <linux/usb/serial.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/bitops.h>
>
>   #define DRIVER_DESC "Silicon Labs CP210x RS232 serial adaptor driver"
>
> @@ -44,10 +46,13 @@ static int cp210x_tiocmset(struct tty_struct *, unsigned int, unsigned int);
>   static int cp210x_tiocmset_port(struct usb_serial_port *port,
>   		unsigned int, unsigned int);
>   static void cp210x_break_ctl(struct tty_struct *, int);
> +static int cp210x_probe(struct usb_serial *, const struct usb_device_id *);
>   static int cp210x_port_probe(struct usb_serial_port *);
>   static int cp210x_port_remove(struct usb_serial_port *);
>   static void cp210x_dtr_rts(struct usb_serial_port *p, int on);
>
> +#define CP210X_FEATURE_HAS_SHARED_GPIO		BIT(0)
> +
>   static const struct usb_device_id id_table[] = {
>   	{ USB_DEVICE(0x045B, 0x0053) }, /* Renesas RX610 RX-Stick */
>   	{ USB_DEVICE(0x0471, 0x066A) }, /* AKTAKOM ACE-1001 cable */
> @@ -132,7 +137,8 @@ static const struct usb_device_id id_table[] = {
>   	{ USB_DEVICE(0x10C4, 0x8A2A) }, /* HubZ dual ZigBee and Z-Wave dongle */
>   	{ USB_DEVICE(0x10C4, 0xEA60) }, /* Silicon Labs factory default */
>   	{ USB_DEVICE(0x10C4, 0xEA61) }, /* Silicon Labs factory default */
> -	{ USB_DEVICE(0x10C4, 0xEA70) }, /* Silicon Labs factory default */
> +	{ USB_DEVICE(0x10C4, 0xEA70),	/* Silicon Labs factory default */
> +	  .driver_info = CP210X_FEATURE_HAS_SHARED_GPIO },
>   	{ USB_DEVICE(0x10C4, 0xEA71) }, /* Infinity GPS-MIC-1 Radio Monophone */
>   	{ USB_DEVICE(0x10C4, 0xF001) }, /* Elan Digital Systems USBscope50 */
>   	{ USB_DEVICE(0x10C4, 0xF002) }, /* Elan Digital Systems USBwave12 */
> @@ -201,6 +207,10 @@ MODULE_DEVICE_TABLE(usb, id_table);
>   struct cp210x_port_private {
>   	__u8			bInterfaceNumber;
>   	bool			has_swapped_line_ctl;
> +#ifdef CONFIG_GPIOLIB
> +	struct usb_serial	*serial;
> +	struct gpio_chip	gc;
> +#endif
>   };
>
>   static struct usb_serial_driver cp210x_device = {
> @@ -219,6 +229,7 @@ static struct usb_serial_driver cp210x_device = {
>   	.tx_empty		= cp210x_tx_empty,
>   	.tiocmget		= cp210x_tiocmget,
>   	.tiocmset		= cp210x_tiocmset,
> +	.probe			= cp210x_probe,
>   	.port_probe		= cp210x_port_probe,
>   	.port_remove		= cp210x_port_remove,
>   	.dtr_rts		= cp210x_dtr_rts
> @@ -261,6 +272,7 @@ static struct usb_serial_driver * const serial_drivers[] = {
>   #define CP210X_SET_CHARS	0x19
>   #define CP210X_GET_BAUDRATE	0x1D
>   #define CP210X_SET_BAUDRATE	0x1E
> +#define CP210X_VENDOR_SPECIFIC	0xFF
>
>   /* CP210X_IFC_ENABLE */
>   #define UART_ENABLE		0x0001
> @@ -303,6 +315,11 @@ static struct usb_serial_driver * const serial_drivers[] = {
>   #define CONTROL_WRITE_DTR	0x0100
>   #define CONTROL_WRITE_RTS	0x0200
>
> +/* CP210X_VENDOR_SPECIFIC values */
> +#define CP210X_GET_DEVICEMODE	0x3711
> +#define CP210X_WRITE_LATCH	0x37E1
> +#define CP210X_READ_LATCH	0x00C2
> +
>   /* CP210X_GET_COMM_STATUS returns these 0x13 bytes */
>   struct cp210x_comm_status {
>   	__le32   ulErrors;
> @@ -991,6 +1008,179 @@ static void cp210x_break_ctl(struct tty_struct *tty, int break_state)
>   	cp210x_set_config(port, CP210X_SET_BREAK, &state, 2);
>   }
>
> +#ifdef CONFIG_GPIOLIB
> +static int cp210x_gpio_direction_get(struct gpio_chip *gc, unsigned gpio)
> +{
> +	return 0;
> +}
> +
> +static int cp210x_gpio_get(struct gpio_chip *gc, unsigned gpio)
> +{
> +	struct cp210x_port_private *port_priv =
> +			 container_of(gc, struct cp210x_port_private, gc);
> +	struct usb_serial *serial = port_priv->serial;
> +	__le32 *buf;
> +	int result, i, length;
> +	int size = 1;
> +	unsigned int data[size];
> +
> +	/* Number of integers required to contain the array */
> +	length = (((size - 1) | 3) + 1) / 4;
> +
> +	buf = kcalloc(length, sizeof(__le32), GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	/* Issue the request, attempting to read 'size' bytes */
> +	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
> +				 CP210X_VENDOR_SPECIFIC,
> +				 REQTYPE_INTERFACE_TO_HOST, CP210X_READ_LATCH,
> +				 port_priv->bInterfaceNumber, buf, size,
> +				 USB_CTRL_GET_TIMEOUT);
> +	if (result != size) {
> +		result = 0;
> +		goto err;
> +	}
> +
> +	/* Convert data into an array of integers */
> +	for (i = 0; i < length; i++)
> +		data[i] = le32_to_cpu(buf[i]);
> +
> +	result = (data[0] >> gpio) & 0x1;
> +
> +	kfree(buf);
> +err:
> +	return result;
> +}
> +
> +static void cp210x_gpio_set(struct gpio_chip *gc, unsigned gpio, int value)
> +{
> +	struct cp210x_port_private *port_priv =
> +			 container_of(gc, struct cp210x_port_private, gc);
> +	struct usb_serial *serial = port_priv->serial;
> +
> +	int result, i, length;
> +	unsigned int data[1];
> +	__le32 *buf;
> +	int size = 2;
> +
> +	/* Number of integers required to contain the array */
> +	length = (((size - 1) | 3) + 1) / 4;
> +
> +	buf = kcalloc(length, sizeof(__le32), GFP_KERNEL);
> +	if (!buf) {
> +		WARN_ON("Unable to alloc memory to perform set");
> +		return;
> +	}
> +
> +	/* Issue the request, attempting to read 'size' bytes */
> +	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
> +				 CP210X_VENDOR_SPECIFIC,
> +				 REQTYPE_INTERFACE_TO_HOST, CP210X_READ_LATCH,
> +				 port_priv->bInterfaceNumber, buf, 1,
> +				 USB_CTRL_GET_TIMEOUT);
> +	if (result != 1) {
> +		WARN_ON("Failed to read data over usb");
> +		goto err;
> +	}
> +
> +	/* Convert data into an array of integers */
> +	for (i = 0; i < length; i++)
> +		data[i] = le32_to_cpu(buf[i]);
> +

Um, v3 needed.

The above read is reading the state of the pins, not the value last 
written. When pins are configured as open-drain, writing to one GPIO 
whilst another is externally pulled down will result in that second GPIO 
being internally pulled low as well. Not good.

Martyn

> +	if (value == 0)
> +		data[0] &= ~BIT(gpio);
> +	else
> +		data[0] |= BIT(gpio);
> +
> +	data[0] = data[0] << 8;
> +
> +	data[0] |= 0x00FF;
> +
> +	/* Array of integers into bytes */
> +	for (i = 0; i < length; i++)
> +		buf[i] = cpu_to_le32(data[i]);
> +
> +	result = usb_control_msg(serial->dev,
> +				 usb_sndctrlpipe(serial->dev, 0),
> +				 CP210X_VENDOR_SPECIFIC,
> +				 REQTYPE_HOST_TO_INTERFACE, CP210X_WRITE_LATCH,
> +				 port_priv->bInterfaceNumber, buf, size,
> +				 USB_CTRL_SET_TIMEOUT);
> +
> +	if (result != size) {
> +		WARN_ON("Failed to read data over usb");
> +		goto err;
> +	}
> +err:
> +	kfree(buf);
> +}
> +
> +static int cp210x_shared_gpio_init(struct usb_serial_port *port)
> +{
> +	struct usb_serial *serial = port->serial;
> +	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
> +	__le32 *buf;
> +	unsigned long tmp;
> +	int result, mode;
> +
> +	tmp = (unsigned long)usb_get_serial_data(serial);
> +
> +	if ((tmp & CP210X_FEATURE_HAS_SHARED_GPIO) == 0)
> +		return 0;
> +
> +	buf = kzalloc(sizeof(buf), GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	/* Issue the request, attempting to read 'size' bytes */
> +	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
> +				 CP210X_VENDOR_SPECIFIC, REQTYPE_DEVICE_TO_HOST,
> +				 CP210X_GET_DEVICEMODE,
> +				 port_priv->bInterfaceNumber, buf,
> +				 2, USB_CTRL_GET_TIMEOUT);
> +
> +	if (result != 2) {
> +		dev_err(&port->dev, "failed to get device mode: %d\n", result);
> +		if (result > 0)
> +			result = -EPROTO;
> +
> +		goto err;
> +	}
> +
> +	mode = le32_to_cpu(*buf);
> +
> +	/*  2 banks of GPIO - One for the pins taken from each serial port */
> +	if (port_priv->bInterfaceNumber == 0 && (mode & 0xFF) != 0) {
> +		port_priv->gc.label = "cp210x_eci";
> +		port_priv->gc.ngpio = 2;
> +	} else if (port_priv->bInterfaceNumber == 1 &&
> +		   ((mode >> 8) & 0xFF) != 0) {
> +		port_priv->gc.label = "cp210x_sci";
> +		port_priv->gc.ngpio = 3;
> +	} else {
> +		result = 0;
> +		goto err;
> +	}
> +
> +	port_priv->gc.get_direction = cp210x_gpio_direction_get;
> +	port_priv->gc.get = cp210x_gpio_get;
> +	port_priv->gc.set = cp210x_gpio_set;
> +	port_priv->gc.owner = THIS_MODULE;
> +	port_priv->gc.dev = &serial->dev->dev;
> +	port_priv->gc.base = -1;
> +
> +	port_priv->serial = serial;
> +
> +	result = gpiochip_add(&port_priv->gc);
> +
> +err:
> +	kfree(buf);
> +
> +	return result;
> +}
> +#endif
> +
>   static int cp210x_port_probe(struct usb_serial_port *port)
>   {
>   	struct usb_serial *serial = port->serial;
> @@ -1008,12 +1198,21 @@ static int cp210x_port_probe(struct usb_serial_port *port)
>   	usb_set_serial_port_data(port, port_priv);
>
>   	ret = cp210x_detect_swapped_line_ctl(port);
> -	if (ret) {
> -		kfree(port_priv);
> -		return ret;
> -	}
> +	if (ret)
> +		goto err_ctl;
> +
> +#ifdef CONFIG_GPIOLIB
> +	ret = cp210x_shared_gpio_init(port);
> +	if (ret < 0)
> +		goto err_ctl;
> +#endif
>
>   	return 0;
> +
> +err_ctl:
> +	kfree(port_priv);
> +
> +	return ret;
>   }
>
>   static int cp210x_port_remove(struct usb_serial_port *port)
> @@ -1021,11 +1220,25 @@ static int cp210x_port_remove(struct usb_serial_port *port)
>   	struct cp210x_port_private *port_priv;
>
>   	port_priv = usb_get_serial_port_data(port);
> +
> +#ifdef CONFIG_GPIOLIB
> +	if (port_priv->gc.label)
> +		gpiochip_remove(&port_priv->gc);
> +#endif
> +
>   	kfree(port_priv);
>
>   	return 0;
>   }
>
> +static int cp210x_probe(struct usb_serial *serial,
> +			const struct usb_device_id *id)
> +{
> +	usb_set_serial_data(serial, (void *)id->driver_info);
> +
> +	return 0;
> +}
> +
>   module_usb_serial_driver(serial_drivers, id_table);
>
>   MODULE_DESCRIPTION(DRIVER_DESC);
>

[PATCH] USB: serial: cp210x: fix noderef.cocci warnings

[kbuild test robot]

drivers/usb/serial/cp210x.c:1132:15-21: ERROR: application of sizeof to pointer

 sizeof when applied to a pointer typed expression gives the size of
 the pointer

Generated by: scripts/coccinelle/misc/noderef.cocci

CC: Martyn Welch <martyn.welch@collabora.co.uk>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---

 cp210x.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -1129,7 +1129,7 @@ static int cp210x_shared_gpio_init(struc
 	if ((tmp & CP210X_FEATURE_HAS_SHARED_GPIO) == 0)
 		return 0;
 
-	buf = kzalloc(sizeof(buf), GFP_KERNEL);
+	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 

RE: [PATCH v2] USB: serial: cp210x: Adding GPIO support for CP2105

[Konstantin Shkolnyy]

Comments inline, not comprehensive by any measure...

> -----Original Message-----
> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-
> owner@vger.kernel.org] On Behalf Of Martyn Welch
> Sent: Wednesday, January 13, 2016 06:31
> To: Johan Hovold; Linus Walleij; Alexandre Courbot
> Cc: Greg Kroah-Hartman; linux-usb@vger.kernel.org; linux-
> gpio@vger.kernel.org; Martyn Welch
> Subject: [PATCH v2] USB: serial: cp210x: Adding GPIO support for CP2105
> 
...

> +#include <linux/gpio/driver.h>
> +#include <linux/bitops.h>

Enclose this in CONFIG_GPIOLIB?
...

> @@ -201,6 +207,10 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  struct cp210x_port_private {
>  	__u8			bInterfaceNumber;
>  	bool			has_swapped_line_ctl;
> +#ifdef CONFIG_GPIOLIB
> +	struct usb_serial	*serial;

If you have port_private (and port), you can always get usb_serial from port->serial, so why store it here?

> +	struct gpio_chip	gc;
> +#endif
>  };

...

> 
>  static struct usb_serial_driver cp210x_device = {
> @@ -219,6 +229,7 @@ static struct usb_serial_driver cp210x_device = {
>  	.tx_empty		= cp210x_tx_empty,
>  	.tiocmget		= cp210x_tiocmget,
>  	.tiocmset		= cp210x_tiocmset,
> +	.probe			= cp210x_probe,

Enclose this in CONFIG_GPIOLIB?
...

> +static int cp210x_gpio_get(struct gpio_chip *gc, unsigned gpio)
> +{
> +	struct cp210x_port_private *port_priv =
> +			 container_of(gc, struct cp210x_port_private, gc);
> +	struct usb_serial *serial = port_priv->serial;
> +	__le32 *buf;
> +	int result, i, length;
> +	int size = 1;
> +	unsigned int data[size];
> +
> +	/* Number of integers required to contain the array */
> +	length = (((size - 1) | 3) + 1) / 4;

usb_control_msg can deal with any size of the buffer, so use __le16 and remove these length calculations.

> +
> +	buf = kcalloc(length, sizeof(__le32), GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	/* Issue the request, attempting to read 'size' bytes */
> +	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev,
> 0),
> +				 CP210X_VENDOR_SPECIFIC,
> +				 REQTYPE_INTERFACE_TO_HOST,
> CP210X_READ_LATCH,
> +				 port_priv->bInterfaceNumber, buf, size,
> +				 USB_CTRL_GET_TIMEOUT);
> +	if (result != size) {

print err message here and any other time this function fails.

> +		result = 0;
> +		goto err;
> +	}
...

> +static int cp210x_shared_gpio_init(struct usb_serial_port *port)
> +{
> +	struct usb_serial *serial = port->serial;
> +	struct cp210x_port_private *port_priv =
> usb_get_serial_port_data(port);
> +	__le32 *buf;
> +	unsigned long tmp;

Wouldn't "chip_features" be better?

> +	int result, mode;

mode is unsigned.
...

>  static int cp210x_port_probe(struct usb_serial_port *port)
>  {
>  	struct usb_serial *serial = port->serial;
> @@ -1008,12 +1198,21 @@ static int cp210x_port_probe(struct
> usb_serial_port *port)
>  	usb_set_serial_port_data(port, port_priv);
> 
>  	ret = cp210x_detect_swapped_line_ctl(port);
> -	if (ret) {
> -		kfree(port_priv);
> -		return ret;
> -	}
> +	if (ret)
> +		goto err_ctl;
> +
> +#ifdef CONFIG_GPIOLIB
> +	ret = cp210x_shared_gpio_init(port);
> +	if (ret < 0)
> +		goto err_ctl;

put kfree and return here
...

Re: [PATCH v2] USB: serial: cp210x: Adding GPIO support for CP2105

[Linus Walleij]

On Thu, Jan 14, 2016 at 1:27 AM, Konstantin Shkolnyy
<Konstantin.Shkolnyy@silabs.com> wrote:

>> +#include <linux/gpio/driver.h>
>> +#include <linux/bitops.h>
>
> Enclose this in CONFIG_GPIOLIB?

Do not advise people to add excess #ifdef:s please. See at the end of:
Documentation/CodingStyle

Yours,
Linus Walleij

Re: [PATCH v2] USB: serial: cp210x: Adding GPIO support for CP2105

[Martyn Welch]

On 14/01/16 00:27, Konstantin Shkolnyy wrote:
> Comments inline, not comprehensive by any measure...
>
>> -----Original Message-----
>> From: linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-usb-
>> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Martyn Welch
>> Sent: Wednesday, January 13, 2016 06:31
>> To: Johan Hovold; Linus Walleij; Alexandre Courbot
>> Cc: Greg Kroah-Hartman; linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
>> gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Martyn Welch
>> Subject: [PATCH v2] USB: serial: cp210x: Adding GPIO support for CP2105
>>
> ...
>
>> +#include <linux/gpio/driver.h>
>> +#include <linux/bitops.h>
>
> Enclose this in CONFIG_GPIOLIB?
> ...

Is there any point? I took a look at a few other drivers which 
optionally support GPIO and they don't ifdef the headers.

I think the contents of the headers will effectively be ignored if not 
used and this won't affect module size.

>
>> @@ -201,6 +207,10 @@ MODULE_DEVICE_TABLE(usb, id_table);
>>   struct cp210x_port_private {
>>   	__u8			bInterfaceNumber;
>>   	bool			has_swapped_line_ctl;
>> +#ifdef CONFIG_GPIOLIB
>> +	struct usb_serial	*serial;
>
> If you have port_private (and port), you can always get usb_serial from port->serial, so why store it here?
>

We don't have usb_serial in the gpio functions. Have to do:

struct cp210x_port_private *port_priv =
		 container_of(gc, struct cp210x_port_private, gc);
struct usb_serial *serial = port_priv->serial;

>> +	struct gpio_chip	gc;
>> +#endif
>>   };
>
> ...
>
>>
>>   static struct usb_serial_driver cp210x_device = {
>> @@ -219,6 +229,7 @@ static struct usb_serial_driver cp210x_device = {
>>   	.tx_empty		= cp210x_tx_empty,
>>   	.tiocmget		= cp210x_tiocmget,
>>   	.tiocmset		= cp210x_tiocmset,
>> +	.probe			= cp210x_probe,
>
> Enclose this in CONFIG_GPIOLIB?
> ...
>

Can do, though splattering ifdefs all over the driver isn't particularly 
nice.

I guess the question I have is: Would the preference be to ifdef out all 
extraneous functionality when GPIOLIB isn't enabled or to minimise the 
number of ifdef's at the expense of building in some functionality that 
wasn't then used?

>> +static int cp210x_gpio_get(struct gpio_chip *gc, unsigned gpio)
>> +{
>> +	struct cp210x_port_private *port_priv =
>> +			 container_of(gc, struct cp210x_port_private, gc);
>> +	struct usb_serial *serial = port_priv->serial;
>> +	__le32 *buf;
>> +	int result, i, length;
>> +	int size = 1;
>> +	unsigned int data[size];
>> +
>> +	/* Number of integers required to contain the array */
>> +	length = (((size - 1) | 3) + 1) / 4;
>
> usb_control_msg can deal with any size of the buffer, so use __le16 and remove these length calculations.
>

OK. (This is the process used in the other calls. Was wondering why they 
are in the first place, any ideas?)

>> +
>> +	buf = kcalloc(length, sizeof(__le32), GFP_KERNEL);
>> +	if (!buf)
>> +		return -ENOMEM;
>> +
>> +	/* Issue the request, attempting to read 'size' bytes */
>> +	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev,
>> 0),
>> +				 CP210X_VENDOR_SPECIFIC,
>> +				 REQTYPE_INTERFACE_TO_HOST,
>> CP210X_READ_LATCH,
>> +				 port_priv->bInterfaceNumber, buf, size,
>> +				 USB_CTRL_GET_TIMEOUT);
>> +	if (result != size) {
>
> print err message here and any other time this function fails.
>

OK.

>> +		result = 0;
>> +		goto err;
>> +	}
> ...
>
>> +static int cp210x_shared_gpio_init(struct usb_serial_port *port)
>> +{
>> +	struct usb_serial *serial = port->serial;
>> +	struct cp210x_port_private *port_priv =
>> usb_get_serial_port_data(port);
>> +	__le32 *buf;
>> +	unsigned long tmp;
>
> Wouldn't "chip_features" be better?
>
>> +	int result, mode;
>
> mode is unsigned.

OK.

> ...
>
>>   static int cp210x_port_probe(struct usb_serial_port *port)
>>   {
>>   	struct usb_serial *serial = port->serial;
>> @@ -1008,12 +1198,21 @@ static int cp210x_port_probe(struct
>> usb_serial_port *port)
>>   	usb_set_serial_port_data(port, port_priv);
>>
>>   	ret = cp210x_detect_swapped_line_ctl(port);
>> -	if (ret) {
>> -		kfree(port_priv);
>> -		return ret;
>> -	}
>> +	if (ret)
>> +		goto err_ctl;
>> +
>> +#ifdef CONFIG_GPIOLIB
>> +	ret = cp210x_shared_gpio_init(port);
>> +	if (ret < 0)
>> +		goto err_ctl;
>
> put kfree and return here
> ...
>

Why not use a shared error path?

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

RE: [PATCH v2] USB: serial: cp210x: Adding GPIO support for CP2105

[Konstantin Shkolnyy]

> From: Martyn Welch [mailto:martyn.welch@collabora.co.uk]
> Sent: Thursday, January 14, 2016 04:23
> To: Konstantin Shkolnyy; Johan Hovold; Linus Walleij; Alexandre Courbot
> Cc: Greg Kroah-Hartman; linux-usb@vger.kernel.org; linux-
> gpio@vger.kernel.org
> Subject: Re: [PATCH v2] USB: serial: cp210x: Adding GPIO support for CP2105
> 
> On 14/01/16 00:27, Konstantin Shkolnyy wrote:
> > Comments inline, not comprehensive by any measure...
> >
> >> -----Original Message-----
> >> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-
> >> owner@vger.kernel.org] On Behalf Of Martyn Welch
> >> Sent: Wednesday, January 13, 2016 06:31
> >> To: Johan Hovold; Linus Walleij; Alexandre Courbot
> >> Cc: Greg Kroah-Hartman; linux-usb@vger.kernel.org; linux-
> >> gpio@vger.kernel.org; Martyn Welch
> >> Subject: [PATCH v2] USB: serial: cp210x: Adding GPIO support for CP2105
> >>
> > ...
> >
> >> +#include <linux/gpio/driver.h>
> >> +#include <linux/bitops.h>
> >
> > Enclose this in CONFIG_GPIOLIB?
> > ...
> 
> Is there any point? I took a look at a few other drivers which
> optionally support GPIO and they don't ifdef the headers.

OK, according to other comment, I need to learn about local ifdef policies. I'm sorry.
To me, knowing *why * a header is included seems beneficial, but obviously there are other considerations.

> I think the contents of the headers will effectively be ignored if not
> used and this won't affect module size.

I think a good linker will throw away anything that is not referenced anyway.
...

> >> +static int cp210x_gpio_get(struct gpio_chip *gc, unsigned gpio)
> >> +{
> >> +	struct cp210x_port_private *port_priv =
> >> +			 container_of(gc, struct cp210x_port_private, gc);
> >> +	struct usb_serial *serial = port_priv->serial;
> >> +	__le32 *buf;
> >> +	int result, i, length;
> >> +	int size = 1;
> >> +	unsigned int data[size];
> >> +
> >> +	/* Number of integers required to contain the array */
> >> +	length = (((size - 1) | 3) + 1) / 4;
> >
> > usb_control_msg can deal with any size of the buffer, so use __le16 and
> remove these length calculations.
> >
> 
> OK. (This is the process used in the other calls. Was wondering why they
> are in the first place, any ideas?)

Johan Hovold previously said he didn't like these functions either. I actually submitted a patch to replace them with simpler ones.
...

> >>   static int cp210x_port_probe(struct usb_serial_port *port)
> >>   {
> >>   	struct usb_serial *serial = port->serial;
> >> @@ -1008,12 +1198,21 @@ static int cp210x_port_probe(struct
> >> usb_serial_port *port)
> >>   	usb_set_serial_port_data(port, port_priv);
> >>
> >>   	ret = cp210x_detect_swapped_line_ctl(port);
> >> -	if (ret) {
> >> -		kfree(port_priv);
> >> -		return ret;
> >> -	}
> >> +	if (ret)
> >> +		goto err_ctl;
> >> +
> >> +#ifdef CONFIG_GPIOLIB
> >> +	ret = cp210x_shared_gpio_init(port);
> >> +	if (ret < 0)
> >> +		goto err_ctl;
> >
> > put kfree and return here
> > ...
> >
> 
> Why not use a shared error path?

I don't have a strong opinion here. Just felt like insulating the code insertion into ifdef CONFIG_GPIOLIB. But you also have a good point...

Re: [PATCH v2] USB: serial: cp210x: Adding GPIO support for CP2105

[Martyn Welch]

On 14/01/16 14:29, Konstantin Shkolnyy wrote:
>> From: Martyn Welch [mailto:martyn.welch@collabora.co.uk]
>> Sent: Thursday, January 14, 2016 04:23
>> To: Konstantin Shkolnyy; Johan Hovold; Linus Walleij; Alexandre Courbot
>> Cc: Greg Kroah-Hartman; linux-usb@vger.kernel.org; linux-
>> gpio@vger.kernel.org
>> Subject: Re: [PATCH v2] USB: serial: cp210x: Adding GPIO support for CP2105
>>
>> On 14/01/16 00:27, Konstantin Shkolnyy wrote:
>>> Comments inline, not comprehensive by any measure...
>>>
>>>> -----Original Message-----
>>>> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-
>>>> owner@vger.kernel.org] On Behalf Of Martyn Welch
>>>> Sent: Wednesday, January 13, 2016 06:31
>>>> To: Johan Hovold; Linus Walleij; Alexandre Courbot
>>>> Cc: Greg Kroah-Hartman; linux-usb@vger.kernel.org; linux-
>>>> gpio@vger.kernel.org; Martyn Welch
>>>> Subject: [PATCH v2] USB: serial: cp210x: Adding GPIO support for CP2105
>>>>
>>> ...
>>>
>>>> +#include <linux/gpio/driver.h>
>>>> +#include <linux/bitops.h>
>>>
>>> Enclose this in CONFIG_GPIOLIB?
>>> ...
>>
>> Is there any point? I took a look at a few other drivers which
>> optionally support GPIO and they don't ifdef the headers.
>
> OK, according to other comment, I need to learn about local ifdef policies. I'm sorry.
> To me, knowing *why * a header is included seems beneficial, but obviously there are other considerations.
>
>> I think the contents of the headers will effectively be ignored if not
>> used and this won't affect module size.
>
> I think a good linker will throw away anything that is not referenced anyway.
> ...
>
>>>> +static int cp210x_gpio_get(struct gpio_chip *gc, unsigned gpio)
>>>> +{
>>>> +	struct cp210x_port_private *port_priv =
>>>> +			 container_of(gc, struct cp210x_port_private, gc);
>>>> +	struct usb_serial *serial = port_priv->serial;
>>>> +	__le32 *buf;
>>>> +	int result, i, length;
>>>> +	int size = 1;
>>>> +	unsigned int data[size];
>>>> +
>>>> +	/* Number of integers required to contain the array */
>>>> +	length = (((size - 1) | 3) + 1) / 4;
>>>
>>> usb_control_msg can deal with any size of the buffer, so use __le16 and
>> remove these length calculations.
>>>
>>
>> OK. (This is the process used in the other calls. Was wondering why they
>> are in the first place, any ideas?)
>
> Johan Hovold previously said he didn't like these functions either. I actually submitted a patch to replace them with simpler ones.
> ...
>

Ah! OK, found them in mail archive. Will take a look.

Martyn

Re: [PATCH v2] USB: serial: cp210x: Adding GPIO support for CP2105

[Johan Hovold]

On Thu, Jan 14, 2016 at 10:23:11AM +0000, Martyn Welch wrote:
> On 14/01/16 00:27, Konstantin Shkolnyy wrote:

> >>   static struct usb_serial_driver cp210x_device = {
> >> @@ -219,6 +229,7 @@ static struct usb_serial_driver cp210x_device = {
> >>   	.tx_empty		= cp210x_tx_empty,
> >>   	.tiocmget		= cp210x_tiocmget,
> >>   	.tiocmset		= cp210x_tiocmset,
> >> +	.probe			= cp210x_probe,
> >
> > Enclose this in CONFIG_GPIOLIB?
> > ...
> >
> 
> Can do, though splattering ifdefs all over the driver isn't particularly 
> nice.
> 
> I guess the question I have is: Would the preference be to ifdef out all 
> extraneous functionality when GPIOLIB isn't enabled or to minimise the 
> number of ifdef's at the expense of building in some functionality that 
> wasn't then used?

Try to minimise the ifdefs and use dummy inline functions in case
!CONFIG_GPIOLIB. That way you should not need to add more than two
ifdefs (data + code).

Thanks,
Johan