|
From: Mark P. <mar...@le...> - 2020-06-02 02:33:14
|
Thanks Henrique
On 6/1/2020 6:42 PM, Henrique de Moraes Holschuh wrote:
> 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 <nj...@le...>
>> Signed-off-by: Nitin Joshi <nj...@le...>
>> Reviewed-by: Sugumaran <sla...@le...>
>> Signed-off-by: Mark Pearson <mar...@le...>
>
> 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:
> pla...@vg...
>
No problem. Always happy to have more input and I'll do that with the
updated version after fixing the below.
> 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.
>
Agreed - will fix and I'll test on an older platform.
As an aside only a few of last years platforms have this implementation
(and then all of this years) so it would impact a lot of devices. Good
catch.
Thanks
Mark
|
