Message-ID: <20251125-kindness-quicken-a70e3fdd0b8c@spud>
Date: Tue, 25 Nov 2025 17:47:42 +0000
From: Conor Dooley <conor@...nel.org>
To: Linus Walleij <linusw@...nel.org>
Cc: Linus Walleij <linus.walleij@...aro.org>,
	Conor Dooley <conor.dooley@...rochip.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
	devicetree@...r.kernel.org, Valentina.FernandezAlanis@...rochip.com,
	Bartosz Golaszewski <brgl@...ev.pl>
Subject: Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

On Tue, Nov 25, 2025 at 02:24:55PM +0100, Linus Walleij wrote:
> On Mon, Nov 24, 2025 at 8:14 PM Conor Dooley <conor@...nel.org> wrote:
> 
> > Started looking at this today too, and I found one of my sources of
> > confusion - the recently added helper which I think is confusingly
> > named. pinconf_generic_dt_node_to_map_pinmux() works differently to
> > pinconf_generic_dt_node_to_map(), because it only works if you have
> > the following setup:
> >
> > label: group {
> >         pinmux = <asjhdasjhlajskd>;
> >         config-item1;
> > };
> >
> > It does not work if you have:
> >
> > label: cfg {
> >         group1 {
> >                 pinmux = <dsjhlfka>;
> >                 config-item2;
> >         };
> >         group2 {
> >                 pinmux = <lsdjhaf>;
> >                 config-item1;
> >         };
> > };
> >
> > Specifically, the label must point to a group.
> > pinconf_generic_dt_node_to_map() does not work like this, it accepts both!
> 
> My feeling is that this is a bug, it should certainly handle configs
> with subnodes.
> 
> > I think the pinconf_generic_dt_node_to_map_pinmux() function should
> > actually be called pinconf_generic_dt_subnode_to_map_pinmux(), because
> > it operates at the same level as pinconf_generic_dt_subnode_to_map().
> 
> If it should be renamed, yes. But I think it should be fixed to
> parse subnodes, if present.
> 
> > Probably there should be a "real" pinconf_generic_dt_node_to_map() that
> > accepts both setups, since AFAICT it is pretty normal to have different
> > pins in a group that get different pinconf settings. Obviously
> 
> I think it should be fine to augment the existing function to handle
> both cases? (configs inside the current node or in subnodes
> alike) I don't see it causing any regressions.

I think the rename to add subnode actually makes sense, as it would
match how the code is broken down for pinconf_generic_dt_node_to_map(),
since the code in the current pinconf_generic_dt_node_to_map_pinmux()
would have to be executed in two different places. That'd be invisible
to the amlogic driver though, if done properly, since a
pinconf_generic_dt_node_to_map_pinmux() would still exist.

> 
> > label: cfg {
> >         group1 {
> >                 pinmux = <dsjhlfka>;
> >                 config-item2;
> >         };
> >         group2 {
> >                 pinmux = <lsdjhaf>;
> >                 config-item1;
> >         };
> > };
> >
> > peripheral {
> >         pinctrl-0 = <&label>;
> > }
> >
> > isn't the only way to do things, and the amlogic user of the current
> > setup could just go and do
> >
> > cfg {
> >         label1: group1 {
> >                 pinmux = <dsjhlfka>;
> >                 config-item2;
> >         };
> >         label2: group2 {
> >                 pinmux = <lsdjhaf>;
> >                 config-item1;
> >         };
> > };
> >
> > peripheral {
> >         pinctrl-0 = <&label1>, <&label2>;
> > }
> 
> That works too, because sometimes you want to pick a few
> different configs and collect them as one.
> 
> > Even then though, I'm not really sure that this function does what I
> > would have expected it to do, because it won't work as a replacement for
> > the custom dt_node_to_map in the spacemit k1 driver, for example, even
> > ignoring the requirement about how the labels are done in the dt. That's
> > because it doesn't actually do anything with the pinmux property, despite
> > that being in the name. It never actually interacts with the pinmux property
> > at all AFAICT!
> 
> I think it's unfortunate naming, people sometimes use the word
> "pinmux" as a DT property, sometimes to describe the subsystem,
> sometimes a part of the subsystem, sometimes anything related
> to pins.

I think I actually understand the naming now. It's called pinmux because
the existing function pinconf_generic_dt_node_to_map() doesn't support
pinmux, so this is the version you need for platforms that are using
pinmux. But then nothing about it limits it actually to pinmuxes, other
than arbitrary property checks, it could actually be used for my pins +
functions use-case, if I added similar code to amlogics in my probe
function that creates the functions and groups.

I still think the naming is poor though, and that it is not as generic as
it purports to be, since it depends on having the exact dt configuration
that amlogic has, and wouldn't work for spacemit that use the first
multi-group example that I gave above. I'd be inclined to say that it
should be shunted back to the amlogic driver, to avoid baiting people
into trying to use it because of the label position problem, the fact
that it requires parsing the dt twice (the first time in probe to
generate the groups/functions), and because the code uses the parent
of node as the pinmux function. IOW, it needs:

pinctrl@bla {

	func-foo {
		label: group-default {
			pinmuxes = <lskdf>;
		};
	};
};

and couldn't be used somewhere like:

pinctrl@bla {

	label: group-default {
		pinmuxes = <lskdf>;
	};
};

Probably there's very few places where this second example makes sense
(cos it requires all pins in a group to always have the same config
parameters, but I could definitely see that being possible.

So ye, proposing moving it back from whence it came, rather than
actually making it work for both label positions.

> I know I should perhaps have shepherded this better :/

idk, I think this is the usual "creating something generic but with only
one user" problem. Hard to know if it actually is generic at all.

> > It seems to depend on aml_pctl_parse_functions() being called
> > during probe which creates the groups and functions.
> > There's a weird warning about expecting a function parent node that seems
> > very amlogic specific too.
> >
> > In my eyes, there should be some generic dt_node_to_map helpers that
> > do it all for you on the "configuration entirely described in dt"
> > platforms because that's what stuff like spacemit k1 driver that do
> > this in their dt_node_to_map implementations.
> 
> I think you're right!

My dilemma now is what to call them and where to put them.
pinconf_generic_dt_node_to_map<something>() feels weird for something
that is also creating functions and groups, which I noticed because I
was having to include pinmux.h in pinconf.c so that I could call
pinmux_generic_add_function().

> 
> > I'm not gonna get in over my head, and just make a helper for doing the
> > pins + function thing that I need for my driver, but would you be open
> > to an equivalent for the pinmux scenario?
> 
> Yes!
> 
> > I'm thinking of something
> > that'd work for both the amlogic platform and for the spacemit k1.
> 
> That's a good start!
> 
> Yours,
> Linus Walleij

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists