Messages in this thread |  | | | From | Janne Grunau <> | | Subject | Re: [PATCH 04/18] lirc driver for 2nd-gen and later Media Center Ed. USB IR transceivers | | Date | Wed, 10 Sep 2008 02:36:03 +0200 |
| |
On Wednesday 10 September 2008 01:30:57 Jonathan Corbet wrote: > On Tue, 9 Sep 2008 00:05:49 -0400 > > Jarod Wilson <jwilson@redhat.com> wrote: > > +/* > > + * LIRC driver for Philips eHome USB Infrared Transceiver > > + * and the Microsoft MCE 2005 Remote Control > > + * > > + * (C) by Martin A. Blatter <martin_a_blatter@yahoo.com> > > + * > > + * Transmitter support and reception code cleanup. > > + * (C) by Daniel Melander <lirc@rajidae.se> > > As I understand it, proper assertions of copyright need the word > "copyright" and a year; the "(C)" trigraph doesn't have any legal > meaning.
Depends on the country. I won't touch copyright notices or attributions.
> > + * This driver will only work reliably with kernel version 2.6.10 > > + * or higher, probably because of differences in USB device > > enumeration + * in the kernel code. Device initialization fails > > most of the time + * with earlier kernel versions. > > My guess is that this will not be a major impediment to mainline > inclusion; this comment can probably go.
Already removed.
> > +/* lock irctl structure */ > > +#define IRLOCK mutex_lock(&ir->lock) > > +#define IRUNLOCK mutex_unlock(&ir->lock) > > Might I request that these go away? They can only obfuscate the > code.
yes, I will manually expand similar macros in the other drivers too.
> Also, set_use_inc() is returning a normal zero-or-negative-error > status to an outside caller. It shouldn't return a locally-defined > symbol like SUCCESS. I'd just use "return 0;".
fixed
> > +#ifdef MODULE > > +static int __init usb_remote_init(void) > > +{ > > I'm curious: you don't need to initialize if not compiled as a > module?
Already fixed in all drivers, relict from out of tree lirc.
> > + int i; > > + > > + printk(KERN_INFO "\n"); > > + printk(KERN_INFO DRIVER_NAME ": " DRIVER_DESC " " DRIVER_VERSION > > "\n"); + printk(KERN_INFO DRIVER_NAME ": " DRIVER_AUTHOR "\n"); > > + dprintk(DRIVER_NAME ": debug mode enabled\n"); > > + > > + request_module("lirc_dev"); > > You're calling functions from lirc_dev.c, so modprobe will not load > this module until lirc_dev is already there. There's no point in > trying to load it explicitly with request_module().
True, request_module("lirc_dev"); removed from all drivers.
> > + i = usb_register(&usb_remote_driver); > > + if (i < 0) { > > + printk(DRIVER_NAME ": usb register failed, result = %d\n", i); > > + return -ENODEV; > > + } > > + > > + return SUCCESS; > > +} > > "return 0;" please.
fixed for all drivers
Janne
|  |