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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD++jLkehJNGxxQJV6vOA9xurBjwpYcL1ufYBQEOqRjNG-h-fg@mail.gmail.com>
Date: Tue, 25 Nov 2025 14:24:55 +0100
From: Linus Walleij <linusw@...nel.org>
To: Conor Dooley <conor@...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 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.

> 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