On Thu, Jan 09, 2025 at 04:40:14PM +0530, Hridesh MG wrote: > On Wed, Jan 8, 2025 at 7:21 PM Kurt Borja <kuurtb@xxxxxxxxx> 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@xxxxxx/ > > > Signed-off-by: Hridesh MG <hridesh699@xxxxxxxxx> > > > --- > > > 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; > } Hi Hridesh, It looks a bit verbose, but for me it's fine, it even caches the value. If max_perf is a global variable now, rename it to something like acer_predator_v4_max_perf. > > 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! Glad I can help :) ~ Kurt > > -- > Thanks, > Hridesh MG
