Re: Telecom Clock driver for MPCBL0010 ATCA compute blade.

From: Jesper Juhl (jesper.juhl_at_gmail.com)
Date: 08/30/05

  • Next message: Ian E. Morgan: "[PATCH] New SBC8360 watchdog driver (revised)" 
    Date:	Tue, 30 Aug 2005 23:19:43 +0200
To: Mark Gross <mgross@linux.intel.com>

    On 8/30/05, Mark Gross <mgross@linux.intel.com> wrote:
    > On Tuesday 30 August 2005 13:31, Mark Gross wrote:
    > > On Tuesday 30 August 2005 12:16, Marcelo Tosatti wrote:
    > > >
    > > > Mark,
    > > >
    > > > Please fix identation accordingly to CodingStyle and repost, it
    > > > looks quite ugly at the moment.
    > > >
    > > Sorry about that.
    > >
    >
    > My email client is f-ing with me. See attached.
    >

    ok, a few small comments :

    +/* sysFS interface definition:

    Isn't it just called "sysfs" without the caps?

    +Uppon loading the driver will create a sysfs directory under class/misc/tlclk.

    s/Uppon/Upon/

    +This directory exports the following interfaces. There opperation is
    documented

    Line is longer than 80 chars - there are a few more such long lines,
    not going to point them all out, just one example. Ohh, and
    "opperation" should be "operation".

    +All sysfs interaces are integers in hex format, i.e echo 99 > refalign

    s/interaces/interfaces/

    +#if CONFIG_DEBUG_KERNEL

    I believe this should be "#ifdef CONFIG_DEBUG_KERNEL" or "#if
    defined(CONFIG_DEBUG_KERNEL)" or you'll run afoul of the -Wundef
    crowd.

    + int ret_val = 0;

    There seems to be a preference for the name "retval" in the kernel source.

    + val = (unsigned char)arg;

    That cast looks pointless.

    +tlclk_read(struct file * filp, char __user * buf, size_t count, loff_t * f_pos)

    tlclk_read(struct file *filp, char __user *buf, size_t count, loff_t *f_pos)
    "*" next to the variable name is generally the preffered coding style
    (several cases of this, only pointing out one).

    + val = (unsigned char)tmp;

    pointless cast. Do take a look at all your casts and check if they are
    really needed and remove them if not.

    + * This is also the probe opperation to avoid driver use on

    s/opperation/operation/

    +/* switchover_timer.function = switchover_timeout; */

    +/* switchover_timer.data = 0; */

    Why submit a driver with commented out code? If this is not supposed
    to be there, then just remove it. If it needs to be added later, then
    submit a patch later to add it.
    Some people may disagree with me here, but that's my oppinion.

    + out3:

    labels belong at column 0 (zero). 

    -- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
  • Next message: Ian E. Morgan: "[PATCH] New SBC8360 watchdog driver (revised)"

    Relevant Pages