Re: [PATCH v5 1/6] leds: triggers: Add current_brightness trigger parameter

["Pali Rohár" <pali.rohar@gmail.com>]

[-- Attachment #1: Type: Text/Plain, Size: 2175 bytes --]

On Thursday 17 November 2016 23:24:36 Hans de Goede wrote:
> In some cases it may be desirable for userspace to be notified when
> a trigger event happens. This commit adds support for a poll-able
> current_brightness trigger specific sysfs attribute which triggers
> may register:
> 
> What:		/sys/class/leds/<led>/current_brightness
> Date:		November 2016
> KernelVersion:	4.10
> Description:
> 	Triggers which support it may register a current_brightness
> 	file. This file supports poll() to detect when the trigger
> 	modifies the brightness of the LED.
> 	Reading this file will always return the current brightness
> 	of the LED.
> 	Writing this file sets the current brightness of the LED,
> 	without influencing the trigger.

Personally I do not like this new sysfs attribute...

Now when somebody look at /sys/class/leds/<something>/, the first thing 
which say would be:

"What the hell, why there are two files (brightness and 
current_brightness) for changing LED level? And which should I use?"

If I understood correctly we need to handle two things:

1) Provide poll() for userspace when LED level is changed (either by HW
   or other user call)

2) Deal with fact that on _some_ hardware, special key is hardwired to
   change LED level

So why for 1) we cannot use existing sysfs file "brightness"? I do not 
see any problem with it.

And for 2) we even do not know on which machines is such hardwired 
feature enabled. Yes, on _some_ (not *all*) Dell machines there is Fn 
key combination which changes level of one LED device. But kernel does 
not know if hardware on which is running is doing such thing or not. 
Some machines do not have to have key for such action and we do not know 
it.

And what about situation when somebody wants to configure e.g. mouse 
movement (or keypress) trigger to enable/disable LED device (which 
belongs to keyboard brightness)? In this case user explicitly know that 
his Fn+Space change level of LED device, so can be careful to not press 
it. With your read-only trigger you basically disable such (I think 
useful) feature.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]