On Mon, Aug 02, 2004 at 01:43:41PM +0200, Florian Echtler wrote:
> --- linux-2.6.7/drivers/usb/misc/idmouse.c 1970-01-01 01:00:00.000000000 +0100
> +++ linux/drivers/usb/misc/idmouse.c 2004-07-30 14:26:15.000000000 +0200
> @@ -0,0 +1,457 @@
> +// Siemens ID Mouse driver v0.3
> +// authors: Florian 'Floe' Echtler <echtler ?T fs.tum.de>
> +// Andreas 'ad' Deresch <aderesch ?T fs.tum.de>
Please don't use "//" for comments. Also, no copyright or license info
for this file?
> +// we don't like those stinkin' debug macros
> +#undef dbg
> +#undef err
> +#undef info
Why not? Yeah, they are a bit verbose, but they are on my list of
things to fix up one of these days... You should use dev_info, dev_dbg,
and dev_err instead if you don't like the usb ones.
> +// we still need a minor number
> +#define USB_IDMOUSE_MINOR_BASE 0xFA
I'll give you one when I add it to the main kernel tree. How many
minors do you need? Is 16 enough?
> +// file operation pointers
> +static struct file_operations idmouse_fops = {
> + .owner = THIS_MODULE,
> + .read = idmouse_read,
> + .write = NULL,
> + .ioctl = NULL,
> + .open = idmouse_open,
> + .release = idmouse_release,
> +};
Just leave the entries that you set to NULL alone (don't even define
them here.)
> + // should be IMGSIZE == 64815
> + info("read %d bytes fingerprint data", bytes_read);
> + return 0;
Shouldn't this be a debug message?
> + // See if the device offered us matches what we can accept
> + if ((udev->descriptor.idVendor != USB_IDMOUSE_VENDOR_ID) ||
> + (udev->descriptor.idProduct != USB_IDMOUSE_PRODUCT_ID))
> + return -ENODEV;
You don't need to do this check, the USB core will have done it for you.
> + // check if we have gotten the data or the hid interface
> + iface_desc = &interface->altsetting[0];
> + if (iface_desc->desc.bInterfaceClass != 0x0A)
> + return -ENODEV;
> +
> + // allocate memory for our device state and initialize it
> + dev = kmalloc(sizeof(struct usb_idmouse), GFP_KERNEL);
How about:
dev = kmalloc(sizeof(*dev), GFP_KERNEL);
instead?
> + // let the user know what node this device is now attached to
> + info("%s connected to USB minor %d", DRIVER_DESC, dev->minor);
Use dev_info(), it makes more sense here.
> + info("%s disconnected", DRIVER_DESC);
Again with the dev_info().
Oh, and all of the other things that everyone else has suggested are
things that need to be fixed up too.
thanks,
greg k-h
-------------------------------------------------------
This SF.Net email is sponsored by OSTG. Have you noticed the changes on
Linux.com, ITManagersJournal and NewsForge in the past few weeks? Now,
one more big change to announce. We are now OSTG- Open Source Technology
Group. Come see the changes on the new OSTG site. www.ostg.com
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
