Skip to contentSkip to conversation

Apple Motion Sensor driver

PATCH New Michael Hanselmann 7 participants 29 messages Aug 2, 2006 – Aug 10, 2006 Hardware monitoring

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Apple Motion Sensor driver

This driver adds support for the Apple Motion Sensor (AMS) as found in 2005 revisions of Apple PowerBooks and iBooks. It implements both the PMU and I?C variants. The I?C driver and mouse emulation is based on code by Stelian Pop <stelian at popies.net>, while the PMU driver has been developped by myself. HD parking support will be added later.

Please apply to -mm for testing.

Signed-off-by Michael Hanselmann <linux-kernel at hansmi.ch>

---

drivers/hwmon/ams/ams-core.c +255 -0
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc1-git10.orig/drivers/hwmon/ams/ams-core.c linux-2.6.18-rc1-git10/drivers/hwmon/ams/ams-core.c
@@ -0,0 +1,255 @@
+/*
+ * Apple Motion Sensor driver
+ *
+ * Copyright (C) 2005 Stelian Pop (stelian at popies.net)
+ * Copyright (C) 2006 Michael Hanselmann (linux-kernel at hansmi.ch)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ */
+
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <asm/pmac_pfunc.h>
+
+#include "ams.h"
+
+/* There is only one motion sensor per machine */
+struct ams ams;
+
+static unsigned int verbose;
+module_param(verbose, bool, 0644);
+MODULE_PARM_DESC(verbose, "Show free falls and shocks in kernel output");
+
+/* Call with lock held! */
+void ams_sensors(s8 *x, s8 *y, s8 *z)
+{
+	u32 orient = ams.vflag? ams.orient1 : ams.orient2;
+
+	if (orient & 0x80)
+		/* X and Y swapped */
+		ams.get_xyz(y, x, z);
+	else
+		ams.get_xyz(x, y, z);
+
+	if (orient & 0x04)
+		*z = ~(*z);
+	if (orient & 0x02)
+		*y = ~(*y);
+	if (orient & 0x01)
+		*x = ~(*x);
+}
+
+static ssize_t ams_show_current(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	s8 x, y, z;
+
+	mutex_lock(&ams.lock);
+	ams_sensors(&x, &y, &z);
+	mutex_unlock(&ams.lock);
+
+	return snprintf(buf, PAGE_SIZE, "%d %d %d\n", x, y, z);
+}
+
+static DEVICE_ATTR(current, S_IRUGO, ams_show_current, NULL);
+
+static void ams_handle_irq(void *data)
+{
+	enum ams_irq irq = *((enum ams_irq *)data);
+
+	spin_lock(&ams.irq_lock);
+
+	ams.worker_irqs |= irq;
+	schedule_work(&ams.worker);
+
+	spin_unlock(&ams.irq_lock);
+}
+
+static enum ams_irq ams_freefall_irq_data = AMS_IRQ_FREEFALL;
+static struct pmf_irq_client ams_freefall_client = {
+	.owner = THIS_MODULE,
+	.handler = ams_handle_irq,
+	.data = &ams_freefall_irq_data,
+};
+
+static enum ams_irq ams_shock_irq_data = AMS_IRQ_SHOCK;
+static struct pmf_irq_client ams_shock_client = {
+	.owner = THIS_MODULE,
+	.handler = ams_handle_irq,
+	.data = &ams_shock_irq_data,
+};
+
+/* Once hard disk parking is implemented in the kernel, this function can
+ * trigger it.
+ */
+static void ams_worker(void *data)
+{
+	mutex_lock(&ams.lock);
+
+	if (ams.has_device) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&ams.irq_lock, flags);
+
+		if (ams.worker_irqs & AMS_IRQ_FREEFALL) {
+			if (verbose)
+				printk(KERN_INFO "ams: freefall detected!\n");
+
+			ams.worker_irqs &= ~AMS_IRQ_FREEFALL;
+
+			/* we must call this with interrupts enabled */
+			spin_unlock_irqrestore(&ams.irq_lock, flags);
+			ams.clear_irq(AMS_IRQ_FREEFALL);
+			spin_lock_irqsave(&ams.irq_lock, flags);
+		}
+
+		if (ams.worker_irqs & AMS_IRQ_SHOCK) {
+			if (verbose)
+				printk(KERN_INFO "ams: shock detected!\n");
+
+			ams.worker_irqs &= ~AMS_IRQ_SHOCK;
+
+			/* we must call this with interrupts enabled */
+			spin_unlock_irqrestore(&ams.irq_lock, flags);
+			ams.clear_irq(AMS_IRQ_SHOCK);
+			spin_lock_irqsave(&ams.irq_lock, flags);
+		}
+
+		spin_unlock_irqrestore(&ams.irq_lock, flags); 
+	}
+
+	mutex_unlock(&ams.lock);
+}
+
+/* Call with lock held! */
+int ams_sensor_attach(void)
+{
+	int result;
+	u32 *prop;
+
+	/* Get orientation */
+	prop = (u32*)get_property(ams.of_node, "orientation", NULL);
+	if (!prop)
+		return -ENODEV;
+	ams.orient1 = *prop;
+	ams.orient2 = *(prop + 1);
+
+	/* Register freefall interrupt handler */
+	result = pmf_register_irq_client(ams.of_node,
+			"accel-int-1",
+			&ams_freefall_client);
+	if (result < 0)
+		return -ENODEV;
+
+	/* Reset saved irqs */
+	ams.worker_irqs = 0;
+
+	/* Register shock interrupt handler */
+	result = pmf_register_irq_client(ams.of_node,
+			"accel-int-2",
+			&ams_shock_client);
+	if (result < 0) {
+		pmf_unregister_irq_client(&ams_freefall_client);
+		return -ENODEV;
+	}
+
+	/* Create device */
+	ams.of_dev = of_platform_device_create(ams.of_node, "ams", NULL);
+	if (!ams.of_dev) {
+		pmf_unregister_irq_client(&ams_shock_client);
+		pmf_unregister_irq_client(&ams_freefall_client);
+		return -ENODEV;
+	}
+
+	/* Create attributes */
+	device_create_file(&ams.of_dev->dev, &dev_attr_current);
+
+	ams.vflag = !!(ams.get_vendor() & 0x10);
+
+	/* Init mouse device */
+	ams_mouse_init();
+
+	return 0;
+}
+
+int __init ams_init(void)
+{
+	struct device_node *np;
+
+	spin_lock_init(&ams.irq_lock);
+	mutex_init(&ams.lock);
+	INIT_WORK(&ams.worker, ams_worker, NULL);
+
+#ifdef CONFIG_SENSORS_AMS_I2C
+	np = of_find_node_by_name(NULL, "accelerometer");
+	if (np && device_is_compatible(np, "AAPL,accelerometer_1"))
+		/* Found I2C motion sensor */
+		return ams_i2c_init(np);
+#endif
+
+#ifdef CONFIG_SENSORS_AMS_PMU
+	np = of_find_node_by_name(NULL, "sms");
+	if (np && device_is_compatible(np, "sms"))
+		/* Found PMU motion sensor */
+		return ams_pmu_init(np);
+#endif
+
+	printk(KERN_ERR "ams: No motion sensor found.\n");
+
+	return -ENODEV;
+}
+
+void ams_exit(void)
+{
+	mutex_lock(&ams.lock);
+
+	if (ams.has_device) {
+		/* Remove mouse device */
+		ams_mouse_exit();
+
+		/* Shut down implementation */
+		ams.exit();
+
+		/* Cancel interrupt worker
+		 *
+		 * We do this after ams.exit(), because an interrupt might
+		 * have arrived before disabling them.
+		 */
+		cancel_delayed_work(&ams.worker);
+		flush_scheduled_work();
+
+		/* Remove attributes */
+		device_remove_file(&ams.of_dev->dev, &dev_attr_current);
+
+		/* Remove device */
+		of_device_unregister(ams.of_dev);
+
+		/* Remove handler */
+		pmf_unregister_irq_client(&ams_shock_client);
+		pmf_unregister_irq_client(&ams_freefall_client);
+	}
+
+	mutex_unlock(&ams.lock);
+}
+
+MODULE_AUTHOR("Stelian Pop, Michael Hanselmann");
+MODULE_DESCRIPTION("Apple Motion Sensor driver");
+MODULE_LICENSE("GPL");
+
+module_init(ams_init);
+module_exit(ams_exit);
drivers/hwmon/ams/ams.h +71 -0

71 lines not on this page · raw message

drivers/hwmon/ams/ams-i2c.c +289 -0

289 lines not on this page · raw message

drivers/hwmon/ams/ams-mouse.c +145 -0

145 lines not on this page · raw message

drivers/hwmon/ams/ams-pmu.c +202 -0

202 lines not on this page · raw message

drivers/hwmon/ams/Makefile +8 -0
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc1-git10.orig/drivers/hwmon/ams/Makefile linux-2.6.18-rc1-git10/drivers/hwmon/ams/Makefile
@@ -0,0 +1,8 @@
+#
+# Makefile for Apple Motion Sensor driver
+#
+
+ams-y					:= ams-core.o ams-mouse.o
+ams-$(CONFIG_SENSORS_AMS_PMU)		+= ams-pmu.o
+ams-$(CONFIG_SENSORS_AMS_I2C)		+= ams-i2c.o
+obj-$(CONFIG_SENSORS_AMS)		+= ams.o
drivers/hwmon/Kconfig +23 -0
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc1-git10.orig/drivers/hwmon/Kconfig linux-2.6.18-rc1-git10/drivers/hwmon/Kconfig
@@ -94,6 +94,29 @@ config SENSORS_ADM9240
 	  This driver can also be built as a module.  If so, the module
 	  will be called adm9240.
 
+config SENSORS_AMS
+	tristate "Apple Motion Sensor driver"
+	depends on HWMON && PPC_PMAC && INPUT && EXPERIMENTAL
+	default y
+	help
+	  Support for the motion sensor included in PowerBooks. Includes
+	  implementations for PMU and I2C.
+
+config SENSORS_AMS_PMU
+	bool "PMU variant"
+	depends on SENSORS_AMS && ADB_PMU
+	default y
+	help
+	  PMU variant of motion sensor, found in late 2005 PowerBooks.
+
+config SENSORS_AMS_I2C
+	bool "I2C variant"
+	depends on SENSORS_AMS && I2C
+	default y
+	help
+	  I2C variant of motion sensor, found in early 2005 PowerBooks and
+	  iBooks.
+
 config SENSORS_ASB100
 	tristate "Asus ASB100 Bach"
 	depends on HWMON && I2C && EXPERIMENTAL
drivers/hwmon/Makefile +1 -0
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc1-git10.orig/drivers/hwmon/Makefile linux-2.6.18-rc1-git10/drivers/hwmon/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_SENSORS_ADM1025)	+= adm1025
 obj-$(CONFIG_SENSORS_ADM1026)	+= adm1026.o
 obj-$(CONFIG_SENSORS_ADM1031)	+= adm1031.o
 obj-$(CONFIG_SENSORS_ADM9240)	+= adm9240.o
+obj-$(CONFIG_SENSORS_AMS)	+= ams/
 obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
 obj-$(CONFIG_SENSORS_DS1621)	+= ds1621.o
 obj-$(CONFIG_SENSORS_F71805F)	+= f71805f.o
#1 lore raw

PATCH 1/1 Apple Motion Sensor driver

Quoted 5 lines · On Wed, 2 Aug 2006 21:15:20 +0200 Michael Hanselmann <linux-kernel at…

This driver adds support for the Apple Motion Sensor (AMS) as found in 2005 revisions of Apple PowerBooks and iBooks. It implements both the PMU and I?C variants. The I?C driver and mouse emulation is based on code by Stelian Pop <stelian at popies.net>, while the PMU driver has been developped by myself. HD parking support will be added later.

Confused. What is the relatioship between this code and ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.18-rc2/2.6.18-rc2-mm1/broken-out/apple-motion-sensor-driver.patch ?

#2 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann
Quoted 9 lines · Andrew Morton
Quoted 5 lines · On Wed, 2 Aug 2006 21:15:20 +0200 Michael Hanselmann <linux-kernel at…

This driver adds support for the Apple Motion Sensor (AMS) as found in 2005 revisions of Apple PowerBooks and iBooks. It implements both the PMU and I?C variants. The I?C driver and mouse emulation is based on code by Stelian Pop <stelian at popies.net>, while the PMU driver has been developped by myself. HD parking support will be added later.

Quoted 3 lines

Confused. What is the relatioship between this code and ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.18-rc2/2.6.18-rc2-mm1/broken-out/apple-motion-sensor-driver.patch ?

You already asked that on July 2 in <20060702201415.791c6eb2.akpm at osdl.org>. My driver replaces the patch from Stelian and includes all of his code.

Here's the mail conversation back then:

Quoted 13 lines · Michael Hanselmann
Quoted 11 lines · Andrew Morton
Quoted 1 line · On Mon, 3 Jul 2006 00:26:49 +0200 Michael Hanselmann <linux-kernel at…

Below you find the latest revision of my AMS driver.

I was about to merge the below, then this comes along. Now what?

From: Stelian Pop <stelian at popies.net>

This driver provides support for the Apple Motion Sensor (ams), which provides an accelerometer and other misc. data. Some Apple PowerBooks

Quoted 7 lines

I just noticed yesterday that Stelian sent a patch to lkml in May. My work is based on his separate driver from his website.

Given the fact that my driver includes all of his functionality and that replacing his with mine later in the process would mean to remove whole files again, I'd suggest to wait until I've fixed the outstanding issues (as seen in this thread) and then to merge mine.

This was approved by Stelian and yourself. So ? I think you can just replace Stelian's patch with mine.

Thanks,
Michael

#3 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann
Quoted 10 lines · On Thu, 3 Aug 2006 08:53:33 +0200 Michael Hanselmann <linux-kernel at…
Quoted 8 lines · Andrew Morton
Quoted 5 lines · On Wed, 2 Aug 2006 21:15:20 +0200 Michael Hanselmann <linux-kernel at…

This driver adds support for the Apple Motion Sensor (AMS) as found in 2005 revisions of Apple PowerBooks and iBooks. It implements both the PMU and I?C variants. The I?C driver and mouse emulation is based on code by Stelian Pop <stelian at popies.net>, while the PMU driver has been developped by myself. HD parking support will be added later.

Quoted 6 lines
Quoted 3 lines

Confused. What is the relatioship between this code and ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.18-rc2/2.6.18-rc2-mm1/broken-out/apple-motion-sensor-driver.patch ?

You already asked that on July 2 in
<20060702201415.791c6eb2.akpm at osdl.org>.

I thought it was such a good question that I'd use it again.

Quoted 2 lines

My driver replaces the patch
from Stelian and includes all of his code.

Stelian, can we have a signed-off-by:, please?

Also, the previous patch had an s-o-b from Johannes. Does this version need one too?

#4 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann

drivers/hwmon/ams/ams-core.c line 32

Quoted 10 lines · On Wed, 2 Aug 2006 21:15:20 +0200 Michael Hanselmann <linux-kernel at…

This driver adds support for the Apple Motion Sensor (AMS) as found in 2005 revisions of Apple PowerBooks and iBooks. It implements both the PMU and I?C variants. The I?C driver and mouse emulation is based on code by Stelian Pop <stelian at popies.net>, while the PMU driver has been developped by myself. HD parking support will be added later.

...

drivers/hwmon/ams/ams-core.c +2 -0
+/* There is only one motion sensor per machine */
+struct ams ams;

That's a somewhat risky name for a global variable.

Then again, nobody else is likely to be so bold as to add another three-character global ;)

drivers/hwmon/ams/ams-core.c line 140

Quoted 2 lines
drivers/hwmon/ams/ams-core.c +2 -0
+/* Call with lock held! */
+int ams_sensor_attach(void)

Which lock?

Quoted 4 lines
drivers/hwmon/ams/ams-i2c.c +4 -0
+static int ams_i2c_cmd(enum ams_i2c_cmd cmd)
+{
+	s32 result;
+	int i;

drivers/hwmon/ams/ams-i2c.c line 98

Quoted 10 lines
drivers/hwmon/ams/ams-i2c.c +10 -0
+
+	ams_i2c_write(AMS_COMMAND, cmd);
+	for (i = 0; i < 10; i++) {
+		mdelay(5);
+		result = ams_i2c_read(AMS_COMMAND);
+		if (result = 0 || result & 0x80)
+			return 0;
+	}
+	return -1;
+}

That's a potentially very long busy wait.

Quoted 13 lines
drivers/hwmon/ams/ams-i2c.c +13 -0
+static void ams_i2c_set_irq(enum ams_irq reg, char enable)
+{
+	if (reg & AMS_IRQ_FREEFALL) {
+		u8 val = ams_i2c_read(AMS_CTRLX);
+		if (enable)     val |= 0x80;
+		else            val &= ~0x80;
+		ams_i2c_write(AMS_CTRLX, val);
+	}
+
+	if (reg & AMS_IRQ_SHOCK) {
+		u8 val = ams_i2c_read(AMS_CTRLY);
+		if (enable)     val |= 0x80;
+		else            val &= ~0x80;

drivers/hwmon/ams/ams-i2c.c line 122

Quoted 10 lines
drivers/hwmon/ams/ams-i2c.c +10 -0
+		ams_i2c_write(AMS_CTRLY, val);
+	}
+
+	if (reg & AMS_IRQ_GLOBAL) {
+		u8 val = ams_i2c_read(AMS_CTRLZ);
+		if (enable)     val |= 0x80;
+		else            val &= ~0x80;
+		ams_i2c_write(AMS_CTRLZ, val);
+	}
+}

Please, let's use

	if (a)
		b;
	else
		c;

drivers/hwmon/ams/ams-mouse.c line 50

Quoted 2 lines
drivers/hwmon/ams/ams-mouse.c +2 -0
+/* Call with lock held! */
+static void ams_mouse_enable(void)

Which lock?

Quoted 28 lines
drivers/hwmon/ams/ams-mouse.c +28 -0
+{
+	s8 x, y, z;
+
+	if (ams.idev)
+		return;
+
+	ams_sensors(&x, &y, &z);
+	ams.xcalib = x;
+	ams.ycalib = y;
+	ams.zcalib = z;
+
+	ams.idev = input_allocate_device();
+	if (!ams.idev)
+		return;
+
+	ams.idev->name = "Apple Motion Sensor";
+	ams.idev->id.bustype = ams.bustype;
+	ams.idev->id.vendor = 0;
+	ams.idev->cdev.dev = &ams.of_dev->dev;
+
+	input_set_abs_params(ams.idev, ABS_X, -50, 50, 3, 0);
+	input_set_abs_params(ams.idev, ABS_Y, -50, 50, 3, 0);
+	input_set_abs_params(ams.idev, ABS_Z, -50, 50, 3, 0);
+
+	set_bit(EV_ABS, ams.idev->evbit);
+	set_bit(EV_KEY, ams.idev->evbit);
+
+	if (input_register_device(ams.idev)) {

drivers/hwmon/ams/ams-mouse.c line 88

Quoted 10 lines
drivers/hwmon/ams/ams-mouse.c +10 -0
+		input_free_device(ams.idev);
+		ams.idev = NULL;
+		return;
+	}
+
+	ams.kthread = kthread_run(ams_mouse_kthread, NULL, "kams");
+	if (IS_ERR(ams.kthread)) {
+		input_unregister_device(ams.idev);
+		ams.idev = NULL;
+		return;

Didn't we just leak ams.idev? Or does the disconnect handler handle that?

<goes to input_unregister_device() for the API description, comes away discouraged>

Quoted 7 lines
drivers/hwmon/ams/ams-mouse.c +7 -0
+		ams.idev = NULL;
+	}
+}
+
+static ssize_t ams_mouse_show_mouse(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{

drivers/hwmon/ams/ams-mouse.c line 114

Quoted 10 lines
drivers/hwmon/ams/ams-mouse.c +10 -0
+	return sprintf(buf, "%d\n", mouse);
+}
+
+static ssize_t ams_mouse_store_mouse(struct device *dev,
+	struct device_attribute *attr, const char *buf, size_t count)
+{
+	if (sscanf(buf, "%d\n", &mouse) != 1)
+		return -EINVAL;
+
+	mouse = !!mouse;

That statement isn't needed.

Quoted 28 lines
drivers/hwmon/ams/ams-pmu.c +25 -0
+	mutex_lock(&ams.lock);
+
+	if (mouse)
+		ams_mouse_enable();
+	else
+		ams_mouse_disable();
+
+	mutex_unlock(&ams.lock);
+
+	return count;
+}
 
...
 
+/* Enables or disables the specified interrupts */
+static void ams_pmu_set_irq(enum ams_irq reg, char enable)
+{
+	if (reg & AMS_IRQ_FREEFALL) {
+		u8 val = ams_pmu_get_register(AMS_FF_ENABLE);
+		if (enable)	val |= 0x80;
+		else		val &= ~0x80;
+		ams_pmu_set_register(AMS_FF_ENABLE, val);
+	}
+
+	if (reg & AMS_IRQ_SHOCK) {
+		u8 val = ams_pmu_get_register(AMS_SHOCK_ENABLE);
+		if (enable)	val |= 0x80;
+		else		val &= ~0x80;

drivers/hwmon/ams/ams-pmu.c line 105

Quoted 10 lines
drivers/hwmon/ams/ams-pmu.c +10 -0
+		ams_pmu_set_register(AMS_SHOCK_ENABLE, val);
+	}
+
+	if (reg & AMS_IRQ_GLOBAL) {
+		u8 val = ams_pmu_get_register(AMS_CONTROL);
+		if (enable)	val |= 0x80;
+		else		val &= ~0x80;
+		ams_pmu_set_register(AMS_CONTROL, val);
+	}
+}

Again - coding style is wrong.

drivers/hwmon/ams/ams-pmu.c line 142

Quoted 2 lines
drivers/hwmon/ams/ams-pmu.c +2 -0
+/* Call with lock held! */
+int __init ams_pmu_init(struct device_node *np)

Which lock??

drivers/hwmon/ams/ams-pmu.c line 147

Quoted 5 lines
drivers/hwmon/ams/ams-pmu.c +5 -0
+{
+	u32 *prop;
+	int result;
+
+	mutex_lock(&ams.lock);

Obviously not that one...

#5 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann
Quoted 4 lines · Andrew Morton

Review

Quoted 2 lines
(quoted diff) +2 -0
+/* There is only one motion sensor per machine */
+struct ams ams;

That's a somewhat risky name for a global variable.

Changed it to ams_info, which should be a bit more unique.

Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+/* Call with lock held! */
+int ams_sensor_attach(void)

Which lock?

ams_info.lock, changed all such comments

Quoted 6 lines
Quoted 4 lines
(quoted diff) +2 -0
+static int ams_i2c_cmd(enum ams_i2c_cmd cmd)
+{

[?]
+}

That's a potentially very long busy wait.

I would like if Aristeu or Stelian could fix this, because I don't own the I?C hardware myself. Thus I left it untouched in the patch below.

Quoted 7 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+	ams.kthread = kthread_run(ams_mouse_kthread, NULL, "kams");
+	if (IS_ERR(ams.kthread)) {
+		input_unregister_device(ams.idev);
+		ams.idev = NULL;
+		return;

Didn't we just leak ams.idev? Or does the disconnect handler handle that?

Indeed we do. Fixed.

Signed-off-by Michael Hanselmann <linux-kernel at hansmi.ch>

---

drivers/hwmon/ams/ams-core.c +48 -48
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc3.orig/drivers/hwmon/ams/ams-core.c linux-2.6.18-rc3/drivers/hwmon/ams/ams-core.c
@@ -29,22 +29,22 @@
 #include "ams.h"
 
 /* There is only one motion sensor per machine */
-struct ams ams;
+struct ams ams_info;
 
 static unsigned int verbose;
 module_param(verbose, bool, 0644);
 MODULE_PARM_DESC(verbose, "Show free falls and shocks in kernel output");
 
-/* Call with lock held! */
+/* Call with ams_info.lock held! */
 void ams_sensors(s8 *x, s8 *y, s8 *z)
 {
-	u32 orient = ams.vflag? ams.orient1 : ams.orient2;
+	u32 orient = ams_info.vflag? ams_info.orient1 : ams_info.orient2;
 
 	if (orient & 0x80)
 		/* X and Y swapped */
-		ams.get_xyz(y, x, z);
+		ams_info.get_xyz(y, x, z);
 	else
-		ams.get_xyz(x, y, z);
+		ams_info.get_xyz(x, y, z);
 
 	if (orient & 0x04)
 		*z = ~(*z);
@@ -59,9 +59,9 @@ static ssize_t ams_show_current(struct d
 {
 	s8 x, y, z;
 
-	mutex_lock(&ams.lock);
+	mutex_lock(&ams_info.lock);
 	ams_sensors(&x, &y, &z);
-	mutex_unlock(&ams.lock);
+	mutex_unlock(&ams_info.lock);
 
 	return snprintf(buf, PAGE_SIZE, "%d %d %d\n", x, y, z);
 }
@@ -72,12 +72,12 @@ static void ams_handle_irq(void *data)
 {
 	enum ams_irq irq = *((enum ams_irq *)data);
 
-	spin_lock(&ams.irq_lock);
+	spin_lock(&ams_info.irq_lock);
 
-	ams.worker_irqs |= irq;
-	schedule_work(&ams.worker);
+	ams_info.worker_irqs |= irq;
+	schedule_work(&ams_info.worker);
 
-	spin_unlock(&ams.irq_lock);
+	spin_unlock(&ams_info.irq_lock);
 }
 
 static enum ams_irq ams_freefall_irq_data = AMS_IRQ_FREEFALL;
@@ -99,68 +99,68 @@ static struct pmf_irq_client ams_shock_c
  */
 static void ams_worker(void *data)
 {
-	mutex_lock(&ams.lock);
+	mutex_lock(&ams_info.lock);
 
-	if (ams.has_device) {
+	if (ams_info.has_device) {
 		unsigned long flags;
 
-		spin_lock_irqsave(&ams.irq_lock, flags);
+		spin_lock_irqsave(&ams_info.irq_lock, flags);
 
-		if (ams.worker_irqs & AMS_IRQ_FREEFALL) {
+		if (ams_info.worker_irqs & AMS_IRQ_FREEFALL) {
 			if (verbose)
 				printk(KERN_INFO "ams: freefall detected!\n");
 
-			ams.worker_irqs &= ~AMS_IRQ_FREEFALL;
+			ams_info.worker_irqs &= ~AMS_IRQ_FREEFALL;
 
 			/* we must call this with interrupts enabled */
-			spin_unlock_irqrestore(&ams.irq_lock, flags);
-			ams.clear_irq(AMS_IRQ_FREEFALL);
-			spin_lock_irqsave(&ams.irq_lock, flags);
+			spin_unlock_irqrestore(&ams_info.irq_lock, flags);
+			ams_info.clear_irq(AMS_IRQ_FREEFALL);
+			spin_lock_irqsave(&ams_info.irq_lock, flags);
 		}
 
-		if (ams.worker_irqs & AMS_IRQ_SHOCK) {
+		if (ams_info.worker_irqs & AMS_IRQ_SHOCK) {
 			if (verbose)
 				printk(KERN_INFO "ams: shock detected!\n");
 
-			ams.worker_irqs &= ~AMS_IRQ_SHOCK;
+			ams_info.worker_irqs &= ~AMS_IRQ_SHOCK;
 
 			/* we must call this with interrupts enabled */
-			spin_unlock_irqrestore(&ams.irq_lock, flags);
-			ams.clear_irq(AMS_IRQ_SHOCK);
-			spin_lock_irqsave(&ams.irq_lock, flags);
+			spin_unlock_irqrestore(&ams_info.irq_lock, flags);
+			ams_info.clear_irq(AMS_IRQ_SHOCK);
+			spin_lock_irqsave(&ams_info.irq_lock, flags);
 		}
 
-		spin_unlock_irqrestore(&ams.irq_lock, flags);
+		spin_unlock_irqrestore(&ams_info.irq_lock, flags);
 	}
 
-	mutex_unlock(&ams.lock);
+	mutex_unlock(&ams_info.lock);
 }
 
-/* Call with lock held! */
+/* Call with ams_info.lock held! */
 int ams_sensor_attach(void)
 {
 	int result;
 	u32 *prop;
 
 	/* Get orientation */
-	prop = (u32*)get_property(ams.of_node, "orientation", NULL);
+	prop = (u32*)get_property(ams_info.of_node, "orientation", NULL);
 	if (!prop)
 		return -ENODEV;
-	ams.orient1 = *prop;
-	ams.orient2 = *(prop + 1);
+	ams_info.orient1 = *prop;
+	ams_info.orient2 = *(prop + 1);
 
 	/* Register freefall interrupt handler */
-	result = pmf_register_irq_client(ams.of_node,
+	result = pmf_register_irq_client(ams_info.of_node,
 			"accel-int-1",
 			&ams_freefall_client);
 	if (result < 0)
 		return -ENODEV;
 
 	/* Reset saved irqs */
-	ams.worker_irqs = 0;
+	ams_info.worker_irqs = 0;
 
 	/* Register shock interrupt handler */
-	result = pmf_register_irq_client(ams.of_node,
+	result = pmf_register_irq_client(ams_info.of_node,
 			"accel-int-2",
 			&ams_shock_client);
 	if (result < 0) {
@@ -169,17 +169,17 @@ int ams_sensor_attach(void)
 	}
 
 	/* Create device */
-	ams.of_dev = of_platform_device_create(ams.of_node, "ams", NULL);
-	if (!ams.of_dev) {
+	ams_info.of_dev = of_platform_device_create(ams_info.of_node, "ams", NULL);
+	if (!ams_info.of_dev) {
 		pmf_unregister_irq_client(&ams_shock_client);
 		pmf_unregister_irq_client(&ams_freefall_client);
 		return -ENODEV;
 	}
 
 	/* Create attributes */
-	device_create_file(&ams.of_dev->dev, &dev_attr_current);
+	device_create_file(&ams_info.of_dev->dev, &dev_attr_current);
 
-	ams.vflag = !!(ams.get_vendor() & 0x10);
+	ams_info.vflag = !!(ams_info.get_vendor() & 0x10);
 
 	/* Init mouse device */
 	ams_mouse_init();
@@ -191,9 +191,9 @@ int __init ams_init(void)
 {
 	struct device_node *np;
 
-	spin_lock_init(&ams.irq_lock);
-	mutex_init(&ams.lock);
-	INIT_WORK(&ams.worker, ams_worker, NULL);
+	spin_lock_init(&ams_info.irq_lock);
+	mutex_init(&ams_info.lock);
+	INIT_WORK(&ams_info.worker, ams_worker, NULL);
 
 #ifdef CONFIG_SENSORS_AMS_I2C
 	np = of_find_node_by_name(NULL, "accelerometer");
@@ -216,35 +216,35 @@ int __init ams_init(void)
 
 void ams_exit(void)
 {
-	mutex_lock(&ams.lock);
+	mutex_lock(&ams_info.lock);
 
-	if (ams.has_device) {
+	if (ams_info.has_device) {
 		/* Remove mouse device */
 		ams_mouse_exit();
 
 		/* Shut down implementation */
-		ams.exit();
+		ams_info.exit();
 
 		/* Cancel interrupt worker
 		 *
-		 * We do this after ams.exit(), because an interrupt might
+		 * We do this after ams_info.exit(), because an interrupt might
 		 * have arrived before disabling them.
 		 */
-		cancel_delayed_work(&ams.worker);
+		cancel_delayed_work(&ams_info.worker);
 		flush_scheduled_work();
 
 		/* Remove attributes */
-		device_remove_file(&ams.of_dev->dev, &dev_attr_current);
+		device_remove_file(&ams_info.of_dev->dev, &dev_attr_current);
 
 		/* Remove device */
-		of_device_unregister(ams.of_dev);
+		of_device_unregister(ams_info.of_dev);
 
 		/* Remove handler */
 		pmf_unregister_irq_client(&ams_shock_client);
 		pmf_unregister_irq_client(&ams_freefall_client);
 	}
 
-	mutex_unlock(&ams.lock);
+	mutex_unlock(&ams_info.lock);
 }
 
 MODULE_AUTHOR("Stelian Pop, Michael Hanselmann");
drivers/hwmon/ams/ams.h +1 -1
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc3.orig/drivers/hwmon/ams/ams.h linux-2.6.18-rc3/drivers/hwmon/ams/ams.h
@@ -60,7 +60,7 @@ struct ams {
 	int xcalib, ycalib, zcalib;
 };
 
-extern struct ams ams;
+extern struct ams ams_info;
 
 extern void ams_sensors(s8 *x, s8 *y, s8 *z);
 extern int ams_sensor_attach(void);
drivers/hwmon/ams/ams-i2c.c +36 -30

146 lines not on this page · raw message

drivers/hwmon/ams/ams-mouse.c +44 -45

151 lines not on this page · raw message

drivers/hwmon/ams/ams-pmu.c +23 -18

88 lines not on this page · raw message

#6 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann
Quoted 2 lines

I would like if Aristeu or Stelian could fix this, because I don't own the I??C hardware myself. Thus I left it untouched in the patch below.

will take a look on this tonight

Signature · 1 line
Aristeu
#7 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann
Quoted 3 lines · Andrew Morton

Also, the previous patch had an s-o-b from Johannes. Does this version need one too?

I didn't really write much code in there, but for any that I might have written I can assure that it's ok to use:

Signed-off-by Johannes Berg <johannes at sipsolutions.net>
#9 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann
Quoted 4 lines
Quoted 1 line

That's a potentially very long busy wait.

I would like if Aristeu or Stelian could fix this, because I don't own the I??C hardware myself. Thus I left it untouched in the patch below.

what about the attached patch? Running right now a kernel with the original patch + Michael fixes + this one and no problems so far.

Signature · 21 lines
Aristeu

-------------- next part --------------
Index: ppc-2.6/drivers/hwmon/ams/ams-i2c.c
=================================--- ppc-2.6.orig/drivers/hwmon/ams/ams-i2c.c	2006-08-04 10:47:57.000000000 -0300
+++ ppc-2.6/drivers/hwmon/ams/ams-i2c.c	2006-08-04 10:48:26.000000000 -0300
@@ -85,11 +85,11 @@
 static int ams_i2c_cmd(enum ams_i2c_cmd cmd)
 {
 	s32 result;
-	int i;
+	int remaining = HZ/20;
 
 	ams_i2c_write(AMS_COMMAND, cmd);
-	for (i = 0; i < 10; i++) {
-		mdelay(5);
+	while (remaining) {
+		remaining = schedule_timeout(remaining);
 		result = ams_i2c_read(AMS_COMMAND);
 		if (result = 0 || result & 0x80)
 			return 0;
#10 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann

Review

Quoted 5 lines · Aristeu Sergio Rozanski Filho

what about the attached patch?

(quoted diff) +0 -2
 	ams_i2c_write(AMS_COMMAND, cmd);
-	for (i = 0; i < 10; i++) {
-		mdelay(5);

This guarantees a minimum wait time of 5msec after writing the value. Looking at the specs[1], this should be done. Your code doesn't do that:

Review

Quoted 5 lines
(quoted diff) +2 -0
+	while (remaining) {
+		remaining = schedule_timeout(remaining);
 		result = ams_i2c_read(AMS_COMMAND);
 		if (result = 0 || result & 0x80)
 			return 0;

I'm not sure wether the 5msec delay is really required, but I would insert it if it's in the specs.

So far, NACK until we got that right.

Next time, please insert the patch inline.

Thanks,
Michael

[1] http://johannes.sipsolutions.net/PowerBook/Apple_Motion_Sensor_Specification/#head-cc4be6b93ad9fcb8d373ad340b12891ed920a05f

#11 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann
Quoted 2 lines

This guarantees a minimum wait time of 5msec after writing the value. Looking at the specs[1], this should be done. Your code doesn't do that:

I doubt that if it's possible to retrying after a fail the hardware will bother if we try to do the first read in less than 5ms... but here it goes. two versions, feel free to choose.

Index: ppc-2.6/drivers/hwmon/ams/ams-i2c.c
=================================--- ppc-2.6.orig/drivers/hwmon/ams/ams-i2c.c 2006-08-04 10:47:57.000000000 -0300
+++ ppc-2.6/drivers/hwmon/ams/ams-i2c.c 2006-08-04 17:18:50.000000000 -0300
@@ -85,14 +85,15 @@
static int ams_i2c_cmd(enum ams_i2c_cmd cmd)
{
s32 result;
- int i;
+ int remaining = HZ/20;

ams_i2c_write(AMS_COMMAND, cmd);
- for (i = 0; i < 10; i++) {
- mdelay(5);
+ mdelay(5);
+ while (remaining) {
result = ams_i2c_read(AMS_COMMAND); if (result = 0 || result & 0x80)
return 0;
+ remaining = schedule_timeout(remaining);
}
return -1;
}

Index: ppc-2.6/drivers/hwmon/ams/ams-i2c.c
=================================--- ppc-2.6.orig/drivers/hwmon/ams/ams-i2c.c 2006-08-04 10:47:57.000000000 -0300
+++ ppc-2.6/drivers/hwmon/ams/ams-i2c.c 2006-08-04 17:34:14.000000000 -0300
@@ -85,11 +85,18 @@
static int ams_i2c_cmd(enum ams_i2c_cmd cmd)
{
s32 result;
- int i;
+ int remaining = HZ/20, tmp;

ams_i2c_write(AMS_COMMAND, cmd);
- for (i = 0; i < 10; i++) {
- mdelay(5);
+ while (remaining) {
+ tmp = remaining;
+ while(1) {
+ remaining = schedule_timeout(remaining);
+ /* wait at least 5ms */
+ if (!remaining || tmp - remaining > HZ/200)
+ break;
+ }
+
result = ams_i2c_read(AMS_COMMAND); if (result = 0 || result & 0x80)
return 0;

#12 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann
Quoted 6 lines · Aristeu Sergio Rozanski Filho
Quoted 2 lines

This guarantees a minimum wait time of 5msec after writing the value. Looking at the specs[1], this should be done. Your code doesn't do that:

I doubt that if it's possible to retrying after a fail the hardware will bother if we try to do the first read in less than 5ms... but here it goes. two versions, feel free to choose.

The specs are written based on the Mac OS X driver, and I think Apple knows their hardware.

After discussing both variants, Ren? Nussbaumer and myself prefer the first one.

Acked-by Michael Hanselmann <linux-kernel at hansmi.ch>
Acked-by Ren? Nussbaumer <linux-kernel at killerfox.forkbomb.ch>

Greets,
Michael

#13 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann
Quoted 8 lines · On Fri, 4 Aug 2006 23:49:24 +0200 Michael Hanselmann <linux-kernel at…
Quoted 6 lines · Aristeu Sergio Rozanski Filho
Quoted 2 lines

This guarantees a minimum wait time of 5msec after writing the value. Looking at the specs[1], this should be done. Your code doesn't do that:

I doubt that if it's possible to retrying after a fail the hardware will bother if we try to do the first read in less than 5ms... but here it goes. two versions, feel free to choose.

Quoted 9 lines

The specs are written based on the Mac OS X driver, and I think Apple knows their hardware.

After discussing both variants, Ren? Nussbaumer and myself prefer the first one.

Acked-by Michael Hanselmann <linux-kernel at hansmi.ch>
Acked-by Ren? Nussbaumer <linux-kernel at killerfox.forkbomb.ch>

Sorry, I've lost track of what's going on here.

Could I please be sent a new copy of the whole patch, with the appropriate acked-bys, signed-off-bys, etc?

Thanks.

#14 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann
Quoted 2 lines · Andrew Morton

Could I please be sent a new copy of the whole patch, with the appropriate acked-bys, signed-off-bys, etc?

Sure, here we go.

- Rename exported variable "ams" to "ams_info"
- Fix leaked input device on error
- Change I?C read code to use schedule_timeout instead of msleep() in case the first read failed

Signed-off-by Michael Hanselmann <linux-kernel at hansmi.ch>
Signed-off-by Aristeu Sergio Rozanski Filho <aris at cathedrallabs.org>
Acked-by Ren? Nussbaumer <linux-kernel at killerfox.forkbomb.ch>

---

drivers/hwmon/ams/ams-core.c +48 -48
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc3.orig/drivers/hwmon/ams/ams-core.c linux-2.6.18-rc3/drivers/hwmon/ams/ams-core.c
@@ -29,22 +29,22 @@
 #include "ams.h"
 
 /* There is only one motion sensor per machine */
-struct ams ams;
+struct ams ams_info;
 
 static unsigned int verbose;
 module_param(verbose, bool, 0644);
 MODULE_PARM_DESC(verbose, "Show free falls and shocks in kernel output");
 
-/* Call with lock held! */
+/* Call with ams_info.lock held! */
 void ams_sensors(s8 *x, s8 *y, s8 *z)
 {
-	u32 orient = ams.vflag? ams.orient1 : ams.orient2;
+	u32 orient = ams_info.vflag? ams_info.orient1 : ams_info.orient2;
 
 	if (orient & 0x80)
 		/* X and Y swapped */
-		ams.get_xyz(y, x, z);
+		ams_info.get_xyz(y, x, z);
 	else
-		ams.get_xyz(x, y, z);
+		ams_info.get_xyz(x, y, z);
 
 	if (orient & 0x04)
 		*z = ~(*z);
@@ -59,9 +59,9 @@ static ssize_t ams_show_current(struct d
 {
 	s8 x, y, z;
 
-	mutex_lock(&ams.lock);
+	mutex_lock(&ams_info.lock);
 	ams_sensors(&x, &y, &z);
-	mutex_unlock(&ams.lock);
+	mutex_unlock(&ams_info.lock);
 
 	return snprintf(buf, PAGE_SIZE, "%d %d %d\n", x, y, z);
 }
@@ -72,12 +72,12 @@ static void ams_handle_irq(void *data)
 {
 	enum ams_irq irq = *((enum ams_irq *)data);
 
-	spin_lock(&ams.irq_lock);
+	spin_lock(&ams_info.irq_lock);
 
-	ams.worker_irqs |= irq;
-	schedule_work(&ams.worker);
+	ams_info.worker_irqs |= irq;
+	schedule_work(&ams_info.worker);
 
-	spin_unlock(&ams.irq_lock);
+	spin_unlock(&ams_info.irq_lock);
 }
 
 static enum ams_irq ams_freefall_irq_data = AMS_IRQ_FREEFALL;
@@ -99,68 +99,68 @@ static struct pmf_irq_client ams_shock_c
  */
 static void ams_worker(void *data)
 {
-	mutex_lock(&ams.lock);
+	mutex_lock(&ams_info.lock);
 
-	if (ams.has_device) {
+	if (ams_info.has_device) {
 		unsigned long flags;
 
-		spin_lock_irqsave(&ams.irq_lock, flags);
+		spin_lock_irqsave(&ams_info.irq_lock, flags);
 
-		if (ams.worker_irqs & AMS_IRQ_FREEFALL) {
+		if (ams_info.worker_irqs & AMS_IRQ_FREEFALL) {
 			if (verbose)
 				printk(KERN_INFO "ams: freefall detected!\n");
 
-			ams.worker_irqs &= ~AMS_IRQ_FREEFALL;
+			ams_info.worker_irqs &= ~AMS_IRQ_FREEFALL;
 
 			/* we must call this with interrupts enabled */
-			spin_unlock_irqrestore(&ams.irq_lock, flags);
-			ams.clear_irq(AMS_IRQ_FREEFALL);
-			spin_lock_irqsave(&ams.irq_lock, flags);
+			spin_unlock_irqrestore(&ams_info.irq_lock, flags);
+			ams_info.clear_irq(AMS_IRQ_FREEFALL);
+			spin_lock_irqsave(&ams_info.irq_lock, flags);
 		}
 
-		if (ams.worker_irqs & AMS_IRQ_SHOCK) {
+		if (ams_info.worker_irqs & AMS_IRQ_SHOCK) {
 			if (verbose)
 				printk(KERN_INFO "ams: shock detected!\n");
 
-			ams.worker_irqs &= ~AMS_IRQ_SHOCK;
+			ams_info.worker_irqs &= ~AMS_IRQ_SHOCK;
 
 			/* we must call this with interrupts enabled */
-			spin_unlock_irqrestore(&ams.irq_lock, flags);
-			ams.clear_irq(AMS_IRQ_SHOCK);
-			spin_lock_irqsave(&ams.irq_lock, flags);
+			spin_unlock_irqrestore(&ams_info.irq_lock, flags);
+			ams_info.clear_irq(AMS_IRQ_SHOCK);
+			spin_lock_irqsave(&ams_info.irq_lock, flags);
 		}
 
-		spin_unlock_irqrestore(&ams.irq_lock, flags);
+		spin_unlock_irqrestore(&ams_info.irq_lock, flags);
 	}
 
-	mutex_unlock(&ams.lock);
+	mutex_unlock(&ams_info.lock);
 }
 
-/* Call with lock held! */
+/* Call with ams_info.lock held! */
 int ams_sensor_attach(void)
 {
 	int result;
 	u32 *prop;
 
 	/* Get orientation */
-	prop = (u32*)get_property(ams.of_node, "orientation", NULL);
+	prop = (u32*)get_property(ams_info.of_node, "orientation", NULL);
 	if (!prop)
 		return -ENODEV;
-	ams.orient1 = *prop;
-	ams.orient2 = *(prop + 1);
+	ams_info.orient1 = *prop;
+	ams_info.orient2 = *(prop + 1);
 
 	/* Register freefall interrupt handler */
-	result = pmf_register_irq_client(ams.of_node,
+	result = pmf_register_irq_client(ams_info.of_node,
 			"accel-int-1",
 			&ams_freefall_client);
 	if (result < 0)
 		return -ENODEV;
 
 	/* Reset saved irqs */
-	ams.worker_irqs = 0;
+	ams_info.worker_irqs = 0;
 
 	/* Register shock interrupt handler */
-	result = pmf_register_irq_client(ams.of_node,
+	result = pmf_register_irq_client(ams_info.of_node,
 			"accel-int-2",
 			&ams_shock_client);
 	if (result < 0) {
@@ -169,17 +169,17 @@ int ams_sensor_attach(void)
 	}
 
 	/* Create device */
-	ams.of_dev = of_platform_device_create(ams.of_node, "ams", NULL);
-	if (!ams.of_dev) {
+	ams_info.of_dev = of_platform_device_create(ams_info.of_node, "ams", NULL);
+	if (!ams_info.of_dev) {
 		pmf_unregister_irq_client(&ams_shock_client);
 		pmf_unregister_irq_client(&ams_freefall_client);
 		return -ENODEV;
 	}
 
 	/* Create attributes */
-	device_create_file(&ams.of_dev->dev, &dev_attr_current);
+	device_create_file(&ams_info.of_dev->dev, &dev_attr_current);
 
-	ams.vflag = !!(ams.get_vendor() & 0x10);
+	ams_info.vflag = !!(ams_info.get_vendor() & 0x10);
 
 	/* Init mouse device */
 	ams_mouse_init();
@@ -191,9 +191,9 @@ int __init ams_init(void)
 {
 	struct device_node *np;
 
-	spin_lock_init(&ams.irq_lock);
-	mutex_init(&ams.lock);
-	INIT_WORK(&ams.worker, ams_worker, NULL);
+	spin_lock_init(&ams_info.irq_lock);
+	mutex_init(&ams_info.lock);
+	INIT_WORK(&ams_info.worker, ams_worker, NULL);
 
 #ifdef CONFIG_SENSORS_AMS_I2C
 	np = of_find_node_by_name(NULL, "accelerometer");
@@ -216,35 +216,35 @@ int __init ams_init(void)
 
 void ams_exit(void)
 {
-	mutex_lock(&ams.lock);
+	mutex_lock(&ams_info.lock);
 
-	if (ams.has_device) {
+	if (ams_info.has_device) {
 		/* Remove mouse device */
 		ams_mouse_exit();
 
 		/* Shut down implementation */
-		ams.exit();
+		ams_info.exit();
 
 		/* Cancel interrupt worker
 		 *
-		 * We do this after ams.exit(), because an interrupt might
+		 * We do this after ams_info.exit(), because an interrupt might
 		 * have arrived before disabling them.
 		 */
-		cancel_delayed_work(&ams.worker);
+		cancel_delayed_work(&ams_info.worker);
 		flush_scheduled_work();
 
 		/* Remove attributes */
-		device_remove_file(&ams.of_dev->dev, &dev_attr_current);
+		device_remove_file(&ams_info.of_dev->dev, &dev_attr_current);
 
 		/* Remove device */
-		of_device_unregister(ams.of_dev);
+		of_device_unregister(ams_info.of_dev);
 
 		/* Remove handler */
 		pmf_unregister_irq_client(&ams_shock_client);
 		pmf_unregister_irq_client(&ams_freefall_client);
 	}
 
-	mutex_unlock(&ams.lock);
+	mutex_unlock(&ams_info.lock);
 }
 
 MODULE_AUTHOR("Stelian Pop, Michael Hanselmann");
drivers/hwmon/ams/ams.h +1 -1
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc3.orig/drivers/hwmon/ams/ams.h linux-2.6.18-rc3/drivers/hwmon/ams/ams.h
@@ -60,7 +60,7 @@ struct ams {
 	int xcalib, ycalib, zcalib;
 };
 
-extern struct ams ams;
+extern struct ams ams_info;
 
 extern void ams_sensors(s8 *x, s8 *y, s8 *z);
 extern int ams_sensor_attach(void);
drivers/hwmon/ams/ams-i2c.c +43 -33

167 lines not on this page · raw message

drivers/hwmon/ams/ams-mouse.c +44 -45

151 lines not on this page · raw message

drivers/hwmon/ams/ams-pmu.c +23 -18

88 lines not on this page · raw message

#15 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann

Le samedi 05 ao?t 2006 ? 01:03 +0200, Michael Hanselmann a ?crit :

Quoted 6 lines · Andrew Morton
Quoted 2 lines

Could I please be sent a new copy of the whole patch, with the appropriate acked-bys, signed-off-bys, etc?

Sure, here we go.

Quoted 8 lines

- Rename exported variable "ams" to "ams_info"
- Fix leaked input device on error
- Change I?C read code to use schedule_timeout instead of msleep() in case the first read failed

Signed-off-by Michael Hanselmann <linux-kernel at hansmi.ch>
Signed-off-by Aristeu Sergio Rozanski Filho <aris at cathedrallabs.org>
Acked-by Ren? Nussbaumer <linux-kernel at killerfox.forkbomb.ch>

You can also add my
Signed-off-by: Stelian Pop <stelian at popies.net>
to the whole ams patch.

Thanks,

Stelian.

Signature · 1 line
Stelian Pop <stelian at popies.net>
#16 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann

drivers/hwmon/Kconfig config SENSORS_ADM9240 · line 99

Quoted 3 lines · Michael Hanselmann
drivers/hwmon/Kconfig +3 -0
+config SENSORS_AMS
+       tristate "Apple Motion Sensor driver"
+       depends on HWMON && PPC_PMAC && INPUT && EXPERIMENTAL

This causes the build failure, because now SENSORS_AMS can be 'y' while

drivers/hwmon/Kconfig config SENSORS_ADM9240 · line 114

Quoted 3 lines
drivers/hwmon/Kconfig +3 -0
+config SENSORS_AMS_I2C
+       bool "I2C variant"
+       depends on SENSORS_AMS && I2C

is satisfied by I2C=m. I think.

SENSORS_AMS should probably have

	depends on HWMON && PPC_PMAC && INPUT && (ADB_PMU || I2C) && EXPERIMENTAL

Is it possible to check further constraints? This still doesn't solve all problems since I2C=m and ADB_PMU=y would satisfy SENSORS_AMS=y, but that'd still lead to a build failure with SENSORS_AMS_I2C=y.

I'm not sure if that's possible to solve at all. But since ADB_PMU is practically required even on those machines where we have the I2C version of the sensor (they'll boot IIRC, but most things won't work without it), how about just depending on that anyway? Hence making the dependency

	depends on HWMON && PPC_PMAC && INPUT && ADB_PMU && I2C && EXPERIMENTAL

Also, "default y" should go away, and be put into the ppc32_defconfig. We don't ever need this on ppc64. The dependency on ADB_PMU would of course stop people from building it on ppc64 :)

johannes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 828 bytes
Desc: This is a digitally signed message part
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060806/29aaade5/attachment.bin

#17 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann
Quoted 2 lines · Johannes Berg

On Wed, 2006-08-02 at 21:15 +0200, Michael Hanselmann wrote:
I'm not sure if that's possible to solve at all.

I think it is, see patch below. However, the solution is not that pretty.

Quoted 1 line
	depends on HWMON && PPC_PMAC && INPUT && ADB_PMU && I2C && EXPERIMENTAL

This would not allow users to build the AMS stuff without having I2C in he kernel -- which is not really required.

What do you think about this patch?

---

drivers/hwmon/Kconfig +4 -2
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc3.orig/drivers/hwmon/Kconfig linux-2.6.18-rc3/drivers/hwmon/Kconfig
@@ -96,12 +96,14 @@ config SENSORS_ADM9240
 
 config SENSORS_AMS
 	tristate "Apple Motion Sensor driver"
-	depends on HWMON && PPC_PMAC && INPUT && EXPERIMENTAL
-	default y
+	depends on HWMON && PPC_PMAC && !PPC64 && INPUT && ((ADB_PMU && I2C = y) || (ADB_PMU && !I2C) || I2C) && EXPERIMENTAL
 	help
 	  Support for the motion sensor included in PowerBooks. Includes
 	  implementations for PMU and I2C.
 
+	  This driver can also be built as a module. If so, the module
+	  will be called ams.
+
 config SENSORS_AMS_PMU
 	bool "PMU variant"
 	depends on SENSORS_AMS && ADB_PMU

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060806/51eddd2b/attachment.bin

#18 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann

Review

Quoted 2 lines · Michael Hanselmann
(quoted diff) +1 -0
+	depends on HWMON && PPC_PMAC && !PPC64 && INPUT && ((ADB_PMU && I2C = y) || (ADB_PMU && !I2C) || I2C) && EXPERIMENTAL

Maybe we should just add "select I2C" instead. Then, if the user had I2C as a module and wants AMS he'll get I2C built-in. I think/hope :)

johannes

#19 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann
Quoted 5 lines

Review

Quoted 2 lines · Michael Hanselmann
(quoted diff) +1 -0
+	depends on HWMON && PPC_PMAC && !PPC64 && INPUT && ((ADB_PMU && I2C = y) || (ADB_PMU && !I2C) || I2C) && EXPERIMENTAL

Maybe we should just add "select I2C" instead. Then, if the user had I2C as a module and wants AMS he'll get I2C built-in. I think/hope :)

No, please. I2C is something which you should depend on, not something you select.

Signature · 1 line
Jean Delvare
#20 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann
Quoted 3 lines · Jean Delvare

No, please. I2C is something which you should depend on, not something you select.

Why? It's infrastructure basically. Anyway. Is it possible to have something like
depends I2C=AMS || I2C=y

If we stuck that into the i2c AMS version that'd help. Though be really ugly for the user since she might get the AMS I2C version only after selecting AMS as a module then. Pretty icky, all that.

johannes

#21 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann
Quoted 6 lines
Quoted 3 lines · Jean Delvare

No, please. I2C is something which you should depend on, not something you select.

Why? It's infrastructure basically.

Every i2c driver to date depends on I2C, and my experience is that mixing select and depends tends to confuse Kconfig, users, or both.

Now if you believe that this is an error and I2C should always be selected rather than depended upon, I'm waiting for your patch :)

Signature · 1 line
Jean Delvare
#22 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann
Quoted 3 lines · Jean Delvare

Now if you believe that this is an error and I2C should always be selected rather than depended upon, I'm waiting for your patch :)

Heh, no, I don't really care. A Power/iBook without I2C isn't much good anyway.

johannes

#23 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann
Quoted 10 lines · Michael Hanselmann
Quoted 6 lines · Aristeu Sergio Rozanski Filho
Quoted 2 lines

This guarantees a minimum wait time of 5msec after writing the value. Looking at the specs[1], this should be done. Your code doesn't do that:

I doubt that if it's possible to retrying after a fail the hardware will bother if we try to do the first read in less than 5ms... but here it goes. two versions, feel free to choose.

The specs are written based on the Mac OS X driver, and I think Apple knows their hardware.

Don't be so sure :)

Quoted 8 lines

After discussing both variants, Ren? Nussbaumer and myself prefer the first one.

Acked-by Michael Hanselmann <linux-kernel at hansmi.ch>
Acked-by Ren? Nussbaumer <linux-kernel at killerfox.forkbomb.ch>

Greets,
Michael

#24 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann
Quoted 6 lines · Johannes Berg
Quoted 3 lines · Jean Delvare

Now if you believe that this is an error and I2C should always be selected rather than depended upon, I'm waiting for your patch :)

Heh, no, I don't really care. A Power/iBook without I2C isn't much good anyway.

Old ones are...

Ben.

#25 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann
Quoted 7 lines · Johannes Berg

Why? It's infrastructure basically. Anyway. Is it possible to have something like
depends I2C=AMS || I2C=y

If we stuck that into the i2c AMS version that'd help. Though be really ugly for the user since she might get the AMS I2C version only after selecting AMS as a module then. Pretty icky, all that.

I'm not really a fan of it. Is there anything wrong with my proposed patch?

Thanks,
Michael

#26 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann
Quoted 3 lines · Michael Hanselmann

I'm not really a fan of it. Is there anything wrong with my proposed patch?

This is what you get if I write things and then don't put the powerbook online for days.

I think your depends statement is hard to parse, but should be fine :) Try actually configuring things in the presence of I2C=m though, please.

johannes

#28 lore raw

PATCH 1/1 Apple Motion Sensor driver

In reply to Michael Hanselmann
Quoted 7 lines · Johannes Berg

Review

Quoted 3 lines · Michael Hanselmann
(quoted diff) +3 -0
+config SENSORS_AMS
+       tristate "Apple Motion Sensor driver"
+       depends on HWMON && PPC_PMAC && INPUT && EXPERIMENTAL

This causes the build failure, because now SENSORS_AMS can be 'y' while

Andrew, can you please apply the patch below? It fixes wrong Kconfig dependencies.

Signed-off-by Michael Hanselmann <linux-kernel at hansmi.ch>

---

drivers/hwmon/Kconfig +4 -2
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc3.orig/drivers/hwmon/Kconfig linux-2.6.18-rc3/drivers/hwmon/Kconfig
@@ -96,12 +96,14 @@ config SENSORS_ADM9240
 
 config SENSORS_AMS
 	tristate "Apple Motion Sensor driver"
-	depends on HWMON && PPC_PMAC && INPUT && EXPERIMENTAL
-	default y
+	depends on HWMON && PPC_PMAC && !PPC64 && INPUT && ((ADB_PMU && I2C = y) || (ADB_PMU && !I2C) || I2C) && EXPERIMENTAL
 	help
 	  Support for the motion sensor included in PowerBooks. Includes
 	  implementations for PMU and I2C.
 
+	  This driver can also be built as a module. If so, the module
+	  will be called ams.
+
 config SENSORS_AMS_PMU
 	bool "PMU variant"
 	depends on SENSORS_AMS && ADB_PMU