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

[Linus Walleij <linusw@kernel.org>]

On Mon, Nov 24, 2025 at 8:14 PM Conor Dooley <conor@kernel.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.

> 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 know I should perhaps have shepherded this better :/

> 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!

> 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