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:4.730 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site