[PATCH v1 0/5] Renesas R9A06G032 PINCTRL Driver

[PATCH v1 0/5] Renesas R9A06G032 PINCTRL Driver

[Michel Pollet]

*WARNING* -- this requires:
+ R9A06G032 base patch v9
+ R9A06G032 SMP patch v5

This implements the pinctrl driver for the R9A06G032. Apart from
the file names, I had to keep using RZN1_ as the headers etc are
already extensively in use -- u-boot, vmworks, cm3 code and threadx
use these constants and the base support to implement pinmux on this
SoC.

Also, there is an existing pretty extensive webapp that allows
configuring the pinmux externally that generates either source
code (for non DT based OSes) or an included .dtsi file for board
specific configs.

Note, I used renesas,rzn1-pinmux node to specify the pinmux constants,
and I also don't use some of the properties documented in
pinctrl-bindings.txt on purpose, as they are too limited for my use
(I need to be able to set, clear, ignore or reset level, pull up/down
and function as the pinmux might be set by another OS/core running
concurently).

v1
 + Just supports fhe UART0 on the DB board.

Michel Pollet (5):
  dt-bindings: Add the r9a06g032-pinctrl.h file
  dt-bindings: clock: renesas,r9a06g032-pinctrl: documentation
  pinctrl: renesas: Renesas R9A06G032 pinctrl driver
  ARM: dts: Renesas R9A06G032 pinctrl node
  ARM: dts: Renesas RZN1D-DB Board: Add UART0 pinmux node

 .../bindings/pinctrl/renesas,r9a06g032-pinctrl.txt | 124 +++
 arch/arm/boot/dts/r9a06g032-rzn1d400-db.dts        |  13 +
 arch/arm/boot/dts/r9a06g032.dtsi                   |   8 +
 drivers/pinctrl/Kconfig                            |  10 +
 drivers/pinctrl/Makefile                           |   1 +
 drivers/pinctrl/pinctrl-r9a06g032.c                | 890 +++++++++++++++++++++
 include/dt-bindings/pinctrl/r9a06g032-pinctrl.h    | 191 +++++
 7 files changed, 1237 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,r9a06g032-pinctrl.txt
 create mode 100644 drivers/pinctrl/pinctrl-r9a06g032.c
 create mode 100644 include/dt-bindings/pinctrl/r9a06g032-pinctrl.h

-- 
2.7.4

[PATCH v1 1/5] dt-bindings: Add the r9a06g032-pinctrl.h file

[Michel Pollet]

This adds the constants necessary to use the renesas,r9a06g032-pinctrl
node.

Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
---
 include/dt-bindings/pinctrl/r9a06g032-pinctrl.h | 191 ++++++++++++++++++++++++
 1 file changed, 191 insertions(+)
 create mode 100644 include/dt-bindings/pinctrl/r9a06g032-pinctrl.h

diff --git a/include/dt-bindings/pinctrl/r9a06g032-pinctrl.h b/include/dt-bindings/pinctrl/r9a06g032-pinctrl.h
new file mode 100644
index 0000000..936ff48
--- /dev/null
+++ b/include/dt-bindings/pinctrl/r9a06g032-pinctrl.h
@@ -0,0 +1,191 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Defines macros and constants for Renesas R9A06G032 pin controller pin
+ * muxing functions.
+ */
+#ifndef __DT_BINDINGS_PINCTRL_RENESAS_R9A06G032_H
+#define __DT_BINDINGS_PINCTRL_RENESAS_R9A06G032_H
+
+#define RZN1_MUX_FUNC_BIT			8
+#define RZN1_MUX_HAS_FUNC_BIT			15
+#define RZN1_MUX_HAS_DRIVE_BIT			16
+#define RZN1_MUX_DRIVE_BIT			17
+#define RZN1_MUX_HAS_PULL_BIT			19
+#define RZN1_MUX_PULL_BIT			20
+
+#define RZN1_MUX_PULL_UP			1
+#define RZN1_MUX_PULL_DOWN			3
+#define RZN1_MUX_PULL_NONE			0
+
+#define RZN1_MUX_DRIVE_4MA			0
+#define RZN1_MUX_DRIVE_6MA			1
+#define RZN1_MUX_DRIVE_8MA			2
+#define RZN1_MUX_DRIVE_12MA			3
+
+#define RZN1_MUX(_gpio, _func) \
+	(((RZN1_FUNC_##_func) << RZN1_MUX_FUNC_BIT) | \
+		(1 << RZN1_MUX_HAS_FUNC_BIT) | \
+		(_gpio))
+
+#define RZN1_MUX_PULL(_pull) \
+		((1 << RZN1_MUX_HAS_PULL_BIT) | \
+		((_pull) << RZN1_MUX_PULL_BIT))
+
+#define RZN1_MUX_DRIVE(_drive) \
+		((1 << RZN1_MUX_HAS_DRIVE_BIT) | \
+		((_drive) << RZN1_MUX_DRIVE_BIT))
+
+#define RZN1_MUX_PUP(_gpio, _func) \
+	(RZN1_MUX(_gpio, _func) | RZN1_MUX_PULL(RZN1_MUX_PULL_UP))
+#define RZN1_MUX_PDOWN(_gpio, _func) \
+	(RZN1_MUX(_gpio, _func) | RZN1_MUX_PULL(RZN1_MUX_PULL_DOWN))
+#define RZN1_MUX_PNONE(_gpio, _func) \
+	(RZN1_MUX(_gpio, _func) | RZN1_MUX_PULL(RZN1_MUX_PULL_NONE))
+
+#define RZN1_MUX_4MA(_gpio, _func) \
+	(RZN1_MUX(_gpio, _func) | RZN1_MUX_DRIVE(RZN1_MUX_DRIVE_4MA))
+#define RZN1_MUX_6MA(_gpio, _func) \
+	(RZN1_MUX(_gpio, _func) | RZN1_MUX_DRIVE(RZN1_MUX_DRIVE_6MA))
+#define RZN1_MUX_8MA(_gpio, _func) \
+	(RZN1_MUX(_gpio, _func) | RZN1_MUX_DRIVE(RZN1_MUX_DRIVE_8MA))
+#define RZN1_MUX_12MA(_gpio, _func) \
+	(RZN1_MUX(_gpio, _func) | RZN1_MUX_DRIVE(RZN1_MUX_DRIVE_12MA))
+
+#define RZN1_MUX_PUP_4MA(_gpio, _func) \
+	(RZN1_MUX_PUP(_gpio, _func) | RZN1_MUX_DRIVE(RZN1_MUX_DRIVE_4MA))
+#define RZN1_MUX_PUP_6MA(_gpio, _func) \
+	(RZN1_MUX_PUP(_gpio, _func) | RZN1_MUX_DRIVE(RZN1_MUX_DRIVE_6MA))
+#define RZN1_MUX_PUP_8MA(_gpio, _func) \
+	(RZN1_MUX_PUP(_gpio, _func) | RZN1_MUX_DRIVE(RZN1_MUX_DRIVE_8MA))
+#define RZN1_MUX_PUP_12MA(_gpio, _func) \
+	(RZN1_MUX_PUP(_gpio, _func) | RZN1_MUX_DRIVE(RZN1_MUX_DRIVE_12MA))
+
+#define RZN1_MUX_PDOWN_4MA(_gpio, _func) \
+	(RZN1_MUX_PDOWN(_gpio, _func) | RZN1_MUX_DRIVE(RZN1_MUX_DRIVE_4MA))
+#define RZN1_MUX_PDOWN_6MA(_gpio, _func) \
+	(RZN1_MUX_PDOWN(_gpio, _func) | RZN1_MUX_DRIVE(RZN1_MUX_DRIVE_6MA))
+#define RZN1_MUX_PDOWN_8MA(_gpio, _func) \
+	(RZN1_MUX_PDOWN(_gpio, _func) | RZN1_MUX_DRIVE(RZN1_MUX_DRIVE_8MA))
+#define RZN1_MUX_PDOWN_12MA(_gpio, _func) \
+	(RZN1_MUX_PDOWN(_gpio, _func) | RZN1_MUX_DRIVE(RZN1_MUX_DRIVE_12MA))
+
+#define RZN1_MUX_PNONE_4MA(_gpio, _func) \
+	(RZN1_MUX_PNONE(_gpio, _func) | RZN1_MUX_DRIVE(RZN1_MUX_DRIVE_4MA))
+#define RZN1_MUX_PNONE_6MA(_gpio, _func) \
+	(RZN1_MUX_PNONE(_gpio, _func) | RZN1_MUX_DRIVE(RZN1_MUX_DRIVE_6MA))
+#define RZN1_MUX_PNONE_8MA(_gpio, _func) \
+	(RZN1_MUX_PNONE(_gpio, _func) | RZN1_MUX_DRIVE(RZN1_MUX_DRIVE_8MA))
+#define RZN1_MUX_PNONE_12MA(_gpio, _func) \
+	(RZN1_MUX_PNONE(_gpio, _func) | RZN1_MUX_DRIVE(RZN1_MUX_DRIVE_12MA))
+
+/*
+ * Use these "gpio" numbers with the RZN1_FUNC_MDIO_MUX* functions
+ * to set the destination of the two MDIO busses.
+ */
+#define RZN1_MDIO_BUS0				170
+#define RZN1_MDIO_BUS1				171
+
+/*
+ * This can be used to set pullups/down/driver for pins without changing
+ * any function that might have already been set
+ */
+#define RZN1_FUNC_NONE				0xff
+
+/*
+ * 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_LEVEL2_OFFSET, and the Level 1 mux will be
+ *         set accordingly.
+ * 72...79 are for the 2 MUDIO muxes for "GPIO" 170/171. These muxes will
+ *         be set to this value - 72.
+ */
+#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_LEVEL2_OFFSET			10	/* I'm Special */
+#define RZN1_FUNC_HIGHZ1			10
+#define RZN1_FUNC_ETHERCAT			11
+#define RZN1_FUNC_SERCOS3			12
+#define RZN1_FUNC_SDIO_E			13
+#define RZN1_FUNC_ETH_MDIO			14
+#define RZN1_FUNC_ETH_MDIO_E1			15
+#define RZN1_FUNC_USB				16
+#define RZN1_FUNC_MSEBIM_E			17
+#define RZN1_FUNC_MSEBIS_E			18
+#define RZN1_FUNC_RSV				19
+#define RZN1_FUNC_RSV_E				20
+#define RZN1_FUNC_RSV_E1			21
+#define RZN1_FUNC_UART0_I			22
+#define RZN1_FUNC_UART0_I_E			23
+#define RZN1_FUNC_UART1_I			24
+#define RZN1_FUNC_UART1_I_E			25
+#define RZN1_FUNC_UART2_I			26
+#define RZN1_FUNC_UART2_I_E			27
+#define RZN1_FUNC_UART0				28
+#define RZN1_FUNC_UART0_E			29
+#define RZN1_FUNC_UART1				30
+#define RZN1_FUNC_UART1_E			31
+#define RZN1_FUNC_UART2				32
+#define RZN1_FUNC_UART2_E			33
+#define RZN1_FUNC_UART3				34
+#define RZN1_FUNC_UART3_E			35
+#define RZN1_FUNC_UART4				36
+#define RZN1_FUNC_UART4_E			37
+#define RZN1_FUNC_UART5				38
+#define RZN1_FUNC_UART5_E			39
+#define RZN1_FUNC_UART6				40
+#define RZN1_FUNC_UART6_E			41
+#define RZN1_FUNC_UART7				42
+#define RZN1_FUNC_UART7_E			43
+#define RZN1_FUNC_SPI0_M			44
+#define RZN1_FUNC_SPI0_M_E			45
+#define RZN1_FUNC_SPI1_M			46
+#define RZN1_FUNC_SPI1_M_E			47
+#define RZN1_FUNC_SPI2_M			48
+#define RZN1_FUNC_SPI2_M_E			49
+#define RZN1_FUNC_SPI3_M			50
+#define RZN1_FUNC_SPI3_M_E			51
+#define RZN1_FUNC_SPI4_S			52
+#define RZN1_FUNC_SPI4_S_E			53
+#define RZN1_FUNC_SPI5_S			54
+#define RZN1_FUNC_SPI5_S_E			55
+#define RZN1_FUNC_SGPIO0_M			56
+#define RZN1_FUNC_SGPIO1_M			57
+#define RZN1_FUNC_GPIO				58
+#define RZN1_FUNC_CAN				59
+#define RZN1_FUNC_I2C				60
+#define RZN1_FUNC_SAFE				61
+#define RZN1_FUNC_PTO_PWM			62
+#define RZN1_FUNC_PTO_PWM1			63
+#define RZN1_FUNC_PTO_PWM2			64
+#define RZN1_FUNC_PTO_PWM3			65
+#define RZN1_FUNC_PTO_PWM4			66
+#define RZN1_FUNC_DELTA_SIGMA			67
+#define RZN1_FUNC_SGPIO2_M			68
+#define RZN1_FUNC_SGPIO3_M			69
+#define RZN1_FUNC_SGPIO4_S			70
+#define RZN1_FUNC_MAC_MTIP_SWITCH		71
+/* These correspond to the functions used for the two MDIO muxes. */
+#define RZN1_FUNC_MDIO_MUX_HIGHZ		72
+#define RZN1_FUNC_MDIO_MUX_MAC0			73
+#define RZN1_FUNC_MDIO_MUX_MAC1			74
+#define RZN1_FUNC_MDIO_MUX_ECAT			75
+#define RZN1_FUNC_MDIO_MUX_S3_MDIO0		76
+#define RZN1_FUNC_MDIO_MUX_S3_MDIO1		77
+#define RZN1_FUNC_MDIO_MUX_HWRTOS		78
+#define RZN1_FUNC_MDIO_MUX_SWITCH		79
+#define RZN1_FUNC_MAX				80
+
+#endif /* __DT_BINDINGS_PINCTRL_RENESAS_R9A06G032_H */
-- 
2.7.4

Re: [PATCH v1 1/5] dt-bindings: Add the r9a06g032-pinctrl.h file

[Rob Herring]

On Thu, Jun 14, 2018 at 12:00:17PM +0100, Michel Pollet wrote:
> This adds the constants necessary to use the renesas,r9a06g032-pinctrl
> node.
> 
> Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
> ---
>  include/dt-bindings/pinctrl/r9a06g032-pinctrl.h | 191 ++++++++++++++++++++++++
>  1 file changed, 191 insertions(+)
>  create mode 100644 include/dt-bindings/pinctrl/r9a06g032-pinctrl.h

This can be combined with patch 2.

[PATCH v1 2/5] dt-bindings: clock: renesas,r9a06g032-pinctrl: documentation

[Michel Pollet]

The Renesas R9A06G032 PINCTRL node description.

Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
---
 .../bindings/pinctrl/renesas,r9a06g032-pinctrl.txt | 124 +++++++++++++++++++++
 1 file changed, 124 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,r9a06g032-pinctrl.txt

diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,r9a06g032-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,r9a06g032-pinctrl.txt
new file mode 100644
index 0000000..f63696f
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/renesas,r9a06g032-pinctrl.txt
@@ -0,0 +1,124 @@
+Renesas RZ/A1 combined Pin and GPIO controller
+
+The Renesas SoCs of the RZ/A1 family feature a combined Pin and GPIO controller,
+named "Ports" in the hardware reference manual.
+Pin multiplexing and GPIO configuration is performed on a per-pin basis
+writing configuration values to per-port register sets.
+Each "port" features up to 16 pins, each of them configurable for GPIO
+function (port mode) or in alternate function mode.
+Up to 8 different alternate function modes exist for each single pin.
+
+Pin controller node
+-------------------
+
+Required properties:
+  - compatible: should be:
+    - "renesas,r9a05g032-pinctrl"
+  - reg
+    address base and length of the memory area where the pin controller
+    hardware is mapped to.
+
+Example:
+	pinctrl: pinctrl@40067000 {
+		compatible = "renesas,r9a06g032-pinctrl";
+		reg = <0x40067000 0x1000>, <0x51000000 0x800>;
+		clocks = <&sysctrl R9A06G032_HCLK_PINCONFIG>;
+		clock-names = "bus";
+		status = "okay";
+	};
+
+
+Sub-nodes
+---------
+  The child nodes of the pin controller node describe a pin multiplexing
+  group that can be used by driver 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.
+
+  The allowed generic formats for a pin multiplexing sub-node are the
+  following ones:
+
+  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:
+    - renesas,rzn1-pinctrl:
+      Array of integers representing each 'pin' and it's configuration.
+
+      A 'pin number' does not correspond 1:1 to the hardware manual notion of
+      PL_GPIO directly. Numbers 0...169 are PL_GPIOs, however there is also two
+      extra 170 and 171 that corresponds to the MDIO0 and MDIO1 bus config.
+
+      A 'function' also does not correspond 1:1 to the hardware manual. There
+      are 2 levels of pin muxing, Level 1, level 2 -- to this are added the
+      MDIO configurations.
+
+      Helper macros to ease assembling the pin index and function identifier
+      are provided by the pin controller header file at:
+      <include/dt-bindings/pinctrl/r9a06g032-pinctrl.h>
+
+Example #1:
+  A simple case configuring only the function for a given 'pin' works as follow:
+	#include <include/dt-bindings/pinctrl/r9a06g032-pinctrl.h>
+	&pinctrl {
+		pinsuart0: pinsuart0 {
+			renesas,rzn1-pinmux-ids = <
+				RZN1_MUX(103, UART0_I)	/* UART0_TXD */
+				RZN1_MUX(104, UART0_I)	/* UART0_RXD */
+			>;
+		};
+	};
+
+	&uart0 {
+		status = "okay";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinsuart0>;
+	};
+  Note that in this case the other functions of the pins are not changed.
+
+Example #2:
+  Here we also set the pullups on the RXD pin:
+	&pinctrl {
+		pinsuart0: pinsuart0 {
+			renesas,rzn1-pinmux-ids = <
+				RZN1_MUX(103, UART0_I)	/* UART0_TXD */
+				RZN1_MUX_PUP(104, UART0_I)	/* UART0_RXD */
+			>;
+		};
+	};
+  There are many alternative macros to set the pullup/down/none and the drive
+  strenght in the r9a06g032-pinctrl.h header file.
+
+Example #3:
+  The Soc has two configurable MDIO muxes, these can also be configured
+  with this interface using the 'special' MDIO constants:
+
+	&pinctrl {
+		mdio_mux: mdiomux {
+			renesas,rzn1-pinmux-ids = <
+				RZN1_MUX(RZN1_MDIO_BUS0, RZN1_FUNC_MDIO_MUX_MAC0)
+				RZN1_MUX(RZN1_MDIO_BUS1, RZN1_FUNC_MDIO_MUX_SWITCH)
+			>;
+		};
+	};
+  Clearly the pull/up/none and drive constants will be ignored, even if
+  specified.
+
+Note that Renesas provides an extensive webapp that can generates a device tree
+file for your board. See their website for this part number for details.
-- 
2.7.4

Re: [PATCH v1 2/5] dt-bindings: clock: renesas,r9a06g032-pinctrl: documentation

[jacopo mondi]

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

Hi Michel,

On Thu, Jun 14, 2018 at 12:00:18PM +0100, Michel Pollet wrote:
> The Renesas R9A06G032 PINCTRL node description.
>
> Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
> ---
>  .../bindings/pinctrl/renesas,r9a06g032-pinctrl.txt | 124 +++++++++++++++++++++
>  1 file changed, 124 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,r9a06g032-pinctrl.txt
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,r9a06g032-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,r9a06g032-pinctrl.txt
> new file mode 100644
> index 0000000..f63696f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/renesas,r9a06g032-pinctrl.txt
> @@ -0,0 +1,124 @@
> +Renesas RZ/A1 combined Pin and GPIO controller
> +
> +The Renesas SoCs of the RZ/A1 family feature a combined Pin and GPIO controller,
> +named "Ports" in the hardware reference manual.
> +Pin multiplexing and GPIO configuration is performed on a per-pin basis
> +writing configuration values to per-port register sets.
> +Each "port" features up to 16 pins, each of them configurable for GPIO
> +function (port mode) or in alternate function mode.
> +Up to 8 different alternate function modes exist for each single pin.

This is a plain copy of the RZ/A1 pin controller documentation.
I don't know RZ/N devices and cannot tell if it applies here too, but
this needs an s/A1/N at the least.

From a very superficial look at the proposed bindings and pinctrl
driver, I wonder if the rza1 pinctrl driver could be expanded and re-used.
Have you considered that before starting one from scratch?

Thanks
   j

> +
> +Pin controller node
> +-------------------
> +
> +Required properties:
> +  - compatible: should be:
> +    - "renesas,r9a05g032-pinctrl"
> +  - reg
> +    address base and length of the memory area where the pin controller
> +    hardware is mapped to.
> +
> +Example:
> +	pinctrl: pinctrl@40067000 {
> +		compatible = "renesas,r9a06g032-pinctrl";
> +		reg = <0x40067000 0x1000>, <0x51000000 0x800>;
> +		clocks = <&sysctrl R9A06G032_HCLK_PINCONFIG>;
> +		clock-names = "bus";
> +		status = "okay";
> +	};
> +
> +
> +Sub-nodes
> +---------
> +  The child nodes of the pin controller node describe a pin multiplexing
> +  group that can be used by driver 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.
> +
> +  The allowed generic formats for a pin multiplexing sub-node are the
> +  following ones:
> +
> +  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:
> +    - renesas,rzn1-pinctrl:
> +      Array of integers representing each 'pin' and it's configuration.
> +
> +      A 'pin number' does not correspond 1:1 to the hardware manual notion of
> +      PL_GPIO directly. Numbers 0...169 are PL_GPIOs, however there is also two
> +      extra 170 and 171 that corresponds to the MDIO0 and MDIO1 bus config.
> +
> +      A 'function' also does not correspond 1:1 to the hardware manual. There
> +      are 2 levels of pin muxing, Level 1, level 2 -- to this are added the
> +      MDIO configurations.
> +
> +      Helper macros to ease assembling the pin index and function identifier
> +      are provided by the pin controller header file at:
> +      <include/dt-bindings/pinctrl/r9a06g032-pinctrl.h>
> +
> +Example #1:
> +  A simple case configuring only the function for a given 'pin' works as follow:
> +	#include <include/dt-bindings/pinctrl/r9a06g032-pinctrl.h>
> +	&pinctrl {
> +		pinsuart0: pinsuart0 {
> +			renesas,rzn1-pinmux-ids = <
> +				RZN1_MUX(103, UART0_I)	/* UART0_TXD */
> +				RZN1_MUX(104, UART0_I)	/* UART0_RXD */
> +			>;
> +		};
> +	};
> +
> +	&uart0 {
> +		status = "okay";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinsuart0>;
> +	};
> +  Note that in this case the other functions of the pins are not changed.
> +
> +Example #2:
> +  Here we also set the pullups on the RXD pin:
> +	&pinctrl {
> +		pinsuart0: pinsuart0 {
> +			renesas,rzn1-pinmux-ids = <
> +				RZN1_MUX(103, UART0_I)	/* UART0_TXD */
> +				RZN1_MUX_PUP(104, UART0_I)	/* UART0_RXD */
> +			>;
> +		};
> +	};
> +  There are many alternative macros to set the pullup/down/none and the drive
> +  strenght in the r9a06g032-pinctrl.h header file.
> +
> +Example #3:
> +  The Soc has two configurable MDIO muxes, these can also be configured
> +  with this interface using the 'special' MDIO constants:
> +
> +	&pinctrl {
> +		mdio_mux: mdiomux {
> +			renesas,rzn1-pinmux-ids = <
> +				RZN1_MUX(RZN1_MDIO_BUS0, RZN1_FUNC_MDIO_MUX_MAC0)
> +				RZN1_MUX(RZN1_MDIO_BUS1, RZN1_FUNC_MDIO_MUX_SWITCH)
> +			>;
> +		};
> +	};
> +  Clearly the pull/up/none and drive constants will be ignored, even if
> +  specified.
> +
> +Note that Renesas provides an extensive webapp that can generates a device tree
> +file for your board. See their website for this part number for details.
> --
> 2.7.4
>

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

RE: [PATCH v1 2/5] dt-bindings: clock: renesas,r9a06g032-pinctrl: documentation

[Michel Pollet]

Hi Jacopo,

On 14 June 2018 13:05, Jacopo wrote:
> Hi Michel,
>
> On Thu, Jun 14, 2018 at 12:00:18PM +0100, Michel Pollet wrote:
> > The Renesas R9A06G032 PINCTRL node description.
> >
> > Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
> > ---
> >  .../bindings/pinctrl/renesas,r9a06g032-pinctrl.txt | 124
> > +++++++++++++++++++++
> >  1 file changed, 124 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/pinctrl/renesas,r9a06g032-pinctrl.tx
> > t
> >
> > diff --git
> > a/Documentation/devicetree/bindings/pinctrl/renesas,r9a06g032-pinctrl.
> > txt
> > b/Documentation/devicetree/bindings/pinctrl/renesas,r9a06g032-pinctrl.
> > txt
> > new file mode 100644
> > index 0000000..f63696f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pinctrl/renesas,r9a06g032-pinc
> > +++ trl.txt
> > @@ -0,0 +1,124 @@
> > +Renesas RZ/A1 combined Pin and GPIO controller
> > +
> > +The Renesas SoCs of the RZ/A1 family feature a combined Pin and GPIO
> > +controller, named "Ports" in the hardware reference manual.
> > +Pin multiplexing and GPIO configuration is performed on a per-pin
> > +basis writing configuration values to per-port register sets.
> > +Each "port" features up to 16 pins, each of them configurable for
> > +GPIO function (port mode) or in alternate function mode.
> > +Up to 8 different alternate function modes exist for each single pin.
>
> This is a plain copy of the RZ/A1 pin controller documentation.
> I don't know RZ/N devices and cannot tell if it applies here too, but this needs
> an s/A1/N at the least.
>
> From a very superficial look at the proposed bindings and pinctrl driver, I
> wonder if the rza1 pinctrl driver could be expanded and re-used.
> Have you considered that before starting one from scratch?

Sorry it's just the start of the documentation which I wrongly cut/pasted -- the
rest is completely different; the only reason A1 is mentioned here is that I used
that file as a template for the header/sections etc... Somehow the title+blurb
went unedited!

And no, in terms of functionalities, they are completely different IPs.

Michel

>
> Thanks
>    j
>
> > +
> > +Pin controller node
> > +-------------------
> > +
> > +Required properties:
> > +  - compatible: should be:
> > +    - "renesas,r9a05g032-pinctrl"
> > +  - reg
> > +    address base and length of the memory area where the pin controller
> > +    hardware is mapped to.
> > +
> > +Example:
> > +pinctrl: pinctrl@40067000 {
> > +compatible = "renesas,r9a06g032-pinctrl";
> > +reg = <0x40067000 0x1000>, <0x51000000 0x800>;
> > +clocks = <&sysctrl R9A06G032_HCLK_PINCONFIG>;
> > +clock-names = "bus";
> > +status = "okay";
> > +};
> > +
> > +
> > +Sub-nodes
> > +---------
> > +  The child nodes of the pin controller node describe a pin
> > +multiplexing
> > +  group that can be used by driver 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.
> > +
> > +  The allowed generic formats for a pin multiplexing sub-node are the
> > + following ones:
> > +
> > +  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:
> > +    - renesas,rzn1-pinctrl:
> > +      Array of integers representing each 'pin' and it's configuration.
> > +
> > +      A 'pin number' does not correspond 1:1 to the hardware manual notion
> of
> > +      PL_GPIO directly. Numbers 0...169 are PL_GPIOs, however there is also
> two
> > +      extra 170 and 171 that corresponds to the MDIO0 and MDIO1 bus
> config.
> > +
> > +      A 'function' also does not correspond 1:1 to the hardware manual.
> There
> > +      are 2 levels of pin muxing, Level 1, level 2 -- to this are added the
> > +      MDIO configurations.
> > +
> > +      Helper macros to ease assembling the pin index and function identifier
> > +      are provided by the pin controller header file at:
> > +      <include/dt-bindings/pinctrl/r9a06g032-pinctrl.h>
> > +
> > +Example #1:
> > +  A simple case configuring only the function for a given 'pin' works as
> follow:
> > +#include <include/dt-bindings/pinctrl/r9a06g032-pinctrl.h>
> > +&pinctrl {
> > +pinsuart0: pinsuart0 {
> > +renesas,rzn1-pinmux-ids = <
> > +RZN1_MUX(103, UART0_I)/*
> UART0_TXD */
> > +RZN1_MUX(104, UART0_I)/*
> UART0_RXD */
> > +>;
> > +};
> > +};
> > +
> > +&uart0 {
> > +status = "okay";
> > +pinctrl-names = "default";
> > +pinctrl-0 = <&pinsuart0>;
> > +};
> > +  Note that in this case the other functions of the pins are not changed.
> > +
> > +Example #2:
> > +  Here we also set the pullups on the RXD pin:
> > +&pinctrl {
> > +pinsuart0: pinsuart0 {
> > +renesas,rzn1-pinmux-ids = <
> > +RZN1_MUX(103, UART0_I)/*
> UART0_TXD */
> > +RZN1_MUX_PUP(104, UART0_I)/*
> UART0_RXD */
> > +>;
> > +};
> > +};
> > +  There are many alternative macros to set the pullup/down/none and
> > +the drive
> > +  strenght in the r9a06g032-pinctrl.h header file.
> > +
> > +Example #3:
> > +  The Soc has two configurable MDIO muxes, these can also be
> > +configured
> > +  with this interface using the 'special' MDIO constants:
> > +
> > +&pinctrl {
> > +mdio_mux: mdiomux {
> > +renesas,rzn1-pinmux-ids = <
> > +RZN1_MUX(RZN1_MDIO_BUS0,
> RZN1_FUNC_MDIO_MUX_MAC0)
> > +RZN1_MUX(RZN1_MDIO_BUS1,
> RZN1_FUNC_MDIO_MUX_SWITCH)
> > +>;
> > +};
> > +};
> > +  Clearly the pull/up/none and drive constants will be ignored, even
> > +if
> > +  specified.
> > +
> > +Note that Renesas provides an extensive webapp that can generates a
> > +device tree file for your board. See their website for this part number for
> details.
> > --
> > 2.7.4
> >



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

RE: [PATCH v1 2/5] dt-bindings: clock: renesas,r9a06g032-pinctrl: documentation

[Chris Brandt]

On Thursday, June 14, 2018, Michel Pollet wrote:
> > From a very superficial look at the proposed bindings and pinctrl driver,
> I
> > wonder if the rza1 pinctrl driver could be expanded and re-used.
> > Have you considered that before starting one from scratch?
> 
> Sorry it's just the start of the documentation which I wrongly cut/pasted
> -- the
> rest is completely different; the only reason A1 is mentioned here is that
> I used
> that file as a template for the header/sections etc... Somehow the
> title+blurb
> went unedited!

Phew!

I can't stand that pinctrl HW in RZ/A1.

I hope no other SoC design team uses that thing again.


Chris

Re: [PATCH v1 2/5] dt-bindings: clock: renesas,r9a06g032-pinctrl: documentation

[jacopo mondi]

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

Hi again Michel,

On Thu, Jun 14, 2018 at 12:00:18PM +0100, Michel Pollet wrote:
> The Renesas R9A06G032 PINCTRL node description.
>
> Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
> ---
>  .../bindings/pinctrl/renesas,r9a06g032-pinctrl.txt | 124 +++++++++++++++++++++
>  1 file changed, 124 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,r9a06g032-pinctrl.txt
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,r9a06g032-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,r9a06g032-pinctrl.txt
> new file mode 100644
> index 0000000..f63696f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/renesas,r9a06g032-pinctrl.txt
> @@ -0,0 +1,124 @@
> +Renesas RZ/A1 combined Pin and GPIO controller
> +
> +The Renesas SoCs of the RZ/A1 family feature a combined Pin and GPIO controller,
> +named "Ports" in the hardware reference manual.
> +Pin multiplexing and GPIO configuration is performed on a per-pin basis
> +writing configuration values to per-port register sets.
> +Each "port" features up to 16 pins, each of them configurable for GPIO
> +function (port mode) or in alternate function mode.
> +Up to 8 different alternate function modes exist for each single pin.

Apart from the above part that should be re-worked, and the
s/clock/pinctrl in the patch subject, I have some more comments on the
proposed bindings.

> +
> +Pin controller node
> +-------------------
> +
> +Required properties:
> +  - compatible: should be:
> +    - "renesas,r9a05g032-pinctrl"
> +  - reg
> +    address base and length of the memory area where the pin controller
> +    hardware is mapped to.
> +
> +Example:
> +	pinctrl: pinctrl@40067000 {
> +		compatible = "renesas,r9a06g032-pinctrl";
> +		reg = <0x40067000 0x1000>, <0x51000000 0x800>;
> +		clocks = <&sysctrl R9A06G032_HCLK_PINCONFIG>;
> +		clock-names = "bus";
> +		status = "okay";
> +	};
> +
> +
> +Sub-nodes
> +---------
> +  The child nodes of the pin controller node describe a pin multiplexing
> +  group that can be used by driver nodes.
> +

s/driver nodes/device 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.
> +

That's fine, even if it's a repetition of what is described in the
generic pinctrl bindings (pinctrl-bindings.txt)

> +  The allowed generic formats for a pin multiplexing sub-node are the
> +  following ones:
> +
> +  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>;
> +      ...
> +  };
> +

That's not necessary imho. Just refer to the generic pinctrl bindings
documentation. Or are there differences I am missing here?

> +  Required properties:
> +    - renesas,rzn1-pinctrl:
> +      Array of integers representing each 'pin' and it's configuration.
> +

Why a custom property? When upstreaming the rz/a1 infamous pinctrl
driver, we extended the generic bindings with the 'pinxmux' property
that allows to specify an array of (pin id + mux) 'pinmux groups', as reported
in the generic bindings documentation:

----------------------------------------------------------------------------
For hardware where pin multiplexing configurations have to be specified for
each single pin the number of required sub-nodes containing "pin" and
"function" properties can quickly escalate and become hard to write and
maintain.

For cases like this, the pin controller driver may use the pinmux helper
property, where the pin identifier is provided with mux configuration settings
in a pinmux group. A pinmux group consists of the pin identifier and mux
settings represented as a single integer or an array of integers.

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

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

Each individual pin controller driver bindings documentation shall specify
how pin IDs and pin multiplexing configuration are defined and assembled
together in a pinmux group.
-----------------------------------------------------------------------------

Do you think too it applies to your use case?

> +      A 'pin number' does not correspond 1:1 to the hardware manual notion of
> +      PL_GPIO directly. Numbers 0...169 are PL_GPIOs, however there is also two
> +      extra 170 and 171 that corresponds to the MDIO0 and MDIO1 bus config.
> +
> +      A 'function' also does not correspond 1:1 to the hardware manual. There
> +      are 2 levels of pin muxing, Level 1, level 2 -- to this are added the
> +      MDIO configurations.
> +
> +      Helper macros to ease assembling the pin index and function identifier
> +      are provided by the pin controller header file at:
> +      <include/dt-bindings/pinctrl/r9a06g032-pinctrl.h>

This part is good and represent what the generic bindings refers to
with:

Each individual pin controller driver bindings documentation shall specify
how pin IDs and pin multiplexing configuration are defined and assembled
together in a pinmux group.

> +
> +Example #1:
> +  A simple case configuring only the function for a given 'pin' works as follow:
> +	#include <include/dt-bindings/pinctrl/r9a06g032-pinctrl.h>
> +	&pinctrl {
> +		pinsuart0: pinsuart0 {
> +			renesas,rzn1-pinmux-ids = <
> +				RZN1_MUX(103, UART0_I)	/* UART0_TXD */
> +				RZN1_MUX(104, UART0_I)	/* UART0_RXD */
> +			>;
> +		};
> +	};

That would be like

	&pinctrl {
		pinsuart0: pinsuart0 {
			pinmux = < RZN1_MUX(103, UART0_I),	/* UART0_TXD */
				   RZN1_MUX(104, UART0_I)	/* UART0_RXD */
			>;
		};
	};

> +
> +	&uart0 {
> +		status = "okay";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinsuart0>;
> +	};

just report the pinmux node, no need for the client in the example.
It's standard stuff.

> +  Note that in this case the other functions of the pins are not changed.

What 'other functions'? The pin configuration as in pull up/down etc?

> +
> +Example #2:
> +  Here we also set the pullups on the RXD pin:
> +	&pinctrl {
> +		pinsuart0: pinsuart0 {
> +			renesas,rzn1-pinmux-ids = <
> +				RZN1_MUX(103, UART0_I)	/* UART0_TXD */
> +				RZN1_MUX_PUP(104, UART0_I)	/* UART0_RXD */
> +			>;
> +		};
> +	};

I recall we used to upstream pin configuration along with pixmuxing,
as you're doing here and it didn't end well. I suggest to use the standard
properties where possible, in this case 'bias-pull-up'.

I think it should look like this:

	&pinctrl {
		pinsuart0: pinsuart0 {
                        pinsuart_rx {
                                /* UART0_TXD */
                                pinmux = <RZN1_MUX(103, UART0_I)>;
                        };
                        pinsuart_tx {
                                /* UART0_TXD with pull-up */
                                pinmux = <RZN1_MUX(104, UART0_I)>;
                                bias-pull-up;

                        };
		};
	};

        &uart0 {
                pinctrl-0 = <&pinsuart0>;
                ...
        };

Then in your driver you should parse pin configuration properties as
collected by the pinctrl core and apply them where appropriate.

> +  There are many alternative macros to set the pullup/down/none and the drive
> +  strenght in the r9a06g032-pinctrl.h header file.
> +
> +Example #3:
> +  The Soc has two configurable MDIO muxes, these can also be configured
> +  with this interface using the 'special' MDIO constants:
> +
> +	&pinctrl {
> +		mdio_mux: mdiomux {
> +			renesas,rzn1-pinmux-ids = <
> +				RZN1_MUX(RZN1_MDIO_BUS0, RZN1_FUNC_MDIO_MUX_MAC0)
> +				RZN1_MUX(RZN1_MDIO_BUS1, RZN1_FUNC_MDIO_MUX_SWITCH)
> +			>;
> +		};
> +	};
> +  Clearly the pull/up/none and drive constants will be ignored, even if
> +  specified.
> +
> +Note that Renesas provides an extensive webapp that can generates a device tree
> +file for your board. See their website for this part number for details.

Imho this shouldn't be mentioned here, and autogeneration of device
tree files with a custom proprietary tool shouldn't be encouraged at
all, at least not from the device bindings.

Thanks
   j

> --
> 2.7.4
>

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

[PATCH v1 3/5] pinctrl: renesas: Renesas R9A06G032 pinctrl driver

[Michel Pollet]

This provides a pinctrl driver for the Renesas R09A06G032.

Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
---
 drivers/pinctrl/Kconfig             |  10 +
 drivers/pinctrl/Makefile            |   1 +
 drivers/pinctrl/pinctrl-r9a06g032.c | 890 ++++++++++++++++++++++++++++++++++++
 3 files changed, 901 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-r9a06g032.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index dd50371..22d7546 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -177,6 +177,16 @@ config PINCTRL_OXNAS
 	select GPIOLIB_IRQCHIP
 	select MFD_SYSCON
 
+config PINCTRL_R9A06G032
+	bool "Renesas R9A06G032 pinctrl driver"
+	depends on OF
+	depends on ARCH_R9A06G032 || COMPILE_TEST
+	select GENERIC_PINCTRL_GROUPS
+	select GENERIC_PINMUX_FUNCTIONS
+	select GENERIC_PINCONF
+	help
+	  This selects pinctrl driver for Renesas R9A06G032.
+
 config PINCTRL_ROCKCHIP
 	bool
 	select PINMUX
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index de40863..5912861 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_PINCTRL_OXNAS)	+= pinctrl-oxnas.o
 obj-$(CONFIG_PINCTRL_PALMAS)	+= pinctrl-palmas.o
 obj-$(CONFIG_PINCTRL_PIC32)	+= pinctrl-pic32.o
 obj-$(CONFIG_PINCTRL_PISTACHIO)	+= pinctrl-pistachio.o
+obj-$(CONFIG_PINCTRL_R9A06G032)	+= pinctrl-r9a06g032.o
 obj-$(CONFIG_PINCTRL_ROCKCHIP)	+= pinctrl-rockchip.o
 obj-$(CONFIG_PINCTRL_RZA1)	+= pinctrl-rza1.o
 obj-$(CONFIG_PINCTRL_SINGLE)	+= pinctrl-single.o
diff --git a/drivers/pinctrl/pinctrl-r9a06g032.c b/drivers/pinctrl/pinctrl-r9a06g032.c
new file mode 100644
index 0000000..a71dd24
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-r9a06g032.c
@@ -0,0 +1,890 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2014-2018 Renesas Electronics Europe Limited
+ *
+ * Michel Pollet <michel.pollet@bp.renesas.com>, <buserror@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <dt-bindings/pinctrl/r9a06g032-pinctrl.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include "core.h"
+
+/*
+ * The pinmux hardware has two levels. The first level functions goes from
+ * 0 to 9, and the level 1 mode '15' (0xf) specifies that the second level
+ * of pinmux should be used instead, that level has a lot more options,
+ * and goes from 0 to ~60.
+ *
+ * For linux, we've compounded both numbers together, so 0 to 9 is level 1,
+ * and anything higher is in fact 10 + level 2 number, so we end up with one
+ * value from 0 to 70 or so.
+ *
+ * There are 170 configurable pins (called PL_GPIO in the datasheet).
+ *
+ * Furthermore, the two MDIO outputs also have a mux each, that can be set
+ * to 8 different values (including highz as well).
+ *
+ * So, for Linux, we also made up to extra "GPIOs" 170 and 171, and also added
+ * extra functions to match their mux. This allow the device tree to be
+ * completely transparent to these subtleties.
+ */
+
+struct rzn1_pinctrl_regs {
+	union {
+		u32	conf[170];
+		u8	pad0[0x400];
+	};
+	union {
+		struct {
+			u32	status_protect;	/* 0x400 */
+			/* MDIO mux registers, l2 only */
+			u32	l2_mdio[2];
+		};
+		u8	pad1[0x80];
+	};
+	u32	l2_gpio_int_mux[8];	/* 0x480 + (k*4) */
+};
+
+
+/**
+ * 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
+ * @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. pinctrl forces us to maintain such an array
+ * @pins: array of pins
+ */
+struct rzn1_pin_group {
+	const char *name;
+	const char *func;
+	unsigned int npins;
+	u32 *pin_ids;
+	u32 *pins;
+};
+
+/**
+ * @dev: a pointer back to containing device
+ * @base: the offset to the controller in virtual memory
+ */
+struct rzn1_pinctrl {
+	struct device *dev;
+	struct pinctrl_dev *pctl;
+	struct rzn1_pinctrl_regs __iomem *lev1;
+	struct rzn1_pinctrl_regs __iomem *lev2;
+	u32 lev1_phys;
+	u32 lev2_phys;
+
+	struct rzn1_pin_group *groups;
+	unsigned int ngroups, maxgroups;
+
+	struct rzn1_pmx_func *functions;
+	unsigned int nfunctions;
+};
+
+#define RZN1_PINS_PROP "renesas,rzn1-pinmux-ids"
+
+#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),
+	PINCTRL_PIN(170, "mdio0"), PINCTRL_PIN(171, "mdio1")
+};
+
+/* Bit number and bit values in the pinmux registers */
+enum {
+	RZN1_LEV_DRIVE_STRENGTH		= 10,
+		RZN1_LEV_DRIVE_4MA		= 0,
+		RZN1_LEV_DRIVE_6MA		= 1,
+		RZN1_LEV_DRIVE_8MA		= 2,
+		RZN1_LEV_DRIVE_12MA		= 3,
+	RZN1_LEV_DRIVE_PULL		= 8,
+		RZN1_LEV_DRIVE_PULL_NONE	= 0,
+		RZN1_LEV_DRIVE_PULL_UP		= 1,
+		RZN1_LEV_DRIVE_PULL_NONE2	= 2,
+		RZN1_LEV_DRIVE_PULL_DOWN	= 3,
+	RZN1_FUNCTION			= 0,
+		RZN1_LEV_FUNCTION_MASK		= 0xf,
+		RZN1_LEV_FUNCTION_LEV2		= 0xf,
+		RZN1_LEV2_FUNCTION_MASK		= 0x3f,
+	RZN1_WP_STATE			= 0,
+};
+
+enum {
+	MDIO_MUX_HIGHZ = 0,
+	MDIO_MUX_MAC0,
+	MDIO_MUX_MAC1,
+	MDIO_MUX_ECAT,
+	MDIO_MUX_S3_MDIO0,
+	MDIO_MUX_S3_MDIO1,
+	MDIO_MUX_HWRTOS,
+	MDIO_MUX_SWITCH,
+};
+
+struct rzn1_pin_desc {
+	u32	pin: 8, func: 7, has_func : 1, has_drive: 1,
+		drive : 2, has_pull : 1, pull : 2;
+};
+
+static struct rzn1_pinctrl *pinctrl;
+
+/*
+ * Breaks down the configuration word (as present in the DT) into
+ * a manageable structural description
+ */
+static void rzn1_get_pin_desc_from_config(
+	struct rzn1_pinctrl *ipctl,
+	u32 pin_config,
+	struct rzn1_pin_desc *o)
+{
+	struct rzn1_pin_desc p = {
+		.pin = pin_config,
+		.func = pin_config >> RZN1_MUX_FUNC_BIT,
+		.has_func = pin_config >> RZN1_MUX_HAS_FUNC_BIT,
+		.has_drive = pin_config >> RZN1_MUX_HAS_DRIVE_BIT,
+		.drive = pin_config >> RZN1_MUX_DRIVE_BIT,
+		.has_pull = pin_config >> RZN1_MUX_HAS_PULL_BIT,
+		.pull = pin_config >> RZN1_MUX_PULL_BIT,
+	};
+
+	if (o)
+		*o = p;
+}
+
+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)
+{
+	if (lock & LOCK_LEVEL1) {
+		u32 val = (ipctl->lev1_phys + 0x400) | !(value & LOCK_LEVEL1);
+
+		writel(val, &ipctl->lev1->status_protect);
+	}
+	if (lock & LOCK_LEVEL2) {
+		u32 val = (ipctl->lev2_phys + 0x400) | !(value & LOCK_LEVEL2);
+
+		writel(val, &ipctl->lev2->status_protect);
+	}
+}
+
+static void rzn1_pinctrl_mdio_select(u8 mdio, u32 func)
+{
+	BUG_ON(!pinctrl || mdio > 1 || func > MDIO_MUX_SWITCH);
+	dev_info(pinctrl->dev, "setting mdio %d to 0x%x\n", mdio, func);
+
+	rzn1_hw_set_lock(pinctrl, LOCK_LEVEL2, LOCK_LEVEL2);
+	writel(func, &pinctrl->lev2->l2_mdio[mdio]);
+	rzn1_hw_set_lock(pinctrl, LOCK_LEVEL2, 0);
+}
+
+/*
+ * 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_parameters(
+	struct rzn1_pinctrl *ipctl,
+	u32 pin_config,
+	u8 use_locks)
+{
+	struct rzn1_pin_desc p;
+	u32 l1, l2, l1_cache, l2_cache;
+
+	rzn1_get_pin_desc_from_config(ipctl, pin_config, &p);
+#if 0 /* bit noisy, even in debug mode */
+	dev_dbg(ipctl->dev,
+		"SET pin %3d:%08x func:%d/%d(0x%2x) drive:%d/%x pull:%d/%x\n",
+		p.pin, pin_config,
+		p.has_func, p.func, p.func - RZN1_FUNC_LEVEL2_OFFSET,
+		p.has_drive, p.drive,
+		p.has_pull, p.pull);
+#endif
+	if (p.pin >= RZN1_MDIO_BUS0 && p.pin <= RZN1_MDIO_BUS1) {
+		if (p.has_func && p.func >= RZN1_FUNC_MDIO_MUX_HIGHZ &&
+				p.func <= RZN1_FUNC_MDIO_MUX_SWITCH) {
+			p.pin -= RZN1_MDIO_BUS0;
+			p.func -= RZN1_FUNC_MDIO_MUX_HIGHZ;
+			dev_dbg(ipctl->dev, "MDIO MUX[%d] set to %d\n",
+				p.pin, p.func);
+			rzn1_pinctrl_mdio_select(p.pin, p.func);
+		} else {
+			dev_warn(ipctl->dev, "MDIO[%d] Invalid configuration: %d\n",
+				p.pin - RZN1_MDIO_BUS0, p.func);
+			return -EINVAL;
+		}
+		return 0;
+	}
+	/* Note here, we do not allow anything past the MDIO Mux values */
+	if (p.pin >= ARRAY_SIZE(ipctl->lev1->conf) ||
+			p.func >= RZN1_FUNC_MDIO_MUX_HIGHZ)
+		return -EINVAL;
+	l1 = readl(&ipctl->lev1->conf[p.pin]);
+	l1_cache = l1;
+	l2 = readl(&ipctl->lev2->conf[p.pin]);
+	l2_cache = l2;
+
+	if (p.has_drive) {
+		l1 &= ~(0x3 << RZN1_LEV_DRIVE_STRENGTH);
+		l1 |= (p.drive << RZN1_LEV_DRIVE_STRENGTH);
+	}
+	if (p.has_pull) {
+		l1 &= ~(0x3 << RZN1_LEV_DRIVE_PULL);
+		l1 |= (p.pull << RZN1_LEV_DRIVE_PULL);
+	}
+	if (p.has_func) {
+		if (p.func < RZN1_FUNC_LEVEL2_OFFSET) {
+			l1 &= ~(RZN1_LEV_FUNCTION_MASK << RZN1_FUNCTION);
+			l1 |= (p.func << RZN1_FUNCTION);
+		} else {
+			l1 &= ~(RZN1_LEV_FUNCTION_MASK << RZN1_FUNCTION);
+			l1 |= (RZN1_LEV_FUNCTION_LEV2 << RZN1_FUNCTION);
+
+			l2 = p.func - RZN1_FUNC_LEVEL2_OFFSET;
+		}
+	}
+	/* If either of the configuration change, we update both
+	 * anyway.
+	 */
+	if (l1 != l1_cache || l2 != l2_cache) {
+		rzn1_hw_set_lock(ipctl, use_locks, LOCK_ALL);
+		writel(l1, &ipctl->lev1->conf[p.pin]);
+		writel(l2, &ipctl->lev2->conf[p.pin]);
+		rzn1_hw_set_lock(ipctl, use_locks, 0);
+	}
+	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;
+}
+
+static void rzn1_pin_dbg_show(
+	struct pinctrl_dev *pctldev,
+	struct seq_file *s,
+	unsigned int offset)
+{
+	seq_printf(s, "%s", dev_name(pctldev->dev));
+}
+
+static int rzn1_dt_node_to_map(
+	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;
+	struct pinctrl_map *new_map;
+	struct device_node *parent;
+	int map_num = 2;
+
+	/*
+	 * first find the group of this node and check if we need create
+	 * config maps for pins
+	 */
+	grp = rzn1_pinctrl_find_group_by_name(ipctl, np->name);
+	if (!grp) {
+		dev_err(ipctl->dev, "unable to find group for node %s\n",
+			np->name);
+		return -EINVAL;
+	}
+
+	new_map = devm_kmalloc_array(ipctl->dev, map_num,
+			sizeof(struct pinctrl_map), GFP_KERNEL);
+	if (!new_map)
+		return -ENOMEM;
+
+	*map = new_map;
+	*num_maps = map_num;
+
+	/* create mux map */
+	parent = of_get_parent(np);
+	if (!parent)
+		return -EINVAL;
+
+	new_map[0].type = PIN_MAP_TYPE_MUX_GROUP;
+	new_map[0].data.mux.function = grp->func;
+	new_map[0].data.mux.group = grp->name;
+	of_node_put(parent);
+
+	new_map[1].type = PIN_MAP_TYPE_CONFIGS_GROUP;
+	new_map[1].data.configs.group_or_pin = grp->name;
+	new_map[1].data.configs.configs = (unsigned long *)grp->pin_ids;
+	new_map[1].data.configs.num_configs = grp->npins;
+
+	dev_dbg(pctldev->dev, "maps: function %s group %s (%d pins)\n",
+		(*map)->data.mux.function, (*map)->data.mux.group,
+		grp->npins);
+
+	return 0;
+}
+
+static void rzn1_dt_free_map(struct pinctrl_dev *pctldev,
+				struct pinctrl_map *map, unsigned int num_maps)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+
+	devm_kfree(ipctl->dev, map);
+}
+
+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,
+	.pin_dbg_show = rzn1_pin_dbg_show,
+	.dt_node_to_map = rzn1_dt_node_to_map,
+	.dt_free_map = rzn1_dt_free_map,
+};
+
+static int rzn1_pmx_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;
+
+	/*
+	 * Configure the mux mode for each pin in the group for a specific
+	 * function.
+	 */
+	grp = &ipctl->groups[group];
+
+	dev_dbg(ipctl->dev, "enable function %s(%d) group %s(%d)\n",
+		ipctl->functions[selector].name, selector, grp->name, group);
+	/*
+	 * Well, theres not much to do here anyway, as the individual pin
+	 * callback is going to be called anyway
+	 */
+	return 0;
+}
+
+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 * 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 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_pmx_set_mux,
+};
+
+static int rzn1_pinconf_get(struct pinctrl_dev *pctldev,
+			     unsigned int pin_id, unsigned long *config)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+
+	pin_id &= 0xff;
+	if (pin_id >= ARRAY_SIZE(ipctl->lev1->conf))
+		return -EINVAL;
+	*config = readl(&ipctl->lev1->conf[pin_id]) & 0xf;
+	if (*config == 0xf)
+		*config = (readl(&ipctl->lev2->conf[pin_id]) & 0x3f) +
+				RZN1_FUNC_LEVEL2_OFFSET;
+	*config = (*config << RZN1_MUX_FUNC_BIT) | pin_id;
+	return 0;
+}
+
+static int rzn1_pinconf_set(struct pinctrl_dev *pctldev,
+			     unsigned int pin_id, unsigned long *configs,
+			     unsigned int num_configs)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+	int i;
+
+	dev_dbg(ipctl->dev, "pinconf set pin %s (%d configs)\n",
+		rzn1_pins[pin_id].name, num_configs);
+
+	for (i = 0; i < num_configs; i++)
+		rzn1_set_hw_pin_parameters(ipctl, configs[i], LOCK_ALL);
+
+	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 i;
+	/*
+	 * Configure the mux mode for each pin in the group for a specific
+	 * function.
+	 */
+	dev_dbg(ipctl->dev, "group set %s selector:%d configs:%p/%d\n",
+		grp->name, selector, configs, num_configs);
+
+	rzn1_hw_set_lock(ipctl, LOCK_ALL, LOCK_ALL);
+	for (i = 0; i < num_configs; i++)
+		rzn1_set_hw_pin_parameters(ipctl, configs[i], 0);
+	rzn1_hw_set_lock(ipctl, LOCK_ALL, 0);
+
+	return 0;
+}
+
+static void rzn1_pinconf_dbg_show(struct pinctrl_dev *pctldev,
+				   struct seq_file *s, unsigned int pin_id)
+{
+	unsigned long config = pin_id;
+
+	seq_printf(s, "0x%lx", config);
+}
+
+static void rzn1_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
+					 struct seq_file *s, unsigned int group)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+	struct rzn1_pin_group *grp;
+	unsigned long config;
+	const char *name;
+	int i, ret;
+
+	if (group > ipctl->ngroups)
+		return;
+
+	seq_puts(s, "\n");
+	grp = &ipctl->groups[group];
+	for (i = 0; i < grp->npins; i++) {
+		name = pin_get_name(pctldev, grp->pin_ids[i] & 0xff);
+		ret = rzn1_pinconf_get(pctldev, grp->pin_ids[i], &config);
+		if (ret)
+			return;
+		seq_printf(s, "%s: 0x%lx", name, config);
+	}
+}
+
+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,
+	.pin_config_dbg_show = rzn1_pinconf_dbg_show,
+	.pin_config_group_dbg_show = rzn1_pinconf_group_dbg_show,
+};
+
+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
+	 *	renesas,rzn1-pinmux-ids = <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;
+	}
+
+	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;
+
+	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 (of_property_count_u32_elems(np, RZN1_PINS_PROP) > 0) {
+		func->groups[i] = np->name;
+		grp = &ipctl->groups[ipctl->ngroups];
+		grp->func = func->name;
+		if (rzn1_pinctrl_parse_groups(np, grp, ipctl) > 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;
+		if (rzn1_pinctrl_parse_groups(child, grp, ipctl) > 0) {
+			i++;
+			ipctl->ngroups++;
+		}
+	}
+	dev_dbg(ipctl->dev, "function %s parsed %d/%d groups\n",
+		np->name, i, func->num_groups);
+
+	return 0;
+}
+
+static ssize_t _rzn1_pinctrl_sysfs_force_mux(struct device *dev,
+		struct device_attribute *attr,
+		const char *buf, size_t len)
+{
+	struct rzn1_pinctrl *ipctl = dev_get_drvdata(dev);
+	unsigned long int val;
+
+	if (kstrtoul(buf, 16, &val)) {
+		dev_err(ipctl->dev, "Invalid hex value %08x", (u32)val);
+		return len;
+	}
+
+	dev_info(ipctl->dev, "setting pin to %08x\n", (u32)val);
+
+	rzn1_set_hw_pin_parameters(ipctl, val, LOCK_ALL);
+
+	return len;
+}
+
+static DEVICE_ATTR(force_mux, 0200, NULL, _rzn1_pinctrl_sysfs_force_mux);
+
+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;
+	u32 nfuncs = 0;
+	u32 i = 0;
+
+	if (!np)
+		return -ENODEV;
+
+	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(struct rzn1_pmx_func),
+				GFP_KERNEL);
+	if (!ipctl->functions)
+		return -ENOMEM;
+
+	ipctl->ngroups = 0;
+	ipctl->maxgroups = 0;
+	for_each_child_of_node(np, child)
+		ipctl->maxgroups += rzn1_pinctrl_count_function_groups(child);
+	ipctl->groups = devm_kmalloc_array(
+				&pdev->dev,
+				ipctl->maxgroups, sizeof(*ipctl->groups),
+				GFP_KERNEL);
+	if (!ipctl->groups)
+		return -ENOMEM;
+
+	for_each_child_of_node(np, child)
+		rzn1_pinctrl_parse_functions(child, ipctl, i++);
+
+	if (device_create_file(&pdev->dev, &dev_attr_force_mux))
+		dev_warn(&pdev->dev, "cannot create status attribute\n");
+
+	return 0;
+}
+
+static int rzn1_pinctrl_probe(struct platform_device *pdev)
+{
+	struct rzn1_pinctrl *ipctl;
+	struct resource *res;
+	struct clk *clk;
+	int ret;
+
+	/* Create state holders etc for this driver */
+	ipctl = devm_kzalloc(&pdev->dev, sizeof(*ipctl), GFP_KERNEL);
+	if (!ipctl)
+		return -ENOMEM;
+
+	clk = devm_clk_get(&pdev->dev, "bus");
+	if (IS_ERR(clk) || clk_prepare_enable(clk))
+		dev_info(&pdev->dev, "no clock source\n");
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	ipctl->lev1_phys = (u32) res->start;
+	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_phys = (u32) res->start;
+	ipctl->lev2 = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(ipctl->lev2))
+		return PTR_ERR(ipctl->lev2);
+
+	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");
+		return ret;
+	}
+
+	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");
+		return -EINVAL;
+	}
+	pinctrl = ipctl;
+	dev_info(&pdev->dev, "initialized rzn1 pinctrl driver\n");
+	return 0;
+}
+
+static int rzn1_pinctrl_remove(struct platform_device *pdev)
+{
+	device_remove_file(&pdev->dev, &dev_attr_force_mux);
+
+	return 0;
+}
+
+static const struct of_device_id rzn1_pinctrl_match[] = {
+	{ .compatible = "renesas,r9a06g032-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		= "r9a06g032-pinctrl",
+		.owner		= THIS_MODULE,
+		.of_match_table	= rzn1_pinctrl_match,
+	},
+};
+
+static int __init _pinctrl_drv_register(void)
+{
+	return platform_driver_register(&rzn1_pinctrl_driver);
+}
+postcore_initcall(_pinctrl_drv_register);
+
+
+MODULE_AUTHOR("Michel Pollet <Michel.Pollet@bp.renesas.com>");
+MODULE_DESCRIPTION("Renesas R9A06G032 pinctrl driver");
+MODULE_LICENSE("GPL");
-- 
2.7.4

Re: [PATCH v1 3/5] pinctrl: renesas: Renesas R9A06G032 pinctrl driver

[jacopo mondi]

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

Hi Michel,

On Thu, Jun 14, 2018 at 12:00:19PM +0100, Michel Pollet wrote:
> This provides a pinctrl driver for the Renesas R09A06G032.
>

As I wait for the comments on the proposed bindings to be discussed,
before reviewing the details of this driver, for now I'm just pointing out the
minor stuff that is easy fixable. This is mostly style related stuff
you would have found out by yourself using checkpatch probably.

> Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
> ---
>  drivers/pinctrl/Kconfig             |  10 +
>  drivers/pinctrl/Makefile            |   1 +
>  drivers/pinctrl/pinctrl-r9a06g032.c | 890 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 901 insertions(+)
>  create mode 100644 drivers/pinctrl/pinctrl-r9a06g032.c
>
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index dd50371..22d7546 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -177,6 +177,16 @@ config PINCTRL_OXNAS
>  	select GPIOLIB_IRQCHIP
>  	select MFD_SYSCON
>
> +config PINCTRL_R9A06G032
> +	bool "Renesas R9A06G032 pinctrl driver"
> +	depends on OF
> +	depends on ARCH_R9A06G032 || COMPILE_TEST
> +	select GENERIC_PINCTRL_GROUPS
> +	select GENERIC_PINMUX_FUNCTIONS
> +	select GENERIC_PINCONF
> +	help
> +	  This selects pinctrl driver for Renesas R9A06G032.
> +
>  config PINCTRL_ROCKCHIP
>  	bool
>  	select PINMUX
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> index de40863..5912861 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_PINCTRL_OXNAS)	+= pinctrl-oxnas.o
>  obj-$(CONFIG_PINCTRL_PALMAS)	+= pinctrl-palmas.o
>  obj-$(CONFIG_PINCTRL_PIC32)	+= pinctrl-pic32.o
>  obj-$(CONFIG_PINCTRL_PISTACHIO)	+= pinctrl-pistachio.o
> +obj-$(CONFIG_PINCTRL_R9A06G032)	+= pinctrl-r9a06g032.o
>  obj-$(CONFIG_PINCTRL_ROCKCHIP)	+= pinctrl-rockchip.o
>  obj-$(CONFIG_PINCTRL_RZA1)	+= pinctrl-rza1.o
>  obj-$(CONFIG_PINCTRL_SINGLE)	+= pinctrl-single.o
> diff --git a/drivers/pinctrl/pinctrl-r9a06g032.c b/drivers/pinctrl/pinctrl-r9a06g032.c
> new file mode 100644
> index 0000000..a71dd24
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-r9a06g032.c
> @@ -0,0 +1,890 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2014-2018 Renesas Electronics Europe Limited
> + *
> + * Michel Pollet <michel.pollet@bp.renesas.com>, <buserror@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.

The purpose of SPDX header is to replace the boilerplate license text.
Please drop it.

> + */
> +
> +#include <dt-bindings/pinctrl/r9a06g032-pinctrl.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/module.h>

Please sort alphabetically.

> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include "core.h"
> +
> +/*
> + * The pinmux hardware has two levels. The first level functions goes from
> + * 0 to 9, and the level 1 mode '15' (0xf) specifies that the second level
> + * of pinmux should be used instead, that level has a lot more options,
> + * and goes from 0 to ~60.
> + *
> + * For linux, we've compounded both numbers together, so 0 to 9 is level 1,
> + * and anything higher is in fact 10 + level 2 number, so we end up with one
> + * value from 0 to 70 or so.
> + *
> + * There are 170 configurable pins (called PL_GPIO in the datasheet).
> + *
> + * Furthermore, the two MDIO outputs also have a mux each, that can be set
> + * to 8 different values (including highz as well).
> + *
> + * So, for Linux, we also made up to extra "GPIOs" 170 and 171, and also added
> + * extra functions to match their mux. This allow the device tree to be
> + * completely transparent to these subtleties.
> + */
> +
> +struct rzn1_pinctrl_regs {
> +	union {
> +		u32	conf[170];
> +		u8	pad0[0x400];
> +	};
> +	union {
> +		struct {
> +			u32	status_protect;	/* 0x400 */
> +			/* MDIO mux registers, l2 only */
> +			u32	l2_mdio[2];
> +		};
> +		u8	pad1[0x80];
> +	};
> +	u32	l2_gpio_int_mux[8];	/* 0x480 + (k*4) */
> +};
> +
> +
> +/**

If you want to use kernel-doc, which imho is not strictly necessary as
this is driver internal stuff, please make sure you have all the
necessary fields to have it compile nicely. In some places the structure or
function names are missing, or some parameters are not documented.

> + * 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
> + * @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. pinctrl forces us to maintain such an array
> + * @pins: array of pins
> + */
> +struct rzn1_pin_group {
> +	const char *name;
> +	const char *func;
> +	unsigned int npins;
> +	u32 *pin_ids;
> +	u32 *pins;
> +};
> +
> +/**

like here

> + * @dev: a pointer back to containing device
> + * @base: the offset to the controller in virtual memory
> + */
> +struct rzn1_pinctrl {
> +	struct device *dev;
> +	struct pinctrl_dev *pctl;
> +	struct rzn1_pinctrl_regs __iomem *lev1;
> +	struct rzn1_pinctrl_regs __iomem *lev2;
> +	u32 lev1_phys;
> +	u32 lev2_phys;
> +
> +	struct rzn1_pin_group *groups;
> +	unsigned int ngroups, maxgroups;
> +
> +	struct rzn1_pmx_func *functions;
> +	unsigned int nfunctions;
> +};
> +
> +#define RZN1_PINS_PROP "renesas,rzn1-pinmux-ids"
> +
> +#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),
> +	PINCTRL_PIN(170, "mdio0"), PINCTRL_PIN(171, "mdio1")
> +};
> +
> +/* Bit number and bit values in the pinmux registers */
> +enum {
> +	RZN1_LEV_DRIVE_STRENGTH		= 10,
> +		RZN1_LEV_DRIVE_4MA		= 0,
> +		RZN1_LEV_DRIVE_6MA		= 1,
> +		RZN1_LEV_DRIVE_8MA		= 2,
> +		RZN1_LEV_DRIVE_12MA		= 3,
> +	RZN1_LEV_DRIVE_PULL		= 8,
> +		RZN1_LEV_DRIVE_PULL_NONE	= 0,
> +		RZN1_LEV_DRIVE_PULL_UP		= 1,
> +		RZN1_LEV_DRIVE_PULL_NONE2	= 2,
> +		RZN1_LEV_DRIVE_PULL_DOWN	= 3,
> +	RZN1_FUNCTION			= 0,
> +		RZN1_LEV_FUNCTION_MASK		= 0xf,
> +		RZN1_LEV_FUNCTION_LEV2		= 0xf,
> +		RZN1_LEV2_FUNCTION_MASK		= 0x3f,
> +	RZN1_WP_STATE			= 0,
> +};

Not sure an enum if the best choice for overlapping values. What about
using plain #define for them?

> +
> +enum {
> +	MDIO_MUX_HIGHZ = 0,
> +	MDIO_MUX_MAC0,
> +	MDIO_MUX_MAC1,
> +	MDIO_MUX_ECAT,
> +	MDIO_MUX_S3_MDIO0,
> +	MDIO_MUX_S3_MDIO1,
> +	MDIO_MUX_HWRTOS,
> +	MDIO_MUX_SWITCH,
> +};
> +
> +struct rzn1_pin_desc {
> +	u32	pin: 8, func: 7, has_func : 1, has_drive: 1,
> +		drive : 2, has_pull : 1, pull : 2;
> +};
> +
> +static struct rzn1_pinctrl *pinctrl;

Ouch a global pointer. I know it's hard to have two instances of this
driver registered on the same system, but you should retrieve the
driver private data from the pinctrl_dev driver_data. Ah, wait, you're
doing that, and you're using this global pointer in a single location,
is there any reason you need to do that?

> +
> +/*
> + * Breaks down the configuration word (as present in the DT) into
> + * a manageable structural description
> + */
> +static void rzn1_get_pin_desc_from_config(

I count three different styles you use to define functions in this
driver. Please be consistent and align below the first open brace, as
in

static struct verylongname *verylongfunctionname(struct param1,
                                                 struct param2,
                                                 struct param3)

When the function name does not span to the whole 80 cols.

For shorter function names, please span to the end of the line,

static void rzna1_get_pin_desc_from_config(struct rzn1_pinctrl *ipctl,
                                           u32 pin_config,
                                           struct rzn1_pin_desc *o)
{

}

also, ./scritps/checkpatch (--strict -f) is your friend.

> +	struct rzn1_pinctrl *ipctl,
> +	u32 pin_config,
> +	struct rzn1_pin_desc *o)
> +{
> +	struct rzn1_pin_desc p = {
> +		.pin = pin_config,
> +		.func = pin_config >> RZN1_MUX_FUNC_BIT,
> +		.has_func = pin_config >> RZN1_MUX_HAS_FUNC_BIT,
> +		.has_drive = pin_config >> RZN1_MUX_HAS_DRIVE_BIT,
> +		.drive = pin_config >> RZN1_MUX_DRIVE_BIT,
> +		.has_pull = pin_config >> RZN1_MUX_HAS_PULL_BIT,
> +		.pull = pin_config >> RZN1_MUX_PULL_BIT,
> +	};
> +
> +	if (o)
> +		*o = p;
> +}
> +
> +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)
> +{
> +	if (lock & LOCK_LEVEL1) {
> +		u32 val = (ipctl->lev1_phys + 0x400) | !(value & LOCK_LEVEL1);
> +
> +		writel(val, &ipctl->lev1->status_protect);
> +	}
> +	if (lock & LOCK_LEVEL2) {
> +		u32 val = (ipctl->lev2_phys + 0x400) | !(value & LOCK_LEVEL2);
> +
> +		writel(val, &ipctl->lev2->status_protect);
> +	}
> +}
> +
> +static void rzn1_pinctrl_mdio_select(u8 mdio, u32 func)
> +{
> +	BUG_ON(!pinctrl || mdio > 1 || func > MDIO_MUX_SWITCH);
> +	dev_info(pinctrl->dev, "setting mdio %d to 0x%x\n", mdio, func);
> +
> +	rzn1_hw_set_lock(pinctrl, LOCK_LEVEL2, LOCK_LEVEL2);
> +	writel(func, &pinctrl->lev2->l2_mdio[mdio]);
> +	rzn1_hw_set_lock(pinctrl, LOCK_LEVEL2, 0);
> +}
> +
> +/*
> + * 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_parameters(
> +	struct rzn1_pinctrl *ipctl,
> +	u32 pin_config,
> +	u8 use_locks)
> +{
> +	struct rzn1_pin_desc p;
> +	u32 l1, l2, l1_cache, l2_cache;

Someone prefers one variable declaration per line. Also, try to order
variable declaration to respect the reverse xmas tree order.

> +
> +	rzn1_get_pin_desc_from_config(ipctl, pin_config, &p);
> +#if 0 /* bit noisy, even in debug mode */
> +	dev_dbg(ipctl->dev,
> +		"SET pin %3d:%08x func:%d/%d(0x%2x) drive:%d/%x pull:%d/%x\n",
> +		p.pin, pin_config,
> +		p.has_func, p.func, p.func - RZN1_FUNC_LEVEL2_OFFSET,
> +		p.has_drive, p.drive,
> +		p.has_pull, p.pull);
> +#endif

So use dev_info or remove it altogether, but no #if 0 if not strictly
necessary, and here it is not.

> +	if (p.pin >= RZN1_MDIO_BUS0 && p.pin <= RZN1_MDIO_BUS1) {
> +		if (p.has_func && p.func >= RZN1_FUNC_MDIO_MUX_HIGHZ &&
> +				p.func <= RZN1_FUNC_MDIO_MUX_SWITCH) {
> +			p.pin -= RZN1_MDIO_BUS0;
> +			p.func -= RZN1_FUNC_MDIO_MUX_HIGHZ;
> +			dev_dbg(ipctl->dev, "MDIO MUX[%d] set to %d\n",
> +				p.pin, p.func);
> +			rzn1_pinctrl_mdio_select(p.pin, p.func);
> +		} else {
> +			dev_warn(ipctl->dev, "MDIO[%d] Invalid configuration: %d\n",
> +				p.pin - RZN1_MDIO_BUS0, p.func);
> +			return -EINVAL;
> +		}
> +		return 0;
> +	}

Man, this code is -dense-. I know there is no strict coding style rule
for that, but I think a line break, eg here, would benefit
readability. That's up to you though.

> +	/* Note here, we do not allow anything past the MDIO Mux values */
> +	if (p.pin >= ARRAY_SIZE(ipctl->lev1->conf) ||
> +			p.func >= RZN1_FUNC_MDIO_MUX_HIGHZ)

Align to open brace please

> +		return -EINVAL;

Empty line here after return?

> +	l1 = readl(&ipctl->lev1->conf[p.pin]);
> +	l1_cache = l1;
> +	l2 = readl(&ipctl->lev2->conf[p.pin]);
> +	l2_cache = l2;
> +
> +	if (p.has_drive) {
> +		l1 &= ~(0x3 << RZN1_LEV_DRIVE_STRENGTH);
> +		l1 |= (p.drive << RZN1_LEV_DRIVE_STRENGTH);
> +	}
> +	if (p.has_pull) {
> +		l1 &= ~(0x3 << RZN1_LEV_DRIVE_PULL);
> +		l1 |= (p.pull << RZN1_LEV_DRIVE_PULL);
> +	}
> +	if (p.has_func) {
> +		if (p.func < RZN1_FUNC_LEVEL2_OFFSET) {
> +			l1 &= ~(RZN1_LEV_FUNCTION_MASK << RZN1_FUNCTION);
> +			l1 |= (p.func << RZN1_FUNCTION);
> +		} else {
> +			l1 &= ~(RZN1_LEV_FUNCTION_MASK << RZN1_FUNCTION);
> +			l1 |= (RZN1_LEV_FUNCTION_LEV2 << RZN1_FUNCTION);
> +
> +			l2 = p.func - RZN1_FUNC_LEVEL2_OFFSET;
> +		}
> +	}
> +	/* If either of the configuration change, we update both
> +	 * anyway.
> +	 */
> +	if (l1 != l1_cache || l2 != l2_cache) {
> +		rzn1_hw_set_lock(ipctl, use_locks, LOCK_ALL);
> +		writel(l1, &ipctl->lev1->conf[p.pin]);
> +		writel(l2, &ipctl->lev2->conf[p.pin]);
> +		rzn1_hw_set_lock(ipctl, use_locks, 0);
> +	}

Be consistent please. I suggest empty line before returning, as you're
doing in other functions.

> +	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;
> +}
> +
> +static void rzn1_pin_dbg_show(
> +	struct pinctrl_dev *pctldev,
> +	struct seq_file *s,
> +	unsigned int offset)
> +{
> +	seq_printf(s, "%s", dev_name(pctldev->dev));
> +}
> +
> +static int rzn1_dt_node_to_map(
> +	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;
> +	struct pinctrl_map *new_map;
> +	struct device_node *parent;
> +	int map_num = 2;
> +
> +	/*
> +	 * first find the group of this node and check if we need create
> +	 * config maps for pins
> +	 */

I see different multi-line comment styles mixed in this driver. It's
either

/*
 * line1...
 * line2..
 */

Or

/* line1..
 * line2..
 */

Also, comments usually start with a capital letter and ends with full
stop...

> +	grp = rzn1_pinctrl_find_group_by_name(ipctl, np->name);
> +	if (!grp) {
> +		dev_err(ipctl->dev, "unable to find group for node %s\n",

use %pOF to printout device node names

> +			np->name);
> +		return -EINVAL;
> +	}
> +
> +	new_map = devm_kmalloc_array(ipctl->dev, map_num,
> +			sizeof(struct pinctrl_map), GFP_KERNEL);
> +	if (!new_map)
> +		return -ENOMEM;
> +
> +	*map = new_map;
> +	*num_maps = map_num;
> +
> +	/* create mux map */
> +	parent = of_get_parent(np);
> +	if (!parent)
> +		return -EINVAL;
> +
> +	new_map[0].type = PIN_MAP_TYPE_MUX_GROUP;
> +	new_map[0].data.mux.function = grp->func;
> +	new_map[0].data.mux.group = grp->name;
> +	of_node_put(parent);

What are you using parent for?

> +
> +	new_map[1].type = PIN_MAP_TYPE_CONFIGS_GROUP;
> +	new_map[1].data.configs.group_or_pin = grp->name;
> +	new_map[1].data.configs.configs = (unsigned long *)grp->pin_ids;
> +	new_map[1].data.configs.num_configs = grp->npins;
> +
> +	dev_dbg(pctldev->dev, "maps: function %s group %s (%d pins)\n",
> +		(*map)->data.mux.function, (*map)->data.mux.group,
> +		grp->npins);
> +
> +	return 0;
> +}
> +
> +static void rzn1_dt_free_map(struct pinctrl_dev *pctldev,
> +				struct pinctrl_map *map, unsigned int num_maps)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	devm_kfree(ipctl->dev, map);
> +}
> +
> +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,
> +	.pin_dbg_show = rzn1_pin_dbg_show,
> +	.dt_node_to_map = rzn1_dt_node_to_map,
> +	.dt_free_map = rzn1_dt_free_map,
> +};
> +
> +static int rzn1_pmx_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;
> +
> +	/*
> +	 * Configure the mux mode for each pin in the group for a specific
> +	 * function.
> +	 */
> +	grp = &ipctl->groups[group];
> +
> +	dev_dbg(ipctl->dev, "enable function %s(%d) group %s(%d)\n",
> +		ipctl->functions[selector].name, selector, grp->name, group);
> +	/*
> +	 * Well, theres not much to do here anyway, as the individual pin
> +	 * callback is going to be called anyway
> +	 */
> +	return 0;
> +}
> +
> +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 * 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 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_pmx_set_mux,
> +};
> +
> +static int rzn1_pinconf_get(struct pinctrl_dev *pctldev,
> +			     unsigned int pin_id, unsigned long *config)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	pin_id &= 0xff;
> +	if (pin_id >= ARRAY_SIZE(ipctl->lev1->conf))
> +		return -EINVAL;
> +	*config = readl(&ipctl->lev1->conf[pin_id]) & 0xf;
> +	if (*config == 0xf)
> +		*config = (readl(&ipctl->lev2->conf[pin_id]) & 0x3f) +
> +				RZN1_FUNC_LEVEL2_OFFSET;
> +	*config = (*config << RZN1_MUX_FUNC_BIT) | pin_id;
> +	return 0;
> +}
> +
> +static int rzn1_pinconf_set(struct pinctrl_dev *pctldev,
> +			     unsigned int pin_id, unsigned long *configs,
> +			     unsigned int num_configs)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +	int i;
> +
> +	dev_dbg(ipctl->dev, "pinconf set pin %s (%d configs)\n",
> +		rzn1_pins[pin_id].name, num_configs);
> +
> +	for (i = 0; i < num_configs; i++)
> +		rzn1_set_hw_pin_parameters(ipctl, configs[i], LOCK_ALL);
> +
> +	return 0;
> +}
> +
> +

2 empty lines. Again, checkpatch helps with trivial mistakes..

> +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 i;

Empty line after variable declaration.

> +	/*
> +	 * Configure the mux mode for each pin in the group for a specific
> +	 * function.
> +	 */
> +	dev_dbg(ipctl->dev, "group set %s selector:%d configs:%p/%d\n",
> +		grp->name, selector, configs, num_configs);
> +
> +	rzn1_hw_set_lock(ipctl, LOCK_ALL, LOCK_ALL);
> +	for (i = 0; i < num_configs; i++)
> +		rzn1_set_hw_pin_parameters(ipctl, configs[i], 0);
> +	rzn1_hw_set_lock(ipctl, LOCK_ALL, 0);
> +
> +	return 0;
> +}
> +
> +static void rzn1_pinconf_dbg_show(struct pinctrl_dev *pctldev,
> +				   struct seq_file *s, unsigned int pin_id)
> +{
> +	unsigned long config = pin_id;
> +
> +	seq_printf(s, "0x%lx", config);
> +}
> +
> +static void rzn1_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
> +					 struct seq_file *s, unsigned int group)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +	struct rzn1_pin_group *grp;
> +	unsigned long config;
> +	const char *name;
> +	int i, ret;
> +
> +	if (group > ipctl->ngroups)
> +		return;
> +
> +	seq_puts(s, "\n");
> +	grp = &ipctl->groups[group];
> +	for (i = 0; i < grp->npins; i++) {
> +		name = pin_get_name(pctldev, grp->pin_ids[i] & 0xff);
> +		ret = rzn1_pinconf_get(pctldev, grp->pin_ids[i], &config);
> +		if (ret)
> +			return;
> +		seq_printf(s, "%s: 0x%lx", name, config);
> +	}
> +}
> +
> +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,
> +	.pin_config_dbg_show = rzn1_pinconf_dbg_show,
> +	.pin_config_group_dbg_show = rzn1_pinconf_group_dbg_show,
> +};
> +
> +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
> +	 *	renesas,rzn1-pinmux-ids = <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;
> +	}
> +
> +	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;
> +
> +	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 (of_property_count_u32_elems(np, RZN1_PINS_PROP) > 0) {
> +		func->groups[i] = np->name;
> +		grp = &ipctl->groups[ipctl->ngroups];
> +		grp->func = func->name;
> +		if (rzn1_pinctrl_parse_groups(np, grp, ipctl) > 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;
> +		if (rzn1_pinctrl_parse_groups(child, grp, ipctl) > 0) {
> +			i++;
> +			ipctl->ngroups++;
> +		}
> +	}
> +	dev_dbg(ipctl->dev, "function %s parsed %d/%d groups\n",
> +		np->name, i, func->num_groups);
> +
> +	return 0;
> +}
> +
> +static ssize_t _rzn1_pinctrl_sysfs_force_mux(struct device *dev,
> +		struct device_attribute *attr,
> +		const char *buf, size_t len)
> +{
> +	struct rzn1_pinctrl *ipctl = dev_get_drvdata(dev);
> +	unsigned long int val;
> +
> +	if (kstrtoul(buf, 16, &val)) {
> +		dev_err(ipctl->dev, "Invalid hex value %08x", (u32)val);
> +		return len;
> +	}
> +
> +	dev_info(ipctl->dev, "setting pin to %08x\n", (u32)val);
> +
> +	rzn1_set_hw_pin_parameters(ipctl, val, LOCK_ALL);
> +
> +	return len;
> +}
> +
> +static DEVICE_ATTR(force_mux, 0200, NULL, _rzn1_pinctrl_sysfs_force_mux);
> +
> +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;
> +	u32 nfuncs = 0;
> +	u32 i = 0;
> +
> +	if (!np)
> +		return -ENODEV;

Driver depends on OF and is probing, can this happen?

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

Please do not break lines for no reason.

> +				&pdev->dev,
> +				nfuncs, sizeof(struct rzn1_pmx_func),
> +				GFP_KERNEL);
> +	if (!ipctl->functions)
> +		return -ENOMEM;
> +
> +	ipctl->ngroups = 0;
> +	ipctl->maxgroups = 0;
> +	for_each_child_of_node(np, child)
> +		ipctl->maxgroups += rzn1_pinctrl_count_function_groups(child);
> +	ipctl->groups = devm_kmalloc_array(
> +				&pdev->dev,
> +				ipctl->maxgroups, sizeof(*ipctl->groups),
> +				GFP_KERNEL);
> +	if (!ipctl->groups)
> +		return -ENOMEM;
> +
> +	for_each_child_of_node(np, child)
> +		rzn1_pinctrl_parse_functions(child, ipctl, i++);
> +
> +	if (device_create_file(&pdev->dev, &dev_attr_force_mux))
> +		dev_warn(&pdev->dev, "cannot create status attribute\n");
> +
> +	return 0;
> +}
> +
> +static int rzn1_pinctrl_probe(struct platform_device *pdev)
> +{
> +	struct rzn1_pinctrl *ipctl;
> +	struct resource *res;
> +	struct clk *clk;
> +	int ret;
> +
> +	/* Create state holders etc for this driver */
> +	ipctl = devm_kzalloc(&pdev->dev, sizeof(*ipctl), GFP_KERNEL);
> +	if (!ipctl)
> +		return -ENOMEM;
> +
> +	clk = devm_clk_get(&pdev->dev, "bus");
> +	if (IS_ERR(clk) || clk_prepare_enable(clk))
> +		dev_info(&pdev->dev, "no clock source\n");
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	ipctl->lev1_phys = (u32) res->start;
> +	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_phys = (u32) res->start;
> +	ipctl->lev2 = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(ipctl->lev2))
> +		return PTR_ERR(ipctl->lev2);
> +
> +	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");
> +		return ret;
> +	}
> +
> +	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");
> +		return -EINVAL;
> +	}
> +	pinctrl = ipctl;
> +	dev_info(&pdev->dev, "initialized rzn1 pinctrl driver\n");
> +	return 0;
> +}
> +
> +static int rzn1_pinctrl_remove(struct platform_device *pdev)
> +{
> +	device_remove_file(&pdev->dev, &dev_attr_force_mux);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id rzn1_pinctrl_match[] = {
> +	{ .compatible = "renesas,r9a06g032-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		= "r9a06g032-pinctrl",
> +		.owner		= THIS_MODULE,
> +		.of_match_table	= rzn1_pinctrl_match,
> +	},
> +};
> +
> +static int __init _pinctrl_drv_register(void)
> +{
> +	return platform_driver_register(&rzn1_pinctrl_driver);
> +}
> +postcore_initcall(_pinctrl_drv_register);
> +
> +
> +MODULE_AUTHOR("Michel Pollet <Michel.Pollet@bp.renesas.com>");
> +MODULE_DESCRIPTION("Renesas R9A06G032 pinctrl driver");
> +MODULE_LICENSE("GPL");

The SPDX header says GPL-v2 and this one GPL only.

As said, this is all minor stuff. Let's get back to the meat of this
driver once bindings are finalized.

Thanks
   j

> --
> 2.7.4
>

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

[PATCH v1 4/5] ARM: dts: Renesas R9A06G032 pinctrl node

[Michel Pollet]

This provides a pinctrl driver for the Renesas R9A06G032 SoC

Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
---
 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 3e45375..fbad039 100644
--- a/arch/arm/boot/dts/r9a06g032.dtsi
+++ b/arch/arm/boot/dts/r9a06g032.dtsi
@@ -88,6 +88,14 @@
 			status = "disabled";
 		};
 
+		pinctrl: pinctrl@40067000 {
+			compatible = "renesas,r9a06g032-pinctrl";
+			reg = <0x40067000 0x1000>, <0x51000000 0x800>;
+			clocks = <&sysctrl R9A06G032_HCLK_PINCONFIG>;
+			clock-names = "bus";
+			status = "okay";
+		};
+
 		gic: gic@44101000 {
 			compatible = "arm,cortex-a7-gic", "arm,gic-400";
 			interrupt-controller;
-- 
2.7.4

Re: [PATCH v1 4/5] ARM: dts: Renesas R9A06G032 pinctrl node

[Sergei Shtylyov]

Hello!

On 06/14/2018 02:00 PM, Michel Pollet wrote:

> This provides a pinctrl driver for the Renesas R9A06G032 SoC
> 
> Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
> ---
>  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 3e45375..fbad039 100644
> --- a/arch/arm/boot/dts/r9a06g032.dtsi
> +++ b/arch/arm/boot/dts/r9a06g032.dtsi
> @@ -88,6 +88,14 @@
>  			status = "disabled";
>  		};
>  
> +		pinctrl: pinctrl@40067000 {

   "pin-controller@..." please, it's looking more generic.

> +			compatible = "renesas,r9a06g032-pinctrl";
> +			reg = <0x40067000 0x1000>, <0x51000000 0x800>;
> +			clocks = <&sysctrl R9A06G032_HCLK_PINCONFIG>;
> +			clock-names = "bus";
> +			status = "okay";
> +		};
> +
>  		gic: gic@44101000 {
>  			compatible = "arm,cortex-a7-gic", "arm,gic-400";
>  			interrupt-controller;

MBR, Sergei

[PATCH v1 5/5] ARM: dts: Renesas RZN1D-DB Board: Add UART0 pinmux node

[Michel Pollet]

This adds the necessary nodes to add pin configuration for the UART0
of that board.

Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
---
 arch/arm/boot/dts/r9a06g032-rzn1d400-db.dts | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm/boot/dts/r9a06g032-rzn1d400-db.dts b/arch/arm/boot/dts/r9a06g032-rzn1d400-db.dts
index 4e57ae2..039ec2e 100644
--- a/arch/arm/boot/dts/r9a06g032-rzn1d400-db.dts
+++ b/arch/arm/boot/dts/r9a06g032-rzn1d400-db.dts
@@ -8,6 +8,8 @@
 
 /dts-v1/;
 
+#include <dt-bindings/pinctrl/r9a06g032-pinctrl.h>
+
 #include "r9a06g032.dtsi"
 
 / {
@@ -23,6 +25,17 @@
 	};
 };
 
+&pinctrl {
+	pinsuart0: pinsuart0 {
+		renesas,rzn1-pinmux-ids = <
+			RZN1_MUX(103, UART0_I)	/* UART0_TXD */
+			RZN1_MUX(104, UART0_I)	/* UART0_RXD */
+		>;
+	};
+};
+
 &uart0 {
 	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinsuart0>;
 };
-- 
2.7.4

Re: [PATCH v1 0/5] Renesas R9A06G032 PINCTRL Driver

[Linus Walleij]

On Thu, Jun 14, 2018 at 1:00 PM, Michel Pollet
<michel.pollet@bp.renesas.com> wrote:

> *WARNING* -- this requires:
> + R9A06G032 base patch v9
> + R9A06G032 SMP patch v5

Is that required for the pin controller itself (compile-time dependence)
or is it required to boot the system (run-time dependence)?

We can merge support for pin control ahead, that's fine.

> This implements the pinctrl driver for the R9A06G032.

Geert Uytterhoeven and Laurent Pinchart maintains the Renesas pin
controllers, and this one is for some reason a totally new one in
drivers/pinctrl/pinctrl-r9a06g032.c

Is it totally different from the other "great old ones" in the SuperH-PFC
series or is there some other reason why it was done like this?

Please include Geert and Laurent on subsequent postings,
their review is pretty much required to move forward with
this.

Yours,
Linus Walleij

Re: [PATCH v1 0/5] Renesas R9A06G032 PINCTRL Driver

[Geert Uytterhoeven]

Hi Linus,

On Mon, Jun 18, 2018 at 10:46 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Jun 14, 2018 at 1:00 PM, Michel Pollet
> <michel.pollet@bp.renesas.com> wrote:
>
> > *WARNING* -- this requires:
> > + R9A06G032 base patch v9
> > + R9A06G032 SMP patch v5
>
> Is that required for the pin controller itself (compile-time dependence)
> or is it required to boot the system (run-time dependence)?

Obviously the last 3 patches in the series touch the DTS, so they depend
on the base patch adding the DTS files.

> We can merge support for pin control ahead, that's fine.

The actual pinctrl driver can indeed be merged separately.

> > This implements the pinctrl driver for the R9A06G032.
>
> Geert Uytterhoeven and Laurent Pinchart maintains the Renesas pin
> controllers, and this one is for some reason a totally new one in
> drivers/pinctrl/pinctrl-r9a06g032.c
>
> Is it totally different from the other "great old ones" in the SuperH-PFC
> series or is there some other reason why it was done like this?

Yes it is, cfr. drivers/pinctrl/pinctrl-rza1.c, which lives outside sh-pfc, too.

Of course I can take it through my sh-pfc tree, once the rough edges have been
removed.

Thanks!

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 v1 0/5] Renesas R9A06G032 PINCTRL Driver

[Linus Walleij]

On Mon, Jun 18, 2018 at 10:57 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Mon, Jun 18, 2018 at 10:46 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Thu, Jun 14, 2018 at 1:00 PM, Michel Pollet
> > <michel.pollet@bp.renesas.com> wrote:

> > Is it totally different from the other "great old ones" in the SuperH-PFC
> > series or is there some other reason why it was done like this?
>
> Yes it is, cfr. drivers/pinctrl/pinctrl-rza1.c, which lives outside sh-pfc, too.

OK I see.

I take it that it is also totally different from the rza1 HW?

It's fine like this, if there are many of them we might want
to create something like a renesas/ subfolder at some point
and stash them in there.

> Of course I can take it through my sh-pfc tree, once the rough edges have been
> removed.

That's best I think, it always works smooth.

Yours,
Linus Walleij