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

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

[Elvis Stansvik]

(I forgot a bunch of recipients in my original reply. Now added.
Apologies to those of you who now get it twice)

See my reply below.

Cheers,
Elvis

Den sön 2 aug. 2020 kl 23:10 skrev Elvis Stansvik <elvstone@gmail.com>:
>
> (Apologies in advance for missing In-Reply-To header in this reply. I subscribed to the list just now.)
>
> Den fre 3 juli 2020 kl 01:24 skrev Mark Pearson <markpearson@lenovo.com>:
> > 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
>
> I tried the patch out on my X1C6 (20KH007BMX), running Lenovo firmware version 0.1.49.
>
> The sysfs path /sys/devices/platform/thinkpad_acpi/dytc_lapmode is showing up, but it's reporting 0 regardless if the laptop is in my lap or on a desk.
>
> I tested this by having the laptop in my lap for five minutes, and then on a desk for five minutes. The value reported through /sys/devices/platform/thinkpad_acpi/dytc_lapmode was always 0.
>
> Regards,
> Elvis
>
> >
> > Co-developed-by: Nitin Joshi <njoshi1@...>
> > Signed-off-by: Nitin Joshi <njoshi1@...>
> > Reviewed-by: Sugumaran <slacshiminar@...>
> > Reviewed-by: Bastien Nocera <bnocera@...>
> > Signed-off-by: Mark Pearson <markpearson@...>
> > ---
> > 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


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

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

[Mark Pearson]

Hi Elvis,

On 8/3/2020 9:20 AM, Elvis Stansvik wrote:
> 
> Den sön 2 aug. 2020 kl 23:10 skrev Elvis Stansvik 
> <elvstone@gmail.com>:
>> 
>> (Apologies in advance for missing In-Reply-To header in this
>> reply. I subscribed to the list just now.)
>> 
>> Den fre 3 juli 2020 kl 01:24 skrev Mark Pearson 
>> <markpearson@lenovo.com>:
>>> 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
>> 
>> I tried the patch out on my X1C6 (20KH007BMX), running Lenovo 
>> firmware version 0.1.49.
>> 
>> The sysfs path /sys/devices/platform/thinkpad_acpi/dytc_lapmode is 
>> showing up, but it's reporting 0 regardless if the laptop is in my 
>> lap or on a desk.
>> 
>> I tested this by having the laptop in my lap for five minutes, and 
>> then on a desk for five minutes. The value reported through 
>> /sys/devices/platform/thinkpad_acpi/dytc_lapmode was always 0.
>> 
Thanks for the update.
The X1C6 doesn't have support for the new thermal mode implementation so 
I'm wondering if the lapmode being available could be related to the 
version of DYTC is available. I'll check with the firmware team and get 
back to you on that - they hadn't listed it to me as a limitation 
previously and unfortunately I don't have an X1C6 so didn't test on that 
platform.

As a note - the lapmode is primarily for the thermal modes (it is also 
being used for WWAN power control but that is not supported for Linux on 
the X1C6 either for other reasons). I'm afraid the interface is of 
limited use to you anyway....

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 v5] platform/x86: thinkpad_acpi: lap or desk mode interface

[Elvis Stansvik]

Den mån 3 aug. 2020 kl 16:27 skrev Mark Pearson <markpearson@lenovo.com>:
>
> Hi Elvis,
>
> On 8/3/2020 9:20 AM, Elvis Stansvik wrote:
> >
> > Den sön 2 aug. 2020 kl 23:10 skrev Elvis Stansvik
> > <elvstone@gmail.com>:
> >>
> >> (Apologies in advance for missing In-Reply-To header in this
> >> reply. I subscribed to the list just now.)
> >>
> >> Den fre 3 juli 2020 kl 01:24 skrev Mark Pearson
> >> <markpearson@lenovo.com>:
> >>> 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
> >>
> >> I tried the patch out on my X1C6 (20KH007BMX), running Lenovo
> >> firmware version 0.1.49.
> >>
> >> The sysfs path /sys/devices/platform/thinkpad_acpi/dytc_lapmode is
> >> showing up, but it's reporting 0 regardless if the laptop is in my
> >> lap or on a desk.
> >>
> >> I tested this by having the laptop in my lap for five minutes, and
> >> then on a desk for five minutes. The value reported through
> >> /sys/devices/platform/thinkpad_acpi/dytc_lapmode was always 0.
> >>
> Thanks for the update.
> The X1C6 doesn't have support for the new thermal mode implementation so
> I'm wondering if the lapmode being available could be related to the
> version of DYTC is available. I'll check with the firmware team and get
> back to you on that - they hadn't listed it to me as a limitation
> previously and unfortunately I don't have an X1C6 so didn't test on that
> platform.
>
> As a note - the lapmode is primarily for the thermal modes (it is also
> being used for WWAN power control but that is not supported for Linux on
> the X1C6 either for other reasons). I'm afraid the interface is of
> limited use to you anyway....

Thanks Mark. I know the X1C6 firmware did not get the new thermal mode
implementation, but thought that I would still be able to query its
lap mode.

The reason I wanted query the lap mode is actually because I'm trying
to help Matthew Garrett out in adding support for the adaptive DPTF
performance policy on the thermald side, instead of on the firmware
side like Lenovo decided to do for more recent models. See that effort
here https://github.com/mjg59/thermal_daemon/issues/7). We were
surprised to find that all of the OEM conditions in the APCT table
remained unchanged when the laptop was taken between lap and desk. So
I started suspecting that perhaps the X1C6 and its firmware simply
does not have the ability to detect lap vs desk. That's why I wanted
to use this patch to query the lap mode, as a debugging aid.

If X1C6 does not have the ability to detect lap mode, then that would
obviously explain why the firmware does not change any APCT condition
when put on/off lap, and any effort to get adaptive performance policy
working on the thermald side is probably doomed :(

Elvis

>
> 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 v5] platform/x86: thinkpad_acpi: lap or desk mode interface

[Mark Pearson]

On 8/3/2020 10:46 AM, Elvis Stansvik wrote:

> 
> Thanks Mark. I know the X1C6 firmware did not get the new thermal
> mode implementation, but thought that I would still be able to query
> its lap mode.
> 
> The reason I wanted query the lap mode is actually because I'm
> trying to help Matthew Garrett out in adding support for the adaptive
> DPTF performance policy on the thermald side, instead of on the
> firmware side like Lenovo decided to do for more recent models. See
> that effort here https://github.com/mjg59/thermal_daemon/issues/7).
> We were surprised to find that all of the OEM conditions in the APCT
> table remained unchanged when the laptop was taken between lap and
> desk. So I started suspecting that perhaps the X1C6 and its firmware
> simply does not have the ability to detect lap vs desk. That's why I
> wanted to use this patch to query the lap mode, as a debugging aid.
> 
> If X1C6 does not have the ability to detect lap mode, then that
> would obviously explain why the firmware does not change any APCT
> condition when put on/off lap, and any effort to get adaptive
> performance policy working on the thermald side is probably doomed
> :(
> 
Just as a follow-up for this mailing list - I responded to the X1C6 
issues on the thermal_daemon thread mentioned above - I will track those 
there.

Anything related to the thinkpad_acpi driver and improvements needed to 
my previous patch will come here. I have a question in to the firmware 
team to understand the limitations of when the lapmode variable should 
be made available or not. Looks like I have some work to do there....

Thanks!
Mark

[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