Re: [PATCH v3 4/5] platform/x86: acer-wmi: use an ACPI bitmap to set the platform profile choices

Hridesh MG
2025-01-09T11:10:52Z

On Wed, Jan 8, 2025 at 7:21 PM Kurt Borja <kuurtb@gmail.com> wrote:
>
> On Wed, Jan 08, 2025 at 02:15:26PM +0530, Hridesh MG wrote:
> > Currently the choices for the platform profile are hardcoded. There is
> > an ACPI bitmap accessible via WMI that specifies the supported profiles,
> > use this bitmap to dynamically set the choices for the platform profile.
> >
> > Link: https://lore.kernel.org/platform-driver-x86/ecb60ee5-3df7-4d7e-8ebf-8c162b339ade@gmx.de/
> > Signed-off-by: Hridesh MG <hridesh699@gmail.com>
> > ---
> > drivers/platform/x86/acer-wmi.c | 55 +++++++++++++++++++++++++++++------------
> > 1 file changed, 39 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> > index 7968fe21507b1cf28fdc575139057c795e6a873b..6c98c1bb3bdce6a7c6559f6da4ff3c6ce56b60e3 100644
> > --- a/drivers/platform/x86/acer-wmi.c
> > +++ b/drivers/platform/x86/acer-wmi.c
> > @@ -33,6 +33,7 @@
> > #include <linux/units.h>
> > #include <linux/unaligned.h>
> > #include <linux/bitfield.h>
> > +#include <linux/bitmap.h>
> >
> > MODULE_AUTHOR("Carlos Corbacho");
> > MODULE_DESCRIPTION("Acer Laptop WMI Extras Driver");
> > @@ -127,6 +128,7 @@ enum acer_wmi_predator_v4_oc {
> > enum acer_wmi_gaming_misc_setting {
> > ACER_WMID_MISC_SETTING_OC_1 = 0x0005,
> > ACER_WMID_MISC_SETTING_OC_2 = 0x0007,
> > + ACER_WMID_MISC_SETTING_SUPPORTED_PROFILES = 0x000A,
> > ACER_WMID_MISC_SETTING_PLATFORM_PROFILE = 0x000B,
> > };
> >
> > @@ -1957,7 +1959,7 @@ static int
> > acer_predator_v4_platform_profile_set(struct platform_profile_handler *pprof,
> > enum platform_profile_option profile)
> > {
> > - int err, tp;
> > + int max_perf, err, tp;
> >
> > switch (profile) {
> > case PLATFORM_PROFILE_PERFORMANCE:
> > @@ -1983,7 +1985,10 @@ acer_predator_v4_platform_profile_set(struct platform_profile_handler *pprof,
> > if (err)
> > return err;
> >
> > - if (tp != ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO)
> > + max_perf = find_last_bit(platform_profile_handler.choices,
> > + PLATFORM_PROFILE_LAST);
> > +
> > + if (tp != max_perf)
>
> You can't directly compare `tp` and `max_perf`. ACER_PREDATOR_V4 values
> may not match PLATFORM_PROFILE ones.
>
> It does in the case of PERFORMANCE and TURBO, but it does not in the
> case of QUIET and BALANCED.
>
> I suggest you store the actual ACER_PREDATOR_V4 max_perf when setting up
> the platform_profile.
Ah this was quite a stupid mistake. I'm not sure why I even assumed
both were equivalent. I have one doubt though, if i set it during
profile setup, the code becomes quite verbose -

/* Iterate through supported profiles in order of increasing
performance */
if (test_bit(ACER_PREDATOR_V4_THERMAL_PROFILE_ECO,
&supported_profiles)) {
set_bit(PLATFORM_PROFILE_LOW_POWER,
platform_profile_handler.choices);
max_perf = ACER_PREDATOR_V4_THERMAL_PROFILE_ECO;
}

if (test_bit(ACER_PREDATOR_V4_THERMAL_PROFILE_QUIET,
&supported_profiles)) {
set_bit(PLATFORM_PROFILE_QUIET,
platform_profile_handler.choices);
max_perf = ACER_PREDATOR_V4_THERMAL_PROFILE_QUIET;
}

if (test_bit(ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED,
&supported_profiles)) {
set_bit(PLATFORM_PROFILE_BALANCED,
platform_profile_handler.choices);
max_perf = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED;
}

if (test_bit(ACER_PREDATOR_V4_THERMAL_PROFILE_PERFORMANCE,
&supported_profiles)) {
set_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE,
platform_profile_handler.choices);
max_perf = ACER_PREDATOR_V4_THERMAL_PROFILE_PERFORMANCE;
}

if (test_bit(ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO,
&supported_profiles)) {
set_bit(PLATFORM_PROFILE_PERFORMANCE,
platform_profile_handler.choices);
max_perf = ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO;
}

Is this fine? Maybe for readability's sake, I could lift it up into a
different function, like what you did in the RFC patch. Btw, thanks a lot
for the detailed reviews so far—they’ve been very helpful!

--
Thanks,
Hridesh MG