LIS3LV02Dx Accelerometer driver (take 4)
Subsystems (2)
PATCH 1/1 LIS3LV02Dx Accelerometer driver (take 4)
Hello,
Here is a submission for 2.6.28 of a driver for the ST LIS3LV02Dx accelerometer, a device found in various laptops (HP in particular) and embedded devices. It's the fourth iteration of what used to be called MDPS. I've now made the driver very simple, hoping to make it unobjectionable for acceptance :-)
In particular, I've removed the misc device to access free-fall interrupts because there was not yet a complete consensus if it was the best interface to do that and I couldn't always detect free-falls on my laptop. The idea is that later on additional features can be added. Judging from the feedback I got previously, just due to the post of the patches on the LKML, there is actually quite a lot of interest from lots of different people, and having it in the vanilla tree would probably encourage people even more to participate improving the driver!
Technically it's situated in hwmon because the other accelerometer drivers are there as well, but hwmon guys are not really interested by those drivers. So I guess it's material for Andrew :-)
The patch is actually against 2.6.27 but I've checked that it does apply
fine against 2.6.28-git du jour. For the gory details of the changes
since the previous version here you are:
* Automatically turns off the device after some time not used (and removes all the infrastructure to turn on/off the device manually)
* Add axis info for nc8510 (reported and tested by Luca Di Stephano)
* Add axis info for nc84x0
* Add axis info for hp2133 (patch by Pavel Macheck)
* Moved the register definition to an include file
* Various clean ups, to make it less specific to ACPI/HP
* Rename from MDPS to lis3lv02d, as the driver aims at not being HP specific
* Change the frequency to 20Hz (half of the sensor freq) instead of 33Hz
* Added myselft as in the maintainer file
* Free-fall interrupt has been removed for now, as it was sometimes not detecting anything. Once it works fine, it can be added again.
Answer to remarks from previous submission:
* Andrew Morton asked to investigate using interrupt-based sensor reading (instead of timer-based): it turns out not useful because the sensor generate interrupts even if the data has not changed, so we end up waking up at 40Hz
* Dmitry suggested to use polled_input: could not because it needs to know when the device is opened/closed in order to turn it on/off (otherwise, it would work fine)
* Andrew suggested to have a cleaner locking for the misc opening: done (uses test_bit now)... and misc is currently not present!
* Andrew suggested to use __WAIT_QUEUE_HEAD_INITIALIZER(): done but misc is gone anyway
* Andrew suggested removing the version number: done
* Andrew asked if it was tested with CONFIG_SYSFS=n. It now has been (and works!)
* Andrew asked why strcpy(acpi_device_name(..),) works. I don't know but every other driver do the same :-S
* Andrew and Pavel Macheck noticed some kernel-doc alike comments which were not: they now follow the correct syntax
* Several people suggested to not put it in hwmon but in the future industrialio susbsystem: fine with that but as the industrialio subsystem is not available yet, it's still in hwmon with the other accelerometer drivers for now, and I'll happily move it whenever industrialio is integrated.
See you,
Eric
=
LIS3LV02Dx Accelerometer driver
This adds a driver to the accelerometer sensor found in several HP laptops (under the commercial names of "HP Mobile Data Protection System 3D" and "HP 3D driveguard"). It tries to have more or less the same interfaces as the hdaps and other accelerometer drivers: in sysfs and as a joystick.
This driver was first written by Yan Burman. Eric Piel has updated it and slimed it up (including the removal of an interface to access to the free-fall feature of the sensor because it is not reliable enough for now). Several people have contributed to the database of the axes.
--- Documentation/hwmon/lis3lv02d | 49 +++ MAINTAINERS | 5 drivers/hwmon/Kconfig | 19 + drivers/hwmon/Makefile | 1 drivers/hwmon/lis3lv02d.c | 603 ++++++++++++++++++++++++++++++++++++++++++ drivers/hwmon/lis3lv02d.h | 149 ++++++++++ 6 files changed, 826 insertions(+)
Documentation/hwmon/lis3lv02d new file +49 -0
@@ -0,0 +1,49 @@ +Kernel driver lis3lv02d +================== + +Supported chips: + + * STMicroelectronics LIS3LV02DL and LIS3LV02DQ + +Author: + Yan Burman <[email protected]> + Eric Piel <[email protected]> + + +Description +----------- + +This driver provides support for the accelerometer found in various HP laptops +sporting the feature officially called "HP Mobile Data Protection System 3D" or +"HP 3D DriveGuard". It detect automatically laptops with this sensor. Known models +(for now the HP 2133, nc6420, nc2510, nc8510, nc84x0, nw9440 and nx9420) will +have their axis automatically oriented on standard way (eg: you can directly +play neverball). The accelerometer data is readable via +/sys/devices/platform/lis3lv02d. + +Sysfs attributes under /sys/devices/platform/lis3lv02d/: +position - 3D position that the accelerometer reports. Format: "(x,y,z)" +calibrate - read: values (x, y, z) that are used as the base for input class device operation. + write: forces the base to be recalibrated with the current position. +rate - reports the sampling rate of the accelerometer device in HZ + +This driver also provides an absolute input class device, allowing +the laptop to act as a pinball machine-esque joystick. + +Axes orientation +---------------- + +For better compatibility between the various laptops. The values reported by +the accelerometer are converted into a "standard" organisation of the axes +(aka "can play neverball out of the box"): + * When the laptop is horizontal the position reported is about 0 for X and Y +and a positive value for Z + * If the left side is elevated, X increases (becomes positive) + * If the front side (where the touchpad is) is elevated, Y decreases (becomes negative) + * If the laptop is put upside-down, Z becomes negative + +If your laptop model is not recognized (cf "dmesg"), you can send an email to the +authors to add it to the database. When reporting a new laptop, please include +the output of "dmidecode" plus the value of /sys/devices/platform/lis3lv02d/position +in these four cases. +
MAINTAINERS +5 -0
@@ -2592,6 +2592,11 @@ L: [email protected] T: git kernel.org:/pub/scm/linux/kernel/git/chrisw/lsm-2.6.git S: Supported +LIS3LV02D ACCELEROMETER DRIVER +P: Eric Piel +M: [email protected] +S: Maintained + LM83 HARDWARE MONITOR DRIVER P: Jean Delvare M: [email protected]
drivers/hwmon/Kconfig +19 -0
@@ -811,6 +811,25 @@ config SENSORS_HDAPS Say Y here if you have an applicable laptop and want to experience the awesome power of hdaps. +config SENSORS_LIS3LV02D + tristate "STMicroeletronics LIS3LV02Dx three-axis digital accelerometer" + depends on ACPI && INPUT + default n + help + This driver provides support for the LIS3LV02Dx accelerometer. In + particular, it can be found in a number of HP laptops, which have the + "Mobile Data Protection System 3D" or "3D DriveGuard" feature. On such + systems the driver should load automatically (via ACPI). The + accelerometer might also be found in other systems, connected via SPI + or I²C. The accelerometer data is readable via + /sys/devices/platform/lis3lv02d. + + 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 lis3lv02d. + config SENSORS_APPLESMC tristate "Apple SMC (Motion sensor, light sensor, keyboard backlight)" depends on INPUT && X86
drivers/hwmon/Makefile +1 -0
@@ -42,6 +42,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_LIS3LV02D) += lis3lv02d.o obj-$(CONFIG_SENSORS_I5K_AMB) += i5k_amb.o obj-$(CONFIG_SENSORS_IBMAEM) += ibmaem.o obj-$(CONFIG_SENSORS_IBMPEX) += ibmpex.o
drivers/hwmon/lis3lv02d.c new file +603 -0
603 lines not on this page · raw message
drivers/hwmon/lis3lv02d.h new file +149 -0
149 lines not on this page · raw message
--
Hi Eric,
Nice looking driver.
Quoted 2 lines
* Several people suggested to not put it in hwmon but in the future industrialio susbsystem: fine with that but as the industrialio subsystem is not available yet, it's still in hwmon with the other accelerometer drivers for now, and I'll happily move it whenever industrialio is integrated.
Hmm.. might be a little while yet and there's still the debate on whether support for input subsystem is a good idea (different requirements etc). I'd be inclined to have a driver for this accelerometer in there, but the form it may be somewhat different (for example, that datardy signal is vital in the sort of apps iio is designed for!)
Also, looking at the code, I'm not sure how similar things are to the equivalent i2c or spi interfaces. Guess that'll only become entirely clear on implementation.
I'll try and get an updated version of industrialio patches as is out later this week.
Documentation/hwmon/lis3lv02d line 25
Quoted 3 lines
Documentation/hwmon/lis3lv02d new file +2
-0+Sysfs attributes under /sys/devices/platform/lis3lv02d/: +position - 3D position that the accelerometer reports. Format: "(x,y,z)"
Hmm. Position? I've probably missed the debate on why it is called this....
Review
Quoted 9 lines
(quoted diff) +6
-0+calibrate - read: values (x, y, z) that are used as the base for input class device operation. + write: forces the base to be recalibrated with the current position. +rate - reports the sampling rate of the accelerometer device in HZ +/* + * The sensor can also generate interrupts (DRDY) but it's pretty pointless + * because their are generated even if the data do not change.
Unless there is some serious filtering the noise level on such a chip will mean
it almost always changes anyway. However, for this type of app I can see your
point!
Quoted 6 lines
So it's better
+ * to keep the interrupt for the free-fall event. The values are updated at
+ * 40Hz (at the lowest frequency), but as it can be pretty time consuming on
+ * some low processor, we poll the sensor only at 20Hz... enough for the
+ * joystick.
+ */
Jonathan Cameron schreef:
Quoted 11 lines
Hi Eric,
Nice looking driver.
Quoted 2 lines
* Several people suggested to not put it in hwmon but in the future industrialio susbsystem: fine with that but as the industrialio subsystem is not available yet, it's still in hwmon with the other accelerometer drivers for now, and I'll happily move it whenever industrialio is integrated.
Hmm.. might be a little while yet and there's still the debate on whether support for input subsystem is a good idea (different requirements etc). I'd be inclined to have a driver for this accelerometer in there, but the form it may be somewhat different (for example, that datardy signal is vital in the sort of apps iio is designed for!)
Quoted 3 lines
Also, looking at the code, I'm not sure how similar things are to the equivalent i2c or spi interfaces. Guess that'll only become entirely clear on implementation.
I'm hopeful it will be rather similar, but time will be judge ;-) I believe the main difference in the driver requirement might not so much come from the interface, but from the original reason someone put an accelerometer in the system. It's in the laptops in order to detect free-fall (for harddisk protection). It's in a phone in order to rotate the screen when the phone is rotated. It's in the arm of a robot in order to provide feedback on the motors. There are guys who just want to use it to play neverball on their laptop. A generic accelerometer subsystem will have to provide interfaces to comply to all the needs of the usages, and should abstract the fact that several interfaces are present from the driver.
Quoted 3 lines
I'll try and get an updated version of industrialio patches as is out later this week.
Great!
Quoted 5 lines
Documentation/hwmon/lis3lv02d line 25
Quoted 3 lines
Documentation/hwmon/lis3lv02d new file +2
-0+Sysfs attributes under /sys/devices/platform/lis3lv02d/: +position - 3D position that the accelerometer reports. Format: "(x,y,z)"Hmm. Position? I've probably missed the debate on why it is called this....
Hehe, probably there has been no debate. I'm using the de-facto standard defined by the hdaps driver. You can consider this as another incentive to propose an official accelerometer subsystem ;-)
Quoted 8 lines
Quoted 3 lines
+/*
+ * The sensor can also generate interrupts (DRDY) but it's pretty pointless
+ * because their are generated even if the data do not change.Unless there is some serious filtering the noise level on such a chip will mean
it almost always changes anyway. However, for this type of app I can see your
point!
Actually, when you leave the laptop without touching it in a calm environment, the movement is very little and the detected changes are around 1Hz. Anyway, the absolute reason that you cannot use the DRDY interrupt is that if you use it, the free-fall and direction detection are not usable anymore (interrupt cannot be shared).
See you,
Eric
Quoted 13 lines
I'm hopeful it will be rather similar, but time will be judge ;-) I believe the main difference in the driver requirement might not so much come from the interface, but from the original reason someone put an accelerometer in the system. It's in the laptops in order to detect free-fall (for harddisk protection). It's in a phone in order to rotate the screen when the phone is rotated. It's in the arm of a robot in order to provide feedback on the motors. There are guys who just want to use it to play neverball on their laptop. A generic accelerometer subsystem will have to provide interfaces to comply to all the needs of the usages, and should abstract the fact that several interfaces are present from the driver.
Agreed to a certain extent. The trick is in avoiding a behemoth of a driver if people
only want some of the functionality. Not to mention ensuring partial drivers supporting
only a subset of the functionality work fine. For now the target of iio is brute force
data capture (cause that's what I want most ;) the others will come in time.
The really fun bit is working the optimum message sets to access to the data you want
as quickly as possible. All sorts of evil interleaving can be done with some chips.
Quoted 14 lines
Quoted 4 lines
I'll try and get an updated version of industrialio patches as is out later this week.
Great!
Quoted 7 lines
Documentation/hwmon/lis3lv02d line 25
Quoted 4 lines
Documentation/hwmon/lis3lv02d new file +2
-0+Sysfs attributes under /sys/devices/platform/lis3lv02d/: +position - 3D position that the accelerometer reports. Format: "(x,y,z)"Hmm. Position? I've probably missed the debate on why it is called this....
Quoted 4 lines
Hehe, probably there has been no debate. I'm using the de-facto standard defined by the hdaps driver. You can consider this as another incentive to propose an official accelerometer subsystem ;-)
lol.
Quoted 8 lines
Actually, when you leave the laptop without touching it in a calm environment, the movement is very little and the detected changes are around 1Hz. Anyway, the absolute reason that you cannot use the DRDY interrupt is that if you use it, the free-fall and direction detection are not usable anymore (interrupt cannot be shared).
Good reason! Though ideally it'll be switchable between the two modes. (Actually this is pretty common and one of the more annoying things to try and put a general framework in place for!)
Hi!
Quoted 5 lines
Here is a submission for 2.6.28 of a driver for the ST LIS3LV02Dx accelerometer, a device found in various laptops (HP in particular) and embedded devices. It's the fourth iteration of what used to be called MDPS. I've now made the driver very simple, hoping to make it unobjectionable for acceptance :-)
Andrew, can you merge this one? It is simple enough, got some testing, and can't really break anything...
Eric:
drivers/hwmon/lis3lv02d.c line 455
Quoted 11 lines
drivers/hwmon/lis3lv02d.c new file +11
-0+static int lis3lv02d_add (struct acpi_device *device) +{ +u8 val; + +if (!device) +return -EINVAL ; + + adev.device = device; +strcpy (acpi_device_name (device),DRIVER_NAME ); +strcpy (acpi_device_class (device),ACPI_MDPS_CLASS ); +acpi_driver_data (device) = &adev;
This one needs to be device->acpi_driver_data = , check -next.
Quoted 4 lines
drivers/hwmon/lis3lv02d.c new file +4
-0+/* For the HP MDPS aka 3D Driveguard */ +static struct acpi_driver lis3lv02d_driver = { + .name =DRIVER_NAME , + .class =ACPI_MDPS_CLASS ,
drivers/hwmon/lis3lv02d.c line 575
Quoted 10 lines
drivers/hwmon/lis3lv02d.c new file +10
-0+ .ids = lis3lv02d_device_ids, + .ops = { + .add = lis3lv02d_add, + .remove = lis3lv02d_remove, +#ifdef CONFIG_PM + .suspend = lis3lv02d_suspend, + .resume = lis3lv02d_resume +#endif + } +};
This one is more nicely done by #defining _suspend/_resume to NULL, IIRC.
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Quoted 8 lines · On Tue, 21 Oct 2008 10:38:22 +0200 Pavel Machek <[email protected]> wrote:
Quoted 5 lines
Here is a submission for 2.6.28 of a driver for the ST LIS3LV02Dx accelerometer, a device found in various laptops (HP in particular) and embedded devices. It's the fourth iteration of what used to be called MDPS. I've now made the driver very simple, hoping to make it unobjectionable for acceptance :-)
Andrew, can you merge this one? It is simple enough, got some testing, and can't really break anything...
It had a rather obvious and rather fatal bug.
What happened to that "given enough eyeballs" thing?
Quoted 9 lines
Quoted 6 lines · On Tue, 21 Oct 2008 10:38:22 +0200 Pavel Machek <[email protected]> wrote:
Quoted 5 lines
Here is a submission for 2.6.28 of a driver for the ST LIS3LV02Dx accelerometer, a device found in various laptops (HP in particular) and embedded devices. It's the fourth iteration of what used to be called MDPS. I've now made the driver very simple, hoping to make it unobjectionable for acceptance :-)
Quoted 6 lines
Quoted 2 lines
Andrew, can you merge this one? It is simple enough, got some testing, and can't really break anything...
It had a rather obvious and rather fatal bug.
What happened to that "given enough eyeballs" thing?
Ok, I guess you have better eyeballs. Yes, the locking was totaly broken (and disabled by initing semaphore to 1 by default). Sorry about that.
This strips some more code, turns semaphore into mutex, and should solve more problems.
I killed the "power down on timer" thingie. I guess we can reintroduce it when we get the basics right (and driver works just fine without that... I did not detect unusual slowness).
Pavel
drivers/hwmon/lis3lv02d.c renamed from drivers/hwmon/lis3lv02d.c.ofic +16 -67
@@ -56,11 +56,6 @@* joystick. */ -/* -* Automatic timeout to turn off the device in jiffies. -* Don't do it too early as it's quite costly to turn it on. -*/ -#define MDPS_TIMEOUT msecs_to_jiffies(30 * 1000)/* 30 seconds */ /* Maximum value our axis may get for the input device (signed 12 bits) */ #define MDPS_MAX_VAL 2048
@@ -75,13 +70,9 @@u32 irq;/* IRQ number */ struct input_dev *idev;/* input device */ struct task_struct *kthread;/* kthread for input */ -struct timer_list poff_timer;/* for automatic power off */ -struct semaphore poff_sem;/* lock to handle on/off timer */ +struct mutex lock;/* lock to handle on/off timer */ struct platform_device *pdev;/* platform device */ atomic_t count;/* interrupt count after last read */ -struct fasync_struct *async_queue;/* queue for the misc device */ -wait_queue_head_t misc_wait;/* Wait queue for the misc device */ -unsigned long misc_opened;/* bit0: whether the device is open */ int xcalib;/* calibrated null value for x */ int ycalib;/* calibrated null value for y */ int zcalib;/* calibrated null value for z */
@@ -90,12 +81,8 @@struct axis_conversion ac;/* hw -> logical axis */ }; -static struct acpi_lis3lv02d adev = { - .misc_wait =__WAIT_QUEUE_HEAD_INITIALIZER (adev.misc_wait), - .poff_sem =__SEMAPHORE_INITIALIZER (adev.poff_sem,1 ), - .usage =0 , - .misc_opened =0 , -}; +static struct acpi_lis3lv02d adev; +static int lis3lv02d_remove_fs (void );static int lis3lv02d_add_fs (struct acpi_device *device);
@@ -162,23 +149,6 @@return acpi_evaluate_integer (handle,"ALWR" , &args, &ret); } -/* -* Write a 16 bit word on a pair of registers -* @handle the handle of the device -* @reg the low register to write to -* @val the value to write -* -* Returns AE_OK on success -*/ -static acpi_status lis3lv02d_write_16 (acpi_handle handle,int reg,s16 val) -{ -acpi_status ret; - ret =lis3lv02d_acpi_write (handle, reg, val &0xff ); -if (ret !=AE_OK ) -return ret; -return lis3lv02d_acpi_write (handle, reg +1 , (val >>8 ) &0xff ); -} -static s16 lis3lv02d_read_16 (acpi_handle handle,int reg) {u8 lo, hi;
@@ -246,17 +216,6 @@lis3lv02d_acpi_read (handle,CTRL_REG2 , &val); val |=CTRL2_BDU |CTRL2_IEN ;lis3lv02d_acpi_write (handle,CTRL_REG2 , val); -if (test_bit (0 , &adev.misc_opened)) { -/* Threshold not too big (10) */ -lis3lv02d_write_16 (handle,FF_WU_THS_L ,10 ); -/* 2 samples in a row before activation */ -lis3lv02d_acpi_write (handle,FF_WU_DURATION ,2 ); -/* detect every direction, don't wait for validation */ -lis3lv02d_acpi_write (handle,FF_WU_CFG ,FF_WU_CFG_AOI - |FF_WU_CFG_XLIE |FF_WU_CFG_XHIE - |FF_WU_CFG_YLIE |FF_WU_CFG_YHIE - |FF_WU_CFG_ZLIE |FF_WU_CFG_ZHIE ); - } }#ifdef CONFIG_PM
@@ -266,7 +225,6 @@lis3lv02d_poweroff (device->handle);return 0 ; } -#endif static int lis3lv02d_resume (struct acpi_device *device) {
@@ -274,6 +232,11 @@ printk(KERN_INFO DRIVER_NAME " Resuming device\n"); return 0; } +#else +#define lis3lv02d_suspend NULL +#define lis3lv02d_resume NULL +#endif +/* * To be called before starting to use the device. It makes sure that the
@@ -282,14 +245,13 @@*/ static void lis3lv02d_increase_use (struct acpi_lis3lv02d *dev) { -down (&dev->poff_sem); +mutex_lock (&dev->lock); dev->usage++;if (dev->usage ==1 ) { -del_timer (&dev->poff_timer);if (!dev->is_on)lis3lv02d_poweron (dev->device->handle); } -up (&dev->poff_sem); +mutex_unlock (&dev->lock); }/*
@@ -298,20 +260,11 @@*/ static void lis3lv02d_decrease_use (struct acpi_lis3lv02d *dev) { -up (&dev->poff_sem); +mutex_lock (&dev->lock); dev->usage--;if (dev->usage ==0 ) -mod_timer (&dev->poff_timer, jiffies +MDPS_TIMEOUT ); -down (&dev->poff_sem); -} - -static void lis3lv02d_poweroff_timeout (unsigned long data) -{ -struct acpi_lis3lv02d *dev = (void *)data; -up (&dev->poff_sem); -lis3lv02d_poweroff (dev->device->handle); -down (&dev->poff_sem); - printk(KERN_DEBUG DRIVER_NAME ": Turning off the device\n"); +lis3lv02d_poweroff (dev->device->handle); +mutex_unlock (&dev->lock); }/**
@@ -435,9 +388,8 @@*/ static int lis3lv02d_init_device (struct acpi_lis3lv02d *dev) { +mutex_init (&dev->lock);lis3lv02d_add_fs (dev->device); -setup_timer (&dev->poff_timer, lis3lv02d_poweroff_timeout, (unsigned long )dev); -init_timer_deferrable (&dev->poff_timer);lis3lv02d_increase_use (dev);if (lis3lv02d_joystick_enable ())
@@ -507,7 +459,7 @@ adev.device = device;strcpy (acpi_device_name (device),DRIVER_NAME );strcpy (acpi_device_class (device),ACPI_MDPS_CLASS ); -acpi_driver_data (device) = &adev; + device->driver_data = &adev;lis3lv02d_acpi_read (device->handle,WHO_AM_I , &val);
@@ -533,7 +485,6 @@return -EINVAL ;lis3lv02d_joystick_disable (); -del_timer (&adev.poff_timer);lis3lv02d_poweroff (device->handle);return lis3lv02d_remove_fs ();
@@ -623,10 +574,8 @@ .ops = { .add = lis3lv02d_add, .remove = lis3lv02d_remove, -#ifdef CONFIG_PM .suspend = lis3lv02d_suspend, - .resume = lis3lv02d_resume -#endif + .resume = lis3lv02d_resume, } };
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Pavel Machek schreef:
Hello,
Quoted 2 lines
Ok, I guess you have better eyeballs. Yes, the locking was totaly broken (and disabled by initing semaphore to 1 by default).
To which value the semaphore should have been initialised? I can see DECLARE_MUTEX(name) declared as:
struct semaphore name = __SEMAPHORE_INITIALIZER(name, 1)
(I'm still learning about the locking in the kernel. To me, it looks like a maze, and I couldn't find one simple document detailing all the mechanisms.)
Quoted 9 lines
Sorry
about that.This strips some more code, turns semaphore into mutex, and should solve more problems.
I killed the "power down on timer" thingie. I guess we can reintroduce it when we get the basics right (and driver works just fine without that... I did not detect unusual slowness).
I inserted the timer thingie because I have a program on my laptop which reads the position twice per second. Using the joystick interface uses two much CPU (it's 20 Hz) and so it's using the sysfs interface. However, turning on and off the device twice per second is rather costly (especially due to the ACPI access). That's why I made it delayed... and I'd like to keep it this way.
I'll send this evening a "delayed power down" based on workqueue. Within the work of a workqueue, it's not possible to use a mutex, right? Only semaphores are allowed?
Eric
Hi!
Quoted 9 lines
Quoted 2 lines
Ok, I guess you have better eyeballs. Yes, the locking was totaly broken (and disabled by initing semaphore to 1 by default).
To which value the semaphore should have been initialised? I can see
DECLARE_MUTEX(name)declared as:
struct semaphore name =__SEMAPHORE_INITIALIZER(name, 1)(I'm still learning about the locking in the kernel. To me, it looks like a maze, and I couldn't find one simple document detailing all the mechanisms.)
It is a bit of maze.
You should not really use semaphores. They are old and tricky.
Quoted 6 lines
Quoted 6 lines
This strips some more code, turns semaphore into mutex, and should solve more problems.
I killed the "power down on timer" thingie. I guess we can reintroduce it when we get the basics right (and driver works just fine without that... I did not detect unusual slowness).
Quoted 6 lines
I inserted the timer thingie because I have a program on my laptop which reads the position twice per second. Using the joystick interface uses two much CPU (it's 20 Hz) and so it's using the sysfs interface. However, turning on and off the device twice per second is rather costly (especially due to the ACPI access). That's why I made it delayed... and I'd like to keep it this way.
Ultimately, we may end up doing that; but for now, I'd like to get very small and very obvious patch merged...
(Possible workaround: sleep 100 < position should keep the accelerometer powered up for 100 seconds, right?)
Quoted 3 lines
I'll send this evening a "delayed power down" based on workqueue. Within the work of a workqueue, it's not possible to use a mutex, right? Only semaphores are allowed?
Within workqueue, mutex and semaphores are both allowed. What you may not do is to keep mutex locked and return control.
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Quoted 18 lines · On Sat, 18 Oct 2008 21:05:12 +0200 Eric Piel <[email protected]> wrote:
...
LIS3LV02Dx Accelerometer driver
This adds a driver to the accelerometer sensor found in several HP laptops (under the commercial names of "HP Mobile Data Protection System 3D" and "HP 3D driveguard"). It tries to have more or less the same interfaces as the hdaps and other accelerometer drivers: in sysfs and as a joystick.
This driver was first written by Yan Burman. Eric Piel has updated it and slimed it up (including the removal of an interface to access to the free-fall feature of the sensor because it is not reliable enough for now). Several people have contributed to the database of the axes.
...
drivers/hwmon/lis3lv02d.c line 76
Quoted 6 lines
drivers/hwmon/lis3lv02d.c new file +6
-0+struct acpi_lis3lv02d { +struct acpi_device *device;/* The ACPI device */ +struct input_dev *idev;/* input device */ +struct task_struct *kthread;/* kthread for input */ +struct timer_list poff_timer;/* for automatic power off */ +struct semaphore poff_sem;/* lock to handle on/off timer */
OK, this can't be a mutex because it's upped from a timer handler (which seems a bit odd, but I assume you know what you're doing ;))
drivers/hwmon/lis3lv02d.c line 89
Quoted 13 lines
drivers/hwmon/lis3lv02d.c new file +13
-0+struct platform_device *pdev;/* platform device */ +atomic_t count;/* interrupt count after last read */ +int xcalib;/* calibrated null value for x */ +int ycalib;/* calibrated null value for y */ +int zcalib;/* calibrated null value for z */ +unsigned char is_on;/* whether the device is on or off */ +unsigned char usage;/* usage counter */ +struct axis_conversion ac;/* hw -> logical axis */ +}; + +static struct acpi_lis3lv02d adev = { + .poff_sem =__SEMAPHORE_INITIALIZER (adev.poff_sem,1 ), + .usage =0 ,
the .foo=0 isn't strictly needed. It can be retained if you believe it has documentary value.
Quoted 4 lines
drivers/hwmon/lis3lv02d.c new file +4
-0+}; + +static int lis3lv02d_remove_fs (void ); +static int lis3lv02d_add_fs (struct acpi_device *device);
drivers/hwmon/lis3lv02d.c line 103
Quoted 10 lines
drivers/hwmon/lis3lv02d.c new file +10
-0+ +/* For automatic insertion of the module */ +static struct acpi_device_id lis3lv02d_device_ids[] = { + {"HPQ0004" ,0 },/* HP Mobile Data Protection System PNP */ + {"" ,0 }, +}; +MODULE_DEVICE_TABLE (acpi, lis3lv02d_device_ids); + +/** +* acpi_init() - ACPI _INI method: initialize the device.
Should be "lis3lv02d_acpi_init"
drivers/hwmon/lis3lv02d.c line 114
Quoted 11 lines
+ * @handle: the handle of the device
+ *
+ * ReturnsAE_OKon success.
+ */drivers/hwmon/lis3lv02d.c new file +7
-0+static inline acpi_status lis3lv02d_acpi_init (acpi_handle handle) +{ +return acpi_evaluate_object (handle,METHOD_NAME__INI ,NULL ,NULL ); +} + +/** +* lis3lv02d_acpi_read() - ACPI ALRD method: read a register
All the kerneldoc comments include the () after the function name. That is unconventional and I do not know what effect it will have upon kerneldoc processing and output.
Quoted 24 lines
+ * @handle: the handle of the device
+ * @reg: the register to read
+ * @ret: result of the operation
+ *
+ * ReturnsAE_OKon success.
+ */drivers/hwmon/lis3lv02d.c new file +15
-0+static acpi_status lis3lv02d_acpi_read (acpi_handle handle,int reg,u8 *ret) +{ +union acpi_object arg0 = {ACPI_TYPE_INTEGER }; +struct acpi_object_list args = {1 , &arg0 }; +unsigned long lret; +acpi_status status; + + arg0.integer.value = reg; + + status =acpi_evaluate_integer (handle,"ALRD" , &args, &lret); + *ret = lret; +return status; +} + +static int lis3lv02d_resume (struct acpi_device *device)
drivers/hwmon/lis3lv02d.c line 247
Quoted 10 lines
drivers/hwmon/lis3lv02d.c new file +10
-0+{ +/* put back the device in the right state (ACPI might turn it on) */ +down (&adev.poff_sem); +if (adev.usage >0 ) +lis3lv02d_poweron (device->handle); +else +lis3lv02d_poweroff (device->handle); +up (&adev.poff_sem); +return 0 ; +}
This function is unused if CONFIG_PM=n.
Please move this inside the ifdef then add
#else
#define lis3lv02d_suspend NULL
#define lis3lv02d_resume NULL
#endif
and remove the ifdefs in the lis3lv02d_driver definition. Standard stuff.
drivers/hwmon/lis3lv02d.c line 285
Quoted 10 lines
...
drivers/hwmon/lis3lv02d.c new file +7
-0+static void lis3lv02d_poweroff_timeout (unsigned long data) +{ +struct acpi_lis3lv02d *dev = (void *)data; + +up (&dev->poff_sem); +lis3lv02d_poweroff (dev->device->handle); +down (&dev->poff_sem);
eek, no, we cannot down a semaphore from a timer handler! It will lead to might_sleep() warnings, scheduling-in-atomic warnings and kernel deadlocks.
drivers/hwmon/lis3lv02d.c line 480
Quoted 12 lines
drivers/hwmon/lis3lv02d.c new file +9
-0+} + +static int lis3lv02d_remove (struct acpi_device *device,int type) +{ +if (!device) +return -EINVAL ; + +lis3lv02d_joystick_disable (); +del_timer (&adev.poff_timer);
Should this be del_timer_sync()?
Bear in mind that the timer handler might be running on another CPU after del_timer() returns...
drivers/hwmon/lis3lv02d.c line 483
Quoted 3 lines
drivers/hwmon/lis3lv02d.c new file +3
-0+lis3lv02d_poweroff (device->handle); + +return lis3lv02d_remove_fs ();
so the above things can occur in parallel with the execution of the timer handler.
Quoted 3 lines
...
Quoted 11 lines · Andrew Morton
Quoted 4 lines · On Sat, 18 Oct 2008 21:05:12 +0200 Eric Piel <[email protected]> wrote:
...
+/**
+ *lis3lv02d_acpi_read()- ACPI ALRD method: read a registerAll the kerneldoc comments include the () after the function name. That is unconventional and I do not know what effect it will have upon kerneldoc processing and output.
Yes, it's unconventional.
It works. It (the "()") will be dropped when processed.
Quoted 8 lines
Quoted 8 lines
+ * @handle: the handle of the device
+ * @reg: the register to read
+ * @ret: result of the operation
+ *
+ * ReturnsAE_OKon success.
+ */(quoted diff) +2
-0+static acpi_status lis3lv02d_acpi_read(acpi_handle handle, int reg, u8 *ret) +{
Randy Dunlap schreef:
Quoted 6 lines
Quoted 5 lines · Andrew Morton
Quoted 2 lines · On Sat, 18 Oct 2008 21:05:12 +0200 Eric Piel <[email protected]> wrote:
...
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
+/**
+ *lis3lv02d_acpi_read()- ACPI ALRD method: read a registerAll the kerneldoc comments include the () after the function name. That is unconventional and I do not know what effect it will have upon kerneldoc processing and output.
Yes, it's unconventional.
It works. It (the "()") will be dropped when processed.
I simply followed the style used in the very first example of the documentation (kernel-doc-nano-HOWTO.txt). Anyhow, I can drop the "()", I'm not attached to them too much ;-)
See you,
Eric
Andrew Morton schreef:
Quoted 11 lines
drivers/hwmon/lis3lv02d.c line 285
Quoted 7 lines
drivers/hwmon/lis3lv02d.c new file +7
-0+static void lis3lv02d_poweroff_timeout (unsigned long data) +{ +struct acpi_lis3lv02d *dev = (void *)data; + +up (&dev->poff_sem); +lis3lv02d_poweroff (dev->device->handle); +down (&dev->poff_sem);eek, no, we cannot down a semaphore from a timer handler! It will lead to
might_sleep()warnings, scheduling-in-atomic warnings and kernel deadlocks.
Ooh... What happened is that I wrote the other functions first. They indirectly use acpi_evaluate_integer(), which can sleep. So I avoided spin_locks, and decided to use a semaphore. But now there is a semaphore in a timer (which is not allowed to sleep).
It seems the semaphore usage is compulsory due to acpi, so I'm planning to change the timer into a scheduled work on a workqueue (with queue_delayed_work(), and cancel_delayed_work_sync()). This workqueue could be the same as the one for the joystick input. Does that look the right way to do it?
Eric
Quoted 17 lines · On Wed, 22 Oct 2008 00:13:42 +0200 Eric Piel <[email protected]> wrote:
Andrew Morton schreef:
Quoted 11 lines
drivers/hwmon/lis3lv02d.c line 285
Quoted 7 lines
drivers/hwmon/lis3lv02d.c new file +7
-0+static void lis3lv02d_poweroff_timeout (unsigned long data) +{ +struct acpi_lis3lv02d *dev = (void *)data; + +up (&dev->poff_sem); +lis3lv02d_poweroff (dev->device->handle); +down (&dev->poff_sem);eek, no, we cannot down a semaphore from a timer handler! It will lead to
might_sleep()warnings, scheduling-in-atomic warnings and kernel deadlocks.Ooh... What happened is that I wrote the other functions first. They indirectly use
acpi_evaluate_integer(), which can sleep. So I avoidedspin_locks, and decided to use a semaphore. But now there is a semaphore in a timer (which is not allowed to sleep).
Quoted 5 lines
It seems the semaphore usage is compulsory due to acpi, so I'm planning to change the timer into a scheduled work on a workqueue (with
queue_delayed_work(), andcancel_delayed_work_sync()). This workqueue could be the same as the one for the joystick input. Does that look the right way to do it?
yep, that's generally a safe transformation.
You might also be able to use keventd, via scheule_delayed_work().
Hi!
Quoted 5 lines
Here is a submission for 2.6.28 of a driver for the ST LIS3LV02Dx accelerometer, a device found in various laptops (HP in particular) and embedded devices. It's the fourth iteration of what used to be called MDPS. I've now made the driver very simple, hoping to make it unobjectionable for acceptance :-)
Here's newer version of patch, just so that I don't lose it. This should fix locking.
I'll go through Andrew's comments and provide nice/clean version addressing all of them.
Documentation/hwmon/lis3lv02d +49 -0
@@ -0,0 +1,49 @@ +Kernel driver lis3lv02d +================== + +Supported chips: + + * STMicroelectronics LIS3LV02DL and LIS3LV02DQ + +Author: + Yan Burman <[email protected]> + Eric Piel <[email protected]> + + +Description +----------- + +This driver provides support for the accelerometer found in various HP laptops +sporting the feature officially called "HP Mobile Data Protection System 3D" or +"HP 3D DriveGuard". It detect automatically laptops with this sensor. Known models +(for now the HP 2133, nc6420, nc2510, nc8510, nc84x0, nw9440 and nx9420) will +have their axis automatically oriented on standard way (eg: you can directly +play neverball). The accelerometer data is readable via +/sys/devices/platform/lis3lv02d. + +Sysfs attributes under /sys/devices/platform/lis3lv02d/: +position - 3D position that the accelerometer reports. Format: "(x,y,z)" +calibrate - read: values (x, y, z) that are used as the base for input class device operation. + write: forces the base to be recalibrated with the current position. +rate - reports the sampling rate of the accelerometer device in HZ + +This driver also provides an absolute input class device, allowing +the laptop to act as a pinball machine-esque joystick. + +Axes orientation +---------------- + +For better compatibility between the various laptops. The values reported by +the accelerometer are converted into a "standard" organisation of the axes +(aka "can play neverball out of the box"): + * When the laptop is horizontal the position reported is about 0 for X and Y +and a positive value for Z + * If the left side is elevated, X increases (becomes positive) + * If the front side (where the touchpad is) is elevated, Y decreases (becomes negative) + * If the laptop is put upside-down, Z becomes negative + +If your laptop model is not recognized (cf "dmesg"), you can send an email to the +authors to add it to the database. When reporting a new laptop, please include +the output of "dmidecode" plus the value of /sys/devices/platform/lis3lv02d/position +in these four cases. +
MAINTAINERS +5 -0
@@ -2638,6 +2638,11 @@ M: [email protected] S: Maintained +LIS3LV02D ACCELEROMETER DRIVER +P: Eric Piel +M: [email protected] +S: Maintained + LM83 HARDWARE MONITOR DRIVER P: Jean Delvare M: [email protected]
drivers/hwmon/Kconfig +19 -0
@@ -825,6 +825,25 @@ Say Y here if you have an applicable laptop and want to experience the awesome power of hdaps. +config SENSORS_LIS3LV02D + tristate "STMicroeletronics LIS3LV02Dx three-axis digital accelerometer" + depends on ACPI && INPUT + default n + help + This driver provides support for the LIS3LV02Dx accelerometer. In + particular, it can be found in a number of HP laptops, which have the + "Mobile Data Protection System 3D" or "3D DriveGuard" feature. On such + systems the driver should load automatically (via ACPI). The + accelerometer might also be found in other systems, connected via SPI + or I²C. The accelerometer data is readable via + /sys/devices/platform/lis3lv02d. + + 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 lis3lv02d. + config SENSORS_APPLESMC tristate "Apple SMC (Motion sensor, light sensor, keyboard backlight)" depends on INPUT && X86
drivers/hwmon/Makefile +1 -0
@@ -48,6 +48,7 @@ obj-$(CONFIG_SENSORS_IBMPEX) += ibmpex.o obj-$(CONFIG_SENSORS_IT87) += it87.o obj-$(CONFIG_SENSORS_K8TEMP) += k8temp.o +obj-$(CONFIG_SENSORS_LIS3LV02D) += lis3lv02d.o obj-$(CONFIG_SENSORS_LM63) += lm63.o obj-$(CONFIG_SENSORS_LM70) += lm70.o obj-$(CONFIG_SENSORS_LM75) += lm75.o
drivers/hwmon/lis3lv02d.c +583 -0
583 lines not on this page · raw message
drivers/hwmon/lis3lv02d.h +149 -0
149 lines not on this page · raw message
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
drivers/hwmon/lis3lv02d.c line 88
Quoted 2 lines
drivers/hwmon/lis3lv02d.c +2
-0+static struct acpi_device_id lis3lv02d_device_ids[] = { + {"HPQ0004" ,0 },/* HP Mobile Data Protection System PNP */
Has HP provided any documentation or engineering support to help support this work?
Note that other laptop drivers that utilize vendor-specific ACPI hooks tend to live under drivers/misc -- for lack of a better home.
thanks,
-Len
Len Brown schreef:
Quoted 5 lines
drivers/hwmon/lis3lv02d.c line 88
Quoted 2 lines
drivers/hwmon/lis3lv02d.c +2
-0+static struct acpi_device_id lis3lv02d_device_ids[] = { + {"HPQ0004" ,0 },/* HP Mobile Data Protection System PNP */ Has HP provided any documentation or engineering support to help support this work?
Not to my knowledge. Ironically, one of the laptops supported, the HP 2133, is even sold with Linux pre-installed!
Quoted 4 lines
Note that other laptop drivers that utilize vendor-specific ACPI hooks tend to live under
drivers/misc-- for lack of a better home.
Yes, this could be also a place for the driver, but as all the other accelerometer drivers are currently in hwmon, it was decided to put it there as well. The plan is to move them to their own subsystem once a accelerometer subsystem is agreed on (Jonathan Cameron is working on this, with industrialio).
See you,
Eric
Quoted 8 lines · Eric Piel
Len Brown schreef:
Quoted 5 lines
drivers/hwmon/lis3lv02d.c line 88
Quoted 2 lines
drivers/hwmon/lis3lv02d.c +2
-0+static struct acpi_device_id lis3lv02d_device_ids[] = { + {"HPQ0004" ,0 },/* HP Mobile Data Protection System PNP */ Has HP provided any documentation or engineering support to help support this work?
Not to my knowledge. Ironically, one of the laptops supported, the HP 2133, is even sold with Linux pre-installed!
Actually, they did promise some docs/support to me, but Eric was just too fast ;-).
Quoted 8 lines
Quoted 3 lines
Note that other laptop drivers that utilize vendor-specific ACPI hooks tend to live under
drivers/misc-- for lack of a better home.Yes, this could be also a place for the driver, but as all the other accelerometer drivers are currently in hwmon, it was decided to put it there as well. The plan is to move them to their own subsystem once a accelerometer subsystem is agreed on (Jonathan Cameron is working on this, with industrialio).
ACK. hdaps is in hwmon, too.
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html