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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- 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.