hp accelerometer: fix LED handling and add freefall detection

hp accelerometer: fix LED handling and add freefall detection

[Pavel Machek]

lis3 chip can provide interrupt when it detects freefall; it is done
by hardware and is rather reliable. This adds (experimental) support
for it. 

LED on HP notebooks is connected through ACPI. That unfortunately
means that it needs to be delayed by using schedule_work() to avoid
calling ACPI interpretter in invalid context. This patch fixes that.

Signed-off-by: Pavel Machek <pavel@suse.cz>

diff --git a/drivers/hwmon/hp_accel.c b/drivers/hwmon/hp_accel.c
index bd8497b..dd98b3e 100644
--- a/drivers/hwmon/hp_accel.c
+++ b/drivers/hwmon/hp_accel.c
@@ -3,7 +3,7 @@
  *
  *  Copyright (C) 2007-2008 Yan Burman
  *  Copyright (C) 2008 Eric Piel
- *  Copyright (C) 2008 Pavel Machek
+ *  Copyright (C) 2008-2009 Pavel Machek
  *
  *  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
@@ -44,6 +44,36 @@ #include "lis3lv02d.h"
 #define DRIVER_NAME     "lis3lv02d"
 #define ACPI_MDPS_CLASS "accelerometer"
 
+/* Delayed LEDs infrastructure ------------------------------------ */ 
+
+/* Special LED class that can defer work */
+struct delayed_led_classdev {
+	struct led_classdev led_classdev;
+	struct work_struct work;
+	enum led_brightness new_brightness;
+
+	unsigned int led;		/* For driver */
+	void (*set_brightness)(struct delayed_led_classdev *data, enum led_brightness value);
+};
+
+static inline void delayed_set_status_worker(struct work_struct *work)
+{
+	struct delayed_led_classdev *data =
+			container_of(work, struct delayed_led_classdev, work);
+
+	data->set_brightness(data, data->new_brightness);
+}
+
+static inline void delayed_sysfs_set(struct led_classdev *led_cdev,
+			      enum led_brightness brightness)
+{
+	struct delayed_led_classdev *data = container_of(led_cdev,
+			     struct delayed_led_classdev, led_classdev);
+	data->new_brightness = brightness;
+	schedule_work(&data->work);
+}
+
+/* HP-specific accelerometer driver ------------------------------------ */ 
 
 /* For automatic insertion of the module */
 static struct acpi_device_id lis3lv02d_device_ids[] = {
@@ -155,29 +185,52 @@ static struct dmi_system_id lis3lv02d_dm
  */
 };
 
-static acpi_status hpled_acpi_write(acpi_handle handle, int reg)
+static void hpled_set(struct delayed_led_classdev *led_cdev, enum led_brightness value)
 {
+	acpi_handle handle = adev.device->handle;
 	unsigned long long ret; /* Not used when writing */
 	union acpi_object in_obj[1];
 	struct acpi_object_list args = { 1, in_obj };
 
 	in_obj[0].type          = ACPI_TYPE_INTEGER;
-	in_obj[0].integer.value = reg;
+	in_obj[0].integer.value = !!value;
 
-	return acpi_evaluate_integer(handle, "ALED", &args, &ret);
+	acpi_evaluate_integer(handle, "ALED", &args, &ret);
 }
 
-static void hpled_set(struct led_classdev *led_cdev,
-			       enum led_brightness value)
+static struct delayed_led_classdev hpled_led = {
+	.led_classdev = {
+		.name			= "hp::hddprotect",
+		.default_trigger	= "none",
+		.brightness_set		= delayed_sysfs_set,
+		.flags                  = LED_CORE_SUSPENDRESUME,
+	},
+	.set_brightness = hpled_set,
+};
+
+static acpi_status
+lis3lv02d_get_resource(struct acpi_resource *resource, void *context)
 {
-	hpled_acpi_write(adev.device->handle, !!value);
+	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 struct led_classdev hpled_led = {
-	.name			= "hp:red:hddprotection",
-	.default_trigger	= "heartbeat",
-	.brightness_set		= hpled_set,
-};
+static void lis3lv02d_enum_resources(struct acpi_device *device)
+{
+	acpi_status status;
+
+	status = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
+					lis3lv02d_get_resource, &adev.irq);
+	if (ACPI_FAILURE(status))
+		printk(KERN_DEBUG DRIVER_NAME ": Error getting resources\n");
+}
 
 static int lis3lv02d_add(struct acpi_device *device)
 {
@@ -208,13 +261,20 @@ static int lis3lv02d_add(struct acpi_dev
 		adev.ac = lis3lv02d_axis_normal;
 	}
 
-	ret = led_classdev_register(NULL, &hpled_led);
+	INIT_WORK(&hpled_led.work, delayed_set_status_worker);
+	ret = led_classdev_register(NULL, &hpled_led.led_classdev);
 	if (ret)
 		return ret;
 
+	/* obtain IRQ number of our device from ACPI */
+	lis3lv02d_enum_resources(adev.device);
+
 	ret = lis3lv02d_init_device(&adev);
 	if (ret) {
-		led_classdev_unregister(&hpled_led);
+		while (work_pending(&hpled_led.work))
+			schedule();
+
+		led_classdev_unregister(&hpled_led.led_classdev);
 		return ret;
 	}
 
@@ -229,7 +289,9 @@ static int lis3lv02d_remove(struct acpi_
 	lis3lv02d_joystick_disable();
 	lis3lv02d_poweroff(device->handle);
 
-	led_classdev_unregister(&hpled_led);
+	while (work_pending(&hpled_led.work))
+		schedule();
+	led_classdev_unregister(&hpled_led.led_classdev);
 
 	return lis3lv02d_remove_fs();
 }
@@ -240,7 +302,6 @@ static int lis3lv02d_suspend(struct acpi
 {
 	/* make sure the device is off when we suspend */
 	lis3lv02d_poweroff(device->handle);
-	led_classdev_suspend(&hpled_led);
 	return 0;
 }
 
@@ -253,7 +314,6 @@ static int lis3lv02d_resume(struct acpi_
 	else
 		lis3lv02d_poweroff(device->handle);
 	mutex_unlock(&adev.lock);
-	led_classdev_resume(&hpled_led);
 	return 0;
 }
 #else
diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
index 219d2d0..3afa3af 100644
--- a/drivers/hwmon/lis3lv02d.c
+++ b/drivers/hwmon/lis3lv02d.c
@@ -3,7 +3,7 @@
  *
  *  Copyright (C) 2007-2008 Yan Burman
  *  Copyright (C) 2008 Eric Piel
- *  Copyright (C) 2008 Pavel Machek
+ *  Copyright (C) 2008-2009 Pavel Machek
  *
  *  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
@@ -35,6 +35,7 @@ #include <linux/wait.h>
 #include <linux/poll.h>
 #include <linux/freezer.h>
 #include <linux/uaccess.h>
+#include <linux/miscdevice.h>
 #include <acpi/acpi_drivers.h>
 #include <asm/atomic.h>
 #include "lis3lv02d.h"
@@ -55,7 +56,10 @@ #define MDPS_POLL_INTERVAL 50
 /* Maximum value our axis may get for the input device (signed 12 bits) */
 #define MDPS_MAX_VAL 2048
 
-struct acpi_lis3lv02d adev;
+struct acpi_lis3lv02d adev = {
+	.misc_wait   = __WAIT_QUEUE_HEAD_INITIALIZER(adev.misc_wait),
+};
+
 EXPORT_SYMBOL_GPL(adev);
 
 static int lis3lv02d_add_fs(struct acpi_device *device);
@@ -110,26 +114,13 @@ static void lis3lv02d_get_xyz(acpi_handl
 void lis3lv02d_poweroff(acpi_handle handle)
 {
 	adev.is_on = 0;
-	/* disable X,Y,Z axis and power down */
-	adev.write(handle, CTRL_REG1, 0x00);
 }
 EXPORT_SYMBOL_GPL(lis3lv02d_poweroff);
 
 void lis3lv02d_poweron(acpi_handle handle)
 {
-	u8 val;
-
 	adev.is_on = 1;
 	adev.init(handle);
-	adev.write(handle, FF_WU_CFG, 0);
-	/*
-	 * BDU: LSB and MSB values are not updated until both have been read.
-	 *      So the value read will always be correct.
-	 * IEN: Interrupt for free-fall and DD, not for data-ready.
-	 */
-	adev.read(handle, CTRL_REG2, &val);
-	val |= CTRL2_BDU | CTRL2_IEN;
-	adev.write(handle, CTRL_REG2, val);
 }
 EXPORT_SYMBOL_GPL(lis3lv02d_poweron);
 
@@ -162,6 +153,140 @@ static void lis3lv02d_decrease_use(struc
 	mutex_unlock(&dev->lock);
 }
 
+static irqreturn_t lis302dl_interrupt(int irq, void *dummy)
+{
+	/*
+	 * Be careful: on some HP laptops the bios force DD when on battery and
+	 * the lid is closed. This leads to interrupts as soon as a little move
+	 * is done.
+	 */
+	atomic_inc(&adev.count);
+
+	wake_up_interruptible(&adev.misc_wait);
+	kill_fasync(&adev.async_queue, SIGIO, POLL_IN);
+	return IRQ_HANDLED;
+}
+
+static int lis3lv02d_misc_open(struct inode *inode, struct file *file)
+{
+	int ret;
+
+	if (test_and_set_bit(0, &adev.misc_opened))
+		return -EBUSY; /* already open */
+
+	atomic_set(&adev.count, 0);
+
+	/*
+	 * The sensor can generate interrupts for free-fall and direction
+	 * detection (distinguishable with FF_WU_SRC and DD_SRC) but to keep
+	 * the things simple and _fast_ we activate it only for free-fall, so
+	 * no need to read register (very slow with ACPI). For the same reason,
+	 * we forbid shared interrupts.
+	 *
+	 * IRQF_TRIGGER_RISING seems pointless on HP laptops because the
+	 * io-apic is not configurable (and generates a warning) but I keep it
+	 * in case of support for other hardware.
+	 */
+	ret = request_irq(adev.irq, lis302dl_interrupt, IRQF_TRIGGER_RISING,
+			  DRIVER_NAME, &adev);
+
+	if (ret) {
+		clear_bit(0, &adev.misc_opened);
+		printk(KERN_ERR DRIVER_NAME ": IRQ%d allocation failed\n", adev.irq);
+		return -EBUSY;
+	}
+	lis3lv02d_increase_use(&adev);
+	printk("lis3: registered interrupt %d\n", adev.irq);
+	return 0;
+}
+
+static int lis3lv02d_misc_release(struct inode *inode, struct file *file)
+{
+	fasync_helper(-1, file, 0, &adev.async_queue);
+	lis3lv02d_decrease_use(&adev);
+	free_irq(adev.irq, &adev);
+	clear_bit(0, &adev.misc_opened); /* release the device */
+	return 0;
+}
+
+static ssize_t lis3lv02d_misc_read(struct file *file, char __user *buf,
+				size_t count, loff_t *pos)
+{
+	DECLARE_WAITQUEUE(wait, current);
+	u32 data;
+	unsigned char byte_data;
+	ssize_t retval = 1;
+
+	if (count < 1)
+		return -EINVAL;
+
+	add_wait_queue(&adev.misc_wait, &wait);
+	while (true) {
+		set_current_state(TASK_INTERRUPTIBLE);
+		data = atomic_xchg(&adev.count, 0);
+		if (data)
+			break;
+
+		if (file->f_flags & O_NONBLOCK) {
+			retval = -EAGAIN;
+			goto out;
+		}
+
+		if (signal_pending(current)) {
+			retval = -ERESTARTSYS;
+			goto out;
+		}
+
+		schedule();
+	}
+
+	if (data < 255)
+		byte_data = data;
+	else
+		byte_data = 255;
+
+	/* make sure we are not going into copy_to_user() with
+	 * TASK_INTERRUPTIBLE state */
+	set_current_state(TASK_RUNNING);
+	if (copy_to_user(buf, &byte_data, sizeof(byte_data)))
+		retval = -EFAULT;
+
+out:
+	__set_current_state(TASK_RUNNING);
+	remove_wait_queue(&adev.misc_wait, &wait);
+
+	return retval;
+}
+
+static unsigned int lis3lv02d_misc_poll(struct file *file, poll_table *wait)
+{
+	poll_wait(file, &adev.misc_wait, wait);
+	if (atomic_read(&adev.count))
+		return POLLIN | POLLRDNORM;
+	return 0;
+}
+
+static int lis3lv02d_misc_fasync(int fd, struct file *file, int on)
+{
+	return fasync_helper(fd, file, on, &adev.async_queue);
+}
+
+static const struct file_operations lis3lv02d_misc_fops = {
+	.owner   = THIS_MODULE,
+	.llseek  = no_llseek,
+	.read    = lis3lv02d_misc_read,
+	.open    = lis3lv02d_misc_open,
+	.release = lis3lv02d_misc_release,
+	.poll    = lis3lv02d_misc_poll,
+	.fasync  = lis3lv02d_misc_fasync,
+};
+
+static struct miscdevice lis3lv02d_misc_device = {
+	.minor   = MISC_DYNAMIC_MINOR,
+	.name    = "freefall",
+	.fops    = &lis3lv02d_misc_fops,
+};
+
 /**
  * lis3lv02d_joystick_kthread - Kthread polling function
  * @data: unused - here to conform to threadfn prototype
@@ -203,7 +328,6 @@ static void lis3lv02d_joystick_close(str
 	lis3lv02d_decrease_use(&adev);
 }
 
-
 static inline void lis3lv02d_calibrate_joystick(void)
 {
 	lis3lv02d_get_xyz(adev.device->handle, &adev.xcalib, &adev.ycalib, &adev.zcalib);
@@ -250,6 +374,7 @@ void lis3lv02d_joystick_disable(void)
 	if (!adev.idev)
 		return;
 
+	misc_deregister(&lis3lv02d_misc_device);
 	input_unregister_device(adev.idev);
 	adev.idev = NULL;
 }
@@ -268,6 +393,16 @@ int lis3lv02d_init_device(struct acpi_li
 	if (lis3lv02d_joystick_enable())
 		printk(KERN_ERR DRIVER_NAME ": joystick initialization failed\n");
 
+	/* if we did not get an IRQ from ACPI - we have nothing more to do */
+	if (!dev->irq) {
+		printk(KERN_ERR DRIVER_NAME
+			": No IRQ in ACPI. Disabling /dev/freefall\n");
+		goto out;
+	}
+
+	if (misc_register(&lis3lv02d_misc_device))
+		printk(KERN_ERR DRIVER_NAME ": misc_register failed\n");
+out:
 	lis3lv02d_decrease_use(dev);
 	return 0;
 }
@@ -351,6 +489,6 @@ int lis3lv02d_remove_fs(void)
 EXPORT_SYMBOL_GPL(lis3lv02d_remove_fs);
 
 MODULE_DESCRIPTION("ST LIS3LV02Dx three-axis digital accelerometer driver");
-MODULE_AUTHOR("Yan Burman and Eric Piel");
+MODULE_AUTHOR("Yan Burman, Eric Piel, Pavel Machek");
 MODULE_LICENSE("GPL");
 
diff --git a/drivers/hwmon/lis3lv02d.h b/drivers/hwmon/lis3lv02d.h
index 223f1c0..2e7597c 100644
--- a/drivers/hwmon/lis3lv02d.h
+++ b/drivers/hwmon/lis3lv02d.h
@@ -170,6 +170,11 @@ struct acpi_lis3lv02d {
 	unsigned char		is_on;     /* whether the device is on or off */
 	unsigned char		usage;     /* usage counter */
 	struct axis_conversion	ac;        /* hw -> logical axis */
+
+	u32			irq;       /* IRQ number */
+	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 lis3lv02d_init_device(struct acpi_lis3lv02d *dev);

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: hp accelerometer: fix LED handling and add freefall detection

[Éric Piel]

Pavel Machek wrote:
> lis3 chip can provide interrupt when it detects freefall; it is done
> by hardware and is rather reliable. This adds (experimental) support
> for it. 
> 
> LED on HP notebooks is connected through ACPI. That unfortunately
> means that it needs to be delayed by using schedule_work() to avoid
> calling ACPI interpretter in invalid context. This patch fixes that.
> 
Hi Pavel,
Thanks a lot for doing it! I'll test it this week. However, I think
eventually the LED fix and the freefall infrastructure should be two
different patches. Also, while adding the freefall support, you should
add the info in the doc, as well as the little C program demonstrating
its use.

See you,
Eric

Re: hp accelerometer: fix LED handling and add freefall detection

[Pavel Machek]

>    Hi,
>    Very good work.

Thanks :-).

>    Really hard to get accurate accelerometer data by sensor,
>    there need some emulation for sensor like your work.
>    I am using lis331 in our phone. :)

Fortunately, HP is doing most of the work for us.
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

hp accelerometer: add freefall detection

[Pavel Machek]

> Thanks a lot for doing it! I'll test it this week. However, I think
> eventually the LED fix and the freefall infrastructure should be two
> different patches. Also, while adding the freefall support, you should
> add the info in the doc, as well as the little C program demonstrating
> its use.

Agreed :-).

---

This adds freefall handling to hp_accel driver. According to HP, it
should just work, without us having to set the chip up by hand.

hpfall.c is example .c program that parks the disk when accelerometer
detects free fall. It should work; for now, it uses fixed 20seconds
protection period.

Signed-off-by: Pavel Machek <pavel@suse.cz>

diff --git a/Documentation/hwmon/hpfall.c b/Documentation/hwmon/hpfall.c
new file mode 100755
index 0000000..7d17764
--- /dev/null
+++ b/Documentation/hwmon/hpfall.c
@@ -0,0 +1,101 @@
+/* Disk protection for HP machines.
+ *
+ * Copyright 2008 Eric Piel
+ * Copyright 2009 Pavel Machek <pavel@suse.cz>
+ *
+ * GPLv2.
+ */
+
+#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>
+#include <errno.h>
+#include <signal.h>
+
+void write_int(char *path, int i)
+{
+	char buf[1024];
+	int fd = open(path, O_RDWR);
+	if (fd < 0) {
+		perror("open");
+		exit(1);
+	}
+	sprintf(buf, "%d", i);
+	if (write(fd, buf, strlen(buf)) != strlen(buf)) {
+		perror("write");
+		exit(1);
+	}
+	close(fd);
+}
+
+void set_led(int on)
+{
+	write_int("/sys/class/leds/hp::hddprotect/brightness", on);
+}
+
+void protect(int seconds)
+{
+	write_int("/sys/block/sda/device/unload_heads", seconds*1000);
+}
+
+int on_ac(void)
+{
+//	/sys/class/power_supply/AC0/online
+}
+
+int lid_open(void)
+{
+//	/proc/acpi/button/lid/LID/state
+}
+
+void ignore_me(void)
+{
+	protect(0);
+	set_led(0);
+
+}
+
+int main(int argc, char* argv[])
+{
+       int fd, ret;
+
+       fd = open("/dev/freefall", O_RDONLY);
+       if (fd < 0) {
+               perror("open");
+               return EXIT_FAILURE;
+       }
+
+	signal(SIGALRM, ignore_me);
+
+       for (;;) {
+	       unsigned char count;
+
+               ret = read(fd, &count, sizeof(count));
+	       alarm(0);
+	       if ((ret == -1) && (errno == EINTR)) {
+		       /* Alarm expired, time to unpark the heads */
+		       continue;
+	       }
+
+               if (ret != sizeof(count)) {
+                       perror("read");
+                       break;
+               }
+
+	       protect(21);
+	       set_led(1);
+	       if (1 || on_ac() || lid_open()) {
+		       alarm(2);
+	       } else {
+		       alarm(20);
+	       }
+       }
+
+       close(fd);
+       return EXIT_SUCCESS;
+}
diff --git a/Documentation/hwmon/lis3lv02d b/Documentation/hwmon/lis3lv02d
index 0fcfc4a..287f8c9 100644
--- a/Documentation/hwmon/lis3lv02d
+++ b/Documentation/hwmon/lis3lv02d
@@ -33,6 +33,14 @@ rate - reports the sampling rate of the 
 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 "freefall" 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 1 bytes from the device.  The
+result is number of free-fall interrupts since the last successful
+read (or 255 if number of interrupts would not fit).
+
+
 Axes orientation
 ----------------
 
diff --git a/drivers/hwmon/hp_accel.c b/drivers/hwmon/hp_accel.c
index 0370524..5474971 100644
--- a/drivers/hwmon/hp_accel.c
+++ b/drivers/hwmon/hp_accel.c
@@ -208,6 +208,30 @@ static struct delayed_led_classdev hpled
 	.set_brightness = hpled_set,
 };
 
+static acpi_status
+lis3lv02d_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 lis3lv02d_enum_resources(struct acpi_device *device)
+{
+	acpi_status status;
+
+	status = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
+					lis3lv02d_get_resource, &adev.irq);
+	if (ACPI_FAILURE(status))
+		printk(KERN_DEBUG DRIVER_NAME ": Error getting resources\n");
+}
+
 static int lis3lv02d_add(struct acpi_device *device)
 {
 	u8 val;
@@ -242,6 +266,9 @@ static int lis3lv02d_add(struct acpi_dev
 	if (ret)
 		return ret;
 
+	/* obtain IRQ number of our device from ACPI */
+	lis3lv02d_enum_resources(adev.device);
+
 	ret = lis3lv02d_init_device(&adev);
 	if (ret) {
 		flush_work(&hpled_led.work);
diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
index 219d2d0..3afa3af 100644
--- a/drivers/hwmon/lis3lv02d.c
+++ b/drivers/hwmon/lis3lv02d.c
@@ -3,7 +3,7 @@
  *
  *  Copyright (C) 2007-2008 Yan Burman
  *  Copyright (C) 2008 Eric Piel
- *  Copyright (C) 2008 Pavel Machek
+ *  Copyright (C) 2008-2009 Pavel Machek
  *
  *  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
@@ -35,6 +35,7 @@ #include <linux/wait.h>
 #include <linux/poll.h>
 #include <linux/freezer.h>
 #include <linux/uaccess.h>
+#include <linux/miscdevice.h>
 #include <acpi/acpi_drivers.h>
 #include <asm/atomic.h>
 #include "lis3lv02d.h"
@@ -55,7 +56,10 @@ #define MDPS_POLL_INTERVAL 50
 /* Maximum value our axis may get for the input device (signed 12 bits) */
 #define MDPS_MAX_VAL 2048
 
-struct acpi_lis3lv02d adev;
+struct acpi_lis3lv02d adev = {
+	.misc_wait   = __WAIT_QUEUE_HEAD_INITIALIZER(adev.misc_wait),
+};
+
 EXPORT_SYMBOL_GPL(adev);
 
 static int lis3lv02d_add_fs(struct acpi_device *device);
@@ -110,26 +114,13 @@ static void lis3lv02d_get_xyz(acpi_handl
 void lis3lv02d_poweroff(acpi_handle handle)
 {
 	adev.is_on = 0;
-	/* disable X,Y,Z axis and power down */
-	adev.write(handle, CTRL_REG1, 0x00);
 }
 EXPORT_SYMBOL_GPL(lis3lv02d_poweroff);
 
 void lis3lv02d_poweron(acpi_handle handle)
 {
-	u8 val;
-
 	adev.is_on = 1;
 	adev.init(handle);
-	adev.write(handle, FF_WU_CFG, 0);
-	/*
-	 * BDU: LSB and MSB values are not updated until both have been read.
-	 *      So the value read will always be correct.
-	 * IEN: Interrupt for free-fall and DD, not for data-ready.
-	 */
-	adev.read(handle, CTRL_REG2, &val);
-	val |= CTRL2_BDU | CTRL2_IEN;
-	adev.write(handle, CTRL_REG2, val);
 }
 EXPORT_SYMBOL_GPL(lis3lv02d_poweron);
 
@@ -162,6 +153,140 @@ static void lis3lv02d_decrease_use(struc
 	mutex_unlock(&dev->lock);
 }
 
+static irqreturn_t lis302dl_interrupt(int irq, void *dummy)
+{
+	/*
+	 * Be careful: on some HP laptops the bios force DD when on battery and
+	 * the lid is closed. This leads to interrupts as soon as a little move
+	 * is done.
+	 */
+	atomic_inc(&adev.count);
+
+	wake_up_interruptible(&adev.misc_wait);
+	kill_fasync(&adev.async_queue, SIGIO, POLL_IN);
+	return IRQ_HANDLED;
+}
+
+static int lis3lv02d_misc_open(struct inode *inode, struct file *file)
+{
+	int ret;
+
+	if (test_and_set_bit(0, &adev.misc_opened))
+		return -EBUSY; /* already open */
+
+	atomic_set(&adev.count, 0);
+
+	/*
+	 * The sensor can generate interrupts for free-fall and direction
+	 * detection (distinguishable with FF_WU_SRC and DD_SRC) but to keep
+	 * the things simple and _fast_ we activate it only for free-fall, so
+	 * no need to read register (very slow with ACPI). For the same reason,
+	 * we forbid shared interrupts.
+	 *
+	 * IRQF_TRIGGER_RISING seems pointless on HP laptops because the
+	 * io-apic is not configurable (and generates a warning) but I keep it
+	 * in case of support for other hardware.
+	 */
+	ret = request_irq(adev.irq, lis302dl_interrupt, IRQF_TRIGGER_RISING,
+			  DRIVER_NAME, &adev);
+
+	if (ret) {
+		clear_bit(0, &adev.misc_opened);
+		printk(KERN_ERR DRIVER_NAME ": IRQ%d allocation failed\n", adev.irq);
+		return -EBUSY;
+	}
+	lis3lv02d_increase_use(&adev);
+	printk("lis3: registered interrupt %d\n", adev.irq);
+	return 0;
+}
+
+static int lis3lv02d_misc_release(struct inode *inode, struct file *file)
+{
+	fasync_helper(-1, file, 0, &adev.async_queue);
+	lis3lv02d_decrease_use(&adev);
+	free_irq(adev.irq, &adev);
+	clear_bit(0, &adev.misc_opened); /* release the device */
+	return 0;
+}
+
+static ssize_t lis3lv02d_misc_read(struct file *file, char __user *buf,
+				size_t count, loff_t *pos)
+{
+	DECLARE_WAITQUEUE(wait, current);
+	u32 data;
+	unsigned char byte_data;
+	ssize_t retval = 1;
+
+	if (count < 1)
+		return -EINVAL;
+
+	add_wait_queue(&adev.misc_wait, &wait);
+	while (true) {
+		set_current_state(TASK_INTERRUPTIBLE);
+		data = atomic_xchg(&adev.count, 0);
+		if (data)
+			break;
+
+		if (file->f_flags & O_NONBLOCK) {
+			retval = -EAGAIN;
+			goto out;
+		}
+
+		if (signal_pending(current)) {
+			retval = -ERESTARTSYS;
+			goto out;
+		}
+
+		schedule();
+	}
+
+	if (data < 255)
+		byte_data = data;
+	else
+		byte_data = 255;
+
+	/* make sure we are not going into copy_to_user() with
+	 * TASK_INTERRUPTIBLE state */
+	set_current_state(TASK_RUNNING);
+	if (copy_to_user(buf, &byte_data, sizeof(byte_data)))
+		retval = -EFAULT;
+
+out:
+	__set_current_state(TASK_RUNNING);
+	remove_wait_queue(&adev.misc_wait, &wait);
+
+	return retval;
+}
+
+static unsigned int lis3lv02d_misc_poll(struct file *file, poll_table *wait)
+{
+	poll_wait(file, &adev.misc_wait, wait);
+	if (atomic_read(&adev.count))
+		return POLLIN | POLLRDNORM;
+	return 0;
+}
+
+static int lis3lv02d_misc_fasync(int fd, struct file *file, int on)
+{
+	return fasync_helper(fd, file, on, &adev.async_queue);
+}
+
+static const struct file_operations lis3lv02d_misc_fops = {
+	.owner   = THIS_MODULE,
+	.llseek  = no_llseek,
+	.read    = lis3lv02d_misc_read,
+	.open    = lis3lv02d_misc_open,
+	.release = lis3lv02d_misc_release,
+	.poll    = lis3lv02d_misc_poll,
+	.fasync  = lis3lv02d_misc_fasync,
+};
+
+static struct miscdevice lis3lv02d_misc_device = {
+	.minor   = MISC_DYNAMIC_MINOR,
+	.name    = "freefall",
+	.fops    = &lis3lv02d_misc_fops,
+};
+
 /**
  * lis3lv02d_joystick_kthread - Kthread polling function
  * @data: unused - here to conform to threadfn prototype
@@ -203,7 +328,6 @@ static void lis3lv02d_joystick_close(str
 	lis3lv02d_decrease_use(&adev);
 }
 
-
 static inline void lis3lv02d_calibrate_joystick(void)
 {
 	lis3lv02d_get_xyz(adev.device->handle, &adev.xcalib, &adev.ycalib, &adev.zcalib);
@@ -250,6 +374,7 @@ void lis3lv02d_joystick_disable(void)
 	if (!adev.idev)
 		return;
 
+	misc_deregister(&lis3lv02d_misc_device);
 	input_unregister_device(adev.idev);
 	adev.idev = NULL;
 }
@@ -268,6 +393,19 @@ int lis3lv02d_init_device(struct acpi_li
 	if (lis3lv02d_joystick_enable())
 		printk(KERN_ERR DRIVER_NAME ": joystick initialization failed\n");
 
+	printk("lis3_init_device: irq %d\n", dev->irq);
+
+	/* if we did not get an IRQ from ACPI - we have nothing more to do */
+	if (!dev->irq) {
+		printk(KERN_ERR DRIVER_NAME
+			": No IRQ in ACPI. Disabling /dev/freefall\n");
+		goto out;
+	}
+
+	printk("lis3: registering device\n");
+	if (misc_register(&lis3lv02d_misc_device))
+		printk(KERN_ERR DRIVER_NAME ": misc_register failed\n");
+out:
 	lis3lv02d_decrease_use(dev);
 	return 0;
 }
@@ -351,6 +489,6 @@ int lis3lv02d_remove_fs(void)
 EXPORT_SYMBOL_GPL(lis3lv02d_remove_fs);
 
 MODULE_DESCRIPTION("ST LIS3LV02Dx three-axis digital accelerometer driver");
-MODULE_AUTHOR("Yan Burman and Eric Piel");
+MODULE_AUTHOR("Yan Burman, Eric Piel, Pavel Machek");
 MODULE_LICENSE("GPL");
 
diff --git a/drivers/hwmon/lis3lv02d.h b/drivers/hwmon/lis3lv02d.h
index 223f1c0..2e7597c 100644
--- a/drivers/hwmon/lis3lv02d.h
+++ b/drivers/hwmon/lis3lv02d.h
@@ -170,6 +170,11 @@ struct acpi_lis3lv02d {
 	unsigned char		is_on;     /* whether the device is on or off */
 	unsigned char		usage;     /* usage counter */
 	struct axis_conversion	ac;        /* hw -> logical axis */
+
+	u32			irq;       /* IRQ number */
+	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 lis3lv02d_init_device(struct acpi_lis3lv02d *dev);


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: hp accelerometer: add freefall detection

[Andrew Morton]

On Fri, 16 Jan 2009 13:19:40 +0100
Pavel Machek <pavel@suse.cz> wrote:

> new file mode 100755
> index 0000000..7d17764
> --- /dev/null
> +++ b/Documentation/hwmon/hpfall.c

checkpatch has fun with this file.

>
> ...
>
> -struct acpi_lis3lv02d adev;
> +struct acpi_lis3lv02d adev = {
> +	.misc_wait   = __WAIT_QUEUE_HEAD_INITIALIZER(adev.misc_wait),
> +};
> +
>  EXPORT_SYMBOL_GPL(adev);

This seems a rather poor name for a globally-visible identifier.

checkpatch fun (was Re: hp accelerometer: add freefall detection)

[Pavel Machek]

On Fri 2009-01-16 14:34:04, Andrew Morton wrote:
> On Fri, 16 Jan 2009 13:19:40 +0100
> Pavel Machek <pavel@suse.cz> wrote:
> 
> > new file mode 100755
> > index 0000000..7d17764
> > --- /dev/null
> > +++ b/Documentation/hwmon/hpfall.c
> 
> checkpatch has fun with this file.

Well, I have fun with checkpatch :-(.

pavel@amd:/data/l/linux/Documentation$ ../scripts/checkpatch.pl
hwmon/hpfall.c
ERROR: Does not appear to be a unified-diff format patch

total: 1 errors, 0 warnings, 0 lines checked

hwmon/hpfall.c has style problems, please review.  If any of these
errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
pavel@amd:/data/l/linux/Documentation$ ../scripts/checkpatch.pl -f
hwmon/hpfall.c
Unknown option: f
pavel@amd:/data/l/linux/Documentation$ ../scripts/checkpatch.pl -h
Unknown option: h
pavel@amd:/data/l/linux/Documentation$ ../scripts/checkpatch.pl --help
Unknown option: help
pavel@amd:/data/l/linux/Documentation$ 

Ok, will fix.

> > -struct acpi_lis3lv02d adev;
> > +struct acpi_lis3lv02d adev = {
> > +	.misc_wait   = __WAIT_QUEUE_HEAD_INITIALIZER(adev.misc_wait),
> > +};
> > +
> >  EXPORT_SYMBOL_GPL(adev);
> 
> This seems a rather poor name for a globally-visible identifier.

Hmm, right. lis_acpi_dev should be better. Will fix.

---

I tried checkpatch-ing a file, and not only my attempts to use it
failed, my attempts to get help failed too.

pavel@amd:/data/l/linux/Documentation$ ../scripts/checkpatch.pl hwmon/hpfall.c
ERROR: Does not appear to be a unified-diff format patch
....
pavel@amd:/data/l/linux/Documentation$ ../scripts/checkpatch.pl -f hwmon/hpfall.c
Unknown option: f
pavel@amd:/data/l/linux/Documentation$ ../scripts/checkpatch.pl -h
Unknown option: h
pavel@amd:/data/l/linux/Documentation$ ../scripts/checkpatch.pl --help
Unknown option: help

Make -f alias of --file, and print help if user passes unknown option.

Signed-off-by: Pavel Machek <pavel@suse.cz>

---
commit 4ef496ab198210648c310b8ae25872b9f99bee27
tree 1155a0dad369dc33362994bb6ff74868c7408f4c
parent f155017822e2fea0bc7434545cc63c2c8d363e59
author Pavel <pavel@suse.cz> Tue, 20 Jan 2009 11:31:52 +0100
committer Pavel <pavel@suse.cz> Tue, 20 Jan 2009 11:31:52 +0100

 scripts/checkpatch.pl |   34 ++++++++++++++++++++--------------
 1 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 45eb0ae..c9eeda9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -28,6 +28,22 @@ my $mailback = 0;
 my $summary_file = 0;
 my $root;
 my %debug;
+
+sub help {
+	print "usage: $P [options] patchfile\n";
+	print "version: $V\n";
+	print "options: -q               => quiet\n";
+	print "         --no-tree        => run without a kernel tree\n";
+	print "         --terse          => one line per report\n";
+	print "         --emacs          => emacs compile window format\n";
+	print "         --file           => check a source file\n";
+	print "         --strict         => enable more subjective tests\n";
+	print "         --root           => path to the kernel tree root\n";
+	print "         --no-summary     => suppress the per-file summary\n";
+	print "         --summary-file   => include the filename in summary\n";
+	exit(1);
+}
+
 GetOptions(
 	'q|quiet+'	=> \$quiet,
 	'tree!'		=> \$tree,
@@ -35,7 +51,7 @@ GetOptions(
 	'patch!'	=> \$chk_patch,
 	'emacs!'	=> \$emacs,
 	'terse!'	=> \$terse,
-	'file!'		=> \$file,
+	'f|file!'	=> \$file,
 	'subjective!'	=> \$check,
 	'strict!'	=> \$check,
 	'root=s'	=> \$root,
@@ -45,23 +61,13 @@ GetOptions(
 
 	'debug=s'	=> \%debug,
 	'test-only=s'	=> \$tst_only,
-) or exit;
+) or help;
 
 my $exit = 0;
 
+
 if ($#ARGV < 0) {
-	print "usage: $P [options] patchfile\n";
-	print "version: $V\n";
-	print "options: -q               => quiet\n";
-	print "         --no-tree        => run without a kernel tree\n";
-	print "         --terse          => one line per report\n";
-	print "         --emacs          => emacs compile window format\n";
-	print "         --file           => check a source file\n";
-	print "         --strict         => enable more subjective tests\n";
-	print "         --root           => path to the kernel tree root\n";
-	print "         --no-summary     => suppress the per-file summary\n";
-	print "         --summary-file   => include the filename in summary\n";
-	exit(1);
+    	help;
 }
 
 my $dbg_values = 0;



-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH] checkpatch: make -f alias --file, add --help, more verbose help message

[Hannes Eder]

Impact:
  - More verbose help/usage message.
  - Make the option -f an alias for --file.
  - On -h, --help, and --version display help message and exit(0).
  - With no FILE(s) given, exit(1) with "no input files".
  - On invalid options display help/usage and exit(1).

Based on a patch by Pavel Machek.

Signed-off-by: Hannes Eder <hannes@hanneseder.net>
Cc: Pavel Machek <pavel@suse.cz>
---

parts from the original mail:

On Tue, Jan 20, 2009 at 11:32 AM, Pavel Machek <pavel@suse.cz> wrote:
> Subject: checkpatch fun (was Re: hp accelerometer: add freefall detection)
>
> [... skip ...]
>
> I tried checkpatch-ing a file, and not only my attempts to use it
> failed, my attempts to get help failed too.
>
> pavel@amd:/data/l/linux/Documentation$ ../scripts/checkpatch.pl hwmon/hpfall.c
> ERROR: Does not appear to be a unified-diff format patch
> ....
> pavel@amd:/data/l/linux/Documentation$ ../scripts/checkpatch.pl -f hwmon/hpfall.c
> Unknown option: f
> pavel@amd:/data/l/linux/Documentation$ ../scripts/checkpatch.pl -h
> Unknown option: h
> pavel@amd:/data/l/linux/Documentation$ ../scripts/checkpatch.pl --help
> Unknown option: help
>
> [... skip ...]
>

 scripts/checkpatch.pl |   55 +++++++++++++++++++++++++++++++++++++------------
 1 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2d5ece7..1e730be 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -28,6 +28,41 @@ my $mailback = 0;
 my $summary_file = 0;
 my $root;
 my %debug;
+my $help = 0;
+
+sub help {
+	my ($exitcode) = @_;
+
+	print << "EOM";
+Usage: $P [OPTION]... [FILE]...
+Version: $V
+
+Options:
+  -q, --quiet                quiet
+  --no-tree                  run without a kernel tree
+  --no-signoff               do not check for 'Signed-off-by' line
+  --patch                    treat FILE as patchfile (default)
+  --emacs                    emacs compile window format
+  --terse                    one line per report
+  -f, --file                 treat FILE as regular source file
+  --subjective, --strict     enable more subjective tests
+  --root=PATH                PATH to the kernel tree root
+  --no-summary               suppress the per-file summary
+  --mailback                 only produce a report in case of warnings/errors
+  --summary-file             include the filename in summary
+  --debug KEY=[0|1]          turn on/off debugging of KEY, where KEY is one of
+                             'values', 'possible', 'type', and 'attr' (default
+                             is all off)
+  --test-only=WORD           report only warnings/errors containing WORD
+                             literally
+  -h, --help, --version      display this help and exit
+
+When FILE is - read standard input.
+EOM
+
+	exit($exitcode);
+}
+
 GetOptions(
 	'q|quiet+'	=> \$quiet,
 	'tree!'		=> \$tree,
@@ -35,7 +70,7 @@ GetOptions(
 	'patch!'	=> \$chk_patch,
 	'emacs!'	=> \$emacs,
 	'terse!'	=> \$terse,
-	'file!'		=> \$file,
+	'f|file!'	=> \$file,
 	'subjective!'	=> \$check,
 	'strict!'	=> \$check,
 	'root=s'	=> \$root,
@@ -45,22 +80,16 @@ GetOptions(
 
 	'debug=s'	=> \%debug,
 	'test-only=s'	=> \$tst_only,
-) or exit;
+	'h|help'	=> \$help,
+	'version'	=> \$help
+) or help(1);
+
+help(0) if ($help);
 
 my $exit = 0;
 
 if ($#ARGV < 0) {
-	print "usage: $P [options] patchfile\n";
-	print "version: $V\n";
-	print "options: -q               => quiet\n";
-	print "         --no-tree        => run without a kernel tree\n";
-	print "         --terse          => one line per report\n";
-	print "         --emacs          => emacs compile window format\n";
-	print "         --file           => check a source file\n";
-	print "         --strict         => enable more subjective tests\n";
-	print "         --root           => path to the kernel tree root\n";
-	print "         --no-summary     => suppress the per-file summary\n";
-	print "         --summary-file   => include the filename in summary\n";
+	print "$P: no input files\n";
 	exit(1);
 }
 

Re: [PATCH] checkpatch: make -f alias --file, add --help, more verbose help message

[Andy Whitcroft]

On Tue, Mar 10, 2009 at 06:21:23PM +0100, Hannes Eder wrote:
> Impact:
>   - More verbose help/usage message.
>   - Make the option -f an alias for --file.
>   - On -h, --help, and --version display help message and exit(0).
>   - With no FILE(s) given, exit(1) with "no input files".
>   - On invalid options display help/usage and exit(1).
> 
> Based on a patch by Pavel Machek.

That looks reasonable.  Will integate it into my next set of updates.

-apw

Re: [PATCH] checkpatch: make -f alias --file, add --help, more verbose help message

[Andy Whitcroft]

On Tue, Mar 10, 2009 at 06:10:23PM +0000, Andy Whitcroft wrote:
> On Tue, Mar 10, 2009 at 06:21:23PM +0100, Hannes Eder wrote:
> > Impact:
> >   - More verbose help/usage message.
> >   - Make the option -f an alias for --file.
> >   - On -h, --help, and --version display help message and exit(0).
> >   - With no FILE(s) given, exit(1) with "no input files".
> >   - On invalid options display help/usage and exit(1).
> > 
> > Based on a patch by Pavel Machek.
> 
> That looks reasonable.  Will integate it into my next set of updates.

Ok, now in my testing branch.  See:

  http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-testing

-apw

Re: [PATCH] checkpatch: make -f alias --file, add --help, more verbose help message

[Pavel Machek]

On Tue 2009-03-10 18:21:23, Hannes Eder wrote:
> Impact:
>   - More verbose help/usage message.
>   - Make the option -f an alias for --file.
>   - On -h, --help, and --version display help message and exit(0).
>   - With no FILE(s) given, exit(1) with "no input files".
>   - On invalid options display help/usage and exit(1).
> 
> Based on a patch by Pavel Machek.
> 
> Signed-off-by: Hannes Eder <hannes@hanneseder.net>
> Cc: Pavel Machek <pavel@suse.cz>


ACK and thanks...

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: hp accelerometer: add freefall detection

[Éric Piel]

From: Pavel Machek <pavel@suse.cz>
> 
> This adds freefall handling to hp_accel driver.  According to HP, it
> should just work, without us having to set the chip up by hand.
> 
> hpfall.c is example .c program that parks the disk when accelerometer
> detects free fall.  It should work; for now, it uses fixed 20seconds
> protection period.
> 
> Signed-off-by: Pavel Machek <pavel@suse.cz>
> Cc: Thomas Renninger <trenn@suse.de>
> Cc: �ric Piel <eric.piel@tremplin-utc.net>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Hi Pavel,
Sorry for being amazingly late, reading the latest patches from
Giuseppe, I noticed some parts in this patch which are strange.
Basically you scrap all the init and all the poweroff. Some of the
things there were put for some purpose, and I wonder if you've verified
everything keeps working correctly.


> @@ -110,26 +114,13 @@ static void lis3lv02d_get_xyz(acpi_handl
>  void lis3lv02d_poweroff(acpi_handle handle)
>  {
>  	adev.is_on = 0;
> -	/* disable X,Y,Z axis and power down */
> -	adev.write(handle, CTRL_REG1, 0x00);
So we are not turning off the device on poweroff()?! Why? Did you notice
problems when we were doing it?

Actually I've always wondered if it was worthy to turn it off because
I've never been able to measure a difference in power consumption. Have
you done the measurement and noticed there is no effect? I'd be glad we
can get rid of this power management thing, but then we can scratch much
more than just this line!


> -	/*
> -	 * BDU: LSB and MSB values are not updated until both have been read.
> -	 *      So the value read will always be correct.
> -	 * IEN: Interrupt for free-fall and DD, not for data-ready.
> -	 */
> -	adev.read(handle, CTRL_REG2, &val);
> -	val |= CTRL2_BDU | CTRL2_IEN;
> -	adev.write(handle, CTRL_REG2, val);
This part is rather scary. Did you read the comment? If you don't
activate BDU you cannot be sure that the data you read is valid. I see
no reason why this would interfere with the free-fall detection so
unless you have a really good case, let's keep it. On the other hand, I
don't mind removing IEN if you've noticed that it's not useful or
interfering.

Eric

Re: hp accelerometer: add freefall detection

[Pavel Machek]

> From: Pavel Machek <pavel@suse.cz>
> > 
> > This adds freefall handling to hp_accel driver.  According to HP, it
> > should just work, without us having to set the chip up by hand.
> > 
> > hpfall.c is example .c program that parks the disk when accelerometer
> > detects free fall.  It should work; for now, it uses fixed 20seconds
> > protection period.
> > 
> > Signed-off-by: Pavel Machek <pavel@suse.cz>
> > Cc: Thomas Renninger <trenn@suse.de>
> > Cc: ???ric Piel <eric.piel@tremplin-utc.net>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> Hi Pavel,
> Sorry for being amazingly late, reading the latest patches from
> Giuseppe, I noticed some parts in this patch which are strange.
> Basically you scrap all the init and all the poweroff. Some of the
> things there were put for some purpose, and I wonder if you've verified
> everything keeps working correctly.

Yes; HP tells me I do not need to touch the hardware, and in my tests
touching the hardware actually broke the stuff. Yes, position and
freefall detection still worked without manually touching the hw.

Unfortunately, I don't have the hardware any more...

> > @@ -110,26 +114,13 @@ static void lis3lv02d_get_xyz(acpi_handl
> >  void lis3lv02d_poweroff(acpi_handle handle)
> >  {
> >  	adev.is_on = 0;
> > -	/* disable X,Y,Z axis and power down */
> > -	adev.write(handle, CTRL_REG1, 0x00);
> So we are not turning off the device on poweroff()?! Why? Did you notice
> problems when we were doing it?

Well, HP told me not to touch the hardware, so I'm not touching it.

> Actually I've always wondered if it was worthy to turn it off because
> I've never been able to measure a difference in power consumption. Have
> you done the measurement and noticed there is no effect? I'd be glad we
> can get rid of this power management thing, but then we can scratch much
> more than just this line!

Well,  if you can't measure the difference, it is probably not worth
doing ... for HP notebooks anyway.

> > -	/*
> > -	 * BDU: LSB and MSB values are not updated until both have been read.
> > -	 *      So the value read will always be correct.
> > -	 * IEN: Interrupt for free-fall and DD, not for data-ready.
> > -	 */
> > -	adev.read(handle, CTRL_REG2, &val);
> > -	val |= CTRL2_BDU | CTRL2_IEN;
> > -	adev.write(handle, CTRL_REG2, val);
> This part is rather scary. Did you read the comment? If you don't
> activate BDU you cannot be sure that the data you read is valid. I see
> no reason why this would interfere with the free-fall detection so
> unless you have a really good case, let's keep it. On the other hand, I
> don't mind removing IEN if you've noticed that it's not useful or
> interfering.

Well, I just removed all the writes at once and notice it
helped... And HP tells me I don't need to touch the hardware...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: hp accelerometer: add freefall detection

[Frans Pop]

Hi Pavel,

> +++ b/drivers/hwmon/lis3lv02d.c
[...]
> +static int lis3lv02d_misc_open(struct inode *inode, struct file *file)
[...]
> +       if (ret) {
> +               clear_bit(0, &adev.misc_opened);
> +               printk(KERN_ERR DRIVER_NAME ": IRQ%d allocation failed\n", ad
> +               return -EBUSY;
> +       }
> +       lis3lv02d_increase_use(&adev);
> +       printk("lis3: registered interrupt %d\n", adev.irq);

Is this a debug printk that should be removed altogether or should it
be given a proper format and KERN_DEBUG?

> +       return 0;
> +}
[...]
> @@ -268,6 +393,19 @@ int lis3lv02d_init_device(struct acpi_lis3lv02d
> *dev) if (lis3lv02d_joystick_enable())
>                 printk(KERN_ERR DRIVER_NAME ": joystick initialization failed
>
> +       printk("lis3_init_device: irq %d\n", dev->irq);

Same here.

> +
> +       /* if we did not get an IRQ from ACPI - we have nothing more to do */
> +       if (!dev->irq) { 
> +               printk(KERN_ERR DRIVER_NAME
> +                       ": No IRQ in ACPI. Disabling /dev/freefall\n");
> +               goto out;
> +       }
> +
> +       printk("lis3: registering device\n");

And here.

> +       if (misc_register(&lis3lv02d_misc_device))
> +               printk(KERN_ERR DRIVER_NAME ": misc_register failed\n");

Cheers,
FJP

Re: hp accelerometer: add freefall detection

[Pavel Machek]

On Sat 2009-02-14 17:01:01, Frans Pop wrote:
> Hi Pavel,
> 
> > +++ b/drivers/hwmon/lis3lv02d.c
> [...]
> > +static int lis3lv02d_misc_open(struct inode *inode, struct file *file)
> [...]
> > +       if (ret) {
> > +               clear_bit(0, &adev.misc_opened);
> > +               printk(KERN_ERR DRIVER_NAME ": IRQ%d allocation failed\n", ad
> > +               return -EBUSY;
> > +       }
> > +       lis3lv02d_increase_use(&adev);
> > +       printk("lis3: registered interrupt %d\n", adev.irq);
> 
> Is this a debug printk that should be removed altogether or should it
> be given a proper format and KERN_DEBUG?

It should be deleted, yes. I'll prepare the patch once previous
changes are merged.
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html