From: Armin Wolf <W_Armin@gmx.de>
To: Hridesh MG <hridesh699@gmail.com>, Kurt Borja <kuurtb@gmail.com>
Cc: "Hans de Goede" <hdegoede@redhat.com>,
"Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>,
platform-driver-x86@vger.kernel.org,
linux-kernel@vger.kernel.org,
"Shuah Khan" <skhan@linuxfoundation.org>
Subject: Re: [PATCH v2 1/3] platform/x86: acer-wmi: use WMI calls for platform profile handling
Date: Sun, 5 Jan 2025 14:02:05 +0100 [thread overview]
Message-ID: <9bce3f5c-ad07-40df-94ef-871f74dead75@gmx.de> (raw)
In-Reply-To: <CALiyAomC-QXuMWs2i50SK2f=F7Te5QTbx1f1i-MWhF=STLmmEw@mail.gmail.com>
Am 05.01.25 um 12:19 schrieb Hridesh MG:
> On Sat, Jan 4, 2025 at 10:43 PM Kurt Borja <kuurtb@gmail.com> wrote:
>> On Sat, Jan 04, 2025 at 08:59:20PM +0530, Hridesh MG wrote:
>>> Improve the platform profile handling by using WMI calls to fetch the
>>> current platform profile instead of directly accessing it from the EC.
>>> This is beneficial because the EC address differs for certain laptops.
>>>
>>> Link: https://lore.kernel.org/platform-driver-x86/d7be714c-3103-42ee-ad15-223a3fe67f80@gmx.de/
>>> Co-developed-by: Armin Wolf <W_Armin@gmx.de>
>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>> Signed-off-by: Hridesh MG <hridesh699@gmail.com>
>> Hi Hridesh,
>>
>>> ---
>>> drivers/platform/x86/acer-wmi.c | 189 ++++++++++++++++++++++++++++------------
>>> 1 file changed, 133 insertions(+), 56 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
>>> index b3043d78a7b38a7b773da5ecd4846ca11e8595f5..5370056fb2d03a768162f2f1643ef27dc6deafa8 100644
>>> --- a/drivers/platform/x86/acer-wmi.c
>>> +++ b/drivers/platform/x86/acer-wmi.c
>>> @@ -31,6 +31,7 @@
>>> #include <acpi/video.h>
>>> #include <linux/hwmon.h>
>>> #include <linux/units.h>
>>> +#include <linux/unaligned.h>
>>> #include <linux/bitfield.h>
>>>
>>> MODULE_AUTHOR("Carlos Corbacho");
>>> @@ -68,8 +69,11 @@ MODULE_LICENSE("GPL");
>>> #define ACER_WMID_GET_GAMING_SYS_INFO_METHODID 5
>>> #define ACER_WMID_SET_GAMING_FAN_BEHAVIOR 14
>>> #define ACER_WMID_SET_GAMING_MISC_SETTING_METHODID 22
>>> +#define ACER_WMID_GET_GAMING_MISC_SETTING_METHODID 23
>>>
>>> -#define ACER_PREDATOR_V4_THERMAL_PROFILE_EC_OFFSET 0x54
>>> +#define ACER_GAMING_MISC_SETTING_STATUS_MASK GENMASK_ULL(7, 0)
>>> +#define ACER_GAMING_MISC_SETTING_INDEX_MASK GENMASK_ULL(7, 0)
>>> +#define ACER_GAMING_MISC_SETTING_VALUE_MASK GENMASK_ULL(15, 8)
>>>
>>> #define ACER_PREDATOR_V4_RETURN_STATUS_BIT_MASK GENMASK_ULL(7, 0)
>>> #define ACER_PREDATOR_V4_SENSOR_INDEX_BIT_MASK GENMASK_ULL(15, 8)
>>> @@ -115,6 +119,13 @@ enum acer_wmi_predator_v4_sensor_id {
>>> ACER_WMID_SENSOR_GPU_TEMPERATURE = 0x0A,
>>> };
>>>
>>> +enum acer_wmi_gaming_misc_setting {
>>> + ACER_WMID_MISC_SETTING_OC_1 = 0x0005,
>>> + ACER_WMID_MISC_SETTING_OC_2 = 0x0007,
>> These OC settings should be added only if you add support for them.
>>
>> I noticed acer_toggle_turbo() uses these settings. For consistency, I
>> think it should be refactored to use WMID_gaming_set_misc_setting()
>> instead of WMID_gaming_set_u64().
> Yeah I agree. Actually, now that we have this function, this
> particular case in WMID_gaming_set_u64() is redundant, so can I remove
> it? (sorry if this is a dumb question)
>
> switch (cap) {
> case ACER_CAP_TURBO_OC:
> method_id = ACER_WMID_SET_GAMING_MISC_SETTING_METHODID;
> break;
> }
Yes i think so. Please do the conversion of the OC settings in a separate patch so that
it is easier to review.
Keep in mind that you emulate the behavior of WMID_gaming_set_u64() and check if the
interface has ACER_CAP_TURBO_OC before calling WMID_gaming_set_misc_settings() to change
the OC settings.
Thanks,
Armin Wolf
>>> + ACER_WMID_MISC_SETTING_SUPPORTED_PROFILES = 0x000A,
>> This should be added in patch [2/3].
>>
>>> + ACER_WMID_MISC_SETTING_PLATFORM_PROFILE = 0x000B,
>>> +};
>>> +
>>> static const struct key_entry acer_wmi_keymap[] __initconst = {
>>> {KE_KEY, 0x01, {KEY_WLAN} }, /* WiFi */
>>> {KE_KEY, 0x03, {KEY_WLAN} }, /* WiFi */
>>> @@ -751,20 +762,12 @@ static bool platform_profile_support;
>>> */
>>> static int last_non_turbo_profile;
>>>
>>> -enum acer_predator_v4_thermal_profile_ec {
>>> - ACER_PREDATOR_V4_THERMAL_PROFILE_ECO = 0x04,
>>> - ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO = 0x03,
>>> - ACER_PREDATOR_V4_THERMAL_PROFILE_PERFORMANCE = 0x02,
>>> - ACER_PREDATOR_V4_THERMAL_PROFILE_QUIET = 0x01,
>>> - ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED = 0x00,
>>> -};
>>> -
>>> -enum acer_predator_v4_thermal_profile_wmi {
>>> - ACER_PREDATOR_V4_THERMAL_PROFILE_ECO_WMI = 0x060B,
>>> - ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO_WMI = 0x050B,
>>> - ACER_PREDATOR_V4_THERMAL_PROFILE_PERFORMANCE_WMI = 0x040B,
>>> - ACER_PREDATOR_V4_THERMAL_PROFILE_QUIET_WMI = 0x0B,
>>> - ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED_WMI = 0x010B,
>>> +enum acer_predator_v4_thermal_profile {
>>> + ACER_PREDATOR_V4_THERMAL_PROFILE_QUIET = 0x00,
>>> + ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED = 0x01,
>>> + ACER_PREDATOR_V4_THERMAL_PROFILE_PERFORMANCE = 0x04,
>>> + ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO = 0x05,
>>> + ACER_PREDATOR_V4_THERMAL_PROFILE_ECO = 0x06,
>>> };
>>>
>>> /* Find which quirks are needed for a particular vendor/ model pair */
>>> @@ -1477,6 +1480,45 @@ WMI_gaming_execute_u64(u32 method_id, u64 in, u64 *out)
>>> return status;
>>> }
>>>
>>> +static int WMI_gaming_execute_u32_u64(u32 method_id, u32 in, u64 *out)
>>> +{
>>> + struct acpi_buffer result = { ACPI_ALLOCATE_BUFFER, NULL };
>>> + struct acpi_buffer input = {
>>> + .length = sizeof(in),
>>> + .pointer = &in,
>>> + };
>>> + union acpi_object *obj;
>>> + acpi_status status;
>>> + int ret = 0;
>>> +
>>> + status = wmi_evaluate_method(WMID_GUID4, 0, method_id, &input, &result);
>>> + if (ACPI_FAILURE(status))
>>> + return -EIO;
>>> +
>>> + obj = result.pointer;
>>> + if (obj && out) {
>>> + switch (obj->type) {
>>> + case ACPI_TYPE_INTEGER:
>>> + *out = obj->integer.value;
>>> + break;
>>> + case ACPI_TYPE_BUFFER:
>>> + if (obj->buffer.length < sizeof(*out))
>>> + ret = -ENOMSG;
>>> + else
>>> + *out = get_unaligned_le64(obj->buffer.pointer);
>>> +
>>> + break;
>>> + default:
>>> + ret = -ENOMSG;
>>> + break;
>>> + }
>>> + }
>>> +
>>> + kfree(obj);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> static acpi_status WMID_gaming_set_u64(u64 value, u32 cap)
>>> {
>>> u32 method_id = 0;
>>> @@ -1565,6 +1607,48 @@ static void WMID_gaming_set_fan_mode(u8 fan_mode)
>>> WMID_gaming_set_u64(gpu_fan_config2 | gpu_fan_config1 << 16, ACER_CAP_TURBO_FAN);
>>> }
>>>
>>> +static int WMID_gaming_set_misc_setting(enum acer_wmi_gaming_misc_setting setting, u8 value)
>>> +{
>>> + acpi_status status;
>>> + u64 input = 0;
>>> + u64 result;
>>> +
>>> + input |= FIELD_PREP(ACER_GAMING_MISC_SETTING_INDEX_MASK, setting);
>>> + input |= FIELD_PREP(ACER_GAMING_MISC_SETTING_VALUE_MASK, value);
>>> +
>>> + status = WMI_gaming_execute_u64(ACER_WMID_SET_GAMING_MISC_SETTING_METHODID, input, &result);
>>> + if (ACPI_FAILURE(status))
>>> + return -EIO;
>>> +
>>> + /* The return status must be zero for the operation to have succeeded */
>>> + if (FIELD_GET(ACER_GAMING_MISC_SETTING_STATUS_MASK, result))
>>> + return -EIO;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int WMID_gaming_get_misc_setting(enum acer_wmi_gaming_misc_setting setting, u8 *value)
>>> +{
>>> + u64 input = 0;
>>> + u64 result;
>>> + int ret;
>>> +
>>> + input |= FIELD_PREP(ACER_GAMING_MISC_SETTING_INDEX_MASK, setting);
>>> +
>>> + ret = WMI_gaming_execute_u32_u64(ACER_WMID_GET_GAMING_MISC_SETTING_METHODID, input,
>>> + &result);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + /* The return status must be zero for the operation to have succeeded */
>>> + if (FIELD_GET(ACER_GAMING_MISC_SETTING_STATUS_MASK, result))
>>> + return -EIO;
>>> +
>>> + *value = FIELD_GET(ACER_GAMING_MISC_SETTING_VALUE_MASK, result);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> /*
>>> * Generic Device (interface-independent)
>>> */
>>> @@ -1833,9 +1917,8 @@ acer_predator_v4_platform_profile_get(struct platform_profile_handler *pprof,
>>> u8 tp;
>>> int err;
>>>
>>> - err = ec_read(ACER_PREDATOR_V4_THERMAL_PROFILE_EC_OFFSET, &tp);
>>> -
>>> - if (err < 0)
>>> + err = WMID_gaming_get_misc_setting(ACER_WMID_MISC_SETTING_PLATFORM_PROFILE, &tp);
>>> + if (err)
>>> return err;
>>>
>>> switch (tp) {
>>> @@ -1865,36 +1948,33 @@ static int
>>> acer_predator_v4_platform_profile_set(struct platform_profile_handler *pprof,
>>> enum platform_profile_option profile)
>>> {
>>> - int tp;
>>> - acpi_status status;
>>> + int tp, err;
>>>
>>> switch (profile) {
>>> case PLATFORM_PROFILE_PERFORMANCE:
>>> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO_WMI;
>>> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO;
>>> break;
>>> case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
>>> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_PERFORMANCE_WMI;
>>> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_PERFORMANCE;
>>> break;
>>> case PLATFORM_PROFILE_BALANCED:
>>> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED_WMI;
>>> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED;
>>> break;
>>> case PLATFORM_PROFILE_QUIET:
>>> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_QUIET_WMI;
>>> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_QUIET;
>>> break;
>>> case PLATFORM_PROFILE_LOW_POWER:
>>> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_ECO_WMI;
>>> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_ECO;
>>> break;
>>> default:
>>> return -EOPNOTSUPP;
>>> }
>>>
>>> - status = WMI_gaming_execute_u64(
>>> - ACER_WMID_SET_GAMING_MISC_SETTING_METHODID, tp, NULL);
>>> -
>>> - if (ACPI_FAILURE(status))
>>> - return -EIO;
>>> + err = WMID_gaming_set_misc_setting(ACER_WMID_MISC_SETTING_PLATFORM_PROFILE, tp);
>>> + if (err)
>>> + return err;
>>>
>>> - if (tp != ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO_WMI)
>>> + if (tp != ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO)
>>> last_non_turbo_profile = tp;
>>>
>>> return 0;
>>> @@ -1923,6 +2003,7 @@ static int acer_platform_profile_setup(struct platform_device *device)
>>> set_bit(PLATFORM_PROFILE_LOW_POWER,
>>> platform_profile_handler.choices);
>>>
>>> +
>> Please, drop this extra line.
>>
>> ~ Kurt
>>
>>> err = platform_profile_register(&platform_profile_handler);
>>> if (err)
>>> return err;
>>> @@ -1931,7 +2012,7 @@ static int acer_platform_profile_setup(struct platform_device *device)
>>>
>>> /* Set default non-turbo profile */
>>> last_non_turbo_profile =
>>> - ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED_WMI;
>>> + ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED;
>>> }
>>> return 0;
>>> }
>>> @@ -1946,12 +2027,10 @@ static int acer_thermal_profile_change(void)
>>> u8 current_tp;
>>> int tp, err;
>>> u64 on_AC;
>>> - acpi_status status;
>>> -
>>> - err = ec_read(ACER_PREDATOR_V4_THERMAL_PROFILE_EC_OFFSET,
>>> - ¤t_tp);
>>>
>>> - if (err < 0)
>>> + err = WMID_gaming_get_misc_setting(ACER_WMID_MISC_SETTING_PLATFORM_PROFILE,
>>> + ¤t_tp);
>>> + if (err)
>>> return err;
>>>
>>> /* Check power source */
>>> @@ -1962,54 +2041,52 @@ static int acer_thermal_profile_change(void)
>>> switch (current_tp) {
>>> case ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO:
>>> if (!on_AC)
>>> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED_WMI;
>>> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED;
>>> else if (cycle_gaming_thermal_profile)
>>> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_ECO_WMI;
>>> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_ECO;
>>> else
>>> tp = last_non_turbo_profile;
>>> break;
>>> case ACER_PREDATOR_V4_THERMAL_PROFILE_PERFORMANCE:
>>> if (!on_AC)
>>> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED_WMI;
>>> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED;
>>> else
>>> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO_WMI;
>>> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO;
>>> break;
>>> case ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED:
>>> if (!on_AC)
>>> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_ECO_WMI;
>>> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_ECO;
>>> else if (cycle_gaming_thermal_profile)
>>> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_PERFORMANCE_WMI;
>>> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_PERFORMANCE;
>>> else
>>> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO_WMI;
>>> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO;
>>> break;
>>> case ACER_PREDATOR_V4_THERMAL_PROFILE_QUIET:
>>> if (!on_AC)
>>> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED_WMI;
>>> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED;
>>> else if (cycle_gaming_thermal_profile)
>>> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED_WMI;
>>> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED;
>>> else
>>> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO_WMI;
>>> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO;
>>> break;
>>> case ACER_PREDATOR_V4_THERMAL_PROFILE_ECO:
>>> if (!on_AC)
>>> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED_WMI;
>>> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED;
>>> else if (cycle_gaming_thermal_profile)
>>> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_QUIET_WMI;
>>> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_QUIET;
>>> else
>>> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO_WMI;
>>> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO;
>>> break;
>>> default:
>>> return -EOPNOTSUPP;
>>> }
>>>
>>> - status = WMI_gaming_execute_u64(
>>> - ACER_WMID_SET_GAMING_MISC_SETTING_METHODID, tp, NULL);
>>> -
>>> - if (ACPI_FAILURE(status))
>>> - return -EIO;
>>> + err = WMID_gaming_set_misc_setting(ACER_WMID_MISC_SETTING_PLATFORM_PROFILE, tp);
>>> + if (err)
>>> + return err;
>>>
>>> /* Store non-turbo profile for turbo mode toggle*/
>>> - if (tp != ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO_WMI)
>>> + if (tp != ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO)
>>> last_non_turbo_profile = tp;
>>>
>>> platform_profile_notify(&platform_profile_handler);
>
>
next prev parent reply other threads:[~2025-01-05 13:02 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-04 15:29 [PATCH v2 0/3] platform/x86 acer-wmi: Improve " Hridesh MG
2025-01-04 15:29 ` [PATCH v2 1/3] platform/x86: acer-wmi: use WMI calls for " Hridesh MG
2025-01-04 17:13 ` Kurt Borja
2025-01-05 11:19 ` Hridesh MG
2025-01-05 13:02 ` Armin Wolf [this message]
2025-01-04 15:29 ` [PATCH v2 2/3] platform/x86: acer-wmi: use an ACPI bitmap to set the platform profile choices Hridesh MG
2025-01-04 17:18 ` Kurt Borja
2025-01-05 13:06 ` Armin Wolf
2025-01-04 15:29 ` [PATCH v2 3/3] platform/x86: acer-wmi: simplify platform profile cycling Hridesh MG
2025-01-04 17:53 ` Kurt Borja
2025-01-04 18:19 ` Hridesh MG
2025-01-05 4:01 ` SungHwan Jung
2025-01-05 4:16 ` Hridesh MG
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9bce3f5c-ad07-40df-94ef-871f74dead75@gmx.de \
--to=w_armin@gmx.de \
--cc=hdegoede@redhat.com \
--cc=hridesh699@gmail.com \
--cc=ilpo.jarvinen@linux.intel.com \
--cc=kuurtb@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=platform-driver-x86@vger.kernel.org \
--cc=skhan@linuxfoundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®