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

[Linus Walleij]

Hi Frank,

thanks for your patch!

On Thu, Feb 26, 2026 at 12:55 AM Frank Li <Frank.Li@xxxxxxx> 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@xxxxxxx>
(...)

> +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.

Other than that I like the concept!

Yours,
Linus Walleij