Re: [ibm-acpi-devel] [PATCH v4] platform/x86: thinkpad_acpi: lap or desk mode interface

[Andy S. <and...@gm...>]

On Mon, Jun 29, 2020 at 10:23 PM Mark Pearson <mar...@le...> wrote:

Thanks for the patch, my comments below.

>   Newer Lenovo Thinkpad platforms have support to identify whether the
>   system is on-lap or not using an ACPI DYTC event from the firmware.
>
>   This patch provides the ability to retrieve the current mode via sysfs
>   entrypoints and will be used by userspace for thermal mode and WWAN
>   functionality

Please use proper indentation (no need to have spaces) and punctuation
(like period at the end of sentences).

...

>  drivers/platform/x86/thinkpad_acpi.c | 111 ++++++++++++++++++++++++++-
>  1 file changed, 109 insertions(+), 2 deletions(-)

You specifically added a new ABI, where is documentation? It's a show stopper.

...

> +       sysfs_notify(&tpacpi_pdev->dev.kobj, NULL,
> +                       "dytc_lapmode");

One line?

...

> +       int output;
> +
> +       output = dytc_command(DYTC_CMD_GET);

> +       if ((output == -ENODEV) || (output == -EIO))
> +               return output;

Why not simple

 if (output < 0)
   return output;

Btw, how will this survive the 31st bit (if some method would like to use it)?

I think your prototype should be

int foo(cmd, *output);

...

> +       new_state = dytc_lapmode_get();
> +       if ((new_state == -ENODEV) || (new_state == -EIO) || (new_state == dytc_lapmode))
> +               return;

What about other errors?
What about MSB being set?

...

> +       dytc_lapmode = dytc_lapmode_get();
> +
> +       if (dytc_lapmode < 0 && dytc_lapmode != -ENODEV)
> +               return dytc_lapmode;

> +       res = sysfs_create_group(&tpacpi_pdev->dev.kobj,
> +                               &dytc_attr_group);
> +
> +       return res;

return ...(...);

So, we create a group for all possible error cases but ENODEV. Why?

> +}

...

> +       sysfs_remove_group(&tpacpi_pdev->dev.kobj,
> +                       &dytc_attr_group);

One line?

...

> +static struct ibm_struct dytc_driver_data = {
> +       .name = "dytc",
> +       .exit = dytc_exit

Comma is missed.

> +};

-- 
With Best Regards,
Andy Shevchenko