Re: [PATCH v2 2/2] platform/x86: ideapad-laptop: DYTC Platform profile support

From: Hans de Goede
Date: Tue Feb 02 2021 - 13:34:03 EST

Next message: Greg KH: "Re: [PATCH v2] platform/chrome: cros_ec_typec: Fix call to typec_partner_set_pd_revision"
Previous message: Peter Zijlstra: "Re: kprobes broken since 0d00449c7a28 ("x86: Replace ist_enter() with nmi_enter()")"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

On 1/5/21 2:14 PM, Jiaxun Yang wrote:
> Add support to ideapad-laptop for Lenovo platforms that have DYTC
> version 5 support or newer to use the platform profile feature.
>
> Mostly based on Mark Pearson <markpearson@xxxxxxxxxx>'s thinkpad-acpi
> work but massaged to fit ideapad driver.
>
> Note that different from ThinkPads, IdeaPads's Thermal Hotkey won't
> trigger profile switch itself, we'll leave it for userspace programs.
>
> Tested on Lenovo Yoga-14S ARE Chinese Edition.
>
> Signed-off-by: Jiaxun Yang <jiaxun.yang@xxxxxxxxxxx>

Now that the acpi/platform_profile stuff has landed I've merged this patch.

Note I've made one small but important change I've replaced all
occurrences of QUIET with LOW_POWER as was done in the latest revision
of the thinkpad_acpi change.

This is not only a cosmetical thing, it also fixes an actual bug:

> +static int convert_dytc_to_profile(int dytcmode, enum platform_profile_option *profile)
> +{
> + switch (dytcmode) {
> + case DYTC_MODE_QUIET:
> + *profile = PLATFORM_PROFILE_LOW_POWER;
> + break;

QUIET -> LOW_POWER, ok there actually is a PLATFORM_PROFILE_QUIET
too, but according to the docs LOW_POWER actually is a better match
(hence the rename).

> + case DYTC_MODE_BALANCE:
> + *profile = PLATFORM_PROFILE_BALANCED;
> + break;
> + case DYTC_MODE_PERFORM:
> + *profile = PLATFORM_PROFILE_PERFORMANCE;
> + break;
> + default: /* Unknown mode */
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static int convert_profile_to_dytc(enum platform_profile_option profile, int *perfmode)
> +{
> + switch (profile) {
> + case PLATFORM_PROFILE_QUIET:
> + *perfmode = DYTC_MODE_QUIET;

QUIET -> QUIET erm, but we weren't using PLATFORM_PROFILE_QUIET in this driver at all, also see:

> + /* Setup supported modes */
> + set_bit(PLATFORM_PROFILE_LOW_POWER, priv->dytc->pprof.choices);
> + set_bit(PLATFORM_PROFILE_BALANCED, priv->dytc->pprof.choices);
> + set_bit(PLATFORM_PROFILE_PERFORMANCE, priv->dytc->pprof.choices);

See we are not advertising PLATFORM_PROFILE_LOW_POWER and above
we actually map DYTC_MODE_QUIET -> PLATFORM_PROFILE_LOW_POWER

So we should do the reverse here.

Anyways replacing all instances of QUIET with LOW_POWER clears up
the confusion which lead to this bug and also actually fixes the bug.

> + break;
> + case PLATFORM_PROFILE_BALANCED:
> + *perfmode = DYTC_MODE_BALANCE;
> + break;
> + case PLATFORM_PROFILE_PERFORMANCE:
> + *perfmode = DYTC_MODE_PERFORM;
> + break;
> + default: /* Unknown profile */
> + return -EOPNOTSUPP;
> + }
> + return 0;
> +}

Thank you for your patch, I've applied this patch to my review-hans
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans

> ---
> drivers/platform/x86/Kconfig | 1 +
> drivers/platform/x86/ideapad-laptop.c | 289 ++++++++++++++++++++++++++
> 2 files changed, 290 insertions(+)
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 91e6176cdfbd..8059143c21bb 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -624,6 +624,7 @@ config IDEAPAD_LAPTOP
> depends on BACKLIGHT_CLASS_DEVICE
> depends on ACPI_VIDEO || ACPI_VIDEO = n
> depends on ACPI_WMI || ACPI_WMI = n
> + depends on ACPI_PLATFORM_PROFILE
> select INPUT_SPARSEKMAP
> help
> This is a driver for Lenovo IdeaPad netbooks contains drivers for
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index 7598cd46cf60..5512367c604a 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -15,6 +15,7 @@
> #include <linux/acpi.h>
> #include <linux/rfkill.h>
> #include <linux/platform_device.h>
> +#include <linux/platform_profile.h>
> #include <linux/input.h>
> #include <linux/input/sparse-keymap.h>
> #include <linux/backlight.h>
> @@ -77,6 +78,13 @@ enum {
> VPCCMD_W_BL_POWER = 0x33,
> };
>
> +struct ideapad_dytc_priv {
> + enum platform_profile_option current_profile;
> + struct platform_profile_handler pprof;
> + struct mutex mutex;
> + struct ideapad_private *priv;
> +};
> +
> struct ideapad_rfk_priv {
> int dev;
> struct ideapad_private *priv;
> @@ -89,6 +97,7 @@ struct ideapad_private {
> struct platform_device *platform_device;
> struct input_dev *inputdev;
> struct backlight_device *blightdev;
> + struct ideapad_dytc_priv *dytc;
> struct dentry *debug;
> unsigned long cfg;
> bool has_hw_rfkill_switch;
> @@ -136,6 +145,28 @@ static int method_int1(acpi_handle handle, char *method, int cmd)
> return ACPI_FAILURE(status) ? -1 : 0;
> }
>
> +static int method_dytc(acpi_handle handle, int cmd, int *ret)
> +{
> + acpi_status status;
> + unsigned long long result;
> + struct acpi_object_list params;
> + union acpi_object in_obj;
> +
> + params.count = 1;
> + params.pointer = &in_obj;
> + in_obj.type = ACPI_TYPE_INTEGER;
> + in_obj.integer.value = cmd;
> +
> + status = acpi_evaluate_integer(handle, "DYTC", &params, &result);
> +
> + if (ACPI_FAILURE(status)) {
> + *ret = -1;
> + return -1;
> + }
> + *ret = result;
> + return 0;
> +}
> +
> static int method_vpcr(acpi_handle handle, int cmd, int *ret)
> {
> acpi_status status;
> @@ -546,6 +577,257 @@ static const struct attribute_group ideapad_attribute_group = {
> .attrs = ideapad_attributes
> };
>
> +/*
> + * DYTC Platform profile
> + */
> +#define DYTC_CMD_QUERY 0 /* To get DYTC status - enable/revision */
> +#define DYTC_CMD_SET 1 /* To enable/disable IC function mode */
> +#define DYTC_CMD_GET 2 /* To get current IC function and mode */
> +#define DYTC_CMD_RESET 0x1ff /* To reset back to default */
> +
> +#define DYTC_QUERY_ENABLE_BIT 8 /* Bit 8 - 0 = disabled, 1 = enabled */
> +#define DYTC_QUERY_SUBREV_BIT 16 /* Bits 16 - 27 - sub revision */
> +#define DYTC_QUERY_REV_BIT 28 /* Bits 28 - 31 - revision */
> +
> +#define DYTC_GET_FUNCTION_BIT 8 /* Bits 8-11 - function setting */
> +#define DYTC_GET_MODE_BIT 12 /* Bits 12-15 - mode setting */
> +
> +#define DYTC_SET_FUNCTION_BIT 12 /* Bits 12-15 - function setting */
> +#define DYTC_SET_MODE_BIT 16 /* Bits 16-19 - mode setting */
> +#define DYTC_SET_VALID_BIT 20 /* Bit 20 - 1 = on, 0 = off */
> +
> +#define DYTC_FUNCTION_STD 0 /* Function = 0, standard mode */
> +#define DYTC_FUNCTION_CQL 1 /* Function = 1, lap mode */
> +#define DYTC_FUNCTION_MMC 11 /* Function = 11, desk mode */
> +
> +#define DYTC_MODE_PERFORM 2 /* High power mode aka performance */
> +#define DYTC_MODE_QUIET 3 /* Low power mode aka quiet */
> +#define DYTC_MODE_BALANCE 0xF /* Default mode aka balanced */
> +
> +#define DYTC_SET_COMMAND(function, mode, on) \
> + (DYTC_CMD_SET | (function) << DYTC_SET_FUNCTION_BIT | \
> + (mode) << DYTC_SET_MODE_BIT | \
> + (on) << DYTC_SET_VALID_BIT)
> +
> +#define DYTC_DISABLE_CQL DYTC_SET_COMMAND(DYTC_FUNCTION_CQL, DYTC_MODE_BALANCE, 0)
> +
> +#define DYTC_ENABLE_CQL DYTC_SET_COMMAND(DYTC_FUNCTION_CQL, DYTC_MODE_BALANCE, 1)
> +
> +static int convert_dytc_to_profile(int dytcmode, enum platform_profile_option *profile)
> +{
> + switch (dytcmode) {
> + case DYTC_MODE_QUIET:
> + *profile = PLATFORM_PROFILE_LOW_POWER;
> + break;
> + case DYTC_MODE_BALANCE:
> + *profile = PLATFORM_PROFILE_BALANCED;
> + break;
> + case DYTC_MODE_PERFORM:
> + *profile = PLATFORM_PROFILE_PERFORMANCE;
> + break;
> + default: /* Unknown mode */
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static int convert_profile_to_dytc(enum platform_profile_option profile, int *perfmode)
> +{
> + switch (profile) {
> + case PLATFORM_PROFILE_QUIET:
> + *perfmode = DYTC_MODE_QUIET;
> + break;
> + case PLATFORM_PROFILE_BALANCED:
> + *perfmode = DYTC_MODE_BALANCE;
> + break;
> + case PLATFORM_PROFILE_PERFORMANCE:
> + *perfmode = DYTC_MODE_PERFORM;
> + break;
> + default: /* Unknown profile */
> + return -EOPNOTSUPP;
> + }
> + return 0;
> +}
> +
> +/*
> + * dytc_profile_get: Function to register with platform_profile
> + * handler. Returns current platform profile.
> + */
> +int dytc_profile_get(struct platform_profile_handler *pprof,
> + enum platform_profile_option *profile)
> +{
> + struct ideapad_dytc_priv *dytc;
> +
> + dytc = container_of(pprof, struct ideapad_dytc_priv, pprof);
> + *profile = dytc->current_profile;
> + return 0;
> +}
> +
> +/*
> + * Helper function - check if we are in CQL mode and if we are
> + * - disable CQL,
> + * - run the command
> + * - enable CQL
> + * If not in CQL mode, just run the command
> + */
> +int dytc_cql_command(struct ideapad_private *priv, int command, int *output)
> +{
> + int err, cmd_err, dummy;
> + int cur_funcmode;
> +
> + /* Determine if we are in CQL mode. This alters the commands we do */
> + err = method_dytc(priv->adev->handle, DYTC_CMD_GET, output);
> + if (err)
> + return err;
> +
> + cur_funcmode = (*output >> DYTC_GET_FUNCTION_BIT) & 0xF;
> + /* Check if we're OK to return immediately */
> + if ((command == DYTC_CMD_GET) && (cur_funcmode != DYTC_FUNCTION_CQL))
> + return 0;
> +
> + if (cur_funcmode == DYTC_FUNCTION_CQL) {
> + err = method_dytc(priv->adev->handle, DYTC_DISABLE_CQL, &dummy);
> + if (err)
> + return err;
> + }
> +
> + cmd_err = method_dytc(priv->adev->handle, command, output);
> + /* Check return condition after we've restored CQL state */
> +
> + if (cur_funcmode == DYTC_FUNCTION_CQL) {
> + err = method_dytc(priv->adev->handle, DYTC_ENABLE_CQL, &dummy);
> + if (err)
> + return err;
> + }
> +
> + return cmd_err;
> +}
> +
> +/*
> + * dytc_profile_set: Function to register with platform_profile
> + * handler. Sets current platform profile.
> + */
> +int dytc_profile_set(struct platform_profile_handler *pprof,
> + enum platform_profile_option profile)
> +{
> + struct ideapad_dytc_priv *dytc;
> + struct ideapad_private *priv;
> + int output;
> + int err;
> +
> + dytc = container_of(pprof, struct ideapad_dytc_priv, pprof);
> + priv = dytc->priv;
> +
> + err = mutex_lock_interruptible(&dytc->mutex);
> + if (err)
> + return err;
> +
> + if (profile == PLATFORM_PROFILE_BALANCED) {
> + /* To get back to balanced mode we just issue a reset command */
> + err = method_dytc(priv->adev->handle, DYTC_CMD_RESET, &output);
> + if (err)
> + goto unlock;
> + } else {
> + int perfmode;
> +
> + err = convert_profile_to_dytc(profile, &perfmode);
> + if (err)
> + goto unlock;
> +
> + /* Determine if we are in CQL mode. This alters the commands we do */
> + err = dytc_cql_command(priv,
> + DYTC_SET_COMMAND(DYTC_FUNCTION_MMC, perfmode, 1),
> + &output);
> + if (err)
> + goto unlock;
> + }
> + /* Success - update current profile */
> + dytc->current_profile = profile;
> +unlock:
> + mutex_unlock(&dytc->mutex);
> + return err;
> +}
> +
> +static void dytc_profile_refresh(struct ideapad_private *priv)
> +{
> + enum platform_profile_option profile;
> + int output, err;
> + int perfmode;
> +
> + mutex_lock(&priv->dytc->mutex);
> + err = dytc_cql_command(priv, DYTC_CMD_GET, &output);
> + mutex_unlock(&priv->dytc->mutex);
> + if (err)
> + return;
> +
> + perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF;
> + convert_dytc_to_profile(perfmode, &profile);
> + if (profile != priv->dytc->current_profile) {
> + priv->dytc->current_profile = profile;
> + platform_profile_notify();
> + }
> +}
> +
> +static int ideapad_dytc_profile_init(struct ideapad_private *priv)
> +{
> + int err, output, dytc_version;
> +
> + err = method_dytc(priv->adev->handle, DYTC_CMD_QUERY, &output);
> + /* For all other errors we can flag the failure */
> + if (err)
> + return err;
> +
> + /* Check DYTC is enabled and supports mode setting */
> + if (!(output & BIT(DYTC_QUERY_ENABLE_BIT)))
> + return -ENODEV;
> +
> + dytc_version = (output >> DYTC_QUERY_REV_BIT) & 0xF;
> + if (dytc_version < 5)
> + return -ENODEV;
> +
> + priv->dytc = kzalloc(sizeof(struct ideapad_dytc_priv), GFP_KERNEL);
> + if (!priv->dytc)
> + return -ENOMEM;
> +
> + mutex_init(&priv->dytc->mutex);
> +
> + priv->dytc->priv = priv;
> + priv->dytc->pprof.profile_get = dytc_profile_get;
> + priv->dytc->pprof.profile_set = dytc_profile_set;
> +
> + /* Setup supported modes */
> + set_bit(PLATFORM_PROFILE_LOW_POWER, priv->dytc->pprof.choices);
> + set_bit(PLATFORM_PROFILE_BALANCED, priv->dytc->pprof.choices);
> + set_bit(PLATFORM_PROFILE_PERFORMANCE, priv->dytc->pprof.choices);
> +
> + /* Create platform_profile structure and register */
> + err = platform_profile_register(&priv->dytc->pprof);
> + if (err)
> + goto mutex_destroy;
> +
> + /* Ensure initial values are correct */
> + dytc_profile_refresh(priv);
> +
> + return 0;
> +
> +mutex_destroy:
> + mutex_destroy(&priv->dytc->mutex);
> + kfree(priv->dytc);
> + priv->dytc = NULL;
> + return err;
> +}
> +
> +static void ideapad_dytc_profile_exit(struct ideapad_private *priv)
> +{
> + if (!priv->dytc)
> + return;
> +
> + platform_profile_remove();
> + mutex_destroy(&priv->dytc->mutex);
> + kfree(priv->dytc);
> + priv->dytc = NULL;
> +}
> +
> /*
> * Rfkill
> */
> @@ -1013,6 +1295,8 @@ static int ideapad_acpi_add(struct platform_device *pdev)
> ideapad_sync_rfk_state(priv);
> ideapad_sync_touchpad_state(priv);
>
> + ideapad_dytc_profile_init(priv);
> +
> if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
> ret = ideapad_backlight_init(priv);
> if (ret && ret != -ENODEV)
> @@ -1066,6 +1350,7 @@ static int ideapad_acpi_remove(struct platform_device *pdev)
> acpi_remove_notify_handler(priv->adev->handle,
> ACPI_DEVICE_NOTIFY, ideapad_acpi_notify);
> ideapad_backlight_exit(priv);
> + ideapad_dytc_profile_exit(priv);
> for (i = 0; i < IDEAPAD_RFKILL_DEV_NUM; i++)
> ideapad_unregister_rfkill(priv, i);
> ideapad_input_exit(priv);
> @@ -1087,6 +1372,10 @@ static int ideapad_acpi_resume(struct device *device)
>
> ideapad_sync_rfk_state(priv);
> ideapad_sync_touchpad_state(priv);
> +
> + if (priv->dytc)
> + dytc_profile_refresh(priv);
> +
> return 0;
> }
> #endif
>

Next message: Greg KH: "Re: [PATCH v2] platform/chrome: cros_ec_typec: Fix call to typec_partner_set_pd_revision"
Previous message: Peter Zijlstra: "Re: kprobes broken since 0d00449c7a28 ("x86: Replace ist_enter() with nmi_enter()")"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]