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 09: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)

> LIRC_SERIAL_TRANSMITTER is used in patch 2 and just to enable a
> module options. Since it is always the case, please remove it.

Added CONFIG_LIRC_SERIAL_TRANSMITTER to Kconfig and removed this from
the Makefile

> I haven't found the source of $src. It is probably a relic.

removed

> >+obj-$(CONFIG_LIRC_DEV) += lirc_dev.o
> >diff --git a/drivers/input/lirc/lirc_dev.c
> > b/drivers/input/lirc/lirc_dev.c new file mode 100644
> >index 0000000..c8f325c
> >--- /dev/null
> >+++ b/drivers/input/lirc/lirc_dev.c
> >@@ -0,0 +1,809 @@
...
> >+
> >+#include <linux/version.h>
>
> You souldn't need this one
>
> >+
> >+#include <linux/autoconf.h>
>
> this as well.

fixed, in all other files as well.

> >+#include <linux/semaphore.h>
>
> haven't found any call of down(). Do you really need this?

A relict. Lirc used many single holder semaphore and I forgot to remove
the include after converting them to mutexes. I'll check the other
files too.

> >+#define __KERNEL_SYSCALLS__
>
> this define shouldn't be here, should it?

Probably not, but it doesn't make a difference for the current kernel.
The only other hit for this symbol is arch/xtensa/kernel/syscall.c
38:#undef __KERNEL_SYSCALLS__

...

> >+static inline void init_irctl(struct irctl *ir)
>
> this inline can go in my opinion. The others here as well. It is not
> *that* performance critical.

There is only one other inline. I doubt it makes a difference there but
I won't change it without test. It does for the object size since the
function is only called from one place.

...

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

yes, and while this is ugly I see no obvious CodingStyle violation.

is following better?

bytes_in_key = p->code_length/8;
if (p->code_length%8)
bytes_in_key++;

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

Afaik yes, all drivers using this symbol are in this patchset and under
GPL. If it is desireable to to use EXPORT_SYMBOL_GPL() has someone else
to decide. Christoph?


> >+/*
> >+ * Overrides for Emacs so that we follow Linus's tabbing style.
> >+ *
> > -------------------------------------------------------------------
> >-------- + * Local variables:
> >+ * c-basic-offset: 8
> >+ * End:
> >+ */
>
> Please don't enforce formating that way.

Removed, from all other files as well.

Thanks for the review. We will address the misssing comments later

Janne


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