Hi Luke,
On 8/16/21 10:32 AM, Hans de Goede wrote:
> Hi Luke,
>
> I have a couple of small remarks once those are resolved in v5
> then this should be ready for merging.
I'm trying to get the pdx86 in order for the upcoming merge-window
and it would be good if we can get this patch included.
Since my suggested changes come down to just dropping 2 unnecessary
"if (!asus->throttle_thermal_policy_available) {...}"
checks, a v5 is not really necessary. I can also just merge v4 and
drop these lines myself when merging, then you don't need to do a v5.
Are you ok with me dropping these extra/unnecessary checks and merging
v4 with that change?
Regards,
Hans
>> + return -ENODATA;
>
> 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
>
