Messages in this thread | | | Date | Fri, 13 Oct 2006 12:06:40 +0200 | From | "Jesper Juhl" <> | Subject | Re: [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/
| |