Re: [PATCH] asus-wmi: Add support for platform_profile
From: Luke Jones
Date: Fri Aug 13 2021 - 03:20:53 EST
On Fri, Aug 13 2021 at 08:47:56 +0200, Hans de Goede
<hdegoede@xxxxxxxxxx> wrote:
Hi Luke,
Thank you for this patch, it is great to see more and more laptop
models getting support for the standard platform_profile userspace
API.
On 8/13/21 4:42 AM, Luke D. Jones wrote:
Add initial support for platform_profile where the support is
based on availability of ASUS_THROTTLE_THERMAL_POLICY.
Signed-off-by: Luke D. Jones <luke@xxxxxxxxxx>
---
drivers/platform/x86/asus-wmi.c | 112
++++++++++++++++++++++++++++++++
1 file changed, 112 insertions(+)
diff --git a/drivers/platform/x86/asus-wmi.c
b/drivers/platform/x86/asus-wmi.c
index 90a6a0d00deb..62260043c15c 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -26,6 +26,7 @@
#include <linux/rfkill.h>
#include <linux/pci.h>
#include <linux/pci_hotplug.h>
+#include <linux/platform_profile.h>
#include <linux/power_supply.h>
#include <linux/hwmon.h>
#include <linux/hwmon-sysfs.h>
@@ -219,6 +220,9 @@ struct asus_wmi {
bool throttle_thermal_policy_available;
u8 throttle_thermal_policy_mode;
+ struct platform_profile_handler platform_profile_handler;
+ bool platform_profile_support;
+
// The RSOC controls the maximum charging percentage.
bool battery_rsoc_available;
@@ -2144,6 +2148,106 @@ static ssize_t
throttle_thermal_policy_store(struct device *dev,
// Throttle thermal policy: 0 - default, 1 - overboost, 2 - silent
static DEVICE_ATTR_RW(throttle_thermal_policy);
+/* Platform profile
***********************************************************/
+static int platform_profile_get(struct platform_profile_handler
*pprof,
+ enum platform_profile_option *profile)
+{
+ struct asus_wmi *asus;
+ int tp;
+
+ asus = container_of(pprof, struct asus_wmi,
platform_profile_handler);
+
+ /* All possible toggles like throttle_thermal_policy here */
+ if (asus->throttle_thermal_policy_available) {
+ tp = asus->throttle_thermal_policy_mode;
+ } else {
+ return -1;
+ }
+
+ if (tp < 0)
+ return tp;
+
+ switch (tp) {
+ case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
+ *profile = PLATFORM_PROFILE_BALANCED;
+ break;
+ case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
+ *profile = PLATFORM_PROFILE_PERFORMANCE;
+ break;
+ case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
+ *profile = PLATFORM_PROFILE_QUIET;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int platform_profile_set(struct platform_profile_handler
*pprof,
+ enum platform_profile_option profile)
+{
+ struct asus_wmi *asus;
+ int tp;
+
+ asus = container_of(pprof, struct asus_wmi,
platform_profile_handler);
+
+ /* All possible toggles like throttle_thermal_policy here */
+ if (!asus->throttle_thermal_policy_available) {
+ return -1;
+ }
+
+ switch (profile) {
+ case PLATFORM_PROFILE_PERFORMANCE:
+ tp = ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST;
+ break;
+ case PLATFORM_PROFILE_BALANCED:
+ tp = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
+ break;
+ case PLATFORM_PROFILE_QUIET:
+ tp = ASUS_THROTTLE_THERMAL_POLICY_SILENT;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ if (asus->throttle_thermal_policy_available) {
You already check for asus->throttle_thermal_policy_available a couple
of lines higher in this function and you return when it is not
available ...
I guess this was intended to also have a second if () for
fan-boost mode ?
Yes, sorry I missed that. I'd initially looked at adding fan-boost also
but as it is *fans only* I didn't see the value in adding it. I've also
not seen any use of this in the wild bar one laptop which was likely to
be retired many months ago.
Since you are not adding support for fan-boost mode at this time,
it would be best (IMHO) to drop this second if for now and
just take this path unconditionally.
Adjusted now.
+ asus->throttle_thermal_policy_mode = tp;
+ return throttle_thermal_policy_write(asus);
+ }
+ return 0;
+}
+
+static int platform_profile_setup(struct asus_wmi *asus)
+{
+ int err;
+
+ if (asus->throttle_thermal_policy_available) {
+ pr_info("Using throttle_thermal_policy for platform_profile
support\n");
+ } else {
+ /*
+ * Not an error if a component platform_profile relies on is
unavailable
+ * so early return, skipping the setup of platform_profile.
+ */
+ return 0;
+ }
+ asus->platform_profile_handler.profile_get = platform_profile_get;
+ asus->platform_profile_handler.profile_set = platform_profile_set;
+
+ set_bit(PLATFORM_PROFILE_QUIET,
asus->platform_profile_handler.choices);
+ set_bit(PLATFORM_PROFILE_BALANCED,
+ asus->platform_profile_handler.choices);
+ set_bit(PLATFORM_PROFILE_PERFORMANCE,
+ asus->platform_profile_handler.choices);
+
+ err = platform_profile_register(&asus->platform_profile_handler);
+ if (err)
+ return err;
+
+ asus->platform_profile_support = true;
+ return 0;
+}
+
/* Backlight
******************************************************************/
static int read_backlight_power(struct asus_wmi *asus)
@@ -2904,6 +3008,10 @@ static int asus_wmi_add(struct
platform_device *pdev)
else
throttle_thermal_policy_set_default(asus);
+ err = platform_profile_setup(asus);
+ if (err)
+ goto fail_platform_profile_setup;
+
err = panel_od_check_present(asus);
if (err)
goto fail_panel_od;
@@ -2993,6 +3101,7 @@ static int asus_wmi_add(struct
platform_device *pdev)
asus_wmi_sysfs_exit(asus->platform_device);
fail_sysfs:
fail_throttle_thermal_policy:
+fail_platform_profile_setup:
fail_fan_boost_mode:
I think you need to add this:
if (asus->platform_profile_support)
platform_profile_remove();
here so that if later fail-exit paths are taken the platform_profile
support gets unregistered again.
Good catch, thanks! :)
Next version will come after some testing.
fail_egpu_enable:
fail_dgpu_disable:
@@ -3017,6 +3126,9 @@ static int asus_wmi_remove(struct
platform_device *device)
asus_fan_set_auto(asus);
asus_wmi_battery_exit(asus);
+ if (asus->platform_profile_support)
+ platform_profile_remove();
+
kfree(asus);
return 0;
}
Regards,
Hans