On Wed, Jul 24, 2013 at 4:12 PM, Mark A. Greer <mgreer@xxxxxxxxxxxxxxx> wrote: > 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.] > Exactly, I read the code on master branch instead of my devel branch. Thanks, -Bryan -- 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
