* [PATCH v2 1/3] Input: add event codes for lap and palmreset proximity switches @ 2020-10-20 0:15 Mark Pearson 2020-10-20 0:15 ` [PATCH v2 2/3] platform/x86: thinkpad_acpi: Add support for Lenovo palm sensor Mark Pearson 2020-10-20 0:15 ` [PATCH v2 3/3] platform/x86: thinkpad_acpi: Add support for Lenovo lap sensor Mark Pearson 0 siblings, 2 replies; 7+ messages in thread From: Mark Pearson @ 2020-10-20 0:15 UTC (permalink / raw) To: markpearson Cc: njoshi1, hdegoede, dmitry.torokhov, platform-driver-x86, linux-input, jeff, anthony.wong, hadess Add infrastructure needed to support lap and palmrest proximity sensors. These sensors are used for identifying thermal mode changes and modifying WWAN transmitter power. Signed-off-by: Mark Pearson <markpearson@lenovo.com> --- include/linux/mod_devicetable.h | 2 +- include/uapi/linux/input-event-codes.h | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h index 5b08a473cdba..897f5a3e7721 100644 --- a/include/linux/mod_devicetable.h +++ b/include/linux/mod_devicetable.h @@ -320,7 +320,7 @@ struct pcmcia_device_id { #define INPUT_DEVICE_ID_LED_MAX 0x0f #define INPUT_DEVICE_ID_SND_MAX 0x07 #define INPUT_DEVICE_ID_FF_MAX 0x7f -#define INPUT_DEVICE_ID_SW_MAX 0x10 +#define INPUT_DEVICE_ID_SW_MAX 0x12 #define INPUT_DEVICE_ID_PROP_MAX 0x1f #define INPUT_DEVICE_ID_MATCH_BUS 1 diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h index 0c2e27d28e0a..26f71a9a6936 100644 --- a/include/uapi/linux/input-event-codes.h +++ b/include/uapi/linux/input-event-codes.h @@ -889,7 +889,9 @@ #define SW_MUTE_DEVICE 0x0e /* set = device disabled */ #define SW_PEN_INSERTED 0x0f /* set = pen inserted */ #define SW_MACHINE_COVER 0x10 /* set = cover closed */ -#define SW_MAX 0x10 +#define SW_LAP_PROXIMITY 0x11 /* set = lap proximity sensor active */ +#define SW_PALMREST_PROXIMITY 0x12 /* set = palmrest proximity sensor active */ +#define SW_MAX 0x12 #define SW_CNT (SW_MAX+1) /* -- 2.28.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/3] platform/x86: thinkpad_acpi: Add support for Lenovo palm sensor 2020-10-20 0:15 [PATCH v2 1/3] Input: add event codes for lap and palmreset proximity switches Mark Pearson @ 2020-10-20 0:15 ` Mark Pearson 2020-10-25 12:04 ` Hans de Goede 2020-10-20 0:15 ` [PATCH v2 3/3] platform/x86: thinkpad_acpi: Add support for Lenovo lap sensor Mark Pearson 1 sibling, 1 reply; 7+ messages in thread From: Mark Pearson @ 2020-10-20 0:15 UTC (permalink / raw) To: markpearson Cc: njoshi1, hdegoede, dmitry.torokhov, platform-driver-x86, linux-input, jeff, anthony.wong, hadess Use input device event support for notifying userspace of palm sensor state changes Signed-off-by: Mark Pearson <markpearson@lenovo.com> --- drivers/platform/x86/thinkpad_acpi.c | 99 +++++++++++++++++++++++++++- 1 file changed, 97 insertions(+), 2 deletions(-) diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index eae3579f106f..5ddf2775fb06 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -4013,6 +4013,7 @@ static bool hotkey_notify_usrevent(const u32 hkey, } static void thermal_dump_all_sensors(void); +static void proxsensor_refresh(void); static bool hotkey_notify_6xxx(const u32 hkey, bool *send_acpi_ev, @@ -4079,8 +4080,8 @@ static bool hotkey_notify_6xxx(const u32 hkey, case TP_HKEY_EV_PALM_DETECTED: case TP_HKEY_EV_PALM_UNDETECTED: - /* palm detected hovering the keyboard, forward to user-space - * via netlink for consumption */ + /* palm detected - pass on to event handler */ + proxsensor_refresh(); return true; default: @@ -9918,6 +9919,96 @@ static struct ibm_struct dytc_driver_data = { .exit = dytc_exit, }; +/************************************************************************* + * Proximity sensor subdriver + */ + +#define PALMSENSOR_PRESENT_BIT 0 /* Determine if psensor present */ +#define PALMSENSOR_ON_BIT 1 /* psensor status */ + +struct input_dev *tpacpi_sw_dev; +bool has_palmsensor; +bool palmsensor_state; + +static int palmsensor_get(bool *present, bool *state) +{ + acpi_handle psensor_handle; + int output; + + if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "GPSS", &psensor_handle))) + return -ENODEV; + if (!acpi_evalf(psensor_handle, &output, NULL, "d")) + return -EIO; + + *present = output & BIT(PALMSENSOR_PRESENT_BIT) ? true : false; + *state = output & BIT(PALMSENSOR_ON_BIT) ? true : false; + return 0; +} + +static void proxsensor_refresh(void) +{ + bool new_state; + int err; + + if (has_palmsensor) { + err = palmsensor_get(&has_palmsensor, &new_state); + if (err) + return; + if (new_state != palmsensor_state) { + input_report_switch(tpacpi_sw_dev, SW_PALMREST_PROXIMITY, new_state); + input_sync(tpacpi_sw_dev); + palmsensor_state = new_state; + } + } +} + +static int tpacpi_proxsensor_init(struct ibm_init_struct *iibm) +{ + int palm_err; + + palm_err = palmsensor_get(&has_palmsensor, &palmsensor_state); + /* If support isn't available (ENODEV) then don't return an error */ + if (palm_err == -ENODEV) + return 0; + /* For all other errors we can flag the failure */ + if (palm_err) + return palm_err; + + if (has_palmsensor) { + tpacpi_sw_dev = input_allocate_device(); + if (!tpacpi_sw_dev) + return -ENOMEM; + tpacpi_sw_dev->name = "Thinkpad proximity switches"; + tpacpi_sw_dev->phys = TPACPI_DRVR_NAME "/input1"; + tpacpi_sw_dev->id.bustype = BUS_HOST; + tpacpi_sw_dev->id.vendor = thinkpad_id.vendor; + tpacpi_sw_dev->id.product = TPACPI_HKEY_INPUT_PRODUCT; + tpacpi_sw_dev->id.version = TPACPI_HKEY_INPUT_VERSION; + tpacpi_sw_dev->dev.parent = &tpacpi_pdev->dev; + + if (has_palmsensor) { + 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) { + input_free_device(tpacpi_sw_dev); + return palm_err; + } + } + return 0; +} + +static void proxsensor_exit(void) +{ + input_unregister_device(tpacpi_sw_dev); + input_free_device(tpacpi_sw_dev); +} + +static struct ibm_struct proxsensor_driver_data = { + .name = "proximity-sensor", + .exit = proxsensor_exit, +}; /**************************************************************************** **************************************************************************** * @@ -10411,6 +10502,10 @@ static struct ibm_init_struct ibms_init[] __initdata = { .init = tpacpi_dytc_init, .data = &dytc_driver_data, }, + { + .init = tpacpi_proxsensor_init, + .data = &proxsensor_driver_data, + }, }; static int __init set_ibm_param(const char *val, const struct kernel_param *kp) -- 2.28.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/3] platform/x86: thinkpad_acpi: Add support for Lenovo palm sensor 2020-10-20 0:15 ` [PATCH v2 2/3] platform/x86: thinkpad_acpi: Add support for Lenovo palm sensor Mark Pearson @ 2020-10-25 12:04 ` Hans de Goede 2020-10-25 23:25 ` [External] " Mark Pearson 0 siblings, 1 reply; 7+ messages in thread From: Hans de Goede @ 2020-10-25 12:04 UTC (permalink / raw) To: Mark Pearson Cc: njoshi1, dmitry.torokhov, platform-driver-x86, linux-input, jeff, anthony.wong, hadess Hi, On 10/20/20 2:15 AM, Mark Pearson wrote: > Use input device event support for notifying userspace of palm sensor > state changes > > Signed-off-by: Mark Pearson <markpearson@lenovo.com> > --- > drivers/platform/x86/thinkpad_acpi.c | 99 +++++++++++++++++++++++++++- > 1 file changed, 97 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > index eae3579f106f..5ddf2775fb06 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -4013,6 +4013,7 @@ static bool hotkey_notify_usrevent(const u32 hkey, > } > > static void thermal_dump_all_sensors(void); > +static void proxsensor_refresh(void); > > static bool hotkey_notify_6xxx(const u32 hkey, > bool *send_acpi_ev, > @@ -4079,8 +4080,8 @@ static bool hotkey_notify_6xxx(const u32 hkey, > > case TP_HKEY_EV_PALM_DETECTED: > case TP_HKEY_EV_PALM_UNDETECTED: > - /* palm detected hovering the keyboard, forward to user-space > - * via netlink for consumption */ > + /* palm detected - pass on to event handler */ > + proxsensor_refresh(); > return true; > > default: > @@ -9918,6 +9919,96 @@ static struct ibm_struct dytc_driver_data = { > .exit = dytc_exit, > }; > > +/************************************************************************* > + * Proximity sensor subdriver > + */ > + > +#define PALMSENSOR_PRESENT_BIT 0 /* Determine if psensor present */ > +#define PALMSENSOR_ON_BIT 1 /* psensor status */ > + > +struct input_dev *tpacpi_sw_dev; > +bool has_palmsensor; > +bool palmsensor_state; There is no need to make palmsensor_state global, see below. > + > +static int palmsensor_get(bool *present, bool *state) > +{ > + acpi_handle psensor_handle; > + int output; > + > + if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "GPSS", &psensor_handle))) > + return -ENODEV; > + if (!acpi_evalf(psensor_handle, &output, NULL, "d")) > + return -EIO; > + > + *present = output & BIT(PALMSENSOR_PRESENT_BIT) ? true : false; > + *state = output & BIT(PALMSENSOR_ON_BIT) ? true : false; > + return 0; > +} > + > +static void proxsensor_refresh(void) > +{ > + bool new_state; > + int err; > + > + if (has_palmsensor) { > + err = palmsensor_get(&has_palmsensor, &new_state); > + if (err) > + return; > + if (new_state != palmsensor_state) { > + input_report_switch(tpacpi_sw_dev, SW_PALMREST_PROXIMITY, new_state); > + input_sync(tpacpi_sw_dev); > + palmsensor_state = new_state; > + } There is no need for the whole new_state / orig_state dance here, the input subsys will only send events to userspace if anything has actually changed, so you can just write the following here: err = palmsensor_get(&has_palmsensor, &new_state); if (err) return; input_report_switch(tpacpi_sw_dev, SW_PALMREST_PROXIMITY, new_state); input_sync(tpacpi_sw_dev); > + } > +} > + > +static int tpacpi_proxsensor_init(struct ibm_init_struct *iibm) > +{ > + int palm_err; > + > + palm_err = palmsensor_get(&has_palmsensor, &palmsensor_state); > + /* If support isn't available (ENODEV) then don't return an error */ > + if (palm_err == -ENODEV) > + return 0; If you return 1 here, then this new-module will not be added to the tpacpi_all_drivers list (which is good since it is non functional) and then tpacpi_proxsensor_exit will also be skipped. > + /* For all other errors we can flag the failure */ > + if (palm_err) > + return palm_err; > + > + if (has_palmsensor) { > + tpacpi_sw_dev = input_allocate_device(); > + if (!tpacpi_sw_dev) > + return -ENOMEM; > + tpacpi_sw_dev->name = "Thinkpad proximity switches"; > + tpacpi_sw_dev->phys = TPACPI_DRVR_NAME "/input1"; > + tpacpi_sw_dev->id.bustype = BUS_HOST; > + tpacpi_sw_dev->id.vendor = thinkpad_id.vendor; > + tpacpi_sw_dev->id.product = TPACPI_HKEY_INPUT_PRODUCT; > + tpacpi_sw_dev->id.version = TPACPI_HKEY_INPUT_VERSION; > + tpacpi_sw_dev->dev.parent = &tpacpi_pdev->dev; > + > + if (has_palmsensor) { > + 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) { > + input_free_device(tpacpi_sw_dev); Maybe do "tpacpi_sw_dev = NULL" here to avoid leaving a dangling global pointer around ? > + return palm_err; > + } return 0 here > + } > + return 0; And return 1 here, as discussed above. This will ... > +} > + > +static void proxsensor_exit(void) > +{ > + input_unregister_device(tpacpi_sw_dev); Avoid a NULL pointer deref here when there is no palmrest sensor. > + input_free_device(tpacpi_sw_dev); The line needs to be deleted, input_free_device() is only for freeing devices before they are registered (so in case of some error) input_unregister_device() already does a put on the device, so also calling input_free_device() here messes up the ref counting. > +} > + > +static struct ibm_struct proxsensor_driver_data = { > + .name = "proximity-sensor", > + .exit = proxsensor_exit, > +}; > /**************************************************************************** > **************************************************************************** > * > @@ -10411,6 +10502,10 @@ static struct ibm_init_struct ibms_init[] __initdata = { > .init = tpacpi_dytc_init, > .data = &dytc_driver_data, > }, > + { > + .init = tpacpi_proxsensor_init, > + .data = &proxsensor_driver_data, > + }, > }; > > static int __init set_ibm_param(const char *val, const struct kernel_param *kp) > Otherwise this looks good to me. Regards, Hans ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [External] Re: [PATCH v2 2/3] platform/x86: thinkpad_acpi: Add support for Lenovo palm sensor 2020-10-25 12:04 ` Hans de Goede @ 2020-10-25 23:25 ` Mark Pearson 0 siblings, 0 replies; 7+ messages in thread From: Mark Pearson @ 2020-10-25 23:25 UTC (permalink / raw) To: Hans de Goede Cc: njoshi1, dmitry.torokhov, platform-driver-x86, linux-input, jeff, anthony.wong, hadess Thanks Hans, On 25/10/2020 08:04, Hans de Goede wrote: > Hi, > > On 10/20/20 2:15 AM, Mark Pearson wrote: >> Use input device event support for notifying userspace of palm sensor >> state changes >> >> Signed-off-by: Mark Pearson <markpearson@lenovo.com> >> --- >> drivers/platform/x86/thinkpad_acpi.c | 99 +++++++++++++++++++++++++++- >> 1 file changed, 97 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c >> index eae3579f106f..5ddf2775fb06 100644 >> --- a/drivers/platform/x86/thinkpad_acpi.c >> +++ b/drivers/platform/x86/thinkpad_acpi.c >> @@ -4013,6 +4013,7 @@ static bool hotkey_notify_usrevent(const u32 hkey, >> } >> >> static void thermal_dump_all_sensors(void); >> +static void proxsensor_refresh(void); >> >> static bool hotkey_notify_6xxx(const u32 hkey, >> bool *send_acpi_ev, >> @@ -4079,8 +4080,8 @@ static bool hotkey_notify_6xxx(const u32 hkey, >> >> case TP_HKEY_EV_PALM_DETECTED: >> case TP_HKEY_EV_PALM_UNDETECTED: >> - /* palm detected hovering the keyboard, forward to user-space >> - * via netlink for consumption */ >> + /* palm detected - pass on to event handler */ >> + proxsensor_refresh(); >> return true; >> >> default: >> @@ -9918,6 +9919,96 @@ static struct ibm_struct dytc_driver_data = { >> .exit = dytc_exit, >> }; >> >> +/************************************************************************* >> + * Proximity sensor subdriver >> + */ >> + >> +#define PALMSENSOR_PRESENT_BIT 0 /* Determine if psensor present */ >> +#define PALMSENSOR_ON_BIT 1 /* psensor status */ >> + >> +struct input_dev *tpacpi_sw_dev; >> +bool has_palmsensor; >> +bool palmsensor_state; > > There is no need to make palmsensor_state global, see below. > >> + >> +static int palmsensor_get(bool *present, bool *state) >> +{ >> + acpi_handle psensor_handle; >> + int output; >> + >> + if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "GPSS", &psensor_handle))) >> + return -ENODEV; >> + if (!acpi_evalf(psensor_handle, &output, NULL, "d")) >> + return -EIO; >> + >> + *present = output & BIT(PALMSENSOR_PRESENT_BIT) ? true : false; >> + *state = output & BIT(PALMSENSOR_ON_BIT) ? true : false; >> + return 0; >> +} >> + >> +static void proxsensor_refresh(void) >> +{ >> + bool new_state; >> + int err; >> + >> + if (has_palmsensor) { >> + err = palmsensor_get(&has_palmsensor, &new_state); >> + if (err) >> + return; >> + if (new_state != palmsensor_state) { >> + input_report_switch(tpacpi_sw_dev, SW_PALMREST_PROXIMITY, new_state); >> + input_sync(tpacpi_sw_dev); >> + palmsensor_state = new_state; >> + } > > There is no need for the whole new_state / orig_state dance here, the input subsys > will only send events to userspace if anything has actually changed, so you can just > write the following here: > > err = palmsensor_get(&has_palmsensor, &new_state); > if (err) > return; > > input_report_switch(tpacpi_sw_dev, SW_PALMREST_PROXIMITY, new_state); > input_sync(tpacpi_sw_dev); > Ah - got it. Thanks! >> + } >> +} >> + >> +static int tpacpi_proxsensor_init(struct ibm_init_struct *iibm) >> +{ >> + int palm_err; >> + >> + palm_err = palmsensor_get(&has_palmsensor, &palmsensor_state); >> + /* If support isn't available (ENODEV) then don't return an error */ >> + if (palm_err == -ENODEV) >> + return 0; > > If you return 1 here, then this new-module will not be added > to the tpacpi_all_drivers list (which is good since it is non functional) > and then tpacpi_proxsensor_exit will also be skipped. Ah - I thought 0 would do that. Agreed and I'll change. > >> + /* For all other errors we can flag the failure */ >> + if (palm_err) >> + return palm_err; >> + >> + if (has_palmsensor) { >> + tpacpi_sw_dev = input_allocate_device(); >> + if (!tpacpi_sw_dev) >> + return -ENOMEM; >> + tpacpi_sw_dev->name = "Thinkpad proximity switches"; >> + tpacpi_sw_dev->phys = TPACPI_DRVR_NAME "/input1"; >> + tpacpi_sw_dev->id.bustype = BUS_HOST; >> + tpacpi_sw_dev->id.vendor = thinkpad_id.vendor; >> + tpacpi_sw_dev->id.product = TPACPI_HKEY_INPUT_PRODUCT; >> + tpacpi_sw_dev->id.version = TPACPI_HKEY_INPUT_VERSION; >> + tpacpi_sw_dev->dev.parent = &tpacpi_pdev->dev; >> + >> + if (has_palmsensor) { >> + 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) { >> + input_free_device(tpacpi_sw_dev); > > Maybe do "tpacpi_sw_dev = NULL" here to avoid leaving a dangling global pointer around ? Agreed - Will do > >> + return palm_err; >> + } > > return 0 here > >> + } >> + return 0; > > And return 1 here, as discussed above. Ack > > This will ... > >> +} >> + >> +static void proxsensor_exit(void) >> +{ >> + input_unregister_device(tpacpi_sw_dev); > > Avoid a NULL pointer deref here when there is no palmrest sensor. > >> + input_free_device(tpacpi_sw_dev); > > The line needs to be deleted, input_free_device() is only for freeing > devices before they are registered (so in case of some error) > input_unregister_device() already does a put on the device, so also > calling input_free_device() here messes up the ref counting. My bad. Will fix. > >> +} >> + >> +static struct ibm_struct proxsensor_driver_data = { >> + .name = "proximity-sensor", >> + .exit = proxsensor_exit, >> +}; >> /**************************************************************************** >> **************************************************************************** >> * >> @@ -10411,6 +10502,10 @@ static struct ibm_init_struct ibms_init[] __initdata = { >> .init = tpacpi_dytc_init, >> .data = &dytc_driver_data, >> }, >> + { >> + .init = tpacpi_proxsensor_init, >> + .data = &proxsensor_driver_data, >> + }, >> }; >> >> static int __init set_ibm_param(const char *val, const struct kernel_param *kp) >> > > Otherwise this looks good to me. > > Regards, > > Hans > Thanks for the review. I'll get those fixes in ASAP Mark ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 3/3] platform/x86: thinkpad_acpi: Add support for Lenovo lap sensor 2020-10-20 0:15 [PATCH v2 1/3] Input: add event codes for lap and palmreset proximity switches Mark Pearson 2020-10-20 0:15 ` [PATCH v2 2/3] platform/x86: thinkpad_acpi: Add support for Lenovo palm sensor Mark Pearson @ 2020-10-20 0:15 ` Mark Pearson 2020-10-25 12:03 ` Hans de Goede 1 sibling, 1 reply; 7+ messages in thread From: Mark Pearson @ 2020-10-20 0:15 UTC (permalink / raw) To: markpearson Cc: njoshi1, hdegoede, dmitry.torokhov, platform-driver-x86, linux-input, jeff, anthony.wong, hadess 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; + +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 */); + } } -- 2.28.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 3/3] platform/x86: thinkpad_acpi: Add support for Lenovo lap sensor 2020-10-20 0:15 ` [PATCH v2 3/3] platform/x86: thinkpad_acpi: Add support for Lenovo lap sensor Mark Pearson @ 2020-10-25 12:03 ` Hans de Goede 2020-10-25 23:27 ` [External] " Mark Pearson 0 siblings, 1 reply; 7+ messages in thread From: Hans de Goede @ 2020-10-25 12:03 UTC (permalink / raw) To: Mark Pearson Cc: njoshi1, dmitry.torokhov, platform-driver-x86, linux-input, jeff, anthony.wong, hadess 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [External] Re: [PATCH v2 3/3] platform/x86: thinkpad_acpi: Add support for Lenovo lap sensor 2020-10-25 12:03 ` Hans de Goede @ 2020-10-25 23:27 ` Mark Pearson 0 siblings, 0 replies; 7+ messages in thread From: Mark Pearson @ 2020-10-25 23:27 UTC (permalink / raw) To: Hans de Goede Cc: njoshi1, dmitry.torokhov, platform-driver-x86, linux-input, jeff, anthony.wong, hadess 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-10-25 23:27 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-20 0:15 [PATCH v2 1/3] Input: add event codes for lap and palmreset proximity switches Mark Pearson 2020-10-20 0:15 ` [PATCH v2 2/3] platform/x86: thinkpad_acpi: Add support for Lenovo palm sensor Mark Pearson 2020-10-25 12:04 ` Hans de Goede 2020-10-25 23:25 ` [External] " Mark Pearson 2020-10-20 0:15 ` [PATCH v2 3/3] platform/x86: thinkpad_acpi: Add support for Lenovo lap sensor Mark Pearson 2020-10-25 12:03 ` Hans de Goede 2020-10-25 23:27 ` [External] " Mark Pearson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).