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

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

Hi Mark,

On 4/23/24 2:15 PM, Mark Pearson wrote:
> Hi Hans
> 
> On Tue, Apr 23, 2024, at 4:35 AM, Hans de Goede wrote:
>> Hi Mark,
>>
>> On 4/22/24 9:27 PM, Mark Pearson wrote:
>>> Hi Hans,
>>>
>>> On Sun, Apr 21, 2024, at 11:45 AM, 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)
>>>> +		return;
>>>> +
>>>> +	adaptive_keyboard_prev_mode = mode;
>>>> +	adaptive_keyboard_mode_is_saved = true;
>>>> +
>>>> +	adaptive_keyboard_set_mode(FUNCTION_MODE);
>>>> +}
>>>> +
>>>>  static bool adaptive_keyboard_hotkey_notify_hotkey(const u32 hkey)
>>>>  {
>>>> -	int current_mode = 0;
>>>> -	int new_mode = 0;
>>>> -
>>>> -	switch (hkey) {
>>>> -	case TP_HKEY_EV_DFR_CHANGE_ROW:
>>>> -		if (adaptive_keyboard_mode_is_saved) {
>>>> -			new_mode = adaptive_keyboard_prev_mode;
>>>> -			adaptive_keyboard_mode_is_saved = false;
>>>> -		} else {
>>>> -			current_mode = adaptive_keyboard_get_mode();
>>>> -			if (current_mode < 0)
>>>> -				return false;
>>>> -			new_mode = adaptive_keyboard_get_next_mode(
>>>> -					current_mode);
>>>> -		}
>>>> -
>>>> -		if (adaptive_keyboard_set_mode(new_mode) < 0)
>>>> -			return false;
>>>> -
>>>> +	if (tpacpi_driver_event(hkey))
>>>>  		return true;
>>>>
>>>> -	case TP_HKEY_EV_DFR_S_QUICKVIEW_ROW:
>>>> -		current_mode = adaptive_keyboard_get_mode();
>>>> -		if (current_mode < 0)
>>>> -			return false;
>>>> -
>>>> -		adaptive_keyboard_prev_mode = current_mode;
>>>> -		adaptive_keyboard_mode_is_saved = true;
>>>> -
>>>> -		if (adaptive_keyboard_set_mode (FUNCTION_MODE) < 0)
>>>> -			return false;
>>>> -		return true;
>>>> -
>>>> -	default:
>>>> -		if (hkey < TP_HKEY_EV_ADAPTIVE_KEY_START ||
>>>> -		    hkey > TP_HKEY_EV_ADAPTIVE_KEY_END) {
>>>> -			pr_info("Unhandled adaptive keyboard key: 0x%x\n", hkey);
>>>> -			return false;
>>>> -		}
>>>> -		tpacpi_input_send_key(hkey - TP_HKEY_EV_ADAPTIVE_KEY_START +
>>>> -				      TP_ACPI_HOTKEYSCAN_ADAPTIVE_START);
>>>> -		return true;
>>>> +	if (hkey < TP_HKEY_EV_ADAPTIVE_KEY_START ||
>>>> +	    hkey > TP_HKEY_EV_ADAPTIVE_KEY_END) {
>>>> +		pr_info("Unhandled adaptive keyboard key: 0x%x\n", hkey);
>>>> +		return false;
>>>>  	}
>>>> +
>>>> +	tpacpi_input_send_key(hkey - TP_HKEY_EV_ADAPTIVE_KEY_START +
>>>> +			      TP_ACPI_HOTKEYSCAN_ADAPTIVE_START);
>>>> +	return true;
>>>>  }
>>>>
>>>>  static bool hotkey_notify_extended_hotkey(const u32 hkey)
>>>> @@ -11117,6 +11116,12 @@ static bool tpacpi_driver_event(const unsigned 
>>>> int hkey_event)
>>>>  		}
>>>>  		/* Key events are suppressed by default hotkey_user_mask */
>>>>  		return false;
>>>> +	case TP_HKEY_EV_DFR_CHANGE_ROW:
>>>> +		adaptive_keyboard_change_row();
>>>> +		return true;
>>>> +	case TP_HKEY_EV_DFR_S_QUICKVIEW_ROW:
>>>> +		adaptive_keyboard_s_quickview_row();
>>>
>>> Was there a reason to get rid of error handling that was previously done with adaptive_keyboard_set_mode and is now not used here?
>>
>> The error handling consisted of returning false instead of true
>> (for known_ev), causing an unknown event message to get logged on
>> top of the error already logged by adaptive_keyboard_get_mode() /
>> adaptive_keyboard_set_mode().
>>
>> This second unknown event error is consfusin / not helpful so
>> I've dropped the "return false" on error behavior, note that
>> the new helpers do still return early if get_mode() fails just
>> as before.
>>
>> I'll add a note about this to the commit message for v2 of
>> the series.
>>
> Thanks for the explanation - makes sense.
> 
> Reviwed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> 
> As a note - I've been working my way thru the patches. Is it OK to send one Reviewed-by at the end for all the patches for which I had no questions, or is it better to ack each one individually?

One Reviewed-by for the series when you're done (with possible
exception for some patches you have outstanding remarks for)
is fine.

Regards,

Hans