Re: [PATCH v2 4/6] pinctrl: add generic board-level pinctrl driver using mux framework

[Frank Li <Frank.li@nxp.com>]

On Fri, Feb 27, 2026 at 10:20:14AM +0100, Linus Walleij wrote:
> Hi Frank,
>
> thanks for your patch!
>
> On Thu, Feb 26, 2026 at 12:55 AM Frank Li <Frank.Li@nxp.com> wrote:
>
> > Many boards use on-board mux chips (often controlled by GPIOs from an I2C
> > expander) to switch shared signals between peripherals.
> >
> > Add a generic pinctrl driver built on top of the mux framework to
> > centralize mux handling and avoid probe ordering issues. Keep board-level
> > routing out of individual drivers and supports boot-time only mux
> > selection.
> >
> > Ensure correct probe ordering, especially when the GPIO expander is probed
> > later.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> (...)
>
> > +static int
> > +mux_pinmux_dt_node_to_map(struct pinctrl_dev *pctldev,
> > +                         struct device_node *np_config,
> > +                         struct pinctrl_map **map, unsigned int *num_maps)
> > +{
> > +       struct mux_pinctrl *mpctl = pinctrl_dev_get_drvdata(pctldev);
> > +       struct mux_pin_function *function;
> > +       struct device *dev = mpctl->dev;
> > +       const char **pgnames;
> > +       int selector;
> > +       int group;
> > +       int ret;
> > +
> > +       *map = devm_kcalloc(dev, 1, sizeof(**map), GFP_KERNEL);
> > +       if (!*map)
> > +               return -ENOMEM;
> > +
> > +       *num_maps = 0;
> > +
> > +       function = devm_kzalloc(dev, sizeof(*function), GFP_KERNEL);
> > +       if (!function) {
> > +               ret = -ENOMEM;
> > +               goto err_func;
> > +       }
> > +
> > +       pgnames = devm_kzalloc(dev, sizeof(*pgnames), GFP_KERNEL);
> > +       if (!pgnames) {
> > +               ret = -ENOMEM;
> > +               goto err_pgnames;
> > +       }
> > +
> > +       pgnames[0] = np_config->name;
> > +
> > +       guard(mutex)(&mpctl->lock);
> > +
> > +       selector = pinmux_generic_add_function(mpctl->pctl, np_config->name,
> > +                                              pgnames, 1, function);
> > +       if (selector < 0) {
> > +               ret = selector;
> > +               goto err_add_func;
> > +       }
> > +
> > +       group = pinctrl_generic_add_group(mpctl->pctl, np_config->name, NULL, 0, mpctl);
> > +       if (group < 0) {
> > +               ret = group;
> > +               goto err_add_group;
> > +       }
> > +
> > +       function->mux_state = devm_mux_state_get_from_np(pctldev->dev, NULL, np_config);
> > +       if (IS_ERR(function->mux_state)) {
> > +               ret = PTR_ERR(function->mux_state);
> > +               goto err_mux_state_get;
> > +       }
> > +
> > +       (*map)->type = PIN_MAP_TYPE_MUX_GROUP;
> > +       (*map)->data.mux.group = np_config->name;
> > +       (*map)->data.mux.function = np_config->name;
> > +
> > +       *num_maps = 1;
> > +
> > +       return 0;
> > +
> > +err_mux_state_get:
> > +       pinctrl_generic_remove_group(mpctl->pctl, group);
> > +err_add_group:
> > +       pinmux_generic_remove_function(mpctl->pctl, selector);
> > +err_add_func:
> > +       devm_kfree(dev, pgnames);
> > +err_pgnames:
> > +       devm_kfree(dev, function);
> > +err_func:
> > +       devm_kfree(dev, *map);
> > +
> > +       return ret;
> > +}
>
> This is so close to the pinctrl-internal helpers that you better work with
> those instead.
>
> Can't you just use pinctrl_generic_pins_function_dt_node_to_map()?
> It was added in the last merge window in
> commit 43722575e5cdcc6c457bfe81fae9c3ad343ea031
> "pinctrl: add generic functions + pins mapper"
>
> There are problems with the above, for example this is only called
> on the probe() path so you would not need any devm_*free calls,
> as you can see in the generic helpers.
>
> I think you need to look into using or extending the existing helpers for this,
>
> > +static void
> > +mux_pinmux_dt_free_map(struct pinctrl_dev *pctldev, struct pinctrl_map *map,
> > +                      unsigned int num_maps)
> > +{
> > +       struct mux_pinctrl *mpctl = pinctrl_dev_get_drvdata(pctldev);
> > +
> > +       devm_kfree(mpctl->dev, map);
> > +}
>
> Just use pinctrl_utils_free_map().
>
> > +static void mux_pinmux_release_mux(struct pinctrl_dev *pctldev,
> > +                                  unsigned int func_selector,
> > +                                  unsigned int group_selector)
> > +{
> > +       struct mux_pinctrl *mpctl = pinctrl_dev_get_drvdata(pctldev);
> > +       const struct function_desc *function;
> > +       struct mux_pin_function *func;
> > +
> > +       guard(mutex)(&mpctl->lock);
> > +
> > +       function = pinmux_generic_get_function(pctldev, func_selector);
> > +       func = function->data;
> > +
> > +       mux_state_deselect(func->mux_state);
> > +
> > +       mpctl->cur_select = -1;
> > +}
>
> As mentioned I have my doubts about this, explain why this hardware
> is so different that this is needed.
>

As board mux (uart and flexcan) exist, for example, only one of UART and
FlexCAN work.

when modprobe uart.ko,  mux_state_select called.

So flexcan driver can't get such mux as expected.

when remmod uart.ko, we need release mux_state, so flexcan driver can
get such resource.

Genernally, DT may only enouble one of UART or flexcan.

but insmod uart.ko
    rmmod uart.ko

    insmod uart.ko (here also need release previous's state at prevous rmmod).

Frank

> Other than that I like the concept!
>
> Yours,
> Linus Walleij