Re: [RFC v2 2/5] pinctrl: add generic functions + pins mapper

[Conor Dooley <conor@kernel.org>]

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

On Fri, Dec 26, 2025 at 10:29:31AM +0100, Linus Walleij wrote:
> Hi Conor,
> 
> sorry for being slow in reviews!
> 
> On Thu, Nov 27, 2025 at 11:58 AM Conor Dooley <conor@kernel.org> wrote:
> 
> > +config GENERIC_PINCTRL_BELLS_AND_WHISTLES
> 
> Interesting name :D
> 
> A bit like GENERIC_PINCTRL_LOCK_STOCK_AND_BARREL.
> 
> Have you considered simply GENERIC_PINCTRL?

Sure, I really didn't know if that was too "core" sounding for something
with almost nothing in it.

> > +obj-$(CONFIG_GENERIC_PINCTRL_BELLS_AND_WHISTLES) += pinctrl-generic.o
> 
> especially since the file is named like so...
> 
> > +/*
> > + * For platforms that do not define groups or functions in the driver, but
> > + * instead use the devicetree to describe them. This function will, unlike
> > + * pinconf_generic_dt_node_to_map() etc which rely on driver defined groups
> > + * and functions, create them in addition to parsing pinconf properties and
> > + * adding mappings.
> > + */
> > +int pinctrl_generic_pins_function_dt_node_to_map(struct pinctrl_dev *pctldev,
> > +                                                struct device_node *np,
> > +                                                struct pinctrl_map **maps,
> > +                                                unsigned int *num_maps)
> 
> All code looks fine.
> 
> There is just the philosophical question whether groups and functions should
> really be in the device tree, as they can obviously be statically defined and
> associated with the compatible.
> 
> I got so much pressure to do it this way because so many driver authors really
> wanted to keep this in the device tree (usually because it saves memory in the
> kernel) that I eventually caved in, and I have also been criticized for being to
> lenient on this because the compatible should suffice.
> 
> For me this is all fine, and with you submitting this I suppose even the DT
> maintainers think this is fine to keep groups and functions in the device
> tree, so there it is.

I think that you're mostly right though. There's a lot of situations
where the pinmux-node properties are used because they're easier than
putting functions and groups structs into a devicetree. They're boring
and annoying to write, when a pinmux property can do the job for you,
particularly on bigger devices where there may be lots of them.
My personal take would be that it comes down to complexity, and the
number of possible groups/functions that a pin can belong to. If that
number is very high then it makes sense I think to put it in dt, but if
there's only a small number than spelling them out in the driver I think
makes the most sense.

The pinmux property on the other hand, were I asked to review it as a
new property, I would reject. I'd probably push for not using pinmux, as
you did with my drivers, more. I think finding good examples to copy
from of it being done that way was why I didn't try. The docs that
explain about statically defined functions and groups, IIRC, kinda come
across as being a mach-foo thing, rather than intended for use with dt,
until you pointed out an example and I was able to link the two.
Based on what I've done for this series, I know that what that property
seeks to do can be done using pins and functions, rather than mixing
both up together into a single property - even worse, it's even a single
cell! I only used it myself in the earlier versions cos everyone else
does it and the infra existed to use it already. I've not looked at
every user, but every user I looked at used it to hold the function, not
some additional mux setting.

> I can merge this when it's out of RFC.

Sweet. I'll hopefully get it out in the next day or so.

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