lkml.org 
[lkml]   [2006]   [Oct]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] HP mobile data protection system driver
On 13/10/06, Andrey Panin <pazke@donpac.ru> 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@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/

\
 
 \ /
  Last update: 2006-10-13 12:11    [W:0.420 / U:0.120 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site