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

Conor Dooley
2026-01-15T17:56:01Z

[-- Attachment #1: Type: text/plain, Size: 3252 bytes --]

On Fri, Dec 26, 2025 at 10:40:07AM +0100, Linus Walleij wrote:
> On Thu, Nov 27, 2025 at 11:58 AM Conor Dooley <conor@kernel.org> 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?

Sure, no 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?

I'll make it do that if it's not already. Just didn't know if you were a
"select everything you use" kinda guy or didn't mind selects selecting.

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

Eh, I feel like sometimes a comment like this is just better than trying
to insert silly defines to unmagic the numbers.

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

Yeah, would be nice. My problem with it was more about the point in time
where this happens rather than doing it in the first place though.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]