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

[Thomas Tuttle <thinkinginbinary@gmail.com>]

[-- 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 --]