Re: [PATCH] platform/x86: thinkpad_acpi: psensor interface

[Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Thu, Jul 16, 2020 at 2:53 AM Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org> 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...

-- 
With Best Regards,
Andy Shevchenko