Re: [PATCH 04/18] lirc driver for 2nd-gen and later Media Center Ed. USB IR transceivers

[Janne Grunau <j@jannau.net>]

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