lkml.org 
[lkml]   [2008]   [Sep]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 01/18] lirc core device driver infrastructure
Date
On Tuesday 09 September 2008 03:40:18 Sebastian Siewior wrote:
> >diff --git a/drivers/input/lirc/Makefile b/drivers/input/lirc/Makefile
> >new file mode 100644
> >index 0000000..cdb4c45
> >--- /dev/null
> >+++ b/drivers/input/lirc/Makefile
> >@@ -0,0 +1,8 @@
> >+# Makefile for the lirc drivers.
> >+#
> >+
> >+# Each configuration option enables a list of files.
> >+
> >+EXTRA_CFLAGS =-DIRCTL_DEV_MAJOR=61 -DLIRC_SERIAL_TRANSMITTER -I$(src)
>
> Do you rely on this specific major? Since your daemon opens /dev/lirc0
> you don't need a fixed major do you?

Good question. Quite honestly, I'm not sure. Christoph?

> LIRC_SERIAL_TRANSMITTER is used in patch 2 and just to enable a module
> options. Since it is always the case, please remove it.
> I haven't found the source of $src. It is probably a relic.

Janne took care of these. Heck, he's already replied, I'll just leave out the
parts he already replied to...

> >+++ b/drivers/input/lirc/lirc_dev.c
> >@@ -0,0 +1,809 @@
> >+/*
> >+ * LIRC base driver
> >+ *
> >+ * (L) by Artur Lipowski <alipowski@interia.pl>
>
> Is that L here on purpose?

Historical. I think it is supposed to signify that this was originally
licensed and written by Artur, but simply removing "(L) " is fine (here and in
lirc_dev.h).

> >+#include <linux/smp_lock.h>
>
> if you need this than you use the BKL back. As far as I remember
> the ioctl() handler in kernel core no longer takes the BKL and I don't
> see any locking in irctl_ioctl().


> >+/* helper function
> >+ * initializes the irctl structure
> >+ */
>
> For all comments above functions:
> - Please use the default comment style.

Apologies, I started on that, didn't finish, then forgot about it. Will do.

> - don't comment obvious things
> - please use kernel doc if
> - please comment the prototypes but the actual function.

Working on this too.

> >+ bytes_in_key = p->code_length/8 + (p->code_length%8 ? 1 : 0);
>
> did you actually pass checkpatch.pl ?

Yeah, but now that you point that out, I'm not sure how... :)

Taken care of by simply using BITS_TO_LONG() instead.

> >+EXPORT_SYMBOL(lirc_register_plugin);
>
> Is EXPORT_SYMBOL_GPL() possible?

Should be, I'm definitely not aware of any non-GPL users.

> >+EXPORT_SYMBOL(lirc_unregister_plugin);

Ditto.

> >+/* ----------------------------------------------------------------------
> > */
>
> I hate those

Me too. Gone.

--
Jarod Wilson
jarod@redhat.com



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