| Message ID | 20201020001556.388099-1-markpearson@lenovo.com |
|---|---|
| State | New |
| 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 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
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
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) /*
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(-)