Re: [RFC PATCH 16/21] alienware-wmi: Make running control state part of platdata

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

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

On Thu, 5 Dec 2024, Kurt Borja wrote:

> On Thu, Dec 05, 2024 at 01:32:46PM +0200, Ilpo Järvinen wrote:
> > On Wed, 4 Dec 2024, Kurt Borja wrote:
> > 
> > > Both WMI devices have a different "RUNNING" control state code. Make the
> > > WMI drivers decide which code to use, and refactor sysfs methods
> > > accordingly.
> > > 
> > > Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> > > ---
> > >  drivers/platform/x86/dell/alienware-wmi.c | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> > > index 25e0139ed78c..fa21a50d66bd 100644
> > > --- a/drivers/platform/x86/dell/alienware-wmi.c
> > > +++ b/drivers/platform/x86/dell/alienware-wmi.c
> > > @@ -431,6 +431,7 @@ struct alienfx_platdata {
> > >  	bool hdmi_mux;
> > >  	bool amplifier;
> > >  	bool deepslp;
> > > +	u8 running_code;
> > >  };
> > >  
> > >  static u8 interface;
> > > @@ -576,18 +577,18 @@ static ssize_t lighting_control_state_store(struct device *dev,
> > >  					    const char *buf, size_t count)
> > >  {
> > >  	struct alienfx_priv *priv;
> > > +	struct alienfx_platdata *pdata;
> > >  	u8 val;
> > >  
> > >  	priv = dev_get_drvdata(dev);
> > > +	pdata = dev_get_platdata(dev);
> > >  
> > >  	if (strcmp(buf, "booting\n") == 0)
> > >  		val = LEGACY_BOOTING;
> > >  	else if (strcmp(buf, "suspend\n") == 0)
> > >  		val = LEGACY_SUSPEND;
> > > -	else if (interface == LEGACY)
> > > -		val = LEGACY_RUNNING;
> > >  	else
> > > -		val = WMAX_RUNNING;
> > > +		val = pdata->running_code;
> > >  
> > >  	priv->lighting_control_state = val;
> > >  	pr_debug("alienware-wmi: updated control state to %d\n",
> > > @@ -1249,6 +1250,7 @@ static int legacy_wmi_probe(struct wmi_device *wdev, const void *context)
> > >  		.hdmi_mux = quirks->hdmi_mux,
> > >  		.amplifier = quirks->amplifier,
> > >  		.deepslp = quirks->deepslp,
> > > +		.running_code = LEGACY_RUNNING,
> > >  	};
> > >  
> > >  	if (quirks->num_zones > 0)
> > > @@ -1333,6 +1335,7 @@ static int wmax_wmi_probe(struct wmi_device *wdev, const void *context)
> > >  		.hdmi_mux = quirks->hdmi_mux,
> > >  		.amplifier = quirks->amplifier,
> > >  		.deepslp = quirks->deepslp,
> > > +		.running_code = WMAX_RUNNING,
> > >  	};
> > >  
> > >  	if (quirks->thermal)
> > > 
> > 
> > I've not taken extensive look at interdependent changes in the series but 
> > while reviewing patch 20/21 I noticed that alienfx_probe() changed there
> > from:
> > 
> > -       if (interface == WMAX)
> > -               priv->lighting_control_state = WMAX_RUNNING;
> > -       else if (interface == LEGACY)
> > -               priv->lighting_control_state = LEGACY_RUNNING;
> > 
> > to:
> > 
> > +       priv->lighting_control_state = pdata->running_code;
> 
> This was a workaround. I forgot to add this change in this patch. Also I'll
> move this earlier in the series.
> 
> > 
> > Somehow, it felt odd and then looking this patch 16, it felt even odder. 
> 
> The reason behind this is that I don't want AlienFX related methods
> depending on global variables like `interface` so I can split the file
> cleanly.

Just to make sure we don't talk past each other, I didn't mean the code 
itself felt odd, just the patch it was changed in. So if your plan was to 
have it in this patch to begin with but you noticed the problem while 
doing the other change and put it "temporarily" there and forgot to move 
it into correct place, no oddity problem then. :-)

> > Perhaps there's a good reason my review that didn't go deep enough failed 
> > to uncover but please check if this is an indication that something is a 
> > miss in the series.
> > 
> 
> Thank you so much for commenting on the series!
> 
> Also, what do you think about the general approach? I was a bit unsure
> about the whole "platdata" stuff, as not many drivers use it in this
> subsystem.

I'm sorry but I've not come across it much so I'm not sure if my opinion 
is that valuable.

What I can say for sure though is that the wide-spread use of global-level 
variables in pdx86 drivers is definitely not setting a good example.

-- 
 i.