On 13/10/06, Andrey Panin <[email protected]> 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 <[email protected]>
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
