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 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.

Regards,

Hans






> 
>> +		return true;
>>  	case TP_HKEY_EV_THM_CSM_COMPLETED:
>>  		lapsensor_refresh();
>>  		/* If we are already accessing DYTC then skip dytc update */
>> -- 
>> 2.44.0
> 
> Thanks
> 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