Re: [PATCH 08/24] platform/x86: thinkpad_acpi: Move adaptive kbd event handling to tpacpi_driver_event()

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 4/22/24 10:29 AM, Ilpo Järvinen wrote:
> On Sun, 21 Apr 2024, Hans de Goede wrote:
> 
>> Factor out the adaptive kbd non hotkey event handling into
>> adaptive_keyboard_change_row() and adaptive_keyboard_s_quickview_row()
>> helpers and move the handling of TP_HKEY_EV_DFR_CHANGE_ROW and
>> TP_HKEY_EV_DFR_S_QUICKVIEW_ROW to tpacpi_driver_event().
>>
>> This groups all the handling of hotkey events which do not emit
>> a key press event together in tpacpi_driver_event().
>>
>> This is a preparation patch for moving to sparse-keymaps.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/platform/x86/thinkpad_acpi.c | 85 +++++++++++++++-------------
>>  1 file changed, 45 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>> index 0bbc462d604c..e8d30f4af126 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -3677,51 +3677,50 @@ static int adaptive_keyboard_get_next_mode(int mode)
>>  	return adaptive_keyboard_modes[i];
>>  }
>>  
>> +static void adaptive_keyboard_change_row(void)
>> +{
>> +	int mode;
>> +
>> +	if (adaptive_keyboard_mode_is_saved) {
>> +		mode = adaptive_keyboard_prev_mode;
>> +		adaptive_keyboard_mode_is_saved = false;
>> +	} else {
>> +		mode = adaptive_keyboard_get_mode();
>> +		if (mode < 0)
>> +			return;
>> +		mode = adaptive_keyboard_get_next_mode(mode);
>> +	}
>> +
>> +	adaptive_keyboard_set_mode(mode);
>> +}
>> +
>> +static void adaptive_keyboard_s_quickview_row(void)
>> +{
>> +	int mode = adaptive_keyboard_get_mode();
>> +
>> +	if (mode < 0)
> 
> Please try to keep call and it's error handling together, it costs one 
> line but takes less effort to understand:
> 
> 	int mode;
> 
> 	mode = adaptive_keyboard_get_mode();
> 	if (mode < 0)

Ack, I've changed this for v2 following your suggestion.

Regards,

Hans




> 
>> +		return;
>> +
>> +	adaptive_keyboard_prev_mode = mode;
>> +	adaptive_keyboard_mode_is_saved = true;
>> +
>> +	adaptive_keyboard_set_mode(FUNCTION_MODE);
>> +}
>