LED on HP notebooks is connected through ACPI. That unfortunately
means that it needs to be delayed by using schedule_work() to avoid
calling ACPI interpretter in invalid context. This patch fixes that.
Signed-off-by: Pavel Machek <[email protected]>
---
commit 66d8f12491f52e259e42148af099a3fc83425a7b
tree ea1d77bc228df0ff2ef0d3983fe62fefc8bfb182
parent 84ef7973b5c6e4f4c5dae03add0a3f37057b61db
author Pavel <[email protected]> Mon, 12 Jan 2009 11:30:08 +0100
committer Pavel <[email protected]> Mon, 12 Jan 2009 11:30:08 +0100
drivers/hwmon/hp_accel.c | 71 ++++++++++++++++++++++++++++++++++------------
1 files changed, 52 insertions(+), 19 deletions(-)
diff --git a/drivers/hwmon/hp_accel.c b/drivers/hwmon/hp_accel.c
index bd8497b..6c3c592 100644
--- a/drivers/hwmon/hp_accel.c
+++ b/drivers/hwmon/hp_accel.c
@@ -3,7 +3,7 @@
*
* Copyright (C) 2007-2008 Yan Burman
* Copyright (C) 2008 Eric Piel
- * Copyright (C) 2008 Pavel Machek
+ * Copyright (C) 2008-2009 Pavel Machek
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -44,6 +44,36 @@ #include "lis3lv02d.h"
#define DRIVER_NAME "lis3lv02d"
#define ACPI_MDPS_CLASS "accelerometer"
+/* Delayed LEDs infrastructure ------------------------------------ */
+
+/* Special LED class that can defer work */
+struct delayed_led_classdev {
+ struct led_classdev led_classdev;
+ struct work_struct work;
+ enum led_brightness new_brightness;
+
+ unsigned int led; /* For driver */
+ void (*set_brightness)(struct delayed_led_classdev *data, enum led_brightness value);
+};
+
+static inline void delayed_set_status_worker(struct work_struct *work)
+{
+ struct delayed_led_classdev *data =
+ container_of(work, struct delayed_led_classdev, work);
+
+ data->set_brightness(data, data->new_brightness);
+}
+
+static inline void delayed_sysfs_set(struct led_classdev *led_cdev,
+ enum led_brightness brightness)
+{
+ struct delayed_led_classdev *data = container_of(led_cdev,
+ struct delayed_led_classdev, led_classdev);
+ data->new_brightness = brightness;
+ schedule_work(&data->work);
+}
+
+/* HP-specific accelerometer driver ------------------------------------ */
/* For automatic insertion of the module */
static struct acpi_device_id lis3lv02d_device_ids[] = {
@@ -155,28 +185,27 @@ static struct dmi_system_id lis3lv02d_dm
*/
};
-static acpi_status hpled_acpi_write(acpi_handle handle, int reg)
+static void hpled_set(struct delayed_led_classdev *led_cdev, enum led_brightness value)
{
+ acpi_handle handle = adev.device->handle;
unsigned long long ret; /* Not used when writing */
union acpi_object in_obj[1];
struct acpi_object_list args = { 1, in_obj };
in_obj[0].type = ACPI_TYPE_INTEGER;
- in_obj[0].integer.value = reg;
+ in_obj[0].integer.value = !!value;
- return acpi_evaluate_integer(handle, "ALED", &args, &ret);
+ acpi_evaluate_integer(handle, "ALED", &args, &ret);
}
-static void hpled_set(struct led_classdev *led_cdev,
- enum led_brightness value)
-{
- hpled_acpi_write(adev.device->handle, !!value);
-}
-
-static struct led_classdev hpled_led = {
- .name = "hp:red:hddprotection",
- .default_trigger = "heartbeat",
- .brightness_set = hpled_set,
+static struct delayed_led_classdev hpled_led = {
+ .led_classdev = {
+ .name = "hp::hddprotect",
+ .default_trigger = "none",
+ .brightness_set = delayed_sysfs_set,
+ .flags = LED_CORE_SUSPENDRESUME,
+ },
+ .set_brightness = hpled_set,
};
static int lis3lv02d_add(struct acpi_device *device)
@@ -208,13 +237,17 @@ static int lis3lv02d_add(struct acpi_dev
adev.ac = lis3lv02d_axis_normal;
}
- ret = led_classdev_register(NULL, &hpled_led);
+ INIT_WORK(&hpled_led.work, delayed_set_status_worker);
+ ret = led_classdev_register(NULL, &hpled_led.led_classdev);
if (ret)
return ret;
ret = lis3lv02d_init_device(&adev);
if (ret) {
- led_classdev_unregister(&hpled_led);
+ while (work_pending(&hpled_led.work))
+ schedule();
+
+ led_classdev_unregister(&hpled_led.led_classdev);
return ret;
}
@@ -229,7 +262,9 @@ static int lis3lv02d_remove(struct acpi_
lis3lv02d_joystick_disable();
lis3lv02d_poweroff(device->handle);
- led_classdev_unregister(&hpled_led);
+ while (work_pending(&hpled_led.work))
+ schedule();
+ led_classdev_unregister(&hpled_led.led_classdev);
return lis3lv02d_remove_fs();
}
@@ -240,7 +275,6 @@ static int lis3lv02d_suspend(struct acpi
{
/* make sure the device is off when we suspend */
lis3lv02d_poweroff(device->handle);
- led_classdev_suspend(&hpled_led);
return 0;
}
@@ -253,7 +287,6 @@ static int lis3lv02d_resume(struct acpi_
else
lis3lv02d_poweroff(device->handle);
mutex_unlock(&adev.lock);
- led_classdev_resume(&hpled_led);
return 0;
}
#else
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Mon, 12 Jan 2009 11:31:55 +0100
Pavel Machek <[email protected]> wrote:
>
> LED on HP notebooks is connected through ACPI. That unfortunately
> means that it needs to be delayed by using schedule_work() to avoid
> calling ACPI interpretter in invalid context. This patch fixes that.
>
> Signed-off-by: Pavel Machek <[email protected]>
>
> ---
> commit 66d8f12491f52e259e42148af099a3fc83425a7b
> tree ea1d77bc228df0ff2ef0d3983fe62fefc8bfb182
> parent 84ef7973b5c6e4f4c5dae03add0a3f37057b61db
> author Pavel <[email protected]> Mon, 12 Jan 2009 11:30:08 +0100
> committer Pavel <[email protected]> Mon, 12 Jan 2009 11:30:08 +0100
>
> drivers/hwmon/hp_accel.c | 71 ++++++++++++++++++++++++++++++++++------------
> 1 files changed, 52 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/hwmon/hp_accel.c b/drivers/hwmon/hp_accel.c
> index bd8497b..6c3c592 100644
> --- a/drivers/hwmon/hp_accel.c
> +++ b/drivers/hwmon/hp_accel.c
> @@ -3,7 +3,7 @@
> *
> * Copyright (C) 2007-2008 Yan Burman
> * Copyright (C) 2008 Eric Piel
> - * Copyright (C) 2008 Pavel Machek
> + * Copyright (C) 2008-2009 Pavel Machek
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License as published by
> @@ -44,6 +44,36 @@ #include "lis3lv02d.h"
> #define DRIVER_NAME "lis3lv02d"
> #define ACPI_MDPS_CLASS "accelerometer"
>
> +/* Delayed LEDs infrastructure ------------------------------------ */
> +
> +/* Special LED class that can defer work */
> +struct delayed_led_classdev {
> + struct led_classdev led_classdev;
> + struct work_struct work;
> + enum led_brightness new_brightness;
> +
> + unsigned int led; /* For driver */
> + void (*set_brightness)(struct delayed_led_classdev *data, enum led_brightness value);
> +};
> +
> +static inline void delayed_set_status_worker(struct work_struct *work)
> +{
> + struct delayed_led_classdev *data =
> + container_of(work, struct delayed_led_classdev, work);
> +
> + data->set_brightness(data, data->new_brightness);
> +}
> +
> +static inline void delayed_sysfs_set(struct led_classdev *led_cdev,
> + enum led_brightness brightness)
> +{
> + struct delayed_led_classdev *data = container_of(led_cdev,
> + struct delayed_led_classdev, led_classdev);
> + data->new_brightness = brightness;
> + schedule_work(&data->work);
> +}
> +
> +/* HP-specific accelerometer driver ------------------------------------ */
>
> /* For automatic insertion of the module */
> static struct acpi_device_id lis3lv02d_device_ids[] = {
> @@ -155,28 +185,27 @@ static struct dmi_system_id lis3lv02d_dm
> */
> };
>
> -static acpi_status hpled_acpi_write(acpi_handle handle, int reg)
> +static void hpled_set(struct delayed_led_classdev *led_cdev, enum led_brightness value)
> {
> + acpi_handle handle = adev.device->handle;
> unsigned long long ret; /* Not used when writing */
> union acpi_object in_obj[1];
> struct acpi_object_list args = { 1, in_obj };
>
> in_obj[0].type = ACPI_TYPE_INTEGER;
> - in_obj[0].integer.value = reg;
> + in_obj[0].integer.value = !!value;
>
> - return acpi_evaluate_integer(handle, "ALED", &args, &ret);
> + acpi_evaluate_integer(handle, "ALED", &args, &ret);
> }
>
> -static void hpled_set(struct led_classdev *led_cdev,
> - enum led_brightness value)
> -{
> - hpled_acpi_write(adev.device->handle, !!value);
> -}
> -
> -static struct led_classdev hpled_led = {
> - .name = "hp:red:hddprotection",
> - .default_trigger = "heartbeat",
Current mainline+lis3lv02d-merge-with-leds-hp-disk.patch has "none"
here, so the patch didn't apply.
> - .brightness_set = hpled_set,
> +static struct delayed_led_classdev hpled_led = {
> + .led_classdev = {
> + .name = "hp::hddprotect",
> + .default_trigger = "none",
But I assume we wanted "none" here anyway, so that's what I'll do.
> + .brightness_set = delayed_sysfs_set,
> + .flags = LED_CORE_SUSPENDRESUME,
> + },
> + .set_brightness = hpled_set,
> };
>
On Mon, 12 Jan 2009 11:31:55 +0100
Pavel Machek <[email protected]> wrote:
>
> LED on HP notebooks is connected through ACPI. That unfortunately
> means that it needs to be delayed by using schedule_work() to avoid
> calling ACPI interpretter in invalid context. This patch fixes that.
>
> ...
>
> +/* Delayed LEDs infrastructure ------------------------------------ */
> +
> +/* Special LED class that can defer work */
> +struct delayed_led_classdev {
> + struct led_classdev led_classdev;
> + struct work_struct work;
> + enum led_brightness new_brightness;
> +
> + unsigned int led; /* For driver */
> + void (*set_brightness)(struct delayed_led_classdev *data, enum led_brightness value);
> +};
> +
> +static inline void delayed_set_status_worker(struct work_struct *work)
> +{
> + struct delayed_led_classdev *data =
> + container_of(work, struct delayed_led_classdev, work);
> +
> + data->set_brightness(data, data->new_brightness);
> +}
> +
> +static inline void delayed_sysfs_set(struct led_classdev *led_cdev,
> + enum led_brightness brightness)
> +{
> + struct delayed_led_classdev *data = container_of(led_cdev,
> + struct delayed_led_classdev, led_classdev);
> + data->new_brightness = brightness;
> + schedule_work(&data->work);
> +}
Is this the only LED driver int he current and future history of the
world which uses ACPI?
coz otherwise, this code might better live in LEDs core somewhere, so
those other drivers can use it?
> +/* HP-specific accelerometer driver ------------------------------------ */
>
> /* For automatic insertion of the module */
> static struct acpi_device_id lis3lv02d_device_ids[] = {
> @@ -155,28 +185,27 @@ static struct dmi_system_id lis3lv02d_dm
> */
> };
>
> -static acpi_status hpled_acpi_write(acpi_handle handle, int reg)
> +static void hpled_set(struct delayed_led_classdev *led_cdev, enum led_brightness value)
> {
> + acpi_handle handle = adev.device->handle;
> unsigned long long ret; /* Not used when writing */
> union acpi_object in_obj[1];
> struct acpi_object_list args = { 1, in_obj };
>
> in_obj[0].type = ACPI_TYPE_INTEGER;
> - in_obj[0].integer.value = reg;
> + in_obj[0].integer.value = !!value;
>
> - return acpi_evaluate_integer(handle, "ALED", &args, &ret);
> + acpi_evaluate_integer(handle, "ALED", &args, &ret);
> }
>
> -static void hpled_set(struct led_classdev *led_cdev,
> - enum led_brightness value)
> -{
> - hpled_acpi_write(adev.device->handle, !!value);
> -}
> -
> -static struct led_classdev hpled_led = {
> - .name = "hp:red:hddprotection",
> - .default_trigger = "heartbeat",
> - .brightness_set = hpled_set,
> +static struct delayed_led_classdev hpled_led = {
> + .led_classdev = {
> + .name = "hp::hddprotect",
> + .default_trigger = "none",
> + .brightness_set = delayed_sysfs_set,
> + .flags = LED_CORE_SUSPENDRESUME,
> + },
> + .set_brightness = hpled_set,
> };
>
> static int lis3lv02d_add(struct acpi_device *device)
> @@ -208,13 +237,17 @@ static int lis3lv02d_add(struct acpi_dev
> adev.ac = lis3lv02d_axis_normal;
> }
>
> - ret = led_classdev_register(NULL, &hpled_led);
> + INIT_WORK(&hpled_led.work, delayed_set_status_worker);
> + ret = led_classdev_register(NULL, &hpled_led.led_classdev);
> if (ret)
> return ret;
>
> ret = lis3lv02d_init_device(&adev);
> if (ret) {
> - led_classdev_unregister(&hpled_led);
> + while (work_pending(&hpled_led.work))
> + schedule();
We have flush_work() for this?
> + while (work_pending(&hpled_led.work))
> + schedule();
ditto.
On Tue 2009-01-13 14:11:16, Andrew Morton wrote:
> On Mon, 12 Jan 2009 11:31:55 +0100
> Pavel Machek <[email protected]> wrote:
>
> >
> > LED on HP notebooks is connected through ACPI. That unfortunately
> > means that it needs to be delayed by using schedule_work() to avoid
> > calling ACPI interpretter in invalid context. This patch fixes that.
> >
> > Signed-off-by: Pavel Machek <[email protected]>
> > -static void hpled_set(struct led_classdev *led_cdev,
> > - enum led_brightness value)
> > -{
> > - hpled_acpi_write(adev.device->handle, !!value);
> > -}
> > -
> > -static struct led_classdev hpled_led = {
> > - .name = "hp:red:hddprotection",
> > - .default_trigger = "heartbeat",
>
> Current mainline+lis3lv02d-merge-with-leds-hp-disk.patch has "none"
> here, so the patch didn't apply.
Oops, sorry about that. I was hand-editing patches a bit too much :-(.
> > - .brightness_set = hpled_set,
> > +static struct delayed_led_classdev hpled_led = {
> > + .led_classdev = {
> > + .name = "hp::hddprotect",
> > + .default_trigger = "none",
>
> But I assume we wanted "none" here anyway, so that's what I'll do.
Thanks!
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Tue 2009-01-13 14:17:21, Andrew Morton wrote:
> On Mon, 12 Jan 2009 11:31:55 +0100
> Pavel Machek <[email protected]> wrote:
>
> >
> > LED on HP notebooks is connected through ACPI. That unfortunately
> > means that it needs to be delayed by using schedule_work() to avoid
> > calling ACPI interpretter in invalid context. This patch fixes that.
> >
> > ...
> >
> > +/* Delayed LEDs infrastructure ------------------------------------ */
> > +
> > +/* Special LED class that can defer work */
> > +struct delayed_led_classdev {
> > + struct led_classdev led_classdev;
> > + struct work_struct work;
> > + enum led_brightness new_brightness;
> > +
> > + unsigned int led; /* For driver */
> > + void (*set_brightness)(struct delayed_led_classdev *data, enum led_brightness value);
> > +};
> > +
> > +static inline void delayed_set_status_worker(struct work_struct *work)
> > +{
> > + struct delayed_led_classdev *data =
> > + container_of(work, struct delayed_led_classdev, work);
> > +
> > + data->set_brightness(data, data->new_brightness);
> > +}
> > +
> > +static inline void delayed_sysfs_set(struct led_classdev *led_cdev,
> > + enum led_brightness brightness)
> > +{
> > + struct delayed_led_classdev *data = container_of(led_cdev,
> > + struct delayed_led_classdev, led_classdev);
> > + data->new_brightness = brightness;
> > + schedule_work(&data->work);
> > +}
>
> Is this the only LED driver int he current and future history of the
> world which uses ACPI?
>
> coz otherwise, this code might better live in LEDs core somewhere, so
> those other drivers can use it?
There are 2 other drivers (thinkpad_acpi and asus-something IIRC) that
want the same infrastructure. So I put that code in
include/linux/delayed-leds.h in my tree, but then I realized I'd need
to coordinate too many maintainers and just inlined it for the
merge. (We do not want to schedule from atomic, right?)
I already have a patch for thinkpad_acpi, but it needs a bit more
testing and perhaps some tweaks by maintainer. So yes, eventually
putting that into shared place is a plan.
> > if (ret) {
> > - led_classdev_unregister(&hpled_led);
> > + while (work_pending(&hpled_led.work))
> > + schedule();
>
> We have flush_work() for this?
I did not realize flush_work() exists/does what I need... sorry.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html