On Wed, Jul 24, 2013 at 03:46:16PM -0700, Bryan Wu wrote: > On Wed, Jul 24, 2013 at 3:37 PM, Mark A. Greer <mgreer@xxxxxxxxxxxxxxx> wrote: > > [Moving this to appropriate thread with minor edits] > > > > On Tue, Jul 23, 2013 at 05:45:54PM -0700, Liam wrote: > >> On Tue, Jul 23, 2013 at 10:54:04AM -0700, Mark A. Greer wrote: > > > >> > NOTE: This patch violates the leds infrastructure > >> > driver interface since the hardware only supports > >> > blinking all LEDs with the same delay_on/delay_off > >> > rates. That is, only the LEDs that are set to blink > >> > will actually blink but all LEDs that are set to blink > >> > will blink in identical fashion. The delay_on/delay_off > >> > values of the last LED that is set to blink will be used > >> > for all of the blinking LEDs. > > > >> As written it triggers software-based blinking if the hardware cannot > >> perform the specified blink pattern. > >> > >> There's no value in hardware-based blinking over software UNLESS you want to > >> sleep the cpu to save power. If you want to save power, you don't want to > >> end up using software blink just to achieve a specific blink pattern. So you > >> should be able to disable hardware blink to obtain any blink pattern, or > >> enable it and save power no matter what. > >> > >> I propose, at the top of blink_set(): > >> if (!HWBLINK) // compile-time or runtime config option > >> > >> return EINVAL; // will use software blink > >> > >> Under the out-of-range test in blink_set(), set in-range defaults instead of > >> return EINVAL. > > > > Power management may or may not be an important issue on a particular board. > > If it is important then its not unreasonable for the sofware controlling the > > blinking to be aware of the hardware's constraints and to keep the blinking > > within those constraints. I prefer that to driver hacks. > > > > Bryan, do you have a preference? > > I basically agree with Liam here, although not just only because of > power saving. > > Since this hardware blink change is not a perfect replacement for > software blinking, we'd better to give users some choices, like > Kconfig option or device tree binding. > > So if people wanna try this hardware aided blinking, just enable the > option or write a right DTS file. Otherwise we use software blinking > by default. > > Looks like leds-pca9633.c doesn't support device tree right now. so > probably Kconfig option is better. [Actually, Tony Lindgren added DT support for that driver. The patch is 60b969f (leds: Add device tree binding for pca9633) which is in linux-leds/devel.] Okay, I will implement what you & Liam suggested. Thanks, Mark -- -- To unsubscribe from this list: send the line "unsubscribe linux-leds" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
