Re: [PATCH v3 2/6] mux: Add helper functions for getting optional and selected mux-state

Josua Mayer
2025-12-29T12:07:32Z

Am 29.12.25 um 13:01 schrieb Josua Mayer:
> Am 22.12.25 um 11:08 schrieb Geert Uytterhoeven:
>> Hi Josua,
>>
>> On Wed, 10 Dec 2025 at 18:39, Josua Mayer <josua@solid-run.com> wrote:
>>> In-tree phy-can-transceiver driver has already implemented a local
>>> version of devm_mux_state_get_optional.
>>>
>>> The omap-i2c driver gets and selects an optional mux in its probe
>>> function without using any helper.
>>>
>>> Add new helper functions covering both aforementioned use-cases:
>>>
>>> - devm_mux_state_get_optional:
>>> Get a mux-state if specified in dt, return NULL otherwise.
>>> - devm_mux_state_get_optional_selected:
>>> Get and select a mux-state if specified in dt, return error or NULL.
>>>
>>> Existing mux_get helper function is changed to return -ENOENT in case dt
>>> did not specify a mux-state or -control matching given name (if valid).
>>> This matches of_parse_phandle_with_args semantics which also returns
>>> -ENOENT if the property does nto exists, or its value is zero.
>>>
>>> The new helper functions check for ENOENT to return NULL for optional
>>> muxes.
>>>
>>> Commit e153fdea9db04 ("phy: can-transceiver: Re-instate "mux-states"
>>> property presence check") noted that "mux_get() always prints an error
>>> message in case of an error, including when the property is not present,
>>> confusing the user."
>>>
>>> The first error message covers the case that a mux name is not matched
>>> in dt. This is removed as the returned error code (-ENOENT) is clear.
>>>
>>> The second error message is based on of_parse_phandle_with_args return
>>> value. In case mux description is missing from DT, it returns -ENOENT.
>>> Print error message only for other error codes.
>>>
>>> This ensures that the new helper functions will not confuse the user
>>> either.
>>>
>>> Signed-off-by: Josua Mayer <josua@solid-run.com>
>> Thanks for your patch!
>>
>>> --- a/drivers/mux/core.c
>>> +++ b/drivers/mux/core.c
>>> @@ -542,11 +542,8 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
>>> else
>>> 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);
>>> - return ERR_PTR(index);
>>> - }
>>> + if (index < 0)
>>> + return ERR_PTR(-ENOENT);
>>> }
>>>
>>> if (state)
>>> @@ -558,8 +555,10 @@ 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 (ret != -ENOENT)
>> I think the non-optional variant should still print an error message in
>> case of -ENOENT, else this has to be duplicated in all drivers using it.
>>
>> This is typically handled by having a non-printing core helper function,
>> and having printing non-optional, and non-printing/ignoring optional wrappers
>> around the former.
> I would prefer letting drivers use dev_err_probe.
> Silent helper functions can more easily share code between them ...
>
> If this is a strong preference I can rework the error behaviour and modify
> the relevant mux_state_get and mux_control_get.
Thinking further ... The main issue is that the core mux_get functio is quite
verbose printing message for each error condition.

Now I have silenced the ENOENT case - because it should be silent for optional
muxes.

So ... actually I propose to update direct callers of mux_get function,
and re-add error message specifically for non-optional variants in case of ENOENT.

>
>>> + dev_err(dev, "%pOF: failed to get mux-%s %s(%i)\n",
>>> + np, state ? "state" : "control",
>>> + mux_name ?: "", index);
>>> return ERR_PTR(ret);
>>> }
>>>
>> Gr{oetje,eeting}s,
>>
>> Geert
> >