Re: [External] Re: [PATCH] thinkpad_acpi: Allow the FnLock LED to change state

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 3/19/21 11:39 AM, Nitin Joshi1 wrote:
> Hello,
> 
>> -----Original Message-----
>> From: Mark RH Pearson <markpearson@xxxxxxxxxx>
>> Sent: Friday, March 19, 2021 5:13 AM
>> To: Hans de Goede <hdegoede@xxxxxxxxxx>; Esteve Varela Colominas
>> <esteve.varela@xxxxxxxxx>; Henrique de Moraes Holschuh <ibm-
>> acpi@xxxxxxxxxx>; Nitin Joshi1 <njoshi1@xxxxxxxxxx>
>> Cc: ibm-acpi-devel@xxxxxxxxxxxxxxxxxxxxx; platform-driver-
>> x86@xxxxxxxxxxxxxxx
>> 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@xxxxxxxxx>
>>>> ---
>>>>  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




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux