Re: [PATCH v6 03/12] platform/x86: alienware-wmi-wmax: Improve internal AWCC API

Ilpo Järvinen
2025-03-31T16:06:42Z

[-- Attachment #1: Type: text/plain, Size: 3527 bytes --]

On Fri, 28 Mar 2025, Kurt Borja wrote:

> On Fri Mar 28, 2025 at 11:51 AM -03, Ilpo Järvinen wrote:
> > On Thu, 13 Mar 2025, Kurt Borja wrote:
> >
> >> Inline all AWCC WMI helper methods and directly return the newly
> >> introduced __awcc_wmi_command() helper to simplify implementation.
> >>
> >> Drop awcc_thermal_control() in favor of awcc_op_activate_profile().
> >>
> >> Add awcc_op_get_resource_id(), awcc_op_get_current_profile() and a new
> >> failure code.
> >>
> >> Reviewed-by: Armin Wolf <W_Armin@gmx.de>
> >> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> >> ---
> >> drivers/platform/x86/dell/alienware-wmi-wmax.c | 150 +++++++++++++++----------
> >> 1 file changed, 91 insertions(+), 59 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/dell/alienware-wmi-wmax.c b/drivers/platform/x86/dell/alienware-wmi-wmax.c
> >> index 80aefba5b22d6b4ac18aeb2ca356f8c911150abd..b9dbfdc8096c571722b3c7ac3e73989e235e2eb9 100644
> >> --- a/drivers/platform/x86/dell/alienware-wmi-wmax.c
> >> +++ b/drivers/platform/x86/dell/alienware-wmi-wmax.c
> >> @@ -32,6 +32,7 @@
> >> #define AWCC_THERMAL_MODE_GMODE 0xAB
> >>
> >> #define AWCC_FAILURE_CODE 0xFFFFFFFF
> >> +#define AWCC_FAILURE_CODE_2 0xFFFFFFFE
> >> #define AWCC_THERMAL_TABLE_MASK GENMASK(7, 4)
> >> #define AWCC_THERMAL_MODE_MASK GENMASK(3, 0)
> >> /* Some IDs have a BIT(8) flag that we ignore */
> >> @@ -443,8 +444,7 @@ const struct attribute_group wmax_deepsleep_attribute_group = {
> >> };
> >>
> >> /*
> >> - * Thermal Profile control
> >> - * - Provides thermal profile control through the Platform Profile API
> >> + * AWCC Helpers
> >> */
> >> static bool is_awcc_thermal_profile_id(u8 code)
> >> {
> >> @@ -463,72 +463,107 @@ static bool is_awcc_thermal_profile_id(u8 code)
> >> return false;
> >> }
> >>
> >> -static int awcc_thermal_information(struct wmi_device *wdev, u8 operation,
> >> - u8 arg, u32 *out_data)
> >> +static int __awcc_wmi_command(struct wmi_device *wdev, u32 method_id,
> >> + struct wmax_u32_args *args, u32 *out)
> >
> > Why did you put __ into the name?
> >
> > Some people oppose __ prefix altogether, I don't entirely agree with their
> > stance but I don't really understand what the underscores here signify.
> >
> > Normally I see __ being used in three main cases:
> > - non-__ variant does some locking around the call too the __ func (though
> > many funcs use _locked postfix these days).
> > - func is "dangerous" and caller has to really know what he/she is
> > doing / be careful for some reason.
> > - non-__ variant exists and somebody cannot come up better name for the
> > internally called function (not very good use case, IMHO)
>
> Oh - I didn't know. Andy also mentioned this prefix is used for
> non-atomic versions of some functions like __set_bit().

I mentioned locking which relates to atomicity, however, here you don't
have two variants, one without locking and the other with it so it becomes
very non-obvious what __ means. (I know you posted v7 without it, so
this is just a clarifying comment.)

> > I don't see any of those apply here, this looks just an ordinary wrapper
> > function, thus the question.
>
> I did it to indicate it's a "private" method, so it's not used directly.
> But ofc that's not the convention here, so I'll drop it.

We have "static" for that. And it's called by something anyway or it would
be dead code :-).

--
i.