On Fri, Dec 01, 2006 at 01:37:22PM -0700, David Lopez wrote:
> From: David Lopez <dave.l.lopez@xxxxxxxxx>
The patch seems linewrapped, which doesn't make it easy to apply :(
Can you resend this?
> +#define BULK_IN_TIMEOUT 1000 /* default bulk in
> read timeout */
What units is this timeout in?
> +static void ljusb_write_bulk_callback(struct urb *urb)
> +{
> + struct usb_ljusb *dev;
> +
> + dev = (struct usb_ljusb *)urb->context;
> +
> + /* sync/async unlink faults aren't errors */
> + if (urb->status &&
> + !(urb->status == -ENOENT ||
> + urb->status == -ECONNRESET ||
> + urb->status == -ESHUTDOWN)) {
> + dbg("%s - nonzero write bulk status received: %d",
> + __FUNCTION__, urb->status);
> + }
A switch statement might work a bit better here. It will let you handle
the different values you might get in a saner way.
> +/**
> + * ljusb_ioctl
> + */
> +static int ljusb_ioctl (struct inode *inode, struct file *file,
> unsigned int cmd, unsigned long arg)
New ioctls are pretty much frowned apon to add. Do you _really_ need
these?
Can you use sysfs instead?
> + /* driver specific commands */
> + switch (cmd) {
> + /* Sets the timeout for usb_bulk_msg reads transfers in ms
> from an integer
> + * argument. If the timeout is set to zero, reads will wait
> forever */
> + case IOCTL_LJ_SET_BULK_IN_TIMEOUT:
> + data = (void __user *) arg;
> + if (data == NULL)
> + break;
> +
> + if (copy_from_user(&timeout, data, sizeof(int))) {
> + retval = -EFAULT;
> + break;
> + }
> +
> + if(timeout < 0)
> + retval = -EINVAL;
> + else
> + dev->bulk_in_timeout = timeout;
> +
> + break;
Is this really needed to be modified?
> + /* Gets the Product ID for the device */
> + case IOCTL_LJ_GET_PRODUCT_ID:
> + retval = put_user(dev->udev->descriptor.idProduct,
> + (unsigned int __user *)arg);
> + break;
You can get this from sysfs or usbfs today. Don't duplicate it please.
> + /* Sets the bulk in endpoint for the next read from an
> integer argument.
> + * There are two bulk endpoints, which are endpoints 0 and 1
> when
> + * setting the integer argument. */
> + case IOCTL_LJ_SET_BULK_IN_ENDPOINT:
> + data = (void __user *) arg;
> + if (data == NULL)
> + break;
> +
> + if (copy_from_user(&ep, data, sizeof(int))) {
> + retval = -EFAULT;
> + break;
> + }
> +
> + if(ep > N_BULK_IN_ENDPOINTS || ep < 0)
> + retval = -EINVAL;
> + else
> + dev->next_bulk_in_endpoint = ep;
> + break;
Why is this needed?