On Tue, Dec 10, 2024 at 05:08:01AM +0100, Armin Wolf wrote:
> Am 10.12.24 um 03:06 schrieb Kurt Borja:
>
> > On Tue, Dec 10, 2024 at 01:16:55AM +0100, Armin Wolf wrote:
> > > If a call to ACER_WMID_GET_GAMING_SYS_INFO_METHODID fails, the lower
> > > 8 bits will be non-zero. Introduce a helper function to check this and
> > > use it when reading gaming system information.
> > >
> > > Tested-by: Rayan Margham <rayanmargham4@xxxxxxxxx>
> > > Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
> > > ---
> > > drivers/platform/x86/acer-wmi.c | 55 +++++++++++++++++++++------------
> > > 1 file changed, 36 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> > > index dd57787466b9..ac4500f33b8c 100644
> > > --- a/drivers/platform/x86/acer-wmi.c
> > > +++ b/drivers/platform/x86/acer-wmi.c
> > > @@ -70,6 +70,7 @@ MODULE_LICENSE("GPL");
> > >
> > > #define ACER_PREDATOR_V4_THERMAL_PROFILE_EC_OFFSET 0x54
> > >
> > > +#define ACER_PREDATOR_V4_RETURN_STATUS_BIT_MASK GENMASK_ULL(7, 0)
> > > #define ACER_PREDATOR_V4_FAN_SPEED_READ_BIT_MASK GENMASK(20, 8)
> > >
> > > /*
> > > @@ -1513,6 +1514,24 @@ static acpi_status WMID_gaming_get_u64(u64 *value, u32 cap)
> > > return status;
> > > }
> > >
> > > +static int WMID_gaming_get_sys_info(u32 command, u64 *out)
> > > +{
> > > + acpi_status status;
> > > + u64 result;
> > > +
> > > + status = WMI_gaming_execute_u64(ACER_WMID_GET_GAMING_SYS_INFO_METHODID, command, &result);
> > > + if (ACPI_FAILURE(status))
> > > + return -EIO;
> > > +
> > > + /* The return status must be zero for the operation to have succeeded */
> > > + if (FIELD_GET(ACER_PREDATOR_V4_RETURN_STATUS_BIT_MASK, result))
> > > + return -EIO;
> > > +
> > > + *out = result;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static void WMID_gaming_set_fan_mode(u8 fan_mode)
> > > {
> > > /* fan_mode = 1 is used for auto, fan_mode = 2 used for turbo*/
> > > @@ -1762,22 +1781,23 @@ static int acer_gsensor_event(void)
> > >
> > > static int acer_get_fan_speed(int fan)
> > > {
> > > - if (quirks->predator_v4) {
> > > - acpi_status status;
> > > - u64 fanspeed;
> > > + u64 fanspeed;
> > > + u32 command;
> > > + int ret;
> > >
> > > - status = WMI_gaming_execute_u64(
> > > - ACER_WMID_GET_GAMING_SYS_INFO_METHODID,
> > > - fan == 0 ? ACER_WMID_CMD_GET_PREDATOR_V4_CPU_FAN_SPEED :
> > > - ACER_WMID_CMD_GET_PREDATOR_V4_GPU_FAN_SPEED,
> > > - &fanspeed);
> > > + if (!quirks->predator_v4)
> > > + return -EOPNOTSUPP;
> > >
> > > - if (ACPI_FAILURE(status))
> > > - return -EIO;
> > > + if (fan == 0)
> > > + command = ACER_WMID_CMD_GET_PREDATOR_V4_CPU_FAN_SPEED;
> > > + else
> > > + command = ACER_WMID_CMD_GET_PREDATOR_V4_GPU_FAN_SPEED;
> > >
> > > - return FIELD_GET(ACER_PREDATOR_V4_FAN_SPEED_READ_BIT_MASK, fanspeed);
> > > - }
> > > - return -EOPNOTSUPP;
> > > + ret = WMID_gaming_get_sys_info(command, &fanspeed);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + return FIELD_GET(ACER_PREDATOR_V4_FAN_SPEED_READ_BIT_MASK, fanspeed);
> > > }
> > I wonder if it's necessary to refactor this function if you are going to
> > drop it on Patch 4.
>
> I did it so each patch has a single well-defined scope. And refactoring this function was necessary for
> the patch being complete.
I'll keep that in mind when submitting.
In that case:
Reviewed-by: Kurt Borja <kuurtb@xxxxxxxxx>
I apologize If this was out of place, as I'm still learning!
~ Kurt
>
> Thanks,
> Armin Wolf
>
> > > /*
> > > @@ -1942,12 +1962,9 @@ static int acer_thermal_profile_change(void)
> > > return err;
> > >
> > > /* Check power source */
> > > - status = WMI_gaming_execute_u64(
> > > - ACER_WMID_GET_GAMING_SYS_INFO_METHODID,
> > > - ACER_WMID_CMD_GET_PREDATOR_V4_BAT_STATUS, &on_AC);
> > > -
> > > - if (ACPI_FAILURE(status))
> > > - return -EIO;
> > > + err = WMID_gaming_get_sys_info(ACER_WMID_CMD_GET_PREDATOR_V4_BAT_STATUS, &on_AC);
> > > + if (err < 0)
> > > + return err;
> > >
> > > switch (current_tp) {
> > > case ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO:
> > > --
> > > 2.39.5
> > >
