On Tue, Jan 18, 2005 at 05:13:08PM +0100, Thomas Winischhofer wrote:
> 1) A minor device number has to be allocated and #defined in sisusb.h. 
> Right now, 64 is being used (see "SISUSB_MINOR").

You know that's already a valid reserved minor number, right?  That's 
not very nice :)


> diff -Nru linux-2.6.10/drivers/usb/Makefile 
> linux-2.6.10-new/drivers/usb/Makefile
> --- linux-2.6.10/drivers/usb/Makefile 2004-12-24 22:35:28.000000000 +0100
> +++ linux-2.6.10-new/drivers/usb/Makefile     2005-01-18 16:08:26.000000000 
> +0100
> @@ -69,5 +69,7 @@
>  obj-$(CONFIG_USB_USS720)     += misc/
>  obj-$(CONFIG_USB_PHIDGETSERVO)       += misc/
>  
> +obj-$(CONFIG_USB_SISUSBVGA)     += misc/sisusbvga/

You forgot the tab.

Anyway, why not put this into the drivers/usb/misc/Makefile instead?
Would be more consistant that way.

> + *
> + * * This program is free software; you can redistribute it and/or modify
> + * * it under the terms of the GNU General Public License as published by
> + * * the Free Software Foundation; either version 2 of the named License,
> + * * or any later version.
> + * *
> + * * This program is distributed in the hope that it will be useful,

<snip> You don't need the whole gpl copy here, it's not needed, right?

> +
> +#define DPRINTK(s, ...)

I think you should use the dev_dbg() macro instead, it will give you
better error reporting as you can pinpoint the exact device you are
havving issues with.

> +
> +#define SISUSB_DONTSYNC      

Trailing whitespace :(

And what's this for?  Why have a #define that will not be changed?

> +     sisusb->sisusb_dev = dev;
> +     sisusb->minor      = intf->minor;
> +     sisusb->fminor     = intf->minor - usb_sisusb_class.minor_base;

This will not work with CONFIG_USB_DYNAMIC_MINORS enabled, right?

> +
> +struct sisusb_urb_context {          /* urb->context for outbound bulk URBs 
> */
> +     void *sisusb;

Why a void pointer?  Don't you know the type here?

> +struct sisusb_usb_data {
> +        struct usb_device *sisusb_dev;  

Mixing tabs and spaces :(  Also trailing whitespace.


> +/* Structure argument for SISUSB_GET_INFO ioctl  */
> +typedef struct _SISUSB_INFO sisusb_info, *psisusb_info;

No typedefs please.


> +struct _SISUSB_INFO {
> +     __u32   sisusb_id;              /* for identifying sisusb */
> +#ifndef SISUSB_ID
> +#define SISUSB_ID  0x53495355        /* Identify myself with 'SISU' */
> +#endif

What is this for?

thanks,

greg k-h


-------------------------------------------------------
The SF.Net email is sponsored by: Beat the post-holiday blues
Get a FREE limited edition SourceForge.net t-shirt from ThinkGeek.
It's fun and FREE -- well, almost....http://www.thinkgeek.com/sfshirt
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Reply via email to