Re: [PATCH v2 16/24] platform/x86: thinkpad_acpi: Change hotkey_reserved_mask initialization

["Mark Pearson" <mpearson-lenovo@squebb.ca>]

On Wed, Apr 24, 2024, at 10:47 AM, Hans de Goede wrote:
> Hi Mark,
>
> On 4/24/24 4:17 PM, Mark Pearson wrote:
>> Hi Hans,
>> 
>> On Wed, Apr 24, 2024, at 8:28 AM, Hans de Goede wrote:
>>> Change the hotkey_reserved_mask initialization to hardcode the list
>>> of reserved keys. There are only a few reserved keys and the code to
>>> iterate over the keymap will be removed when moving to sparse-keymaps.
>>>
>>> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>  drivers/platform/x86/thinkpad_acpi.c | 21 +++++++++++++++------
>>>  1 file changed, 15 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/thinkpad_acpi.c 
>>> b/drivers/platform/x86/thinkpad_acpi.c
>>> index 952bac635a18..cf5c741d1343 100644
>>> --- a/drivers/platform/x86/thinkpad_acpi.c
>>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>>> @@ -3545,6 +3545,19 @@ static int __init hotkey_init(struct 
>>> ibm_init_struct *iibm)
>>>  	dbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_HKEY,
>>>  		   "using keymap number %lu\n", keymap_id);
>>>
>>> +	/* Keys which should be reserved on both IBM and Lenovo models */
>>> +	hotkey_reserved_mask = TP_ACPI_HKEY_KBD_LIGHT_MASK |
>>> +			       TP_ACPI_HKEY_VOLUP_MASK |
>>> +			       TP_ACPI_HKEY_VOLDWN_MASK |
>>> +			       TP_ACPI_HKEY_MUTE_MASK;
>>> +	/*
>>> +	 * Reserve brightness up/down unconditionally on IBM models, on Lenovo
>>> +	 * models these are disabled based on acpi_video_get_backlight_type().
>>> +	 */
>>> +	if (keymap_id == TPACPI_KEYMAP_IBM_GENERIC)
>>> +		hotkey_reserved_mask |= TP_ACPI_HKEY_BRGHTUP_MASK |
>>> +					TP_ACPI_HKEY_BRGHTDWN_MASK;
>>> +
>>>  	hotkey_keycode_map = kmemdup(&tpacpi_keymaps[keymap_id],
>>>  			TPACPI_HOTKEY_MAP_SIZE,	GFP_KERNEL);
>>>  	if (!hotkey_keycode_map) {
>>> @@ -3560,9 +3573,6 @@ static int __init hotkey_init(struct 
>>> ibm_init_struct *iibm)
>>>  		if (hotkey_keycode_map[i] != KEY_RESERVED) {
>>>  			input_set_capability(tpacpi_inputdev, EV_KEY,
>>>  						hotkey_keycode_map[i]);
>>> -		} else {
>>> -			if (i < sizeof(hotkey_reserved_mask)*8)
>>> -				hotkey_reserved_mask |= 1 << i;
>> 
>> Just to check my understanding here - does this change mean that the keys marked as KEY_RESERVED in the lenovo map won't make it into the mask?
>> 
>> e.g the ones in this block:
>>                 KEY_RESERVED,        /* Mute held, 0x103 */
>>                 KEY_BRIGHTNESS_MIN,  /* Backlight off */
>>                 KEY_RESERVED,        /* Clipping tool */
>>                 KEY_RESERVED,        /* Cloud */
>>                 KEY_RESERVED,
>>                 KEY_VOICECOMMAND,    /* Voice */
>>                 KEY_RESERVED,
>>                 KEY_RESERVED,        /* Gestures */
>>                 KEY_RESERVED,
>>                 KEY_RESERVED,
>>                 KEY_RESERVED,
>>                 KEY_CONFIG,          /* Settings */
>>                 KEY_RESERVED,        /* New tab */
>>                 KEY_REFRESH,         /* Reload */
>>                 KEY_BACK,            /* Back */
>>                 KEY_RESERVED,        /* Microphone down */
>>                 KEY_RESERVED,        /* Microphone up */
>>                 KEY_RESERVED,        /* Microphone cancellation */
>>                 KEY_RESERVED,        /* Camera mode */
>>                 KEY_RESERVED,        /* Rotate display, 0x116 */
>> 
>> I'm not sure what the effect will be and I don't have the 2014 X1 Carbon (I assume it's the G1) available to test with unfortunately.
>
> Only the 32 original hotkeys are affected by any of the hotkey_*_mask
> values, note:
>
> 			if (i < sizeof(hotkey_reserved_mask)*8)
> 				hotkey_reserved_mask |= 1 << i;
>
> The (i < sizeof(hotkey_reserved_mask)*8) condition translates to
> (i < 32) so this code only ever set bits in hotkey_reserved_mask
> for the 32 original hotkeys.
>
Ah - excellent. I had missed that.
Thanks!

Reviewed-by Mark Pearson <mpearson-lenovo@squebb.ca>
(and that applies for all patches in the series up to this one...getting through them slowly)

Mark