| Message ID | 20241205004534.2186539-2-kuurtb@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | alienware-wmi driver rework | expand |
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; Somehow, it felt odd and then looking this patch 16, it felt even odder. 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.
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. > 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. >
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.
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@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. :-) 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.
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)
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(-)