On Mon, 01 Jun 2020, Mark Pearson wrote:
>   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
> 
> Co-developed-by: Nitin Joshi <njos...@lenovo.com>
> Signed-off-by: Nitin Joshi <njos...@lenovo.com>
> Reviewed-by: Sugumaran <slacshimi...@lenovo.com>
> Signed-off-by: Mark Pearson <markpear...@lenovo.com>

We need to take this through the kernel platform-driver-x86 ML.

It would be nice to have standard events for "latop on a surface you
don't want to burn ("lap"), and the input people might want to suggest
something, too, so I'd also ask the input maintainer.

Please resend, cc to:
platform-driver-...@vger.kernel.org

While at it there is something I noticed right away:

> +static int dytc_command(int command)
> +{
> +     acpi_handle dytc_handle;
> +     int output;
> +
> +     if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) {
> +             pr_warn("Thinkpad ACPI has no DYTC interface.\n");
> +             return -ENODEV;
> +     }
> +     if (!acpi_evalf(dytc_handle, &output, NULL, "dd", command))
> +             return -EIO;
> +     return output;
> +}

dytc_command() is called by dytc_lapmode_get():

> +static int dytc_lapmode_get(void)
> +{
> +     int output;
> +
> +     output = dytc_command(DYTC_CMD_GET);
> +     if ((output == -ENODEV) || (output == -EIO))
> +             return output;
> +
> +     return ((output >> DYTC_GET_LAPMODE_SHIFT) &
> +                             DYTC_GET_ENABLE_MASK);
> +}

Which is used by dytc_lapmode_init():


> +static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
> +{
> +     int res;
> +
> +     dytc_available = false;
> +     dytc_lapmode = dytc_lapmode_get();
> +
> +     if (dytc_lapmode < 0 && dytc_lapmode != -ENODEV)
> +             return dytc_lapmode;
> +
> +     dytc_available = true;
> +
> +     res = sysfs_create_group(&tpacpi_pdev->dev.kobj,
> +                             &dytc_attr_group);
> +
> +     return res;
> +}

Looks like this code flow is going to spam people with pr_warn() on an
older thinkpad laptop that doesn't have DYTC.  Please fix this, it is
not strange for an older thinkpad to not have DYTC (even if it is a
decade's old thinkpad).

There is a thinkpad-acpi driver-level debug facility you can use to send
developer-level debug info (such as the init function of a subdriver did
not find what it wanted), if you want.

Also, if the code flow goes through dytc_init fine and registers the
sub-driver, you likely don't have to optimize the rest of the code flow
for DYTC disappearing from APCI tables ;-)  ENODEV silently would be
fine for something so unlikely to happen.

-- 
  Henrique Holschuh


_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Reply via email to