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.
