LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109

[Hans de Goede]

Hi,

On 15-11-16 14:28, Jacek Anaszewski wrote:
> On 11/15/2016 01:06 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 15-11-16 12:48, Pavel Machek wrote:
>>> Hi!
>>>
>>>>>>> The LED you are talking about _has_ a trigger, implemented in
>>>>>>> hardware. That trigger can change LED brightness behind kernel's (and
>>>>>>> userspace's) back. Don't pretend the trigger does not exist, it does.
>>>>>>>
>>>>>>> And when you do that, you'll have nice place to report changes to
>>>>>>> userspace -- trigger can now export that information, and offer
>>>>>>> poll()
>>>>>>> interface.
>>>>>>
>>>>>> Well, that sounds interesting. It is logically justifiable.
>>>>>
>>>>> Thanks.
>>>>>
>>>>>> I initially proposed exactly this solution, with recently
>>>>>> added userspace LED being a trigger listener. It seems a bit
>>>>>> awkward though. How would you listen to the trigger events?
>>>>>
>>>>> Trigger exposes a file in sysfs, with poll() working on that file
>>>>
>>>> Hmm, a new file would give the advantage of making it easy for
>>>> userspace to see if the trigger is poll-able, this is likely
>>>> better then my own proposal I just send.
>>>
>>> Good.
>>>
>>>>> (and
>>>>> probably read exposing the current brightness).
>>>>
>>>> If we do this, can we please make it mirror brightness, iow
>>>> also make it writable, that will make it easier for userspace
>>>> to deal with it. We can simply re-use the existing show / store
>>>> methods for brightness for this.
>>>
>>> Actually, echo 0 > brightness disables the trigger, IIRC. I'd avoid
>>> that here, you want to be able to turn off the backlight but still
>>> keep the trigger (and be notified of future changes).
>>
>> True, that is easy to do the store method will just need to call
>> led_set_brightness_nosleep instead of led_set_brightness, this
>> will skip the checks to stop blinking in led_set_brightness and
>> otherwise is equivalent.
>>
>>>> I suggest we call it:
>>>>
>>>> trigger_brightness
>>>>
>>>> And only register it when a poll-able trigger is present.
>>>
>>> I'd call it 'current_brightness', but that's no big deal. Yes, only
>>> registering it for poll-able triggers makes sense.
>>
>> current_brightness works for me. I will take a shot a patch-set
>> implementing this.
>
> Word "current" is not precise here.
>
> It can be thought of as either last brightness set by the
> user or the brightness currently written to the device
> (returned by brightness file).
>
> There is a semantic discrepancy in our requirements -
> we want the file representing both permanent brightness
> set by the user and brightness set by the hardware.
>
> The two stand in contradiction to each other since
> brightness set by the user can be adjusted by the hardware.
>
> Reading the file shouldn't update brightness property of
> struct led_classdev, so it shouldn't call led_update_brightness()
> but it still should allow reading brightness set by the
> hardware, as a result of each POLLPRI event. So in fact in
> the same time it should report both according to our requirements
> which is impossible. Do we need three brightness files ?

I don't think so, current_brightness actually is an accurate
name, if the brightness was last changed by writing from
sysfs, the keyboard backlight will honor that and the current_brightness
attribute will show the brightness last set through writing it,
which matches the actual current brightness of the keyboard backlight.

Likewise if it was changed with the hotkey last then the keyboard
backlight brightness will be changed and reading from current_brightness
will return the new actual brightness. Basically reading from this
file will be no different then reading from the normal "brightness"
file the difference will be in that it is poll-able and that
writing 0 turns off the LED without stopping blinking.

Regards,

Hans