[PATCH 1/3] usb: cp210x: Corrected USB request type definitions

[PATCH 1/3] usb: cp210x: Corrected USB request type definitions

[Preston Fick]

The original request types in the cp210x driver are labled as "DEVICE_TO_HOST" and
"HOST_TO_DEVICE" but the actual bit definition corresponds to a request to the
interface. This has been corrected, and the actual definition for the device
requests have been added.

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

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index ec30f95..e67ccf3 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -188,8 +188,10 @@ static struct usb_serial_driver * const serial_drivers[] = {
 };
 
 /* 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
@@ -286,7 +288,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 +342,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);
 	}
-- 
1.7.5.4

[PATCH 2/3] usb: cp210x: Added in support to get and store part number

[Preston Fick]

This change gets the part number of the device when the driver is loaded and
stores it in the private portion of the port structure. This addition will
allow for part specific functionality to be added to the driver if needed.

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

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index e67ccf3..b3646b8 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -154,6 +154,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 
 struct cp210x_port_private {
 	__u8			bInterfaceNumber;
+	__u8			bPartNumber;
 };
 
 static struct usb_driver cp210x_driver = {
@@ -187,6 +188,13 @@ 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
+
 /* Config request types */
 #define REQTYPE_HOST_TO_INTERFACE	0x41
 #define REQTYPE_INTERFACE_TO_HOST	0xc1
@@ -220,11 +228,15 @@ 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_GET_PARTNUM	0x370B
+
 /* CP210X_(SET|GET)_BAUDDIV */
 #define BAUD_RATE_GEN_FREQ	0x384000
 
@@ -862,6 +874,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);
@@ -876,6 +889,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

Re: [PATCH 2/3] usb: cp210x: Added in support to get and store part number

[Sergei Shtylyov]

Hello.

On 01-05-2012 8:06, Preston Fick wrote:

> This change gets the part number of the device when the driver is loaded and
> stores it in the private portion of the port structure. This addition will
> allow for part specific functionality to be added to the driver if needed.

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

> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index e67ccf3..b3646b8 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
[...]
> @@ -862,6 +874,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);
> @@ -876,6 +889,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,

    This may involve DMA...

> +			usb_rcvctrlpipe(serial->dev, 0),
> +			CP210X_VENDOR_SPECIFIC,
> +			REQTYPE_DEVICE_TO_HOST,
> +			CP210X_GET_PARTNUM,
> +			port_priv->bInterfaceNumber,
> +			&partNum, 1,

    You can't do DMA to a buffer situated on stack. You should kmalloc() the 
buffer.

> +			USB_CTRL_GET_TIMEOUT);
> +		port_priv->bPartNumber = partNum&  0xFF;
>   	}
>
>   	return 0;

WBR, Sergei

Re: [PATCH 2/3] usb: cp210x: Added in support to get and store part number

[Greg KH]

On Tue, May 01, 2012 at 08:16:09PM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 01-05-2012 8:06, Preston Fick wrote:
> 
> >This change gets the part number of the device when the driver is loaded and
> >stores it in the private portion of the port structure. This addition will
> >allow for part specific functionality to be added to the driver if needed.
> 
> >Signed-off-by: Preston Fick<preston.fick@silabs.com>
> >---
> >  drivers/usb/serial/cp210x.c |   24 ++++++++++++++++++++++++
> >  1 files changed, 24 insertions(+), 0 deletions(-)
> 
> >diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> >index e67ccf3..b3646b8 100644
> >--- a/drivers/usb/serial/cp210x.c
> >+++ b/drivers/usb/serial/cp210x.c
> [...]
> >@@ -862,6 +874,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);
> >@@ -876,6 +889,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,
> 
>    This may involve DMA...
> 
> >+			usb_rcvctrlpipe(serial->dev, 0),
> >+			CP210X_VENDOR_SPECIFIC,
> >+			REQTYPE_DEVICE_TO_HOST,
> >+			CP210X_GET_PARTNUM,
> >+			port_priv->bInterfaceNumber,
> >+			&partNum, 1,
> 
>    You can't do DMA to a buffer situated on stack. You should
> kmalloc() the buffer.

You "must" kmalloc() the buffer, otherwise the driver will break on some
systems.

Sorry, I can't accept this patch because of this.  Please redo it.

greg k-h

Re: [PATCH 2/3] usb: cp210x: Added in support to get and store part number

[Greg KH]

On Tue, May 01, 2012 at 08:16:09PM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 01-05-2012 8:06, Preston Fick wrote:
> 
> >This change gets the part number of the device when the driver is loaded and
> >stores it in the private portion of the port structure. This addition will
> >allow for part specific functionality to be added to the driver if needed.
> 
> >Signed-off-by: Preston Fick<preston.fick-S6d6foEdJf7QT0dZR+AlfA@public.gmane.org>
> >---
> >  drivers/usb/serial/cp210x.c |   24 ++++++++++++++++++++++++
> >  1 files changed, 24 insertions(+), 0 deletions(-)
> 
> >diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> >index e67ccf3..b3646b8 100644
> >--- a/drivers/usb/serial/cp210x.c
> >+++ b/drivers/usb/serial/cp210x.c
> [...]
> >@@ -862,6 +874,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);
> >@@ -876,6 +889,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,
> 
>    This may involve DMA...
> 
> >+			usb_rcvctrlpipe(serial->dev, 0),
> >+			CP210X_VENDOR_SPECIFIC,
> >+			REQTYPE_DEVICE_TO_HOST,
> >+			CP210X_GET_PARTNUM,
> >+			port_priv->bInterfaceNumber,
> >+			&partNum, 1,
> 
>    You can't do DMA to a buffer situated on stack. You should
> kmalloc() the buffer.

You "must" kmalloc() the buffer, otherwise the driver will break on some
systems.

Sorry, I can't accept this patch because of this.  Please redo it.

greg k-h
--
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

[PATCH 3/3] usb: cp210x: Add ioctl for GPIO support

[Preston Fick]

This patch adds support for GPIO for CP210x devices that support it through two
IOCTLs to get or set the GPIO latch on a CP210x device. The specification for
this can be found in Silicon Labs AN571 document on section 5.27.1-4.

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

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index b3646b8..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,
@@ -175,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,
@@ -195,6 +198,10 @@ static struct usb_serial_driver * const serial_drivers[] = {
 #define CP2104_PARTNUM		0x04
 #define CP2105_PARTNUM		0x05
 
+/* IOCTLs */
+#define IOCTL_GPIOGET		0x8000
+#define IOCTL_GPIOSET		0x8001
+
 /* Config request types */
 #define REQTYPE_HOST_TO_INTERFACE	0x41
 #define REQTYPE_INTERFACE_TO_HOST	0xc1
@@ -235,6 +242,8 @@ static struct usb_serial_driver * const serial_drivers[] = {
 #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 */
@@ -467,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
-- 
1.7.5.4

Re: [PATCH 3/3] usb: cp210x: Add ioctl for GPIO support

[Greg KH]

On Mon, Apr 30, 2012 at 11:06:50PM -0500, Preston Fick wrote:
> This patch adds support for GPIO for CP210x devices that support it through two
> IOCTLs to get or set the GPIO latch on a CP210x device. The specification for
> this can be found in Silicon Labs AN571 document on section 5.27.1-4.
> 
> Signed-off-by: Preston Fick <preston.fick@silabs.com>
> ---
>  drivers/usb/serial/cp210x.c |   98 +++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 98 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index b3646b8..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,
> @@ -175,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,
> @@ -195,6 +198,10 @@ static struct usb_serial_driver * const serial_drivers[] = {
>  #define CP2104_PARTNUM		0x04
>  #define CP2105_PARTNUM		0x05
>  
> +/* IOCTLs */
> +#define IOCTL_GPIOGET		0x8000
> +#define IOCTL_GPIOSET		0x8001

As Alan pointed out, we can't just add random ioctls for individual
drivers for this type of thing.  We need to standardize on this.

Actually, why can't you use the GPIO subsystem for something like this?
Can't you export your device as both a usb-serial device and a gpio
device and have things work properly that way?

thanks,

greg k-h

Re: [PATCH 3/3] usb: cp210x: Add ioctl for GPIO support

[Alan Cox]

> Actually, why can't you use the GPIO subsystem for something like this?
> Can't you export your device as both a usb-serial device and a gpio
> device and have things work properly that way?

You still need the ioctls even then in order to discover the gpio
numbers, and having done that youi've got potential races with unload
when you try and open them. You've also got permissions considerations
and synchronization between gpio and data problems.

It's not a good way to go. It might make sense in some platforms to
expose them as both but its not a good general model.

I'm currently favouring adding some 'additional control line' bits to
termiox.

Re: [PATCH 3/3] usb: cp210x: Add ioctl for GPIO support

[Greg KH]

On Wed, May 02, 2012 at 09:49:01PM +0100, Alan Cox wrote:
> > Actually, why can't you use the GPIO subsystem for something like this?
> > Can't you export your device as both a usb-serial device and a gpio
> > device and have things work properly that way?
> 
> You still need the ioctls even then in order to discover the gpio
> numbers

What discovery?  The device knows what gpio values it has in it, and
should be able to register those with the gpio subsystem.

> and having done that youi've got potential races with unload
> when you try and open them. You've also got permissions considerations
> and synchronization between gpio and data problems.

That can be handled in the driver itself, if it really is a problem (the
existing patch sure didn't handle any of that at all, so I'm guessing
either it wasn't considered, or it isn't a problem.)

> It's not a good way to go. It might make sense in some platforms to
> expose them as both but its not a good general model.

Why isn't the gpio subsystem a good general model?  I thought that is
what it was created to solve?

> I'm currently favouring adding some 'additional control line' bits to
> termiox.

Yes, but that's only good for usb-serial devices that also have gpio
pins on the controller side.  Which seems pretty limited to me.

If I have a userspace program, and I want to use GPIO, I shouldn't have
to care/know that the pins are really on the end of a USB->serial
bridge, or somewhere hanging off of a SOC, the same userspace api should
"just work", right?

Or am I missing something really obvious here?

thanks,

greg k-h

Re: [PATCH 3/3] usb: cp210x: Add ioctl for GPIO support

[Alan Cox]

On Wed, 2 May 2012 14:52:26 -0700
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Wed, May 02, 2012 at 09:49:01PM +0100, Alan Cox wrote:
> > > Actually, why can't you use the GPIO subsystem for something like this?
> > > Can't you export your device as both a usb-serial device and a gpio
> > > device and have things work properly that way?
> > 
> > You still need the ioctls even then in order to discover the gpio
> > numbers
> 
> What discovery?  The device knows what gpio values it has in it, and
> should be able to register those with the gpio subsystem.

You open /dev/ttyUSB0

Ok now in your user application how are you going to find which gpio
numbers to use that are associated with this specific port, and how is
udev going to do that to manage permissions ?

So you need an ioctl to give you the range that is mapped to this (or a
sysfs node, but the sysfs node makes the security problem pretty
much insoluble)

> > and having done that youi've got potential races with unload
> > when you try and open them. You've also got permissions considerations
> > and synchronization between gpio and data problems.
> 
> That can be handled in the driver itself, if it really is a problem (the
> existing patch sure didn't handle any of that at all, so I'm guessing
> either it wasn't considered, or it isn't a problem.)

Not reliably

	open /dev/ttyUSB0 [or sysfs node]
	read gpio numbers
	close 

	open gpio foo

Oh dear... so random shell scripting user is going to screw up horribly.

> > It's not a good way to go. It might make sense in some platforms to
> > expose them as both but its not a good general model.
> 
> Why isn't the gpio subsystem a good general model?  I thought that is
> what it was created to solve?

gpio provides a flat abstraction for arbitary pins on a platform. It's
really oriented to fairly fixed system stuff. It doesn't provide a useful
abstraction for extra carrier lines.

Imagine if RTS and DTR were driven by GPIO pins instead of the tty
layer. Many devices use them as extra magic GPIO lines not as tty control
lines, so its an equivalent argument.

> > I'm currently favouring adding some 'additional control line' bits to
> > termiox.
> 
> Yes, but that's only good for usb-serial devices that also have gpio
> pins on the controller side.  Which seems pretty limited to me.
> 
> If I have a userspace program, and I want to use GPIO, I shouldn't have
> to care/know that the pins are really on the end of a USB->serial
> bridge, or somewhere hanging off of a SOC, the same userspace api should
> "just work", right?

For certain applications that makes more sense, which is why I said you
may well want to expose it as both. However you still need both, and
ideally we need a standardised pattern of assignments for line
disciplines that use the extra control lines (which has come up recently
for another device with extra lines)

I think it basically boils down to this

If you have a serial port with some gpio lines that drive arbitary
unrelated electronics then the gpio interface is handy because you can
use the same code logic as if it was wired to other pins elsewhere

If you are using them as part of the tty interface as extra control lines
(eg for smartcard protocols) then you want them driven via the tty
interface and doubly so once we add some of the smartcard/sim ldisc
support.

Hence we really need to expose them both ways because end users are doing
both things with the gpio pins on these ports.

So I'd suggest we expose them via termiox bits and also via the tty
providing gpio range info in a standardised way.

Alan

Re: [PATCH 3/3] usb: cp210x: Add ioctl for GPIO support

[Greg KH]

On Wed, May 02, 2012 at 11:10:27PM +0100, Alan Cox wrote:
> On Wed, 2 May 2012 14:52:26 -0700
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Wed, May 02, 2012 at 09:49:01PM +0100, Alan Cox wrote:
> > > > Actually, why can't you use the GPIO subsystem for something like this?
> > > > Can't you export your device as both a usb-serial device and a gpio
> > > > device and have things work properly that way?
> > > 
> > > You still need the ioctls even then in order to discover the gpio
> > > numbers
> > 
> > What discovery?  The device knows what gpio values it has in it, and
> > should be able to register those with the gpio subsystem.
> 
> You open /dev/ttyUSB0
> 
> Ok now in your user application how are you going to find which gpio
> numbers to use that are associated with this specific port,

Just look at the gpio device that has ttyUSB0 as its parent.

> and how is udev going to do that to manage permissions ?

How does udev handle permissions for gpio devices today?

> So you need an ioctl to give you the range that is mapped to this (or a
> sysfs node, but the sysfs node makes the security problem pretty
> much insoluble)

range for what?  Doesn't the gpio interfac provide the size of the gpio
registers to userspace?

As for security, how is that handled today with gpio devices?

> > > and having done that youi've got potential races with unload
> > > when you try and open them. You've also got permissions considerations
> > > and synchronization between gpio and data problems.
> > 
> > That can be handled in the driver itself, if it really is a problem (the
> > existing patch sure didn't handle any of that at all, so I'm guessing
> > either it wasn't considered, or it isn't a problem.)
> 
> Not reliably
> 
> 	open /dev/ttyUSB0 [or sysfs node]
> 	read gpio numbers
> 	close 
> 
> 	open gpio foo
> 
> Oh dear... so random shell scripting user is going to screw up horribly.

What's the odds that the data going across the tty link corrisponds with
the gpio control?

And shell scripting the gpio interface is used today, I've seen it on
the beaglebone machine.

> > > It's not a good way to go. It might make sense in some platforms to
> > > expose them as both but its not a good general model.
> > 
> > Why isn't the gpio subsystem a good general model?  I thought that is
> > what it was created to solve?
> 
> gpio provides a flat abstraction for arbitary pins on a platform. It's
> really oriented to fairly fixed system stuff. It doesn't provide a useful
> abstraction for extra carrier lines.
> 
> Imagine if RTS and DTR were driven by GPIO pins instead of the tty
> layer. Many devices use them as extra magic GPIO lines not as tty control
> lines, so its an equivalent argument.

Ok, fair enough, but RTS and DTR are "well defined" as part of the RS232
specs, and handle flow of the data itself.  These gpio lines are not tty
control, but as you point out, used for other things, so keeping them
separate from the tty node seems to make sense to me.

> > > I'm currently favouring adding some 'additional control line' bits to
> > > termiox.
> > 
> > Yes, but that's only good for usb-serial devices that also have gpio
> > pins on the controller side.  Which seems pretty limited to me.
> > 
> > If I have a userspace program, and I want to use GPIO, I shouldn't have
> > to care/know that the pins are really on the end of a USB->serial
> > bridge, or somewhere hanging off of a SOC, the same userspace api should
> > "just work", right?
> 
> For certain applications that makes more sense, which is why I said you
> may well want to expose it as both. However you still need both, and
> ideally we need a standardised pattern of assignments for line
> disciplines that use the extra control lines (which has come up recently
> for another device with extra lines)
> 
> I think it basically boils down to this
> 
> If you have a serial port with some gpio lines that drive arbitary
> unrelated electronics then the gpio interface is handy because you can
> use the same code logic as if it was wired to other pins elsewhere
> 
> If you are using them as part of the tty interface as extra control lines
> (eg for smartcard protocols) then you want them driven via the tty
> interface and doubly so once we add some of the smartcard/sim ldisc
> support.
> 
> Hence we really need to expose them both ways because end users are doing
> both things with the gpio pins on these ports.
> 
> So I'd suggest we expose them via termiox bits and also via the tty
> providing gpio range info in a standardised way.

Ok, I'll wait for your proposed standardised way before complaining any
more :)

thanks,

greg k-h

Re: [PATCH 3/3] usb: cp210x: Add ioctl for GPIO support

[Alan Cox]

> > Ok now in your user application how are you going to find which gpio
> > numbers to use that are associated with this specific port,
> 
> Just look at the gpio device that has ttyUSB0 as its parent.

Taking care of course that you keep the ttyUSB file handle open while you
do so, ugly from user space, hideous from kernel space.

> 
> > and how is udev going to do that to manage permissions ?
> 
> How does udev handle permissions for gpio devices today?

It sticks its fingers in its ears and goes "la la la"

> > So you need an ioctl to give you the range that is mapped to this (or a
> > sysfs node, but the sysfs node makes the security problem pretty
> > much insoluble)
> 
> range for what?  Doesn't the gpio interfac provide the size of the gpio
> registers to userspace?

If you fish them out via sysfs trees yes.

>> > 	open /dev/ttyUSB0 [or sysfs node]
> > 	read gpio numbers
> > 	close 
> > 
> > 	open gpio foo
> > 
> > Oh dear... so random shell scripting user is going to screw up horribly.
> 
> What's the odds that the data going across the tty link corrisponds with
> the gpio control?

I think you missed the problem - which speaks volumes for the interface
issue

	open sysfs node
	read gpio numbers
	close

			unplug 
			new device
				assigned the same gpio numbers
	open gpio
	whoops - that was the milling machine not the smartcard

> And shell scripting the gpio interface is used today, I've seen it on
> the beaglebone machine.

Yes it works very well but that isn't the issue.

> > I think it basically boils down to this
> > 
> > If you have a serial port with some gpio lines that drive arbitary
> > unrelated electronics then the gpio interface is handy because you can
> > use the same code logic as if it was wired to other pins elsewhere
> > 
> > If you are using them as part of the tty interface as extra control lines
> > (eg for smartcard protocols) then you want them driven via the tty
> > interface and doubly so once we add some of the smartcard/sim ldisc
> > support.
> > 
> > Hence we really need to expose them both ways because end users are doing
> > both things with the gpio pins on these ports.
> > 
> > So I'd suggest we expose them via termiox bits and also via the tty
> > providing gpio range info in a standardised way.
> 
> Ok, I'll wait for your proposed standardised way before complaining any
> more :)

I think I'd suggest we support the following


open /dev/ttyUSB0
get gpio info somehow
run via gpio interface
close /dev/ttyUSB0

and

open /dev/ttyUSB0
via termiox( what gpios do you have )
via termiox( set/get gpio values)
close /dev/ttyUSB0

that would support the ldisc use of them. I need to go read the specs on
that and look at some hardware. I think we may need a mapping ioctl too
because the pin allocations may need to be described in terms of "pin 0
is this reader signal", "pin 1 is that"

Alan

Re: [PATCH 1/3] usb: cp210x: Corrected USB request type definitions

[Bjørn Mork]

Preston Fick <pffick@gmail.com> writes:

> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index ec30f95..e67ccf3 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -188,8 +188,10 @@ static struct usb_serial_driver * const serial_drivers[] = {
>  };
>  
>  /* 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


Any particular reason you need to define these instead of just using the
standard flags from linux/usb/ch9.h directly in the requests?:

(USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_OUT)
(USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_IN)
(USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT)
(USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN)

If nothing else, using those from the beginning would have avoided the
mis-labelling you are fixing up.



Bjørn

RE: [PATCH 1/3] usb: cp210x: Corrected USB request type definitions

[Preston Fick]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2632 bytes --]

Hi Bjorn -

I agree - I was not the original author of this driver, but am helping to bring it up to date to fix some issues and add missing support from our product line. I just simply added this in to stick with the way that it had already been developed, however I can submit another patch to setup those defines using the standard USB definitions. Thanks for the suggestion.

Kind Regards -
Preston

-----Original Message-----
From: Bjørn Mork [mailto:bjorn@mork.no] 
Sent: Thursday, May 03, 2012 3:59 AM
To: Preston Fick
Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; linux-serial@vger.kernel.org; Preston Fick
Subject: Re: [PATCH 1/3] usb: cp210x: Corrected USB request type definitions

Preston Fick <pffick@gmail.com> writes:

> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index ec30f95..e67ccf3 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -188,8 +188,10 @@ static struct usb_serial_driver * const serial_drivers[] = {
>  };
>  
>  /* 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


Any particular reason you need to define these instead of just using the
standard flags from linux/usb/ch9.h directly in the requests?:

(USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_OUT)
(USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_IN)
(USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT)
(USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN)

If nothing else, using those from the beginning would have avoided the
mis-labelling you are fixing up.



Bjørn

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.

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 1/3] usb: cp210x: Corrected USB request type definitions

[Preston Fick]

Hi Bjorn -

I agree - I was not the original author of this driver, but am helping to bring it up to date to fix some issues and add missing support from our product line. I just simply added this in to stick with the way that it had already been developed, however I can submit another patch to setup those defines using the standard USB definitions. Thanks for the suggestion.

Kind Regards -
Preston

-----Original Message-----
From: Bjørn Mork [mailto:bjorn@mork.no] 
Sent: Thursday, May 03, 2012 3:59 AM
To: Preston Fick
Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; linux-serial@vger.kernel.org; Preston Fick
Subject: Re: [PATCH 1/3] usb: cp210x: Corrected USB request type definitions

Preston Fick <pffick@gmail.com> writes:

> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index ec30f95..e67ccf3 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -188,8 +188,10 @@ static struct usb_serial_driver * const serial_drivers[] = {
>  };
>  
>  /* 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


Any particular reason you need to define these instead of just using the
standard flags from linux/usb/ch9.h directly in the requests?:

(USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_OUT)
(USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_IN)
(USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT)
(USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN)

If nothing else, using those from the beginning would have avoided the
mis-labelling you are fixing up.



Bjørn

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.