Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Conor Dooley <conor@kernel.org>]

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

On Wed, Nov 19, 2025 at 01:08:26PM +0100, Linus Walleij wrote:

> On Wed, Nov 12, 2025 at 3:33 PM Conor Dooley <conor@kernel.org> wrote:

> > +static int mpfs_pinctrl_dt_node_to_map(struct pinctrl_dev *pctrl_dev, struct device_node *np,
> > +                                      struct pinctrl_map **maps, unsigned int *num_maps)
> 
> I saw in the cover letter that you wanted this to use more generic helpers.
> 
> If you see room for improvement of the generic code, do not hesitate.
> Doing a new driver is the only time when you actually have all these
> details in your head and can create good helpers.

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!
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().

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

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>;
}

if it never needs more than one set of configs so this isn't a bug in
the amlogic stuff, just something I found misleading while trying to
make my own helper.

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! 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'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? I'm thinking of something
that'd work for both the amlogic platform and for the spacemit k1.

Cheers,
Conor.

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