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.
Thanks in advance
Yan Burman
[email protected]
_________________________________________________________________
Express yourself instantly with MSN Messenger! Download today it's FREE!
http://messenger.msn.click-url.com/go/onm00200471ave/direct/01/
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.
diff -Nrubp linux-2.6.18.orig/drivers/hwmon/Kconfig linux-2.6.18.mdps/drivers/hwmon/Kconfig
--- linux-2.6.18.orig/drivers/hwmon/Kconfig 2006-10-11 14:20:08.000000000 +0200
+++ linux-2.6.18.mdps/drivers/hwmon/Kconfig 2006-10-13 08:52:42.000000000 +0200
@@ -507,6 +507,22 @@ config SENSORS_HDAPS
Say Y here if you have an applicable laptop and want to experience
the awesome power of hdaps.
+config SENSORS_MDPS
+ tristate "HP Mobile Data Protection System 3D (mdps)"
+ depends on ACPI && HWMON && INPUT && X86
+ default n
+ help
+ This driver provides support for the HP Mobile Data Protection
+ System 3D (mdps), which is an accelerometer. Only HP nc6400 is supported
+ right now, but it may work on other models as well. The
+ accelerometer data is readable via /proc/drivers/mdps.
+
+ This driver also provides an absolute input class device, allowing
+ the laptop to act as a pinball machine-esque joystick.
+
+ This driver can also be built as a module. If so, the module
+ will be called mdps.
+
config HWMON_DEBUG_CHIP
bool "Hardware Monitoring Chip debugging messages"
depends on HWMON
diff -Nrubp linux-2.6.18.orig/drivers/hwmon/Makefile linux-2.6.18.mdps/drivers/hwmon/Makefile
--- linux-2.6.18.orig/drivers/hwmon/Makefile 2006-10-11 14:20:08.000000000 +0200
+++ linux-2.6.18.mdps/drivers/hwmon/Makefile 2006-10-13 10:14:10.000000000 +0200
@@ -26,6 +26,7 @@ obj-$(CONFIG_SENSORS_FSCPOS) += fscpos.o
obj-$(CONFIG_SENSORS_GL518SM) += gl518sm.o
obj-$(CONFIG_SENSORS_GL520SM) += gl520sm.o
obj-$(CONFIG_SENSORS_HDAPS) += hdaps.o
+obj-$(CONFIG_SENSORS_MDPS) += mdps.o
obj-$(CONFIG_SENSORS_IT87) += it87.o
obj-$(CONFIG_SENSORS_LM63) += lm63.o
obj-$(CONFIG_SENSORS_LM70) += lm70.o
diff -Nrubp linux-2.6.18.orig/drivers/hwmon/mdps.c linux-2.6.18.mdps/drivers/hwmon/mdps.c
--- linux-2.6.18.orig/drivers/hwmon/mdps.c 1970-01-01 02:00:00.000000000 +0200
+++ linux-2.6.18.mdps/drivers/hwmon/mdps.c 2006-10-13 10:13:49.000000000 +0200
@@ -0,0 +1,700 @@
+/*
+ * mdps.c - HP Mobile Data Protection System 3D ACPI driver
+ *
+ * Copyright (C) 2006 Yan Burman
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/interrupt.h>
+#include <linux/proc_fs.h>
+#include <linux/seq_file.h>
+#include <linux/input.h>
+#include <linux/kthread.h>
+#include <linux/delay.h>
+
+#include <acpi/acpi_drivers.h>
+#include <acpi/acnamesp.h>
+
+#include <asm/uaccess.h>
+
+#define VERSION "0.1"
+
+MODULE_DESCRIPTION("HP three-axis digital accelerometer ACPI driver");
+MODULE_AUTHOR("Yan Burman ([email protected])");
+MODULE_VERSION(VERSION);
+MODULE_LICENSE("GPL");
+
+#define DRIVER_NAME "mdps"
+#define ACPI_MDPS_CLASS "accelerometer"
+#define ACPI_MDPS_ID "HPQ0004"
+
+#define MDPS_PROC_ROOT "driver/mdps"
+
+// The actual chip is STMicroelectronics LIS3LV02DL or LIS3LV02DQ
+
+#define MDPS_WHO_AM_I 0x0F //r 00111010
+#define MDPS_OFFSET_X 0x16 //rw
+#define MDPS_OFFSET_Y 0x17 //rw
+#define MDPS_OFFSET_Z 0x18 //rw
+#define MDPS_GAIN_X 0x19 //rw
+#define MDPS_GAIN_Y 0x1A //rw
+#define MDPS_GAIN_Z 0x1B //rw
+#define MDPS_CTRL_REG1 0x20 //rw 00000111
+#define MDPS_CTRL_REG2 0x21 //rw 00000000
+#define MDPS_CTRL_REG3 0x22 //rw 00001000
+#define MDPS_HP_FILTER RESET 0x23 //r
+#define MDPS_STATUS_REG 0x27 //rw 00000000
+#define MDPS_OUTX_L 0x28 //r
+#define MDPS_OUTX_H 0x29 //r
+#define MDPS_OUTY_L 0x2A //r
+#define MDPS_OUTY_H 0x2B //r
+#define MDPS_OUTZ_L 0x2C //r
+#define MDPS_OUTZ_H 0x2D //r
+#define MDPS_FF_WU_CFG 0x30 //rw 00000000
+#define MDPS_FF_WU_SRC 0x31 //rw 00000000
+#define MDPS_FF_WU_ACK 0x32 //r
+#define MDPS_FF_WU_THS_L 0x34 //rw 00000000
+#define MDPS_FF_WU_THS_H 0x35 //rw 00000000
+#define MDPS_FF_WU_DURATION 0x36 //rw 00000000
+#define MDPS_DD_CFG 0x38 //rw 00000000
+#define MDPS_DD_SRC 0x39 //rw 00000000
+#define MDPS_DD_ACK 0x3A //r
+#define MDPS_DD_THSI_L 0x3C //rw 00000000
+#define MDPS_DD_THSI_H 0x3D //rw 00000000
+#define MDPS_DD_THSE_L 0x3E //rw 00000000
+#define MDPS_DD_THSE_H 0x3F //rw 00000000
+
+#define MDPS_ID 0x3A
+
+// mouse device poll interval in milliseconds
+#define MDPS_POLL_INTERVAL 30
+
+static unsigned int mouse;
+module_param(mouse, bool, 0);
+MODULE_PARM_DESC(mouse, "Enable the input class device on module load");
+
+#ifdef CONFIG_PROC_FS
+static unsigned int power_off;
+module_param(power_off, bool, 0);
+MODULE_PARM_DESC(power_off, "Turn off device on module load");
+#endif
+
+struct acpi_mdps
+{
+ struct acpi_device* device; /* The ACPI device */
+ u32 irq; /* IRQ number */
+ struct input_dev* idev; /* input device */
+ struct task_struct* kthread; /* kthread for input */
+ int xcalib; /* calibrated null value for x */
+ int ycalib; /* calibrated null value for y */
+ int is_on; /* whether the device is on or off */
+#ifdef CONFIG_PROC_FS
+ struct proc_dir_entry *dir;
+#endif
+};
+
+static struct acpi_mdps mdps;
+
+static int mdps_add(struct acpi_device *device);
+static int mdps_remove(struct acpi_device *device, int type);
+static int mdps_suspend(struct acpi_device * device, int state);
+static int mdps_resume(struct acpi_device * device, int state);
+static int mdps_remove_fs(void);
+static int mdps_add_fs(struct acpi_device *device);
+static void mdps_mouse_enable(void);
+static void mdps_mouse_disable(void);
+
+static struct acpi_driver mdps_driver =
+{
+ .name = DRIVER_NAME,
+ .class = ACPI_MDPS_CLASS,
+ .ids = ACPI_MDPS_ID,
+ .ops = {
+ .add = mdps_add,
+ .remove = mdps_remove,
+ .suspend = mdps_suspend,
+ .resume = mdps_resume
+ }
Strange indentation.
+};
+
+/** Create a single value from 2 bytes received from the accelerometer
+ * @param hi the high byte
+ * @param lo the low byte
+ * @return the resulting value
+ */
Hmm, kernel doesn't use doxygen...
+static inline s16 mdps_glue_bytes(unsigned long hi, unsigned long lo)
+{
+ // In "12 bit right justified" mode, bit 6, bit 7, bit 8 = bit 5
+ if (hi & 0x10)
+ hi |= 0xE0;
+ return (s16)(lo | ((hi << 8)));
+}
+
+static acpi_status read_acpi_int_param(acpi_handle handle, acpi_string method,
+ int val, unsigned long* ret)
+{
+ union acpi_object arg0 = { ACPI_TYPE_INTEGER };
+ struct acpi_object_list args = { 1, &arg0 };
+
+ arg0.integer.value = val;
+
+ return acpi_evaluate_integer(handle, method, &args, ret);
+}
+
+/** ACPI _STA method: get device status
+ * @param handle the handle of the device
+ * @param[out] ret result of the operation
+ * @return AE_OK on success
+ */
+static inline acpi_status mdps__STA(acpi_handle handle, unsigned long* ret)
+{
+ return acpi_evaluate_integer(handle, METHOD_NAME__STA, NULL, ret);
+}
+
+/** ACPI ALRD method: read a register
+ * @param handle the handle of the device
+ * @param reg the register to read
+ * @param[out] ret result of the operation
+ * @return AE_OK on success
+ */
+static inline acpi_status mdps_ALRD(acpi_handle handle, int reg,
+ unsigned long* ret)
+{
+ return read_acpi_int_param(handle, "ALRD", reg, ret);
+}
+
+/** ACPI _INI method: initialize the device.
+ * @param handle the handle of the device
+ * @return 0 on success
+ */
+static inline acpi_status mdps__INI(acpi_handle handle)
+{
+ return acpi_evaluate_object(handle, METHOD_NAME__INI, NULL, NULL);
+}
+
+/** ACPI ALWR method: write to a register
+ * @param handle the handle of the device
+ * @param reg the register to write to
+ * @param val the value to write
+ * @param[out] ret result of the operation
+ * @return AE_OK on success
+ */
+static acpi_status mdps_ALWR(acpi_handle handle, int reg, int val,
+ unsigned long* ret)
+{
+ union acpi_object in_obj[2];
+ struct acpi_object_list args;
+
+ args.count = 2;
+ args.pointer = in_obj;
+ in_obj[0].type = ACPI_TYPE_INTEGER;
+ in_obj[0].integer.value = reg;
+ in_obj[1].type = ACPI_TYPE_INTEGER;
+ in_obj[1].integer.value = val;
+
+ return acpi_evaluate_integer(handle, "ALWR", &args, ret);
+}
+
+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;
+}
+
+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;
+}
+
+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);
+}
+
+static inline int mdps_poweron(acpi_handle handle)
+{
+ mdps.is_on = 1;
+ return (mdps__INI(handle) == AE_OK);
+}
+
+int mdps_suspend(struct acpi_device * device, int state)
This function was declared static earlier.
+{
+ mdps_poweroff(mdps.device->handle);
+
+ mdps_mouse_disable();
+
+ return 0;
+}
+
+int mdps_resume(struct acpi_device * device, int state)
Missing static again.
+{
+ mdps_poweron(mdps.device->handle);
+
+ if (mouse)
+ mdps_mouse_enable();
+
+ return 0;
+}
+
+static acpi_status
+mdps_get_resource(struct acpi_resource *resource, void *context)
+{
+ if (resource->type == ACPI_RESOURCE_TYPE_EXTENDED_IRQ) {
+ struct acpi_resource_extended_irq* irq;
+ u32* device_irq = context;
+
+ irq = &resource->data.extended_irq;
+ *device_irq = irq->interrupts[0];
+ }
+
+ return AE_OK;
+}
+
+static void mdps_enum_resources(struct acpi_device * device)
+{
+ acpi_status status;
+
+ status = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
+ mdps_get_resource, &mdps.irq);
+ if (ACPI_FAILURE(status))
+ printk(KERN_DEBUG "mdps: Error getting resources\n");
+}
+
+int mdps_add(struct acpi_device *device)
And again. And some more below, please check.
+{
+ unsigned long val;
+
+ if (!device)
+ return -EINVAL;
+
+ mdps.device = device;
+ strcpy(acpi_device_name(device), "mdps");
+ strcpy(acpi_device_class(device), ACPI_MDPS_CLASS);
+ acpi_driver_data(device) = &mdps;
+
+ mdps_ALRD(device->handle, MDPS_WHO_AM_I, &val);
+ if (val != MDPS_ID) {
+ printk(KERN_INFO "mdps: Accelerometer chip not LIS3LV02D{L,Q}\n");
+ return -ENODEV;
+ }
+
+ mdps_enum_resources(device);
+ mdps_add_fs(device);
+ mdps_resume(device, 3);
+
+#ifdef CONFIG_PROC_FS
+ if (power_off)
+ mdps_poweroff(mdps.device->handle);
+#endif
+
+ return 0;
+}
+
+int mdps_remove(struct acpi_device *device, int type)
+{
+ if (!device)
+ return -EINVAL;
+
+ if (mouse)
+ mdps_mouse_disable();
+
+ return mdps_remove_fs();
+}
+
+static inline void mdps_calibrate_mouse(void)
+{
+ int x, y;
+ mdps_get_xy(mdps.device->handle, &x, &y);
+
+ mdps.xcalib = x;
+ mdps.ycalib = y;
+}
+
+#ifdef CONFIG_PROC_FS
+static int mdps_proc_position_show(struct seq_file *seq, void *v)
+{
+ unsigned long z_lo, z_hi;
+ int x, y, z;
+
+ mdps_get_xy(mdps.device->handle, &x, &y);
+
+ mdps_ALRD(mdps.device->handle, MDPS_OUTZ_L, &z_lo);
+ mdps_ALRD(mdps.device->handle, MDPS_OUTZ_H, &z_hi);
+
+ z = mdps_glue_bytes(z_hi, z_lo);
+
+ seq_printf(seq, "(%d, %d, %d)\n", x, y, z);
+
+ return 0;
+}
+
+static int mdps_proc_state_show(struct seq_file *seq, void *v)
+{
+ seq_puts(seq, (mdps.is_on ? "on\n" : "off\n"));
+
+ return 0;
+}
+
+static int mdps_proc_calibrate_mouse_show(struct seq_file *seq, void *v)
+{
+ return 0;
+}
+
+static int mdps_proc_mouse_show(struct seq_file *seq, void *v)
+{
+ seq_puts(seq, (mouse ? "enabled\n" : "disabled\n"));
+
+ return 0;
+}
+
+static ssize_t
+mdps_write_calibrate_mouse(struct file *file, const char __user * buffer,
+ size_t count, loff_t * ppos)
+{
+ mdps_calibrate_mouse();
+
+ return count;
+}
+
+static int mdps_proc_rate_show(struct seq_file *seq, void *v)
+{
+ unsigned long ctrl;
+ int rate = 0;
+
+ mdps_ALRD(mdps.device->handle, MDPS_CTRL_REG1, &ctrl);
+
+ switch (ctrl & 0x30)
+ {
+ case 0x00:
+ rate = 40;
+ break;
+
+ case 0x10:
+ rate = 160;
+ break;
+
+ case 0x20:
+ rate = 640;
+ break;
+
+ case 0x30:
+ rate = 2560;
+ break;
+ }
+
+ seq_printf(seq, "sampling rate:\t%dHz\n", rate);
+
+ return 0;
+}
+
+static ssize_t
+mdps_write_state(struct file *file, const char __user * buffer,
+ size_t count, loff_t * ppos)
+{
+ char state_string[12] = { '\0' };
+
+ if ((count > sizeof(state_string) - 1))
+ return -EINVAL;
+
+ if (copy_from_user(state_string, buffer, count))
+ return -EFAULT;
+
+ state_string[count] = '\0';
+
+ mdps.is_on = simple_strtoul(state_string, NULL, 0);
+
+ if (mdps.is_on)
+ mdps_poweron(mdps.device->handle);
+ else
+ mdps_poweroff(mdps.device->handle);
+
+ return count;
+}
+
+static ssize_t mdps_write_mouse(struct file *file, const char __user * buffer,
+ size_t count, loff_t * ppos)
+{
+ char state_string[12] = { '\0' };
+
+ if ((count > sizeof(state_string) - 1))
+ return -EINVAL;
+
+ if (copy_from_user(state_string, buffer, count))
+ return -EFAULT;
+
+ state_string[count] = '\0';
+
+ mouse = simple_strtoul(state_string, NULL, 0);
+
+ if (mouse)
+ mdps_mouse_enable();
+ else
+ mdps_mouse_disable();
+
+ return count;
+}
+
+static int mdps_proc_position_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, mdps_proc_position_show, NULL);
+}
+
+static int mdps_proc_state_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, mdps_proc_state_show, NULL);
+}
+
+static int mdps_proc_rate_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, mdps_proc_rate_show, NULL);
+}
+
+static int mdps_proc_mouse_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, mdps_proc_mouse_show, NULL);
+}
+
+static int mdps_proc_calibrate_mouse_open(struct inode *inode,
+ struct file *file)
+{
+ return single_open(file, mdps_proc_calibrate_mouse_show, NULL);
+}
+
+static const struct file_operations mdps_proc_position_fops =
+{
+ .owner = THIS_MODULE,
+ .open = mdps_proc_position_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
+static const struct file_operations mdps_proc_state_fops =
+{
+ .owner = THIS_MODULE,
+ .open = mdps_proc_state_open,
+ .read = seq_read,
+ .write = mdps_write_state,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
+static const struct file_operations mdps_proc_mouse_fops =
+{
+ .owner = THIS_MODULE,
+ .open = mdps_proc_mouse_open,
+ .read = seq_read,
+ .write = mdps_write_mouse,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
+static const struct file_operations mdps_proc_calibrate_mouse_fops =
+{
+ .owner = THIS_MODULE,
+ .open = mdps_proc_calibrate_mouse_open,
+ .write = mdps_write_calibrate_mouse,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
+static const struct file_operations mdps_proc_rate_fops =
+{
+ .owner = THIS_MODULE,
+ .open = mdps_proc_rate_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+#endif // CONFIG_PROCFS
+
+void mdps_mouse_enable(void)
+{
+ if (mdps.idev)
+ return;
+
+ mdps.idev = input_allocate_device();
+ if (!mdps.idev)
+ return;
+
+ mdps_calibrate_mouse();
+
+ mdps.idev->name = "HP Mobile Data Protection System";
+ mdps.idev->id.bustype = BUS_I2C;
+ mdps.idev->id.vendor = 0;
+
+ input_set_abs_params(mdps.idev, ABS_X, -2048, 2048, 3, 0);
+ input_set_abs_params(mdps.idev, ABS_Y, -2048, 2048, 3, 0);
+
+ set_bit(EV_ABS, mdps.idev->evbit);
+ set_bit(EV_KEY, mdps.idev->evbit);
+ set_bit(BTN_TOUCH, mdps.idev->keybit);
+
+ if (input_register_device(mdps.idev)) {
+ input_free_device(mdps.idev);
+ mdps.idev = NULL;
+ return;
+ }
+
+ mdps.kthread = kthread_run(mdps_mouse_kthread, NULL, "kmdps");
+ if (IS_ERR(mdps.kthread)) {
+ input_unregister_device(mdps.idev);
+ mdps.idev = NULL;
+ return;
+ }
+
+ mouse = 1;
+
+#ifdef CONFIG_PROC_FS
+ {
+ struct proc_dir_entry *ent;
+ ent = create_proc_entry("calibrate_mouse", S_IWUSR, mdps.dir);
+ if (!ent)
+ {
+ return;
+ }
+
+ ent->proc_fops = &mdps_proc_calibrate_mouse_fops;
+ }
+#endif
+}
+
+void mdps_mouse_disable(void)
+{
+ if (!mdps.idev)
+ return;
+
+ kthread_stop(mdps.kthread);
+
+ input_unregister_device(mdps.idev);
+ mdps.idev = NULL;
+
+#ifdef CONFIG_PROC_FS
+ remove_proc_entry("calibrate_mouse", mdps.dir);
+#endif
+}
+
+int mdps_add_fs(struct acpi_device *device)
+{
+#ifdef CONFIG_PROC_FS
+ struct proc_dir_entry *ent;
+
+ mdps.dir = proc_mkdir(MDPS_PROC_ROOT, NULL);
+ if (!mdps.dir)
+ return -ENOMEM;
+
+ ent = create_proc_entry("position", S_IFREG | S_IRUGO, mdps.dir);
+ if (!ent)
+ return -ENOMEM;
+
+ ent->proc_fops = &mdps_proc_position_fops;
+
+ ent = create_proc_entry("state", S_IFREG | S_IRUGO | S_IWUSR, mdps.dir);
+ if (!ent)
+ return -ENOMEM;
+
+ ent->proc_fops = &mdps_proc_state_fops;
+
+ ent = create_proc_entry("rate", S_IFREG | S_IRUGO, mdps.dir);
+ if (!ent)
+ return -ENOMEM;
+
+ ent->proc_fops = &mdps_proc_rate_fops;
+
+ ent = create_proc_entry("mouse", S_IFREG | S_IRUGO | S_IWUSR, mdps.dir);
+ if (!ent)
+ return -ENOMEM;
+
+ ent->proc_fops = &mdps_proc_mouse_fops;
+#endif
+
+ return 0;
+}
+
+int mdps_remove_fs(void)
+{
+#ifdef CONFIG_PROC_FS
+ remove_proc_entry("position", mdps.dir);
+ remove_proc_entry("state", mdps.dir);
+ remove_proc_entry("rate", mdps.dir);
+ remove_proc_entry("mouse", mdps.dir);
+ remove_proc_entry(MDPS_PROC_ROOT, NULL);
+#endif
+
+ return 0;
+}
+
+static int __init mdps_init_module(void)
+{
+ int ret;
+ acpi_status status;
+ acpi_handle handle = 0;
+
+ if (acpi_disabled)
+ return -ENODEV;
+
+ // device detection: see if our device is present
+ status = acpi_get_handle(NULL, "\\_SB.C002.ACEL", &handle);
+ if (ACPI_FAILURE(status)) {
+ printk(KERN_INFO "mdps: HP Mobile Data Protection System 3D device not found\n");
+ return -ENODEV;
+ }
+
+ ret = acpi_bus_register_driver(&mdps_driver);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static void __exit mdps_exit_module(void)
+{
+ mdps_remove(mdps.device, 1);
+
+ acpi_bus_unregister_driver(&mdps_driver);
+}
+
+module_init(mdps_init_module);
+module_exit(mdps_exit_module);
--
Andrey Panin | Linux and UNIX system administrator
[email protected] | PGP key: wwwkeys.pgp.net
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
On Fri, Oct 13, 2006 at 12:06:40PM +0200, Jesper Juhl wrote:
> > + 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.
Better yet, would be to use the same interface the hdaps driver uses
so that userspace written for one accelerometer works with any hardware.
Having to cope with a dozen different drivers who export in different
places is just silly.
Dave
--
http://www.codemonkey.org.uk
>From: Dave Jones <[email protected]>
>To: Jesper Juhl <[email protected]>
>CC: Burman Yan <[email protected]>, [email protected],
>Andrey Panin <[email protected]>
>Subject: Re: [PATCH] HP mobile data protection system driver
>Date: Fri, 13 Oct 2006 11:50:20 -0400
>Better yet, would be to use the same interface the hdaps driver uses
>so that userspace written for one accelerometer works with any hardware.
>Having to cope with a dozen different drivers who export in different
>places is just silly.
>
That would probably mean that there is a need for single interface. Making
all accelerometers
export to /sys/devices/platform/hdaps sounds wrong to me. It should be a
neutral place then.
There aren't that many accelerometers right now as far as I could tell by
googling.
There are hdaps, amc (something for MAC I think) and now mdps ;)
Only amc and mdps are 3D - hdaps is 2D, and also amc and mdps in theory
support
a free-fall interrupt instead of polling - I just couldn't get the interrupt
part to work yet.
I did make the interface as close as I could to hdaps - seem my later post
with a new patch.
Yan
_________________________________________________________________
Express yourself instantly with MSN Messenger! Download today it's FREE!
http://messenger.msn.click-url.com/go/onm00200471ave/direct/01/
> That would probably mean that there is a need for single interface.
yes please
> Making
> all accelerometers
> export to /sys/devices/platform/hdaps sounds wrong to me. It should be a
> neutral place then.
well.... breaking stuff for no reason other than "but it sounds like HIS
name" is I thing bad. Yes the name is unfortunate, but if you can use
the interface... why not? Just because the name isn't perfect everyone
should change over, including keeping compatibility mess etc etc?
That needs a stronger reason than "it sounds like his name" to me...
Now if the interface itself isn't good enough, that's a different matter
of course; but from what I read so far that's not really the case.
Greetings,
Arjan van de Ven
Ar Gwe, 2006-10-13 am 11:50 -0400, ysgrifennodd Dave Jones:
> Better yet, would be to use the same interface the hdaps driver uses
> so that userspace written for one accelerometer works with any hardware.
> Having to cope with a dozen different drivers who export in different
> places is just silly.
Agreed. We already have an interface layer for reporting multiple
dimensions of motion control - the joystick interface. I think we really
ought to use that with some way to spot that its HDAPS etc so drivers
can decide to use it for its intended purpose (eg a different device
name scheme). The joystick compatibility would also mean it can "just
work" if you tell games to use the device.
Alan
>From: Alan Cox <[email protected]>
>To: Dave Jones <[email protected]>
>CC: Jesper Juhl <[email protected]>, Burman Yan <[email protected]>,
> [email protected], Andrey Panin <[email protected]>
>Subject: Re: [PATCH] HP mobile data protection system driver
>Date: Fri, 13 Oct 2006 18:13:41 +0100
>
>
>Agreed. We already have an interface layer for reporting multiple
>dimensions of motion control - the joystick interface. I think we really
>ought to use that with some way to spot that its HDAPS etc so drivers
>can decide to use it for its intended purpose (eg a different device
>name scheme). The joystick compatibility would also mean it can "just
>work" if you tell games to use the device.
>
>Alan
mdps works as an input device if you choose to enable that functionality (at
load time or at runtime).
Already had a great time playing neverball with it :)
Yan
_________________________________________________________________
Express yourself instantly with MSN Messenger! Download today it's FREE!
http://messenger.msn.click-url.com/go/onm00200471ave/direct/01/
>From: Arjan van de Ven <[email protected]>
>To: Burman Yan <[email protected]>
>CC: [email protected], [email protected], [email protected],
>[email protected]
>
>well.... breaking stuff for no reason other than "but it sounds like HIS
>name" is I thing bad. Yes the name is unfortunate, but if you can use
>the interface... why not? Just because the name isn't perfect everyone
>should change over, including keeping compatibility mess etc etc?
>That needs a stronger reason than "it sounds like his name" to me...
>
>Now if the interface itself isn't good enough, that's a different matter
>of course; but from what I read so far that's not really the case.
>
You have a point, but the thing is that I hope to make this work interrupt
driven in the future.
Right now for some reason request_irq fails with ENOSYS (I don't know if
it's a bios acpi bug, or my bug
or acpi parsing bug yet, although I see that I get the right IRQ - the same
as that other a bit less
known OS...;) and also request_irq succeeds on xen kernel, but fails on RH
2.6.17.13 and vanilla 2.6.18).
To make a long story short, the interface should probably be different from
hdaps'
in the future + hdapsd will have to be modified anyway since I cannot
provide the functionality
to detect keyboard and mouse activity through the accelerometer, since it
doesn't support it.
Right now I can emulate hdaps and perhaps I can even provide the same
methods that will return
dummy result for the stuff I don't have. You think I should emulate hdaps'
functionalty
that is missing from mdps? I actually didn't look at why hdapsd needs
keyboard and mouse activity for.
I'm guessing that it's to see if you're using your laptop, since if there is
activity, chances are
that your laptop is not falling and if it is, it's falling with you along...
Need to check.
Regards
Yan
_________________________________________________________________
Don't just search. Find. Check out the new MSN Search!
http://search.msn.click-url.com/go/onm00200636ave/direct/01/
>From: Arjan van de Ven <[email protected]>
>To: Burman Yan <[email protected]>
>CC: [email protected], [email protected], [email protected],
>[email protected]
>Subject: Re: [PATCH] HP mobile data protection system driver
>Date: Fri, 13 Oct 2006 18:25:02 +0200
>
>
>well.... breaking stuff for no reason other than "but it sounds like HIS
>name" is I thing bad. Yes the name is unfortunate, but if you can use
>the interface... why not? Just because the name isn't perfect everyone
>should change over, including keeping compatibility mess etc etc?
>That needs a stronger reason than "it sounds like his name" to me...
>
>Now if the interface itself isn't good enough, that's a different matter
>of course; but from what I read so far that's not really the case.
>
Well, I just tested hdapsd with mdps and it works - need a relatively high
threshold, since at the
recommended 15 it tries to park heads if you simply lift the laptop. But
regarding mouse/keyboard activity
detection, hdapsd simply right out "open failed" kind of messages and keeps
on working.
I guess it may be a good idea after all to change the sysfs file it to hdaps
for now.
Yan
_________________________________________________________________
Express yourself instantly with MSN Messenger! Download today it's FREE!
http://messenger.msn.click-url.com/go/onm00200471ave/direct/01/
Ar Gwe, 2006-10-13 am 20:59 +0200, ysgrifennodd Burman Yan:
> I guess it may be a good idea after all to change the sysfs file it to hdaps
> for now.
Perhaps it would be better to provide a new they all provide which
indicates 2 v 3 dimensions and also provides a scaled data source so
that you don't get confusion or have to adjust the daemon for each user.
If HDAPS is an IBM^WLeonovo mark thats another reason the final generic
file shouldn't be called HDAPS
Hi.
I finally got 2.6.19-rc2 to work on my laptop and it apparently fixed a bug
related
to MDPS's IRQ handling, so I was able to register the IRQ handler.
I created a newer version of my driver that can now act on interrupts
from the device instead of doing polling on the position that the
accelerometer
reports.
I wrote an interface that is still compatible with hdapsd + made a misc
device that acts similar to /dev/rtc - I also wrote a user space daemon
(it's very
easy now) that can print something like "your laptop is falling: press any
key to catch it"
whenever I drop it 10-15 cm and catch it.
Attached is the patch that applies to 2.6.18 and 2.6.19-rc{2,3}.
I didn't inline the patch, since hotmail screws it up.
Any comments are welcome.
Also, should I send it perhaps to the hwmon maintainer as well?
>From: Alan Cox <[email protected]>
>To: Burman Yan <[email protected]>
>CC: [email protected], [email protected], [email protected],
>[email protected], [email protected]
>Subject: Re: [PATCH] HP mobile data protection system driver
>Date: Fri, 13 Oct 2006 21:34:45 +0100
>
>Ar Gwe, 2006-10-13 am 20:59 +0200, ysgrifennodd Burman Yan:
> > I guess it may be a good idea after all to change the sysfs file it to
>hdaps
> > for now.
>
>Perhaps it would be better to provide a new they all provide which
>indicates 2 v 3 dimensions and also provides a scaled data source so
>that you don't get confusion or have to adjust the daemon for each user.
>
>If HDAPS is an IBM^WLeonovo mark thats another reason the final generic
>file shouldn't be called HDAPS
>
_________________________________________________________________
Express yourself instantly with MSN Messenger! Download today it's FREE!
http://messenger.msn.click-url.com/go/onm00200471ave/direct/01/