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
