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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 15-11-16 15:04, Jacek Anaszewski wrote:
> On 11/15/2016 02:48 PM, Hans de Goede wrote:
>> 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.
>
> If so then when software blinking enabled, it will return 0 on low
> blink cycle no matter what current brightness level is.

For software blinking there will not be a current_brightness atrribute,
as we will only add that for LEDs with poll-able triggers.

Also during the low cycle the LED is off, so its brightness at the
moment of reading (iow currently) is 0.

Regards,

Hans