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.
I do wonder what the use-case for this exactly is?
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)
2) If HPD is supported on this machine, is it also enabled or
disabled in the BIOS?
IOW this is not about actually getting the HPD result,
which would be "human present" or "human not present", right ?
Any plans to export the actual HPD result ?
Also if this is just about checking the BIOS setting why not
just use the think-lmi driver / firmware-attribute sysfs API
for that ?
>> 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".
>> +
>> +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.
>> +
>> + 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
