Re: [PATCH] mux: suppress lookup errors for mux controls

[Johan Hovold]

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@xxxxxxxxxxxxxxxxxxxx/
> > Signed-off-by: Johan Hovold <johan+linaro@xxxxxxxxxx>

> > --- 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@xxxxxxxxxx/

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