Hi Andy ,
>-----Original Message-----
>From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
>Sent: Monday, July 27, 2020 7:35 PM
>On Thu, Jul 16, 2020 at 2:53 AM Mark Pearson <markpearson@xxxxxxxxxx>
>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
