platform-driver-x86.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Allow the FnLock LED to change state
@ 2021-03-15 19:58 Esteve Varela Colominas
  2021-03-18 16:49 ` [PATCH] thinkpad_acpi: " Hans de Goede
  2021-03-21 16:38 ` [PATCH] " Hans de Goede
  0 siblings, 2 replies; 12+ messages in thread
From: Esteve Varela Colominas @ 2021-03-15 19:58 UTC (permalink / raw)
  To: Henrique de Moraes Holschuh
  Cc: Esteve Varela Colominas, ibm-acpi-devel, platform-driver-x86

On many recent ThinkPad laptops, there's a new LED next to the ESC key,
that indicates the FnLock status.
When the Fn+ESC combo is pressed, FnLock is toggled, which causes the
Media Key functionality to change, making it so that the media keys
either perform their media key function, or function as an F-key by
default. The Fn key can be used the access the alternate function at any
time.

With the current linux kernel, the LED doens't change state if you press
the Fn+ESC key combo. However, the media key functionality *does*
change. This is annoying, since the LED will stay on if it was on during
bootup, and it makes it hard to keep track what the current state of the
FnLock is.

This patch calls an ACPI function, that gets the current media key
state, when the Fn+ESC key combo is pressed. Through testing it was
discovered that this function causes the LED to update correctly to
reflect the current state when this function is called.

The relevant ACPI calls are the following:
\_SB_.PCI0.LPC0.EC0_.HKEY.GMKS: Get media key state, returns 0x603 if the FnLock mode is enabled, and 0x602 if it's disabled.
\_SB_.PCI0.LPC0.EC0_.HKEY.SMKS: Set media key state, sending a 1 will enable FnLock mode, and a 0 will disable it.

Relevant discussion:
https://bugzilla.kernel.org/show_bug.cgi?id=207841
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1881015

Signed-off-by: Esteve Varela Colominas <esteve.varela@gmail.com>
---
 drivers/platform/x86/thinkpad_acpi.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index c40470637..09362dd74 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -4079,13 +4079,19 @@ static bool hotkey_notify_6xxx(const u32 hkey,
 
 	case TP_HKEY_EV_KEY_NUMLOCK:
 	case TP_HKEY_EV_KEY_FN:
-	case TP_HKEY_EV_KEY_FN_ESC:
 		/* key press events, we just ignore them as long as the EC
 		 * is still reporting them in the normal keyboard stream */
 		*send_acpi_ev = false;
 		*ignore_acpi_ev = true;
 		return true;
 
+	case TP_HKEY_EV_KEY_FN_ESC:
+		/* Get the media key status to foce the status LED to update */
+		acpi_evalf(hkey_handle, NULL, "GMKS", "v");
+		*send_acpi_ev = false;
+		*ignore_acpi_ev = true;
+		return true;
+
 	case TP_HKEY_EV_TABLET_CHANGED:
 		tpacpi_input_send_tabletsw();
 		hotkey_tablet_mode_notify_change();
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread
* Re: [PATCH] thinkpad_acpi: Allow the FnLock LED to change state
  2021-03-15 19:58 [PATCH] Allow the FnLock LED to change state Esteve Varela Colominas
@ 2021-03-18 16:49 ` Hans de Goede
  2021-03-18 20:13   ` [External] " Mark Pearson
  2021-03-21 16:38 ` [PATCH] " Hans de Goede
  1 sibling, 1 reply; 12+ messages in thread
From: Hans de Goede @ 2021-03-18 16:49 UTC (permalink / raw)
  To: Esteve Varela Colominas, Henrique de Moraes Holschuh, Mark Pearson
  Cc: ibm-acpi-devel, platform-driver-x86

Hi,

On 3/15/21 8:58 PM, Esteve Varela Colominas wrote:
> On many recent ThinkPad laptops, there's a new LED next to the ESC key,
> that indicates the FnLock status.
> When the Fn+ESC combo is pressed, FnLock is toggled, which causes the
> Media Key functionality to change, making it so that the media keys
> either perform their media key function, or function as an F-key by
> default. The Fn key can be used the access the alternate function at any
> time.
> 
> With the current linux kernel, the LED doens't change state if you press
> the Fn+ESC key combo. However, the media key functionality *does*
> change. This is annoying, since the LED will stay on if it was on during
> bootup, and it makes it hard to keep track what the current state of the
> FnLock is.
> 
> This patch calls an ACPI function, that gets the current media key
> state, when the Fn+ESC key combo is pressed. Through testing it was
> discovered that this function causes the LED to update correctly to
> reflect the current state when this function is called.
> 
> The relevant ACPI calls are the following:
> \_SB_.PCI0.LPC0.EC0_.HKEY.GMKS: Get media key state, returns 0x603 if the FnLock mode is enabled, and 0x602 if it's disabled.
> \_SB_.PCI0.LPC0.EC0_.HKEY.SMKS: Set media key state, sending a 1 will enable FnLock mode, and a 0 will disable it.
> 
> Relevant discussion:
> https://bugzilla.kernel.org/show_bug.cgi?id=207841
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1881015
> 
> Signed-off-by: Esteve Varela Colominas <esteve.varela@gmail.com>
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index c40470637..09362dd74 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -4079,13 +4079,19 @@ static bool hotkey_notify_6xxx(const u32 hkey,
>  
>  	case TP_HKEY_EV_KEY_NUMLOCK:
>  	case TP_HKEY_EV_KEY_FN:
> -	case TP_HKEY_EV_KEY_FN_ESC:
>  		/* key press events, we just ignore them as long as the EC
>  		 * is still reporting them in the normal keyboard stream */
>  		*send_acpi_ev = false;
>  		*ignore_acpi_ev = true;
>  		return true;
>  
> +	case TP_HKEY_EV_KEY_FN_ESC:
> +		/* Get the media key status to foce the status LED to update */
> +		acpi_evalf(hkey_handle, NULL, "GMKS", "v");

Sicne this is a getter function I guess that calling it is mostly harmless
and if it works around what seems to be a firmware bug on some of the E?95
ThinkPad models then I guess that this is fine by me.

Mark, do you have any comments on this ?

Regards,

Hans



> +		*send_acpi_ev = false;
> +		*ignore_acpi_ev = true;
> +		return true;
> +
>  	case TP_HKEY_EV_TABLET_CHANGED:
>  		tpacpi_input_send_tabletsw();
>  		hotkey_tablet_mode_notify_change();
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [External] Re: [PATCH] thinkpad_acpi: Allow the FnLock LED to change state
  2021-03-18 16:49 ` [PATCH] thinkpad_acpi: " Hans de Goede
@ 2021-03-18 20:13   ` Mark Pearson
  2021-03-19 10:39     ` Nitin Joshi1
  0 siblings, 1 reply; 12+ messages in thread
From: Mark Pearson @ 2021-03-18 20:13 UTC (permalink / raw)
  To: Hans de Goede, Esteve Varela Colominas,
	Henrique de Moraes Holschuh, Nitin Joshi1
  Cc: ibm-acpi-devel, platform-driver-x86

Thanks Hans

On 18/03/2021 12:49, Hans de Goede wrote:
> Hi,
> 
> On 3/15/21 8:58 PM, Esteve Varela Colominas wrote:
>> On many recent ThinkPad laptops, there's a new LED next to the ESC key,
>> that indicates the FnLock status.
>> When the Fn+ESC combo is pressed, FnLock is toggled, which causes the
>> Media Key functionality to change, making it so that the media keys
>> either perform their media key function, or function as an F-key by
>> default. The Fn key can be used the access the alternate function at any
>> time.
>>
>> With the current linux kernel, the LED doens't change state if you press
>> the Fn+ESC key combo. However, the media key functionality *does*
>> change. This is annoying, since the LED will stay on if it was on during
>> bootup, and it makes it hard to keep track what the current state of the
>> FnLock is.
>>
>> This patch calls an ACPI function, that gets the current media key
>> state, when the Fn+ESC key combo is pressed. Through testing it was
>> discovered that this function causes the LED to update correctly to
>> reflect the current state when this function is called.
>>
>> The relevant ACPI calls are the following:
>> \_SB_.PCI0.LPC0.EC0_.HKEY.GMKS: Get media key state, returns 0x603 if the FnLock mode is enabled, and 0x602 if it's disabled.
>> \_SB_.PCI0.LPC0.EC0_.HKEY.SMKS: Set media key state, sending a 1 will enable FnLock mode, and a 0 will disable it.
>>
>> Relevant discussion:
>> https://bugzilla.kernel.org/show_bug.cgi?id=207841
>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1881015
>>
>> Signed-off-by: Esteve Varela Colominas <esteve.varela@gmail.com>
>> ---
>>  drivers/platform/x86/thinkpad_acpi.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>> index c40470637..09362dd74 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -4079,13 +4079,19 @@ static bool hotkey_notify_6xxx(const u32 hkey,
>>  
>>  	case TP_HKEY_EV_KEY_NUMLOCK:
>>  	case TP_HKEY_EV_KEY_FN:
>> -	case TP_HKEY_EV_KEY_FN_ESC:
>>  		/* key press events, we just ignore them as long as the EC
>>  		 * is still reporting them in the normal keyboard stream */
>>  		*send_acpi_ev = false;
>>  		*ignore_acpi_ev = true;
>>  		return true;
>>  
>> +	case TP_HKEY_EV_KEY_FN_ESC:
>> +		/* Get the media key status to foce the status LED to update */
>> +		acpi_evalf(hkey_handle, NULL, "GMKS", "v");
> 
> Sicne this is a getter function I guess that calling it is mostly harmless
> and if it works around what seems to be a firmware bug on some of the E?95
> ThinkPad models then I guess that this is fine by me.
> 
> Mark, do you have any comments on this ?
I'd like to follow up with the firmware team to make sure we've got the
correct details and implementation (kudos on the reverse engineering
though).

Nitin - you've worked with the firmware team on hotkeys, would you mind
digging into this one please to confirm. In particular if there's been a
change how do we make sure we don't impact older platforms etc.

Mark

^ permalink raw reply	[flat|nested] 12+ messages in thread
* RE: [External] Re: [PATCH] thinkpad_acpi: Allow the FnLock LED to change state
  2021-03-18 20:13   ` [External] " Mark Pearson
@ 2021-03-19 10:39     ` Nitin Joshi1
  2021-03-19 11:35       ` Enrico Weigelt, metux IT consult
  2021-03-21 16:37       ` Hans de Goede
  0 siblings, 2 replies; 12+ messages in thread
From: Nitin Joshi1 @ 2021-03-19 10:39 UTC (permalink / raw)
  To: Mark RH Pearson, Hans de Goede, Esteve Varela Colominas,
	Henrique de Moraes Holschuh
  Cc: ibm-acpi-devel, platform-driver-x86

Hello,

>-----Original Message-----
>From: Mark RH Pearson <markpearson@lenovo.com>
>Sent: Friday, March 19, 2021 5:13 AM
>To: Hans de Goede <hdegoede@redhat.com>; Esteve Varela Colominas
><esteve.varela@gmail.com>; Henrique de Moraes Holschuh <ibm-
>acpi@hmh.eng.br>; Nitin Joshi1 <njoshi1@lenovo.com>
>Cc: ibm-acpi-devel@lists.sourceforge.net; platform-driver-
>x86@vger.kernel.org
>Subject: Re: [External] Re: [PATCH] thinkpad_acpi: Allow the FnLock LED to
>change state
>
>Thanks Hans
>
>On 18/03/2021 12:49, Hans de Goede wrote:
>> Hi,
>>
>> On 3/15/21 8:58 PM, Esteve Varela Colominas wrote:
>>> On many recent ThinkPad laptops, there's a new LED next to the ESC
>>> key, that indicates the FnLock status.
>>> When the Fn+ESC combo is pressed, FnLock is toggled, which causes the
>>> Media Key functionality to change, making it so that the media keys
>>> either perform their media key function, or function as an F-key by
>>> default. The Fn key can be used the access the alternate function at
>>> any time.
>>>
>>> With the current linux kernel, the LED doens't change state if you
>>> press the Fn+ESC key combo. However, the media key functionality
>>> *does* change. This is annoying, since the LED will stay on if it was
>>> on during bootup, and it makes it hard to keep track what the current
>>> state of the FnLock is.
>>>
>>> This patch calls an ACPI function, that gets the current media key
>>> state, when the Fn+ESC key combo is pressed. Through testing it was
>>> discovered that this function causes the LED to update correctly to
>>> reflect the current state when this function is called.
>>>
>>> The relevant ACPI calls are the following:
>>> \_SB_.PCI0.LPC0.EC0_.HKEY.GMKS: Get media key state, returns 0x603 if
>the FnLock mode is enabled, and 0x602 if it's disabled.
>>> \_SB_.PCI0.LPC0.EC0_.HKEY.SMKS: Set media key state, sending a 1 will
>enable FnLock mode, and a 0 will disable it.
>>>
>>> Relevant discussion:
>>> https://bugzilla.kernel.org/show_bug.cgi?id=207841
>>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1881015
>>>
>>> Signed-off-by: Esteve Varela Colominas <esteve.varela@gmail.com>
>>> ---
>>>  drivers/platform/x86/thinkpad_acpi.c | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/platform/x86/thinkpad_acpi.c
>>> b/drivers/platform/x86/thinkpad_acpi.c
>>> index c40470637..09362dd74 100644
>>> --- a/drivers/platform/x86/thinkpad_acpi.c
>>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>>> @@ -4079,13 +4079,19 @@ static bool hotkey_notify_6xxx(const u32
>>> hkey,
>>>
>>>  	case TP_HKEY_EV_KEY_NUMLOCK:
>>>  	case TP_HKEY_EV_KEY_FN:
>>> -	case TP_HKEY_EV_KEY_FN_ESC:
>>>  		/* key press events, we just ignore them as long as the EC
>>>  		 * is still reporting them in the normal keyboard stream */
>>>  		*send_acpi_ev = false;
>>>  		*ignore_acpi_ev = true;
>>>  		return true;
>>>
>>> +	case TP_HKEY_EV_KEY_FN_ESC:
>>> +		/* Get the media key status to foce the status LED to update */
>>> +		acpi_evalf(hkey_handle, NULL, "GMKS", "v");
>>
>> Sicne this is a getter function I guess that calling it is mostly
>> harmless and if it works around what seems to be a firmware bug on
>> some of the E?95 ThinkPad models then I guess that this is fine by me.
>>
>> Mark, do you have any comments on this ?
>I'd like to follow up with the firmware team to make sure we've got the correct
>details and implementation (kudos on the reverse engineering though).
>
>Nitin - you've worked with the firmware team on hotkeys, would you mind
>digging into this one please to confirm. In particular if there's been a change
>how do we make sure we don't impact older platforms etc.

Regarding "GMKS" method, it does not have "version" related information. So , its unlikely to impact any older platforms.
However, I got it confirmed that definition of GMKS method itself doesn't include any workaround feature. 

But, since its getter function , I also think its harmless and if it workaround some issue then I don’t see any concern.

>
>Mark

Thanks & Regards,
Nitin Joshi  

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [External] Re: [PATCH] thinkpad_acpi: Allow the FnLock LED to change state
  2021-03-19 10:39     ` Nitin Joshi1
@ 2021-03-19 11:35       ` Enrico Weigelt, metux IT consult
  2021-03-22  0:53         ` Nitin Joshi1
  2021-03-21 16:37       ` Hans de Goede
  1 sibling, 1 reply; 12+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2021-03-19 11:35 UTC (permalink / raw)
  To: Nitin Joshi1, Mark RH Pearson, Hans de Goede,
	Esteve Varela Colominas, Henrique de Moraes Holschuh
  Cc: ibm-acpi-devel, platform-driver-x86

On 19.03.21 11:39, Nitin Joshi1 wrote:

Hi,

> Regarding "GMKS" method, it does not have "version" related information. So , its unlikely to impact any older platforms.
> However, I got it confirmed that definition of GMKS method itself doesn't include any workaround feature.
> 
> But, since its getter function , I also think its harmless and if it workaround some issue then I don’t see any concern.

How about publishing schematics / specs, so we can confirm what it's
really doing ?

In recent decades I've learn to mistrust vendor-provided firmware
(especially when it comes to acpi). The reason why I'm currently
bisecting AMD's AGESA blob, in order to completely replace it.

It's a tedious job, had to write my own analysis tool, but step
by step making progress. (and already learned they've been using a
pretty inefficient compiler that can't even inline trivial memcpy().
Probably some older msvc ... :p)

--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [External] Re: [PATCH] thinkpad_acpi: Allow the FnLock LED to change state
  2021-03-19 10:39     ` Nitin Joshi1
  2021-03-19 11:35       ` Enrico Weigelt, metux IT consult
@ 2021-03-21 16:37       ` Hans de Goede
  1 sibling, 0 replies; 12+ messages in thread
From: Hans de Goede @ 2021-03-21 16:37 UTC (permalink / raw)
  To: Nitin Joshi1, Mark RH Pearson, Esteve Varela Colominas,
	Henrique de Moraes Holschuh
  Cc: ibm-acpi-devel, platform-driver-x86

Hi,

On 3/19/21 11:39 AM, Nitin Joshi1 wrote:
> Hello,
> 
>> -----Original Message-----
>> From: Mark RH Pearson <markpearson@lenovo.com>
>> Sent: Friday, March 19, 2021 5:13 AM
>> To: Hans de Goede <hdegoede@redhat.com>; Esteve Varela Colominas
>> <esteve.varela@gmail.com>; Henrique de Moraes Holschuh <ibm-
>> acpi@hmh.eng.br>; Nitin Joshi1 <njoshi1@lenovo.com>
>> Cc: ibm-acpi-devel@lists.sourceforge.net; platform-driver-
>> x86@vger.kernel.org
>> Subject: Re: [External] Re: [PATCH] thinkpad_acpi: Allow the FnLock LED to
>> change state
>>
>> Thanks Hans
>>
>> On 18/03/2021 12:49, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 3/15/21 8:58 PM, Esteve Varela Colominas wrote:
>>>> On many recent ThinkPad laptops, there's a new LED next to the ESC
>>>> key, that indicates the FnLock status.
>>>> When the Fn+ESC combo is pressed, FnLock is toggled, which causes the
>>>> Media Key functionality to change, making it so that the media keys
>>>> either perform their media key function, or function as an F-key by
>>>> default. The Fn key can be used the access the alternate function at
>>>> any time.
>>>>
>>>> With the current linux kernel, the LED doens't change state if you
>>>> press the Fn+ESC key combo. However, the media key functionality
>>>> *does* change. This is annoying, since the LED will stay on if it was
>>>> on during bootup, and it makes it hard to keep track what the current
>>>> state of the FnLock is.
>>>>
>>>> This patch calls an ACPI function, that gets the current media key
>>>> state, when the Fn+ESC key combo is pressed. Through testing it was
>>>> discovered that this function causes the LED to update correctly to
>>>> reflect the current state when this function is called.
>>>>
>>>> The relevant ACPI calls are the following:
>>>> \_SB_.PCI0.LPC0.EC0_.HKEY.GMKS: Get media key state, returns 0x603 if
>> the FnLock mode is enabled, and 0x602 if it's disabled.
>>>> \_SB_.PCI0.LPC0.EC0_.HKEY.SMKS: Set media key state, sending a 1 will
>> enable FnLock mode, and a 0 will disable it.
>>>>
>>>> Relevant discussion:
>>>> https://bugzilla.kernel.org/show_bug.cgi?id=207841
>>>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1881015
>>>>
>>>> Signed-off-by: Esteve Varela Colominas <esteve.varela@gmail.com>
>>>> ---
>>>>  drivers/platform/x86/thinkpad_acpi.c | 8 +++++++-
>>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/platform/x86/thinkpad_acpi.c
>>>> b/drivers/platform/x86/thinkpad_acpi.c
>>>> index c40470637..09362dd74 100644
>>>> --- a/drivers/platform/x86/thinkpad_acpi.c
>>>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>>>> @@ -4079,13 +4079,19 @@ static bool hotkey_notify_6xxx(const u32
>>>> hkey,
>>>>
>>>>  	case TP_HKEY_EV_KEY_NUMLOCK:
>>>>  	case TP_HKEY_EV_KEY_FN:
>>>> -	case TP_HKEY_EV_KEY_FN_ESC:
>>>>  		/* key press events, we just ignore them as long as the EC
>>>>  		 * is still reporting them in the normal keyboard stream */
>>>>  		*send_acpi_ev = false;
>>>>  		*ignore_acpi_ev = true;
>>>>  		return true;
>>>>
>>>> +	case TP_HKEY_EV_KEY_FN_ESC:
>>>> +		/* Get the media key status to foce the status LED to update */
>>>> +		acpi_evalf(hkey_handle, NULL, "GMKS", "v");
>>>
>>> Sicne this is a getter function I guess that calling it is mostly
>>> harmless and if it works around what seems to be a firmware bug on
>>> some of the E?95 ThinkPad models then I guess that this is fine by me.
>>>
>>> Mark, do you have any comments on this ?
>> I'd like to follow up with the firmware team to make sure we've got the correct
>> details and implementation (kudos on the reverse engineering though).
>>
>> Nitin - you've worked with the firmware team on hotkeys, would you mind
>> digging into this one please to confirm. In particular if there's been a change
>> how do we make sure we don't impact older platforms etc.
> 
> Regarding "GMKS" method, it does not have "version" related information. So , its unlikely to impact any older platforms.
> However, I got it confirmed that definition of GMKS method itself doesn't include any workaround feature. 
> 
> But, since its getter function , I also think its harmless and if it workaround some issue then I don’t see any concern.

Ok, I'll merge this patch then.

Regards,

Hans


^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] Allow the FnLock LED to change state
  2021-03-15 19:58 [PATCH] Allow the FnLock LED to change state Esteve Varela Colominas
  2021-03-18 16:49 ` [PATCH] thinkpad_acpi: " Hans de Goede
@ 2021-03-21 16:38 ` Hans de Goede
  2021-03-21 17:15   ` Esteve Varela
  1 sibling, 1 reply; 12+ messages in thread
From: Hans de Goede @ 2021-03-21 16:38 UTC (permalink / raw)
  To: Esteve Varela Colominas, Henrique de Moraes Holschuh
  Cc: ibm-acpi-devel, platform-driver-x86

Hi,

On 3/15/21 8:58 PM, Esteve Varela Colominas wrote:
> On many recent ThinkPad laptops, there's a new LED next to the ESC key,
> that indicates the FnLock status.
> When the Fn+ESC combo is pressed, FnLock is toggled, which causes the
> Media Key functionality to change, making it so that the media keys
> either perform their media key function, or function as an F-key by
> default. The Fn key can be used the access the alternate function at any
> time.
> 
> With the current linux kernel, the LED doens't change state if you press
> the Fn+ESC key combo. However, the media key functionality *does*
> change. This is annoying, since the LED will stay on if it was on during
> bootup, and it makes it hard to keep track what the current state of the
> FnLock is.
> 
> This patch calls an ACPI function, that gets the current media key
> state, when the Fn+ESC key combo is pressed. Through testing it was
> discovered that this function causes the LED to update correctly to
> reflect the current state when this function is called.
> 
> The relevant ACPI calls are the following:
> \_SB_.PCI0.LPC0.EC0_.HKEY.GMKS: Get media key state, returns 0x603 if the FnLock mode is enabled, and 0x602 if it's disabled.
> \_SB_.PCI0.LPC0.EC0_.HKEY.SMKS: Set media key state, sending a 1 will enable FnLock mode, and a 0 will disable it.
> 
> Relevant discussion:
> https://bugzilla.kernel.org/show_bug.cgi?id=207841
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1881015
> 
> Signed-off-by: Esteve Varela Colominas <esteve.varela@gmail.com>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

And I'll also add it to the fixes branch so that it gets included in
one of the future 5.12-rc releases.

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans


> ---
>  drivers/platform/x86/thinkpad_acpi.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index c40470637..09362dd74 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -4079,13 +4079,19 @@ static bool hotkey_notify_6xxx(const u32 hkey,
>  
>  	case TP_HKEY_EV_KEY_NUMLOCK:
>  	case TP_HKEY_EV_KEY_FN:
> -	case TP_HKEY_EV_KEY_FN_ESC:
>  		/* key press events, we just ignore them as long as the EC
>  		 * is still reporting them in the normal keyboard stream */
>  		*send_acpi_ev = false;
>  		*ignore_acpi_ev = true;
>  		return true;
>  
> +	case TP_HKEY_EV_KEY_FN_ESC:
> +		/* Get the media key status to foce the status LED to update */
> +		acpi_evalf(hkey_handle, NULL, "GMKS", "v");
> +		*send_acpi_ev = false;
> +		*ignore_acpi_ev = true;
> +		return true;
> +
>  	case TP_HKEY_EV_TABLET_CHANGED:
>  		tpacpi_input_send_tabletsw();
>  		hotkey_tablet_mode_notify_change();
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] Allow the FnLock LED to change state
  2021-03-21 16:38 ` [PATCH] " Hans de Goede
@ 2021-03-21 17:15   ` Esteve Varela
  2021-03-21 17:23     ` Hans de Goede
  0 siblings, 1 reply; 12+ messages in thread
From: Esteve Varela @ 2021-03-21 17:15 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Henrique de Moraes Holschuh, ibm-acpi-devel, platform-driver-x86

2021-03-21 16:38 GMT, Hans de Goede <hdegoede@redhat.com>:
> Hi,
>
> Thank you for your patch, I've applied this patch to my review-hans
> branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
>
> And I'll also add it to the fixes branch so that it gets included in
> one of the future 5.12-rc releases.
>
> Note it will show up in my review-hans branch once I've pushed my
> local branch there, which might take a while.
>
> Once I've run some tests on this branch the patches there will be
> added to the platform-drivers-x86/for-next branch and eventually
> will be included in the pdx86 pull-request to Linus for the next
> merge-window.
>
> Regards,
>
> Hans

Thanks a ton! Especially for making sure there's no hardware
compatibility issues, since I was afraid that might've been an issue.
I just noticed that there's a typo in a comment, "foce" that should be
"force". Maybe that can be amended before the branch is merged
elsewhere.

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] Allow the FnLock LED to change state
  2021-03-21 17:15   ` Esteve Varela
@ 2021-03-21 17:23     ` Hans de Goede
  2021-03-21 18:35       ` [PATCH] platform/x86: thinkpad_acpi: Correct minor typo Esteve Varela Colominas
  0 siblings, 1 reply; 12+ messages in thread
From: Hans de Goede @ 2021-03-21 17:23 UTC (permalink / raw)
  To: Esteve Varela
  Cc: Henrique de Moraes Holschuh, ibm-acpi-devel, platform-driver-x86

Hi,

On 3/21/21 6:15 PM, Esteve Varela wrote:
> 2021-03-21 16:38 GMT, Hans de Goede <hdegoede@redhat.com>:
>> Hi,
>>
>> Thank you for your patch, I've applied this patch to my review-hans
>> branch:
>> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
>>
>> And I'll also add it to the fixes branch so that it gets included in
>> one of the future 5.12-rc releases.
>>
>> Note it will show up in my review-hans branch once I've pushed my
>> local branch there, which might take a while.
>>
>> Once I've run some tests on this branch the patches there will be
>> added to the platform-drivers-x86/for-next branch and eventually
>> will be included in the pdx86 pull-request to Linus for the next
>> merge-window.
>>
>> Regards,
>>
>> Hans
> 
> Thanks a ton! Especially for making sure there's no hardware
> compatibility issues, since I was afraid that might've been an issue.
> I just noticed that there's a typo in a comment, "foce" that should be
> "force". Maybe that can be amended before the branch is merged
> elsewhere.

I've already pushed this to the public pdx86 git repo for-next and fixes
branches, so too late to amend.

If you can send a followup-patch fixing the type, then I'll add that to
for-next.

Regards,

Hans


^ permalink raw reply	[flat|nested] 12+ messages in thread
* [PATCH] platform/x86: thinkpad_acpi: Correct minor typo
  2021-03-21 17:23     ` Hans de Goede
@ 2021-03-21 18:35       ` Esteve Varela Colominas
  2021-03-23 20:10         ` Hans de Goede
  0 siblings, 1 reply; 12+ messages in thread
From: Esteve Varela Colominas @ 2021-03-21 18:35 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Esteve Varela Colominas, ibm-acpi-devel, platform-driver-x86

Signed-off-by: Esteve Varela Colominas <esteve.varela@gmail.com>
---
 drivers/platform/x86/thinkpad_acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 09362dd74..f3f7ae6f6 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -4086,7 +4086,7 @@ static bool hotkey_notify_6xxx(const u32 hkey,
 		return true;
 
 	case TP_HKEY_EV_KEY_FN_ESC:
-		/* Get the media key status to foce the status LED to update */
+		/* Get the media key status to force the status LED to update */
 		acpi_evalf(hkey_handle, NULL, "GMKS", "v");
 		*send_acpi_ev = false;
 		*ignore_acpi_ev = true;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread
* RE: [External] Re: [PATCH] thinkpad_acpi: Allow the FnLock LED to change state
  2021-03-19 11:35       ` Enrico Weigelt, metux IT consult
@ 2021-03-22  0:53         ` Nitin Joshi1
  0 siblings, 0 replies; 12+ messages in thread
From: Nitin Joshi1 @ 2021-03-22  0:53 UTC (permalink / raw)
  To: Enrico Weigelt, metux IT consult, Mark RH Pearson, Hans de Goede,
	Esteve Varela Colominas, Henrique de Moraes Holschuh
  Cc: ibm-acpi-devel, platform-driver-x86

Hello Enrico,

>-----Original Message-----
>From: Enrico Weigelt, metux IT consult <info@metux.net>
>Sent: Friday, March 19, 2021 8:36 PM
>To: Nitin Joshi1 <njoshi1@lenovo.com>; Mark RH Pearson
><markpearson@lenovo.com>; Hans de Goede <hdegoede@redhat.com>;
>Esteve Varela Colominas <esteve.varela@gmail.com>; Henrique de Moraes
>Holschuh <ibm-acpi@hmh.eng.br>
>Cc: ibm-acpi-devel@lists.sourceforge.net; platform-driver-
>x86@vger.kernel.org
>Subject: Re: [External] Re: [PATCH] thinkpad_acpi: Allow the FnLock LED to
>change state
>
>On 19.03.21 11:39, Nitin Joshi1 wrote:
>
>Hi,
>
>> Regarding "GMKS" method, it does not have "version" related information.
>So , its unlikely to impact any older platforms.
>> However, I got it confirmed that definition of GMKS method itself doesn't
>include any workaround feature.
>>
>> But, since its getter function , I also think its harmless and if it workaround
>some issue then I don’t see any concern.
>
>How about publishing schematics / specs, so we can confirm what it's really
>doing ?
Thank you for your comment.
I will check regarding this and feedback you later.

>
>In recent decades I've learn to mistrust vendor-provided firmware (especially
>when it comes to acpi). The reason why I'm currently bisecting AMD's AGESA
>blob, in order to completely replace it.
>
>It's a tedious job, had to write my own analysis tool, but step by step making
>progress. (and already learned they've been using a pretty inefficient compiler
>that can't even inline trivial memcpy().
>Probably some older msvc ... :p)
>
>--mtx

Thanks & Regards,
Nitin Joshi 
>
>--
>---
>Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
>werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
>GPG/PGP-Schlüssel zu.
>---
>Enrico Weigelt, metux IT consult
>Free software and Linux embedded engineering info@metux.net -- +49-151-
>27565287

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] platform/x86: thinkpad_acpi: Correct minor typo
  2021-03-21 18:35       ` [PATCH] platform/x86: thinkpad_acpi: Correct minor typo Esteve Varela Colominas
@ 2021-03-23 20:10         ` Hans de Goede
  0 siblings, 0 replies; 12+ messages in thread
From: Hans de Goede @ 2021-03-23 20:10 UTC (permalink / raw)
  To: Esteve Varela Colominas; +Cc: ibm-acpi-devel, platform-driver-x86

Hi,

On 3/21/21 7:35 PM, Esteve Varela Colominas wrote:
> Signed-off-by: Esteve Varela Colominas <esteve.varela@gmail.com>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans


> ---
>  drivers/platform/x86/thinkpad_acpi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 09362dd74..f3f7ae6f6 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -4086,7 +4086,7 @@ static bool hotkey_notify_6xxx(const u32 hkey,
>  		return true;
>  
>  	case TP_HKEY_EV_KEY_FN_ESC:
> -		/* Get the media key status to foce the status LED to update */
> +		/* Get the media key status to force the status LED to update */
>  		acpi_evalf(hkey_handle, NULL, "GMKS", "v");
>  		*send_acpi_ev = false;
>  		*ignore_acpi_ev = true;
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-03-23 20:11 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-15 19:58 [PATCH] Allow the FnLock LED to change state Esteve Varela Colominas
2021-03-18 16:49 ` [PATCH] thinkpad_acpi: " Hans de Goede
2021-03-18 20:13   ` [External] " Mark Pearson
2021-03-19 10:39     ` Nitin Joshi1
2021-03-19 11:35       ` Enrico Weigelt, metux IT consult
2021-03-22  0:53         ` Nitin Joshi1
2021-03-21 16:37       ` Hans de Goede
2021-03-21 16:38 ` [PATCH] " Hans de Goede
2021-03-21 17:15   ` Esteve Varela
2021-03-21 17:23     ` Hans de Goede
2021-03-21 18:35       ` [PATCH] platform/x86: thinkpad_acpi: Correct minor typo Esteve Varela Colominas
2021-03-23 20:10         ` Hans de Goede

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).