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

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

 



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@xxxxxxxxxx>
>>> ---
>>>  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@xxxxxxxxx>

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?

Mark




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux