[PATCH v2] platform/x86: thinkpad_acpi: lap or desk mode interface

[PATCH v2] platform/x86: thinkpad_acpi: lap or desk mode interface

[Mark Pearson]

  Newer Lenovo Thinkpad platforms have support to identify whether the
  system is on-lap or not using an ACPI DYTC event from the firmware.

  This patch provides the ability to retrieve the current mode via sysfs
  entrypoints and will be used by userspace for thermal mode and WWAN
  functionality

Co-developed-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Sugumaran <slacshiminar-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
---
Changes in v2:
- cleaned up initialisation sequence to be cleaner and avoid spamming
  platforms that don't have DYTC with warning message. Tested on P52
- Adding platform-driver-x86 mailing list for review as requested

 drivers/platform/x86/thinkpad_acpi.c | 113 +++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 0f704484ae1d..8f51bbba21cd 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -4049,6 +4049,7 @@ static bool hotkey_notify_6xxx(const u32 hkey,
 		pr_debug("EC reports: Thermal Control Command set completed (DYTC)\n");
 		/* recommended action: do nothing, we don't have
 		 * Lenovo ATM information */
+		tpacpi_driver_event(hkey);
 		return true;
 	case TP_HKEY_EV_THM_TRANSFM_CHANGED:
 		pr_debug("EC reports: Thermal Transformation changed (GMTS)\n");
@@ -9811,6 +9812,110 @@ static struct ibm_struct lcdshadow_driver_data = {
 	.write = lcdshadow_write,
 };
 
+/*************************************************************************
+ * DYTC subdriver, for the Lenovo performace mode feature
+ */
+
+#define DYTC_CMD_GET      2 /*To get current IC function and mode*/
+
+#define DYTC_GET_ENABLE_MASK  0x1 /*0 = disabled, 1 = enabled*/
+#define DYTC_GET_LAPMODE_SHIFT 17
+
+static int  dytc_lapmode;
+static void dytc_lapmode_notify_change(void)
+{
+	sysfs_notify(&tpacpi_pdev->dev.kobj, NULL,
+			"dytc_lapmode");
+}
+
+static int dytc_command(int command)
+{
+	acpi_handle dytc_handle;
+	int output;
+
+	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) {
+		/*Platform doesn't support DYTC*/
+		return -ENODEV;
+	}
+	if (!acpi_evalf(dytc_handle, &output, NULL, "dd", command))
+		return -EIO;
+	return output;
+}
+
+static int dytc_lapmode_get(void)
+{
+	int output;
+
+	output = dytc_command(DYTC_CMD_GET);
+	if ((output == -ENODEV) || (output == -EIO))
+		return output;
+
+	return ((output >> DYTC_GET_LAPMODE_SHIFT) &
+				DYTC_GET_ENABLE_MASK);
+}
+
+static void dytc_lapmode_refresh(void)
+{
+	int new_state;
+
+	new_state = dytc_lapmode_get();
+	if ((new_state == -ENODEV) || (new_state == -EIO))
+		return;
+
+	if (dytc_lapmode != new_state) {
+		dytc_lapmode = new_state;
+		dytc_lapmode_notify_change();
+	}
+}
+
+/* sysfs lapmode entry */
+static ssize_t dytc_lapmode_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	if (dytc_lapmode < 0)
+		return dytc_lapmode;
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", dytc_lapmode);
+}
+
+static DEVICE_ATTR_RO(dytc_lapmode);
+
+static struct attribute *dytc_attributes[] = {
+	&dev_attr_dytc_lapmode.attr,
+	NULL
+};
+
+static const struct attribute_group dytc_attr_group = {
+	.attrs = dytc_attributes,
+};
+
+static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
+{
+	int res;
+
+	dytc_lapmode = dytc_lapmode_get();
+
+	if (dytc_lapmode < 0 && dytc_lapmode != -ENODEV)
+		return dytc_lapmode;
+
+	res = sysfs_create_group(&tpacpi_pdev->dev.kobj,
+				&dytc_attr_group);
+
+	return res;
+}
+
+static void dytc_exit(void)
+{
+	sysfs_remove_group(&tpacpi_pdev->dev.kobj,
+			&dytc_attr_group);
+}
+
+static struct ibm_struct dytc_driver_data = {
+	.name = "dytc",
+	.exit = dytc_exit
+};
+
 /****************************************************************************
  ****************************************************************************
  *
@@ -9858,6 +9963,10 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
 
 		mutex_unlock(&kbdlight_mutex);
 	}
+
+	if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED)
+		dytc_lapmode_refresh();
+
 }
 
 static void hotkey_driver_event(const unsigned int scancode)
@@ -10296,6 +10405,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
 		.init = tpacpi_lcdshadow_init,
 		.data = &lcdshadow_driver_data,
 	},
+	{
+		.init = tpacpi_dytc_init,
+		.data = &dytc_driver_data,
+	},
 };
 
 static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
-- 
2.26.2

[RESEND PATCH v2] platform/x86: thinkpad_acpi: lap or desk mode interface

[Mark Pearson]

  Newer Lenovo Thinkpad platforms have support to identify whether the
  system is on-lap or not using an ACPI DYTC event from the firmware.

  This patch provides the ability to retrieve the current mode via sysfs
  entrypoints and will be used by userspace for thermal mode and WWAN
  functionality

Co-developed-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Sugumaran <slacshiminar-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
---
Resend:
- Adding Bastien Nocera to cc list as requested
Changes in v2:
- cleaned up initialisation sequence to be cleaner and avoid spamming
  platforms that don't have DYTC with warning message. Tested on P52
- Adding platform-driver-x86 mailing list for review as requested

 drivers/platform/x86/thinkpad_acpi.c | 113 +++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 0f704484ae1d..8f51bbba21cd 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -4049,6 +4049,7 @@ static bool hotkey_notify_6xxx(const u32 hkey,
 		pr_debug("EC reports: Thermal Control Command set completed (DYTC)\n");
 		/* recommended action: do nothing, we don't have
 		 * Lenovo ATM information */
+		tpacpi_driver_event(hkey);
 		return true;
 	case TP_HKEY_EV_THM_TRANSFM_CHANGED:
 		pr_debug("EC reports: Thermal Transformation changed (GMTS)\n");
@@ -9811,6 +9812,110 @@ static struct ibm_struct lcdshadow_driver_data = {
 	.write = lcdshadow_write,
 };
 
+/*************************************************************************
+ * DYTC subdriver, for the Lenovo performace mode feature
+ */
+
+#define DYTC_CMD_GET      2 /*To get current IC function and mode*/
+
+#define DYTC_GET_ENABLE_MASK  0x1 /*0 = disabled, 1 = enabled*/
+#define DYTC_GET_LAPMODE_SHIFT 17
+
+static int  dytc_lapmode;
+static void dytc_lapmode_notify_change(void)
+{
+	sysfs_notify(&tpacpi_pdev->dev.kobj, NULL,
+			"dytc_lapmode");
+}
+
+static int dytc_command(int command)
+{
+	acpi_handle dytc_handle;
+	int output;
+
+	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) {
+		/*Platform doesn't support DYTC*/
+		return -ENODEV;
+	}
+	if (!acpi_evalf(dytc_handle, &output, NULL, "dd", command))
+		return -EIO;
+	return output;
+}
+
+static int dytc_lapmode_get(void)
+{
+	int output;
+
+	output = dytc_command(DYTC_CMD_GET);
+	if ((output == -ENODEV) || (output == -EIO))
+		return output;
+
+	return ((output >> DYTC_GET_LAPMODE_SHIFT) &
+				DYTC_GET_ENABLE_MASK);
+}
+
+static void dytc_lapmode_refresh(void)
+{
+	int new_state;
+
+	new_state = dytc_lapmode_get();
+	if ((new_state == -ENODEV) || (new_state == -EIO))
+		return;
+
+	if (dytc_lapmode != new_state) {
+		dytc_lapmode = new_state;
+		dytc_lapmode_notify_change();
+	}
+}
+
+/* sysfs lapmode entry */
+static ssize_t dytc_lapmode_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	if (dytc_lapmode < 0)
+		return dytc_lapmode;
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", dytc_lapmode);
+}
+
+static DEVICE_ATTR_RO(dytc_lapmode);
+
+static struct attribute *dytc_attributes[] = {
+	&dev_attr_dytc_lapmode.attr,
+	NULL
+};
+
+static const struct attribute_group dytc_attr_group = {
+	.attrs = dytc_attributes,
+};
+
+static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
+{
+	int res;
+
+	dytc_lapmode = dytc_lapmode_get();
+
+	if (dytc_lapmode < 0 && dytc_lapmode != -ENODEV)
+		return dytc_lapmode;
+
+	res = sysfs_create_group(&tpacpi_pdev->dev.kobj,
+				&dytc_attr_group);
+
+	return res;
+}
+
+static void dytc_exit(void)
+{
+	sysfs_remove_group(&tpacpi_pdev->dev.kobj,
+			&dytc_attr_group);
+}
+
+static struct ibm_struct dytc_driver_data = {
+	.name = "dytc",
+	.exit = dytc_exit
+};
+
 /****************************************************************************
  ****************************************************************************
  *
@@ -9858,6 +9963,10 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
 
 		mutex_unlock(&kbdlight_mutex);
 	}
+
+	if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED)
+		dytc_lapmode_refresh();
+
 }
 
 static void hotkey_driver_event(const unsigned int scancode)
@@ -10296,6 +10405,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
 		.init = tpacpi_lcdshadow_init,
 		.data = &lcdshadow_driver_data,
 	},
+	{
+		.init = tpacpi_dytc_init,
+		.data = &dytc_driver_data,
+	},
 };
 
 static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
-- 
2.26.2

Re: [External] Re: [RESEND PATCH v2] platform/x86: thinkpad_acpi: lap or desk mode interface

[Mark Pearson]

Hi Bastien

On 6/19/2020 9:52 AM, Bastien Nocera wrote:
> Hey Mark,
> 
> ----- Original Message -----
<snip>
>>   
>> +/*************************************************************************
>> + * DYTC subdriver, for the Lenovo performace mode feature
>> + */
> 
> I don't think this should mention the performance mode, as it's a lap/table
> detection mode. Do we need to call that "DYTC"? "lapmode"? "lap_detection"?
> Or does the DYTC interface offer more functionality that we'd export in the
> future?
> 
I've just noticed that I can't spell performance either which is
embarrassing :)

Originally I developed this code for a thermal modes feature - but
this portion of it is also needed for WWAN so we pulled out just this
piece as the first bit to get in. Having WWAN available for users is
more important than the thermal mode interface (there are a lot of users
who want WWAN working properly on our laptops).

So...yes, DYTC does offer more functionality and I'm planning on
proposing the thermal patch as soon as this one makes it through, but I
agree that in the context of this patch the comment is misleading. I
will clean that up for this version.

>> +
>> +#define DYTC_CMD_GET      2 /*To get current IC function and mode*/
> 
> For this comment and all the ones below, space after "/*" and before "*/"
> 
Ack

>> +#define DYTC_GET_ENABLE_MASK  0x1 /*0 = disabled, 1 = enabled*/
> 
> Is that necessary?
> 
Another hangover in that the other fields used for the thermal mode have
more interesting masks and this fitted in with that. I can remove for
now if it's really a problem.

>> +#define DYTC_GET_LAPMODE_SHIFT 17
> 
> You'd probably want to call this "bit" rather than shift. We shift it to read
> the value, but 17 is the bit's position. (See BIT() usage in the driver)
> 
Ack
> Do you want to add a comment here? Is there anything else that could be
> documented (the other bits, which versions of firmware would have that, if
> there's a publicly available version, or which hardware if publicly available)
> 
So what is the preference normally? More pieces will definitely be made
public when I release the thermal mode updates but I assumed we kept
things related only to the code used. I can add more detail here if that
helps. Not trying to hide anything :)

>> +static int  dytc_lapmode;
>> +static void dytc_lapmode_notify_change(void)
>> +{
>> +	sysfs_notify(&tpacpi_pdev->dev.kobj, NULL,
>> +			"dytc_lapmode");
>> +}
>> +
>> +static int dytc_command(int command)
>> +{
>> +	acpi_handle dytc_handle;
>> +	int output;
>> +
>> +	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) {
>> +		/*Platform doesn't support DYTC*/
>> +		return -ENODEV;
>> +	}
>> +	if (!acpi_evalf(dytc_handle, &output, NULL, "dd", command))
>> +		return -EIO;
>> +	return output;
>> +}
>> +
>> +static int dytc_lapmode_get(void)
>> +{
>> +	int output;
>> +
>> +	output = dytc_command(DYTC_CMD_GET);
>> +	if ((output == -ENODEV) || (output == -EIO))
>> +		return output;
>> +
>> +	return ((output >> DYTC_GET_LAPMODE_SHIFT) &
>> +				DYTC_GET_ENABLE_MASK);
> 
> Use BIT() instead? eg.
> return (output & BIT(DYTC_GET_LAPMODE_SHIFT));
> 
Ack
>> +}
>> +
>> +static void dytc_lapmode_refresh(void)
>> +{
>> +	int new_state;
>> +
>> +	new_state = dytc_lapmode_get();
>> +	if ((new_state == -ENODEV) || (new_state == -EIO))
>> +		return;
> 
> You could also return early if "dytc_lapmode == new_state".
> 
Good point.

> Rest looks good to me.
> 
Great - thanks for the review. I'll prepare the updates and if there's
any feedback on the questions above please let me know

Mark

[PATCH v3] platform/x86: thinkpad_acpi: lap or desk mode interface

[Mark Pearson]

  Newer Lenovo Thinkpad platforms have support to identify whether the
  system is on-lap or not using an ACPI DYTC event from the firmware.

  This patch provides the ability to retrieve the current mode via sysfs
  entrypoints and will be used by userspace for thermal mode and WWAN
  functionality

Co-developed-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Sugumaran <slacshiminar-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
---
Changes in v3:
- Fixed inaccurate comments
- Used BIT macro to check lapmode bit setting as recommended and update
  define name
- Check for new_state == dytc_lapmode in dytc_lapmode_refresh

Changes in v2:
- cleaned up initialisation sequence to be cleaner and avoid spamming
  platforms that don't have DYTC with warning message. Tested on P52
- Adding platform-driver-x86 mailing list for review as requested

 drivers/platform/x86/thinkpad_acpi.c | 110 +++++++++++++++++++++++++++
 1 file changed, 110 insertions(+)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 0f704484ae1d..ce3bfb546109 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -4049,6 +4049,7 @@ static bool hotkey_notify_6xxx(const u32 hkey,
 		pr_debug("EC reports: Thermal Control Command set completed (DYTC)\n");
 		/* recommended action: do nothing, we don't have
 		 * Lenovo ATM information */
+		tpacpi_driver_event(hkey);
 		return true;
 	case TP_HKEY_EV_THM_TRANSFM_CHANGED:
 		pr_debug("EC reports: Thermal Transformation changed (GMTS)\n");
@@ -9811,6 +9812,107 @@ static struct ibm_struct lcdshadow_driver_data = {
 	.write = lcdshadow_write,
 };
 
+/*************************************************************************
+ * DYTC subdriver, for the Lenovo lapmode feature
+ */
+
+#define DYTC_CMD_GET          2 /* To get current IC function and mode */
+#define DYTC_GET_LAPMODE_BIT 17 /* Set when in lapmode */
+
+static int  dytc_lapmode;
+static void dytc_lapmode_notify_change(void)
+{
+	sysfs_notify(&tpacpi_pdev->dev.kobj, NULL,
+			"dytc_lapmode");
+}
+
+static int dytc_command(int command)
+{
+	acpi_handle dytc_handle;
+	int output;
+
+	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) {
+		/* Platform doesn't support DYTC */
+		return -ENODEV;
+	}
+	if (!acpi_evalf(dytc_handle, &output, NULL, "dd", command))
+		return -EIO;
+	return output;
+}
+
+static int dytc_lapmode_get(void)
+{
+	int output;
+
+	output = dytc_command(DYTC_CMD_GET);
+	if ((output == -ENODEV) || (output == -EIO))
+		return output;
+
+	return (output & BIT(DYTC_GET_LAPMODE_BIT) ? 1 : 0);
+}
+
+static void dytc_lapmode_refresh(void)
+{
+	int new_state;
+
+	new_state = dytc_lapmode_get();
+	if ((new_state == -ENODEV) || (new_state == -EIO) || (new_state == dytc_lapmode))
+		return;
+
+	if (dytc_lapmode != new_state) {
+		dytc_lapmode = new_state;
+		dytc_lapmode_notify_change();
+	}
+}
+
+/* sysfs lapmode entry */
+static ssize_t dytc_lapmode_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	if (dytc_lapmode < 0)
+		return dytc_lapmode;
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", dytc_lapmode);
+}
+
+static DEVICE_ATTR_RO(dytc_lapmode);
+
+static struct attribute *dytc_attributes[] = {
+	&dev_attr_dytc_lapmode.attr,
+	NULL
+};
+
+static const struct attribute_group dytc_attr_group = {
+	.attrs = dytc_attributes,
+};
+
+static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
+{
+	int res;
+
+	dytc_lapmode = dytc_lapmode_get();
+
+	if (dytc_lapmode < 0 && dytc_lapmode != -ENODEV)
+		return dytc_lapmode;
+
+	res = sysfs_create_group(&tpacpi_pdev->dev.kobj,
+				&dytc_attr_group);
+
+	return res;
+}
+
+static void dytc_exit(void)
+{
+	sysfs_remove_group(&tpacpi_pdev->dev.kobj,
+			&dytc_attr_group);
+}
+
+static struct ibm_struct dytc_driver_data = {
+	.name = "dytc",
+	.exit = dytc_exit
+};
+
 /****************************************************************************
  ****************************************************************************
  *
@@ -9858,6 +9960,10 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
 
 		mutex_unlock(&kbdlight_mutex);
 	}
+
+	if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED)
+		dytc_lapmode_refresh();
+
 }
 
 static void hotkey_driver_event(const unsigned int scancode)
@@ -10296,6 +10402,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
 		.init = tpacpi_lcdshadow_init,
 		.data = &lcdshadow_driver_data,
 	},
+	{
+		.init = tpacpi_dytc_init,
+		.data = &dytc_driver_data,
+	},
 };
 
 static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
-- 
2.26.2

[PATCH v4] platform/x86: thinkpad_acpi: lap or desk mode interface

[Mark Pearson]

  Newer Lenovo Thinkpad platforms have support to identify whether the
  system is on-lap or not using an ACPI DYTC event from the firmware.

  This patch provides the ability to retrieve the current mode via sysfs
  entrypoints and will be used by userspace for thermal mode and WWAN
  functionality

Co-developed-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Sugumaran <slacshiminar-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
---
Changes in v4:
 - Correct hotkey event comment as we're handling event
 - Remove unnecessary check in dytc_lapmode_refresh
Changes in v3:
- Fixed inaccurate comments
- Used BIT macro to check lapmode bit setting as recommended and update
  define name
- Check for new_state == dytc_lapmode in dytc_lapmode_refresh

Changes in v2:
- cleaned up initialisation sequence to be cleaner and avoid spamming
  platforms that don't have DYTC with warning message. Tested on P52
- Adding platform-driver-x86 mailing list for review as requested

 drivers/platform/x86/thinkpad_acpi.c | 111 ++++++++++++++++++++++++++-
 1 file changed, 109 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 0f704484ae1d..859b40c7113e 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -4047,8 +4047,8 @@ static bool hotkey_notify_6xxx(const u32 hkey,
 		return true;
 	case TP_HKEY_EV_THM_CSM_COMPLETED:
 		pr_debug("EC reports: Thermal Control Command set completed (DYTC)\n");
-		/* recommended action: do nothing, we don't have
-		 * Lenovo ATM information */
+		/* Thermal event - pass on to event handler */
+		tpacpi_driver_event(hkey);
 		return true;
 	case TP_HKEY_EV_THM_TRANSFM_CHANGED:
 		pr_debug("EC reports: Thermal Transformation changed (GMTS)\n");
@@ -9811,6 +9811,105 @@ static struct ibm_struct lcdshadow_driver_data = {
 	.write = lcdshadow_write,
 };
 
+/*************************************************************************
+ * DYTC subdriver, for the Lenovo lapmode feature
+ */
+
+#define DYTC_CMD_GET          2 /* To get current IC function and mode */
+#define DYTC_GET_LAPMODE_BIT 17 /* Set when in lapmode */
+
+static int  dytc_lapmode;
+static void dytc_lapmode_notify_change(void)
+{
+	sysfs_notify(&tpacpi_pdev->dev.kobj, NULL,
+			"dytc_lapmode");
+}
+
+static int dytc_command(int command)
+{
+	acpi_handle dytc_handle;
+	int output;
+
+	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) {
+		/* Platform doesn't support DYTC */
+		return -ENODEV;
+	}
+	if (!acpi_evalf(dytc_handle, &output, NULL, "dd", command))
+		return -EIO;
+	return output;
+}
+
+static int dytc_lapmode_get(void)
+{
+	int output;
+
+	output = dytc_command(DYTC_CMD_GET);
+	if ((output == -ENODEV) || (output == -EIO))
+		return output;
+
+	return (output & BIT(DYTC_GET_LAPMODE_BIT) ? 1 : 0);
+}
+
+static void dytc_lapmode_refresh(void)
+{
+	int new_state;
+
+	new_state = dytc_lapmode_get();
+	if ((new_state == -ENODEV) || (new_state == -EIO) || (new_state == dytc_lapmode))
+		return;
+
+	dytc_lapmode = new_state;
+	dytc_lapmode_notify_change();
+}
+
+/* sysfs lapmode entry */
+static ssize_t dytc_lapmode_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	if (dytc_lapmode < 0)
+		return dytc_lapmode;
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", dytc_lapmode);
+}
+
+static DEVICE_ATTR_RO(dytc_lapmode);
+
+static struct attribute *dytc_attributes[] = {
+	&dev_attr_dytc_lapmode.attr,
+	NULL
+};
+
+static const struct attribute_group dytc_attr_group = {
+	.attrs = dytc_attributes,
+};
+
+static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
+{
+	int res;
+
+	dytc_lapmode = dytc_lapmode_get();
+
+	if (dytc_lapmode < 0 && dytc_lapmode != -ENODEV)
+		return dytc_lapmode;
+
+	res = sysfs_create_group(&tpacpi_pdev->dev.kobj,
+				&dytc_attr_group);
+
+	return res;
+}
+
+static void dytc_exit(void)
+{
+	sysfs_remove_group(&tpacpi_pdev->dev.kobj,
+			&dytc_attr_group);
+}
+
+static struct ibm_struct dytc_driver_data = {
+	.name = "dytc",
+	.exit = dytc_exit
+};
+
 /****************************************************************************
  ****************************************************************************
  *
@@ -9858,6 +9957,10 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
 
 		mutex_unlock(&kbdlight_mutex);
 	}
+
+	if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED)
+		dytc_lapmode_refresh();
+
 }
 
 static void hotkey_driver_event(const unsigned int scancode)
@@ -10296,6 +10399,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
 		.init = tpacpi_lcdshadow_init,
 		.data = &lcdshadow_driver_data,
 	},
+	{
+		.init = tpacpi_dytc_init,
+		.data = &dytc_driver_data,
+	},
 };
 
 static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
-- 
2.26.2

Re: [PATCH v4] platform/x86: thinkpad_acpi: lap or desk mode interface

[Bastien Nocera]

----- Original Message -----
> Newer Lenovo Thinkpad platforms have support to identify whether the
>   system is on-lap or not using an ACPI DYTC event from the firmware.
> 
>   This patch provides the ability to retrieve the current mode via sysfs
>   entrypoints and will be used by userspace for thermal mode and WWAN
>   functionality
> 
> Co-developed-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Sugumaran <slacshiminar-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>


You can add my:
Reviewed-by: Bastien Nocera <bnocera-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Cheers

Re: [External] Re: [PATCH v4] platform/x86: thinkpad_acpi: lap or desk mode interface

[Nitin Joshi1]

Hello Bastien

>-----Original Message-----
>From: Bastien Nocera <bnocera-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

>----- Original Message -----
>> Newer Lenovo Thinkpad platforms have support to identify whether the
>>   system is on-lap or not using an ACPI DYTC event from the firmware.
>>
>>   This patch provides the ability to retrieve the current mode via sysfs
>>   entrypoints and will be used by userspace for thermal mode and WWAN
>>   functionality
>>
>> Co-developed-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
>> Reviewed-by: Sugumaran <slacshiminar-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
>
>
>You can add my:
>Reviewed-by: Bastien Nocera <bnocera-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

It's already added in latest patch and currently in "for-next"
http://git.infradead.org/linux-platform-drivers-x86.git/commit/acf7f4a59114471c3964f118564fe8e7a6f34bb8

Thanks 

Re: [External] Re: [PATCH v4] platform/x86: thinkpad_acpi: lap or desk mode interface

[Bastien Nocera]

----- Original Message -----
> Hello Bastien
> 
> >-----Original Message-----
> >From: Bastien Nocera <bnocera-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> >----- Original Message -----
> >> Newer Lenovo Thinkpad platforms have support to identify whether the
> >>   system is on-lap or not using an ACPI DYTC event from the firmware.
> >>
> >>   This patch provides the ability to retrieve the current mode via sysfs
> >>   entrypoints and will be used by userspace for thermal mode and WWAN
> >>   functionality
> >>
> >> Co-developed-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> >> Signed-off-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> >> Reviewed-by: Sugumaran <slacshiminar-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> >> Signed-off-by: Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> >
> >
> >You can add my:
> >Reviewed-by: Bastien Nocera <bnocera-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> It's already added in latest patch and currently in "for-next"
> http://git.infradead.org/linux-platform-drivers-x86.git/commit/acf7f4a59114471c3964f118564fe8e7a6f34bb8

I sent my message nearly a month ago, 2 days before the authoring date
of the patch that was merged, so I'm not sure what you're trying to
tell me here :)

Re: [External] Re: [PATCH v4] platform/x86: thinkpad_acpi: lap or desk mode interface

[Nitin Joshi1]

>-----Original Message-----
>From: Bastien Nocera <bnocera@redhat.com>
>Sent: Monday, July 27, 2020 8:04 PM
>To: Nitin Joshi1 <njoshi1@lenovo.com>
>> >>
>> >> Co-developed-by: Nitin Joshi <njoshi1@lenovo.com>
>> >> Signed-off-by: Nitin Joshi <njoshi1@lenovo.com>
>> >> Reviewed-by: Sugumaran <slacshiminar@lenovo.com>
>> >> Signed-off-by: Mark Pearson <markpearson@lenovo.com>
>> >
>> >
>> >You can add my:
>> >Reviewed-by: Bastien Nocera <bnocera@redhat.com>
>>
>> It's already added in latest patch and currently in "for-next"
>> http://git.infradead.org/linux-platform-drivers-x86.git/commit/acf7f4a
>> 59114471c3964f118564fe8e7a6f34bb8
>
>I sent my message nearly a month ago, 2 days before the authoring date of
>the patch that was merged, so I'm not sure what you're trying to tell me
>here :)

Sorry , please ignore this e-mail . Don’t know why it came up on my mailbox today . But its my mistake , I would have seen date when this e-mail was sent :) 
Apologize for inconvenience caused.

Thanks & Regards,
Nitin Joshi

_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Re: [PATCH v4] platform/x86: thinkpad_acpi: lap or desk mode interface

[Andy Shevchenko]

On Mon, Jun 29, 2020 at 10:23 PM Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org> wrote:

Thanks for the patch, my comments below.

>   Newer Lenovo Thinkpad platforms have support to identify whether the
>   system is on-lap or not using an ACPI DYTC event from the firmware.
>
>   This patch provides the ability to retrieve the current mode via sysfs
>   entrypoints and will be used by userspace for thermal mode and WWAN
>   functionality

Please use proper indentation (no need to have spaces) and punctuation
(like period at the end of sentences).

...

>  drivers/platform/x86/thinkpad_acpi.c | 111 ++++++++++++++++++++++++++-
>  1 file changed, 109 insertions(+), 2 deletions(-)

You specifically added a new ABI, where is documentation? It's a show stopper.

...

> +       sysfs_notify(&tpacpi_pdev->dev.kobj, NULL,
> +                       "dytc_lapmode");

One line?

...

> +       int output;
> +
> +       output = dytc_command(DYTC_CMD_GET);

> +       if ((output == -ENODEV) || (output == -EIO))
> +               return output;

Why not simple

 if (output < 0)
   return output;

Btw, how will this survive the 31st bit (if some method would like to use it)?

I think your prototype should be

int foo(cmd, *output);

...

> +       new_state = dytc_lapmode_get();
> +       if ((new_state == -ENODEV) || (new_state == -EIO) || (new_state == dytc_lapmode))
> +               return;

What about other errors?
What about MSB being set?

...

> +       dytc_lapmode = dytc_lapmode_get();
> +
> +       if (dytc_lapmode < 0 && dytc_lapmode != -ENODEV)
> +               return dytc_lapmode;

> +       res = sysfs_create_group(&tpacpi_pdev->dev.kobj,
> +                               &dytc_attr_group);
> +
> +       return res;

return ...(...);

So, we create a group for all possible error cases but ENODEV. Why?

> +}

...

> +       sysfs_remove_group(&tpacpi_pdev->dev.kobj,
> +                       &dytc_attr_group);

One line?

...

> +static struct ibm_struct dytc_driver_data = {
> +       .name = "dytc",
> +       .exit = dytc_exit

Comma is missed.

> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [External] Re: [PATCH v4] platform/x86: thinkpad_acpi: lap or desk mode interface

[Mark Pearson]

Thanks Andy

On 7/2/2020 5:29 AM, Andy Shevchenko wrote:
> On Mon, Jun 29, 2020 at 10:23 PM Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org> wrote:
> 
> Thanks for the patch, my comments below.
> 
>>    Newer Lenovo Thinkpad platforms have support to identify whether the
>>    system is on-lap or not using an ACPI DYTC event from the firmware.
>>
>>    This patch provides the ability to retrieve the current mode via sysfs
>>    entrypoints and will be used by userspace for thermal mode and WWAN
>>    functionality
> 
> Please use proper indentation (no need to have spaces) and punctuation
> (like period at the end of sentences).
Ack
> 
> ...
> 
>>   drivers/platform/x86/thinkpad_acpi.c | 111 ++++++++++++++++++++++++++-
>>   1 file changed, 109 insertions(+), 2 deletions(-)
> 
> You specifically added a new ABI, where is documentation? It's a show stopper.
Ah - my apologies I didn't know that was a requirement.

Any pointers on where to add it? I looked in Documentation/ABI and I 
couldn't find anything around thinkpad_acpi to add this to.
Should there be a sysfs-devices-platform-thinkpad_acpi file?

If that's the case I'm happy to look at creating that but as a first 
time kernel contributor would you object if I took that on as a separate 
exercise rather than as part of this patch. I'm guessing it would need 
more time, care and reviewers from other contributors to the 
thinkpad_acpi.c driver
> 
> ...
> 
>> +       sysfs_notify(&tpacpi_pdev->dev.kobj, NULL,
>> +                       "dytc_lapmode");
> 
> One line?
Ack
> 
> ...
> 
>> +       int output;
>> +
>> +       output = dytc_command(DYTC_CMD_GET);
> 
>> +       if ((output == -ENODEV) || (output == -EIO))
>> +               return output;
> 
> Why not simple
> 
>   if (output < 0)
>     return output;
Agreed. I'll fix

> 
> Btw, how will this survive the 31st bit (if some method would like to use it)?
Hmmm - good point. I'll revisit this.

> 
> I think your prototype should be
> 
> int foo(cmd, *output);
Looking at it again - I agree.

> 
> ...
> 
>> +       new_state = dytc_lapmode_get();
>> +       if ((new_state == -ENODEV) || (new_state == -EIO) || (new_state == dytc_lapmode))
>> +               return;
> 
> What about other errors?
> What about MSB being set?
Ack - this needs fixing

> 
> ...
> 
>> +       dytc_lapmode = dytc_lapmode_get();
>> +
>> +       if (dytc_lapmode < 0 && dytc_lapmode != -ENODEV)
>> +               return dytc_lapmode;
> 
>> +       res = sysfs_create_group(&tpacpi_pdev->dev.kobj,
>> +                               &dytc_attr_group);
>> +
>> +       return res;
> 
> return ...(...);
> 
> So, we create a group for all possible error cases but ENODEV. Why?
There seemed a good reason when I originally wrote it - but now I'm 
wondering why too.
I specifically was catching the ENODEV because of concerns around 
creating the group on platforms that didn't have the support for this 
feature - but I think in doing that I missed the obvious of all errors.

I'll revisit and fix.

> 
>> +}
> 
> ...
> 
>> +       sysfs_remove_group(&tpacpi_pdev->dev.kobj,
>> +                       &dytc_attr_group);
> 
> One line?
Ack.
As a minor note I think these all arose because of getting checkpatch to 
run cleanly. I prefer one line too and if that's your preference it 
works for me.

> 
> ...
> 
>> +static struct ibm_struct dytc_driver_data = {
>> +       .name = "dytc",
>> +       .exit = dytc_exit
> 
> Comma is missed.
Ack

> 
>> +};
> 
I'll work on these and get an updated version out for review. Thank you 
for your time looking at these.

Mark

Re: [External] Re: [PATCH v4] platform/x86: thinkpad_acpi: lap or desk mode interface

[Andy Shevchenko]

On Thu, Jul 2, 2020 at 1:45 PM Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org> wrote:
> On 7/2/2020 5:29 AM, Andy Shevchenko wrote:
> > On Mon, Jun 29, 2020 at 10:23 PM Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org> wrote:

...

> > You specifically added a new ABI, where is documentation? It's a show stopper.
> Ah - my apologies I didn't know that was a requirement.
>
> Any pointers on where to add it? I looked in Documentation/ABI and I
> couldn't find anything around thinkpad_acpi to add this to.
> Should there be a sysfs-devices-platform-thinkpad_acpi file?
>
> If that's the case I'm happy to look at creating that but as a first
> time kernel contributor would you object if I took that on as a separate
> exercise rather than as part of this patch. I'm guessing it would need
> more time, care and reviewers from other contributors to the
> thinkpad_acpi.c driver

Since it's an old driver its ABI is listed here

https://elixir.bootlin.com/linux/latest/source/Documentation/admin-guide/laptops/thinkpad-acpi.rst

...

> > Why not simple
> >
> >   if (output < 0)
> >     return output;
> Agreed. I'll fix

> > I think your prototype should be
> >
> > int foo(cmd, *output);
> Looking at it again - I agree.

And after returning only error codes, you may do above as simple as

int ret;

ret = ...(.., &output);
if (ret)
  return ret;
...
return 0;

...

> As a minor note I think these all arose because of getting checkpatch to
> run cleanly. I prefer one line too and if that's your preference it
> works for me.

Checkpatch shouldn't complain (update it if it does).

-- 
With Best Regards,
Andy Shevchenko

[PATCH v5] platform/x86: thinkpad_acpi: lap or desk mode interface

[Mark Pearson]

Newer Lenovo Thinkpad platforms have support to identify whether the
system is on-lap or not using an ACPI DYTC event from the firmware.

This patch provides the ability to retrieve the current mode via sysfs
entrypoints and will be used by userspace for thermal mode and WWAN
functionality

Co-developed-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Sugumaran <slacshiminar-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Bastien Nocera <bnocera-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
---
Changes in v5:
 - Updated with review changes from Andy Shevchenko
 - Added ABI information to thinkpad-acpi.rst
 - improved error handling and parameter passing as recommended
 - code cleanup as recommended
 - added review tag from bnocera
Changes in v4:
 - Correct hotkey event comment as we're handling event
 - Remove unnecessary check in dytc_lapmode_refresh
Changes in v3:
- Fixed inaccurate comments
- Used BIT macro to check lapmode bit setting as recommended and update
  define name
- Check for new_state == dytc_lapmode in dytc_lapmode_refresh
Changes in v2:
- cleaned up initialisation sequence to be cleaner and avoid spamming
  platforms that don't have DYTC with warning message. Tested on P52
- Adding platform-driver-x86 mailing list for review as requested

 .../admin-guide/laptops/thinkpad-acpi.rst     |  15 +++
 drivers/platform/x86/thinkpad_acpi.c          | 111 +++++++++++++++++-
 2 files changed, 124 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
index 822907dcc845..99066aa8d97b 100644
--- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
+++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
@@ -50,6 +50,7 @@ detailed description):
 	- WAN enable and disable
 	- UWB enable and disable
 	- LCD Shadow (PrivacyGuard) enable and disable
+	- Lap mode sensor
 
 A compatibility table by model and feature is maintained on the web
 site, http://ibm-acpi.sf.net/. I appreciate any success or failure
@@ -1432,6 +1433,20 @@ The first command ensures the best viewing angle and the latter one turns
 on the feature, restricting the viewing angles.
 
 
+DYTC Lapmode sensor
+------------------
+
+sysfs: dytc_lapmode
+
+Newer thinkpads and mobile workstations have the ability to determine if
+the device is in deskmode or lapmode. This feature is used by user space
+to decide if WWAN transmission can be increased to maximum power and is
+also useful for understanding the different thermal modes available as
+they differ between desk and lap mode.
+
+The property is read-only. If the platform doesn't have support the sysfs
+class is not created.
+
 EXPERIMENTAL: UWB
 -----------------
 
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index ff7f0a4f2475..037eb77414f9 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -4022,8 +4022,8 @@ static bool hotkey_notify_6xxx(const u32 hkey,
 		return true;
 	case TP_HKEY_EV_THM_CSM_COMPLETED:
 		pr_debug("EC reports: Thermal Control Command set completed (DYTC)\n");
-		/* recommended action: do nothing, we don't have
-		 * Lenovo ATM information */
+		/* Thermal event - pass on to event handler */
+		tpacpi_driver_event(hkey);
 		return true;
 	case TP_HKEY_EV_THM_TRANSFM_CHANGED:
 		pr_debug("EC reports: Thermal Transformation changed (GMTS)\n");
@@ -9795,6 +9795,105 @@ static struct ibm_struct lcdshadow_driver_data = {
 	.write = lcdshadow_write,
 };
 
+/*************************************************************************
+ * DYTC subdriver, for the Lenovo lapmode feature
+ */
+
+#define DYTC_CMD_GET          2 /* To get current IC function and mode */
+#define DYTC_GET_LAPMODE_BIT 17 /* Set when in lapmode */
+
+static bool dytc_lapmode;
+
+static void dytc_lapmode_notify_change(void)
+{
+	sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "dytc_lapmode");
+}
+
+static int dytc_command(int command, int *output)
+{
+	acpi_handle dytc_handle;
+
+	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) {
+		/* Platform doesn't support DYTC */
+		return -ENODEV;
+	}
+	if (!acpi_evalf(dytc_handle, output, NULL, "dd", command))
+		return -EIO;
+	return 0;
+}
+
+static int dytc_lapmode_get(bool *state)
+{
+	int output, err;
+
+	err = dytc_command(DYTC_CMD_GET, &output);
+	if (err)
+		return err;
+	*state = output & BIT(DYTC_GET_LAPMODE_BIT) ? true : false;
+	return 0;
+}
+
+static void dytc_lapmode_refresh(void)
+{
+	bool new_state;
+	int err;
+
+	err = dytc_lapmode_get(&new_state);
+	if (err || (new_state == dytc_lapmode))
+		return;
+
+	dytc_lapmode = new_state;
+	dytc_lapmode_notify_change();
+}
+
+/* sysfs lapmode entry */
+static ssize_t dytc_lapmode_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%d\n", dytc_lapmode);
+}
+
+static DEVICE_ATTR_RO(dytc_lapmode);
+
+static struct attribute *dytc_attributes[] = {
+	&dev_attr_dytc_lapmode.attr,
+	NULL,
+};
+
+static const struct attribute_group dytc_attr_group = {
+	.attrs = dytc_attributes,
+};
+
+static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
+{
+	int 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
+	 */
+	if (err == -ENODEV)
+		return 0;
+	/* For all other errors we can flag the failure */
+	if (err)
+		return err;
+
+	/* Platform supports this feature - create the group */
+	err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
+	return err;
+}
+
+static void dytc_exit(void)
+{
+	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
+}
+
+static struct ibm_struct dytc_driver_data = {
+	.name = "dytc",
+	.exit = dytc_exit,
+};
+
 /****************************************************************************
  ****************************************************************************
  *
@@ -9842,6 +9941,10 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
 
 		mutex_unlock(&kbdlight_mutex);
 	}
+
+	if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED)
+		dytc_lapmode_refresh();
+
 }
 
 static void hotkey_driver_event(const unsigned int scancode)
@@ -10280,6 +10383,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
 		.init = tpacpi_lcdshadow_init,
 		.data = &lcdshadow_driver_data,
 	},
+	{
+		.init = tpacpi_dytc_init,
+		.data = &dytc_driver_data,
+	},
 };
 
 static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
-- 
2.26.2

Re: [PATCH v5] platform/x86: thinkpad_acpi: lap or desk mode interface

[Andy Shevchenko]

On Fri, Jul 3, 2020 at 4:24 AM Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org> wrote:
>
> Newer Lenovo Thinkpad platforms have support to identify whether the
> system is on-lap or not using an ACPI DYTC event from the firmware.
>
> This patch provides the ability to retrieve the current mode via sysfs
> entrypoints and will be used by userspace for thermal mode and WWAN
> functionality

Hans, do you think it's good to have custom ABI for this? I think you
may be know better what types of ABI we already have for such thing.

> Co-developed-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Sugumaran <slacshiminar-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Bastien Nocera <bnocera-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> ---
> Changes in v5:
>  - Updated with review changes from Andy Shevchenko
>  - Added ABI information to thinkpad-acpi.rst
>  - improved error handling and parameter passing as recommended
>  - code cleanup as recommended
>  - added review tag from bnocera
> Changes in v4:
>  - Correct hotkey event comment as we're handling event
>  - Remove unnecessary check in dytc_lapmode_refresh
> Changes in v3:
> - Fixed inaccurate comments
> - Used BIT macro to check lapmode bit setting as recommended and update
>   define name
> - Check for new_state == dytc_lapmode in dytc_lapmode_refresh
> Changes in v2:
> - cleaned up initialisation sequence to be cleaner and avoid spamming
>   platforms that don't have DYTC with warning message. Tested on P52
> - Adding platform-driver-x86 mailing list for review as requested
>
>  .../admin-guide/laptops/thinkpad-acpi.rst     |  15 +++
>  drivers/platform/x86/thinkpad_acpi.c          | 111 +++++++++++++++++-
>  2 files changed, 124 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> index 822907dcc845..99066aa8d97b 100644
> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> @@ -50,6 +50,7 @@ detailed description):
>         - WAN enable and disable
>         - UWB enable and disable
>         - LCD Shadow (PrivacyGuard) enable and disable
> +       - Lap mode sensor
>
>  A compatibility table by model and feature is maintained on the web
>  site, http://ibm-acpi.sf.net/. I appreciate any success or failure
> @@ -1432,6 +1433,20 @@ The first command ensures the best viewing angle and the latter one turns
>  on the feature, restricting the viewing angles.
>
>
> +DYTC Lapmode sensor
> +------------------
> +
> +sysfs: dytc_lapmode
> +
> +Newer thinkpads and mobile workstations have the ability to determine if
> +the device is in deskmode or lapmode. This feature is used by user space
> +to decide if WWAN transmission can be increased to maximum power and is
> +also useful for understanding the different thermal modes available as
> +they differ between desk and lap mode.
> +
> +The property is read-only. If the platform doesn't have support the sysfs
> +class is not created.
> +
>  EXPERIMENTAL: UWB
>  -----------------
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index ff7f0a4f2475..037eb77414f9 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -4022,8 +4022,8 @@ static bool hotkey_notify_6xxx(const u32 hkey,
>                 return true;
>         case TP_HKEY_EV_THM_CSM_COMPLETED:
>                 pr_debug("EC reports: Thermal Control Command set completed (DYTC)\n");
> -               /* recommended action: do nothing, we don't have
> -                * Lenovo ATM information */
> +               /* Thermal event - pass on to event handler */
> +               tpacpi_driver_event(hkey);
>                 return true;
>         case TP_HKEY_EV_THM_TRANSFM_CHANGED:
>                 pr_debug("EC reports: Thermal Transformation changed (GMTS)\n");
> @@ -9795,6 +9795,105 @@ static struct ibm_struct lcdshadow_driver_data = {
>         .write = lcdshadow_write,
>  };
>
> +/*************************************************************************
> + * DYTC subdriver, for the Lenovo lapmode feature
> + */
> +
> +#define DYTC_CMD_GET          2 /* To get current IC function and mode */
> +#define DYTC_GET_LAPMODE_BIT 17 /* Set when in lapmode */
> +
> +static bool dytc_lapmode;
> +
> +static void dytc_lapmode_notify_change(void)
> +{
> +       sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "dytc_lapmode");
> +}
> +
> +static int dytc_command(int command, int *output)
> +{
> +       acpi_handle dytc_handle;
> +
> +       if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) {
> +               /* Platform doesn't support DYTC */
> +               return -ENODEV;
> +       }
> +       if (!acpi_evalf(dytc_handle, output, NULL, "dd", command))
> +               return -EIO;
> +       return 0;
> +}
> +
> +static int dytc_lapmode_get(bool *state)
> +{
> +       int output, err;
> +
> +       err = dytc_command(DYTC_CMD_GET, &output);
> +       if (err)
> +               return err;
> +       *state = output & BIT(DYTC_GET_LAPMODE_BIT) ? true : false;
> +       return 0;
> +}
> +
> +static void dytc_lapmode_refresh(void)
> +{
> +       bool new_state;
> +       int err;
> +
> +       err = dytc_lapmode_get(&new_state);
> +       if (err || (new_state == dytc_lapmode))
> +               return;
> +
> +       dytc_lapmode = new_state;
> +       dytc_lapmode_notify_change();
> +}
> +
> +/* sysfs lapmode entry */
> +static ssize_t dytc_lapmode_show(struct device *dev,
> +                                       struct device_attribute *attr,
> +                                       char *buf)
> +{
> +       return snprintf(buf, PAGE_SIZE, "%d\n", dytc_lapmode);
> +}
> +
> +static DEVICE_ATTR_RO(dytc_lapmode);
> +
> +static struct attribute *dytc_attributes[] = {
> +       &dev_attr_dytc_lapmode.attr,
> +       NULL,
> +};
> +
> +static const struct attribute_group dytc_attr_group = {
> +       .attrs = dytc_attributes,
> +};
> +
> +static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
> +{
> +       int 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
> +        */
> +       if (err == -ENODEV)
> +               return 0;
> +       /* For all other errors we can flag the failure */
> +       if (err)
> +               return err;
> +
> +       /* Platform supports this feature - create the group */
> +       err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
> +       return err;
> +}
> +
> +static void dytc_exit(void)
> +{
> +       sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
> +}
> +
> +static struct ibm_struct dytc_driver_data = {
> +       .name = "dytc",
> +       .exit = dytc_exit,
> +};
> +
>  /****************************************************************************
>   ****************************************************************************
>   *
> @@ -9842,6 +9941,10 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
>
>                 mutex_unlock(&kbdlight_mutex);
>         }
> +
> +       if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED)
> +               dytc_lapmode_refresh();
> +
>  }
>
>  static void hotkey_driver_event(const unsigned int scancode)
> @@ -10280,6 +10383,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
>                 .init = tpacpi_lcdshadow_init,
>                 .data = &lcdshadow_driver_data,
>         },
> +       {
> +               .init = tpacpi_dytc_init,
> +               .data = &dytc_driver_data,
> +       },
>  };
>
>  static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
> --
> 2.26.2
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5] platform/x86: thinkpad_acpi: lap or desk mode interface

[Hans de Goede]

Hi,

On 7/9/20 8:02 PM, Andy Shevchenko wrote:
> On Fri, Jul 3, 2020 at 4:24 AM Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org> wrote:
>>
>> Newer Lenovo Thinkpad platforms have support to identify whether the
>> system is on-lap or not using an ACPI DYTC event from the firmware.
>>
>> This patch provides the ability to retrieve the current mode via sysfs
>> entrypoints and will be used by userspace for thermal mode and WWAN
>> functionality
> 
> Hans, do you think it's good to have custom ABI for this? I think you
> may be know better what types of ABI we already have for such thing.

Actually, Mark asked me the same question before submitting his
patch upstream. I'm never a fan of custom ABI for this. But for now
the solution Lenovo has chosen to deal with thermal management
issues on modern hw is unique to Lenovo and we do not have anything
like this anywhere else.

So for now I believe that a custom ABI is best.

If we see this becoming a common feature on more platforms then we can
design a generic API for it once we have a better idea how this would
look like when implemented by others and then thinkpad_acpi can easily
add support for the new generic interface, while keeping its own
custom interface for backward compatibility.

Regards,

Hans



> 
>> Co-developed-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
>> Reviewed-by: Sugumaran <slacshiminar-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
>> Reviewed-by: Bastien Nocera <bnocera-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
>> ---
>> Changes in v5:
>>   - Updated with review changes from Andy Shevchenko
>>   - Added ABI information to thinkpad-acpi.rst
>>   - improved error handling and parameter passing as recommended
>>   - code cleanup as recommended
>>   - added review tag from bnocera
>> Changes in v4:
>>   - Correct hotkey event comment as we're handling event
>>   - Remove unnecessary check in dytc_lapmode_refresh
>> Changes in v3:
>> - Fixed inaccurate comments
>> - Used BIT macro to check lapmode bit setting as recommended and update
>>    define name
>> - Check for new_state == dytc_lapmode in dytc_lapmode_refresh
>> Changes in v2:
>> - cleaned up initialisation sequence to be cleaner and avoid spamming
>>    platforms that don't have DYTC with warning message. Tested on P52
>> - Adding platform-driver-x86 mailing list for review as requested
>>
>>   .../admin-guide/laptops/thinkpad-acpi.rst     |  15 +++
>>   drivers/platform/x86/thinkpad_acpi.c          | 111 +++++++++++++++++-
>>   2 files changed, 124 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> index 822907dcc845..99066aa8d97b 100644
>> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> @@ -50,6 +50,7 @@ detailed description):
>>          - WAN enable and disable
>>          - UWB enable and disable
>>          - LCD Shadow (PrivacyGuard) enable and disable
>> +       - Lap mode sensor
>>
>>   A compatibility table by model and feature is maintained on the web
>>   site, http://ibm-acpi.sf.net/. I appreciate any success or failure
>> @@ -1432,6 +1433,20 @@ The first command ensures the best viewing angle and the latter one turns
>>   on the feature, restricting the viewing angles.
>>
>>
>> +DYTC Lapmode sensor
>> +------------------
>> +
>> +sysfs: dytc_lapmode
>> +
>> +Newer thinkpads and mobile workstations have the ability to determine if
>> +the device is in deskmode or lapmode. This feature is used by user space
>> +to decide if WWAN transmission can be increased to maximum power and is
>> +also useful for understanding the different thermal modes available as
>> +they differ between desk and lap mode.
>> +
>> +The property is read-only. If the platform doesn't have support the sysfs
>> +class is not created.
>> +
>>   EXPERIMENTAL: UWB
>>   -----------------
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>> index ff7f0a4f2475..037eb77414f9 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -4022,8 +4022,8 @@ static bool hotkey_notify_6xxx(const u32 hkey,
>>                  return true;
>>          case TP_HKEY_EV_THM_CSM_COMPLETED:
>>                  pr_debug("EC reports: Thermal Control Command set completed (DYTC)\n");
>> -               /* recommended action: do nothing, we don't have
>> -                * Lenovo ATM information */
>> +               /* Thermal event - pass on to event handler */
>> +               tpacpi_driver_event(hkey);
>>                  return true;
>>          case TP_HKEY_EV_THM_TRANSFM_CHANGED:
>>                  pr_debug("EC reports: Thermal Transformation changed (GMTS)\n");
>> @@ -9795,6 +9795,105 @@ static struct ibm_struct lcdshadow_driver_data = {
>>          .write = lcdshadow_write,
>>   };
>>
>> +/*************************************************************************
>> + * DYTC subdriver, for the Lenovo lapmode feature
>> + */
>> +
>> +#define DYTC_CMD_GET          2 /* To get current IC function and mode */
>> +#define DYTC_GET_LAPMODE_BIT 17 /* Set when in lapmode */
>> +
>> +static bool dytc_lapmode;
>> +
>> +static void dytc_lapmode_notify_change(void)
>> +{
>> +       sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "dytc_lapmode");
>> +}
>> +
>> +static int dytc_command(int command, int *output)
>> +{
>> +       acpi_handle dytc_handle;
>> +
>> +       if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) {
>> +               /* Platform doesn't support DYTC */
>> +               return -ENODEV;
>> +       }
>> +       if (!acpi_evalf(dytc_handle, output, NULL, "dd", command))
>> +               return -EIO;
>> +       return 0;
>> +}
>> +
>> +static int dytc_lapmode_get(bool *state)
>> +{
>> +       int output, err;
>> +
>> +       err = dytc_command(DYTC_CMD_GET, &output);
>> +       if (err)
>> +               return err;
>> +       *state = output & BIT(DYTC_GET_LAPMODE_BIT) ? true : false;
>> +       return 0;
>> +}
>> +
>> +static void dytc_lapmode_refresh(void)
>> +{
>> +       bool new_state;
>> +       int err;
>> +
>> +       err = dytc_lapmode_get(&new_state);
>> +       if (err || (new_state == dytc_lapmode))
>> +               return;
>> +
>> +       dytc_lapmode = new_state;
>> +       dytc_lapmode_notify_change();
>> +}
>> +
>> +/* sysfs lapmode entry */
>> +static ssize_t dytc_lapmode_show(struct device *dev,
>> +                                       struct device_attribute *attr,
>> +                                       char *buf)
>> +{
>> +       return snprintf(buf, PAGE_SIZE, "%d\n", dytc_lapmode);
>> +}
>> +
>> +static DEVICE_ATTR_RO(dytc_lapmode);
>> +
>> +static struct attribute *dytc_attributes[] = {
>> +       &dev_attr_dytc_lapmode.attr,
>> +       NULL,
>> +};
>> +
>> +static const struct attribute_group dytc_attr_group = {
>> +       .attrs = dytc_attributes,
>> +};
>> +
>> +static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
>> +{
>> +       int 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
>> +        */
>> +       if (err == -ENODEV)
>> +               return 0;
>> +       /* For all other errors we can flag the failure */
>> +       if (err)
>> +               return err;
>> +
>> +       /* Platform supports this feature - create the group */
>> +       err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
>> +       return err;
>> +}
>> +
>> +static void dytc_exit(void)
>> +{
>> +       sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
>> +}
>> +
>> +static struct ibm_struct dytc_driver_data = {
>> +       .name = "dytc",
>> +       .exit = dytc_exit,
>> +};
>> +
>>   /****************************************************************************
>>    ****************************************************************************
>>    *
>> @@ -9842,6 +9941,10 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
>>
>>                  mutex_unlock(&kbdlight_mutex);
>>          }
>> +
>> +       if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED)
>> +               dytc_lapmode_refresh();
>> +
>>   }
>>
>>   static void hotkey_driver_event(const unsigned int scancode)
>> @@ -10280,6 +10383,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
>>                  .init = tpacpi_lcdshadow_init,
>>                  .data = &lcdshadow_driver_data,
>>          },
>> +       {
>> +               .init = tpacpi_dytc_init,
>> +               .data = &dytc_driver_data,
>> +       },
>>   };
>>
>>   static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
>> --
>> 2.26.2
>>
> 
> 

Re: [PATCH v5] platform/x86: thinkpad_acpi: lap or desk mode interface

[Andy Shevchenko]

On Fri, Jul 10, 2020 at 11:00 AM Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
> Hi,
>
> On 7/9/20 8:02 PM, Andy Shevchenko wrote:
> > On Fri, Jul 3, 2020 at 4:24 AM Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org> wrote:
> >>
> >> Newer Lenovo Thinkpad platforms have support to identify whether the
> >> system is on-lap or not using an ACPI DYTC event from the firmware.
> >>
> >> This patch provides the ability to retrieve the current mode via sysfs
> >> entrypoints and will be used by userspace for thermal mode and WWAN
> >> functionality
> >
> > Hans, do you think it's good to have custom ABI for this? I think you
> > may be know better what types of ABI we already have for such thing.
>
> Actually, Mark asked me the same question before submitting his
> patch upstream. I'm never a fan of custom ABI for this. But for now
> the solution Lenovo has chosen to deal with thermal management
> issues on modern hw is unique to Lenovo and we do not have anything
> like this anywhere else.
>
> So for now I believe that a custom ABI is best.
>
> If we see this becoming a common feature on more platforms then we can
> design a generic API for it once we have a better idea how this would
> look like when implemented by others and then thinkpad_acpi can easily
> add support for the new generic interface, while keeping its own
> custom interface for backward compatibility.

Thank you very much for the elaborative comment, appreciated!

-- 
With Best Regards,
Andy Shevchenko

Re: [External] Re: [PATCH v5] platform/x86: thinkpad_acpi: lap or desk mode interface

[Mark Pearson]

On 7/10/2020 8:20 AM, Andy Shevchenko wrote:
> On Fri, Jul 10, 2020 at 11:00 AM Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>
>> Hi,
>>
>> On 7/9/20 8:02 PM, Andy Shevchenko wrote:
>>> On Fri, Jul 3, 2020 at 4:24 AM Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org> wrote:
>>>>
>>>> Newer Lenovo Thinkpad platforms have support to identify whether the
>>>> system is on-lap or not using an ACPI DYTC event from the firmware.
>>>>
>>>> This patch provides the ability to retrieve the current mode via sysfs
>>>> entrypoints and will be used by userspace for thermal mode and WWAN
>>>> functionality
>>>
>>> Hans, do you think it's good to have custom ABI for this? I think you
>>> may be know better what types of ABI we already have for such thing.
>>
>> Actually, Mark asked me the same question before submitting his
>> patch upstream. I'm never a fan of custom ABI for this. But for now
>> the solution Lenovo has chosen to deal with thermal management
>> issues on modern hw is unique to Lenovo and we do not have anything
>> like this anywhere else.
>>
>> So for now I believe that a custom ABI is best.
>>
>> If we see this becoming a common feature on more platforms then we can
>> design a generic API for it once we have a better idea how this would
>> look like when implemented by others and then thinkpad_acpi can easily
>> add support for the new generic interface, while keeping its own
>> custom interface for backward compatibility.
> 
> Thank you very much for the elaborative comment, appreciated!
> 
Yes, thanks Hans from me too.

Just to note, I'm very happy to work on or contribute to a generic 
framework if that makes sense in the future. I'd likely need some help & 
guidance on the best way to do it but if you see a need let me know.

Andy - let me know if you need anything else for this patch. I have a 
follow on patch for a similar sensor that I'll send up once this one is 
approved - I figured we should learn from our mistakes on this one 
before duplicating the errors.

Thanks all for the support, reviews and guidance

Mark

Re: [PATCH v5] platform/x86: thinkpad_acpi: lap or desk mode interface

[Andy Shevchenko]

On Fri, Jul 3, 2020 at 4:24 AM Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org> wrote:
>
> Newer Lenovo Thinkpad platforms have support to identify whether the
> system is on-lap or not using an ACPI DYTC event from the firmware.
>
> This patch provides the ability to retrieve the current mode via sysfs
> entrypoints and will be used by userspace for thermal mode and WWAN
> functionality
>

Pushed to my review and testing queue, thanks!

> Co-developed-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Sugumaran <slacshiminar-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Bastien Nocera <bnocera-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> ---
> Changes in v5:
>  - Updated with review changes from Andy Shevchenko
>  - Added ABI information to thinkpad-acpi.rst
>  - improved error handling and parameter passing as recommended
>  - code cleanup as recommended
>  - added review tag from bnocera
> Changes in v4:
>  - Correct hotkey event comment as we're handling event
>  - Remove unnecessary check in dytc_lapmode_refresh
> Changes in v3:
> - Fixed inaccurate comments
> - Used BIT macro to check lapmode bit setting as recommended and update
>   define name
> - Check for new_state == dytc_lapmode in dytc_lapmode_refresh
> Changes in v2:
> - cleaned up initialisation sequence to be cleaner and avoid spamming
>   platforms that don't have DYTC with warning message. Tested on P52
> - Adding platform-driver-x86 mailing list for review as requested
>
>  .../admin-guide/laptops/thinkpad-acpi.rst     |  15 +++
>  drivers/platform/x86/thinkpad_acpi.c          | 111 +++++++++++++++++-
>  2 files changed, 124 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> index 822907dcc845..99066aa8d97b 100644
> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> @@ -50,6 +50,7 @@ detailed description):
>         - WAN enable and disable
>         - UWB enable and disable
>         - LCD Shadow (PrivacyGuard) enable and disable
> +       - Lap mode sensor
>
>  A compatibility table by model and feature is maintained on the web
>  site, http://ibm-acpi.sf.net/. I appreciate any success or failure
> @@ -1432,6 +1433,20 @@ The first command ensures the best viewing angle and the latter one turns
>  on the feature, restricting the viewing angles.
>
>
> +DYTC Lapmode sensor
> +------------------
> +
> +sysfs: dytc_lapmode
> +
> +Newer thinkpads and mobile workstations have the ability to determine if
> +the device is in deskmode or lapmode. This feature is used by user space
> +to decide if WWAN transmission can be increased to maximum power and is
> +also useful for understanding the different thermal modes available as
> +they differ between desk and lap mode.
> +
> +The property is read-only. If the platform doesn't have support the sysfs
> +class is not created.
> +
>  EXPERIMENTAL: UWB
>  -----------------
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index ff7f0a4f2475..037eb77414f9 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -4022,8 +4022,8 @@ static bool hotkey_notify_6xxx(const u32 hkey,
>                 return true;
>         case TP_HKEY_EV_THM_CSM_COMPLETED:
>                 pr_debug("EC reports: Thermal Control Command set completed (DYTC)\n");
> -               /* recommended action: do nothing, we don't have
> -                * Lenovo ATM information */
> +               /* Thermal event - pass on to event handler */
> +               tpacpi_driver_event(hkey);
>                 return true;
>         case TP_HKEY_EV_THM_TRANSFM_CHANGED:
>                 pr_debug("EC reports: Thermal Transformation changed (GMTS)\n");
> @@ -9795,6 +9795,105 @@ static struct ibm_struct lcdshadow_driver_data = {
>         .write = lcdshadow_write,
>  };
>
> +/*************************************************************************
> + * DYTC subdriver, for the Lenovo lapmode feature
> + */
> +
> +#define DYTC_CMD_GET          2 /* To get current IC function and mode */
> +#define DYTC_GET_LAPMODE_BIT 17 /* Set when in lapmode */
> +
> +static bool dytc_lapmode;
> +
> +static void dytc_lapmode_notify_change(void)
> +{
> +       sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "dytc_lapmode");
> +}
> +
> +static int dytc_command(int command, int *output)
> +{
> +       acpi_handle dytc_handle;
> +
> +       if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) {
> +               /* Platform doesn't support DYTC */
> +               return -ENODEV;
> +       }
> +       if (!acpi_evalf(dytc_handle, output, NULL, "dd", command))
> +               return -EIO;
> +       return 0;
> +}
> +
> +static int dytc_lapmode_get(bool *state)
> +{
> +       int output, err;
> +
> +       err = dytc_command(DYTC_CMD_GET, &output);
> +       if (err)
> +               return err;
> +       *state = output & BIT(DYTC_GET_LAPMODE_BIT) ? true : false;
> +       return 0;
> +}
> +
> +static void dytc_lapmode_refresh(void)
> +{
> +       bool new_state;
> +       int err;
> +
> +       err = dytc_lapmode_get(&new_state);
> +       if (err || (new_state == dytc_lapmode))
> +               return;
> +
> +       dytc_lapmode = new_state;
> +       dytc_lapmode_notify_change();
> +}
> +
> +/* sysfs lapmode entry */
> +static ssize_t dytc_lapmode_show(struct device *dev,
> +                                       struct device_attribute *attr,
> +                                       char *buf)
> +{
> +       return snprintf(buf, PAGE_SIZE, "%d\n", dytc_lapmode);
> +}
> +
> +static DEVICE_ATTR_RO(dytc_lapmode);
> +
> +static struct attribute *dytc_attributes[] = {
> +       &dev_attr_dytc_lapmode.attr,
> +       NULL,
> +};
> +
> +static const struct attribute_group dytc_attr_group = {
> +       .attrs = dytc_attributes,
> +};
> +
> +static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
> +{
> +       int 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
> +        */
> +       if (err == -ENODEV)
> +               return 0;
> +       /* For all other errors we can flag the failure */
> +       if (err)
> +               return err;
> +
> +       /* Platform supports this feature - create the group */
> +       err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
> +       return err;
> +}
> +
> +static void dytc_exit(void)
> +{
> +       sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
> +}
> +
> +static struct ibm_struct dytc_driver_data = {
> +       .name = "dytc",
> +       .exit = dytc_exit,
> +};
> +
>  /****************************************************************************
>   ****************************************************************************
>   *
> @@ -9842,6 +9941,10 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
>
>                 mutex_unlock(&kbdlight_mutex);
>         }
> +
> +       if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED)
> +               dytc_lapmode_refresh();
> +
>  }
>
>  static void hotkey_driver_event(const unsigned int scancode)
> @@ -10280,6 +10383,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
>                 .init = tpacpi_lcdshadow_init,
>                 .data = &lcdshadow_driver_data,
>         },
> +       {
> +               .init = tpacpi_dytc_init,
> +               .data = &dytc_driver_data,
> +       },
>  };
>
>  static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
> --
> 2.26.2
>


-- 
With Best Regards,
Andy Shevchenko

[PATCH] platform/x86: thinkpad_acpi: psensor interface

[Mark Pearson]

Some Lenovo Thinkpad platforms are equipped with a 'palm sensor' so as
to be able to determine if a user is physically proximate to the device.

This patch provides the ability to retrieve the psensor state via sysfs
entrypoints and will be used by userspace for WWAN functionality to
control the transmission level safely

Co-developed-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
---
 .../admin-guide/laptops/thinkpad-acpi.rst     |  19 ++++
 drivers/platform/x86/thinkpad_acpi.c          | 104 +++++++++++++++++-
 2 files changed, 121 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
index 99066aa8d97b..c2c238d5d5f6 100644
--- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
+++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
@@ -51,6 +51,7 @@ detailed description):
 	- UWB enable and disable
 	- LCD Shadow (PrivacyGuard) enable and disable
 	- Lap mode sensor
+        - Palm sensor (aka p-sensor)
 
 A compatibility table by model and feature is maintained on the web
 site, http://ibm-acpi.sf.net/. I appreciate any success or failure
@@ -1447,6 +1448,24 @@ they differ between desk and lap mode.
 The property is read-only. If the platform doesn't have support the sysfs
 class is not created.
 
+Palm sensor
+------------------
+
+sysfs: psensor_state
+
+Certain thinkpads and mobile workstations are equipped with a palm sensor to
+detect when a user is physically near the device. This device, when present,
+is used in conjunction with the lapmode sensor to decide if WWAN transmission
+can be increased to maximum power.
+
+The property is read-only. If the platform doesn't have support the sysfs
+class is not created.
+
+Note - some platforms have a limitation whereby the EC firmware cannot
+determine if the sensor is not installed or not. On these platforms the
+p-sensor state will always be reported as true to avoid high power being used
+incorrectly.
+
 EXPERIMENTAL: UWB
 -----------------
 
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 037eb77414f9..40f2e368fdf9 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -4071,8 +4071,8 @@ static bool hotkey_notify_6xxx(const u32 hkey,
 
 	case TP_HKEY_EV_PALM_DETECTED:
 	case TP_HKEY_EV_PALM_UNDETECTED:
-		/* palm detected hovering the keyboard, forward to user-space
-		 * via netlink for consumption */
+		/* palm detected - pass on to event handler */
+		tpacpi_driver_event(hkey);
 		return true;
 
 	default:
@@ -9894,6 +9894,99 @@ static struct ibm_struct dytc_driver_data = {
 	.exit = dytc_exit,
 };
 
+/*************************************************************************
+ * Palm sensor subdriver
+ */
+#define PSENSOR_PRESENT_BIT 0 /* Determine if psensor present */
+#define PSENSOR_ON_BIT      1 /* psensor status */
+
+static bool psensor_state;
+
+static void psensor_notify_change(void)
+{
+	sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "psensor_state");
+}
+
+static int psensor_get(bool *state)
+{
+	acpi_handle psensor_handle;
+	int output;
+
+	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "GPSS", &psensor_handle)))
+		return -ENODEV;
+
+	if (!acpi_evalf(psensor_handle, &output, NULL, "d"))
+		return -EIO;
+
+	/* Check if sensor has a Psensor */
+	if (!(output & BIT(PSENSOR_PRESENT_BIT)))
+		return -ENODEV;
+
+	/* Return if psensor is set or not */
+	*state = output & BIT(PSENSOR_ON_BIT) ? true : false;
+	return 0;
+}
+
+static void psensor_state_refresh(void)
+{
+	bool new_state;
+	int err;
+
+	err = psensor_get(&new_state);
+	if (err || (new_state == psensor_state))
+		return;
+
+	psensor_state = new_state;
+	psensor_notify_change();
+}
+
+/* sysfs psensor entry */
+static ssize_t psensor_state_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%d\n", psensor_state);
+}
+
+static DEVICE_ATTR_RO(psensor_state);
+
+static struct attribute *psensor_attributes[] = {
+	&dev_attr_psensor_state.attr,
+	NULL,
+};
+
+static const struct attribute_group psensor_attr_group = {
+	.attrs = psensor_attributes,
+};
+
+static int tpacpi_psensor_init(struct ibm_init_struct *iibm)
+{
+	int err;
+
+	err = psensor_get(&psensor_state);
+	/* If support isn't available (ENODEV) then don't return an error
+	 * but just don't create the sysfs group
+	 */
+	if (err == -ENODEV)
+		return 0;
+	/* For all other errors we can flag the failure */
+	if (err)
+		return err;
+
+	err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &psensor_attr_group);
+	return err;
+}
+
+static void psensor_exit(void)
+{
+	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &psensor_attr_group);
+}
+
+static struct ibm_struct psensor_driver_data = {
+	.name = "psensor",
+	.exit = psensor_exit,
+};
+
 /****************************************************************************
  ****************************************************************************
  *
@@ -9945,6 +10038,9 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
 	if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED)
 		dytc_lapmode_refresh();
 
+	if ((hkey_event == TP_HKEY_EV_PALM_DETECTED) ||
+		(hkey_event == TP_HKEY_EV_PALM_UNDETECTED))
+		psensor_state_refresh();
 }
 
 static void hotkey_driver_event(const unsigned int scancode)
@@ -10387,6 +10483,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
 		.init = tpacpi_dytc_init,
 		.data = &dytc_driver_data,
 	},
+	{
+		.init = tpacpi_psensor_init,
+		.data = &psensor_driver_data,
+	},
 };
 
 static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
-- 
2.26.2

Re: [PATCH] platform/x86: thinkpad_acpi: psensor interface

[Andy Shevchenko]

On Thu, Jul 16, 2020 at 2:53 AM Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org> wrote:
>
> Some Lenovo Thinkpad platforms are equipped with a 'palm sensor' so as
> to be able to determine if a user is physically proximate to the device.
>
> This patch provides the ability to retrieve the psensor state via sysfs
> entrypoints and will be used by userspace for WWAN functionality to
> control the transmission level safely

...


>         case TP_HKEY_EV_PALM_DETECTED:
>         case TP_HKEY_EV_PALM_UNDETECTED:
> -               /* palm detected hovering the keyboard, forward to user-space
> -                * via netlink for consumption */
> +               /* palm detected - pass on to event handler */
> +               tpacpi_driver_event(hkey);
>                 return true;

Comment here tells something about the netlink interface to user
space. Can you elaborate why we need sysfs now and how it's all
supposed to work?

...

> +static int psensor_get(bool *state)
> +{
> +       acpi_handle psensor_handle;
> +       int output;
> +
> +       if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "GPSS", &psensor_handle)))
> +               return -ENODEV;
> +
> +       if (!acpi_evalf(psensor_handle, &output, NULL, "d"))
> +               return -EIO;
> +
> +       /* Check if sensor has a Psensor */
> +       if (!(output & BIT(PSENSOR_PRESENT_BIT)))
> +               return -ENODEV;
> +
> +       /* Return if psensor is set or not */
> +       *state = output & BIT(PSENSOR_ON_BIT) ? true : false;
> +       return 0;
> +}

It reminds me of a function you created in one of the previous
changes. Can you rather create a parameterized helper which will serve
for both?

...

> +/* sysfs psensor entry */
> +static ssize_t psensor_state_show(struct device *dev,
> +                                       struct device_attribute *attr,
> +                                       char *buf)
> +{

> +       return snprintf(buf, PAGE_SIZE, "%d\n", psensor_state);

We know that %d takes much less than PAGE_SIZE, use sprintf().

> +}

> +

No blank line here.

> +static DEVICE_ATTR_RO(psensor_state);

...

> +static struct attribute *psensor_attributes[] = {
> +       &dev_attr_psensor_state.attr,

> +       NULL,

No comma for terminator line(s).

> +};

...

> +       /* If support isn't available (ENODEV) then don't return an error
> +        * but just don't create the sysfs group
> +        */

/*
 * Consider to use a proper multi-line comment style.
 * Like here. (It's applicable to the entire patch)
 */

...

> +       err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &psensor_attr_group);
> +       return err;

return sysfs...

-- 
With Best Regards,
Andy Shevchenko

Re: [External] Re: [PATCH] platform/x86: thinkpad_acpi: psensor interface

[Nitin Joshi1]

Hi Andy ,

>-----Original Message-----
>From: Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>Sent: Monday, July 27, 2020 7:35 PM
>On Thu, Jul 16, 2020 at 2:53 AM Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
>wrote:
>>
>>         case TP_HKEY_EV_PALM_DETECTED:
>>         case TP_HKEY_EV_PALM_UNDETECTED:
>> -               /* palm detected hovering the keyboard, forward to user-space
>> -                * via netlink for consumption */
>> +               /* palm detected - pass on to event handler */
>> +               tpacpi_driver_event(hkey);
>>                 return true;
>
>Comment here tells something about the netlink interface to user space.
>Can you elaborate why we need sysfs now and how it's all supposed to
>work?
Using  netlink , we were getting proximity events like '0x60b0' and '0x60b1' but for our WWAN requirement, we need default and current 
p-sensor state too .  
Some tools like "acpi-listen" uses netlink to display events but we need default and current p-sensor state also as per our requirement. 
hence , we have added new sysfs to get current p-sensor state using 'GPSS' method from BIOS .
This will be used for implementing "Dynamic power reduction" app which is used to control Body SAR value as per FCC requirement .
When Body or any object is near or away from p-sensor location on thinkpad system , then sysfs will be set .

>
>...
>
>> +static int psensor_get(bool *state)
>> +{
>> +       acpi_handle psensor_handle;
>> +       int output;
>> +
>> +       if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "GPSS",
>&psensor_handle)))
>> +               return -ENODEV;
>> +
>> +       if (!acpi_evalf(psensor_handle, &output, NULL, "d"))
>> +               return -EIO;
>> +
>> +       /* Check if sensor has a Psensor */
>> +       if (!(output & BIT(PSENSOR_PRESENT_BIT)))
>> +               return -ENODEV;
>> +
>> +       /* Return if psensor is set or not */
>> +       *state = output & BIT(PSENSOR_ON_BIT) ? true : false;
>> +       return 0;
>> +}
>
>It reminds me of a function you created in one of the previous changes. Can
>you rather create a parameterized helper which will serve for both?

Ack , we will check it .

>
>...
>
>> +/* sysfs psensor entry */
>> +static ssize_t psensor_state_show(struct device *dev,
>> +                                       struct device_attribute *attr,
>> +                                       char *buf) {
>
>> +       return snprintf(buf, PAGE_SIZE, "%d\n", psensor_state);
>
>We know that %d takes much less than PAGE_SIZE, use sprintf().
>
>> +}
>
>> +
>
>No blank line here.
>
Ack

>> +static DEVICE_ATTR_RO(psensor_state);
>
>...
>
>> +static struct attribute *psensor_attributes[] = {
>> +       &dev_attr_psensor_state.attr,
>
>> +       NULL,
>
>No comma for terminator line(s).
>

Ack 

>> +};
>
>...
>
>> +       /* If support isn't available (ENODEV) then don't return an error
>> +        * but just don't create the sysfs group
>> +        */
>
>/*
> * Consider to use a proper multi-line comment style.
> * Like here. (It's applicable to the entire patch)  */
>
>...
>
>> +       err = sysfs_create_group(&tpacpi_pdev->dev.kobj,
>&psensor_attr_group);
>> +       return err;
>
>return sysfs...
Ack 

Thanks & Regards,
Nitin Joshi

Re: [External] Re: [PATCH] platform/x86: thinkpad_acpi: psensor interface

[Mark Pearson]

Hi Andy,

First off apologies for my delay in replying and thanks to Nitin for 
covering for me. I took a week of PTO and then suffered the consequences 
of that for the week after - it's taken me a bit to catch-up.


On 7/27/2020 11:51 PM, Nitin Joshi1 wrote:
> Hi Andy ,
> 
>> -----Original Message-----
>> From: Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Sent: Monday, July 27, 2020 7:35 PM
>> On Thu, Jul 16, 2020 at 2:53 AM Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
>> wrote:
>>>
>>>          case TP_HKEY_EV_PALM_DETECTED:
>>>          case TP_HKEY_EV_PALM_UNDETECTED:
>>> -               /* palm detected hovering the keyboard, forward to user-space
>>> -                * via netlink for consumption */
>>> +               /* palm detected - pass on to event handler */
>>> +               tpacpi_driver_event(hkey);
>>>                  return true;
>>
>> Comment here tells something about the netlink interface to user space.
>> Can you elaborate why we need sysfs now and how it's all supposed to
>> work?
> Using  netlink , we were getting proximity events like '0x60b0' and '0x60b1' but for our WWAN requirement, we need default and current
> p-sensor state too .
> Some tools like "acpi-listen" uses netlink to display events but we need default and current p-sensor state also as per our requirement.
> hence , we have added new sysfs to get current p-sensor state using 'GPSS' method from BIOS .
> This will be used for implementing "Dynamic power reduction" app which is used to control Body SAR value as per FCC requirement .
> When Body or any object is near or away from p-sensor location on thinkpad system , then sysfs will be set .
> 
I think Nitin has covered it. Let us know if any follow on questions or 
concerns here.

>>
>> ...
>>
>>> +static int psensor_get(bool *state)
>>> +{
>>> +       acpi_handle psensor_handle;
>>> +       int output;
>>> +
>>> +       if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "GPSS",
>> &psensor_handle)))
>>> +               return -ENODEV;
>>> +
>>> +       if (!acpi_evalf(psensor_handle, &output, NULL, "d"))
>>> +               return -EIO;
>>> +
>>> +       /* Check if sensor has a Psensor */
>>> +       if (!(output & BIT(PSENSOR_PRESENT_BIT)))
>>> +               return -ENODEV;
>>> +
>>> +       /* Return if psensor is set or not */
>>> +       *state = output & BIT(PSENSOR_ON_BIT) ? true : false;
>>> +       return 0;
>>> +}
>>
>> It reminds me of a function you created in one of the previous changes. Can
>> you rather create a parameterized helper which will serve for both?
> 
> Ack , we will check it .
I've been looking at this and I understand where you're coming from but 
the benefits of combining them aren't working for me.

The previous change was for the lapmode sensor which is a separate piece 
of hardware. The ACPI handle and access format is different and the 
lapmode sensor has some extra handling logic needed. The code has enough 
differences too that I don't think combining it makes a lot of sense.

Let me know if I'm missing something obvious or if you disagree.
> 
>>
>> ...
>>
>>> +/* sysfs psensor entry */
>>> +static ssize_t psensor_state_show(struct device *dev,
>>> +                                       struct device_attribute *attr,
>>> +                                       char *buf) {
>>
>>> +       return snprintf(buf, PAGE_SIZE, "%d\n", psensor_state);
>>
>> We know that %d takes much less than PAGE_SIZE, use sprintf().
>>
>>> +}
>>
>>> +
>>
>> No blank line here.
>>
> Ack
> 
>>> +static DEVICE_ATTR_RO(psensor_state);
>>
>> ...
>>
>>> +static struct attribute *psensor_attributes[] = {
>>> +       &dev_attr_psensor_state.attr,
>>
>>> +       NULL,
>>
>> No comma for terminator line(s).
>>
> 
> Ack
> 
>>> +};
>>
>> ...
>>
>>> +       /* If support isn't available (ENODEV) then don't return an error
>>> +        * but just don't create the sysfs group
>>> +        */
>>
>> /*
>> * Consider to use a proper multi-line comment style.
>> * Like here. (It's applicable to the entire patch)  */
>>
>> ...
>>
>>> +       err = sysfs_create_group(&tpacpi_pdev->dev.kobj,
>> &psensor_attr_group);
>>> +       return err;
>>
>> return sysfs...
> Ack
> 
I'll push a patch soon with these other adjustments.

Thanks for the review
Mark

[PATCH] platform/x86: thinkpad_acpi: performance mode interface

[Mark Pearson]

Lenovo Thinkpad platforms with DYTC version 5 and newer have enhanced
firmware to provide different performance/thermal modes.

The modes can be controlled by hotkeys (FN+H, FN+M, FN+L) to switch
the operating mode between three different modes.

H - High performance. Maximum power is available and the temperature is
allowed to increase to the maximum for the platform.
M - Medium performance (aka balance). In this mode power will be limited
and the laptop will have a lower maximum temperature.
L - Low performance (aka quiet). In this mode power consumption is reduced
and the device will be cooler.

High performance mode is only available when the device is in 'desk mode'.
If the device detects that it is on a lap then it will automatically drop
into medium mode to maintain a safer operating temperature.

This patch provides an interface to determine the current mode and to also
allow the setting of the mode through the dytc_perfmode sysfs entry. This
can be used by userspace for improved control.

Reviewed-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
---
 .../admin-guide/laptops/thinkpad-acpi.rst     |  35 +++
 drivers/platform/x86/thinkpad_acpi.c          | 209 +++++++++++++++++-
 2 files changed, 235 insertions(+), 9 deletions(-)

diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
index 2e9d62903ead..d5fef0bb562b 100644
--- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
+++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
@@ -52,6 +52,7 @@ detailed description):
 	- LCD Shadow (PrivacyGuard) enable and disable
 	- Lap mode sensor
         - Palm sensor (aka psensor)
+        - Thermal mode status and control
 
 A compatibility table by model and feature is maintained on the web
 site, http://ibm-acpi.sf.net/. I appreciate any success or failure
@@ -1466,6 +1467,40 @@ determine if the sensor is not installed or not. On these platforms the
 psensor state will always be reported as true to avoid high power being used
 incorrectly.
 
+DYTC Thermal mode status and control
+------------------------------------
+
+sysfs: dytc_perfmode
+
+Lenovo Thinkpad platforms with DYTC version 5 and newer have enhanced firmware to
+provide improved performance control.
+
+The firmware can be controlled by hotkeys (FN+H, FN+M, FN+L) to switch the
+operating mode between three different modes. This sysfs node provide a better
+interface for user space to use
+
+The modes available are:
+
+H - High performance. Maximum power is available and the temperature is
+allowed to increase to the maximum for the platform.
+
+M - Medium performance (aka balance). In this mode power will be limited and
+the laptop will remain cooler.
+
+L - Low performance (aka quiet). In this mode power consumption is reduced and
+the device will be cooler and quieter
+
+Note: High performance mode is only available when the device is in 'deskmode'. If
+thde device detects that it is on a lap then it will automatically drop into medium
+mode to maintain a safer operating temperature.
+
+The sysfs entry provides the ability to return the current status and to set the
+desired mode. For example::
+
+        echo H > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
+        echo M > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
+        echo L > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
+
 EXPERIMENTAL: UWB
 -----------------
 
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 40f2e368fdf9..5aebaf1718b1 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -9799,10 +9799,33 @@ 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 */
 
 static bool dytc_lapmode;
+static bool dytc_mode_available;
 
 static void dytc_lapmode_notify_change(void)
 {
@@ -9822,7 +9845,81 @@ static int dytc_command(int command, int *output)
 	return 0;
 }
 
+static int dytc_perfmode_get(int *perfmode, int *funcmode)
+{
+	int output, err;
+
+	if (!dytc_mode_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.
+		 */
+		err = dytc_command(0x000F1001 /*Disable CQL*/, &dummy);
+		if (err)
+			return err;
+		err = dytc_command(DYTC_CMD_GET, &output);
+		if (err)
+			return err;
+		err = dytc_command(0x001F1001 /*Enable CQL*/, &dummy);
+		if (err)
+			return err;
+	}
+	*perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF;
+	return 0;
+}
+
+static int dytc_perfmode_set(int perfmode)
+{
+	int err, dytc_set;
+	int output;
+	int cur_perfmode, cur_funcmode;
+
+	if (!dytc_mode_available)
+		return -ENODEV;
+
+	if (perfmode == DYTC_MODE_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 {
+		/* 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*/
+			err = dytc_command(0x000F1001 /*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) {
+			err = dytc_command(0x001F1001 /*Enable CQL*/, &output);
+			if (err)
+				return err;
+		}
+	}
+	return 0;
+}
+
 static int dytc_lapmode_get(bool *state)
+
 {
 	int output, err;
 
@@ -9846,6 +9943,77 @@ static void dytc_lapmode_refresh(void)
 	dytc_lapmode_notify_change();
 }
 
+/* sysfs perfmode entry */
+static ssize_t dytc_perfmode_show(struct device *dev,
+				  struct device_attribute *attr,
+				  char *buf)
+{
+	int err;
+	int perfmode, funcmode;
+
+	err = dytc_perfmode_get(&perfmode, &funcmode);
+	if (err)
+		return err;
+
+	switch (perfmode) {
+	case DYTC_MODE_PERFORM:
+		/* High performance is only available in deskmode */
+		if (funcmode == DYTC_FUNCTION_CQL)
+			return snprintf(buf, PAGE_SIZE, "Medium (Reduced as lapmode active)\n");
+		else
+			return snprintf(buf, PAGE_SIZE, "High\n");
+	case DYTC_MODE_QUIET:
+		return snprintf(buf, PAGE_SIZE, "Low\n");
+	case DYTC_MODE_BALANCE:
+		return snprintf(buf, PAGE_SIZE, "Medium\n");
+	default:
+		return snprintf(buf, PAGE_SIZE, "Unknown (%d)\n", perfmode);
+	}
+}
+
+static ssize_t dytc_perfmode_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	int err;
+
+	switch (buf[0]) {
+	case 'l':
+	case 'L':
+		err = dytc_perfmode_set(DYTC_MODE_QUIET);
+		break;
+	case 'm':
+	case 'M':
+		err = dytc_perfmode_set(DYTC_MODE_BALANCE);
+		break;
+	case 'h':
+	case 'H':
+		err = dytc_perfmode_set(DYTC_MODE_PERFORM);
+		break;
+	default:
+		err = -EINVAL;
+		pr_err("Unknown operating mode. Ignoring\n");
+		break;
+	}
+	if (err)
+		return err;
+
+	tpacpi_disclose_usertask(attr->attr.name,
+				"Performance mode set to %c\n", buf[0]);
+	return count;
+}
+
+static DEVICE_ATTR_RW(dytc_perfmode);
+
+static struct attribute *dytc_perfmode_attributes[] = {
+	&dev_attr_dytc_perfmode.attr,
+	NULL,
+};
+
+static const struct attribute_group dytc_perf_attr_group = {
+	.attrs = dytc_perfmode_attributes,
+};
+
 /* sysfs lapmode entry */
 static ssize_t dytc_lapmode_show(struct device *dev,
 					struct device_attribute *attr,
@@ -9856,22 +10024,22 @@ static ssize_t dytc_lapmode_show(struct device *dev,
 
 static DEVICE_ATTR_RO(dytc_lapmode);
 
-static struct attribute *dytc_attributes[] = {
+static struct attribute *dytc_lap_attributes[] = {
 	&dev_attr_dytc_lapmode.attr,
 	NULL,
 };
 
-static const struct attribute_group dytc_attr_group = {
-	.attrs = dytc_attributes,
+static const struct attribute_group dytc_lap_attr_group = {
+	.attrs = dytc_lap_attributes,
 };
 
 static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
 {
-	int err;
+	int err, output;
 
-	err = dytc_lapmode_get(&dytc_lapmode);
+	err = dytc_command(DYTC_CMD_QUERY, &output);
 	/* If support isn't available (ENODEV) then don't return an error
-	 * but just don't create the sysfs group
+	 * just don't create the sysfs group
 	 */
 	if (err == -ENODEV)
 		return 0;
@@ -9879,14 +10047,37 @@ static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
 	if (err)
 		return err;
 
+	/* Check DYTC is enabled and supports mode setting */
+	dytc_mode_available = 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) {
+			pr_info("DYTC thermal mode configuration available\n");
+			dytc_mode_available = true;
+			/* Platform supports this feature - create the group */
+			err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &dytc_perf_attr_group);
+			if (err)
+				return err;
+		}
+	}
+
+	err = dytc_lapmode_get(&dytc_lapmode);
+	if (err)
+		return err;
+
 	/* Platform supports this feature - create the group */
-	err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
+	err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &dytc_lap_attr_group);
 	return err;
 }
 
 static void dytc_exit(void)
 {
-	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
+	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_lap_attr_group);
+	if (dytc_mode_available)
+		sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_perf_attr_group);
 }
 
 static struct ibm_struct dytc_driver_data = {
-- 
2.26.2

Re: [PATCH] platform/x86: thinkpad_acpi: performance mode interface

[Limonciello, Mario]

> 
> Lenovo Thinkpad platforms with DYTC version 5 and newer have enhanced
> firmware to provide different performance/thermal modes.
> 
> The modes can be controlled by hotkeys (FN+H, FN+M, FN+L) to switch
> the operating mode between three different modes.
> 
> H - High performance. Maximum power is available and the temperature is
> allowed to increase to the maximum for the platform.
> M - Medium performance (aka balance). In this mode power will be limited
> and the laptop will have a lower maximum temperature.
> L - Low performance (aka quiet). In this mode power consumption is reduced
> and the device will be cooler.
> 
> High performance mode is only available when the device is in 'desk mode'.
> If the device detects that it is on a lap then it will automatically drop
> into medium mode to maintain a safer operating temperature.
> 
> This patch provides an interface to determine the current mode and to also
> allow the setting of the mode through the dytc_perfmode sysfs entry. This
> can be used by userspace for improved control.
> 
> Reviewed-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> ---
>  .../admin-guide/laptops/thinkpad-acpi.rst     |  35 +++
>  drivers/platform/x86/thinkpad_acpi.c          | 209 +++++++++++++++++-
>  2 files changed, 235 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> index 2e9d62903ead..d5fef0bb562b 100644
> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> @@ -52,6 +52,7 @@ detailed description):
>  	- LCD Shadow (PrivacyGuard) enable and disable
>  	- Lap mode sensor
>          - Palm sensor (aka psensor)
> +        - Thermal mode status and control
> 
>  A compatibility table by model and feature is maintained on the web
>  site, http://ibm-acpi.sf.net/. I appreciate any success or failure
> @@ -1466,6 +1467,40 @@ determine if the sensor is not installed or not. On
> these platforms the
>  psensor state will always be reported as true to avoid high power being used
>  incorrectly.
> 
> +DYTC Thermal mode status and control
> +------------------------------------
> +
> +sysfs: dytc_perfmode
> +
> +Lenovo Thinkpad platforms with DYTC version 5 and newer have enhanced
> firmware to
> +provide improved performance control.
> +
> +The firmware can be controlled by hotkeys (FN+H, FN+M, FN+L) to switch the
> +operating mode between three different modes. This sysfs node provide a
> better
> +interface for user space to use

So is userspace also notified in some way when you use the hotkey to change, or
is the event usurped by the EC?  Is this by the event TP_HKEY_EV_THM_CSM_COMPLETED?

You might consider to mention what other interfaces will conflict with this
and document them and/or artificially block them when this is loaded to prevent
such a conflict.

> +
> +The modes available are:
> +
> +H - High performance. Maximum power is available and the temperature is
> +allowed to increase to the maximum for the platform.
> +
> +M - Medium performance (aka balance). In this mode power will be limited and
> +the laptop will remain cooler.
> +
> +L - Low performance (aka quiet). In this mode power consumption is reduced
> and
> +the device will be cooler and quieter
> +
> +Note: High performance mode is only available when the device is in
> 'deskmode'. If
> +thde device detects that it is on a lap then it will automatically drop into

the

> medium
> +mode to maintain a safer operating temperature.

I don't see an explicit notification path for this, how will userspace know about it?

Doesn't it need some sort of change uevent?
Or is this implied by the event TP_HKEY_EV_THM_CSM_COMPLETED?

> +
> +The sysfs entry provides the ability to return the current status and to set
> the
> +desired mode. For example::
> +
> +        echo H > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
> +        echo M > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
> +        echo L > /sys/devices/platform/thinkpad_acpi/dytc_perfmode

Doesn't this need ABI documentation submitted as part of the patch?

> +
>  EXPERIMENTAL: UWB
>  -----------------
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index 40f2e368fdf9..5aebaf1718b1 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -9799,10 +9799,33 @@ 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 */
> 
>  static bool dytc_lapmode;
> +static bool dytc_mode_available;
> 
>  static void dytc_lapmode_notify_change(void)
>  {
> @@ -9822,7 +9845,81 @@ static int dytc_command(int command, int *output)
>  	return 0;
>  }
> 
> +static int dytc_perfmode_get(int *perfmode, int *funcmode)
> +{
> +	int output, err;
> +
> +	if (!dytc_mode_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.
> +		 */
> +		err = dytc_command(0x000F1001 /*Disable CQL*/, &dummy);
> +		if (err)
> +			return err;
> +		err = dytc_command(DYTC_CMD_GET, &output);
> +		if (err)
> +			return err;
> +		err = dytc_command(0x001F1001 /*Enable CQL*/, &dummy);
> +		if (err)
> +			return err;
> +	}
> +	*perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF;
> +	return 0;
> +}
> +
> +static int dytc_perfmode_set(int perfmode)
> +{
> +	int err, dytc_set;
> +	int output;
> +	int cur_perfmode, cur_funcmode;
> +
> +	if (!dytc_mode_available)
> +		return -ENODEV;
> +
> +	if (perfmode == DYTC_MODE_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 {
> +		/* 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*/
> +			err = dytc_command(0x000F1001 /*Disable CQL*/, &output);

Why not put 0x000F1001 and 0x001F1001 as defines at the top?

> +			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) {
> +			err = dytc_command(0x001F1001 /*Enable CQL*/, &output);
> +			if (err)
> +				return err;
> +		}
> +	}
> +	return 0;
> +}
> +
>  static int dytc_lapmode_get(bool *state)
> +
>  {
>  	int output, err;
> 
> @@ -9846,6 +9943,77 @@ static void dytc_lapmode_refresh(void)
>  	dytc_lapmode_notify_change();
>  }
> 
> +/* sysfs perfmode entry */
> +static ssize_t dytc_perfmode_show(struct device *dev,
> +				  struct device_attribute *attr,
> +				  char *buf)
> +{
> +	int err;
> +	int perfmode, funcmode;
> +
> +	err = dytc_perfmode_get(&perfmode, &funcmode);
> +	if (err)
> +		return err;
> +
> +	switch (perfmode) {
> +	case DYTC_MODE_PERFORM:
> +		/* High performance is only available in deskmode */
> +		if (funcmode == DYTC_FUNCTION_CQL)
> +			return snprintf(buf, PAGE_SIZE, "Medium (Reduced as lapmode
> active)\n");
> +		else
> +			return snprintf(buf, PAGE_SIZE, "High\n");
> +	case DYTC_MODE_QUIET:
> +		return snprintf(buf, PAGE_SIZE, "Low\n");
> +	case DYTC_MODE_BALANCE:
> +		return snprintf(buf, PAGE_SIZE, "Medium\n");
> +	default:
> +		return snprintf(buf, PAGE_SIZE, "Unknown (%d)\n", perfmode);
> +	}
> +}

I think it's pretty confusing that you write "H/M/L" into this file, but you
get back a full string.

> +
> +static ssize_t dytc_perfmode_store(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	int err;
> +
> +	switch (buf[0]) {
> +	case 'l':
> +	case 'L':
> +		err = dytc_perfmode_set(DYTC_MODE_QUIET);
> +		break;
> +	case 'm':
> +	case 'M':
> +		err = dytc_perfmode_set(DYTC_MODE_BALANCE);
> +		break;
> +	case 'h':
> +	case 'H':
> +		err = dytc_perfmode_set(DYTC_MODE_PERFORM);
> +		break;
> +	default:
> +		err = -EINVAL;
> +		pr_err("Unknown operating mode. Ignoring\n");

Shouldn't this be dev_err?

> +		break;
> +	}
> +	if (err)
> +		return err;
> +
> +	tpacpi_disclose_usertask(attr->attr.name,
> +				"Performance mode set to %c\n", buf[0]);
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(dytc_perfmode);
> +
> +static struct attribute *dytc_perfmode_attributes[] = {
> +	&dev_attr_dytc_perfmode.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group dytc_perf_attr_group = {
> +	.attrs = dytc_perfmode_attributes,
> +};
> +
>  /* sysfs lapmode entry */
>  static ssize_t dytc_lapmode_show(struct device *dev,
>  					struct device_attribute *attr,
> @@ -9856,22 +10024,22 @@ static ssize_t dytc_lapmode_show(struct device *dev,
> 
>  static DEVICE_ATTR_RO(dytc_lapmode);
> 
> -static struct attribute *dytc_attributes[] = {
> +static struct attribute *dytc_lap_attributes[] = {
>  	&dev_attr_dytc_lapmode.attr,
>  	NULL,
>  };
> 
> -static const struct attribute_group dytc_attr_group = {
> -	.attrs = dytc_attributes,
> +static const struct attribute_group dytc_lap_attr_group = {
> +	.attrs = dytc_lap_attributes,
>  };
> 
>  static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
>  {
> -	int err;
> +	int err, output;
> 
> -	err = dytc_lapmode_get(&dytc_lapmode);
> +	err = dytc_command(DYTC_CMD_QUERY, &output);
>  	/* If support isn't available (ENODEV) then don't return an error
> -	 * but just don't create the sysfs group
> +	 * just don't create the sysfs group
>  	 */
>  	if (err == -ENODEV)
>  		return 0;
> @@ -9879,14 +10047,37 @@ static int tpacpi_dytc_init(struct ibm_init_struct
> *iibm)
>  	if (err)
>  		return err;
> 
> +	/* Check DYTC is enabled and supports mode setting */
> +	dytc_mode_available = 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) {
> +			pr_info("DYTC thermal mode configuration available\n");

I would argue this isn't useful to most people.
1) You should decrease this to debug for use with dynamic debugging
2) Output in the log what integer value you returned back in case of a need
to identify future firmware bugs.

> +			dytc_mode_available = true;

I think you shouldn't set this flag until after the group is actually created.

> +			/* Platform supports this feature - create the group */
> +			err = sysfs_create_group(&tpacpi_pdev->dev.kobj,
> &dytc_perf_attr_group);
> +			if (err)
> +				return err;
> +		}
> +	}
> +
> +	err = dytc_lapmode_get(&dytc_lapmode);
> +	if (err)
> +		return err;
> +
>  	/* Platform supports this feature - create the group */
> -	err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
> +	err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &dytc_lap_attr_group);
>  	return err;
>  }
> 
>  static void dytc_exit(void)
>  {
> -	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
> +	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_lap_attr_group);
> +	if (dytc_mode_available)
> +		sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_perf_attr_group);
>  }
> 
>  static struct ibm_struct dytc_driver_data = {
> --
> 2.26.2

Re: [External] RE: [PATCH] platform/x86: thinkpad_acpi: performance mode interface

[Mark Pearson]

Hi Mario

On 7/22/2020 2:46 PM, Limonciello, Mario wrote:
<snip>
>>
>> +DYTC Thermal mode status and control
>> +------------------------------------
>> +
>> +sysfs: dytc_perfmode
>> +
>> +Lenovo Thinkpad platforms with DYTC version 5 and newer have enhanced
>> firmware to
>> +provide improved performance control.
>> +
>> +The firmware can be controlled by hotkeys (FN+H, FN+M, FN+L) to switch the
>> +operating mode between three different modes. This sysfs node provide a
>> better
>> +interface for user space to use
> 
> So is userspace also notified in some way when you use the hotkey to change, or
> is the event usurped by the EC?  Is this by the event TP_HKEY_EV_THM_CSM_COMPLETED?
> 
I haven't added that yet - my aim with this patch was to get the sysfs 
API available. I'll look at adding the notification.

> You might consider to mention what other interfaces will conflict with this
> and document them and/or artificially block them when this is loaded to prevent
> such a conflict.
I'm afraid I don't know what other interface will be conflicted with. Is 
there anything in particular I should be looking for? What did you have 
in mind?

The firmware is operating by default and this patch is just providing 
user space with a way of determining the current mode and changing it by 
an alternate mechanism than hotkeys (I know some people dislike the 
hotkeys...)

> 
<snip>
>> +
>> +The sysfs entry provides the ability to return the current status and to set
>> the
>> +desired mode. For example::
>> +
>> +        echo H > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
>> +        echo M > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
>> +        echo L > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
> 
> Doesn't this need ABI documentation submitted as part of the patch?
OK - I'll need some help here as I'm not sure what I missed. Isn't that 
what this part of the patch is covering? If you can give me some 
pointers for what I should be putting where I'll do that.
> 
<snip>

>> +
>> +	if (perfmode == DYTC_MODE_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 {
>> +		/* 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*/
>> +			err = dytc_command(0x000F1001 /*Disable CQL*/, &output);
> 
> Why not put 0x000F1001 and 0x001F1001 as defines at the top?
Fair point - I will fix that.

> 
<snip>

>> +
>> +	switch (perfmode) {
>> +	case DYTC_MODE_PERFORM:
>> +		/* High performance is only available in deskmode */
>> +		if (funcmode == DYTC_FUNCTION_CQL)
>> +			return snprintf(buf, PAGE_SIZE, "Medium (Reduced as lapmode
>> active)\n");
>> +		else
>> +			return snprintf(buf, PAGE_SIZE, "High\n");
>> +	case DYTC_MODE_QUIET:
>> +		return snprintf(buf, PAGE_SIZE, "Low\n");
>> +	case DYTC_MODE_BALANCE:
>> +		return snprintf(buf, PAGE_SIZE, "Medium\n");
>> +	default:
>> +		return snprintf(buf, PAGE_SIZE, "Unknown (%d)\n", perfmode);
>> +	}
>> +}
> 
> I think it's pretty confusing that you write "H/M/L" into this file, but you
> get back a full string.
The main reason here for the string is the need to let the user know 
they are operating in medium mode even though high has been configured - 
because the device is on their lap.
My thinking was you can parse the first letter to get H/M/L but more 
information is available for the subtleties.
I considered another letter but couldn't determine something that was 
obvious to a user (Lower case 'h' is my best candidate?) and decided a 
string was nicer.

I'd appreciate input from others as to the best thing to provide here.

> 
>> +
>> +static ssize_t dytc_perfmode_store(struct device *dev,
>> +				   struct device_attribute *attr,
>> +				   const char *buf, size_t count)
>> +{
>> +	int err;
>> +
>> +	switch (buf[0]) {
>> +	case 'l':
>> +	case 'L':
>> +		err = dytc_perfmode_set(DYTC_MODE_QUIET);
>> +		break;
>> +	case 'm':
>> +	case 'M':
>> +		err = dytc_perfmode_set(DYTC_MODE_BALANCE);
>> +		break;
>> +	case 'h':
>> +	case 'H':
>> +		err = dytc_perfmode_set(DYTC_MODE_PERFORM);
>> +		break;
>> +	default:
>> +		err = -EINVAL;
>> +		pr_err("Unknown operating mode. Ignoring\n");
> 
> Shouldn't this be dev_err?
Ack - I will correct

<snip>
>>
>> +	/* Check DYTC is enabled and supports mode setting */
>> +	dytc_mode_available = 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) {
>> +			pr_info("DYTC thermal mode configuration available\n");
> 
> I would argue this isn't useful to most people.
> 1) You should decrease this to debug for use with dynamic debugging
> 2) Output in the log what integer value you returned back in case of a need
> to identify future firmware bugs.
Agreed on both fronts. I will fix.

> 
>> +			dytc_mode_available = true;
> 
> I think you shouldn't set this flag until after the group is actually created.
> 
Agreed. I will fix

Thanks for the feedback - very much appreciated.

Mark

Re: [External] RE: [PATCH] platform/x86: thinkpad_acpi: performance mode interface

[Limonciello, Mario]

> 
> On 7/22/2020 2:46 PM, Limonciello, Mario wrote:
> <snip>
> >>
> >> +DYTC Thermal mode status and control
> >> +------------------------------------
> >> +
> >> +sysfs: dytc_perfmode
> >> +
> >> +Lenovo Thinkpad platforms with DYTC version 5 and newer have enhanced
> >> firmware to
> >> +provide improved performance control.
> >> +
> >> +The firmware can be controlled by hotkeys (FN+H, FN+M, FN+L) to switch the
> >> +operating mode between three different modes. This sysfs node provide a
> >> better
> >> +interface for user space to use
> >
> > So is userspace also notified in some way when you use the hotkey to change,
> or
> > is the event usurped by the EC?  Is this by the event
> TP_HKEY_EV_THM_CSM_COMPLETED?
> >
> I haven't added that yet - my aim with this patch was to get the sysfs
> API available. I'll look at adding the notification.

Yeah I just think touch the kernel/user ABI as atomically as possible
to avoid userspace to have to know 5.9 behaves this way and you need to poll for a value
and 5.10 you get a notification etc.

> 
> > You might consider to mention what other interfaces will conflict with this
> > and document them and/or artificially block them when this is loaded to
> prevent
> > such a conflict.
> I'm afraid I don't know what other interface will be conflicted with. Is
> there anything in particular I should be looking for? What did you have
> in mind?

Since it's not mentioned I can only guess your firmware implementation associated
with this code.  I would think for example that touching some PLx related MSR or
possibly RAPL interface might cause unexpected behaviors.

Assuming that's right kernel lockdown might prevent some of the MSR, but if you really
want user fully in control of this decision by one knob, you shouldn't let common
userspace tools like thermald, tuned, tlp or the like touch the related objects.

> 
> The firmware is operating by default and this patch is just providing
> user space with a way of determining the current mode and changing it by
> an alternate mechanism than hotkeys (I know some people dislike the
> hotkeys...)

In which case if the firmware preference is that it's user control, I think all
the more reason to block out those other things while offering this interface.

> 
> >
> <snip>
> >> +
> >> +The sysfs entry provides the ability to return the current status and to
> set
> >> the
> >> +desired mode. For example::
> >> +
> >> +        echo H > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
> >> +        echo M > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
> >> +        echo L > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
> >
> > Doesn't this need ABI documentation submitted as part of the patch?
> OK - I'll need some help here as I'm not sure what I missed. Isn't that
> what this part of the patch is covering? If you can give me some
> pointers for what I should be putting where I'll do that.

I think it's common to document how your sysfs attributes work in a file in
Documentation/ABI/testing.  You can look at the format for some others
for examples.

> >
> <snip>
> 
> >> +
> >> +	if (perfmode == DYTC_MODE_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 {
> >> +		/* 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*/
> >> +			err = dytc_command(0x000F1001 /*Disable CQL*/, &output);
> >
> > Why not put 0x000F1001 and 0x001F1001 as defines at the top?
> Fair point - I will fix that.
> 
> >
> <snip>
> 
> >> +
> >> +	switch (perfmode) {
> >> +	case DYTC_MODE_PERFORM:
> >> +		/* High performance is only available in deskmode */
> >> +		if (funcmode == DYTC_FUNCTION_CQL)
> >> +			return snprintf(buf, PAGE_SIZE, "Medium (Reduced as lapmode
> >> active)\n");
> >> +		else
> >> +			return snprintf(buf, PAGE_SIZE, "High\n");
> >> +	case DYTC_MODE_QUIET:
> >> +		return snprintf(buf, PAGE_SIZE, "Low\n");
> >> +	case DYTC_MODE_BALANCE:
> >> +		return snprintf(buf, PAGE_SIZE, "Medium\n");
> >> +	default:
> >> +		return snprintf(buf, PAGE_SIZE, "Unknown (%d)\n", perfmode);
> >> +	}
> >> +}
> >
> > I think it's pretty confusing that you write "H/M/L" into this file, but you
> > get back a full string.
> The main reason here for the string is the need to let the user know
> they are operating in medium mode even though high has been configured -
> because the device is on their lap.
> My thinking was you can parse the first letter to get H/M/L but more
> information is available for the subtleties.
> I considered another letter but couldn't determine something that was
> obvious to a user (Lower case 'h' is my best candidate?) and decided a
> string was nicer.
> 
> I'd appreciate input from others as to the best thing to provide here.

My own personal opinion (and there may be others that offer different view
so don't take it authoritative):

If you're offering High/Medium/Low, you should accept an input of High/Medium/Low.
If you offer H/M/L you should accept H/M/L.

A good way to indicate the reduced mode would be to add an asterisk for medium.
So it could be:
Write: H/M/L
Read: H/M*/M/L

The actual decoding of the information can be placed in that Documentation file
I mentioned above.  In general a userspace tool will be making this pretty and
translated I would guess, so no need to do High versus high or Foo (bar) when
it could be Foo*

> 
> >
> >> +
> >> +static ssize_t dytc_perfmode_store(struct device *dev,
> >> +				   struct device_attribute *attr,
> >> +				   const char *buf, size_t count)
> >> +{
> >> +	int err;
> >> +
> >> +	switch (buf[0]) {
> >> +	case 'l':
> >> +	case 'L':
> >> +		err = dytc_perfmode_set(DYTC_MODE_QUIET);
> >> +		break;
> >> +	case 'm':
> >> +	case 'M':
> >> +		err = dytc_perfmode_set(DYTC_MODE_BALANCE);
> >> +		break;
> >> +	case 'h':
> >> +	case 'H':
> >> +		err = dytc_perfmode_set(DYTC_MODE_PERFORM);
> >> +		break;
> >> +	default:
> >> +		err = -EINVAL;
> >> +		pr_err("Unknown operating mode. Ignoring\n");
> >
> > Shouldn't this be dev_err?
> Ack - I will correct
> 
> <snip>
> >>
> >> +	/* Check DYTC is enabled and supports mode setting */
> >> +	dytc_mode_available = 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) {
> >> +			pr_info("DYTC thermal mode configuration available\n");
> >
> > I would argue this isn't useful to most people.
> > 1) You should decrease this to debug for use with dynamic debugging
> > 2) Output in the log what integer value you returned back in case of a need
> > to identify future firmware bugs.
> Agreed on both fronts. I will fix.

Similar to the pr_err vs dev_err, make sure you use the dev_dbg here instead of
pr_dbg.

> 
> >
> >> +			dytc_mode_available = true;
> >
> > I think you shouldn't set this flag until after the group is actually
> created.
> >
> Agreed. I will fix
> 
> Thanks for the feedback - very much appreciated.

Sure thing.

Re: [External] RE: [PATCH] platform/x86: thinkpad_acpi: performance mode interface

[Mark Pearson]

On 7/22/2020 3:46 PM, Limonciello, Mario wrote:
>>
>> On 7/22/2020 2:46 PM, Limonciello, Mario wrote:
>> <snip>
>>>>
>>>> +DYTC Thermal mode status and control
>>>> +------------------------------------
>>>> +
>>>> +sysfs: dytc_perfmode
>>>> +
>>>> +Lenovo Thinkpad platforms with DYTC version 5 and newer have enhanced
>>>> firmware to
>>>> +provide improved performance control.
>>>> +
>>>> +The firmware can be controlled by hotkeys (FN+H, FN+M, FN+L) to switch the
>>>> +operating mode between three different modes. This sysfs node provide a
>>>> better
>>>> +interface for user space to use
>>>
>>> So is userspace also notified in some way when you use the hotkey to change,
>> or
>>> is the event usurped by the EC?  Is this by the event
>> TP_HKEY_EV_THM_CSM_COMPLETED?
>>>
>> I haven't added that yet - my aim with this patch was to get the sysfs
>> API available. I'll look at adding the notification.
> 
> Yeah I just think touch the kernel/user ABI as atomically as possible
> to avoid userspace to have to know 5.9 behaves this way and you need to poll for a value
> and 5.10 you get a notification etc.
> 
OK - fair point. I'll look into implementing that as well.

>>
>>> You might consider to mention what other interfaces will conflict with this
>>> and document them and/or artificially block them when this is loaded to
>> prevent
>>> such a conflict.
>> I'm afraid I don't know what other interface will be conflicted with. Is
>> there anything in particular I should be looking for? What did you have
>> in mind?
> 
> Since it's not mentioned I can only guess your firmware implementation associated
> with this code.  I would think for example that touching some PLx related MSR or
> possibly RAPL interface might cause unexpected behaviors.
> 
> Assuming that's right kernel lockdown might prevent some of the MSR, but if you really
> want user fully in control of this decision by one knob, you shouldn't let common
> userspace tools like thermald, tuned, tlp or the like touch the related objects.
> 
Hmmm - I think I disagree here.

I don't think this should control what other userspace tools (like 
thermald) want to do with the CPU registers. Adding hooks into those 
other pieces of code also seems to me to be complicated and unnecessary 
in the kernel (and way beyond the scope of this patch). As an aside - my 
experience from testing is that thermald will override what the firmware 
is doing anyway.

I can see the value of adding a feature to *disable* the Lenovo firmware 
implementation as that doesn't currently exist. I will talk to the 
firmware team and see what can be done and take that on as a separate 
task. If there's a mechanism to do that already in a safe way then I'll 
add that to this.

>>
>> The firmware is operating by default and this patch is just providing
>> user space with a way of determining the current mode and changing it by
>> an alternate mechanism than hotkeys (I know some people dislike the
>> hotkeys...)
> 
> In which case if the firmware preference is that it's user control, I think all
> the more reason to block out those other things while offering this interface.
Covered above
> 
>>
>>>
>> <snip>
>>>> +
>>>> +The sysfs entry provides the ability to return the current status and to
>> set
>>>> the
>>>> +desired mode. For example::
>>>> +
>>>> +        echo H > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
>>>> +        echo M > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
>>>> +        echo L > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
>>>
>>> Doesn't this need ABI documentation submitted as part of the patch?
>> OK - I'll need some help here as I'm not sure what I missed. Isn't that
>> what this part of the patch is covering? If you can give me some
>> pointers for what I should be putting where I'll do that.
> 
> I think it's common to document how your sysfs attributes work in a file in
> Documentation/ABI/testing.  You can look at the format for some others
> for examples.
Ah - that was new to me. Thanks. I'm guessing I need to add a new 
sysfs-devices-platform-thinkpad_acpi file there. Strange there's not one 
already :)

> 
>>>
>> <snip>
>>
>>>> +
>>>> +	if (perfmode == DYTC_MODE_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 {
>>>> +		/* 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*/
>>>> +			err = dytc_command(0x000F1001 /*Disable CQL*/, &output);
>>>
>>> Why not put 0x000F1001 and 0x001F1001 as defines at the top?
>> Fair point - I will fix that.
>>
>>>
>> <snip>
>>
>>>> +
>>>> +	switch (perfmode) {
>>>> +	case DYTC_MODE_PERFORM:
>>>> +		/* High performance is only available in deskmode */
>>>> +		if (funcmode == DYTC_FUNCTION_CQL)
>>>> +			return snprintf(buf, PAGE_SIZE, "Medium (Reduced as lapmode
>>>> active)\n");
>>>> +		else
>>>> +			return snprintf(buf, PAGE_SIZE, "High\n");
>>>> +	case DYTC_MODE_QUIET:
>>>> +		return snprintf(buf, PAGE_SIZE, "Low\n");
>>>> +	case DYTC_MODE_BALANCE:
>>>> +		return snprintf(buf, PAGE_SIZE, "Medium\n");
>>>> +	default:
>>>> +		return snprintf(buf, PAGE_SIZE, "Unknown (%d)\n", perfmode);
>>>> +	}
>>>> +}
>>>
>>> I think it's pretty confusing that you write "H/M/L" into this file, but you
>>> get back a full string.
>> The main reason here for the string is the need to let the user know
>> they are operating in medium mode even though high has been configured -
>> because the device is on their lap.
>> My thinking was you can parse the first letter to get H/M/L but more
>> information is available for the subtleties.
>> I considered another letter but couldn't determine something that was
>> obvious to a user (Lower case 'h' is my best candidate?) and decided a
>> string was nicer.
>>
>> I'd appreciate input from others as to the best thing to provide here.
> 
> My own personal opinion (and there may be others that offer different view
> so don't take it authoritative):
> 
> If you're offering High/Medium/Low, you should accept an input of High/Medium/Low.
> If you offer H/M/L you should accept H/M/L.
> 
> A good way to indicate the reduced mode would be to add an asterisk for medium.
> So it could be:
> Write: H/M/L
> Read: H/M*/M/L

I like this. Unless someone jumps in and says otherwise I'm good to 
switch to this.
> 
> The actual decoding of the information can be placed in that Documentation file
> I mentioned above.  In general a userspace tool will be making this pretty and
> translated I would guess, so no need to do High versus high or Foo (bar) when
> it could be Foo*
Ack

>>
>>>
>>>> +
>>>> +static ssize_t dytc_perfmode_store(struct device *dev,
>>>> +				   struct device_attribute *attr,
>>>> +				   const char *buf, size_t count)
>>>> +{
>>>> +	int err;
>>>> +
>>>> +	switch (buf[0]) {
>>>> +	case 'l':
>>>> +	case 'L':
>>>> +		err = dytc_perfmode_set(DYTC_MODE_QUIET);
>>>> +		break;
>>>> +	case 'm':
>>>> +	case 'M':
>>>> +		err = dytc_perfmode_set(DYTC_MODE_BALANCE);
>>>> +		break;
>>>> +	case 'h':
>>>> +	case 'H':
>>>> +		err = dytc_perfmode_set(DYTC_MODE_PERFORM);
>>>> +		break;
>>>> +	default:
>>>> +		err = -EINVAL;
>>>> +		pr_err("Unknown operating mode. Ignoring\n");
>>>
>>> Shouldn't this be dev_err?
>> Ack - I will correct
>>
>> <snip>
>>>>
>>>> +	/* Check DYTC is enabled and supports mode setting */
>>>> +	dytc_mode_available = 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) {
>>>> +			pr_info("DYTC thermal mode configuration available\n");
>>>
>>> I would argue this isn't useful to most people.
>>> 1) You should decrease this to debug for use with dynamic debugging
>>> 2) Output in the log what integer value you returned back in case of a need
>>> to identify future firmware bugs.
>> Agreed on both fronts. I will fix.
> 
> Similar to the pr_err vs dev_err, make sure you use the dev_dbg here instead of
> pr_dbg.
> 
>>
>>>
>>>> +			dytc_mode_available = true;
>>>
>>> I think you shouldn't set this flag until after the group is actually
>> created.
>>>
>> Agreed. I will fix
>>
>> Thanks for the feedback - very much appreciated.
> 
> Sure thing.
> 

Re: [External] RE: [PATCH] platform/x86: thinkpad_acpi: performance mode interface

[Limonciello, Mario]

> >
> > Since it's not mentioned I can only guess your firmware implementation
> associated
> > with this code.  I would think for example that touching some PLx related
> MSR or
> > possibly RAPL interface might cause unexpected behaviors.
> >
> > Assuming that's right kernel lockdown might prevent some of the MSR, but
> if you really
> > want user fully in control of this decision by one knob, you shouldn't
> let common
> > userspace tools like thermald, tuned, tlp or the like touch the related
> objects.
> >
> Hmmm - I think I disagree here.
> 
> I don't think this should control what other userspace tools (like
> thermald) want to do with the CPU registers. Adding hooks into those
> other pieces of code also seems to me to be complicated and unnecessary
> in the kernel (and way beyond the scope of this patch). As an aside - my
> experience from testing is that thermald will override what the firmware
> is doing anyway.

I'm actually in agreement it is potentially quite complicated and shouldn't be in
this specific patch.  I was going to suggest it should either come as other
patches, or perhaps in documentation along the lines of "Users using this interface
should not use other tools to modify X, Y and Z".  No need to mention the actual
tools, but you should try to help prevent users shooting themselves in the foot
unintentionally.

> 
> I can see the value of adding a feature to *disable* the Lenovo firmware
> implementation as that doesn't currently exist. I will talk to the
> firmware team and see what can be done and take that on as a separate
> task. If there's a mechanism to do that already in a safe way then I'll
> add that to this.

This is actually even better to me.  Back to the H/M/L approach if you can have
an extra one for "off" then userspace tools that want to control the same levers
can turn this off when they are taking control.

[PATCH v2] platform/x86: thinkpad_acpi: psensor interface

[Mark Pearson]

Some Lenovo Thinkpad platforms are equipped with a 'palm sensor' so as
to be able to determine if a user is physically proximate to the device.

This patch provides the ability to retrieve the psensor state via sysfs
entrypoints and will be used by userspace for WWAN functionality to
control the transmission level safely

Co-developed-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
---
Changes in v2:
 - addressed formatting and coding style errors identified during review

 drivers/platform/x86/thinkpad_acpi.c | 107 ++++++++++++++++++++++++++-
 1 file changed, 104 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 4864a5c189d4..41b75dd4755c 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -4079,10 +4079,9 @@ static bool hotkey_notify_6xxx(const u32 hkey,
 
 	case TP_HKEY_EV_PALM_DETECTED:
 	case TP_HKEY_EV_PALM_UNDETECTED:
-		/* palm detected hovering the keyboard, forward to user-space
-		 * via netlink for consumption */
+		/* palm detected - pass on to event handler */
+		tpacpi_driver_event(hkey);
 		return true;
-
 	default:
 		/* report simply as unknown, no sensor dump */
 		return false;
@@ -9916,6 +9915,99 @@ static struct ibm_struct dytc_driver_data = {
 	.exit = dytc_exit,
 };
 
+/**********************************************************************
+ * Palm sensor subdriver
+ */
+
+#define PSENSOR_PRESENT_BIT 0 /* Determine if psensor present */
+#define PSENSOR_ON_BIT      1 /* psensor status */
+
+static bool psensor_state;
+
+static void psensor_notify_change(void)
+{
+	sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "psensor_state");
+}
+
+static int psensor_get(bool *state)
+{
+	acpi_handle psensor_handle;
+	int output;
+
+	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "GPSS", &psensor_handle)))
+		return -ENODEV;
+
+	if (!acpi_evalf(psensor_handle, &output, NULL, "d"))
+		return -EIO;
+
+	/* Check if sensor has a Psensor */
+	if (!(output & BIT(PSENSOR_PRESENT_BIT)))
+		return -ENODEV;
+
+	/* Return if psensor is set or not */
+	*state = output & BIT(PSENSOR_ON_BIT) ? true : false;
+	return 0;
+}
+
+static void psensor_state_refresh(void)
+{
+	bool new_state;
+	int err;
+
+	err = psensor_get(&new_state);
+	if (err || (new_state == psensor_state))
+		return;
+
+	psensor_state = new_state;
+	psensor_notify_change();
+}
+
+/* sysfs psensor entry */
+static ssize_t psensor_state_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	return sprintf(buf, "%d\n", psensor_state);
+}
+static DEVICE_ATTR_RO(psensor_state);
+
+static struct attribute *psensor_attributes[] = {
+	&dev_attr_psensor_state.attr,
+	NULL
+};
+
+static const struct attribute_group psensor_attr_group = {
+	.attrs = psensor_attributes,
+};
+
+static int tpacpi_psensor_init(struct ibm_init_struct *iibm)
+{
+	int err;
+
+	err = psensor_get(&psensor_state);
+	/*
+	 * If support isn't available (ENODEV) then don't return an error,
+	 * just don't create the sysfs group.
+	 */
+	if (err == -ENODEV)
+		return 0;
+	/* For all other errors we can flag the failure */
+	if (err)
+		return err;
+
+	return sysfs_create_group(&tpacpi_pdev->dev.kobj, &psensor_attr_group);
+}
+
+static void psensor_exit(void)
+{
+	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &psensor_attr_group);
+}
+
+static struct ibm_struct psensor_driver_data = {
+	.name = "psensor",
+	.exit = psensor_exit,
+};
+
 /****************************************************************************
  ****************************************************************************
  *
@@ -9967,6 +10059,10 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
 	if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED)
 		dytc_lapmode_refresh();
 
+	if ((hkey_event == TP_HKEY_EV_PALM_DETECTED) ||
+		(hkey_event == TP_HKEY_EV_PALM_UNDETECTED))
+		psensor_state_refresh();
+
 }
 
 static void hotkey_driver_event(const unsigned int scancode)
@@ -10409,6 +10505,11 @@ static struct ibm_init_struct ibms_init[] __initdata = {
 		.init = tpacpi_dytc_init,
 		.data = &dytc_driver_data,
 	},
+	{
+		.init = tpacpi_psensor_init,
+		.data = &psensor_driver_data,
+	},
+
 };
 
 static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
-- 
2.26.2

[PATCH v3] platform/x86: thinkpad_acpi: psensor interface

[Mark Pearson]

Some Lenovo Thinkpad platforms are equipped with a 'palm sensor' so as
to be able to determine if a user is physically proximate to the device.

This patch provides the ability to retrieve the psensor state via sysfs
entrypoints and will be used by userspace for WWAN functionality to
control the transmission level safely

Co-developed-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
---
Changes in v2:
 - addressed formatting and coding style errors identified during review
Changes in v3:
 - I had missed updating thinkpad-acpi.rst in the v2 update so adding
   that back so it is correct. 

 .../admin-guide/laptops/thinkpad-acpi.rst     |  18 +++
 drivers/platform/x86/thinkpad_acpi.c          | 107 +++++++++++++++++-
 2 files changed, 122 insertions(+), 3 deletions(-)

diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
index 5e477869df18..6b57c52d8f13 100644
--- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
+++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
@@ -51,6 +51,7 @@ detailed description):
 	- UWB enable and disable
 	- LCD Shadow (PrivacyGuard) enable and disable
 	- Lap mode sensor
+        - Palm sensor (aka psensor)
 
 A compatibility table by model and feature is maintained on the web
 site, http://ibm-acpi.sf.net/. I appreciate any success or failure
@@ -1447,6 +1448,23 @@ they differ between desk and lap mode.
 The property is read-only. If the platform doesn't have support the sysfs
 class is not created.
 
+Palm sensor
+------------------
+
+sysfs: psensor_state
+
+Certain thinkpads and mobile workstations are equipped with a palm sensor to
+detect when a user is physically near the device. This device, when present,
+is used in conjunction with the lapmode sensor to decide if WWAN transmission
+can be increased to maximum power.
+
+The property is read-only. If the platform doesn't have support the sysfs
+class is not created.
+
+Note - some platforms have a limitation whereby the EC firmware cannot
+determine if the sensor is installed or not. On these platforms the psensor
+state will always be reported as true to avoid high power being used incorrectly.
+
 EXPERIMENTAL: UWB
 -----------------
 
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 4864a5c189d4..41b75dd4755c 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -4079,10 +4079,9 @@ static bool hotkey_notify_6xxx(const u32 hkey,
 
 	case TP_HKEY_EV_PALM_DETECTED:
 	case TP_HKEY_EV_PALM_UNDETECTED:
-		/* palm detected hovering the keyboard, forward to user-space
-		 * via netlink for consumption */
+		/* palm detected - pass on to event handler */
+		tpacpi_driver_event(hkey);
 		return true;
-
 	default:
 		/* report simply as unknown, no sensor dump */
 		return false;
@@ -9916,6 +9915,99 @@ static struct ibm_struct dytc_driver_data = {
 	.exit = dytc_exit,
 };
 
+/**********************************************************************
+ * Palm sensor subdriver
+ */
+
+#define PSENSOR_PRESENT_BIT 0 /* Determine if psensor present */
+#define PSENSOR_ON_BIT      1 /* psensor status */
+
+static bool psensor_state;
+
+static void psensor_notify_change(void)
+{
+	sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "psensor_state");
+}
+
+static int psensor_get(bool *state)
+{
+	acpi_handle psensor_handle;
+	int output;
+
+	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "GPSS", &psensor_handle)))
+		return -ENODEV;
+
+	if (!acpi_evalf(psensor_handle, &output, NULL, "d"))
+		return -EIO;
+
+	/* Check if sensor has a Psensor */
+	if (!(output & BIT(PSENSOR_PRESENT_BIT)))
+		return -ENODEV;
+
+	/* Return if psensor is set or not */
+	*state = output & BIT(PSENSOR_ON_BIT) ? true : false;
+	return 0;
+}
+
+static void psensor_state_refresh(void)
+{
+	bool new_state;
+	int err;
+
+	err = psensor_get(&new_state);
+	if (err || (new_state == psensor_state))
+		return;
+
+	psensor_state = new_state;
+	psensor_notify_change();
+}
+
+/* sysfs psensor entry */
+static ssize_t psensor_state_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	return sprintf(buf, "%d\n", psensor_state);
+}
+static DEVICE_ATTR_RO(psensor_state);
+
+static struct attribute *psensor_attributes[] = {
+	&dev_attr_psensor_state.attr,
+	NULL
+};
+
+static const struct attribute_group psensor_attr_group = {
+	.attrs = psensor_attributes,
+};
+
+static int tpacpi_psensor_init(struct ibm_init_struct *iibm)
+{
+	int err;
+
+	err = psensor_get(&psensor_state);
+	/*
+	 * If support isn't available (ENODEV) then don't return an error,
+	 * just don't create the sysfs group.
+	 */
+	if (err == -ENODEV)
+		return 0;
+	/* For all other errors we can flag the failure */
+	if (err)
+		return err;
+
+	return sysfs_create_group(&tpacpi_pdev->dev.kobj, &psensor_attr_group);
+}
+
+static void psensor_exit(void)
+{
+	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &psensor_attr_group);
+}
+
+static struct ibm_struct psensor_driver_data = {
+	.name = "psensor",
+	.exit = psensor_exit,
+};
+
 /****************************************************************************
  ****************************************************************************
  *
@@ -9967,6 +10059,10 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
 	if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED)
 		dytc_lapmode_refresh();
 
+	if ((hkey_event == TP_HKEY_EV_PALM_DETECTED) ||
+		(hkey_event == TP_HKEY_EV_PALM_UNDETECTED))
+		psensor_state_refresh();
+
 }
 
 static void hotkey_driver_event(const unsigned int scancode)
@@ -10409,6 +10505,11 @@ static struct ibm_init_struct ibms_init[] __initdata = {
 		.init = tpacpi_dytc_init,
 		.data = &dytc_driver_data,
 	},
+	{
+		.init = tpacpi_psensor_init,
+		.data = &psensor_driver_data,
+	},
+
 };
 
 static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
-- 
2.26.2

[PATCH v2] platform/x86: thinkpad_acpi: performance mode interface

[Mark Pearson]

Lenovo Thinkpad platforms with DYTC version 5 and newer have enhanced
firmware to provide different performance/thermal modes.

The modes can be controlled by hotkeys (FN+H, FN+M, FN+L) to switch
the operating mode between three different modes.

H - High performance. Maximum power is available and the temperature is
allowed to increase to the maximum for the platform.
M - Medium performance (aka balance). In this mode power will be limited
and the laptop will have a lower maximum temperature.
L - Low performance (aka quiet). In this mode power consumption is reduced
and the device will be cooler.

High performance mode is only available when the device is in 'desk mode'.
If the device detects that it is on a lap then it will automatically drop
into medium mode to maintain a safer operating temperature.

This patch provides an interface to determine the current mode and to also
allow the setting of the mode through the dytc_perfmode sysfs entry. This
can be used by userspace for improved control.

Reviewed-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
---
Changes in v2:
 - Add userspace notification to hkey event handler. Note this got
   somewhat more complicated than expected as retrieving the performance
   mode when in lapmode generates an event itself; added a flag to
   ignore these known events.
 - Updated the values returned to be simpler (H/M*/M/L) as suggested.
 - Added ABI documentation as requested. Lap and palm sensor details
   added whilst I was doing this.
 - Cleaned up code based on recommendations as well as feedback received
   from other patch reviews.
 - Based on discussion with firmware team the lapmode sensor should only
   be made available for DYTC v5 and later. Added this to init logic.

 .../sysfs-devices-platform-thinkpad_acpi      |  34 +++
 .../admin-guide/laptops/thinkpad-acpi.rst     |  35 +++
 drivers/platform/x86/thinkpad_acpi.c          | 267 ++++++++++++++++--
 3 files changed, 310 insertions(+), 26 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-devices-platform-thinkpad_acpi

diff --git a/Documentation/ABI/testing/sysfs-devices-platform-thinkpad_acpi b/Documentation/ABI/testing/sysfs-devices-platform-thinkpad_acpi
new file mode 100644
index 000000000000..28f07753a889
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-devices-platform-thinkpad_acpi
@@ -0,0 +1,34 @@
+What:		/sys/devices/platform/thinkpad_acpi/dytc_perfmode
+Date:		August 2020
+Contact:	Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
+Description:
+		Reads return the current performance mode setting configured in firmware using
+		the below nomenclature.
+
+		Writes configure the performance mode setting by using the below nomenclature.
+
+		H  - High performance mode. Maximum power and temperature available.
+		M* - High performance mode but performance is limited to medium as system is
+		     in lapmode. Power and temperature maximums reduced to a safe threshold.
+		M  - Medium performance mode (aka 'balance'). Lower maximum power and temperatures
+		     but better battery life.
+		L  - Low performance mode (aka 'quiet'). Reduced power setting gives lower
+		     temperatures and extended battery life. Fans run quieter.
+
+What:		/sys/devices/platform/thinkpad_acpi/dytc_lapmode
+Date:		August 2020
+Contact:	Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
+Description:
+		Reads returns the current value of the lapmode sensor.
+
+		0 - desk mode is detected
+		1 - lap mode is detected
+
+What:		/sys/devices/platform/thinkpad_acpi/psensor_state
+Date:		August 2020
+Contact:	Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
+Description:
+		Reads returns the current value of the palm sensor.
+
+		0 - palm not detected
+		1 - palm detected
diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
index 6b57c52d8f13..b98f0de9e063 100644
--- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
+++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
@@ -52,6 +52,7 @@ detailed description):
 	- LCD Shadow (PrivacyGuard) enable and disable
 	- Lap mode sensor
         - Palm sensor (aka psensor)
+	- Thermal mode status and control
 
 A compatibility table by model and feature is maintained on the web
 site, http://ibm-acpi.sf.net/. I appreciate any success or failure
@@ -1465,6 +1466,40 @@ Note - some platforms have a limitation whereby the EC firmware cannot
 determine if the sensor is installed or not. On these platforms the psensor
 state will always be reported as true to avoid high power being used incorrectly.
 
+DYTC Thermal mode status and control
+------------------------------------
+
+sysfs: dytc_perfmode
+
+Lenovo Thinkpad platforms with DYTC version 5 and newer have enhanced firmware to
+provide improved performance control.
+
+The firmware can be controlled by hotkeys (FN+H, FN+M, FN+L) to switch the
+operating mode between three different modes. This sysfs node provides a better
+interface for user space to use.
+
+The modes available are:
+
+H - High performance. Maximum power is available and the temperature is
+allowed to increase to the maximum for the platform.
+
+M - Medium performance (aka balance). In this mode power will be limited and
+the laptop will remain cooler.
+
+L - Low performance (aka quiet). In this mode power consumption is reduced and
+the device will be cooler and quieter.
+
+Note: High performance mode is only available when the device is in 'deskmode'. If
+the device detects that it is on a lap then it will automatically drop into medium
+mode to maintain a safer operating temperature.
+
+The sysfs entry provides the ability to return the current status and to set the
+desired mode. For example::
+
+        echo H > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
+        echo M > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
+        echo L > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
+
 EXPERIMENTAL: UWB
 -----------------
 
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 41b75dd4755c..8fcb660aa5a2 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -9817,18 +9817,43 @@ static struct ibm_struct lcdshadow_driver_data = {
 };
 
 /*************************************************************************
- * DYTC subdriver, for the Lenovo lapmode feature
+ * DYTC subdriver, for the Lenovo lap and performance mode 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 */
 
-static bool dytc_lapmode;
+#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 */
 
-static void dytc_lapmode_notify_change(void)
-{
-	sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "dytc_lapmode");
-}
+#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 int  dytc_perfmode;
+static bool dytc_available;
+static bool dytc_ignore_next_event;
 
 static int dytc_command(int command, int *output)
 {
@@ -9843,6 +9868,87 @@ static int dytc_command(int command, int *output)
 	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;
+}
+
+static int dytc_perfmode_set(int perfmode)
+{
+	int err, dytc_set;
+	int output;
+	int cur_perfmode, cur_funcmode;
+
+	if (!dytc_available)
+		return -ENODEV;
+
+	if (perfmode == DYTC_MODE_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 {
+		/* 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;
+		}
+	}
+	return 0;
+}
+
 static int dytc_lapmode_get(bool *state)
 {
 	int output, err;
@@ -9854,45 +9960,130 @@ static int dytc_lapmode_get(bool *state)
 	return 0;
 }
 
-static void dytc_lapmode_refresh(void)
+static void dytc_refresh(void)
 {
-	bool new_state;
+	bool lapmode;
+	int perfmode, funcmode;
 	int err;
 
-	err = dytc_lapmode_get(&new_state);
-	if (err || (new_state == dytc_lapmode))
+	err = dytc_lapmode_get(&lapmode);
+	if (err)
+		return;
+	if (dytc_ignore_next_event) {
+		dytc_ignore_next_event = false; /*clear setting*/
 		return;
+	}
+	if (lapmode != dytc_lapmode) {
+		dytc_lapmode = lapmode;
+		sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "dytc_lapmode");
+	}
+	err = dytc_perfmode_get(&perfmode, &funcmode);
+	if (err)
+		return;
+	if (perfmode != dytc_perfmode) {
+		dytc_perfmode = perfmode;
+		sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "dytc_perfmode");
+	}
+}
+
+/* sysfs perfmode entry */
+static ssize_t dytc_perfmode_show(struct device *dev,
+				  struct device_attribute *attr,
+				  char *buf)
+{
+	int err;
+	int perfmode, funcmode;
+
+	err = dytc_perfmode_get(&perfmode, &funcmode);
+	if (err)
+		return err;
 
-	dytc_lapmode = new_state;
-	dytc_lapmode_notify_change();
+	switch (perfmode) {
+	case DYTC_MODE_PERFORM:
+		/* High performance is only available in deskmode */
+		if (funcmode == DYTC_FUNCTION_CQL)
+			return sprintf(buf, "M*\n");
+		else
+			return sprintf(buf, "H\n");
+	case DYTC_MODE_QUIET:
+		return sprintf(buf, "L\n");
+	case DYTC_MODE_BALANCE:
+		return sprintf(buf, "M\n");
+	default:
+		return sprintf(buf, "Unknown (%d)\n", perfmode);
+	}
 }
 
+static ssize_t dytc_perfmode_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	int err;
+
+	switch (buf[0]) {
+	case 'l':
+	case 'L':
+		err = dytc_perfmode_set(DYTC_MODE_QUIET);
+		break;
+	case 'm':
+	case 'M':
+		err = dytc_perfmode_set(DYTC_MODE_BALANCE);
+		break;
+	case 'h':
+	case 'H':
+		err = dytc_perfmode_set(DYTC_MODE_PERFORM);
+		break;
+	default:
+		err = -EINVAL;
+		pr_err("Unknown operating mode. Ignoring\n");
+		break;
+	}
+	if (err)
+		return err;
+
+	tpacpi_disclose_usertask(attr->attr.name,
+				"Performance mode set to %c\n", buf[0]);
+	return count;
+}
+
+static DEVICE_ATTR_RW(dytc_perfmode);
+
+static struct attribute *dytc_perfmode_attributes[] = {
+	&dev_attr_dytc_perfmode.attr,
+	NULL
+};
+
+static const struct attribute_group dytc_perf_attr_group = {
+	.attrs = dytc_perfmode_attributes
+};
+
 /* sysfs lapmode entry */
 static ssize_t dytc_lapmode_show(struct device *dev,
 					struct device_attribute *attr,
 					char *buf)
 {
-	return snprintf(buf, PAGE_SIZE, "%d\n", dytc_lapmode);
+	return sprintf(buf, "%d\n", dytc_lapmode);
 }
 
 static DEVICE_ATTR_RO(dytc_lapmode);
 
-static struct attribute *dytc_attributes[] = {
+static struct attribute *dytc_lap_attributes[] = {
 	&dev_attr_dytc_lapmode.attr,
-	NULL,
+	NULL
 };
 
-static const struct attribute_group dytc_attr_group = {
-	.attrs = dytc_attributes,
+static const struct attribute_group dytc_lap_attr_group = {
+	.attrs = dytc_lap_attributes
 };
 
 static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
 {
-	int err;
+	int err, output;
 
-	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
+	err = dytc_command(DYTC_CMD_QUERY, &output);
+	/*
+	 * If support isn't available (ENODEV) then don't return an error
+	 * just don't create the sysfs group
 	 */
 	if (err == -ENODEV)
 		return 0;
@@ -9900,14 +10091,38 @@ 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);
+	/* 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;
+			/* Platform supports this feature - create the group */
+			err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &dytc_perf_attr_group);
+			if (err)
+				return err;
+
+			err = dytc_lapmode_get(&dytc_lapmode);
+			if (err)
+				return err;
+			err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &dytc_lap_attr_group);
+		}
+	}
 	return err;
 }
 
 static void dytc_exit(void)
 {
-	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
+	if (dytc_available) {
+		sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_lap_attr_group);
+		sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_perf_attr_group);
+	}
 }
 
 static struct ibm_struct dytc_driver_data = {
@@ -10057,7 +10272,7 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
 	}
 
 	if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED)
-		dytc_lapmode_refresh();
+		dytc_refresh();
 
 	if ((hkey_event == TP_HKEY_EV_PALM_DETECTED) ||
 		(hkey_event == TP_HKEY_EV_PALM_UNDETECTED))
-- 
2.26.2

Re: [PATCH v2] platform/x86: thinkpad_acpi: performance mode interface

[Limonciello, Mario]

> 
> Lenovo Thinkpad platforms with DYTC version 5 and newer have enhanced
> firmware to provide different performance/thermal modes.
> 
> The modes can be controlled by hotkeys (FN+H, FN+M, FN+L) to switch
> the operating mode between three different modes.
> 
> H - High performance. Maximum power is available and the temperature is
> allowed to increase to the maximum for the platform.
> M - Medium performance (aka balance). In this mode power will be limited
> and the laptop will have a lower maximum temperature.
> L - Low performance (aka quiet). In this mode power consumption is reduced
> and the device will be cooler.
> 
> High performance mode is only available when the device is in 'desk mode'.
> If the device detects that it is on a lap then it will automatically drop
> into medium mode to maintain a safer operating temperature.
> 
> This patch provides an interface to determine the current mode and to also
> allow the setting of the mode through the dytc_perfmode sysfs entry. This
> can be used by userspace for improved control.
> 
> Reviewed-by: Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> ---
> Changes in v2:
>  - Add userspace notification to hkey event handler. Note this got
>    somewhat more complicated than expected as retrieving the performance
>    mode when in lapmode generates an event itself; added a flag to
>    ignore these known events.
>  - Updated the values returned to be simpler (H/M*/M/L) as suggested.
>  - Added ABI documentation as requested. Lap and palm sensor details
>    added whilst I was doing this.
>  - Cleaned up code based on recommendations as well as feedback received
>    from other patch reviews.
>  - Based on discussion with firmware team the lapmode sensor should only
>    be made available for DYTC v5 and later. Added this to init logic.

Thanks for adapting a lot of my feedback, this looks much better to me.

> 
>  .../sysfs-devices-platform-thinkpad_acpi      |  34 +++
>  .../admin-guide/laptops/thinkpad-acpi.rst     |  35 +++
>  drivers/platform/x86/thinkpad_acpi.c          | 267 ++++++++++++++++--
>  3 files changed, 310 insertions(+), 26 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-devices-platform-
> thinkpad_acpi
> 
> diff --git a/Documentation/ABI/testing/sysfs-devices-platform-thinkpad_acpi
> b/Documentation/ABI/testing/sysfs-devices-platform-thinkpad_acpi
> new file mode 100644
> index 000000000000..28f07753a889
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-devices-platform-thinkpad_acpi
> @@ -0,0 +1,34 @@
> +What:		/sys/devices/platform/thinkpad_acpi/dytc_perfmode
> +Date:		August 2020
> +Contact:	Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> +Description:
> +		Reads return the current performance mode setting configured in
> firmware using
> +		the below nomenclature.
> +
> +		Writes configure the performance mode setting by using the below
> nomenclature.
> +
> +		H  - High performance mode. Maximum power and temperature
> available.
> +		M* - High performance mode but performance is limited to medium as
> system is
> +		     in lapmode. Power and temperature maximums reduced to a safe
> threshold.
> +		M  - Medium performance mode (aka 'balance'). Lower maximum power
> and temperatures
> +		     but better battery life.
> +		L  - Low performance mode (aka 'quiet'). Reduced power setting
> gives lower
> +		     temperatures and extended battery life. Fans run quieter.
> +
> +What:		/sys/devices/platform/thinkpad_acpi/dytc_lapmode
> +Date:		August 2020
> +Contact:	Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> +Description:
> +		Reads returns the current value of the lapmode sensor.
> +
> +		0 - desk mode is detected
> +		1 - lap mode is detected
> +
> +What:		/sys/devices/platform/thinkpad_acpi/psensor_state
> +Date:		August 2020
> +Contact:	Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> +Description:
> +		Reads returns the current value of the palm sensor.
> +
> +		0 - palm not detected
> +		1 - palm detected
> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> index 6b57c52d8f13..b98f0de9e063 100644
> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> @@ -52,6 +52,7 @@ detailed description):
>  	- LCD Shadow (PrivacyGuard) enable and disable
>  	- Lap mode sensor
>          - Palm sensor (aka psensor)
> +	- Thermal mode status and control
> 
>  A compatibility table by model and feature is maintained on the web
>  site, http://ibm-acpi.sf.net/. I appreciate any success or failure
> @@ -1465,6 +1466,40 @@ Note - some platforms have a limitation whereby the EC
> firmware cannot
>  determine if the sensor is installed or not. On these platforms the psensor
>  state will always be reported as true to avoid high power being used
> incorrectly.
> 
> +DYTC Thermal mode status and control
> +------------------------------------
> +
> +sysfs: dytc_perfmode
> +
> +Lenovo Thinkpad platforms with DYTC version 5 and newer have enhanced
> firmware to
> +provide improved performance control.
> +
> +The firmware can be controlled by hotkeys (FN+H, FN+M, FN+L) to switch the
> +operating mode between three different modes. This sysfs node provides a
> better
> +interface for user space to use.
> +
> +The modes available are:
> +
> +H - High performance. Maximum power is available and the temperature is
> +allowed to increase to the maximum for the platform.
> +
> +M - Medium performance (aka balance). In this mode power will be limited and
> +the laptop will remain cooler.
> +
> +L - Low performance (aka quiet). In this mode power consumption is reduced
> and
> +the device will be cooler and quieter.
> +
> +Note: High performance mode is only available when the device is in
> 'deskmode'. If
> +the device detects that it is on a lap then it will automatically drop into
> medium
> +mode to maintain a safer operating temperature.
> +
> +The sysfs entry provides the ability to return the current status and to set
> the
> +desired mode. For example::
> +
> +        echo H > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
> +        echo M > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
> +        echo L > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
> +

I was thinking about this some more, do you actually want another mode that "disables"
this feature?   IE "O" turns it off an calls DYTC_DISABLE_CQL.

For example if a user wanted to test the recently landed code in thermald 2.3
and compare performance between the two it seems like this and that "might" fight.
As an outsider looking in - I of course may be wrong too here.

If at some point in the future thermald does a better job than this implementation you
might also want an "out" to let thermald or another piece of userland turn this off
if it's in the picture.

>  EXPERIMENTAL: UWB
>  -----------------
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index 41b75dd4755c..8fcb660aa5a2 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -9817,18 +9817,43 @@ static struct ibm_struct lcdshadow_driver_data = {
>  };
> 
>  /*************************************************************************
> - * DYTC subdriver, for the Lenovo lapmode feature
> + * DYTC subdriver, for the Lenovo lap and performance mode 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 */
> 
> -static bool dytc_lapmode;
> +#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 */
> 
> -static void dytc_lapmode_notify_change(void)
> -{
> -	sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "dytc_lapmode");
> -}
> +#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 int  dytc_perfmode;
> +static bool dytc_available;
> +static bool dytc_ignore_next_event;
> 
>  static int dytc_command(int command, int *output)
>  {
> @@ -9843,6 +9868,87 @@ static int dytc_command(int command, int *output)
>  	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;
> +}
> +
> +static int dytc_perfmode_set(int perfmode)
> +{
> +	int err, dytc_set;
> +	int output;
> +	int cur_perfmode, cur_funcmode;
> +
> +	if (!dytc_available)
> +		return -ENODEV;
> +
> +	if (perfmode == DYTC_MODE_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 {
> +		/* 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;
> +		}
> +	}
> +	return 0;
> +}
> +
>  static int dytc_lapmode_get(bool *state)
>  {
>  	int output, err;
> @@ -9854,45 +9960,130 @@ static int dytc_lapmode_get(bool *state)
>  	return 0;
>  }
> 
> -static void dytc_lapmode_refresh(void)
> +static void dytc_refresh(void)
>  {
> -	bool new_state;
> +	bool lapmode;
> +	int perfmode, funcmode;
>  	int err;
> 
> -	err = dytc_lapmode_get(&new_state);
> -	if (err || (new_state == dytc_lapmode))
> +	err = dytc_lapmode_get(&lapmode);
> +	if (err)
> +		return;
> +	if (dytc_ignore_next_event) {
> +		dytc_ignore_next_event = false; /*clear setting*/
>  		return;
> +	}
> +	if (lapmode != dytc_lapmode) {
> +		dytc_lapmode = lapmode;
> +		sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "dytc_lapmode");
> +	}
> +	err = dytc_perfmode_get(&perfmode, &funcmode);
> +	if (err)
> +		return;
> +	if (perfmode != dytc_perfmode) {
> +		dytc_perfmode = perfmode;
> +		sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "dytc_perfmode");
> +	}
> +}
> +
> +/* sysfs perfmode entry */
> +static ssize_t dytc_perfmode_show(struct device *dev,
> +				  struct device_attribute *attr,
> +				  char *buf)
> +{
> +	int err;
> +	int perfmode, funcmode;
> +
> +	err = dytc_perfmode_get(&perfmode, &funcmode);
> +	if (err)
> +		return err;
> 
> -	dytc_lapmode = new_state;
> -	dytc_lapmode_notify_change();
> +	switch (perfmode) {
> +	case DYTC_MODE_PERFORM:
> +		/* High performance is only available in deskmode */
> +		if (funcmode == DYTC_FUNCTION_CQL)
> +			return sprintf(buf, "M*\n");
> +		else
> +			return sprintf(buf, "H\n");
> +	case DYTC_MODE_QUIET:
> +		return sprintf(buf, "L\n");
> +	case DYTC_MODE_BALANCE:
> +		return sprintf(buf, "M\n");
> +	default:
> +		return sprintf(buf, "Unknown (%d)\n", perfmode);
> +	}
>  }
> 
> +static ssize_t dytc_perfmode_store(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	int err;
> +
> +	switch (buf[0]) {
> +	case 'l':
> +	case 'L':
> +		err = dytc_perfmode_set(DYTC_MODE_QUIET);
> +		break;
> +	case 'm':
> +	case 'M':
> +		err = dytc_perfmode_set(DYTC_MODE_BALANCE);
> +		break;
> +	case 'h':
> +	case 'H':
> +		err = dytc_perfmode_set(DYTC_MODE_PERFORM);
> +		break;
> +	default:
> +		err = -EINVAL;
> +		pr_err("Unknown operating mode. Ignoring\n");
> +		break;
> +	}
> +	if (err)
> +		return err;
> +
> +	tpacpi_disclose_usertask(attr->attr.name,
> +				"Performance mode set to %c\n", buf[0]);
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(dytc_perfmode);
> +
> +static struct attribute *dytc_perfmode_attributes[] = {
> +	&dev_attr_dytc_perfmode.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group dytc_perf_attr_group = {
> +	.attrs = dytc_perfmode_attributes
> +};
> +
>  /* sysfs lapmode entry */
>  static ssize_t dytc_lapmode_show(struct device *dev,
>  					struct device_attribute *attr,
>  					char *buf)
>  {
> -	return snprintf(buf, PAGE_SIZE, "%d\n", dytc_lapmode);
> +	return sprintf(buf, "%d\n", dytc_lapmode);
>  }
> 
>  static DEVICE_ATTR_RO(dytc_lapmode);
> 
> -static struct attribute *dytc_attributes[] = {
> +static struct attribute *dytc_lap_attributes[] = {
>  	&dev_attr_dytc_lapmode.attr,
> -	NULL,
> +	NULL
>  };
> 
> -static const struct attribute_group dytc_attr_group = {
> -	.attrs = dytc_attributes,
> +static const struct attribute_group dytc_lap_attr_group = {
> +	.attrs = dytc_lap_attributes
>  };
> 
>  static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
>  {
> -	int err;
> +	int err, output;
> 
> -	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
> +	err = dytc_command(DYTC_CMD_QUERY, &output);
> +	/*
> +	 * If support isn't available (ENODEV) then don't return an error
> +	 * just don't create the sysfs group
>  	 */
>  	if (err == -ENODEV)
>  		return 0;
> @@ -9900,14 +10091,38 @@ 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);
> +	/* 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;
> +			/* Platform supports this feature - create the group */
> +			err = sysfs_create_group(&tpacpi_pdev->dev.kobj,
> &dytc_perf_attr_group);
> +			if (err)
> +				return err;
> +
> +			err = dytc_lapmode_get(&dytc_lapmode);
> +			if (err)
> +				return err;
> +			err = sysfs_create_group(&tpacpi_pdev->dev.kobj,
> &dytc_lap_attr_group);
> +		}
> +	}
>  	return err;
>  }
> 
>  static void dytc_exit(void)
>  {
> -	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
> +	if (dytc_available) {
> +		sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_lap_attr_group);
> +		sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_perf_attr_group);
> +	}
>  }
> 
>  static struct ibm_struct dytc_driver_data = {
> @@ -10057,7 +10272,7 @@ static void tpacpi_driver_event(const unsigned int
> hkey_event)
>  	}
> 
>  	if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED)
> -		dytc_lapmode_refresh();
> +		dytc_refresh();
> 
>  	if ((hkey_event == TP_HKEY_EV_PALM_DETECTED) ||
>  		(hkey_event == TP_HKEY_EV_PALM_UNDETECTED))
> --
> 2.26.2

Re: [External] RE: [PATCH v2] platform/x86: thinkpad_acpi: performance mode interface

[Mark Pearson]

Hi Mario

On 8/21/2020 3:15 PM, Limonciello, Mario wrote:
>>
<snip>
> 
> Thanks for adapting a lot of my feedback, this looks much better to me.
> 
No problems and genuinely thank you for the feedback. Sorry it took so 
long to get this update out - PTO and other things got in the way :)

>>
<snip>
>> +
>> +The sysfs entry provides the ability to return the current status and to set
>> the
>> +desired mode. For example::
>> +
>> +        echo H > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
>> +        echo M > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
>> +        echo L > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
>> +
> 
> I was thinking about this some more, do you actually want another mode that "disables"
> this feature?   IE "O" turns it off an calls DYTC_DISABLE_CQL.
> 
> For example if a user wanted to test the recently landed code in thermald 2.3
> and compare performance between the two it seems like this and that "might" fight.
> As an outsider looking in - I of course may be wrong too here.
> 
> If at some point in the future thermald does a better job than this implementation you
> might also want an "out" to let thermald or another piece of userland turn this off
> if it's in the picture.
> 
I'm still digging into this one. Right now I haven't found a good clean 
way of just disabling the firmware. Currently when thermald goes in and 
tweaks the CPU power registers it has the effect of overriding the FW 
anyway - but I appreciate that's not quite the same as actually doing it 
explicitly.

I'm still discussing with the BIOS team how to implement this - these 
conversations take time.... I suspect I'll need a new BIOS API which 
will take a while to implement and get rolled out. I didn't want to hold 
up this patch as there are benefits for users in the meantime and Lenovo 
do plan on supporting the thermal software going forwards. I'm not not 
dropping the plan to have a disable function as I can see the benefits.

Thanks for the input. Let me know if you want a reviewed-by tag added. I 
think it's kinda awesome having input from a Dell engineer on a Lenovo 
related driver and I hope in the future I can provide the same help ;) 
Open source is awesome.

Mark

Re: [External] RE: [PATCH v2] platform/x86: thinkpad_acpi: performance mode interface

[Limonciello, Mario]

> -----Original Message-----
> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
> owner@vger.kernel.org> On Behalf Of Mark Pearson
> Sent: Friday, August 21, 2020 14:36
> To: Limonciello, Mario
> Cc: ibm-acpi-devel@lists.sourceforge.net; platform-driver-x86@vger.kernel.org;
> ibm-acpi@hmh.eng.br; bnocera@redhat.com; Nitin Joshi
> Subject: Re: [External] RE: [PATCH v2] platform/x86: thinkpad_acpi:
> performance mode interface
> 
> 
> [EXTERNAL EMAIL]
> 
> Hi Mario
> 
> On 8/21/2020 3:15 PM, Limonciello, Mario wrote:
> >>
> <snip>
> >
> > Thanks for adapting a lot of my feedback, this looks much better to me.
> >
> No problems and genuinely thank you for the feedback. Sorry it took so
> long to get this update out - PTO and other things got in the way :)
> 
> >>
> <snip>
> >> +
> >> +The sysfs entry provides the ability to return the current status and to
> set
> >> the
> >> +desired mode. For example::
> >> +
> >> +        echo H > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
> >> +        echo M > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
> >> +        echo L > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
> >> +
> >
> > I was thinking about this some more, do you actually want another mode that
> "disables"
> > this feature?   IE "O" turns it off an calls DYTC_DISABLE_CQL.
> >
> > For example if a user wanted to test the recently landed code in thermald
> 2.3
> > and compare performance between the two it seems like this and that "might"
> fight.
> > As an outsider looking in - I of course may be wrong too here.
> >
> > If at some point in the future thermald does a better job than this
> implementation you
> > might also want an "out" to let thermald or another piece of userland turn
> this off
> > if it's in the picture.
> >
> I'm still digging into this one. Right now I haven't found a good clean
> way of just disabling the firmware. Currently when thermald goes in and
> tweaks the CPU power registers it has the effect of overriding the FW
> anyway - but I appreciate that's not quite the same as actually doing it
> explicitly.
> 

What about a modprobe parameter to disable at least?  That would at least
make it pretty easy to make a change, reboot and compare with thermald (or
other software) without disabling the rest of the functionality of the
thinkpad_acpi driver.

> I'm still discussing with the BIOS team how to implement this - these
> conversations take time.... I suspect I'll need a new BIOS API which
> will take a while to implement and get rolled out. I didn't want to hold
> up this patch as there are benefits for users in the meantime and Lenovo
> do plan on supporting the thermal software going forwards. I'm not not
> dropping the plan to have a disable function as I can see the benefits.
> 


Yup, well aware how these types of things go.

> Thanks for the input. Let me know if you want a reviewed-by tag added. I
> think it's kinda awesome having input from a Dell engineer on a Lenovo
> related driver and I hope in the future I can provide the same help ;)
> Open source is awesome.
> 
> Mark

😊

_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Re: [External] RE: [PATCH v2] platform/x86: thinkpad_acpi: performance mode interface

[Mark Pearson]

On 8/21/2020 4:00 PM, Limonciello, Mario wrote:
<snip>
>>>> + +The sysfs entry provides the ability to return the current
>>>> status and to set
>>>> the +desired mode. For example:: + +        echo H >
>>>> /sys/devices/platform/thinkpad_acpi/dytc_perfmode +        echo
>>>> M > /sys/devices/platform/thinkpad_acpi/dytc_perfmode +
>>>> echo L > /sys/devices/platform/thinkpad_acpi/dytc_perfmode +
>>> 
>>> I was thinking about this some more, do you actually want another
>>> mode that "disables"
>>> this feature?   IE "O" turns it off an calls DYTC_DISABLE_CQL.
>>> 
>>> For example if a user wanted to test the recently landed code in
>>> thermald 2.3
>>> and compare performance between the two it seems like this and
>>> that "might" fight.
>>> As an outsider looking in - I of course may be wrong too here.
>>> 
>>> If at some point in the future thermald does a better job than
>>> this implementation you
>>> might also want an "out" to let thermald or another piece of
>>> userland turn this off if it's in the picture.
>>> 
>> I'm still digging into this one. Right now I haven't found a good
>> clean way of just disabling the firmware. Currently when thermald
>> goes in and tweaks the CPU power registers it has the effect of
>> overriding the FW anyway - but I appreciate that's not quite the
>> same as actually doing it explicitly.
>> 
> 
> What about a modprobe parameter to disable at least?  That would at
> least make it pretty easy to make a change, reboot and compare with
> thermald (or other software) without disabling the rest of the
> functionality of the thinkpad_acpi driver.
> 
The problem is I don't have a good way to disable the firmware (that I 
know of yet) so a modprobe parameter wouldn't really do much. I guess it 
could skip providing the sysfs entry points - but the FW will still be 
there doing it's thing, so I'm not sure I see the benefit of that. At 
least the sysfs entry point gives a bit more insight into what is going on.
Let me know if I'm missing something obvious.

Mark

Re: [External] RE: [PATCH v2] platform/x86: thinkpad_acpi: performance mode interface

[Limonciello, Mario]

> -----Original Message-----
> From: Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> Sent: Friday, August 21, 2020 15:06
> To: Limonciello, Mario
> Cc: ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; platform-driver-x86-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> ibm-acpi-N3TV7GIv+o9fyO9Q7EP/yw@public.gmane.org; bnocera-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; Nitin Joshi
> Subject: Re: [External] RE: [PATCH v2] platform/x86: thinkpad_acpi:
> performance mode interface
> 
> 
> [EXTERNAL EMAIL]
> 
> On 8/21/2020 4:00 PM, Limonciello, Mario wrote:
> <snip>
> >>>> + +The sysfs entry provides the ability to return the current
> >>>> status and to set
> >>>> the +desired mode. For example:: + +        echo H >
> >>>> /sys/devices/platform/thinkpad_acpi/dytc_perfmode +        echo
> >>>> M > /sys/devices/platform/thinkpad_acpi/dytc_perfmode +
> >>>> echo L > /sys/devices/platform/thinkpad_acpi/dytc_perfmode +
> >>>
> >>> I was thinking about this some more, do you actually want another
> >>> mode that "disables"
> >>> this feature?   IE "O" turns it off an calls DYTC_DISABLE_CQL.
> >>>
> >>> For example if a user wanted to test the recently landed code in
> >>> thermald 2.3
> >>> and compare performance between the two it seems like this and
> >>> that "might" fight.
> >>> As an outsider looking in - I of course may be wrong too here.
> >>>
> >>> If at some point in the future thermald does a better job than
> >>> this implementation you
> >>> might also want an "out" to let thermald or another piece of
> >>> userland turn this off if it's in the picture.
> >>>
> >> I'm still digging into this one. Right now I haven't found a good
> >> clean way of just disabling the firmware. Currently when thermald
> >> goes in and tweaks the CPU power registers it has the effect of
> >> overriding the FW anyway - but I appreciate that's not quite the
> >> same as actually doing it explicitly.
> >>
> >
> > What about a modprobe parameter to disable at least?  That would at
> > least make it pretty easy to make a change, reboot and compare with
> > thermald (or other software) without disabling the rest of the
> > functionality of the thinkpad_acpi driver.
> >
> The problem is I don't have a good way to disable the firmware (that I
> know of yet) so a modprobe parameter wouldn't really do much. I guess it
> could skip providing the sysfs entry points - but the FW will still be
> there doing it's thing, so I'm not sure I see the benefit of that. At
> least the sysfs entry point gives a bit more insight into what is going on.
> Let me know if I'm missing something obvious.
> 

Oh so it's not actually the driver loading tells the firmware it's supposed
to work this way.  The firmware actually detects "I'm running on Linux, so I'll
do this differently"?

Re: [External] RE: [PATCH v2] platform/x86: thinkpad_acpi: performance mode interface

[Mark Pearson]

On 8/21/2020 4:43 PM, Limonciello, Mario wrote:
>> -----Original Message----- From: Mark Pearson 
>> <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org> Sent: Friday, August 21, 2020 15:06
>> 
>> On 8/21/2020 4:00 PM, Limonciello, Mario wrote: <snip>
>>>>>> + +The sysfs entry provides the ability to return the 
>>>>>> current status and to set the +desired mode. For example::
>>>>>>  + +        echo H > 
>>>>>> /sys/devices/platform/thinkpad_acpi/dytc_perfmode + echo M
>>>>>>> /sys/devices/platform/thinkpad_acpi/dytc_perfmode + echo
>>>>>> L > /sys/devices/platform/thinkpad_acpi/dytc_perfmode +
>>>>> 
>>>>> I was thinking about this some more, do you actually want 
>>>>> another mode that "disables" this feature?   IE "O" turns it
>>>>>  off an calls DYTC_DISABLE_CQL.
>>>>> 
>>>>> For example if a user wanted to test the recently landed code
>>>>> in thermald 2.3 and compare performance between the two it
>>>>> seems like this and that "might" fight. As an outsider 
>>>>> looking in - I of course may be wrong too here.
>>>>> 
>>>>> If at some point in the future thermald does a better job 
>>>>> than this implementation you might also want an "out" to let
>>>>>  thermald or another piece of userland turn this off if it's
>>>>>  in the picture.
>>>>> 
>>>> I'm still digging into this one. Right now I haven't found a 
>>>> good clean way of just disabling the firmware. Currently when 
>>>> thermald goes in and tweaks the CPU power registers it has the
>>>>  effect of overriding the FW anyway - but I appreciate that's 
>>>> not quite the same as actually doing it explicitly.
>>>> 
>>> 
>>> What about a modprobe parameter to disable at least?  That would
>>>  at least make it pretty easy to make a change, reboot and 
>>> compare with thermald (or other software) without disabling the 
>>> rest of the functionality of the thinkpad_acpi driver.
>>> 
>> The problem is I don't have a good way to disable the firmware 
>> (that I know of yet) so a modprobe parameter wouldn't really do 
>> much. I guess it could skip providing the sysfs entry points - but
>>  the FW will still be there doing it's thing, so I'm not sure I see
>>  the benefit of that. At least the sysfs entry point gives a bit 
>> more insight into what is going on. Let me know if I'm missing 
>> something obvious.
>> 
> 
> Oh so it's not actually the driver loading tells the firmware it's 
> supposed to work this way.  The firmware actually detects "I'm 
> running on Linux, so I'll do this differently"?
> 
Right. This patch is just providing a more friendly interface so user 
space can see what is going on and be able to do control of the 
different modes without the need for hotkeys.

Afraid I don't have much insight into the nitty gritty details of 
exactly how that works - I believe it's related to detecting the use of 
DPTF but the details aren't shared with me because of NDA's. As a 
thought it's quite possible (probable?) that if thermald is using DPTF 
the firmware will automatically disable itself. Proving that might be 
tricky - I'll see if the firmware team have any ideas.

Mark

Re: [External] RE: [PATCH v2] platform/x86: thinkpad_acpi: performance mode interface

[Limonciello, Mario]

> On 8/21/2020 4:43 PM, Limonciello, Mario wrote:
> >> -----Original Message----- From: Mark Pearson
> >> <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org> Sent: Friday, August 21, 2020 15:06
> >>
> >> On 8/21/2020 4:00 PM, Limonciello, Mario wrote: <snip>
> >>>>>> + +The sysfs entry provides the ability to return the
> >>>>>> current status and to set the +desired mode. For example::
> >>>>>>  + +        echo H >
> >>>>>> /sys/devices/platform/thinkpad_acpi/dytc_perfmode + echo M
> >>>>>>> /sys/devices/platform/thinkpad_acpi/dytc_perfmode + echo
> >>>>>> L > /sys/devices/platform/thinkpad_acpi/dytc_perfmode +
> >>>>>
> >>>>> I was thinking about this some more, do you actually want
> >>>>> another mode that "disables" this feature?   IE "O" turns it
> >>>>>  off an calls DYTC_DISABLE_CQL.
> >>>>>
> >>>>> For example if a user wanted to test the recently landed code
> >>>>> in thermald 2.3 and compare performance between the two it
> >>>>> seems like this and that "might" fight. As an outsider
> >>>>> looking in - I of course may be wrong too here.
> >>>>>
> >>>>> If at some point in the future thermald does a better job
> >>>>> than this implementation you might also want an "out" to let
> >>>>>  thermald or another piece of userland turn this off if it's
> >>>>>  in the picture.
> >>>>>
> >>>> I'm still digging into this one. Right now I haven't found a
> >>>> good clean way of just disabling the firmware. Currently when
> >>>> thermald goes in and tweaks the CPU power registers it has the
> >>>>  effect of overriding the FW anyway - but I appreciate that's
> >>>> not quite the same as actually doing it explicitly.
> >>>>
> >>>
> >>> What about a modprobe parameter to disable at least?  That would
> >>>  at least make it pretty easy to make a change, reboot and
> >>> compare with thermald (or other software) without disabling the
> >>> rest of the functionality of the thinkpad_acpi driver.
> >>>
> >> The problem is I don't have a good way to disable the firmware
> >> (that I know of yet) so a modprobe parameter wouldn't really do
> >> much. I guess it could skip providing the sysfs entry points - but
> >>  the FW will still be there doing it's thing, so I'm not sure I see
> >>  the benefit of that. At least the sysfs entry point gives a bit
> >> more insight into what is going on. Let me know if I'm missing
> >> something obvious.
> >>
> >
> > Oh so it's not actually the driver loading tells the firmware it's
> > supposed to work this way.  The firmware actually detects "I'm
> > running on Linux, so I'll do this differently"?
> >
> Right. This patch is just providing a more friendly interface so user
> space can see what is going on and be able to do control of the
> different modes without the need for hotkeys.
> 
> Afraid I don't have much insight into the nitty gritty details of
> exactly how that works - I believe it's related to detecting the use of
> DPTF but the details aren't shared with me because of NDA's. As a
> thought it's quite possible (probable?) that if thermald is using DPTF
> the firmware will automatically disable itself. Proving that might be
> tricky - I'll see if the firmware team have any ideas.
> 

Right - it's a reverse engineered implementation done in thermald and there
are still some missing items.  So your comment makes perfect sense to me now,
and if/when those other things are added this may change and it will make
sense to offer a way to turn this off at that time.

Re: [PATCH v2] platform/x86: thinkpad_acpi: performance mode interface

[Benjamin Berg]

[-- Attachment #1.1: Type: text/plain, Size: 15461 bytes --]

Hi Mark,

On Fri, 2020-08-21 at 13:53 -0400, Mark Pearson wrote:
> +		H  - High performance mode. Maximum power and temperature available.
> +		M* - High performance mode but performance is limited to medium as system is
> +		     in lapmode. Power and temperature maximums reduced to a safe threshold.
> +		M  - Medium performance mode (aka 'balance'). Lower maximum power and temperatures
> +		     but better battery life.
> +		L  - Low performance mode (aka 'quiet'). Reduced power setting gives lower
> +		     temperatures and extended battery life. Fans run quieter.

I tested the patch yesterday and I see some minor issues right now.

First, right now change notifications do not happen for a lapmode
change. The sysfs file will switch between "H" and "M*" without any
notification. This will be an easy fix.

Second, I personally see the current "M*" more as a degraded
performance mode rather than an effective balanced mode. For example, H
and M* match in the sense that the machine will be more noisy as fans
are turned on more aggressively.

Third, we still have a mismatch when writing the file. i.e. you write
"H" but you will read "M*".

So, I am thinking of using H* instead. That means we directly represent
the firmwares performance mode rather than making an interpretation
about an "effective" state. And with the * we continue to convey that
there is a major impact on performance due to other factors.

Benjamin

> +What:		/sys/devices/platform/thinkpad_acpi/dytc_lapmod
> e
> +Date:		August 2020
> +Contact:	Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> +Description:
> +		Reads returns the current value of the lapmode sensor.
> +
> +		0 - desk mode is detected
> +		1 - lap mode is detected
> +
> +What:		/sys/devices/platform/thinkpad_acpi/psensor_sta
> te
> +Date:		August 2020
> +Contact:	Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
> +Description:
> +		Reads returns the current value of the palm sensor.
> +
> +		0 - palm not detected
> +		1 - palm detected
> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> index 6b57c52d8f13..b98f0de9e063 100644
> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> @@ -52,6 +52,7 @@ detailed description):
>  	- LCD Shadow (PrivacyGuard) enable and disable
>  	- Lap mode sensor
>          - Palm sensor (aka psensor)
> +	- Thermal mode status and control
>  
>  A compatibility table by model and feature is maintained on the web
>  site, http://ibm-acpi.sf.net/. I appreciate any success or failure
> @@ -1465,6 +1466,40 @@ Note - some platforms have a limitation
> whereby the EC firmware cannot
>  determine if the sensor is installed or not. On these platforms the
> psensor
>  state will always be reported as true to avoid high power being used
> incorrectly.
>  
> +DYTC Thermal mode status and control
> +------------------------------------
> +
> +sysfs: dytc_perfmode
> +
> +Lenovo Thinkpad platforms with DYTC version 5 and newer have
> enhanced firmware to
> +provide improved performance control.
> +
> +The firmware can be controlled by hotkeys (FN+H, FN+M, FN+L) to
> switch the
> +operating mode between three different modes. This sysfs node
> provides a better
> +interface for user space to use.
> +
> +The modes available are:
> +
> +H - High performance. Maximum power is available and the temperature
> is
> +allowed to increase to the maximum for the platform.
> +
> +M - Medium performance (aka balance). In this mode power will be
> limited and
> +the laptop will remain cooler.
> +
> +L - Low performance (aka quiet). In this mode power consumption is
> reduced and
> +the device will be cooler and quieter.
> +
> +Note: High performance mode is only available when the device is in
> 'deskmode'. If
> +the device detects that it is on a lap then it will automatically
> drop into medium
> +mode to maintain a safer operating temperature.
> +
> +The sysfs entry provides the ability to return the current status
> and to set the
> +desired mode. For example::
> +
> +        echo H > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
> +        echo M > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
> +        echo L > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
> +
>  EXPERIMENTAL: UWB
>  -----------------
>  
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index 41b75dd4755c..8fcb660aa5a2 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -9817,18 +9817,43 @@ static struct ibm_struct
> lcdshadow_driver_data = {
>  };
>  
>  /*******************************************************************
> ******
> - * DYTC subdriver, for the Lenovo lapmode feature
> + * DYTC subdriver, for the Lenovo lap and performance mode 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 */
>  
> -static bool dytc_lapmode;
> +#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 */
>  
> -static void dytc_lapmode_notify_change(void)
> -{
> -	sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "dytc_lapmode");
> -}
> +#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 int  dytc_perfmode;
> +static bool dytc_available;
> +static bool dytc_ignore_next_event;
>  
>  static int dytc_command(int command, int *output)
>  {
> @@ -9843,6 +9868,87 @@ static int dytc_command(int command, int
> *output)
>  	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;
> +}
> +
> +static int dytc_perfmode_set(int perfmode)
> +{
> +	int err, dytc_set;
> +	int output;
> +	int cur_perfmode, cur_funcmode;
> +
> +	if (!dytc_available)
> +		return -ENODEV;
> +
> +	if (perfmode == DYTC_MODE_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 {
> +		/* 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;
> +		}
> +	}
> +	return 0;
> +}
> +
>  static int dytc_lapmode_get(bool *state)
>  {
>  	int output, err;
> @@ -9854,45 +9960,130 @@ static int dytc_lapmode_get(bool *state)
>  	return 0;
>  }
>  
> -static void dytc_lapmode_refresh(void)
> +static void dytc_refresh(void)
>  {
> -	bool new_state;
> +	bool lapmode;
> +	int perfmode, funcmode;
>  	int err;
>  
> -	err = dytc_lapmode_get(&new_state);
> -	if (err || (new_state == dytc_lapmode))
> +	err = dytc_lapmode_get(&lapmode);
> +	if (err)
> +		return;
> +	if (dytc_ignore_next_event) {
> +		dytc_ignore_next_event = false; /*clear setting*/
>  		return;
> +	}
> +	if (lapmode != dytc_lapmode) {
> +		dytc_lapmode = lapmode;
> +		sysfs_notify(&tpacpi_pdev->dev.kobj, NULL,
> "dytc_lapmode");
> +	}
> +	err = dytc_perfmode_get(&perfmode, &funcmode);
> +	if (err)
> +		return;
> +	if (perfmode != dytc_perfmode) {
> +		dytc_perfmode = perfmode;
> +		sysfs_notify(&tpacpi_pdev->dev.kobj, NULL,
> "dytc_perfmode");
> +	}
> +}
> +
> +/* sysfs perfmode entry */
> +static ssize_t dytc_perfmode_show(struct device *dev,
> +				  struct device_attribute *attr,
> +				  char *buf)
> +{
> +	int err;
> +	int perfmode, funcmode;
> +
> +	err = dytc_perfmode_get(&perfmode, &funcmode);
> +	if (err)
> +		return err;
>  
> -	dytc_lapmode = new_state;
> -	dytc_lapmode_notify_change();
> +	switch (perfmode) {
> +	case DYTC_MODE_PERFORM:
> +		/* High performance is only available in deskmode */
> +		if (funcmode == DYTC_FUNCTION_CQL)
> +			return sprintf(buf, "M*\n");
> +		else
> +			return sprintf(buf, "H\n");
> +	case DYTC_MODE_QUIET:
> +		return sprintf(buf, "L\n");
> +	case DYTC_MODE_BALANCE:
> +		return sprintf(buf, "M\n");
> +	default:
> +		return sprintf(buf, "Unknown (%d)\n", perfmode);
> +	}
>  }
>  
> +static ssize_t dytc_perfmode_store(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	int err;
> +
> +	switch (buf[0]) {
> +	case 'l':
> +	case 'L':
> +		err = dytc_perfmode_set(DYTC_MODE_QUIET);
> +		break;
> +	case 'm':
> +	case 'M':
> +		err = dytc_perfmode_set(DYTC_MODE_BALANCE);
> +		break;
> +	case 'h':
> +	case 'H':
> +		err = dytc_perfmode_set(DYTC_MODE_PERFORM);
> +		break;
> +	default:
> +		err = -EINVAL;
> +		pr_err("Unknown operating mode. Ignoring\n");
> +		break;
> +	}
> +	if (err)
> +		return err;
> +
> +	tpacpi_disclose_usertask(attr->attr.name,
> +				"Performance mode set to %c\n",
> buf[0]);
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(dytc_perfmode);
> +
> +static struct attribute *dytc_perfmode_attributes[] = {
> +	&dev_attr_dytc_perfmode.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group dytc_perf_attr_group = {
> +	.attrs = dytc_perfmode_attributes
> +};
> +
>  /* sysfs lapmode entry */
>  static ssize_t dytc_lapmode_show(struct device *dev,
>  					struct device_attribute *attr,
>  					char *buf)
>  {
> -	return snprintf(buf, PAGE_SIZE, "%d\n", dytc_lapmode);
> +	return sprintf(buf, "%d\n", dytc_lapmode);
>  }
>  
>  static DEVICE_ATTR_RO(dytc_lapmode);
>  
> -static struct attribute *dytc_attributes[] = {
> +static struct attribute *dytc_lap_attributes[] = {
>  	&dev_attr_dytc_lapmode.attr,
> -	NULL,
> +	NULL
>  };
>  
> -static const struct attribute_group dytc_attr_group = {
> -	.attrs = dytc_attributes,
> +static const struct attribute_group dytc_lap_attr_group = {
> +	.attrs = dytc_lap_attributes
>  };
>  
>  static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
>  {
> -	int err;
> +	int err, output;
>  
> -	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
> +	err = dytc_command(DYTC_CMD_QUERY, &output);
> +	/*
> +	 * If support isn't available (ENODEV) then don't return an
> error
> +	 * just don't create the sysfs group
>  	 */
>  	if (err == -ENODEV)
>  		return 0;
> @@ -9900,14 +10091,38 @@ 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);
> +	/* 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;
> +			/* Platform supports this feature - create the
> group */
> +			err = sysfs_create_group(&tpacpi_pdev-
> >dev.kobj, &dytc_perf_attr_group);
> +			if (err)
> +				return err;
> +
> +			err = dytc_lapmode_get(&dytc_lapmode);
> +			if (err)
> +				return err;
> +			err = sysfs_create_group(&tpacpi_pdev-
> >dev.kobj, &dytc_lap_attr_group);
> +		}
> +	}
>  	return err;
>  }
>  
>  static void dytc_exit(void)
>  {
> -	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
> +	if (dytc_available) {
> +		sysfs_remove_group(&tpacpi_pdev->dev.kobj,
> &dytc_lap_attr_group);
> +		sysfs_remove_group(&tpacpi_pdev->dev.kobj,
> &dytc_perf_attr_group);
> +	}
>  }
>  
>  static struct ibm_struct dytc_driver_data = {
> @@ -10057,7 +10272,7 @@ static void tpacpi_driver_event(const
> unsigned int hkey_event)
>  	}
>  
>  	if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED)
> -		dytc_lapmode_refresh();
> +		dytc_refresh();
>  
>  	if ((hkey_event == TP_HKEY_EV_PALM_DETECTED) ||
>  		(hkey_event == TP_HKEY_EV_PALM_UNDETECTED))

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 201 bytes --]

_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Re: [External] Re: [PATCH v2] platform/x86: thinkpad_acpi: performance mode interface

[Mark Pearson]

Thanks Benjamin!

On 9/16/2020 5:31 AM, Benjamin Berg wrote:
> Hi Mark,
> 
> On Fri, 2020-08-21 at 13:53 -0400, Mark Pearson wrote:
>> +		H  - High performance mode. Maximum power and temperature available.
>> +		M* - High performance mode but performance is limited to medium as system is
>> +		     in lapmode. Power and temperature maximums reduced to a safe threshold.
>> +		M  - Medium performance mode (aka 'balance'). Lower maximum power and temperatures
>> +		     but better battery life.
>> +		L  - Low performance mode (aka 'quiet'). Reduced power setting gives lower
>> +		     temperatures and extended battery life. Fans run quieter.
> 
> I tested the patch yesterday and I see some minor issues right now.
> 
> First, right now change notifications do not happen for a lapmode
> change. The sysfs file will switch between "H" and "M*" without any
> notification. This will be an easy fix.
OK - I'll look into this and get that implemented. Good catch.
> 
> Second, I personally see the current "M*" more as a degraded
> performance mode rather than an effective balanced mode. For example, H
> and M* match in the sense that the machine will be more noisy as fans
> are turned on more aggressively.
> 
> Third, we still have a mismatch when writing the file. i.e. you write
> "H" but you will read "M*".
> 
> So, I am thinking of using H* instead. That means we directly represent
> the firmwares performance mode rather than making an interpretation
> about an "effective" state. And with the * we continue to convey that
> there is a major impact on performance due to other factors.
I agree - that makes sense to me. I'll make that change.

Thanks for the review
Mark

> 
> Benjamin
> 
>> +What:		/sys/devices/platform/thinkpad_acpi/dytc_lapmod
>> e
>> +Date:		August 2020
>> +Contact:	Mark Pearson <markpearson-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
>> +Description:
>> +		Reads returns the current value of the lapmode sensor.
>> +
>> +		0 - desk mode is detected
>> +		1 - lap mode is detected
>> +
>> +What:		/sys/devices/platform/thinkpad_acpi/psensor_sta
>> te
>> +Date:		August 2020
>> +Contact:	Nitin Joshi <njoshi1-6jq1YtArVR3QT0dZR+AlfA@public.gmane.org>
>> +Description:
>> +		Reads returns the current value of the palm sensor.
>> +
>> +		0 - palm not detected
>> +		1 - palm detected
>> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> index 6b57c52d8f13..b98f0de9e063 100644
>> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> @@ -52,6 +52,7 @@ detailed description):
>>   	- LCD Shadow (PrivacyGuard) enable and disable
>>   	- Lap mode sensor
>>           - Palm sensor (aka psensor)
>> +	- Thermal mode status and control
>>   
>>   A compatibility table by model and feature is maintained on the web
>>   site, http://ibm-acpi.sf.net/. I appreciate any success or failure
>> @@ -1465,6 +1466,40 @@ Note - some platforms have a limitation
>> whereby the EC firmware cannot
>>   determine if the sensor is installed or not. On these platforms the
>> psensor
>>   state will always be reported as true to avoid high power being used
>> incorrectly.
>>   
>> +DYTC Thermal mode status and control
>> +------------------------------------
>> +
>> +sysfs: dytc_perfmode
>> +
>> +Lenovo Thinkpad platforms with DYTC version 5 and newer have
>> enhanced firmware to
>> +provide improved performance control.
>> +
>> +The firmware can be controlled by hotkeys (FN+H, FN+M, FN+L) to
>> switch the
>> +operating mode between three different modes. This sysfs node
>> provides a better
>> +interface for user space to use.
>> +
>> +The modes available are:
>> +
>> +H - High performance. Maximum power is available and the temperature
>> is
>> +allowed to increase to the maximum for the platform.
>> +
>> +M - Medium performance (aka balance). In this mode power will be
>> limited and
>> +the laptop will remain cooler.
>> +
>> +L - Low performance (aka quiet). In this mode power consumption is
>> reduced and
>> +the device will be cooler and quieter.
>> +
>> +Note: High performance mode is only available when the device is in
>> 'deskmode'. If
>> +the device detects that it is on a lap then it will automatically
>> drop into medium
>> +mode to maintain a safer operating temperature.
>> +
>> +The sysfs entry provides the ability to return the current status
>> and to set the
>> +desired mode. For example::
>> +
>> +        echo H > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
>> +        echo M > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
>> +        echo L > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
>> +
>>   EXPERIMENTAL: UWB
>>   -----------------
>>   
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c
>> b/drivers/platform/x86/thinkpad_acpi.c
>> index 41b75dd4755c..8fcb660aa5a2 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -9817,18 +9817,43 @@ static struct ibm_struct
>> lcdshadow_driver_data = {
>>   };
>>   
>>   /*******************************************************************
>> ******
>> - * DYTC subdriver, for the Lenovo lapmode feature
>> + * DYTC subdriver, for the Lenovo lap and performance mode 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 */
>>   
>> -static bool dytc_lapmode;
>> +#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 */
>>   
>> -static void dytc_lapmode_notify_change(void)
>> -{
>> -	sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "dytc_lapmode");
>> -}
>> +#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 int  dytc_perfmode;
>> +static bool dytc_available;
>> +static bool dytc_ignore_next_event;
>>   
>>   static int dytc_command(int command, int *output)
>>   {
>> @@ -9843,6 +9868,87 @@ static int dytc_command(int command, int
>> *output)
>>   	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;
>> +}
>> +
>> +static int dytc_perfmode_set(int perfmode)
>> +{
>> +	int err, dytc_set;
>> +	int output;
>> +	int cur_perfmode, cur_funcmode;
>> +
>> +	if (!dytc_available)
>> +		return -ENODEV;
>> +
>> +	if (perfmode == DYTC_MODE_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 {
>> +		/* 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;
>> +		}
>> +	}
>> +	return 0;
>> +}
>> +
>>   static int dytc_lapmode_get(bool *state)
>>   {
>>   	int output, err;
>> @@ -9854,45 +9960,130 @@ static int dytc_lapmode_get(bool *state)
>>   	return 0;
>>   }
>>   
>> -static void dytc_lapmode_refresh(void)
>> +static void dytc_refresh(void)
>>   {
>> -	bool new_state;
>> +	bool lapmode;
>> +	int perfmode, funcmode;
>>   	int err;
>>   
>> -	err = dytc_lapmode_get(&new_state);
>> -	if (err || (new_state == dytc_lapmode))
>> +	err = dytc_lapmode_get(&lapmode);
>> +	if (err)
>> +		return;
>> +	if (dytc_ignore_next_event) {
>> +		dytc_ignore_next_event = false; /*clear setting*/
>>   		return;
>> +	}
>> +	if (lapmode != dytc_lapmode) {
>> +		dytc_lapmode = lapmode;
>> +		sysfs_notify(&tpacpi_pdev->dev.kobj, NULL,
>> "dytc_lapmode");
>> +	}
>> +	err = dytc_perfmode_get(&perfmode, &funcmode);
>> +	if (err)
>> +		return;
>> +	if (perfmode != dytc_perfmode) {
>> +		dytc_perfmode = perfmode;
>> +		sysfs_notify(&tpacpi_pdev->dev.kobj, NULL,
>> "dytc_perfmode");
>> +	}
>> +}
>> +
>> +/* sysfs perfmode entry */
>> +static ssize_t dytc_perfmode_show(struct device *dev,
>> +				  struct device_attribute *attr,
>> +				  char *buf)
>> +{
>> +	int err;
>> +	int perfmode, funcmode;
>> +
>> +	err = dytc_perfmode_get(&perfmode, &funcmode);
>> +	if (err)
>> +		return err;
>>   
>> -	dytc_lapmode = new_state;
>> -	dytc_lapmode_notify_change();
>> +	switch (perfmode) {
>> +	case DYTC_MODE_PERFORM:
>> +		/* High performance is only available in deskmode */
>> +		if (funcmode == DYTC_FUNCTION_CQL)
>> +			return sprintf(buf, "M*\n");
>> +		else
>> +			return sprintf(buf, "H\n");
>> +	case DYTC_MODE_QUIET:
>> +		return sprintf(buf, "L\n");
>> +	case DYTC_MODE_BALANCE:
>> +		return sprintf(buf, "M\n");
>> +	default:
>> +		return sprintf(buf, "Unknown (%d)\n", perfmode);
>> +	}
>>   }
>>   
>> +static ssize_t dytc_perfmode_store(struct device *dev,
>> +				   struct device_attribute *attr,
>> +				   const char *buf, size_t count)
>> +{
>> +	int err;
>> +
>> +	switch (buf[0]) {
>> +	case 'l':
>> +	case 'L':
>> +		err = dytc_perfmode_set(DYTC_MODE_QUIET);
>> +		break;
>> +	case 'm':
>> +	case 'M':
>> +		err = dytc_perfmode_set(DYTC_MODE_BALANCE);
>> +		break;
>> +	case 'h':
>> +	case 'H':
>> +		err = dytc_perfmode_set(DYTC_MODE_PERFORM);
>> +		break;
>> +	default:
>> +		err = -EINVAL;
>> +		pr_err("Unknown operating mode. Ignoring\n");
>> +		break;
>> +	}
>> +	if (err)
>> +		return err;
>> +
>> +	tpacpi_disclose_usertask(attr->attr.name,
>> +				"Performance mode set to %c\n",
>> buf[0]);
>> +	return count;
>> +}
>> +
>> +static DEVICE_ATTR_RW(dytc_perfmode);
>> +
>> +static struct attribute *dytc_perfmode_attributes[] = {
>> +	&dev_attr_dytc_perfmode.attr,
>> +	NULL
>> +};
>> +
>> +static const struct attribute_group dytc_perf_attr_group = {
>> +	.attrs = dytc_perfmode_attributes
>> +};
>> +
>>   /* sysfs lapmode entry */
>>   static ssize_t dytc_lapmode_show(struct device *dev,
>>   					struct device_attribute *attr,
>>   					char *buf)
>>   {
>> -	return snprintf(buf, PAGE_SIZE, "%d\n", dytc_lapmode);
>> +	return sprintf(buf, "%d\n", dytc_lapmode);
>>   }
>>   
>>   static DEVICE_ATTR_RO(dytc_lapmode);
>>   
>> -static struct attribute *dytc_attributes[] = {
>> +static struct attribute *dytc_lap_attributes[] = {
>>   	&dev_attr_dytc_lapmode.attr,
>> -	NULL,
>> +	NULL
>>   };
>>   
>> -static const struct attribute_group dytc_attr_group = {
>> -	.attrs = dytc_attributes,
>> +static const struct attribute_group dytc_lap_attr_group = {
>> +	.attrs = dytc_lap_attributes
>>   };
>>   
>>   static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
>>   {
>> -	int err;
>> +	int err, output;
>>   
>> -	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
>> +	err = dytc_command(DYTC_CMD_QUERY, &output);
>> +	/*
>> +	 * If support isn't available (ENODEV) then don't return an
>> error
>> +	 * just don't create the sysfs group
>>   	 */
>>   	if (err == -ENODEV)
>>   		return 0;
>> @@ -9900,14 +10091,38 @@ 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);
>> +	/* 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;
>> +			/* Platform supports this feature - create the
>> group */
>> +			err = sysfs_create_group(&tpacpi_pdev-
>>> dev.kobj, &dytc_perf_attr_group);
>> +			if (err)
>> +				return err;
>> +
>> +			err = dytc_lapmode_get(&dytc_lapmode);
>> +			if (err)
>> +				return err;
>> +			err = sysfs_create_group(&tpacpi_pdev-
>>> dev.kobj, &dytc_lap_attr_group);
>> +		}
>> +	}
>>   	return err;
>>   }
>>   
>>   static void dytc_exit(void)
>>   {
>> -	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
>> +	if (dytc_available) {
>> +		sysfs_remove_group(&tpacpi_pdev->dev.kobj,
>> &dytc_lap_attr_group);
>> +		sysfs_remove_group(&tpacpi_pdev->dev.kobj,
>> &dytc_perf_attr_group);
>> +	}
>>   }
>>   
>>   static struct ibm_struct dytc_driver_data = {
>> @@ -10057,7 +10272,7 @@ static void tpacpi_driver_event(const
>> unsigned int hkey_event)
>>   	}
>>   
>>   	if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED)
>> -		dytc_lapmode_refresh();
>> +		dytc_refresh();
>>   
>>   	if ((hkey_event == TP_HKEY_EV_PALM_DETECTED) ||
>>   		(hkey_event == TP_HKEY_EV_PALM_UNDETECTED))

Re: [PATCH v2] platform/x86: thinkpad_acpi: performance mode interface

[Hans de Goede]

Hi,

On 8/21/20 7:53 PM, Mark Pearson wrote:
> Lenovo Thinkpad platforms with DYTC version 5 and newer have enhanced
> firmware to provide different performance/thermal modes.
> 
> The modes can be controlled by hotkeys (FN+H, FN+M, FN+L) to switch
> the operating mode between three different modes.
> 
> H - High performance. Maximum power is available and the temperature is
> allowed to increase to the maximum for the platform.
> M - Medium performance (aka balance). In this mode power will be limited
> and the laptop will have a lower maximum temperature.
> L - Low performance (aka quiet). In this mode power consumption is reduced
> and the device will be cooler.
> 
> High performance mode is only available when the device is in 'desk mode'.
> If the device detects that it is on a lap then it will automatically drop
> into medium mode to maintain a safer operating temperature.
> 
> This patch provides an interface to determine the current mode and to also
> allow the setting of the mode through the dytc_perfmode sysfs entry. This
> can be used by userspace for improved control.
> 
> Reviewed-by: Nitin Joshi <njoshi1@lenovo.com>
> Signed-off-by: Mark Pearson <markpearson@lenovo.com>
> ---
> Changes in v2:
>   - Add userspace notification to hkey event handler. Note this got
>     somewhat more complicated than expected as retrieving the performance
>     mode when in lapmode generates an event itself; added a flag to
>     ignore these known events.
>   - Updated the values returned to be simpler (H/M*/M/L) as suggested.
>   - Added ABI documentation as requested. Lap and palm sensor details
>     added whilst I was doing this.
>   - Cleaned up code based on recommendations as well as feedback received
>     from other patch reviews.
>   - Based on discussion with firmware team the lapmode sensor should only
>     be made available for DYTC v5 and later. Added this to init logic.
> 
>   .../sysfs-devices-platform-thinkpad_acpi      |  34 +++
>   .../admin-guide/laptops/thinkpad-acpi.rst     |  35 +++
>   drivers/platform/x86/thinkpad_acpi.c          | 267 ++++++++++++++++--
>   3 files changed, 310 insertions(+), 26 deletions(-)
>   create mode 100644 Documentation/ABI/testing/sysfs-devices-platform-thinkpad_acpi
> 
> diff --git a/Documentation/ABI/testing/sysfs-devices-platform-thinkpad_acpi b/Documentation/ABI/testing/sysfs-devices-platform-thinkpad_acpi
> new file mode 100644
> index 000000000000..28f07753a889
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-devices-platform-thinkpad_acpi
> @@ -0,0 +1,34 @@
> +What:		/sys/devices/platform/thinkpad_acpi/dytc_perfmode
> +Date:		August 2020
> +Contact:	Mark Pearson <markpearson@lenovo.com>
> +Description:
> +		Reads return the current performance mode setting configured in firmware using
> +		the below nomenclature.
> +
> +		Writes configure the performance mode setting by using the below nomenclature.
> +
> +		H  - High performance mode. Maximum power and temperature available.
> +		M* - High performance mode but performance is limited to medium as system is
> +		     in lapmode. Power and temperature maximums reduced to a safe threshold.
> +		M  - Medium performance mode (aka 'balance'). Lower maximum power and temperatures
> +		     but better battery life.
> +		L  - Low performance mode (aka 'quiet'). Reduced power setting gives lower
> +		     temperatures and extended battery life. Fans run quieter.
> +
> +What:		/sys/devices/platform/thinkpad_acpi/dytc_lapmode
> +Date:		August 2020
> +Contact:	Mark Pearson <markpearson@lenovo.com>
> +Description:
> +		Reads returns the current value of the lapmode sensor.
> +
> +		0 - desk mode is detected
> +		1 - lap mode is detected
> +
> +What:		/sys/devices/platform/thinkpad_acpi/psensor_state
> +Date:		August 2020
> +Contact:	Nitin Joshi <njoshi1@lenovo.com>
> +Description:
> +		Reads returns the current value of the palm sensor.
> +
> +		0 - palm not detected
> +		1 - palm detected
> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> index 6b57c52d8f13..b98f0de9e063 100644
> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> @@ -52,6 +52,7 @@ detailed description):
>   	- LCD Shadow (PrivacyGuard) enable and disable
>   	- Lap mode sensor
>           - Palm sensor (aka psensor)
> +	- Thermal mode status and control
>   
>   A compatibility table by model and feature is maintained on the web
>   site, http://ibm-acpi.sf.net/. I appreciate any success or failure
> @@ -1465,6 +1466,40 @@ Note - some platforms have a limitation whereby the EC firmware cannot
>   determine if the sensor is installed or not. On these platforms the psensor
>   state will always be reported as true to avoid high power being used incorrectly.
>   
> +DYTC Thermal mode status and control
> +------------------------------------
> +
> +sysfs: dytc_perfmode
> +
> +Lenovo Thinkpad platforms with DYTC version 5 and newer have enhanced firmware to
> +provide improved performance control.
> +
> +The firmware can be controlled by hotkeys (FN+H, FN+M, FN+L) to switch the
> +operating mode between three different modes. This sysfs node provides a better
> +interface for user space to use.
> +
> +The modes available are:
> +
> +H - High performance. Maximum power is available and the temperature is
> +allowed to increase to the maximum for the platform.
> +
> +M - Medium performance (aka balance). In this mode power will be limited and
> +the laptop will remain cooler.
> +
> +L - Low performance (aka quiet). In this mode power consumption is reduced and
> +the device will be cooler and quieter.
> +
> +Note: High performance mode is only available when the device is in 'deskmode'. If
> +the device detects that it is on a lap then it will automatically drop into medium
> +mode to maintain a safer operating temperature.
> +
> +The sysfs entry provides the ability to return the current status and to set the
> +desired mode. For example::
> +
> +        echo H > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
> +        echo M > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
> +        echo L > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
> +


So we recently had another driver show up with almost the same interface / thing:

https://patchwork.kernel.org/patch/11774509/

So I believe that we really should come up with a standardize sysfs interface
for this under /sys/class/<some-name>

Please see the "RFC: offering a standardized (/sys/class) userspace API for selecting
system/laptop performance-profiles" mail thread which I just started.

Note I've not looked at the rest of this patch at all yet. But this patch should
not be merged until the userspace API question is settled so nack to this patch
for now (sorry).

Mark, once we have an agreement on what the userspace API for this should
look like I can implement a small helper-library / class-core for registering
the class device for this, modify this patch to use that and test it on a X1C8.

Or if you prefer you can do this yourself. Please let me know how you want to
proceed with this.

Regards,

Hans



>   EXPERIMENTAL: UWB
>   -----------------
>   
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 41b75dd4755c..8fcb660aa5a2 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -9817,18 +9817,43 @@ static struct ibm_struct lcdshadow_driver_data = {
>   };
>   
>   /*************************************************************************
> - * DYTC subdriver, for the Lenovo lapmode feature
> + * DYTC subdriver, for the Lenovo lap and performance mode 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 */
>   
> -static bool dytc_lapmode;
> +#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 */
>   
> -static void dytc_lapmode_notify_change(void)
> -{
> -	sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "dytc_lapmode");
> -}
> +#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 int  dytc_perfmode;
> +static bool dytc_available;
> +static bool dytc_ignore_next_event;
>   
>   static int dytc_command(int command, int *output)
>   {
> @@ -9843,6 +9868,87 @@ static int dytc_command(int command, int *output)
>   	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;
> +}
> +
> +static int dytc_perfmode_set(int perfmode)
> +{
> +	int err, dytc_set;
> +	int output;
> +	int cur_perfmode, cur_funcmode;
> +
> +	if (!dytc_available)
> +		return -ENODEV;
> +
> +	if (perfmode == DYTC_MODE_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 {
> +		/* 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;
> +		}
> +	}
> +	return 0;
> +}
> +
>   static int dytc_lapmode_get(bool *state)
>   {
>   	int output, err;
> @@ -9854,45 +9960,130 @@ static int dytc_lapmode_get(bool *state)
>   	return 0;
>   }
>   
> -static void dytc_lapmode_refresh(void)
> +static void dytc_refresh(void)
>   {
> -	bool new_state;
> +	bool lapmode;
> +	int perfmode, funcmode;
>   	int err;
>   
> -	err = dytc_lapmode_get(&new_state);
> -	if (err || (new_state == dytc_lapmode))
> +	err = dytc_lapmode_get(&lapmode);
> +	if (err)
> +		return;
> +	if (dytc_ignore_next_event) {
> +		dytc_ignore_next_event = false; /*clear setting*/
>   		return;
> +	}
> +	if (lapmode != dytc_lapmode) {
> +		dytc_lapmode = lapmode;
> +		sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "dytc_lapmode");
> +	}
> +	err = dytc_perfmode_get(&perfmode, &funcmode);
> +	if (err)
> +		return;
> +	if (perfmode != dytc_perfmode) {
> +		dytc_perfmode = perfmode;
> +		sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "dytc_perfmode");
> +	}
> +}
> +
> +/* sysfs perfmode entry */
> +static ssize_t dytc_perfmode_show(struct device *dev,
> +				  struct device_attribute *attr,
> +				  char *buf)
> +{
> +	int err;
> +	int perfmode, funcmode;
> +
> +	err = dytc_perfmode_get(&perfmode, &funcmode);
> +	if (err)
> +		return err;
>   
> -	dytc_lapmode = new_state;
> -	dytc_lapmode_notify_change();
> +	switch (perfmode) {
> +	case DYTC_MODE_PERFORM:
> +		/* High performance is only available in deskmode */
> +		if (funcmode == DYTC_FUNCTION_CQL)
> +			return sprintf(buf, "M*\n");
> +		else
> +			return sprintf(buf, "H\n");
> +	case DYTC_MODE_QUIET:
> +		return sprintf(buf, "L\n");
> +	case DYTC_MODE_BALANCE:
> +		return sprintf(buf, "M\n");
> +	default:
> +		return sprintf(buf, "Unknown (%d)\n", perfmode);
> +	}
>   }
>   
> +static ssize_t dytc_perfmode_store(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	int err;
> +
> +	switch (buf[0]) {
> +	case 'l':
> +	case 'L':
> +		err = dytc_perfmode_set(DYTC_MODE_QUIET);
> +		break;
> +	case 'm':
> +	case 'M':
> +		err = dytc_perfmode_set(DYTC_MODE_BALANCE);
> +		break;
> +	case 'h':
> +	case 'H':
> +		err = dytc_perfmode_set(DYTC_MODE_PERFORM);
> +		break;
> +	default:
> +		err = -EINVAL;
> +		pr_err("Unknown operating mode. Ignoring\n");
> +		break;
> +	}
> +	if (err)
> +		return err;
> +
> +	tpacpi_disclose_usertask(attr->attr.name,
> +				"Performance mode set to %c\n", buf[0]);
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(dytc_perfmode);
> +
> +static struct attribute *dytc_perfmode_attributes[] = {
> +	&dev_attr_dytc_perfmode.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group dytc_perf_attr_group = {
> +	.attrs = dytc_perfmode_attributes
> +};
> +
>   /* sysfs lapmode entry */
>   static ssize_t dytc_lapmode_show(struct device *dev,
>   					struct device_attribute *attr,
>   					char *buf)
>   {
> -	return snprintf(buf, PAGE_SIZE, "%d\n", dytc_lapmode);
> +	return sprintf(buf, "%d\n", dytc_lapmode);
>   }
>   
>   static DEVICE_ATTR_RO(dytc_lapmode);
>   
> -static struct attribute *dytc_attributes[] = {
> +static struct attribute *dytc_lap_attributes[] = {
>   	&dev_attr_dytc_lapmode.attr,
> -	NULL,
> +	NULL
>   };
>   
> -static const struct attribute_group dytc_attr_group = {
> -	.attrs = dytc_attributes,
> +static const struct attribute_group dytc_lap_attr_group = {
> +	.attrs = dytc_lap_attributes
>   };
>   
>   static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
>   {
> -	int err;
> +	int err, output;
>   
> -	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
> +	err = dytc_command(DYTC_CMD_QUERY, &output);
> +	/*
> +	 * If support isn't available (ENODEV) then don't return an error
> +	 * just don't create the sysfs group
>   	 */
>   	if (err == -ENODEV)
>   		return 0;
> @@ -9900,14 +10091,38 @@ 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);
> +	/* 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;
> +			/* Platform supports this feature - create the group */
> +			err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &dytc_perf_attr_group);
> +			if (err)
> +				return err;
> +
> +			err = dytc_lapmode_get(&dytc_lapmode);
> +			if (err)
> +				return err;
> +			err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &dytc_lap_attr_group);
> +		}
> +	}
>   	return err;
>   }
>   
>   static void dytc_exit(void)
>   {
> -	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
> +	if (dytc_available) {
> +		sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_lap_attr_group);
> +		sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_perf_attr_group);
> +	}
>   }
>   
>   static struct ibm_struct dytc_driver_data = {
> @@ -10057,7 +10272,7 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
>   	}
>   
>   	if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED)
> -		dytc_lapmode_refresh();
> +		dytc_refresh();
>   
>   	if ((hkey_event == TP_HKEY_EV_PALM_DETECTED) ||
>   		(hkey_event == TP_HKEY_EV_PALM_UNDETECTED))
> 

Re: [External] Re: [PATCH v2] platform/x86: thinkpad_acpi: performance mode interface

[Mark Pearson]

Hi Hans,

On 9/17/2020 7:39 AM, Hans de Goede wrote:
> Hi,
> 
> On 8/21/20 7:53 PM, Mark Pearson wrote:
>> Lenovo Thinkpad platforms with DYTC version 5 and newer have enhanced
>> firmware to provide different performance/thermal modes.
>>
<Snip>
> 
> 
> So we recently had another driver show up with almost the same interface 
> / thing:
> 
> https://patchwork.kernel.org/patch/11774509/
> 
> So I believe that we really should come up with a standardize sysfs 
> interface
> for this under /sys/class/<some-name>
> 
> Please see the "RFC: offering a standardized (/sys/class) userspace API 
> for selecting
> system/laptop performance-profiles" mail thread which I just started.
> 
> Note I've not looked at the rest of this patch at all yet. But this 
> patch should
> not be merged until the userspace API question is settled so nack to 
> this patch
> for now (sorry).
> 
> Mark, once we have an agreement on what the userspace API for this should
> look like I can implement a small helper-library / class-core for 
> registering
> the class device for this, modify this patch to use that and test it on 
> a X1C8.
> 
> Or if you prefer you can do this yourself. Please let me know how you 
> want to
> proceed with this.
I'll join in that thread and depending on what is involved and the level 
of kernel expertise involved will contribute as much as I can. For sure 
will modify, integrate and test this implementation once that is available.

If there is a way I can 'tweak' this patch to expose the current 
performance mode to users in a way that is acceptable until the generic 
solution is available let me know and I'll do that. I'm just guessing 
that a generic solution needing input from multiple vendors could take a 
little while to develop - having a way to check what mode your system is 
in is really useful given you can change the mode using hotkeys.

Mark