[PATCH 0/2] IdeaPad platform profile support

[PATCH 0/2] IdeaPad platform profile support

[Jiaxun Yang]

Tested on Lenovo Yoga-14SARE Chinese Edition.

Jiaxun Yang (2):
  ACPI: platform-profile: Introduce data parameter to handler
  platform/x86: ideapad-laptop: DYTC Platform profile support

 drivers/acpi/platform_profile.c       |   4 +-
 drivers/platform/x86/Kconfig          |   1 +
 drivers/platform/x86/ideapad-laptop.c | 281 ++++++++++++++++++++++++++
 include/linux/platform_profile.h      |   5 +-
 4 files changed, 287 insertions(+), 4 deletions(-)

-- 
2.30.0

[PATCH 1/2] ACPI: platform-profile: Introduce data parameter to handler

[Jiaxun Yang]

Add a data parameter to handler callbacks to avoid having
global variables everywhere.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 drivers/acpi/platform_profile.c  | 4 ++--
 include/linux/platform_profile.h | 5 +++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 91be50a32cc8..60072f6e032d 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -64,7 +64,7 @@ static ssize_t platform_profile_show(struct device *dev,
 		return -ENODEV;
 	}
 
-	err = cur_profile->profile_get(&profile);
+	err = cur_profile->profile_get(cur_profile->data, &profile);
 	mutex_unlock(&profile_lock);
 	if (err)
 		return err;
@@ -104,7 +104,7 @@ static ssize_t platform_profile_store(struct device *dev,
 		return -EOPNOTSUPP;
 	}
 
-	err = cur_profile->profile_set(i);
+	err = cur_profile->profile_set(cur_profile->data, i);
 	mutex_unlock(&profile_lock);
 	if (err)
 		return err;
diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
index 3623d7108421..272faf55875d 100644
--- a/include/linux/platform_profile.h
+++ b/include/linux/platform_profile.h
@@ -27,9 +27,10 @@ enum platform_profile_option {
 };
 
 struct platform_profile_handler {
+	void *data;
 	unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
-	int (*profile_get)(enum platform_profile_option *profile);
-	int (*profile_set)(enum platform_profile_option profile);
+	int (*profile_get)(void *data, enum platform_profile_option *profile);
+	int (*profile_set)(void *data, enum platform_profile_option profile);
 };
 
 int platform_profile_register(const struct platform_profile_handler *pprof);
-- 
2.30.0

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

[Jiaxun Yang]

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@lenovo.com>'s thinkpad-acpi
work but massaged to fit ideapad driver.

Note that different from ThinkPads, IdeaPads's Thermal Hotkey won't
trigger profile wwitch itself, we'll leave it for userspace programs.

Tested on Lenovo Yoga-14SARE Chinese Edition.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 drivers/platform/x86/Kconfig          |   1 +
 drivers/platform/x86/ideapad-laptop.c | 281 ++++++++++++++++++++++++++
 2 files changed, 282 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..8e91b813e623 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,12 @@ enum {
 	VPCCMD_W_BL_POWER = 0x33,
 };
 
+struct ideapad_dytc_priv {
+	enum platform_profile_option current_profile;
+	struct platform_profile_handler handler;
+	struct mutex mutex;
+};
+
 struct ideapad_rfk_priv {
 	int dev;
 	struct ideapad_private *priv;
@@ -89,6 +96,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 +144,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 +576,250 @@ 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(void *data, enum platform_profile_option *profile)
+{
+	struct ideapad_private *priv = data;
+
+	*profile = priv->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(void *data, enum platform_profile_option profile)
+{
+	struct ideapad_private *priv = data;
+	int output;
+	int err;
+
+	err = mutex_lock_interruptible(&priv->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 */
+	priv->dytc->current_profile = profile;
+unlock:
+	mutex_unlock(&priv->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->handler.data = priv;
+	priv->dytc->handler.profile_get = dytc_profile_get;
+	priv->dytc->handler.profile_set = dytc_profile_set;
+
+	/* Setup supported modes */
+	set_bit(PLATFORM_PROFILE_LOW_POWER, priv->dytc->handler.choices);
+	set_bit(PLATFORM_PROFILE_BALANCED, priv->dytc->handler.choices);
+	set_bit(PLATFORM_PROFILE_PERFORMANCE, priv->dytc->handler.choices);
+
+	/* Create platform_profile structure and register */
+	err = platform_profile_register(&priv->dytc->handler);
+	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 +1287,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 +1342,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 +1364,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
-- 
2.30.0

Re: [PATCH 1/2] ACPI: platform-profile: Introduce data parameter to handler

[Hans de Goede]

Hi,

On 1/1/21 1:56 PM, Jiaxun Yang wrote:
> Add a data parameter to handler callbacks to avoid having
> global variables everywhere.
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  drivers/acpi/platform_profile.c  | 4 ++--
>  include/linux/platform_profile.h | 5 +++--
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 91be50a32cc8..60072f6e032d 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -64,7 +64,7 @@ static ssize_t platform_profile_show(struct device *dev,
>  		return -ENODEV;
>  	}
>  
> -	err = cur_profile->profile_get(&profile);
> +	err = cur_profile->profile_get(cur_profile->data, &profile);
>  	mutex_unlock(&profile_lock);
>  	if (err)
>  		return err;
> @@ -104,7 +104,7 @@ static ssize_t platform_profile_store(struct device *dev,
>  		return -EOPNOTSUPP;
>  	}
>  
> -	err = cur_profile->profile_set(i);
> +	err = cur_profile->profile_set(cur_profile->data, i);
>  	mutex_unlock(&profile_lock);
>  	if (err)
>  		return err;
> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
> index 3623d7108421..272faf55875d 100644
> --- a/include/linux/platform_profile.h
> +++ b/include/linux/platform_profile.h
> @@ -27,9 +27,10 @@ enum platform_profile_option {
>  };
>  
>  struct platform_profile_handler {
> +	void *data;
>  	unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> -	int (*profile_get)(enum platform_profile_option *profile);
> -	int (*profile_set)(enum platform_profile_option profile);
> +	int (*profile_get)(void *data, enum platform_profile_option *profile);
> +	int (*profile_set)(void *data, enum platform_profile_option profile);
>  };
>  
>  int platform_profile_register(const struct platform_profile_handler *pprof);
> 

Re: [PATCH 0/2] IdeaPad platform profile support

[Hans de Goede]

Hi,

On 1/1/21 1:56 PM, Jiaxun Yang wrote:
> Tested on Lenovo Yoga-14SARE Chinese Edition.
> 
> Jiaxun Yang (2):
>   ACPI: platform-profile: Introduce data parameter to handler
>   platform/x86: ideapad-laptop: DYTC Platform profile support
> 
>  drivers/acpi/platform_profile.c       |   4 +-
>  drivers/platform/x86/Kconfig          |   1 +
>  drivers/platform/x86/ideapad-laptop.c | 281 ++++++++++++++++++++++++++
>  include/linux/platform_profile.h      |   5 +-
>  4 files changed, 287 insertions(+), 4 deletions(-)


Thank you for your series, unfortunately the
"ACPI: platform-profile: Introduce data parameter to handler"
patch causes a conflict with the pending:
"[PATCH v8 3/3] platform/x86: thinkpad_acpi: Add platform profile support"
patch.

But I do agree that adding that data parameter makes sense, so
it might be best to merge:

"ACPI: platform-profile: Introduce data parameter to handler"

First and then rebase the thinkpad_acpi patch on top.

Rafael, do you think you could add:

"ACPI: platform-profile: Introduce data parameter to handler"

To the 2 ACPI: platform-profile patches which you already have pending for 5.11-rc# ?

Regards,

Hans

Re: [PATCH 0/2] IdeaPad platform profile support

[Rafael J. Wysocki]

On Mon, Jan 4, 2021 at 3:36 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 1/1/21 1:56 PM, Jiaxun Yang wrote:
> > Tested on Lenovo Yoga-14SARE Chinese Edition.
> >
> > Jiaxun Yang (2):
> >   ACPI: platform-profile: Introduce data parameter to handler
> >   platform/x86: ideapad-laptop: DYTC Platform profile support
> >
> >  drivers/acpi/platform_profile.c       |   4 +-
> >  drivers/platform/x86/Kconfig          |   1 +
> >  drivers/platform/x86/ideapad-laptop.c | 281 ++++++++++++++++++++++++++
> >  include/linux/platform_profile.h      |   5 +-
> >  4 files changed, 287 insertions(+), 4 deletions(-)
>
>
> Thank you for your series, unfortunately the
> "ACPI: platform-profile: Introduce data parameter to handler"
> patch causes a conflict with the pending:
> "[PATCH v8 3/3] platform/x86: thinkpad_acpi: Add platform profile support"
> patch.
>
> But I do agree that adding that data parameter makes sense, so
> it might be best to merge:
>
> "ACPI: platform-profile: Introduce data parameter to handler"
>
> First and then rebase the thinkpad_acpi patch on top.
>
> Rafael, do you think you could add:
>
> "ACPI: platform-profile: Introduce data parameter to handler"
>
> To the 2 ACPI: platform-profile patches which you already have pending for 5.11-rc# ?

I'm not sure why that patch is needed at all, because whoever
registers a platform profile handler needs to have access to the
original handler object anyway.

Also, on a somewhat related note, I'm afraid that it may not be a good
idea to push this series for 5.11-rc in the face of recent objections
against new material going in after the merge window.

Cheers!

Re: [PATCH 0/2] IdeaPad platform profile support

[Hans de Goede]

Hi,

On 1/4/21 9:33 PM, Rafael J. Wysocki wrote:
> On Mon, Jan 4, 2021 at 3:36 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 1/1/21 1:56 PM, Jiaxun Yang wrote:
>>> Tested on Lenovo Yoga-14SARE Chinese Edition.
>>>
>>> Jiaxun Yang (2):
>>>   ACPI: platform-profile: Introduce data parameter to handler
>>>   platform/x86: ideapad-laptop: DYTC Platform profile support
>>>
>>>  drivers/acpi/platform_profile.c       |   4 +-
>>>  drivers/platform/x86/Kconfig          |   1 +
>>>  drivers/platform/x86/ideapad-laptop.c | 281 ++++++++++++++++++++++++++
>>>  include/linux/platform_profile.h      |   5 +-
>>>  4 files changed, 287 insertions(+), 4 deletions(-)
>>
>>
>> Thank you for your series, unfortunately the
>> "ACPI: platform-profile: Introduce data parameter to handler"
>> patch causes a conflict with the pending:
>> "[PATCH v8 3/3] platform/x86: thinkpad_acpi: Add platform profile support"
>> patch.
>>
>> But I do agree that adding that data parameter makes sense, so
>> it might be best to merge:
>>
>> "ACPI: platform-profile: Introduce data parameter to handler"
>>
>> First and then rebase the thinkpad_acpi patch on top.
>>
>> Rafael, do you think you could add:
>>
>> "ACPI: platform-profile: Introduce data parameter to handler"
>>
>> To the 2 ACPI: platform-profile patches which you already have pending for 5.11-rc# ?
> 
> I'm not sure why that patch is needed at all, because whoever
> registers a platform profile handler needs to have access to the
> original handler object anyway.

True, I was actually thinking that instead of the data argument, we might
pass a pointer to the original handler object like this:

@@ -64,7 +64,7 @@ static ssize_t platform_profile_show(struct device *dev,
 		return -ENODEV;
 	}
 
-	err = cur_profile->profile_get(&profile);
+	err = cur_profile->profile_get(cur_profile, &profile);
 	mutex_unlock(&profile_lock);
 	if (err)
 		return err;

And then the driver which has registered the cur_profile, can get to
its own data by using container of on the cur_profile pointer.

With the code currently in your bleeding-edge branch, there is no way
for any driver-code to get to its own (possibly/likely dynamically
allocated) driver-data struct.

E.g. a typical driver using only dynamic data tied to device_get_drvdata,
might have this:

struct driver_data {
	...
	struct platform_profile_handler profile_handler;
	...
};

int probe(...) {
	struct driver_data *my_data;

	my_data = devm_kzalloc(dev, sizeof(*my_data), GFP_KERNEL);

	...

	ret = platform_profile_register(&my_data->profile_handler);
	...
}

And with the change which I suggest above would then be able to
get the struct driver_data *my_data back from the profile_get callback by
using container_of on the struct platform_profile_handler *profile_handler
argument added to the profile_get callback.

I know that the platform_profile stuff is intended to only have a
single provider, so this could use global variables, but some
drivers which may be a provider use 0 global variables (other then
module_params) atm and it would be a lot cleaner from the pov
of the design of these drivers to be able to do something like the
pseudo code above. Which is why I added my Reviewed-by to patch 1/2
of the series from this thread.

Patch 1/2 does use a slightly different approach then I suggest above,
thinking more about this it would be cleaner IMHO to just pass the
cur_profile pointer to the callbacks as the pseudo-code patch which I
wrote above does. Drivers which use globals can then just ignore
the extra argument (and keep the platform_profile_handler struct const)
where as drivers which use dynamic allocation can embed the struct in
their driver's data-struct.

> Also, on a somewhat related note, I'm afraid that it may not be a good
> idea to push this series for 5.11-rc in the face of recent objections
> against new material going in after the merge window.

That is fine with me, since this did not make rc1 (nor rc2) I'm not entirely
comfortable with sending out a late pull-req for the pdx86 side of this
either, so lets postpone this to 5.12 (sorry Mark).

Rafael, once we have the discussion with the passing a pointer back to
the drivers data thing resolved (and a patch merged for that if we go
that route) can you provide me with an immutable branch to merge into
pdx86/for-next so that I can then merge the pdx86 bits on top ?

Note this does not need to be done right now around say rc4 would be fine,
so that we have some time for the patches currently in bleeding-edge to
settle a bit.

Regards,

Hans

Re: [External] Re: [PATCH 0/2] IdeaPad platform profile support

[Mark Pearson]

On 04/01/2021 15:58, Hans de Goede wrote:
> Hi,
> 
> On 1/4/21 9:33 PM, Rafael J. Wysocki wrote:
>> On Mon, Jan 4, 2021 at 3:36 PM Hans de Goede <hdegoede@redhat.com>
>>  wrote:
>>> 
>>> Hi,
>>> 
>>> On 1/1/21 1:56 PM, Jiaxun Yang wrote:
>>>> Tested on Lenovo Yoga-14SARE Chinese Edition.
>>>> 
<snip>
> 
>> Also, on a somewhat related note, I'm afraid that it may not be a 
>> good idea to push this series for 5.11-rc in the face of recent 
>> objections against new material going in after the merge window.
> 
> That is fine with me, since this did not make rc1 (nor rc2) I'm not 
> entirely comfortable with sending out a late pull-req for the pdx86 
> side of this either, so lets postpone this to 5.12 (sorry Mark).
It is what it is.

> 
> Rafael, once we have the discussion with the passing a pointer back 
> to the drivers data thing resolved (and a patch merged for that if we
> go that route) can you provide me with an immutable branch to merge
> into pdx86/for-next so that I can then merge the pdx86 bits on top ?
> 
> Note this does not need to be done right now around say rc4 would be
>  fine, so that we have some time for the patches currently in 
> bleeding-edge to settle a bit.
> 
Just for my understanding of what happens next....please correct me if I
have anything wrong:

 - platform_profile gets pulled from ACPI for 5.11

 - platform_profile gets updated to add this data/pointer implementation
and goes into 5.12. Jiaxun, let me know if you're happy with following
up on that based on Hans suggestions, If you are pushed for time let me
know and I'll happily help out/implement/test as required. I sadly don't
have any ideapads but very happy to support your efforts any way I can.

 - Can we get the x86 portion done at the same time or does that end up
going to 5.13? I had been looking at the ideapad_laptop.c patch and have
some concerns there as Jiaxun's patch is essentially a duplicate of what
I implemented in thinkpad_acpi.c which doesn't seem to be ideal
(especially as there is a V6 version of DYTC coming out this year). I
haven't had time to look at code to consider better alternatives though...

Mark

Re: [External] Re: [PATCH 0/2] IdeaPad platform profile support

[Jiaxun Yang]

在 2021/1/5 上午5:58, Mark Pearson 写道:
> On 04/01/2021 15:58, Hans de Goede wrote:
>> Hi,
>>
>> On 1/4/21 9:33 PM, Rafael J. Wysocki wrote:
>>> On Mon, Jan 4, 2021 at 3:36 PM Hans de Goede <hdegoede@redhat.com>
>>>   wrote:
>>>> Hi,
>>>>
>>>> On 1/1/21 1:56 PM, Jiaxun Yang wrote:
>>>>> Tested on Lenovo Yoga-14SARE Chinese Edition.
>>>>>
[...]
> Just for my understanding of what happens next....please correct me if I
> have anything wrong:
>
>   - platform_profile gets pulled from ACPI for 5.11
>
>   - platform_profile gets updated to add this data/pointer implementation
> and goes into 5.12.
Hi all,

Another approach could be just let all the patch go through pdx86 tree 
and with
pointer part acked by Rafael as it's unlikely to have merge conflicts.

> Jiaxun, let me know if you're happy with following
> up on that based on Hans suggestions, If you are pushed for time let me
> know and I'll happily help out/implement/test as required. I sadly don't
> have any ideapads but very happy to support your efforts any way I can.


I'm happy with Hans suggestion, will send v2 for it later.

I've been ask Lenovo engineers about DYTC and other ideapad ACPI
stuff on Lenovo China forum[1], but moderator here told me Lenovo won't
invest any Linux effort on their consumer product line :-(

Is it possible to publish a DYTC specification or documents to help us 
further
understand these mechanisms?

I'm tired of reading disassembly AML and code to figure out these internals.

>
>   - Can we get the x86 portion done at the same time or does that end up
> going to 5.13? I had been looking at the ideapad_laptop.c patch and have
> some concerns there as Jiaxun's patch is essentially a duplicate of what
> I implemented in thinkpad_acpi.c which doesn't seem to be ideal
> (especially as there is a V6 version of DYTC coming out this year). I
> haven't had time to look at code to consider better alternatives though...

It may be worthy to share these code but I'm comfort to have this 
duplication as I'm
unsure about the future of DYTC. Will DYTC for thinkpads always coherent 
with DYTC
for ideapads?

Thanks.

[1]: https://club.lenovo.com.cn/thread-5980431-1-1.html

- Jiaxun

>
> Mark

Re: [External] Re: [PATCH 0/2] IdeaPad platform profile support

[Hans de Goede]

Hi,

On 1/4/21 10:58 PM, Mark Pearson wrote:
> On 04/01/2021 15:58, Hans de Goede wrote:
>> Hi,
>>
>> On 1/4/21 9:33 PM, Rafael J. Wysocki wrote:
>>> On Mon, Jan 4, 2021 at 3:36 PM Hans de Goede <hdegoede@redhat.com>
>>>  wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 1/1/21 1:56 PM, Jiaxun Yang wrote:
>>>>> Tested on Lenovo Yoga-14SARE Chinese Edition.
>>>>>
> <snip>
>>
>>> Also, on a somewhat related note, I'm afraid that it may not be a 
>>> good idea to push this series for 5.11-rc in the face of recent 
>>> objections against new material going in after the merge window.
>>
>> That is fine with me, since this did not make rc1 (nor rc2) I'm not 
>> entirely comfortable with sending out a late pull-req for the pdx86 
>> side of this either, so lets postpone this to 5.12 (sorry Mark).
> It is what it is.
> 
>>
>> Rafael, once we have the discussion with the passing a pointer back 
>> to the drivers data thing resolved (and a patch merged for that if we
>> go that route) can you provide me with an immutable branch to merge
>> into pdx86/for-next so that I can then merge the pdx86 bits on top ?
>>
>> Note this does not need to be done right now around say rc4 would be
>>  fine, so that we have some time for the patches currently in 
>> bleeding-edge to settle a bit.
>>
> Just for my understanding of what happens next....please correct me if I
> have anything wrong:
> 
>  - platform_profile gets pulled from ACPI for 5.11
> 
>  - platform_profile gets updated to add this data/pointer implementation
> and goes into 5.12. Jiaxun, let me know if you're happy with following
> up on that based on Hans suggestions, If you are pushed for time let me
> know and I'll happily help out/implement/test as required. I sadly don't
> have any ideapads but very happy to support your efforts any way I can.
> 
>  - Can we get the x86 portion done at the same time or does that end up
> going to 5.13?

No, the plan is to get it all in 5.12. This is why I asked Rafael for
an immutable branch with the ACPI bits, then I can merge that branch
into pdx86/for-next and then apply the thinkpad and ideapad patches on
top, all for 5.12 .

Regards,

Hans

Re: [External] Re: [PATCH 0/2] IdeaPad platform profile support

[Mark Pearson]

Hi Jiaxun,

On 05/01/2021 01:24, Jiaxun Yang wrote:
> 在 2021/1/5 上午5:58, Mark Pearson 写道:
>> On 04/01/2021 15:58, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 1/4/21 9:33 PM, Rafael J. Wysocki wrote:
>>>> On Mon, Jan 4, 2021 at 3:36 PM Hans de Goede <hdegoede@redhat.com>
>>>>   wrote:
>>>>> Hi,
>>>>>
>>>>> On 1/1/21 1:56 PM, Jiaxun Yang wrote:
>>>>>> Tested on Lenovo Yoga-14SARE Chinese Edition.
>>>>>>
> [...]
>> Just for my understanding of what happens next....please correct me if I
>> have anything wrong:
>>
>>   - platform_profile gets pulled from ACPI for 5.11
>>
>>   - platform_profile gets updated to add this data/pointer implementation
>> and goes into 5.12.
> Hi all,
> 
> Another approach could be just let all the patch go through pdx86 tree
> and with
> pointer part acked by Rafael as it's unlikely to have merge conflicts.
> 
>> Jiaxun, let me know if you're happy with following
>> up on that based on Hans suggestions, If you are pushed for time let me
>> know and I'll happily help out/implement/test as required. I sadly don't
>> have any ideapads but very happy to support your efforts any way I can.
> 
> 
> I'm happy with Hans suggestion, will send v2 for it later.
> 
> I've been ask Lenovo engineers about DYTC and other ideapad ACPI
> stuff on Lenovo China forum[1], but moderator here told me Lenovo won't
> invest any Linux effort on their consumer product line :-(
> 
> Is it possible to publish a DYTC specification or documents to help us
> further
> understand these mechanisms?
> 
> I'm tired of reading disassembly AML and code to figure out these
> internals.
> 
I hear you :)

Afraid I'm not allowed to publish the full DYTC spec - but I make public
the bits that I can.

I don't have many hooks into the ideapad team as it's not in the Linux
plan - so I can't answer your questions with confidence. I am going to
ask the firmware team if they can confirm if ideapad is using the same
spec - it would make sense if they are.
Feel free to bug me off mailing list and I'll happily help out with any
debugging issues.

I'm also not allowed to confirm or deny future plans but my personal
hope is that the Linux project in Lenovo grows. Fingers crossed for the
future.

>>
>>   - Can we get the x86 portion done at the same time or does that end up
>> going to 5.13? I had been looking at the ideapad_laptop.c patch and have
>> some concerns there as Jiaxun's patch is essentially a duplicate of what
>> I implemented in thinkpad_acpi.c which doesn't seem to be ideal
>> (especially as there is a V6 version of DYTC coming out this year). I
>> haven't had time to look at code to consider better alternatives
>> though...
> 
> It may be worthy to share these code but I'm comfort to have this
> duplication as I'm
> unsure about the future of DYTC. Will DYTC for thinkpads always coherent
> with DYTC
> for ideapads?
I'll see if I can find out.

> 
> Thanks.
> 
> [1]: https://club.lenovo.com.cn/thread-5980431-1-1.html
> 
> - Jiaxun
> 
>>
>> Mark
> 

Re: [PATCH 0/2] IdeaPad platform profile support

[Rafael J. Wysocki]

On Mon, Jan 4, 2021 at 9:58 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 1/4/21 9:33 PM, Rafael J. Wysocki wrote:
> > On Mon, Jan 4, 2021 at 3:36 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi,
> >>
> >> On 1/1/21 1:56 PM, Jiaxun Yang wrote:
> >>> Tested on Lenovo Yoga-14SARE Chinese Edition.
> >>>
> >>> Jiaxun Yang (2):
> >>>   ACPI: platform-profile: Introduce data parameter to handler
> >>>   platform/x86: ideapad-laptop: DYTC Platform profile support
> >>>
> >>>  drivers/acpi/platform_profile.c       |   4 +-
> >>>  drivers/platform/x86/Kconfig          |   1 +
> >>>  drivers/platform/x86/ideapad-laptop.c | 281 ++++++++++++++++++++++++++
> >>>  include/linux/platform_profile.h      |   5 +-
> >>>  4 files changed, 287 insertions(+), 4 deletions(-)
> >>
> >>
> >> Thank you for your series, unfortunately the
> >> "ACPI: platform-profile: Introduce data parameter to handler"
> >> patch causes a conflict with the pending:
> >> "[PATCH v8 3/3] platform/x86: thinkpad_acpi: Add platform profile support"
> >> patch.
> >>
> >> But I do agree that adding that data parameter makes sense, so
> >> it might be best to merge:
> >>
> >> "ACPI: platform-profile: Introduce data parameter to handler"
> >>
> >> First and then rebase the thinkpad_acpi patch on top.
> >>
> >> Rafael, do you think you could add:
> >>
> >> "ACPI: platform-profile: Introduce data parameter to handler"
> >>
> >> To the 2 ACPI: platform-profile patches which you already have pending for 5.11-rc# ?
> >
> > I'm not sure why that patch is needed at all, because whoever
> > registers a platform profile handler needs to have access to the
> > original handler object anyway.
>
> True, I was actually thinking that instead of the data argument, we might
> pass a pointer to the original handler object like this:
>
> @@ -64,7 +64,7 @@ static ssize_t platform_profile_show(struct device *dev,
>                 return -ENODEV;
>         }
>
> -       err = cur_profile->profile_get(&profile);
> +       err = cur_profile->profile_get(cur_profile, &profile);
>         mutex_unlock(&profile_lock);
>         if (err)
>                 return err;

I would prefer this approach.

>
> And then the driver which has registered the cur_profile, can get to
> its own data by using container of on the cur_profile pointer.
>
> With the code currently in your bleeding-edge branch, there is no way
> for any driver-code to get to its own (possibly/likely dynamically
> allocated) driver-data struct.
>
> E.g. a typical driver using only dynamic data tied to device_get_drvdata,
> might have this:
>
> struct driver_data {
>         ...
>         struct platform_profile_handler profile_handler;
>         ...
> };
>
> int probe(...) {
>         struct driver_data *my_data;
>
>         my_data = devm_kzalloc(dev, sizeof(*my_data), GFP_KERNEL);
>
>         ...
>
>         ret = platform_profile_register(&my_data->profile_handler);
>         ...
> }
>
> And with the change which I suggest above would then be able to
> get the struct driver_data *my_data back from the profile_get callback by
> using container_of on the struct platform_profile_handler *profile_handler
> argument added to the profile_get callback.

OK, fair enough.

> I know that the platform_profile stuff is intended to only have a
> single provider, so this could use global variables, but some
> drivers which may be a provider use 0 global variables (other then
> module_params) atm and it would be a lot cleaner from the pov
> of the design of these drivers to be able to do something like the
> pseudo code above. Which is why I added my Reviewed-by to patch 1/2
> of the series from this thread.
>
> Patch 1/2 does use a slightly different approach then I suggest above,
> thinking more about this it would be cleaner IMHO to just pass the
> cur_profile pointer to the callbacks as the pseudo-code patch which I
> wrote above does. Drivers which use globals can then just ignore
> the extra argument (and keep the platform_profile_handler struct const)
> where as drivers which use dynamic allocation can embed the struct in
> their driver's data-struct.

Agreed.

> > Also, on a somewhat related note, I'm afraid that it may not be a good
> > idea to push this series for 5.11-rc in the face of recent objections
> > against new material going in after the merge window.
>
> That is fine with me, since this did not make rc1 (nor rc2) I'm not entirely
> comfortable with sending out a late pull-req for the pdx86 side of this
> either, so lets postpone this to 5.12 (sorry Mark).
>
> Rafael, once we have the discussion with the passing a pointer back to
> the drivers data thing resolved (and a patch merged for that if we go
> that route) can you provide me with an immutable branch to merge into
> pdx86/for-next so that I can then merge the pdx86 bits on top ?

Sure, no problem.

> Note this does not need to be done right now around say rc4 would be fine,
> so that we have some time for the patches currently in bleeding-edge to
> settle a bit.

OK

Cheers!

Re: [PATCH 0/2] IdeaPad platform profile support

[Hans de Goede]

Hi,

On 1/5/21 6:18 PM, Rafael J. Wysocki wrote:
> On Mon, Jan 4, 2021 at 9:58 PM Hans de Goede <hdegoede@redhat.com> wrote:

<snip>

>> Patch 1/2 does use a slightly different approach then I suggest above,
>> thinking more about this it would be cleaner IMHO to just pass the
>> cur_profile pointer to the callbacks as the pseudo-code patch which I
>> wrote above does. Drivers which use globals can then just ignore
>> the extra argument (and keep the platform_profile_handler struct const)
>> where as drivers which use dynamic allocation can embed the struct in
>> their driver's data-struct.
> 
> Agreed.

Note that Jiaxun has provided a v2 of this patch-set with patch 1/2 implementing
the new approach.

Can you merge merge that patch please and then once you're happy that this
has seen enough exposure in -next, provide me with an immutable branch with
the 3 platform-profile patches in it ?

Regards,

Hans

Re: [PATCH 0/2] IdeaPad platform profile support

[Rafael J. Wysocki]

On Wed, Jan 6, 2021 at 10:17 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 1/5/21 6:18 PM, Rafael J. Wysocki wrote:
> > On Mon, Jan 4, 2021 at 9:58 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> <snip>
>
> >> Patch 1/2 does use a slightly different approach then I suggest above,
> >> thinking more about this it would be cleaner IMHO to just pass the
> >> cur_profile pointer to the callbacks as the pseudo-code patch which I
> >> wrote above does. Drivers which use globals can then just ignore
> >> the extra argument (and keep the platform_profile_handler struct const)
> >> where as drivers which use dynamic allocation can embed the struct in
> >> their driver's data-struct.
> >
> > Agreed.
>
> Note that Jiaxun has provided a v2 of this patch-set with patch 1/2 implementing
> the new approach.
>
> Can you merge merge that patch please and then once you're happy that this
> has seen enough exposure in -next, provide me with an immutable branch with
> the 3 platform-profile patches in it ?

I will, thanks!