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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Wed, 24 Apr 2024, 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.

Hi,

Consider extending this explanation. It's hard to see how the old and new 
code are identical because there are more KEY_RESERVED entries in the 
array than in the new code. I can see the list of keys in the new code 
matches to those set using tpacpi_hotkey_driver_mask_set() but it's hard 
to connect the dots here.

> 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;
>  		}
>  	}
>  
> @@ -3587,9 +3597,8 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
>  		/* Disable brightness up/down on Lenovo thinkpads when
>  		 * ACPI is handling them, otherwise it is plain impossible
>  		 * for userspace to do something even remotely sane */
> -		hotkey_reserved_mask |=
> -			(1 << TP_ACPI_HOTKEYSCAN_FNHOME)
> -			| (1 << TP_ACPI_HOTKEYSCAN_FNEND);
> +		hotkey_reserved_mask |= TP_ACPI_HKEY_BRGHTUP_MASK |
> +					TP_ACPI_HKEY_BRGHTDWN_MASK;

This is a simple define replace that would belong to own patch?


-- 
 i.