Re: [PATCH] HP mobile data protection system driver

[Jesper Juhl]

On 13/10/06, Andrey Panin <pazke@xxxxxxxxx> wrote:
> On 286, 10 13, 2006 at 10:47:15 +0200, Burman Yan wrote:
> > Hi, all.
> >
> > I'm new to the list so forgive me in advance for any netiquette mistakes I
> > make.
> >
> > I wrote a driver for the accelerometer chip on HP nc6400 laptop (I think
> > the same chip is present on
> > other NCxxxx models). This driver uses ACPI interface present in the bios
> > and behaves pretty much like
> > hdaps. This is a fully functional version - tested on 2.6.17 and 2.6.18
> > (not 2.6.19-rc1 since I have a
> > problem with that kernel on my laptop). It applies on 2.6.19-rc1 as well
> > though. I would like your
> > remarks and suggestions on this. Also, should I mail this patch to a kernel
> > maintainer? I could not find a maintainer that looks like the address for
> > this patch. The closest one is the lm_sensors maintainer, but
> > that's probably wrong.
>
> Some comments:
>
> 1. Use hard tabs instead of 8 spaces;
> 2. C++ comments are tolerated, but not welcomed;
> 3. You missed Signed-off-by: line.
Agree with those points. Some additional ones below.


> +          accelerometer data is readable via /proc/drivers/mdps.

I believe it would be better to use sysfs. Procfs is cluttered enough
as it is and there's not much will to clutter it further.


> +static int mdps_get_xy(acpi_handle handle, int* x, int* y)
> +{
> +        unsigned long x_lo, x_hi, y_lo, y_hi;
> +
> +        mdps_ALRD(mdps.device->handle, MDPS_OUTX_L, &x_lo);
> +        mdps_ALRD(mdps.device->handle, MDPS_OUTX_H, &x_hi);
> +        mdps_ALRD(mdps.device->handle, MDPS_OUTY_L, &y_lo);
> +        mdps_ALRD(mdps.device->handle, MDPS_OUTY_H, &y_hi);
> +
> +        *x = mdps_glue_bytes(x_hi, x_lo);
> +        *y = mdps_glue_bytes(y_hi, y_lo);
> +
> +        return 0;
> +}
> +

You never return anything but 0 here, so why not just make the
function return void instead?


> +static int mdps_mouse_kthread(void *data)
> +{
> +        int x, y;
> +
> +        while (!kthread_should_stop()) {
> +                mdps_get_xy(mdps.device->handle, &x, &y);
> +
> +                // need to invert the X axis for this to look natural
> +                input_report_abs(mdps.idev, ABS_X, -(x - mdps.xcalib));
> +                input_report_abs(mdps.idev, ABS_Y, y - mdps.ycalib);
> +
> +                input_sync(mdps.idev);
> +
> +                msleep_interruptible(MDPS_POLL_INTERVAL);
> +        }
> +
> +        return 0;
> +}

void return ??   Other functions could probably return void as well,
not pointing out any more.


> +
> +static inline int mdps_poweroff(acpi_handle handle)
> +{
> +        unsigned long ret;
> +        mdps.is_on = 0;
> +        return (mdps_ALWR(handle, MDPS_CTRL_REG1, 0x00, &ret) == AE_OK);


> +int mdps_suspend(struct acpi_device * device, int state)

Accepted CodingStyle is :
   int mdps_suspend(struct acpi_device *device, int state)

> +                if (!ent)
> +                {
> +                        return;
> +                }

We don't use braces when the body is just a single statement. It
should be written like so:
   if (!ent)
       return;

And when there is more than a single statement in the body, the
opening brace should be placed on the same line as the 'if' :
   if (some_condition) {
       foo();
       bar();
   } else {
       foobar();
       snafu();
   }




> +static void __exit mdps_exit_module(void)
> +{
> +        mdps_remove(mdps.device, 1);
> +
            ^^^^ superfluous blank line...
> +        acpi_bus_unregister_driver(&mdps_driver);
> +}


--
Jesper Juhl <jesper.juhl@xxxxxxxxx>
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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/