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

[Kurt Borja]

On Thu, Dec 05, 2024 at 04:06:23PM +0200, Ilpo Järvinen wrote:
> 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@xxxxxxxxx>
> > > > ---
> > > >  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. :-)

No worries, I explained my intention just in case it was confusing.
Thank you for clarifying :)

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

In that case I'll stick with it for now!

~ Kurt

> 
> -- 
>  i.