| Message ID | 20201020001556.388099-2-markpearson@lenovo.com |
|---|---|
| State | Superseded |
| 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
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)
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(-)