| Message ID | 20201020001556.388099-3-markpearson@lenovo.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | [v2,1/3] Input: add event codes for lap and palmreset proximity switches | expand |
Hi, On 10/20/20 2:15 AM, Mark Pearson wrote: > Use input device event support for notifying userspace of lap mode sensor > state changes. > > Signed-off-by: Mark Pearson <markpearson@lenovo.com> > --- > drivers/platform/x86/thinkpad_acpi.c | 75 ++++++++++++++++++++++------ > 1 file changed, 59 insertions(+), 16 deletions(-) > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > index 5ddf2775fb06..c20b9902270b 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -4013,7 +4013,7 @@ static bool hotkey_notify_usrevent(const u32 hkey, > } > > static void thermal_dump_all_sensors(void); > -static void proxsensor_refresh(void); > +static void proxsensor_refresh(bool palm, bool lap); > > static bool hotkey_notify_6xxx(const u32 hkey, > bool *send_acpi_ev, > @@ -4081,7 +4081,7 @@ static bool hotkey_notify_6xxx(const u32 hkey, > case TP_HKEY_EV_PALM_DETECTED: > case TP_HKEY_EV_PALM_UNDETECTED: > /* palm detected - pass on to event handler */ > - proxsensor_refresh(); > + proxsensor_refresh(true /* palm */, false /* lap */); > return true; > > default: > @@ -9929,6 +9929,23 @@ static struct ibm_struct dytc_driver_data = { > struct input_dev *tpacpi_sw_dev; > bool has_palmsensor; > bool palmsensor_state; > +bool has_lapsensor; > +bool lapsensor_state; Again, drop the global _state caching, it is not necessary. > + > +static int lapsensor_get(bool *present, bool *state) > +{ > + acpi_handle dytc_handle; > + int output; > + > + *present = false; > + if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) > + return -ENODEV; > + if (!acpi_evalf(dytc_handle, &output, NULL, "dd", DYTC_CMD_GET)) > + return -EIO; > + *present = true; /*If we get his far, we have lapmode support*/ > + *state = output & BIT(DYTC_GET_LAPMODE_BIT) ? true : false; > + return 0; > +} > > static int palmsensor_get(bool *present, bool *state) > { > @@ -9945,36 +9962,56 @@ static int palmsensor_get(bool *present, bool *state) > return 0; > } > > -static void proxsensor_refresh(void) > +static void proxsensor_refresh(bool palm, bool lap) There is zero shared code between the palm ==true and the lap ==true paths, please just make this 2 separate functions. And then I guess rename the original proxsensor_refresh to palmsensor_refresh (note please do this in the 2/3 patch) and add a new lapsensor_refresh > { > bool new_state; > int err; > > - if (has_palmsensor) { > + if (palm && has_palmsensor) { > err = palmsensor_get(&has_palmsensor, &new_state); > - if (err) > - return; And then you can also keep the if (err) return; construct, which is a bit cleaner (more common used) solution vs the !err way of handling errors. > - if (new_state != palmsensor_state) { > + if (!err && (new_state != palmsensor_state)) { > input_report_switch(tpacpi_sw_dev, SW_PALMREST_PROXIMITY, new_state); > input_sync(tpacpi_sw_dev); > palmsensor_state = new_state; > } > } > + > + if (lap && has_lapsensor) { > + err = lapsensor_get(&has_lapsensor, &new_state); > + if (!err && (new_state != lapsensor_state)) { > + input_report_switch(tpacpi_sw_dev, SW_LAP_PROXIMITY, new_state); > + input_sync(tpacpi_sw_dev); > + lapsensor_state = new_state; Same as with the other patch there is no need for the new_state != lapsensor_state check, the input core does this for you turning reporting the same state twice into a no-op. > + } > + } > } > > static int tpacpi_proxsensor_init(struct ibm_init_struct *iibm) > { > - int palm_err; > + int palm_err, lap_err, err; > > + /* Make sure globals are set to a sensible initial value */ > + has_palmsensor = false; > + has_lapsensor = false; > palm_err = palmsensor_get(&has_palmsensor, &palmsensor_state); > + lap_err = lapsensor_get(&has_lapsensor, &lapsensor_state); > + > /* If support isn't available (ENODEV) then don't return an error */ > - if (palm_err == -ENODEV) > + if ((palm_err == -ENODEV) && (lap_err == -ENODEV)) > return 0; return 1, see comment on previous patch. ## begin block ### > - /* For all other errors we can flag the failure */ > + /* If both sensors error out - return an error */ > + if (palm_err && lap_err) > + return palm_err ? palm_err : lap_err; > + /* > + * If just one sensor not available, we still want the input device, > + * so just flag it and carry on > + */ > if (palm_err) > - return palm_err; > + pr_info("Palm sensor returned error %d", palm_err); > + if (lap_err) > + pr_info("Lap sensor returned error %d", lap_err); ### end block ### thinkpad_acpi will typically error out completely on non -ENODEV errors and the palmsensor code from patch 2/3 also does that. Note that returning an error from a module/sub-driver's init() is fatal (causes the module to not load), so before this change the palmsensor_get call failing with a non -ENODEV error was fatal. This may seem a bit harsh, but it is how error handling in thinkpad_acpi has worked so far, so lets be consistent here. Also if now only 1 of the 2 sensors is available you will log the -ENODEV error. So this whole block should be replaced with something like this: if (palm_err && palm_err != ENODEV) return palm_err; if (lap_err && lap_err != ENODEV) return lap_err; > > - if (has_palmsensor) { > + if (has_palmsensor || has_lapsensor) { > tpacpi_sw_dev = input_allocate_device(); > if (!tpacpi_sw_dev) > return -ENOMEM; > @@ -9990,10 +10027,14 @@ static int tpacpi_proxsensor_init(struct ibm_init_struct *iibm) > input_set_capability(tpacpi_sw_dev, EV_SW, SW_PALMREST_PROXIMITY); > input_report_switch(tpacpi_sw_dev, SW_PALMREST_PROXIMITY, palmsensor_state); > } > - palm_err = input_register_device(tpacpi_sw_dev); > - if (palm_err) { > + if (has_lapsensor) { > + input_set_capability(tpacpi_sw_dev, EV_SW, SW_LAP_PROXIMITY); > + input_report_switch(tpacpi_sw_dev, SW_LAP_PROXIMITY, lapsensor_state); > + } > + err = input_register_device(tpacpi_sw_dev); > + if (err) { > input_free_device(tpacpi_sw_dev); > - return palm_err; > + return err; > } > } > return 0; > @@ -10057,8 +10098,10 @@ static void tpacpi_driver_event(const unsigned int hkey_event) > mutex_unlock(&kbdlight_mutex); > } > > - if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED) > + if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED) { > dytc_lapmode_refresh(); > + proxsensor_refresh(false /* palm */, true /* lap */); > + } > > } > > Otherwise this looks good to me, Regards, Hans
Hi Hans, On 25/10/2020 08:03, Hans de Goede wrote: > Hi, > > On 10/20/20 2:15 AM, Mark Pearson wrote: >> Use input device event support for notifying userspace of lap mode sensor >> state changes. >> >> Signed-off-by: Mark Pearson <markpearson@lenovo.com> >> --- >> drivers/platform/x86/thinkpad_acpi.c | 75 ++++++++++++++++++++++------ >> 1 file changed, 59 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c >> index 5ddf2775fb06..c20b9902270b 100644 >> --- a/drivers/platform/x86/thinkpad_acpi.c >> +++ b/drivers/platform/x86/thinkpad_acpi.c >> @@ -4013,7 +4013,7 @@ static bool hotkey_notify_usrevent(const u32 hkey, >> } >> >> static void thermal_dump_all_sensors(void); >> -static void proxsensor_refresh(void); >> +static void proxsensor_refresh(bool palm, bool lap); >> >> static bool hotkey_notify_6xxx(const u32 hkey, >> bool *send_acpi_ev, >> @@ -4081,7 +4081,7 @@ static bool hotkey_notify_6xxx(const u32 hkey, >> case TP_HKEY_EV_PALM_DETECTED: >> case TP_HKEY_EV_PALM_UNDETECTED: >> /* palm detected - pass on to event handler */ >> - proxsensor_refresh(); >> + proxsensor_refresh(true /* palm */, false /* lap */); >> return true; >> >> default: >> @@ -9929,6 +9929,23 @@ static struct ibm_struct dytc_driver_data = { >> struct input_dev *tpacpi_sw_dev; >> bool has_palmsensor; >> bool palmsensor_state; >> +bool has_lapsensor; >> +bool lapsensor_state; > > Again, drop the global _state caching, it is not necessary. Agreed - will do. > >> + >> +static int lapsensor_get(bool *present, bool *state) >> +{ >> + acpi_handle dytc_handle; >> + int output; >> + >> + *present = false; >> + if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) >> + return -ENODEV; >> + if (!acpi_evalf(dytc_handle, &output, NULL, "dd", DYTC_CMD_GET)) >> + return -EIO; >> + *present = true; /*If we get his far, we have lapmode support*/ >> + *state = output & BIT(DYTC_GET_LAPMODE_BIT) ? true : false; >> + return 0; >> +} >> >> static int palmsensor_get(bool *present, bool *state) >> { >> @@ -9945,36 +9962,56 @@ static int palmsensor_get(bool *present, bool *state) >> return 0; >> } >> >> -static void proxsensor_refresh(void) >> +static void proxsensor_refresh(bool palm, bool lap) > > There is zero shared code between the palm ==true and the > lap ==true paths, please just make this 2 separate functions. > > And then I guess rename the original proxsensor_refresh to > palmsensor_refresh (note please do this in the 2/3 patch) > and add a new lapsensor_refresh Yeah - makes sense. Will do. > >> { >> bool new_state; >> int err; >> >> - if (has_palmsensor) { >> + if (palm && has_palmsensor) { >> err = palmsensor_get(&has_palmsensor, &new_state); >> - if (err) >> - return; > > And then you can also keep the if (err) return; construct, which > is a bit cleaner (more common used) solution vs the !err way > of handling errors. Agreed. > >> - if (new_state != palmsensor_state) { >> + if (!err && (new_state != palmsensor_state)) { >> input_report_switch(tpacpi_sw_dev, SW_PALMREST_PROXIMITY, new_state); >> input_sync(tpacpi_sw_dev); >> palmsensor_state = new_state; >> } >> } >> + >> + if (lap && has_lapsensor) { >> + err = lapsensor_get(&has_lapsensor, &new_state); >> + if (!err && (new_state != lapsensor_state)) { >> + input_report_switch(tpacpi_sw_dev, SW_LAP_PROXIMITY, new_state); >> + input_sync(tpacpi_sw_dev); >> + lapsensor_state = new_state; > > Same as with the other patch there is no need for the > new_state != lapsensor_state check, the input core does this for you > turning reporting the same state twice into a no-op. Agreed > >> + } >> + } >> } >> >> static int tpacpi_proxsensor_init(struct ibm_init_struct *iibm) >> { >> - int palm_err; >> + int palm_err, lap_err, err; >> >> + /* Make sure globals are set to a sensible initial value */ >> + has_palmsensor = false; >> + has_lapsensor = false; >> palm_err = palmsensor_get(&has_palmsensor, &palmsensor_state); >> + lap_err = lapsensor_get(&has_lapsensor, &lapsensor_state); >> + >> /* If support isn't available (ENODEV) then don't return an error */ >> - if (palm_err == -ENODEV) >> + if ((palm_err == -ENODEV) && (lap_err == -ENODEV)) >> return 0; > > return 1, see comment on previous patch. > > ## begin block ### >> - /* For all other errors we can flag the failure */ >> + /* If both sensors error out - return an error */ >> + if (palm_err && lap_err) >> + return palm_err ? palm_err : lap_err; >> + /* >> + * If just one sensor not available, we still want the input device, >> + * so just flag it and carry on >> + */ >> if (palm_err) >> - return palm_err; >> + pr_info("Palm sensor returned error %d", palm_err); >> + if (lap_err) >> + pr_info("Lap sensor returned error %d", lap_err); > ### end block ### > > thinkpad_acpi will typically error out completely on non -ENODEV > errors and the palmsensor code from patch 2/3 also does that. > Note that returning an error from a module/sub-driver's init() is > fatal (causes the module to not load), so before this change the > palmsensor_get call failing with a non -ENODEV error was fatal. > This may seem a bit harsh, but it is how error handling in > thinkpad_acpi has worked so far, so lets be consistent here. > > Also if now only 1 of the 2 sensors is available you will log > the -ENODEV error. > > So this whole block should be replaced with something like this: > > if (palm_err && palm_err != ENODEV) > return palm_err; > > if (lap_err && lap_err != ENODEV) > return lap_err; > Understood - that was my aim and I think I just messed up here. Thanks >> >> - if (has_palmsensor) { >> + if (has_palmsensor || has_lapsensor) { >> tpacpi_sw_dev = input_allocate_device(); >> if (!tpacpi_sw_dev) >> return -ENOMEM; >> @@ -9990,10 +10027,14 @@ static int tpacpi_proxsensor_init(struct ibm_init_struct *iibm) >> input_set_capability(tpacpi_sw_dev, EV_SW, SW_PALMREST_PROXIMITY); >> input_report_switch(tpacpi_sw_dev, SW_PALMREST_PROXIMITY, palmsensor_state); >> } >> - palm_err = input_register_device(tpacpi_sw_dev); >> - if (palm_err) { >> + if (has_lapsensor) { >> + input_set_capability(tpacpi_sw_dev, EV_SW, SW_LAP_PROXIMITY); >> + input_report_switch(tpacpi_sw_dev, SW_LAP_PROXIMITY, lapsensor_state); >> + } >> + err = input_register_device(tpacpi_sw_dev); >> + if (err) { >> input_free_device(tpacpi_sw_dev); >> - return palm_err; >> + return err; >> } >> } >> return 0; >> @@ -10057,8 +10098,10 @@ static void tpacpi_driver_event(const unsigned int hkey_event) >> mutex_unlock(&kbdlight_mutex); >> } >> >> - if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED) >> + if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED) { >> dytc_lapmode_refresh(); >> + proxsensor_refresh(false /* palm */, true /* lap */); >> + } >> >> } >> >> > > Otherwise this looks good to me, > > Regards, > > Hans > Thanks for the review Mark
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index 5ddf2775fb06..c20b9902270b 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -4013,7 +4013,7 @@ static bool hotkey_notify_usrevent(const u32 hkey, } static void thermal_dump_all_sensors(void); -static void proxsensor_refresh(void); +static void proxsensor_refresh(bool palm, bool lap); static bool hotkey_notify_6xxx(const u32 hkey, bool *send_acpi_ev, @@ -4081,7 +4081,7 @@ static bool hotkey_notify_6xxx(const u32 hkey, case TP_HKEY_EV_PALM_DETECTED: case TP_HKEY_EV_PALM_UNDETECTED: /* palm detected - pass on to event handler */ - proxsensor_refresh(); + proxsensor_refresh(true /* palm */, false /* lap */); return true; default: @@ -9929,6 +9929,23 @@ static struct ibm_struct dytc_driver_data = { struct input_dev *tpacpi_sw_dev; bool has_palmsensor; bool palmsensor_state; +bool has_lapsensor; +bool lapsensor_state; + +static int lapsensor_get(bool *present, bool *state) +{ + acpi_handle dytc_handle; + int output; + + *present = false; + if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) + return -ENODEV; + if (!acpi_evalf(dytc_handle, &output, NULL, "dd", DYTC_CMD_GET)) + return -EIO; + *present = true; /*If we get his far, we have lapmode support*/ + *state = output & BIT(DYTC_GET_LAPMODE_BIT) ? true : false; + return 0; +} static int palmsensor_get(bool *present, bool *state) { @@ -9945,36 +9962,56 @@ static int palmsensor_get(bool *present, bool *state) return 0; } -static void proxsensor_refresh(void) +static void proxsensor_refresh(bool palm, bool lap) { bool new_state; int err; - if (has_palmsensor) { + if (palm && has_palmsensor) { err = palmsensor_get(&has_palmsensor, &new_state); - if (err) - return; - if (new_state != palmsensor_state) { + if (!err && (new_state != palmsensor_state)) { input_report_switch(tpacpi_sw_dev, SW_PALMREST_PROXIMITY, new_state); input_sync(tpacpi_sw_dev); palmsensor_state = new_state; } } + + if (lap && has_lapsensor) { + err = lapsensor_get(&has_lapsensor, &new_state); + if (!err && (new_state != lapsensor_state)) { + input_report_switch(tpacpi_sw_dev, SW_LAP_PROXIMITY, new_state); + input_sync(tpacpi_sw_dev); + lapsensor_state = new_state; + } + } } static int tpacpi_proxsensor_init(struct ibm_init_struct *iibm) { - int palm_err; + int palm_err, lap_err, err; + /* Make sure globals are set to a sensible initial value */ + has_palmsensor = false; + has_lapsensor = false; palm_err = palmsensor_get(&has_palmsensor, &palmsensor_state); + lap_err = lapsensor_get(&has_lapsensor, &lapsensor_state); + /* If support isn't available (ENODEV) then don't return an error */ - if (palm_err == -ENODEV) + if ((palm_err == -ENODEV) && (lap_err == -ENODEV)) return 0; - /* For all other errors we can flag the failure */ + /* If both sensors error out - return an error */ + if (palm_err && lap_err) + return palm_err ? palm_err : lap_err; + /* + * If just one sensor not available, we still want the input device, + * so just flag it and carry on + */ if (palm_err) - return palm_err; + pr_info("Palm sensor returned error %d", palm_err); + if (lap_err) + pr_info("Lap sensor returned error %d", lap_err); - if (has_palmsensor) { + if (has_palmsensor || has_lapsensor) { tpacpi_sw_dev = input_allocate_device(); if (!tpacpi_sw_dev) return -ENOMEM; @@ -9990,10 +10027,14 @@ static int tpacpi_proxsensor_init(struct ibm_init_struct *iibm) input_set_capability(tpacpi_sw_dev, EV_SW, SW_PALMREST_PROXIMITY); input_report_switch(tpacpi_sw_dev, SW_PALMREST_PROXIMITY, palmsensor_state); } - palm_err = input_register_device(tpacpi_sw_dev); - if (palm_err) { + if (has_lapsensor) { + input_set_capability(tpacpi_sw_dev, EV_SW, SW_LAP_PROXIMITY); + input_report_switch(tpacpi_sw_dev, SW_LAP_PROXIMITY, lapsensor_state); + } + err = input_register_device(tpacpi_sw_dev); + if (err) { input_free_device(tpacpi_sw_dev); - return palm_err; + return err; } } return 0; @@ -10057,8 +10098,10 @@ static void tpacpi_driver_event(const unsigned int hkey_event) mutex_unlock(&kbdlight_mutex); } - if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED) + if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED) { dytc_lapmode_refresh(); + proxsensor_refresh(false /* palm */, true /* lap */); + } }
Use input device event support for notifying userspace of lap mode sensor state changes. Signed-off-by: Mark Pearson <markpearson@lenovo.com> --- drivers/platform/x86/thinkpad_acpi.c | 75 ++++++++++++++++++++++------ 1 file changed, 59 insertions(+), 16 deletions(-)