Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pavel Machek <pavel@ucw.cz>]

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

Hi!

> >>>In my eyes trigger approach is neccessary at least for some hardware,
> >>>and things it pretty clear: trigger on == LED changes without
> >>>userspace involvement. trigger off == userspace controls the LED.
> >>
> >>It is likely that it would break many existing users.
> >
> >Can you elaborate on that?
> 
> There might exist users that adjust LED brightness while having
> active trigger. The best example is default-on trigger - it sets
> brightness only on init, but remains active all the time. Whereas
> this could be fixed, there is another case: think of changing blinking
> brightness - it would be impossible.

I don't see the breakage. For existing blinking and default-on
triggers, existing behaviour would be kept. Difference would be that
for hardware that changes triggers itself (like the keyboard light) we
would present it as a trigger. And you are right -- there would be
user visible changes there. You could not assign blinking, because
.. it already has a trigger. But I believe it is reasonable.

> >I just tried with leds on thinkpad
> >
> >root@duo:/sys/class/leds/tpacpi::power# echo 1 > brightness
> >root@duo:/sys/class/leds/tpacpi::power# echo 0 > brightness
> >root@duo:/sys/class/leds/tpacpi::power# cat trigger
> >[none] bluetooth-power kbd-scrollock kbd-numlock kbd-capslock
> >kbd-kanalock kbd-shiftlock kbd-altgrlock kbd-ctrllock kbd-altlock
> >kbd-shiftllock kbd-shiftrlock kbd-ctrlllock kbd-ctrlrlock AC-online
> >BAT0-charging-or-full BAT0-charging BAT0-full
> >BAT0-charging-blink-full-solid rfkill0 phy0rx phy0tx phy0assoc
> >phy0radio phy0tpt mmc0 timer pattern rfkill1 hci0-power rfkill74
> >heartbeat
> >root@duo:/sys/class/leds/tpacpi::power#
> >
> >I can control the LED from userspace, but then there's no way to put
> >the LED back to firmware control. That's just broken.
> >
> >Do you have a proposal how to handle that?
> 
> Isn't it under firmware control all the time?

I'm not 100% sure in the thinkpad case. In the N900 case, we have LED
that displays (user_brightness || (user_enabled_indicator && cpu_activity)).

I believe clean way to do that would be a trigger.

> >>>So I'd do the trigger here. It is same way we can handle LEDs on
> >>>thinkpad. Yes, it means you won't be able to do oneshot trigger on
> >>>backlight. I don't think that's a huge problem.
> >>
> >>There have been voices in this discussion claiming the opposite. :-)
> >
> >Well, lets ignore those voices until the voices understand the current
> >design :-).
> 
> Sheer user is not interested in design, but in usability.

Well, end user is not expected to touch /sys files directly -- we
should create a script for that. /sys should be logical and map well
to what we do in kernel. Being "nice to use from shell" is
secondary...

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

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