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@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?
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
