Re: [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: 4204 bytes --]

On Friday 11 November 2016 15:33:48 Hans de Goede wrote:
> Hi,
> 
> On 11-11-16 15:12, Pali Rohár wrote:
> > On Tuesday 01 November 2016 14:37:46 Hans de Goede wrote:
> >> Make thinkpad_acpi call led_notify_brightness_change on the
> >> kbd_led led_classdev registered by thinkpad_acpi when the kbd
> >> backlight brightness changes.
> >> 
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >> Changes in v3:
> >> -This is a new patch in v3 of this patch-set
> >> Changes in v4:
> >> -No Changes
> >> ---
> >> 
> >>  drivers/platform/x86/thinkpad_acpi.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >> 
> >> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> >> b/drivers/platform/x86/thinkpad_acpi.c index b65ce75..5dcd7d8b
> >> 100644
> >> --- a/drivers/platform/x86/thinkpad_acpi.c
> >> +++ b/drivers/platform/x86/thinkpad_acpi.c
> >> @@ -162,6 +162,7 @@ enum tpacpi_hkey_event_t {
> >> 
> >>  	TP_HKEY_EV_HOTKEY_BASE		= 0x1001, /* first hotkey (FN+F1) */
> >>  	TP_HKEY_EV_BRGHT_UP		= 0x1010, /* Brightness up */
> >>  	TP_HKEY_EV_BRGHT_DOWN		= 0x1011, /* Brightness down */
> >> 
> >> +	TP_HKEY_EV_THINKLIGHT		= 0x1012, /* Thinklight/kbd backlight 
*/
> > 
> > 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

That is not truth. X230 is in variant with both Thinklight (light above 
display) and keyboard backlight. Plus in BIOS it is possible to 
configure if you want to enable only Thinkligh, only keyboard backlight 
or both. Probably T430 and other 30s models can be found with both. But 
I do not own those modules...

> as those both
> serve the same purpose so it looks like the re-used
> the scancode.

So I could guess that it reports event when one of those leds are 
changed...

> I've tried this code on both a W541 and a P50 and both
> emit TP_ACPI_HOTKEYSCAN_FNPAGEUP for the backlight
> keyboard hotkey (fn + space), I'm naming the event for
> this TP_HKEY_EV_THINKLIGHT for consistency with the
> existing TP_ACPI_HKEY_THNKLGHT_MASK which corresponds
> to TP_ACPI_HOTKEYSCAN_FNPAGEUP.
> 
> >>  	TP_HKEY_EV_VOL_UP		= 0x1015, /* Volume up or unmute */
> >>  	TP_HKEY_EV_VOL_DOWN		= 0x1016, /* Volume down or unmute */
> >>  	TP_HKEY_EV_VOL_MUTE		= 0x1017, /* Mixer output mute */
> >> 
> >> @@ -5167,6 +5168,8 @@ static int __init kbdlight_init(struct
> >> ibm_init_struct *iibm) return rc;
> >> 
> >>  	}
> >> 
> >> +	tpacpi_hotkey_driver_mask_set(hotkey_driver_mask |
> >> +				      TP_ACPI_HKEY_THNKLGHT_MASK);
> >> 
> >>  	return 0;
> >>  
> >>  }
> >> 
> >> @@ -9114,6 +9117,8 @@ static void tpacpi_driver_event(const
> >> unsigned int hkey_event) volume_alsa_notify_change();
> >> 
> >>  		}
> >>  	
> >>  	}
> >> 
> >> +	if (tp_features.kbdlight && hkey_event == TP_HKEY_EV_THINKLIGHT)
> >> +	
> >> led_notify_brightness_change(&tpacpi_led_kbdlight.led_classdev)
> >> ;
> > 
> > This looks incorrect. You are trying to inform tpacpi_led_kbdlight
> > when tpacpi_led_thinklight change led status?
> 
> No as said the scancode / event is shared, notice the check for
> tp_features.kbdlight, so the notify will only happen on laptops which
> actually have a backlit keyboard.

If that event is send when either Thinklight or keyb-backlight is 
changed, should not we inform both led devices about level change? Do 
you have a chance to test it on notebook with Thinklight?

And funny question is what would happen on those machines which have 
both Thinklight and keyboard backlight?

Anyway, I would suggest different name, not TP_HKEY_EV_THINKLIGHT as it 
is not thinklight-only and could be misleading in keyboard backlight 
context...

> Regards,
> 
> Hans
> 
> >>  }
> >>  
> >>  static void hotkey_driver_event(const unsigned int scancode)

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

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