On Mon, Jun 29, 2020 at 10:23 PM Mark Pearson <markpearson@xxxxxxxxxx> 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
