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

[Josua Mayer <josua@solid-run.com>]

On 30/01/2026 12:28, Ulf Hansson wrote:
> On Thu, 29 Jan 2026 at 18:46, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Thu, 29 Jan 2026 at 17:06, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On Wed, 28 Jan 2026 at 15:46, 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:
>>>>
>>>> - mux_control_get_optional:
>>>>    Get a mux-control if specified in dt, return NULL otherwise.
>>>> - devm_mux_state_get_optional:
>>>>    Get a mux-state if specified in dt, return NULL otherwise.
>>>> - devm_mux_state_get_selected:
>>>>    Get and select a mux-state specified in dt, return error 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 take an extra argument
>>>> indicating whether the mux is optional.
>>>> In this case no error is printed, and NULL returned in case of ENOENT.
>>>>
>>>> Calling code is adapted to handle NULL return case, and to pass optional
>>>> argument as required.
>>>>
>>>> To support automatic deselect for _selected helper, a new structure is
>>>> created storing an exit pointer similar to clock core which is called on
>>>> release.
>>>>
>>>> To facilitate code sharing between optional/mandatory/selected helpers,
>>>> a new internal helper function is added to handle quiet (optional) and
>>>> verbose (mandatory) errors, as well as storing the correct callback for
>>>> devm release: __devm_mux_state_get
>>>>
>>>> Due to this structure devm_mux_state_get_*_selected can no longer print
>>>> a useful error message when select fails. Instead callers should print
>>>> errors where needed.
>>>>
>>>> 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. The second error message is based on of_parse_phandle_with_args
>>>> return value.
>>>>
>>>> In optional case no error is printed and NULL is returned.
>>>> This ensures that the new helper functions will not confuse the user
>>>> either.
>>>>
>>>> With the addition of optional helper functions it became clear that
>>>> drivers should compile and link even if CONFIG_MULTIPLEXER was not enabled.
>>>> Add stubs for all symbols exported by mux core.
>>>>
>>>> Signed-off-by: Josua Mayer <josua@solid-run.com>
>>>> ---
>>>>   drivers/mux/core.c           | 178 ++++++++++++++++++++++++++++++++++++-------
>>>>   include/linux/mux/consumer.h | 108 +++++++++++++++++++++++++-
>>>>   2 files changed, 253 insertions(+), 33 deletions(-)
>>>>
>>>> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
>>>> index a3840fe0995f..b01ec126caaf 100644
>>>> --- a/drivers/mux/core.c
>>>> +++ b/drivers/mux/core.c
>>> [...]
>>>
>>>>   static void devm_mux_state_release(struct device *dev, void *res)
>>>>   {
>>>> -       struct mux_state *mstate = *(struct mux_state **)res;
>>>> +       struct devm_mux_state_state *devm_state = res;
>>>>
>>>> -       mux_state_put(mstate);
>>>> +       if (devm_state->exit)
>>>> +               devm_state->exit(devm_state->mstate);
>>>> +
>>>> +       mux_state_put(devm_state->mstate);
>>>>   }
>>>>
>>>>   /**
>>>> - * devm_mux_state_get() - Get the mux-state for a device, with resource
>>>> - *                       management.
>>>> - * @dev: The device that needs a mux-control.
>>>> - * @mux_name: The name identifying the mux-control.
>>>> + * __devm_mux_state_get() - Get the optional mux-state for a device,
>>>> + *                         with resource management.
>>>> + * @dev: The device that needs a mux-state.
>>>> + * @mux_name: The name identifying the mux-state.
>>>> + * @optional: Whether to return NULL and silence errors when mux doesn't exist.
>>>> + * @init: Optional function pointer for mux-state object initialisation.
>>>> + * @exit: Optional function pointer for mux-state object cleanup on release.
>>>>    *
>>>>    * Return: Pointer to the mux-state, or an ERR_PTR with a negative errno.
>>>>    */
>>>> -struct mux_state *devm_mux_state_get(struct device *dev,
>>>> -                                    const char *mux_name)
>>>> +static struct mux_state *__devm_mux_state_get(struct device *dev, const char *mux_name,
>>>> +                                             bool optional,
>>>> +                                             int (*init)(struct mux_state *mstate),
>>>> +                                             int (*exit)(struct mux_state *mstate))
>>>>   {
>>>> -       struct mux_state **ptr, *mstate;
>>>> +       struct devm_mux_state_state *devm_state;
>>>> +       struct mux_state *mstate;
>>>> +       int ret;
>>>>
>>>> -       ptr = devres_alloc(devm_mux_state_release, sizeof(*ptr), GFP_KERNEL);
>>>> -       if (!ptr)
>>>> +       devm_state = devres_alloc(devm_mux_state_release, sizeof(*devm_state), GFP_KERNEL);
>>>> +       if (!devm_state)
>>>>                  return ERR_PTR(-ENOMEM);
>>>>
>>>> -       mstate = mux_state_get(dev, mux_name);
>>>> -       if (IS_ERR(mstate)) {
>>>> -               devres_free(ptr);
>>>> -               return mstate;
>>>> +       mstate = mux_state_get(dev, mux_name, optional);
>>>> +       if (IS_ERR_OR_NULL(mstate)) {
>>>> +               ret = PTR_ERR(mstate);
>>> Should this be PTR_ERR_OR_ZERO?
>> "mux_state_get() never returns NULL"
>> https://lore.kernel.org/202601221036.J0kR78Uw-lkp@intel.com
> By looking at the code in mux_state_get() and mux_get() that $subject
> patch changes, it intends to add a "bool optional" as in-parameter. If
> it's set, it looks like the intent is to allow returning NULL, which
> makes sense to me.
The intent behind "optional" parameter was to conditionally
silence certain error paths that would eventually lead to
at least the exported devm_* functions to return NULL.

I'll review my logic ...

>
> Anyway, there seems to be some additional smatch warnings to fix
> before this is ready to go.
>
> [...]
>
> Kind regards
> Uffe
>