Am Freitag, 5. Januar 2007 19:10 schrieb Greg KH:
> On Fri, Jan 05, 2007 at 02:39:21PM +0100, Oliver Neukum wrote:
> > +/* --- prototypes --- */
> > +
> > +static int __init wdm_init(void);
> > +static void __exit wdm_exit(void);
> > +
> > +static int wdm_probe(struct usb_interface *intf, const struct
> > usb_device_id *id);
> > +static void wdm_disconnect(struct usb_interface *intf);
> > +
> > +static ssize_t wdm_read(struct file *file, char __user *buffer, size_t
> > count, loff_t *ppos);
> > +static ssize_t wdm_write(struct file *file, const char __user *buffer,
> > size_t count, loff_t *ppos);
> > +static int wdm_release(struct inode *inode, struct file *file);
> > +static int wdm_open(struct inode *inode, struct file *file);
> > +static int wdm_flush (struct file * file, fl_owner_t id);
> > +
> > +static void wdm_out_callback (struct urb *urb);
> > +static void wdm_in_callback (struct urb *urb);
> > +
> > +static void free_urbs(struct wdm_device *desc);
> > +static void cleanup (struct wdm_device *desc);
> > +static void kill_urbs (struct wdm_device *desc);
> > +static int run_poll(struct wdm_device *desc, gfp_t gfp);
>
> Are all of these prototypes really needed?
Yes. Compilation breaks without them.
> > +/* --- table of supported interfaces ---*/
> > +
> > +static struct usb_device_id wdm_ids[] = {
> > + {
> > + .match_flags = USB_DEVICE_ID_MATCH_INT_CLASS |
> > USB_DEVICE_ID_MATCH_INT_SUBCLASS,
> > + .bInterfaceClass = USB_CLASS_COMM,
> > + .bInterfaceSubClass = USB_CDC_SUBCLASS_DMM
> > + },
> > + { }
> > +};
> > +
> > +MODULE_DEVICE_TABLE (usb, wdm_ids);
> > +
> > +#define WDM_MINOR_BASE 32
>
> Where did you get this minor number from? I think it conflicts with a
> very old usb driver, but you still need to reserve a real number from me
> :)
I needed one for experimentation. This driver is not yet ready.
But if you feel like assigning a number, I'll take it.
> Oh, and this will not work with the dynamic minors for usb devices, you
> need to fix that up.
Roger.
> > --- /dev/null 2005-03-19 23:01:40.000000000 +0100
> > +++ drivers/usb/class/cdc-wdm.c 2007-01-05 12:52:33.000000000 +0100
> > @@ -0,0 +1,576 @@
> > +/* cdc-wdm.c
> > +
> > +This driver supports USB CDC WCM Device Management.
> > +
> > +Copyright (c) 2007 Oliver Neukum
>
> Need an email and a license here.
Roger.
> > +
> > +Some code taken from cdc-acm.c
> > +*/
> > +#include <linux/kernel.h>
> > +#include <linux/errno.h>
> > +#include <linux/slab.h>
> > +#include <linux/module.h>
> > +#include <linux/smp_lock.h>
> > +#include <linux/mutex.h>
> > +#include <asm/uaccess.h>
> > +#include <linux/usb.h>
> > +#include <linux/usb/cdc.h>
> > +#include <asm/byteorder.h>
> > +#include <asm/bitops.h>
> > +#include <asm/unaligned.h>
> > +
> > +#include "cdc-wdm.h"
>
> Do we really need a separate .h file for this driver?
Don't you like header files? They make things cleaner.
> > +#include "cdc-acm.h" /* for request types */
> > +
> > +#define DEBUG
>
> This should not be needed due to the CONFIG_USB_DEBUG config option.
Roger.
> > +static ssize_t wdm_write(struct file *file, const char __user *buffer,
> > size_t count, loff_t *ppos)
> > +{
> > + u8 *buf;
> > + int rv = -EMSGSIZE, r, we;
> > + unsigned long flags;
> > + struct wdm_device *desc = file->private_data;
> > + struct usb_ctrlrequest *req;
> > +
> > + if (count > desc->wMaxCommand)
> > + count = desc->wMaxCommand;
> > +
> > + spin_lock_irqsave(&desc->iuspin, flags);
> > + we = desc->werr;
> > + desc->werr = 0;
> > + spin_unlock_irqrestore(&desc->iuspin, flags);
> > + if (we < 0)
> > + return -EIO;
> > +
> > + run_poll(desc, GFP_KERNEL); /* read responses right on so that the
> > output buffer not stall */
> > + r = mutex_lock_interruptible(&desc->wlock); /* concurrent writes */
> > + rv = -ERESTARTSYS;
> > + if (r) {
> > + goto outnl;
> > + }
>
> Minor nit, no { } needed for one line if statements. You do this in a
> few other places.
Yes, leftover from versions with debugging instructions. They'll be removed.
[..]
> > + rv = wait_event_interruptible(desc->wait, test_bit(WDM_READ,
> > &desc->flags));
> > +
>
> Trailing whitespace :(
Roger.
> > +static void cleanup (struct wdm_device *desc)
> > +{
> > + //usb_buffer_free(interface_to_usbdev(desc->intf), sizeof(struct
> > usb_cdc_notification), desc->sbuf, desc->shandle);
> > + //usb_buffer_free(interface_to_usbdev(desc->intf), desc->wMaxCommand,
> > desc->inbuf, desc->ihandle);
> > +kfree(desc->sbuf);
> > +kfree(desc->inbuf);
> > + kfree(desc->orq);
> > + kfree(desc->irq);
> > + kfree(desc->ubuf);
> > + free_urbs(desc);
> > + kfree(desc);
> > +}
>
> Odd indentation...
Roger.
Thanks
Oliver
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
