[PATCH] Integrate asus_acpi LED's with new LED subsystem

[PATCH] Integrate asus_acpi LED's with new LED subsystem

[Thomas Tuttle]

[-- Attachment #1.1: Type: text/plain, Size: 561 bytes --]

(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.

Signed-off-by: Thomas Tuttle <thinkinginbinary@gmail.com>

[-- Attachment #1.2: asus-acpi-led-subsystem.patch --]
[-- Type: text/plain, Size: 7583 bytes --]

diff -udrN linux-2.6.17-git25/drivers/acpi/asus_acpi.c linux-2.6.17-git25-mine/drivers/acpi/asus_acpi.c
--- linux-2.6.17-git25/drivers/acpi/asus_acpi.c	2006-07-05 22:11:37.000000000 -0400
+++ linux-2.6.17-git25-mine/drivers/acpi/asus_acpi.c	2006-07-05 22:26:51.000000000 -0400
@@ -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))
diff -udrN linux-2.6.17-git25/drivers/acpi/Kconfig linux-2.6.17-git25-mine/drivers/acpi/Kconfig
--- linux-2.6.17-git25/drivers/acpi/Kconfig	2006-07-05 22:44:33.000000000 -0400
+++ linux-2.6.17-git25-mine/drivers/acpi/Kconfig	2006-07-05 22:44:43.000000000 -0400
@@ -199,6 +199,15 @@
           something works not quite as expected, please use the mailing list
           available on the above page (acpi4asus-user@lists.sourceforge.net)
           
+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

[-- Attachment #2: Type: application/pgp-signature, Size: 191 bytes --]

Re: [PATCH] Integrate asus_acpi LED's with new LED subsystem

[Richard Purdie]

On Thu, 2006-07-06 at 15:31 -0400, Thomas Tuttle wrote:
> +#include <linux/config.h>

Not needed.

>  #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

Doesn't need the ifdefs, the header should be harmless

> +/* 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... :)

> +/* 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.
            
> +/* 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);

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...

> +/* 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);
> +        }
> +}

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

Re: [PATCH] Integrate asus_acpi LED's with new LED subsystem

[Andrew Morton]

Thomas Tuttle <thinkinginbinary@gmail.com> wrote:
>
> (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.
> 
> ...
>
>  #include <linux/types.h>
>  #include <linux/proc_fs.h>
> +#ifdef CONFIG_ACPI_ASUS_NEW_LED
> +#include <linux/leds.h>
> +#endif

The ifdef shouldn't be required.  If it is, ldes.h needs fixing.

> +#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

> +/* 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

> +/* 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

> +/* 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.

> +/* 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.

> +/* LED work functions. */
> +static void led_update_mled(void *private) {

The opening brace goes in column 1.

> +        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);
> +}

It's usual to put a blank line at the end of the declaration of the
automatic valriables.

> +
> +/* Registers LED class devices and sets up workqueue. */
> +{
> +        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

Add:

#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 +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          
> +

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.

Re: [PATCH] Integrate asus_acpi LED's with new LED subsystem

[Pavel Machek]

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


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

Re: [PATCH] Integrate asus_acpi LED's with new LED subsystem

[Thomas Tuttle]

[-- Attachment #1: Type: text/plain, Size: 2910 bytes --]

On July 06 at 18:39 EDT, Richard Purdie hastily scribbled:
> On Thu, 2006-07-06 at 15:31 -0400, Thomas Tuttle wrote:
> > +#include <linux/config.h>
> Not needed.
Will be fixed.

> > +#ifdef CONFIG_ACPI_ASUS_NEW_LED
> > +#include <linux/leds.h>
> > +#endif
> Doesn't need the ifdefs, the header should be harmless
Will be fixed.
> 
> > +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.

> > +/* 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.

> > +        hotk->status = (led_out) ? (hotk->status | MLED_ON) :
> > (hotk->status & ~MLED_ON);
> 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...
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.

> 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...
Will be fixed.

> 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...
/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

[-- Attachment #2: Type: application/pgp-signature, Size: 191 bytes --]

Re: [PATCH] Integrate asus_acpi LED's with new LED subsystem

[Thomas Tuttle]

[-- Attachment #1.1: Type: text/plain, Size: 2760 bytes --]

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.

Signed-off-by: Thomas Tuttle <thinkinginbinary@gmail.com>


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:
> Thomas Tuttle <thinkinginbinary@gmail.com> wrote:
> > +#ifdef CONFIG_ACPI_ASUS_NEW_LED
> > +#include <linux/leds.h>
> > +#endif
> The ifdef shouldn't be required.  If it is, ldes.h needs fixing.
Fixed.

> 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.)

> 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.

> > +/* LED update work structs. */
> These all should be declared static.
Fixed.

> > +/* LED work functions. */
> > +static void led_update_mled(void *private) {
> The opening brace goes in column 1.
Fixed.

> It's usual to put a blank line at the end of the declaration of the
> automatic valriables.
Fixed.

> Add:
> 
> #else
> static inline int led_initialize(struct device *parent)
> {
> }
> 
> static inline void led_terminate(void)
> {
> }
> #endif
Fixed.

> > +#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

[-- Attachment #1.2: asus-acpi-led-subsystem.patch --]
[-- Type: text/plain, Size: 6784 bytes --]

diff -udrN linux-2.6.17-git25/drivers/acpi/asus_acpi.c linux-2.6.17-git25-mine/drivers/acpi/asus_acpi.c
--- linux-2.6.17-git25/drivers/acpi/asus_acpi.c	2006-07-05 22:11:37.000000000 -0400
+++ linux-2.6.17-git25-mine/drivers/acpi/asus_acpi.c	2006-07-06 21:06:58.000000000 -0400
@@ -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))
diff -udrN linux-2.6.17-git25/drivers/acpi/Kconfig linux-2.6.17-git25-mine/drivers/acpi/Kconfig
--- linux-2.6.17-git25/drivers/acpi/Kconfig	2006-07-05 22:44:33.000000000 -0400
+++ linux-2.6.17-git25-mine/drivers/acpi/Kconfig	2006-07-05 22:44:43.000000000 -0400
@@ -199,6 +199,15 @@
           something works not quite as expected, please use the mailing list
           available on the above page (acpi4asus-user@lists.sourceforge.net)
           
+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

[-- Attachment #2: Type: application/pgp-signature, Size: 191 bytes --]

Re: [PATCH] Integrate asus_acpi LED's with new LED subsystem

[Thomas Tuttle]

[-- Attachment #1: Type: text/plain, Size: 833 bytes --]

On July 06 at 19:50 EDT, Pavel Machek hastily scribbled:
> 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?

> yes, it looks good.

Nice. :-)

> 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.)

> 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

[-- Attachment #2: Type: application/pgp-signature, Size: 191 bytes --]

Re: [PATCH] Integrate asus_acpi LED's with new LED subsystem

[Richard Purdie]

On Thu, 2006-07-06 at 21:20 -0400, Thomas Tuttle wrote:

> +/* 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.

> +#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
Acked-by: Richard Purdie <rpurdie@rpsys.net>

Richard

Re: [PATCH] Integrate asus_acpi LED's with new LED subsystem

[Pavel Machek]

Hi!

> > 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?
> 
> > yes, it looks good.
> 
> Nice. :-)
> 
> > 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.)

Attach them to parallel port... I've done that long time ago. You can
place 12 leds there...
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] Integrate asus_acpi LED's with new LED subsystem

[Johannes Engel]

Thomas Tuttle schrieb:
> 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.
> 
> Signed-off-by: Thomas Tuttle <thinkinginbinary@gmail.com>
> 
> 
> 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