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