| Message ID | 20240421154520.37089-4-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: > send_acpi_ev, ignore_acpi_ev are already initialized to true resp. false by Wording here is odd (but I'm not native so could be I just don't understand what "true resp. false" is supposed to mean/fit into the general structure of this sentence). I could nonetheless guess the general meaning of the sentence despite that, but you might want to consider rewording it into something that is easier to understand. The code change is fine, Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
On Mon, Apr 22, 2024 at 11:07 AM Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > On Sun, 21 Apr 2024, Hans de Goede wrote: > > > send_acpi_ev, ignore_acpi_ev are already initialized to true resp. false by > > Wording here is odd (but I'm not native so could be I just don't > understand what "true resp. false" is supposed to mean/fit into the > general structure of this sentence). I could nonetheless guess the > general meaning of the sentence despite that, but you might want to > consider rewording it into something that is easier to understand. I read that as "true and false respectively", which is probably better to be spelled this way.
On Mon, 22 Apr 2024, Andy Shevchenko wrote: > On Mon, Apr 22, 2024 at 11:07 AM Ilpo Järvinen > <ilpo.jarvinen@linux.intel.com> wrote: > > On Sun, 21 Apr 2024, Hans de Goede wrote: > > > > > send_acpi_ev, ignore_acpi_ev are already initialized to true resp. false by > > > > Wording here is odd (but I'm not native so could be I just don't > > understand what "true resp. false" is supposed to mean/fit into the > > general structure of this sentence). I could nonetheless guess the > > general meaning of the sentence despite that, but you might want to > > consider rewording it into something that is easier to understand. > > I read that as "true and false respectively", which is probably better That's what I expected to see (although with a comma before respectively). Looking a bit more into it, it seems the placement in the middle is probably an artifact from another language (claimed to not be correct English): https://en.wiktionary.org/wiki/resp. > to be spelled this way.
Hi, On 4/22/24 10:07 AM, Ilpo Järvinen wrote: > On Sun, 21 Apr 2024, Hans de Goede wrote: > >> send_acpi_ev, ignore_acpi_ev are already initialized to true resp. false by > > Wording here is odd (but I'm not native so could be I just don't > understand what "true resp. false" is supposed to mean/fit into the > general structure of this sentence). I could nonetheless guess the > general meaning of the sentence despite that, but you might want to > consider rewording it into something that is easier to understand. Ok, I have changed this to: """ send_acpi_ev and ignore_acpi_ev are already initialized to true and false respectively by hotkey_notify() before calling the various helpers. Drop the needless re-initialization from the helpers. """ now. > > The code change is fine, > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Thank you. Regards, Hans
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index fc5681808c3b..007223fded30 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -3754,14 +3754,12 @@ static bool hotkey_notify_extended_hotkey(const u32 hkey) return false; } +/* 0x1000-0x1FFF: key presses */ static bool hotkey_notify_hotkey(const u32 hkey, bool *send_acpi_ev, bool *ignore_acpi_ev) { - /* 0x1000-0x1FFF: key presses */ unsigned int scancode = hkey & 0xfff; - *send_acpi_ev = true; - *ignore_acpi_ev = false; /* * Original events are in the 0x10XX range, the adaptive keyboard @@ -3794,14 +3792,11 @@ static bool hotkey_notify_hotkey(const u32 hkey, return false; } +/* 0x2000-0x2FFF: Wakeup reason */ static bool hotkey_notify_wakeup(const u32 hkey, bool *send_acpi_ev, bool *ignore_acpi_ev) { - /* 0x2000-0x2FFF: Wakeup reason */ - *send_acpi_ev = true; - *ignore_acpi_ev = false; - switch (hkey) { case TP_HKEY_EV_WKUP_S3_UNDOCK: /* suspend, undock */ case TP_HKEY_EV_WKUP_S4_UNDOCK: /* hibernation, undock */ @@ -3834,14 +3829,11 @@ static bool hotkey_notify_wakeup(const u32 hkey, return true; } +/* 0x4000-0x4FFF: dock-related events */ static bool hotkey_notify_dockevent(const u32 hkey, bool *send_acpi_ev, bool *ignore_acpi_ev) { - /* 0x4000-0x4FFF: dock-related events */ - *send_acpi_ev = true; - *ignore_acpi_ev = false; - switch (hkey) { case TP_HKEY_EV_UNDOCK_ACK: /* ACPI undock operation completed after wakeup */ @@ -3879,14 +3871,11 @@ static bool hotkey_notify_dockevent(const u32 hkey, } } +/* 0x5000-0x5FFF: human interface helpers */ static bool hotkey_notify_usrevent(const u32 hkey, bool *send_acpi_ev, bool *ignore_acpi_ev) { - /* 0x5000-0x5FFF: human interface helpers */ - *send_acpi_ev = true; - *ignore_acpi_ev = false; - switch (hkey) { case TP_HKEY_EV_PEN_INSERTED: /* X61t: tablet pen inserted into bay */ case TP_HKEY_EV_PEN_REMOVED: /* X61t: tablet pen removed from bay */ @@ -3914,14 +3903,11 @@ static bool hotkey_notify_usrevent(const u32 hkey, static void thermal_dump_all_sensors(void); static void palmsensor_refresh(void); +/* 0x6000-0x6FFF: thermal alarms/notices and keyboard events */ static bool hotkey_notify_6xxx(const u32 hkey, bool *send_acpi_ev, bool *ignore_acpi_ev) { - /* 0x6000-0x6FFF: thermal alarms/notices and keyboard events */ - *send_acpi_ev = true; - *ignore_acpi_ev = false; - switch (hkey) { case TP_HKEY_EV_THM_TABLE_CHANGED: pr_debug("EC reports: Thermal Table has changed\n");
send_acpi_ev, ignore_acpi_ev are already initialized to true resp. false by hotkey_notify() before calling the various helpers. Drop the needless re-initialization from the helpers. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/platform/x86/thinkpad_acpi.c | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-)