Re: [ibm-acpi-devel] [PATCH v4 2/4] platform: x86: thinkpad: Call led_notify_brightness_change on kbd brightness change

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

HI,

On 12-11-16 12:52, Pali Rohár wrote:
> On Friday 11 November 2016 19:40:51 Kevin Locke wrote:
>> On Fri, 2016-11-11 at 15:33 +0100, Hans de Goede wrote:
>>> On 11-11-16 15:12, Pali Rohár wrote:
>>>> My question remains. Is this for Thinklight or keyboard backlight?
>>>> Because Thinklinght has led device "tpacpi_led_thinklight" and
>>>> keyboard backlight has led device "tpacpi_led_kbdlight".
>>>
>>> I would say both, this matches with the pre-existing
>>> TP_ACPI_HKEY_THNKLGHT_MASK (they have a 1:1 mapping),
>>> keep in mind that there are no thinkpads with both
>>> a thinklight and a backlit keyboard, as those both
>>> serve the same purpose so it looks like the re-used
>>> the scancode.
>>
>> That's not entirely correct.  The ThinkPad T430 has both a ThinkLight
>> and a keyboard backlight.  Pressing Fn-Space toggles between 4
>> states:
>>
>> Both Off -> Backlight Low -> Backlight High -> ThinkLight On (BL Off)
>>
>> I tested out your patch and observed the following behavior (printing
>> brightness initially and on POLLPRI):
>>
>> # Initial state with both lights off.
>> tpacpi::kbd_backlight/brightness: 0
>> tpacpi::thinklight/brightness: 0
>> # Press Fn-Space.  KBD Backlight comes on low.
>> tpacpi::kbd_backlight/brightness: 1
>> # Press Fn-Space.  KBD Backlight brightens to high.
>> tpacpi::kbd_backlight/brightness: 2
>> # Press Fn-Space.  KBD Backlight turns off.  ThinkLight turns on.
>> tpacpi::kbd_backlight/brightness: 0
>> # Press Fn-Space.  ThinkLight turns off.
>> tpacpi::kbd_backlight/brightness: 0
>>
>> It works, but the behavior is not quite what I would hope for.  There
>> are no poll events for tpacpi::thinklight/brightness
>
> Yes, this is what I already pointed... That we should send event also
> for tpacpi::thinklight.

Agreed.

>> and when the
>> ThinkLight turns off it triggers an unnecessary
>> tpacpi::kbd_backlight/brightness poll event.
>
> It is problem? Or are forced to send event only if brightness level is
> really changed? I think that bigger problem is when brightness changes,
> but we do not send event.
>
> Should not be event treated as "hey, brightness level was probably
> changed, read file to get current status"?

That is what I was thinking too, waking up userspace poll()
waiters while nothing has changed is not 100% ideal, but
is not a big deal (esp. given the frequency with which this
will happen).

I will do a new version renaming the events / mask to indicate
that they apply to both the thinklight and the keyboard backlight;
and to also notify the thinklight led_classdev of changes
(if it is present).

Note that the led-class changes this relies on are still being
discussed, I will do a new version once that is sorted out.

>> If there is anything else I can do to assist, let me know.
>
> Great, this is really useful to see somebody who can test patches on
> those machines with both Thinklight and keyboard backlight...

+1 thank you for testing and for the feedback.

Regards,

Hans

>