Hi Luke,
I have a couple of small remarks once those are resolved in v5
then this should be ready for merging.
On 8/16/21 1:05 AM, Luke D. Jones wrote:
> Add initial support for platform_profile where the support is
> based on availability of ASUS_THROTTLE_THERMAL_POLICY.
>
> Because throttle_thermal_policy is used by platform_profile and is
> writeable separately to platform_profile any userspace changes to
> throttle_thermal_policy need to notify platform_profile.
>
> In future throttle_thermal_policy sysfs should be removed so that
> only one method controls the laptop power profile.
>
> Signed-off-by: Luke D. Jones <luke@xxxxxxxxxx>
> ---
> drivers/platform/x86/Kconfig | 1 +
> drivers/platform/x86/asus-wmi.c | 138 +++++++++++++++++++++++++++++++-
> 2 files changed, 135 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index d12db6c316ea..46dec48a36c1 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -281,6 +281,7 @@ config ASUS_WMI
> select INPUT_SPARSEKMAP
> select LEDS_CLASS
> select NEW_LEDS
> + select ACPI_PLATFORM_PROFILE
> help
> Say Y here if you have a WMI aware Asus laptop (like Eee PCs or new
> Asus Notebooks).
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 90a6a0d00deb..c11ce06073d1 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -11,6 +11,7 @@
> * Copyright (C) 2005 Dmitry Torokhov <dtor@xxxxxxx>
> */
>
> +#include <asm-generic/errno.h>
This is a weird place to add an include, also please use
linux/errno.h. But this is not necessary at all, because the
2 if-s using -ENODATA which need this can be dropped
(see below) so please just drop this all together.
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> #include <linux/kernel.h>
> @@ -26,6 +27,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 +221,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;
>
> @@ -2103,12 +2108,23 @@ static int throttle_thermal_policy_set_default(struct asus_wmi *asus)
> static int throttle_thermal_policy_switch_next(struct asus_wmi *asus)
> {
> u8 new_mode = asus->throttle_thermal_policy_mode + 1;
> + int err;
>
> if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
> new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
>
> asus->throttle_thermal_policy_mode = new_mode;
> - return throttle_thermal_policy_write(asus);
> + err = throttle_thermal_policy_write(asus);
> + if (err)
> + return err;
> +
> + /*
> + * Ensure that platform_profile updates userspace with the change to ensure
> + * that platform_profile and throttle_thermal_policy_mode are in sync.
> + */
> + platform_profile_notify();
> +
> + return 0;
> }
>
> static ssize_t throttle_thermal_policy_show(struct device *dev,
> @@ -2124,9 +2140,10 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - int result;
> - u8 new_mode;
> struct asus_wmi *asus = dev_get_drvdata(dev);
> + u8 new_mode;
> + int result;
> + int err;
>
> result = kstrtou8(buf, 10, &new_mode);
> if (result < 0)
> @@ -2136,7 +2153,15 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
> return -EINVAL;
>
> asus->throttle_thermal_policy_mode = new_mode;
> - throttle_thermal_policy_write(asus);
> + err = throttle_thermal_policy_write(asus);
> + if (err)
> + return err;
> +
> + /*
> + * Ensure that platform_profile updates userspace with the change to ensure
> + * that platform_profile and throttle_thermal_policy_mode are in sync.
> + */
> + platform_profile_notify();
>
> return count;
> }
> @@ -2144,6 +2169,101 @@ 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);
> +
> + if (!asus->throttle_thermal_policy_available) {
> + return -ENODATA;
> + }
Please don't use {} for an if before a single statement.
Also platform_profile_get() only gets used if platform_profile_setup()
proceeds beyond the same check which never happens, so this entire check is
not necessary, please just drop the entire check.
> + tp = asus->throttle_thermal_policy_mode;
> +
> + 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 */
This comment seems to be a left-over from when you considered also adding
fan-boost-mode, please drop.
> + if (!asus->throttle_thermal_policy_available) {
> + return -ENODATA;
> + }
As above, please drop.
> +
> + 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;
> + }
> +
> + asus->throttle_thermal_policy_mode = tp;
> + return throttle_thermal_policy_write(asus);
> +}
> +
> +static int platform_profile_setup(struct asus_wmi *asus)
> +{
> + int err;
> +
> + if (!asus->throttle_thermal_policy_available) {
> + /*
> + * Not an error if a component platform_profile relies on is unavailable
> + * so early return, skipping the setup of platform_profile.
> + */
> + return 0;
> + }
> + dev_info("Using throttle_thermal_policy for platform_profile support\n");
> +
> + 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 +3024,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 +3117,9 @@ 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:
> + if (asus->platform_profile_support)
> + platform_profile_remove();
> fail_fan_boost_mode:
> fail_egpu_enable:
> fail_dgpu_disable:
> @@ -3017,6 +3144,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
