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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 20-11-16 15:59, Pali Rohár wrote:
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.

That was our first attempt at this, but because the brightness may also
be changed by triggers / blink-timers, we need to wakeup poll() in those
cases too (anything else would be inconsistent) and doing such a wakeup
in that case has turned out to cause too much overhead in some cases
(even if userspace is not listening), specifically the idle power uses
on some systems got multiplied by a factor of 5 or more.

So this approach was rejected.

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.

This has already been discussed and the third patch in the set, which is
the one making the trigger read-only has been dropped.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux