drivers/mux/core.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
Since commit eec611d26f84 ("ASoC: codecs: wcd938x: add mux control
support for hp audio mux") we have drivers looking up mux controls that
are optional. This results in errors incorrectly being logged on
machines like the Lenovo ThinkPad X13s where the mux is missing:
wcd938x_codec audio-codec: /audio-codec: failed to get mux-control (0)
Suppress the error message when lookup of mux controls fails and make
sure to return -ENOENT consistently also when looking up controls by
name so that consumer drivers can easily determine how to proceed.
Note that most current consumers already log mux lookup failures
themselves.
Fixes: eec611d26f84 ("ASoC: codecs: wcd938x: add mux control support for hp audio mux")
Link: https://lore.kernel.org/lkml/Z-z_ZAyVBK5ui50k@hovoldconsulting.com/
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
drivers/mux/core.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index 02be4ba37257..b95bc03e3d6b 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -544,8 +544,13 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
index = of_property_match_string(np, "mux-control-names",
mux_name);
if (index < 0) {
- dev_err(dev, "mux controller '%s' not found\n",
- mux_name);
+ if (!state && index == -EINVAL)
+ index = -ENOENT;
+
+ if (index != -ENOENT) {
+ dev_err(dev, "mux controller '%s' not found\n",
+ mux_name);
+ }
return ERR_PTR(index);
}
}
@@ -559,8 +564,11 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
"mux-controls", "#mux-control-cells",
index, &args);
if (ret) {
- dev_err(dev, "%pOF: failed to get mux-%s %s(%i)\n",
- np, state ? "state" : "control", mux_name ?: "", index);
+ if (state || ret != -ENOENT) {
+ dev_err(dev, "%pOF: failed to get mux-%s %s(%i)\n",
+ np, state ? "state" : "control",
+ mux_name ?: "", index);
+ }
return ERR_PTR(ret);
}
--
2.49.0Hi!
2025-04-14 at 14:42, Johan Hovold wrote:
> Since commit eec611d26f84 ("ASoC: codecs: wcd938x: add mux control
> support for hp audio mux") we have drivers looking up mux controls that
> are optional. This results in errors incorrectly being logged on
> machines like the Lenovo ThinkPad X13s where the mux is missing:
>
> wcd938x_codec audio-codec: /audio-codec: failed to get mux-control (0)
>
> Suppress the error message when lookup of mux controls fails and make
> sure to return -ENOENT consistently also when looking up controls by
> name so that consumer drivers can easily determine how to proceed.
>
> Note that most current consumers already log mux lookup failures
> themselves.
>
> Fixes: eec611d26f84 ("ASoC: codecs: wcd938x: add mux control support for hp audio mux")
> Link: https://lore.kernel.org/lkml/Z-z_ZAyVBK5ui50k@hovoldconsulting.com/
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
> drivers/mux/core.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
> index 02be4ba37257..b95bc03e3d6b 100644
> --- a/drivers/mux/core.c
> +++ b/drivers/mux/core.c
> @@ -544,8 +544,13 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
> index = of_property_match_string(np, "mux-control-names",
> mux_name);
> if (index < 0) {
> - dev_err(dev, "mux controller '%s' not found\n",
> - mux_name);
> + if (!state && index == -EINVAL)
> + index = -ENOENT;
Why exclude states? For me, that's entirely random and inconsistent. If there's
a reason to exclude them, I'd like to hear about it. If there is no reason and
this is just defensive programming, then I'd like for someone to dig into it
and either find a reason for the difference or clean up the inconsistency.
I think the model of explicitly marking when you'd like a mux to be optional
is a better and less fragile model. Who is to say that -EINVAL from some other
call is, and will remain, a perfect match for the optional case you are aiming
for?
Srinivas Kandagatla is looking into optional muxes as a side issue to
exclusive muxes.
https://lore.kernel.org/all/20250326154613.3735-1-srinivas.kandagatla@linaro.org/
Cheers,
Peter
> +
> + if (index != -ENOENT) {
> + dev_err(dev, "mux controller '%s' not found\n",
> + mux_name);
> + }
> return ERR_PTR(index);
> }
> }
> @@ -559,8 +564,11 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
> "mux-controls", "#mux-control-cells",
> index, &args);
> if (ret) {
> - dev_err(dev, "%pOF: failed to get mux-%s %s(%i)\n",
> - np, state ? "state" : "control", mux_name ?: "", index);
> + if (state || ret != -ENOENT) {
> + dev_err(dev, "%pOF: failed to get mux-%s %s(%i)\n",
> + np, state ? "state" : "control",
> + mux_name ?: "", index);
> + }
> return ERR_PTR(ret);
> }
>
On Mon, Apr 14, 2025 at 03:18:03PM +0200, Peter Rosin wrote:
> 2025-04-14 at 14:42, Johan Hovold wrote:
> > Since commit eec611d26f84 ("ASoC: codecs: wcd938x: add mux control
> > support for hp audio mux") we have drivers looking up mux controls that
> > are optional. This results in errors incorrectly being logged on
> > machines like the Lenovo ThinkPad X13s where the mux is missing:
> >
> > wcd938x_codec audio-codec: /audio-codec: failed to get mux-control (0)
> >
> > Suppress the error message when lookup of mux controls fails and make
> > sure to return -ENOENT consistently also when looking up controls by
> > name so that consumer drivers can easily determine how to proceed.
> >
> > Note that most current consumers already log mux lookup failures
> > themselves.
> >
> > Fixes: eec611d26f84 ("ASoC: codecs: wcd938x: add mux control support for hp audio mux")
> > Link: https://lore.kernel.org/lkml/Z-z_ZAyVBK5ui50k@hovoldconsulting.com/
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > --- a/drivers/mux/core.c
> > +++ b/drivers/mux/core.c
> > @@ -544,8 +544,13 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
> > index = of_property_match_string(np, "mux-control-names",
> > mux_name);
> > if (index < 0) {
> > - dev_err(dev, "mux controller '%s' not found\n",
> > - mux_name);
> > + if (!state && index == -EINVAL)
> > + index = -ENOENT;
>
> Why exclude states? For me, that's entirely random and inconsistent. If there's
> a reason to exclude them, I'd like to hear about it. If there is no reason and
> this is just defensive programming, then I'd like for someone to dig into it
> and either find a reason for the difference or clean up the inconsistency.
I only found one user of "mux states" and I'm still not quite sure why
there are two interfaces for looking up muxes. But my impression was
that if you need a mux set to a specific state and you even encode that
directly in DT, then there should be no need to support optional
resources.
After taking a closer look at the single consumer now, I see that it
already implements optional lookups itself and thus could benefit from
generalising this.
There's no other reason for why this could not be extended to "mux
states".
> I think the model of explicitly marking when you'd like a mux to be optional
> is a better and less fragile model. Who is to say that -EINVAL from some other
> call is, and will remain, a perfect match for the optional case you are aiming
> for?
-EINVAL is simply the error returned from the OF helpers when the name
properties are missing. I map that to -ENOENT for consistency with index
lookups (i.e. when the "mux-controls" property is missing) and that
error is much less likely to be returned for other reasons.
> Srinivas Kandagatla is looking into optional muxes as a side issue to
> exclusive muxes.
> https://lore.kernel.org/all/20250326154613.3735-1-srinivas.kandagatla@linaro.org/
The audio codec change introduces a de-facto regression so if you want
something different, we'll have to fix this in the codec driver directly
by checking for a "mux-controls" property before doing the lookup for
now (i.e. like is done in the TI driver looking up an optional "mux
state").
Johan
On 14/04/2025 15:01, Johan Hovold wrote: >> Srinivas Kandagatla is looking into optional muxes as a side issue to >> exclusive muxes. >> https://lore.kernel.org/all/20250326154613.3735-1-srinivas.kandagatla@linaro.org/ > The audio codec change introduces a de-facto regression so if you want > something different, we'll have to fix this in the codec driver directly > by checking for a "mux-controls" property before doing the lookup for This is not scalable solution, we need something in the core to allow optional mux like any other framworks. --srini > now (i.e. like is done in the TI driver looking up an optional "mux
On Mon, Apr 14, 2025 at 03:35:03PM +0100, Srinivas Kandagatla wrote: > On 14/04/2025 15:01, Johan Hovold wrote: > >> Srinivas Kandagatla is looking into optional muxes as a side issue to > >> exclusive muxes. > >> https://lore.kernel.org/all/20250326154613.3735-1-srinivas.kandagatla@linaro.org/ > > The audio codec change introduces a de-facto regression so if you want > > something different, we'll have to fix this in the codec driver directly > > by checking for a "mux-controls" property before doing the lookup for > > This is not scalable solution, we need something in the core to allow > optional mux like any other framworks. Maybe, but that's not something that exists today and we should not knowingly introduce bogus errors. I'll suppress the error in the codec driver instead and you guys can continue discussing how you want to rework the mux driver. Johan
© 2016 - 2026 Red Hat, Inc.