Re: [RFC v2 3/5] pinctrl: add polarfire soc mssio pinctrl driver

[Linus Walleij]

On Thu, Nov 27, 2025 at 11:58 AM Conor Dooley <conor@xxxxxxxxxx> wrote:

>  drivers/pinctrl/Kconfig              |   7 +-
>  drivers/pinctrl/Makefile             |   1 +
>  drivers/pinctrl/pinctrl-mpfs-mssio.c | 750 +++++++++++++++++++++++++++

Time to move the drivers to drivers/pinctrl/microchip
before it becomes an overpopulation problem?

(The previous drivers can be moved in a separate patch.)


> +       select GENERIC_PINCTRL_GROUPS
> +       select GENERIC_PINMUX_FUNCTIONS
> +       select GENERIC_PINCTRL_BELLS_AND_WHISTLES

Just the bottom select will bring it all in, right?

> +static int mpfs_pinctrl_pin_to_iocfg_reg(unsigned int pin)
> +{
> +       u32 reg = MPFS_PINCTRL_IOCFG01_REG;
> +
> +       if (pin >= MPFS_PINCTRL_BANK2_START)
> +               reg += MPFS_PINCTRL_INTER_BANK_GAP;
> +
> +       // 2 pins per 32-bit register
> +       reg += (pin / 2) * 0x4;

It's helpful with these nice comments that ease the reading of the code
quite a bit.

> +static int mpfs_pinctrl_set_mux(struct pinctrl_dev *pctrl_dev, unsigned int fsel,
> +                               unsigned int gsel)
> +{
> +       struct mpfs_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
> +       const struct group_desc *group;
> +       const char **functions;
> +
> +       group = pinctrl_generic_get_group(pctrl_dev, gsel);
> +       if (!group)
> +               return -EINVAL;
> +
> +       functions = group->data;
> +
> +       for (int i = 0; i < group->grp.npins; i++) {
> +               u32 function;
> +
> +               //TODO @Linus my new function being actually generic means that
> +               // the mapping of function string to something the hardware
> +               // understands only happens at this point.
> +               // I think this is fine, because dt validation would whinge
> +               // about something invalid, but it's the "catch" with my approach.
> +               // The other option I considered was to provide a mapping
> +               // function pointer that the driver can populate, but I think
> +               // that's overkill.
> +               function = mpfs_pinctrl_function_map(functions[i]);
> +               if (function < 0) {
> +                       dev_err(pctrl->dev, "invalid function %s\n", functions[i]);
> +                       return function;
> +               }

This is fine with me.

Ideally I would like code that does a lot of string stacking and comparing
to be using Rust, but we cannot yet use that in core code so that is for
another day.

Yours,
Linus Walleij