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

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

 



Hi Ilpo,

Thank you for reviewing this series.

On 4/25/24 11:14 AM, Ilpo Järvinen wrote:
> 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.

Right, this is basically the same question as which Mark asked. I've added
the following to the commit message while merging this series to clarify this:

"""
Note 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. Therefor this patch does not set any bits in hotkey_reserved_mask
for the KEY_RESERVED mappings for the adaptive keyboard scancodes.
"""

> 
>> Tested-by: Mark Pearson <mpearson-lenovo@xxxxxxxxx>
>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>> ---
>>  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?

This makes the code style identical to how the brightness keys are added
to the mask above. I guess this could have been in a separate patch, but
since it is less work for me to just leave it here I'm going to leave it here.

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