What about something like this?
[Alternatively, I can add a flag to the leds class, and make delayed
leds a built-in functionality...]
[Attached is driver that uses new infrastructure for hp_accel, but it
does a bit more.]
---
It reduces code duplication and cleans
up code a bit. The code is already useful for thinkpad and hp_accel,
and there's one more driver that could use conversion.
Signed-off-by: Pavel Machek <[email protected]>
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 3478453..ee68654 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -69,6 +69,8 @@ #include <linux/hwmon-sysfs.h>
#include <linux/input.h>
#include <linux/leds.h>
#include <linux/rfkill.h>
+#include <linux/delayed-leds.h>
+
#include <asm/uaccess.h>
#include <linux/dmi.h>
@@ -79,7 +81,6 @@ #include <acpi/acpi_drivers.h>
#include <linux/pci_ids.h>
-
/* ThinkPad CMOS commands */
#define TP_CMOS_VOLUME_DOWN 0
#define TP_CMOS_VOLUME_UP 1
@@ -280,13 +281,8 @@ static u32 dbg_level;
static struct workqueue_struct *tpacpi_wq;
-/* Special LED class that can defer work */
-struct tpacpi_led_classdev {
- struct led_classdev led_classdev;
- struct work_struct work;
- enum led_brightness new_brightness;
- unsigned int led;
-};
+
+
/****************************************************************************
****************************************************************************
@@ -3462,9 +3458,13 @@ static int light_get_status(void)
return -ENXIO;
}
-static int light_set_status(int status)
+static int light_set_brightness(struct delayed_led_classdev *data, enum led_brightness value)
{
int rc;
+ int status = (value != LED_OFF);
+
+ if (tpacpi_lifecycle != TPACPI_LIFE_RUNNING)
+ return -EINVAL;
if (tp_features.light) {
if (cmos_handle) {
@@ -3482,37 +3482,24 @@ static int light_set_status(int status)
return -ENXIO;
}
-static void light_set_status_worker(struct work_struct *work)
+static void light_set_class_brightness(struct delayed_led_classdev *data, enum led_brightness value)
{
- struct tpacpi_led_classdev *data =
- container_of(work, struct tpacpi_led_classdev, work);
-
- if (likely(tpacpi_lifecycle == TPACPI_LIFE_RUNNING))
- light_set_status((data->new_brightness != LED_OFF));
+ light_set_brightness(data, value);
}
-static void light_sysfs_set(struct led_classdev *led_cdev,
- enum led_brightness brightness)
-{
- struct tpacpi_led_classdev *data =
- container_of(led_cdev,
- struct tpacpi_led_classdev,
- led_classdev);
- data->new_brightness = brightness;
- queue_work(tpacpi_wq, &data->work);
-}
static enum led_brightness light_sysfs_get(struct led_classdev *led_cdev)
{
return (light_get_status() == 1)? LED_FULL : LED_OFF;
}
-static struct tpacpi_led_classdev tpacpi_led_thinklight = {
+static struct delayed_led_classdev tpacpi_led_thinklight = {
.led_classdev = {
.name = "tpacpi::thinklight",
- .brightness_set = &light_sysfs_set,
+ .brightness_set = &delayed_sysfs_set,
.brightness_get = &light_sysfs_get,
- }
+ },
+ .set_brightness = light_set_class_brightness,
};
static int __init light_init(struct ibm_init_struct *iibm)
@@ -3524,7 +3511,7 @@ static int __init light_init(struct ibm_
TPACPI_ACPIHANDLE_INIT(ledb);
TPACPI_ACPIHANDLE_INIT(lght);
TPACPI_ACPIHANDLE_INIT(cmos);
- INIT_WORK(&tpacpi_led_thinklight.work, light_set_status_worker);
+ INIT_WORK(&tpacpi_led_thinklight.work, delayed_set_status_worker);
/* light not supported on 570, 600e/x, 770e, 770x, G4x, R30, R31 */
tp_features.light = (cmos_handle || lght_handle) && !ledb_handle;
@@ -3558,8 +3545,8 @@ static int __init light_init(struct ibm_
static void light_exit(void)
{
led_classdev_unregister(&tpacpi_led_thinklight.led_classdev);
- if (work_pending(&tpacpi_led_thinklight.work))
- flush_workqueue(tpacpi_wq);
+ while (work_pending(&tpacpi_led_thinklight.work))
+ schedule();
}
static int light_read(char *p)
@@ -3600,7 +3587,7 @@ static int light_write(char *buf)
return -EINVAL;
}
- return light_set_status(newstatus);
+ return light_set_brightness(&tpacpi_led_thinklight, newstatus);
}
static struct ibm_struct light_driver_data = {
@@ -4020,7 +4007,7 @@ TPACPI_HANDLE(led, ec, "SLED", /* 570 */
); /* R30, R31 */
#define TPACPI_LED_NUMLEDS 8
-static struct tpacpi_led_classdev *tpacpi_leds;
+static struct delayed_led_classdev *tpacpi_leds;
static enum led_status_t tpacpi_led_state_cache[TPACPI_LED_NUMLEDS];
static const char * const tpacpi_led_names[TPACPI_LED_NUMLEDS] = {
/* there's a limit of 19 chars + NULL before 2.6.26 */
@@ -4064,7 +4051,6 @@ static int led_set_status(const unsigned
/* off, on, blink. Index is led_status_t */
static const unsigned int led_sled_arg1[] = { 0, 1, 3 };
static const unsigned int led_led_arg1[] = { 0, 0x80, 0xc0 };
-
int rc = 0;
switch (led_supported) {
@@ -4104,40 +4090,21 @@ static int led_set_status(const unsigned
return rc;
}
-static void led_sysfs_set_status(unsigned int led,
- enum led_brightness brightness)
-{
- led_set_status(led,
- (brightness == LED_OFF) ?
- TPACPI_LED_OFF :
- (tpacpi_led_state_cache[led] == TPACPI_LED_BLINK) ?
- TPACPI_LED_BLINK : TPACPI_LED_ON);
-}
-
-static void led_set_status_worker(struct work_struct *work)
-{
- struct tpacpi_led_classdev *data =
- container_of(work, struct tpacpi_led_classdev, work);
-
- if (likely(tpacpi_lifecycle == TPACPI_LIFE_RUNNING))
- led_sysfs_set_status(data->led, data->new_brightness);
-}
-
-static void led_sysfs_set(struct led_classdev *led_cdev,
- enum led_brightness brightness)
+static void led_set_class_brightness(struct delayed_led_classdev *data, enum led_brightness value)
{
- struct tpacpi_led_classdev *data = container_of(led_cdev,
- struct tpacpi_led_classdev, led_classdev);
-
- data->new_brightness = brightness;
- queue_work(tpacpi_wq, &data->work);
+ if (tpacpi_lifecycle != TPACPI_LIFE_RUNNING)
+ return;
+ led_set_status(data->led, (value == LED_OFF) ?
+ TPACPI_LED_OFF :
+ (tpacpi_led_state_cache[data->led] == TPACPI_LED_BLINK) ?
+ TPACPI_LED_BLINK : TPACPI_LED_ON);
}
static int led_sysfs_blink_set(struct led_classdev *led_cdev,
unsigned long *delay_on, unsigned long *delay_off)
{
- struct tpacpi_led_classdev *data = container_of(led_cdev,
- struct tpacpi_led_classdev, led_classdev);
+ struct delayed_led_classdev *data = container_of(led_cdev,
+ struct delayed_led_classdev, led_classdev);
/* Can we choose the flash rate? */
if (*delay_on == 0 && *delay_off == 0) {
@@ -4148,7 +4115,7 @@ static int led_sysfs_blink_set(struct le
return -EINVAL;
data->new_brightness = TPACPI_LED_BLINK;
- queue_work(tpacpi_wq, &data->work);
+ schedule_work(&data->work);
return 0;
}
@@ -4157,8 +4124,8 @@ static enum led_brightness led_sysfs_get
{
int rc;
- struct tpacpi_led_classdev *data = container_of(led_cdev,
- struct tpacpi_led_classdev, led_classdev);
+ struct delayed_led_classdev *data = container_of(led_cdev,
+ struct delayed_led_classdev, led_classdev);
rc = led_get_status(data->led);
@@ -4217,15 +4184,16 @@ static int __init led_init(struct ibm_in
for (i = 0; i < TPACPI_LED_NUMLEDS; i++) {
tpacpi_leds[i].led = i;
- tpacpi_leds[i].led_classdev.brightness_set = &led_sysfs_set;
+ tpacpi_leds[i].led_classdev.brightness_set = &delayed_sysfs_set;
tpacpi_leds[i].led_classdev.blink_set = &led_sysfs_blink_set;
if (led_supported == TPACPI_LED_570)
tpacpi_leds[i].led_classdev.brightness_get =
&led_sysfs_get;
tpacpi_leds[i].led_classdev.name = tpacpi_led_names[i];
+ tpacpi_leds[i].set_brightness = led_set_class_brightness;
- INIT_WORK(&tpacpi_leds[i].work, led_set_status_worker);
+ INIT_WORK(&tpacpi_leds[i].work, delayed_set_status_worker);
rc = led_classdev_register(&tpacpi_pdev->dev,
&tpacpi_leds[i].led_classdev);
diff --git a/include/linux/delayed-leds.h b/include/linux/delayed-leds.h
new file mode 100644
index 0000000..31644b8
--- /dev/null
+++ b/include/linux/delayed-leds.h
@@ -0,0 +1,39 @@
+/*
+ * delayed-leds.h - LEDs that can't be set from atomic context
+ *
+ *
+ * Copyright (C) 2004-2005 Borislav Deianov <[email protected]>
+ * Copyright (C) 2006-2008 Henrique de Moraes Holschuh <[email protected]>
+ *
+ * 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.
+ */
+
+/* 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);
+}
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sun, 11 Jan 2009, Pavel Machek wrote:
> What about something like this?
>
> [Alternatively, I can add a flag to the leds class, and make delayed
> leds a built-in functionality...]
>
> [Attached is driver that uses new infrastructure for hp_accel, but it
> does a bit more.]
FWIW, I am looking over the thinkpad-acpi side of this. I like the idea,
but I am not completely sure I agree fully with the changes to
thinkpad-acpi.
Which isn't a problem, as long as the thinkpad-acpi hunks are NOT merged to
any tree before my ACK, please. I will test and comment on the patch before
the weekend.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Mon 2009-01-12 08:05:01, Henrique de Moraes Holschuh wrote:
> On Sun, 11 Jan 2009, Pavel Machek wrote:
> > What about something like this?
> >
> > [Alternatively, I can add a flag to the leds class, and make delayed
> > leds a built-in functionality...]
> >
> > [Attached is driver that uses new infrastructure for hp_accel, but it
> > does a bit more.]
>
> FWIW, I am looking over the thinkpad-acpi side of this. I like the idea,
> but I am not completely sure I agree fully with the changes to
> thinkpad-acpi.
>
> Which isn't a problem, as long as the thinkpad-acpi hunks are NOT merged to
> any tree before my ACK, please. I will test and comment on the patch before
> the weekend.
Any news?
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Mon, 19 Jan 2009, Pavel Machek wrote:
> On Mon 2009-01-12 08:05:01, Henrique de Moraes Holschuh wrote:
> > On Sun, 11 Jan 2009, Pavel Machek wrote:
> > > What about something like this?
> > >
> > > [Alternatively, I can add a flag to the leds class, and make delayed
> > > leds a built-in functionality...]
> > >
> > > [Attached is driver that uses new infrastructure for hp_accel, but it
> > > does a bit more.]
> >
> > FWIW, I am looking over the thinkpad-acpi side of this. I like the idea,
> > but I am not completely sure I agree fully with the changes to
> > thinkpad-acpi.
> >
> > Which isn't a problem, as long as the thinkpad-acpi hunks are NOT merged to
> > any tree before my ACK, please. I will test and comment on the patch before
> > the weekend.
>
> Any news?
Sure. Sorry for not replying earlier.
I don't like the loss of functionality of the private workqueue. I kicked
the thinkpad led handling to a private workqueue in order to never tie up
the system-wide one with crap spinning around in the ACPI layer, etc. In
fact, all thinkpad-acpi deferred work is in the private workqueue for this
reason.
This is easy enough to fix, you just need to provide both versions of the
interface (standard workqueue, and private workqueue). OTOH, if nowadays
the system-wide workqueue runs tasks in parallel and there is no risk of a
task blocking others from completely unrelated subsystems, I can ditch the
private workqueue.
Also, if it is going to be generic, I'd suggest the usual "void *data"
opaque type to carry information for the driver, instead of a "led number".
I didn't see any comments from Richard, but your comment about a flag seems
to imply there was a private one? IMHO enhancing the led class would be the
best way to go about it, although deferred leds are simple enough that it
doesn't matter much.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Tue, 2009-01-20 at 10:04 -0200, Henrique de Moraes Holschuh wrote:
> I don't like the loss of functionality of the private workqueue. I kicked
> the thinkpad led handling to a private workqueue in order to never tie up
> the system-wide one with crap spinning around in the ACPI layer, etc. In
> fact, all thinkpad-acpi deferred work is in the private workqueue for this
> reason.
>
> This is easy enough to fix, you just need to provide both versions of the
> interface (standard workqueue, and private workqueue). OTOH, if nowadays
> the system-wide workqueue runs tasks in parallel and there is no risk of a
> task blocking others from completely unrelated subsystems, I can ditch the
> private workqueue.
>
> Also, if it is going to be generic, I'd suggest the usual "void *data"
> opaque type to carry information for the driver, instead of a "led number".
>
> I didn't see any comments from Richard, but your comment about a flag seems
> to imply there was a private one? IMHO enhancing the led class would be the
> best way to go about it, although deferred leds are simple enough that it
> doesn't matter much.
I think the best approach is to get this header working and then see how
many people can really share it. If its common enough we can add to the
core but lets see if its a useful abstraction first?
Cheers,
Richard
--
Richard Purdie
Intel Open Source Technology Centre
On Tue 2009-01-20 10:04:00, Henrique de Moraes Holschuh wrote:
> On Mon, 19 Jan 2009, Pavel Machek wrote:
> > On Mon 2009-01-12 08:05:01, Henrique de Moraes Holschuh wrote:
> > > On Sun, 11 Jan 2009, Pavel Machek wrote:
> > > > What about something like this?
> > > >
> > > > [Alternatively, I can add a flag to the leds class, and make delayed
> > > > leds a built-in functionality...]
> > > >
> > > > [Attached is driver that uses new infrastructure for hp_accel, but it
> > > > does a bit more.]
> > >
> > > FWIW, I am looking over the thinkpad-acpi side of this. I like the idea,
> > > but I am not completely sure I agree fully with the changes to
> > > thinkpad-acpi.
> > >
> > > Which isn't a problem, as long as the thinkpad-acpi hunks are NOT merged to
> > > any tree before my ACK, please. I will test and comment on the patch before
> > > the weekend.
> >
> > Any news?
>
> Sure. Sorry for not replying earlier.
>
> I don't like the loss of functionality of the private workqueue. I kicked
> the thinkpad led handling to a private workqueue in order to never tie up
> the system-wide one with crap spinning around in the ACPI layer, etc. In
> fact, all thinkpad-acpi deferred work is in the private workqueue for this
> reason.
Is the private workqueue really required? AFAICT workqueues are not
exactly cheap, and leds are not toggled that often. Was it problem in
practice?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sun, 08 Mar 2009, Pavel Machek wrote:
> On Tue 2009-01-20 10:04:00, Henrique de Moraes Holschuh wrote:
> > On Mon, 19 Jan 2009, Pavel Machek wrote:
> > > On Mon 2009-01-12 08:05:01, Henrique de Moraes Holschuh wrote:
> > > > On Sun, 11 Jan 2009, Pavel Machek wrote:
> > > > > What about something like this?
> > > > >
> > > > > [Alternatively, I can add a flag to the leds class, and make delayed
> > > > > leds a built-in functionality...]
> > > > >
> > > > > [Attached is driver that uses new infrastructure for hp_accel, but it
> > > > > does a bit more.]
> > > >
> > > > FWIW, I am looking over the thinkpad-acpi side of this. I like the idea,
> > > > but I am not completely sure I agree fully with the changes to
> > > > thinkpad-acpi.
> > > >
> > > > Which isn't a problem, as long as the thinkpad-acpi hunks are NOT merged to
> > > > any tree before my ACK, please. I will test and comment on the patch before
> > > > the weekend.
> > >
> > > Any news?
> >
> > Sure. Sorry for not replying earlier.
> >
> > I don't like the loss of functionality of the private workqueue. I kicked
> > the thinkpad led handling to a private workqueue in order to never tie up
> > the system-wide one with crap spinning around in the ACPI layer, etc. In
> > fact, all thinkpad-acpi deferred work is in the private workqueue for this
> > reason.
>
> Is the private workqueue really required? AFAICT workqueues are not
> exactly cheap, and leds are not toggled that often. Was it problem in
> practice?
Well, the day we can trust the main workqueue not to block or delay
important stuff, I will use it. Until then, I feel it is best to have
thinkpad-acpi work scheduled independently from the important stuff.
The private workqueue is NOT used just for the LEDs, it is used for
every thinkpad-acpi deferred-work needs. And I'd rather the thinkpad-acpi
LED work didn't move to the main workqueue.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh