On Fri, Jun 24, 2005 at 09:04:59AM -0600, Michael Downey wrote:
> It looks like my last email got messed up even though I spent 15 minutes
> making sure it was correct. I'm attaching the patch as it looks like
> Thunderbird does an appropriate job attaching text. If this isn't
> satisfactory please let me know and I'll try to dig up something else.
No, that worked. But I still need a full patch description and a
"Signed-off-by:" line. Care to redo that?
Oh, a few comments about the patch, inline below...
> +/* Use our own dbg macro */
> +#define dprintk(fmt, args...) \
> + do { \
> + if(debug) printk(KERN_DEBUG __FILE__ ": " \
> + fmt, ## args); \
> + } while(0)
Don't, use dev_dbg() instead. It's much better.
> +/* Define these values to match your devices */
> +#define USB_KEYSPAN_VENDOR_ID 0x06CD
> +#define USB_KEYSPAN_PRODUCT_UIA11 0x0202
I think that comment can be changed :)
> +/* Structure to store all the real stuff that a remote sends to us. */
> +struct keyspan_message {
> + unsigned short system;
> + unsigned char button;
> + unsigned char toggle;
> +};
u16 instead?
> +/* Structure used for all the bit testing magic needed to be done. */
> +struct bit_tester {
> + unsigned int tester;
> + int len;
> + int pos;
> + int bits_left;
> + unsigned char buffer[32];
> +};
You might want to specify the sizes of these too.
> + if ((remote->data.tester & SYNC_MASK) == SYNC) {
> + remote->data.tester =
> remote->data.tester >> 14;
> + remote->data.bits_left -= 14;
> + found = 1;
> + break;
> + }
> + else {
> + remote->data.tester =
> remote->data.tester >> 1;
> + --remote->data.bits_left;
> + }
The else should look like:
} else {
> + }
> + }
> +
> + if (!found) {
> + remote->stage = 0;
> + remote->data.len = 0;
> + }
> +
> + else {
> + remote->stage = 2;
Hm, odd indentation here.
> + }
> + break;
You have mixed spaces and tabs, please fix that.
> + if (retval) {
> + dprintk("%s - failed to set bit rate due to error: %d\n",
> __FUNCTION__, retval);
> + return(retval);
> + }
Again, use dev_dbg() instead of dprintk().
> + /* Device went away so don't keep trying to read from it. */
Spaces used instead of tabs...
> + remote->input.id.vendor = remote->udev->descriptor.idVendor;
> + remote->input.id.product = remote->udev->descriptor.idProduct;
> + remote->input.id.version = remote->udev->descriptor.bcdDevice;
These are all le16 values. I think you need to convert them to the
proper endian before storing them. See the other input driver in the
2.6.12 kernel for examples of how to do this.
other than those minor things, looks good. Care to fix them up and
resend it?
thanks,
greg k-h
-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
