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

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

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

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.

> 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"?

> 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...

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

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