* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
@ 2006-08-03 2:11 ` Andrew Morton
2006-08-03 6:53 ` Michael Hanselmann
` (26 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Andrew Morton @ 2006-08-03 2:11 UTC (permalink / raw)
To: lm-sensors
On Wed, 2 Aug 2006 21:15:20 +0200
Michael Hanselmann <linux-kernel at hansmi.ch> wrote:
> 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
?
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
2006-08-03 2:11 ` Andrew Morton
@ 2006-08-03 6:53 ` Michael Hanselmann
2006-08-03 7:26 ` Andrew Morton
` (25 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Michael Hanselmann @ 2006-08-03 6:53 UTC (permalink / raw)
To: lm-sensors
On Wed, Aug 02, 2006 at 07:11:27PM -0700, Andrew Morton wrote:
> On Wed, 2 Aug 2006 21:15:20 +0200
> Michael Hanselmann <linux-kernel at hansmi.ch> wrote:
> > 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
> ?
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:
On Mon, Jul 03, 2006 at 08:56:28AM +0200, Michael Hanselmann wrote:
> On Sun, Jul 02, 2006 at 08:14:15PM -0700, Andrew Morton wrote:
> > On Mon, 3 Jul 2006 00:26:49 +0200
> > Michael Hanselmann <linux-kernel at hansmi.ch> wrote:
> > > 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
> 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
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
2006-08-03 2:11 ` Andrew Morton
2006-08-03 6:53 ` Michael Hanselmann
@ 2006-08-03 7:26 ` Andrew Morton
2006-08-03 7:36 ` Andrew Morton
` (24 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Andrew Morton @ 2006-08-03 7:26 UTC (permalink / raw)
To: lm-sensors
On Thu, 3 Aug 2006 08:53:33 +0200
Michael Hanselmann <linux-kernel at hansmi.ch> wrote:
> On Wed, Aug 02, 2006 at 07:11:27PM -0700, Andrew Morton wrote:
> > On Wed, 2 Aug 2006 21:15:20 +0200
> > Michael Hanselmann <linux-kernel at hansmi.ch> wrote:
>
> > > 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
> > ?
>
> 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.
> 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?
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (2 preceding siblings ...)
2006-08-03 7:26 ` Andrew Morton
@ 2006-08-03 7:36 ` Andrew Morton
2006-08-03 19:49 ` Michael Hanselmann
` (23 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Andrew Morton @ 2006-08-03 7:36 UTC (permalink / raw)
To: lm-sensors
On Wed, 2 Aug 2006 21:15:20 +0200
Michael Hanselmann <linux-kernel at hansmi.ch> wrote:
> 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.
>
> ...
>
> +/* 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 ;)
> +/* Call with lock held! */
> +int ams_sensor_attach(void)
Which lock?
> +static int ams_i2c_cmd(enum ams_i2c_cmd cmd)
> +{
> + s32 result;
> + int i;
> +
> + 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.
> +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;
> + 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;
> +/* Call with lock held! */
> +static void ams_mouse_enable(void)
Which lock?
> +{
> + 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)) {
> + 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>
> + ams.idev = NULL;
> + }
> +}
> +
> +static ssize_t ams_mouse_show_mouse(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + 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.
> + 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;
> + 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.
> +/* Call with lock held! */
> +int __init ams_pmu_init(struct device_node *np)
Which lock??
> +{
> + u32 *prop;
> + int result;
> +
> + mutex_lock(&ams.lock);
Obviously not that one...
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (3 preceding siblings ...)
2006-08-03 7:36 ` Andrew Morton
@ 2006-08-03 19:49 ` Michael Hanselmann
2006-08-03 20:08 ` Aristeu Sergio Rozanski Filho
` (22 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Michael Hanselmann @ 2006-08-03 19:49 UTC (permalink / raw)
To: lm-sensors
On Thu, Aug 03, 2006 at 12:36:55AM -0700, Andrew Morton wrote:
> > +/* 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.
> > +/* Call with lock held! */
> > +int ams_sensor_attach(void)
> Which lock?
ams_info.lock, changed all such comments
> > +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.
> > + 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>
---
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
--- linux-2.6.18-rc3.orig/drivers/hwmon/ams/ams-core.c 2006-08-03 21:36:49.000000000 +0200
+++ linux-2.6.18-rc3/drivers/hwmon/ams/ams-core.c 2006-08-03 21:27:28.000000000 +0200
@@ -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");
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
--- linux-2.6.18-rc3.orig/drivers/hwmon/ams/ams.h 2006-08-03 21:36:49.000000000 +0200
+++ linux-2.6.18-rc3/drivers/hwmon/ams/ams.h 2006-08-03 21:19:28.000000000 +0200
@@ -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);
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc3.orig/drivers/hwmon/ams/ams-i2c.c linux-2.6.18-rc3/drivers/hwmon/ams/ams-i2c.c
--- linux-2.6.18-rc3.orig/drivers/hwmon/ams/ams-i2c.c 2006-08-03 21:36:49.000000000 +0200
+++ linux-2.6.18-rc3/drivers/hwmon/ams/ams-i2c.c 2006-08-03 21:29:58.000000000 +0200
@@ -74,12 +74,12 @@ static struct i2c_driver ams_i2c_driver
static s32 ams_i2c_read(u8 reg)
{
- return i2c_smbus_read_byte_data(&ams.i2c_client, reg);
+ return i2c_smbus_read_byte_data(&ams_info.i2c_client, reg);
}
static int ams_i2c_write(u8 reg, u8 value)
{
- return i2c_smbus_write_byte_data(&ams.i2c_client, reg, value);
+ return i2c_smbus_write_byte_data(&ams_info.i2c_client, reg, value);
}
static int ams_i2c_cmd(enum ams_i2c_cmd cmd)
@@ -101,22 +101,28 @@ static void ams_i2c_set_irq(enum ams_irq
{
if (reg & AMS_IRQ_FREEFALL) {
u8 val = ams_i2c_read(AMS_CTRLX);
- if (enable) val |= 0x80;
- else val &= ~0x80;
+ 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;
+ if (enable)
+ val |= 0x80;
+ else
+ val &= ~0x80;
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;
+ if (enable)
+ val |= 0x80;
+ else
+ val &= ~0x80;
ams_i2c_write(AMS_CTRLZ, val);
}
}
@@ -149,20 +155,20 @@ static int ams_i2c_attach(struct i2c_ada
int result;
/* There can be only one */
- if (unlikely(ams.has_device))
+ if (unlikely(ams_info.has_device))
return -ENODEV;
if (strncmp(adapter->name, "uni-n", 5))
return -ENODEV;
bus = simple_strtoul(adapter->name + 6, NULL, 10);
- if (bus != ams.i2c_bus)
+ if (bus != ams_info.i2c_bus)
return -ENODEV;
- ams.i2c_client.addr = ams.i2c_address;
- ams.i2c_client.adapter = adapter;
- ams.i2c_client.driver = &ams_i2c_driver;
- strcpy(ams.i2c_client.name, "Apple Motion Sensor");
+ ams_info.i2c_client.addr = ams_info.i2c_address;
+ ams_info.i2c_client.adapter = adapter;
+ ams_info.i2c_client.driver = &ams_i2c_driver;
+ strcpy(ams_info.i2c_client.name, "Apple Motion Sensor");
if (ams_i2c_cmd(AMS_CMD_RESET)) {
printk(KERN_INFO "ams: Failed to reset the device\n");
@@ -217,7 +223,7 @@ static int ams_i2c_attach(struct i2c_ada
/* Clear interrupts */
ams_i2c_clear_irq(AMS_IRQ_ALL);
- ams.has_device = 1;
+ ams_info.has_device = 1;
/* Enable interrupts */
ams_i2c_set_irq(AMS_IRQ_ALL, 1);
@@ -229,7 +235,7 @@ static int ams_i2c_attach(struct i2c_ada
static int ams_i2c_detach(struct i2c_adapter *adapter)
{
- if (ams.has_device) {
+ if (ams_info.has_device) {
/* Disable interrupts */
ams_i2c_set_irq(AMS_IRQ_ALL, 0);
@@ -238,7 +244,7 @@ static int ams_i2c_detach(struct i2c_ada
printk(KERN_INFO "ams: Unloading\n");
- ams.has_device = 0;
+ ams_info.has_device = 0;
}
return 0;
@@ -255,35 +261,35 @@ int __init ams_i2c_init(struct device_no
int result;
u32 *prop;
- mutex_lock(&ams.lock);
+ mutex_lock(&ams_info.lock);
/* Set implementation stuff */
- ams.of_node = np;
- ams.exit = ams_i2c_exit;
- ams.get_vendor = ams_i2c_get_vendor;
- ams.get_xyz = ams_i2c_get_xyz;
- ams.clear_irq = ams_i2c_clear_irq;
- ams.bustype = BUS_I2C;
+ ams_info.of_node = np;
+ ams_info.exit = ams_i2c_exit;
+ ams_info.get_vendor = ams_i2c_get_vendor;
+ ams_info.get_xyz = ams_i2c_get_xyz;
+ ams_info.clear_irq = ams_i2c_clear_irq;
+ ams_info.bustype = BUS_I2C;
/* look for bus either using "reg" or by path */
- prop = (u32*)get_property(ams.of_node, "reg", NULL);
+ prop = (u32*)get_property(ams_info.of_node, "reg", NULL);
if (!prop) {
result = -ENODEV;
goto exit;
}
- tmp_bus = strstr(ams.of_node->full_name, "/i2c-bus@");
+ tmp_bus = strstr(ams_info.of_node->full_name, "/i2c-bus@");
if (tmp_bus)
- ams.i2c_bus = *(tmp_bus + 9) - '0';
+ ams_info.i2c_bus = *(tmp_bus + 9) - '0';
else
- ams.i2c_bus = ((*prop) >> 8) & 0x0f;
- ams.i2c_address = ((*prop) & 0xff) >> 1;
+ ams_info.i2c_bus = ((*prop) >> 8) & 0x0f;
+ ams_info.i2c_address = ((*prop) & 0xff) >> 1;
result = i2c_add_driver(&ams_i2c_driver);
exit:
- mutex_unlock(&ams.lock);
+ mutex_unlock(&ams_info.lock);
return result;
}
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc3.orig/drivers/hwmon/ams/ams-mouse.c linux-2.6.18-rc3/drivers/hwmon/ams/ams-mouse.c
--- linux-2.6.18-rc3.orig/drivers/hwmon/ams/ams-mouse.c 2006-08-03 21:36:49.000000000 +0200
+++ linux-2.6.18-rc3/drivers/hwmon/ams/ams-mouse.c 2006-08-03 21:32:30.000000000 +0200
@@ -28,17 +28,17 @@ static int ams_mouse_kthread(void *data)
s8 x, y, z;
while (!kthread_should_stop()) {
- mutex_lock(&ams.lock);
+ mutex_lock(&ams_info.lock);
ams_sensors(&x, &y, &z);
- input_report_abs(ams.idev, ABS_X, x - ams.xcalib);
- input_report_abs(ams.idev, ABS_Y, y - ams.ycalib);
- input_report_abs(ams.idev, ABS_Z, z - ams.zcalib);
+ input_report_abs(ams_info.idev, ABS_X, x - ams_info.xcalib);
+ input_report_abs(ams_info.idev, ABS_Y, y - ams_info.ycalib);
+ input_report_abs(ams_info.idev, ABS_Z, z - ams_info.zcalib);
- input_sync(ams.idev);
+ input_sync(ams_info.idev);
- mutex_unlock(&ams.lock);
+ mutex_unlock(&ams_info.lock);
msleep(25);
}
@@ -46,56 +46,57 @@ static int ams_mouse_kthread(void *data)
return 0;
}
-/* Call with lock held! */
+/* Call with ams_info.lock held! */
static void ams_mouse_enable(void)
{
s8 x, y, z;
- if (ams.idev)
+ if (ams_info.idev)
return;
ams_sensors(&x, &y, &z);
- ams.xcalib = x;
- ams.ycalib = y;
- ams.zcalib = z;
+ ams_info.xcalib = x;
+ ams_info.ycalib = y;
+ ams_info.zcalib = z;
- ams.idev = input_allocate_device();
- if (!ams.idev)
+ ams_info.idev = input_allocate_device();
+ if (!ams_info.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)) {
- input_free_device(ams.idev);
- ams.idev = NULL;
+ ams_info.idev->name = "Apple Motion Sensor";
+ ams_info.idev->id.bustype = ams_info.bustype;
+ ams_info.idev->id.vendor = 0;
+ ams_info.idev->cdev.dev = &ams_info.of_dev->dev;
+
+ input_set_abs_params(ams_info.idev, ABS_X, -50, 50, 3, 0);
+ input_set_abs_params(ams_info.idev, ABS_Y, -50, 50, 3, 0);
+ input_set_abs_params(ams_info.idev, ABS_Z, -50, 50, 3, 0);
+
+ set_bit(EV_ABS, ams_info.idev->evbit);
+ set_bit(EV_KEY, ams_info.idev->evbit);
+
+ if (input_register_device(ams_info.idev)) {
+ input_free_device(ams_info.idev);
+ ams_info.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;
+ ams_info.kthread = kthread_run(ams_mouse_kthread, NULL, "kams");
+ if (IS_ERR(ams_info.kthread)) {
+ input_unregister_device(ams_info.idev);
+ input_free_device(ams_info.idev);
+ ams_info.idev = NULL;
return;
}
}
-/* Call with lock held! */
+/* Call with ams_info.lock held! */
static void ams_mouse_disable(void)
{
- if (ams.idev) {
- kthread_stop(ams.kthread);
- input_unregister_device(ams.idev);
- ams.idev = NULL;
+ if (ams_info.idev) {
+ kthread_stop(ams_info.kthread);
+ input_unregister_device(ams_info.idev);
+ ams_info.idev = NULL;
}
}
@@ -111,16 +112,14 @@ static ssize_t ams_mouse_store_mouse(str
if (sscanf(buf, "%d\n", &mouse) != 1)
return -EINVAL;
- mouse = !!mouse;
-
- mutex_lock(&ams.lock);
+ mutex_lock(&ams_info.lock);
if (mouse)
ams_mouse_enable();
else
ams_mouse_disable();
- mutex_unlock(&ams.lock);
+ mutex_unlock(&ams_info.lock);
return count;
}
@@ -128,18 +127,18 @@ static ssize_t ams_mouse_store_mouse(str
static DEVICE_ATTR(mouse, S_IRUGO | S_IWUSR,
ams_mouse_show_mouse, ams_mouse_store_mouse);
-/* Call with lock held! */
+/* Call with ams_info.lock held! */
void ams_mouse_init()
{
- device_create_file(&ams.of_dev->dev, &dev_attr_mouse);
+ device_create_file(&ams_info.of_dev->dev, &dev_attr_mouse);
if (mouse)
ams_mouse_enable();
}
-/* Call with lock held! */
+/* Call with ams_info.lock held! */
void ams_mouse_exit()
{
ams_mouse_disable();
- device_remove_file(&ams.of_dev->dev, &dev_attr_mouse);
+ device_remove_file(&ams_info.of_dev->dev, &dev_attr_mouse);
}
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc3.orig/drivers/hwmon/ams/ams-pmu.c linux-2.6.18-rc3/drivers/hwmon/ams/ams-pmu.c
--- linux-2.6.18-rc3.orig/drivers/hwmon/ams/ams-pmu.c 2006-08-03 21:36:49.000000000 +0200
+++ linux-2.6.18-rc3/drivers/hwmon/ams/ams-pmu.c 2006-08-03 21:34:12.000000000 +0200
@@ -84,22 +84,28 @@ static void ams_pmu_set_irq(enum ams_irq
{
if (reg & AMS_IRQ_FREEFALL) {
u8 val = ams_pmu_get_register(AMS_FF_ENABLE);
- if (enable) val |= 0x80;
- else val &= ~0x80;
+ 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;
+ if (enable)
+ val |= 0x80;
+ else
+ val &= ~0x80;
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;
+ if (enable)
+ val |= 0x80;
+ else
+ val &= ~0x80;
ams_pmu_set_register(AMS_CONTROL, val);
}
}
@@ -133,29 +139,28 @@ static void ams_pmu_exit(void)
/* Clear interrupts */
ams_pmu_clear_irq(AMS_IRQ_ALL);
- ams.has_device = 0;
+ ams_info.has_device = 0;
printk(KERN_INFO "ams: Unloading\n");
}
-/* Call with lock held! */
int __init ams_pmu_init(struct device_node *np)
{
u32 *prop;
int result;
- mutex_lock(&ams.lock);
+ mutex_lock(&ams_info.lock);
/* Set implementation stuff */
- ams.of_node = np;
- ams.exit = ams_pmu_exit;
- ams.get_vendor = ams_pmu_get_vendor;
- ams.get_xyz = ams_pmu_get_xyz;
- ams.clear_irq = ams_pmu_clear_irq;
- ams.bustype = BUS_HOST;
+ ams_info.of_node = np;
+ ams_info.exit = ams_pmu_exit;
+ ams_info.get_vendor = ams_pmu_get_vendor;
+ ams_info.get_xyz = ams_pmu_get_xyz;
+ ams_info.clear_irq = ams_pmu_clear_irq;
+ ams_info.bustype = BUS_HOST;
/* Get PMU command, should be 0x4e, but we can never know */
- prop = (u32*)get_property(ams.of_node, "reg", NULL);
+ prop = (u32*)get_property(ams_info.of_node, "reg", NULL);
if (!prop) {
result = -ENODEV;
goto exit;
@@ -186,7 +191,7 @@ int __init ams_pmu_init(struct device_no
/* Clear interrupts */
ams_pmu_clear_irq(AMS_IRQ_ALL);
- ams.has_device = 1;
+ ams_info.has_device = 1;
/* Enable interrupts */
ams_pmu_set_irq(AMS_IRQ_ALL, 1);
@@ -196,7 +201,7 @@ int __init ams_pmu_init(struct device_no
result = 0;
exit:
- mutex_unlock(&ams.lock);
+ mutex_unlock(&ams_info.lock);
return result;
}
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (4 preceding siblings ...)
2006-08-03 19:49 ` Michael Hanselmann
@ 2006-08-03 20:08 ` Aristeu Sergio Rozanski Filho
2006-08-04 8:13 ` Johannes Berg
` (21 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Aristeu Sergio Rozanski Filho @ 2006-08-03 20:08 UTC (permalink / raw)
To: lm-sensors
> 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
--
Aristeu
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (5 preceding siblings ...)
2006-08-03 20:08 ` Aristeu Sergio Rozanski Filho
@ 2006-08-04 8:13 ` Johannes Berg
2006-08-04 8:34 ` Johannes Berg
` (20 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Johannes Berg @ 2006-08-04 8:13 UTC (permalink / raw)
To: lm-sensors
Andrew Morton wrote:
> 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>
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (6 preceding siblings ...)
2006-08-04 8:13 ` Johannes Berg
@ 2006-08-04 8:34 ` Johannes Berg
2006-08-04 14:11 ` Aristeu Sergio Rozanski Filho
` (19 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Johannes Berg @ 2006-08-04 8:34 UTC (permalink / raw)
To: lm-sensors
Michael Hanselmann wrote:
> Please apply to -mm for testing.
>
Btw. I really wonder... Does anyone with a powerbook run/test -mm?
johannes
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (7 preceding siblings ...)
2006-08-04 8:34 ` Johannes Berg
@ 2006-08-04 14:11 ` Aristeu Sergio Rozanski Filho
2006-08-04 19:52 ` Michael Hanselmann
` (18 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Aristeu Sergio Rozanski Filho @ 2006-08-04 14:11 UTC (permalink / raw)
To: lm-sensors
> > 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.
--
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;
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (8 preceding siblings ...)
2006-08-04 14:11 ` Aristeu Sergio Rozanski Filho
@ 2006-08-04 19:52 ` Michael Hanselmann
2006-08-04 20:39 ` Aristeu Sergio Rozanski Filho
` (17 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Michael Hanselmann @ 2006-08-04 19:52 UTC (permalink / raw)
To: lm-sensors
On Fri, Aug 04, 2006 at 11:11:31AM -0300, Aristeu Sergio Rozanski Filho wrote:
> what about the attached patch?
> 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:
> + 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
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (9 preceding siblings ...)
2006-08-04 19:52 ` Michael Hanselmann
@ 2006-08-04 20:39 ` Aristeu Sergio Rozanski Filho
2006-08-04 21:49 ` Michael Hanselmann
` (16 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Aristeu Sergio Rozanski Filho @ 2006-08-04 20:39 UTC (permalink / raw)
To: lm-sensors
> 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;
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (10 preceding siblings ...)
2006-08-04 20:39 ` Aristeu Sergio Rozanski Filho
@ 2006-08-04 21:49 ` Michael Hanselmann
2006-08-04 22:19 ` Andrew Morton
` (15 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Michael Hanselmann @ 2006-08-04 21:49 UTC (permalink / raw)
To: lm-sensors
On Fri, Aug 04, 2006 at 05:39:32PM -0300, Aristeu Sergio Rozanski Filho wrote:
> > 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
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (11 preceding siblings ...)
2006-08-04 21:49 ` Michael Hanselmann
@ 2006-08-04 22:19 ` Andrew Morton
2006-08-04 23:03 ` Michael Hanselmann
` (14 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Andrew Morton @ 2006-08-04 22:19 UTC (permalink / raw)
To: lm-sensors
On Fri, 4 Aug 2006 23:49:24 +0200
Michael Hanselmann <linux-kernel at hansmi.ch> wrote:
> On Fri, Aug 04, 2006 at 05:39:32PM -0300, Aristeu Sergio Rozanski Filho wrote:
> > > 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>
>
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.
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (12 preceding siblings ...)
2006-08-04 22:19 ` Andrew Morton
@ 2006-08-04 23:03 ` Michael Hanselmann
2006-08-05 7:26 ` Stelian Pop
` (13 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Michael Hanselmann @ 2006-08-04 23:03 UTC (permalink / raw)
To: lm-sensors
On Fri, Aug 04, 2006 at 03:19:52PM -0700, Andrew Morton wrote:
> 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>
---
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
--- linux-2.6.18-rc3.orig/drivers/hwmon/ams/ams-core.c 2006-08-05 00:47:46.000000000 +0200
+++ linux-2.6.18-rc3/drivers/hwmon/ams/ams-core.c 2006-08-05 00:49:56.000000000 +0200
@@ -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");
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
--- linux-2.6.18-rc3.orig/drivers/hwmon/ams/ams.h 2006-08-05 00:47:46.000000000 +0200
+++ linux-2.6.18-rc3/drivers/hwmon/ams/ams.h 2006-08-05 00:49:56.000000000 +0200
@@ -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);
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc3.orig/drivers/hwmon/ams/ams-i2c.c linux-2.6.18-rc3/drivers/hwmon/ams/ams-i2c.c
--- linux-2.6.18-rc3.orig/drivers/hwmon/ams/ams-i2c.c 2006-08-05 00:47:46.000000000 +0200
+++ linux-2.6.18-rc3/drivers/hwmon/ams/ams-i2c.c 2006-08-05 00:51:33.000000000 +0200
@@ -74,26 +74,30 @@ static struct i2c_driver ams_i2c_driver
static s32 ams_i2c_read(u8 reg)
{
- return i2c_smbus_read_byte_data(&ams.i2c_client, reg);
+ return i2c_smbus_read_byte_data(&ams_info.i2c_client, reg);
}
static int ams_i2c_write(u8 reg, u8 value)
{
- return i2c_smbus_write_byte_data(&ams.i2c_client, reg, value);
+ return i2c_smbus_write_byte_data(&ams_info.i2c_client, reg, value);
}
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;
}
@@ -101,22 +105,28 @@ static void ams_i2c_set_irq(enum ams_irq
{
if (reg & AMS_IRQ_FREEFALL) {
u8 val = ams_i2c_read(AMS_CTRLX);
- if (enable) val |= 0x80;
- else val &= ~0x80;
+ 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;
+ if (enable)
+ val |= 0x80;
+ else
+ val &= ~0x80;
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;
+ if (enable)
+ val |= 0x80;
+ else
+ val &= ~0x80;
ams_i2c_write(AMS_CTRLZ, val);
}
}
@@ -149,20 +159,20 @@ static int ams_i2c_attach(struct i2c_ada
int result;
/* There can be only one */
- if (unlikely(ams.has_device))
+ if (unlikely(ams_info.has_device))
return -ENODEV;
if (strncmp(adapter->name, "uni-n", 5))
return -ENODEV;
bus = simple_strtoul(adapter->name + 6, NULL, 10);
- if (bus != ams.i2c_bus)
+ if (bus != ams_info.i2c_bus)
return -ENODEV;
- ams.i2c_client.addr = ams.i2c_address;
- ams.i2c_client.adapter = adapter;
- ams.i2c_client.driver = &ams_i2c_driver;
- strcpy(ams.i2c_client.name, "Apple Motion Sensor");
+ ams_info.i2c_client.addr = ams_info.i2c_address;
+ ams_info.i2c_client.adapter = adapter;
+ ams_info.i2c_client.driver = &ams_i2c_driver;
+ strcpy(ams_info.i2c_client.name, "Apple Motion Sensor");
if (ams_i2c_cmd(AMS_CMD_RESET)) {
printk(KERN_INFO "ams: Failed to reset the device\n");
@@ -217,7 +227,7 @@ static int ams_i2c_attach(struct i2c_ada
/* Clear interrupts */
ams_i2c_clear_irq(AMS_IRQ_ALL);
- ams.has_device = 1;
+ ams_info.has_device = 1;
/* Enable interrupts */
ams_i2c_set_irq(AMS_IRQ_ALL, 1);
@@ -229,7 +239,7 @@ static int ams_i2c_attach(struct i2c_ada
static int ams_i2c_detach(struct i2c_adapter *adapter)
{
- if (ams.has_device) {
+ if (ams_info.has_device) {
/* Disable interrupts */
ams_i2c_set_irq(AMS_IRQ_ALL, 0);
@@ -238,7 +248,7 @@ static int ams_i2c_detach(struct i2c_ada
printk(KERN_INFO "ams: Unloading\n");
- ams.has_device = 0;
+ ams_info.has_device = 0;
}
return 0;
@@ -255,35 +265,35 @@ int __init ams_i2c_init(struct device_no
int result;
u32 *prop;
- mutex_lock(&ams.lock);
+ mutex_lock(&ams_info.lock);
/* Set implementation stuff */
- ams.of_node = np;
- ams.exit = ams_i2c_exit;
- ams.get_vendor = ams_i2c_get_vendor;
- ams.get_xyz = ams_i2c_get_xyz;
- ams.clear_irq = ams_i2c_clear_irq;
- ams.bustype = BUS_I2C;
+ ams_info.of_node = np;
+ ams_info.exit = ams_i2c_exit;
+ ams_info.get_vendor = ams_i2c_get_vendor;
+ ams_info.get_xyz = ams_i2c_get_xyz;
+ ams_info.clear_irq = ams_i2c_clear_irq;
+ ams_info.bustype = BUS_I2C;
/* look for bus either using "reg" or by path */
- prop = (u32*)get_property(ams.of_node, "reg", NULL);
+ prop = (u32*)get_property(ams_info.of_node, "reg", NULL);
if (!prop) {
result = -ENODEV;
goto exit;
}
- tmp_bus = strstr(ams.of_node->full_name, "/i2c-bus@");
+ tmp_bus = strstr(ams_info.of_node->full_name, "/i2c-bus@");
if (tmp_bus)
- ams.i2c_bus = *(tmp_bus + 9) - '0';
+ ams_info.i2c_bus = *(tmp_bus + 9) - '0';
else
- ams.i2c_bus = ((*prop) >> 8) & 0x0f;
- ams.i2c_address = ((*prop) & 0xff) >> 1;
+ ams_info.i2c_bus = ((*prop) >> 8) & 0x0f;
+ ams_info.i2c_address = ((*prop) & 0xff) >> 1;
result = i2c_add_driver(&ams_i2c_driver);
exit:
- mutex_unlock(&ams.lock);
+ mutex_unlock(&ams_info.lock);
return result;
}
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc3.orig/drivers/hwmon/ams/ams-mouse.c linux-2.6.18-rc3/drivers/hwmon/ams/ams-mouse.c
--- linux-2.6.18-rc3.orig/drivers/hwmon/ams/ams-mouse.c 2006-08-05 00:47:46.000000000 +0200
+++ linux-2.6.18-rc3/drivers/hwmon/ams/ams-mouse.c 2006-08-05 00:49:56.000000000 +0200
@@ -28,17 +28,17 @@ static int ams_mouse_kthread(void *data)
s8 x, y, z;
while (!kthread_should_stop()) {
- mutex_lock(&ams.lock);
+ mutex_lock(&ams_info.lock);
ams_sensors(&x, &y, &z);
- input_report_abs(ams.idev, ABS_X, x - ams.xcalib);
- input_report_abs(ams.idev, ABS_Y, y - ams.ycalib);
- input_report_abs(ams.idev, ABS_Z, z - ams.zcalib);
+ input_report_abs(ams_info.idev, ABS_X, x - ams_info.xcalib);
+ input_report_abs(ams_info.idev, ABS_Y, y - ams_info.ycalib);
+ input_report_abs(ams_info.idev, ABS_Z, z - ams_info.zcalib);
- input_sync(ams.idev);
+ input_sync(ams_info.idev);
- mutex_unlock(&ams.lock);
+ mutex_unlock(&ams_info.lock);
msleep(25);
}
@@ -46,56 +46,57 @@ static int ams_mouse_kthread(void *data)
return 0;
}
-/* Call with lock held! */
+/* Call with ams_info.lock held! */
static void ams_mouse_enable(void)
{
s8 x, y, z;
- if (ams.idev)
+ if (ams_info.idev)
return;
ams_sensors(&x, &y, &z);
- ams.xcalib = x;
- ams.ycalib = y;
- ams.zcalib = z;
+ ams_info.xcalib = x;
+ ams_info.ycalib = y;
+ ams_info.zcalib = z;
- ams.idev = input_allocate_device();
- if (!ams.idev)
+ ams_info.idev = input_allocate_device();
+ if (!ams_info.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)) {
- input_free_device(ams.idev);
- ams.idev = NULL;
+ ams_info.idev->name = "Apple Motion Sensor";
+ ams_info.idev->id.bustype = ams_info.bustype;
+ ams_info.idev->id.vendor = 0;
+ ams_info.idev->cdev.dev = &ams_info.of_dev->dev;
+
+ input_set_abs_params(ams_info.idev, ABS_X, -50, 50, 3, 0);
+ input_set_abs_params(ams_info.idev, ABS_Y, -50, 50, 3, 0);
+ input_set_abs_params(ams_info.idev, ABS_Z, -50, 50, 3, 0);
+
+ set_bit(EV_ABS, ams_info.idev->evbit);
+ set_bit(EV_KEY, ams_info.idev->evbit);
+
+ if (input_register_device(ams_info.idev)) {
+ input_free_device(ams_info.idev);
+ ams_info.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;
+ ams_info.kthread = kthread_run(ams_mouse_kthread, NULL, "kams");
+ if (IS_ERR(ams_info.kthread)) {
+ input_unregister_device(ams_info.idev);
+ input_free_device(ams_info.idev);
+ ams_info.idev = NULL;
return;
}
}
-/* Call with lock held! */
+/* Call with ams_info.lock held! */
static void ams_mouse_disable(void)
{
- if (ams.idev) {
- kthread_stop(ams.kthread);
- input_unregister_device(ams.idev);
- ams.idev = NULL;
+ if (ams_info.idev) {
+ kthread_stop(ams_info.kthread);
+ input_unregister_device(ams_info.idev);
+ ams_info.idev = NULL;
}
}
@@ -111,16 +112,14 @@ static ssize_t ams_mouse_store_mouse(str
if (sscanf(buf, "%d\n", &mouse) != 1)
return -EINVAL;
- mouse = !!mouse;
-
- mutex_lock(&ams.lock);
+ mutex_lock(&ams_info.lock);
if (mouse)
ams_mouse_enable();
else
ams_mouse_disable();
- mutex_unlock(&ams.lock);
+ mutex_unlock(&ams_info.lock);
return count;
}
@@ -128,18 +127,18 @@ static ssize_t ams_mouse_store_mouse(str
static DEVICE_ATTR(mouse, S_IRUGO | S_IWUSR,
ams_mouse_show_mouse, ams_mouse_store_mouse);
-/* Call with lock held! */
+/* Call with ams_info.lock held! */
void ams_mouse_init()
{
- device_create_file(&ams.of_dev->dev, &dev_attr_mouse);
+ device_create_file(&ams_info.of_dev->dev, &dev_attr_mouse);
if (mouse)
ams_mouse_enable();
}
-/* Call with lock held! */
+/* Call with ams_info.lock held! */
void ams_mouse_exit()
{
ams_mouse_disable();
- device_remove_file(&ams.of_dev->dev, &dev_attr_mouse);
+ device_remove_file(&ams_info.of_dev->dev, &dev_attr_mouse);
}
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc3.orig/drivers/hwmon/ams/ams-pmu.c linux-2.6.18-rc3/drivers/hwmon/ams/ams-pmu.c
--- linux-2.6.18-rc3.orig/drivers/hwmon/ams/ams-pmu.c 2006-08-05 00:47:46.000000000 +0200
+++ linux-2.6.18-rc3/drivers/hwmon/ams/ams-pmu.c 2006-08-05 00:49:56.000000000 +0200
@@ -84,22 +84,28 @@ static void ams_pmu_set_irq(enum ams_irq
{
if (reg & AMS_IRQ_FREEFALL) {
u8 val = ams_pmu_get_register(AMS_FF_ENABLE);
- if (enable) val |= 0x80;
- else val &= ~0x80;
+ 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;
+ if (enable)
+ val |= 0x80;
+ else
+ val &= ~0x80;
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;
+ if (enable)
+ val |= 0x80;
+ else
+ val &= ~0x80;
ams_pmu_set_register(AMS_CONTROL, val);
}
}
@@ -133,29 +139,28 @@ static void ams_pmu_exit(void)
/* Clear interrupts */
ams_pmu_clear_irq(AMS_IRQ_ALL);
- ams.has_device = 0;
+ ams_info.has_device = 0;
printk(KERN_INFO "ams: Unloading\n");
}
-/* Call with lock held! */
int __init ams_pmu_init(struct device_node *np)
{
u32 *prop;
int result;
- mutex_lock(&ams.lock);
+ mutex_lock(&ams_info.lock);
/* Set implementation stuff */
- ams.of_node = np;
- ams.exit = ams_pmu_exit;
- ams.get_vendor = ams_pmu_get_vendor;
- ams.get_xyz = ams_pmu_get_xyz;
- ams.clear_irq = ams_pmu_clear_irq;
- ams.bustype = BUS_HOST;
+ ams_info.of_node = np;
+ ams_info.exit = ams_pmu_exit;
+ ams_info.get_vendor = ams_pmu_get_vendor;
+ ams_info.get_xyz = ams_pmu_get_xyz;
+ ams_info.clear_irq = ams_pmu_clear_irq;
+ ams_info.bustype = BUS_HOST;
/* Get PMU command, should be 0x4e, but we can never know */
- prop = (u32*)get_property(ams.of_node, "reg", NULL);
+ prop = (u32*)get_property(ams_info.of_node, "reg", NULL);
if (!prop) {
result = -ENODEV;
goto exit;
@@ -186,7 +191,7 @@ int __init ams_pmu_init(struct device_no
/* Clear interrupts */
ams_pmu_clear_irq(AMS_IRQ_ALL);
- ams.has_device = 1;
+ ams_info.has_device = 1;
/* Enable interrupts */
ams_pmu_set_irq(AMS_IRQ_ALL, 1);
@@ -196,7 +201,7 @@ int __init ams_pmu_init(struct device_no
result = 0;
exit:
- mutex_unlock(&ams.lock);
+ mutex_unlock(&ams_info.lock);
return result;
}
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (13 preceding siblings ...)
2006-08-04 23:03 ` Michael Hanselmann
@ 2006-08-05 7:26 ` Stelian Pop
2006-08-06 11:41 ` Johannes Berg
` (12 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Stelian Pop @ 2006-08-05 7:26 UTC (permalink / raw)
To: lm-sensors
Le samedi 05 ao?t 2006 ? 01:03 +0200, Michael Hanselmann a ?crit :
> On Fri, Aug 04, 2006 at 03:19:52PM -0700, Andrew Morton wrote:
> > 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>
You can also add my
Signed-off-by: Stelian Pop <stelian at popies.net>
to the whole ams patch.
Thanks,
Stelian.
--
Stelian Pop <stelian at popies.net>
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (14 preceding siblings ...)
2006-08-05 7:26 ` Stelian Pop
@ 2006-08-06 11:41 ` Johannes Berg
2006-08-06 14:38 ` Michael Hanselmann
` (11 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Johannes Berg @ 2006-08-06 11:41 UTC (permalink / raw)
To: lm-sensors
On Wed, 2006-08-02 at 21:15 +0200, Michael Hanselmann wrote:
> +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
> +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
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (15 preceding siblings ...)
2006-08-06 11:41 ` Johannes Berg
@ 2006-08-06 14:38 ` Michael Hanselmann
2006-08-07 7:09 ` Johannes Berg
` (10 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Michael Hanselmann @ 2006-08-06 14:38 UTC (permalink / raw)
To: lm-sensors
On Sun, Aug 06, 2006 at 01:41:02PM +0200, Johannes Berg wrote:
> 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.
> 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?
---
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc3.orig/drivers/hwmon/Kconfig linux-2.6.18-rc3/drivers/hwmon/Kconfig
--- linux-2.6.18-rc3.orig/drivers/hwmon/Kconfig 2006-08-06 14:26:13.000000000 +0200
+++ linux-2.6.18-rc3/drivers/hwmon/Kconfig 2006-08-06 16:31:11.000000000 +0200
@@ -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
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (16 preceding siblings ...)
2006-08-06 14:38 ` Michael Hanselmann
@ 2006-08-07 7:09 ` Johannes Berg
2006-08-07 8:02 ` Jean Delvare
` (9 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Johannes Berg @ 2006-08-07 7:09 UTC (permalink / raw)
To: lm-sensors
Michael Hanselmann wrote:
> + 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
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (17 preceding siblings ...)
2006-08-07 7:09 ` Johannes Berg
@ 2006-08-07 8:02 ` Jean Delvare
2006-08-07 8:06 ` Johannes Berg
` (8 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Jean Delvare @ 2006-08-07 8:02 UTC (permalink / raw)
To: lm-sensors
> Michael Hanselmann wrote:
> > + 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.
--
Jean Delvare
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (18 preceding siblings ...)
2006-08-07 8:02 ` Jean Delvare
@ 2006-08-07 8:06 ` Johannes Berg
2006-08-07 9:28 ` Jean Delvare
` (7 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Johannes Berg @ 2006-08-07 8:06 UTC (permalink / raw)
To: lm-sensors
Jean Delvare wrote:
> 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
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (19 preceding siblings ...)
2006-08-07 8:06 ` Johannes Berg
@ 2006-08-07 9:28 ` Jean Delvare
2006-08-07 9:36 ` Johannes Berg
` (6 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Jean Delvare @ 2006-08-07 9:28 UTC (permalink / raw)
To: lm-sensors
> Jean Delvare wrote:
> > 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 :)
--
Jean Delvare
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (20 preceding siblings ...)
2006-08-07 9:28 ` Jean Delvare
@ 2006-08-07 9:36 ` Johannes Berg
2006-08-09 10:07 ` Benjamin Herrenschmidt
` (5 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Johannes Berg @ 2006-08-07 9:36 UTC (permalink / raw)
To: lm-sensors
Jean Delvare wrote:
> 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
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (21 preceding siblings ...)
2006-08-07 9:36 ` Johannes Berg
@ 2006-08-09 10:07 ` Benjamin Herrenschmidt
2006-08-09 10:37 ` Benjamin Herrenschmidt
` (4 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Benjamin Herrenschmidt @ 2006-08-09 10:07 UTC (permalink / raw)
To: lm-sensors
On Fri, 2006-08-04 at 23:49 +0200, Michael Hanselmann wrote:
> On Fri, Aug 04, 2006 at 05:39:32PM -0300, Aristeu Sergio Rozanski Filho wrote:
> > > 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 :)
> 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
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (22 preceding siblings ...)
2006-08-09 10:07 ` Benjamin Herrenschmidt
@ 2006-08-09 10:37 ` Benjamin Herrenschmidt
2006-08-09 12:17 ` Michael Hanselmann
` (3 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Benjamin Herrenschmidt @ 2006-08-09 10:37 UTC (permalink / raw)
To: lm-sensors
On Mon, 2006-08-07 at 11:36 +0200, Johannes Berg wrote:
> Jean Delvare wrote:
> > 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.
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (23 preceding siblings ...)
2006-08-09 10:37 ` Benjamin Herrenschmidt
@ 2006-08-09 12:17 ` Michael Hanselmann
2006-08-09 12:25 ` Johannes Berg
` (2 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Michael Hanselmann @ 2006-08-09 12:17 UTC (permalink / raw)
To: lm-sensors
On Mon, Aug 07, 2006 at 10:06:38AM +0200, Johannes Berg wrote:
> 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
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (24 preceding siblings ...)
2006-08-09 12:17 ` Michael Hanselmann
@ 2006-08-09 12:25 ` Johannes Berg
2006-08-09 12:39 ` Johannes Berg
2006-08-10 18:33 ` Michael Hanselmann
27 siblings, 0 replies; 29+ messages in thread
From: Johannes Berg @ 2006-08-09 12:25 UTC (permalink / raw)
To: lm-sensors
Michael Hanselmann wrote:
> 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
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (25 preceding siblings ...)
2006-08-09 12:25 ` Johannes Berg
@ 2006-08-09 12:39 ` Johannes Berg
2006-08-10 18:33 ` Michael Hanselmann
27 siblings, 0 replies; 29+ messages in thread
From: Johannes Berg @ 2006-08-09 12:39 UTC (permalink / raw)
To: lm-sensors
Benjamin Herrenschmidt wrote:
> Old ones are...
>
They also don't have an acceleration sensor :)
johannes
^ permalink raw reply [flat|nested] 29+ messages in thread* [lm-sensors] [PATCH] Apple Motion Sensor driver
2006-08-02 19:15 [lm-sensors] [PATCH] Apple Motion Sensor driver Michael Hanselmann
` (26 preceding siblings ...)
2006-08-09 12:39 ` Johannes Berg
@ 2006-08-10 18:33 ` Michael Hanselmann
27 siblings, 0 replies; 29+ messages in thread
From: Michael Hanselmann @ 2006-08-10 18:33 UTC (permalink / raw)
To: lm-sensors
On Sun, Aug 06, 2006 at 01:41:02PM +0200, Johannes Berg wrote:
> On Wed, 2006-08-02 at 21:15 +0200, Michael Hanselmann wrote:
> > +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>
---
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc3.orig/drivers/hwmon/Kconfig linux-2.6.18-rc3/drivers/hwmon/Kconfig
--- linux-2.6.18-rc3.orig/drivers/hwmon/Kconfig 2006-08-06 14:26:13.000000000 +0200
+++ linux-2.6.18-rc3/drivers/hwmon/Kconfig 2006-08-06 16:31:11.000000000 +0200
@@ -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
^ permalink raw reply [flat|nested] 29+ messages in thread