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

[Conor Dooley <conor@kernel.org>]

[-- 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 --]