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
>
