| Message ID | 20210814043103.2535842-2-luke@ljones.dev (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | asus-wmi: add platform_profile support | expand |
On Sat, Aug 14, 2021 at 7:33 AM Luke D. Jones <luke@ljones.dev> 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. Some comments below. ... > + /* > + * Ensure that platform_profile updates userspace with the change to ensure > + * that platform_profile and throttle_thermal_policy_mode are in sync Missed period here and in other multi-line comments. > + */ ... > + /* 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; This will be better in a form if (!..._available) return -ENODATA; // what -1 means? tp = ...; if (tp < 0) return tp; ... > + /* All possible toggles like throttle_thermal_policy here */ > + if (!asus->throttle_thermal_policy_available) { > + return -1; See above. > + } ... > + if (asus->throttle_thermal_policy_available) { > + pr_info("Using throttle_thermal_policy for platform_profile support\n"); Why pr_*()? > + } else { > + /* > + * Not an error if a component platform_profile relies on is unavailable > + * so early return, skipping the setup of platform_profile. > + */ > + return 0; Do it other way around, /* * Comment */ if (!...) return 0; > + }
Hi Andy, thanks for the feedback. All is addressed, and some inline comment/reply. New version incoming pending pr_info() vs dev_info() On Sat, Aug 14 2021 at 12:40:39 +0300, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sat, Aug 14, 2021 at 7:33 AM Luke D. Jones <luke@ljones.dev> 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. > > Some comments below. > > ... > >> + /* >> + * Ensure that platform_profile updates userspace with the >> change to ensure >> + * that platform_profile and throttle_thermal_policy_mode >> are in sync > > Missed period here and in other multi-line comments. > >> + */ > > ... > >> + /* 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; > > This will be better in a form > > if (!..._available) > return -ENODATA; // what -1 means? > I wasn't sure what the best return was here. On your suggestion I've changed to ENODATA > tp = ...; > if (tp < 0) > return tp; > > ... > >> + /* All possible toggles like throttle_thermal_policy here */ >> + if (!asus->throttle_thermal_policy_available) { >> + return -1; > > See above. > >> + } > > ... > >> + if (asus->throttle_thermal_policy_available) { >> + pr_info("Using throttle_thermal_policy for >> platform_profile support\n"); > > Why pr_*()? That seemed to be the convention? I see there is also dev_info(), so I've switched to that as it seems more appropriate. > >> + } else { >> + /* >> + * Not an error if a component platform_profile >> relies on is unavailable >> + * so early return, skipping the setup of >> platform_profile. >> + */ >> + return 0; > > Do it other way around, > > /* > * Comment > */ > if (!...) > return 0; Thanks, I think I was transliterating thought process to code as I went along. All updated now. > >> + } > > > -- > With Best Regards, > Andy Shevchenko
A very quick question: should I be enabling platform_profile by default if asus_wmi is enabled in kconfig? On Sat, Aug 14 2021 at 23:46:06 +1200, Luke Jones <luke@ljones.dev> wrote: > Hi Andy, thanks for the feedback. All is addressed, and some inline > comment/reply. New version incoming pending pr_info() vs dev_info() > > On Sat, Aug 14 2021 at 12:40:39 +0300, Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> On Sat, Aug 14, 2021 at 7:33 AM Luke D. Jones <luke@ljones.dev> >> 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. >> >> Some comments below. >> >> ... >> >>> + /* >>> + * Ensure that platform_profile updates userspace with the >>> change to ensure >>> + * that platform_profile and throttle_thermal_policy_mode >>> are in sync >> >> Missed period here and in other multi-line comments. >> >>> + */ >> >> ... >> >>> + /* 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; >> >> This will be better in a form >> >> if (!..._available) >> return -ENODATA; // what -1 means? >> > > I wasn't sure what the best return was here. On your suggestion I've > changed to ENODATA > >> tp = ...; >> if (tp < 0) >> return tp; >> >> ... >> >>> + /* All possible toggles like throttle_thermal_policy here >>> */ >>> + if (!asus->throttle_thermal_policy_available) { >>> + return -1; >> >> See above. >> >>> + } >> >> ... >> >>> + if (asus->throttle_thermal_policy_available) { >>> + pr_info("Using throttle_thermal_policy for >>> platform_profile support\n"); >> >> Why pr_*()? > > That seemed to be the convention? I see there is also dev_info(), so > I've switched to that as it seems more appropriate. > >> >>> + } else { >>> + /* >>> + * Not an error if a component platform_profile >>> relies on is unavailable >>> + * so early return, skipping the setup of >>> platform_profile. >>> + */ >>> + return 0; >> >> Do it other way around, >> >> /* >> * Comment >> */ >> if (!...) >> return 0; > > Thanks, I think I was transliterating thought process to code as I > went along. > All updated now. > >> >>> + } >> >> >> -- >> With Best Regards, >> Andy Shevchenko >
On Sat, Aug 14, 2021 at 2:46 PM Luke Jones <luke@ljones.dev> wrote: > On Sat, Aug 14 2021 at 12:40:39 +0300, Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Sat, Aug 14, 2021 at 7:33 AM Luke D. Jones <luke@ljones.dev> wrote: ... > >> + pr_info("Using throttle_thermal_policy for > >> platform_profile support\n"); > > > > Why pr_*()? > > That seemed to be the convention? I see there is also dev_info(), so > I've switched to that as it seems more appropriate. The rule of thumb is if you have the device the message belongs to, use dev_*(), if it's really stuff before we know anything about devices, use pr_*(). In some cases if you have ACPI handle, you may use acpi_handle_*().
On Sat, Aug 14, 2021 at 3:47 PM Luke Jones <luke@ljones.dev> wrote: > A very quick question: should I be enabling platform_profile by default > if asus_wmi is enabled in kconfig? Very quick for you, but me. I dunno. I think the best thing is what Hans can tell here. Nevertheless, I think that this kind of default should be applied to all modules (yes, I know there might be possible exceptions).
Hi, On 8/14/21 2:47 PM, Luke Jones wrote: > A very quick question: should I be enabling platform_profile by default if asus_wmi is enabled in kconfig? You should add a "select ACPI_PLATFORM_PROFILE" to the Kconfig part for ASUS_WMI, the PLATFORM_PROFILE support / common code is a library, so it needs to be selected by the drivers which need it. Regards, Hans > > On Sat, Aug 14 2021 at 23:46:06 +1200, Luke Jones <luke@ljones.dev> wrote: >> Hi Andy, thanks for the feedback. All is addressed, and some inline comment/reply. New version incoming pending pr_info() vs dev_info() >> >> On Sat, Aug 14 2021 at 12:40:39 +0300, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >>> On Sat, Aug 14, 2021 at 7:33 AM Luke D. Jones <luke@ljones.dev> 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. >>> >>> Some comments below. >>> >>> ... >>> >>>> + /* >>>> + * Ensure that platform_profile updates userspace with the change to ensure >>>> + * that platform_profile and throttle_thermal_policy_mode are in sync >>> >>> Missed period here and in other multi-line comments. >>> >>>> + */ >>> >>> ... >>> >>>> + /* 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; >>> >>> This will be better in a form >>> >>> if (!..._available) >>> return -ENODATA; // what -1 means? >>> >> >> I wasn't sure what the best return was here. On your suggestion I've changed to ENODATA >> >>> tp = ...; >>> if (tp < 0) >>> return tp; >>> >>> ... >>> >>>> + /* All possible toggles like throttle_thermal_policy here */ >>>> + if (!asus->throttle_thermal_policy_available) { >>>> + return -1; >>> >>> See above. >>> >>>> + } >>> >>> ... >>> >>>> + if (asus->throttle_thermal_policy_available) { >>>> + pr_info("Using throttle_thermal_policy for platform_profile support\n"); >>> >>> Why pr_*()? >> >> That seemed to be the convention? I see there is also dev_info(), so I've switched to that as it seems more appropriate. >> >>> >>>> + } else { >>>> + /* >>>> + * Not an error if a component platform_profile relies on is unavailable >>>> + * so early return, skipping the setup of platform_profile. >>>> + */ >>>> + return 0; >>> >>> Do it other way around, >>> >>> /* >>> * Comment >>> */ >>> if (!...) >>> return 0; >> >> Thanks, I think I was transliterating thought process to code as I went along. >> All updated now. >> >>> >>>> + } >>> >>> >>> -- >>> With Best Regards, >>> Andy Shevchenko >> > >
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c index 90a6a0d00deb..909fc2663008 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; @@ -2103,12 +2107,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 +2139,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 +2152,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 +2168,103 @@ 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; + } + + 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) { + 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 +3025,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 +3118,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 +3145,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; }
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@ljones.dev> --- drivers/platform/x86/asus-wmi.c | 139 +++++++++++++++++++++++++++++++- 1 file changed, 135 insertions(+), 4 deletions(-)