[PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

[PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

[Preston Fick]

This fix contains several changes that allow toggling of GPIO on CP210x
devices that support it. Changes inclue:
* Added in part number support, necessary to see if the connected device
supports the GPIO functionality
* Added two IOCTLs and ioctl function to allow GET/SET of GPIO
* Modified cp210x_get/set_config functions to allow for both interface and
device requests (also modified all calling functions)
* Removed cp210x_set_config_single since the new cp210x_set_config allows for
variable length requests to be sent
* Added in new #defines for partnum support, new USB requests and changed
"Config request types" section to contain more correct definitions

Signed-off-by: Preston Fick <preston.fick@silabs.com>
---
 drivers/usb/serial/cp210x.c |  239 ++++++++++++++++++++++++++++++-------------
 1 files changed, 170 insertions(+), 69 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index ec30f95..188eb78 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -35,6 +35,8 @@
  */
 static int cp210x_open(struct tty_struct *tty, struct usb_serial_port *);
 static void cp210x_close(struct usb_serial_port *);
+static int cp210x_ioctl(struct tty_struct *tty,
+	unsigned int cmd, unsigned long arg);
 static void cp210x_get_termios(struct tty_struct *,
 	struct usb_serial_port *port);
 static void cp210x_get_termios_port(struct usb_serial_port *port,
@@ -154,6 +156,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 
 struct cp210x_port_private {
 	__u8			bInterfaceNumber;
+	__u8			bPartNumber;
 };
 
 static struct usb_driver cp210x_driver = {
@@ -174,6 +177,7 @@ static struct usb_serial_driver cp210x_device = {
 	.bulk_out_size		= 256,
 	.open			= cp210x_open,
 	.close			= cp210x_close,
+	.ioctl			= cp210x_ioctl,
 	.break_ctl		= cp210x_break_ctl,
 	.set_termios		= cp210x_set_termios,
 	.tiocmget 		= cp210x_tiocmget,
@@ -187,9 +191,22 @@ static struct usb_serial_driver * const serial_drivers[] = {
 	&cp210x_device, NULL
 };
 
+/* Part number definitions */
+#define CP2101_PARTNUM		0x01
+#define CP2102_PARTNUM		0x02
+#define CP2103_PARTNUM		0x03
+#define CP2104_PARTNUM		0x04
+#define CP2105_PARTNUM		0x05
+
+/* IOCTLs */
+#define IOCTL_GPIOGET		0x8000
+#define IOCTL_GPIOSET		0x8001
+
 /* Config request types */
-#define REQTYPE_HOST_TO_DEVICE	0x41
-#define REQTYPE_DEVICE_TO_HOST	0xc1
+#define REQTYPE_HOST_TO_INTERFACE	0x41
+#define REQTYPE_INTERFACE_TO_HOST	0xc1
+#define REQTYPE_HOST_TO_DEVICE	0x40
+#define REQTYPE_DEVICE_TO_HOST	0xc0
 
 /* Config request codes */
 #define CP210X_IFC_ENABLE	0x00
@@ -218,11 +235,17 @@ 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
 #define UART_DISABLE		0x0000
 
+/* CP210X_VENDOR_SPECIFIC */
+#define CP210X_WRITE_LATCH	0x37E1
+#define CP210X_READ_LATCH	0x00C2
+#define CP210X_GET_PARTNUM	0x370B
+
 /* CP210X_(SET|GET)_BAUDDIV */
 #define BAUD_RATE_GEN_FREQ	0x384000
 
@@ -267,8 +290,8 @@ static struct usb_serial_driver * const serial_drivers[] = {
  * 'data' is a pointer to a pre-allocated array of integers large
  * enough to hold 'size' bytes (with 4 bytes to each integer)
  */
-static int cp210x_get_config(struct usb_serial_port *port, u8 request,
-		unsigned int *data, int size)
+static int cp210x_get_config(struct usb_serial_port *port, u8 requestType,
+		u8 request, int value, unsigned int *data, int size)
 {
 	struct usb_serial *serial = port->serial;
 	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
@@ -286,7 +309,7 @@ static int cp210x_get_config(struct usb_serial_port *port, u8 request,
 
 	/* Issue the request, attempting to read 'size' bytes */
 	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
-				request, REQTYPE_DEVICE_TO_HOST, 0x0000,
+				request, requestType, value,
 				port_priv->bInterfaceNumber, buf, size,
 				USB_CTRL_GET_TIMEOUT);
 
@@ -315,45 +338,39 @@ static int cp210x_get_config(struct usb_serial_port *port, u8 request,
  * Values less than 16 bits wide are sent directly
  * 'size' is specified in bytes.
  */
-static int cp210x_set_config(struct usb_serial_port *port, u8 request,
-		unsigned int *data, int size)
+static int cp210x_set_config(struct usb_serial_port *port, u8 requestType,
+		u8 request, int value, unsigned int *data, int size)
 {
 	struct usb_serial *serial = port->serial;
 	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
-	__le32 *buf;
-	int result, i, length;
+	__le32 *buf = NULL;
+	int result, i, length = 0;
 
-	/* Number of integers required to contain the array */
-	length = (((size - 1) | 3) + 1)/4;
-
-	buf = kmalloc(length * sizeof(__le32), GFP_KERNEL);
-	if (!buf) {
-		dev_err(&port->dev, "%s - out of memory.\n",
-				__func__);
-		return -ENOMEM;
-	}
+	if (size) {
+		/* Number of integers required to contain the array */
+		length = (((size - 1) | 3) + 1)/4;
 
-	/* Array of integers into bytes */
-	for (i = 0; i < length; i++)
-		buf[i] = cpu_to_le32(data[i]);
+		buf = kmalloc(length * sizeof(__le32), GFP_KERNEL);
+		if (!buf) {
+			dev_err(&port->dev, "%s - out of memory.\n",
+					__func__);
+			return -ENOMEM;
+		}
 
-	if (size > 2) {
-		result = usb_control_msg(serial->dev,
-				usb_sndctrlpipe(serial->dev, 0),
-				request, REQTYPE_HOST_TO_DEVICE, 0x0000,
-				port_priv->bInterfaceNumber, buf, size,
-				USB_CTRL_SET_TIMEOUT);
-	} else {
-		result = usb_control_msg(serial->dev,
-				usb_sndctrlpipe(serial->dev, 0),
-				request, REQTYPE_HOST_TO_DEVICE, data[0],
-				port_priv->bInterfaceNumber, NULL, 0,
-				USB_CTRL_SET_TIMEOUT);
+		/* 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),
+			request, requestType, value,
+			port_priv->bInterfaceNumber, buf, size,
+			USB_CTRL_SET_TIMEOUT);
+
 	kfree(buf);
 
-	if ((size > 2 && result != size) || result < 0) {
+	if (result != size) {
 		dbg("%s - Unable to send request, "
 				"request=0x%x size=%d result=%d",
 				__func__, request, size, result);
@@ -367,17 +384,6 @@ static int cp210x_set_config(struct usb_serial_port *port, u8 request,
 }
 
 /*
- * cp210x_set_config_single
- * Convenience function for calling cp210x_set_config on single data values
- * without requiring an integer pointer
- */
-static inline int cp210x_set_config_single(struct usb_serial_port *port,
-		u8 request, unsigned int data)
-{
-	return cp210x_set_config(port, request, &data, 2);
-}
-
-/*
  * cp210x_quantise_baudrate
  * Quantises the baud rate as per AN205 Table 1
  */
@@ -424,8 +430,8 @@ static int cp210x_open(struct tty_struct *tty, struct usb_serial_port *port)
 
 	dbg("%s - port %d", __func__, port->number);
 
-	result = cp210x_set_config_single(port, CP210X_IFC_ENABLE,
-								UART_ENABLE);
+	result = cp210x_set_config(port, REQTYPE_HOST_TO_INTERFACE,
+				CP210X_IFC_ENABLE, UART_ENABLE, NULL, 0);
 	if (result) {
 		dev_err(&port->dev, "%s - Unable to enable UART\n", __func__);
 		return result;
@@ -449,10 +455,79 @@ static void cp210x_close(struct usb_serial_port *port)
 
 	mutex_lock(&port->serial->disc_mutex);
 	if (!port->serial->disconnected)
-		cp210x_set_config_single(port, CP210X_IFC_ENABLE, UART_DISABLE);
+		cp210x_set_config(port, REQTYPE_HOST_TO_INTERFACE,
+				CP210X_IFC_ENABLE, UART_DISABLE, NULL, 0);
 	mutex_unlock(&port->serial->disc_mutex);
 }
 
+static int cp210x_ioctl(struct tty_struct *tty,
+	unsigned int cmd, unsigned long arg)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
+	int result = 0;
+	unsigned int latch_mask_setting_to_write = 0;
+
+	switch (cmd) {
+
+	case IOCTL_GPIOGET:
+		if ((port_priv->bPartNumber == CP2103_PARTNUM) ||
+			(port_priv->bPartNumber == CP2104_PARTNUM)) {
+			return cp210x_get_config(port, REQTYPE_DEVICE_TO_HOST,
+						CP210X_VENDOR_SPECIFIC,
+						CP210X_READ_LATCH,
+						(unsigned int *)arg, 1);
+		} else if (port_priv->bPartNumber == CP2105_PARTNUM) {
+			return cp210x_get_config(port,
+						REQTYPE_INTERFACE_TO_HOST,
+						CP210X_VENDOR_SPECIFIC,
+						CP210X_READ_LATCH,
+						(unsigned int *)arg, 1);
+		} else {
+			return -ENOTSUPP;
+		}
+		break;
+
+	case IOCTL_GPIOSET:
+		if ((port_priv->bPartNumber == CP2103_PARTNUM) ||
+			(port_priv->bPartNumber == CP2104_PARTNUM)) {
+			latch_mask_setting_to_write =
+				*(unsigned int *)arg & 0x000000FF;
+			latch_mask_setting_to_write |=
+				(*(unsigned int *)arg & 0x00FF0000) >> 8;
+			result = usb_control_msg(port->serial->dev,
+					usb_sndctrlpipe(port->serial->dev, 0),
+					CP210X_VENDOR_SPECIFIC,
+					REQTYPE_HOST_TO_DEVICE,
+					CP210X_WRITE_LATCH,
+					latch_mask_setting_to_write,
+					NULL, 0, USB_CTRL_SET_TIMEOUT);
+			if (result != 0)
+				return -EPROTO;
+
+			return 0;
+		} else if (port_priv->bPartNumber == CP2105_PARTNUM) {
+			latch_mask_setting_to_write =
+				*(unsigned int *)arg & 0x000000FF;
+			latch_mask_setting_to_write |=
+				(*(unsigned int *)arg & 0x00FF0000) >> 8;
+			return cp210x_set_config(port,
+					REQTYPE_HOST_TO_INTERFACE,
+					CP210X_VENDOR_SPECIFIC,
+					CP210X_WRITE_LATCH,
+					&latch_mask_setting_to_write, 2);
+		} else {
+			return -ENOTSUPP;
+		}
+		break;
+
+	default:
+		break;
+	}
+
+	return -ENOIOCTLCMD;
+}
+
 /*
  * cp210x_get_termios
  * Reads the baud rate, data bits, parity, stop bits and flow control mode
@@ -490,14 +565,16 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 
 	dbg("%s - port %d", __func__, port->number);
 
-	cp210x_get_config(port, CP210X_GET_BAUDRATE, &baud, 4);
+	cp210x_get_config(port, REQTYPE_INTERFACE_TO_HOST,
+			CP210X_GET_BAUDRATE, 0, &baud, 4);
 
 	dbg("%s - baud rate = %d", __func__, baud);
 	*baudp = baud;
 
 	cflag = *cflagp;
 
-	cp210x_get_config(port, CP210X_GET_LINE_CTL, &bits, 2);
+	cp210x_get_config(port, REQTYPE_INTERFACE_TO_HOST,
+			CP210X_GET_LINE_CTL, 0, &bits, 2);
 	cflag &= ~CSIZE;
 	switch (bits & BITS_DATA_MASK) {
 	case BITS_DATA_5:
@@ -522,14 +599,16 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 		cflag |= CS8;
 		bits &= ~BITS_DATA_MASK;
 		bits |= BITS_DATA_8;
-		cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2);
+		cp210x_set_config(port, REQTYPE_HOST_TO_INTERFACE,
+				CP210X_SET_LINE_CTL, 0, &bits, 2);
 		break;
 	default:
 		dbg("%s - Unknown number of data bits, using 8", __func__);
 		cflag |= CS8;
 		bits &= ~BITS_DATA_MASK;
 		bits |= BITS_DATA_8;
-		cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2);
+		cp210x_set_config(port, REQTYPE_HOST_TO_INTERFACE,
+				CP210X_SET_LINE_CTL, 0, &bits, 2);
 		break;
 	}
 
@@ -560,7 +639,8 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 		dbg("%s - Unknown parity mode, disabling parity", __func__);
 		cflag &= ~PARENB;
 		bits &= ~BITS_PARITY_MASK;
-		cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2);
+		cp210x_set_config(port, REQTYPE_HOST_TO_INTERFACE,
+				CP210X_SET_LINE_CTL, 0, &bits, 2);
 		break;
 	}
 
@@ -573,7 +653,8 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 		dbg("%s - stop bits = 1.5 (not supported, using 1 stop bit)",
 								__func__);
 		bits &= ~BITS_STOP_MASK;
-		cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2);
+		cp210x_set_config(port, REQTYPE_HOST_TO_INTERFACE,
+				CP210X_SET_LINE_CTL, 0, &bits, 2);
 		break;
 	case BITS_STOP_2:
 		dbg("%s - stop bits = 2", __func__);
@@ -583,11 +664,13 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 		dbg("%s - Unknown number of stop bits, using 1 stop bit",
 								__func__);
 		bits &= ~BITS_STOP_MASK;
-		cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2);
+		cp210x_set_config(port, REQTYPE_HOST_TO_INTERFACE,
+				CP210X_SET_LINE_CTL, 0, &bits, 2);
 		break;
 	}
 
-	cp210x_get_config(port, CP210X_GET_FLOW, modem_ctl, 16);
+	cp210x_get_config(port, REQTYPE_INTERFACE_TO_HOST,
+			CP210X_GET_FLOW, 0, modem_ctl, 16);
 	if (modem_ctl[0] & 0x0008) {
 		dbg("%s - flow control = CRTSCTS", __func__);
 		cflag |= CRTSCTS;
@@ -640,8 +723,8 @@ static void cp210x_change_speed(struct tty_struct *tty,
 	baud = cp210x_quantise_baudrate(baud);
 
 	dbg("%s - setting baud rate to %u", __func__, baud);
-	if (cp210x_set_config(port, CP210X_SET_BAUDRATE, &baud,
-							sizeof(baud))) {
+	if (cp210x_set_config(port, REQTYPE_HOST_TO_INTERFACE,
+			CP210X_SET_BAUDRATE, 0, &baud, sizeof(baud))) {
 		dev_warn(&port->dev, "failed to set baud rate to %u\n", baud);
 		if (old_termios)
 			baud = old_termios->c_ospeed;
@@ -672,7 +755,8 @@ static void cp210x_set_termios(struct tty_struct *tty,
 
 	/* If the number of data bits is to be updated */
 	if ((cflag & CSIZE) != (old_cflag & CSIZE)) {
-		cp210x_get_config(port, CP210X_GET_LINE_CTL, &bits, 2);
+		cp210x_get_config(port, REQTYPE_INTERFACE_TO_HOST,
+				CP210X_GET_LINE_CTL, 0, &bits, 2);
 		bits &= ~BITS_DATA_MASK;
 		switch (cflag & CSIZE) {
 		case CS5:
@@ -702,14 +786,16 @@ static void cp210x_set_termios(struct tty_struct *tty,
 				bits |= BITS_DATA_8;
 				break;
 		}
-		if (cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2))
+		if (cp210x_set_config(port, REQTYPE_HOST_TO_INTERFACE,
+				CP210X_SET_LINE_CTL, 0, &bits, 2))
 			dbg("Number of data bits requested "
 					"not supported by device");
 	}
 
 	if ((cflag     & (PARENB|PARODD|CMSPAR)) !=
 	    (old_cflag & (PARENB|PARODD|CMSPAR))) {
-		cp210x_get_config(port, CP210X_GET_LINE_CTL, &bits, 2);
+		cp210x_get_config(port, REQTYPE_INTERFACE_TO_HOST,
+				CP210X_GET_LINE_CTL, 0, &bits, 2);
 		bits &= ~BITS_PARITY_MASK;
 		if (cflag & PARENB) {
 			if (cflag & CMSPAR) {
@@ -730,12 +816,14 @@ static void cp210x_set_termios(struct tty_struct *tty,
 			    }
 			}
 		}
-		if (cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2))
+		if (cp210x_set_config(port, REQTYPE_HOST_TO_INTERFACE,
+				CP210X_SET_LINE_CTL, 0, &bits, 2))
 			dbg("Parity mode not supported by device");
 	}
 
 	if ((cflag & CSTOPB) != (old_cflag & CSTOPB)) {
-		cp210x_get_config(port, CP210X_GET_LINE_CTL, &bits, 2);
+		cp210x_get_config(port, REQTYPE_INTERFACE_TO_HOST,
+				CP210X_GET_LINE_CTL, 0, &bits, 2);
 		bits &= ~BITS_STOP_MASK;
 		if (cflag & CSTOPB) {
 			bits |= BITS_STOP_2;
@@ -744,13 +832,15 @@ static void cp210x_set_termios(struct tty_struct *tty,
 			bits |= BITS_STOP_1;
 			dbg("%s - stop bits = 1", __func__);
 		}
-		if (cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2))
+		if (cp210x_set_config(port, REQTYPE_HOST_TO_INTERFACE,
+				CP210X_SET_LINE_CTL, 0, &bits, 2))
 			dbg("Number of stop bits requested "
 					"not supported by device");
 	}
 
 	if ((cflag & CRTSCTS) != (old_cflag & CRTSCTS)) {
-		cp210x_get_config(port, CP210X_GET_FLOW, modem_ctl, 16);
+		cp210x_get_config(port, REQTYPE_INTERFACE_TO_HOST,
+				CP210X_GET_FLOW, 0, modem_ctl, 16);
 		dbg("%s - read modem controls = 0x%.4x 0x%.4x 0x%.4x 0x%.4x",
 				__func__, modem_ctl[0], modem_ctl[1],
 				modem_ctl[2], modem_ctl[3]);
@@ -770,7 +860,8 @@ static void cp210x_set_termios(struct tty_struct *tty,
 		dbg("%s - write modem controls = 0x%.4x 0x%.4x 0x%.4x 0x%.4x",
 				__func__, modem_ctl[0], modem_ctl[1],
 				modem_ctl[2], modem_ctl[3]);
-		cp210x_set_config(port, CP210X_SET_FLOW, modem_ctl, 16);
+		cp210x_set_config(port, REQTYPE_HOST_TO_INTERFACE,
+				CP210X_SET_FLOW, 0, modem_ctl, 16);
 	}
 
 }
@@ -808,7 +899,8 @@ static int cp210x_tiocmset_port(struct usb_serial_port *port,
 
 	dbg("%s - control = 0x%.4x", __func__, control);
 
-	return cp210x_set_config(port, CP210X_SET_MHS, &control, 2);
+	return cp210x_set_config(port, REQTYPE_HOST_TO_INTERFACE,
+				CP210X_SET_MHS, 0, &control, 2);
 }
 
 static void cp210x_dtr_rts(struct usb_serial_port *p, int on)
@@ -827,7 +919,8 @@ static int cp210x_tiocmget (struct tty_struct *tty)
 
 	dbg("%s - port %d", __func__, port->number);
 
-	cp210x_get_config(port, CP210X_GET_MDMSTS, &control, 1);
+	cp210x_get_config(port, REQTYPE_INTERFACE_TO_HOST,
+			CP210X_GET_MDMSTS, 0, &control, 1);
 
 	result = ((control & CONTROL_DTR) ? TIOCM_DTR : 0)
 		|((control & CONTROL_RTS) ? TIOCM_RTS : 0)
@@ -853,13 +946,15 @@ static void cp210x_break_ctl (struct tty_struct *tty, int break_state)
 		state = BREAK_ON;
 	dbg("%s - turning break %s", __func__,
 			state == BREAK_OFF ? "off" : "on");
-	cp210x_set_config(port, CP210X_SET_BREAK, &state, 2);
+	cp210x_set_config(port, REQTYPE_HOST_TO_INTERFACE,
+			CP210X_SET_BREAK, 0, &state, 2);
 }
 
 static int cp210x_startup(struct usb_serial *serial)
 {
 	struct cp210x_port_private *port_priv;
 	int i;
+	unsigned int partNum;
 
 	/* cp210x buffers behave strangely unless device is reset */
 	usb_reset_device(serial->dev);
@@ -874,6 +969,12 @@ static int cp210x_startup(struct usb_serial *serial)
 		    serial->interface->cur_altsetting->desc.bInterfaceNumber;
 
 		usb_set_serial_port_data(serial->port[i], port_priv);
+
+		/* Get the 1-byte part number of the cp210x device */
+		cp210x_get_config(serial->port[i],
+			REQTYPE_DEVICE_TO_HOST, CP210X_VENDOR_SPECIFIC,
+			CP210X_GET_PARTNUM, &partNum, 1);
+		port_priv->bPartNumber = partNum & 0xFF;
 	}
 
 	return 0;
-- 
1.7.5.4

Re: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

[Alan Cox]

>   */
> @@ -424,8 +430,8 @@ static int cp210x_open(struct tty_struct *tty, struct usb_serial_port *port)
>  
>  	dbg("%s - port %d", __func__, port->number);
>  
> -	result = cp210x_set_config_single(port, CP210X_IFC_ENABLE,
> -								UART_ENABLE);
> +	result = cp210x_set_config(port, REQTYPE_HOST_TO_INTERFACE,
> +				CP210X_IFC_ENABLE, UART_ENABLE, NULL, 0);

These kind of unrelated changes make review hard. Why are they there ?

> +	case IOCTL_GPIOGET:
> +		if ((port_priv->bPartNumber == CP2103_PARTNUM) ||


All the part specific stuff in ifs rapidly gets unmaintainable. Better to
set up a port_priv->get_gpio/set_gpio.

>  /*
>   * cp210x_get_termios
>   * Reads the baud rate, data bits, parity, stop bits and flow control mode
> @@ -490,14 +565,16 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
>  
>  	dbg("%s - port %d", __func__, port->number);
>  
> -	cp210x_get_config(port, CP210X_GET_BAUDRATE, &baud, 4);
> +	cp210x_get_config(port, REQTYPE_INTERFACE_TO_HOST,
> +			CP210X_GET_BAUDRATE, 0, &baud, 4);


And again it disturbs all the rest of the code for no apparent good
reason.

  
The large number of changes all over the code for a single localised
feature change ought to be flagging up that it's not being done in a
clean way.

The other question is whether having some custom gpio poking interface is
actually a good idea. I suspect probably not. The kernel gpio layer can
help a bit but doesn't really solve the problem as there is no way to tie
a gpio to a port. Given how many devices seem to have gpios these days I
wonder if we need a gpio setting interface via termiox.

We could also the agree how that maps onto the extra gpio lines used with
SIM card readers and the like so we can standardise that.

Alan

Re: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

[Uwe Bonnes]

>>>>> "Alan" == Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

...
    Alan> The other question is whether having some custom gpio poking
    Alan> interface is actually a good idea. I suspect probably not. The
    Alan> kernel gpio layer can help a bit but doesn't really solve the
    Alan> problem as there is no way to tie a gpio to a port. Given how many
    Alan> devices seem to have gpios these days I wonder if we need a gpio
    Alan> setting interface via termiox.

Is this really kernel stuff or better handled in libusb(x)?

Bye
-- 
Uwe Bonnes                bon@elektron.ikp.physik.tu-darmstadt.de

Institut fuer Kernphysik  Schlossgartenstrasse 9  64289 Darmstadt
--------- Tel. 06151 162516 -------- Fax. 06151 164321 ----------

Re: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

[Alan Cox]

On Sat, 28 Apr 2012 20:17:07 +0200
Uwe Bonnes <bon@elektron.ikp.physik.tu-darmstadt.de> wrote:

> >>>>> "Alan" == Alan Cox <alan@lxorguk.ukuu.org.uk> writes:
> 
> ...
>     Alan> The other question is whether having some custom gpio poking
>     Alan> interface is actually a good idea. I suspect probably not. The
>     Alan> kernel gpio layer can help a bit but doesn't really solve the
>     Alan> problem as there is no way to tie a gpio to a port. Given how many
>     Alan> devices seem to have gpios these days I wonder if we need a gpio
>     Alan> setting interface via termiox.
> 
> Is this really kernel stuff or better handled in libusb(x)

Tricky to do it that way when the kernel driver owns the interface. Also
it's looking increasingly like we'll need to support a variety of "serial
and a couple of extra magic lines" type interfaces for things like SIM
readers.

Alan

RE: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

[Preston Fick]

Hi Alan -

Thanks for the feedback, as only my third (and most intricate) patch I'm still getting used to this process.

Please see responses inline:

Kind Regards -
Preston
________________________________________
From: Alan Cox [alan@lxorguk.ukuu.org.uk]
Sent: Saturday, April 28, 2012 1:05 PM
To: Preston Fick
Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; Preston Fick; linux-serial@vger.kernel.org
Subject: Re: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

>   */
> @@ -424,8 +430,8 @@ static int cp210x_open(struct tty_struct *tty, struct usb_serial_port *port)
>
>       dbg("%s - port %d", __func__, port->number);
>
> -     result = cp210x_set_config_single(port, CP210X_IFC_ENABLE,
> -                                                             UART_ENABLE);
> +     result = cp210x_set_config(port, REQTYPE_HOST_TO_INTERFACE,
> +                             CP210X_IFC_ENABLE, UART_ENABLE, NULL, 0);

>>These kind of unrelated changes make review hard. Why are they there ?

I believe that this driver was originally developed by reverse engineering our device, and wasn't developed in a way that easily configures and calls the USB specific functions - everything is sent as a device request and certain calls actually need to be interface requests. I changed this so that it fills out the URB in a more complete way than it was before by allowing the call to contain more specific information.

I understand that this change is all over the place in this code, so I'm willing to change it back, and just use raw usb functions contained in the ones I modified. This should make it simpler and eliminate this problem.

> +     case IOCTL_GPIOGET:
> +             if ((port_priv->bPartNumber == CP2103_PARTNUM) ||


>>All the part specific stuff in ifs rapidly gets unmaintainable. Better to
>>set up a port_priv->get_gpio/set_gpio.

Can you elaborate on this a bit - I'm not sure I follow.

>  /*
>   * cp210x_get_termios
>   * Reads the baud rate, data bits, parity, stop bits and flow control mode
> @@ -490,14 +565,16 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
>
>       dbg("%s - port %d", __func__, port->number);
>
> -     cp210x_get_config(port, CP210X_GET_BAUDRATE, &baud, 4);
> +     cp210x_get_config(port, REQTYPE_INTERFACE_TO_HOST,
> +                     CP210X_GET_BAUDRATE, 0, &baud, 4);


>>And again it disturbs all the rest of the code for no apparent good
>>reason.

>>The large number of changes all over the code for a single localised
>>feature change ought to be flagging up that it's not being done in a
>>clean way.

Understood, I can plan to resubmit.

>>The other question is whether having some custom gpio poking interface is
>>actually a good idea. I suspect probably not. The kernel gpio layer can
>>help a bit but doesn't really solve the problem as there is no way to tie
>>a gpio to a port. Given how many devices seem to have gpios these days I
>>wonder if we need a gpio setting interface via termiox.

Is there a better way to get this type of support for our devices? The reason I'm adding this here is because our customers need and use this functionality. The way we do this on Windows and Mac is through custom ioctl calls, so I assumed this would be the appropriate way to do this here as well.

>>We could also the agree how that maps onto the extra gpio lines used with
>>SIM card readers and the like so we can standardise that.

I'm open to suggestions on how to properly get this implemented, so if there is some more feedback you can give to point me in the right direction I'd be glad to consider it and resubmit.

>>Alan
This message (including any attachments) is intended only for the use of the individual or entity to which it is addressed and may contain information that is non-public, proprietary, privileged, confidential, and exempt from disclosure under applicable law or may constitute as attorney work product.  If you are not the intended recipient, you are hereby notified that any use, dissemination, distribution, or copying of this communication is strictly prohibited. If you have received this communication in error, notify us immediately by telephone and (i) destroy this message if a facsimile or (ii) delete this message immediately if this is an electronic communication.  

Thank you.

Re: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

[Alan Cox]

> I understand that this change is all over the place in this code, so I'm willing to change it back, and just use raw usb functions contained in the ones I modified. This should make it simpler and eliminate this problem.

I would suggest you split this into two patches then. The first patch
which changes the submission handling but nothing else, and a second
patch which adds the GPIO functions.

> Is there a better way to get this type of support for our devices? The reason I'm adding this here is because our customers need and use this functionality. The way we do this on Windows and Mac is through custom ioctl calls, so I assumed this would be the appropriate way to do this here as well.

I've Cc'd the linux-serial list as well to see what people think. I'd
like to avoid chip specific custom ioctls in favour of a standardised
way of doing it, but I'm not entirely sure how that should look.

That is really a minor detail but an important one so I'd like to see
what other feedback appears over the next few days (including some
weekdays).
 
> >>We could also the agree how that maps onto the extra gpio lines used with
> >>SIM card readers and the like so we can standardise that.
> 
> I'm open to suggestions on how to properly get this implemented, so if there is some more feedback you can give to point me in the right direction I'd be glad to consider it and resubmit.

For what sort of things are the GPIO lines generally used by customers ?

Alan

RE: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

[Preston Fick]

Hi Alan -

More in line:

Kind Regards -
Preston
________________________________________
From: Alan Cox [alan@lxorguk.ukuu.org.uk]
Sent: Saturday, April 28, 2012 4:08 PM
To: Preston Fick
Cc: Preston Fick; gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; linux-serial@vger.kernel.org
Subject: Re: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

> I understand that this change is all over the place in this code, so I'm willing to change it back, and just use raw usb functions contained in the ones I modified. This should make it simpler and eliminate this problem.

>>I would suggest you split this into two patches then. The first patch
>>which changes the submission handling but nothing else, and a second
>>patch which adds the GPIO functions.

For the time being I'm going add in the GPIO support using the raw usb_control_msg function, this should make the patch much simpler. Then if needed I can roll everything into a new calling mechanism later. I'll need to test with our devices then I'll resubmit tomorrow or Monday.

> Is there a better way to get this type of support for our devices? The reason I'm adding this here is because our customers need and use this functionality. The way we do this on Windows and Mac is through custom ioctl calls, so I assumed this would be the appropriate way to do this here as well.

>>I've Cc'd the linux-serial list as well to see what people think. I'd
>>like to avoid chip specific custom ioctls in favour of a standardised
>>way of doing it, but I'm not entirely sure how that should look.

>>That is really a minor detail but an important one so I'd like to see
>>what other feedback appears over the next few days (including some
>>weekdays).

Yes - I can see how something like this could be standardized, but my knowledge of the kernel outside of this driver is limited.

Hopefully this patch can get accepted with my modifications, but if a more generic access to this type of feature is available at some point I'd be glad to help get the driver using that feature instead of something so part specific, as it is now.

> >>We could also the agree how that maps onto the extra gpio lines used with
> >>SIM card readers and the like so we can standardise that.
>
> I'm open to suggestions on how to properly get this implemented, so if there is some more feedback you can give to point me in the right direction I'd be glad to consider it and resubmit.

>>For what sort of things are the GPIO lines generally used by customers ?

I would say they are mostly these would be tied to an LED of some sort, but since these are GPIO they could go to anything. I've seen some customers use them to set states from the host as well to notify whatever MCU may be attached to the RS232 side of the device. They are really just raw pins that can be set low or high for whatever is listening on the other side.

Unfortunately the number of pins and way we access them differs from device to device, and some of the CP210x's don't have GPIO at all. This patch supports our complete portfolio of this type of adapter, so this shouldn't need to be modified often, only if we decide to create a new flavor.

>>Alan

Thanks for the feedback, I agree it will be interesting to see if this sparks a more generic discussion on how the GPIO functionality could be added for multiple devices and drivers in the kernel.
This message (including any attachments) is intended only for the use of the individual or entity to which it is addressed and may contain information that is non-public, proprietary, privileged, confidential, and exempt from disclosure under applicable law or may constitute as attorney work product.  If you are not the intended recipient, you are hereby notified that any use, dissemination, distribution, or copying of this communication is strictly prohibited. If you have received this communication in error, notify us immediately by telephone and (i) destroy this message if a facsimile or (ii) delete this message immediately if this is an electronic communication.  

Thank you.

Re: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

[Uwe Bonnes]

>>>>> "Alan" == Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

    Alan> On Sat, 28 Apr 2012 20:17:07 +0200 Uwe Bonnes
    Alan> <bon@elektron.ikp.physik.tu-darmstadt.de> wrote:

    >> >>>>> "Alan" == Alan Cox <alan@lxorguk.ukuu.org.uk> writes:
    >> 
    >> ...
    Alan> The other question is whether having some custom gpio poking
    Alan> interface is actually a good idea. I suspect probably not. The
    Alan> kernel gpio layer can help a bit but doesn't really solve the
    Alan> problem as there is no way to tie a gpio to a port. Given how many
    Alan> devices seem to have gpios these days I wonder if we need a gpio
    Alan> setting interface via termiox.
    >>  Is this really kernel stuff or better handled in libusb(x)

    Alan> Tricky to do it that way when the kernel driver owns the
    Alan> interface. Also it's looking increasingly like we'll need to
    Alan> support a variety of "serial and a couple of extra magic lines"
    Alan> type interfaces for things like SIM readers.

Well,

most RS232/USB Adapter don't supply and most applications working with these
adapters don't need hard realtime. So even writing and reading the serial
lines can be handled with libusb, like libftdi does.

But as soon as higher level protocols based on the serial line get involved,
a kernel driver is needed...

So when thinking some termiox, some thoughts for the FTDI special modes,
like MPSSE or synchronous FIFO should be spent. It would come handy if I
could open /dev/ttyUSBx, set som termiox and could use the kernel driver for
sending and receiving MPSSE commands. MPSSE is e.g. used in OpenOCD and
xc3sprog to talk e.g. JTAG to external devices.

Bye 
-- 
Uwe Bonnes                bon@elektron.ikp.physik.tu-darmstadt.de

Institut fuer Kernphysik  Schlossgartenstrasse 9  64289 Darmstadt
--------- Tel. 06151 162516 -------- Fax. 06151 164321 ----------

Re: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

[Xiaofan Chen]

On Sun, Apr 29, 2012 at 9:02 PM, Uwe Bonnes
<bon@elektron.ikp.physik.tu-darmstadt.de> wrote:
> most RS232/USB Adapter don't supply and most applications working with these
> adapters don't need hard realtime. So even writing and reading the serial
> lines can be handled with libusb, like libftdi does.
>
> But as soon as higher level protocols based on the serial line get involved,
> a kernel driver is needed...
>
> So when thinking some termiox, some thoughts for the FTDI special modes,
> like MPSSE or synchronous FIFO should be spent. It would come handy if I
> could open /dev/ttyUSBx, set som termiox and could use the kernel driver for
> sending and receiving MPSSE commands. MPSSE is e.g. used in OpenOCD and
> xc3sprog to talk e.g. JTAG to external devices.

On the other hand, OpenOCD and your xc3sprog will probably remain
to be cross-plattform and in that case libusb/libftdi is a good option
to go. There is a patch under review in OpenOCD where libusb-1.0
is used for the MPSSE engine instead of libftdi/ftd2xx and the
performance seems to be better.



-- 
Xiaofan

Re: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

[Greg KH]

On Wed, May 16, 2012 at 04:33:47PM +0100, Alan Cox wrote:
> So the patch looks like this, which seems nice and compact (UNTESTED)
> 
> commit 4164f9b7074e682fe71dad3b57e78521ef9df492
> Author: Alan Cox <alan@linux.intel.com>
> Date:   Wed May 16 15:13:02 2012 +0100
> 
>     tty: Add a gpio helper set
>     
>     Various tty devices have additional control lines which are sometimes used
>     as GPIO pins and sometimes also tied with the serial port to implement
>     protocols such as ISO7816.
>     
>     This code provides a kernel interface for querying the GPIO range of a tty,
>     and to describe the mapping between GPIO pins and control lines. The latter
>     will be needed for some upcoming line discipline support.
>     
>     [Proposal do not merge yet]
>     
>     Not-Signed-off-by: Alan Cox <alan@linux.intel.com>

Wow, that looks really nice and tiny, if that's all that is needed in
the core, that's great.

> diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
> index a1b9a2f..60550e7 100644
> --- a/drivers/tty/tty_ioctl.c
> +++ b/drivers/tty/tty_ioctl.c
> @@ -1071,6 +1071,39 @@ int tty_mode_ioctl(struct tty_struct *tty, struct file *file,
>  	case TCSETXF:
>  		return set_termiox(real_tty, p, TERMIOS_FLUSH);
>  #endif		
> +#ifdef TCGGPIO
> +	case TCGGPIO: {
> +		struct tcgpio gpio;
> +
> +		if (tty->gpio == NULL)
> +			return -EOPNOTSUPP;
> +		mutex_lock(&real_tty->termios_mutex);
> +		memset(&gpio, 0, sizeof(gpio));
> +		gpio.base = tty->gpio->base;
> +		gpio.num = tty->gpio->num;
> +		memcpy(gpio.map, tty->gpio->map, sizeof(gpio.map));
> +		mutex_unlock(&real_tty->termios_mutex);
> +		if (copy_to_user(p, &gpio, sizeof(gpio)))
> +			return -EFAULT;
> +		return 0;
> +	}
> +	case TCSGPIO:
> +        {
> +		struct tcgpio gpio;
> +
> +		if (tty->gpio == NULL)
> +			return -EOPNOTSUPP;
> +		if (copy_from_user(&gpio, p, sizeof(gpio)))
> +			return -EFAULT;
> +		mutex_lock(&real_tty->termios_mutex);
> +		memcpy(tty->gpio->map, gpio.map, sizeof(tty->gpio->map));
> +		/* An ldisc can see this by watching the ioctl go through
> +		but we may want to add a hook */
> +		mutex_unlock(&real_tty->termios_mutex);
> +                return 0;

So how would the lower tty driver get the ioctl to know to now set these
values to the hardware?  I think we at least need a hook for that,
right?  Or would that go through the ldisc?


> +        }
> +                
> +#endif
>  	case TIOCGSOFTCAR:
>  		copy_termios(real_tty, &kterm);
>  		ret = put_user((kterm.c_cflag & CLOCAL) ? 1 : 0,
> diff --git a/include/asm-generic/ioctls.h b/include/asm-generic/ioctls.h
> index 199975f..fee17d3 100644
> --- a/include/asm-generic/ioctls.h
> +++ b/include/asm-generic/ioctls.h
> @@ -74,6 +74,8 @@
>  #define TCSETXW		0x5435
>  #define TIOCSIG		_IOW('T', 0x36, int)  /* pty: generate signal */
>  #define TIOCVHANGUP	0x5437
> +#define TCGGPIO		_IOR('T', 0x38, struct tcgpio)
> +#define TCSGPIO		_IOW('T', 0x39, struct tcgpio)
>  
>  #define FIONCLEX	0x5450
>  #define FIOCLEX		0x5451
> diff --git a/include/asm-generic/termios.h b/include/asm-generic/termios.h
> index d0922ad..3adda38 100644
> --- a/include/asm-generic/termios.h
> +++ b/include/asm-generic/termios.h
> @@ -18,6 +18,18 @@ struct winsize {
>  	unsigned short ws_ypixel;
>  };
>  
> +
> +/* GPIO handling */
> +#define NR_TTY_GPIOMAP	8
> +struct tcgpio			/* User copied version */
> +{
> +	u32 base;
> +	u16 num;
> +	u16 reserved;
> +	u32 map[NR_TTY_GPIOMAP];
> +	u32 reserved2[6];
> +};

__u32 and friends instead?

greg k-h

Re: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

[Alan Cox]

So the patch looks like this, which seems nice and compact (UNTESTED)

commit 4164f9b7074e682fe71dad3b57e78521ef9df492
Author: Alan Cox <alan@linux.intel.com>
Date:   Wed May 16 15:13:02 2012 +0100

    tty: Add a gpio helper set
    
    Various tty devices have additional control lines which are sometimes used
    as GPIO pins and sometimes also tied with the serial port to implement
    protocols such as ISO7816.
    
    This code provides a kernel interface for querying the GPIO range of a tty,
    and to describe the mapping between GPIO pins and control lines. The latter
    will be needed for some upcoming line discipline support.
    
    [Proposal do not merge yet]
    
    Not-Signed-off-by: Alan Cox <alan@linux.intel.com>

diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index a1b9a2f..60550e7 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -1071,6 +1071,39 @@ int tty_mode_ioctl(struct tty_struct *tty, struct file *file,
 	case TCSETXF:
 		return set_termiox(real_tty, p, TERMIOS_FLUSH);
 #endif		
+#ifdef TCGGPIO
+	case TCGGPIO: {
+		struct tcgpio gpio;
+
+		if (tty->gpio == NULL)
+			return -EOPNOTSUPP;
+		mutex_lock(&real_tty->termios_mutex);
+		memset(&gpio, 0, sizeof(gpio));
+		gpio.base = tty->gpio->base;
+		gpio.num = tty->gpio->num;
+		memcpy(gpio.map, tty->gpio->map, sizeof(gpio.map));
+		mutex_unlock(&real_tty->termios_mutex);
+		if (copy_to_user(p, &gpio, sizeof(gpio)))
+			return -EFAULT;
+		return 0;
+	}
+	case TCSGPIO:
+        {
+		struct tcgpio gpio;
+
+		if (tty->gpio == NULL)
+			return -EOPNOTSUPP;
+		if (copy_from_user(&gpio, p, sizeof(gpio)))
+			return -EFAULT;
+		mutex_lock(&real_tty->termios_mutex);
+		memcpy(tty->gpio->map, gpio.map, sizeof(tty->gpio->map));
+		/* An ldisc can see this by watching the ioctl go through
+		but we may want to add a hook */
+		mutex_unlock(&real_tty->termios_mutex);
+                return 0;
+        }
+                
+#endif
 	case TIOCGSOFTCAR:
 		copy_termios(real_tty, &kterm);
 		ret = put_user((kterm.c_cflag & CLOCAL) ? 1 : 0,
diff --git a/include/asm-generic/ioctls.h b/include/asm-generic/ioctls.h
index 199975f..fee17d3 100644
--- a/include/asm-generic/ioctls.h
+++ b/include/asm-generic/ioctls.h
@@ -74,6 +74,8 @@
 #define TCSETXW		0x5435
 #define TIOCSIG		_IOW('T', 0x36, int)  /* pty: generate signal */
 #define TIOCVHANGUP	0x5437
+#define TCGGPIO		_IOR('T', 0x38, struct tcgpio)
+#define TCSGPIO		_IOW('T', 0x39, struct tcgpio)
 
 #define FIONCLEX	0x5450
 #define FIOCLEX		0x5451
diff --git a/include/asm-generic/termios.h b/include/asm-generic/termios.h
index d0922ad..3adda38 100644
--- a/include/asm-generic/termios.h
+++ b/include/asm-generic/termios.h
@@ -18,6 +18,18 @@ struct winsize {
 	unsigned short ws_ypixel;
 };
 
+
+/* GPIO handling */
+#define NR_TTY_GPIOMAP	8
+struct tcgpio			/* User copied version */
+{
+	u32 base;
+	u16 num;
+	u16 reserved;
+	u32 map[NR_TTY_GPIOMAP];
+	u32 reserved2[6];
+};
+
 #define NCC 8
 struct termio {
 	unsigned short c_iflag;		/* input mode flags */
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 9f47ab5..19daf03 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -243,6 +243,18 @@ struct tty_port {
 };
 
 /*
+ * GPIO block, optional
+ */
+
+struct tty_kgpio		/* Kernel gpio struct */
+{
+	u32 base;
+	u16 num;
+	u16 reserved;
+	u32 map[NR_TTY_GPIOMAP];
+};
+	
+/* 
  * Where all of the state associated with a tty is kept while the tty
  * is open.  Since the termios state should be kept even if the tty
  * has been closed --- for things like the baud rate, etc --- it is
@@ -331,6 +343,8 @@ struct tty_struct {
 	/* If the tty has a pending do_SAK, queue it here - akpm */
 	struct work_struct SAK_work;
 	struct tty_port *port;
+
+	struct tty_kgpio *gpio;	/* Optional */
 };
 
 /* Each of a tty's open files has private_data pointing to tty_file_private */

Re: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

[Greg KH]

On Sat, May 05, 2012 at 12:01:37PM +0100, Alan Cox wrote:
> > Will the new ldisc mess with the tty stuff to prevent "normal" serial
> > data and line settings from being handled properly?  If not, that all
> > looks very good to me, thanks for working this out.
> 
> It'll have no effect on that. What it will do is provide a way for things
> like ISO7816 ldiscs to use the extra pins (as they can call from the
> ldisc into the gpio layer) and to know the mapping of the extra control
> wires, which varies by device.

Ok, that sounds good to me.

Re: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

[Alan Cox]

> Will the new ldisc mess with the tty stuff to prevent "normal" serial
> data and line settings from being handled properly?  If not, that all
> looks very good to me, thanks for working this out.

It'll have no effect on that. What it will do is provide a way for things
like ISO7816 ldiscs to use the extra pins (as they can call from the
ldisc into the gpio layer) and to know the mapping of the extra control
wires, which varies by device.

Alan

Re: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

[Greg KH]

On Thu, May 03, 2012 at 09:34:56PM +0100, Alan Cox wrote:
> 
> Ok this is my suggestion based on GregKH comments and a couple of others
> plus some other driver and ldisc stuff that is pending
> 
> - register the gpio lines with the gpio layer dynamically
> - put them in the right place in the device tree (I'll let Greg advise on
>   the best way to do that bit), plus make them visible via the ioctls for
>   convenience and as they will be needed anyway in kernel

Hang them off of the USB-serial device?

> That provides the user space API
> 
> After that I'll add the hooks to the core tty layer code which allow an
> ldisc to adjust the gpio lines.
> 
> For that we'll need
> 
> struct tty_gpio {
> 	u32 base;
> 	u16 num;
> 	u16 reserved;
> #define NR_TTY_GPIOMAP 8
> 	u16 map[NR_TTY_GPIOMAP];
> 	u32 reserved2[4];
> };
> 
> and
> 
> tty->gpiomap
> 
> which will be NULL for most users.
> 
> 
> Plus
> 
> struct tty_gpio d;
> ioctl(tty, TIOCGPIO, &d)
> 
> and
> 
> ioctl(tty, TIOCSGPIO, &d)
> 
> where the only bits that can be updated will be the map.
> 
> 
> 
> So the normal use case from user space would be
> 
> struct tty_gpio d;
> int fd = open("/dev/ttyUSB0", O_RDWR);
> ioctl(tty, TIOCSGPIO, &d);
> 
> stuff using the gpio driver interfaces
> 
> close(fd);
> 
> 
> And setting up for a kernel ldisc something like
> 
> 
> /* Set a GPIO to LDISC signal mapping for ISO7816 */
> ioctl(tty, TIOCGPIO, &d);
> d.map[TTY_GPIO_ISO7816_RESET] = d.base;
> d.map]TTY_GPIO_ISO7816_VCC] = d.base + 1;
> ioctl(tty, TIOCSGPIO, &d);
> 
> /* Switch to the ldisc */
> ld = N_ISO7816;
> ioctl(tty, TCSETD, &ld);
> 
> 
> and we can then abstract all the wiring details away to keep the ldisc
> portable.
> 
> 
> Thoughts ?

Will the new ldisc mess with the tty stuff to prevent "normal" serial
data and line settings from being handled properly?  If not, that all
looks very good to me, thanks for working this out.

greg k-h

Re: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

[Alan Cox]

On Fri, 04 May 2012 18:26:58 +0200
Alexander Stein <a.stein@systec-electronic.com> wrote:

> Am Montag, 30. April 2012, 15:27:17 schrieb Preston Fick:
> > This fix contains several changes that allow toggling of GPIO on CP210x
> > devices that support it. Changes include:
> > * Added in part number support, necessary to see if the connected device
> > supports the GPIO functionality
> > * Added two IOCTLs and ioctl function to allow GET/SET of GPIO
> > * Added in new #defines for partnum support, new USB requests
> > * Changed "Config request types" section to contain more correct definitions
> > for request recipient
> > 
> > Signed-off-by: Preston Fick <preston.fick@silabs.com>
> 
> Do you actually need to create new ioctls? You also could support gpiolib and 
> create a gpiochip for that.

See my follow up posting, and comment on that proposal.

Re: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

[Alexander Stein]

Am Montag, 30. April 2012, 15:27:17 schrieb Preston Fick:
> This fix contains several changes that allow toggling of GPIO on CP210x
> devices that support it. Changes include:
> * Added in part number support, necessary to see if the connected device
> supports the GPIO functionality
> * Added two IOCTLs and ioctl function to allow GET/SET of GPIO
> * Added in new #defines for partnum support, new USB requests
> * Changed "Config request types" section to contain more correct definitions
> for request recipient
> 
> Signed-off-by: Preston Fick <preston.fick@silabs.com>

Do you actually need to create new ioctls? You also could support gpiolib and 
create a gpiochip for that.

just my 2 cents,
Alexander

RE: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

[Preston Fick]

Hi Alan -

Thanks for this detailed explanation - I'll continue to look deeper into this, a lot of this is new to me. If Greg is in agreement here, then I will work on this as a solution to our GPIO support and submit a series of patches for this when it's ready and tested.

Kind Regards -
Preston

-----Original Message-----
From: Alan Cox [mailto:alan@lxorguk.ukuu.org.uk] 
Sent: Thursday, May 03, 2012 3:35 PM
To: Preston Fick
Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; linux-serial@vger.kernel.org; Preston Fick; linux-arm-kernel@lists.arm.linux.org.uk
Subject: Re: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)


Ok this is my suggestion based on GregKH comments and a couple of others
plus some other driver and ldisc stuff that is pending

- register the gpio lines with the gpio layer dynamically
- put them in the right place in the device tree (I'll let Greg advise on
  the best way to do that bit), plus make them visible via the ioctls for
  convenience and as they will be needed anyway in kernel

That provides the user space API

After that I'll add the hooks to the core tty layer code which allow an
ldisc to adjust the gpio lines.

For that we'll need

struct tty_gpio {
	u32 base;
	u16 num;
	u16 reserved;
#define NR_TTY_GPIOMAP 8
	u16 map[NR_TTY_GPIOMAP];
	u32 reserved2[4];
};

and

tty->gpiomap

which will be NULL for most users.


Plus

struct tty_gpio d;
ioctl(tty, TIOCGPIO, &d)

and

ioctl(tty, TIOCSGPIO, &d)

where the only bits that can be updated will be the map.



So the normal use case from user space would be

struct tty_gpio d;
int fd = open("/dev/ttyUSB0", O_RDWR);
ioctl(tty, TIOCSGPIO, &d);

stuff using the gpio driver interfaces

close(fd);


And setting up for a kernel ldisc something like


/* Set a GPIO to LDISC signal mapping for ISO7816 */
ioctl(tty, TIOCGPIO, &d);
d.map[TTY_GPIO_ISO7816_RESET] = d.base;
d.map]TTY_GPIO_ISO7816_VCC] = d.base + 1;
ioctl(tty, TIOCSGPIO, &d);

/* Switch to the ldisc */
ld = N_ISO7816;
ioctl(tty, TCSETD, &ld);


and we can then abstract all the wiring details away to keep the ldisc
portable.


Thoughts ?

Alan
This message (including any attachments) is intended only for the use of the individual or entity to which it is addressed and may contain information that is non-public, proprietary, privileged, confidential, and exempt from disclosure under applicable law or may constitute as attorney work product.  If you are not the intended recipient, you are hereby notified that any use, dissemination, distribution, or copying of this communication is strictly prohibited. If you have received this communication in error, notify us immediately by telephone and (i) destroy this message if a facsimile or (ii) delete this message immediately if this is an electronic communication.  

Thank you.

Re: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

[Alan Cox]

Ok this is my suggestion based on GregKH comments and a couple of others
plus some other driver and ldisc stuff that is pending

- register the gpio lines with the gpio layer dynamically
- put them in the right place in the device tree (I'll let Greg advise on
  the best way to do that bit), plus make them visible via the ioctls for
  convenience and as they will be needed anyway in kernel

That provides the user space API

After that I'll add the hooks to the core tty layer code which allow an
ldisc to adjust the gpio lines.

For that we'll need

struct tty_gpio {
	u32 base;
	u16 num;
	u16 reserved;
#define NR_TTY_GPIOMAP 8
	u16 map[NR_TTY_GPIOMAP];
	u32 reserved2[4];
};

and

tty->gpiomap

which will be NULL for most users.


Plus

struct tty_gpio d;
ioctl(tty, TIOCGPIO, &d)

and

ioctl(tty, TIOCSGPIO, &d)

where the only bits that can be updated will be the map.



So the normal use case from user space would be

struct tty_gpio d;
int fd = open("/dev/ttyUSB0", O_RDWR);
ioctl(tty, TIOCSGPIO, &d);

stuff using the gpio driver interfaces

close(fd);


And setting up for a kernel ldisc something like


/* Set a GPIO to LDISC signal mapping for ISO7816 */
ioctl(tty, TIOCGPIO, &d);
d.map[TTY_GPIO_ISO7816_RESET] = d.base;
d.map]TTY_GPIO_ISO7816_VCC] = d.base + 1;
ioctl(tty, TIOCSGPIO, &d);

/* Switch to the ldisc */
ld = N_ISO7816;
ioctl(tty, TCSETD, &ld);


and we can then abstract all the wiring details away to keep the ldisc
portable.


Thoughts ?

Alan

Re: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

[Greg KH]

On Mon, Apr 30, 2012 at 03:27:17PM -0500, Preston Fick wrote:
> This fix contains several changes that allow toggling of GPIO on CP210x
> devices that support it. Changes include:
> * Added in part number support, necessary to see if the connected device
> supports the GPIO functionality
> * Added two IOCTLs and ioctl function to allow GET/SET of GPIO
> * Added in new #defines for partnum support, new USB requests
> * Changed "Config request types" section to contain more correct definitions
> for request recipient

No, please break this out into one-patch-per-change, don't bundle all of
this together into one, as I can't take it as-is, sorry.

Especially with those new ioctls...

thanks,

greg k-h

[PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

[Preston Fick]

This fix contains several changes that allow toggling of GPIO on CP210x
devices that support it. Changes include:
* Added in part number support, necessary to see if the connected device
supports the GPIO functionality
* Added two IOCTLs and ioctl function to allow GET/SET of GPIO
* Added in new #defines for partnum support, new USB requests
* Changed "Config request types" section to contain more correct definitions
for request recipient

Signed-off-by: Preston Fick <preston.fick@silabs.com>
---
 drivers/usb/serial/cp210x.c |  134 +++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 129 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index ec30f95..9d1e542 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -35,6 +35,8 @@
  */
 static int cp210x_open(struct tty_struct *tty, struct usb_serial_port *);
 static void cp210x_close(struct usb_serial_port *);
+static int cp210x_ioctl(struct tty_struct *tty,
+	unsigned int cmd, unsigned long arg);
 static void cp210x_get_termios(struct tty_struct *,
 	struct usb_serial_port *port);
 static void cp210x_get_termios_port(struct usb_serial_port *port,
@@ -154,6 +156,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 
 struct cp210x_port_private {
 	__u8			bInterfaceNumber;
+	__u8			bPartNumber;
 };
 
 static struct usb_driver cp210x_driver = {
@@ -174,6 +177,7 @@ static struct usb_serial_driver cp210x_device = {
 	.bulk_out_size		= 256,
 	.open			= cp210x_open,
 	.close			= cp210x_close,
+	.ioctl			= cp210x_ioctl,
 	.break_ctl		= cp210x_break_ctl,
 	.set_termios		= cp210x_set_termios,
 	.tiocmget 		= cp210x_tiocmget,
@@ -187,9 +191,22 @@ static struct usb_serial_driver * const serial_drivers[] = {
 	&cp210x_device, NULL
 };
 
+/* Part number definitions */
+#define CP2101_PARTNUM		0x01
+#define CP2102_PARTNUM		0x02
+#define CP2103_PARTNUM		0x03
+#define CP2104_PARTNUM		0x04
+#define CP2105_PARTNUM		0x05
+
+/* IOCTLs */
+#define IOCTL_GPIOGET		0x8000
+#define IOCTL_GPIOSET		0x8001
+
 /* Config request types */
-#define REQTYPE_HOST_TO_DEVICE	0x41
-#define REQTYPE_DEVICE_TO_HOST	0xc1
+#define REQTYPE_HOST_TO_INTERFACE	0x41
+#define REQTYPE_INTERFACE_TO_HOST	0xc1
+#define REQTYPE_HOST_TO_DEVICE	0x40
+#define REQTYPE_DEVICE_TO_HOST	0xc0
 
 /* Config request codes */
 #define CP210X_IFC_ENABLE	0x00
@@ -218,11 +235,17 @@ 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
 #define UART_DISABLE		0x0000
 
+/* CP210X_VENDOR_SPECIFIC */
+#define CP210X_WRITE_LATCH	0x37E1
+#define CP210X_READ_LATCH	0x00C2
+#define CP210X_GET_PARTNUM	0x370B
+
 /* CP210X_(SET|GET)_BAUDDIV */
 #define BAUD_RATE_GEN_FREQ	0x384000
 
@@ -286,7 +309,7 @@ static int cp210x_get_config(struct usb_serial_port *port, u8 request,
 
 	/* Issue the request, attempting to read 'size' bytes */
 	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
-				request, REQTYPE_DEVICE_TO_HOST, 0x0000,
+				request, REQTYPE_INTERFACE_TO_HOST, 0x0000,
 				port_priv->bInterfaceNumber, buf, size,
 				USB_CTRL_GET_TIMEOUT);
 
@@ -340,13 +363,13 @@ static int cp210x_set_config(struct usb_serial_port *port, u8 request,
 	if (size > 2) {
 		result = usb_control_msg(serial->dev,
 				usb_sndctrlpipe(serial->dev, 0),
-				request, REQTYPE_HOST_TO_DEVICE, 0x0000,
+				request, REQTYPE_HOST_TO_INTERFACE, 0x0000,
 				port_priv->bInterfaceNumber, buf, size,
 				USB_CTRL_SET_TIMEOUT);
 	} else {
 		result = usb_control_msg(serial->dev,
 				usb_sndctrlpipe(serial->dev, 0),
-				request, REQTYPE_HOST_TO_DEVICE, data[0],
+				request, REQTYPE_HOST_TO_INTERFACE, data[0],
 				port_priv->bInterfaceNumber, NULL, 0,
 				USB_CTRL_SET_TIMEOUT);
 	}
@@ -453,6 +476,95 @@ static void cp210x_close(struct usb_serial_port *port)
 	mutex_unlock(&port->serial->disc_mutex);
 }
 
+static int cp210x_ioctl(struct tty_struct *tty,
+	unsigned int cmd, unsigned long arg)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
+	int result = 0;
+	unsigned int latch_setting = 0;
+
+	switch (cmd) {
+
+	case IOCTL_GPIOGET:
+		if ((port_priv->bPartNumber == CP2103_PARTNUM) ||
+			(port_priv->bPartNumber == CP2104_PARTNUM)) {
+			result = usb_control_msg(port->serial->dev,
+					usb_rcvctrlpipe(port->serial->dev, 0),
+					CP210X_VENDOR_SPECIFIC,
+					REQTYPE_DEVICE_TO_HOST,
+					CP210X_READ_LATCH,
+					port_priv->bInterfaceNumber,
+					&latch_setting, 1,
+					USB_CTRL_GET_TIMEOUT);
+			if (result != 1)
+				return -EPROTO;
+			*(unsigned long *)arg = (unsigned long)latch_setting;
+			return 0;
+		} else if (port_priv->bPartNumber == CP2105_PARTNUM) {
+			result = usb_control_msg(port->serial->dev,
+					usb_rcvctrlpipe(port->serial->dev, 0),
+					CP210X_VENDOR_SPECIFIC,
+					REQTYPE_INTERFACE_TO_HOST,
+					CP210X_READ_LATCH,
+					port_priv->bInterfaceNumber,
+					&latch_setting, 1,
+					USB_CTRL_GET_TIMEOUT);
+			if (result != 1)
+				return -EPROTO;
+			*(unsigned long *)arg = (unsigned long)latch_setting;
+			return 0;
+		} else {
+			return -ENOTSUPP;
+		}
+		break;
+
+	case IOCTL_GPIOSET:
+		if ((port_priv->bPartNumber == CP2103_PARTNUM) ||
+			(port_priv->bPartNumber == CP2104_PARTNUM)) {
+			latch_setting =
+				*(unsigned int *)arg & 0x000000FF;
+			latch_setting |=
+				(*(unsigned int *)arg & 0x00FF0000) >> 8;
+			result = usb_control_msg(port->serial->dev,
+					usb_sndctrlpipe(port->serial->dev, 0),
+					CP210X_VENDOR_SPECIFIC,
+					REQTYPE_HOST_TO_DEVICE,
+					CP210X_WRITE_LATCH,
+					latch_setting,
+					NULL, 0,
+					USB_CTRL_SET_TIMEOUT);
+			if (result != 0)
+				return -EPROTO;
+			return 0;
+		} else if (port_priv->bPartNumber == CP2105_PARTNUM) {
+			latch_setting =
+				*(unsigned int *)arg & 0x000000FF;
+			latch_setting |=
+				(*(unsigned int *)arg & 0x00FF0000) >> 8;
+			result = usb_control_msg(port->serial->dev,
+					usb_sndctrlpipe(port->serial->dev, 0),
+					CP210X_VENDOR_SPECIFIC,
+					REQTYPE_HOST_TO_INTERFACE,
+					CP210X_WRITE_LATCH,
+					port_priv->bInterfaceNumber,
+					&latch_setting, 2,
+					USB_CTRL_SET_TIMEOUT);
+			if (result != 2)
+				return -EPROTO;
+			return 0;
+		} else {
+			return -ENOTSUPP;
+		}
+		break;
+
+	default:
+		break;
+	}
+
+	return -ENOIOCTLCMD;
+}
+
 /*
  * cp210x_get_termios
  * Reads the baud rate, data bits, parity, stop bits and flow control mode
@@ -860,6 +972,7 @@ static int cp210x_startup(struct usb_serial *serial)
 {
 	struct cp210x_port_private *port_priv;
 	int i;
+	unsigned int partNum;
 
 	/* cp210x buffers behave strangely unless device is reset */
 	usb_reset_device(serial->dev);
@@ -874,6 +987,17 @@ static int cp210x_startup(struct usb_serial *serial)
 		    serial->interface->cur_altsetting->desc.bInterfaceNumber;
 
 		usb_set_serial_port_data(serial->port[i], port_priv);
+
+		/* Get the 1-byte part number of the cp210x device */
+		usb_control_msg(serial->dev,
+			usb_rcvctrlpipe(serial->dev, 0),
+			CP210X_VENDOR_SPECIFIC,
+			REQTYPE_DEVICE_TO_HOST,
+			CP210X_GET_PARTNUM,
+			port_priv->bInterfaceNumber,
+			&partNum, 1,
+			USB_CTRL_GET_TIMEOUT);
+		port_priv->bPartNumber = partNum & 0xFF;
 	}
 
 	return 0;
-- 
1.7.5.4