LinuxLists.cc - leds-hp-disk vs lis3lv02d

2008-10-25 10:42:49

Subject: leds-hp-disk vs lis3lv02d

Pavel Machek schreef:
> Hi!
>
>> shouldn't this one be integrated into Eric's/Yan's driver?
>
> It is separate piece of hardware, completely unrelated to lids3v
> chip. No, it should not be integrated, but I'll makesure they work
> toggether well.
Hello,
I think I talked too fast: it seems impossible to have both drivers
(leds-hp-disk and lis3lv02d) working at the same time. Only the first
driver loaded is used.

After a little look at it, I think it comes from the fact that both
drivers are assigned to the same MODALIAS (HPQ0004). The ACPI PNP
(through the generic bus infrastructure) only declare the device to one
of the drivers supporting it, not all of them.

How can I tell to ACPI that it should load both drivers for the same PNP
ID match?

Eric

2008-10-27 09:05:20

by Pavel Machek

[permalink] [raw]

Subject: Re: leds-hp-disk vs lis3lv02d

Hi!

> >> shouldn't this one be integrated into Eric's/Yan's driver?
> >
> > It is separate piece of hardware, completely unrelated to lids3v
> > chip. No, it should not be integrated, but I'll makesure they work
> > toggether well.
> Hello,
> I think I talked too fast: it seems impossible to have both drivers
> (leds-hp-disk and lis3lv02d) working at the same time. Only the first
> driver loaded is used.
>
> After a little look at it, I think it comes from the fact that both
> drivers are assigned to the same MODALIAS (HPQ0004). The ACPI PNP
> (through the generic bus infrastructure) only declare the device to one
> of the drivers supporting it, not all of them.

I can reproduce it here and it obviously needs fixing. OTOH it should
not block merge; both drivers still work and are useful.

> How can I tell to ACPI that it should load both drivers for the same PNP
> ID match?

I'll take a look if I can figure something out...

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

2008-10-27 12:45:24

by Thomas Renninger

[permalink] [raw]

Subject: Re: leds-hp-disk vs lis3lv02d

On Sunday 26 October 2008 18:40:25 Pavel Machek wrote:
> Hi!
>
> > >> shouldn't this one be integrated into Eric's/Yan's driver?
> > >
> > > It is separate piece of hardware, completely unrelated to lids3v
> > > chip. No, it should not be integrated, but I'll makesure they work
> > > toggether well.
> >
> > Hello,
> > I think I talked too fast: it seems impossible to have both drivers
> > (leds-hp-disk and lis3lv02d) working at the same time. Only the first
> > driver loaded is used.
> >
> > After a little look at it, I think it comes from the fact that both
> > drivers are assigned to the same MODALIAS (HPQ0004). The ACPI PNP
> > (through the generic bus infrastructure) only declare the device to one
> > of the drivers supporting it, not all of them.
>
> I can reproduce it here and it obviously needs fixing.
These are the first ACPI drivers who register for the same ACPI Hardware ID.

> OTOH it should
> not block merge; both drivers still work and are useful.
But for long-term the HPQ0004 specific things in the lids3v driver should get
merged with your HP leds driver also registering for HPQ0004 and the lids3v
specific things should get a separate driver which HPQ0004 driver makes use
of?
So in the end also for the HPQ0004 device only one driver should register for?
>
> > How can I tell to ACPI that it should load both drivers for the same PNP
> > ID match?
>
> I'll take a look if I can figure something out...
Kay may know whether this should work and how.
IMO having several drivers registering for the same HID should get avoided if
possible, it's confusing.

Thomas

Pavel: I am also not sure whether it's a good idea to mis-use the HP's LED for
mail notification or similar or to expose this one as a general LED. This LED
is intended to be used with the disk parking feature by the vendor?
I don't know the LED interface well, but if it's now possible that every mail
or other app can reprogram the disk parking LED for it's own purposes, this
sounds wrong. The LED can still be accessed and activated, e.g. when disk
gets parked behind the OS'es back. If this is a nice hack to e.g. use LED for
suspend debugging or similar, then it should be well hidden to the outside
world.

2008-10-27 12:58:38

by Kay Sievers

[permalink] [raw]

Subject: Re: leds-hp-disk vs lis3lv02d

On Mon, Oct 27, 2008 at 13:45, Thomas Renninger <[email protected]> wrote:
> On Sunday 26 October 2008 18:40:25 Pavel Machek wrote:

>> > How can I tell to ACPI that it should load both drivers for the same PNP
>> > ID match?
>>
>> I'll take a look if I can figure something out...
> Kay may know whether this should work and how.
> IMO having several drivers registering for the same HID should get avoided if
> possible, it's confusing.

There is currently no driver core support to bind two "struct device"
to one parent "struct device" from different drivers.

You can work around that by creating a custom "struct bus_type",
create the several function devices there, and let them bind different
drivers. This is what matches a lot of hardware like custom devices
all hiding behind a single PCI device.

Or the code for the two functions must live in the same driver, and
create class devices, so it looks to the core like a single binding.

There is ongoing work in the driver core to allow
multi-driver-binding, but it's not ready to use today.

Kay

2008-10-27 13:03:54

by Pavel Machek

[permalink] [raw]

Subject: Re: leds-hp-disk vs lis3lv02d

> > OTOH it should
> > not block merge; both drivers still work and are useful.
> But for long-term the HPQ0004 specific things in the lids3v driver should get
> merged with your HP leds driver also registering for HPQ0004 and the lids3v
> specific things should get a separate driver which HPQ0004 driver makes use
> of?

Yep, that sounds sane.

> Pavel: I am also not sure whether it's a good idea to mis-use the HP's LED for
> mail notification or similar or to expose this one as a general LED. This LED
> is intended to be used with the disk parking feature by the vendor?
> I don't know the LED interface well, but if it's now possible that
> every mail

That's how LED interface works. This _is_ general LED.

Check permissions in /sys; root permissons should be needed to
blink the LED.

> or other app can reprogram the disk parking LED for it's own purposes, this
> sounds wrong. The LED can still be accessed and activated, e.g. when disk
> gets parked behind the OS'es back. If this is a nice hack to e.g. use LED for
> suspend debugging or similar, then it should be well hidden to the outside
> world.

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

2008-11-05 22:58:47

by Pavel Machek

[permalink] [raw]

Subject: Re: leds-hp-disk vs lis3lv02d

On Sat 2008-10-25 12:42:31, Eric Piel wrote:
> Pavel Machek schreef:
> > Hi!
> >
> >> shouldn't this one be integrated into Eric's/Yan's driver?
> >
> > It is separate piece of hardware, completely unrelated to lids3v
> > chip. No, it should not be integrated, but I'll makesure they work
> > toggether well.
> Hello,
> I think I talked too fast: it seems impossible to have both drivers
> (leds-hp-disk and lis3lv02d) working at the same time. Only the first
> driver loaded is used.
>
> After a little look at it, I think it comes from the fact that both
> drivers are assigned to the same MODALIAS (HPQ0004). The ACPI PNP
> (through the generic bus infrastructure) only declare the device to one
> of the drivers supporting it, not all of them.
>
> How can I tell to ACPI that it should load both drivers for the same PNP
> ID match?

Ok, I guess lis3lv.c should be split into hw-dependend and
acpi-dependend parts, and then we can merge led driver and the acpi
parts....?

This is version that compiles :-)... relative to latest -mm.

Pavel

diff -ur clean-mm/drivers/hwmon/Makefile linux-mm/drivers/hwmon/Makefile
--- clean-mm/drivers/hwmon/Makefile 2008-11-05 23:48:31.000000000 +0100
+++ linux-mm/drivers/hwmon/Makefile 2008-11-05 23:22:15.000000000 +0100
@@ -48,7 +48,7 @@
obj-$(CONFIG_SENSORS_IBMPEX) += ibmpex.o
obj-$(CONFIG_SENSORS_IT87) += it87.o
obj-$(CONFIG_SENSORS_K8TEMP) += k8temp.o
-obj-$(CONFIG_SENSORS_LIS3LV02D) += lis3lv02d.o
+obj-$(CONFIG_SENSORS_LIS3LV02D) += lis3lv02d.o hp_accel.o
obj-$(CONFIG_SENSORS_LM63) += lm63.o
obj-$(CONFIG_SENSORS_LM70) += lm70.o
obj-$(CONFIG_SENSORS_LM75) += lm75.o
diff -ur clean-mm/drivers/hwmon/hp_accel.c linux-mm/drivers/hwmon/hp_accel.c
--- clean-mm/drivers/hwmon/hp_accel.c 2008-11-05 23:53:06.000000000 +0100
+++ linux-mm/drivers/hwmon/hp_accel.c 2008-11-05 23:49:16.000000000 +0100
@@ -0,0 +1,261 @@
+/*
+ * hp_accel.c - Interface between LIS3LV02DL driver and HP ACPI BIOS
+ *
+ * Copyright (C) 2007-2008 Yan Burman
+ * Copyright (C) 2008 Eric Piel
+ * Copyright (C) 2008 Pavel Machek
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * 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., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/dmi.h>
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/input.h>
+#include <linux/kthread.h>
+#include <linux/semaphore.h>
+#include <linux/delay.h>
+#include <linux/wait.h>
+#include <linux/poll.h>
+#include <linux/freezer.h>
+#include <linux/version.h>
+#include <linux/uaccess.h>
+#include <acpi/acpi_drivers.h>
+#include <asm/atomic.h>
+#include "lis3lv02d.h"
+
+#define DRIVER_NAME "lis3lv02d"
+#define ACPI_MDPS_CLASS "accelerometer"
+
+
+/* For automatic insertion of the module */
+static struct acpi_device_id lis3lv02d_device_ids[] = {
+ {"HPQ0004", 0}, /* HP Mobile Data Protection System PNP */
+ {"", 0},
+};
+MODULE_DEVICE_TABLE(acpi, lis3lv02d_device_ids);
+
+
+/**
+ * lis3lv02d_acpi_init - ACPI _INI method: initialize the device.
+ * @handle: the handle of the device
+ *
+ * Returns AE_OK on success.
+ */
+acpi_status lis3lv02d_acpi_init(acpi_handle handle)
+{
+ return acpi_evaluate_object(handle, METHOD_NAME__INI, NULL, NULL);
+}
+
+/**
+ * lis3lv02d_acpi_read - ACPI ALRD method: read a register
+ * @handle: the handle of the device
+ * @reg: the register to read
+ * @ret: result of the operation
+ *
+ * Returns AE_OK on success.
+ */
+acpi_status lis3lv02d_acpi_read(acpi_handle handle, int reg, u8 *ret)
+{
+ union acpi_object arg0 = { ACPI_TYPE_INTEGER };
+ struct acpi_object_list args = { 1, &arg0 };
+ unsigned long long lret;
+ acpi_status status;
+
+ arg0.integer.value = reg;
+
+ status = acpi_evaluate_integer(handle, "ALRD", &args, &lret);
+ *ret = lret;
+ return status;
+}
+
+/**
+ * lis3lv02d_acpi_write - ACPI ALWR method: write to a register
+ * @handle: the handle of the device
+ * @reg: the register to write to
+ * @val: the value to write
+ *
+ * Returns AE_OK on success.
+ */
+acpi_status lis3lv02d_acpi_write(acpi_handle handle, int reg, u8 val)
+{
+ unsigned long long ret; /* Not used when writting */
+ union acpi_object in_obj[2];
+ struct acpi_object_list args = { 2, in_obj };
+
+ in_obj[0].type = ACPI_TYPE_INTEGER;
+ in_obj[0].integer.value = reg;
+ in_obj[1].type = ACPI_TYPE_INTEGER;
+ in_obj[1].integer.value = val;
+
+ return acpi_evaluate_integer(handle, "ALWR", &args, &ret);
+}
+
+static int lis3lv02d_dmi_matched(const struct dmi_system_id *dmi)
+{
+ adev.ac = *((struct axis_conversion *)dmi->driver_data);
+ printk(KERN_INFO DRIVER_NAME ": hardware type %s found.\n", dmi->ident);
+
+ return 1;
+}
+
+/* Represents, for each axis seen by userspace, the corresponding hw axis (+1).
+ * If the value is negative, the opposite of the hw value is used. */
+static struct axis_conversion lis3lv02d_axis_normal = {1, 2, 3};
+static struct axis_conversion lis3lv02d_axis_y_inverted = {1, -2, 3};
+static struct axis_conversion lis3lv02d_axis_x_inverted = {-1, 2, 3};
+static struct axis_conversion lis3lv02d_axis_z_inverted = {1, 2, -3};
+static struct axis_conversion lis3lv02d_axis_xy_rotated_left = {-2, 1, 3};
+static struct axis_conversion lis3lv02d_axis_xy_swap_inverted = {-2, -1, 3};
+
+#define AXIS_DMI_MATCH(_ident, _name, _axis) { \
+ .ident = _ident, \
+ .callback = lis3lv02d_dmi_matched, \
+ .matches = { \
+ DMI_MATCH(DMI_PRODUCT_NAME, _name) \
+ }, \
+ .driver_data = &lis3lv02d_axis_##_axis \
+}
+static struct dmi_system_id lis3lv02d_dmi_ids[] = {
+ /* product names are truncated to match all kinds of a same model */
+ AXIS_DMI_MATCH("NC64x0", "HP Compaq nc64", x_inverted),
+ AXIS_DMI_MATCH("NC84x0", "HP Compaq nc84", z_inverted),
+ AXIS_DMI_MATCH("NX9420", "HP Compaq nx9420", x_inverted),
+ AXIS_DMI_MATCH("NW9440", "HP Compaq nw9440", x_inverted),
+ AXIS_DMI_MATCH("NC2510", "HP Compaq 2510", y_inverted),
+ AXIS_DMI_MATCH("NC8510", "HP Compaq 8510", xy_swap_inverted),
+ AXIS_DMI_MATCH("HP2133", "HP 2133", xy_rotated_left),
+ { NULL, }
+/* Laptop models without axis info (yet):
+ * "NC651xx" "HP Compaq 651"
+ * "NC671xx" "HP Compaq 671"
+ * "NC6910" "HP Compaq 6910"
+ * HP Compaq 8710x Notebook PC / Mobile Workstation
+ * "NC2400" "HP Compaq nc2400"
+ * "NX74x0" "HP Compaq nx74"
+ * "NX6325" "HP Compaq nx6325"
+ * "NC4400" "HP Compaq nc4400"
+ */
+};
+
+
+static int lis3lv02d_add(struct acpi_device *device)
+{
+ u8 val;
+
+ if (!device)
+ return -EINVAL;
+
+ adev.device = device;
+ strcpy(acpi_device_name(device), DRIVER_NAME);
+ strcpy(acpi_device_class(device), ACPI_MDPS_CLASS);
+ device->driver_data = &adev;
+
+ lis3lv02d_acpi_read(device->handle, WHO_AM_I, &val);
+ if ((val != LIS3LV02DL_ID) && (val != LIS302DL_ID)) {
+ printk(KERN_ERR DRIVER_NAME
+ ": Accelerometer chip not LIS3LV02D{L,Q}\n");
+ }
+
+ /* If possible use a "standard" axes order */
+ if (dmi_check_system(lis3lv02d_dmi_ids) == 0) {
+ printk(KERN_INFO DRIVER_NAME ": laptop model unknown, "
+ "using default axes configuration\n");
+ adev.ac = lis3lv02d_axis_normal;
+ }
+
+ return lis3lv02d_init_device(&adev);
+}
+
+static int lis3lv02d_remove(struct acpi_device *device, int type)
+{
+ if (!device)
+ return -EINVAL;
+
+ lis3lv02d_joystick_disable();
+ lis3lv02d_poweroff(device->handle);
+
+ return lis3lv02d_remove_fs();
+}
+
+
+#ifdef CONFIG_PM
+static int lis3lv02d_suspend(struct acpi_device *device, pm_message_t state)
+{
+ /* make sure the device is off when we suspend */
+ lis3lv02d_poweroff(device->handle);
+ return 0;
+}
+
+static int lis3lv02d_resume(struct acpi_device *device)
+{
+ /* put back the device in the right state (ACPI might turn it on) */
+ mutex_lock(&adev.lock);
+ if (adev.usage > 0)
+ lis3lv02d_poweron(device->handle);
+ else
+ lis3lv02d_poweroff(device->handle);
+ mutex_unlock(&adev.lock);
+ return 0;
+}
+#else
+#define lis3lv02d_suspend NULL
+#define lis3lv02d_resume NULL
+#endif
+
+/* For the HP MDPS aka 3D Driveguard */
+static struct acpi_driver lis3lv02d_driver = {
+ .name = DRIVER_NAME,
+ .class = ACPI_MDPS_CLASS,
+ .ids = lis3lv02d_device_ids,
+ .ops = {
+ .add = lis3lv02d_add,
+ .remove = lis3lv02d_remove,
+ .suspend = lis3lv02d_suspend,
+ .resume = lis3lv02d_resume,
+ }
+};
+
+static int __init lis3lv02d_init_module(void)
+{
+ int ret;
+
+ if (acpi_disabled)
+ return -ENODEV;
+
+ ret = acpi_bus_register_driver(&lis3lv02d_driver);
+ if (ret < 0)
+ return ret;
+
+ printk(KERN_INFO DRIVER_NAME " driver loaded.\n");
+
+ return 0;
+}
+
+static void __exit lis3lv02d_exit_module(void)
+{
+ acpi_bus_unregister_driver(&lis3lv02d_driver);
+}
+
+MODULE_DESCRIPTION("Glue between LIS3LV02Dx and HP ACPI BIOS");
+MODULE_AUTHOR("Yan Burman, Eric Piel, Pavel Machek");
+MODULE_LICENSE("GPL");
+
+module_init(lis3lv02d_init_module);
+module_exit(lis3lv02d_exit_module);
diff -ur clean-mm/drivers/hwmon/lis3lv02d.c linux-mm/drivers/hwmon/lis3lv02d.c
--- clean-mm/drivers/hwmon/lis3lv02d.c 2008-11-05 23:48:31.000000000 +0100
+++ linux-mm/drivers/hwmon/lis3lv02d.c 2008-11-05 23:53:41.000000000 +0100
@@ -3,6 +3,7 @@
*
* Copyright (C) 2007-2008 Yan Burman
* Copyright (C) 2008 Eric Piel
+ * Copyright (C) 2008 Pavel Machek
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -42,6 +43,7 @@
#define DRIVER_NAME "lis3lv02d"
#define ACPI_MDPS_CLASS "accelerometer"

+
/* joystick device poll interval in milliseconds */
#define MDPS_POLL_INTERVAL 50
/*
@@ -56,94 +58,12 @@
/* Maximum value our axis may get for the input device (signed 12 bits) */
#define MDPS_MAX_VAL 2048

-struct axis_conversion {
- s8 x;
- s8 y;
- s8 z;
-};

-struct acpi_lis3lv02d {
- struct acpi_device *device; /* The ACPI device */
- struct input_dev *idev; /* input device */
- struct task_struct *kthread; /* kthread for input */
- struct mutex lock;
- struct platform_device *pdev; /* platform device */
- atomic_t count; /* interrupt count after last read */
- int xcalib; /* calibrated null value for x */
- int ycalib; /* calibrated null value for y */
- int zcalib; /* calibrated null value for z */
- unsigned char is_on; /* whether the device is on or off */
- unsigned char usage; /* usage counter */
- struct axis_conversion ac; /* hw -> logical axis */
-};

-static struct acpi_lis3lv02d adev;
+struct acpi_lis3lv02d adev;

-static int lis3lv02d_remove_fs(void);
static int lis3lv02d_add_fs(struct acpi_device *device);

-/* For automatic insertion of the module */
-static struct acpi_device_id lis3lv02d_device_ids[] = {
- {"HPQ0004", 0}, /* HP Mobile Data Protection System PNP */
- {"", 0},
-};
-MODULE_DEVICE_TABLE(acpi, lis3lv02d_device_ids);
-
-/**
- * lis3lv02d_acpi_init - ACPI _INI method: initialize the device.
- * @handle: the handle of the device
- *
- * Returns AE_OK on success.
- */
-static inline acpi_status lis3lv02d_acpi_init(acpi_handle handle)
-{
- return acpi_evaluate_object(handle, METHOD_NAME__INI, NULL, NULL);
-}
-
-/**
- * lis3lv02d_acpi_read - ACPI ALRD method: read a register
- * @handle: the handle of the device
- * @reg: the register to read
- * @ret: result of the operation
- *
- * Returns AE_OK on success.
- */
-static acpi_status lis3lv02d_acpi_read(acpi_handle handle, int reg, u8 *ret)
-{
- union acpi_object arg0 = { ACPI_TYPE_INTEGER };
- struct acpi_object_list args = { 1, &arg0 };
- unsigned long long lret;
- acpi_status status;
-
- arg0.integer.value = reg;
-
- status = acpi_evaluate_integer(handle, "ALRD", &args, &lret);
- *ret = lret;
- return status;
-}
-
-/**
- * lis3lv02d_acpi_write - ACPI ALWR method: write to a register
- * @handle: the handle of the device
- * @reg: the register to write to
- * @val: the value to write
- *
- * Returns AE_OK on success.
- */
-static acpi_status lis3lv02d_acpi_write(acpi_handle handle, int reg, u8 val)
-{
- unsigned long long ret; /* Not used when writting */
- union acpi_object in_obj[2];
- struct acpi_object_list args = { 2, in_obj };
-
- in_obj[0].type = ACPI_TYPE_INTEGER;
- in_obj[0].integer.value = reg;
- in_obj[1].type = ACPI_TYPE_INTEGER;
- in_obj[1].integer.value = val;
-
- return acpi_evaluate_integer(handle, "ALWR", &args, &ret);
-}
-
static s16 lis3lv02d_read_16(acpi_handle handle, int reg)
{
u8 lo, hi;
@@ -191,14 +111,14 @@
*z = lis3lv02d_get_axis(adev.ac.z, position);
}

-static inline void lis3lv02d_poweroff(acpi_handle handle)
+void lis3lv02d_poweroff(acpi_handle handle)
{
adev.is_on = 0;
/* disable X,Y,Z axis and power down */
lis3lv02d_acpi_write(handle, CTRL_REG1, 0x00);
}

-static void lis3lv02d_poweron(acpi_handle handle)
+void lis3lv02d_poweron(acpi_handle handle)
{
u8 val;

@@ -215,30 +135,6 @@
lis3lv02d_acpi_write(handle, CTRL_REG2, val);
}

-#ifdef CONFIG_PM
-static int lis3lv02d_suspend(struct acpi_device *device, pm_message_t state)
-{
- /* make sure the device is off when we suspend */
- lis3lv02d_poweroff(device->handle);
- return 0;
-}
-
-static int lis3lv02d_resume(struct acpi_device *device)
-{
- /* put back the device in the right state (ACPI might turn it on) */
- mutex_lock(&adev.lock);
- if (adev.usage > 0)
- lis3lv02d_poweron(device->handle);
- else
- lis3lv02d_poweroff(device->handle);
- mutex_unlock(&adev.lock);
- return 0;
-}
-#else
-#define lis3lv02d_suspend NULL
-#define lis3lv02d_resume NULL
-#endif
-

/*
* To be called before starting to use the device. It makes sure that the
@@ -316,7 +212,7 @@
lis3lv02d_get_xyz(adev.device->handle, &adev.xcalib, &adev.ycalib, &adev.zcalib);
}

-static int lis3lv02d_joystick_enable(void)
+int lis3lv02d_joystick_enable(void)
{
int err;

@@ -351,7 +247,7 @@
return err;
}

-static void lis3lv02d_joystick_disable(void)
+void lis3lv02d_joystick_disable(void)
{
if (!adev.idev)
return;
@@ -361,11 +257,12 @@
}

+
/*
* Initialise the accelerometer and the various subsystems.
* Should be rather independant of the bus system.
*/
-static int lis3lv02d_init_device(struct acpi_lis3lv02d *dev)
+int lis3lv02d_init_device(struct acpi_lis3lv02d *dev)
{
mutex_init(&dev->lock);
lis3lv02d_add_fs(dev->device);
@@ -378,91 +275,6 @@
return 0;
}

-static int lis3lv02d_dmi_matched(const struct dmi_system_id *dmi)
-{
- adev.ac = *((struct axis_conversion *)dmi->driver_data);
- printk(KERN_INFO DRIVER_NAME ": hardware type %s found.\n", dmi->ident);
-
- return 1;
-}
-
-/* Represents, for each axis seen by userspace, the corresponding hw axis (+1).
- * If the value is negative, the opposite of the hw value is used. */
-static struct axis_conversion lis3lv02d_axis_normal = {1, 2, 3};
-static struct axis_conversion lis3lv02d_axis_y_inverted = {1, -2, 3};
-static struct axis_conversion lis3lv02d_axis_x_inverted = {-1, 2, 3};
-static struct axis_conversion lis3lv02d_axis_z_inverted = {1, 2, -3};
-static struct axis_conversion lis3lv02d_axis_xy_rotated_left = {-2, 1, 3};
-static struct axis_conversion lis3lv02d_axis_xy_swap_inverted = {-2, -1, 3};
-
-#define AXIS_DMI_MATCH(_ident, _name, _axis) { \
- .ident = _ident, \
- .callback = lis3lv02d_dmi_matched, \
- .matches = { \
- DMI_MATCH(DMI_PRODUCT_NAME, _name) \
- }, \
- .driver_data = &lis3lv02d_axis_##_axis \
-}
-static struct dmi_system_id lis3lv02d_dmi_ids[] = {
- /* product names are truncated to match all kinds of a same model */
- AXIS_DMI_MATCH("NC64x0", "HP Compaq nc64", x_inverted),
- AXIS_DMI_MATCH("NC84x0", "HP Compaq nc84", z_inverted),
- AXIS_DMI_MATCH("NX9420", "HP Compaq nx9420", x_inverted),
- AXIS_DMI_MATCH("NW9440", "HP Compaq nw9440", x_inverted),
- AXIS_DMI_MATCH("NC2510", "HP Compaq 2510", y_inverted),
- AXIS_DMI_MATCH("NC8510", "HP Compaq 8510", xy_swap_inverted),
- AXIS_DMI_MATCH("HP2133", "HP 2133", xy_rotated_left),
- { NULL, }
-/* Laptop models without axis info (yet):
- * "NC651xx" "HP Compaq 651"
- * "NC671xx" "HP Compaq 671"
- * "NC6910" "HP Compaq 6910"
- * HP Compaq 8710x Notebook PC / Mobile Workstation
- * "NC2400" "HP Compaq nc2400"
- * "NX74x0" "HP Compaq nx74"
- * "NX6325" "HP Compaq nx6325"
- * "NC4400" "HP Compaq nc4400"
- */
-};
-
-static int lis3lv02d_add(struct acpi_device *device)
-{
- u8 val;
-
- if (!device)
- return -EINVAL;
-
- adev.device = device;
- strcpy(acpi_device_name(device), DRIVER_NAME);
- strcpy(acpi_device_class(device), ACPI_MDPS_CLASS);
- device->driver_data = &adev;
-
- lis3lv02d_acpi_read(device->handle, WHO_AM_I, &val);
- if ((val != LIS3LV02DL_ID) && (val != LIS302DL_ID)) {
- printk(KERN_ERR DRIVER_NAME
- ": Accelerometer chip not LIS3LV02D{L,Q}\n");
- }
-
- /* If possible use a "standard" axes order */
- if (dmi_check_system(lis3lv02d_dmi_ids) == 0) {
- printk(KERN_INFO DRIVER_NAME ": laptop model unknown, "
- "using default axes configuration\n");
- adev.ac = lis3lv02d_axis_normal;
- }
-
- return lis3lv02d_init_device(&adev);
-}
-
-static int lis3lv02d_remove(struct acpi_device *device, int type)
-{
- if (!device)
- return -EINVAL;
-
- lis3lv02d_joystick_disable();
- lis3lv02d_poweroff(device->handle);
-
- return lis3lv02d_remove_fs();
-}

/* Sysfs stuff */
@@ -524,6 +336,7 @@
.attrs = lis3lv02d_attributes
};

+
static int lis3lv02d_add_fs(struct acpi_device *device)
{
adev.pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
@@ -533,50 +346,13 @@
return sysfs_create_group(&adev.pdev->dev.kobj, &lis3lv02d_attribute_group);
}

-static int lis3lv02d_remove_fs(void)
+int lis3lv02d_remove_fs(void)
{
sysfs_remove_group(&adev.pdev->dev.kobj, &lis3lv02d_attribute_group);
platform_device_unregister(adev.pdev);
return 0;
}

-/* For the HP MDPS aka 3D Driveguard */
-static struct acpi_driver lis3lv02d_driver = {
- .name = DRIVER_NAME,
- .class = ACPI_MDPS_CLASS,
- .ids = lis3lv02d_device_ids,
- .ops = {
- .add = lis3lv02d_add,
- .remove = lis3lv02d_remove,
- .suspend = lis3lv02d_suspend,
- .resume = lis3lv02d_resume,
- }
-};
-
-static int __init lis3lv02d_init_module(void)
-{
- int ret;
-
- if (acpi_disabled)
- return -ENODEV;
-
- ret = acpi_bus_register_driver(&lis3lv02d_driver);
- if (ret < 0)
- return ret;
-
- printk(KERN_INFO DRIVER_NAME " driver loaded.\n");
-
- return 0;
-}
-
-static void __exit lis3lv02d_exit_module(void)
-{
- acpi_bus_unregister_driver(&lis3lv02d_driver);
-}
-
MODULE_DESCRIPTION("ST LIS3LV02Dx three-axis digital accelerometer driver");
MODULE_AUTHOR("Yan Burman and Eric Piel");
MODULE_LICENSE("GPL");
-
-module_init(lis3lv02d_init_module);
-module_exit(lis3lv02d_exit_module);
Only in linux-mm/drivers/hwmon: lis3lv02d.c~
diff -ur clean-mm/drivers/hwmon/lis3lv02d.h linux-mm/drivers/hwmon/lis3lv02d.h
--- clean-mm/drivers/hwmon/lis3lv02d.h 2008-11-05 23:48:31.000000000 +0100
+++ linux-mm/drivers/hwmon/lis3lv02d.h 2008-11-05 23:52:52.000000000 +0100
@@ -147,3 +147,36 @@
DD_SRC_IA = 0x40,
};

+acpi_status lis3lv02d_acpi_init(acpi_handle handle);
+acpi_status lis3lv02d_acpi_read(acpi_handle handle, int reg, u8 *ret);
+acpi_status lis3lv02d_acpi_write(acpi_handle handle, int reg, u8 val);
+
+struct axis_conversion {
+ s8 x;
+ s8 y;
+ s8 z;
+};
+
+struct acpi_lis3lv02d {
+ struct acpi_device *device; /* The ACPI device */
+ struct input_dev *idev; /* input device */
+ struct task_struct *kthread; /* kthread for input */
+ struct mutex lock;
+ struct platform_device *pdev; /* platform device */
+ atomic_t count; /* interrupt count after last read */
+ int xcalib; /* calibrated null value for x */
+ int ycalib; /* calibrated null value for y */
+ int zcalib; /* calibrated null value for z */
+ unsigned char is_on; /* whether the device is on or off */
+ unsigned char usage; /* usage counter */
+ struct axis_conversion ac; /* hw -> logical axis */
+};
+
+int lis3lv02d_init_device(struct acpi_lis3lv02d *dev);
+int lis3lv02d_joystick_enable(void);
+void lis3lv02d_joystick_disable(void);
+void lis3lv02d_poweroff(acpi_handle handle);
+void lis3lv02d_poweron(acpi_handle handle);
+int lis3lv02d_remove_fs(void);
+
+extern struct acpi_lis3lv02d adev;

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

2008-11-06 12:14:24

by Pavel Machek

[permalink] [raw]

Subject: Re: leds-hp-disk vs lis3lv02d

Hi!

> > > I think I talked too fast: it seems impossible to have both drivers
> > > (leds-hp-disk and lis3lv02d) working at the same time. Only the first
> > > driver loaded is used.
> > >
> > > After a little look at it, I think it comes from the fact that both
> > > drivers are assigned to the same MODALIAS (HPQ0004). The ACPI PNP
> > > (through the generic bus infrastructure) only declare the device to one
> > > of the drivers supporting it, not all of them.
> >
> > I can reproduce it here and it obviously needs fixing.
> These are the first ACPI drivers who register for the same ACPI Hardware ID.
>
> > OTOH it should
> > not block merge; both drivers still work and are useful.
> But for long-term the HPQ0004 specific things in the lids3v driver should get
> merged with your HP leds driver also registering for HPQ0004 and the lids3v
> specific things should get a separate driver which HPQ0004 driver makes use
> of?
> So in the end also for the HPQ0004 device only one driver should register for?

Yep... and this is the step in that direction. Relative to my previous
patch...

Later led stuff will get merged to hp_accel.c...

(Not for andrew, yet. I'd like basic support to get merged, first)
diff -ur clean-mm/drivers/hwmon/hp_accel.c linux-mm/drivers/hwmon/hp_accel.c
--- clean-mm/drivers/hwmon/hp_accel.c 2008-11-06 09:18:16.000000000 +0100
+++ linux-mm/drivers/hwmon/hp_accel.c 2008-11-06 12:26:03.000000000 +0100
@@ -163,6 +163,9 @@
return -EINVAL;

adev.device = device;
+ adev.init = lis3lv02d_acpi_init;
+ adev.read = lis3lv02d_acpi_read;
+ adev.write = lis3lv02d_acpi_write;
strcpy(acpi_device_name(device), DRIVER_NAME);
strcpy(acpi_device_class(device), ACPI_MDPS_CLASS);
device->driver_data = &adev;
@@ -259,3 +262,5 @@

module_init(lis3lv02d_init_module);
module_exit(lis3lv02d_exit_module);
+
+
diff -ur clean-mm/drivers/hwmon/lis3lv02d.c linux-mm/drivers/hwmon/lis3lv02d.c
--- clean-mm/drivers/hwmon/lis3lv02d.c 2008-11-06 09:18:16.000000000 +0100
+++ linux-mm/drivers/hwmon/lis3lv02d.c 2008-11-06 12:27:27.000000000 +0100
@@ -68,8 +68,8 @@
{
u8 lo, hi;

- lis3lv02d_acpi_read(handle, reg, &lo);
- lis3lv02d_acpi_read(handle, reg + 1, &hi);
+ adev.read(handle, reg, &lo);
+ adev.read(handle, reg + 1, &hi);
/* In "12 bit right justified" mode, bit 6, bit 7, bit 8 = bit 5 */
return (s16)((hi << 8) | lo);
}
@@ -115,7 +115,7 @@
{
adev.is_on = 0;
/* disable X,Y,Z axis and power down */
- lis3lv02d_acpi_write(handle, CTRL_REG1, 0x00);
+ adev.write(handle, CTRL_REG1, 0x00);
}

void lis3lv02d_poweron(acpi_handle handle)
@@ -123,16 +123,16 @@
u8 val;

adev.is_on = 1;
- lis3lv02d_acpi_init(handle);
- lis3lv02d_acpi_write(handle, FF_WU_CFG, 0);
+ adev.init(handle);
+ adev.write(handle, FF_WU_CFG, 0);
/*
* BDU: LSB and MSB values are not updated until both have been read.
* So the value read will always be correct.
* IEN: Interrupt for free-fall and DD, not for data-ready.
*/
- lis3lv02d_acpi_read(handle, CTRL_REG2, &val);
+ adev.read(handle, CTRL_REG2, &val);
val |= CTRL2_BDU | CTRL2_IEN;
- lis3lv02d_acpi_write(handle, CTRL_REG2, val);
+ adev.write(handle, CTRL_REG2, val);
}

@@ -314,7 +314,7 @@
int val;

lis3lv02d_increase_use(&adev);
- lis3lv02d_acpi_read(adev.device->handle, CTRL_REG1, &ctrl);
+ adev.read(adev.device->handle, CTRL_REG1, &ctrl);
lis3lv02d_decrease_use(&adev);
val = (ctrl & (CTRL1_DF0 | CTRL1_DF1)) >> 4;
return sprintf(buf, "%d\n", lis3lv02dl_df_val[val]);
@@ -356,3 +356,12 @@
MODULE_DESCRIPTION("ST LIS3LV02Dx three-axis digital accelerometer driver");
MODULE_AUTHOR("Yan Burman and Eric Piel");
MODULE_LICENSE("GPL");
+
+EXPORT_SYMBOL_GPL(lis3lv02d_init_device);
+EXPORT_SYMBOL_GPL(lis3lv02d_joystick_enable);
+EXPORT_SYMBOL_GPL(lis3lv02d_joystick_disable);
+EXPORT_SYMBOL_GPL(lis3lv02d_poweroff);
+EXPORT_SYMBOL_GPL(lis3lv02d_poweron);
+EXPORT_SYMBOL_GPL(lis3lv02d_remove_fs);
+
+EXPORT_SYMBOL_GPL(adev);
diff -ur clean-mm/drivers/hwmon/lis3lv02d.h linux-mm/drivers/hwmon/lis3lv02d.h
--- clean-mm/drivers/hwmon/lis3lv02d.h 2008-11-06 09:18:16.000000000 +0100
+++ linux-mm/drivers/hwmon/lis3lv02d.h 2008-11-06 12:24:05.000000000 +0100
@@ -148,8 +148,6 @@
};

acpi_status lis3lv02d_acpi_init(acpi_handle handle);
-acpi_status lis3lv02d_acpi_read(acpi_handle handle, int reg, u8 *ret);
-acpi_status lis3lv02d_acpi_write(acpi_handle handle, int reg, u8 val);

struct axis_conversion {
s8 x;
@@ -159,6 +157,10 @@

struct acpi_lis3lv02d {
struct acpi_device *device; /* The ACPI device */
+ acpi_status (* init) (acpi_handle handle);
+ acpi_status (* write) (acpi_handle handle, int reg, u8 val);
+ acpi_status (* read) (acpi_handle handle, int reg, u8 *ret);
+
struct input_dev *idev; /* input device */
struct task_struct *kthread; /* kthread for input */
struct mutex lock;

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

2008-11-06 21:45:36

by Éric Piel

[permalink] [raw]

Subject: Re: leds-hp-disk vs lis3lv02d

Pavel Machek schreef:
>>> OTOH it should
>>> not block merge; both drivers still work and are useful.
>> But for long-term the HPQ0004 specific things in the lids3v driver should get
>> merged with your HP leds driver also registering for HPQ0004 and the lids3v
>> specific things should get a separate driver which HPQ0004 driver makes use
>> of?
>> So in the end also for the HPQ0004 device only one driver should register for?
>
> Yep... and this is the step in that direction. Relative to my previous
> patch...
>
> Later led stuff will get merged to hp_accel.c...
Hi Pavel,
Just to say I like this approach. It should be completely compatible
with the idea of allowing different buses (SPI and I?C) as well. Of
course, I wish it could be possible to have the led and accelerometer
drivers separated because they have no common point but the fact that
they are advertised by the same PNP ID. But I can deal with that.

Andrew, is there anything preventing the merge of the lis3lv02d driver
from your tree to Linus' one? I think it's still worth to integrate it
as is, and later on this refactoring can take place.

See you,
Eric

2008-11-06 22:23:19

by Andrew Morton

[permalink] [raw]

Subject: Re: leds-hp-disk vs lis3lv02d

On Thu, 06 Nov 2008 22:45:05 +0100
__ric Piel <[email protected]> wrote:

> Andrew, is there anything preventing the merge of the lis3lv02d driver
> from your tree to Linus' one?

Nope. I have a number of shiny new drivers which I plan to send in for
2.6.28 (I sent an fbdev one this morning). I need to get down and do a
sweep to round tham all up.

2008-11-10 12:00:22

by Pavel Machek

[permalink] [raw]

Subject: Re: leds-hp-disk vs lis3lv02d

Attachments:

(No filename) (14.18 kB)
lis.4inc.diff (3.40 kB)
Download all attachments