Messages in this thread | | | Subject | Re: [RFC PATCH 1/8] LED: Add LED Class | From | Richard Purdie <> | Date | Mon, 05 Dec 2005 15:38:26 +0000 |
| |
On Mon, 2005-12-05 at 14:59 +0000, David Vrabel wrote: > This LED subsystem isn't usable with LEDs that are controlled by I2C > GPIO devices. Getting rid of (struct led_device).lock would go some way > to making it work. It's not clear to me why it's needed anyway.
The lock is so no two code paths try and change led_device at once. This is particularly apparent when triggers are considered as you could otherwise end up with two calls trying to change an led to different triggers or other equally bizarre circumstances. We need to guarantee any trigger init and exit calls are made, other wise we'd have memory leaks (and worse). Locking the brightness calls is just an extension of that so the practise is fully evident.
For i2c devices to use this, the devices will have to use a workqueue for brightness changes, lock or no lock. The reason is that trigger events can come from undetermined kernel contexts and therefore the brightness changing function should not sleep.
> Suspend and resume probably needs to be LED specific.
The core functions are probably applicable in 95% of cases and any LED driver has the option of not using them if it so wishes.
Out of interest, what would an LED device wish to do instead of this? Corgi/Spitz already don't suspend one of the leds under certain circumstances as a device specific trigger (charging) is know to be suspend aware.
> > + > > +menu "LED devices" > > + > > +config NEW_LEDS > > Is there a better name than NEW_LEDS? It won't be 'new' for very long...
I'd prefer LEDS but this will clash with ARM. I'll wait for Russell's comments but I'm open to alternative suggestions.
> 0-255 is probably a better range to use. Might be worth having an enum > like. > > enum led_brightness { > LED_OFF = 0, LED_HALF_BRIGHT = 127, LED_FULL_BRIGHT = 255, > };
Yes, that's probably not a bad idea.
Cheers,
Richard
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |