Hey Mark,
----- Original Message -----
> 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 <njoshi1@xxxxxxxxxx>
> Signed-off-by: Nitin Joshi <njoshi1@xxxxxxxxxx>
> Reviewed-by: Sugumaran <slacshiminar@xxxxxxxxxx>
> Signed-off-by: Mark Pearson <markpearson@xxxxxxxxxx>
> ---
> Resend:
> - Adding Bastien Nocera to cc list as requested
> Changes in v2:
> - cleaned up initialisation sequence to be cleaner and avoid spamming
> platforms that don't have DYTC with warning message. Tested on P52
> - Adding platform-driver-x86 mailing list for review as requested
>
> drivers/platform/x86/thinkpad_acpi.c | 113 +++++++++++++++++++++++++++
> 1 file changed, 113 insertions(+)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index 0f704484ae1d..8f51bbba21cd 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -4049,6 +4049,7 @@ static bool hotkey_notify_6xxx(const u32 hkey,
> pr_debug("EC reports: Thermal Control Command set completed (DYTC)\n");
> /* recommended action: do nothing, we don't have
> * Lenovo ATM information */
> + tpacpi_driver_event(hkey);
> return true;
> case TP_HKEY_EV_THM_TRANSFM_CHANGED:
> pr_debug("EC reports: Thermal Transformation changed (GMTS)\n");
> @@ -9811,6 +9812,110 @@ static struct ibm_struct lcdshadow_driver_data = {
> .write = lcdshadow_write,
> };
>
> +/*************************************************************************
> + * DYTC subdriver, for the Lenovo performace mode feature
> + */
I don't think this should mention the performance mode, as it's a lap/table
detection mode. Do we need to call that "DYTC"? "lapmode"? "lap_detection"?
Or does the DYTC interface offer more functionality that we'd export in the
future?
> +
> +#define DYTC_CMD_GET 2 /*To get current IC function and mode*/
For this comment and all the ones below, space after "/*" and before "*/"
> +#define DYTC_GET_ENABLE_MASK 0x1 /*0 = disabled, 1 = enabled*/
Is that necessary?
> +#define DYTC_GET_LAPMODE_SHIFT 17
You'd probably want to call this "bit" rather than shift. We shift it to read
the value, but 17 is the bit's position. (See BIT() usage in the driver)
Do you want to add a comment here? Is there anything else that could be
documented (the other bits, which versions of firmware would have that, if
there's a publicly available version, or which hardware if publicly available)
> +static int dytc_lapmode;
> +static void dytc_lapmode_notify_change(void)
> +{
> + sysfs_notify(&tpacpi_pdev->dev.kobj, NULL,
> + "dytc_lapmode");
> +}
> +
> +static int dytc_command(int command)
> +{
> + acpi_handle dytc_handle;
> + int output;
> +
> + if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) {
> + /*Platform doesn't support DYTC*/
> + return -ENODEV;
> + }
> + if (!acpi_evalf(dytc_handle, &output, NULL, "dd", command))
> + return -EIO;
> + return output;
> +}
> +
> +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);
Use BIT() instead? eg.
return (output & BIT(DYTC_GET_LAPMODE_SHIFT));
> +}
> +
> +static void dytc_lapmode_refresh(void)
> +{
> + int new_state;
> +
> + new_state = dytc_lapmode_get();
> + if ((new_state == -ENODEV) || (new_state == -EIO))
> + return;
You could also return early if "dytc_lapmode == new_state".
Rest looks good to me.
