Hi Hans,
Thank you very much for your comments.
>-----Original Message-----
>From: Hans de Goede <hdegoede@xxxxxxxxxx>
>Sent: Wednesday, March 5, 2025 8:37 PM
>To: Mark Pearson <mpearson-lenovo@xxxxxxxxx>; Nitin Joshi
><nitjoshi@xxxxxxxxx>; Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
>Cc: platform-driver-x86@xxxxxxxxxxxxxxx; ibm-acpi-
>devel@xxxxxxxxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx; Nitin Joshi1
><njoshi1@xxxxxxxxxx>
>Subject: [External] Re: [PATCH] platform/x86: thinkpad_acpi: Add new sysfs to
>check user presence sensing status
>
>Hi Nitin, Mark,
>
>On 5-Mar-25 4:20 AM, Mark Pearson wrote:
>>
>> On Tue, Mar 4, 2025, at 9:33 PM, Nitin Joshi wrote:
>>> Some Thinkpad products support Human Presence Detection (HPD)
>features.
>>> Add new sysfs entry so that userspace can determine if feature is
>>> supported or not.
>>>
>>> Reviewed-by: Mark Pearson <mpearson-lenovo@xxxxxxxxx>
>>
>> Just in case we're breaking protocol - I have reviewed this off mailing list with
>Nitin and gave it the thumbs up. The tag is correct.
>
>Adding a Reviewed-by tag based on internal reviews done before submitting
>v1 is fine, no worries.
Noted , Thank you !
>
>I do wonder what the use-case for this exactly is?
>
This setting will be used to enable or disable features like "Lock on Leave" etc from user-space.
>The current documentation of "so that userspace can determine if feature
>related to HPD should be enabled or disabled."
>
>is a bit vague. The reason I'm asking is because I'm wondering if this is the best
>API to expose this to userspace.
>
>Also if I understand things correctly this is only about checking
>if:
>
>1) There is HPD support on the machine at all (if yes this file will exist)
I am also checking sensor status i.e CHPD_GET_SENSOR_STATUS from BIOS.
So , if there is no sensor present then file will not exist .
>2) If HPD is supported on this machine, is it also enabled or disabled in the
>BIOS?
User need to explicitly enable or disable it in BIOS for example user need to disable it , if does not
want to use "HPD" related features like "Lock on Leave" etc .
>
>IOW this is not about actually getting the HPD result, which would be "human
>present" or "human not present", right ?
>
Yes , your understanding is correct .
>Any plans to export the actual HPD result ?
Yes , there is discussion on-going with Intel to export actual HPD result but there is no fixed time plan
as of now.
>
>Also if this is just about checking the BIOS setting why not just use the think-lmi
>driver / firmware-attribute sysfs API for that ?
Sorry, I completely missed it .
Yes, as of now , we can use firmware-attribute sysfs to check enable and disable
status . In case , we need additional information after actual HPD data is available then
we can revisit this patch .
Is this acceptable ? Sorry, if any inconvenience !
>
>
>>> Signed-off-by: Nitin Joshi <nitjoshi@xxxxxxxxx>
>>> ---
>>> .../admin-guide/laptops/thinkpad-acpi.rst | 20 +++++
>>> drivers/platform/x86/thinkpad_acpi.c | 79 +++++++++++++++++++
>>> 2 files changed, 99 insertions(+)
>>>
>>> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>> b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>> index 4ab0fef7d440..02e6c4306f90 100644
>>> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>> @@ -1576,6 +1576,26 @@ percentage level, above which charging will stop.
>>> The exact semantics of the attributes may be found in
>>> Documentation/ABI/testing/sysfs-class-power.
>>>
>>> +User Presence Sensing Detection
>>> +------
>>> +
>>> +sysfs: hpd_bios_enabled
>>> +
>>> +Some Thinkpad products support Human Presence Detection (HPD)
>features.
>>> +Added new sysfs entry so that userspace can determine if feature
>>> +related to HPD should be enabled or disabled.
>
>"Added new sysfs entry ..." sounds more like something for a commit message
>then for in an ABI Documentation file. In 5 years the "adding new sysfs"
>language is going to look really weird in this file.
>
>Please just describe the function + intended uses without using "Adding new".
>
Noted , Thanks !
>>> +
>>> +The available commands are::
>>> +
>>> + cat /sys/devices/platform/thinkpad_acpi/hpd_bios_enabled
>>> +
>>> +BIOS status is mentioned as below:
>>> +- 0 = Disable
>>> +- 1 = Enable
>>> +
>>> +The property is read-only. If the platform doesn't have support the
>>> +sysfs class is not created.
>>> +
>>> Multiple Commands, Module Parameters
>>> ------------------------------------
>>>
>>> diff --git a/drivers/platform/x86/thinkpad_acpi.c
>>> b/drivers/platform/x86/thinkpad_acpi.c
>>> index 72a10ed2017c..daf31b2a4c73 100644
>>> --- a/drivers/platform/x86/thinkpad_acpi.c
>>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>>> @@ -11039,6 +11039,80 @@ static const struct attribute_group
>>> auxmac_attr_group = {
>>> .attrs = auxmac_attributes,
>>> };
>>>
>>>
>+/***************************************************************
>****
>>> +******
>>> + * CHPD subdriver, for the Lenovo Human Presence Detection feature.
>>> + */
>>> +#define CHPD_GET_SENSOR_STATUS 0x00200000
>>> +#define CHPD_GET_BIOS_UI_STATUS 0x00100000
>>> +
>>> +static bool has_user_presence_sensing; static int hpd_bios_status;
>>> +static int chpd_command(int command, int *output) {
>>> + acpi_handle chpd_handle;
>>> +
>>> + if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "CHPD",
>&chpd_handle))) {
>>> + /* Platform doesn't support CHPD */
>>> + return -ENODEV;
>>> + }
>>> +
>>> + if (!acpi_evalf(chpd_handle, output, NULL, "dd", command))
>>> + return -EIO;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/* sysfs hpd bios status */
>>> +static ssize_t hpd_bios_enabled_show(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + return sysfs_emit(buf, "%d\n", hpd_bios_status); } static
>>> +DEVICE_ATTR_RO(hpd_bios_enabled);
>>> +
>>> +static struct attribute *chpd_attributes[] = {
>>> + &dev_attr_hpd_bios_enabled.attr,
>>> + NULL
>>> +};
>>> +
>>> +static umode_t chpd_attr_is_visible(struct kobject *kobj,
>>> + struct attribute *attr, int n)
>>> +{
>>> + return has_user_presence_sensing ? attr->mode : 0; }
>>> +
>>> +static const struct attribute_group chpd_attr_group = {
>>> + .is_visible = chpd_attr_is_visible,
>>> + .attrs = chpd_attributes,
>>> +};
>>> +
>>> +static int tpacpi_chpd_init(struct ibm_init_struct *iibm) {
>>> + int err, output;
>>> +
>>> + err = chpd_command(CHPD_GET_SENSOR_STATUS, &output);
>>> + if (err)
>>> + return err;
>>> +
>>> + if (output == 1)
>>> + return -ENODEV;
>>> +
>>> + has_user_presence_sensing = true;
>>> + /* Get User Presence Sensing BIOS status */
>>> + err = chpd_command(CHPD_GET_BIOS_UI_STATUS, &output);
>>> + if (err)
>>> + return err;
>>> +
>>> + hpd_bios_status = (output >> 1) & BIT(0);
>
>Please add a define for this rather then just hardcoding a shift by 1.
>
Ack , Thank you !
>>> +
>>> + return err;
>>> +}
>>> +
>>> +static struct ibm_struct chpd_driver_data = {
>>> + .name = "chpd",
>>> +};
>>> +
>>> /*
>>> ---------------------------------------------------------------------
>>> */
>>>
>>> static struct attribute *tpacpi_driver_attributes[] = { @@ -11098,6
>>> +11172,7 @@ static const struct attribute_group *tpacpi_groups[] = {
>>> &kbdlang_attr_group,
>>> &dprc_attr_group,
>>> &auxmac_attr_group,
>>> + &chpd_attr_group,
>>> NULL,
>>> };
>>>
>>> @@ -11694,6 +11769,10 @@ static struct ibm_init_struct ibms_init[]
>>> __initdata = {
>>> .init = auxmac_init,
>>> .data = &auxmac_data,
>>> },
>>> + {
>>> + .init = tpacpi_chpd_init,
>>> + .data = &chpd_driver_data,
>>> + },
>>> };
>>>
>>> static int __init set_ibm_param(const char *val, const struct
>>> kernel_param *kp)
>>> --
>>> 2.43.0
>>
>
>
>Regards,
>
>Hans
Thanks & Regards,
Nitin Joshi
