[PATCH 00/04] ARM: shmobile: r7s72100 and Genmai GPIO / PINCTRL support

[PATCH 00/04] ARM: shmobile: r7s72100 and Genmai GPIO / PINCTRL support

[Magnus Damm]

ARM: shmobile: r7s72100 and Genmai GPIO / PINCTRL support

[PATCH 01/04] ARM: shmobile: r7s72100 GPIO and PINCTRL device nodes
[PATCH 02/04] ARM: shmobile: Genmai LED1 and LED2 support
[PATCH 03/04] ARM: shmobile: Genmai I2C-over-GPIO support
[PATCH 04/04] ARM: shmobile: Genmai SCIF2 PINCTRL configuration

Add various device nodes to the r7s72100 SoC and the Genmai board,
nothing special just basic support for LEDs and I2C-over-GPIO.

Patch 4/4 depends on the r7s72100 PINCTRL pin grouping, but 1-3
depends on the GPIO driver.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 Written against renesas.git tag renesas-devel-v3.13-rc1-20131125 and
 [PATCH] pinctrl: sh-pfc: Initial r7s72100 support
 [PATCH v3] gpio: Renesas RZ GPIO driver V3 

 The following patches are needed for 4/4:
 [PATCH 00/03] pinctrl: sh-pfc: r7s72100 SCIF2 support
 [PATCH 01/03] pinctrl: sh-pfc: r7s72100 SCIF2 port3 support
 [PATCH 02/03] pinctrl: sh-pfc: r7s72100 SCIF2 macro conversion
 [PATCH 03/03] pinctrl: sh-pfc: r7s72100 SCIF2 port4, 6 and 8 support

 arch/arm/boot/dts/r7s72100-genmai-reference.dts |   41 +++++-
 arch/arm/boot/dts/r7s72100.dtsi                 |  154 +++++++++++++++++++++++
 2 files changed, 194 insertions(+), 1 deletion(-)

[PATCH 01/04] ARM: shmobile: r7s72100 GPIO and PINCTRL device nodes

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

Add support for r7s72100 PFC and GPIO device nodes port0 -> port11
and jtagport0.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 arch/arm/boot/dts/r7s72100.dtsi |  154 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 154 insertions(+)

--- 0001/arch/arm/boot/dts/r7s72100.dtsi
+++ work/arch/arm/boot/dts/r7s72100.dtsi	2013-11-27 16:06:36.000000000 +0900
@@ -14,6 +14,22 @@
 	#address-cells = <1>;
 	#size-cells = <1>;
 
+	aliases {
+		gpio0 = &port0;
+		gpio1 = &port1;
+		gpio2 = &port2;
+		gpio3 = &port3;
+		gpio4 = &port4;
+		gpio5 = &port5;
+		gpio6 = &port6;
+		gpio7 = &port7;
+		gpio8 = &port8;
+		gpio9 = &port9;
+		gpio10 = &port10;
+		gpio11 = &port11;
+		gpio12 = &jtagport0;
+	};
+
 	cpus {
 		#address-cells = <1>;
 		#size-cells = <0>;
@@ -33,4 +49,142 @@
 		reg = <0xe8201000 0x1000>,
 			<0xe8202000 0x1000>;
 	};
+
+	pfc: pfc at fcfe3300 {
+		compatible = "renesas,pfc-r7s72100";
+		reg = <0xfcfe3300 0x400>, /* PM, PMC, PFC, PFCE */
+		  	  <0xfcfe3a00 0x100>, /* PFCAE */
+			  <0xfcfe7000 0x300>, /* PIBC, PBDC, PIPC */
+			  <0xfcfe7b40 0x04>, /* JPMC */
+			  <0xfcfe7b90 0x04>, /* JPMCSR */
+			  <0xfcfe7f00 0x04>; /* JPIBC */
+	};
+
+	port0: gpio at fcfe3100 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3100 0x4>, /* PSR */
+		  	  <0xfcfe3200 0x2>, /* PPR */
+			  <0xfcfe3800 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 0 6>;
+	};
+
+	port1: gpio at fcfe3104 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3104 0x4>, /* PSR */
+		  	  <0xfcfe3204 0x2>, /* PPR */
+			  <0xfcfe3804 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 16 16>;
+	};
+
+	port2: gpio at fcfe3108 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3108 0x4>, /* PSR */
+		  	  <0xfcfe3208 0x2>, /* PPR */
+			  <0xfcfe3808 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 32 16>;
+	};
+
+	port3: gpio at fcfe310c {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe310c 0x4>, /* PSR */
+		  	  <0xfcfe320c 0x2>, /* PPR */
+			  <0xfcfe380c 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 48 16>;
+	};
+
+	port4: gpio at fcfe3110 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3110 0x4>, /* PSR */
+		  	  <0xfcfe3210 0x2>, /* PPR */
+			  <0xfcfe3810 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 64 16>;
+	};
+
+	port5: gpio at fcfe3114 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3114 0x4>, /* PSR */
+		  	  <0xfcfe3214 0x2>, /* PPR */
+			  <0xfcfe3814 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 80 11>;
+	};
+
+	port6: gpio at fcfe3118 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3118 0x4>, /* PSR */
+		  	  <0xfcfe3218 0x2>, /* PPR */
+			  <0xfcfe3818 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 96 16>;
+	};
+
+	port7: gpio at fcfe311c {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe311c 0x4>, /* PSR */
+		  	  <0xfcfe321c 0x2>, /* PPR */
+			  <0xfcfe381c 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 112 16>;
+	};
+
+	port8: gpio at fcfe3120 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3120 0x4>, /* PSR */
+		  	  <0xfcfe3220 0x2>, /* PPR */
+			  <0xfcfe3820 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 128 16>;
+	};
+
+	port9: gpio at fcfe3124 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3124 0x4>, /* PSR */
+		  	  <0xfcfe3224 0x2>, /* PPR */
+			  <0xfcfe3824 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 144 8>;
+	};
+
+	port10: gpio at fcfe3128 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3128 0x4>, /* PSR */
+		  	  <0xfcfe3228 0x2>, /* PPR */
+			  <0xfcfe3828 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 160 16>;
+	};
+
+	port11: gpio at fcfe312c {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe312c 0x4>, /* PSR */
+		  	  <0xfcfe322c 0x2>, /* PPR */
+			  <0xfcfe382c 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 176 16>;
+	};
+
+	jtagport0: gpio at fcfe7b20 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe7b20 0x2>; /* JPPR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 192 2>;
+	};
 };

[PATCH 02/04] ARM: shmobile: Genmai LED1 and LED2 support

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

Add support for Genmai board LED1 and LED2 via gpio-leds.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 arch/arm/boot/dts/r7s72100-genmai-reference.dts |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

--- 0001/arch/arm/boot/dts/r7s72100-genmai-reference.dts
+++ work/arch/arm/boot/dts/r7s72100-genmai-reference.dts	2013-11-27 15:49:27.000000000 +0900
@@ -9,7 +9,8 @@
  */
 
 /dts-v1/;
-/include/ "r7s72100.dtsi"
+#include "r7s72100.dtsi"
+#include <dt-bindings/gpio/gpio.h>
 
 / {
 	model = "Genmai";
@@ -28,4 +29,14 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 	};
+
+	leds {
+		compatible = "gpio-leds";
+		led1 {
+			gpios = <&port4 10 GPIO_ACTIVE_LOW>;
+		};
+		led2 {
+			gpios = <&port4 11 GPIO_ACTIVE_LOW>;
+		};
+	};
 };

[PATCH 03/04] ARM: shmobile: Genmai I2C-over-GPIO support

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

Add support for the Genmai I2C bus hooked up to P1_5 and P1_4 using
the i2c-gpio driver. On the bus sits a 24c128 EEPRROM.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 arch/arm/boot/dts/r7s72100-genmai-reference.dts |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

--- 0008/arch/arm/boot/dts/r7s72100-genmai-reference.dts
+++ work/arch/arm/boot/dts/r7s72100-genmai-reference.dts	2013-11-27 15:51:31.000000000 +0900
@@ -39,4 +39,22 @@
 			gpios = <&port4 11 GPIO_ACTIVE_LOW>;
 		};
 	};
+
+	i2c at 0 {
+		compatible = "i2c-gpio";
+		gpios = <&port1 5 GPIO_ACTIVE_HIGH /* sda */
+			 &port1 4 GPIO_ACTIVE_HIGH /* scl */
+			>;
+		i2c-gpio,sda-open-drain;
+		i2c-gpio,scl-open-drain;
+		i2c-gpio,delay-us = <5>;	/* ~100 kHz */
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		eeprom: 24c128 at 50 {
+			compatible = "at,24c128";
+			reg = <0x50>;
+		};
+	};
+
 };

[PATCH 04/04] ARM: shmobile: Genmai SCIF2 PINCTRL configuration

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

Configure the r7s72100 PINCTRL hardware and select pin function
for the SCIF2 serial console.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 arch/arm/boot/dts/r7s72100-genmai-reference.dts |   10 ++++++++++
 1 file changed, 10 insertions(+)

--- 0009/arch/arm/boot/dts/r7s72100-genmai-reference.dts
+++ work/arch/arm/boot/dts/r7s72100-genmai-reference.dts	2013-11-27 17:13:43.000000000 +0900
@@ -58,3 +58,13 @@
 	};
 
 };
+
+&pfc {
+	pinctrl-0 = <&scif2_pins>;
+	pinctrl-names = "default";
+
+	scif2_pins: serial2 {
+		renesas,groups = "scif2_txd_p3_0", "scif2_rxd_p3_2";
+		renesas,function = "scif2";
+	};
+};

[PATCH 03/04] ARM: shmobile: Genmai I2C-over-GPIO support

[Wolfram Sang]

> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		eeprom: 24c128 at 50 {
> +			compatible = "at,24c128";

Minor nit: Should be a vendor name, not the name of the driver

> +			reg = <0x50>;
> +		};
> +	};
> +
>  };
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131127/0fa420d3/attachment.sig>

[PATCH 00/04] ARM: shmobile: r7s72100 and Genmai GPIO / PINCTRL support

[Laurent Pinchart]

Hi Magnus,

Thank you for the patches.

Looking at how the PFC and GPIO registers are interleaved I'll really need the 
datasheet to properly review this and the previous PFC and GPIO patches.

On Wednesday 27 November 2013 17:27:36 Magnus Damm wrote:
> ARM: shmobile: r7s72100 and Genmai GPIO / PINCTRL support
> 
> [PATCH 01/04] ARM: shmobile: r7s72100 GPIO and PINCTRL device nodes
> [PATCH 02/04] ARM: shmobile: Genmai LED1 and LED2 support
> [PATCH 03/04] ARM: shmobile: Genmai I2C-over-GPIO support
> [PATCH 04/04] ARM: shmobile: Genmai SCIF2 PINCTRL configuration
> 
> Add various device nodes to the r7s72100 SoC and the Genmai board,
> nothing special just basic support for LEDs and I2C-over-GPIO.
> 
> Patch 4/4 depends on the r7s72100 PINCTRL pin grouping, but 1-3
> depends on the GPIO driver.
> 
> Signed-off-by: Magnus Damm <damm@opensource.se>
> ---
> 
>  Written against renesas.git tag renesas-devel-v3.13-rc1-20131125 and
>  [PATCH] pinctrl: sh-pfc: Initial r7s72100 support
>  [PATCH v3] gpio: Renesas RZ GPIO driver V3
> 
>  The following patches are needed for 4/4:
>  [PATCH 00/03] pinctrl: sh-pfc: r7s72100 SCIF2 support
>  [PATCH 01/03] pinctrl: sh-pfc: r7s72100 SCIF2 port3 support
>  [PATCH 02/03] pinctrl: sh-pfc: r7s72100 SCIF2 macro conversion
>  [PATCH 03/03] pinctrl: sh-pfc: r7s72100 SCIF2 port4, 6 and 8 support
> 
>  arch/arm/boot/dts/r7s72100-genmai-reference.dts |   41 +++++-
>  arch/arm/boot/dts/r7s72100.dtsi                 |  154
> +++++++++++++++++++++++ 2 files changed, 194 insertions(+), 1 deletion(-)
-- 
Regards,

Laurent Pinchart

[PATCH 03/04] ARM: shmobile: Genmai I2C-over-GPIO support

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

On Wednesday 27 November 2013 17:28:05 Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> Add support for the Genmai I2C bus hooked up to P1_5 and P1_4 using
> the i2c-gpio driver. On the bus sits a 24c128 EEPRROM.

Is this a temporary workaround until we get a proper I2C controller driver, or 
is the EEPROM really hooked up to pins that are not wired to a hardware I2C 
controller ?

> Signed-off-by: Magnus Damm <damm@opensource.se>
> ---
> 
>  arch/arm/boot/dts/r7s72100-genmai-reference.dts |   18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> --- 0008/arch/arm/boot/dts/r7s72100-genmai-reference.dts
> +++ work/arch/arm/boot/dts/r7s72100-genmai-reference.dts	2013-11-27
> 15:51:31.000000000 +0900 @@ -39,4 +39,22 @@
>  			gpios = <&port4 11 GPIO_ACTIVE_LOW>;
>  		};
>  	};
> +
> +	i2c at 0 {
> +		compatible = "i2c-gpio";
> +		gpios = <&port1 5 GPIO_ACTIVE_HIGH /* sda */
> +			 &port1 4 GPIO_ACTIVE_HIGH /* scl */
> +			>;
> +		i2c-gpio,sda-open-drain;
> +		i2c-gpio,scl-open-drain;
> +		i2c-gpio,delay-us = <5>;	/* ~100 kHz */
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		eeprom: 24c128 at 50 {
> +			compatible = "at,24c128";
> +			reg = <0x50>;
> +		};
> +	};
> +
>  };
-- 
Regards,

Laurent Pinchart

[PATCH 04/04] ARM: shmobile: Genmai SCIF2 PINCTRL configuration

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

On Wednesday 27 November 2013 17:28:14 Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> Configure the r7s72100 PINCTRL hardware and select pin function
> for the SCIF2 serial console.
> 
> Signed-off-by: Magnus Damm <damm@opensource.se>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
>  arch/arm/boot/dts/r7s72100-genmai-reference.dts |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> --- 0009/arch/arm/boot/dts/r7s72100-genmai-reference.dts
> +++ work/arch/arm/boot/dts/r7s72100-genmai-reference.dts	2013-11-27
> 17:13:43.000000000 +0900 @@ -58,3 +58,13 @@
>  	};
> 
>  };
> +
> +&pfc {
> +	pinctrl-0 = <&scif2_pins>;
> +	pinctrl-names = "default";
> +
> +	scif2_pins: serial2 {
> +		renesas,groups = "scif2_txd_p3_0", "scif2_rxd_p3_2";
> +		renesas,function = "scif2";
> +	};
> +};
-- 
Regards,

Laurent Pinchart

[PATCH 02/04] ARM: shmobile: Genmai LED1 and LED2 support

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

On Wednesday 27 November 2013 17:27:55 Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> Add support for Genmai board LED1 and LED2 via gpio-leds.
> 
> Signed-off-by: Magnus Damm <damm@opensource.se>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
>  arch/arm/boot/dts/r7s72100-genmai-reference.dts |   13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> --- 0001/arch/arm/boot/dts/r7s72100-genmai-reference.dts
> +++ work/arch/arm/boot/dts/r7s72100-genmai-reference.dts	2013-11-27
> 15:49:27.000000000 +0900 @@ -9,7 +9,8 @@
>   */
> 
>  /dts-v1/;
> -/include/ "r7s72100.dtsi"
> +#include "r7s72100.dtsi"
> +#include <dt-bindings/gpio/gpio.h>
> 
>  / {
>  	model = "Genmai";
> @@ -28,4 +29,14 @@
>  		#address-cells = <1>;
>  		#size-cells = <1>;
>  	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +		led1 {
> +			gpios = <&port4 10 GPIO_ACTIVE_LOW>;
> +		};
> +		led2 {
> +			gpios = <&port4 11 GPIO_ACTIVE_LOW>;
> +		};
> +	};
>  };
-- 
Regards,

Laurent Pinchart

[PATCH 03/04] ARM: shmobile: Genmai I2C-over-GPIO support

[Magnus Damm]

Hi Laurent,

On Wed, Nov 27, 2013 at 8:15 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> Thank you for the patch.
>
> On Wednesday 27 November 2013 17:28:05 Magnus Damm wrote:
>> From: Magnus Damm <damm@opensource.se>
>>
>> Add support for the Genmai I2C bus hooked up to P1_5 and P1_4 using
>> the i2c-gpio driver. On the bus sits a 24c128 EEPRROM.
>
> Is this a temporary workaround until we get a proper I2C controller driver, or
> is the EEPROM really hooked up to pins that are not wired to a hardware I2C
> controller ?

This is a stop-gap solution until we have upstream driver support for
the I2C master hardware. The pins used for the I2C bus can be
configured in GPIO mode or a zillion other pin functions including SCL
and SDA.

Cheers,

/ magnus

[PATCH 03/04] ARM: shmobile: Genmai I2C-over-GPIO support

[Laurent Pinchart]

Hi Magnus,

On Thursday 28 November 2013 09:44:31 Magnus Damm wrote:
> On Wed, Nov 27, 2013 at 8:15 PM, Laurent Pinchart wrote:
> > On Wednesday 27 November 2013 17:28:05 Magnus Damm wrote:
> >> From: Magnus Damm <damm@opensource.se>
> >> 
> >> Add support for the Genmai I2C bus hooked up to P1_5 and P1_4 using
> >> the i2c-gpio driver. On the bus sits a 24c128 EEPRROM.
> > 
> > Is this a temporary workaround until we get a proper I2C controller
> > driver, or is the EEPROM really hooked up to pins that are not wired to a
> > hardware I2C controller ?
> 
> This is a stop-gap solution until we have upstream driver support for the
> I2C master hardware.

OK, no problem. Is there a timeline for that ?

> The pins used for the I2C bus can be configured in GPIO mode or a zillion
> other pin functions including SCL and SDA.

-- 
Regards,

Laurent Pinchart

[PATCH 03/04] ARM: shmobile: Genmai I2C-over-GPIO support

[Sergei Shtylyov]

Hello.

On 27-11-2013 15:04, Wolfram Sang wrote:

>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		eeprom: 24c128 at 50 {
>> +			compatible = "at,24c128";

> Minor nit: Should be a vendor name, not the name of the driver

    Moreover, the node should be named generically, like "flash at 50", not 
"24c128 at 50".

WBR, Sergei

[PATCH 03/04] ARM: shmobile: Genmai I2C-over-GPIO support

[Laurent Pinchart]

On Thursday 28 November 2013 21:11:48 Sergei Shtylyov wrote:
> On 27-11-2013 15:04, Wolfram Sang wrote:
> >> +		#address-cells = <1>;
> >> +		#size-cells = <0>;
> >> +
> >> +		eeprom: 24c128 at 50 {
> >> +			compatible = "at,24c128";
> > 
> > Minor nit: Should be a vendor name, not the name of the driver
> 
> Moreover, the node should be named generically, like "flash at 50", not
> "24c128 at 50".

Or, given that it's an eeprom, "eeprom at 50" ? :-)

-- 
Regards,

Laurent Pinchart

[PATCH 03/04] ARM: shmobile: Genmai I2C-over-GPIO support

[Sergei Shtylyov]

Hello.

On 28-11-2013 21:14, Laurent Pinchart wrote:

>>>> +		#address-cells = <1>;
>>>> +		#size-cells = <0>;
>>>> +
>>>> +		eeprom: 24c128 at 50 {
>>>> +			compatible = "at,24c128";

>>> Minor nit: Should be a vendor name, not the name of the driver

>> Moreover, the node should be named generically, like "flash at 50", not
>> "24c128 at 50".

> Or, given that it's an eeprom, "eeprom at 50" ? :-)

    At least ePAPR spec. only has "flash".

WBR, Sergei

[PATCH 01/04] ARM: shmobile: r7s72100 GPIO and PINCTRL device nodes

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

Add support for r7s72100 PFC and GPIO device nodes port0 -> port11
and jtagport0.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 arch/arm/boot/dts/r7s72100.dtsi |  154 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 154 insertions(+)

--- 0001/arch/arm/boot/dts/r7s72100.dtsi
+++ work/arch/arm/boot/dts/r7s72100.dtsi	2013-11-27 16:06:36.000000000 +0900
@@ -14,6 +14,22 @@
 	#address-cells = <1>;
 	#size-cells = <1>;
 
+	aliases {
+		gpio0 = &port0;
+		gpio1 = &port1;
+		gpio2 = &port2;
+		gpio3 = &port3;
+		gpio4 = &port4;
+		gpio5 = &port5;
+		gpio6 = &port6;
+		gpio7 = &port7;
+		gpio8 = &port8;
+		gpio9 = &port9;
+		gpio10 = &port10;
+		gpio11 = &port11;
+		gpio12 = &jtagport0;
+	};
+
 	cpus {
 		#address-cells = <1>;
 		#size-cells = <0>;
@@ -33,4 +49,142 @@
 		reg = <0xe8201000 0x1000>,
 			<0xe8202000 0x1000>;
 	};
+
+	pfc: pfc at fcfe3300 {
+		compatible = "renesas,pfc-r7s72100";
+		reg = <0xfcfe3300 0x400>, /* PM, PMC, PFC, PFCE */
+		  	  <0xfcfe3a00 0x100>, /* PFCAE */
+			  <0xfcfe7000 0x300>, /* PIBC, PBDC, PIPC */
+			  <0xfcfe7b40 0x04>, /* JPMC */
+			  <0xfcfe7b90 0x04>, /* JPMCSR */
+			  <0xfcfe7f00 0x04>; /* JPIBC */
+	};
+
+	port0: gpio at fcfe3100 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3100 0x4>, /* PSR */
+		  	  <0xfcfe3200 0x2>, /* PPR */
+			  <0xfcfe3800 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 0 6>;
+	};
+
+	port1: gpio at fcfe3104 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3104 0x4>, /* PSR */
+		  	  <0xfcfe3204 0x2>, /* PPR */
+			  <0xfcfe3804 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 16 16>;
+	};
+
+	port2: gpio at fcfe3108 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3108 0x4>, /* PSR */
+		  	  <0xfcfe3208 0x2>, /* PPR */
+			  <0xfcfe3808 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 32 16>;
+	};
+
+	port3: gpio at fcfe310c {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe310c 0x4>, /* PSR */
+		  	  <0xfcfe320c 0x2>, /* PPR */
+			  <0xfcfe380c 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 48 16>;
+	};
+
+	port4: gpio at fcfe3110 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3110 0x4>, /* PSR */
+		  	  <0xfcfe3210 0x2>, /* PPR */
+			  <0xfcfe3810 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 64 16>;
+	};
+
+	port5: gpio at fcfe3114 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3114 0x4>, /* PSR */
+		  	  <0xfcfe3214 0x2>, /* PPR */
+			  <0xfcfe3814 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 80 11>;
+	};
+
+	port6: gpio at fcfe3118 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3118 0x4>, /* PSR */
+		  	  <0xfcfe3218 0x2>, /* PPR */
+			  <0xfcfe3818 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 96 16>;
+	};
+
+	port7: gpio at fcfe311c {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe311c 0x4>, /* PSR */
+		  	  <0xfcfe321c 0x2>, /* PPR */
+			  <0xfcfe381c 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 112 16>;
+	};
+
+	port8: gpio at fcfe3120 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3120 0x4>, /* PSR */
+		  	  <0xfcfe3220 0x2>, /* PPR */
+			  <0xfcfe3820 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 128 16>;
+	};
+
+	port9: gpio at fcfe3124 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3124 0x4>, /* PSR */
+		  	  <0xfcfe3224 0x2>, /* PPR */
+			  <0xfcfe3824 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 144 8>;
+	};
+
+	port10: gpio at fcfe3128 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3128 0x4>, /* PSR */
+		  	  <0xfcfe3228 0x2>, /* PPR */
+			  <0xfcfe3828 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 160 16>;
+	};
+
+	port11: gpio at fcfe312c {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe312c 0x4>, /* PSR */
+		  	  <0xfcfe322c 0x2>, /* PPR */
+			  <0xfcfe382c 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 176 16>;
+	};
+
+	jtagport0: gpio at fcfe7b20 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe7b20 0x2>; /* JPPR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 192 2>;
+	};
 };

[PATCH 01/04] ARM: shmobile: r7s72100 GPIO and PINCTRL device nodes

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

On Tuesday 17 December 2013 14:02:42 Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> Add support for r7s72100 PFC and GPIO device nodes port0 -> port11
> and jtagport0.
> 
> Signed-off-by: Magnus Damm <damm@opensource.se>
> ---
> 
>  arch/arm/boot/dts/r7s72100.dtsi |  154 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 154 insertions(+)
> 
> --- 0001/arch/arm/boot/dts/r7s72100.dtsi
> +++ work/arch/arm/boot/dts/r7s72100.dtsi	2013-11-27 16:06:36.000000000 
+0900
> @@ -14,6 +14,22 @@
>  	#address-cells = <1>;
>  	#size-cells = <1>;
> 
> +	aliases {
> +		gpio0 = &port0;
> +		gpio1 = &port1;
> +		gpio2 = &port2;
> +		gpio3 = &port3;
> +		gpio4 = &port4;
> +		gpio5 = &port5;
> +		gpio6 = &port6;
> +		gpio7 = &port7;
> +		gpio8 = &port8;
> +		gpio9 = &port9;
> +		gpio10 = &port10;
> +		gpio11 = &port11;
> +		gpio12 = &jtagport0;
> +	};
> +
>  	cpus {
>  		#address-cells = <1>;
>  		#size-cells = <0>;
> @@ -33,4 +49,142 @@
>  		reg = <0xe8201000 0x1000>,
>  			<0xe8202000 0x1000>;
>  	};
> +
> +	pfc: pfc at fcfe3300 {
> +		compatible = "renesas,pfc-r7s72100";
> +		reg = <0xfcfe3300 0x400>, /* PM, PMC, PFC, PFCE */
> +		  	  <0xfcfe3a00 0x100>, /* PFCAE */
> +			  <0xfcfe7000 0x300>, /* PIBC, PBDC, PIPC */
> +			  <0xfcfe7b40 0x04>, /* JPMC */
> +			  <0xfcfe7b90 0x04>, /* JPMCSR */
> +			  <0xfcfe7f00 0x04>; /* JPIBC */
> +	};
> +
> +	port0: gpio at fcfe3100 {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe3100 0x4>, /* PSR */
> +		  	  <0xfcfe3200 0x2>, /* PPR */
> +			  <0xfcfe3800 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 0 6>;
> +	};
> +
> +	port1: gpio at fcfe3104 {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe3104 0x4>, /* PSR */
> +		  	  <0xfcfe3204 0x2>, /* PPR */
> +			  <0xfcfe3804 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 16 16>;

As P0 has 6 pins only this should ideally be 

		gpio-ranges = <&pfc 0 6 16>;

Otherwise the PFC driver will expose pins that don't exist. However, that 
would require computing the pin numbers in the PFC driver differently, as we 
currently just use the bank * 16 + index formula. Given that we only have 
three ports with less than 16 pins we could come up with a not overly complex 
formula that can be evaluated at compile time. Something like this should do.

#define RZ_PORT_PIN(bank, pin) \
	(bank) < 1 ? (pin) : \
	(bank) < 6 ? 6 + (((bank) - 1) * 16) + (pin)) : \
	(bank) < 10 ? 6 + 11 + 4 * 16 + (((bank) - 6) * 16) + (pin)) : \
	6 + 11 + 8 + 7 * 16 + (((bank) - 10) * 16) + (pin))

> +	};
> +
> +	port2: gpio at fcfe3108 {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe3108 0x4>, /* PSR */
> +		  	  <0xfcfe3208 0x2>, /* PPR */
> +			  <0xfcfe3808 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 32 16>;
> +	};
> +
> +	port3: gpio at fcfe310c {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe310c 0x4>, /* PSR */
> +		  	  <0xfcfe320c 0x2>, /* PPR */
> +			  <0xfcfe380c 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 48 16>;
> +	};
> +
> +	port4: gpio at fcfe3110 {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe3110 0x4>, /* PSR */
> +		  	  <0xfcfe3210 0x2>, /* PPR */
> +			  <0xfcfe3810 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 64 16>;
> +	};
> +
> +	port5: gpio at fcfe3114 {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe3114 0x4>, /* PSR */
> +		  	  <0xfcfe3214 0x2>, /* PPR */
> +			  <0xfcfe3814 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 80 11>;
> +	};
> +
> +	port6: gpio at fcfe3118 {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe3118 0x4>, /* PSR */
> +		  	  <0xfcfe3218 0x2>, /* PPR */
> +			  <0xfcfe3818 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 96 16>;
> +	};
> +
> +	port7: gpio at fcfe311c {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe311c 0x4>, /* PSR */
> +		  	  <0xfcfe321c 0x2>, /* PPR */
> +			  <0xfcfe381c 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 112 16>;
> +	};
> +
> +	port8: gpio at fcfe3120 {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe3120 0x4>, /* PSR */
> +		  	  <0xfcfe3220 0x2>, /* PPR */
> +			  <0xfcfe3820 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 128 16>;
> +	};
> +
> +	port9: gpio at fcfe3124 {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe3124 0x4>, /* PSR */
> +		  	  <0xfcfe3224 0x2>, /* PPR */
> +			  <0xfcfe3824 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 144 8>;
> +	};
> +
> +	port10: gpio at fcfe3128 {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe3128 0x4>, /* PSR */
> +		  	  <0xfcfe3228 0x2>, /* PPR */
> +			  <0xfcfe3828 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 160 16>;
> +	};
> +
> +	port11: gpio at fcfe312c {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe312c 0x4>, /* PSR */
> +		  	  <0xfcfe322c 0x2>, /* PPR */
> +			  <0xfcfe382c 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 176 16>;
> +	};
> +
> +	jtagport0: gpio at fcfe7b20 {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe7b20 0x2>; /* JPPR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 192 2>;
> +	};
>  };
-- 
Regards,

Laurent Pinchart

[PATCH 01/04] ARM: shmobile: r7s72100 GPIO and PINCTRL device nodes

[Wolfram Sang]

On Tue, Dec 17, 2013 at 02:02:42PM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> Add support for r7s72100 PFC and GPIO device nodes port0 -> port11
> and jtagport0.
> 
> Signed-off-by: Magnus Damm <damm@opensource.se>

Got this when applying the patch:

warning: squelched 8 whitespace errors
warning: 13 lines add whitespace errors.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131217/07649155/attachment.sig>

[PATCH 01/04] ARM: shmobile: r7s72100 GPIO and PINCTRL device nodes

[Magnus Damm]

Hi Laurent,

On Wed, Dec 18, 2013 at 1:29 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> Thank you for the patch.
>
> On Tuesday 17 December 2013 14:02:42 Magnus Damm wrote:
>> From: Magnus Damm <damm@opensource.se>
>>
>> Add support for r7s72100 PFC and GPIO device nodes port0 -> port11
>> and jtagport0.
>>
>> Signed-off-by: Magnus Damm <damm@opensource.se>
>> ---
>>
>>  arch/arm/boot/dts/r7s72100.dtsi |  154 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 154 insertions(+)
>>
>> --- 0001/arch/arm/boot/dts/r7s72100.dtsi
>> +++ work/arch/arm/boot/dts/r7s72100.dtsi      2013-11-27 16:06:36.000000000
> +0900
>> @@ -14,6 +14,22 @@
>>       #address-cells = <1>;
>>       #size-cells = <1>;
>>
>> +     aliases {
>> +             gpio0 = &port0;
>> +             gpio1 = &port1;
>> +             gpio2 = &port2;
>> +             gpio3 = &port3;
>> +             gpio4 = &port4;
>> +             gpio5 = &port5;
>> +             gpio6 = &port6;
>> +             gpio7 = &port7;
>> +             gpio8 = &port8;
>> +             gpio9 = &port9;
>> +             gpio10 = &port10;
>> +             gpio11 = &port11;
>> +             gpio12 = &jtagport0;
>> +     };
>> +
>>       cpus {
>>               #address-cells = <1>;
>>               #size-cells = <0>;
>> @@ -33,4 +49,142 @@
>>               reg = <0xe8201000 0x1000>,
>>                       <0xe8202000 0x1000>;
>>       };
>> +
>> +     pfc: pfc at fcfe3300 {
>> +             compatible = "renesas,pfc-r7s72100";
>> +             reg = <0xfcfe3300 0x400>, /* PM, PMC, PFC, PFCE */
>> +                       <0xfcfe3a00 0x100>, /* PFCAE */
>> +                       <0xfcfe7000 0x300>, /* PIBC, PBDC, PIPC */
>> +                       <0xfcfe7b40 0x04>, /* JPMC */
>> +                       <0xfcfe7b90 0x04>, /* JPMCSR */
>> +                       <0xfcfe7f00 0x04>; /* JPIBC */
>> +     };
>> +
>> +     port0: gpio at fcfe3100 {
>> +             compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
>> +             reg = <0xfcfe3100 0x4>, /* PSR */
>> +                       <0xfcfe3200 0x2>, /* PPR */
>> +                       <0xfcfe3800 0x4>; /* PMSR */
>> +             #gpio-cells = <2>;
>> +             gpio-controller;
>> +             gpio-ranges = <&pfc 0 0 6>;
>> +     };
>> +
>> +     port1: gpio at fcfe3104 {
>> +             compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
>> +             reg = <0xfcfe3104 0x4>, /* PSR */
>> +                       <0xfcfe3204 0x2>, /* PPR */
>> +                       <0xfcfe3804 0x4>; /* PMSR */
>> +             #gpio-cells = <2>;
>> +             gpio-controller;
>> +             gpio-ranges = <&pfc 0 16 16>;
>
> As P0 has 6 pins only this should ideally be
>
>                 gpio-ranges = <&pfc 0 6 16>;
>
> Otherwise the PFC driver will expose pins that don't exist. However, that
> would require computing the pin numbers in the PFC driver differently, as we
> currently just use the bank * 16 + index formula. Given that we only have
> three ports with less than 16 pins we could come up with a not overly complex
> formula that can be evaluated at compile time. Something like this should do.
>
> #define RZ_PORT_PIN(bank, pin) \
>         (bank) < 1 ? (pin) : \
>         (bank) < 6 ? 6 + (((bank) - 1) * 16) + (pin)) : \
>         (bank) < 10 ? 6 + 11 + 4 * 16 + (((bank) - 6) * 16) + (pin)) : \
>         6 + 11 + 8 + 7 * 16 + (((bank) - 10) * 16) + (pin))

Uhm, well, you can make the mapping more compact yes, but I'm not sure
if I agree that it becomes any better. Isn't it better to simply
follow the per-port setup that the manual defines? Is there an actual
problem with having unused GPIOs?

Actually, I prefer going in the opposite direction so I would like to
share the simple version of RZ_PORT_PIN() in a header file like we do
with RCAR_GP_PIN() in <linux/platform_data/gpio-rcar.h>. This because
we would like to use the same macro in the GPIO driver and in the
current PFC code (and potentially more PFCs using the same GPIO
driver).

Please let me know what you think.

Cheers,

/ magnus

[PATCH 01/04] ARM: shmobile: r7s72100 GPIO and PINCTRL device nodes

[Laurent Pinchart]

Hi Magnus,

On Wednesday 18 December 2013 07:41:57 Magnus Damm wrote:
> On Wed, Dec 18, 2013 at 1:29 AM, Laurent Pinchart wrote:
> > On Tuesday 17 December 2013 14:02:42 Magnus Damm wrote:
> >> From: Magnus Damm <damm@opensource.se>
> >> 
> >> Add support for r7s72100 PFC and GPIO device nodes port0 -> port11
> >> and jtagport0.
> >> 
> >> Signed-off-by: Magnus Damm <damm@opensource.se>
> >> ---
> >> 
> >>  arch/arm/boot/dts/r7s72100.dtsi |  154 +++++++++++++++++++++++++++++++++
> >>  1 file changed, 154 insertions(+)
> >> 
> >> --- 0001/arch/arm/boot/dts/r7s72100.dtsi
> >> +++ work/arch/arm/boot/dts/r7s72100.dtsi      2013-11-27
> >> 16:06:36.000000000 +0900

[snip]

> >> +
> >> +     port0: gpio at fcfe3100 {
> >> +             compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> >> +             reg = <0xfcfe3100 0x4>, /* PSR */
> >> +                       <0xfcfe3200 0x2>, /* PPR */
> >> +                       <0xfcfe3800 0x4>; /* PMSR */
> >> +             #gpio-cells = <2>;
> >> +             gpio-controller;
> >> +             gpio-ranges = <&pfc 0 0 6>;
> >> +     };
> >> +
> >> +     port1: gpio at fcfe3104 {
> >> +             compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> >> +             reg = <0xfcfe3104 0x4>, /* PSR */
> >> +                       <0xfcfe3204 0x2>, /* PPR */
> >> +                       <0xfcfe3804 0x4>; /* PMSR */
> >> +             #gpio-cells = <2>;
> >> +             gpio-controller;
> >> +             gpio-ranges = <&pfc 0 16 16>;
> > 
> > As P0 has 6 pins only this should ideally be
> > 
> >                 gpio-ranges = <&pfc 0 6 16>;
> > 
> > Otherwise the PFC driver will expose pins that don't exist. However, that
> > would require computing the pin numbers in the PFC driver differently, as
> > we currently just use the bank * 16 + index formula. Given that we only
> > have three ports with less than 16 pins we could come up with a not
> > overly complex formula that can be evaluated at compile time. Something
> > like this should do.
> > 
> > #define RZ_PORT_PIN(bank, pin) \
> > 
> >         (bank) < 1 ? (pin) : \
> >         (bank) < 6 ? 6 + (((bank) - 1) * 16) + (pin)) : \
> >         (bank) < 10 ? 6 + 11 + 4 * 16 + (((bank) - 6) * 16) + (pin)) : \
> >         6 + 11 + 8 + 7 * 16 + (((bank) - 10) * 16) + (pin))
> 
> Uhm, well, you can make the mapping more compact yes, but I'm not sure
> if I agree that it becomes any better. Isn't it better to simply
> follow the per-port setup that the manual defines? Is there an actual
> problem with having unused GPIOs?

If I'm not mistaken it's unused pins, not unused GPIOs. They waste memory in 
data tables, although by a relatively small amount. Oh, and of course, it's 
not clean ;-)

Speaking of data tables, I'm thinking about simplifying them. The RZ/A1H is a 
good candidate for that, as each pin is handled individually, and several 
registers could be handled to with a small amount of code instead of large 
data tables. It's just a thought for now, I have more urgent tasks to work on.

> Actually, I prefer going in the opposite direction so I would like to
> share the simple version of RZ_PORT_PIN() in a header file like we do
> with RCAR_GP_PIN() in <linux/platform_data/gpio-rcar.h>. This because
> we would like to use the same macro in the GPIO driver and in the
> current PFC code (and potentially more PFCs using the same GPIO
> driver).

What do you need it for in the GPIO driver ?

> Please let me know what you think.

-- 
Regards,

Laurent Pinchart

[PATCH 01/04] ARM: shmobile: r7s72100 GPIO and PINCTRL device nodes

[Magnus Damm]

Hi Laurent,

On Wed, Dec 18, 2013 at 9:40 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> On Wednesday 18 December 2013 07:41:57 Magnus Damm wrote:
>> On Wed, Dec 18, 2013 at 1:29 AM, Laurent Pinchart wrote:
>> > On Tuesday 17 December 2013 14:02:42 Magnus Damm wrote:
>> >> From: Magnus Damm <damm@opensource.se>
>> >>
>> >> Add support for r7s72100 PFC and GPIO device nodes port0 -> port11
>> >> and jtagport0.
>> >>
>> >> Signed-off-by: Magnus Damm <damm@opensource.se>
>> >> ---
>> >>
>> >>  arch/arm/boot/dts/r7s72100.dtsi |  154 +++++++++++++++++++++++++++++++++
>> >>  1 file changed, 154 insertions(+)
>> >>
>> >> --- 0001/arch/arm/boot/dts/r7s72100.dtsi
>> >> +++ work/arch/arm/boot/dts/r7s72100.dtsi      2013-11-27
>> >> 16:06:36.000000000 +0900
>
> [snip]
>
>> >> +
>> >> +     port0: gpio at fcfe3100 {
>> >> +             compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
>> >> +             reg = <0xfcfe3100 0x4>, /* PSR */
>> >> +                       <0xfcfe3200 0x2>, /* PPR */
>> >> +                       <0xfcfe3800 0x4>; /* PMSR */
>> >> +             #gpio-cells = <2>;
>> >> +             gpio-controller;
>> >> +             gpio-ranges = <&pfc 0 0 6>;
>> >> +     };
>> >> +
>> >> +     port1: gpio at fcfe3104 {
>> >> +             compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
>> >> +             reg = <0xfcfe3104 0x4>, /* PSR */
>> >> +                       <0xfcfe3204 0x2>, /* PPR */
>> >> +                       <0xfcfe3804 0x4>; /* PMSR */
>> >> +             #gpio-cells = <2>;
>> >> +             gpio-controller;
>> >> +             gpio-ranges = <&pfc 0 16 16>;
>> >
>> > As P0 has 6 pins only this should ideally be
>> >
>> >                 gpio-ranges = <&pfc 0 6 16>;
>> >
>> > Otherwise the PFC driver will expose pins that don't exist. However, that
>> > would require computing the pin numbers in the PFC driver differently, as
>> > we currently just use the bank * 16 + index formula. Given that we only
>> > have three ports with less than 16 pins we could come up with a not
>> > overly complex formula that can be evaluated at compile time. Something
>> > like this should do.
>> >
>> > #define RZ_PORT_PIN(bank, pin) \
>> >
>> >         (bank) < 1 ? (pin) : \
>> >         (bank) < 6 ? 6 + (((bank) - 1) * 16) + (pin)) : \
>> >         (bank) < 10 ? 6 + 11 + 4 * 16 + (((bank) - 6) * 16) + (pin)) : \
>> >         6 + 11 + 8 + 7 * 16 + (((bank) - 10) * 16) + (pin))
>>
>> Uhm, well, you can make the mapping more compact yes, but I'm not sure
>> if I agree that it becomes any better. Isn't it better to simply
>> follow the per-port setup that the manual defines? Is there an actual
>> problem with having unused GPIOs?
>
> If I'm not mistaken it's unused pins, not unused GPIOs. They waste memory in
> data tables, although by a relatively small amount. Oh, and of course, it's
> not clean ;-)

Yes, you are correct about pins vs GPIOs. Regarding how to implement
RZ_PORT_PIN(), I believe the only way not to shoot yourself in the
foot is to keep things simple. I also think that some level of
redundancy is an acceptable tradeoff if it keeps things simpler. So I
suppose cleanliness is a matter of taste. =)

> Speaking of data tables, I'm thinking about simplifying them. The RZ/A1H is a
> good candidate for that, as each pin is handled individually, and several
> registers could be handled to with a small amount of code instead of large
> data tables. It's just a thought for now, I have more urgent tasks to work on.

Incremental patches to improve the state is always nice, thanks.

>> Actually, I prefer going in the opposite direction so I would like to
>> share the simple version of RZ_PORT_PIN() in a header file like we do
>> with RCAR_GP_PIN() in <linux/platform_data/gpio-rcar.h>. This because
>> we would like to use the same macro in the GPIO driver and in the
>> current PFC code (and potentially more PFCs using the same GPIO
>> driver).
>
> What do you need it for in the GPIO driver ?

Well, I thought I needed it but it turns out that I'm wrong. =)

Initially I had the following two in the header file:
+#define RZ_GPIOS_PER_PORT 16
+#define RZ_PORT_PIN(bank, pin) (((bank) * RZ_GPIOS_PER_PORT) + (pin))

RZ_GPIOS_PER_PORT was used in both the GPIO driver and
RZ_PORT_PIN() was used in the PFC driver

On a second though, I don't mind duplicating them.

I do however think your version of the RZ_PORT_PIN() is overly
complex. And that needs to be matched with updated gpio-ranges that
together seem quite error prone to me.

How would you like me to proceed?

Cheers,

/ magnus

[PATCH 01/04] ARM: shmobile: r7s72100 GPIO and PINCTRL device nodes

[Laurent Pinchart]

Hi Magnus (and Linus, as there's a question for your below),

On Wednesday 18 December 2013 11:07:15 Magnus Damm wrote:
> On Wed, Dec 18, 2013 at 9:40 AM, Laurent Pinchart wrote:
> > On Wednesday 18 December 2013 07:41:57 Magnus Damm wrote:
> >> On Wed, Dec 18, 2013 at 1:29 AM, Laurent Pinchart wrote:
> >> > On Tuesday 17 December 2013 14:02:42 Magnus Damm wrote:
> >> >> From: Magnus Damm <damm@opensource.se>
> >> >> 
> >> >> Add support for r7s72100 PFC and GPIO device nodes port0 -> port11
> >> >> and jtagport0.
> >> >> 
> >> >> Signed-off-by: Magnus Damm <damm@opensource.se>
> >> >> ---
> >> >> 
> >> >>  arch/arm/boot/dts/r7s72100.dtsi |  154 ++++++++++++++++++++++++++++++
> >> >>  1 file changed, 154 insertions(+)
> >> >> 
> >> >> --- 0001/arch/arm/boot/dts/r7s72100.dtsi
> >> >> +++ work/arch/arm/boot/dts/r7s72100.dtsi      2013-11-27
> >> >> 16:06:36.000000000 +0900
> > 
> > [snip]
> > 
> >> >> +
> >> >> +     port0: gpio at fcfe3100 {
> >> >> +             compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> >> >> +             reg = <0xfcfe3100 0x4>, /* PSR */
> >> >> +                       <0xfcfe3200 0x2>, /* PPR */
> >> >> +                       <0xfcfe3800 0x4>; /* PMSR */
> >> >> +             #gpio-cells = <2>;
> >> >> +             gpio-controller;
> >> >> +             gpio-ranges = <&pfc 0 0 6>;
> >> >> +     };
> >> >> +
> >> >> +     port1: gpio at fcfe3104 {
> >> >> +             compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> >> >> +             reg = <0xfcfe3104 0x4>, /* PSR */
> >> >> +                       <0xfcfe3204 0x2>, /* PPR */
> >> >> +                       <0xfcfe3804 0x4>; /* PMSR */
> >> >> +             #gpio-cells = <2>;
> >> >> +             gpio-controller;
> >> >> +             gpio-ranges = <&pfc 0 16 16>;
> >> > 
> >> > As P0 has 6 pins only this should ideally be
> >> > 
> >> >                 gpio-ranges = <&pfc 0 6 16>;
> >> > 
> >> > Otherwise the PFC driver will expose pins that don't exist. However,
> >> > that would require computing the pin numbers in the PFC driver
> >> > differently, as we currently just use the bank * 16 + index formula.
> >> > Given that we only have three ports with less than 16 pins we could
> >> > come up with a not overly complex formula that can be evaluated at
> >> > compile time. Something like this should do.
> >> > 
> >> > #define RZ_PORT_PIN(bank, pin) \
> >> > 
> >> >         (bank) < 1 ? (pin) : \
> >> >         (bank) < 6 ? 6 + (((bank) - 1) * 16) + (pin)) : \
> >> >         (bank) < 10 ? 6 + 11 + 4 * 16 + (((bank) - 6) * 16) + (pin)) :
> >> >         \
> >> >         6 + 11 + 8 + 7 * 16 + (((bank) - 10) * 16) + (pin))
> >> 
> >> Uhm, well, you can make the mapping more compact yes, but I'm not sure
> >> if I agree that it becomes any better. Isn't it better to simply
> >> follow the per-port setup that the manual defines? Is there an actual
> >> problem with having unused GPIOs?
> > 
> > If I'm not mistaken it's unused pins, not unused GPIOs. They waste memory
> > in data tables, although by a relatively small amount. Oh, and of course,
> > it's not clean ;-)
> 
> Yes, you are correct about pins vs GPIOs. Regarding how to implement
> RZ_PORT_PIN(), I believe the only way not to shoot yourself in the foot is
> to keep things simple. I also think that some level of redundancy is an
> acceptable tradeoff if it keeps things simpler. So I suppose cleanliness is
> a matter of taste. =)

Absolutely, and there's no universal reason why my cleanliness would be better 
than yours :-)

> > Speaking of data tables, I'm thinking about simplifying them. The RZ/A1H
> > is a good candidate for that, as each pin is handled individually, and
> > several registers could be handled to with a small amount of code instead
> > of large data tables. It's just a thought for now, I have more urgent
> > tasks to work on.
>
> Incremental patches to improve the state is always nice, thanks.

You're welcome.

> >> Actually, I prefer going in the opposite direction so I would like to
> >> share the simple version of RZ_PORT_PIN() in a header file like we do
> >> with RCAR_GP_PIN() in <linux/platform_data/gpio-rcar.h>. This because
> >> we would like to use the same macro in the GPIO driver and in the
> >> current PFC code (and potentially more PFCs using the same GPIO
> >> driver).
> > 
> > What do you need it for in the GPIO driver ?
> 
> Well, I thought I needed it but it turns out that I'm wrong. =)
> 
> Initially I had the following two in the header file:
> +#define RZ_GPIOS_PER_PORT 16
> +#define RZ_PORT_PIN(bank, pin) (((bank) * RZ_GPIOS_PER_PORT) + (pin))
> 
> RZ_GPIOS_PER_PORT was used in both the GPIO driver and
> RZ_PORT_PIN() was used in the PFC driver
> 
> On a second though, I don't mind duplicating them.

I agree here, I don't think we need to share RZ_GPIOS_PER_PORT between the two 
drivers.

> I do however think your version of the RZ_PORT_PIN() is overly complex. And
> that needs to be matched with updated gpio-ranges that together seem quite
> error prone to me.
> 
> How would you like me to proceed?

I have mixed feelings about this, and understand your concern about 
complexity. I usually tend to favor correctness over complexity (without 
reaching the overcomplexity level). In this case I'd like to hear Linus' point 
of view. If he's fine with your version of the code I'll be fine as well. Is 
that OK ?

-- 
Regards,

Laurent Pinchart

[PATCH 01/04] ARM: shmobile: r7s72100 GPIO and PINCTRL device nodes

[Magnus Damm]

Hi Laurent,

On Thu, Dec 19, 2013 at 9:15 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus (and Linus, as there's a question for your below),
>
> On Wednesday 18 December 2013 11:07:15 Magnus Damm wrote:
>> On Wed, Dec 18, 2013 at 9:40 AM, Laurent Pinchart wrote:
>> > On Wednesday 18 December 2013 07:41:57 Magnus Damm wrote:
>> >> On Wed, Dec 18, 2013 at 1:29 AM, Laurent Pinchart wrote:
>> >> > On Tuesday 17 December 2013 14:02:42 Magnus Damm wrote:
>> >> >> From: Magnus Damm <damm@opensource.se>
>> >> >>
>> >> >> Add support for r7s72100 PFC and GPIO device nodes port0 -> port11
>> >> >> and jtagport0.
>> >> >>
>> >> >> Signed-off-by: Magnus Damm <damm@opensource.se>
>> >> >> ---
>> >> >>
>> >> >>  arch/arm/boot/dts/r7s72100.dtsi |  154 ++++++++++++++++++++++++++++++
>> >> >>  1 file changed, 154 insertions(+)
>> >> >>
>> >> >> --- 0001/arch/arm/boot/dts/r7s72100.dtsi
>> >> >> +++ work/arch/arm/boot/dts/r7s72100.dtsi      2013-11-27
>> >> >> 16:06:36.000000000 +0900
>> >
>> > [snip]
>> >
>> >> >> +
>> >> >> +     port0: gpio at fcfe3100 {
>> >> >> +             compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
>> >> >> +             reg = <0xfcfe3100 0x4>, /* PSR */
>> >> >> +                       <0xfcfe3200 0x2>, /* PPR */
>> >> >> +                       <0xfcfe3800 0x4>; /* PMSR */
>> >> >> +             #gpio-cells = <2>;
>> >> >> +             gpio-controller;
>> >> >> +             gpio-ranges = <&pfc 0 0 6>;
>> >> >> +     };
>> >> >> +
>> >> >> +     port1: gpio at fcfe3104 {
>> >> >> +             compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
>> >> >> +             reg = <0xfcfe3104 0x4>, /* PSR */
>> >> >> +                       <0xfcfe3204 0x2>, /* PPR */
>> >> >> +                       <0xfcfe3804 0x4>; /* PMSR */
>> >> >> +             #gpio-cells = <2>;
>> >> >> +             gpio-controller;
>> >> >> +             gpio-ranges = <&pfc 0 16 16>;
>> >> >
>> >> > As P0 has 6 pins only this should ideally be
>> >> >
>> >> >                 gpio-ranges = <&pfc 0 6 16>;
>> >> >
>> >> > Otherwise the PFC driver will expose pins that don't exist. However,
>> >> > that would require computing the pin numbers in the PFC driver
>> >> > differently, as we currently just use the bank * 16 + index formula.
>> >> > Given that we only have three ports with less than 16 pins we could
>> >> > come up with a not overly complex formula that can be evaluated at
>> >> > compile time. Something like this should do.
>> >> >
>> >> > #define RZ_PORT_PIN(bank, pin) \
>> >> >
>> >> >         (bank) < 1 ? (pin) : \
>> >> >         (bank) < 6 ? 6 + (((bank) - 1) * 16) + (pin)) : \
>> >> >         (bank) < 10 ? 6 + 11 + 4 * 16 + (((bank) - 6) * 16) + (pin)) :
>> >> >         \
>> >> >         6 + 11 + 8 + 7 * 16 + (((bank) - 10) * 16) + (pin))
>> >>
>> >> Uhm, well, you can make the mapping more compact yes, but I'm not sure
>> >> if I agree that it becomes any better. Isn't it better to simply
>> >> follow the per-port setup that the manual defines? Is there an actual
>> >> problem with having unused GPIOs?
>> >
>> > If I'm not mistaken it's unused pins, not unused GPIOs. They waste memory
>> > in data tables, although by a relatively small amount. Oh, and of course,
>> > it's not clean ;-)
>>
>> Yes, you are correct about pins vs GPIOs. Regarding how to implement
>> RZ_PORT_PIN(), I believe the only way not to shoot yourself in the foot is
>> to keep things simple. I also think that some level of redundancy is an
>> acceptable tradeoff if it keeps things simpler. So I suppose cleanliness is
>> a matter of taste. =)
>
> Absolutely, and there's no universal reason why my cleanliness would be better
> than yours :-)

I think we should count LOC, just to add a rigged metric to my side. =)

>> > Speaking of data tables, I'm thinking about simplifying them. The RZ/A1H
>> > is a good candidate for that, as each pin is handled individually, and
>> > several registers could be handled to with a small amount of code instead
>> > of large data tables. It's just a thought for now, I have more urgent
>> > tasks to work on.
>>
>> Incremental patches to improve the state is always nice, thanks.
>
> You're welcome.

Thanks.

>> >> Actually, I prefer going in the opposite direction so I would like to
>> >> share the simple version of RZ_PORT_PIN() in a header file like we do
>> >> with RCAR_GP_PIN() in <linux/platform_data/gpio-rcar.h>. This because
>> >> we would like to use the same macro in the GPIO driver and in the
>> >> current PFC code (and potentially more PFCs using the same GPIO
>> >> driver).
>> >
>> > What do you need it for in the GPIO driver ?
>>
>> Well, I thought I needed it but it turns out that I'm wrong. =)
>>
>> Initially I had the following two in the header file:
>> +#define RZ_GPIOS_PER_PORT 16
>> +#define RZ_PORT_PIN(bank, pin) (((bank) * RZ_GPIOS_PER_PORT) + (pin))
>>
>> RZ_GPIOS_PER_PORT was used in both the GPIO driver and
>> RZ_PORT_PIN() was used in the PFC driver
>>
>> On a second though, I don't mind duplicating them.
>
> I agree here, I don't think we need to share RZ_GPIOS_PER_PORT between the two
> drivers.

They have separate name spaces for GPIOS and pins anyway, so yes.

>> I do however think your version of the RZ_PORT_PIN() is overly complex. And
>> that needs to be matched with updated gpio-ranges that together seem quite
>> error prone to me.
>>
>> How would you like me to proceed?
>
> I have mixed feelings about this, and understand your concern about
> complexity. I usually tend to favor correctness over complexity (without
> reaching the overcomplexity level). In this case I'd like to hear Linus' point
> of view. If he's fine with your version of the code I'll be fine as well. Is
> that OK ?

Sure, that's fine. I may resend the series as-is and see what happens. =)

/ magnus