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

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

On 11/25/2016 09:40 PM, Pavel Machek wrote:
> Hi!
>
>>>> Triggers are not limited to periodic blinking or reporting cpu
>>>> activity. There is also oneshot trigger that can be used e.g. when
>>>> user touches the screen, as Pali mentioned.
>>>
>>> Using oneshot trigger for this would be pretty strange.
>>
>> It was only an example to mention other than periodic triggers.
>> You could have a trigger that just turns the LED permanently on
>> after user touches the screen.
>
> Well.. triggers kind of assume they control the LED. They were not
> prepared to deal with hardware changing the brightness behind their
> back.
>
>>> Notice that in
>>> some cases (thinkpad battery led, for example) we either have firmware
>>> controls the LED (but then software can't control it) or we have
>>> software controlling the LED (but then we don't know what firmware
>>> would put there). Maybe keyboard backlight can be controlled
>>> "simultaneously" by both software and firmware, but there are
>>> certainly LEDs that can't handle that, and IMO it would be nice to
>>> have same interface.
>>>
>>>>> Well.. actually... I think this is a little bit over complex and
>>>>> probably unneccessary. I'd let Hans implement whatever he thinks is
>>>>> easiest.
>>>>
>>>> I'd say this is the trigger approach which is a bit convoluted.
>>>
>>> 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 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?

>
>>> 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.

-- 
Best regards,
Jacek Anaszewski