Hello,

A couple of years ago, Silicon Labs granted my company access to API 
documentation for their cp210x "runtime" and "manufacturing" DLLs (provided 
we release our work via the GPL, which we did).  The former allows USB host 
manipulation of cp210c GPIO pins; the latter allows setting cp210x port 
configuration and USB descriptors.  With this information I was able to add 
support for these features in the cp210x driver, exposed via ioctl.  We use 
this driver regularly with good results.

SiLabs wanted to push the changes to the Linux community itself, but doesn't 
seem to have ever done so.  I'd like to make them available now.  However, 
I'm no kernel developer and have only passing familiarity with the relevant 
kernel interfaces.  I'm hoping that someone might be willing to 'mentor' me 
through the process of getting this code suitably crafted and organized for 
possible inclusion.

The patch for our driver is included below, relative to cp210x from Linus' 
tree @ v2.6.34-rc7.

All the best,
Steve

--- cp210x.c-2.6.34-rc7	2010-05-13 16:48:48.000000000 -0600
+++ cp210x.c.tmi	2010-05-13 17:36:10.000000000 -0600
@@ -11,6 +11,9 @@
  * thanks to Karl Hiramoto karl@hiramoto.org. RTSCTS hardware flow
  * control thanks to Munir Nassar nassarmu@real-time.com
  *
+ * R. Steve McKown, Titanium Mirror, Inc., rsmckown@gmail.com.
+ * Added port configuration, usb descriptor and gpio management.
+ *
  */
 
 #include <linux/kernel.h>
@@ -50,6 +53,8 @@
 static void cp210x_break_ctl(struct tty_struct *, int);
 static int cp210x_startup(struct usb_serial *);
 static void cp210x_disconnect(struct usb_serial *);
+static int cp210x_ioctl(struct tty_struct *, struct file *,
+		unsigned int, unsigned long);
 static void cp210x_dtr_rts(struct usb_serial_port *p, int on);
 static int cp210x_carrier_raised(struct usb_serial_port *p);
 
@@ -140,6 +145,7 @@
 	.num_ports		= 1,
 	.open			= cp210x_open,
 	.close			= cp210x_close,
+	.ioctl			= cp210x_ioctl,
 	.break_ctl		= ...
[ message continues ]

Thanks for the patch, but could you take a look at the file,
Documentation/SubmittingPatches and resend the patch in a format that I
could apply it in?  We also need a Signed-off-by: line to do so.


This isn't needed, it will show up in the git changelog instead.  Please

Have you thought about using the standard gpio interface instead of
custom ioctls?  That would play much nicer with the rest of the kernel



Why is this needed?  Can't we just expose the version in a sysfs file







If this really is crossing the user/kernel boundry, please use the
variable types that are safe to do so.  They all start with '__'.  For
this one, I would suggest:
	__u8 *buf;
	__s32 len;

Also, please do not define new typedefs.  Running your code through the
checkpatch.pl tool will point this out.  That's always good to do before


What do you mean here?  sizeof will take the "real" size.  If you handle
your structures correctly, you can use sizeof().  You might need to

Please run the 'sparse' tool on this code and fix all of the warnings it



Care to fix this up before submitting it?

That's enough to get started with :)

thanks,

greg k-h
--

[ message continues ]

Hi Greg,

I believe I've addressed your earlier points except for kernel gpio, and 
part number visibility via sysfs.  I'll try to add the former in the 
coming weeks.  The patch passes checkpatch.pl and the code shows no 
errors from 'sparse'.  I hope you are willing to continue the mentorship 
for a few more rounds.  What additional changes are needed?  Thanks again 
for taking the extra time with me on this.


As of a year or so ago, CP210X internal firmware didn't offer the ability 
to change the USB Manufacturer descriptor.  SiLabs said they might change 
this.  See cp210x_has_setmfg().  Perhaps I should yank all the mfg set 

cp210x_get_partnum() is used internally only to interact with the cp210x 
according to what part it actually is.  For example, only cp2103 supports 
GPIO.  Not sure how useful it is to export this information to userspace.

And now for the patch proper
============================

Add GPIO, port configuration and USB descriptor management
via IOCTL to cp210x.

Signed-off-by: R. Steve McKown <rsmckown@gmail.com>
---

This patch is based on linus' v2.6.34-rc7.

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index ec9b044..3086ab6 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -23,6 +23,7 @@
 #include <linux/usb.h>
 #include <linux/uaccess.h>
 #include <linux/usb/serial.h>
+#include "cp210x.h"
 
 /*
  * Version Information
@@ -50,6 +51,8 @@ static int cp210x_tiocmset_port(struct usb_serial_port *port, struct file *,
 static void cp210x_break_ctl(struct tty_struct *, int);
 static int cp210x_startup(struct usb_serial *);
 static void cp210x_disconnect(struct usb_serial *);
+static int cp210x_ioctl(struct tty_struct *, struct file *,
+		unsigned int, unsigned long);
 static void cp210x_dtr_rts(struct usb_serial_port *p, int on);
 static int cp210x_carrier_raised(struct usb_serial_port *p);
 
@@ -140,6 +143,7 @@ static struct usb_serial_driver cp210x_device = {
 	.num_ports		= 1,
 ...
[ message continues ]

Internally is fine, but I don't think we need an ioctl for something

Don't redefine an existing, well-known, define.  That forces people to
try to have to dig back and figure out what is going on here.  What's



Helper based on what?  16bit character string length?  Again, just spell

Is this something that we need/want in the usb core?  Are you sure we




No, you need to fail this properly with -EFAULT somehow back to

Could you just have overflowed that t.len check?  Be _very_ careful

t.buf should already be a __u8 __user *, right?  Don't use the same
structure types in userspace and in the kernel.

And how does this handle a 64bit kernel in a 32bit userspace?  Have you
hooked up the ioctl compatibility layer properly?



Wait, this isn't the standard Linux GPIO layer.  What happened to using
that instead of a custom ioctl?  That is the correct way to do this,
don't create custom messages here please, it makes support much more
difficult.

Just hook up these GPIO pins to the already-in-kernel gpio layer and
that will export them to userspace properly and you will not have to add
any new ioctls.

thanks,

greg k-h
--

[ message continues ]

Would following the guidance of ftdi_sio be a better approach?

/* FTDI_SIO_RESET */
#define FTDI_SIO_RESET_REQUEST FTDI_SIO_RESET
#define FTDI_SIO_RESET_REQUEST_TYPE 0x40

then

        rv = usb_control_msg(port->serial->dev,
                               usb_sndctrlpipe(port->serial->dev, 0),
                               FTDI_SIO_SET_MODEM_CTRL_REQUEST,
                               FTDI_SIO_SET_MODEM_CTRL_REQUEST_TYPE,
                               urb_value, priv->interface,

I can fix the unhelpful comment.

Determining bytes needed to store a string properly encoded to send to the 
cp210x happens in several places through the code.  I thought it would be 
more correct to encapsulate the logic behind this mapping in a single 


OK, I'll try to get up to speed on unicode and figure out something better 

You're right.  This is a static function only called from the local compile 

Yes.  The snippet above is in cp210x_usbstr_from_user.  All callers are in 
cp210x_ioctl and must see a non-zero return from cp210x_usbstr_from_user else 

Do you mean that t.len*2+2, which is what the USBSTRLEN(t.len) resolves to, 
might wrap in its size_t?  I can solve this by moving the assigment of slen 


I'm not at all familiar with "ioctl compatibility layer" and will do some 


It's already on my todo list; I just haven't got to it yet.

I'll get to work on your latest suggestions. 
Thank you for your continued help.
Steve
--

[ message continues ]

No, please just spell out the "real" usb flags here, don't bury them

I agree it is good to put it in a consistant place, but you are using
"strlen" which kind of implies that this is a real function that will

No, sorry, it was more a joke.  USB strings were always supposed to be a
"real" 16bit value per character, but yet, it is very rare that anyone
ever used that other 8 bits.  You are doing the same thing here, never

Ok, but how are you detecting the two different error modes (invalid
parameters and bad copy)?

As I think you will be getting rid of your ioctl call, this isn't going

Yes.  What happens if I pass a "negative" or very big value as len here?


Ok, that's the big change that I would like to see happen.  I don't want
to add new ioctls to the kernel if at all possible, especially one-off
ones like this that only work for one specific driver/device.

thanks,

greg k-h
--

[ message continues ]

A few ioctls remain in the patch even after porting to kernel gpio:

- Get and set port configuration.  Defines the configuration of the
	internal port pins on the cp210x.  This includes things like
	pull-ups, pin function, reset state, etc.
- Set VID, PID and descriptors: product, serial, and device version.

We use these ioctls to properly configure the cp210x embedded in several of 
our USB device designs.  Currently, the patch includes cp210x.h which can be 
included from userspace too, so that the ioctl commands, starting at 
SIOCDEVPRIVATE, can be known without defining new ioctls globally.  Is this 
enough isolation to consider their inclusion?  If no, what other methods 
could be considered?

Thanks,
Steve
--

[ message continues ]

You can dynamically change the vid and pid and stuff?  Heh, that's going
to be fun to track :)


configfs was created for configuring things, perhaps you might want to
look into using that?

Or, if they are single value things, like the vid/pid and such, then a
writable sysfs file also would work well, and be much simpler than an
ioctl for userspace to control.

thanks,

greg k-h
--

[ message continues ]