| Message ID | 20240421154520.37089-9-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | platform/x86: thinkpad_acpi: Refactor hotkey handling and add support for some new hotkeys | expand |
On Sun, 21 Apr 2024, 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@redhat.com> > --- > 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) Please try to keep call and it's error handling together, it costs one line but takes less effort to understand: int mode; 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); > +}
On Mon, Apr 22, 2024 at 11:29 AM Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > On Sun, 21 Apr 2024, Hans de Goede wrote: ... > > + int mode = adaptive_keyboard_get_mode(); > > + > > + if (mode < 0) > > Please try to keep call and it's error handling together, it costs one > line but takes less effort to understand: > > int mode; > > mode = adaptive_keyboard_get_mode(); > if (mode < 0) > > > + return; And not only that. In long-term maintenance the original code is prone to subtle errors in case some other code is squeezed in between.
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@redhat.com> > --- > 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? > + 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
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@redhat.com> >> --- >> 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 >
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@redhat.com> >>> --- >>> 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@squebb.ca> 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
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@redhat.com> >>>> --- >>>> 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@squebb.ca> > > 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
Hi, On 4/22/24 10:29 AM, Ilpo Järvinen wrote: > On Sun, 21 Apr 2024, 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@redhat.com> >> --- >> 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) > > Please try to keep call and it's error handling together, it costs one > line but takes less effort to understand: > > int mode; > > mode = adaptive_keyboard_get_mode(); > if (mode < 0) Ack, I've changed this for v2 following your suggestion. Regards, Hans > >> + return; >> + >> + adaptive_keyboard_prev_mode = mode; >> + adaptive_keyboard_mode_is_saved = true; >> + >> + adaptive_keyboard_set_mode(FUNCTION_MODE); >> +} >
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(); + return true; case TP_HKEY_EV_THM_CSM_COMPLETED: lapsensor_refresh(); /* If we are already accessing DYTC then skip dytc update */
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@redhat.com> --- drivers/platform/x86/thinkpad_acpi.c | 85 +++++++++++++++------------- 1 file changed, 45 insertions(+), 40 deletions(-)