Re: [PATCH 01/18] lirc core device driver infrastructure

[Jarod Wilson <jwilson@redhat.com>]

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