[PATCH v3 0/3] Renesas R9A06G032 PINCTRL Driver

[PATCH v3 0/3] Renesas R9A06G032 PINCTRL Driver

[Phil Edworthy]

This implements the pinctrl driver for the RZ/N1 family of devices, including
the R9A06G032 (RZ/N1D) device.

This series was originally written by Michel Pollet whilst at Renesas, and I
have taken over this work.

Main changes:
v3:
 - Use standard DT props instead of proprietary ones.
 - Replace virtual pins used for MDIO muxing with extra funcs.
 - Use pinctrl_utils funcs to handle the maps.
 - Remove the dbg functions to keep things simple.
 - Change the way the functions are defined so it is easy to check
   against the hardware numbering.

v2:
 - Change to generic rzn1 family driver, instead of device specific.
 - Review comments fixed.
 - Fix error handling during probe

Phil Edworthy (3):
  dt-bindings: pinctrl: renesas,rzn1-pinctrl: documentation
  pinctrl: renesas: Renesas RZ/N1 pinctrl driver
  ARM: dts: r9a06g032: Add pinctrl node

 .../bindings/pinctrl/renesas,rzn1-pinctrl.txt | 129 +++
 arch/arm/boot/dts/r9a06g032.dtsi              |   8 +
 drivers/pinctrl/Kconfig                       |  10 +
 drivers/pinctrl/Makefile                      |   1 +
 drivers/pinctrl/pinctrl-rzn1.c                | 926 ++++++++++++++++++
 include/dt-bindings/pinctrl/rzn1-pinctrl.h    | 141 +++
 6 files changed, 1215 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,rzn1-pinctrl.txt
 create mode 100644 drivers/pinctrl/pinctrl-rzn1.c
 create mode 100644 include/dt-bindings/pinctrl/rzn1-pinctrl.h

-- 
2.17.1

[PATCH v3 1/3] dt-bindings: pinctrl: renesas,rzn1-pinctrl: documentation

[Phil Edworthy]

The Renesas RZ/N1 device family PINCTRL node description.

Based on a patch originally written by Michel Pollet at Renesas.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
v3:
 - Use standard bindings
 - Change the way the functions are defined so it is easy to check
   against the hardware numbering.
 - Add functions for the MDIO source peripheral instead of using
   virtual pins.

v2:
 - Change filename to generic rzn1, instead of device specific.
 - Add "renesas,rzn1-pinctrl" compatible fallback string.
 - Example register size corrected.
 - Typos fixed.
 - Changes suggested by Jacopo Mondi.
 - rzn1-pinctrl.h squashed into this as requested by Rob Herring.
---
 .../bindings/pinctrl/renesas,rzn1-pinctrl.txt | 129 ++++++++++++++++
 include/dt-bindings/pinctrl/rzn1-pinctrl.h    | 141 ++++++++++++++++++
 2 files changed, 270 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,rzn1-pinctrl.txt
 create mode 100644 include/dt-bindings/pinctrl/rzn1-pinctrl.h

diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,rzn1-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,rzn1-pinctrl.txt
new file mode 100644
index 000000000000..203131ed8d2a
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/renesas,rzn1-pinctrl.txt
@@ -0,0 +1,129 @@
+Renesas RZ/N1 SoC Pinctrl node description.
+
+Pin controller node
+-------------------
+Required properties:
+- compatible: SoC-specific compatible string "renesas,<soc-specific>-pinctrl"
+  followed by "renesas,rzn1-pinctrl" as fallback. The SoC-specific compatible
+  strings must be one of:
+	"renesas,r9a06g032-pinctrl" for RZ/N1D
+	"renesas,r9a06g033-pinctrl" for RZ/N1S
+- reg: Address base and length of the memory area where the pin controller
+  hardware is mapped to.
+- clocks: phandles for the clock, see the description of clock-names below.
+- clock-names: Contains the name of the clock:
+    "bus", the bus clock, sometimes described as pclk, for register accesses.
+
+Example:
+	pinctrl: pin-controller@40067000 {
+	    compatible = "renesas,r9a06g032-pinctrl", "renesas,rzn1-pinctrl";
+	    reg = <0x40067000 0x1000>, <0x51000000 0x480>;
+	    clocks = <&sysctrl R9A06G032_HCLK_PINCONFIG>;
+	    clock-names = "bus";
+	};
+
+Sub-nodes
+---------
+
+The child nodes of the pin controller node describe a pin multiplexing
+function or a GPIO controller alternatively.
+
+- Pin multiplexing sub-nodes:
+  A pin multiplexing sub-node describes how to configure a set of
+  (or a single) pin in some desired alternate function mode.
+  A single sub-node may define several pin configurations.
+  Please refer to pinctrl-bindings.txt to get to know more on generic
+  pin properties usage.
+
+  The allowed generic formats for a pin multiplexing sub-node are the
+  following ones:
+
+  node-1 {
+      pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
+      GENERIC_PINCONFIG;
+  };
+
+  node-2 {
+      sub-node-1 {
+          pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
+          GENERIC_PINCONFIG;
+      };
+
+      sub-node-2 {
+          pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
+          GENERIC_PINCONFIG;
+      };
+
+      ...
+
+      sub-node-n {
+          pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
+          GENERIC_PINCONFIG;
+      };
+  };
+
+  Use the second format when pins part of the same logical group need to have
+  different generic pin configuration flags applied.
+
+  Client sub-nodes shall refer to pin multiplexing sub-nodes using the phandle
+  of the most external one.
+
+  Eg.
+
+  client-1 {
+      ...
+      pinctrl-0 = <&node-1>;
+      ...
+  };
+
+  client-2 {
+      ...
+      pinctrl-0 = <&node-2>;
+      ...
+  };
+
+  Required properties:
+    - pinmux:
+      integer array representing pin number and pin multiplexing configuration.
+      When a pin has to be configured in alternate function mode, use this
+      property to identify the pin by its global index, and provide its
+      alternate function configuration number along with it.
+      When multiple pins are required to be configured as part of the same
+      alternate function they shall be specified as members of the same
+      argument list of a single "pinmux" property.
+      Integers values in the "pinmux" argument list are assembled as:
+      (PIN | MUX_FUNC << 8)
+      where PIN directly corresponds to the pl_gpio pin number and MUX_FUNC is
+      one of the alternate function identifiers defined in:
+      <include/dt-bindings/pinctrl/rzn1-pinctrl.h>
+      These identifiers collapse the IO Multiplex Configuration Level 1 and
+      Level 2 numbers that are detailed in the hardware reference manual into a
+      single number. The identifiers for Level 2 are simply offset by 10.
+      Additional identifiers are provided to specify the MDIO source peripheral.
+
+  Example:
+  A serial communication interface with a TX output pin and an RX input pin.
+
+  &pinctrl {
+	pins_uart0: pins_uart0 {
+		pinmux = <
+			RZN1_MUX(103, UART0_I)	/* UART0_TXD */
+			RZN1_MUX(104, UART0_I)	/* UART0_RXD */
+		>;
+	};
+  };
+
+  Example 2:
+  Here we set the pull up on the RXD pin of the UART.
+
+  &pinctrl {
+	pins_uart0: pins_uart0 {
+		pins_uart6_tx {
+			pinmux = <RZN1_MUX(103, UART0_I)>;	/* UART0_TXD */
+		};
+		pins_uart6_rx {
+			pinmux = <RZN1_MUX(104, UART0_I)>;	/* UART0_RXD */
+			bias-pull-up;
+		};
+	};
+  };
diff --git a/include/dt-bindings/pinctrl/rzn1-pinctrl.h b/include/dt-bindings/pinctrl/rzn1-pinctrl.h
new file mode 100644
index 000000000000..40369ee36e6a
--- /dev/null
+++ b/include/dt-bindings/pinctrl/rzn1-pinctrl.h
@@ -0,0 +1,141 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Defines macros and constants for Renesas RZ/N1 pin controller pin
+ * muxing functions.
+ */
+#ifndef __DT_BINDINGS_RZN1_PINCTRL_H
+#define __DT_BINDINGS_RZN1_PINCTRL_H
+
+#define RZN1_MUX(_gpio, _func) \
+	(((RZN1_FUNC_##_func) << 8) | (_gpio))
+
+/*
+ * Given the different levels of muxing on the SoC, it was decided to
+ * 'linearize' them into one numerical space. So mux level 1, 2 and the MDIO
+ * muxes are all represented by one single value.
+ *
+ * You can derive the hardware value pretty easily too, as
+ * 0...9   are Level 1
+ * 10...71 are Level 2. The Level 2 mux will be set to this
+ *         value - RZN1_FUNC_L2_OFFSET, and the Level 1 mux will be
+ *         set accordingly.
+ * 72...103 are for the 2 MDIO muxes.
+ */
+#define RZN1_FUNC_HIGHZ				0
+#define RZN1_FUNC_0L				1
+#define RZN1_FUNC_CLK_ETH_MII_RGMII_RMII	2
+#define RZN1_FUNC_CLK_ETH_NAND			3
+#define RZN1_FUNC_QSPI				4
+#define RZN1_FUNC_SDIO				5
+#define RZN1_FUNC_LCD				6
+#define RZN1_FUNC_LCD_E				7
+#define RZN1_FUNC_MSEBIM			8
+#define RZN1_FUNC_MSEBIS			9
+#define RZN1_FUNC_L2_OFFSET			10	/* I'm Special */
+
+#define RZN1_FUNC_HIGHZ1			(RZN1_FUNC_L2_OFFSET + 0)
+#define RZN1_FUNC_ETHERCAT			(RZN1_FUNC_L2_OFFSET + 1)
+#define RZN1_FUNC_SERCOS3			(RZN1_FUNC_L2_OFFSET + 2)
+#define RZN1_FUNC_SDIO_E			(RZN1_FUNC_L2_OFFSET + 3)
+#define RZN1_FUNC_ETH_MDIO			(RZN1_FUNC_L2_OFFSET + 4)
+#define RZN1_FUNC_ETH_MDIO_E1			(RZN1_FUNC_L2_OFFSET + 5)
+#define RZN1_FUNC_USB				(RZN1_FUNC_L2_OFFSET + 6)
+#define RZN1_FUNC_MSEBIM_E			(RZN1_FUNC_L2_OFFSET + 7)
+#define RZN1_FUNC_MSEBIS_E			(RZN1_FUNC_L2_OFFSET + 8)
+#define RZN1_FUNC_RSV				(RZN1_FUNC_L2_OFFSET + 9)
+#define RZN1_FUNC_RSV_E				(RZN1_FUNC_L2_OFFSET + 10)
+#define RZN1_FUNC_RSV_E1			(RZN1_FUNC_L2_OFFSET + 11)
+#define RZN1_FUNC_UART0_I			(RZN1_FUNC_L2_OFFSET + 12)
+#define RZN1_FUNC_UART0_I_E			(RZN1_FUNC_L2_OFFSET + 13)
+#define RZN1_FUNC_UART1_I			(RZN1_FUNC_L2_OFFSET + 14)
+#define RZN1_FUNC_UART1_I_E			(RZN1_FUNC_L2_OFFSET + 15)
+#define RZN1_FUNC_UART2_I			(RZN1_FUNC_L2_OFFSET + 16)
+#define RZN1_FUNC_UART2_I_E			(RZN1_FUNC_L2_OFFSET + 17)
+#define RZN1_FUNC_UART0				(RZN1_FUNC_L2_OFFSET + 18)
+#define RZN1_FUNC_UART0_E			(RZN1_FUNC_L2_OFFSET + 19)
+#define RZN1_FUNC_UART1				(RZN1_FUNC_L2_OFFSET + 20)
+#define RZN1_FUNC_UART1_E			(RZN1_FUNC_L2_OFFSET + 21)
+#define RZN1_FUNC_UART2				(RZN1_FUNC_L2_OFFSET + 22)
+#define RZN1_FUNC_UART2_E			(RZN1_FUNC_L2_OFFSET + 23)
+#define RZN1_FUNC_UART3				(RZN1_FUNC_L2_OFFSET + 24)
+#define RZN1_FUNC_UART3_E			(RZN1_FUNC_L2_OFFSET + 25)
+#define RZN1_FUNC_UART4				(RZN1_FUNC_L2_OFFSET + 26)
+#define RZN1_FUNC_UART4_E			(RZN1_FUNC_L2_OFFSET + 27)
+#define RZN1_FUNC_UART5				(RZN1_FUNC_L2_OFFSET + 28)
+#define RZN1_FUNC_UART5_E			(RZN1_FUNC_L2_OFFSET + 29)
+#define RZN1_FUNC_UART6				(RZN1_FUNC_L2_OFFSET + 30)
+#define RZN1_FUNC_UART6_E			(RZN1_FUNC_L2_OFFSET + 31)
+#define RZN1_FUNC_UART7				(RZN1_FUNC_L2_OFFSET + 32)
+#define RZN1_FUNC_UART7_E			(RZN1_FUNC_L2_OFFSET + 33)
+#define RZN1_FUNC_SPI0_M			(RZN1_FUNC_L2_OFFSET + 34)
+#define RZN1_FUNC_SPI0_M_E			(RZN1_FUNC_L2_OFFSET + 35)
+#define RZN1_FUNC_SPI1_M			(RZN1_FUNC_L2_OFFSET + 36)
+#define RZN1_FUNC_SPI1_M_E			(RZN1_FUNC_L2_OFFSET + 37)
+#define RZN1_FUNC_SPI2_M			(RZN1_FUNC_L2_OFFSET + 38)
+#define RZN1_FUNC_SPI2_M_E			(RZN1_FUNC_L2_OFFSET + 39)
+#define RZN1_FUNC_SPI3_M			(RZN1_FUNC_L2_OFFSET + 40)
+#define RZN1_FUNC_SPI3_M_E			(RZN1_FUNC_L2_OFFSET + 41)
+#define RZN1_FUNC_SPI4_S			(RZN1_FUNC_L2_OFFSET + 42)
+#define RZN1_FUNC_SPI4_S_E			(RZN1_FUNC_L2_OFFSET + 43)
+#define RZN1_FUNC_SPI5_S			(RZN1_FUNC_L2_OFFSET + 44)
+#define RZN1_FUNC_SPI5_S_E			(RZN1_FUNC_L2_OFFSET + 45)
+#define RZN1_FUNC_SGPIO0_M			(RZN1_FUNC_L2_OFFSET + 46)
+#define RZN1_FUNC_SGPIO1_M			(RZN1_FUNC_L2_OFFSET + 47)
+#define RZN1_FUNC_GPIO				(RZN1_FUNC_L2_OFFSET + 48)
+#define RZN1_FUNC_CAN				(RZN1_FUNC_L2_OFFSET + 49)
+#define RZN1_FUNC_I2C				(RZN1_FUNC_L2_OFFSET + 50)
+#define RZN1_FUNC_SAFE				(RZN1_FUNC_L2_OFFSET + 51)
+#define RZN1_FUNC_PTO_PWM			(RZN1_FUNC_L2_OFFSET + 52)
+#define RZN1_FUNC_PTO_PWM1			(RZN1_FUNC_L2_OFFSET + 53)
+#define RZN1_FUNC_PTO_PWM2			(RZN1_FUNC_L2_OFFSET + 54)
+#define RZN1_FUNC_PTO_PWM3			(RZN1_FUNC_L2_OFFSET + 55)
+#define RZN1_FUNC_PTO_PWM4			(RZN1_FUNC_L2_OFFSET + 56)
+#define RZN1_FUNC_DELTA_SIGMA			(RZN1_FUNC_L2_OFFSET + 57)
+#define RZN1_FUNC_SGPIO2_M			(RZN1_FUNC_L2_OFFSET + 58)
+#define RZN1_FUNC_SGPIO3_M			(RZN1_FUNC_L2_OFFSET + 59)
+#define RZN1_FUNC_SGPIO4_S			(RZN1_FUNC_L2_OFFSET + 60)
+#define RZN1_FUNC_MAC_MTIP_SWITCH		(RZN1_FUNC_L2_OFFSET + 61)
+
+#define RZN1_FUNC_MDIO_OFFSET			(RZN1_FUNC_L2_OFFSET + 62)
+
+/* These are MDIO0 peripherals for the RZN1_FUNC_ETH_MDIO function */
+#define RZN1_FUNC_MDIO0_HIGHZ			(RZN1_FUNC_MDIO_OFFSET + 0)
+#define RZN1_FUNC_MDIO0_GMAC0			(RZN1_FUNC_MDIO_OFFSET + 1)
+#define RZN1_FUNC_MDIO0_GMAC1			(RZN1_FUNC_MDIO_OFFSET + 2)
+#define RZN1_FUNC_MDIO0_ECAT			(RZN1_FUNC_MDIO_OFFSET + 3)
+#define RZN1_FUNC_MDIO0_S3_MDIO0		(RZN1_FUNC_MDIO_OFFSET + 4)
+#define RZN1_FUNC_MDIO0_S3_MDIO1		(RZN1_FUNC_MDIO_OFFSET + 5)
+#define RZN1_FUNC_MDIO0_HWRTOS			(RZN1_FUNC_MDIO_OFFSET + 6)
+#define RZN1_FUNC_MDIO0_SWITCH			(RZN1_FUNC_MDIO_OFFSET + 7)
+/* These are MDIO0 peripherals for the RZN1_FUNC_ETH_MDIO_E1 function */
+#define RZN1_FUNC_MDIO0_E1_HIGHZ		(RZN1_FUNC_MDIO_OFFSET + 8)
+#define RZN1_FUNC_MDIO0_E1_GMAC0		(RZN1_FUNC_MDIO_OFFSET + 9)
+#define RZN1_FUNC_MDIO0_E1_GMAC1		(RZN1_FUNC_MDIO_OFFSET + 10)
+#define RZN1_FUNC_MDIO0_E1_ECAT			(RZN1_FUNC_MDIO_OFFSET + 11)
+#define RZN1_FUNC_MDIO0_E1_S3_MDIO0		(RZN1_FUNC_MDIO_OFFSET + 12)
+#define RZN1_FUNC_MDIO0_E1_S3_MDIO1		(RZN1_FUNC_MDIO_OFFSET + 13)
+#define RZN1_FUNC_MDIO0_E1_HWRTOS		(RZN1_FUNC_MDIO_OFFSET + 14)
+#define RZN1_FUNC_MDIO0_E1_SWITCH		(RZN1_FUNC_MDIO_OFFSET + 15)
+
+/* These are MDIO1 peripherals for the RZN1_FUNC_ETH_MDIO function */
+#define RZN1_FUNC_MDIO1_HIGHZ			(RZN1_FUNC_MDIO_OFFSET + 16)
+#define RZN1_FUNC_MDIO1_GMAC0			(RZN1_FUNC_MDIO_OFFSET + 17)
+#define RZN1_FUNC_MDIO1_GMAC1			(RZN1_FUNC_MDIO_OFFSET + 18)
+#define RZN1_FUNC_MDIO1_ECAT			(RZN1_FUNC_MDIO_OFFSET + 19)
+#define RZN1_FUNC_MDIO1_S3_MDIO0		(RZN1_FUNC_MDIO_OFFSET + 20)
+#define RZN1_FUNC_MDIO1_S3_MDIO1		(RZN1_FUNC_MDIO_OFFSET + 21)
+#define RZN1_FUNC_MDIO1_HWRTOS			(RZN1_FUNC_MDIO_OFFSET + 22)
+#define RZN1_FUNC_MDIO1_SWITCH			(RZN1_FUNC_MDIO_OFFSET + 23)
+/* These are MDIO1 peripherals for the RZN1_FUNC_ETH_MDIO_E1 function */
+#define RZN1_FUNC_MDIO1_E1_HIGHZ		(RZN1_FUNC_MDIO_OFFSET + 24)
+#define RZN1_FUNC_MDIO1_E1_GMAC0		(RZN1_FUNC_MDIO_OFFSET + 25)
+#define RZN1_FUNC_MDIO1_E1_GMAC1		(RZN1_FUNC_MDIO_OFFSET + 26)
+#define RZN1_FUNC_MDIO1_E1_ECAT			(RZN1_FUNC_MDIO_OFFSET + 27)
+#define RZN1_FUNC_MDIO1_E1_S3_MDIO0		(RZN1_FUNC_MDIO_OFFSET + 28)
+#define RZN1_FUNC_MDIO1_E1_S3_MDIO1		(RZN1_FUNC_MDIO_OFFSET + 29)
+#define RZN1_FUNC_MDIO1_E1_HWRTOS		(RZN1_FUNC_MDIO_OFFSET + 30)
+#define RZN1_FUNC_MDIO1_E1_SWITCH		(RZN1_FUNC_MDIO_OFFSET + 31)
+
+#define RZN1_FUNC_MAX				(RZN1_FUNC_MDIO_OFFSET + 32)
+
+#endif /* __DT_BINDINGS_RZN1_PINCTRL_H */
-- 
2.17.1

Re: [PATCH v3 1/3] dt-bindings: pinctrl: renesas,rzn1-pinctrl: documentation

[jacopo mondi]

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

Hi Phil,
   thanks for the update. This looks much better :)

On Mon, Sep 17, 2018 at 05:36:07PM +0100, Phil Edworthy wrote:
> The Renesas RZ/N1 device family PINCTRL node description.
>
> Based on a patch originally written by Michel Pollet at Renesas.
>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> ---
> v3:
>  - Use standard bindings
>  - Change the way the functions are defined so it is easy to check
>    against the hardware numbering.
>  - Add functions for the MDIO source peripheral instead of using
>    virtual pins.
>
> v2:
>  - Change filename to generic rzn1, instead of device specific.
>  - Add "renesas,rzn1-pinctrl" compatible fallback string.
>  - Example register size corrected.
>  - Typos fixed.
>  - Changes suggested by Jacopo Mondi.
>  - rzn1-pinctrl.h squashed into this as requested by Rob Herring.
> ---
>  .../bindings/pinctrl/renesas,rzn1-pinctrl.txt | 129 ++++++++++++++++
>  include/dt-bindings/pinctrl/rzn1-pinctrl.h    | 141 ++++++++++++++++++
>  2 files changed, 270 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,rzn1-pinctrl.txt
>  create mode 100644 include/dt-bindings/pinctrl/rzn1-pinctrl.h
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,rzn1-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,rzn1-pinctrl.txt
> new file mode 100644
> index 000000000000..203131ed8d2a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/renesas,rzn1-pinctrl.txt
> @@ -0,0 +1,129 @@
> +Renesas RZ/N1 SoC Pinctrl node description.
> +
> +Pin controller node
> +-------------------
> +Required properties:
> +- compatible: SoC-specific compatible string "renesas,<soc-specific>-pinctrl"
> +  followed by "renesas,rzn1-pinctrl" as fallback. The SoC-specific compatible
> +  strings must be one of:
> +	"renesas,r9a06g032-pinctrl" for RZ/N1D
> +	"renesas,r9a06g033-pinctrl" for RZ/N1S
> +- reg: Address base and length of the memory area where the pin controller
> +  hardware is mapped to.
> +- clocks: phandles for the clock, see the description of clock-names below.
> +- clock-names: Contains the name of the clock:
> +    "bus", the bus clock, sometimes described as pclk, for register accesses.

If that's the only clock, the clock name is optional. If you drop it,
then please mention that the only phandle in 'clocks' refers to the
"bus" clock.

> +
> +Example:
> +	pinctrl: pin-controller@40067000 {
> +	    compatible = "renesas,r9a06g032-pinctrl", "renesas,rzn1-pinctrl";
> +	    reg = <0x40067000 0x1000>, <0x51000000 0x480>;
> +	    clocks = <&sysctrl R9A06G032_HCLK_PINCONFIG>;
> +	    clock-names = "bus";
> +	};
> +
> +Sub-nodes
> +---------
> +
> +The child nodes of the pin controller node describe a pin multiplexing
> +function or a GPIO controller alternatively.
> +
> +- Pin multiplexing sub-nodes:
> +  A pin multiplexing sub-node describes how to configure a set of
> +  (or a single) pin in some desired alternate function mode.
> +  A single sub-node may define several pin configurations.
> +  Please refer to pinctrl-bindings.txt to get to know more on generic
> +  pin properties usage.
> +
> +  The allowed generic formats for a pin multiplexing sub-node are the
> +  following ones:
> +
> +  node-1 {
> +      pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
> +      GENERIC_PINCONFIG;
> +  };
> +
> +  node-2 {
> +      sub-node-1 {
> +          pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
> +          GENERIC_PINCONFIG;
> +      };
> +
> +      sub-node-2 {
> +          pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
> +          GENERIC_PINCONFIG;
> +      };
> +
> +      ...
> +
> +      sub-node-n {
> +          pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
> +          GENERIC_PINCONFIG;
> +      };
> +  };
> +
> +  Use the second format when pins part of the same logical group need to have
> +  different generic pin configuration flags applied.
> +
> +  Client sub-nodes shall refer to pin multiplexing sub-nodes using the phandle
> +  of the most external one.
> +
> +  Eg.
> +
> +  client-1 {
> +      ...
> +      pinctrl-0 = <&node-1>;
> +      ...
> +  };
> +
> +  client-2 {
> +      ...
> +      pinctrl-0 = <&node-2>;
> +      ...
> +  };
> +
> +  Required properties:
> +    - pinmux:
> +      integer array representing pin number and pin multiplexing configuration.
> +      When a pin has to be configured in alternate function mode, use this
> +      property to identify the pin by its global index, and provide its
> +      alternate function configuration number along with it.
> +      When multiple pins are required to be configured as part of the same
> +      alternate function they shall be specified as members of the same
> +      argument list of a single "pinmux" property.
> +      Integers values in the "pinmux" argument list are assembled as:
> +      (PIN | MUX_FUNC << 8)
> +      where PIN directly corresponds to the pl_gpio pin number and MUX_FUNC is
> +      one of the alternate function identifiers defined in:
> +      <include/dt-bindings/pinctrl/rzn1-pinctrl.h>

You have a macro for assembling pin and mux functions, do you think it
is worth mentioning it here?

> +      These identifiers collapse the IO Multiplex Configuration Level 1 and
> +      Level 2 numbers that are detailed in the hardware reference manual into a
> +      single number. The identifiers for Level 2 are simply offset by 10.
> +      Additional identifiers are provided to specify the MDIO source peripheral.

As we discussed offline, I don't see that much value in mentioning
details about the pinmuxing levels, as this is driver internal stuff
that reflects on which set of registers to use depending on the muxing
'level'. I understand though that as the SoC manual mentions levels,
you may want to refer to them. Up to you....

> +
> +  Example:
> +  A serial communication interface with a TX output pin and an RX input pin.
> +
> +  &pinctrl {
> +	pins_uart0: pins_uart0 {
> +		pinmux = <
> +			RZN1_MUX(103, UART0_I)	/* UART0_TXD */
> +			RZN1_MUX(104, UART0_I)	/* UART0_RXD */
> +		>;
> +	};
> +  };
> +
> +  Example 2:
> +  Here we set the pull up on the RXD pin of the UART.
> +
> +  &pinctrl {
> +	pins_uart0: pins_uart0 {
> +		pins_uart6_tx {
> +			pinmux = <RZN1_MUX(103, UART0_I)>;	/* UART0_TXD */
> +		};
> +		pins_uart6_rx {
> +			pinmux = <RZN1_MUX(104, UART0_I)>;	/* UART0_RXD */
> +			bias-pull-up;
> +		};
> +	};
> +  };

I like this version much more. With those minor issues clarified (up
to you to decide how to handle them) please add my:
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Cheers
   j


> diff --git a/include/dt-bindings/pinctrl/rzn1-pinctrl.h b/include/dt-bindings/pinctrl/rzn1-pinctrl.h
> new file mode 100644
> index 000000000000..40369ee36e6a
> --- /dev/null
> +++ b/include/dt-bindings/pinctrl/rzn1-pinctrl.h
> @@ -0,0 +1,141 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Defines macros and constants for Renesas RZ/N1 pin controller pin
> + * muxing functions.
> + */
> +#ifndef __DT_BINDINGS_RZN1_PINCTRL_H
> +#define __DT_BINDINGS_RZN1_PINCTRL_H
> +
> +#define RZN1_MUX(_gpio, _func) \
> +	(((RZN1_FUNC_##_func) << 8) | (_gpio))
> +
> +/*
> + * Given the different levels of muxing on the SoC, it was decided to
> + * 'linearize' them into one numerical space. So mux level 1, 2 and the MDIO
> + * muxes are all represented by one single value.
> + *
> + * You can derive the hardware value pretty easily too, as
> + * 0...9   are Level 1
> + * 10...71 are Level 2. The Level 2 mux will be set to this
> + *         value - RZN1_FUNC_L2_OFFSET, and the Level 1 mux will be
> + *         set accordingly.
> + * 72...103 are for the 2 MDIO muxes.
> + */
> +#define RZN1_FUNC_HIGHZ				0
> +#define RZN1_FUNC_0L				1
> +#define RZN1_FUNC_CLK_ETH_MII_RGMII_RMII	2
> +#define RZN1_FUNC_CLK_ETH_NAND			3
> +#define RZN1_FUNC_QSPI				4
> +#define RZN1_FUNC_SDIO				5
> +#define RZN1_FUNC_LCD				6
> +#define RZN1_FUNC_LCD_E				7
> +#define RZN1_FUNC_MSEBIM			8
> +#define RZN1_FUNC_MSEBIS			9
> +#define RZN1_FUNC_L2_OFFSET			10	/* I'm Special */
> +
> +#define RZN1_FUNC_HIGHZ1			(RZN1_FUNC_L2_OFFSET + 0)
> +#define RZN1_FUNC_ETHERCAT			(RZN1_FUNC_L2_OFFSET + 1)
> +#define RZN1_FUNC_SERCOS3			(RZN1_FUNC_L2_OFFSET + 2)
> +#define RZN1_FUNC_SDIO_E			(RZN1_FUNC_L2_OFFSET + 3)
> +#define RZN1_FUNC_ETH_MDIO			(RZN1_FUNC_L2_OFFSET + 4)
> +#define RZN1_FUNC_ETH_MDIO_E1			(RZN1_FUNC_L2_OFFSET + 5)
> +#define RZN1_FUNC_USB				(RZN1_FUNC_L2_OFFSET + 6)
> +#define RZN1_FUNC_MSEBIM_E			(RZN1_FUNC_L2_OFFSET + 7)
> +#define RZN1_FUNC_MSEBIS_E			(RZN1_FUNC_L2_OFFSET + 8)
> +#define RZN1_FUNC_RSV				(RZN1_FUNC_L2_OFFSET + 9)
> +#define RZN1_FUNC_RSV_E				(RZN1_FUNC_L2_OFFSET + 10)
> +#define RZN1_FUNC_RSV_E1			(RZN1_FUNC_L2_OFFSET + 11)
> +#define RZN1_FUNC_UART0_I			(RZN1_FUNC_L2_OFFSET + 12)
> +#define RZN1_FUNC_UART0_I_E			(RZN1_FUNC_L2_OFFSET + 13)
> +#define RZN1_FUNC_UART1_I			(RZN1_FUNC_L2_OFFSET + 14)
> +#define RZN1_FUNC_UART1_I_E			(RZN1_FUNC_L2_OFFSET + 15)
> +#define RZN1_FUNC_UART2_I			(RZN1_FUNC_L2_OFFSET + 16)
> +#define RZN1_FUNC_UART2_I_E			(RZN1_FUNC_L2_OFFSET + 17)
> +#define RZN1_FUNC_UART0				(RZN1_FUNC_L2_OFFSET + 18)
> +#define RZN1_FUNC_UART0_E			(RZN1_FUNC_L2_OFFSET + 19)
> +#define RZN1_FUNC_UART1				(RZN1_FUNC_L2_OFFSET + 20)
> +#define RZN1_FUNC_UART1_E			(RZN1_FUNC_L2_OFFSET + 21)
> +#define RZN1_FUNC_UART2				(RZN1_FUNC_L2_OFFSET + 22)
> +#define RZN1_FUNC_UART2_E			(RZN1_FUNC_L2_OFFSET + 23)
> +#define RZN1_FUNC_UART3				(RZN1_FUNC_L2_OFFSET + 24)
> +#define RZN1_FUNC_UART3_E			(RZN1_FUNC_L2_OFFSET + 25)
> +#define RZN1_FUNC_UART4				(RZN1_FUNC_L2_OFFSET + 26)
> +#define RZN1_FUNC_UART4_E			(RZN1_FUNC_L2_OFFSET + 27)
> +#define RZN1_FUNC_UART5				(RZN1_FUNC_L2_OFFSET + 28)
> +#define RZN1_FUNC_UART5_E			(RZN1_FUNC_L2_OFFSET + 29)
> +#define RZN1_FUNC_UART6				(RZN1_FUNC_L2_OFFSET + 30)
> +#define RZN1_FUNC_UART6_E			(RZN1_FUNC_L2_OFFSET + 31)
> +#define RZN1_FUNC_UART7				(RZN1_FUNC_L2_OFFSET + 32)
> +#define RZN1_FUNC_UART7_E			(RZN1_FUNC_L2_OFFSET + 33)
> +#define RZN1_FUNC_SPI0_M			(RZN1_FUNC_L2_OFFSET + 34)
> +#define RZN1_FUNC_SPI0_M_E			(RZN1_FUNC_L2_OFFSET + 35)
> +#define RZN1_FUNC_SPI1_M			(RZN1_FUNC_L2_OFFSET + 36)
> +#define RZN1_FUNC_SPI1_M_E			(RZN1_FUNC_L2_OFFSET + 37)
> +#define RZN1_FUNC_SPI2_M			(RZN1_FUNC_L2_OFFSET + 38)
> +#define RZN1_FUNC_SPI2_M_E			(RZN1_FUNC_L2_OFFSET + 39)
> +#define RZN1_FUNC_SPI3_M			(RZN1_FUNC_L2_OFFSET + 40)
> +#define RZN1_FUNC_SPI3_M_E			(RZN1_FUNC_L2_OFFSET + 41)
> +#define RZN1_FUNC_SPI4_S			(RZN1_FUNC_L2_OFFSET + 42)
> +#define RZN1_FUNC_SPI4_S_E			(RZN1_FUNC_L2_OFFSET + 43)
> +#define RZN1_FUNC_SPI5_S			(RZN1_FUNC_L2_OFFSET + 44)
> +#define RZN1_FUNC_SPI5_S_E			(RZN1_FUNC_L2_OFFSET + 45)
> +#define RZN1_FUNC_SGPIO0_M			(RZN1_FUNC_L2_OFFSET + 46)
> +#define RZN1_FUNC_SGPIO1_M			(RZN1_FUNC_L2_OFFSET + 47)
> +#define RZN1_FUNC_GPIO				(RZN1_FUNC_L2_OFFSET + 48)
> +#define RZN1_FUNC_CAN				(RZN1_FUNC_L2_OFFSET + 49)
> +#define RZN1_FUNC_I2C				(RZN1_FUNC_L2_OFFSET + 50)
> +#define RZN1_FUNC_SAFE				(RZN1_FUNC_L2_OFFSET + 51)
> +#define RZN1_FUNC_PTO_PWM			(RZN1_FUNC_L2_OFFSET + 52)
> +#define RZN1_FUNC_PTO_PWM1			(RZN1_FUNC_L2_OFFSET + 53)
> +#define RZN1_FUNC_PTO_PWM2			(RZN1_FUNC_L2_OFFSET + 54)
> +#define RZN1_FUNC_PTO_PWM3			(RZN1_FUNC_L2_OFFSET + 55)
> +#define RZN1_FUNC_PTO_PWM4			(RZN1_FUNC_L2_OFFSET + 56)
> +#define RZN1_FUNC_DELTA_SIGMA			(RZN1_FUNC_L2_OFFSET + 57)
> +#define RZN1_FUNC_SGPIO2_M			(RZN1_FUNC_L2_OFFSET + 58)
> +#define RZN1_FUNC_SGPIO3_M			(RZN1_FUNC_L2_OFFSET + 59)
> +#define RZN1_FUNC_SGPIO4_S			(RZN1_FUNC_L2_OFFSET + 60)
> +#define RZN1_FUNC_MAC_MTIP_SWITCH		(RZN1_FUNC_L2_OFFSET + 61)
> +
> +#define RZN1_FUNC_MDIO_OFFSET			(RZN1_FUNC_L2_OFFSET + 62)
> +
> +/* These are MDIO0 peripherals for the RZN1_FUNC_ETH_MDIO function */
> +#define RZN1_FUNC_MDIO0_HIGHZ			(RZN1_FUNC_MDIO_OFFSET + 0)
> +#define RZN1_FUNC_MDIO0_GMAC0			(RZN1_FUNC_MDIO_OFFSET + 1)
> +#define RZN1_FUNC_MDIO0_GMAC1			(RZN1_FUNC_MDIO_OFFSET + 2)
> +#define RZN1_FUNC_MDIO0_ECAT			(RZN1_FUNC_MDIO_OFFSET + 3)
> +#define RZN1_FUNC_MDIO0_S3_MDIO0		(RZN1_FUNC_MDIO_OFFSET + 4)
> +#define RZN1_FUNC_MDIO0_S3_MDIO1		(RZN1_FUNC_MDIO_OFFSET + 5)
> +#define RZN1_FUNC_MDIO0_HWRTOS			(RZN1_FUNC_MDIO_OFFSET + 6)
> +#define RZN1_FUNC_MDIO0_SWITCH			(RZN1_FUNC_MDIO_OFFSET + 7)
> +/* These are MDIO0 peripherals for the RZN1_FUNC_ETH_MDIO_E1 function */
> +#define RZN1_FUNC_MDIO0_E1_HIGHZ		(RZN1_FUNC_MDIO_OFFSET + 8)
> +#define RZN1_FUNC_MDIO0_E1_GMAC0		(RZN1_FUNC_MDIO_OFFSET + 9)
> +#define RZN1_FUNC_MDIO0_E1_GMAC1		(RZN1_FUNC_MDIO_OFFSET + 10)
> +#define RZN1_FUNC_MDIO0_E1_ECAT			(RZN1_FUNC_MDIO_OFFSET + 11)
> +#define RZN1_FUNC_MDIO0_E1_S3_MDIO0		(RZN1_FUNC_MDIO_OFFSET + 12)
> +#define RZN1_FUNC_MDIO0_E1_S3_MDIO1		(RZN1_FUNC_MDIO_OFFSET + 13)
> +#define RZN1_FUNC_MDIO0_E1_HWRTOS		(RZN1_FUNC_MDIO_OFFSET + 14)
> +#define RZN1_FUNC_MDIO0_E1_SWITCH		(RZN1_FUNC_MDIO_OFFSET + 15)
> +
> +/* These are MDIO1 peripherals for the RZN1_FUNC_ETH_MDIO function */
> +#define RZN1_FUNC_MDIO1_HIGHZ			(RZN1_FUNC_MDIO_OFFSET + 16)
> +#define RZN1_FUNC_MDIO1_GMAC0			(RZN1_FUNC_MDIO_OFFSET + 17)
> +#define RZN1_FUNC_MDIO1_GMAC1			(RZN1_FUNC_MDIO_OFFSET + 18)
> +#define RZN1_FUNC_MDIO1_ECAT			(RZN1_FUNC_MDIO_OFFSET + 19)
> +#define RZN1_FUNC_MDIO1_S3_MDIO0		(RZN1_FUNC_MDIO_OFFSET + 20)
> +#define RZN1_FUNC_MDIO1_S3_MDIO1		(RZN1_FUNC_MDIO_OFFSET + 21)
> +#define RZN1_FUNC_MDIO1_HWRTOS			(RZN1_FUNC_MDIO_OFFSET + 22)
> +#define RZN1_FUNC_MDIO1_SWITCH			(RZN1_FUNC_MDIO_OFFSET + 23)
> +/* These are MDIO1 peripherals for the RZN1_FUNC_ETH_MDIO_E1 function */
> +#define RZN1_FUNC_MDIO1_E1_HIGHZ		(RZN1_FUNC_MDIO_OFFSET + 24)
> +#define RZN1_FUNC_MDIO1_E1_GMAC0		(RZN1_FUNC_MDIO_OFFSET + 25)
> +#define RZN1_FUNC_MDIO1_E1_GMAC1		(RZN1_FUNC_MDIO_OFFSET + 26)
> +#define RZN1_FUNC_MDIO1_E1_ECAT			(RZN1_FUNC_MDIO_OFFSET + 27)
> +#define RZN1_FUNC_MDIO1_E1_S3_MDIO0		(RZN1_FUNC_MDIO_OFFSET + 28)
> +#define RZN1_FUNC_MDIO1_E1_S3_MDIO1		(RZN1_FUNC_MDIO_OFFSET + 29)
> +#define RZN1_FUNC_MDIO1_E1_HWRTOS		(RZN1_FUNC_MDIO_OFFSET + 30)
> +#define RZN1_FUNC_MDIO1_E1_SWITCH		(RZN1_FUNC_MDIO_OFFSET + 31)
> +
> +#define RZN1_FUNC_MAX				(RZN1_FUNC_MDIO_OFFSET + 32)
> +
> +#endif /* __DT_BINDINGS_RZN1_PINCTRL_H */
> --
> 2.17.1
>

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

RE: [PATCH v3 1/3] dt-bindings: pinctrl: renesas,rzn1-pinctrl: documentation

[Phil Edworthy]

Hi Jacopo,

On 18 September 2018 10:27 jacopo mondi wrote:
> Hi Phil,
>    thanks for the update. This looks much better :)
> 
> On Mon, Sep 17, 2018 at 05:36:07PM +0100, Phil Edworthy wrote:
> > The Renesas RZ/N1 device family PINCTRL node description.
> >
> > Based on a patch originally written by Michel Pollet at Renesas.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > ---
> > v3:
> >  - Use standard bindings
> >  - Change the way the functions are defined so it is easy to check
> >    against the hardware numbering.
> >  - Add functions for the MDIO source peripheral instead of using
> >    virtual pins.
> >
> > v2:
> >  - Change filename to generic rzn1, instead of device specific.
> >  - Add "renesas,rzn1-pinctrl" compatible fallback string.
> >  - Example register size corrected.
> >  - Typos fixed.
> >  - Changes suggested by Jacopo Mondi.
> >  - rzn1-pinctrl.h squashed into this as requested by Rob Herring.
> > ---
> >  .../bindings/pinctrl/renesas,rzn1-pinctrl.txt | 129 ++++++++++++++++
> >  include/dt-bindings/pinctrl/rzn1-pinctrl.h    | 141 ++++++++++++++++++
> >  2 files changed, 270 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/pinctrl/renesas,rzn1-pinctrl.txt
> >  create mode 100644 include/dt-bindings/pinctrl/rzn1-pinctrl.h
> >
> > diff --git
> > a/Documentation/devicetree/bindings/pinctrl/renesas,rzn1-pinctrl.txt
> > b/Documentation/devicetree/bindings/pinctrl/renesas,rzn1-pinctrl.txt
> > new file mode 100644
> > index 000000000000..203131ed8d2a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pinctrl/renesas,rzn1-pinctrl.t
> > +++ xt
> > @@ -0,0 +1,129 @@
> > +Renesas RZ/N1 SoC Pinctrl node description.
> > +
> > +Pin controller node
> > +-------------------
> > +Required properties:
> > +- compatible: SoC-specific compatible string "renesas,<soc-specific>-
> pinctrl"
> > +  followed by "renesas,rzn1-pinctrl" as fallback. The SoC-specific
> > +compatible
> > +  strings must be one of:
> > +	"renesas,r9a06g032-pinctrl" for RZ/N1D
> > +	"renesas,r9a06g033-pinctrl" for RZ/N1S
> > +- reg: Address base and length of the memory area where the pin
> > +controller
> > +  hardware is mapped to.
> > +- clocks: phandles for the clock, see the description of clock-names below.
> > +- clock-names: Contains the name of the clock:
> > +    "bus", the bus clock, sometimes described as pclk, for register accesses.
> 
> If that's the only clock, the clock name is optional. If you drop it, then please
> mention that the only phandle in 'clocks' refers to the "bus" clock.
The driver currently specifies the "bus" name when calling devm_clk_get(), so
you need the clock-names.
If other changes are required, I will change the driver and update the binding
doc so they are not needed.

> > +
> > +Example:
> > +	pinctrl: pin-controller@40067000 {
> > +	    compatible = "renesas,r9a06g032-pinctrl", "renesas,rzn1-pinctrl";
> > +	    reg = <0x40067000 0x1000>, <0x51000000 0x480>;
> > +	    clocks = <&sysctrl R9A06G032_HCLK_PINCONFIG>;
> > +	    clock-names = "bus";
> > +	};
> > +
> > +Sub-nodes
> > +---------
> > +
> > +The child nodes of the pin controller node describe a pin
> > +multiplexing function or a GPIO controller alternatively.
> > +
> > +- Pin multiplexing sub-nodes:
> > +  A pin multiplexing sub-node describes how to configure a set of
> > +  (or a single) pin in some desired alternate function mode.
> > +  A single sub-node may define several pin configurations.
> > +  Please refer to pinctrl-bindings.txt to get to know more on generic
> > +  pin properties usage.
> > +
> > +  The allowed generic formats for a pin multiplexing sub-node are the
> > + following ones:
> > +
> > +  node-1 {
> > +      pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
> > +      GENERIC_PINCONFIG;
> > +  };
> > +
> > +  node-2 {
> > +      sub-node-1 {
> > +          pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
> > +          GENERIC_PINCONFIG;
> > +      };
> > +
> > +      sub-node-2 {
> > +          pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
> > +          GENERIC_PINCONFIG;
> > +      };
> > +
> > +      ...
> > +
> > +      sub-node-n {
> > +          pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
> > +          GENERIC_PINCONFIG;
> > +      };
> > +  };
> > +
> > +  Use the second format when pins part of the same logical group need
> > + to have  different generic pin configuration flags applied.
> > +
> > +  Client sub-nodes shall refer to pin multiplexing sub-nodes using
> > + the phandle  of the most external one.
> > +
> > +  Eg.
> > +
> > +  client-1 {
> > +      ...
> > +      pinctrl-0 = <&node-1>;
> > +      ...
> > +  };
> > +
> > +  client-2 {
> > +      ...
> > +      pinctrl-0 = <&node-2>;
> > +      ...
> > +  };
> > +
> > +  Required properties:
> > +    - pinmux:
> > +      integer array representing pin number and pin multiplexing
> configuration.
> > +      When a pin has to be configured in alternate function mode, use this
> > +      property to identify the pin by its global index, and provide its
> > +      alternate function configuration number along with it.
> > +      When multiple pins are required to be configured as part of the same
> > +      alternate function they shall be specified as members of the same
> > +      argument list of a single "pinmux" property.
> > +      Integers values in the "pinmux" argument list are assembled as:
> > +      (PIN | MUX_FUNC << 8)
> > +      where PIN directly corresponds to the pl_gpio pin number and
> MUX_FUNC is
> > +      one of the alternate function identifiers defined in:
> > +      <include/dt-bindings/pinctrl/rzn1-pinctrl.h>
> 
> You have a macro for assembling pin and mux functions, do you think it is
> worth mentioning it here?
Not sure it’s worth it, it's used in the examples anyway.

> > +      These identifiers collapse the IO Multiplex Configuration Level 1 and
> > +      Level 2 numbers that are detailed in the hardware reference manual
> into a
> > +      single number. The identifiers for Level 2 are simply offset by 10.
> > +      Additional identifiers are provided to specify the MDIO source
> peripheral.
> 
> As we discussed offline, I don't see that much value in mentioning details
> about the pinmuxing levels, as this is driver internal stuff that reflects on
> which set of registers to use depending on the muxing 'level'. I understand
> though that as the SoC manual mentions levels, you may want to refer to
> them. Up to you....
I'll leave it as is unless there are other comments.

> > +
> > +  Example:
> > +  A serial communication interface with a TX output pin and an RX input
> pin.
> > +
> > +  &pinctrl {
> > +	pins_uart0: pins_uart0 {
> > +		pinmux = <
> > +			RZN1_MUX(103, UART0_I)	/* UART0_TXD */
> > +			RZN1_MUX(104, UART0_I)	/* UART0_RXD */
> > +		>;
> > +	};
> > +  };
> > +
> > +  Example 2:
> > +  Here we set the pull up on the RXD pin of the UART.
> > +
> > +  &pinctrl {
> > +	pins_uart0: pins_uart0 {
> > +		pins_uart6_tx {
> > +			pinmux = <RZN1_MUX(103, UART0_I)>;	/*
> UART0_TXD */
> > +		};
> > +		pins_uart6_rx {
> > +			pinmux = <RZN1_MUX(104, UART0_I)>;	/*
> UART0_RXD */
> > +			bias-pull-up;
> > +		};
> > +	};
> > +  };
> 
> I like this version much more. With those minor issues clarified (up to you to
> decide how to handle them) please add my:
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Many thanks for your review!
Phil


> Cheers
>    j
> 
> 
> > diff --git a/include/dt-bindings/pinctrl/rzn1-pinctrl.h
> > b/include/dt-bindings/pinctrl/rzn1-pinctrl.h
> > new file mode 100644
> > index 000000000000..40369ee36e6a
> > --- /dev/null
> > +++ b/include/dt-bindings/pinctrl/rzn1-pinctrl.h
> > @@ -0,0 +1,141 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Defines macros and constants for Renesas RZ/N1 pin controller pin
> > + * muxing functions.
> > + */
> > +#ifndef __DT_BINDINGS_RZN1_PINCTRL_H
> > +#define __DT_BINDINGS_RZN1_PINCTRL_H
> > +
> > +#define RZN1_MUX(_gpio, _func) \
> > +	(((RZN1_FUNC_##_func) << 8) | (_gpio))
> > +
> > +/*
> > + * Given the different levels of muxing on the SoC, it was decided to
> > + * 'linearize' them into one numerical space. So mux level 1, 2 and
> > +the MDIO
> > + * muxes are all represented by one single value.
> > + *
> > + * You can derive the hardware value pretty easily too, as
> > + * 0...9   are Level 1
> > + * 10...71 are Level 2. The Level 2 mux will be set to this
> > + *         value - RZN1_FUNC_L2_OFFSET, and the Level 1 mux will be
> > + *         set accordingly.
> > + * 72...103 are for the 2 MDIO muxes.
> > + */
> > +#define RZN1_FUNC_HIGHZ				0
> > +#define RZN1_FUNC_0L				1
> > +#define RZN1_FUNC_CLK_ETH_MII_RGMII_RMII	2
> > +#define RZN1_FUNC_CLK_ETH_NAND			3
> > +#define RZN1_FUNC_QSPI				4
> > +#define RZN1_FUNC_SDIO				5
> > +#define RZN1_FUNC_LCD				6
> > +#define RZN1_FUNC_LCD_E				7
> > +#define RZN1_FUNC_MSEBIM			8
> > +#define RZN1_FUNC_MSEBIS			9
> > +#define RZN1_FUNC_L2_OFFSET			10	/* I'm Special
> */
> > +
> > +#define RZN1_FUNC_HIGHZ1
> 	(RZN1_FUNC_L2_OFFSET + 0)
> > +#define RZN1_FUNC_ETHERCAT
> 	(RZN1_FUNC_L2_OFFSET + 1)
> > +#define RZN1_FUNC_SERCOS3
> 	(RZN1_FUNC_L2_OFFSET + 2)
> > +#define RZN1_FUNC_SDIO_E
> 	(RZN1_FUNC_L2_OFFSET + 3)
> > +#define RZN1_FUNC_ETH_MDIO
> 	(RZN1_FUNC_L2_OFFSET + 4)
> > +#define RZN1_FUNC_ETH_MDIO_E1
> 	(RZN1_FUNC_L2_OFFSET + 5)
> > +#define RZN1_FUNC_USB
> 	(RZN1_FUNC_L2_OFFSET + 6)
> > +#define RZN1_FUNC_MSEBIM_E
> 	(RZN1_FUNC_L2_OFFSET + 7)
> > +#define RZN1_FUNC_MSEBIS_E
> 	(RZN1_FUNC_L2_OFFSET + 8)
> > +#define RZN1_FUNC_RSV
> 	(RZN1_FUNC_L2_OFFSET + 9)
> > +#define RZN1_FUNC_RSV_E
> 	(RZN1_FUNC_L2_OFFSET + 10)
> > +#define RZN1_FUNC_RSV_E1
> 	(RZN1_FUNC_L2_OFFSET + 11)
> > +#define RZN1_FUNC_UART0_I
> 	(RZN1_FUNC_L2_OFFSET + 12)
> > +#define RZN1_FUNC_UART0_I_E
> 	(RZN1_FUNC_L2_OFFSET + 13)
> > +#define RZN1_FUNC_UART1_I
> 	(RZN1_FUNC_L2_OFFSET + 14)
> > +#define RZN1_FUNC_UART1_I_E
> 	(RZN1_FUNC_L2_OFFSET + 15)
> > +#define RZN1_FUNC_UART2_I
> 	(RZN1_FUNC_L2_OFFSET + 16)
> > +#define RZN1_FUNC_UART2_I_E
> 	(RZN1_FUNC_L2_OFFSET + 17)
> > +#define RZN1_FUNC_UART0
> 	(RZN1_FUNC_L2_OFFSET + 18)
> > +#define RZN1_FUNC_UART0_E
> 	(RZN1_FUNC_L2_OFFSET + 19)
> > +#define RZN1_FUNC_UART1
> 	(RZN1_FUNC_L2_OFFSET + 20)
> > +#define RZN1_FUNC_UART1_E
> 	(RZN1_FUNC_L2_OFFSET + 21)
> > +#define RZN1_FUNC_UART2
> 	(RZN1_FUNC_L2_OFFSET + 22)
> > +#define RZN1_FUNC_UART2_E
> 	(RZN1_FUNC_L2_OFFSET + 23)
> > +#define RZN1_FUNC_UART3
> 	(RZN1_FUNC_L2_OFFSET + 24)
> > +#define RZN1_FUNC_UART3_E
> 	(RZN1_FUNC_L2_OFFSET + 25)
> > +#define RZN1_FUNC_UART4
> 	(RZN1_FUNC_L2_OFFSET + 26)
> > +#define RZN1_FUNC_UART4_E
> 	(RZN1_FUNC_L2_OFFSET + 27)
> > +#define RZN1_FUNC_UART5
> 	(RZN1_FUNC_L2_OFFSET + 28)
> > +#define RZN1_FUNC_UART5_E
> 	(RZN1_FUNC_L2_OFFSET + 29)
> > +#define RZN1_FUNC_UART6
> 	(RZN1_FUNC_L2_OFFSET + 30)
> > +#define RZN1_FUNC_UART6_E
> 	(RZN1_FUNC_L2_OFFSET + 31)
> > +#define RZN1_FUNC_UART7
> 	(RZN1_FUNC_L2_OFFSET + 32)
> > +#define RZN1_FUNC_UART7_E
> 	(RZN1_FUNC_L2_OFFSET + 33)
> > +#define RZN1_FUNC_SPI0_M
> 	(RZN1_FUNC_L2_OFFSET + 34)
> > +#define RZN1_FUNC_SPI0_M_E
> 	(RZN1_FUNC_L2_OFFSET + 35)
> > +#define RZN1_FUNC_SPI1_M
> 	(RZN1_FUNC_L2_OFFSET + 36)
> > +#define RZN1_FUNC_SPI1_M_E
> 	(RZN1_FUNC_L2_OFFSET + 37)
> > +#define RZN1_FUNC_SPI2_M
> 	(RZN1_FUNC_L2_OFFSET + 38)
> > +#define RZN1_FUNC_SPI2_M_E
> 	(RZN1_FUNC_L2_OFFSET + 39)
> > +#define RZN1_FUNC_SPI3_M
> 	(RZN1_FUNC_L2_OFFSET + 40)
> > +#define RZN1_FUNC_SPI3_M_E
> 	(RZN1_FUNC_L2_OFFSET + 41)
> > +#define RZN1_FUNC_SPI4_S
> 	(RZN1_FUNC_L2_OFFSET + 42)
> > +#define RZN1_FUNC_SPI4_S_E
> 	(RZN1_FUNC_L2_OFFSET + 43)
> > +#define RZN1_FUNC_SPI5_S
> 	(RZN1_FUNC_L2_OFFSET + 44)
> > +#define RZN1_FUNC_SPI5_S_E
> 	(RZN1_FUNC_L2_OFFSET + 45)
> > +#define RZN1_FUNC_SGPIO0_M
> 	(RZN1_FUNC_L2_OFFSET + 46)
> > +#define RZN1_FUNC_SGPIO1_M
> 	(RZN1_FUNC_L2_OFFSET + 47)
> > +#define RZN1_FUNC_GPIO
> 	(RZN1_FUNC_L2_OFFSET + 48)
> > +#define RZN1_FUNC_CAN
> 	(RZN1_FUNC_L2_OFFSET + 49)
> > +#define RZN1_FUNC_I2C
> 	(RZN1_FUNC_L2_OFFSET + 50)
> > +#define RZN1_FUNC_SAFE
> 	(RZN1_FUNC_L2_OFFSET + 51)
> > +#define RZN1_FUNC_PTO_PWM
> 	(RZN1_FUNC_L2_OFFSET + 52)
> > +#define RZN1_FUNC_PTO_PWM1
> 	(RZN1_FUNC_L2_OFFSET + 53)
> > +#define RZN1_FUNC_PTO_PWM2
> 	(RZN1_FUNC_L2_OFFSET + 54)
> > +#define RZN1_FUNC_PTO_PWM3
> 	(RZN1_FUNC_L2_OFFSET + 55)
> > +#define RZN1_FUNC_PTO_PWM4
> 	(RZN1_FUNC_L2_OFFSET + 56)
> > +#define RZN1_FUNC_DELTA_SIGMA
> 	(RZN1_FUNC_L2_OFFSET + 57)
> > +#define RZN1_FUNC_SGPIO2_M
> 	(RZN1_FUNC_L2_OFFSET + 58)
> > +#define RZN1_FUNC_SGPIO3_M
> 	(RZN1_FUNC_L2_OFFSET + 59)
> > +#define RZN1_FUNC_SGPIO4_S
> 	(RZN1_FUNC_L2_OFFSET + 60)
> > +#define RZN1_FUNC_MAC_MTIP_SWITCH
> 	(RZN1_FUNC_L2_OFFSET + 61)
> > +
> > +#define RZN1_FUNC_MDIO_OFFSET
> 	(RZN1_FUNC_L2_OFFSET + 62)
> > +
> > +/* These are MDIO0 peripherals for the RZN1_FUNC_ETH_MDIO function
> */
> > +#define RZN1_FUNC_MDIO0_HIGHZ
> 	(RZN1_FUNC_MDIO_OFFSET + 0)
> > +#define RZN1_FUNC_MDIO0_GMAC0
> 	(RZN1_FUNC_MDIO_OFFSET + 1)
> > +#define RZN1_FUNC_MDIO0_GMAC1
> 	(RZN1_FUNC_MDIO_OFFSET + 2)
> > +#define RZN1_FUNC_MDIO0_ECAT
> 	(RZN1_FUNC_MDIO_OFFSET + 3)
> > +#define RZN1_FUNC_MDIO0_S3_MDIO0
> 	(RZN1_FUNC_MDIO_OFFSET + 4)
> > +#define RZN1_FUNC_MDIO0_S3_MDIO1
> 	(RZN1_FUNC_MDIO_OFFSET + 5)
> > +#define RZN1_FUNC_MDIO0_HWRTOS
> 	(RZN1_FUNC_MDIO_OFFSET + 6)
> > +#define RZN1_FUNC_MDIO0_SWITCH
> 	(RZN1_FUNC_MDIO_OFFSET + 7)
> > +/* These are MDIO0 peripherals for the RZN1_FUNC_ETH_MDIO_E1
> function */
> > +#define RZN1_FUNC_MDIO0_E1_HIGHZ
> 	(RZN1_FUNC_MDIO_OFFSET + 8)
> > +#define RZN1_FUNC_MDIO0_E1_GMAC0
> 	(RZN1_FUNC_MDIO_OFFSET + 9)
> > +#define RZN1_FUNC_MDIO0_E1_GMAC1
> 	(RZN1_FUNC_MDIO_OFFSET + 10)
> > +#define RZN1_FUNC_MDIO0_E1_ECAT
> 	(RZN1_FUNC_MDIO_OFFSET + 11)
> > +#define RZN1_FUNC_MDIO0_E1_S3_MDIO0
> 	(RZN1_FUNC_MDIO_OFFSET + 12)
> > +#define RZN1_FUNC_MDIO0_E1_S3_MDIO1
> 	(RZN1_FUNC_MDIO_OFFSET + 13)
> > +#define RZN1_FUNC_MDIO0_E1_HWRTOS
> 	(RZN1_FUNC_MDIO_OFFSET + 14)
> > +#define RZN1_FUNC_MDIO0_E1_SWITCH
> 	(RZN1_FUNC_MDIO_OFFSET + 15)
> > +
> > +/* These are MDIO1 peripherals for the RZN1_FUNC_ETH_MDIO function
> */
> > +#define RZN1_FUNC_MDIO1_HIGHZ
> 	(RZN1_FUNC_MDIO_OFFSET + 16)
> > +#define RZN1_FUNC_MDIO1_GMAC0
> 	(RZN1_FUNC_MDIO_OFFSET + 17)
> > +#define RZN1_FUNC_MDIO1_GMAC1
> 	(RZN1_FUNC_MDIO_OFFSET + 18)
> > +#define RZN1_FUNC_MDIO1_ECAT
> 	(RZN1_FUNC_MDIO_OFFSET + 19)
> > +#define RZN1_FUNC_MDIO1_S3_MDIO0
> 	(RZN1_FUNC_MDIO_OFFSET + 20)
> > +#define RZN1_FUNC_MDIO1_S3_MDIO1
> 	(RZN1_FUNC_MDIO_OFFSET + 21)
> > +#define RZN1_FUNC_MDIO1_HWRTOS
> 	(RZN1_FUNC_MDIO_OFFSET + 22)
> > +#define RZN1_FUNC_MDIO1_SWITCH
> 	(RZN1_FUNC_MDIO_OFFSET + 23)
> > +/* These are MDIO1 peripherals for the RZN1_FUNC_ETH_MDIO_E1
> function */
> > +#define RZN1_FUNC_MDIO1_E1_HIGHZ
> 	(RZN1_FUNC_MDIO_OFFSET + 24)
> > +#define RZN1_FUNC_MDIO1_E1_GMAC0
> 	(RZN1_FUNC_MDIO_OFFSET + 25)
> > +#define RZN1_FUNC_MDIO1_E1_GMAC1
> 	(RZN1_FUNC_MDIO_OFFSET + 26)
> > +#define RZN1_FUNC_MDIO1_E1_ECAT
> 	(RZN1_FUNC_MDIO_OFFSET + 27)
> > +#define RZN1_FUNC_MDIO1_E1_S3_MDIO0
> 	(RZN1_FUNC_MDIO_OFFSET + 28)
> > +#define RZN1_FUNC_MDIO1_E1_S3_MDIO1
> 	(RZN1_FUNC_MDIO_OFFSET + 29)
> > +#define RZN1_FUNC_MDIO1_E1_HWRTOS
> 	(RZN1_FUNC_MDIO_OFFSET + 30)
> > +#define RZN1_FUNC_MDIO1_E1_SWITCH
> 	(RZN1_FUNC_MDIO_OFFSET + 31)
> > +
> > +#define RZN1_FUNC_MAX
> 	(RZN1_FUNC_MDIO_OFFSET + 32)
> > +
> > +#endif /* __DT_BINDINGS_RZN1_PINCTRL_H */
> > --
> > 2.17.1
> >

Re: [PATCH v3 1/3] dt-bindings: pinctrl: renesas,rzn1-pinctrl: documentation

[jacopo mondi]

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

Hi Phil,

On Tue, Sep 18, 2018 at 09:44:40AM +0000, Phil Edworthy wrote:
> Hi Jacopo,
>
> On 18 September 2018 10:27 jacopo mondi wrote:
> > Hi Phil,
> >    thanks for the update. This looks much better :)
> >
> > On Mon, Sep 17, 2018 at 05:36:07PM +0100, Phil Edworthy wrote:
> > > The Renesas RZ/N1 device family PINCTRL node description.
> > >
> > > Based on a patch originally written by Michel Pollet at Renesas.
> > >
> > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > > ---
> > > v3:
> > >  - Use standard bindings
> > >  - Change the way the functions are defined so it is easy to check
> > >    against the hardware numbering.
> > >  - Add functions for the MDIO source peripheral instead of using
> > >    virtual pins.
> > >
> > > v2:
> > >  - Change filename to generic rzn1, instead of device specific.
> > >  - Add "renesas,rzn1-pinctrl" compatible fallback string.
> > >  - Example register size corrected.
> > >  - Typos fixed.
> > >  - Changes suggested by Jacopo Mondi.
> > >  - rzn1-pinctrl.h squashed into this as requested by Rob Herring.
> > > ---
> > >  .../bindings/pinctrl/renesas,rzn1-pinctrl.txt | 129 ++++++++++++++++
> > >  include/dt-bindings/pinctrl/rzn1-pinctrl.h    | 141 ++++++++++++++++++
> > >  2 files changed, 270 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/pinctrl/renesas,rzn1-pinctrl.txt
> > >  create mode 100644 include/dt-bindings/pinctrl/rzn1-pinctrl.h
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/pinctrl/renesas,rzn1-pinctrl.txt
> > > b/Documentation/devicetree/bindings/pinctrl/renesas,rzn1-pinctrl.txt
> > > new file mode 100644
> > > index 000000000000..203131ed8d2a
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pinctrl/renesas,rzn1-pinctrl.t
> > > +++ xt
> > > @@ -0,0 +1,129 @@
> > > +Renesas RZ/N1 SoC Pinctrl node description.
> > > +
> > > +Pin controller node
> > > +-------------------
> > > +Required properties:
> > > +- compatible: SoC-specific compatible string "renesas,<soc-specific>-
> > pinctrl"
> > > +  followed by "renesas,rzn1-pinctrl" as fallback. The SoC-specific
> > > +compatible
> > > +  strings must be one of:
> > > +	"renesas,r9a06g032-pinctrl" for RZ/N1D
> > > +	"renesas,r9a06g033-pinctrl" for RZ/N1S
> > > +- reg: Address base and length of the memory area where the pin
> > > +controller
> > > +  hardware is mapped to.
> > > +- clocks: phandles for the clock, see the description of clock-names below.
> > > +- clock-names: Contains the name of the clock:
> > > +    "bus", the bus clock, sometimes described as pclk, for register accesses.
> >
> > If that's the only clock, the clock name is optional. If you drop it, then please
> > mention that the only phandle in 'clocks' refers to the "bus" clock.
> The driver currently specifies the "bus" name when calling devm_clk_get(), so
> you need the clock-names.

well, you need them, because the driver implementation requries that,
and you implemented it that way :) I won't argue on something this
stupid though, what you have here is fine

> If other changes are required, I will change the driver and update the binding
> doc so they are not needed.
>
> > > +
> > > +Example:
> > > +	pinctrl: pin-controller@40067000 {
> > > +	    compatible = "renesas,r9a06g032-pinctrl", "renesas,rzn1-pinctrl";
> > > +	    reg = <0x40067000 0x1000>, <0x51000000 0x480>;
> > > +	    clocks = <&sysctrl R9A06G032_HCLK_PINCONFIG>;
> > > +	    clock-names = "bus";
> > > +	};
> > > +
> > > +Sub-nodes
> > > +---------
> > > +
> > > +The child nodes of the pin controller node describe a pin
> > > +multiplexing function or a GPIO controller alternatively.

You have no gpio-controller support, so no GPIO configuration
sub-nodes.

> > > +
> > > +- Pin multiplexing sub-nodes:
> > > +  A pin multiplexing sub-node describes how to configure a set of
> > > +  (or a single) pin in some desired alternate function mode.
> > > +  A single sub-node may define several pin configurations.
> > > +  Please refer to pinctrl-bindings.txt to get to know more on generic
> > > +  pin properties usage.
> > > +
> > > +  The allowed generic formats for a pin multiplexing sub-node are the
> > > + following ones:
> > > +
> > > +  node-1 {
> > > +      pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
> > > +      GENERIC_PINCONFIG;
> > > +  };
> > > +
> > > +  node-2 {
> > > +      sub-node-1 {
> > > +          pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
> > > +          GENERIC_PINCONFIG;
> > > +      };
> > > +
> > > +      sub-node-2 {
> > > +          pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
> > > +          GENERIC_PINCONFIG;
> > > +      };
> > > +
> > > +      ...
> > > +
> > > +      sub-node-n {
> > > +          pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
> > > +          GENERIC_PINCONFIG;
> > > +      };
> > > +  };
> > > +
> > > +  Use the second format when pins part of the same logical group need
> > > + to have  different generic pin configuration flags applied.
> > > +
> > > +  Client sub-nodes shall refer to pin multiplexing sub-nodes using
> > > + the phandle  of the most external one.
> > > +
> > > +  Eg.
> > > +
> > > +  client-1 {
> > > +      ...
> > > +      pinctrl-0 = <&node-1>;
> > > +      ...
> > > +  };
> > > +
> > > +  client-2 {
> > > +      ...
> > > +      pinctrl-0 = <&node-2>;
> > > +      ...
> > > +  };
> > > +
> > > +  Required properties:
> > > +    - pinmux:
> > > +      integer array representing pin number and pin multiplexing
> > configuration.
> > > +      When a pin has to be configured in alternate function mode, use this
> > > +      property to identify the pin by its global index, and provide its
> > > +      alternate function configuration number along with it.
> > > +      When multiple pins are required to be configured as part of the same
> > > +      alternate function they shall be specified as members of the same
> > > +      argument list of a single "pinmux" property.
> > > +      Integers values in the "pinmux" argument list are assembled as:
> > > +      (PIN | MUX_FUNC << 8)
> > > +      where PIN directly corresponds to the pl_gpio pin number and
> > MUX_FUNC is
> > > +      one of the alternate function identifiers defined in:
> > > +      <include/dt-bindings/pinctrl/rzn1-pinctrl.h>
> >
> > You have a macro for assembling pin and mux functions, do you think it is
> > worth mentioning it here?
> Not sure it’s worth it, it's used in the examples anyway.
>
> > > +      These identifiers collapse the IO Multiplex Configuration Level 1 and
> > > +      Level 2 numbers that are detailed in the hardware reference manual
> > into a
> > > +      single number. The identifiers for Level 2 are simply offset by 10.
> > > +      Additional identifiers are provided to specify the MDIO source
> > peripheral.
> >
> > As we discussed offline, I don't see that much value in mentioning details
> > about the pinmuxing levels, as this is driver internal stuff that reflects on
> > which set of registers to use depending on the muxing 'level'. I understand
> > though that as the SoC manual mentions levels, you may want to refer to
> > them. Up to you....
> I'll leave it as is unless there are other comments.
>

I do, actually, after reviewing the driver too.
1) You should list which generic properties this peripheral supports.
2) The tiny GPIO controller thing mentioned above.

Thanks
   j

> > > +
> > > +  Example:
> > > +  A serial communication interface with a TX output pin and an RX input
> > pin.
> > > +
> > > +  &pinctrl {
> > > +	pins_uart0: pins_uart0 {
> > > +		pinmux = <
> > > +			RZN1_MUX(103, UART0_I)	/* UART0_TXD */
> > > +			RZN1_MUX(104, UART0_I)	/* UART0_RXD */
> > > +		>;
> > > +	};
> > > +  };
> > > +
> > > +  Example 2:
> > > +  Here we set the pull up on the RXD pin of the UART.
> > > +
> > > +  &pinctrl {
> > > +	pins_uart0: pins_uart0 {
> > > +		pins_uart6_tx {
> > > +			pinmux = <RZN1_MUX(103, UART0_I)>;	/*
> > UART0_TXD */
> > > +		};
> > > +		pins_uart6_rx {
> > > +			pinmux = <RZN1_MUX(104, UART0_I)>;	/*
> > UART0_RXD */
> > > +			bias-pull-up;
> > > +		};
> > > +	};
> > > +  };
> >
> > I like this version much more. With those minor issues clarified (up to you to
> > decide how to handle them) please add my:
> > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> Many thanks for your review!
> Phil
>
>
> > Cheers
> >    j
> >
> >
> > > diff --git a/include/dt-bindings/pinctrl/rzn1-pinctrl.h
> > > b/include/dt-bindings/pinctrl/rzn1-pinctrl.h
> > > new file mode 100644
> > > index 000000000000..40369ee36e6a
> > > --- /dev/null
> > > +++ b/include/dt-bindings/pinctrl/rzn1-pinctrl.h
> > > @@ -0,0 +1,141 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * Defines macros and constants for Renesas RZ/N1 pin controller pin
> > > + * muxing functions.
> > > + */
> > > +#ifndef __DT_BINDINGS_RZN1_PINCTRL_H
> > > +#define __DT_BINDINGS_RZN1_PINCTRL_H
> > > +
> > > +#define RZN1_MUX(_gpio, _func) \
> > > +	(((RZN1_FUNC_##_func) << 8) | (_gpio))
> > > +
> > > +/*
> > > + * Given the different levels of muxing on the SoC, it was decided to
> > > + * 'linearize' them into one numerical space. So mux level 1, 2 and
> > > +the MDIO
> > > + * muxes are all represented by one single value.
> > > + *
> > > + * You can derive the hardware value pretty easily too, as
> > > + * 0...9   are Level 1
> > > + * 10...71 are Level 2. The Level 2 mux will be set to this
> > > + *         value - RZN1_FUNC_L2_OFFSET, and the Level 1 mux will be
> > > + *         set accordingly.
> > > + * 72...103 are for the 2 MDIO muxes.
> > > + */
> > > +#define RZN1_FUNC_HIGHZ				0
> > > +#define RZN1_FUNC_0L				1
> > > +#define RZN1_FUNC_CLK_ETH_MII_RGMII_RMII	2
> > > +#define RZN1_FUNC_CLK_ETH_NAND			3
> > > +#define RZN1_FUNC_QSPI				4
> > > +#define RZN1_FUNC_SDIO				5
> > > +#define RZN1_FUNC_LCD				6
> > > +#define RZN1_FUNC_LCD_E				7
> > > +#define RZN1_FUNC_MSEBIM			8
> > > +#define RZN1_FUNC_MSEBIS			9
> > > +#define RZN1_FUNC_L2_OFFSET			10	/* I'm Special
> > */
> > > +
> > > +#define RZN1_FUNC_HIGHZ1
> > 	(RZN1_FUNC_L2_OFFSET + 0)
> > > +#define RZN1_FUNC_ETHERCAT
> > 	(RZN1_FUNC_L2_OFFSET + 1)
> > > +#define RZN1_FUNC_SERCOS3
> > 	(RZN1_FUNC_L2_OFFSET + 2)
> > > +#define RZN1_FUNC_SDIO_E
> > 	(RZN1_FUNC_L2_OFFSET + 3)
> > > +#define RZN1_FUNC_ETH_MDIO
> > 	(RZN1_FUNC_L2_OFFSET + 4)
> > > +#define RZN1_FUNC_ETH_MDIO_E1
> > 	(RZN1_FUNC_L2_OFFSET + 5)
> > > +#define RZN1_FUNC_USB
> > 	(RZN1_FUNC_L2_OFFSET + 6)
> > > +#define RZN1_FUNC_MSEBIM_E
> > 	(RZN1_FUNC_L2_OFFSET + 7)
> > > +#define RZN1_FUNC_MSEBIS_E
> > 	(RZN1_FUNC_L2_OFFSET + 8)
> > > +#define RZN1_FUNC_RSV
> > 	(RZN1_FUNC_L2_OFFSET + 9)
> > > +#define RZN1_FUNC_RSV_E
> > 	(RZN1_FUNC_L2_OFFSET + 10)
> > > +#define RZN1_FUNC_RSV_E1
> > 	(RZN1_FUNC_L2_OFFSET + 11)
> > > +#define RZN1_FUNC_UART0_I
> > 	(RZN1_FUNC_L2_OFFSET + 12)
> > > +#define RZN1_FUNC_UART0_I_E
> > 	(RZN1_FUNC_L2_OFFSET + 13)
> > > +#define RZN1_FUNC_UART1_I
> > 	(RZN1_FUNC_L2_OFFSET + 14)
> > > +#define RZN1_FUNC_UART1_I_E
> > 	(RZN1_FUNC_L2_OFFSET + 15)
> > > +#define RZN1_FUNC_UART2_I
> > 	(RZN1_FUNC_L2_OFFSET + 16)
> > > +#define RZN1_FUNC_UART2_I_E
> > 	(RZN1_FUNC_L2_OFFSET + 17)
> > > +#define RZN1_FUNC_UART0
> > 	(RZN1_FUNC_L2_OFFSET + 18)
> > > +#define RZN1_FUNC_UART0_E
> > 	(RZN1_FUNC_L2_OFFSET + 19)
> > > +#define RZN1_FUNC_UART1
> > 	(RZN1_FUNC_L2_OFFSET + 20)
> > > +#define RZN1_FUNC_UART1_E
> > 	(RZN1_FUNC_L2_OFFSET + 21)
> > > +#define RZN1_FUNC_UART2
> > 	(RZN1_FUNC_L2_OFFSET + 22)
> > > +#define RZN1_FUNC_UART2_E
> > 	(RZN1_FUNC_L2_OFFSET + 23)
> > > +#define RZN1_FUNC_UART3
> > 	(RZN1_FUNC_L2_OFFSET + 24)
> > > +#define RZN1_FUNC_UART3_E
> > 	(RZN1_FUNC_L2_OFFSET + 25)
> > > +#define RZN1_FUNC_UART4
> > 	(RZN1_FUNC_L2_OFFSET + 26)
> > > +#define RZN1_FUNC_UART4_E
> > 	(RZN1_FUNC_L2_OFFSET + 27)
> > > +#define RZN1_FUNC_UART5
> > 	(RZN1_FUNC_L2_OFFSET + 28)
> > > +#define RZN1_FUNC_UART5_E
> > 	(RZN1_FUNC_L2_OFFSET + 29)
> > > +#define RZN1_FUNC_UART6
> > 	(RZN1_FUNC_L2_OFFSET + 30)
> > > +#define RZN1_FUNC_UART6_E
> > 	(RZN1_FUNC_L2_OFFSET + 31)
> > > +#define RZN1_FUNC_UART7
> > 	(RZN1_FUNC_L2_OFFSET + 32)
> > > +#define RZN1_FUNC_UART7_E
> > 	(RZN1_FUNC_L2_OFFSET + 33)
> > > +#define RZN1_FUNC_SPI0_M
> > 	(RZN1_FUNC_L2_OFFSET + 34)
> > > +#define RZN1_FUNC_SPI0_M_E
> > 	(RZN1_FUNC_L2_OFFSET + 35)
> > > +#define RZN1_FUNC_SPI1_M
> > 	(RZN1_FUNC_L2_OFFSET + 36)
> > > +#define RZN1_FUNC_SPI1_M_E
> > 	(RZN1_FUNC_L2_OFFSET + 37)
> > > +#define RZN1_FUNC_SPI2_M
> > 	(RZN1_FUNC_L2_OFFSET + 38)
> > > +#define RZN1_FUNC_SPI2_M_E
> > 	(RZN1_FUNC_L2_OFFSET + 39)
> > > +#define RZN1_FUNC_SPI3_M
> > 	(RZN1_FUNC_L2_OFFSET + 40)
> > > +#define RZN1_FUNC_SPI3_M_E
> > 	(RZN1_FUNC_L2_OFFSET + 41)
> > > +#define RZN1_FUNC_SPI4_S
> > 	(RZN1_FUNC_L2_OFFSET + 42)
> > > +#define RZN1_FUNC_SPI4_S_E
> > 	(RZN1_FUNC_L2_OFFSET + 43)
> > > +#define RZN1_FUNC_SPI5_S
> > 	(RZN1_FUNC_L2_OFFSET + 44)
> > > +#define RZN1_FUNC_SPI5_S_E
> > 	(RZN1_FUNC_L2_OFFSET + 45)
> > > +#define RZN1_FUNC_SGPIO0_M
> > 	(RZN1_FUNC_L2_OFFSET + 46)
> > > +#define RZN1_FUNC_SGPIO1_M
> > 	(RZN1_FUNC_L2_OFFSET + 47)
> > > +#define RZN1_FUNC_GPIO
> > 	(RZN1_FUNC_L2_OFFSET + 48)
> > > +#define RZN1_FUNC_CAN
> > 	(RZN1_FUNC_L2_OFFSET + 49)
> > > +#define RZN1_FUNC_I2C
> > 	(RZN1_FUNC_L2_OFFSET + 50)
> > > +#define RZN1_FUNC_SAFE
> > 	(RZN1_FUNC_L2_OFFSET + 51)
> > > +#define RZN1_FUNC_PTO_PWM
> > 	(RZN1_FUNC_L2_OFFSET + 52)
> > > +#define RZN1_FUNC_PTO_PWM1
> > 	(RZN1_FUNC_L2_OFFSET + 53)
> > > +#define RZN1_FUNC_PTO_PWM2
> > 	(RZN1_FUNC_L2_OFFSET + 54)
> > > +#define RZN1_FUNC_PTO_PWM3
> > 	(RZN1_FUNC_L2_OFFSET + 55)
> > > +#define RZN1_FUNC_PTO_PWM4
> > 	(RZN1_FUNC_L2_OFFSET + 56)
> > > +#define RZN1_FUNC_DELTA_SIGMA
> > 	(RZN1_FUNC_L2_OFFSET + 57)
> > > +#define RZN1_FUNC_SGPIO2_M
> > 	(RZN1_FUNC_L2_OFFSET + 58)
> > > +#define RZN1_FUNC_SGPIO3_M
> > 	(RZN1_FUNC_L2_OFFSET + 59)
> > > +#define RZN1_FUNC_SGPIO4_S
> > 	(RZN1_FUNC_L2_OFFSET + 60)
> > > +#define RZN1_FUNC_MAC_MTIP_SWITCH
> > 	(RZN1_FUNC_L2_OFFSET + 61)
> > > +
> > > +#define RZN1_FUNC_MDIO_OFFSET
> > 	(RZN1_FUNC_L2_OFFSET + 62)
> > > +
> > > +/* These are MDIO0 peripherals for the RZN1_FUNC_ETH_MDIO function
> > */
> > > +#define RZN1_FUNC_MDIO0_HIGHZ
> > 	(RZN1_FUNC_MDIO_OFFSET + 0)
> > > +#define RZN1_FUNC_MDIO0_GMAC0
> > 	(RZN1_FUNC_MDIO_OFFSET + 1)
> > > +#define RZN1_FUNC_MDIO0_GMAC1
> > 	(RZN1_FUNC_MDIO_OFFSET + 2)
> > > +#define RZN1_FUNC_MDIO0_ECAT
> > 	(RZN1_FUNC_MDIO_OFFSET + 3)
> > > +#define RZN1_FUNC_MDIO0_S3_MDIO0
> > 	(RZN1_FUNC_MDIO_OFFSET + 4)
> > > +#define RZN1_FUNC_MDIO0_S3_MDIO1
> > 	(RZN1_FUNC_MDIO_OFFSET + 5)
> > > +#define RZN1_FUNC_MDIO0_HWRTOS
> > 	(RZN1_FUNC_MDIO_OFFSET + 6)
> > > +#define RZN1_FUNC_MDIO0_SWITCH
> > 	(RZN1_FUNC_MDIO_OFFSET + 7)
> > > +/* These are MDIO0 peripherals for the RZN1_FUNC_ETH_MDIO_E1
> > function */
> > > +#define RZN1_FUNC_MDIO0_E1_HIGHZ
> > 	(RZN1_FUNC_MDIO_OFFSET + 8)
> > > +#define RZN1_FUNC_MDIO0_E1_GMAC0
> > 	(RZN1_FUNC_MDIO_OFFSET + 9)
> > > +#define RZN1_FUNC_MDIO0_E1_GMAC1
> > 	(RZN1_FUNC_MDIO_OFFSET + 10)
> > > +#define RZN1_FUNC_MDIO0_E1_ECAT
> > 	(RZN1_FUNC_MDIO_OFFSET + 11)
> > > +#define RZN1_FUNC_MDIO0_E1_S3_MDIO0
> > 	(RZN1_FUNC_MDIO_OFFSET + 12)
> > > +#define RZN1_FUNC_MDIO0_E1_S3_MDIO1
> > 	(RZN1_FUNC_MDIO_OFFSET + 13)
> > > +#define RZN1_FUNC_MDIO0_E1_HWRTOS
> > 	(RZN1_FUNC_MDIO_OFFSET + 14)
> > > +#define RZN1_FUNC_MDIO0_E1_SWITCH
> > 	(RZN1_FUNC_MDIO_OFFSET + 15)
> > > +
> > > +/* These are MDIO1 peripherals for the RZN1_FUNC_ETH_MDIO function
> > */
> > > +#define RZN1_FUNC_MDIO1_HIGHZ
> > 	(RZN1_FUNC_MDIO_OFFSET + 16)
> > > +#define RZN1_FUNC_MDIO1_GMAC0
> > 	(RZN1_FUNC_MDIO_OFFSET + 17)
> > > +#define RZN1_FUNC_MDIO1_GMAC1
> > 	(RZN1_FUNC_MDIO_OFFSET + 18)
> > > +#define RZN1_FUNC_MDIO1_ECAT
> > 	(RZN1_FUNC_MDIO_OFFSET + 19)
> > > +#define RZN1_FUNC_MDIO1_S3_MDIO0
> > 	(RZN1_FUNC_MDIO_OFFSET + 20)
> > > +#define RZN1_FUNC_MDIO1_S3_MDIO1
> > 	(RZN1_FUNC_MDIO_OFFSET + 21)
> > > +#define RZN1_FUNC_MDIO1_HWRTOS
> > 	(RZN1_FUNC_MDIO_OFFSET + 22)
> > > +#define RZN1_FUNC_MDIO1_SWITCH
> > 	(RZN1_FUNC_MDIO_OFFSET + 23)
> > > +/* These are MDIO1 peripherals for the RZN1_FUNC_ETH_MDIO_E1
> > function */
> > > +#define RZN1_FUNC_MDIO1_E1_HIGHZ
> > 	(RZN1_FUNC_MDIO_OFFSET + 24)
> > > +#define RZN1_FUNC_MDIO1_E1_GMAC0
> > 	(RZN1_FUNC_MDIO_OFFSET + 25)
> > > +#define RZN1_FUNC_MDIO1_E1_GMAC1
> > 	(RZN1_FUNC_MDIO_OFFSET + 26)
> > > +#define RZN1_FUNC_MDIO1_E1_ECAT
> > 	(RZN1_FUNC_MDIO_OFFSET + 27)
> > > +#define RZN1_FUNC_MDIO1_E1_S3_MDIO0
> > 	(RZN1_FUNC_MDIO_OFFSET + 28)
> > > +#define RZN1_FUNC_MDIO1_E1_S3_MDIO1
> > 	(RZN1_FUNC_MDIO_OFFSET + 29)
> > > +#define RZN1_FUNC_MDIO1_E1_HWRTOS
> > 	(RZN1_FUNC_MDIO_OFFSET + 30)
> > > +#define RZN1_FUNC_MDIO1_E1_SWITCH
> > 	(RZN1_FUNC_MDIO_OFFSET + 31)
> > > +
> > > +#define RZN1_FUNC_MAX
> > 	(RZN1_FUNC_MDIO_OFFSET + 32)
> > > +
> > > +#endif /* __DT_BINDINGS_RZN1_PINCTRL_H */
> > > --
> > > 2.17.1
> > >

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

[PATCH v3 2/3] pinctrl: renesas: Renesas RZ/N1 pinctrl driver

[Phil Edworthy]

This provides a pinctrl driver for the Renesas RZ/N1 device family.

Based on a patch originally written by Michel Pollet at Renesas.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
v3:
 - Use standard DT props instead of proprietary ones.
 - Replace virtual pins used for MDIO muxing with extra funcs.
 - Use pinctrl_utils funcs to handle the maps.
 - Remove the dbg functions to keep things simple.

v2:
 - Change filename to generic rzn1, instead of device specific.
 - Changed Kconfig symbol and file name to generic rzn1 family.
 - Added "renesas,rzn1-pinctrl" compatible fallback string
 - Changes suggested by Jacopo Mondi. Mainly formatting, plus:
   - Removed global ptr
   - Removed unused code accessing parent of node.
   - Removed check for null OF np that can't happen.
   - Replaced overlapping enums with #defines
 - Renamed some variables and symbols to clarify their use
 - Fix error handling during probe
 - Move probe from postcore_initcall to subsys_initcall to ensure
   drivers that require clocks get them instead of having to defer
   probing.
---
 drivers/pinctrl/Kconfig        |  10 +
 drivers/pinctrl/Makefile       |   1 +
 drivers/pinctrl/pinctrl-rzn1.c | 926 +++++++++++++++++++++++++++++++++
 3 files changed, 937 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-rzn1.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index e86752be1f19..e524eb101384 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -195,6 +195,16 @@ config PINCTRL_RZA1
 	help
 	  This selects pinctrl driver for Renesas RZ/A1 platforms.
 
+config PINCTRL_RZN1
+	bool "Renesas RZ/N1 pinctrl driver"
+	depends on OF
+	depends on ARCH_RZN1 || COMPILE_TEST
+	select GENERIC_PINCTRL_GROUPS
+	select GENERIC_PINMUX_FUNCTIONS
+	select GENERIC_PINCONF
+	help
+	  This selects pinctrl driver for Renesas RZ/N1 devices.
+
 config PINCTRL_SINGLE
 	tristate "One-register-per-pin type device tree based pinctrl driver"
 	depends on OF
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 46ef9bd52096..d07f9a20f6ae 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_PINCTRL_PIC32)	+= pinctrl-pic32.o
 obj-$(CONFIG_PINCTRL_PISTACHIO)	+= pinctrl-pistachio.o
 obj-$(CONFIG_PINCTRL_ROCKCHIP)	+= pinctrl-rockchip.o
 obj-$(CONFIG_PINCTRL_RZA1)	+= pinctrl-rza1.o
+obj-$(CONFIG_PINCTRL_RZN1)	+= pinctrl-rzn1.o
 obj-$(CONFIG_PINCTRL_SINGLE)	+= pinctrl-single.o
 obj-$(CONFIG_PINCTRL_SIRF)	+= sirf/
 obj-$(CONFIG_PINCTRL_SX150X)	+= pinctrl-sx150x.o
diff --git a/drivers/pinctrl/pinctrl-rzn1.c b/drivers/pinctrl/pinctrl-rzn1.c
new file mode 100644
index 000000000000..8f0caa266dbb
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-rzn1.c
@@ -0,0 +1,926 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2014-2018 Renesas Electronics Europe Limited
+ *
+ * Phil Edworthy <phil.edworthy@renesas.com>
+ * Based on a driver originally written by Michel Pollet at Renesas.
+ */
+
+#include <dt-bindings/pinctrl/rzn1-pinctrl.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include "core.h"
+#include "pinconf.h"
+#include "pinctrl-utils.h"
+
+/* Field positions and masks in the pinmux registers */
+#define RZN1_L1_PIN_DRIVE_STRENGTH	10
+#define RZN1_L1_PIN_DRIVE_STRENGTH_4MA	0
+#define RZN1_L1_PIN_DRIVE_STRENGTH_6MA	1
+#define RZN1_L1_PIN_DRIVE_STRENGTH_8MA	2
+#define RZN1_L1_PIN_DRIVE_STRENGTH_12MA	3
+#define RZN1_L1_PIN_PULL		8
+#define RZN1_L1_PIN_PULL_NONE		0
+#define RZN1_L1_PIN_PULL_UP		1
+#define RZN1_L1_PIN_PULL_DOWN		3
+#define RZN1_L1_FUNCTION		0
+#define RZN1_L1_FUNC_MASK		0xf
+#define RZN1_L1_FUNCTION_L2		0xf
+
+/*
+ * The hardware manual describes two levels of multiplexing, but it's more
+ * logical to think of the hardware as three levels, with level 3 consisting of
+ * the multiplexing for Ethernet MDIO signals.
+ *
+ * Level 1 functions go from 0 to 9, with level 1 function '15' (0xf) specifying
+ * that level 2 functions are used instead. Level 2 has a lot more options,
+ * going from 0 to 61. Level 3 allows selection of MDIO functions which can be
+ * floating, or one of seven internal peripherals. Unfortunately, there are two
+ * level 2 functions that can select MDIO, and two MDIO channels so we have four
+ * sets of level 3 functions.
+ *
+ * For this driver, we've compounded the numbers together, so:
+ *    0 to   9 is level 1
+ *   10 to  71 is 10 + level 2 number
+ *   72 to  79 is 72 + MDIO0 source for level 2 MDIO function.
+ *   80 to  87 is 80 + MDIO0 source for level 2 MDIO_E1 function.
+ *   88 to  95 is 88 + MDIO1 source for level 2 MDIO function.
+ *   96 to 103 is 96 + MDIO1 source for level 2 MDIO_E1 function.
+ * Examples:
+ *  Function 28 corresponds UART0
+ *  Function 73 corresponds to MDIO0 to GMAC0
+ *
+ * There are 170 configurable pins (called PL_GPIO in the datasheet).
+ */
+
+/*
+ * Structure detailing the HW registers on the RZ/N1 devices.
+ * Both the Level 1 mux registers and Level 2 mux registers have the same
+ * structure. The only difference is that Level 2 has additional MDIO registers
+ * at the end.
+ */
+struct rzn1_pinctrl_regs {
+	union {
+		u32	conf[170];
+		u8	pad0[0x400];
+	};
+	u32	status_protect;	/* 0x400 */
+	/* MDIO mux registers, level2 only */
+	u32	l2_mdio[2];
+};
+
+/**
+ * struct rzn1_pmx_func - describes rzn1 pinmux functions
+ * @name: the name of this specific function
+ * @groups: corresponding pin groups
+ * @num_groups: the number of groups
+ */
+struct rzn1_pmx_func {
+	const char *name;
+	const char **groups;
+	unsigned int num_groups;
+};
+
+/**
+ * struct rzn1_pin_group - describes an rzn1 pin group
+ * @name: the name of this specific pin group
+ * @func: the name of the function selected by this group
+ * @npins: the number of pins in this group array, i.e. the number of
+ *	elements in .pins so we can iterate over that array
+ * @pin_ids: array of pin_ids, i.e. the value used to select the mux
+ * @pins: array of pins. Needed due to pinctrl_ops.get_group_pins()
+ */
+struct rzn1_pin_group {
+	const char *name;
+	const char *func;
+	unsigned int npins;
+	u8 *pin_ids;
+	u32 *pins;
+};
+
+struct rzn1_pinctrl {
+	struct device *dev;
+	struct clk *clk;
+	struct pinctrl_dev *pctl;
+	struct rzn1_pinctrl_regs __iomem *lev1;
+	struct rzn1_pinctrl_regs __iomem *lev2;
+	u32 lev1_protect_phys;
+	u32 lev2_protect_phys;
+	int mdio_func[2];
+
+	struct rzn1_pin_group *groups;
+	unsigned int ngroups;
+
+	struct rzn1_pmx_func *functions;
+	unsigned int nfunctions;
+};
+
+#define RZN1_PINS_PROP "pinmux"
+
+#define RZN1_PIN(pin) PINCTRL_PIN(pin, "pl_gpio"#pin)
+
+static const struct pinctrl_pin_desc rzn1_pins[] = {
+	RZN1_PIN(0), RZN1_PIN(1), RZN1_PIN(2), RZN1_PIN(3), RZN1_PIN(4),
+	RZN1_PIN(5), RZN1_PIN(6), RZN1_PIN(7), RZN1_PIN(8), RZN1_PIN(9),
+	RZN1_PIN(10), RZN1_PIN(11), RZN1_PIN(12), RZN1_PIN(13), RZN1_PIN(14),
+	RZN1_PIN(15), RZN1_PIN(16), RZN1_PIN(17), RZN1_PIN(18), RZN1_PIN(19),
+	RZN1_PIN(20), RZN1_PIN(21), RZN1_PIN(22), RZN1_PIN(23), RZN1_PIN(24),
+	RZN1_PIN(25), RZN1_PIN(26), RZN1_PIN(27), RZN1_PIN(28), RZN1_PIN(29),
+	RZN1_PIN(30), RZN1_PIN(31), RZN1_PIN(32), RZN1_PIN(33), RZN1_PIN(34),
+	RZN1_PIN(35), RZN1_PIN(36), RZN1_PIN(37), RZN1_PIN(38), RZN1_PIN(39),
+	RZN1_PIN(40), RZN1_PIN(41), RZN1_PIN(42), RZN1_PIN(43), RZN1_PIN(44),
+	RZN1_PIN(45), RZN1_PIN(46), RZN1_PIN(47), RZN1_PIN(48), RZN1_PIN(49),
+	RZN1_PIN(50), RZN1_PIN(51), RZN1_PIN(52), RZN1_PIN(53), RZN1_PIN(54),
+	RZN1_PIN(55), RZN1_PIN(56), RZN1_PIN(57), RZN1_PIN(58), RZN1_PIN(59),
+	RZN1_PIN(60), RZN1_PIN(61), RZN1_PIN(62), RZN1_PIN(63), RZN1_PIN(64),
+	RZN1_PIN(65), RZN1_PIN(66), RZN1_PIN(67), RZN1_PIN(68), RZN1_PIN(69),
+	RZN1_PIN(70), RZN1_PIN(71), RZN1_PIN(72), RZN1_PIN(73), RZN1_PIN(74),
+	RZN1_PIN(75), RZN1_PIN(76), RZN1_PIN(77), RZN1_PIN(78), RZN1_PIN(79),
+	RZN1_PIN(80), RZN1_PIN(81), RZN1_PIN(82), RZN1_PIN(83), RZN1_PIN(84),
+	RZN1_PIN(85), RZN1_PIN(86), RZN1_PIN(87), RZN1_PIN(88), RZN1_PIN(89),
+	RZN1_PIN(90), RZN1_PIN(91), RZN1_PIN(92), RZN1_PIN(93), RZN1_PIN(94),
+	RZN1_PIN(95), RZN1_PIN(96), RZN1_PIN(97), RZN1_PIN(98), RZN1_PIN(99),
+	RZN1_PIN(100), RZN1_PIN(101), RZN1_PIN(102), RZN1_PIN(103),
+	RZN1_PIN(104), RZN1_PIN(105), RZN1_PIN(106), RZN1_PIN(107),
+	RZN1_PIN(108), RZN1_PIN(109), RZN1_PIN(110), RZN1_PIN(111),
+	RZN1_PIN(112), RZN1_PIN(113), RZN1_PIN(114), RZN1_PIN(115),
+	RZN1_PIN(116), RZN1_PIN(117), RZN1_PIN(118), RZN1_PIN(119),
+	RZN1_PIN(120), RZN1_PIN(121), RZN1_PIN(122), RZN1_PIN(123),
+	RZN1_PIN(124), RZN1_PIN(125), RZN1_PIN(126), RZN1_PIN(127),
+	RZN1_PIN(128), RZN1_PIN(129), RZN1_PIN(130), RZN1_PIN(131),
+	RZN1_PIN(132), RZN1_PIN(133), RZN1_PIN(134), RZN1_PIN(135),
+	RZN1_PIN(136), RZN1_PIN(137), RZN1_PIN(138), RZN1_PIN(139),
+	RZN1_PIN(140), RZN1_PIN(141), RZN1_PIN(142), RZN1_PIN(143),
+	RZN1_PIN(144), RZN1_PIN(145), RZN1_PIN(146), RZN1_PIN(147),
+	RZN1_PIN(148), RZN1_PIN(149), RZN1_PIN(150), RZN1_PIN(151),
+	RZN1_PIN(152), RZN1_PIN(153), RZN1_PIN(154), RZN1_PIN(155),
+	RZN1_PIN(156), RZN1_PIN(157), RZN1_PIN(158), RZN1_PIN(159),
+	RZN1_PIN(160), RZN1_PIN(161), RZN1_PIN(162), RZN1_PIN(163),
+	RZN1_PIN(164), RZN1_PIN(165), RZN1_PIN(166), RZN1_PIN(167),
+	RZN1_PIN(168), RZN1_PIN(169),
+};
+
+enum {
+	LOCK_LEVEL1 = 0x1,
+	LOCK_LEVEL2 = 0x2,
+	LOCK_ALL = LOCK_LEVEL1 | LOCK_LEVEL2,
+};
+
+static void rzn1_hw_set_lock(struct rzn1_pinctrl *ipctl, u8 lock, u8 value)
+{
+	/*
+	 * The pinmux configuration is locked by writing the physical address of
+	 * the status_protect register to itself. It is unlocked by writing the
+	 * address | 1.
+	 */
+	if (lock & LOCK_LEVEL1) {
+		u32 val = ipctl->lev1_protect_phys | !(value & LOCK_LEVEL1);
+
+		writel(val, &ipctl->lev1->status_protect);
+	}
+
+	if (lock & LOCK_LEVEL2) {
+		u32 val = ipctl->lev2_protect_phys | !(value & LOCK_LEVEL2);
+
+		writel(val, &ipctl->lev2->status_protect);
+	}
+}
+
+static void rzn1_pinctrl_mdio_select(struct rzn1_pinctrl *ipctl, u8 mdio,
+				     u32 func)
+{
+	if (ipctl->mdio_func[mdio] >= 0 && ipctl->mdio_func[mdio] != func)
+		dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n", mdio);
+	ipctl->mdio_func[mdio] = func;
+
+	dev_dbg(ipctl->dev, "setting mdio%d to %d\n", mdio, func);
+
+	writel(func, &ipctl->lev2->l2_mdio[mdio]);
+}
+
+/*
+ * Using a composite pin description, set the hardware pinmux registers
+ * with the corresponding values.
+ * Make sure to unlock write protection and reset it afterward.
+ *
+ * NOTE: There is no protection for potential concurrency, it is assumed these
+ * calls are serialized already.
+ */
+static int rzn1_set_hw_pin_func(struct rzn1_pinctrl *ipctl, u32 pin,
+				u32 pin_config, u8 use_locks)
+{
+	u32 l1_cache;
+	u32 l2_cache;
+	u32 l1;
+	u32 l2;
+
+	/* Level 3 MDIO multiplexing */
+	if (pin_config >= RZN1_FUNC_MDIO0_HIGHZ &&
+	    pin_config <= RZN1_FUNC_MDIO1_E1_SWITCH) {
+		u8 mdio_channel;
+		u32 mdio_func;
+
+		if (pin_config <= RZN1_FUNC_MDIO1_HIGHZ)
+			mdio_channel = 0;
+		else
+			mdio_channel = 1;
+
+		/* Get MDIO func, and convert the func to the level 2 number */
+		if (pin_config <= RZN1_FUNC_MDIO0_SWITCH) {
+			mdio_func = pin_config - RZN1_FUNC_MDIO0_HIGHZ;
+			pin_config = RZN1_FUNC_ETH_MDIO;
+		} else if (pin_config <= RZN1_FUNC_MDIO0_E1_SWITCH) {
+			mdio_func = pin_config - RZN1_FUNC_MDIO0_E1_HIGHZ;
+			pin_config = RZN1_FUNC_ETH_MDIO_E1;
+		} else if (pin_config <= RZN1_FUNC_MDIO1_SWITCH) {
+			mdio_func = pin_config - RZN1_FUNC_MDIO1_HIGHZ;
+			pin_config = RZN1_FUNC_ETH_MDIO;
+		} else {
+			mdio_func = pin_config - RZN1_FUNC_MDIO1_E1_HIGHZ;
+			pin_config = RZN1_FUNC_ETH_MDIO_E1;
+		}
+		rzn1_pinctrl_mdio_select(ipctl, mdio_channel, mdio_func);
+	}
+
+	/* Note here, we do not allow anything past the MDIO Mux values */
+	if (pin >= ARRAY_SIZE(ipctl->lev1->conf) ||
+	    pin_config >= RZN1_FUNC_MDIO0_HIGHZ)
+		return -EINVAL;
+
+	l1 = readl(&ipctl->lev1->conf[pin]);
+	l1_cache = l1;
+	l2 = readl(&ipctl->lev2->conf[pin]);
+	l2_cache = l2;
+
+	dev_dbg(ipctl->dev, "setting func for pin %d to %d\n", pin, pin_config);
+
+	if (pin_config < RZN1_FUNC_L2_OFFSET) {
+		l1 &= ~(RZN1_L1_FUNC_MASK << RZN1_L1_FUNCTION);
+		l1 |= (pin_config << RZN1_L1_FUNCTION);
+	} else {
+		l1 &= ~(RZN1_L1_FUNC_MASK << RZN1_L1_FUNCTION);
+		l1 |= (RZN1_L1_FUNCTION_L2 << RZN1_L1_FUNCTION);
+
+		l2 = pin_config - RZN1_FUNC_L2_OFFSET;
+	}
+
+	/* If either configuration changes, we update both anyway */
+	if (l1 != l1_cache || l2 != l2_cache) {
+		writel(l1, &ipctl->lev1->conf[pin]);
+		writel(l2, &ipctl->lev2->conf[pin]);
+	}
+
+	return 0;
+}
+
+static const struct rzn1_pin_group *rzn1_pinctrl_find_group_by_name(
+	const struct rzn1_pinctrl *ipctl, const char *name)
+{
+	const struct rzn1_pin_group *grp = NULL;
+	int i;
+
+	for (i = 0; i < ipctl->ngroups; i++) {
+		if (!strcmp(ipctl->groups[i].name, name)) {
+			grp = &ipctl->groups[i];
+			break;
+		}
+	}
+
+	return grp;
+}
+
+static int rzn1_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+
+	return ipctl->ngroups;
+}
+
+static const char *rzn1_get_group_name(struct pinctrl_dev *pctldev,
+				       unsigned int selector)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+
+	return ipctl->groups[selector].name;
+}
+
+static int rzn1_get_group_pins(struct pinctrl_dev *pctldev,
+			       unsigned int selector, const unsigned int **pins,
+			       unsigned int *npins)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+
+	if (selector >= ipctl->ngroups)
+		return -EINVAL;
+
+	*pins = ipctl->groups[selector].pins;
+	*npins = ipctl->groups[selector].npins;
+
+	return 0;
+}
+
+/*
+ * This function is called for each pinctl 'Function' node.
+ * Sub-nodes can be used to describe multiple 'Groups' for the 'Function'
+ * If there aren't any sub-nodes, the 'Group' is essentially the 'Function'.
+ * Each 'Group' uses pinmux = <...> to detail the pins and data used to select
+ * the functionality. Each 'Group' has optional pin configurations that apply
+ * to all pins in the 'Group'.
+ */
+static int rzn1_dt_node_to_map_one(struct pinctrl_dev *pctldev,
+				   struct device_node *np,
+				   struct pinctrl_map **map,
+				   unsigned int *num_maps)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+	const struct rzn1_pin_group *grp;
+	unsigned long *configs = NULL;
+	unsigned int num_configs = 0;
+	unsigned int reserved_maps = *num_maps;
+	unsigned int reserve = 1;
+	int ret;
+
+	dev_dbg(ipctl->dev, "processing node %pOF\n", np);
+
+	grp = rzn1_pinctrl_find_group_by_name(ipctl, np->name);
+	if (!grp) {
+		dev_err(ipctl->dev, "unable to find group for node %pOF\n", np);
+		return -EINVAL;
+	}
+
+	/* Get the group's pin configuration */
+	ret = pinconf_generic_parse_dt_config(np, pctldev, &configs,
+					      &num_configs);
+	if (ret < 0) {
+		dev_err(ipctl->dev, "%s: could not parse node property\n",
+			of_node_full_name(np));
+		return ret;
+	}
+
+	if (num_configs)
+		reserve++;
+
+	/* Increase the number of maps to cover this group */
+	ret = pinctrl_utils_reserve_map(pctldev, map, &reserved_maps, num_maps,
+					reserve);
+	if (ret < 0)
+		goto out;
+
+	/* Associate the group with the function */
+	ret = pinctrl_utils_add_map_mux(pctldev, map, &reserved_maps, num_maps,
+					grp->name, grp->func);
+	if (ret < 0)
+		goto out;
+
+	if (num_configs) {
+		/* Associate the group's pin configuration with the group */
+		ret = pinctrl_utils_add_map_configs(pctldev, map,
+				&reserved_maps, num_maps, grp->name,
+				configs, num_configs,
+				PIN_MAP_TYPE_CONFIGS_GROUP);
+		if (ret < 0)
+			goto out;
+	}
+
+	dev_dbg(pctldev->dev, "maps: function %s group %s (%d pins)\n",
+		grp->func, grp->name, grp->npins);
+
+out:
+	kfree(configs);
+	return 0;
+}
+
+static int rzn1_dt_node_to_map(struct pinctrl_dev *pctldev,
+			       struct device_node *np,
+			       struct pinctrl_map **map,
+			       unsigned int *num_maps)
+{
+	struct device_node *child;
+	int ret;
+
+	*map = NULL;
+	*num_maps = 0;
+
+	ret = rzn1_dt_node_to_map_one(pctldev, np, map, num_maps);
+	if (ret < 0)
+		return ret;
+
+	for_each_child_of_node(np, child) {
+		ret = rzn1_dt_node_to_map_one(pctldev, child, map, num_maps);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static const struct pinctrl_ops rzn1_pctrl_ops = {
+	.get_groups_count = rzn1_get_groups_count,
+	.get_group_name = rzn1_get_group_name,
+	.get_group_pins = rzn1_get_group_pins,
+	.dt_node_to_map = rzn1_dt_node_to_map,
+	.dt_free_map = pinctrl_utils_free_map,
+};
+
+static int rzn1_pmx_get_funcs_count(struct pinctrl_dev *pctldev)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+
+	return ipctl->nfunctions;
+}
+
+static const char *rzn1_pmx_get_func_name(struct pinctrl_dev *pctldev,
+					  unsigned int selector)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+
+	return ipctl->functions[selector].name;
+}
+
+static int rzn1_pmx_get_groups(struct pinctrl_dev *pctldev,
+			       unsigned int selector,
+			       const char * const **groups,
+			       unsigned int * const num_groups)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+
+	*groups = ipctl->functions[selector].groups;
+	*num_groups = ipctl->functions[selector].num_groups;
+
+	return 0;
+}
+
+static int rzn1_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
+			unsigned int group)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+	struct rzn1_pin_group *grp = &ipctl->groups[group];
+	unsigned int i, grp_pins = grp->npins;
+
+	dev_dbg(ipctl->dev, "set mux %s(%d) group %s(%d)\n",
+		ipctl->functions[selector].name, selector, grp->name, group);
+
+	rzn1_hw_set_lock(ipctl, LOCK_ALL, LOCK_ALL);
+	for (i = 0; i < grp_pins; i++)
+		rzn1_set_hw_pin_func(ipctl, grp->pins[i], grp->pin_ids[i], 0);
+	rzn1_hw_set_lock(ipctl, LOCK_ALL, 0);
+
+	return 0;
+}
+
+static const struct pinmux_ops rzn1_pmx_ops = {
+	.get_functions_count = rzn1_pmx_get_funcs_count,
+	.get_function_name = rzn1_pmx_get_func_name,
+	.get_function_groups = rzn1_pmx_get_groups,
+	.set_mux = rzn1_set_mux,
+};
+
+static int rzn1_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
+			    unsigned long *config)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param param = pinconf_to_config_param(*config);
+	const u32 reg_drive[4] = { 4, 6, 8, 12 };
+	u32 l1, l2;
+	u32 pull, drive, l1mux;
+	u32 arg = 0;
+
+	if (pin >= ARRAY_SIZE(ipctl->lev1->conf))
+		return -EINVAL;
+
+	l1 = readl(&ipctl->lev1->conf[pin]);
+
+	l1mux = l1 & RZN1_L1_FUNC_MASK;
+	pull = (l1 >> RZN1_L1_PIN_PULL) & 0x3;
+	drive = (l1 >> RZN1_L1_PIN_DRIVE_STRENGTH) & 0x3;
+
+	switch (param) {
+	case PIN_CONFIG_BIAS_PULL_UP:
+		if (pull == RZN1_L1_PIN_PULL_UP)
+			arg = 1;
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		if (pull == RZN1_L1_PIN_PULL_DOWN)
+			arg = 1;
+		break;
+	case PIN_CONFIG_BIAS_DISABLE:
+		if (pull == RZN1_L1_PIN_PULL_NONE)
+			arg = 1;
+		break;
+	case PIN_CONFIG_DRIVE_STRENGTH:
+		arg = reg_drive[drive];
+		break;
+	case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+		l2 = readl(&ipctl->lev1->conf[pin]);
+		if (l1mux == RZN1_FUNC_HIGHZ)
+			arg = 1;
+		if (l1mux == RZN1_L1_FUNCTION_L2 && l2 == 0)
+			arg = 1;
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
+	*config = pinconf_to_config_packed(param, arg);
+
+	return 0;
+}
+
+static int rzn1_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
+			    unsigned long *configs, unsigned int num_configs)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param param;
+	int i;
+	u32 arg;
+	u32 l1, l1_cache;
+	u32 drv;
+
+	if (pin >= ARRAY_SIZE(ipctl->lev1->conf))
+		return -EINVAL;
+
+	l1 = readl(&ipctl->lev1->conf[pin]);
+	l1_cache = l1;
+
+	for (i = 0; i < num_configs; i++) {
+		param = pinconf_to_config_param(configs[i]);
+		arg = pinconf_to_config_argument(configs[i]);
+
+		switch (param) {
+		case PIN_CONFIG_BIAS_PULL_UP:
+			dev_dbg(ipctl->dev, "set pin %d pull up\n", pin);
+			l1 &= ~(0x3 << RZN1_L1_PIN_PULL);
+			l1 |= (RZN1_L1_PIN_PULL_UP << RZN1_L1_PIN_PULL);
+			break;
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+			dev_dbg(ipctl->dev, "set pin %d pull down\n", pin);
+			l1 &= ~(0x3 << RZN1_L1_PIN_PULL);
+			l1 |= (RZN1_L1_PIN_PULL_DOWN << RZN1_L1_PIN_PULL);
+			break;
+		case PIN_CONFIG_BIAS_DISABLE:
+			dev_dbg(ipctl->dev, "set pin %d bias off\n", pin);
+			l1 &= ~(0x3 << RZN1_L1_PIN_PULL);
+			l1 |= (RZN1_L1_PIN_PULL_NONE << RZN1_L1_PIN_PULL);
+			break;
+		case PIN_CONFIG_DRIVE_STRENGTH:
+			dev_dbg(ipctl->dev, "set pin %d drv %umA\n", pin, arg);
+			drv = 0;
+			switch (arg) {
+			case 4:
+				drv = RZN1_L1_PIN_DRIVE_STRENGTH_4MA;
+				break;
+			case 6:
+				drv = RZN1_L1_PIN_DRIVE_STRENGTH_6MA;
+				break;
+			case 8:
+				drv = RZN1_L1_PIN_DRIVE_STRENGTH_8MA;
+				break;
+			case 12:
+				drv = RZN1_L1_PIN_DRIVE_STRENGTH_12MA;
+				break;
+			default:
+				dev_err(ipctl->dev,
+					"Drive strength %umA not supported\n",
+					arg);
+				return -EINVAL;
+			}
+
+			l1 &= ~(0x3 << RZN1_L1_PIN_DRIVE_STRENGTH);
+			l1 |= (drv << RZN1_L1_PIN_DRIVE_STRENGTH);
+			break;
+
+		case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+			dev_dbg(ipctl->dev, "set pin %d High-Z\n", pin);
+			l1 &= ~RZN1_L1_FUNC_MASK;
+			l1 |= RZN1_FUNC_HIGHZ;
+			break;
+		default:
+			return -ENOTSUPP;
+		}
+	}
+
+	if (l1 != l1_cache) {
+		rzn1_hw_set_lock(ipctl, LOCK_LEVEL1, LOCK_LEVEL1);
+		writel(l1, &ipctl->lev1->conf[pin]);
+		rzn1_hw_set_lock(ipctl, LOCK_LEVEL1, 0);
+	}
+
+	return 0;
+}
+
+static int rzn1_pin_config_group_set(struct pinctrl_dev *pctldev,
+				     unsigned int selector,
+				     unsigned long *configs,
+				     unsigned int num_configs)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+	struct rzn1_pin_group *grp = &ipctl->groups[selector];
+	int ret, i;
+
+	dev_dbg(ipctl->dev, "group set %s selector:%d configs:%p/%d\n",
+		grp->name, selector, configs, num_configs);
+
+	for (i = 0; i < grp->npins; i++) {
+		unsigned int pin = grp->pins[i];
+
+		ret = rzn1_pinconf_set(pctldev, pin, configs, num_configs);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static const struct pinconf_ops rzn1_pinconf_ops = {
+	.pin_config_get = rzn1_pinconf_get,
+	.pin_config_set = rzn1_pinconf_set,
+	.pin_config_group_set = rzn1_pin_config_group_set,
+};
+
+static struct pinctrl_desc rzn1_pinctrl_desc = {
+	.pctlops = &rzn1_pctrl_ops,
+	.pmxops = &rzn1_pmx_ops,
+	.confops = &rzn1_pinconf_ops,
+	.owner = THIS_MODULE,
+};
+
+static int rzn1_pinctrl_parse_groups(struct device_node *np,
+				     struct rzn1_pin_group *grp,
+				     struct rzn1_pinctrl *ipctl)
+{
+	int size;
+	const __be32 *list;
+	int i;
+
+	dev_dbg(ipctl->dev, "%s: %s\n", __func__, np->name);
+
+	/* Initialise group */
+	grp->name = np->name;
+
+	/*
+	 * The binding format is
+	 *	pinmux = <PIN_FUNC_ID CONFIG ...>,
+	 * do sanity check and calculate pins number
+	 */
+	list = of_get_property(np, RZN1_PINS_PROP, &size);
+	if (!list) {
+		dev_err(ipctl->dev,
+			"no " RZN1_PINS_PROP " property in node %s\n",
+			np->full_name);
+		return -EINVAL;
+	}
+
+	/* We do not check return since it's safe node passed down */
+	if (!size) {
+		dev_err(ipctl->dev, "Invalid " RZN1_PINS_PROP " in node %s\n",
+			np->full_name);
+		return -EINVAL;
+	}
+
+	grp->npins = size / sizeof(list[0]);
+	if (!grp->npins)
+		return 0;
+
+	grp->pin_ids = devm_kmalloc_array(ipctl->dev,
+					  grp->npins, sizeof(grp->pin_ids[0]),
+					  GFP_KERNEL);
+	grp->pins = devm_kmalloc_array(ipctl->dev,
+				       grp->npins, sizeof(grp->pins[0]),
+				       GFP_KERNEL);
+	if (!grp->pin_ids || !grp->pins)
+		return -ENOMEM;
+
+	for (i = 0; i < grp->npins; i++) {
+		u32 pin_id = be32_to_cpu(*list++);
+
+		grp->pins[i] = pin_id & 0xff;
+		grp->pin_ids[i] = (pin_id >> 8) & 0x7f;
+	}
+
+	return grp->npins;
+}
+
+static int rzn1_pinctrl_count_function_groups(struct device_node *np)
+{
+	struct device_node *child;
+	int count = 0;
+
+	if (of_property_count_u32_elems(np, RZN1_PINS_PROP) > 0)
+		count++;
+
+	for_each_child_of_node(np, child) {
+		if (of_property_count_u32_elems(child, RZN1_PINS_PROP) > 0)
+			count++;
+	}
+
+	return count;
+}
+
+static int rzn1_pinctrl_parse_functions(struct device_node *np,
+					struct rzn1_pinctrl *ipctl, u32 index)
+{
+	struct device_node *child;
+	struct rzn1_pmx_func *func;
+	struct rzn1_pin_group *grp;
+	u32 i = 0;
+	int ret;
+
+	dev_dbg(ipctl->dev, "parse function(%d): %s\n", index, np->name);
+
+	func = &ipctl->functions[index];
+
+	/* Initialise function */
+	func->name = np->name;
+	func->num_groups = rzn1_pinctrl_count_function_groups(np);
+	dev_dbg(ipctl->dev, "function %s has %d groups\n",
+		np->name, func->num_groups);
+	if (func->num_groups == 0) {
+		dev_err(ipctl->dev, "no groups defined in %s\n", np->full_name);
+		return -EINVAL;
+	}
+
+	func->groups = devm_kmalloc_array(ipctl->dev,
+					  func->num_groups, sizeof(char *),
+					  GFP_KERNEL);
+	if (!func->groups)
+		return -ENOMEM;
+
+	if (of_property_count_u32_elems(np, RZN1_PINS_PROP) > 0) {
+		func->groups[i] = np->name;
+		grp = &ipctl->groups[ipctl->ngroups];
+		grp->func = func->name;
+		ret = rzn1_pinctrl_parse_groups(np, grp, ipctl);
+		if (ret < 0)
+			return ret;
+		if (ret > 0) {
+			i++;
+			ipctl->ngroups++;
+		}
+	}
+
+	for_each_child_of_node(np, child) {
+		func->groups[i] = child->name;
+		grp = &ipctl->groups[ipctl->ngroups];
+		grp->func = func->name;
+		ret = rzn1_pinctrl_parse_groups(child, grp, ipctl);
+		if (ret < 0)
+			return ret;
+		if (ret > 0) {
+			i++;
+			ipctl->ngroups++;
+		}
+	}
+
+	dev_dbg(ipctl->dev, "function %s parsed %d/%d groups\n",
+		np->name, i, func->num_groups);
+
+	return 0;
+}
+
+static int rzn1_pinctrl_probe_dt(struct platform_device *pdev,
+				 struct rzn1_pinctrl *ipctl)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct device_node *child;
+	unsigned int maxgroups = 0;
+	u32 nfuncs = 0;
+	u32 i = 0;
+	int ret;
+
+	nfuncs = of_get_child_count(np);
+	if (nfuncs <= 0) {
+		dev_err(&pdev->dev, "no functions defined\n");
+		return -EINVAL;
+	}
+
+	ipctl->nfunctions = nfuncs;
+	ipctl->functions = devm_kmalloc_array(&pdev->dev, nfuncs,
+					      sizeof(*ipctl->functions),
+					      GFP_KERNEL);
+	if (!ipctl->functions)
+		return -ENOMEM;
+
+	ipctl->ngroups = 0;
+	for_each_child_of_node(np, child)
+		maxgroups += rzn1_pinctrl_count_function_groups(child);
+
+	ipctl->groups = devm_kmalloc_array(&pdev->dev,
+					   maxgroups,
+					   sizeof(*ipctl->groups),
+					   GFP_KERNEL);
+	if (!ipctl->groups)
+		return -ENOMEM;
+
+	for_each_child_of_node(np, child) {
+		ret = rzn1_pinctrl_parse_functions(child, ipctl, i++);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int rzn1_pinctrl_probe(struct platform_device *pdev)
+{
+	struct rzn1_pinctrl *ipctl;
+	struct resource *res;
+	int ret;
+
+	/* Create state holders etc for this driver */
+	ipctl = devm_kzalloc(&pdev->dev, sizeof(*ipctl), GFP_KERNEL);
+	if (!ipctl)
+		return -ENOMEM;
+
+	ipctl->mdio_func[0] = -1;
+	ipctl->mdio_func[1] = -1;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	ipctl->lev1_protect_phys = (u32)res->start + 0x400;
+	ipctl->lev1 = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(ipctl->lev1))
+		return PTR_ERR(ipctl->lev1);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	ipctl->lev2_protect_phys = (u32)res->start + 0x400;
+	ipctl->lev2 = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(ipctl->lev2))
+		return PTR_ERR(ipctl->lev2);
+
+	ipctl->clk = devm_clk_get(&pdev->dev, "bus");
+	if (IS_ERR(ipctl->clk))
+		return PTR_ERR(ipctl->clk);
+	ret = clk_prepare_enable(ipctl->clk);
+	if (ret)
+		return ret;
+
+	ipctl->dev = &pdev->dev;
+	rzn1_pinctrl_desc.name = dev_name(&pdev->dev);
+	rzn1_pinctrl_desc.pins = rzn1_pins;
+	rzn1_pinctrl_desc.npins = ARRAY_SIZE(rzn1_pins);
+
+	ret = rzn1_pinctrl_probe_dt(pdev, ipctl);
+	if (ret) {
+		dev_err(&pdev->dev, "fail to probe dt properties\n");
+		goto err_clk;
+	}
+
+	platform_set_drvdata(pdev, ipctl);
+	ipctl->pctl = pinctrl_register(&rzn1_pinctrl_desc, &pdev->dev, ipctl);
+	if (!ipctl->pctl) {
+		dev_err(&pdev->dev, "could not register rzn1 pinctrl driver\n");
+		ret = -EINVAL;
+		goto err_clk;
+	}
+
+	dev_info(&pdev->dev, "probed\n");
+
+	return 0;
+
+err_clk:
+	clk_disable_unprepare(ipctl->clk);
+
+	return ret;
+}
+
+static int rzn1_pinctrl_remove(struct platform_device *pdev)
+{
+	struct rzn1_pinctrl *ipctl = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(ipctl->clk);
+
+	return 0;
+}
+
+static const struct of_device_id rzn1_pinctrl_match[] = {
+	{ .compatible = "renesas,rzn1-pinctrl", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, rzn1_pinctrl_match);
+
+static struct platform_driver rzn1_pinctrl_driver = {
+	.probe	= rzn1_pinctrl_probe,
+	.remove = rzn1_pinctrl_remove,
+	.driver	= {
+		.name		= "rzn1-pinctrl",
+		.owner		= THIS_MODULE,
+		.of_match_table	= rzn1_pinctrl_match,
+	},
+};
+
+static int __init _pinctrl_drv_register(void)
+{
+	return platform_driver_register(&rzn1_pinctrl_driver);
+}
+subsys_initcall(_pinctrl_drv_register);
+
+MODULE_AUTHOR("Phil Edworthy <phil.edworthy@renesas.com>");
+MODULE_DESCRIPTION("Renesas RZ/N1 pinctrl driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

Re: [PATCH v3 2/3] pinctrl: renesas: Renesas RZ/N1 pinctrl driver

[jacopo mondi]

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

Hi Phil,
   thanks for the patch

On Mon, Sep 17, 2018 at 05:36:08PM +0100, Phil Edworthy wrote:
> This provides a pinctrl driver for the Renesas RZ/N1 device family.
>
> Based on a patch originally written by Michel Pollet at Renesas.
>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> ---
> v3:
>  - Use standard DT props instead of proprietary ones.
>  - Replace virtual pins used for MDIO muxing with extra funcs.
>  - Use pinctrl_utils funcs to handle the maps.
>  - Remove the dbg functions to keep things simple.
>
> v2:
>  - Change filename to generic rzn1, instead of device specific.
>  - Changed Kconfig symbol and file name to generic rzn1 family.
>  - Added "renesas,rzn1-pinctrl" compatible fallback string
>  - Changes suggested by Jacopo Mondi. Mainly formatting, plus:
>    - Removed global ptr
>    - Removed unused code accessing parent of node.
>    - Removed check for null OF np that can't happen.
>    - Replaced overlapping enums with #defines
>  - Renamed some variables and symbols to clarify their use
>  - Fix error handling during probe
>  - Move probe from postcore_initcall to subsys_initcall to ensure
>    drivers that require clocks get them instead of having to defer
>    probing.
> ---
>  drivers/pinctrl/Kconfig        |  10 +
>  drivers/pinctrl/Makefile       |   1 +
>  drivers/pinctrl/pinctrl-rzn1.c | 926 +++++++++++++++++++++++++++++++++
>  3 files changed, 937 insertions(+)
>  create mode 100644 drivers/pinctrl/pinctrl-rzn1.c
>
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index e86752be1f19..e524eb101384 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -195,6 +195,16 @@ config PINCTRL_RZA1
>  	help
>  	  This selects pinctrl driver for Renesas RZ/A1 platforms.
>
> +config PINCTRL_RZN1
> +	bool "Renesas RZ/N1 pinctrl driver"
> +	depends on OF
> +	depends on ARCH_RZN1 || COMPILE_TEST
> +	select GENERIC_PINCTRL_GROUPS
> +	select GENERIC_PINMUX_FUNCTIONS
> +	select GENERIC_PINCONF
> +	help
> +	  This selects pinctrl driver for Renesas RZ/N1 devices.
> +
>  config PINCTRL_SINGLE
>  	tristate "One-register-per-pin type device tree based pinctrl driver"
>  	depends on OF
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> index 46ef9bd52096..d07f9a20f6ae 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_PINCTRL_PIC32)	+= pinctrl-pic32.o
>  obj-$(CONFIG_PINCTRL_PISTACHIO)	+= pinctrl-pistachio.o
>  obj-$(CONFIG_PINCTRL_ROCKCHIP)	+= pinctrl-rockchip.o
>  obj-$(CONFIG_PINCTRL_RZA1)	+= pinctrl-rza1.o
> +obj-$(CONFIG_PINCTRL_RZN1)	+= pinctrl-rzn1.o
>  obj-$(CONFIG_PINCTRL_SINGLE)	+= pinctrl-single.o
>  obj-$(CONFIG_PINCTRL_SIRF)	+= sirf/
>  obj-$(CONFIG_PINCTRL_SX150X)	+= pinctrl-sx150x.o
> diff --git a/drivers/pinctrl/pinctrl-rzn1.c b/drivers/pinctrl/pinctrl-rzn1.c
> new file mode 100644
> index 000000000000..8f0caa266dbb
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-rzn1.c
> @@ -0,0 +1,926 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2014-2018 Renesas Electronics Europe Limited
> + *
> + * Phil Edworthy <phil.edworthy@renesas.com>
> + * Based on a driver originally written by Michel Pollet at Renesas.
> + */
> +
> +#include <dt-bindings/pinctrl/rzn1-pinctrl.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include "core.h"
> +#include "pinconf.h"
> +#include "pinctrl-utils.h"
> +
> +/* Field positions and masks in the pinmux registers */
> +#define RZN1_L1_PIN_DRIVE_STRENGTH	10
> +#define RZN1_L1_PIN_DRIVE_STRENGTH_4MA	0
> +#define RZN1_L1_PIN_DRIVE_STRENGTH_6MA	1
> +#define RZN1_L1_PIN_DRIVE_STRENGTH_8MA	2
> +#define RZN1_L1_PIN_DRIVE_STRENGTH_12MA	3
> +#define RZN1_L1_PIN_PULL		8
> +#define RZN1_L1_PIN_PULL_NONE		0
> +#define RZN1_L1_PIN_PULL_UP		1
> +#define RZN1_L1_PIN_PULL_DOWN		3
> +#define RZN1_L1_FUNCTION		0
> +#define RZN1_L1_FUNC_MASK		0xf
> +#define RZN1_L1_FUNCTION_L2		0xf
> +
> +/*
> + * The hardware manual describes two levels of multiplexing, but it's more
> + * logical to think of the hardware as three levels, with level 3 consisting of
> + * the multiplexing for Ethernet MDIO signals.
> + *
> + * Level 1 functions go from 0 to 9, with level 1 function '15' (0xf) specifying
> + * that level 2 functions are used instead. Level 2 has a lot more options,
> + * going from 0 to 61. Level 3 allows selection of MDIO functions which can be
> + * floating, or one of seven internal peripherals. Unfortunately, there are two
> + * level 2 functions that can select MDIO, and two MDIO channels so we have four
> + * sets of level 3 functions.
> + *
> + * For this driver, we've compounded the numbers together, so:
> + *    0 to   9 is level 1
> + *   10 to  71 is 10 + level 2 number
> + *   72 to  79 is 72 + MDIO0 source for level 2 MDIO function.
> + *   80 to  87 is 80 + MDIO0 source for level 2 MDIO_E1 function.
> + *   88 to  95 is 88 + MDIO1 source for level 2 MDIO function.
> + *   96 to 103 is 96 + MDIO1 source for level 2 MDIO_E1 function.
> + * Examples:
> + *  Function 28 corresponds UART0
> + *  Function 73 corresponds to MDIO0 to GMAC0
> + *
> + * There are 170 configurable pins (called PL_GPIO in the datasheet).
> + */
> +
> +/*
> + * Structure detailing the HW registers on the RZ/N1 devices.
> + * Both the Level 1 mux registers and Level 2 mux registers have the same
> + * structure. The only difference is that Level 2 has additional MDIO registers
> + * at the end.
> + */
> +struct rzn1_pinctrl_regs {
> +	union {
> +		u32	conf[170];
> +		u8	pad0[0x400];

Is pad0 actually used?

> +	};
> +	u32	status_protect;	/* 0x400 */
> +	/* MDIO mux registers, level2 only */
> +	u32	l2_mdio[2];
> +};
> +
> +/**
> + * struct rzn1_pmx_func - describes rzn1 pinmux functions
> + * @name: the name of this specific function
> + * @groups: corresponding pin groups
> + * @num_groups: the number of groups
> + */
> +struct rzn1_pmx_func {
> +	const char *name;
> +	const char **groups;
> +	unsigned int num_groups;
> +};
> +
> +/**
> + * struct rzn1_pin_group - describes an rzn1 pin group
> + * @name: the name of this specific pin group
> + * @func: the name of the function selected by this group
> + * @npins: the number of pins in this group array, i.e. the number of
> + *	elements in .pins so we can iterate over that array
> + * @pin_ids: array of pin_ids, i.e. the value used to select the mux
> + * @pins: array of pins. Needed due to pinctrl_ops.get_group_pins()
> + */
> +struct rzn1_pin_group {
> +	const char *name;
> +	const char *func;
> +	unsigned int npins;
> +	u8 *pin_ids;
> +	u32 *pins;
> +};
> +
> +struct rzn1_pinctrl {
> +	struct device *dev;
> +	struct clk *clk;
> +	struct pinctrl_dev *pctl;
> +	struct rzn1_pinctrl_regs __iomem *lev1;
> +	struct rzn1_pinctrl_regs __iomem *lev2;
> +	u32 lev1_protect_phys;
> +	u32 lev2_protect_phys;
> +	int mdio_func[2];
> +
> +	struct rzn1_pin_group *groups;
> +	unsigned int ngroups;
> +
> +	struct rzn1_pmx_func *functions;
> +	unsigned int nfunctions;
> +};
> +
> +#define RZN1_PINS_PROP "pinmux"
> +
> +#define RZN1_PIN(pin) PINCTRL_PIN(pin, "pl_gpio"#pin)
> +
> +static const struct pinctrl_pin_desc rzn1_pins[] = {
> +	RZN1_PIN(0), RZN1_PIN(1), RZN1_PIN(2), RZN1_PIN(3), RZN1_PIN(4),
> +	RZN1_PIN(5), RZN1_PIN(6), RZN1_PIN(7), RZN1_PIN(8), RZN1_PIN(9),
> +	RZN1_PIN(10), RZN1_PIN(11), RZN1_PIN(12), RZN1_PIN(13), RZN1_PIN(14),
> +	RZN1_PIN(15), RZN1_PIN(16), RZN1_PIN(17), RZN1_PIN(18), RZN1_PIN(19),
> +	RZN1_PIN(20), RZN1_PIN(21), RZN1_PIN(22), RZN1_PIN(23), RZN1_PIN(24),
> +	RZN1_PIN(25), RZN1_PIN(26), RZN1_PIN(27), RZN1_PIN(28), RZN1_PIN(29),
> +	RZN1_PIN(30), RZN1_PIN(31), RZN1_PIN(32), RZN1_PIN(33), RZN1_PIN(34),
> +	RZN1_PIN(35), RZN1_PIN(36), RZN1_PIN(37), RZN1_PIN(38), RZN1_PIN(39),
> +	RZN1_PIN(40), RZN1_PIN(41), RZN1_PIN(42), RZN1_PIN(43), RZN1_PIN(44),
> +	RZN1_PIN(45), RZN1_PIN(46), RZN1_PIN(47), RZN1_PIN(48), RZN1_PIN(49),
> +	RZN1_PIN(50), RZN1_PIN(51), RZN1_PIN(52), RZN1_PIN(53), RZN1_PIN(54),
> +	RZN1_PIN(55), RZN1_PIN(56), RZN1_PIN(57), RZN1_PIN(58), RZN1_PIN(59),
> +	RZN1_PIN(60), RZN1_PIN(61), RZN1_PIN(62), RZN1_PIN(63), RZN1_PIN(64),
> +	RZN1_PIN(65), RZN1_PIN(66), RZN1_PIN(67), RZN1_PIN(68), RZN1_PIN(69),
> +	RZN1_PIN(70), RZN1_PIN(71), RZN1_PIN(72), RZN1_PIN(73), RZN1_PIN(74),
> +	RZN1_PIN(75), RZN1_PIN(76), RZN1_PIN(77), RZN1_PIN(78), RZN1_PIN(79),
> +	RZN1_PIN(80), RZN1_PIN(81), RZN1_PIN(82), RZN1_PIN(83), RZN1_PIN(84),
> +	RZN1_PIN(85), RZN1_PIN(86), RZN1_PIN(87), RZN1_PIN(88), RZN1_PIN(89),
> +	RZN1_PIN(90), RZN1_PIN(91), RZN1_PIN(92), RZN1_PIN(93), RZN1_PIN(94),
> +	RZN1_PIN(95), RZN1_PIN(96), RZN1_PIN(97), RZN1_PIN(98), RZN1_PIN(99),
> +	RZN1_PIN(100), RZN1_PIN(101), RZN1_PIN(102), RZN1_PIN(103),
> +	RZN1_PIN(104), RZN1_PIN(105), RZN1_PIN(106), RZN1_PIN(107),
> +	RZN1_PIN(108), RZN1_PIN(109), RZN1_PIN(110), RZN1_PIN(111),
> +	RZN1_PIN(112), RZN1_PIN(113), RZN1_PIN(114), RZN1_PIN(115),
> +	RZN1_PIN(116), RZN1_PIN(117), RZN1_PIN(118), RZN1_PIN(119),
> +	RZN1_PIN(120), RZN1_PIN(121), RZN1_PIN(122), RZN1_PIN(123),
> +	RZN1_PIN(124), RZN1_PIN(125), RZN1_PIN(126), RZN1_PIN(127),
> +	RZN1_PIN(128), RZN1_PIN(129), RZN1_PIN(130), RZN1_PIN(131),
> +	RZN1_PIN(132), RZN1_PIN(133), RZN1_PIN(134), RZN1_PIN(135),
> +	RZN1_PIN(136), RZN1_PIN(137), RZN1_PIN(138), RZN1_PIN(139),
> +	RZN1_PIN(140), RZN1_PIN(141), RZN1_PIN(142), RZN1_PIN(143),
> +	RZN1_PIN(144), RZN1_PIN(145), RZN1_PIN(146), RZN1_PIN(147),
> +	RZN1_PIN(148), RZN1_PIN(149), RZN1_PIN(150), RZN1_PIN(151),
> +	RZN1_PIN(152), RZN1_PIN(153), RZN1_PIN(154), RZN1_PIN(155),
> +	RZN1_PIN(156), RZN1_PIN(157), RZN1_PIN(158), RZN1_PIN(159),
> +	RZN1_PIN(160), RZN1_PIN(161), RZN1_PIN(162), RZN1_PIN(163),
> +	RZN1_PIN(164), RZN1_PIN(165), RZN1_PIN(166), RZN1_PIN(167),
> +	RZN1_PIN(168), RZN1_PIN(169),
> +};
> +
> +enum {
> +	LOCK_LEVEL1 = 0x1,
> +	LOCK_LEVEL2 = 0x2,
> +	LOCK_ALL = LOCK_LEVEL1 | LOCK_LEVEL2,

0x03 is already (0x01 | 0x02) :)
Not a big deal though.

> +};
> +
> +static void rzn1_hw_set_lock(struct rzn1_pinctrl *ipctl, u8 lock, u8 value)
> +{
> +	/*
> +	 * The pinmux configuration is locked by writing the physical address of
> +	 * the status_protect register to itself. It is unlocked by writing the
> +	 * address | 1.
> +	 */
> +	if (lock & LOCK_LEVEL1) {
> +		u32 val = ipctl->lev1_protect_phys | !(value & LOCK_LEVEL1);
> +
> +		writel(val, &ipctl->lev1->status_protect);
> +	}
> +
> +	if (lock & LOCK_LEVEL2) {
> +		u32 val = ipctl->lev2_protect_phys | !(value & LOCK_LEVEL2);
> +
> +		writel(val, &ipctl->lev2->status_protect);
> +	}
> +}
> +
> +static void rzn1_pinctrl_mdio_select(struct rzn1_pinctrl *ipctl, u8 mdio,
> +				     u32 func)
> +{
> +	if (ipctl->mdio_func[mdio] >= 0 && ipctl->mdio_func[mdio] != func)
> +		dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n", mdio);
> +	ipctl->mdio_func[mdio] = func;
> +
> +	dev_dbg(ipctl->dev, "setting mdio%d to %d\n", mdio, func);
> +
> +	writel(func, &ipctl->lev2->l2_mdio[mdio]);
> +}
> +
> +/*
> + * Using a composite pin description, set the hardware pinmux registers
> + * with the corresponding values.
> + * Make sure to unlock write protection and reset it afterward.
> + *
> + * NOTE: There is no protection for potential concurrency, it is assumed these
> + * calls are serialized already.
> + */
> +static int rzn1_set_hw_pin_func(struct rzn1_pinctrl *ipctl, u32 pin,
> +				u32 pin_config, u8 use_locks)
> +{
> +	u32 l1_cache;
> +	u32 l2_cache;
> +	u32 l1;
> +	u32 l2;
> +
> +	/* Level 3 MDIO multiplexing */
> +	if (pin_config >= RZN1_FUNC_MDIO0_HIGHZ &&
> +	    pin_config <= RZN1_FUNC_MDIO1_E1_SWITCH) {
> +		u8 mdio_channel;
> +		u32 mdio_func;
> +
> +		if (pin_config <= RZN1_FUNC_MDIO1_HIGHZ)
> +			mdio_channel = 0;
> +		else
> +			mdio_channel = 1;
> +
> +		/* Get MDIO func, and convert the func to the level 2 number */
> +		if (pin_config <= RZN1_FUNC_MDIO0_SWITCH) {
> +			mdio_func = pin_config - RZN1_FUNC_MDIO0_HIGHZ;
> +			pin_config = RZN1_FUNC_ETH_MDIO;
> +		} else if (pin_config <= RZN1_FUNC_MDIO0_E1_SWITCH) {
> +			mdio_func = pin_config - RZN1_FUNC_MDIO0_E1_HIGHZ;
> +			pin_config = RZN1_FUNC_ETH_MDIO_E1;
> +		} else if (pin_config <= RZN1_FUNC_MDIO1_SWITCH) {
> +			mdio_func = pin_config - RZN1_FUNC_MDIO1_HIGHZ;
> +			pin_config = RZN1_FUNC_ETH_MDIO;
> +		} else {
> +			mdio_func = pin_config - RZN1_FUNC_MDIO1_E1_HIGHZ;
> +			pin_config = RZN1_FUNC_ETH_MDIO_E1;
> +		}
> +		rzn1_pinctrl_mdio_select(ipctl, mdio_channel, mdio_func);
> +	}
> +
> +	/* Note here, we do not allow anything past the MDIO Mux values */
> +	if (pin >= ARRAY_SIZE(ipctl->lev1->conf) ||
> +	    pin_config >= RZN1_FUNC_MDIO0_HIGHZ)
> +		return -EINVAL;
> +
> +	l1 = readl(&ipctl->lev1->conf[pin]);
> +	l1_cache = l1;
> +	l2 = readl(&ipctl->lev2->conf[pin]);
> +	l2_cache = l2;
> +
> +	dev_dbg(ipctl->dev, "setting func for pin %d to %d\n", pin, pin_config);
> +
> +	if (pin_config < RZN1_FUNC_L2_OFFSET) {
> +		l1 &= ~(RZN1_L1_FUNC_MASK << RZN1_L1_FUNCTION);
> +		l1 |= (pin_config << RZN1_L1_FUNCTION);
> +	} else {
> +		l1 &= ~(RZN1_L1_FUNC_MASK << RZN1_L1_FUNCTION);
> +		l1 |= (RZN1_L1_FUNCTION_L2 << RZN1_L1_FUNCTION);
> +
> +		l2 = pin_config - RZN1_FUNC_L2_OFFSET;
> +	}
> +
> +	/* If either configuration changes, we update both anyway */
> +	if (l1 != l1_cache || l2 != l2_cache) {
> +		writel(l1, &ipctl->lev1->conf[pin]);
> +		writel(l2, &ipctl->lev2->conf[pin]);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct rzn1_pin_group *rzn1_pinctrl_find_group_by_name(
> +	const struct rzn1_pinctrl *ipctl, const char *name)
> +{
> +	const struct rzn1_pin_group *grp = NULL;
> +	int i;
> +
> +	for (i = 0; i < ipctl->ngroups; i++) {
> +		if (!strcmp(ipctl->groups[i].name, name)) {
> +			grp = &ipctl->groups[i];
> +			break;
> +		}
> +	}
> +
> +	return grp;
> +}
> +
> +static int rzn1_get_groups_count(struct pinctrl_dev *pctldev)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return ipctl->ngroups;
> +}
> +
> +static const char *rzn1_get_group_name(struct pinctrl_dev *pctldev,
> +				       unsigned int selector)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return ipctl->groups[selector].name;
> +}
> +
> +static int rzn1_get_group_pins(struct pinctrl_dev *pctldev,
> +			       unsigned int selector, const unsigned int **pins,
> +			       unsigned int *npins)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	if (selector >= ipctl->ngroups)
> +		return -EINVAL;
> +
> +	*pins = ipctl->groups[selector].pins;
> +	*npins = ipctl->groups[selector].npins;
> +
> +	return 0;
> +}
> +
> +/*
> + * This function is called for each pinctl 'Function' node.
> + * Sub-nodes can be used to describe multiple 'Groups' for the 'Function'
> + * If there aren't any sub-nodes, the 'Group' is essentially the 'Function'.
> + * Each 'Group' uses pinmux = <...> to detail the pins and data used to select
> + * the functionality. Each 'Group' has optional pin configurations that apply
> + * to all pins in the 'Group'.
> + */
> +static int rzn1_dt_node_to_map_one(struct pinctrl_dev *pctldev,
> +				   struct device_node *np,
> +				   struct pinctrl_map **map,
> +				   unsigned int *num_maps)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +	const struct rzn1_pin_group *grp;
> +	unsigned long *configs = NULL;
> +	unsigned int num_configs = 0;
> +	unsigned int reserved_maps = *num_maps;
> +	unsigned int reserve = 1;
> +	int ret;
> +
> +	dev_dbg(ipctl->dev, "processing node %pOF\n", np);
> +
> +	grp = rzn1_pinctrl_find_group_by_name(ipctl, np->name);
> +	if (!grp) {
> +		dev_err(ipctl->dev, "unable to find group for node %pOF\n", np);
> +		return -EINVAL;
> +	}
> +
> +	/* Get the group's pin configuration */
> +	ret = pinconf_generic_parse_dt_config(np, pctldev, &configs,
> +					      &num_configs);
> +	if (ret < 0) {
> +		dev_err(ipctl->dev, "%s: could not parse node property\n",
> +			of_node_full_name(np));

Why %pOF above and of_node_full_name? I would use the first, if
nothing changed recently with OF naming.

> +		return ret;
> +	}
> +
> +	if (num_configs)
> +		reserve++;
> +
> +	/* Increase the number of maps to cover this group */
> +	ret = pinctrl_utils_reserve_map(pctldev, map, &reserved_maps, num_maps,
> +					reserve);
> +	if (ret < 0)
> +		goto out;
> +
> +	/* Associate the group with the function */
> +	ret = pinctrl_utils_add_map_mux(pctldev, map, &reserved_maps, num_maps,
> +					grp->name, grp->func);
> +	if (ret < 0)
> +		goto out;
> +
> +	if (num_configs) {
> +		/* Associate the group's pin configuration with the group */
> +		ret = pinctrl_utils_add_map_configs(pctldev, map,
> +				&reserved_maps, num_maps, grp->name,
> +				configs, num_configs,
> +				PIN_MAP_TYPE_CONFIGS_GROUP);
> +		if (ret < 0)
> +			goto out;
> +	}
> +
> +	dev_dbg(pctldev->dev, "maps: function %s group %s (%d pins)\n",
> +		grp->func, grp->name, grp->npins);
> +
> +out:
> +	kfree(configs);
> +	return 0;

You never return error here (nit: newline before returning, here and
in other places)

> +}
> +
> +static int rzn1_dt_node_to_map(struct pinctrl_dev *pctldev,
> +			       struct device_node *np,
> +			       struct pinctrl_map **map,
> +			       unsigned int *num_maps)
> +{
> +	struct device_node *child;
> +	int ret;
> +
> +	*map = NULL;
> +	*num_maps = 0;
> +
> +	ret = rzn1_dt_node_to_map_one(pctldev, np, map, num_maps);
> +	if (ret < 0)
> +		return ret;
> +
> +	for_each_child_of_node(np, child) {
> +		ret = rzn1_dt_node_to_map_one(pctldev, child, map, num_maps);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct pinctrl_ops rzn1_pctrl_ops = {
> +	.get_groups_count = rzn1_get_groups_count,
> +	.get_group_name = rzn1_get_group_name,
> +	.get_group_pins = rzn1_get_group_pins,
> +	.dt_node_to_map = rzn1_dt_node_to_map,
> +	.dt_free_map = pinctrl_utils_free_map,
> +};
> +
> +static int rzn1_pmx_get_funcs_count(struct pinctrl_dev *pctldev)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return ipctl->nfunctions;
> +}
> +
> +static const char *rzn1_pmx_get_func_name(struct pinctrl_dev *pctldev,
> +					  unsigned int selector)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return ipctl->functions[selector].name;
> +}
> +
> +static int rzn1_pmx_get_groups(struct pinctrl_dev *pctldev,
> +			       unsigned int selector,
> +			       const char * const **groups,
> +			       unsigned int * const num_groups)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	*groups = ipctl->functions[selector].groups;
> +	*num_groups = ipctl->functions[selector].num_groups;
> +
> +	return 0;
> +}
> +
> +static int rzn1_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
> +			unsigned int group)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +	struct rzn1_pin_group *grp = &ipctl->groups[group];
> +	unsigned int i, grp_pins = grp->npins;
> +
> +	dev_dbg(ipctl->dev, "set mux %s(%d) group %s(%d)\n",
> +		ipctl->functions[selector].name, selector, grp->name, group);
> +
> +	rzn1_hw_set_lock(ipctl, LOCK_ALL, LOCK_ALL);
> +	for (i = 0; i < grp_pins; i++)
> +		rzn1_set_hw_pin_func(ipctl, grp->pins[i], grp->pin_ids[i], 0);
> +	rzn1_hw_set_lock(ipctl, LOCK_ALL, 0);
> +
> +	return 0;
> +}
> +
> +static const struct pinmux_ops rzn1_pmx_ops = {
> +	.get_functions_count = rzn1_pmx_get_funcs_count,
> +	.get_function_name = rzn1_pmx_get_func_name,
> +	.get_function_groups = rzn1_pmx_get_groups,
> +	.set_mux = rzn1_set_mux,
> +};
> +
> +static int rzn1_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
> +			    unsigned long *config)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +	enum pin_config_param param = pinconf_to_config_param(*config);
> +	const u32 reg_drive[4] = { 4, 6, 8, 12 };
> +	u32 l1, l2;
> +	u32 pull, drive, l1mux;
> +	u32 arg = 0;
> +
> +	if (pin >= ARRAY_SIZE(ipctl->lev1->conf))
> +		return -EINVAL;
> +
> +	l1 = readl(&ipctl->lev1->conf[pin]);
> +
> +	l1mux = l1 & RZN1_L1_FUNC_MASK;
> +	pull = (l1 >> RZN1_L1_PIN_PULL) & 0x3;
> +	drive = (l1 >> RZN1_L1_PIN_DRIVE_STRENGTH) & 0x3;
> +
> +	switch (param) {
> +	case PIN_CONFIG_BIAS_PULL_UP:
> +		if (pull == RZN1_L1_PIN_PULL_UP)
> +			arg = 1;
> +		break;
> +	case PIN_CONFIG_BIAS_PULL_DOWN:
> +		if (pull == RZN1_L1_PIN_PULL_DOWN)
> +			arg = 1;
> +		break;
> +	case PIN_CONFIG_BIAS_DISABLE:
> +		if (pull == RZN1_L1_PIN_PULL_NONE)
> +			arg = 1;
> +		break;
> +	case PIN_CONFIG_DRIVE_STRENGTH:
> +		arg = reg_drive[drive];
> +		break;
> +	case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
> +		l2 = readl(&ipctl->lev1->conf[pin]);
> +		if (l1mux == RZN1_FUNC_HIGHZ)
> +			arg = 1;
> +		if (l1mux == RZN1_L1_FUNCTION_L2 && l2 == 0)
> +			arg = 1;
> +		break;
> +	default:
> +		return -ENOTSUPP;
> +	}
> +
> +	*config = pinconf_to_config_packed(param, arg);
> +
> +	return 0;
> +}
> +
> +static int rzn1_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
> +			    unsigned long *configs, unsigned int num_configs)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +	enum pin_config_param param;
> +	int i;
> +	u32 arg;
> +	u32 l1, l1_cache;
> +	u32 drv;
> +
> +	if (pin >= ARRAY_SIZE(ipctl->lev1->conf))
> +		return -EINVAL;
> +
> +	l1 = readl(&ipctl->lev1->conf[pin]);
> +	l1_cache = l1;
> +
> +	for (i = 0; i < num_configs; i++) {
> +		param = pinconf_to_config_param(configs[i]);
> +		arg = pinconf_to_config_argument(configs[i]);
> +
> +		switch (param) {
> +		case PIN_CONFIG_BIAS_PULL_UP:
> +			dev_dbg(ipctl->dev, "set pin %d pull up\n", pin);
> +			l1 &= ~(0x3 << RZN1_L1_PIN_PULL);
> +			l1 |= (RZN1_L1_PIN_PULL_UP << RZN1_L1_PIN_PULL);
> +			break;
> +		case PIN_CONFIG_BIAS_PULL_DOWN:
> +			dev_dbg(ipctl->dev, "set pin %d pull down\n", pin);
> +			l1 &= ~(0x3 << RZN1_L1_PIN_PULL);
> +			l1 |= (RZN1_L1_PIN_PULL_DOWN << RZN1_L1_PIN_PULL);
> +			break;
> +		case PIN_CONFIG_BIAS_DISABLE:
> +			dev_dbg(ipctl->dev, "set pin %d bias off\n", pin);
> +			l1 &= ~(0x3 << RZN1_L1_PIN_PULL);
> +			l1 |= (RZN1_L1_PIN_PULL_NONE << RZN1_L1_PIN_PULL);
> +			break;
> +		case PIN_CONFIG_DRIVE_STRENGTH:
> +			dev_dbg(ipctl->dev, "set pin %d drv %umA\n", pin, arg);
> +			drv = 0;
> +			switch (arg) {
> +			case 4:
> +				drv = RZN1_L1_PIN_DRIVE_STRENGTH_4MA;
> +				break;
> +			case 6:
> +				drv = RZN1_L1_PIN_DRIVE_STRENGTH_6MA;
> +				break;
> +			case 8:
> +				drv = RZN1_L1_PIN_DRIVE_STRENGTH_8MA;
> +				break;
> +			case 12:
> +				drv = RZN1_L1_PIN_DRIVE_STRENGTH_12MA;
> +				break;
> +			default:
> +				dev_err(ipctl->dev,
> +					"Drive strength %umA not supported\n",
> +					arg);
> +				return -EINVAL;
> +			}
> +
> +			l1 &= ~(0x3 << RZN1_L1_PIN_DRIVE_STRENGTH);
> +			l1 |= (drv << RZN1_L1_PIN_DRIVE_STRENGTH);
> +			break;
> +
> +		case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
> +			dev_dbg(ipctl->dev, "set pin %d High-Z\n", pin);
> +			l1 &= ~RZN1_L1_FUNC_MASK;
> +			l1 |= RZN1_FUNC_HIGHZ;
> +			break;
> +		default:

I'll re-comment on bindings too, but the pinconf properties you
support should be listed there

> +			return -ENOTSUPP;
> +		}
> +	}
> +
> +	if (l1 != l1_cache) {
> +		rzn1_hw_set_lock(ipctl, LOCK_LEVEL1, LOCK_LEVEL1);
> +		writel(l1, &ipctl->lev1->conf[pin]);
> +		rzn1_hw_set_lock(ipctl, LOCK_LEVEL1, 0);
> +	}
> +
> +	return 0;
> +}
> +
> +static int rzn1_pin_config_group_set(struct pinctrl_dev *pctldev,
> +				     unsigned int selector,
> +				     unsigned long *configs,
> +				     unsigned int num_configs)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +	struct rzn1_pin_group *grp = &ipctl->groups[selector];
> +	int ret, i;
> +
> +	dev_dbg(ipctl->dev, "group set %s selector:%d configs:%p/%d\n",
> +		grp->name, selector, configs, num_configs);
> +
> +	for (i = 0; i < grp->npins; i++) {
> +		unsigned int pin = grp->pins[i];
> +
> +		ret = rzn1_pinconf_set(pctldev, pin, configs, num_configs);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct pinconf_ops rzn1_pinconf_ops = {
> +	.pin_config_get = rzn1_pinconf_get,
> +	.pin_config_set = rzn1_pinconf_set,
> +	.pin_config_group_set = rzn1_pin_config_group_set,

Nit: you register a pinconf map with PIN_MAP_TYPE_CONFIGS_GROUP so the
'pinconf_[set|get]' should never be called. Have you tested if they
do ? (just for my better understanding of the pinctrl framework :)

> +};
> +
> +static struct pinctrl_desc rzn1_pinctrl_desc = {
> +	.pctlops = &rzn1_pctrl_ops,
> +	.pmxops = &rzn1_pmx_ops,
> +	.confops = &rzn1_pinconf_ops,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int rzn1_pinctrl_parse_groups(struct device_node *np,
> +				     struct rzn1_pin_group *grp,
> +				     struct rzn1_pinctrl *ipctl)
> +{
> +	int size;
> +	const __be32 *list;
> +	int i;

unsigned int i;

It is only used in the bottom for loop afaict

> +
> +	dev_dbg(ipctl->dev, "%s: %s\n", __func__, np->name);
> +
> +	/* Initialise group */
> +	grp->name = np->name;
> +
> +	/*
> +	 * The binding format is
> +	 *	pinmux = <PIN_FUNC_ID CONFIG ...>,
> +	 * do sanity check and calculate pins number
> +	 */
> +	list = of_get_property(np, RZN1_PINS_PROP, &size);
> +	if (!list) {
> +		dev_err(ipctl->dev,
> +			"no " RZN1_PINS_PROP " property in node %s\n",
> +			np->full_name);

Isn't it better to print out the OF node name with %pOF? Here and
below...

> +		return -EINVAL;
> +	}
> +
> +	/* We do not check return since it's safe node passed down */
> +	if (!size) {
> +		dev_err(ipctl->dev, "Invalid " RZN1_PINS_PROP " in node %s\n",
> +			np->full_name);
> +		return -EINVAL;
> +	}
> +
> +	grp->npins = size / sizeof(list[0]);
> +	if (!grp->npins)
> +		return 0;

If you get here the property is there and has > 0 length
> +
> +	grp->pin_ids = devm_kmalloc_array(ipctl->dev,
> +					  grp->npins, sizeof(grp->pin_ids[0]),
> +					  GFP_KERNEL);
> +	grp->pins = devm_kmalloc_array(ipctl->dev,
> +				       grp->npins, sizeof(grp->pins[0]),
> +				       GFP_KERNEL);
> +	if (!grp->pin_ids || !grp->pins)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < grp->npins; i++) {
> +		u32 pin_id = be32_to_cpu(*list++);
> +
> +		grp->pins[i] = pin_id & 0xff;
> +		grp->pin_ids[i] = (pin_id >> 8) & 0x7f;
> +	}
> +
> +	return grp->npins;
> +}
> +
> +static int rzn1_pinctrl_count_function_groups(struct device_node *np)
> +{
> +	struct device_node *child;
> +	int count = 0;
> +
> +	if (of_property_count_u32_elems(np, RZN1_PINS_PROP) > 0)
> +		count++;
> +
> +	for_each_child_of_node(np, child) {
> +		if (of_property_count_u32_elems(child, RZN1_PINS_PROP) > 0)
> +			count++;
> +	}
> +
> +	return count;
> +}
> +
> +static int rzn1_pinctrl_parse_functions(struct device_node *np,
> +					struct rzn1_pinctrl *ipctl, u32 index)
> +{
> +	struct device_node *child;
> +	struct rzn1_pmx_func *func;
> +	struct rzn1_pin_group *grp;
> +	u32 i = 0;
> +	int ret;
> +
> +	dev_dbg(ipctl->dev, "parse function(%d): %s\n", index, np->name);
> +
> +	func = &ipctl->functions[index];
> +
> +	/* Initialise function */
> +	func->name = np->name;
> +	func->num_groups = rzn1_pinctrl_count_function_groups(np);
> +	dev_dbg(ipctl->dev, "function %s has %d groups\n",
> +		np->name, func->num_groups);

Maybe move this debug printout below the error check, it helps to know
parsing went fine.

> +	if (func->num_groups == 0) {
> +		dev_err(ipctl->dev, "no groups defined in %s\n", np->full_name);
> +		return -EINVAL;
> +	}
> +
> +	func->groups = devm_kmalloc_array(ipctl->dev,
> +					  func->num_groups, sizeof(char *),
> +					  GFP_KERNEL);
> +	if (!func->groups)
> +		return -ENOMEM;
> +
> +	if (of_property_count_u32_elems(np, RZN1_PINS_PROP) > 0) {
> +		func->groups[i] = np->name;
> +		grp = &ipctl->groups[ipctl->ngroups];
> +		grp->func = func->name;
> +		ret = rzn1_pinctrl_parse_groups(np, grp, ipctl);
> +		if (ret < 0)
> +			return ret;
> +		if (ret > 0) {

You never return grp->npins == 0; it's either error or > 0. I think
you can drop this check.

> +			i++;
> +			ipctl->ngroups++;
> +		}

This design allows a sub-node with the following layout

        n-1 {
                pinmux = <..>;

                sub-n-1 {
                        pinmux = <..>;
                };

                sub-n-2 {
                        pinmux = <..>;
                };
        };

The bindings documentation only allows either this

        n-1 {
                pinmux = <..>;
        };

Or this
        n-1 {
                sub-n-1 {
                        pinmux = <..>;
                };

                sub-n-2 {
                        pinmux = <..>;
                };
        };

So I guess it's one or the other here. What do you think?

> +	}
> +
> +	for_each_child_of_node(np, child) {
> +		func->groups[i] = child->name;
> +		grp = &ipctl->groups[ipctl->ngroups];
> +		grp->func = func->name;
> +		ret = rzn1_pinctrl_parse_groups(child, grp, ipctl);
> +		if (ret < 0)
> +			return ret;
> +		if (ret > 0) {
> +			i++;
> +			ipctl->ngroups++;
> +		}
> +	}
> +
> +	dev_dbg(ipctl->dev, "function %s parsed %d/%d groups\n",
> +		np->name, i, func->num_groups);
> +
> +	return 0;
> +}
> +
> +static int rzn1_pinctrl_probe_dt(struct platform_device *pdev,
> +				 struct rzn1_pinctrl *ipctl)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device_node *child;
> +	unsigned int maxgroups = 0;
> +	u32 nfuncs = 0;
> +	u32 i = 0;
> +	int ret;
> +
> +	nfuncs = of_get_child_count(np);
> +	if (nfuncs <= 0) {
> +		dev_err(&pdev->dev, "no functions defined\n");
> +		return -EINVAL;
> +	}

Do you really want to fail if no pinmuxing sub-node is defined? I can
think of boards which includes the SoC .dtsi but do not define (yet)
any pinmux/conf sub-nodes. This shouldn't be an error.

> +
> +	ipctl->nfunctions = nfuncs;
> +	ipctl->functions = devm_kmalloc_array(&pdev->dev, nfuncs,
> +					      sizeof(*ipctl->functions),
> +					      GFP_KERNEL);
> +	if (!ipctl->functions)
> +		return -ENOMEM;
> +
> +	ipctl->ngroups = 0;
> +	for_each_child_of_node(np, child)
> +		maxgroups += rzn1_pinctrl_count_function_groups(child);
> +
> +	ipctl->groups = devm_kmalloc_array(&pdev->dev,
> +					   maxgroups,
> +					   sizeof(*ipctl->groups),
> +					   GFP_KERNEL);
> +	if (!ipctl->groups)
> +		return -ENOMEM;
> +
> +	for_each_child_of_node(np, child) {
> +		ret = rzn1_pinctrl_parse_functions(child, ipctl, i++);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rzn1_pinctrl_probe(struct platform_device *pdev)
> +{
> +	struct rzn1_pinctrl *ipctl;
> +	struct resource *res;
> +	int ret;
> +
> +	/* Create state holders etc for this driver */
> +	ipctl = devm_kzalloc(&pdev->dev, sizeof(*ipctl), GFP_KERNEL);
> +	if (!ipctl)
> +		return -ENOMEM;
> +
> +	ipctl->mdio_func[0] = -1;
> +	ipctl->mdio_func[1] = -1;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	ipctl->lev1_protect_phys = (u32)res->start + 0x400;
> +	ipctl->lev1 = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(ipctl->lev1))
> +		return PTR_ERR(ipctl->lev1);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	ipctl->lev2_protect_phys = (u32)res->start + 0x400;
> +	ipctl->lev2 = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(ipctl->lev2))
> +		return PTR_ERR(ipctl->lev2);
> +
> +	ipctl->clk = devm_clk_get(&pdev->dev, "bus");
> +	if (IS_ERR(ipctl->clk))
> +		return PTR_ERR(ipctl->clk);
> +	ret = clk_prepare_enable(ipctl->clk);
> +	if (ret)
> +		return ret;
> +
> +	ipctl->dev = &pdev->dev;
> +	rzn1_pinctrl_desc.name = dev_name(&pdev->dev);
> +	rzn1_pinctrl_desc.pins = rzn1_pins;
> +	rzn1_pinctrl_desc.npins = ARRAY_SIZE(rzn1_pins);
> +
> +	ret = rzn1_pinctrl_probe_dt(pdev, ipctl);
> +	if (ret) {
> +		dev_err(&pdev->dev, "fail to probe dt properties\n");
> +		goto err_clk;
> +	}
> +
> +	platform_set_drvdata(pdev, ipctl);
> +	ipctl->pctl = pinctrl_register(&rzn1_pinctrl_desc, &pdev->dev, ipctl);
> +	if (!ipctl->pctl) {
> +		dev_err(&pdev->dev, "could not register rzn1 pinctrl driver\n");
> +		ret = -EINVAL;
> +		goto err_clk;
> +	}

Bindings claim this is a pinctrl/gpio controller, but I don't see any
gpio support here. I'll comment on the bindings for reference too.

Thanks
   j

> +
> +	dev_info(&pdev->dev, "probed\n");
> +
> +	return 0;
> +
> +err_clk:
> +	clk_disable_unprepare(ipctl->clk);
> +
> +	return ret;
> +}
> +
> +static int rzn1_pinctrl_remove(struct platform_device *pdev)
> +{
> +	struct rzn1_pinctrl *ipctl = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(ipctl->clk);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id rzn1_pinctrl_match[] = {
> +	{ .compatible = "renesas,rzn1-pinctrl", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, rzn1_pinctrl_match);
> +
> +static struct platform_driver rzn1_pinctrl_driver = {
> +	.probe	= rzn1_pinctrl_probe,
> +	.remove = rzn1_pinctrl_remove,
> +	.driver	= {
> +		.name		= "rzn1-pinctrl",
> +		.owner		= THIS_MODULE,
> +		.of_match_table	= rzn1_pinctrl_match,
> +	},
> +};
> +
> +static int __init _pinctrl_drv_register(void)
> +{
> +	return platform_driver_register(&rzn1_pinctrl_driver);
> +}
> +subsys_initcall(_pinctrl_drv_register);
> +
> +MODULE_AUTHOR("Phil Edworthy <phil.edworthy@renesas.com>");
> +MODULE_DESCRIPTION("Renesas RZ/N1 pinctrl driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.17.1
>

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

RE: [PATCH v3 2/3] pinctrl: renesas: Renesas RZ/N1 pinctrl driver

[Phil Edworthy]

Hi Jacopo,

On 18 September 2018 11:44 jacopo mondi wrote:
> Hi Phil,
>    thanks for the patch
Thanks for the review!


> On Mon, Sep 17, 2018 at 05:36:08PM +0100, Phil Edworthy wrote:
> > This provides a pinctrl driver for the Renesas RZ/N1 device family.
> >
> > Based on a patch originally written by Michel Pollet at Renesas.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > ---
> > v3:
> >  - Use standard DT props instead of proprietary ones.
> >  - Replace virtual pins used for MDIO muxing with extra funcs.
> >  - Use pinctrl_utils funcs to handle the maps.
> >  - Remove the dbg functions to keep things simple.
> >
> > v2:
> >  - Change filename to generic rzn1, instead of device specific.
> >  - Changed Kconfig symbol and file name to generic rzn1 family.
> >  - Added "renesas,rzn1-pinctrl" compatible fallback string
> >  - Changes suggested by Jacopo Mondi. Mainly formatting, plus:
> >    - Removed global ptr
> >    - Removed unused code accessing parent of node.
> >    - Removed check for null OF np that can't happen.
> >    - Replaced overlapping enums with #defines
> >  - Renamed some variables and symbols to clarify their use
> >  - Fix error handling during probe
> >  - Move probe from postcore_initcall to subsys_initcall to ensure
> >    drivers that require clocks get them instead of having to defer
> >    probing.
> > ---
> >  drivers/pinctrl/Kconfig        |  10 +
> >  drivers/pinctrl/Makefile       |   1 +
> >  drivers/pinctrl/pinctrl-rzn1.c | 926
> > +++++++++++++++++++++++++++++++++
> >  3 files changed, 937 insertions(+)
> >  create mode 100644 drivers/pinctrl/pinctrl-rzn1.c
> >
> > diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index
> > e86752be1f19..e524eb101384 100644
> > --- a/drivers/pinctrl/Kconfig
> > +++ b/drivers/pinctrl/Kconfig
> > @@ -195,6 +195,16 @@ config PINCTRL_RZA1
> >  	help
> >  	  This selects pinctrl driver for Renesas RZ/A1 platforms.
> >
> > +config PINCTRL_RZN1
> > +	bool "Renesas RZ/N1 pinctrl driver"
> > +	depends on OF
> > +	depends on ARCH_RZN1 || COMPILE_TEST
> > +	select GENERIC_PINCTRL_GROUPS
> > +	select GENERIC_PINMUX_FUNCTIONS
> > +	select GENERIC_PINCONF
> > +	help
> > +	  This selects pinctrl driver for Renesas RZ/N1 devices.
> > +
> >  config PINCTRL_SINGLE
> >  	tristate "One-register-per-pin type device tree based pinctrl driver"
> >  	depends on OF
> > diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index
> > 46ef9bd52096..d07f9a20f6ae 100644
> > --- a/drivers/pinctrl/Makefile
> > +++ b/drivers/pinctrl/Makefile
> > @@ -27,6 +27,7 @@ obj-$(CONFIG_PINCTRL_PIC32)	+= pinctrl-pic32.o
> >  obj-$(CONFIG_PINCTRL_PISTACHIO)	+= pinctrl-pistachio.o
> >  obj-$(CONFIG_PINCTRL_ROCKCHIP)	+= pinctrl-rockchip.o
> >  obj-$(CONFIG_PINCTRL_RZA1)	+= pinctrl-rza1.o
> > +obj-$(CONFIG_PINCTRL_RZN1)	+= pinctrl-rzn1.o
> >  obj-$(CONFIG_PINCTRL_SINGLE)	+= pinctrl-single.o
> >  obj-$(CONFIG_PINCTRL_SIRF)	+= sirf/
> >  obj-$(CONFIG_PINCTRL_SX150X)	+= pinctrl-sx150x.o
> > diff --git a/drivers/pinctrl/pinctrl-rzn1.c
> > b/drivers/pinctrl/pinctrl-rzn1.c new file mode 100644 index
> > 000000000000..8f0caa266dbb
> > --- /dev/null
> > +++ b/drivers/pinctrl/pinctrl-rzn1.c
> > @@ -0,0 +1,926 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2014-2018 Renesas Electronics Europe Limited
> > + *
> > + * Phil Edworthy <phil.edworthy@renesas.com>
> > + * Based on a driver originally written by Michel Pollet at Renesas.
> > + */
> > +
> > +#include <dt-bindings/pinctrl/rzn1-pinctrl.h>
> > +#include <linux/clk.h>
> > +#include <linux/device.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/pinctrl/pinconf-generic.h> #include
> > +<linux/pinctrl/pinctrl.h> #include <linux/pinctrl/pinmux.h> #include
> > +<linux/platform_device.h> #include <linux/slab.h> #include "core.h"
> > +#include "pinconf.h"
> > +#include "pinctrl-utils.h"
> > +
> > +/* Field positions and masks in the pinmux registers */
> > +#define RZN1_L1_PIN_DRIVE_STRENGTH	10
> > +#define RZN1_L1_PIN_DRIVE_STRENGTH_4MA	0
> > +#define RZN1_L1_PIN_DRIVE_STRENGTH_6MA	1
> > +#define RZN1_L1_PIN_DRIVE_STRENGTH_8MA	2
> > +#define RZN1_L1_PIN_DRIVE_STRENGTH_12MA	3
> > +#define RZN1_L1_PIN_PULL		8
> > +#define RZN1_L1_PIN_PULL_NONE		0
> > +#define RZN1_L1_PIN_PULL_UP		1
> > +#define RZN1_L1_PIN_PULL_DOWN		3
> > +#define RZN1_L1_FUNCTION		0
> > +#define RZN1_L1_FUNC_MASK		0xf
> > +#define RZN1_L1_FUNCTION_L2		0xf
> > +
> > +/*
> > + * The hardware manual describes two levels of multiplexing, but it's
> > +more
> > + * logical to think of the hardware as three levels, with level 3
> > +consisting of
> > + * the multiplexing for Ethernet MDIO signals.
> > + *
> > + * Level 1 functions go from 0 to 9, with level 1 function '15' (0xf)
> > +specifying
> > + * that level 2 functions are used instead. Level 2 has a lot more
> > +options,
> > + * going from 0 to 61. Level 3 allows selection of MDIO functions
> > +which can be
> > + * floating, or one of seven internal peripherals. Unfortunately,
> > +there are two
> > + * level 2 functions that can select MDIO, and two MDIO channels so
> > +we have four
> > + * sets of level 3 functions.
> > + *
> > + * For this driver, we've compounded the numbers together, so:
> > + *    0 to   9 is level 1
> > + *   10 to  71 is 10 + level 2 number
> > + *   72 to  79 is 72 + MDIO0 source for level 2 MDIO function.
> > + *   80 to  87 is 80 + MDIO0 source for level 2 MDIO_E1 function.
> > + *   88 to  95 is 88 + MDIO1 source for level 2 MDIO function.
> > + *   96 to 103 is 96 + MDIO1 source for level 2 MDIO_E1 function.
> > + * Examples:
> > + *  Function 28 corresponds UART0
> > + *  Function 73 corresponds to MDIO0 to GMAC0
> > + *
> > + * There are 170 configurable pins (called PL_GPIO in the datasheet).
> > + */
> > +
> > +/*
> > + * Structure detailing the HW registers on the RZ/N1 devices.
> > + * Both the Level 1 mux registers and Level 2 mux registers have the
> > +same
> > + * structure. The only difference is that Level 2 has additional MDIO
> > +registers
> > + * at the end.
> > + */
> > +struct rzn1_pinctrl_regs {
> > +	union {
> > +		u32	conf[170];
> > +		u8	pad0[0x400];
> 
> Is pad0 actually used?
No, it's just to implement the padding. Would you prefer not using a union
here?

> > +	};
> > +	u32	status_protect;	/* 0x400 */
> > +	/* MDIO mux registers, level2 only */
> > +	u32	l2_mdio[2];
> > +};
> > +
> > +/**
> > + * struct rzn1_pmx_func - describes rzn1 pinmux functions
> > + * @name: the name of this specific function
> > + * @groups: corresponding pin groups
> > + * @num_groups: the number of groups
> > + */
> > +struct rzn1_pmx_func {
> > +	const char *name;
> > +	const char **groups;
> > +	unsigned int num_groups;
> > +};
> > +
> > +/**
> > + * struct rzn1_pin_group - describes an rzn1 pin group
> > + * @name: the name of this specific pin group
> > + * @func: the name of the function selected by this group
> > + * @npins: the number of pins in this group array, i.e. the number of
> > + *	elements in .pins so we can iterate over that array
> > + * @pin_ids: array of pin_ids, i.e. the value used to select the mux
> > + * @pins: array of pins. Needed due to pinctrl_ops.get_group_pins()
> > +*/ struct rzn1_pin_group {
> > +	const char *name;
> > +	const char *func;
> > +	unsigned int npins;
> > +	u8 *pin_ids;
> > +	u32 *pins;
> > +};
> > +
> > +struct rzn1_pinctrl {
> > +	struct device *dev;
> > +	struct clk *clk;
> > +	struct pinctrl_dev *pctl;
> > +	struct rzn1_pinctrl_regs __iomem *lev1;
> > +	struct rzn1_pinctrl_regs __iomem *lev2;
> > +	u32 lev1_protect_phys;
> > +	u32 lev2_protect_phys;
> > +	int mdio_func[2];
> > +
> > +	struct rzn1_pin_group *groups;
> > +	unsigned int ngroups;
> > +
> > +	struct rzn1_pmx_func *functions;
> > +	unsigned int nfunctions;
> > +};
> > +
> > +#define RZN1_PINS_PROP "pinmux"
> > +
> > +#define RZN1_PIN(pin) PINCTRL_PIN(pin, "pl_gpio"#pin)
> > +
> > +static const struct pinctrl_pin_desc rzn1_pins[] = {
> > +	RZN1_PIN(0), RZN1_PIN(1), RZN1_PIN(2), RZN1_PIN(3),
> RZN1_PIN(4),
> > +	RZN1_PIN(5), RZN1_PIN(6), RZN1_PIN(7), RZN1_PIN(8),
> RZN1_PIN(9),
> > +	RZN1_PIN(10), RZN1_PIN(11), RZN1_PIN(12), RZN1_PIN(13),
> RZN1_PIN(14),
> > +	RZN1_PIN(15), RZN1_PIN(16), RZN1_PIN(17), RZN1_PIN(18),
> RZN1_PIN(19),
> > +	RZN1_PIN(20), RZN1_PIN(21), RZN1_PIN(22), RZN1_PIN(23),
> RZN1_PIN(24),
> > +	RZN1_PIN(25), RZN1_PIN(26), RZN1_PIN(27), RZN1_PIN(28),
> RZN1_PIN(29),
> > +	RZN1_PIN(30), RZN1_PIN(31), RZN1_PIN(32), RZN1_PIN(33),
> RZN1_PIN(34),
> > +	RZN1_PIN(35), RZN1_PIN(36), RZN1_PIN(37), RZN1_PIN(38),
> RZN1_PIN(39),
> > +	RZN1_PIN(40), RZN1_PIN(41), RZN1_PIN(42), RZN1_PIN(43),
> RZN1_PIN(44),
> > +	RZN1_PIN(45), RZN1_PIN(46), RZN1_PIN(47), RZN1_PIN(48),
> RZN1_PIN(49),
> > +	RZN1_PIN(50), RZN1_PIN(51), RZN1_PIN(52), RZN1_PIN(53),
> RZN1_PIN(54),
> > +	RZN1_PIN(55), RZN1_PIN(56), RZN1_PIN(57), RZN1_PIN(58),
> RZN1_PIN(59),
> > +	RZN1_PIN(60), RZN1_PIN(61), RZN1_PIN(62), RZN1_PIN(63),
> RZN1_PIN(64),
> > +	RZN1_PIN(65), RZN1_PIN(66), RZN1_PIN(67), RZN1_PIN(68),
> RZN1_PIN(69),
> > +	RZN1_PIN(70), RZN1_PIN(71), RZN1_PIN(72), RZN1_PIN(73),
> RZN1_PIN(74),
> > +	RZN1_PIN(75), RZN1_PIN(76), RZN1_PIN(77), RZN1_PIN(78),
> RZN1_PIN(79),
> > +	RZN1_PIN(80), RZN1_PIN(81), RZN1_PIN(82), RZN1_PIN(83),
> RZN1_PIN(84),
> > +	RZN1_PIN(85), RZN1_PIN(86), RZN1_PIN(87), RZN1_PIN(88),
> RZN1_PIN(89),
> > +	RZN1_PIN(90), RZN1_PIN(91), RZN1_PIN(92), RZN1_PIN(93),
> RZN1_PIN(94),
> > +	RZN1_PIN(95), RZN1_PIN(96), RZN1_PIN(97), RZN1_PIN(98),
> RZN1_PIN(99),
> > +	RZN1_PIN(100), RZN1_PIN(101), RZN1_PIN(102), RZN1_PIN(103),
> > +	RZN1_PIN(104), RZN1_PIN(105), RZN1_PIN(106), RZN1_PIN(107),
> > +	RZN1_PIN(108), RZN1_PIN(109), RZN1_PIN(110), RZN1_PIN(111),
> > +	RZN1_PIN(112), RZN1_PIN(113), RZN1_PIN(114), RZN1_PIN(115),
> > +	RZN1_PIN(116), RZN1_PIN(117), RZN1_PIN(118), RZN1_PIN(119),
> > +	RZN1_PIN(120), RZN1_PIN(121), RZN1_PIN(122), RZN1_PIN(123),
> > +	RZN1_PIN(124), RZN1_PIN(125), RZN1_PIN(126), RZN1_PIN(127),
> > +	RZN1_PIN(128), RZN1_PIN(129), RZN1_PIN(130), RZN1_PIN(131),
> > +	RZN1_PIN(132), RZN1_PIN(133), RZN1_PIN(134), RZN1_PIN(135),
> > +	RZN1_PIN(136), RZN1_PIN(137), RZN1_PIN(138), RZN1_PIN(139),
> > +	RZN1_PIN(140), RZN1_PIN(141), RZN1_PIN(142), RZN1_PIN(143),
> > +	RZN1_PIN(144), RZN1_PIN(145), RZN1_PIN(146), RZN1_PIN(147),
> > +	RZN1_PIN(148), RZN1_PIN(149), RZN1_PIN(150), RZN1_PIN(151),
> > +	RZN1_PIN(152), RZN1_PIN(153), RZN1_PIN(154), RZN1_PIN(155),
> > +	RZN1_PIN(156), RZN1_PIN(157), RZN1_PIN(158), RZN1_PIN(159),
> > +	RZN1_PIN(160), RZN1_PIN(161), RZN1_PIN(162), RZN1_PIN(163),
> > +	RZN1_PIN(164), RZN1_PIN(165), RZN1_PIN(166), RZN1_PIN(167),
> > +	RZN1_PIN(168), RZN1_PIN(169),
> > +};
> > +
> > +enum {
> > +	LOCK_LEVEL1 = 0x1,
> > +	LOCK_LEVEL2 = 0x2,
> > +	LOCK_ALL = LOCK_LEVEL1 | LOCK_LEVEL2,
> 
> 0x03 is already (0x01 | 0x02) :)
> Not a big deal though.
Sure


> > +};
> > +
> > +static void rzn1_hw_set_lock(struct rzn1_pinctrl *ipctl, u8 lock, u8
> > +value) {
> > +	/*
> > +	 * The pinmux configuration is locked by writing the physical address
> of
> > +	 * the status_protect register to itself. It is unlocked by writing the
> > +	 * address | 1.
> > +	 */
> > +	if (lock & LOCK_LEVEL1) {
> > +		u32 val = ipctl->lev1_protect_phys | !(value & LOCK_LEVEL1);
> > +
> > +		writel(val, &ipctl->lev1->status_protect);
> > +	}
> > +
> > +	if (lock & LOCK_LEVEL2) {
> > +		u32 val = ipctl->lev2_protect_phys | !(value & LOCK_LEVEL2);
> > +
> > +		writel(val, &ipctl->lev2->status_protect);
> > +	}
> > +}
> > +
> > +static void rzn1_pinctrl_mdio_select(struct rzn1_pinctrl *ipctl, u8 mdio,
> > +				     u32 func)
> > +{
> > +	if (ipctl->mdio_func[mdio] >= 0 && ipctl->mdio_func[mdio] != func)
> > +		dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n",
> mdio);
> > +	ipctl->mdio_func[mdio] = func;
> > +
> > +	dev_dbg(ipctl->dev, "setting mdio%d to %d\n", mdio, func);
> > +
> > +	writel(func, &ipctl->lev2->l2_mdio[mdio]); }
> > +
> > +/*
> > + * Using a composite pin description, set the hardware pinmux
> > +registers
> > + * with the corresponding values.
> > + * Make sure to unlock write protection and reset it afterward.
> > + *
> > + * NOTE: There is no protection for potential concurrency, it is
> > +assumed these
> > + * calls are serialized already.
> > + */
> > +static int rzn1_set_hw_pin_func(struct rzn1_pinctrl *ipctl, u32 pin,
> > +				u32 pin_config, u8 use_locks)
> > +{
> > +	u32 l1_cache;
> > +	u32 l2_cache;
> > +	u32 l1;
> > +	u32 l2;
> > +
> > +	/* Level 3 MDIO multiplexing */
> > +	if (pin_config >= RZN1_FUNC_MDIO0_HIGHZ &&
> > +	    pin_config <= RZN1_FUNC_MDIO1_E1_SWITCH) {
> > +		u8 mdio_channel;
> > +		u32 mdio_func;
> > +
> > +		if (pin_config <= RZN1_FUNC_MDIO1_HIGHZ)
> > +			mdio_channel = 0;
> > +		else
> > +			mdio_channel = 1;
> > +
> > +		/* Get MDIO func, and convert the func to the level 2
> number */
> > +		if (pin_config <= RZN1_FUNC_MDIO0_SWITCH) {
> > +			mdio_func = pin_config -
> RZN1_FUNC_MDIO0_HIGHZ;
> > +			pin_config = RZN1_FUNC_ETH_MDIO;
> > +		} else if (pin_config <= RZN1_FUNC_MDIO0_E1_SWITCH) {
> > +			mdio_func = pin_config -
> RZN1_FUNC_MDIO0_E1_HIGHZ;
> > +			pin_config = RZN1_FUNC_ETH_MDIO_E1;
> > +		} else if (pin_config <= RZN1_FUNC_MDIO1_SWITCH) {
> > +			mdio_func = pin_config -
> RZN1_FUNC_MDIO1_HIGHZ;
> > +			pin_config = RZN1_FUNC_ETH_MDIO;
> > +		} else {
> > +			mdio_func = pin_config -
> RZN1_FUNC_MDIO1_E1_HIGHZ;
> > +			pin_config = RZN1_FUNC_ETH_MDIO_E1;
> > +		}
> > +		rzn1_pinctrl_mdio_select(ipctl, mdio_channel, mdio_func);
> > +	}
> > +
> > +	/* Note here, we do not allow anything past the MDIO Mux values */
> > +	if (pin >= ARRAY_SIZE(ipctl->lev1->conf) ||
> > +	    pin_config >= RZN1_FUNC_MDIO0_HIGHZ)
> > +		return -EINVAL;
> > +
> > +	l1 = readl(&ipctl->lev1->conf[pin]);
> > +	l1_cache = l1;
> > +	l2 = readl(&ipctl->lev2->conf[pin]);
> > +	l2_cache = l2;
> > +
> > +	dev_dbg(ipctl->dev, "setting func for pin %d to %d\n", pin,
> > +pin_config);
> > +
> > +	if (pin_config < RZN1_FUNC_L2_OFFSET) {
> > +		l1 &= ~(RZN1_L1_FUNC_MASK << RZN1_L1_FUNCTION);
> > +		l1 |= (pin_config << RZN1_L1_FUNCTION);
> > +	} else {
> > +		l1 &= ~(RZN1_L1_FUNC_MASK << RZN1_L1_FUNCTION);
> > +		l1 |= (RZN1_L1_FUNCTION_L2 << RZN1_L1_FUNCTION);
> > +
> > +		l2 = pin_config - RZN1_FUNC_L2_OFFSET;
> > +	}
> > +
> > +	/* If either configuration changes, we update both anyway */
> > +	if (l1 != l1_cache || l2 != l2_cache) {
> > +		writel(l1, &ipctl->lev1->conf[pin]);
> > +		writel(l2, &ipctl->lev2->conf[pin]);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct rzn1_pin_group *rzn1_pinctrl_find_group_by_name(
> > +	const struct rzn1_pinctrl *ipctl, const char *name) {
> > +	const struct rzn1_pin_group *grp = NULL;
> > +	int i;
> > +
> > +	for (i = 0; i < ipctl->ngroups; i++) {
> > +		if (!strcmp(ipctl->groups[i].name, name)) {
> > +			grp = &ipctl->groups[i];
> > +			break;
> > +		}
> > +	}
> > +
> > +	return grp;
> > +}
> > +
> > +static int rzn1_get_groups_count(struct pinctrl_dev *pctldev) {
> > +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +
> > +	return ipctl->ngroups;
> > +}
> > +
> > +static const char *rzn1_get_group_name(struct pinctrl_dev *pctldev,
> > +				       unsigned int selector)
> > +{
> > +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +
> > +	return ipctl->groups[selector].name; }
> > +
> > +static int rzn1_get_group_pins(struct pinctrl_dev *pctldev,
> > +			       unsigned int selector, const unsigned int **pins,
> > +			       unsigned int *npins)
> > +{
> > +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +
> > +	if (selector >= ipctl->ngroups)
> > +		return -EINVAL;
> > +
> > +	*pins = ipctl->groups[selector].pins;
> > +	*npins = ipctl->groups[selector].npins;
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * This function is called for each pinctl 'Function' node.
> > + * Sub-nodes can be used to describe multiple 'Groups' for the 'Function'
> > + * If there aren't any sub-nodes, the 'Group' is essentially the 'Function'.
> > + * Each 'Group' uses pinmux = <...> to detail the pins and data used
> > +to select
> > + * the functionality. Each 'Group' has optional pin configurations
> > +that apply
> > + * to all pins in the 'Group'.
> > + */
> > +static int rzn1_dt_node_to_map_one(struct pinctrl_dev *pctldev,
> > +				   struct device_node *np,
> > +				   struct pinctrl_map **map,
> > +				   unsigned int *num_maps)
> > +{
> > +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +	const struct rzn1_pin_group *grp;
> > +	unsigned long *configs = NULL;
> > +	unsigned int num_configs = 0;
> > +	unsigned int reserved_maps = *num_maps;
> > +	unsigned int reserve = 1;
> > +	int ret;
> > +
> > +	dev_dbg(ipctl->dev, "processing node %pOF\n", np);
> > +
> > +	grp = rzn1_pinctrl_find_group_by_name(ipctl, np->name);
> > +	if (!grp) {
> > +		dev_err(ipctl->dev, "unable to find group for node %pOF\n",
> np);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Get the group's pin configuration */
> > +	ret = pinconf_generic_parse_dt_config(np, pctldev, &configs,
> > +					      &num_configs);
> > +	if (ret < 0) {
> > +		dev_err(ipctl->dev, "%s: could not parse node property\n",
> > +			of_node_full_name(np));
> 
> Why %pOF above and of_node_full_name? I would use the first, if nothing
> changed recently with OF naming.
You are right, %pOF should be used


> > +		return ret;
> > +	}
> > +
> > +	if (num_configs)
> > +		reserve++;
> > +
> > +	/* Increase the number of maps to cover this group */
> > +	ret = pinctrl_utils_reserve_map(pctldev, map, &reserved_maps,
> num_maps,
> > +					reserve);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	/* Associate the group with the function */
> > +	ret = pinctrl_utils_add_map_mux(pctldev, map, &reserved_maps,
> num_maps,
> > +					grp->name, grp->func);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	if (num_configs) {
> > +		/* Associate the group's pin configuration with the group */
> > +		ret = pinctrl_utils_add_map_configs(pctldev, map,
> > +				&reserved_maps, num_maps, grp->name,
> > +				configs, num_configs,
> > +				PIN_MAP_TYPE_CONFIGS_GROUP);
> > +		if (ret < 0)
> > +			goto out;
> > +	}
> > +
> > +	dev_dbg(pctldev->dev, "maps: function %s group %s (%d pins)\n",
> > +		grp->func, grp->name, grp->npins);
> > +
> > +out:
> > +	kfree(configs);
> > +	return 0;
> 
> You never return error here (nit: newline before returning, here and in other
> places)
Ok, will fix.


> > +}
> > +
> > +static int rzn1_dt_node_to_map(struct pinctrl_dev *pctldev,
> > +			       struct device_node *np,
> > +			       struct pinctrl_map **map,
> > +			       unsigned int *num_maps)
> > +{
> > +	struct device_node *child;
> > +	int ret;
> > +
> > +	*map = NULL;
> > +	*num_maps = 0;
> > +
> > +	ret = rzn1_dt_node_to_map_one(pctldev, np, map, num_maps);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	for_each_child_of_node(np, child) {
> > +		ret = rzn1_dt_node_to_map_one(pctldev, child, map,
> num_maps);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct pinctrl_ops rzn1_pctrl_ops = {
> > +	.get_groups_count = rzn1_get_groups_count,
> > +	.get_group_name = rzn1_get_group_name,
> > +	.get_group_pins = rzn1_get_group_pins,
> > +	.dt_node_to_map = rzn1_dt_node_to_map,
> > +	.dt_free_map = pinctrl_utils_free_map, };
> > +
> > +static int rzn1_pmx_get_funcs_count(struct pinctrl_dev *pctldev) {
> > +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +
> > +	return ipctl->nfunctions;
> > +}
> > +
> > +static const char *rzn1_pmx_get_func_name(struct pinctrl_dev *pctldev,
> > +					  unsigned int selector)
> > +{
> > +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +
> > +	return ipctl->functions[selector].name; }
> > +
> > +static int rzn1_pmx_get_groups(struct pinctrl_dev *pctldev,
> > +			       unsigned int selector,
> > +			       const char * const **groups,
> > +			       unsigned int * const num_groups) {
> > +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +
> > +	*groups = ipctl->functions[selector].groups;
> > +	*num_groups = ipctl->functions[selector].num_groups;
> > +
> > +	return 0;
> > +}
> > +
> > +static int rzn1_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
> > +			unsigned int group)
> > +{
> > +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +	struct rzn1_pin_group *grp = &ipctl->groups[group];
> > +	unsigned int i, grp_pins = grp->npins;
> > +
> > +	dev_dbg(ipctl->dev, "set mux %s(%d) group %s(%d)\n",
> > +		ipctl->functions[selector].name, selector, grp->name,
> group);
> > +
> > +	rzn1_hw_set_lock(ipctl, LOCK_ALL, LOCK_ALL);
> > +	for (i = 0; i < grp_pins; i++)
> > +		rzn1_set_hw_pin_func(ipctl, grp->pins[i], grp->pin_ids[i], 0);
> > +	rzn1_hw_set_lock(ipctl, LOCK_ALL, 0);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct pinmux_ops rzn1_pmx_ops = {
> > +	.get_functions_count = rzn1_pmx_get_funcs_count,
> > +	.get_function_name = rzn1_pmx_get_func_name,
> > +	.get_function_groups = rzn1_pmx_get_groups,
> > +	.set_mux = rzn1_set_mux,
> > +};
> > +
> > +static int rzn1_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
> > +			    unsigned long *config)
> > +{
> > +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +	enum pin_config_param param =
> pinconf_to_config_param(*config);
> > +	const u32 reg_drive[4] = { 4, 6, 8, 12 };
> > +	u32 l1, l2;
> > +	u32 pull, drive, l1mux;
> > +	u32 arg = 0;
> > +
> > +	if (pin >= ARRAY_SIZE(ipctl->lev1->conf))
> > +		return -EINVAL;
> > +
> > +	l1 = readl(&ipctl->lev1->conf[pin]);
> > +
> > +	l1mux = l1 & RZN1_L1_FUNC_MASK;
> > +	pull = (l1 >> RZN1_L1_PIN_PULL) & 0x3;
> > +	drive = (l1 >> RZN1_L1_PIN_DRIVE_STRENGTH) & 0x3;
> > +
> > +	switch (param) {
> > +	case PIN_CONFIG_BIAS_PULL_UP:
> > +		if (pull == RZN1_L1_PIN_PULL_UP)
> > +			arg = 1;
> > +		break;
> > +	case PIN_CONFIG_BIAS_PULL_DOWN:
> > +		if (pull == RZN1_L1_PIN_PULL_DOWN)
> > +			arg = 1;
> > +		break;
> > +	case PIN_CONFIG_BIAS_DISABLE:
> > +		if (pull == RZN1_L1_PIN_PULL_NONE)
> > +			arg = 1;
> > +		break;
> > +	case PIN_CONFIG_DRIVE_STRENGTH:
> > +		arg = reg_drive[drive];
> > +		break;
> > +	case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
> > +		l2 = readl(&ipctl->lev1->conf[pin]);
> > +		if (l1mux == RZN1_FUNC_HIGHZ)
> > +			arg = 1;
> > +		if (l1mux == RZN1_L1_FUNCTION_L2 && l2 == 0)
> > +			arg = 1;
> > +		break;
> > +	default:
> > +		return -ENOTSUPP;
> > +	}
> > +
> > +	*config = pinconf_to_config_packed(param, arg);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rzn1_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
> > +			    unsigned long *configs, unsigned int num_configs)
> {
> > +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +	enum pin_config_param param;
> > +	int i;
> > +	u32 arg;
> > +	u32 l1, l1_cache;
> > +	u32 drv;
> > +
> > +	if (pin >= ARRAY_SIZE(ipctl->lev1->conf))
> > +		return -EINVAL;
> > +
> > +	l1 = readl(&ipctl->lev1->conf[pin]);
> > +	l1_cache = l1;
> > +
> > +	for (i = 0; i < num_configs; i++) {
> > +		param = pinconf_to_config_param(configs[i]);
> > +		arg = pinconf_to_config_argument(configs[i]);
> > +
> > +		switch (param) {
> > +		case PIN_CONFIG_BIAS_PULL_UP:
> > +			dev_dbg(ipctl->dev, "set pin %d pull up\n", pin);
> > +			l1 &= ~(0x3 << RZN1_L1_PIN_PULL);
> > +			l1 |= (RZN1_L1_PIN_PULL_UP <<
> RZN1_L1_PIN_PULL);
> > +			break;
> > +		case PIN_CONFIG_BIAS_PULL_DOWN:
> > +			dev_dbg(ipctl->dev, "set pin %d pull down\n", pin);
> > +			l1 &= ~(0x3 << RZN1_L1_PIN_PULL);
> > +			l1 |= (RZN1_L1_PIN_PULL_DOWN <<
> RZN1_L1_PIN_PULL);
> > +			break;
> > +		case PIN_CONFIG_BIAS_DISABLE:
> > +			dev_dbg(ipctl->dev, "set pin %d bias off\n", pin);
> > +			l1 &= ~(0x3 << RZN1_L1_PIN_PULL);
> > +			l1 |= (RZN1_L1_PIN_PULL_NONE <<
> RZN1_L1_PIN_PULL);
> > +			break;
> > +		case PIN_CONFIG_DRIVE_STRENGTH:
> > +			dev_dbg(ipctl->dev, "set pin %d drv %umA\n", pin,
> arg);
> > +			drv = 0;
> > +			switch (arg) {
> > +			case 4:
> > +				drv = RZN1_L1_PIN_DRIVE_STRENGTH_4MA;
> > +				break;
> > +			case 6:
> > +				drv = RZN1_L1_PIN_DRIVE_STRENGTH_6MA;
> > +				break;
> > +			case 8:
> > +				drv = RZN1_L1_PIN_DRIVE_STRENGTH_8MA;
> > +				break;
> > +			case 12:
> > +				drv =
> RZN1_L1_PIN_DRIVE_STRENGTH_12MA;
> > +				break;
> > +			default:
> > +				dev_err(ipctl->dev,
> > +					"Drive strength %umA not
> supported\n",
> > +					arg);
> > +				return -EINVAL;
> > +			}
> > +
> > +			l1 &= ~(0x3 << RZN1_L1_PIN_DRIVE_STRENGTH);
> > +			l1 |= (drv << RZN1_L1_PIN_DRIVE_STRENGTH);
> > +			break;
> > +
> > +		case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
> > +			dev_dbg(ipctl->dev, "set pin %d High-Z\n", pin);
> > +			l1 &= ~RZN1_L1_FUNC_MASK;
> > +			l1 |= RZN1_FUNC_HIGHZ;
> > +			break;
> > +		default:
> 
> I'll re-comment on bindings too, but the pinconf properties you support
> should be listed there
Ok, I'll fix that.


> > +			return -ENOTSUPP;
> > +		}
> > +	}
> > +
> > +	if (l1 != l1_cache) {
> > +		rzn1_hw_set_lock(ipctl, LOCK_LEVEL1, LOCK_LEVEL1);
> > +		writel(l1, &ipctl->lev1->conf[pin]);
> > +		rzn1_hw_set_lock(ipctl, LOCK_LEVEL1, 0);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int rzn1_pin_config_group_set(struct pinctrl_dev *pctldev,
> > +				     unsigned int selector,
> > +				     unsigned long *configs,
> > +				     unsigned int num_configs)
> > +{
> > +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +	struct rzn1_pin_group *grp = &ipctl->groups[selector];
> > +	int ret, i;
> > +
> > +	dev_dbg(ipctl->dev, "group set %s selector:%d configs:%p/%d\n",
> > +		grp->name, selector, configs, num_configs);
> > +
> > +	for (i = 0; i < grp->npins; i++) {
> > +		unsigned int pin = grp->pins[i];
> > +
> > +		ret = rzn1_pinconf_set(pctldev, pin, configs, num_configs);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct pinconf_ops rzn1_pinconf_ops = {
> > +	.pin_config_get = rzn1_pinconf_get,
> > +	.pin_config_set = rzn1_pinconf_set,
> > +	.pin_config_group_set = rzn1_pin_config_group_set,
> 
> Nit: you register a pinconf map with PIN_MAP_TYPE_CONFIGS_GROUP so
> the 'pinconf_[set|get]' should never be called. Have you tested if they do ?
> (just for my better understanding of the pinctrl framework :)
Ah, right!  pin_config_get is not called at all. pin_config_set is only called
from rzn1_pin_config_group_set().

So, I guess that means I should implement pin_config_group_get as well.


> > +};
> > +
> > +static struct pinctrl_desc rzn1_pinctrl_desc = {
> > +	.pctlops = &rzn1_pctrl_ops,
> > +	.pmxops = &rzn1_pmx_ops,
> > +	.confops = &rzn1_pinconf_ops,
> > +	.owner = THIS_MODULE,
> > +};
> > +
> > +static int rzn1_pinctrl_parse_groups(struct device_node *np,
> > +				     struct rzn1_pin_group *grp,
> > +				     struct rzn1_pinctrl *ipctl)
> > +{
> > +	int size;
> > +	const __be32 *list;
> > +	int i;
> 
> unsigned int i;
> 
> It is only used in the bottom for loop afaict
Yes indeed, I'll fix.


> > +
> > +	dev_dbg(ipctl->dev, "%s: %s\n", __func__, np->name);
> > +
> > +	/* Initialise group */
> > +	grp->name = np->name;
> > +
> > +	/*
> > +	 * The binding format is
> > +	 *	pinmux = <PIN_FUNC_ID CONFIG ...>,
> > +	 * do sanity check and calculate pins number
> > +	 */
> > +	list = of_get_property(np, RZN1_PINS_PROP, &size);
> > +	if (!list) {
> > +		dev_err(ipctl->dev,
> > +			"no " RZN1_PINS_PROP " property in node %s\n",
> > +			np->full_name);
> 
> Isn't it better to print out the OF node name with %pOF? Here and below...
Sure


> > +		return -EINVAL;
> > +	}
> > +
> > +	/* We do not check return since it's safe node passed down */
> > +	if (!size) {
> > +		dev_err(ipctl->dev, "Invalid " RZN1_PINS_PROP " in node
> %s\n",
> > +			np->full_name);
> > +		return -EINVAL;
> > +	}
> > +
> > +	grp->npins = size / sizeof(list[0]);
> > +	if (!grp->npins)
> > +		return 0;
> 
> If you get here the property is there and has > 0 length
True


> > +
> > +	grp->pin_ids = devm_kmalloc_array(ipctl->dev,
> > +					  grp->npins, sizeof(grp->pin_ids[0]),
> > +					  GFP_KERNEL);
> > +	grp->pins = devm_kmalloc_array(ipctl->dev,
> > +				       grp->npins, sizeof(grp->pins[0]),
> > +				       GFP_KERNEL);
> > +	if (!grp->pin_ids || !grp->pins)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < grp->npins; i++) {
> > +		u32 pin_id = be32_to_cpu(*list++);
> > +
> > +		grp->pins[i] = pin_id & 0xff;
> > +		grp->pin_ids[i] = (pin_id >> 8) & 0x7f;
> > +	}
> > +
> > +	return grp->npins;
> > +}
> > +
> > +static int rzn1_pinctrl_count_function_groups(struct device_node *np)
> > +{
> > +	struct device_node *child;
> > +	int count = 0;
> > +
> > +	if (of_property_count_u32_elems(np, RZN1_PINS_PROP) > 0)
> > +		count++;
> > +
> > +	for_each_child_of_node(np, child) {
> > +		if (of_property_count_u32_elems(child, RZN1_PINS_PROP)
> > 0)
> > +			count++;
> > +	}
> > +
> > +	return count;
> > +}
> > +
> > +static int rzn1_pinctrl_parse_functions(struct device_node *np,
> > +					struct rzn1_pinctrl *ipctl, u32 index) {
> > +	struct device_node *child;
> > +	struct rzn1_pmx_func *func;
> > +	struct rzn1_pin_group *grp;
> > +	u32 i = 0;
> > +	int ret;
> > +
> > +	dev_dbg(ipctl->dev, "parse function(%d): %s\n", index, np->name);
> > +
> > +	func = &ipctl->functions[index];
> > +
> > +	/* Initialise function */
> > +	func->name = np->name;
> > +	func->num_groups = rzn1_pinctrl_count_function_groups(np);
> > +	dev_dbg(ipctl->dev, "function %s has %d groups\n",
> > +		np->name, func->num_groups);
> 
> Maybe move this debug printout below the error check, it helps to know
> parsing went fine.
Makes sense


> > +	if (func->num_groups == 0) {
> > +		dev_err(ipctl->dev, "no groups defined in %s\n", np-
> >full_name);
> > +		return -EINVAL;
> > +	}
> > +
> > +	func->groups = devm_kmalloc_array(ipctl->dev,
> > +					  func->num_groups, sizeof(char *),
> > +					  GFP_KERNEL);
> > +	if (!func->groups)
> > +		return -ENOMEM;
> > +
> > +	if (of_property_count_u32_elems(np, RZN1_PINS_PROP) > 0) {
> > +		func->groups[i] = np->name;
> > +		grp = &ipctl->groups[ipctl->ngroups];
> > +		grp->func = func->name;
> > +		ret = rzn1_pinctrl_parse_groups(np, grp, ipctl);
> > +		if (ret < 0)
> > +			return ret;
> > +		if (ret > 0) {
> 
> You never return grp->npins == 0; it's either error or > 0. I think you can drop
> this check.
> 
> > +			i++;
> > +			ipctl->ngroups++;
> > +		}
> 
> This design allows a sub-node with the following layout
> 
>         n-1 {
>                 pinmux = <..>;
> 
>                 sub-n-1 {
>                         pinmux = <..>;
>                 };
> 
>                 sub-n-2 {
>                         pinmux = <..>;
>                 };
>         };
> 
> The bindings documentation only allows either this
> 
>         n-1 {
>                 pinmux = <..>;
>         };
> 
> Or this
>         n-1 {
>                 sub-n-1 {
>                         pinmux = <..>;
>                 };
> 
>                 sub-n-2 {
>                         pinmux = <..>;
>                 };
>         };
> 
> So I guess it's one or the other here. What do you think?
Indeed, the bindings need updating to cover the extra case. I think
this extra case is quite neat.


> > +	}
> > +
> > +	for_each_child_of_node(np, child) {
> > +		func->groups[i] = child->name;
> > +		grp = &ipctl->groups[ipctl->ngroups];
> > +		grp->func = func->name;
> > +		ret = rzn1_pinctrl_parse_groups(child, grp, ipctl);
> > +		if (ret < 0)
> > +			return ret;
> > +		if (ret > 0) {
> > +			i++;
> > +			ipctl->ngroups++;
> > +		}
> > +	}
> > +
> > +	dev_dbg(ipctl->dev, "function %s parsed %d/%d groups\n",
> > +		np->name, i, func->num_groups);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rzn1_pinctrl_probe_dt(struct platform_device *pdev,
> > +				 struct rzn1_pinctrl *ipctl)
> > +{
> > +	struct device_node *np = pdev->dev.of_node;
> > +	struct device_node *child;
> > +	unsigned int maxgroups = 0;
> > +	u32 nfuncs = 0;
> > +	u32 i = 0;
> > +	int ret;
> > +
> > +	nfuncs = of_get_child_count(np);
> > +	if (nfuncs <= 0) {
> > +		dev_err(&pdev->dev, "no functions defined\n");
> > +		return -EINVAL;
> > +	}
> 
> Do you really want to fail if no pinmuxing sub-node is defined? I can think of
> boards which includes the SoC .dtsi but do not define (yet) any pinmux/conf
> sub-nodes. This shouldn't be an error.
Makes sense.


> > +
> > +	ipctl->nfunctions = nfuncs;
> > +	ipctl->functions = devm_kmalloc_array(&pdev->dev, nfuncs,
> > +					      sizeof(*ipctl->functions),
> > +					      GFP_KERNEL);
> > +	if (!ipctl->functions)
> > +		return -ENOMEM;
> > +
> > +	ipctl->ngroups = 0;
> > +	for_each_child_of_node(np, child)
> > +		maxgroups += rzn1_pinctrl_count_function_groups(child);
> > +
> > +	ipctl->groups = devm_kmalloc_array(&pdev->dev,
> > +					   maxgroups,
> > +					   sizeof(*ipctl->groups),
> > +					   GFP_KERNEL);
> > +	if (!ipctl->groups)
> > +		return -ENOMEM;
> > +
> > +	for_each_child_of_node(np, child) {
> > +		ret = rzn1_pinctrl_parse_functions(child, ipctl, i++);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int rzn1_pinctrl_probe(struct platform_device *pdev) {
> > +	struct rzn1_pinctrl *ipctl;
> > +	struct resource *res;
> > +	int ret;
> > +
> > +	/* Create state holders etc for this driver */
> > +	ipctl = devm_kzalloc(&pdev->dev, sizeof(*ipctl), GFP_KERNEL);
> > +	if (!ipctl)
> > +		return -ENOMEM;
> > +
> > +	ipctl->mdio_func[0] = -1;
> > +	ipctl->mdio_func[1] = -1;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	ipctl->lev1_protect_phys = (u32)res->start + 0x400;
> > +	ipctl->lev1 = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(ipctl->lev1))
> > +		return PTR_ERR(ipctl->lev1);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +	ipctl->lev2_protect_phys = (u32)res->start + 0x400;
> > +	ipctl->lev2 = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(ipctl->lev2))
> > +		return PTR_ERR(ipctl->lev2);
> > +
> > +	ipctl->clk = devm_clk_get(&pdev->dev, "bus");
> > +	if (IS_ERR(ipctl->clk))
> > +		return PTR_ERR(ipctl->clk);
> > +	ret = clk_prepare_enable(ipctl->clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ipctl->dev = &pdev->dev;
> > +	rzn1_pinctrl_desc.name = dev_name(&pdev->dev);
> > +	rzn1_pinctrl_desc.pins = rzn1_pins;
> > +	rzn1_pinctrl_desc.npins = ARRAY_SIZE(rzn1_pins);
> > +
> > +	ret = rzn1_pinctrl_probe_dt(pdev, ipctl);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "fail to probe dt properties\n");
> > +		goto err_clk;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, ipctl);
> > +	ipctl->pctl = pinctrl_register(&rzn1_pinctrl_desc, &pdev->dev, ipctl);
> > +	if (!ipctl->pctl) {
> > +		dev_err(&pdev->dev, "could not register rzn1 pinctrl
> driver\n");
> > +		ret = -EINVAL;
> > +		goto err_clk;
> > +	}
> 
> Bindings claim this is a pinctrl/gpio controller, but I don't see any gpio support
> here. I'll comment on the bindings for reference too.
Indeed, I'll remove from the binding.

Thanks again for the review!
Phil


> Thanks
>    j
> 
> > +
> > +	dev_info(&pdev->dev, "probed\n");
> > +
> > +	return 0;
> > +
> > +err_clk:
> > +	clk_disable_unprepare(ipctl->clk);
> > +
> > +	return ret;
> > +}
> > +
> > +static int rzn1_pinctrl_remove(struct platform_device *pdev) {
> > +	struct rzn1_pinctrl *ipctl = platform_get_drvdata(pdev);
> > +
> > +	clk_disable_unprepare(ipctl->clk);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id rzn1_pinctrl_match[] = {
> > +	{ .compatible = "renesas,rzn1-pinctrl", },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, rzn1_pinctrl_match);
> > +
> > +static struct platform_driver rzn1_pinctrl_driver = {
> > +	.probe	= rzn1_pinctrl_probe,
> > +	.remove = rzn1_pinctrl_remove,
> > +	.driver	= {
> > +		.name		= "rzn1-pinctrl",
> > +		.owner		= THIS_MODULE,
> > +		.of_match_table	= rzn1_pinctrl_match,
> > +	},
> > +};
> > +
> > +static int __init _pinctrl_drv_register(void) {
> > +	return platform_driver_register(&rzn1_pinctrl_driver);
> > +}
> > +subsys_initcall(_pinctrl_drv_register);
> > +
> > +MODULE_AUTHOR("Phil Edworthy <phil.edworthy@renesas.com>");
> > +MODULE_DESCRIPTION("Renesas RZ/N1 pinctrl driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.17.1
> >

Re: [PATCH v3 2/3] pinctrl: renesas: Renesas RZ/N1 pinctrl driver

[jacopo mondi]

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

Hi Phil,

On Tue, Sep 18, 2018 at 11:55:16AM +0000, Phil Edworthy wrote:
> Hi Jacopo,
>

[snip]

> > > +
> > > +/*
> > > + * Structure detailing the HW registers on the RZ/N1 devices.
> > > + * Both the Level 1 mux registers and Level 2 mux registers have the
> > > +same
> > > + * structure. The only difference is that Level 2 has additional MDIO
> > > +registers
> > > + * at the end.
> > > + */
> > > +struct rzn1_pinctrl_regs {
> > > +	union {
> > > +		u32	conf[170];
> > > +		u8	pad0[0x400];
> >
> > Is pad0 actually used?
> No, it's just to implement the padding. Would you prefer not using a union
> here?

Oh, I did the math wrong, to me it was (32*170 > 8*400) but it's
actually (32*170 < 8*1024).

Also using a struct to define the memory region layout confused me and
I was about to ask "WHY ARE YOU RESERVING MEMORY HERE???" but this
type is just used for pointers, and it makes accessing HW locations
nicer actually (thanks Geert for having saved me a silly comment on this).

Cheers
   j


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

[PATCH v3 3/3] ARM: dts: r9a06g032: Add pinctrl node

[Phil Edworthy]

This provides a pinctrl driver for the Renesas R9A06G032 SoC

Based on a patch originally written by Michel Pollet at Renesas.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
v3:
 - No changes.
v2:
 - Add "renesas,rzn1-pinctrl" compatible fallback string
 - Register size corrected.
---
 arch/arm/boot/dts/r9a06g032.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/r9a06g032.dtsi b/arch/arm/boot/dts/r9a06g032.dtsi
index eaf94976ed6d..2322268bc862 100644
--- a/arch/arm/boot/dts/r9a06g032.dtsi
+++ b/arch/arm/boot/dts/r9a06g032.dtsi
@@ -165,6 +165,14 @@
 			status = "disabled";
 		};
 
+		pinctrl: pin-controller@40067000 {
+			compatible = "renesas,r9a06g032-pinctrl", "renesas,rzn1-pinctrl";
+			reg = <0x40067000 0x1000>, <0x51000000 0x480>;
+			clocks = <&sysctrl R9A06G032_HCLK_PINCONFIG>;
+			clock-names = "bus";
+			status = "okay";
+		};
+
 		gic: gic@44101000 {
 			compatible = "arm,cortex-a7-gic", "arm,gic-400";
 			interrupt-controller;
-- 
2.17.1

Re: [PATCH v3 3/3] ARM: dts: r9a06g032: Add pinctrl node

[Simon Horman]

On Mon, Sep 17, 2018 at 05:36:09PM +0100, Phil Edworthy wrote:
> This provides a pinctrl driver for the Renesas R9A06G032 SoC
> 
> Based on a patch originally written by Michel Pollet at Renesas.
> 
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>

Thanks Phil, I will defer this patch pending acceptance of the bindings.
Please continue to repost this patch with updates to the bindings
or otherwise ping me once they have been accepted.

> ---
> v3:
>  - No changes.
> v2:
>  - Add "renesas,rzn1-pinctrl" compatible fallback string
>  - Register size corrected.
> ---
>  arch/arm/boot/dts/r9a06g032.dtsi | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/r9a06g032.dtsi b/arch/arm/boot/dts/r9a06g032.dtsi
> index eaf94976ed6d..2322268bc862 100644
> --- a/arch/arm/boot/dts/r9a06g032.dtsi
> +++ b/arch/arm/boot/dts/r9a06g032.dtsi
> @@ -165,6 +165,14 @@
>  			status = "disabled";
>  		};
>  
> +		pinctrl: pin-controller@40067000 {
> +			compatible = "renesas,r9a06g032-pinctrl", "renesas,rzn1-pinctrl";
> +			reg = <0x40067000 0x1000>, <0x51000000 0x480>;
> +			clocks = <&sysctrl R9A06G032_HCLK_PINCONFIG>;
> +			clock-names = "bus";
> +			status = "okay";
> +		};
> +
>  		gic: gic@44101000 {
>  			compatible = "arm,cortex-a7-gic", "arm,gic-400";
>  			interrupt-controller;
> -- 
> 2.17.1
> 

RE: [PATCH v3 3/3] ARM: dts: r9a06g032: Add pinctrl node

[Phil Edworthy]

Hi Simon,

On 19 September 2018 10:15 Simon Horman wrote:
> On Mon, Sep 17, 2018 at 05:36:09PM +0100, Phil Edworthy wrote:
> > This provides a pinctrl driver for the Renesas R9A06G032 SoC
> >
> > Based on a patch originally written by Michel Pollet at Renesas.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> 
> Thanks Phil, I will defer this patch pending acceptance of the bindings.
> Please continue to repost this patch with updates to the bindings or
> otherwise ping me once they have been accepted.
Thanks, however I will send a new version of this patch to remove the
clock-names prop as it is not needed.

Thanks
Phil


> > ---
> > v3:
> >  - No changes.
> > v2:
> >  - Add "renesas,rzn1-pinctrl" compatible fallback string
> >  - Register size corrected.
> > ---
> >  arch/arm/boot/dts/r9a06g032.dtsi | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/r9a06g032.dtsi
> > b/arch/arm/boot/dts/r9a06g032.dtsi
> > index eaf94976ed6d..2322268bc862 100644
> > --- a/arch/arm/boot/dts/r9a06g032.dtsi
> > +++ b/arch/arm/boot/dts/r9a06g032.dtsi
> > @@ -165,6 +165,14 @@
> >  			status = "disabled";
> >  		};
> >
> > +		pinctrl: pin-controller@40067000 {
> > +			compatible = "renesas,r9a06g032-pinctrl",
> "renesas,rzn1-pinctrl";
> > +			reg = <0x40067000 0x1000>, <0x51000000 0x480>;
> > +			clocks = <&sysctrl R9A06G032_HCLK_PINCONFIG>;
> > +			clock-names = "bus";
> > +			status = "okay";
> > +		};
> > +
> >  		gic: gic@44101000 {
> >  			compatible = "arm,cortex-a7-gic", "arm,gic-400";
> >  			interrupt-controller;
> > --
> > 2.17.1
> >

Re: [PATCH v3 3/3] ARM: dts: r9a06g032: Add pinctrl node

[Geert Uytterhoeven]

Hi Phil,

On Wed, Sep 19, 2018 at 11:18 AM Phil Edworthy
<phil.edworthy@renesas.com> wrote:
> On 19 September 2018 10:15 Simon Horman wrote:
> > On Mon, Sep 17, 2018 at 05:36:09PM +0100, Phil Edworthy wrote:
> > > This provides a pinctrl driver for the Renesas R9A06G032 SoC
> > >
> > > Based on a patch originally written by Michel Pollet at Renesas.
> > >
> > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> >
> > Thanks Phil, I will defer this patch pending acceptance of the bindings.
> > Please continue to repost this patch with updates to the bindings or
> > otherwise ping me once they have been accepted.
> Thanks, however I will send a new version of this patch to remove the
> clock-names prop as it is not needed.

I haven't reviewed the bindings patch yet, but it's a good idea to have
clock-names, even for a single clock.
It makes life easier if the same module is reused or enhanced in a later SoC,
with more clock inputs.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v3 3/3] ARM: dts: r9a06g032: Add pinctrl node

[Phil Edworthy]

Hi Geert,

On 19 September 2018 10:24 Geert Uytterhoeven wrote:
> On Wed, Sep 19, 2018 at 11:18 AM Phil Edworthy  wrote:
> > On 19 September 2018 10:15 Simon Horman wrote:
> > > On Mon, Sep 17, 2018 at 05:36:09PM +0100, Phil Edworthy wrote:
> > > > This provides a pinctrl driver for the Renesas R9A06G032 SoC
> > > >
> > > > Based on a patch originally written by Michel Pollet at Renesas.
> > > >
> > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > >
> > > Thanks Phil, I will defer this patch pending acceptance of the bindings.
> > > Please continue to repost this patch with updates to the bindings or
> > > otherwise ping me once they have been accepted.
> > Thanks, however I will send a new version of this patch to remove the
> > clock-names prop as it is not needed.
> 
> I haven't reviewed the bindings patch yet, but it's a good idea to have
> clock-names, even for a single clock.
> It makes life easier if the same module is reused or enhanced in a later SoC,
> with more clock inputs.
Ok, I'll put it back :)

I should be able to get a new version out today so you may want to delay
your review until then.

Thanks
Phil

Re: [PATCH v3 3/3] ARM: dts: r9a06g032: Add pinctrl node

[Simon Horman]

On Wed, Sep 19, 2018 at 09:18:41AM +0000, Phil Edworthy wrote:
> Hi Simon,
> 
> On 19 September 2018 10:15 Simon Horman wrote:
> > On Mon, Sep 17, 2018 at 05:36:09PM +0100, Phil Edworthy wrote:
> > > This provides a pinctrl driver for the Renesas R9A06G032 SoC
> > >
> > > Based on a patch originally written by Michel Pollet at Renesas.
> > >
> > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > 
> > Thanks Phil, I will defer this patch pending acceptance of the bindings.
> > Please continue to repost this patch with updates to the bindings or
> > otherwise ping me once they have been accepted.
> Thanks, however I will send a new version of this patch to remove the
> clock-names prop as it is not needed.

Thanks, sounds good.