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

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

Hi,

On 12/21/2016 07:49 PM, Pavel Machek wrote:
> 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.

We've already received negative feedback regarding blocking
application of different trigger when hw brightness change notifications
are enabled. One solution to that is making the notifications
orthogonal to the trigger mechanism. The other possibility is to allow
for having more than one active trigger for a LED.

We could think of defining a special type of persistent trigger that
would be always enabled.

Best regards,
Jacek Anaszewski


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