When changing the thermal policy using the platform profile API,
a Vivobook thermal policy is stored in throttle_thermal_policy_mode.
However everywhere else a normal thermal policy is stored inside this
variable, potentially confusing the platform profile.
Fix this by always storing normal thermal policy values inside
throttle_thermal_policy_mode and only do the conversion when writing
the thermal policy to hardware.
Fixes: bcbfcebda2cb ("platform/x86: asus-wmi: add support for vivobook fan profiles")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/platform/x86/asus-wmi.c | 64 +++++++++++----------------------
1 file changed, 21 insertions(+), 43 deletions(-)
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index ab9342a01a48..ce60835d0303 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -3696,10 +3696,28 @@ static int asus_wmi_custom_fan_curve_init(struct asus_wmi *asus)
/* Throttle thermal policy ****************************************************/
static int throttle_thermal_policy_write(struct asus_wmi *asus)
{
- u8 value = asus->throttle_thermal_policy_mode;
u32 retval;
+ u8 value;
int err;
+ if (asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO) {
+ switch (asus->throttle_thermal_policy_mode) {
+ case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
+ value = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO;
+ break;
+ case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
+ value = ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO;
+ break;
+ case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
+ value = ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO;
+ break;
+ default:
+ return -EINVAL;
+ }
+ } else {
+ value = asus->throttle_thermal_policy_mode;
+ }
+
err = asus_wmi_set_devstate(asus->throttle_thermal_policy_dev,
value, &retval);
@@ -3804,46 +3822,6 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
static DEVICE_ATTR_RW(throttle_thermal_policy);
/* Platform profile ***********************************************************/
-static int asus_wmi_platform_profile_to_vivo(struct asus_wmi *asus, int mode)
-{
- bool vivo;
-
- vivo = asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO;
-
- if (vivo) {
- switch (mode) {
- case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
- return ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO;
- case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
- return ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO;
- case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
- return ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO;
- }
- }
-
- return mode;
-}
-
-static int asus_wmi_platform_profile_mode_from_vivo(struct asus_wmi *asus, int mode)
-{
- bool vivo;
-
- vivo = asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO;
-
- if (vivo) {
- switch (mode) {
- case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO:
- return ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
- case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO:
- return ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST;
- case ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO:
- return ASUS_THROTTLE_THERMAL_POLICY_SILENT;
- }
- }
-
- return mode;
-}
-
static int asus_wmi_platform_profile_get(struct platform_profile_handler *pprof,
enum platform_profile_option *profile)
{
@@ -3853,7 +3831,7 @@ static int asus_wmi_platform_profile_get(struct platform_profile_handler *pprof,
asus = container_of(pprof, struct asus_wmi, platform_profile_handler);
tp = asus->throttle_thermal_policy_mode;
- switch (asus_wmi_platform_profile_mode_from_vivo(asus, tp)) {
+ switch (tp) {
case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
*profile = PLATFORM_PROFILE_BALANCED;
break;
@@ -3892,7 +3870,7 @@ static int asus_wmi_platform_profile_set(struct platform_profile_handler *pprof,
return -EOPNOTSUPP;
}
- asus->throttle_thermal_policy_mode = asus_wmi_platform_profile_to_vivo(asus, tp);
+ asus->throttle_thermal_policy_mode = tp;
return throttle_thermal_policy_write(asus);
}
--
2.39.5On Fri, Oct 25, 2024 at 09:15:14PM +0200, Armin Wolf wrote:
> When changing the thermal policy using the platform profile API,
> a Vivobook thermal policy is stored in throttle_thermal_policy_mode.
>
> However everywhere else a normal thermal policy is stored inside this
> variable, potentially confusing the platform profile.
>
> Fix this by always storing normal thermal policy values inside
> throttle_thermal_policy_mode and only do the conversion when writing
> the thermal policy to hardware.
>
> Fixes: bcbfcebda2cb ("platform/x86: asus-wmi: add support for vivobook fan profiles")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
> drivers/platform/x86/asus-wmi.c | 64 +++++++++++----------------------
> 1 file changed, 21 insertions(+), 43 deletions(-)
the original patch that i submitted did actually have the remapping
of the different fan profiles in the throttle_thermal_policy_write() methods
because it was the cleaner solution [1]. however after having a discussion with luke,
he shared that he might be planning to remove the throttle_thermal_policy sysfs interface
in favour of platform_profiles [2] because of a refactoring he had been working on.
currently to control fan profiles through this driver you could use
either /sys/devices/platform/asus-nb-wmi/throttle_thermal_policy
(redundant and might get removed in the future) or through platform profiles which is the
better way of doing things.
for the reasons mentionned above, I decided to keep
throttle_therma_policy_write() unchanged and to move the remapping logic
to the asus_wmi_platform_profile_set(). this adopts the approach of
having a logical mapping stored in asus_wmi struct that has to be
converted to a physical mapping whenever needed [3].
so, if luke thinks that this won't cause any merge conflicts with his
work [4] then i see no problem with this approach even though it might cause an
order change when calling throttle_thermal_policy_switch_next()
Best Regards,
Mohamed G.
Link: https://lore.kernel.org/platform-driver-x86/20240421194320.48258-2-mohamed.ghanmi@supcom.tn/ # [1]
Link: https://lore.kernel.org/platform-driver-x86/4de768c5-aae5-4fda-a139-a8b73c8495a1@app.fastmail.com/ # [2]
Link: https://lore.kernel.org/platform-driver-x86/ZnlEuiP4Dgqpf51C@laptop/ # [3]
Link: https://lore.kernel.org/platform-driver-x86/20240930000046.51388-1-luke@ljones.dev/ # [4]
Am 26.10.24 um 12:45 schrieb Mohamed Ghanmi:
> On Fri, Oct 25, 2024 at 09:15:14PM +0200, Armin Wolf wrote:
>> When changing the thermal policy using the platform profile API,
>> a Vivobook thermal policy is stored in throttle_thermal_policy_mode.
>>
>> However everywhere else a normal thermal policy is stored inside this
>> variable, potentially confusing the platform profile.
>>
>> Fix this by always storing normal thermal policy values inside
>> throttle_thermal_policy_mode and only do the conversion when writing
>> the thermal policy to hardware.
>>
>> Fixes: bcbfcebda2cb ("platform/x86: asus-wmi: add support for vivobook fan profiles")
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>> drivers/platform/x86/asus-wmi.c | 64 +++++++++++----------------------
>> 1 file changed, 21 insertions(+), 43 deletions(-)
> the original patch that i submitted did actually have the remapping
> of the different fan profiles in the throttle_thermal_policy_write() methods
> because it was the cleaner solution [1]. however after having a discussion with luke,
> he shared that he might be planning to remove the throttle_thermal_policy sysfs interface
> in favour of platform_profiles [2] because of a refactoring he had been working on.
>
> currently to control fan profiles through this driver you could use
> either /sys/devices/platform/asus-nb-wmi/throttle_thermal_policy
> (redundant and might get removed in the future) or through platform profiles which is the
> better way of doing things.
>
> for the reasons mentionned above, I decided to keep
> throttle_therma_policy_write() unchanged and to move the remapping logic
> to the asus_wmi_platform_profile_set(). this adopts the approach of
> having a logical mapping stored in asus_wmi struct that has to be
> converted to a physical mapping whenever needed [3].
>
> so, if luke thinks that this won't cause any merge conflicts with his
> work [4] then i see no problem with this approach even though it might cause an
> order change when calling throttle_thermal_policy_switch_next()
Ok, i will wait for Luke to give feedback on this series. In my opinion the order change is
ok since users likely expect the "old" Asus switching order.
>
> Best Regards,
> Mohamed G.
>
> Link: https://lore.kernel.org/platform-driver-x86/20240421194320.48258-2-mohamed.ghanmi@supcom.tn/ # [1]
> Link: https://lore.kernel.org/platform-driver-x86/4de768c5-aae5-4fda-a139-a8b73c8495a1@app.fastmail.com/ # [2]
> Link: https://lore.kernel.org/platform-driver-x86/ZnlEuiP4Dgqpf51C@laptop/ # [3]
> Link: https://lore.kernel.org/platform-driver-x86/20240930000046.51388-1-luke@ljones.dev/ # [4]
>
Hi,
On 26-Oct-24 12:45 PM, Mohamed Ghanmi wrote:
> On Fri, Oct 25, 2024 at 09:15:14PM +0200, Armin Wolf wrote:
>> When changing the thermal policy using the platform profile API,
>> a Vivobook thermal policy is stored in throttle_thermal_policy_mode.
>>
>> However everywhere else a normal thermal policy is stored inside this
>> variable, potentially confusing the platform profile.
>>
>> Fix this by always storing normal thermal policy values inside
>> throttle_thermal_policy_mode and only do the conversion when writing
>> the thermal policy to hardware.
>>
>> Fixes: bcbfcebda2cb ("platform/x86: asus-wmi: add support for vivobook fan profiles")
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>> drivers/platform/x86/asus-wmi.c | 64 +++++++++++----------------------
>> 1 file changed, 21 insertions(+), 43 deletions(-)
>
> the original patch that i submitted did actually have the remapping
> of the different fan profiles in the throttle_thermal_policy_write() methods
> because it was the cleaner solution [1]. however after having a discussion with luke,
> he shared that he might be planning to remove the throttle_thermal_policy sysfs interface
> in favour of platform_profiles [2] because of a refactoring he had been working on.
>
> currently to control fan profiles through this driver you could use
> either /sys/devices/platform/asus-nb-wmi/throttle_thermal_policy
> (redundant and might get removed in the future) or through platform profiles which is the
> better way of doing things.
>
> for the reasons mentionned above, I decided to keep
> throttle_therma_policy_write() unchanged and to move the remapping logic
> to the asus_wmi_platform_profile_set(). this adopts the approach of
> having a logical mapping stored in asus_wmi struct that has to be
> converted to a physical mapping whenever needed [3].
>
> so, if luke thinks that this won't cause any merge conflicts with his
> work [4] then i see no problem with this approach even though it might cause an
> order change when calling throttle_thermal_policy_switch_next()
Talking about throttle_thermal_policy_switch_next() we also
have platform_profile_cycle() and since asus-wmi supports
platform-profiles now I'm wondering if it would not be better
to simply completely drop throttle_thermal_policy_switch_next()
and call platform_profile_cycle() instead?
This will also keep the cycle order the same for "normal" vs
vivo even after Armin's patch.
Anyways I'll go and apply patch 1/2 to pdx86/fixes since that one is
obviously correct and fixes th Lunar Lake performance issues.
And we can keep discussing what to do wrt 2/2 and maybe also drop
throttle_thermal_policy_switch_next() if favor of
platform_profile_cycle().
Regards,
Hans
>
> Best Regards,
> Mohamed G.
>
> Link: https://lore.kernel.org/platform-driver-x86/20240421194320.48258-2-mohamed.ghanmi@supcom.tn/ # [1]
> Link: https://lore.kernel.org/platform-driver-x86/4de768c5-aae5-4fda-a139-a8b73c8495a1@app.fastmail.com/ # [2]
> Link: https://lore.kernel.org/platform-driver-x86/ZnlEuiP4Dgqpf51C@laptop/ # [3]
> Link: https://lore.kernel.org/platform-driver-x86/20240930000046.51388-1-luke@ljones.dev/ # [4]
>
Am 26.10.24 um 12:59 schrieb Hans de Goede:
> Hi,
>
> On 26-Oct-24 12:45 PM, Mohamed Ghanmi wrote:
>> On Fri, Oct 25, 2024 at 09:15:14PM +0200, Armin Wolf wrote:
>>> When changing the thermal policy using the platform profile API,
>>> a Vivobook thermal policy is stored in throttle_thermal_policy_mode.
>>>
>>> However everywhere else a normal thermal policy is stored inside this
>>> variable, potentially confusing the platform profile.
>>>
>>> Fix this by always storing normal thermal policy values inside
>>> throttle_thermal_policy_mode and only do the conversion when writing
>>> the thermal policy to hardware.
>>>
>>> Fixes: bcbfcebda2cb ("platform/x86: asus-wmi: add support for vivobook fan profiles")
>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>> ---
>>> drivers/platform/x86/asus-wmi.c | 64 +++++++++++----------------------
>>> 1 file changed, 21 insertions(+), 43 deletions(-)
>> the original patch that i submitted did actually have the remapping
>> of the different fan profiles in the throttle_thermal_policy_write() methods
>> because it was the cleaner solution [1]. however after having a discussion with luke,
>> he shared that he might be planning to remove the throttle_thermal_policy sysfs interface
>> in favour of platform_profiles [2] because of a refactoring he had been working on.
>>
>> currently to control fan profiles through this driver you could use
>> either /sys/devices/platform/asus-nb-wmi/throttle_thermal_policy
>> (redundant and might get removed in the future) or through platform profiles which is the
>> better way of doing things.
>>
>> for the reasons mentionned above, I decided to keep
>> throttle_therma_policy_write() unchanged and to move the remapping logic
>> to the asus_wmi_platform_profile_set(). this adopts the approach of
>> having a logical mapping stored in asus_wmi struct that has to be
>> converted to a physical mapping whenever needed [3].
>>
>> so, if luke thinks that this won't cause any merge conflicts with his
>> work [4] then i see no problem with this approach even though it might cause an
>> order change when calling throttle_thermal_policy_switch_next()
> Talking about throttle_thermal_policy_switch_next() we also
> have platform_profile_cycle() and since asus-wmi supports
> platform-profiles now I'm wondering if it would not be better
> to simply completely drop throttle_thermal_policy_switch_next()
> and call platform_profile_cycle() instead?
>
> This will also keep the cycle order the same for "normal" vs
> vivo even after Armin's patch.
>
> Anyways I'll go and apply patch 1/2 to pdx86/fixes since that one is
> obviously correct and fixes th Lunar Lake performance issues.
>
> And we can keep discussing what to do wrt 2/2 and maybe also drop
> throttle_thermal_policy_switch_next() if favor of
> platform_profile_cycle().
>
> Regards,
>
> Hans
Good idea, using platform_profile_cycle() would also solve a potential locking issue here.
Thanks,
Armin Wolf
>> Best Regards,
>> Mohamed G.
>>
>> Link: https://lore.kernel.org/platform-driver-x86/20240421194320.48258-2-mohamed.ghanmi@supcom.tn/ # [1]
>> Link: https://lore.kernel.org/platform-driver-x86/4de768c5-aae5-4fda-a139-a8b73c8495a1@app.fastmail.com/ # [2]
>> Link: https://lore.kernel.org/platform-driver-x86/ZnlEuiP4Dgqpf51C@laptop/ # [3]
>> Link: https://lore.kernel.org/platform-driver-x86/20240930000046.51388-1-luke@ljones.dev/ # [4]
>>
>
Hi Armin,
On 25-Oct-24 9:15 PM, Armin Wolf wrote:
> When changing the thermal policy using the platform profile API,
> a Vivobook thermal policy is stored in throttle_thermal_policy_mode.
>
> However everywhere else a normal thermal policy is stored inside this
> variable, potentially confusing the platform profile.
You say "potentially confusing the platform profile", but did you
spot any actual issues when reviewing the code ?
> Fix this by always storing normal thermal policy values inside
> throttle_thermal_policy_mode and only do the conversion when writing
> the thermal policy to hardware.
>
> Fixes: bcbfcebda2cb ("platform/x86: asus-wmi: add support for vivobook fan profiles")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
The problem with this approach is that it changes the order in which
we step through the modes in throttle_thermal_policy_switch_next()
after this change we have:
Normal Asus: balanced -> performance -> silent -> balanced -> etc.
Vivobook: balanced -> silent -> performance -> balanced -> etc.
where if we see "silent" as lower performance then the other 2,
the vivobook order is a bit weird.
I wonder if this is a big enough issue to really worry about it
though; and I do like the cleanup / simpler code.
Note that this also causes a behavior change in
fan_curve_get_factory_default() as I mentioned in my cover-letter.
I think that that behavior change might be a good thing to do actually,
but at a minimum it needs to be documented in the commit msg.
Regards,
Hans
> ---
> drivers/platform/x86/asus-wmi.c | 64 +++++++++++----------------------
> 1 file changed, 21 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index ab9342a01a48..ce60835d0303 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -3696,10 +3696,28 @@ static int asus_wmi_custom_fan_curve_init(struct asus_wmi *asus)
> /* Throttle thermal policy ****************************************************/
> static int throttle_thermal_policy_write(struct asus_wmi *asus)
> {
> - u8 value = asus->throttle_thermal_policy_mode;
> u32 retval;
> + u8 value;
> int err;
>
> + if (asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO) {
> + switch (asus->throttle_thermal_policy_mode) {
> + case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
> + value = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO;
> + break;
> + case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
> + value = ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO;
> + break;
> + case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
> + value = ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO;
> + break;
> + default:
> + return -EINVAL;
> + }
> + } else {
> + value = asus->throttle_thermal_policy_mode;
> + }
> +
> err = asus_wmi_set_devstate(asus->throttle_thermal_policy_dev,
> value, &retval);
>
> @@ -3804,46 +3822,6 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
> static DEVICE_ATTR_RW(throttle_thermal_policy);
>
> /* Platform profile ***********************************************************/
> -static int asus_wmi_platform_profile_to_vivo(struct asus_wmi *asus, int mode)
> -{
> - bool vivo;
> -
> - vivo = asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO;
> -
> - if (vivo) {
> - switch (mode) {
> - case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
> - return ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO;
> - case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
> - return ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO;
> - case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
> - return ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO;
> - }
> - }
> -
> - return mode;
> -}
> -
> -static int asus_wmi_platform_profile_mode_from_vivo(struct asus_wmi *asus, int mode)
> -{
> - bool vivo;
> -
> - vivo = asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO;
> -
> - if (vivo) {
> - switch (mode) {
> - case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO:
> - return ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
> - case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO:
> - return ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST;
> - case ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO:
> - return ASUS_THROTTLE_THERMAL_POLICY_SILENT;
> - }
> - }
> -
> - return mode;
> -}
> -
> static int asus_wmi_platform_profile_get(struct platform_profile_handler *pprof,
> enum platform_profile_option *profile)
> {
> @@ -3853,7 +3831,7 @@ static int asus_wmi_platform_profile_get(struct platform_profile_handler *pprof,
> asus = container_of(pprof, struct asus_wmi, platform_profile_handler);
> tp = asus->throttle_thermal_policy_mode;
>
> - switch (asus_wmi_platform_profile_mode_from_vivo(asus, tp)) {
> + switch (tp) {
> case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
> *profile = PLATFORM_PROFILE_BALANCED;
> break;
> @@ -3892,7 +3870,7 @@ static int asus_wmi_platform_profile_set(struct platform_profile_handler *pprof,
> return -EOPNOTSUPP;
> }
>
> - asus->throttle_thermal_policy_mode = asus_wmi_platform_profile_to_vivo(asus, tp);
> + asus->throttle_thermal_policy_mode = tp;
> return throttle_thermal_policy_write(asus);
> }
>
> --
> 2.39.5
>
Am 26.10.24 um 11:56 schrieb Hans de Goede:
> Hi Armin,
>
> On 25-Oct-24 9:15 PM, Armin Wolf wrote:
>> When changing the thermal policy using the platform profile API,
>> a Vivobook thermal policy is stored in throttle_thermal_policy_mode.
>>
>> However everywhere else a normal thermal policy is stored inside this
>> variable, potentially confusing the platform profile.
> You say "potentially confusing the platform profile", but did you
> spot any actual issues when reviewing the code ?
>
Yes, for example:
1. User sets thermal policy to "1" (overboost) through the throttle_thermal_policy sysfs attr.
2. "1" gets stored inside throttle_thermal_policy_mode.
3. Platform profile will now think that ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO (1) is set.
4. Platform profile reports current mode as PLATFORM_PROFILE_QUIET.
=> error!
>> Fix this by always storing normal thermal policy values inside
>> throttle_thermal_policy_mode and only do the conversion when writing
>> the thermal policy to hardware.
>>
>> Fixes: bcbfcebda2cb ("platform/x86: asus-wmi: add support for vivobook fan profiles")
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> The problem with this approach is that it changes the order in which
> we step through the modes in throttle_thermal_policy_switch_next()
> after this change we have:
>
> Normal Asus: balanced -> performance -> silent -> balanced -> etc.
> Vivobook: balanced -> silent -> performance -> balanced -> etc.
>
> where if we see "silent" as lower performance then the other 2,
> the vivobook order is a bit weird.
>
> I wonder if this is a big enough issue to really worry about it
> though; and I do like the cleanup / simpler code.
I think users expect the normal Asus switching behavior, but you are right
that this change should be explained inside the commit message.
>
> Note that this also causes a behavior change in
> fan_curve_get_factory_default() as I mentioned in my cover-letter.
Since fan_curve_get_factory_default() was introduced before commit bcbfcebda2cb,
i think this patches actually fixes the behavior of this function.
>
> I think that that behavior change might be a good thing to do actually,
> but at a minimum it needs to be documented in the commit msg.
>
> Regards,
>
> Hans
>
I will wait till the maintainer of asus-wmi can clarify whether or not
fan_curve_get_factory_default() expects normal Asus thermal policy values
or not.
Once we resolved this i will send an updated series.
Thanks,
Armin Wolf
>> ---
>> drivers/platform/x86/asus-wmi.c | 64 +++++++++++----------------------
>> 1 file changed, 21 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>> index ab9342a01a48..ce60835d0303 100644
>> --- a/drivers/platform/x86/asus-wmi.c
>> +++ b/drivers/platform/x86/asus-wmi.c
>> @@ -3696,10 +3696,28 @@ static int asus_wmi_custom_fan_curve_init(struct asus_wmi *asus)
>> /* Throttle thermal policy ****************************************************/
>> static int throttle_thermal_policy_write(struct asus_wmi *asus)
>> {
>> - u8 value = asus->throttle_thermal_policy_mode;
>> u32 retval;
>> + u8 value;
>> int err;
>>
>> + if (asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO) {
>> + switch (asus->throttle_thermal_policy_mode) {
>> + case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
>> + value = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO;
>> + break;
>> + case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
>> + value = ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO;
>> + break;
>> + case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
>> + value = ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> + } else {
>> + value = asus->throttle_thermal_policy_mode;
>> + }
>> +
>> err = asus_wmi_set_devstate(asus->throttle_thermal_policy_dev,
>> value, &retval);
>>
>> @@ -3804,46 +3822,6 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
>> static DEVICE_ATTR_RW(throttle_thermal_policy);
>>
>> /* Platform profile ***********************************************************/
>> -static int asus_wmi_platform_profile_to_vivo(struct asus_wmi *asus, int mode)
>> -{
>> - bool vivo;
>> -
>> - vivo = asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO;
>> -
>> - if (vivo) {
>> - switch (mode) {
>> - case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
>> - return ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO;
>> - case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
>> - return ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO;
>> - case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
>> - return ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO;
>> - }
>> - }
>> -
>> - return mode;
>> -}
>> -
>> -static int asus_wmi_platform_profile_mode_from_vivo(struct asus_wmi *asus, int mode)
>> -{
>> - bool vivo;
>> -
>> - vivo = asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO;
>> -
>> - if (vivo) {
>> - switch (mode) {
>> - case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO:
>> - return ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
>> - case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO:
>> - return ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST;
>> - case ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO:
>> - return ASUS_THROTTLE_THERMAL_POLICY_SILENT;
>> - }
>> - }
>> -
>> - return mode;
>> -}
>> -
>> static int asus_wmi_platform_profile_get(struct platform_profile_handler *pprof,
>> enum platform_profile_option *profile)
>> {
>> @@ -3853,7 +3831,7 @@ static int asus_wmi_platform_profile_get(struct platform_profile_handler *pprof,
>> asus = container_of(pprof, struct asus_wmi, platform_profile_handler);
>> tp = asus->throttle_thermal_policy_mode;
>>
>> - switch (asus_wmi_platform_profile_mode_from_vivo(asus, tp)) {
>> + switch (tp) {
>> case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
>> *profile = PLATFORM_PROFILE_BALANCED;
>> break;
>> @@ -3892,7 +3870,7 @@ static int asus_wmi_platform_profile_set(struct platform_profile_handler *pprof,
>> return -EOPNOTSUPP;
>> }
>>
>> - asus->throttle_thermal_policy_mode = asus_wmi_platform_profile_to_vivo(asus, tp);
>> + asus->throttle_thermal_policy_mode = tp;
>> return throttle_thermal_policy_write(asus);
>> }
>>
>> --
>> 2.39.5
>>
>
© 2016 - 2024 Red Hat, Inc.