On Fri, May 14, 2010 at 10:20:17AM -0600, Steve McKown wrote:
> 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.
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.
A few comments on the changes.
> @@ -11,6 +11,9 @@
> * thanks to Karl Hiramoto karl@xxxxxxxxxxxxx RTSCTS hardware flow
> * control thanks to Munir Nassar nassarmu@xxxxxxxxxxxxx
> *
> + * R. Steve McKown, Titanium Mirror, Inc., rsmckown@xxxxxxxxxx
> + * Added port configuration, usb descriptor and gpio management.
> + *
This isn't needed, it will show up in the git changelog instead. Please
include it in your changelog comments.
> @@ -221,6 +227,339 @@
> #define CONTROL_WRITE_DTR 0x0100
> #define CONTROL_WRITE_RTS 0x0200
>
> +/* CP2103 ioctls */
> +#define IOCTL_GPIOGET 0x8000 /* Get gpio bits */
> +#define IOCTL_GPIOSET 0x8001 /* Set gpio bits */
> +#define IOCTL_GPIOBIC 0x8002 /* Clear specific gpio bit(s) */
> +#define IOCTL_GPIOBIS 0x8003 /* Set specific gpio bit(s) */
Have you thought about using the standard gpio interface instead of
custom ioctls? That would play much nicer with the rest of the kernel
and any userspace tools that are used to accessing gpio ports.
> +/* CP210x ioctls principally used during initial device configuration */
> +#define IOCTL_DEVICERESET 0x8004 /* Reset the cp210x */
> +#define IOCTL_PORTCONFGET 0x8005 /* Get port configuration */
> +#define IOCTL_PORTCONFSET 0x8006 /* Set port configuration */
> +#define IOCTL_SETVID 0x8007 /* Set vendor id */
> +#define IOCTL_SETPID 0x8008 /* Set product id */
> +#define IOCTL_SETMFG 0x8009 /* Set manufacturer string */
> +#define IOCTL_SETPRODUCT 0x800a /* Set product string */
> +#define IOCTL_SETSERIAL 0x800b /* Set serial number string */
> +#define IOCTL_SETDEVVER 0x800c /* set device version id */
Shouldn't these all be in a .h file somewhere?
> +/* FIXME: where is IOCTL_SETMFG? */
What is this message?
> +/* CP2103 GPIO */
> +#define GPIO_0 0x01
> +#define GPIO_1 0x02
> +#define GPIO_2 0x04
> +#define GPIO_3 0x08
> +#define GPIO_MASK (GPIO_0|GPIO_1|GPIO_2|GPIO_3)
> +
> +/* GetDeviceVersion() return codes */
> +#define CP210x_CP2101_VERSION 0x01
> +#define CP210x_CP2102_VERSION 0x02
> +#define CP210x_CP2103_VERSION 0x03
Why is this needed? Can't we just expose the version in a sysfs file
instead of using an ioctl?
> +
> +/* Return codes */
> +#define CP210x_SUCCESS 0x00
> +#define CP210x_DEVICE_NOT_FOUND 0xFF
This isn't ever used, and doesn't make sense :)
> +#define CP210x_INVALID_HANDLE 0x01
> +#define CP210x_INVALID_PARAMETER 0x02
These aren't used.
> +#define CP210x_DEVICE_IO_FAILED 0x03
> +#define CP210x_FUNCTION_NOT_SUPPORTED 0x04
This one isn't ever used.
> +#define CP210x_GLOBAL_DATA_ERROR 0x05
> +#define CP210x_FILE_ERROR 0x06
> +#define CP210x_COMMAND_FAILED 0x08
> +#define CP210x_INVALID_ACCESS_TYPE 0x09
Nor are these four.
> +/* USB descriptor sizes */
> +#define CP210x_MAX_DEVICE_STRLEN 256
This is never used.
> +#define CP210x_MAX_PRODUCT_STRLEN 126
> +#define CP210x_MAX_SERIAL_STRLEN 63
> +#define CP210x_MAX_MAXPOWER 250
And this one isn't used either.
> +
> +/* Used to pass variable sized buffers between user and kernel space (ioctls)
> */
> +typedef struct {
> + char *buf;
> + size_t len;
> +} cp210x_buffer_t;
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
submitting a kernel patch.
> +
> +/* Port config definitions */
> +typedef struct {
> + uint16_t mode; /* Push-pull = 1, Open-drain = 0 */
> + uint16_t lowPower;
> + uint16_t latch; /* Logic high = 1, Logic low = 0 */
> +} cp210x_port_state_t;
> +
> +typedef struct {
> + cp210x_port_state_t reset;
> + cp210x_port_state_t suspend;
> + uint8_t enhancedFxn;
> +} cp210x_port_config_t;
Same thing here for typedefs and variable types.
> +
> +#define PORT_CONFIG_LEN 13 /* Because sizeof() will pad to full words */
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
"pack" your structure then, right?
> +/*
> + * cp210x_buf_from_user
> + * Copy a buffer from user space, returning the number of bytes copied
> + * from ubuf.buf into kbuf. klen is the size of the buffer at kbuf.
> + */
> +static size_t copy_buf_from_user(char *kbuf, unsigned long ubuf, size_t klen)
> +{
> + cp210x_buffer_t t;
> +
> + if (!kbuf || !ubuf || !klen ||
> + copy_from_user(&t, (cp210x_buffer_t *)ubuf, sizeof(t)))
> + return 0;
> + if (t.len < klen)
> + klen = t.len;
> + if (!t.buf || !t.len || copy_from_user(kbuf, t.buf, klen))
> + return 0;
> + return klen;
> +}
Please run the 'sparse' tool on this code and fix all of the warnings it
will spit out here.
> +/*
> + * cp210x_ctlmsg
> + * A generic usb control message interface.
> + * Returns the actual size of the data read or written within the message, 0
> + * if no data were read or written, or a negative value to indicate an error.
> + */
> +static int cp210x_ctlmsg(struct usb_serial_port *port, u8 request,
> + u8 requestype, u16 value, u16 index, void *data, u16 size)
> +{
> + struct usb_device *dev = port->serial->dev;
> + u8 *tbuf;
> + int ret;
> +
> + if (!(tbuf = kmalloc(size, GFP_KERNEL)))
> + return -ENOMEM;
> + if (requestype & 0x80) {
Can you use the proper usb.h define here to make it a bit more readable?
> + ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0), request,
> + requestype, value, index, tbuf, size, 300);
> + if (ret > 0 && size)
> + memcpy(data, tbuf, size);
> + } else {
> + if (size)
> + memcpy(tbuf, data, size);
> + ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), request,
> + requestype, value, index, tbuf, size, 300);
> + }
> + kfree(tbuf);
> + if (ret < 0 && ret != -EPIPE) {
> + dev_printk(KERN_DEBUG, &dev->dev, "cp210x: control failed cmd rqt %u "
> + "rq %u len %u ret %d\n", requestype, request, size, ret);
> + }
> + return ret;
> +}
> +
> +static int cp210x_reset(struct usb_serial_port *port)
> +{
> + dbg("%s", __FUNCTION__);
> +
> +#if 1
> + /* Is this better than usb_device_reset? It may be. Once a client
> issues
Your email client wrapped the patch :(
> + * the reset ioctl, it must disconnect and reconnect, since the USB
> + * connections are torn down. We also ignore the error return, since
> + * the part resets and doesn't send one...
> + */
> + cp210x_ctlmsg(port, 0xff, 0x40, 0x0008, 0x00, 0, 0);
> +#else
> + usb_reset_device(port->serial->dev);
> +#endif
> + return 0;
Care to fix this up before submitting it?
That's enough to get started with :)
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
