| Message ID | 20200715235242.4934-1-markpearson@lenovo.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: thinkpad_acpi: psensor interface | expand |
On Thu, Jul 16, 2020 at 2:53 AM Mark Pearson <markpearson@lenovo.com> wrote: > > Some Lenovo Thinkpad platforms are equipped with a 'palm sensor' so as > to be able to determine if a user is physically proximate to the device. > > This patch provides the ability to retrieve the psensor state via sysfs > entrypoints and will be used by userspace for WWAN functionality to > control the transmission level safely ... > 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 */ > + tpacpi_driver_event(hkey); > return true; Comment here tells something about the netlink interface to user space. Can you elaborate why we need sysfs now and how it's all supposed to work? ... > +static int psensor_get(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; > + > + /* Check if sensor has a Psensor */ > + if (!(output & BIT(PSENSOR_PRESENT_BIT))) > + return -ENODEV; > + > + /* Return if psensor is set or not */ > + *state = output & BIT(PSENSOR_ON_BIT) ? true : false; > + return 0; > +} It reminds me of a function you created in one of the previous changes. Can you rather create a parameterized helper which will serve for both? ... > +/* sysfs psensor entry */ > +static ssize_t psensor_state_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + return snprintf(buf, PAGE_SIZE, "%d\n", psensor_state); We know that %d takes much less than PAGE_SIZE, use sprintf(). > +} > + No blank line here. > +static DEVICE_ATTR_RO(psensor_state); ... > +static struct attribute *psensor_attributes[] = { > + &dev_attr_psensor_state.attr, > + NULL, No comma for terminator line(s). > +}; ... > + /* If support isn't available (ENODEV) then don't return an error > + * but just don't create the sysfs group > + */ /* * Consider to use a proper multi-line comment style. * Like here. (It's applicable to the entire patch) */ ... > + err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &psensor_attr_group); > + return err; return sysfs...
Hi Andy , >-----Original Message----- >From: Andy Shevchenko <andy.shevchenko@gmail.com> >Sent: Monday, July 27, 2020 7:35 PM >On Thu, Jul 16, 2020 at 2:53 AM Mark Pearson <markpearson@lenovo.com> >wrote: >> >> 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 */ >> + tpacpi_driver_event(hkey); >> return true; > >Comment here tells something about the netlink interface to user space. >Can you elaborate why we need sysfs now and how it's all supposed to >work? Using netlink , we were getting proximity events like '0x60b0' and '0x60b1' but for our WWAN requirement, we need default and current p-sensor state too . Some tools like "acpi-listen" uses netlink to display events but we need default and current p-sensor state also as per our requirement. hence , we have added new sysfs to get current p-sensor state using 'GPSS' method from BIOS . This will be used for implementing "Dynamic power reduction" app which is used to control Body SAR value as per FCC requirement . When Body or any object is near or away from p-sensor location on thinkpad system , then sysfs will be set . > >... > >> +static int psensor_get(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; >> + >> + /* Check if sensor has a Psensor */ >> + if (!(output & BIT(PSENSOR_PRESENT_BIT))) >> + return -ENODEV; >> + >> + /* Return if psensor is set or not */ >> + *state = output & BIT(PSENSOR_ON_BIT) ? true : false; >> + return 0; >> +} > >It reminds me of a function you created in one of the previous changes. Can >you rather create a parameterized helper which will serve for both? Ack , we will check it . > >... > >> +/* sysfs psensor entry */ >> +static ssize_t psensor_state_show(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) { > >> + return snprintf(buf, PAGE_SIZE, "%d\n", psensor_state); > >We know that %d takes much less than PAGE_SIZE, use sprintf(). > >> +} > >> + > >No blank line here. > Ack >> +static DEVICE_ATTR_RO(psensor_state); > >... > >> +static struct attribute *psensor_attributes[] = { >> + &dev_attr_psensor_state.attr, > >> + NULL, > >No comma for terminator line(s). > Ack >> +}; > >... > >> + /* If support isn't available (ENODEV) then don't return an error >> + * but just don't create the sysfs group >> + */ > >/* > * Consider to use a proper multi-line comment style. > * Like here. (It's applicable to the entire patch) */ > >... > >> + err = sysfs_create_group(&tpacpi_pdev->dev.kobj, >&psensor_attr_group); >> + return err; > >return sysfs... Ack Thanks & Regards, Nitin Joshi
Hi Andy, First off apologies for my delay in replying and thanks to Nitin for covering for me. I took a week of PTO and then suffered the consequences of that for the week after - it's taken me a bit to catch-up. On 7/27/2020 11:51 PM, Nitin Joshi1 wrote: > Hi Andy , > >> -----Original Message----- >> From: Andy Shevchenko <andy.shevchenko@gmail.com> >> Sent: Monday, July 27, 2020 7:35 PM >> On Thu, Jul 16, 2020 at 2:53 AM Mark Pearson <markpearson@lenovo.com> >> wrote: >>> >>> 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 */ >>> + tpacpi_driver_event(hkey); >>> return true; >> >> Comment here tells something about the netlink interface to user space. >> Can you elaborate why we need sysfs now and how it's all supposed to >> work? > Using netlink , we were getting proximity events like '0x60b0' and '0x60b1' but for our WWAN requirement, we need default and current > p-sensor state too . > Some tools like "acpi-listen" uses netlink to display events but we need default and current p-sensor state also as per our requirement. > hence , we have added new sysfs to get current p-sensor state using 'GPSS' method from BIOS . > This will be used for implementing "Dynamic power reduction" app which is used to control Body SAR value as per FCC requirement . > When Body or any object is near or away from p-sensor location on thinkpad system , then sysfs will be set . > I think Nitin has covered it. Let us know if any follow on questions or concerns here. >> >> ... >> >>> +static int psensor_get(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; >>> + >>> + /* Check if sensor has a Psensor */ >>> + if (!(output & BIT(PSENSOR_PRESENT_BIT))) >>> + return -ENODEV; >>> + >>> + /* Return if psensor is set or not */ >>> + *state = output & BIT(PSENSOR_ON_BIT) ? true : false; >>> + return 0; >>> +} >> >> It reminds me of a function you created in one of the previous changes. Can >> you rather create a parameterized helper which will serve for both? > > Ack , we will check it . I've been looking at this and I understand where you're coming from but the benefits of combining them aren't working for me. The previous change was for the lapmode sensor which is a separate piece of hardware. The ACPI handle and access format is different and the lapmode sensor has some extra handling logic needed. The code has enough differences too that I don't think combining it makes a lot of sense. Let me know if I'm missing something obvious or if you disagree. > >> >> ... >> >>> +/* sysfs psensor entry */ >>> +static ssize_t psensor_state_show(struct device *dev, >>> + struct device_attribute *attr, >>> + char *buf) { >> >>> + return snprintf(buf, PAGE_SIZE, "%d\n", psensor_state); >> >> We know that %d takes much less than PAGE_SIZE, use sprintf(). >> >>> +} >> >>> + >> >> No blank line here. >> > Ack > >>> +static DEVICE_ATTR_RO(psensor_state); >> >> ... >> >>> +static struct attribute *psensor_attributes[] = { >>> + &dev_attr_psensor_state.attr, >> >>> + NULL, >> >> No comma for terminator line(s). >> > > Ack > >>> +}; >> >> ... >> >>> + /* If support isn't available (ENODEV) then don't return an error >>> + * but just don't create the sysfs group >>> + */ >> >> /* >> * Consider to use a proper multi-line comment style. >> * Like here. (It's applicable to the entire patch) */ >> >> ... >> >>> + err = sysfs_create_group(&tpacpi_pdev->dev.kobj, >> &psensor_attr_group); >>> + return err; >> >> return sysfs... > Ack > I'll push a patch soon with these other adjustments. Thanks for the review Mark
diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst index 99066aa8d97b..c2c238d5d5f6 100644 --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst @@ -51,6 +51,7 @@ detailed description): - UWB enable and disable - LCD Shadow (PrivacyGuard) enable and disable - Lap mode sensor + - Palm sensor (aka p-sensor) A compatibility table by model and feature is maintained on the web site, http://ibm-acpi.sf.net/. I appreciate any success or failure @@ -1447,6 +1448,24 @@ they differ between desk and lap mode. The property is read-only. If the platform doesn't have support the sysfs class is not created. +Palm sensor +------------------ + +sysfs: psensor_state + +Certain thinkpads and mobile workstations are equipped with a palm sensor to +detect when a user is physically near the device. This device, when present, +is used in conjunction with the lapmode sensor to decide if WWAN transmission +can be increased to maximum power. + +The property is read-only. If the platform doesn't have support the sysfs +class is not created. + +Note - some platforms have a limitation whereby the EC firmware cannot +determine if the sensor is not installed or not. On these platforms the +p-sensor state will always be reported as true to avoid high power being used +incorrectly. + EXPERIMENTAL: UWB ----------------- diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index 037eb77414f9..40f2e368fdf9 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -4071,8 +4071,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 */ + tpacpi_driver_event(hkey); return true; default: @@ -9894,6 +9894,99 @@ static struct ibm_struct dytc_driver_data = { .exit = dytc_exit, }; +/************************************************************************* + * Palm sensor subdriver + */ +#define PSENSOR_PRESENT_BIT 0 /* Determine if psensor present */ +#define PSENSOR_ON_BIT 1 /* psensor status */ + +static bool psensor_state; + +static void psensor_notify_change(void) +{ + sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "psensor_state"); +} + +static int psensor_get(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; + + /* Check if sensor has a Psensor */ + if (!(output & BIT(PSENSOR_PRESENT_BIT))) + return -ENODEV; + + /* Return if psensor is set or not */ + *state = output & BIT(PSENSOR_ON_BIT) ? true : false; + return 0; +} + +static void psensor_state_refresh(void) +{ + bool new_state; + int err; + + err = psensor_get(&new_state); + if (err || (new_state == psensor_state)) + return; + + psensor_state = new_state; + psensor_notify_change(); +} + +/* sysfs psensor entry */ +static ssize_t psensor_state_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + return snprintf(buf, PAGE_SIZE, "%d\n", psensor_state); +} + +static DEVICE_ATTR_RO(psensor_state); + +static struct attribute *psensor_attributes[] = { + &dev_attr_psensor_state.attr, + NULL, +}; + +static const struct attribute_group psensor_attr_group = { + .attrs = psensor_attributes, +}; + +static int tpacpi_psensor_init(struct ibm_init_struct *iibm) +{ + int err; + + err = psensor_get(&psensor_state); + /* If support isn't available (ENODEV) then don't return an error + * but just don't create the sysfs group + */ + if (err == -ENODEV) + return 0; + /* For all other errors we can flag the failure */ + if (err) + return err; + + err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &psensor_attr_group); + return err; +} + +static void psensor_exit(void) +{ + sysfs_remove_group(&tpacpi_pdev->dev.kobj, &psensor_attr_group); +} + +static struct ibm_struct psensor_driver_data = { + .name = "psensor", + .exit = psensor_exit, +}; + /**************************************************************************** **************************************************************************** * @@ -9945,6 +10038,9 @@ static void tpacpi_driver_event(const unsigned int hkey_event) if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED) dytc_lapmode_refresh(); + if ((hkey_event == TP_HKEY_EV_PALM_DETECTED) || + (hkey_event == TP_HKEY_EV_PALM_UNDETECTED)) + psensor_state_refresh(); } static void hotkey_driver_event(const unsigned int scancode) @@ -10387,6 +10483,10 @@ static struct ibm_init_struct ibms_init[] __initdata = { .init = tpacpi_dytc_init, .data = &dytc_driver_data, }, + { + .init = tpacpi_psensor_init, + .data = &psensor_driver_data, + }, }; static int __init set_ibm_param(const char *val, const struct kernel_param *kp)