From: Conor Dooley <conor.dooley@microchip.com>

Hey Linus, or whoever else,

Working on some pinctrl drivers for my devices, and I had two questions,
so I am sending this as an RFC in the hopes of getting an answer before
progressing further with the third pin controller on the platform.

Firstly, both of the drivers I have produced so far have only pinmuxing
functions and no pincfg role - they only determine internal routing in
the SoC. I've got an identical dt_node_to_map implementation in both
drivers, as all they are doing is populating the pin and mux bit setting
from dt. I used what the recently added spacemit k1 was doing as a
guide, but removed the loop since there's no pincfg stuff that can
differ between pins. I notice there are generic implementations of
dt_node_to_map but I didn't get them to work. Have I missed a trick here
(either on the dt side, or in the driver) that would let me use a generic
function instead of having my own implementation, either in the driver
or in how I've set up the dt side?
I got to this point by partially writing the third driver first, based
on that spacemit k1 driver's approach, so I might've blinded myself to
the correct/simple approach to things as a result of having to handle
the pincfg stuff etc in dt_node_to_map there.

Secondly, particularly if there's not some neat way to simplify things
in dt_node_to_map, should I merge the two drivers, at least partially?
They're effectively only different in their pinctrl_pin_desc and how the
regmap is generated in probe (from a syscon regmap versus
regmap_init_mmio). I've kept the bindings apart, as despite similarity,
they're really only similar due to simplicity.

None of this is in any sort of final state, it's either WIP in and off
itself or depends on other stuff that's not yet accepted, so please
excuse any build warnings etc.

Cheers,
Conor.

CC: Linus Walleij <linus.walleij@linaro.org>
CC: Rob Herring <robh@kernel.org>
CC: Krzysztof Kozlowski <krzk+dt@kernel.org>
CC: linux-kernel@vger.kernel.org
CC: linux-gpio@vger.kernel.org
CC: devicetree@vger.kernel.org

Conor Dooley (5):
  dt-bindings: pinctrl: add polarfire soc iomux0 pinmux
  dt-bindings: pinctrl: add pic64gx "gpio2" pinmux
  pinctrl: add polarfire soc iomux0 pinmux driver
  pinctrl: add pic64gx "gpio2" pinmux driver
  riscv: dts: microchip: add pinctrl nodes for iomux0

 .../microchip,mpfs-pinctrl-iomux0.yaml        |  77 +++++
 .../microchip,pic64gx-pinctrl-gpio2.yaml      |  74 +++++
 .../microchip,mpfs-mss-top-sysreg.yaml        |  17 +-
 .../dts/microchip/mpfs-icicle-kit-fabric.dtsi |  56 ++++
 .../boot/dts/microchip/mpfs-icicle-kit.dts    |   1 -
 .../boot/dts/microchip/mpfs-pinctrl.dtsi      | 117 ++++++++
 arch/riscv/boot/dts/microchip/mpfs.dtsi       |   9 +
 drivers/pinctrl/Kconfig                       |  14 +
 drivers/pinctrl/Makefile                      |   2 +
 drivers/pinctrl/pinctrl-mpfs-iomux0.c         | 252 ++++++++++++++++
 drivers/pinctrl/pinctrl-pic64gx-gpio2.c       | 283 ++++++++++++++++++
 11 files changed, 900 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,mpfs-pinctrl-iomux0.yaml
 create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,pic64gx-pinctrl-gpio2.yaml
 create mode 100644 arch/riscv/boot/dts/microchip/mpfs-pinctrl.dtsi
 create mode 100644 drivers/pinctrl/pinctrl-mpfs-iomux0.c
 create mode 100644 drivers/pinctrl/pinctrl-pic64gx-gpio2.c

-- 
2.47.3

Hi Conor,

thanks for your patches!

looking at the drivers it appears to be trying extensively to make use
of the pinmux = <>; property to mux entire groups of pins.

pinmux = <nn>; is supposed to mux *one* pin per group, not entire
groups of pins from one property. See
Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml:

  The pinmux property accepts an array of pinmux groups, each of them describing
  a single pin multiplexing configuration.

  pincontroller {
    state_0_node_a {
      pinmux = <PINMUX_GROUP>, <PINMUX_GROUP>, ...;
    };
  };

So e.g. when you do this:

       spi0_mssio: spi0-mssio-pins {
         pinmux = <MPFS_PINFUNC(0, 0)>;
       };

We all know SPI uses more than one pin so this is clearly abusing
the pinmux property.

It is unfortunate that so many drivers now use this "mux one pin
individually" concept that we cannot see the diversity of pin
controllers.

I cannot recommend using the pinmux property for this SoC.

What you need to do is to define the actual pins and groups
that you have.

Look for example at
Documentation/devicetree/bindings/pinctrl/cortina,gemini-pinctrl.txt
drivers/pinctrl/pinctrl-gemini.c
arch/arm/boot/dts/gemini/gemini.dtsi

This is another SoC that muxes pins in groups, not in single per-pin
settings.

Notice that the driver in this case enumerates and registers all 323
pins on the package! This is done because some of the groups
are mutually exclusive and this way the pin control framework
will do its job to detect collisions between pin groups and disallow
this, and that is what pin control is supposed to be doing.

I.e. do not orient your design around which registers and settings
you have, and do not model your driver around that, instead
model the driver around which actual pins exist on the physical
component, how these are sorted into groups, how the groups
are related to function (such as the group of SPI pins being
related to the spi function) and define these pins, groups
and functions in your driver.

Yours,
Linus Walleij

On Wed, Oct 01, 2025 at 01:29:01PM +0200, Linus Walleij wrote:
> Hi Conor,
> 
> thanks for your patches!
> 
> looking at the drivers it appears to be trying extensively to make use
> of the pinmux = <>; property to mux entire groups of pins.
> 
> pinmux = <nn>; is supposed to mux *one* pin per group, not entire
> groups of pins from one property. See
> Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml:
> 
>   The pinmux property accepts an array of pinmux groups, each of them describing
>   a single pin multiplexing configuration.
> 
>   pincontroller {
>     state_0_node_a {
>       pinmux = <PINMUX_GROUP>, <PINMUX_GROUP>, ...;
>     };
>   };
> 
> So e.g. when you do this:
> 
>        spi0_mssio: spi0-mssio-pins {
>          pinmux = <MPFS_PINFUNC(0, 0)>;
>        };
> 
> We all know SPI uses more than one pin so this is clearly abusing
> the pinmux property.
> 
> It is unfortunate that so many drivers now use this "mux one pin
> individually" concept that we cannot see the diversity of pin
> controllers.
> 
> I cannot recommend using the pinmux property for this SoC.
> 
> What you need to do is to define the actual pins and groups
> that you have.
> 
> Look for example at
> Documentation/devicetree/bindings/pinctrl/cortina,gemini-pinctrl.txt
> drivers/pinctrl/pinctrl-gemini.c
> arch/arm/boot/dts/gemini/gemini.dtsi
> 
> This is another SoC that muxes pins in groups, not in single per-pin
> settings.

This looks like something that the "gpio2" stuff could definitely go to,
since it covers multiple functions trying to access the same pin. Do you
have an "approved" example for a more demultiplexer case, where the
contention is about which of multiple possible pins (or pin analogues)
an IO from a particular block must be routed to?

> Notice that the driver in this case enumerates and registers all 323
> pins on the package! This is done because some of the groups
> are mutually exclusive and this way the pin control framework
> will do its job to detect collisions between pin groups and disallow
> this, and that is what pin control is supposed to be doing.

In that case, the mutual exclusion would be that a function can only be
routed to one "pin", but there's no concern about multiple functions
being routed to any given "pin".

> 
> I.e. do not orient your design around which registers and settings
> you have, and do not model your driver around that, instead
> model the driver around which actual pins exist on the physical
> component, how these are sorted into groups, how the groups
> are related to function (such as the group of SPI pins being
> related to the spi function) and define these pins, groups
> and functions in your driver.
> 
> Yours,
> Linus Walleij

On Wed, Oct 01, 2025 at 05:15:07PM +0100, Conor Dooley wrote:
> On Wed, Oct 01, 2025 at 01:29:01PM +0200, Linus Walleij wrote:
> > Hi Conor,
> > 
> > thanks for your patches!
> > 
> > looking at the drivers it appears to be trying extensively to make use
> > of the pinmux = <>; property to mux entire groups of pins.
> > 
> > pinmux = <nn>; is supposed to mux *one* pin per group, not entire
> > groups of pins from one property. See
> > Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml:
> > 
> >   The pinmux property accepts an array of pinmux groups, each of them describing
> >   a single pin multiplexing configuration.
> > 
> >   pincontroller {
> >     state_0_node_a {
> >       pinmux = <PINMUX_GROUP>, <PINMUX_GROUP>, ...;
> >     };
> >   };
> > 
> > So e.g. when you do this:
> > 
> >        spi0_mssio: spi0-mssio-pins {
> >          pinmux = <MPFS_PINFUNC(0, 0)>;
> >        };
> > 
> > We all know SPI uses more than one pin so this is clearly abusing
> > the pinmux property.
> > 
> > It is unfortunate that so many drivers now use this "mux one pin
> > individually" concept that we cannot see the diversity of pin
> > controllers.
> > 
> > I cannot recommend using the pinmux property for this SoC.
> > 
> > What you need to do is to define the actual pins and groups
> > that you have.
> > 
> > Look for example at
> > Documentation/devicetree/bindings/pinctrl/cortina,gemini-pinctrl.txt
> > drivers/pinctrl/pinctrl-gemini.c
> > arch/arm/boot/dts/gemini/gemini.dtsi
> > 
> > This is another SoC that muxes pins in groups, not in single per-pin
> > settings.
> 
> This looks like something that the "gpio2" stuff could definitely go to,
> since it covers multiple functions trying to access the same pin. Do you
> have an "approved" example for a more demultiplexer case, where the
> contention is about which of multiple possible pins (or pin analogues)
> an IO from a particular block must be routed to?
> 
> > Notice that the driver in this case enumerates and registers all 323
> > pins on the package! This is done because some of the groups
> > are mutually exclusive and this way the pin control framework
> > will do its job to detect collisions between pin groups and disallow
> > this, and that is what pin control is supposed to be doing.
> 
> In that case, the mutual exclusion would be that a function can only be
> routed to one "pin", but there's no concern about multiple functions
> being routed to any given "pin".

So, what I ended up doing is moving the "gpio2" stuff to use
functions/groups as your gemini stuff does, so each function contains
one group containing all the pins it needs - except for the gpio
function which contains analogues for each of the function's groups.
I'll send a patchset next week when the merge window is closed, but for
now that driver is here:
https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=pinctrl&id=8c0fdf0c76fe99549d293894121d64300dc4057f
Unlike "gpio2", where each pin supports two mutually exclusive
functions and so fits nicely into the pins/groups/functions hierarchy,
for iomux0 each function has two mutually exclusive routings but there's
no contention over the "pins", the corresponding function is either
routed there or it is entirely unused. I implemented this by co-opting
the pin structure to really contain functions, with each appearing in
two groups, one per routing. Each function then contains those two
groups - I think that then takes advantage of the framework's collision
detection like you requested? It's here in case you care enough to take
a look before I send a proper patchset next week:
https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=pinctrl&id=5aec232c11bd45262fb2cfaf7994e3030fd6d947

Cheers,
Conor.

> 
> > 
> > I.e. do not orient your design around which registers and settings
> > you have, and do not model your driver around that, instead
> > model the driver around which actual pins exist on the physical
> > component, how these are sorted into groups, how the groups
> > are related to function (such as the group of SPI pins being
> > related to the spi function) and define these pins, groups
> > and functions in your driver.
> > 
> > Yours,
> > Linus Walleij

On Thu, Oct 9, 2025 at 5:55 PM Conor Dooley <conor@kernel.org> wrote:

> So, what I ended up doing is moving the "gpio2" stuff to use
> functions/groups as your gemini stuff does, so each function contains
> one group containing all the pins it needs - except for the gpio
> function which contains analogues for each of the function's groups.

I don't know exactly what you mean by this, but if it entails any
entanglement of the GPIO function with another function, then
there is the recent patch from Bartosz in commit
11aa02d6a9c222260490f952d041dec6d7f16a92
which makes it possible to give the pin control framework
an awareness of what a GPIO function is by reading hardware
properties, and that it is sometimes separate from other functions.

Yours,
Linus Walleij

On Mon, Oct 13, 2025 at 03:27:57PM +0200, Linus Walleij wrote:
> On Thu, Oct 9, 2025 at 5:55 PM Conor Dooley <conor@kernel.org> wrote:
> 
> > So, what I ended up doing is moving the "gpio2" stuff to use
> > functions/groups as your gemini stuff does, so each function contains
> > one group containing all the pins it needs - except for the gpio
> > function which contains analogues for each of the function's groups.
> 
> I don't know exactly what you mean by this, but if it entails any

All I meant is that the functions for non-gpio things contain a group
with the pins they need, up to 10 groups for 10 non-gpio functions, and
that the gpio function, since each pin can do gpio and exactly one other
function, contains 10 groups, all of which are identical to a group
already defined for the non-gpio function. That's instead of having one
huge group with all 32 pins.

> entanglement of the GPIO function with another function, then
> there is the recent patch from Bartosz in commit
> 11aa02d6a9c222260490f952d041dec6d7f16a92
> which makes it possible to give the pin control framework
> an awareness of what a GPIO function is by reading hardware
> properties, and that it is sometimes separate from other functions.

That is unrelated, but interesting. What I don't really understand from
the commit message itself is whether this is useful if the pinctrl
driver is not also acting as a gpiochip driver. In my case, the pinctrl
hardware is not capable of doing anything more than muxing functions,
and the gpio function I talk about means routing a "real" gpio
controller's IO to the pins controlled by the driver I am talking about.
The 2 in "gpio 2" refers to the specific controller.
The rest of that thread makes it seem like this is intended for some
qcom devices where the pinctrl hardware is also a gpiochip.

Cheers,
Conor.

On Mon, Oct 13, 2025 at 3:55 PM Conor Dooley <conor@kernel.org> wrote:

[me]
> > entanglement of the GPIO function with another function, then
> > there is the recent patch from Bartosz in commit
> > 11aa02d6a9c222260490f952d041dec6d7f16a92
> > which makes it possible to give the pin control framework
> > an awareness of what a GPIO function is by reading hardware
> > properties, and that it is sometimes separate from other functions.
>
> That is unrelated, but interesting. What I don't really understand from
> the commit message itself is whether this is useful if the pinctrl
> driver is not also acting as a gpiochip driver. In my case, the pinctrl
> hardware is not capable of doing anything more than muxing functions,
> and the gpio function I talk about means routing a "real" gpio
> controller's IO to the pins controlled by the driver I am talking about.
> The 2 in "gpio 2" refers to the specific controller.
> The rest of that thread makes it seem like this is intended for some
> qcom devices where the pinctrl hardware is also a gpiochip.

It's useful if you want to use the .strict setting on the pin
controller and implement the shortcut GPIO enablement functions
such as .gpio_request_enable, .gpio_disable_free
and .gpio_set_direction.

These are often preferred when using the pin control driver
as a "back-end" for a GPIO "front-end".

Yours,
Linus Walleij

On Wed, Oct 01, 2025 at 01:29:01PM +0200, Linus Walleij wrote:
> Hi Conor,
> 
> thanks for your patches!
> 
> looking at the drivers it appears to be trying extensively to make use
> of the pinmux = <>; property to mux entire groups of pins.
> 
> pinmux = <nn>; is supposed to mux *one* pin per group, not entire
> groups of pins from one property. See
> Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml:
> 
>   The pinmux property accepts an array of pinmux groups, each of them describing
>   a single pin multiplexing configuration.
> 
>   pincontroller {
>     state_0_node_a {
>       pinmux = <PINMUX_GROUP>, <PINMUX_GROUP>, ...;
>     };
>   };
> 
> So e.g. when you do this:
> 
>        spi0_mssio: spi0-mssio-pins {
>          pinmux = <MPFS_PINFUNC(0, 0)>;
>        };
> 
> We all know SPI uses more than one pin so this is clearly abusing
> the pinmux property.
> 
> It is unfortunate that so many drivers now use this "mux one pin
> individually" concept that we cannot see the diversity of pin
> controllers.
> 
> I cannot recommend using the pinmux property for this SoC.
> 
> What you need to do is to define the actual pins and groups
> that you have.
> 
> Look for example at
> Documentation/devicetree/bindings/pinctrl/cortina,gemini-pinctrl.txt
> drivers/pinctrl/pinctrl-gemini.c

Does this driver have a mistake in L2074, using ARRAY_SIZE(idegrps) for
dram?

> arch/arm/boot/dts/gemini/gemini.dtsi
> 
> This is another SoC that muxes pins in groups, not in single per-pin
> settings.
> 
> Notice that the driver in this case enumerates and registers all 323
> pins on the package! This is done because some of the groups
> are mutually exclusive and this way the pin control framework
> will do its job to detect collisions between pin groups and disallow
> this, and that is what pin control is supposed to be doing.
> 
> I.e. do not orient your design around which registers and settings
> you have, and do not model your driver around that, instead
> model the driver around which actual pins exist on the physical
> component, how these are sorted into groups, how the groups
> are related to function (such as the group of SPI pins being
> related to the spi function) and define these pins, groups
> and functions in your driver.
> 
> Yours,
> Linus Walleij