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