lkml.org 
[lkml]   [2008]   [Sep]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 04/18] lirc driver for 2nd-gen and later Media Center Ed. USB IR transceivers
Date
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


\
 
 \ /
  Last update: 2008-09-10 02:39    [W:0.600 / U:0.992 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site