Integrate asus_acpi LED's with new LED subsystem
PATCH 1/1 Integrate asus_acpi LED's with new LED subsystem
(Sorry to repost... just going through the motions of ChangeLog and Signed-off-by headers.)
Here is a patch to the asus_acpi driver that links the Asus laptop LED's into the new LED subsystem. It creates LED class devices named asus:mail, asus:wireless, and asus:touchpad, depending on if the laptop supports the mled, wled, and tled LED's.
Since it's so new, I added a config option to turn it on and off. It's worked for me, though I have an Asus M2N, which only has the mail and wireless LED's.
drivers/acpi/asus_acpi.c +152 -0
@@ -27,14 +27,19 @@* Johann Wiesner - Small compile fixes * John Belmonte - ACPI code for Toshiba laptop was a good starting point. * �ric Burghard - LED display support for W1N +* Thomas Tuttle - LED subsystem integration * */ +#include <linux/config.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/init.h> #include <linux/types.h> #include <linux/proc_fs.h> +#ifdef CONFIG_ACPI_ASUS_NEW_LED +#include <linux/leds.h> +#endif #include <acpi/acpi_drivers.h> #include <acpi/acpi_bus.h> #include <asm/uaccess.h>
@@ -916,6 +921,145 @@return 0 ; } +#ifdef CONFIG_ACPI_ASUS_NEW_LED + +/* These functions are called by the LED subsystem to update the desired +* state of the LED's. */ +static void led_set_mled (struct led_classdev *led_cdev, +enum led_brightness value); +static void led_set_wled (struct led_classdev *led_cdev, +enum led_brightness value); +static void led_set_tled (struct led_classdev *led_cdev, +enum led_brightness value); + +/* LED class devices. */ +static struct led_classdev led_cdev_mled = + { .name ="asus:mail" , .brightness_set = led_set_mled }; +static struct led_classdev led_cdev_wled = + { .name ="asus:wireless" , .brightness_set = led_set_wled }; +static struct led_classdev led_cdev_tled = + { .name ="asus:touchpad" , .brightness_set = led_set_tled }; + +/* These functions actually update the LED's, and are called from a +* workqueue. By doing this as separate work rather than when the LED +* subsystem asks, I avoid messing with the Asus ACPI stuff during a +* potentially bad time, such as a timer interrupt. */ +static void led_update_mled (void *private); +static void led_update_wled (void *private); +static void led_update_tled (void *private); + +/* Desired values of LED's. */ +static int led_mled_value =0 ; +static int led_wled_value =0 ; +static int led_tled_value =0 ; + +/* LED workqueue. */ +static struct workqueue_struct *led_workqueue; + +/* LED update work structs. */ +DECLARE_WORK (led_mled_work, led_update_mled,NULL ); +DECLARE_WORK (led_wled_work, led_update_wled,NULL ); +DECLARE_WORK (led_tled_work, led_update_tled,NULL ); + +/* LED subsystem callbacks. */ +static void led_set_mled (struct led_classdev *led_cdev, +enum led_brightness value) +{ + led_mled_value = value; +queue_work (led_workqueue, &led_mled_work); +} + +static void led_set_wled (struct led_classdev *led_cdev, +enum led_brightness value) +{ + led_wled_value = value; +queue_work (led_workqueue, &led_wled_work); +} + +static void led_set_tled (struct led_classdev *led_cdev, +enum led_brightness value) +{ + led_tled_value = value; +queue_work (led_workqueue, &led_tled_work); +} + +/* LED work functions. */ +static void led_update_mled (void *private) { +char *ledname = hotk->methods->mt_mled; +int led_out = led_mled_value ?1 :0 ; + hotk->status = (led_out) ? (hotk->status |MLED_ON ) : (hotk->status & ~MLED_ON ); + led_out =1 - led_out; +if (!write_acpi_int (hotk->handle, ledname, led_out,NULL )) +printk (KERN_WARNING "Asus ACPI: LED (%s) write failed\n" , + ledname); +} + +static void led_update_wled (void *private) { +char *ledname = hotk->methods->mt_wled; +int led_out = led_wled_value ?1 :0 ; + hotk->status = (led_out) ? (hotk->status |WLED_ON ) : (hotk->status & ~WLED_ON ); +if (!write_acpi_int (hotk->handle, ledname, led_out,NULL )) +printk (KERN_WARNING "Asus ACPI: LED (%s) write failed\n" , + ledname); +} + +static void led_update_tled (void *private) { +char *ledname = hotk->methods->mt_tled; +int led_out = led_tled_value ?1 :0 ; + hotk->status = (led_out) ? (hotk->status |TLED_ON ) : (hotk->status & ~TLED_ON ); +if (!write_acpi_int (hotk->handle, ledname, led_out,NULL )) +printk (KERN_WARNING "Asus ACPI: LED (%s) write failed\n" , + ledname); +} + +/* Registers LED class devices and sets up workqueue. */ +static int led_initialize (struct device *parent) +{ +int result; + +if (hotk->methods->mt_mled) { + result =led_classdev_register (parent, &led_cdev_mled); +if (result) +return result; + } + +if (hotk->methods->mt_wled) { + result =led_classdev_register (parent, &led_cdev_wled); +if (result) +return result; + } + +if (hotk->methods->mt_tled) { + result =led_classdev_register (parent, &led_cdev_tled); +if (result) +return result; + } + + led_workqueue =create_singlethread_workqueue ("led_workqueue" ); + +return 0 ; +} + +/* Destroys the workqueue and unregisters the LED class devices. */ +static void led_terminate (void ) +{ +destroy_workqueue (led_workqueue); + +if (hotk->methods->mt_tled) { +led_classdev_unregister (&led_cdev_tled); + } + +if (hotk->methods->mt_wled) { +led_classdev_unregister (&led_cdev_wled); + } + +if (hotk->methods->mt_mled) { +led_classdev_unregister (&led_cdev_mled); + } +} + +#endif +static int asus_hotk_add_fs (struct acpi_device *device) {struct proc_dir_entry *proc;
@@ -1299,6 +1443,10 @@/* LED display is off by default */ hotk->ledd_status =0xFFF ; +#ifdef CONFIG_ACPI_ASUS_NEW_LED + result =led_initialize (acpi_get_physical_device (device->handle)); +#endif + end:if (result) {kfree (hotk);
@@ -1314,6 +1462,10 @@if (!device || !acpi_driver_data (device))return -EINVAL ; +#ifdef CONFIG_ACPI_ASUS_NEW_LED +led_terminate (); +#endif + status =acpi_remove_notify_handler (hotk->handle,ACPI_SYSTEM_NOTIFY , asus_hotk_notify);if (ACPI_FAILURE (status))
drivers/acpi/Kconfig +9 -0
@@ -199,6 +199,15 @@ something works not quite as expected, please use the mailing list available on the above page ([email protected]) +config ACPI_ASUS_NEW_LED + bool "ASUS/Medion LED subsystem integration" + depends on ACPI_ASUS + depends on LEDS_CLASS + help + This adds support for the new LED subsystem to the asus_acpi + driver. The LED's will show up as asus:mail, asus:wireless, + and asus:touchpad, as applicable to your laptop. + config ACPI_IBM tristate "IBM ThinkPad Laptop Extras" depends on X86
Quoted 1 line · Thomas Tuttle
+#include <
linux/config.h>
Not needed.
drivers/acpi/asus_acpi.c line 42
Quoted 8 lines
#include <
linux/kernel.h>
#include <linux/module.h>
#include <linux/init.h>
#include <linux/types.h>
#include <linux/proc_fs.h>drivers/acpi/asus_acpi.c +3
-0+#ifdef CONFIG_ACPI_ASUS_NEW_LED +#include <linux/leds.h> +#endif
Doesn't need the ifdefs, the header should be harmless
drivers/acpi/asus_acpi.c line 941
Quoted 7 lines
drivers/acpi/asus_acpi.c +7
-0+/* LED class devices. */ +static struct led_classdev led_cdev_mled = + { .name ="asus:mail" , .brightness_set = led_set_mled }; +static struct led_classdev led_cdev_wled = + { .name ="asus:wireless" , .brightness_set = led_set_wled }; +static struct led_classdev led_cdev_tled = + { .name ="asus:touchpad" , .brightness_set = led_set_tled };
Not the formatting I'm used to but I'm not sure if it breaks the CodingStyle... :)
Quoted 5 lines
+/* These functions actually update the LED's, and are called from a
+ * workqueue. By doing this as separate work rather than when the LED
+ * subsystem asks, I avoid messing with the Asus ACPI stuff during a
+ * potentially bad time, such as a timer interrupt. */
More simply, "The led update functions can be called in interrupt context so we use a work queue to pass the updates to acpi" or similar.
Words like "I" become meaningless in the source.
Quoted 11 lines
drivers/acpi/asus_acpi.c +5
-0+/* LED work functions. */ +static void led_update_mled (void *private) { +char *ledname = hotk->methods->mt_mled; +int led_out = led_mled_value ?1 :0 ; + hotk->status = (led_out) ? (hotk->status | MLED_ON) :(
hotk->status& ~MLED_ON);drivers/acpi/asus_acpi.c +3
-0+ led_out =1 - led_out; +if (!write_acpi_int (hotk->handle, ledname, led_out,NULL )) + printk(KERN_WARNING "Asus ACPI: LED (%s) write failed\n",
+ ledname);
The lack of locking on hotk->status makes me nervous but since it appears to only contain LED data, its probably not too important and the write_led function already there is equally bad...
Quoted 39 lines
drivers/acpi/asus_acpi.c +7
-0+/* Registers LED class devices and sets up workqueue. */ +static int led_initialize (struct device *parent) +{ +int result; + +if (hotk->methods->mt_mled) { + result = led_classdev_register(parent,
&led_cdev_mled);drivers/acpi/asus_acpi.c +6
-0+if (result) +return result; + } + +if (hotk->methods->mt_wled) { + result = led_classdev_register(parent,
&led_cdev_wled);drivers/acpi/asus_acpi.c +6
-0+if (result) +return result; + } + +if (hotk->methods->mt_tled) { + result = led_classdev_register(parent,
&led_cdev_tled);drivers/acpi/asus_acpi.c +5
-0+if (result) +return result; + } + + led_workqueue =
create_singlethread_workqueue("led_workqueue");drivers/acpi/asus_acpi.c +11
-0+ +return 0 ; +} + +/* Destroys the workqueue and unregisters the LED class devices. */ +static void led_terminate (void ) +{ +destroy_workqueue (led_workqueue); + +if (hotk->methods->mt_tled) { +led_classdev_unregister (&led_cdev_tled);
drivers/acpi/asus_acpi.c line 1059
Quoted 10 lines
drivers/acpi/asus_acpi.c +10
-0+ } + +if (hotk->methods->mt_wled) { +led_classdev_unregister (&led_cdev_wled); + } + +if (hotk->methods->mt_mled) { +led_classdev_unregister (&led_cdev_mled); + } +}
What happens if the first led_classdev_register succeeds and the subsequent calls fail? You need to think about all the different error cases here...
Also, having looked at that ACPI driver, what happens to the existing LED access functions via proc and how do they coexist with the LED subsystem? Ultimately I guess they'd get removed but in the meantime they present a problem. The LED subsystem does not have a brightness_get function and assumes it has complete control of the LED. It therefore caches the brightness value internally to itself. If we have a lot of cases where this isn't going to work (like here), we could look at adding an optional brightness_get function but I'd prefer to keep complexity out of the class if possible. How common is that problem going to be I wonder...
Richard
On July 06 at 18:39 EDT, Richard Purdie hastily scribbled:
Quoted 3 lines · Thomas Tuttle
Quoted 1 line
+#include <
linux/config.h>Not needed.
Will be fixed.
Quoted 4 lines
drivers/acpi/asus_acpi.c line 42
Quoted 3 lines
drivers/acpi/asus_acpi.c +3
-0+#ifdef CONFIG_ACPI_ASUS_NEW_LED +#include <linux/leds.h> +#endif Doesn't need the ifdefs, the header should be harmless
Will be fixed.
Quoted 5 lines
drivers/acpi/asus_acpi.c line 937
Quoted 3 lines
drivers/acpi/asus_acpi.c +2
-0+static struct led_classdev led_cdev_mled = + { .name ="asus:mail" , .brightness_set = led_set_mled };Not the formatting I'm used to but I'm not sure if it breaks the CodingStyle... :)
Will be fixed.
Quoted 7 lines
Quoted 5 lines
+/* These functions actually update the LED's, and are called from a
+ * workqueue. By doing this as separate work rather than when the LED
+ * subsystem asks, I avoid messing with the Asus ACPI stuff during a
+ * potentially bad time, such as a timer interrupt. */More simply, "The led update functions can be called in interrupt context so we use a work queue to pass the updates to acpi" or similar.
Will be fixed. I hope you don't mind being quoted.
Quoted 5 lines
Quoted 2 lines
+
hotk->status= (led_out) ? (hotk->status|MLED_ON) :
(hotk->status& ~MLED_ON);The lack of locking on
hotk->statusmakes me nervous but since it appears to only contain LED data, its probably not too important and thewrite_ledfunction already there is equally bad...
Agreed, if the other functions don't have problems, mine shouldn't. Besides, the inconsistency wouldn't be life-threatening. It's just internal state, not hardware stuff.
Quoted 3 lines
What happens if the first
led_classdev_registersucceeds and the subsequent calls fail? You need to think about all the different error cases here...
Will be fixed.
Quoted 10 lines
Also, having looked at that ACPI driver, what happens to the existing LED access functions via proc and how do they coexist with the LED subsystem? Ultimately I guess they'd get removed but in the meantime they present a problem. The LED subsystem does not have a
brightness_getfunction and assumes it has complete control of the LED. It therefore caches the brightness value internally to itself. If we have a lot of cases where this isn't going to work (like here), we could look at adding an optionalbrightness_getfunction but I'd prefer to keep complexity out of the class if possible. How common is that problem going to be I wonder...
/me figures, leave it to the user. Just as it's unwise but possible to run two userspace CPU governors, it's unwise to tell the LED subsystem to control LEDs and then change them yourself. I don't think it's an issue, but if you really want, I can make it so the /proc functions are disabled unless the trigger is set to "none".
I've already written an email in response to Andrew Morton's suggestions (many of which you also made) and it will contain yet another revision of the patch.
--Thomas Tuttle
Quoted 17 lines · Thomas Tuttle
(Sorry to repost... just going through the motions of ChangeLog and Signed-off-by headers.)
Here is a patch to the
asus_acpidriver that links the Asus laptop LED's into the new LED subsystem. It creates LED class devices named asus:mail, asus:wireless, and asus:touchpad, depending on if the laptop supports the mled, wled, and tled LED's.Since it's so new, I added a config option to turn it on and off. It's worked for me, though I have an Asus M2N, which only has the mail and wireless LED's.
...
#include <
linux/types.h>
#include <linux/proc_fs.h>
drivers/acpi/asus_acpi.c line 42
Quoted 3 lines
drivers/acpi/asus_acpi.c +3
-0+#ifdef CONFIG_ACPI_ASUS_NEW_LED +#include <linux/leds.h> +#endif
The ifdef shouldn't be required. If it is, ldes.h needs fixing.
drivers/acpi/asus_acpi.c line 933
Quoted 10 lines
drivers/acpi/asus_acpi.c +10
-0+#ifdef CONFIG_ACPI_ASUS_NEW_LED + +/* These functions are called by the LED subsystem to update the desired +* state of the LED's. */ +static void led_set_mled (struct led_classdev *led_cdev, +enum led_brightness value); +static void led_set_wled (struct led_classdev *led_cdev, +enum led_brightness value); +static void led_set_tled (struct led_classdev *led_cdev, +enum led_brightness value);
declaration
drivers/acpi/asus_acpi.c line 941
Quoted 7 lines
drivers/acpi/asus_acpi.c +7
-0+/* LED class devices. */ +static struct led_classdev led_cdev_mled = + { .name ="asus:mail" , .brightness_set = led_set_mled }; +static struct led_classdev led_cdev_wled = + { .name ="asus:wireless" , .brightness_set = led_set_wled }; +static struct led_classdev led_cdev_tled = + { .name ="asus:touchpad" , .brightness_set = led_set_tled };
definition
drivers/acpi/asus_acpi.c line 949
Quoted 7 lines
drivers/acpi/asus_acpi.c +7
-0+/* These functions actually update the LED's, and are called from a +* workqueue. By doing this as separate work rather than when the LED +* subsystem asks, I avoid messing with the Asus ACPI stuff during a +* potentially bad time, such as a timer interrupt. */ +static void led_update_mled (void *private); +static void led_update_wled (void *private); +static void led_update_tled (void *private);
declaration
drivers/acpi/asus_acpi.c line 954
Quoted 4 lines
drivers/acpi/asus_acpi.c +4
-0+/* Desired values of LED's. */ +static int led_mled_value =0 ; +static int led_wled_value =0 ; +static int led_tled_value =0 ;
definition
This mingles declarations with definitions. It's more conventional to keep them together.
Please don' t initialise static storage to zero (the "= 0"). The C runtime environment does that already, and the above construct will place the values into .data and hence into .vmlinux.
drivers/acpi/asus_acpi.c line 962
Quoted 7 lines
drivers/acpi/asus_acpi.c +7
-0+/* LED workqueue. */ +static struct workqueue_struct *led_workqueue; + +/* LED update work structs. */ +DECLARE_WORK (led_mled_work, led_update_mled,NULL ); +DECLARE_WORK (led_wled_work, led_update_wled,NULL ); +DECLARE_WORK (led_tled_work, led_update_tled,NULL );
These all should be declared static.
drivers/acpi/asus_acpi.c line 987
Quoted 2 lines
drivers/acpi/asus_acpi.c +2
-0+/* LED work functions. */ +static void led_update_mled (void *private) {
The opening brace goes in column 1.
Quoted 7 lines
drivers/acpi/asus_acpi.c +7
-0+char *ledname = hotk->methods->mt_mled; +int led_out = led_mled_value ?1 :0 ; + hotk->status = (led_out) ? (hotk->status |MLED_ON ) : (hotk->status & ~MLED_ON ); + led_out =1 - led_out; +if (!write_acpi_int (hotk->handle, ledname, led_out,NULL )) +printk (KERN_WARNING "Asus ACPI: LED (%s) write failed\n" , + ledname);
drivers/acpi/asus_acpi.c line 1004
Quoted 10 lines
drivers/acpi/asus_acpi.c +10
-0+} + +static void led_update_wled (void *private) { +char *ledname = hotk->methods->mt_wled; +int led_out = led_wled_value ?1 :0 ; + hotk->status = (led_out) ? (hotk->status |WLED_ON ) : (hotk->status & ~WLED_ON ); +if (!write_acpi_int (hotk->handle, ledname, led_out,NULL )) +printk (KERN_WARNING "Asus ACPI: LED (%s) write failed\n" , + ledname); +}
It's usual to put a blank line at the end of the declaration of the automatic valriables.
Quoted 9 lines
drivers/acpi/asus_acpi.c +9
-0+ +/* Registers LED class devices and sets up workqueue. */ +{ +destroy_workqueue (led_workqueue); + +if (hotk->methods->mt_tled) { +led_classdev_unregister (&led_cdev_tled); + } +
drivers/acpi/asus_acpi.c line 1061
Quoted 10 lines
drivers/acpi/asus_acpi.c +10
-0+if (hotk->methods->mt_wled) { +led_classdev_unregister (&led_cdev_wled); + } + +if (hotk->methods->mt_mled) { +led_classdev_unregister (&led_cdev_mled); + } +} + +#endif
Add:
#else
static inline int led_initialize(struct device *parent) {
}
static inline void led_terminate(void)
{
}
#endif
Quoted 15 lines
static int
asus_hotk_add_fs(struct acpi_device *device){drivers/acpi/asus_acpi.c +4
-0struct proc_dir_entry *proc;@@ -1299,6 +1443,10 @@/* LED display is off by default */ hotk->ledd_status =0xFFF ; +#ifdef CONFIG_ACPI_ASUS_NEW_LED + result =led_initialize (acpi_get_physical_device (device->handle)); +#endif + end:if (result) {kfree (hotk);
drivers/acpi/asus_acpi.c line 1468
Quoted 7 lines
drivers/acpi/asus_acpi.c +4
-0@@ -1314,6 +1462,10 @@if (!device || !acpi_driver_data (device))return -EINVAL ; +#ifdef CONFIG_ACPI_ASUS_NEW_LED +led_terminate (); +#endif +
And remove these ifdefs.
That way we avoid sprinkling ifdefs in the middle of the C code and we get syntax- and type-checking even if the feature is configged off.
Here is a patch to the asus_acpi driver that links the Asus laptop LED's into the new LED subsystem. It creates LED class devices named asus:mail, asus:wireless, and asus:touchpad, depending on if the laptop supports the mled, wled, and tled LED's.
Since it's so new, I added a config option to turn it on and off. It's worked for me, though I have an Asus M2N, which only has the mail and wireless LED's.
I believe I've fixed everything you asked about, plus a few things Richard Purdie (the LED subsystem guy) suggested.
On July 06 at 18:49 EDT, Andrew Morton hastily scribbled:
Quoted 5 lines
Review
Quoted 3 lines · Thomas Tuttle
(quoted diff) +3
-0+#ifdef CONFIG_ACPI_ASUS_NEW_LED +#include <linux/leds.h> +#endifThe ifdef shouldn't be required. If it is,
ldes.hneeds fixing.
Fixed.
Quoted 2 lines
This mingles declarations with definitions. It's more conventional to keep them together.
Fixed. I moved all the declarations together, and initialized the led_cdev structs if and when they are going to be registered. (Just as with the zero initializers, this should stop them from wasting space in .vmlinux.)
Quoted 3 lines
Please don' t initialise static storage to zero (the "= 0"). The C runtime environment does that already, and the above construct will place the values into .data and hence into .vmlinux.
Fixed.
Quoted 2 lines
Quoted 1 line
+/* LED update work structs. */
These all should be declared static.
Fixed.
Quoted 3 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+/* LED work functions. */ +static void led_update_mled(void *private) {The opening brace goes in column 1.
Fixed.
Quoted 2 lines
It's usual to put a blank line at the end of the declaration of the automatic valriables.
Fixed.
Quoted 11 lines
Add:
#else
static inline intled_initialize(struct device *parent){
}static inline void
led_terminate(void)
{
}
#endif
Fixed.
Quoted 10 lines
Review
Quoted 9 lines
(quoted diff) +8
-0+#ifdef CONFIG_ACPI_ASUS_NEW_LED + result = led_initialize(acpi_get_physical_device(device->handle)); +#endif + +#ifdef CONFIG_ACPI_ASUS_NEW_LED + led_terminate(); +#endif +And remove these ifdefs.
Fixed.
I believe it's a little less ugly now. The declarations are better organized, error recovery from the initialization is better (sorry to use goto, but if I didn't, I'd have to duplicate the abort code for each possible point of failure).
This revision is essentially the same, functionally, and it appears to work in quick testing. I think it would be nice if someone else with an Asus laptop could give this a test before it goes into mainline. The code that updates the LED's is basically copied from the /proc interface, but testing is good.
-- Thomas Tuttle
drivers/acpi/asus_acpi.c +158 -0
@@ -27,6 +27,7 @@* Johann Wiesner - Small compile fixes * John Belmonte - ACPI code for Toshiba laptop was a good starting point. * Éric Burghard - LED display support for W1N +* Thomas Tuttle - LED subsystem integration * */
@@ -35,6 +36,7 @@#include <linux/init.h> #include <linux/types.h> #include <linux/proc_fs.h> +#include <linux/leds.h> #include <acpi/acpi_drivers.h> #include <acpi/acpi_bus.h> #include <asm/uaccess.h>
@@ -916,6 +918,158 @@return 0 ; } +#ifdef CONFIG_ACPI_ASUS_NEW_LED + +static struct led_classdev led_cdev_mled, led_cdev_wled, led_cdev_tled; + +/* Desired values of LED's. */ +static int led_mled_value; +static int led_wled_value; +static int led_tled_value; + +/* These functions are called by the LED subsystem to update the desired +* state of the LED's. */ +static void led_set_mled (struct led_classdev *led_cdev, +enum led_brightness value); +static void led_set_wled (struct led_classdev *led_cdev, +enum led_brightness value); +static void led_set_tled (struct led_classdev *led_cdev, +enum led_brightness value); + +/* The LED update functions can be called in interrupt context, so we +* use a work queue to pass the updates to ACPI at a safer time. */ +static void led_update_mled (void *private); +static void led_update_wled (void *private); +static void led_update_tled (void *private); + +/* LED workqueue. */ +static struct workqueue_struct *led_workqueue; + +/* LED update work structs. */ +static DECLARE_WORK(led_mled_work, led_update_mled, NULL); +static DECLARE_WORK(led_wled_work, led_update_wled, NULL); +static DECLARE_WORK(led_tled_work, led_update_tled, NULL); + +/* LED subsystem callbacks. */ +static void led_set_mled (struct led_classdev *led_cdev, +enum led_brightness value) +{ + led_mled_value = value; +queue_work (led_workqueue, &led_mled_work); +} + +static void led_set_wled (struct led_classdev *led_cdev, +enum led_brightness value) +{ + led_wled_value = value; +queue_work (led_workqueue, &led_wled_work); +} + +static void led_set_tled (struct led_classdev *led_cdev, +enum led_brightness value) +{ + led_tled_value = value; +queue_work (led_workqueue, &led_tled_work); +} + +/* LED work functions. */ +static void led_update_mled (void *private) +{ +char *ledname = hotk->methods->mt_mled; +int led_out = led_mled_value ?1 :0 ; + + hotk->status = (led_out) ? (hotk->status |MLED_ON ) : (hotk->status & ~MLED_ON ); + led_out =1 - led_out; +if (!write_acpi_int (hotk->handle, ledname, led_out,NULL )) +printk (KERN_WARNING "Asus ACPI: Writing MLED failed.\n" ); +} + +static void led_update_wled (void *private) +{ +char *ledname = hotk->methods->mt_wled; +int led_out = led_wled_value ?1 :0 ; + + hotk->status = (led_out) ? (hotk->status |WLED_ON ) : (hotk->status & ~WLED_ON ); +if (!write_acpi_int (hotk->handle, ledname, led_out,NULL )) +printk (KERN_WARNING "Asus ACPI: Writing WLED failed.\n" ); +} + +static void led_update_tled (void *private) +{ +char *ledname = hotk->methods->mt_tled; +int led_out = led_tled_value ?1 :0 ; + + hotk->status = (led_out) ? (hotk->status |TLED_ON ) : (hotk->status & ~TLED_ON ); +if (!write_acpi_int (hotk->handle, ledname, led_out,NULL )) +printk (KERN_WARNING "Asus ACPI: Writing TLED failed.\n" ); +} + +/* Registers LED class devices and sets up workqueue. */ +static int led_initialize (struct device *parent) +{ +int result; + +if (hotk->methods->mt_mled) { + led_cdev_mled.name ="asus:mail" ; + led_cdev_mled.brightness_set = led_set_mled; + result =led_classdev_register (parent, &led_cdev_mled); +if (result) goto mled_abort; + } + +if (hotk->methods->mt_wled) { + led_cdev_wled.name ="asus:wireless" ; + led_cdev_wled.brightness_set = led_set_wled; + result =led_classdev_register (parent, &led_cdev_wled); +if (result) goto wled_abort; + } + +if (hotk->methods->mt_tled) { + led_cdev_tled.name ="asus:touchpad" ; + led_cdev_tled.brightness_set = led_set_tled; + result =led_classdev_register (parent, &led_cdev_tled); +if (result) goto tled_abort; + } + + led_workqueue =create_singlethread_workqueue ("led_workqueue" ); + +return 0 ; + +tled_abort: +if (hotk->methods->mt_wled)led_classdev_unregister (&led_cdev_wled); +wled_abort: +if (hotk->methods->mt_mled)led_classdev_unregister (&led_cdev_mled); +mled_abort: +return result; + +} + +/* Destroys the workqueue and unregisters the LED class devices. */ +static void led_terminate (void ) +{ +destroy_workqueue (led_workqueue); + +if (hotk->methods->mt_tled) +led_classdev_unregister (&led_cdev_tled); + +if (hotk->methods->mt_wled) +led_classdev_unregister (&led_cdev_wled); + +if (hotk->methods->mt_mled) +led_classdev_unregister (&led_cdev_mled); +} + +#else + +static inline int led_initialize (struct device *parent) +{ +} + +static inline void led_terminate (void ) +{ +} + +#endif +static int asus_hotk_add_fs (struct acpi_device *device) {struct proc_dir_entry *proc;
@@ -1299,6 +1453,8 @@/* LED display is off by default */ hotk->ledd_status =0xFFF ; + result =led_initialize (acpi_get_physical_device (device->handle)); + end:if (result) {kfree (hotk);
@@ -1314,6 +1470,8 @@if (!device || !acpi_driver_data (device))return -EINVAL ; +led_terminate (); + status =acpi_remove_notify_handler (hotk->handle,ACPI_SYSTEM_NOTIFY , asus_hotk_notify);if (ACPI_FAILURE (status))
drivers/acpi/Kconfig +9 -0
@@ -199,6 +199,15 @@ something works not quite as expected, please use the mailing list available on the above page ([email protected]) +config ACPI_ASUS_NEW_LED + bool "ASUS/Medion LED subsystem integration" + depends on ACPI_ASUS + depends on LEDS_CLASS + help + This adds support for the new LED subsystem to the asus_acpi + driver. The LED's will show up as asus:mail, asus:wireless, + and asus:touchpad, as applicable to your laptop. + config ACPI_IBM tristate "IBM ThinkPad Laptop Extras" depends on X86
Richard Purdie
Acked-bydrivers/acpi/asus_acpi.c line 937
Quoted 8 lines · Thomas Tuttle
drivers/acpi/asus_acpi.c +8
-0+/* These functions are called by the LED subsystem to update the desired +* state of the LED's. */ +static void led_set_mled (struct led_classdev *led_cdev, +enum led_brightness value); +static void led_set_wled (struct led_classdev *led_cdev, +enum led_brightness value); +static void led_set_tled (struct led_classdev *led_cdev, +enum led_brightness value);
I don't think you need these anymore.
drivers/acpi/asus_acpi.c line 1066
Quoted 6 lines
drivers/acpi/asus_acpi.c +6
-0+#else + +static inline int led_initialize (struct device *parent) +{ +} +
This turns the code into a game of Russian roulette when CONFIG_ACPI_ASUS_NEW_LED isn't set (think return values). I'm sure Andrew was just testing with that ;-)
If you fix these issues, you can add an
Richard
Quoted 9 lines · Thomas Tuttle
Here is a patch to the
asus_acpidriver that links the Asus laptop LED's into the new LED subsystem. It creates LED class devices named asus:mail, asus:wireless, and asus:touchpad, depending on if the laptop supports the mled, wled, and tled LED's.Since it's so new, I added a config option to turn it on and off. It's worked for me, though I have an Asus M2N, which only has the mail and wireless LED's.
Quoted 6 lines
Signed-off-by Thomas Tuttle <[email protected]>I believe I've fixed everything you asked about, plus a few things Richard Purdie (the LED subsystem guy) suggested.
Thanks, Thomas, for your patch. It works well for me (ASUS V6V). There is only one thing I want to remark: Since the most recent BIOS ASUS changed the behaviour of the touchpad LED, it is inverted now. Until now I got around this in userspace (adapting the handler script). But with the new led class it seems to me, we will have to deal with that in the kernel module.
My suggestion is a new flag (tled_inv) which has to be set for every model (always use most recent BIOS). What do you think about this?
Greetings, Johannes
Hi!
Apart from codingstyle issues... yes, it looks good. Hooking various leds into led subsystem is way better than having all the separate drivers. I guess I'll have to convert ibm_acpi...
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On July 06 at 19:50 EDT, Pavel Machek hastily scribbled:
Quoted 1 line
Apart from codingstyle issues...
I've fixed a bunch that Andrew Morton and Richard Purdie mentioned (since your message, I've posted a new revision). Are there any changes I missed?
Quoted 1 line
yes, it looks good.
Nice. :-)
Quoted 3 lines
Hooking various
leds into led subsystem is way better than having all the separate drivers.
Yes. The LED subsystem is a really cool idea, because what good are blinkenlights if you have to write shell scripts to control them? The kernel should do that for you! (/me wishes someone made a laptop with something like 8 LED's of different colors, so he could just make them do what he liked.)
Quoted 1 line
I guess I'll have to convert
ibm_acpi...
I'm willing to take a first whack at it, if you can put me in touch with someone to test it.
--Thomas Tuttle
Hi!
Quoted 14 lines
Quoted 1 line
Apart from codingstyle issues...
I've fixed a bunch that Andrew Morton and Richard Purdie mentioned (since your message, I've posted a new revision). Are there any changes I missed?
Quoted 1 line
yes, it looks good.
Nice. :-)
Quoted 3 lines
Hooking various
leds into led subsystem is way better than having all the separate drivers.
Quoted 5 lines
Yes. The LED subsystem is a really cool idea, because what good are blinkenlights if you have to write shell scripts to control them? The kernel should do that for you! (/me wishes someone made a laptop with something like 8 LED's of different colors, so he could just make them do what he liked.)
Attach them to parallel port... I've done that long time ago. You can place 12 leds there...
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html