hwmon: HP Mobile Data Protection System 3D ACPI driver
Subsystems (2)
PATCH 1/1 hwmon: HP Mobile Data Protection System 3D ACPI driver
HP Mobile Data Protection System 3D ACPI driver. Similar to hdaps in functionality. This driver provides 4 kinds of functionality:
1) Creates a misc device /dev/accel that acts similar to /dev/rtc and unblocks the process reading from it when the device detects free-fall interrupt
2) Functions as an input class device to provide similar functionality to hdaps, in order to be able to use the laptop as a joystick
3) Provides an interface similar to hdaps, so that hdapsd could work with it
4) Makes it possible to power the device off.
Applications such as hdaps-gl and hdapsd as well as neverball work with this driver unmodified.
Documentation/hwmon/mdps +86 -0
@@ -0,0 +1,86 @@ +Kernel driver mdps +================== + +Supported chips: + + * STMicroelectronics LIS3LV02DL and LIS3LV02DQ + +Author: + Yan Burman <[email protected]> + + +Description +----------- + +This driver provides support for the HP Mobile Data Protection +System 3D (mdps), which is an accelerometer. HP nc6420 and nx9420 +are supported right now, but it may work on other models as well. The +accelerometer data is readable via /sys/devices/platform/mdps. + +Sysfs attributes under /sys/devices/platform/mdps/: +position - 2D position that the accelerometer reports. Format: "(x,y)" +position3d - 3D position that the accelerometer reports. Format: "(x,y,z)" +calibrate - read: values (x, y) that are used as the base for input class device operation. + write: forces the base to be recalibrated. +rate - reports the sampling rate of the accelerometer device in HZ +state - read: the current power state of the accelerometer device + write: "0" or "1" to power on/off the device +joystick - read: whether the input class device is active or not + write: "0" or "1" to enable/disable the input device + +Load time parameters: +bool joystick - whether to enable the input class device or not (default 1) +bool power_off - whether to power off the device on module load (default 0) +bool hdaps_compat - Make the driver export same interfaces as hdaps, + so that apps like hdaps-gl will work the same as with hdaps (default 0) + The effect of this is: + 1) Instead of /sys/devices/platform/mdps/, /sys/devices/platform/hdaps/ is created + 2) Sensitivity is adjusted to match that of hdaps +bool input_3d - Whether to operate as a 3D input device. (default 0) + BIG FAT WARNING: Do not enable this mode unless you are sure + that you want it, since it eats more CPU + +This driver also provides an absolute input class device, allowing +the laptop to act as a pinball machine-esque joystick. + +Another feature of the driver is misc device called accel that acts +similar to /dev/rtc and reacts on free-fall interrupts received from +the device. It supports blocking operations, poll/select and fasync +operation modes. You must read 4 bytes from the device. +The result is number of free-fall interrupts since the last successful read. + +Example userspace code: + +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> +#include <fcntl.h> +#include <sys/stat.h> +#include <sys/types.h> +#include <string.h> +#include <stdint.h> + +int main(int argc, char* argv[]) +{ + int fd, ret; + uint32_t count; + + fd = open("/dev/accel", O_RDONLY); + if (fd < 0) { + perror("open"); + return EXIT_FAILURE; + } + + for (;;) { + ret = read(fd, &count, sizeof(count)); + if (ret != sizeof(count)) { + perror("read"); + break; + } + + printf("Got %d free-fall interrupts\n", count); + } + + close(fd); + return EXIT_SUCCESS; +}
drivers/hwmon/Kconfig +20 -0
@@ -593,6 +593,26 @@ 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. HP nc6420 and nx9420 + are supported right now, but it may work on other models as well. The + accelerometer data is readable via /sys/devices/platform/mdps. + + This driver also provides an absolute input class device, allowing + the laptop to act as a pinball machine-esque joystick. + + Another feature of the driver is misc device called accel that acts + similar to /dev/rtc and reacts on free-fall interrupts received from + the device. + + 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
drivers/hwmon/Makefile +1 -0
@@ -29,6 +29,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_K8TEMP) += k8temp.o obj-$(CONFIG_SENSORS_LM63) += lm63.o
drivers/hwmon/mdps.c +801 -0
801 lines not on this page · raw message
Hi Yan,
drivers/hwmon/mdps.c line 109
Quoted 4 lines · Yan Burman
drivers/hwmon/mdps.c +4
-0+ +static unsigned int input_3d; +module_param (input_3d, bool,S_IRUGO ); +MODULE_PARM_DESC (input_3d,"Operate as a 3D joystick instead of 2D" );
Why do you need that? Just have the driver always report all 3 events and have applications decide if they want to use ABS_Z events...
Signature · 1 line
Dmitry
Quoted 11 lines · Dmitry Torokhov
Hi Yan,
drivers/hwmon/mdps.c line 109
Quoted 4 lines · Yan Burman
drivers/hwmon/mdps.c +4
-0+ +static unsigned int input_3d; +module_param (input_3d, bool,S_IRUGO ); +MODULE_PARM_DESC (input_3d,"Operate as a 3D joystick instead of 2D" );Why do you need that? Just have the driver always report all 3 events and have applications decide if they want to use
ABS_Zevents...
Getting the coordinates goes through ACPI, so it eats up CPU. I think I remember seeing up to 15-16% CPU usage with all 3 events.
That is why you probably want to disable it in most cases.
Hi,
your code looks very nice and clean, only few comments, see below
Quoted 9 lines
drivers/hwmon/mdps.c +9
-0+static int mdps_joystick_kthread (void *data) +{ +int x =0 , y =0 , z =0 ; + +while (!kthread_should_stop ()) { +if (input_3d) { +mdps_get_xyz (mdps.device->handle, &x, &y, &z); +input_report_abs (mdps.idev,ABS_Z , z - mdps.zcalib); + }else
drivers/hwmon/mdps.c line 283
Quoted 10 lines
drivers/hwmon/mdps.c +10
-0+mdps_get_xy (mdps.device->handle, &x, &y); + +input_report_abs (mdps.idev,ABS_X , x - mdps.xcalib); +input_report_abs (mdps.idev,ABS_Y , y - mdps.ycalib); + +input_sync (mdps.idev); + +try_to_freeze (); +msleep_interruptible (MDPS_POLL_INTERVAL ); + }
what if you get a signal? you probably at least want to handle that somehow. Also, waking up every 30 miliseconds is going to suck up power ... but that might not be avoidable I suppose if you have to poll the hardware.
drivers/hwmon/mdps.c line 337
Quoted 8 lines
drivers/hwmon/mdps.c +8
-0+static int mdps_misc_open (struct inode *inode,struct file *file) +{ +int ret; + +if (!atomic_dec_and_test (&mdps.available)) { +atomic_inc (&mdps.available); +return -EBUSY ;/* already open */ + }
I often get a bit nervous at seeing such "open me once" code, but it might well be ok ... I'll let others comment
drivers/hwmon/mdps.c line 341
Quoted 4 lines
drivers/hwmon/mdps.c +4
-0+ +atomic_set (&mdps.count,0 ); + + ret =request_irq (mdps.irq, mdps_irq,0 ,"mdps" , mdps_irq);
don't you want to allow shared interrupts?
drivers/hwmon/mdps.c line 345
Quoted 4 lines
drivers/hwmon/mdps.c +4
-0+if (ret) { +printk (KERN_ERR "mdps: IRQ%d allocation failed\n" , mdps.irq); +return -ENODEV ; + }
wouldn't you want to inc the atomic in this case?
Quoted 21 lines
drivers/hwmon/mdps.c +21
-0+static ssize_t mdps_misc_read (struct file *file,char __user *buf, +size_t count,loff_t *pos) +{ +DECLARE_WAITQUEUE (wait, current); +u32 data; +ssize_t retval = count; + +if (count !=sizeof (u32)) +return -EINVAL ; + +add_wait_queue (&mdps.misc_wait, &wait); +for (;;) { +set_current_state (TASK_INTERRUPTIBLE ); + data =atomic_xchg (&mdps.count,0 ); +if (data) +break ; + +if (file->f_flags &O_NONBLOCK ) { + retval = -EAGAIN ; + goto out; + }
drivers/hwmon/mdps.c line 390
Quoted 10 lines
drivers/hwmon/mdps.c +10
-0+ +if (signal_pending (current)) { + retval = -ERESTARTSYS ; + goto out; + } + +schedule (); + } + +if (copy_to_user (buf, &data,sizeof (data)))
I'm not entirely sure you want to go into copy_to_user() with a TASK_INTERRUPTIBLE state, I would feel a lot better if you did an explicit __set_current_state(TASK_RUNNING) just before this.
drivers/hwmon/mdps.c line 394
Quoted 4 lines
drivers/hwmon/mdps.c +4
-0+ retval = -EFAULT ; + +out: +set_current_state (TASK_RUNNING );
.. which you do here anyway
drivers/hwmon/mdps.c line 439
Quoted 8 lines
mdps_get_resource(struct acpi_resource *resource, void *context)drivers/hwmon/mdps.c +7
-0+{ +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 ];
eh wait.. if this thing gives you an interrupt.. why do you need to poll every 30 msec? am I missing something?
Quoted 4 lines · Arjan van de Ven
Hi,
your code looks very nice and clean, only few comments, see below
Thanks
Quoted 18 lines
Quoted 18 lines
drivers/hwmon/mdps.c +17
-0+static int mdps_joystick_kthread (void *data) +{ +int x =0 , y =0 , z =0 ; + +while (!kthread_should_stop ()) { +if (input_3d) { +mdps_get_xyz (mdps.device->handle, &x, &y, &z); +input_report_abs (mdps.idev,ABS_Z , z - mdps.zcalib); + }else +mdps_get_xy (mdps.device->handle, &x, &y); + +input_report_abs (mdps.idev,ABS_X , x - mdps.xcalib); +input_report_abs (mdps.idev,ABS_Y , y - mdps.ycalib); + +input_sync (mdps.idev); + +try_to_freeze ();
Quoted 9 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ msleep_interruptible(MDPS_POLL_INTERVAL); + }what if you get a signal? you probably at least want to handle that somehow. Also, waking up every 30 miliseconds is going to suck up power ... but that might not be avoidable I suppose if you have to poll the hardware.
How do I handle signals in kthreads? I looked at quite a few kthreads in the drivers sources, but couldn't find even one example that has any explicit handling of signals.
Quoted 8 lines
drivers/hwmon/mdps.c line 341
Quoted 5 lines
drivers/hwmon/mdps.c +4
-0+ +atomic_set (&mdps.count,0 ); + + ret =request_irq (mdps.irq, mdps_irq,0 ,"mdps" , mdps_irq);don't you want to allow shared interrupts?
I had some problems with that before (2.6.19 I think), but in 2.6.21-rc4 it seems OK, so I will allow shared interrupts in my next version.
Quoted 9 lines
drivers/hwmon/mdps.c line 345
Quoted 6 lines
drivers/hwmon/mdps.c +4
-0+if (ret) { +printk (KERN_ERR "mdps: IRQ%d allocation failed\n" , mdps.irq); +return -ENODEV ; + }wouldn't you want to inc the atomic in this case?
You are right. I missed that after refactoring the code before.
Quoted 29 lines
Quoted 29 lines
drivers/hwmon/mdps.c +28
-0+static ssize_t mdps_misc_read (struct file *file,char __user *buf, +size_t count,loff_t *pos) +{ +DECLARE_WAITQUEUE (wait, current); +u32 data; +ssize_t retval = count; + +if (count !=sizeof (u32)) +return -EINVAL ; + +add_wait_queue (&mdps.misc_wait, &wait); +for (;;) { +set_current_state (TASK_INTERRUPTIBLE ); + data =atomic_xchg (&mdps.count,0 ); +if (data) +break ; + +if (file->f_flags &O_NONBLOCK ) { + retval = -EAGAIN ; + goto out; + } + +if (signal_pending (current)) { + retval = -ERESTARTSYS ; + goto out; + } + +schedule ();
Quoted 9 lines
drivers/hwmon/mdps.c line 390
Quoted 4 lines
drivers/hwmon/mdps.c +3
-0+ } + +if (copy_to_user (buf, &data,sizeof (data)))I'm not entirely sure you want to go into
copy_to_user()with aTASK_INTERRUPTIBLEstate, I would feel a lot better if you did an explicit__set_current_state(TASK_RUNNING)just before this.
Done.
Quoted 10 lines
drivers/hwmon/mdps.c line 394
Quoted 6 lines
drivers/hwmon/mdps.c +4
-0+ retval = -EFAULT ; + +out: +set_current_state (TASK_RUNNING );.. which you do here anyway
Quoted 15 lines
drivers/hwmon/mdps.c line 439
Quoted 9 lines
mdps_get_resource(struct acpi_resource *resource, void *context)drivers/hwmon/mdps.c +7
-0+{ +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 ];eh wait.. if this thing gives you an interrupt.. why do you need to poll every 30 msec? am I missing something?
The problem is that if I use interrupts for mouse-like behavior, I have no way of knowing (unless I do some hacks to remember the last position and see if it changed more than some threshold, or something like that - I'm not sure that at this point I can do that reliably)
what was the source of the interrupt - was that motion or free-fall event. This means I can't both use it as joystick and detect when
the laptop is falling. That's why I use the interrupts for free-fall events (as it was intended for this chip in the laptop context), and use polling for mouse like behavior. This is the same approach as other accelerometer drivers in the kernel (hdaps and ams).