[PATCH v2 1/3] Documentation: Add documentation for new platform_profile sysfs attribute

[PATCH v2 1/3] Documentation: Add documentation for new platform_profile sysfs attribute

[Mark Pearson]

On modern systems the platform performance, temperature, fan and other
hardware related characteristics are often dynamically configurable. The
profile is often automatically adjusted to the load by somei
automatic-mechanism (which may very well live outside the kernel).

These auto platform-adjustment mechanisms often can be configured with
one of several 'platform-profiles', with either a bias towards low-power
consumption or towards performance (and higher power consumption and
thermals).

Introduce a new platform_profile sysfs API which offers a generic API for
selecting the performance-profile of these automatic-mechanisms.

Co-developed-by: Mark Pearson <markpearson@lenovo.com>
Signed-off-by: Mark Pearson <markpearson@lenovo.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
 - updated to rst format

 .../ABI/testing/sysfs-platform_profile.rst    | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-platform_profile.rst

diff --git a/Documentation/ABI/testing/sysfs-platform_profile.rst b/Documentation/ABI/testing/sysfs-platform_profile.rst
new file mode 100644
index 000000000000..5f7b2a94409b
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform_profile.rst
@@ -0,0 +1,66 @@
+=======================================================================
+ Platform Profile Selection (e.g. /sys/firmware/acpi/platform_profile)
+=======================================================================
+
+
+On modern systems the platform performance, temperature, fan and other
+hardware related characteristics are often dynamically configurable. The
+profile is often automatically adjusted to the load by some
+automatic mechanism (which may very well live outside the kernel).
+
+These auto platform adjustment mechanisms often can be configured with
+one of several platform profiles, with either a bias towards low power
+operation or towards performance.
+
+The purpose of the platform_profile attribute is to offer a generic sysfs
+API for selecting the platform profile of these automatic mechanisms.
+
+Note that this API is only for selecting the platform profile, it is
+NOT a goal of this API to allow monitoring the resulting performance
+characteristics. Monitoring performance is best done with device/vendor
+specific tools such as e.g. turbostat.
+
+Specifically when selecting a high performance profile the actual achieved
+performance may be limited by various factors such as: the heat generated
+by other components, room temperature, free air flow at the bottom of a
+laptop, etc. It is explicitly NOT a goal of this API to let userspace know
+about any sub-optimal conditions which are impeding reaching the requested
+performance level.
+
+Since numbers on their own cannot represent the multiple variables that a
+profile will adjust (power consumption, heat generation, etc) this API
+uses strings to describe the various profiles. To make sure that userspace
+gets a consistent experience this API document defines a fixed set of
+profile names. Drivers *must* map their internal profile representation
+onto this fixed set.
+
+
+If there is no good match when mapping then a new profile name may be
+added. Drivers which wish to introduce new profile names must:
+
+ 1. Explain why the existing profile names canot be used.
+ 2. Add the new profile name, along with a clear description of the
+    expected behaviour, to the documentation.
+
+:What:        /sys/firmware/acpi/platform_profile_choices
+:Date:        October 2020
+:Contact:     Hans de Goede <hdegoede@redhat.com>
+:Description: This file contains a space-separated list of profiles supported for this device.
+
+              Drivers must use the following standard profile-names::
+
+         		 low-power:     Low power consumption
+         		 cool:          Cooler operation
+		         quiet:         Quieter operation
+		         balanced:      Balance between low power consumption and performance
+		         performance:   High performance operation
+
+              Userspace may expect drivers to offer more than one of these
+              standard profile names.
+
+:What:        /sys/firmware/acpi/platform_profile
+:Date:        October 2020
+:Contact:     Hans de Goede <hdegoede@redhat.com>
+:Description: Reading this file gives the current selected profile for this
+              device. Writing this file with one of the strings from
+              available_profiles changes the profile to the new value.
-- 
2.25.1

[PATCH v2 2/3] ACPI: platform-profile: Add platform profile support

[Mark Pearson]

This is the initial implementation of the platform-profile feature.
It provides the details discussed and outlined in the
sysfs-platform_profile document.

Many modern systems have the ability to modify the operating profile to
control aspects like fan speed, temperature and power levels. This
module provides a common sysfs interface that platform modules can register
against to control their individual profile options.

Signed-off-by: Mark Pearson <markpearson@lenovo.com>
---
Changes in v2:
 Address (hopefully) all recommendations from review including:
 - reorder includes list alphabetically
 - make globals statics and use const as required
 - change profile name scanning to use full string
 - clean up profile name lists to remove unwanted additions
 - use sysfs_emit and sysfs_emit_at appropriately (much nicer!)
 - improve error handling. Return errors to user in all cases and use
   better error codes where appropriate (ENOOPSUPP)
 - clean up sysfs output for better readability
 - formatting fixes where needed
 - improve structure and enum names to be clearer
 - remove cur_profile field from structure. It is now local to the
   actual platform driver file (patch 3 in series)
 - improve checking so if future profile options are added profile_names
   will be updated as well.
 - move CONFIG option next to ACPI_THERMAL as it seemed slightly related
 - removed MAINTAINERS update as not appropriate (note warning message
   is seen when running checkpatch)

Big thank you to reviewers for all the suggestions.

 drivers/acpi/Kconfig            |  33 ++++++
 drivers/acpi/Makefile           |   1 +
 drivers/acpi/platform_profile.c | 181 ++++++++++++++++++++++++++++++++
 3 files changed, 215 insertions(+)
 create mode 100644 drivers/acpi/platform_profile.c

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index edf1558c1105..73a99af5ec2c 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -326,6 +326,20 @@ config ACPI_THERMAL
 	  To compile this driver as a module, choose M here:
 	  the module will be called thermal.
 
+config ACPI_PLATFORM_PROFILE
+	tristate "ACPI Platform Profile Driver"
+	default y
+	help
+	  This driver adds support for platform-profiles on platforms that
+	  support it.
+	  Platform-profiles can be used to control the platform behaviour. For
+	  example whether to operate in a lower power mode, in a higher
+	  power performance mode or between the two.
+	  This driver provides the sysfs interface and is used as the registration
+	  point for platform specific drivers.
+	  Which profiles are supported is determined on a per-platform basis and
+	  should be obtained from the platform specific driver.
+
 config ACPI_CUSTOM_DSDT_FILE
 	string "Custom DSDT Table file to include"
 	default ""
@@ -538,3 +552,22 @@ config X86_PM_TIMER
 
 	  You should nearly always say Y here because many modern
 	  systems require this timer.
+
+config ACPI_PLATFORM_PROFILE
+	tristate "ACPI Platform Profile Driver"
+	default y
+	help
+	  This driver adds support for platform-profiles on platforms that
+	  support it.
+
+	  Platform-profiles can be used to control the platform behaviour. For
+	  example whether to operate in a lower power mode, in a higher
+	  power performance mode or between the two.
+
+	  This driver provides the sysfs interface and is used as the registration
+	  point for platform specific drivers.
+
+	  Which profiles are supported is determined on a per-platform basis and
+	  should be obtained from the platform specific driver.
+
+
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 44e412506317..c64a8af106c0 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -78,6 +78,7 @@ obj-$(CONFIG_ACPI_PCI_SLOT)	+= pci_slot.o
 obj-$(CONFIG_ACPI_PROCESSOR)	+= processor.o
 obj-$(CONFIG_ACPI)		+= container.o
 obj-$(CONFIG_ACPI_THERMAL)	+= thermal.o
+obj-$(CONFIG_ACPI_PLATFORM_PROFILE) 	+= platform_profile.o
 obj-$(CONFIG_ACPI_NFIT)		+= nfit/
 obj-$(CONFIG_ACPI_NUMA)		+= numa/
 obj-$(CONFIG_ACPI)		+= acpi_memhotplug.o
diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
new file mode 100644
index 000000000000..e4bbee48c0f8
--- /dev/null
+++ b/drivers/acpi/platform_profile.c
@@ -0,0 +1,181 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/* Platform profile sysfs interface */
+
+#include <linux/acpi.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/init.h>
+#include <linux/kobject.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_profile.h>
+#include <linux/printk.h>
+#include <linux/string.h>
+#include <linux/sysfs.h>
+
+static struct platform_profile_handler *cur_profile;
+static DEFINE_MUTEX(profile_lock);
+
+/* Ensure the first char of each profile is unique */
+static const char * const profile_names[] = {
+	[profile_low] = "low-power",
+	[profile_cool] = "cool",
+	[profile_quiet] = "quiet",
+	[profile_balance] = "balance",
+	[profile_perform] = "performance",
+};
+static_assert(ARRAY_SIZE(profile_names) == profile_perform+1);
+
+static ssize_t platform_profile_choices_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	int len = 0;
+	int i;
+
+	mutex_lock(&profile_lock);
+	if (!cur_profile) {
+		mutex_unlock(&profile_lock);
+		return -ENODEV;
+	}
+
+	if (!cur_profile->choices) {
+		mutex_unlock(&profile_lock);
+		return sysfs_emit(buf, "\n");
+	}
+
+	for (i = profile_low; i <= profile_perform; i++) {
+		if (cur_profile->choices & BIT(i)) {
+			if (len == 0)
+				len += sysfs_emit_at(buf, len, "%s",  profile_names[i]);
+			else
+				len += sysfs_emit_at(buf, len, " %s",  profile_names[i]);
+		}
+	}
+	len += sysfs_emit_at(buf, len, "\n");
+	mutex_unlock(&profile_lock);
+
+	return len;
+}
+
+static ssize_t platform_profile_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	enum platform_profile_option profile;
+	int err;
+
+	mutex_lock(&profile_lock);
+	if (!cur_profile) {
+		mutex_unlock(&profile_lock);
+		return -EOPNOTSUPP;
+	}
+
+	if (!cur_profile->profile_get) {
+		mutex_unlock(&profile_lock);
+		return -EOPNOTSUPP;
+	}
+
+	err = cur_profile->profile_get(&profile);
+	mutex_unlock(&profile_lock);
+	if (err < 0)
+		return err;
+
+	return sysfs_emit(buf, "%s\n", profile_names[profile]);
+}
+
+static ssize_t platform_profile_store(struct device *dev,
+			    struct device_attribute *attr,
+			    const char *buf, size_t count)
+{
+	int err, i;
+
+	mutex_lock(&profile_lock);
+	if (!cur_profile) {
+		mutex_unlock(&profile_lock);
+		return -EOPNOTSUPP;
+	}
+
+	/* Scan for a matching profile */
+	i = sysfs_match_string(profile_names, buf);
+	if (i < 0) {
+		mutex_unlock(&profile_lock);
+		return -EINVAL;
+	}
+
+	if (!cur_profile->profile_set) {
+		mutex_unlock(&profile_lock);
+		return -EOPNOTSUPP;
+	}
+
+	err = cur_profile->profile_set(i);
+	mutex_unlock(&profile_lock);
+	if (err)
+		return err;
+
+	return count;
+}
+
+static DEVICE_ATTR_RO(platform_profile_choices);
+static DEVICE_ATTR_RW(platform_profile);
+
+static struct attribute *platform_profile_attrs[] = {
+	&dev_attr_platform_profile_choices.attr,
+	&dev_attr_platform_profile.attr,
+	NULL
+};
+
+static const struct attribute_group platform_profile_group = {
+	.attrs = platform_profile_attrs
+};
+
+int platform_profile_notify(void)
+{
+	if (!cur_profile)
+		return -EOPNOTSUPP;
+	sysfs_notify(acpi_kobj, NULL, "platform_profile");
+	return 0;
+}
+EXPORT_SYMBOL_GPL(platform_profile_notify);
+
+int platform_profile_register(struct platform_profile_handler *pprof)
+{
+	mutex_lock(&profile_lock);
+	/* We can only have one active profile */
+	if (cur_profile) {
+		mutex_unlock(&profile_lock);
+		return -EEXIST;
+	}
+
+	cur_profile = pprof;
+	mutex_unlock(&profile_lock);
+	return sysfs_create_group(acpi_kobj, &platform_profile_group);
+}
+EXPORT_SYMBOL_GPL(platform_profile_register);
+
+int platform_profile_unregister(void)
+{
+	mutex_lock(&profile_lock);
+	sysfs_remove_group(acpi_kobj, &platform_profile_group);
+	cur_profile = NULL;
+	mutex_unlock(&profile_lock);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(platform_profile_unregister);
+
+static int __init platform_profile_init(void)
+{
+	return 0;
+}
+module_init(platform_profile_init);
+
+static void __exit platform_profile_exit(void)
+{
+	sysfs_remove_group(acpi_kobj, &platform_profile_group);
+	cur_profile = NULL;
+}
+module_exit(platform_profile_exit);
+
+MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>");
+MODULE_LICENSE("GPL");
-- 
2.25.1

[PATCH v2 3/3] platform/x86: thinkpad_acpi: Add platform profile support

[Mark Pearson]

Add support to thinkpad_acpi for Lenovo platforms that have DYTC
version 5 support or newer to use the platform profile feature.

This will allow users to determine and control the platform modes
between low-power, balanced operation and performance modes.

Signed-off-by: Mark Pearson <markpearson@lenovo.com>
---
Changes in v2:
 Address (hopefully) all recommendations from review including:
 - use IS_ENABLED instead of IS_DEFINED
 - update driver to work with all the fixes in platform_profile update
 - improve error handling for invalid inputs
 - move tracking of current profile mode into this driver

 drivers/platform/x86/thinkpad_acpi.c | 278 +++++++++++++++++++++++++--
 1 file changed, 261 insertions(+), 17 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 890dda284a00..13352ccdfdaf 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -72,6 +72,7 @@
 #include <linux/uaccess.h>
 #include <acpi/battery.h>
 #include <acpi/video.h>
+#include <linux/platform_profile.h>
 
 /* ThinkPad CMOS commands */
 #define TP_CMOS_VOLUME_DOWN	0
@@ -9832,10 +9833,40 @@ static struct ibm_struct lcdshadow_driver_data = {
  * DYTC subdriver, for the Lenovo lapmode feature
  */
 
+#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_GET_LAPMODE_BIT 17 /* Set when in lapmode */
+#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 revisision */
+#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_GET_LAPMODE_BIT  17 /* Bit 17 - lapmode. Set when on lap */
+
+#define DYTC_SET_FUNCTION_BIT 12 /* Bits 12-15 - funct 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 balance */
+
+#define DYTC_DISABLE_CQL ((DYTC_MODE_BALANCE << DYTC_SET_MODE_BIT) | \
+		(DYTC_FUNCTION_CQL << DYTC_SET_FUNCTION_BIT) | \
+		DYTC_CMD_SET)
+#define DYTC_ENABLE_CQL (DYTC_DISABLE_CQL | (1 << DYTC_SET_VALID_BIT))
 
 static bool dytc_lapmode;
+static bool dytc_available;
+static bool dytc_ignore_next_event;
 
 static void dytc_lapmode_notify_change(void)
 {
@@ -9889,22 +9920,188 @@ static ssize_t dytc_lapmode_show(struct device *dev,
 
 static DEVICE_ATTR_RO(dytc_lapmode);
 
-static struct attribute *dytc_attributes[] = {
-	&dev_attr_dytc_lapmode.attr,
-	NULL,
-};
+#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE)
+static struct platform_profile_handler dytc_profile;
+static enum platform_profile_option dytc_current_profile;
 
-static const struct attribute_group dytc_attr_group = {
-	.attrs = dytc_attributes,
-};
+static int convert_dytc_to_profile(int dytcmode, enum platform_profile_option *profile)
+{
+	switch (dytcmode) {
+	case DYTC_MODE_QUIET:
+		*profile = profile_low;
+		break;
+	case DYTC_MODE_BALANCE:
+		*profile =  profile_balance;
+		break;
+	case DYTC_MODE_PERFORM:
+		*profile =  profile_perform;
+		break;
+	default: /* Unknown mode */
+		return -EINVAL;
+	}
+	return 0;
+}
 
-static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
+static int convert_profile_to_dytc(enum platform_profile_option profile, int *perfmode)
+{
+	switch (profile) {
+	case profile_low:
+		*perfmode = DYTC_MODE_QUIET;
+		break;
+	case profile_balance:
+		*perfmode = DYTC_MODE_BALANCE;
+		break;
+	case profile_perform:
+		*perfmode = DYTC_MODE_PERFORM;
+		break;
+	default: /* Unknown profile */
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int dytc_perfmode_get(int *perfmode, int *funcmode)
+{
+	int output, err;
+
+	if (!dytc_available)
+		return -ENODEV;
+
+	err = dytc_command(DYTC_CMD_GET, &output);
+	if (err)
+		return err;
+
+	*funcmode = (output >> DYTC_GET_FUNCTION_BIT) & 0xF;
+	if (*funcmode == DYTC_FUNCTION_CQL) {
+		int dummy;
+		/*
+		 * We can't get the mode when in CQL mode - so we disable CQL
+		 * mode retrieve the mode and then enable it again.
+		 * As disabling/enabling CQL triggers an event we set a flag to
+		 * ignore these events. This will be cleared by the event handler
+		 */
+		dytc_ignore_next_event = true;
+		err = dytc_command(DYTC_DISABLE_CQL, &dummy);
+		if (err)
+			return err;
+		err = dytc_command(DYTC_CMD_GET, &output);
+		if (err)
+			return err;
+		/* Again ignore this event */
+		dytc_ignore_next_event = true;
+		err = dytc_command(DYTC_ENABLE_CQL, &dummy);
+		if (err)
+			return err;
+	}
+	*perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF;
+	return 0;
+}
+
+/*
+ * dytc_profile_get: Function to register with platform_profile
+ * handler. Returns current platform profile.
+ */
+int dytc_profile_get(enum platform_profile_option *profile)
 {
 	int err;
+	int funcmode, perfmode;
+
+	err = dytc_perfmode_get(&perfmode, &funcmode);
+	if (err)
+		return err;
 
-	err = dytc_lapmode_get(&dytc_lapmode);
-	/* If support isn't available (ENODEV) then don't return an error
-	 * but just don't create the sysfs group
+	/* Convert Lenovo DYTC profile to platform_profile */
+	err = convert_dytc_to_profile(perfmode, profile);
+	if (err)
+		return err;
+
+	dytc_current_profile = *profile;
+	return 0;
+}
+
+/*
+ * dytc_profile_set: Function to register with platform_profile
+ * handler. Sets current platform profile.
+ */
+int dytc_profile_set(enum platform_profile_option profile)
+{
+	int cur_perfmode, cur_funcmode;
+	int err, dytc_set;
+	int output;
+
+	if (!dytc_available)
+		return -ENODEV;
+
+	if (profile == profile_balance) {
+		/* To get back to balance mode we just issue a reset command */
+		err = dytc_command(DYTC_CMD_RESET, &output);
+		if (err)
+			return err;
+	} else {
+		int perfmode;
+		int err;
+
+		err = convert_profile_to_dytc(profile, &perfmode);
+		if (err)
+			return err;
+
+		/* Determine if we are in CQL mode. This alters the commands we do */
+		err = dytc_perfmode_get(&cur_perfmode, &cur_funcmode);
+		if (err)
+			return err;
+
+		if (cur_funcmode == DYTC_FUNCTION_CQL) {
+			/* To set the mode we need to disable CQL first*/
+			dytc_ignore_next_event = true; /* Ignore event */
+			err = dytc_command(DYTC_DISABLE_CQL, &output);
+			if (err)
+				return err;
+		}
+		dytc_set = (1 << DYTC_SET_VALID_BIT) |
+			(DYTC_FUNCTION_MMC << DYTC_SET_FUNCTION_BIT) |
+			(perfmode << DYTC_SET_MODE_BIT) |
+			DYTC_CMD_SET;
+		err = dytc_command(dytc_set, &output);
+		if (err)
+			return err;
+		if (cur_funcmode == DYTC_FUNCTION_CQL) {
+			dytc_ignore_next_event = true; /* Ignore event */
+			err = dytc_command(DYTC_ENABLE_CQL, &output);
+			if (err)
+				return err;
+		}
+	}
+	/* Success - update current profile */
+	dytc_current_profile = profile;
+	return 0;
+}
+
+static void dytc_profile_refresh(void)
+{
+	enum platform_profile_option profile;
+	int perfmode, funcmode;
+	int err;
+
+	err = dytc_perfmode_get(&perfmode, &funcmode);
+	if (err)
+		return;
+
+	err = convert_dytc_to_profile(perfmode, &profile);
+	if (profile != dytc_current_profile) {
+		dytc_current_profile = profile;
+		platform_profile_notify();
+	}
+}
+#endif
+
+static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
+{
+	int err, output;
+
+	err = dytc_command(DYTC_CMD_QUERY, &output);
+	/*
+	 * If support isn't available (ENODEV) then don't return an error
+	 * and don't create the sysfs group
 	 */
 	if (err == -ENODEV)
 		return 0;
@@ -9912,14 +10109,54 @@ static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
 	if (err)
 		return err;
 
-	/* Platform supports this feature - create the group */
-	err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
-	return err;
+	/* Check DYTC is enabled and supports mode setting */
+	dytc_available = false;
+	dytc_ignore_next_event = false;
+
+	if (output & BIT(DYTC_QUERY_ENABLE_BIT)) {
+		/* Only DYTC v5.0 and later has this feature. */
+		int dytc_version;
+
+		dytc_version = (output >> DYTC_QUERY_REV_BIT) & 0xF;
+		if (dytc_version >= 5) {
+			dbg_printk(TPACPI_DBG_INIT,
+				   "DYTC version %d: thermal mode available\n", dytc_version);
+			dytc_available = true;
+#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE)
+			/* Create platform_profile structure and register */
+			dytc_profile.choices = (1 << profile_low) |
+				(1 << profile_balance) |
+				(1 << profile_perform);
+			dytc_profile.profile_get = dytc_profile_get;
+			dytc_profile.profile_set = dytc_profile_set;
+			err = platform_profile_register(&dytc_profile);
+			/*
+			 * If for some reason platform_profiles aren't enabled
+			 * don't quit terminally.
+			 */
+			if (err)
+				return 0;
+#endif
+			/*
+			 * Note - this has been deprecated by the input sensor implementation,
+			 * but can't be removed until we confirm user space is no longer using
+			 */
+			dytc_lapmode_get(&dytc_lapmode);
+			return device_create_file(&tpacpi_pdev->dev, &dev_attr_dytc_lapmode);
+		}
+	}
+	return 0;
 }
 
 static void dytc_exit(void)
 {
-	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
+	if (dytc_available) {
+		device_remove_file(&tpacpi_pdev->dev, &dev_attr_dytc_lapmode);
+#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE)
+		platform_profile_unregister();
+#endif
+		dytc_available = false;
+	}
 }
 
 static struct ibm_struct dytc_driver_data = {
@@ -10103,8 +10340,15 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
 	}
 
 	if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED) {
-		dytc_lapmode_refresh();
-		lapsensor_refresh();
+		if (dytc_ignore_next_event)
+			dytc_ignore_next_event = false; /*clear setting*/
+		else {
+			dytc_lapmode_refresh();
+#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE)
+			dytc_profile_refresh();
+#endif
+			lapsensor_refresh();
+		}
 	}
 
 }
-- 
2.25.1

Re: [PATCH v2 2/3] ACPI: platform-profile: Add platform profile support

[Barnabás Pőcze]

Hi

> [..]
>  drivers/acpi/Kconfig            |  33 ++++++
>  drivers/acpi/Makefile           |   1 +
>  drivers/acpi/platform_profile.c | 181 ++++++++++++++++++++++++++++++++
>  3 files changed, 215 insertions(+)

I believe the header file is missing from the patch.

> [...]


Regards,
Barnabás Pőcze

Re: [External] Re: [PATCH v2 2/3] ACPI: platform-profile: Add platform profile support

[Mark Pearson]

On 14/11/2020 10:10, Barnabás Pőcze wrote:
> Hi
> 
>> [..]
>>   drivers/acpi/Kconfig            |  33 ++++++
>>   drivers/acpi/Makefile           |   1 +
>>   drivers/acpi/platform_profile.c | 181 ++++++++++++++++++++++++++++++++
>>   3 files changed, 215 insertions(+)
> 
> I believe the header file is missing from the patch.
> 
Man....that's embarrassing. Not sure how I managed to miss that :/

Updated patch coming shortly.

Mark

Re: [PATCH v2 3/3] platform/x86: thinkpad_acpi: Add platform profile support

[Barnabás Pőcze]

2020. november 14., szombat 16:01 keltezéssel, Mark Pearson <markpearson@lenovo.com> írta:

Hi


I think there are a couple places where the BIT() macro could be used.


> [...]
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 890dda284a00..13352ccdfdaf 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -72,6 +72,7 @@
>  #include <linux/uaccess.h>
>  #include <acpi/battery.h>
>  #include <acpi/video.h>
> +#include <linux/platform_profile.h>
>
>  /* ThinkPad CMOS commands */
>  #define TP_CMOS_VOLUME_DOWN	0
> @@ -9832,10 +9833,40 @@ static struct ibm_struct lcdshadow_driver_data = {
>   * DYTC subdriver, for the Lenovo lapmode feature

This comment should be updated, no? It does more than report the "lap mode" state?


>   */
>
> +#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_GET_LAPMODE_BIT 17 /* Set when in lapmode */

`DYTC_GET_LAPMODE_BIT` is defined a couple lines below, I think this definition
should be removed.


> +#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 revisision */
                                                          ^
"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 */
                                              ^
The spacing is inconsistent in the comments.  |


> +#define DYTC_GET_LAPMODE_BIT  17 /* Bit 17 - lapmode. Set when on lap */
> +
> +#define DYTC_SET_FUNCTION_BIT 12 /* Bits 12-15 - funct setting */

If all letters of "function" were spelled a couple lines above, I think
it should be here as well.


> +#define DYTC_SET_MODE_BIT     16 /* Bits 16-19 - mode setting */
> +#define DYTC_SET_VALID_BIT    20 /* Bit 20 - 1 = on, 0 = off */
> +

I personally would create a DYTC_SET_COMMAND() - or similarly named - macro
along these lines:
```
#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)
```
and use that later on. I believe this helps readability and reduces the chances
of accindental mistakes.


> +#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 */

It seems strange to me that there is a leap from 1 to 11.


> +
> +#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 balance */

I suggest you capitalize the last two comments to be consistent with the rest
of the patch.


> +
> +#define DYTC_DISABLE_CQL ((DYTC_MODE_BALANCE << DYTC_SET_MODE_BIT) | \
> +		(DYTC_FUNCTION_CQL << DYTC_SET_FUNCTION_BIT) | \
> +		DYTC_CMD_SET)
> +#define DYTC_ENABLE_CQL (DYTC_DISABLE_CQL | (1 << DYTC_SET_VALID_BIT))
> [...]
> +static int convert_profile_to_dytc(enum platform_profile_option profile, int *perfmode)
> +{
> +	switch (profile) {
> +	case profile_low:
> +		*perfmode = DYTC_MODE_QUIET;
> +		break;
> +	case profile_balance:
> +		*perfmode = DYTC_MODE_BALANCE;
> +		break;
> +	case profile_perform:
> +		*perfmode = DYTC_MODE_PERFORM;
> +		break;
> +	default: /* Unknown profile */
> +		return -EOPNOTSUPP;

I personally think EINVAL would be better here,
just like in `convert_dytc_to_profile()`.


> +	}
> +	return 0;
> +}
> +
> +static int dytc_perfmode_get(int *perfmode, int *funcmode)
> +{
> +	int output, err;
> +
> +	if (!dytc_available)
> +		return -ENODEV;
> +
> +	err = dytc_command(DYTC_CMD_GET, &output);
> +	if (err)
> +		return err;
> +
> +	*funcmode = (output >> DYTC_GET_FUNCTION_BIT) & 0xF;
> +	if (*funcmode == DYTC_FUNCTION_CQL) {
> +		int dummy;
> +		/*
> +		 * We can't get the mode when in CQL mode - so we disable CQL
> +		 * mode retrieve the mode and then enable it again.
> +		 * As disabling/enabling CQL triggers an event we set a flag to
> +		 * ignore these events. This will be cleared by the event handler
> +		 */
> +		dytc_ignore_next_event = true;
> +		err = dytc_command(DYTC_DISABLE_CQL, &dummy);
> +		if (err)
> +			return err;
> +		err = dytc_command(DYTC_CMD_GET, &output);
> +		if (err)
> +			return err;

If `DYTC_CMD_GET` fails, then CQL state is not restored. I think that may be
undesired?


> +		/* Again ignore this event */
> +		dytc_ignore_next_event = true;
> +		err = dytc_command(DYTC_ENABLE_CQL, &dummy);
> +		if (err)
> +			return err;
> +	}
> +	*perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF;
> +	return 0;
> +}
> [...]
> +/*
> + * dytc_profile_set: Function to register with platform_profile
> + * handler. Sets current platform profile.
> + */
> +int dytc_profile_set(enum platform_profile_option profile)
> +{
> +	int cur_perfmode, cur_funcmode;
> +	int err, dytc_set;
> +	int output;
> +
> +	if (!dytc_available)
> +		return -ENODEV;
> +
> +	if (profile == profile_balance) {
> +		/* To get back to balance mode we just issue a reset command */
> +		err = dytc_command(DYTC_CMD_RESET, &output);
> +		if (err)
> +			return err;
> +	} else {
> +		int perfmode;
> +		int err;
> +
> +		err = convert_profile_to_dytc(profile, &perfmode);
> +		if (err)
> +			return err;
> +
> +		/* Determine if we are in CQL mode. This alters the commands we do */
> +		err = dytc_perfmode_get(&cur_perfmode, &cur_funcmode);
> +		if (err)
> +			return err;
> +
> +		if (cur_funcmode == DYTC_FUNCTION_CQL) {
> +			/* To set the mode we need to disable CQL first*/
> +			dytc_ignore_next_event = true; /* Ignore event */
> +			err = dytc_command(DYTC_DISABLE_CQL, &output);
> +			if (err)
> +				return err;
> +		}
> +		dytc_set = (1 << DYTC_SET_VALID_BIT) |
> +			(DYTC_FUNCTION_MMC << DYTC_SET_FUNCTION_BIT) |
> +			(perfmode << DYTC_SET_MODE_BIT) |
> +			DYTC_CMD_SET;
> +		err = dytc_command(dytc_set, &output);
> +		if (err)
> +			return err;

If I see it correctly, if CQL is turned off successfully, but the this command
fails, then the function returns with an error, but does not restore CQL state.
Which may or may not be desired?


> +		if (cur_funcmode == DYTC_FUNCTION_CQL) {
> +			dytc_ignore_next_event = true; /* Ignore event */
> +			err = dytc_command(DYTC_ENABLE_CQL, &output);
> +			if (err)
> +				return err;
> +		}
> +	}
> +	/* Success - update current profile */
> +	dytc_current_profile = profile;
> +	return 0;
> +}
> +
> +static void dytc_profile_refresh(void)
> +{
> +	enum platform_profile_option profile;
> +	int perfmode, funcmode;
> +	int err;
> +
> +	err = dytc_perfmode_get(&perfmode, &funcmode);
> +	if (err)
> +		return;
> +
> +	err = convert_dytc_to_profile(perfmode, &profile);

`err` is not checked.


> +	if (profile != dytc_current_profile) {
> +		dytc_current_profile = profile;
> +		platform_profile_notify();
> +	}
> +}
> +#endif
> +
> +static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
> +{
> +	int err, output;
> +
> +	err = dytc_command(DYTC_CMD_QUERY, &output);
> +	/*
> +	 * If support isn't available (ENODEV) then don't return an error
> +	 * and don't create the sysfs group
>  	 */
>  	if (err == -ENODEV)
>  		return 0;
> @@ -9912,14 +10109,54 @@ static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
>  	if (err)
>  		return err;
>
> -	/* Platform supports this feature - create the group */
> -	err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
> -	return err;
> +	/* Check DYTC is enabled and supports mode setting */
> +	dytc_available = false;
> +	dytc_ignore_next_event = false;
> +
> +	if (output & BIT(DYTC_QUERY_ENABLE_BIT)) {
> +		/* Only DYTC v5.0 and later has this feature. */
> +		int dytc_version;
> +
> +		dytc_version = (output >> DYTC_QUERY_REV_BIT) & 0xF;
> +		if (dytc_version >= 5) {
> +			dbg_printk(TPACPI_DBG_INIT,
> +				   "DYTC version %d: thermal mode available\n", dytc_version);
> +			dytc_available = true;
> +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE)
> +			/* Create platform_profile structure and register */
> +			dytc_profile.choices = (1 << profile_low) |
> +				(1 << profile_balance) |
> +				(1 << profile_perform);
> +			dytc_profile.profile_get = dytc_profile_get;
> +			dytc_profile.profile_set = dytc_profile_set;

By the way, wouldn't it be easier to initialize this struct when it's defined?
```
static platform_profile_handler dytc_profile = {
  .choices = ...,
  .profile_set = ...,
  .profile_get = ....,
};
```
?


> +			err = platform_profile_register(&dytc_profile);
> +			/*
> +			 * If for some reason platform_profiles aren't enabled
> +			 * don't quit terminally.
> +			 */
> +			if (err)
> +				return 0;

If I see it correctly, if `platform_profile_register()` fails for some reason,
then the dytc_lapmode attribute won't be created, is that the expected behaviour?


> +#endif
> +			/*
> +			 * Note - this has been deprecated by the input sensor implementation,
> +			 * but can't be removed until we confirm user space is no longer using
> +			 */
> +			dytc_lapmode_get(&dytc_lapmode);
> +			return device_create_file(&tpacpi_pdev->dev, &dev_attr_dytc_lapmode);

Previously, the DYTC version (and the "enable bit") wasn't checked, the lap mode
attribute was always created if DYTC was available. This patch changes that,
why?


> +		}
> +	}
> +	return 0;
>  }
>
>  static void dytc_exit(void)
>  {
> -	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
> +	if (dytc_available) {
> +		device_remove_file(&tpacpi_pdev->dev, &dev_attr_dytc_lapmode);
> +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE)
> +		platform_profile_unregister();

`platform_profile_unregister()` is called even if `platform_profile_register()`
failed.


> +#endif
> +		dytc_available = false;
> +	}
>  }
>
>  static struct ibm_struct dytc_driver_data = {
> @@ -10103,8 +10340,15 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
>  	}
>
>  	if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED) {
> -		dytc_lapmode_refresh();
> -		lapsensor_refresh();
> +		if (dytc_ignore_next_event)
> +			dytc_ignore_next_event = false; /*clear setting*/

Either none or all of the blocks should be surrounded with {} [1].


> +		else {
> +			dytc_lapmode_refresh();
> +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE)
> +			dytc_profile_refresh();
> +#endif
> +			lapsensor_refresh();
> +		}
>  	}
>
>  }
> --
> 2.25.1


[1]: https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces


Regards,
Barnabás Pőcze

Re: [External] Re: [PATCH v2 3/3] platform/x86: thinkpad_acpi: Add platform profile support

[Mark Pearson]

Hi Barnabas

On 15/11/2020 13:33, Barnabás Pőcze wrote:
> 
> 2020. november 14., szombat 16:01 keltezéssel, Mark Pearson <markpearson@lenovo.com> írta:
> 
> Hi
> 
> 
> I think there are a couple places where the BIT() macro could be used.
> 
> 
>> [...]
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>> index 890dda284a00..13352ccdfdaf 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -72,6 +72,7 @@
>>   #include <linux/uaccess.h>
>>   #include <acpi/battery.h>
>>   #include <acpi/video.h>
>> +#include <linux/platform_profile.h>
>>
>>   /* ThinkPad CMOS commands */
>>   #define TP_CMOS_VOLUME_DOWN	0
>> @@ -9832,10 +9833,40 @@ static struct ibm_struct lcdshadow_driver_data = {
>>    * DYTC subdriver, for the Lenovo lapmode feature
> 
> This comment should be updated, no? It does more than report the "lap mode" state?
Agreed.
> 
> 
>>    */
>>
>> +#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_GET_LAPMODE_BIT 17 /* Set when in lapmode */
> 
> `DYTC_GET_LAPMODE_BIT` is defined a couple lines below, I think this definition
> should be removed.
Crud - I missed that. Amazing the compiler didn't object. Will fix.
> 
> 
>> +#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 revisision */
>                                                            ^
> "revision"                                                |
ack
> 
> 
>> +#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 */
>                                                ^
> The spacing is inconsistent in the comments.  |
ok
> 
> 
>> +#define DYTC_GET_LAPMODE_BIT  17 /* Bit 17 - lapmode. Set when on lap */
>> +
>> +#define DYTC_SET_FUNCTION_BIT 12 /* Bits 12-15 - funct setting */
> 
> If all letters of "function" were spelled a couple lines above, I think
> it should be here as well.
agreed
> 
> 
>> +#define DYTC_SET_MODE_BIT     16 /* Bits 16-19 - mode setting */
>> +#define DYTC_SET_VALID_BIT    20 /* Bit 20 - 1 = on, 0 = off */
>> +
> 
> I personally would create a DYTC_SET_COMMAND() - or similarly named - macro
> along these lines:
> ```
> #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)
> ```
> and use that later on. I believe this helps readability and reduces the chances
> of accindental mistakes.
Sounds good
> 
> 
>> +#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 */
> 
> It seems strange to me that there is a leap from 1 to 11.
That one is outside my control - it's how the HW team defined the numbers :)
> 
> 
>> +
>> +#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 balance */
> 
> I suggest you capitalize the last two comments to be consistent with the rest
> of the patch.
Ack
> 
> 
>> +
>> +#define DYTC_DISABLE_CQL ((DYTC_MODE_BALANCE << DYTC_SET_MODE_BIT) | \
>> +		(DYTC_FUNCTION_CQL << DYTC_SET_FUNCTION_BIT) | \
>> +		DYTC_CMD_SET)
>> +#define DYTC_ENABLE_CQL (DYTC_DISABLE_CQL | (1 << DYTC_SET_VALID_BIT))
>> [...]
>> +static int convert_profile_to_dytc(enum platform_profile_option profile, int *perfmode)
>> +{
>> +	switch (profile) {
>> +	case profile_low:
>> +		*perfmode = DYTC_MODE_QUIET;
>> +		break;
>> +	case profile_balance:
>> +		*perfmode = DYTC_MODE_BALANCE;
>> +		break;
>> +	case profile_perform:
>> +		*perfmode = DYTC_MODE_PERFORM;
>> +		break;
>> +	default: /* Unknown profile */
>> +		return -EOPNOTSUPP;
> 
> I personally think EINVAL would be better here,
> just like in `convert_dytc_to_profile()`.
I liked how this worked when testing.
If you put in an invalid profile name then platform_profile returned 
EINVAL but if you got this far you'd provided a valid profile setting 
that this driver doesn't support and the not supported message seemed 
clearer. As an example 'cool' is used on HP platforms but not Lenovo.
I'd like to leave this one as it is please.
> 
> 
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int dytc_perfmode_get(int *perfmode, int *funcmode)
>> +{
>> +	int output, err;
>> +
>> +	if (!dytc_available)
>> +		return -ENODEV;
>> +
>> +	err = dytc_command(DYTC_CMD_GET, &output);
>> +	if (err)
>> +		return err;
>> +
>> +	*funcmode = (output >> DYTC_GET_FUNCTION_BIT) & 0xF;
>> +	if (*funcmode == DYTC_FUNCTION_CQL) {
>> +		int dummy;
>> +		/*
>> +		 * We can't get the mode when in CQL mode - so we disable CQL
>> +		 * mode retrieve the mode and then enable it again.
>> +		 * As disabling/enabling CQL triggers an event we set a flag to
>> +		 * ignore these events. This will be cleared by the event handler
>> +		 */
>> +		dytc_ignore_next_event = true;
>> +		err = dytc_command(DYTC_DISABLE_CQL, &dummy);
>> +		if (err)
>> +			return err;
>> +		err = dytc_command(DYTC_CMD_GET, &output);
>> +		if (err)
>> +			return err;
> 
> If `DYTC_CMD_GET` fails, then CQL state is not restored. I think that may be
> undesired?
Agreed - thank you, that's an important catch.

> 
> 
>> +		/* Again ignore this event */
>> +		dytc_ignore_next_event = true;
>> +		err = dytc_command(DYTC_ENABLE_CQL, &dummy);
>> +		if (err)
>> +			return err;
>> +	}
>> +	*perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF;
>> +	return 0;
>> +}
>> [...]
>> +/*
>> + * dytc_profile_set: Function to register with platform_profile
>> + * handler. Sets current platform profile.
>> + */
>> +int dytc_profile_set(enum platform_profile_option profile)
>> +{
>> +	int cur_perfmode, cur_funcmode;
>> +	int err, dytc_set;
>> +	int output;
>> +
>> +	if (!dytc_available)
>> +		return -ENODEV;
>> +
>> +	if (profile == profile_balance) {
>> +		/* To get back to balance mode we just issue a reset command */
>> +		err = dytc_command(DYTC_CMD_RESET, &output);
>> +		if (err)
>> +			return err;
>> +	} else {
>> +		int perfmode;
>> +		int err;
>> +
>> +		err = convert_profile_to_dytc(profile, &perfmode);
>> +		if (err)
>> +			return err;
>> +
>> +		/* Determine if we are in CQL mode. This alters the commands we do */
>> +		err = dytc_perfmode_get(&cur_perfmode, &cur_funcmode);
>> +		if (err)
>> +			return err;
>> +
>> +		if (cur_funcmode == DYTC_FUNCTION_CQL) {
>> +			/* To set the mode we need to disable CQL first*/
>> +			dytc_ignore_next_event = true; /* Ignore event */
>> +			err = dytc_command(DYTC_DISABLE_CQL, &output);
>> +			if (err)
>> +				return err;
>> +		}
>> +		dytc_set = (1 << DYTC_SET_VALID_BIT) |
>> +			(DYTC_FUNCTION_MMC << DYTC_SET_FUNCTION_BIT) |
>> +			(perfmode << DYTC_SET_MODE_BIT) |
>> +			DYTC_CMD_SET;
>> +		err = dytc_command(dytc_set, &output);
>> +		if (err)
>> +			return err;
> 
> If I see it correctly, if CQL is turned off successfully, but the this command
> fails, then the function returns with an error, but does not restore CQL state.
> Which may or may not be desired?
Right - not desired. I'll fix
> 
> 
>> +		if (cur_funcmode == DYTC_FUNCTION_CQL) {
>> +			dytc_ignore_next_event = true; /* Ignore event */
>> +			err = dytc_command(DYTC_ENABLE_CQL, &output);
>> +			if (err)
>> +				return err;
>> +		}
>> +	}
>> +	/* Success - update current profile */
>> +	dytc_current_profile = profile;
>> +	return 0;
>> +}
>> +
>> +static void dytc_profile_refresh(void)
>> +{
>> +	enum platform_profile_option profile;
>> +	int perfmode, funcmode;
>> +	int err;
>> +
>> +	err = dytc_perfmode_get(&perfmode, &funcmode);
>> +	if (err)
>> +		return;
>> +
>> +	err = convert_dytc_to_profile(perfmode, &profile);
> 
> `err` is not checked.
ack
> 
> 
>> +	if (profile != dytc_current_profile) {
>> +		dytc_current_profile = profile;
>> +		platform_profile_notify();
>> +	}
>> +}
>> +#endif
>> +
>> +static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
>> +{
>> +	int err, output;
>> +
>> +	err = dytc_command(DYTC_CMD_QUERY, &output);
>> +	/*
>> +	 * If support isn't available (ENODEV) then don't return an error
>> +	 * and don't create the sysfs group
>>   	 */
>>   	if (err == -ENODEV)
>>   		return 0;
>> @@ -9912,14 +10109,54 @@ static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
>>   	if (err)
>>   		return err;
>>
>> -	/* Platform supports this feature - create the group */
>> -	err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
>> -	return err;
>> +	/* Check DYTC is enabled and supports mode setting */
>> +	dytc_available = false;
>> +	dytc_ignore_next_event = false;
>> +
>> +	if (output & BIT(DYTC_QUERY_ENABLE_BIT)) {
>> +		/* Only DYTC v5.0 and later has this feature. */
>> +		int dytc_version;
>> +
>> +		dytc_version = (output >> DYTC_QUERY_REV_BIT) & 0xF;
>> +		if (dytc_version >= 5) {
>> +			dbg_printk(TPACPI_DBG_INIT,
>> +				   "DYTC version %d: thermal mode available\n", dytc_version);
>> +			dytc_available = true;
>> +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE)
>> +			/* Create platform_profile structure and register */
>> +			dytc_profile.choices = (1 << profile_low) |
>> +				(1 << profile_balance) |
>> +				(1 << profile_perform);
>> +			dytc_profile.profile_get = dytc_profile_get;
>> +			dytc_profile.profile_set = dytc_profile_set;
> 
> By the way, wouldn't it be easier to initialize this struct when it's defined?
> ```
> static platform_profile_handler dytc_profile = {
>    .choices = ...,
>    .profile_set = ...,
>    .profile_get = ....,
> };
> ```
> ?
Yep, it would. I'll fix.
> 
> 
>> +			err = platform_profile_register(&dytc_profile);
>> +			/*
>> +			 * If for some reason platform_profiles aren't enabled
>> +			 * don't quit terminally.
>> +			 */
>> +			if (err)
>> +				return 0;
> 
> If I see it correctly, if `platform_profile_register()` fails for some reason,
> then the dytc_lapmode attribute won't be created, is that the expected behaviour?
Hmmm - that's not ideal. I'll look at this section again.

> 
> 
>> +#endif
>> +			/*
>> +			 * Note - this has been deprecated by the input sensor implementation,
>> +			 * but can't be removed until we confirm user space is no longer using
>> +			 */
>> +			dytc_lapmode_get(&dytc_lapmode);
>> +			return device_create_file(&tpacpi_pdev->dev, &dev_attr_dytc_lapmode);
> 
> Previously, the DYTC version (and the "enable bit") wasn't checked, the lap mode
> attribute was always created if DYTC was available. This patch changes that,
> why?
It's an improvement/fix.
It probably should have been in the original version really but I only 
found out after some clarification from the FW team when there were some 
issues on X1C6 with DYTC 4 returning lapmode always on.
> 
> 
>> +		}
>> +	}
>> +	return 0;
>>   }
>>
>>   static void dytc_exit(void)
>>   {
>> -	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
>> +	if (dytc_available) {
>> +		device_remove_file(&tpacpi_pdev->dev, &dev_attr_dytc_lapmode);
>> +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE)
>> +		platform_profile_unregister();
> 
> `platform_profile_unregister()` is called even if `platform_profile_register()`
> failed.
Agreed - I'll fix
> 
> 
>> +#endif
>> +		dytc_available = false;
>> +	}
>>   }
>>
>>   static struct ibm_struct dytc_driver_data = {
>> @@ -10103,8 +10340,15 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
>>   	}
>>
>>   	if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED) {
>> -		dytc_lapmode_refresh();
>> -		lapsensor_refresh();
>> +		if (dytc_ignore_next_event)
>> +			dytc_ignore_next_event = false; /*clear setting*/
> 
> Either none or all of the blocks should be surrounded with {} [1].
Ah - interesting. I'll fix.

> 
> 
>> +		else {
>> +			dytc_lapmode_refresh();
>> +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE)
>> +			dytc_profile_refresh();
>> +#endif
>> +			lapsensor_refresh();
>> +		}
>>   	}
>>
>>   }
>> --
>> 2.25.1
> 
> 
> [1]: https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces
> 
> 
> Regards,
> Barnabás Pőcze
> 
Thank you!
Mark

Re: [External] Re: [PATCH v2 3/3] platform/x86: thinkpad_acpi: Add platform profile support

[Barnabás Pőcze]

Hi


2020. november 16., hétfő 0:22 keltezéssel, Mark Pearson írta:

> [...]
> >> +static int convert_profile_to_dytc(enum platform_profile_option profile, int *perfmode)
> >> +{
> >> +	switch (profile) {
> >> +	case profile_low:
> >> +		*perfmode = DYTC_MODE_QUIET;
> >> +		break;
> >> +	case profile_balance:
> >> +		*perfmode = DYTC_MODE_BALANCE;
> >> +		break;
> >> +	case profile_perform:
> >> +		*perfmode = DYTC_MODE_PERFORM;
> >> +		break;
> >> +	default: /* Unknown profile */
> >> +		return -EOPNOTSUPP;
> >
> > I personally think EINVAL would be better here,
> > just like in `convert_dytc_to_profile()`.
> I liked how this worked when testing.
> If you put in an invalid profile name then platform_profile returned
> EINVAL but if you got this far you'd provided a valid profile setting
> that this driver doesn't support and the not supported message seemed
> clearer. As an example 'cool' is used on HP platforms but not Lenovo.
> I'd like to leave this one as it is please.
> >

I have just realized that the platform profile module does not check if
the profile the user wants to set is supported by the handler. As I've
mentioned in my other email, I think that should be checked. You could return
EOPNOTSUPP there (in the platform profile module). After reading the explanation
why you want to use EOPNOTSUPP, I believe it makes sense and it's fine.


> >
> >> +	}
> >> +	return 0;
> >> +}
> [...]


Regards,
Barnabás Pőcze