Re: [PATCH] LIS3LV02Dx Accelerometer driver (take 4)
From: Pavel Machek
Date: Wed Oct 22 2008 - 06:27:29 EST
> On Tue, 21 Oct 2008 10:38:22 +0200
> Pavel Machek <pavel@xxxxxxx> wrote:
>
> > > 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?
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
--- linux/drivers/hwmon/lis3lv02d.c.ofic 2008-10-22 10:06:26.000000000 +0200
+++ linux/drivers/hwmon/lis3lv02d.c 2008-10-22 12:11:01.000000000 +0200
@@ -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,
}
};
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/