[PATCH V2 0/5] arm: shmobile: r7s72100: add native i2c support

[PATCH V2 0/5] arm: shmobile: r7s72100: add native i2c support

[Wolfram Sang]

Here is a series to support the I2C core of the r7s72100 natively. It has been
tested with a genmai board where an eeprom is connected. The series can be
found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/riic

It is based on the latest r7s72100 topic branch from Simon and the latest patch
series from Magnus. Note there is one issue with the current PFC driver which
needs to be sorted out. Check patch 1/5 for details.

I've addressed all comments from V1. See patches for details.

Comments welcome. Thanks,

   Wolfram


Wolfram Sang (5):
  pinctrl: r7s72100: add riic groups
  arm: shmobile: r7s72100: add i2c clocks
  arm: shmobile: r7s72100: add nodes for i2c controllers to dtsi
  arm: shmobile: genmai: adapt dts to use native i2c driver
  i2c: riic: add driver

 Documentation/devicetree/bindings/i2c/i2c-riic.txt |  29 ++
 arch/arm/boot/dts/r7s72100-genmai-reference.dts    |  29 +-
 arch/arm/boot/dts/r7s72100.dtsi                    | 102 +++++
 arch/arm/mach-shmobile/clock-r7s72100.c            |  12 +-
 drivers/i2c/busses/Kconfig                         |  10 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-riic.c                      | 426 +++++++++++++++++++++
 drivers/pinctrl/sh-pfc/pfc-r7s72100.c              |  45 +++
 8 files changed, 635 insertions(+), 19 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-riic.txt
 create mode 100644 drivers/i2c/busses/i2c-riic.c

-- 
1.8.4.2

[PATCH V2 1/5] pinctrl: r7s72100: add riic groups

[Wolfram Sang]

From: Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>

Tested RIIC2 on a genmai board. Other riic groups are untested but seem
trivial enough to be added.

Signed-off-by: Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
Acked-by: Magnus Damm <damm-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>
---

V2: keep sorting alphabetical

Note: With the current PFC driver as posted by Magnus, it needs another patch
to work. Yet, I think this is a seperate PFC issue which needs to be sorted out
seperately and shouldn't affect these declarations. I'll add the needed patch
as a response to this mail.

 drivers/pinctrl/sh-pfc/pfc-r7s72100.c | 45 +++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/pinctrl/sh-pfc/pfc-r7s72100.c b/drivers/pinctrl/sh-pfc/pfc-r7s72100.c
index a662876..7ee0442 100644
--- a/drivers/pinctrl/sh-pfc/pfc-r7s72100.c
+++ b/drivers/pinctrl/sh-pfc/pfc-r7s72100.c
@@ -117,6 +117,27 @@ static const unsigned int __RZ_STR(pfx, hw, bank, pin, _mux)[] = {	\
 #define RZ_GROUPS(pfx, hw, bank, pin, fn) \
 	__RZ_GROUPS(pfx##_##hw##_p##bank##_##pin),
 
+#define RIIC0(fn)			\
+	fn(riic0, scl, 1, 0, 1)		\
+	fn(riic0, sda, 1, 1, 1)
+
+#define RIIC1(fn)			\
+	fn(riic1, scl, 1, 2, 1)		\
+	fn(riic1, sda, 1, 3, 1)
+
+#define RIIC2(fn)			\
+	fn(riic2, scl, 1, 4, 1)		\
+	fn(riic2, sda, 1, 5, 1)
+
+#define RIIC3(fn)			\
+	fn(riic3, scl, 1, 6, 1)		\
+	fn(riic3, sda, 1, 7, 1)
+
+RIIC0(RZ_PIN_AND_MUX)
+RIIC1(RZ_PIN_AND_MUX)
+RIIC2(RZ_PIN_AND_MUX)
+RIIC3(RZ_PIN_AND_MUX)
+
 #define SCIF0(fn)			\
 	fn(scif0, clk, 2, 13, 6)	\
 	fn(scif0, txd, 2, 14, 6)	\
@@ -230,6 +251,10 @@ SCIF6(RZ_PIN_AND_MUX)
 SCIF7(RZ_PIN_AND_MUX)
 
 static const struct sh_pfc_pin_group pinmux_groups[] = {
+	RIIC0(RZ_PMX_GROUP)
+	RIIC1(RZ_PMX_GROUP)
+	RIIC2(RZ_PMX_GROUP)
+	RIIC3(RZ_PMX_GROUP)
 	SCIF0(RZ_PMX_GROUP)
 	SCIF1(RZ_PMX_GROUP)
 	SCIF2(RZ_PMX_GROUP)
@@ -240,6 +265,22 @@ static const struct sh_pfc_pin_group pinmux_groups[] = {
 	SCIF7(RZ_PMX_GROUP)
 };
 
+static const char * const riic0_groups[] = {
+	RIIC0(RZ_GROUPS)
+};
+
+static const char * const riic1_groups[] = {
+	RIIC1(RZ_GROUPS)
+};
+
+static const char * const riic2_groups[] = {
+	RIIC2(RZ_GROUPS)
+};
+
+static const char * const riic3_groups[] = {
+	RIIC3(RZ_GROUPS)
+};
+
 static const char * const scif0_groups[] = {
 	SCIF0(RZ_GROUPS)
 };
@@ -273,6 +314,10 @@ static const char * const scif7_groups[] = {
 };
 
 static const struct sh_pfc_function pinmux_functions[] = {
+	SH_PFC_FUNCTION(riic0),
+	SH_PFC_FUNCTION(riic1),
+	SH_PFC_FUNCTION(riic2),
+	SH_PFC_FUNCTION(riic3),
 	SH_PFC_FUNCTION(scif0),
 	SH_PFC_FUNCTION(scif1),
 	SH_PFC_FUNCTION(scif2),
-- 
1.8.4.2

Re: [PATCH V2 1/5] pinctrl: r7s72100: add riic groups

[Wolfram Sang]

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

On Wed, Dec 18, 2013 at 10:31:57PM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
> 
> Tested RIIC2 on a genmai board. Other riic groups are untested but seem
> trivial enough to be added.
> 
> Signed-off-by: Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
> Acked-by: Magnus Damm <damm-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>
> ---
> 
> V2: keep sorting alphabetical
> 
> Note: With the current PFC driver as posted by Magnus, it needs another patch
> to work. Yet, I think this is a seperate PFC issue which needs to be sorted out
> seperately and shouldn't affect these declarations. I'll add the needed patch
> as a response to this mail.

From: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
Subject: [PATCH] HACK: make magnus new driver work

Signed-off-by: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
---
 drivers/pinctrl/sh-pfc/pfc-r7s72100.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/sh-pfc/pfc-r7s72100.c b/drivers/pinctrl/sh-pfc/pfc-r7s72100.c
index 00f67e8..a662876 100644
--- a/drivers/pinctrl/sh-pfc/pfc-r7s72100.c
+++ b/drivers/pinctrl/sh-pfc/pfc-r7s72100.c
@@ -68,11 +68,11 @@ enum {
 #define _P_GPIO(bank, _pin, _name, sfx) _GP_GPIO(16, bank, _pin, _name, sfx)
 
 #define _P_DATA(bank, pin, name, sfx)					\
-	PINMUX_DATA(name##_DATA, name##_PMC_0, name##_PIPC_0,		\
+	PINMUX_DATA(name##_DATA, name##_PMC_0,		\
 		    name##_PIBC_1, name##_PBDC_1)
 
 #define _P_FN(n, fn, pfcae, pfce, pfc)					\
-	PINMUX_DATA(n##_MARK_FN##fn, n##_PMC_1,	n##_PIPC_1,		\
+	PINMUX_DATA(n##_MARK_FN##fn, n##_PMC_1,		\
 		    n##_PFCAE_##pfcae, n##_PFCE_##pfce, n##_PFC_##pfc)
 
 #define _P_MARK_FN1(bank, pin, name, sfx) _P_FN(name, 1, 0, 0, 0)


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH V2 3/5] arm: shmobile: r7s72100: add nodes for i2c controllers to dtsi

[Wolfram Sang]

From: Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>

I decided to put the pinmuxing into the dtsi file since there is only
one pinmux posiibility which one probably wants to have when using the
bus.

Signed-off-by: Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
Acked-by: Magnus Damm <damm-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>
---
 arch/arm/boot/dts/r7s72100.dtsi | 102 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
index d396b38..ff0bd6b 100644
--- a/arch/arm/boot/dts/r7s72100.dtsi
+++ b/arch/arm/boot/dts/r7s72100.dtsi
@@ -8,6 +8,8 @@
  * kind, whether express or implied.
  */
 
+#include <dt-bindings/interrupt-controller/irq.h>
+
 / {
 	compatible = "renesas,r7s72100";
 	interrupt-parent = <&gic>;
@@ -15,6 +17,10 @@
 	#size-cells = <1>;
 
 	aliases {
+		i2c0 = &i2c0;
+		i2c1 = &i2c1;
+		i2c2 = &i2c2;
+		i2c3 = &i2c3;
 		gpio0 = &port0;
 		gpio1 = &port1;
 		gpio2 = &port2;
@@ -58,6 +64,26 @@
 			  <0xfcfe7b40 0x04>, /* JPMC */
 			  <0xfcfe7b90 0x04>, /* JPMCSR */
 			  <0xfcfe7f00 0x04>; /* JPIBC */
+
+		riic0_pins: i2c0 {
+			renesas,groups = "riic0_scl_p1_0", "riic0_sda_p1_1";
+			renesas,function = "riic0";
+		};
+
+		riic1_pins: i2c1 {
+			renesas,groups = "riic1_scl_p1_2", "riic1_sda_p1_3";
+			renesas,function = "riic1";
+		};
+
+		riic2_pins: i2c2 {
+			renesas,groups = "riic2_scl_p1_4", "riic2_sda_p1_5";
+			renesas,function = "riic2";
+		};
+
+		riic3_pins: i2c3 {
+			renesas,groups = "riic3_scl_p1_6", "riic3_sda_p1_7";
+			renesas,function = "riic3";
+		};
 	};
 
 	port0: gpio@fcfe3100 {
@@ -187,4 +213,80 @@
 		gpio-controller;
 		gpio-ranges = <&pfc 0 192 2>;
 	};
+
+	i2c0: i2c@fcfee000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "renesas,riic-r7s72100", "renesas,riic-rz";
+		reg = <0xfcfee000 0x44>;
+		interrupts = <0 157 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 158 IRQ_TYPE_EDGE_RISING>,
+			     <0 159 IRQ_TYPE_EDGE_RISING>,
+			     <0 160 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 161 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 162 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 163 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 164 IRQ_TYPE_LEVEL_HIGH>;
+		pinctrl-0 = <&riic0_pins>;
+		pinctrl-names = "default";
+		clock-frequency = <100000>;
+		status = "disabled";
+	};
+
+	i2c1: i2c@fcfee400 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "renesas,riic-r7s72100", "renesas,riic-rz";
+		reg = <0xfcfee400 0x44>;
+		interrupts = <0 165 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 166 IRQ_TYPE_EDGE_RISING>,
+			     <0 167 IRQ_TYPE_EDGE_RISING>,
+			     <0 168 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 169 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 170 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 171 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 172 IRQ_TYPE_LEVEL_HIGH>;
+		pinctrl-0 = <&riic1_pins>;
+		pinctrl-names = "default";
+		clock-frequency = <100000>;
+		status = "disabled";
+	};
+
+	i2c2: i2c@fcfee800 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "renesas,riic-r7s72100", "renesas,riic-rz";
+		reg = <0xfcfee800 0x44>;
+		interrupts = <0 173 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 174 IRQ_TYPE_EDGE_RISING>,
+			     <0 175 IRQ_TYPE_EDGE_RISING>,
+			     <0 176 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 177 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 178 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 179 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 180 IRQ_TYPE_LEVEL_HIGH>;
+		pinctrl-0 = <&riic2_pins>;
+		pinctrl-names = "default";
+		clock-frequency = <100000>;
+		status = "disabled";
+	};
+
+	i2c3: i2c@fcfeec00 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "renesas,riic-r7s72100", "renesas,riic-rz";
+		reg = <0xfcfeec00 0x44>;
+		interrupts = <0 181 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 182 IRQ_TYPE_EDGE_RISING>,
+			     <0 183 IRQ_TYPE_EDGE_RISING>,
+			     <0 184 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 185 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 186 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 187 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 188 IRQ_TYPE_LEVEL_HIGH>;
+		pinctrl-0 = <&riic3_pins>;
+		pinctrl-names = "default";
+		clock-frequency = <100000>;
+		status = "disabled";
+	};
 };
-- 
1.8.4.2

Re: [PATCH V2 3/5] arm: shmobile: r7s72100: add nodes for i2c controllers to dtsi

[Wolfram Sang]

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

On Wed, Dec 18, 2013 at 10:31:59PM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
> 
> I decided to put the pinmuxing into the dtsi file since there is only
> one pinmux posiibility which one probably wants to have when using the
> bus.
> 
> Signed-off-by: Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
> Acked-by: Magnus Damm <damm-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>

Simon, I think you could pull in these patches, too. My series does not
really depend on the pinctrl stuff. I2C pins are function group 1 and
thus activated by default if the bootloader doesn't do weird things.
Actually, I coded the PFC stuff after the driver was finished :)


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V2 3/5] arm: shmobile: r7s72100: add nodes for i2c controllers to dtsi

[Simon Horman]

On Fri, Dec 27, 2013 at 05:27:47AM +0100, Wolfram Sang wrote:
> On Wed, Dec 18, 2013 at 10:31:59PM +0100, Wolfram Sang wrote:
> > From: Wolfram Sang <wsa@sang-engineering.com>
> > 
> > I decided to put the pinmuxing into the dtsi file since there is only
> > one pinmux posiibility which one probably wants to have when using the
> > bus.
> > 
> > Signed-off-by: Wolfram Sang <wsa@sang-engineering.com>
> > Acked-by: Magnus Damm <damm@opensource.se>
> 
> Simon, I think you could pull in these patches, too. My series does not
> really depend on the pinctrl stuff. I2C pins are function group 1 and
> thus activated by default if the bootloader doesn't do weird things.
> Actually, I coded the PFC stuff after the driver was finished :)

That seems reasonable.

Could you send a series which contains the patches you would like
queued up? Please base it on the latest devel tag, currently
renesas-devel-v3.13-rc5-20131230, without other patches applied.

[PATCH V2 4/5] arm: shmobile: genmai: adapt dts to use native i2c driver

[Wolfram Sang]

From: Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>

Switch from the gpio-driver to the shiny new native driver. Tested by
accessing the eeprom on the genmai board.

Signed-off-by: Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
Acked-by: Magnus Damm <damm-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>
---

V2: fixed the eeprom label

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

diff --git a/arch/arm/boot/dts/r7s72100-genmai-reference.dts b/arch/arm/boot/dts/r7s72100-genmai-reference.dts
index ce5da0b..5e9f41d 100644
--- a/arch/arm/boot/dts/r7s72100-genmai-reference.dts
+++ b/arch/arm/boot/dts/r7s72100-genmai-reference.dts
@@ -39,24 +39,6 @@
 			gpios = <&port4 11 GPIO_ACTIVE_LOW>;
 		};
 	};
-
-	i2c@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>;
-
-		flash@50 {
-			compatible = "renesas,24c128";
-			reg = <0x50>;
-			pagesize = <64>;
-		};
-	};
 };
 
 &pfc {
@@ -68,3 +50,14 @@
 		renesas,function = "scif2";
 	};
 };
+
+&i2c2 {
+	status = "okay";
+	clock-frequency = <400000>;
+
+	eeprom@50 {
+		compatible = "renesas,24c128";
+		reg = <0x50>;
+		pagesize = <64>;
+	};
+};
-- 
1.8.4.2

[PATCH V2 2/5] arm: shmobile: r7s72100: add i2c clocks

[Wolfram Sang]

From: Wolfram Sang <wsa@sang-engineering.com>

Tested with RIIC2 on a genmai board. Others untested but hopefully
trivial enough to be added.

Signed-off-by: Wolfram Sang <wsa@sang-engineering.com>
Acked-by: Magnus Damm <damm@opensource.se>
---

V2: adapt to established sorting

 arch/arm/mach-shmobile/clock-r7s72100.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-shmobile/clock-r7s72100.c b/arch/arm/mach-shmobile/clock-r7s72100.c
index 7b457ae..9cda215 100644
--- a/arch/arm/mach-shmobile/clock-r7s72100.c
+++ b/arch/arm/mach-shmobile/clock-r7s72100.c
@@ -27,6 +27,7 @@
 #define FRQCR2		0xfcfe0014
 #define STBCR3		0xfcfe0420
 #define STBCR4		0xfcfe0424
+#define STBCR9		0xfcfe0438
 
 #define PLL_RATE 30
 
@@ -144,10 +145,15 @@ struct clk div4_clks[DIV4_NR] = {
 					| CLK_ENABLE_ON_INIT),
 };
 
-enum { MSTP47, MSTP46, MSTP45, MSTP44, MSTP43, MSTP42, MSTP41, MSTP40,
+enum {	MSTP97, MSTP96, MSTP95, MSTP94,
+	MSTP47, MSTP46, MSTP45, MSTP44, MSTP43, MSTP42, MSTP41, MSTP40,
 	MSTP33,	MSTP_NR };
 
 static struct clk mstp_clks[MSTP_NR] = {
+	[MSTP97] = SH_CLK_MSTP8(&peripheral0_clk, STBCR9, 7, 0), /* RIIC0 */
+	[MSTP96] = SH_CLK_MSTP8(&peripheral0_clk, STBCR9, 6, 0), /* RIIC1 */
+	[MSTP95] = SH_CLK_MSTP8(&peripheral0_clk, STBCR9, 5, 0), /* RIIC2 */
+	[MSTP94] = SH_CLK_MSTP8(&peripheral0_clk, STBCR9, 4, 0), /* RIIC3 */
 	[MSTP47] = SH_CLK_MSTP8(&peripheral1_clk, STBCR4, 7, 0), /* SCIF0 */
 	[MSTP46] = SH_CLK_MSTP8(&peripheral1_clk, STBCR4, 6, 0), /* SCIF1 */
 	[MSTP45] = SH_CLK_MSTP8(&peripheral1_clk, STBCR4, 5, 0), /* SCIF2 */
@@ -170,6 +176,10 @@ static struct clk_lookup lookups[] = {
 	CLKDEV_CON_ID("cpu_clk", &div4_clks[DIV4_I]),
 
 	/* MSTP clocks */
+	CLKDEV_DEV_ID("fcfee000.i2c", &mstp_clks[MSTP97]),
+	CLKDEV_DEV_ID("fcfee400.i2c", &mstp_clks[MSTP96]),
+	CLKDEV_DEV_ID("fcfee800.i2c", &mstp_clks[MSTP95]),
+	CLKDEV_DEV_ID("fcfeec00.i2c", &mstp_clks[MSTP94]),
 	CLKDEV_CON_ID("mtu2_fck", &mstp_clks[MSTP33]),
 
 	/* ICK */
-- 
1.8.4.2

Re: [PATCH V2 2/5] arm: shmobile: r7s72100: add i2c clocks

[Simon Horman]

On Wed, Dec 18, 2013 at 10:31:58PM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
> 
> Tested with RIIC2 on a genmai board. Others untested but hopefully
> trivial enough to be added.
> 
> Signed-off-by: Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
> Acked-by: Magnus Damm <damm-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>
> ---
> 
> V2: adapt to established sorting

Thanks. This seems safe enough and I will queue it up.

> 
>  arch/arm/mach-shmobile/clock-r7s72100.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-shmobile/clock-r7s72100.c b/arch/arm/mach-shmobile/clock-r7s72100.c
> index 7b457ae..9cda215 100644
> --- a/arch/arm/mach-shmobile/clock-r7s72100.c
> +++ b/arch/arm/mach-shmobile/clock-r7s72100.c
> @@ -27,6 +27,7 @@
>  #define FRQCR2		0xfcfe0014
>  #define STBCR3		0xfcfe0420
>  #define STBCR4		0xfcfe0424
> +#define STBCR9		0xfcfe0438
>  
>  #define PLL_RATE 30
>  
> @@ -144,10 +145,15 @@ struct clk div4_clks[DIV4_NR] = {
>  					| CLK_ENABLE_ON_INIT),
>  };
>  
> -enum { MSTP47, MSTP46, MSTP45, MSTP44, MSTP43, MSTP42, MSTP41, MSTP40,
> +enum {	MSTP97, MSTP96, MSTP95, MSTP94,
> +	MSTP47, MSTP46, MSTP45, MSTP44, MSTP43, MSTP42, MSTP41, MSTP40,
>  	MSTP33,	MSTP_NR };
>  
>  static struct clk mstp_clks[MSTP_NR] = {
> +	[MSTP97] = SH_CLK_MSTP8(&peripheral0_clk, STBCR9, 7, 0), /* RIIC0 */
> +	[MSTP96] = SH_CLK_MSTP8(&peripheral0_clk, STBCR9, 6, 0), /* RIIC1 */
> +	[MSTP95] = SH_CLK_MSTP8(&peripheral0_clk, STBCR9, 5, 0), /* RIIC2 */
> +	[MSTP94] = SH_CLK_MSTP8(&peripheral0_clk, STBCR9, 4, 0), /* RIIC3 */
>  	[MSTP47] = SH_CLK_MSTP8(&peripheral1_clk, STBCR4, 7, 0), /* SCIF0 */
>  	[MSTP46] = SH_CLK_MSTP8(&peripheral1_clk, STBCR4, 6, 0), /* SCIF1 */
>  	[MSTP45] = SH_CLK_MSTP8(&peripheral1_clk, STBCR4, 5, 0), /* SCIF2 */
> @@ -170,6 +176,10 @@ static struct clk_lookup lookups[] = {
>  	CLKDEV_CON_ID("cpu_clk", &div4_clks[DIV4_I]),
>  
>  	/* MSTP clocks */
> +	CLKDEV_DEV_ID("fcfee000.i2c", &mstp_clks[MSTP97]),
> +	CLKDEV_DEV_ID("fcfee400.i2c", &mstp_clks[MSTP96]),
> +	CLKDEV_DEV_ID("fcfee800.i2c", &mstp_clks[MSTP95]),
> +	CLKDEV_DEV_ID("fcfeec00.i2c", &mstp_clks[MSTP94]),
>  	CLKDEV_CON_ID("mtu2_fck", &mstp_clks[MSTP33]),
>  
>  	/* ICK */
> -- 
> 1.8.4.2
> 

Re: [PATCH V2 2/5] arm: shmobile: r7s72100: add i2c clocks

[Geert Uytterhoeven]

On Tue, Dec 24, 2013 at 3:48 AM, Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org> wrote:
> On Wed, Dec 18, 2013 at 10:31:58PM +0100, Wolfram Sang wrote:
>> From: Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
>>
>> Tested with RIIC2 on a genmai board. Others untested but hopefully
>> trivial enough to be added.
>>
>> Signed-off-by: Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
>> Acked-by: Magnus Damm <damm-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>
>> ---
>>
>> V2: adapt to established sorting
>
> Thanks. This seems safe enough and I will queue it up.

Note that after dropping these lines:

       CLKDEV_DEV_ID("fcfee000.i2c", &mstp_clks[MSTP97]),
       CLKDEV_DEV_ID("fcfee400.i2c", &mstp_clks[MSTP96]),
       CLKDEV_DEV_ID("fcfee800.i2c", &mstp_clks[MSTP95]),
       CLKDEV_DEV_ID("fcfeec00.i2c", &mstp_clks[MSTP94]),

i2c stopped working.

Before:

of_irq_parse_one: dev=/i2c@fcfee800/eeprom@50, index=0
at24 2-0050: 16384 byte 24c128 EEPROM, writable, 64 bytes/write
i2c-riic fcfee800.i2c: registered with 400000Hz bus speed

After:

i2c-riic fcfee800.i2c: missing controller clock
i2c-riic: probe of fcfee800.i2c failed with error -2

Am I missing some other patch?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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 V2 2/5] arm: shmobile: r7s72100: add i2c clocks

[Wolfram Sang]

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

On Thu, Dec 26, 2013 at 10:20:23PM +0100, Geert Uytterhoeven wrote:
> On Tue, Dec 24, 2013 at 3:48 AM, Simon Horman <horms@verge.net.au> wrote:
> > On Wed, Dec 18, 2013 at 10:31:58PM +0100, Wolfram Sang wrote:
> >> From: Wolfram Sang <wsa@sang-engineering.com>
> >>
> >> Tested with RIIC2 on a genmai board. Others untested but hopefully
> >> trivial enough to be added.
> >>
> >> Signed-off-by: Wolfram Sang <wsa@sang-engineering.com>
> >> Acked-by: Magnus Damm <damm@opensource.se>
> >> ---
> >>
> >> V2: adapt to established sorting
> >
> > Thanks. This seems safe enough and I will queue it up.
> 
> Note that after dropping these lines:
> 
>        CLKDEV_DEV_ID("fcfee000.i2c", &mstp_clks[MSTP97]),
>        CLKDEV_DEV_ID("fcfee400.i2c", &mstp_clks[MSTP96]),
>        CLKDEV_DEV_ID("fcfee800.i2c", &mstp_clks[MSTP95]),
>        CLKDEV_DEV_ID("fcfeec00.i2c", &mstp_clks[MSTP94]),
> 
> i2c stopped working.

??? Why do you want to drop these lines?

If the i2c clocks are not registered, you won't get i2c support? Am I
missing something?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V2 2/5] arm: shmobile: r7s72100: add i2c clocks

[Geert Uytterhoeven]

On Thu, Dec 26, 2013 at 10:25 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> On Thu, Dec 26, 2013 at 10:20:23PM +0100, Geert Uytterhoeven wrote:
>> On Tue, Dec 24, 2013 at 3:48 AM, Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org> wrote:
>> > On Wed, Dec 18, 2013 at 10:31:58PM +0100, Wolfram Sang wrote:
>> >> From: Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
>> >>
>> >> Tested with RIIC2 on a genmai board. Others untested but hopefully
>> >> trivial enough to be added.
>> >>
>> >> Signed-off-by: Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
>> >> Acked-by: Magnus Damm <damm-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>
>> >> ---
>> >>
>> >> V2: adapt to established sorting
>> >
>> > Thanks. This seems safe enough and I will queue it up.
>>
>> Note that after dropping these lines:
>>
>>        CLKDEV_DEV_ID("fcfee000.i2c", &mstp_clks[MSTP97]),
>>        CLKDEV_DEV_ID("fcfee400.i2c", &mstp_clks[MSTP96]),
>>        CLKDEV_DEV_ID("fcfee800.i2c", &mstp_clks[MSTP95]),
>>        CLKDEV_DEV_ID("fcfeec00.i2c", &mstp_clks[MSTP94]),
>>
>> i2c stopped working.
>
> ??? Why do you want to drop these lines?

Your v2 dropped them, presumably in response to Sergei's ICKy comments.
And that's what Simon committed...

> If the i2c clocks are not registered, you won't get i2c support? Am I
> missing something?

Ah OK. So you do need them. Good, /me getting less confused ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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 V2 2/5] arm: shmobile: r7s72100: add i2c clocks

[Geert Uytterhoeven]

On Thu, Dec 26, 2013 at 10:34 PM, Geert Uytterhoeven
<geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
>>> Note that after dropping these lines:
>>>
>>>        CLKDEV_DEV_ID("fcfee000.i2c", &mstp_clks[MSTP97]),
>>>        CLKDEV_DEV_ID("fcfee400.i2c", &mstp_clks[MSTP96]),
>>>        CLKDEV_DEV_ID("fcfee800.i2c", &mstp_clks[MSTP95]),
>>>        CLKDEV_DEV_ID("fcfeec00.i2c", &mstp_clks[MSTP94]),
>>>
>>> i2c stopped working.
>>
>> ??? Why do you want to drop these lines?
>
> Your v2 dropped them, presumably in response to Sergei's ICKy comments.
> And that's what Simon committed...

Upon checking again, your v2 did contain them, but gmail tried to smart
me out by not showing those few lines.
So it must have been Simon who dropped them, or lost them during rebase?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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 V2 2/5] arm: shmobile: r7s72100: add i2c clocks

[Simon Horman]

On Thu, Dec 26, 2013 at 10:51:34PM +0100, Geert Uytterhoeven wrote:
> On Thu, Dec 26, 2013 at 10:34 PM, Geert Uytterhoeven
> <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
> >>> Note that after dropping these lines:
> >>>
> >>>        CLKDEV_DEV_ID("fcfee000.i2c", &mstp_clks[MSTP97]),
> >>>        CLKDEV_DEV_ID("fcfee400.i2c", &mstp_clks[MSTP96]),
> >>>        CLKDEV_DEV_ID("fcfee800.i2c", &mstp_clks[MSTP95]),
> >>>        CLKDEV_DEV_ID("fcfeec00.i2c", &mstp_clks[MSTP94]),
> >>>
> >>> i2c stopped working.
> >>
> >> ??? Why do you want to drop these lines?
> >
> > Your v2 dropped them, presumably in response to Sergei's ICKy comments.
> > And that's what Simon committed...
> 
> Upon checking again, your v2 did contain them, but gmail tried to smart
> me out by not showing those few lines.
> So it must have been Simon who dropped them, or lost them during rebase?

Yes, it seems so. Sorry about that!

Wolfram, could you post the these missing clocks as an incremental patch?

[PATCH V2 5/5] i2c: riic: add driver

[Wolfram Sang]

From: Wolfram Sang <wsa@sang-engineering.com>

Tested with a r7s72100 genmai board acessing an eeprom.

Signed-off-by: Wolfram Sang <wsa@sang-engineering.com>
---

V2: fixed the name typo in the binding docs

 Documentation/devicetree/bindings/i2c/i2c-riic.txt |  29 ++
 drivers/i2c/busses/Kconfig                         |  10 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-riic.c                      | 426 +++++++++++++++++++++
 4 files changed, 466 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-riic.txt
 create mode 100644 drivers/i2c/busses/i2c-riic.c

diff --git a/Documentation/devicetree/bindings/i2c/i2c-riic.txt b/Documentation/devicetree/bindings/i2c/i2c-riic.txt
new file mode 100644
index 0000000..0bcc471
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-riic.txt
@@ -0,0 +1,29 @@
+Device tree configuration for Renesas RIIC driver
+
+Required properties:
+- compatible      : "renesas,riic-<soctype>". "renesas,riic-rz" as fallback
+- reg             : address start and address range size of device
+- interrupts      : 8 interrupts (TEI, RI, TI, SPI, STI, NAKI, ALI, TMOI)
+- clock-frequency : frequency of bus clock in Hz
+- #address-cells  : should be <1>
+- #size-cells     : should be <0>
+
+Pinctrl properties might be needed, too. See there.
+
+Example:
+
+	i2c0: i2c@fcfee000 {
+		compatible = "renesas,riic-r7s72100", "renesas,riic-rz";
+		reg = <0xfcfee000 0x44>;
+		interrupts = <0 157 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 158 IRQ_TYPE_EDGE_RISING>,
+			     <0 159 IRQ_TYPE_EDGE_RISING>,
+			     <0 160 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 161 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 162 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 163 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 164 IRQ_TYPE_LEVEL_HIGH>;
+		clock-frequency = <100000>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+	};
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 3b26129..8e8332d 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -648,6 +648,16 @@ config I2C_PXA_SLAVE
 	  is necessary for systems where the PXA may be a target on the
 	  I2C bus.
 
+config I2C_RIIC
+	tristate "Renesas RIIC adapter"
+	depends on ARCH_SHMOBILE || COMPILE_TEST
+	help
+	  If you say yes to this option, support will be included for the
+	  Renesas RIIC I2C interface.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-riic.
+
 config HAVE_S3C2410_I2C
 	bool
 	help
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index c73eb0e..dca041b 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -63,6 +63,7 @@ obj-$(CONFIG_I2C_PNX)		+= i2c-pnx.o
 obj-$(CONFIG_I2C_PUV3)		+= i2c-puv3.o
 obj-$(CONFIG_I2C_PXA)		+= i2c-pxa.o
 obj-$(CONFIG_I2C_PXA_PCI)	+= i2c-pxa-pci.o
+obj-$(CONFIG_I2C_RIIC)		+= i2c-riic.o
 obj-$(CONFIG_I2C_S3C2410)	+= i2c-s3c2410.o
 obj-$(CONFIG_I2C_S6000)		+= i2c-s6000.o
 obj-$(CONFIG_I2C_SH7760)	+= i2c-sh7760.o
diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
new file mode 100644
index 0000000..ae0df13
--- /dev/null
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -0,0 +1,426 @@
+/*
+ * Renesas RIIC driver
+ *
+ * Copyright (C) 2013 Wolfram Sang <wsa@sang-engineering.com>
+ * Copyright (C) 2013 Renesas Solutions Corp.
+ *
+ * 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.
+ */
+
+/*
+ * This i2c core has a lot of interrupts, namely 8. We use their chaining as
+ * some kind of state machine.
+ *
+ * 1) The main xfer routine kicks off a transmission by putting the start bit
+ * (or repeated start) on the bus and enabling the transmit interrupt (TIE)
+ * since we need to send the slave address + RW bit in every case.
+ *
+ * 2) TIE sends slave address + RW bit and selects how to continue.
+ *
+ * 3a) Write case: We keep utilizing TIE as long as we have data to send. If we
+ * are done, we switch over to the transmission done interrupt (TEIE) and mark
+ * the message as completed (includes sending STOP) there.
+ *
+ * 3b) Read case: We switch over to receive interrupt (RIE). One dummy read is
+ * needed to start clocking, then we keep receiving until we are done. Note
+ * that we use the RDRFS mode all the time, i.e. we ACK/NACK every byte by
+ * writing to the ACKBT bit. I tried using the RDRFS mode only at the end of a
+ * message to create the final NACK as sketched in the datasheet. This caused
+ * some subtle races (when byte n was processed and byte n+1 was already
+ * waiting), though, and I started with the safe approach.
+ *
+ * 4) If we got a NACK somewhere, we flag the error and stop the transmission
+ * via NAKIE.
+ *
+ * Also check the comments in the interrupt routines for some gory details.
+ */
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#define RIIC_ICCR1	0x00
+#define RIIC_ICCR2	0x04
+#define RIIC_ICMR1	0x08
+#define RIIC_ICMR3	0x10
+#define RIIC_ICSER	0x18
+#define RIIC_ICIER	0x1c
+#define RIIC_ICSR2	0x24
+#define RIIC_ICBRL	0x34
+#define RIIC_ICBRH	0x38
+#define RIIC_ICDRT	0x3c
+#define RIIC_ICDRR	0x40
+
+#define ICCR1_ICE	0x80
+#define ICCR1_IICRST	0x40
+#define ICCR1_SOWP	0x10
+
+#define ICCR2_BBSY	0x80
+#define ICCR2_SP	0x08
+#define ICCR2_RS	0x04
+#define ICCR2_ST	0x02
+
+#define ICMR1_CKS_MASK	0x70
+#define ICMR1_BCWP	0x08
+#define ICMR1_CKS(_x)	((((_x) << 4) & ICMR1_CKS_MASK) | ICMR1_BCWP)
+
+#define ICMR3_RDRFS	0x20
+#define ICMR3_ACKWP	0x10
+#define ICMR3_ACKBT	0x08
+
+#define ICIER_TIE	0x80
+#define ICIER_TEIE	0x40
+#define ICIER_RIE	0x20
+#define ICIER_NAKIE	0x10
+
+#define ICSR2_NACKF	0x10
+
+/* ICBRx (@ PCLK 33MHz) */
+#define ICBR_RESERVED	0xe0 /* Should be 1 on writes */
+#define ICBRL_SP100K	(19 | ICBR_RESERVED)
+#define ICBRH_SP100K	(16 | ICBR_RESERVED)
+#define ICBRL_SP400K	(21 | ICBR_RESERVED)
+#define ICBRH_SP400K	(9 | ICBR_RESERVED)
+
+#define RIIC_INIT_MSG	-1
+
+struct riic_dev {
+	void __iomem *base;
+	u8 *buf;
+	struct i2c_msg *msg;
+	int bytes_left;
+	int err;
+	int is_last;
+	struct completion msg_done;
+	struct i2c_adapter adapter;
+	struct clk *clk;
+};
+
+struct riic_irq_desc {
+	int res_num;
+	irq_handler_t isr;
+	char *name;
+};
+
+static inline void riic_clear_set_bit(struct riic_dev *riic, u8 clear, u8 set, u8 reg)
+{
+	writeb((readb(riic->base + reg) & ~clear) | set, riic->base + reg);
+}
+
+static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
+{
+	struct riic_dev *riic = i2c_get_adapdata(adap);
+	int i, ret;
+	u8 start_bit;
+
+	ret = clk_prepare_enable(riic->clk);
+	if (ret)
+		return ret;
+
+	if (readb(riic->base + RIIC_ICCR2) & ICCR2_BBSY) {
+		riic->err = -EBUSY;
+		goto out;
+	}
+
+	reinit_completion(&riic->msg_done);
+	riic->err = 0;
+
+	writeb(0, riic->base + RIIC_ICSR2);
+
+	for (i = 0, start_bit = ICCR2_ST; i < num; i++) {
+		riic->bytes_left = RIIC_INIT_MSG;
+		riic->buf = msgs[i].buf;
+		riic->msg = &msgs[i];
+		riic->is_last = (i == num - 1);
+
+		writeb(ICIER_NAKIE | ICIER_TIE, riic->base + RIIC_ICIER);
+
+		writeb(start_bit, riic->base + RIIC_ICCR2);
+
+		ret = wait_for_completion_timeout(&riic->msg_done, riic->adapter.timeout);
+		if (ret == 0)
+			riic->err = -ETIMEDOUT;
+
+		if (riic->err)
+			break;
+
+		start_bit = ICCR2_RS;
+	}
+
+ out:
+	clk_disable_unprepare(riic->clk);
+
+	return riic->err ?: num;
+}
+
+static irqreturn_t riic_tdre_isr(int irq, void *data)
+{
+	struct riic_dev *riic = data;
+	u8 val;
+
+	if (!riic->bytes_left)
+		return IRQ_NONE;
+
+	if (riic->bytes_left == RIIC_INIT_MSG) {
+		val = !!(riic->msg->flags & I2C_M_RD);
+		if (val)
+			/* On read, switch over to receive interrupt */
+			riic_clear_set_bit(riic, ICIER_TIE, ICIER_RIE, RIIC_ICIER);
+		else
+			/* On write, initialize length */
+			riic->bytes_left = riic->msg->len;
+
+		val |= (riic->msg->addr << 1);
+	} else {
+		val = *riic->buf;
+		riic->buf++;
+		riic->bytes_left--;
+	}
+
+	/*
+	 * Switch to transmission ended interrupt when done. Do check here
+	 * after bytes_left was initialized to support SMBUS_QUICK (new msg has
+	 * 0 length then)
+	 */
+	if (riic->bytes_left == 0)
+		riic_clear_set_bit(riic, ICIER_TIE, ICIER_TEIE, RIIC_ICIER);
+
+	/*
+	 * This acks the TIE interrupt. We get another TIE immediately if our
+	 * value could be moved to the shadow shift register right away. So
+	 * this must be after updates to ICIER (where we want to disable TIE)!
+	 */
+	writeb(val, riic->base + RIIC_ICDRT);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t riic_tend_isr(int irq, void *data)
+{
+	struct riic_dev *riic = data;
+
+	if (readb(riic->base + RIIC_ICSR2) & ICSR2_NACKF) {
+		/* We got a NACKIE */
+		readb(riic->base + RIIC_ICDRR);	/* dummy read */
+		riic->err = -ENXIO;
+	} else if (riic->bytes_left) {
+		return IRQ_NONE;
+	}
+
+	if (riic->is_last || riic->err)
+		writeb(ICCR2_SP, riic->base + RIIC_ICCR2);
+
+	writeb(0, riic->base + RIIC_ICIER);
+	complete(&riic->msg_done);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t riic_rdrf_isr(int irq, void *data)
+{
+	struct riic_dev *riic = data;
+
+	if (!riic->bytes_left)
+		return IRQ_NONE;
+
+	if (riic->bytes_left == RIIC_INIT_MSG) {
+		riic->bytes_left = riic->msg->len;
+		readb(riic->base + RIIC_ICDRR);	/* dummy read */
+		return IRQ_HANDLED;
+	}
+
+	if (riic->bytes_left == 1) {
+		/* STOP must come before we set ACKBT! */
+		if (riic->is_last)
+			writeb(ICCR2_SP, riic->base + RIIC_ICCR2);
+
+		riic_clear_set_bit(riic, 0, ICMR3_ACKBT, RIIC_ICMR3);
+
+		writeb(0, riic->base + RIIC_ICIER);
+		complete(&riic->msg_done);
+	} else {
+		riic_clear_set_bit(riic, ICMR3_ACKBT, 0, RIIC_ICMR3);
+	}
+
+	/* Reading acks the RIE interrupt */
+	*riic->buf = readb(riic->base + RIIC_ICDRR);
+	riic->buf++;
+	riic->bytes_left--;
+
+	return IRQ_HANDLED;
+}
+
+static u32 riic_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm riic_algo = {
+	.master_xfer	= riic_xfer,
+	.functionality	= riic_func,
+};
+
+static int riic_init_hw(struct riic_dev *riic, u32 spd)
+{
+	int ret;
+	unsigned long rate;
+
+	ret = clk_prepare_enable(riic->clk);
+	if (ret)
+		return ret;
+
+	/*
+	 * TODO: Implement formula to calculate the timing values depending on
+	 * variable parent clock rate and arbitrary bus speed
+	 */
+	rate = clk_get_rate(riic->clk);
+	if (rate != 33325000) {
+		dev_err(&riic->adapter.dev,
+			"invalid parent clk (%lu). Must be 33325000Hz\n", rate);
+		clk_disable_unprepare(riic->clk);
+		return -EINVAL;
+	}
+
+	/* Changing the order of accessing IICRST and ICE may break things! */
+	writeb(ICCR1_IICRST | ICCR1_SOWP, riic->base + RIIC_ICCR1);
+	riic_clear_set_bit(riic, 0, ICCR1_ICE, RIIC_ICCR1);
+
+	switch (spd) {
+	case 100000:
+		writeb(ICMR1_CKS(3), riic->base + RIIC_ICMR1);
+		writeb(ICBRH_SP100K, riic->base + RIIC_ICBRH);
+		writeb(ICBRL_SP100K, riic->base + RIIC_ICBRL);
+		break;
+	case 400000:
+		writeb(ICMR1_CKS(1), riic->base + RIIC_ICMR1);
+		writeb(ICBRH_SP400K, riic->base + RIIC_ICBRH);
+		writeb(ICBRL_SP400K, riic->base + RIIC_ICBRL);
+		break;
+	default:
+		dev_err(&riic->adapter.dev,
+			"unsupported bus speed (%dHz). Use 100000 or 400000\n", spd);
+		clk_disable_unprepare(riic->clk);
+		return -EINVAL;
+	}
+
+	writeb(0, riic->base + RIIC_ICSER);
+	writeb(ICMR3_ACKWP | ICMR3_RDRFS, riic->base + RIIC_ICMR3);
+
+	riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1);
+
+	clk_disable_unprepare(riic->clk);
+
+	return 0;
+}
+
+static struct riic_irq_desc riic_irqs[] = {
+	{ .res_num = 0, .isr = riic_tend_isr, .name = "riic-tend" },
+	{ .res_num = 1, .isr = riic_rdrf_isr, .name = "riic-rdrf" },
+	{ .res_num = 2, .isr = riic_tdre_isr, .name = "riic-tdre" },
+	{ .res_num = 5, .isr = riic_tend_isr, .name = "riic-nack" },
+};
+
+static int riic_i2c_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct riic_dev *riic;
+	struct i2c_adapter *adap;
+	struct resource *res;
+	u32 bus_rate = 0;
+	int i, ret;
+
+	riic = devm_kzalloc(&pdev->dev, sizeof(*riic), GFP_KERNEL);
+	if (!riic)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	riic->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(riic->base))
+		return PTR_ERR(riic->base);
+
+	riic->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(riic->clk)) {
+		dev_err(&pdev->dev, "missing controller clock");
+		return PTR_ERR(riic->clk);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(riic_irqs); i++) {
+		res = platform_get_resource(pdev, IORESOURCE_IRQ, riic_irqs[i].res_num);
+		if (!res)
+			return -ENODEV;
+
+		ret = devm_request_irq(&pdev->dev, res->start, riic_irqs[i].isr,
+					0, riic_irqs[i].name, riic);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to request irq %s\n", riic_irqs[i].name);
+			return ret;
+		}
+	}
+
+	adap = &riic->adapter;
+	i2c_set_adapdata(adap, riic);
+	strlcpy(adap->name, "Renesas RIIC adapter", sizeof(adap->name));
+	adap->owner = THIS_MODULE;
+	adap->algo = &riic_algo;
+	adap->dev.parent = &pdev->dev;
+	adap->dev.of_node = pdev->dev.of_node;
+
+	init_completion(&riic->msg_done);
+
+	of_property_read_u32(np, "clock-frequency", &bus_rate);
+	ret = riic_init_hw(riic, bus_rate);
+	if (ret)
+		return ret;
+
+
+	ret = i2c_add_adapter(adap);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to add adapter\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, riic);
+
+	dev_info(&pdev->dev, "registered with %dHz bus speed\n", bus_rate);
+	return 0;
+}
+
+static int riic_i2c_remove(struct platform_device *pdev)
+{
+	struct riic_dev *riic = platform_get_drvdata(pdev);
+
+	writeb(0, riic->base + RIIC_ICIER);
+	i2c_del_adapter(&riic->adapter);
+
+	return 0;
+}
+
+static struct of_device_id riic_i2c_dt_ids[] = {
+	{ .compatible = "renesas,riic-rz" },
+	{ /* Sentinel */ },
+};
+
+static struct platform_driver riic_i2c_driver = {
+	.probe		= riic_i2c_probe,
+	.remove		= riic_i2c_remove,
+	.driver		= {
+		.name	= "i2c-riic",
+		.owner	= THIS_MODULE,
+		.of_match_table = riic_i2c_dt_ids,
+	},
+};
+
+module_platform_driver(riic_i2c_driver);
+
+MODULE_DESCRIPTION("Renesas RIIC adapter");
+MODULE_AUTHOR("Wolfram Sang <wsa@sang-engineering.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DEVICE_TABLE(of, riic_i2c_dt_ids);
-- 
1.8.4.2

Re: [PATCH V2 5/5] i2c: riic: add driver

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

As this patch adds DT bindings, please remember to CC the devicetree mailing 
list (which I have CC'ed on this reply).

On Wednesday 18 December 2013 22:32:01 Wolfram Sang wrote:
> From: Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
> 
> Tested with a r7s72100 genmai board acessing an eeprom.
> 
> Signed-off-by: Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
> ---
> 
> V2: fixed the name typo in the binding docs
> 
>  Documentation/devicetree/bindings/i2c/i2c-riic.txt |  29 ++
>  drivers/i2c/busses/Kconfig                         |  10 +
>  drivers/i2c/busses/Makefile                        |   1 +
>  drivers/i2c/busses/i2c-riic.c                      | 426 ++++++++++++++++++
>  4 files changed, 466 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-riic.txt
>  create mode 100644 drivers/i2c/busses/i2c-riic.c
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-riic.txt
> b/Documentation/devicetree/bindings/i2c/i2c-riic.txt new file mode 100644
> index 0000000..0bcc471
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-riic.txt
> @@ -0,0 +1,29 @@
> +Device tree configuration for Renesas RIIC driver
> +
> +Required properties:
> +- compatible      : "renesas,riic-<soctype>". "renesas,riic-rz" as fallback
> +- reg             : address start and address range size of device
> +- interrupts      : 8 interrupts (TEI, RI, TI, SPI, STI, NAKI, ALI, TMOI)
> +- clock-frequency : frequency of bus clock in Hz
> +- #address-cells  : should be <1>
> +- #size-cells     : should be <0>
> +
> +Pinctrl properties might be needed, too. See there.
> +
> +Example:
> +
> +	i2c0: i2c@fcfee000 {
> +		compatible = "renesas,riic-r7s72100", "renesas,riic-rz";
> +		reg = <0xfcfee000 0x44>;
> +		interrupts = <0 157 IRQ_TYPE_LEVEL_HIGH>,
> +			     <0 158 IRQ_TYPE_EDGE_RISING>,
> +			     <0 159 IRQ_TYPE_EDGE_RISING>,
> +			     <0 160 IRQ_TYPE_LEVEL_HIGH>,
> +			     <0 161 IRQ_TYPE_LEVEL_HIGH>,
> +			     <0 162 IRQ_TYPE_LEVEL_HIGH>,
> +			     <0 163 IRQ_TYPE_LEVEL_HIGH>,
> +			     <0 164 IRQ_TYPE_LEVEL_HIGH>;
> +		clock-frequency = <100000>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +	};
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 3b26129..8e8332d 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -648,6 +648,16 @@ config I2C_PXA_SLAVE
>  	  is necessary for systems where the PXA may be a target on the
>  	  I2C bus.
> 
> +config I2C_RIIC
> +	tristate "Renesas RIIC adapter"
> +	depends on ARCH_SHMOBILE || COMPILE_TEST
> +	help
> +	  If you say yes to this option, support will be included for the
> +	  Renesas RIIC I2C interface.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c-riic.
> +
>  config HAVE_S3C2410_I2C
>  	bool
>  	help
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index c73eb0e..dca041b 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -63,6 +63,7 @@ obj-$(CONFIG_I2C_PNX)		+= i2c-pnx.o
>  obj-$(CONFIG_I2C_PUV3)		+= i2c-puv3.o
>  obj-$(CONFIG_I2C_PXA)		+= i2c-pxa.o
>  obj-$(CONFIG_I2C_PXA_PCI)	+= i2c-pxa-pci.o
> +obj-$(CONFIG_I2C_RIIC)		+= i2c-riic.o
>  obj-$(CONFIG_I2C_S3C2410)	+= i2c-s3c2410.o
>  obj-$(CONFIG_I2C_S6000)		+= i2c-s6000.o
>  obj-$(CONFIG_I2C_SH7760)	+= i2c-sh7760.o
> diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
> new file mode 100644
> index 0000000..ae0df13
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-riic.c
> @@ -0,0 +1,426 @@
> +/*
> + * Renesas RIIC driver
> + *
> + * Copyright (C) 2013 Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
> + * Copyright (C) 2013 Renesas Solutions Corp.
> + *
> + * 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.
> + */
> +
> +/*
> + * This i2c core has a lot of interrupts, namely 8. We use their chaining
> as
> + * some kind of state machine.

I have mixed feelings about this. Wouldn't it be more efficient to have an 
internal state machine (which you partially have already, using RIIC_INIT_MSG 
for instance) instead of relying on enabling/disabling interrupts ? The latter 
has a larger overhead.

> + * 1) The main xfer routine kicks off a transmission by putting the start
> bit
> + * (or repeated start) on the bus and enabling the transmit interrupt (TIE)
> + * since we need to send the slave address + RW bit in every case.
> + *
> + * 2) TIE sends slave address + RW bit and selects how to continue.
> + *
> + * 3a) Write case: We keep utilizing TIE as long as we have data to send.
> If we
> + * are done, we switch over to the transmission done interrupt (TEIE) and
> mark
> + * the message as completed (includes sending STOP) there.
> + *
> + * 3b) Read case: We switch over to receive interrupt (RIE). One dummy read
> is
> + * needed to start clocking, then we keep receiving until we are done. Note
> + * that we use the RDRFS mode all the time, i.e. we ACK/NACK every byte by
> + * writing to the ACKBT bit. I tried using the RDRFS mode only at the end
> of a
> + * message to create the final NACK as sketched in the datasheet. This
> caused
> + * some subtle races (when byte n was processed and byte n+1 was already
> + * waiting), though, and I started with the safe approach.
> + *
> + * 4) If we got a NACK somewhere, we flag the error and stop the
> transmission
> + * via NAKIE.
> + *
> + * Also check the comments in the interrupt routines for some gory details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +#define RIIC_ICCR1	0x00
> +#define RIIC_ICCR2	0x04
> +#define RIIC_ICMR1	0x08
> +#define RIIC_ICMR3	0x10
> +#define RIIC_ICSER	0x18
> +#define RIIC_ICIER	0x1c
> +#define RIIC_ICSR2	0x24
> +#define RIIC_ICBRL	0x34
> +#define RIIC_ICBRH	0x38
> +#define RIIC_ICDRT	0x3c
> +#define RIIC_ICDRR	0x40
> +
> +#define ICCR1_ICE	0x80
> +#define ICCR1_IICRST	0x40
> +#define ICCR1_SOWP	0x10
> +
> +#define ICCR2_BBSY	0x80
> +#define ICCR2_SP	0x08
> +#define ICCR2_RS	0x04
> +#define ICCR2_ST	0x02
> +
> +#define ICMR1_CKS_MASK	0x70
> +#define ICMR1_BCWP	0x08
> +#define ICMR1_CKS(_x)	((((_x) << 4) & ICMR1_CKS_MASK) | ICMR1_BCWP)
> +
> +#define ICMR3_RDRFS	0x20
> +#define ICMR3_ACKWP	0x10
> +#define ICMR3_ACKBT	0x08
> +
> +#define ICIER_TIE	0x80
> +#define ICIER_TEIE	0x40
> +#define ICIER_RIE	0x20
> +#define ICIER_NAKIE	0x10
> +
> +#define ICSR2_NACKF	0x10
> +
> +/* ICBRx (@ PCLK 33MHz) */
> +#define ICBR_RESERVED	0xe0 /* Should be 1 on writes */
> +#define ICBRL_SP100K	(19 | ICBR_RESERVED)
> +#define ICBRH_SP100K	(16 | ICBR_RESERVED)
> +#define ICBRL_SP400K	(21 | ICBR_RESERVED)
> +#define ICBRH_SP400K	(9 | ICBR_RESERVED)
> +
> +#define RIIC_INIT_MSG	-1
> +
> +struct riic_dev {
> +	void __iomem *base;
> +	u8 *buf;
> +	struct i2c_msg *msg;
> +	int bytes_left;
> +	int err;
> +	int is_last;
> +	struct completion msg_done;
> +	struct i2c_adapter adapter;
> +	struct clk *clk;
> +};
> +
> +struct riic_irq_desc {
> +	int res_num;
> +	irq_handler_t isr;
> +	char *name;
> +};
> +
> +static inline void riic_clear_set_bit(struct riic_dev *riic, u8 clear, u8
> set, u8 reg)
> +{
> +	writeb((readb(riic->base + reg) & ~clear) | set, riic->base + reg);
> +}
> +
> +static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int
> num)
> +{
> +	struct riic_dev *riic = i2c_get_adapdata(adap);
> +	int i, ret;

One of my favorite bikeshedding comments is to ask for unsigned int when the 
variable can't be negative :-)

> +	u8 start_bit;
> +
> +	ret = clk_prepare_enable(riic->clk);
> +	if (ret)
> +		return ret;
> +
> +	if (readb(riic->base + RIIC_ICCR2) & ICCR2_BBSY) {
> +		riic->err = -EBUSY;
> +		goto out;
> +	}
> +
> +	reinit_completion(&riic->msg_done);
> +	riic->err = 0;
> +
> +	writeb(0, riic->base + RIIC_ICSR2);
> +
> +	for (i = 0, start_bit = ICCR2_ST; i < num; i++) {
> +		riic->bytes_left = RIIC_INIT_MSG;
> +		riic->buf = msgs[i].buf;
> +		riic->msg = &msgs[i];
> +		riic->is_last = (i == num - 1);
> +
> +		writeb(ICIER_NAKIE | ICIER_TIE, riic->base + RIIC_ICIER);
> +
> +		writeb(start_bit, riic->base + RIIC_ICCR2);
> +
> +		ret = wait_for_completion_timeout(&riic->msg_done,
> riic->adapter.timeout);
> +		if (ret == 0)
> +			riic->err = -ETIMEDOUT;
> +
> +		if (riic->err)
> +			break;
> +
> +		start_bit = ICCR2_RS;
> +	}
> +
> + out:
> +	clk_disable_unprepare(riic->clk);
> +
> +	return riic->err ?: num;
> +}
> +
> +static irqreturn_t riic_tdre_isr(int irq, void *data)
> +{
> +	struct riic_dev *riic = data;
> +	u8 val;
> +
> +	if (!riic->bytes_left)
> +		return IRQ_NONE;
> +
> +	if (riic->bytes_left == RIIC_INIT_MSG) {
> +		val = !!(riic->msg->flags & I2C_M_RD);
> +		if (val)
> +			/* On read, switch over to receive interrupt */
> +			riic_clear_set_bit(riic, ICIER_TIE, ICIER_RIE, RIIC_ICIER);
> +		else
> +			/* On write, initialize length */
> +			riic->bytes_left = riic->msg->len;
> +
> +		val |= (riic->msg->addr << 1);
> +	} else {
> +		val = *riic->buf;
> +		riic->buf++;
> +		riic->bytes_left--;
> +	}
> +
> +	/*
> +	 * Switch to transmission ended interrupt when done. Do check here
> +	 * after bytes_left was initialized to support SMBUS_QUICK (new msg has
> +	 * 0 length then)
> +	 */
> +	if (riic->bytes_left == 0)
> +		riic_clear_set_bit(riic, ICIER_TIE, ICIER_TEIE, RIIC_ICIER);
> +
> +	/*
> +	 * This acks the TIE interrupt. We get another TIE immediately if our
> +	 * value could be moved to the shadow shift register right away. So
> +	 * this must be after updates to ICIER (where we want to disable TIE)!
> +	 */
> +	writeb(val, riic->base + RIIC_ICDRT);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t riic_tend_isr(int irq, void *data)
> +{
> +	struct riic_dev *riic = data;
> +
> +	if (readb(riic->base + RIIC_ICSR2) & ICSR2_NACKF) {
> +		/* We got a NACKIE */
> +		readb(riic->base + RIIC_ICDRR);	/* dummy read */
> +		riic->err = -ENXIO;
> +	} else if (riic->bytes_left) {
> +		return IRQ_NONE;
> +	}
> +
> +	if (riic->is_last || riic->err)
> +		writeb(ICCR2_SP, riic->base + RIIC_ICCR2);
> +
> +	writeb(0, riic->base + RIIC_ICIER);
> +	complete(&riic->msg_done);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t riic_rdrf_isr(int irq, void *data)
> +{
> +	struct riic_dev *riic = data;
> +
> +	if (!riic->bytes_left)
> +		return IRQ_NONE;
> +
> +	if (riic->bytes_left == RIIC_INIT_MSG) {
> +		riic->bytes_left = riic->msg->len;
> +		readb(riic->base + RIIC_ICDRR);	/* dummy read */
> +		return IRQ_HANDLED;
> +	}
> +
> +	if (riic->bytes_left == 1) {
> +		/* STOP must come before we set ACKBT! */
> +		if (riic->is_last)
> +			writeb(ICCR2_SP, riic->base + RIIC_ICCR2);
> +
> +		riic_clear_set_bit(riic, 0, ICMR3_ACKBT, RIIC_ICMR3);
> +
> +		writeb(0, riic->base + RIIC_ICIER);
> +		complete(&riic->msg_done);
> +	} else {
> +		riic_clear_set_bit(riic, ICMR3_ACKBT, 0, RIIC_ICMR3);
> +	}
> +
> +	/* Reading acks the RIE interrupt */
> +	*riic->buf = readb(riic->base + RIIC_ICDRR);
> +	riic->buf++;
> +	riic->bytes_left--;
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static u32 riic_func(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static const struct i2c_algorithm riic_algo = {
> +	.master_xfer	= riic_xfer,
> +	.functionality	= riic_func,
> +};
> +
> +static int riic_init_hw(struct riic_dev *riic, u32 spd)
> +{
> +	int ret;
> +	unsigned long rate;
> +
> +	ret = clk_prepare_enable(riic->clk);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * TODO: Implement formula to calculate the timing values depending on
> +	 * variable parent clock rate and arbitrary bus speed
> +	 */
> +	rate = clk_get_rate(riic->clk);
> +	if (rate != 33325000) {
> +		dev_err(&riic->adapter.dev,
> +			"invalid parent clk (%lu). Must be 33325000Hz\n", rate);

What about a "goto done;" here and below to avoid repeating the 
clk_disable_unprepare() call ?

> +		clk_disable_unprepare(riic->clk);
> +		return -EINVAL;
> +	}
> +
> +	/* Changing the order of accessing IICRST and ICE may break things! */
> +	writeb(ICCR1_IICRST | ICCR1_SOWP, riic->base + RIIC_ICCR1);
> +	riic_clear_set_bit(riic, 0, ICCR1_ICE, RIIC_ICCR1);
> +
> +	switch (spd) {
> +	case 100000:
> +		writeb(ICMR1_CKS(3), riic->base + RIIC_ICMR1);
> +		writeb(ICBRH_SP100K, riic->base + RIIC_ICBRH);
> +		writeb(ICBRL_SP100K, riic->base + RIIC_ICBRL);
> +		break;
> +	case 400000:
> +		writeb(ICMR1_CKS(1), riic->base + RIIC_ICMR1);
> +		writeb(ICBRH_SP400K, riic->base + RIIC_ICBRH);
> +		writeb(ICBRL_SP400K, riic->base + RIIC_ICBRL);

Couldn't you compute the ICMR1, ICBRH and ICBRL values at runtime instead ?

> +		break;
> +	default:
> +		dev_err(&riic->adapter.dev,
> +			"unsupported bus speed (%dHz). Use 100000 or 400000\n", spd);
> +		clk_disable_unprepare(riic->clk);
> +		return -EINVAL;
> +	}
> +
> +	writeb(0, riic->base + RIIC_ICSER);
> +	writeb(ICMR3_ACKWP | ICMR3_RDRFS, riic->base + RIIC_ICMR3);
> +
> +	riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1);
> +

... with

done:

here, and a return ret.

> +	clk_disable_unprepare(riic->clk);
> +
> +	return 0;
> +}
> +
> +static struct riic_irq_desc riic_irqs[] = {
> +	{ .res_num = 0, .isr = riic_tend_isr, .name = "riic-tend" },
> +	{ .res_num = 1, .isr = riic_rdrf_isr, .name = "riic-rdrf" },
> +	{ .res_num = 2, .isr = riic_tdre_isr, .name = "riic-tdre" },
> +	{ .res_num = 5, .isr = riic_tend_isr, .name = "riic-nack" },
> +};
> +
> +static int riic_i2c_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct riic_dev *riic;
> +	struct i2c_adapter *adap;
> +	struct resource *res;
> +	u32 bus_rate = 0;
> +	int i, ret;
> +
> +	riic = devm_kzalloc(&pdev->dev, sizeof(*riic), GFP_KERNEL);
> +	if (!riic)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	riic->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(riic->base))
> +		return PTR_ERR(riic->base);
> +
> +	riic->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(riic->clk)) {
> +		dev_err(&pdev->dev, "missing controller clock");
> +		return PTR_ERR(riic->clk);
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(riic_irqs); i++) {
> +		res = platform_get_resource(pdev, IORESOURCE_IRQ, 
riic_irqs[i].res_num);
> +		if (!res)
> +			return -ENODEV;
> +
> +		ret = devm_request_irq(&pdev->dev, res->start, riic_irqs[i].isr,
> +					0, riic_irqs[i].name, riic);
> +		if (ret) {
> +			dev_err(&pdev->dev, "failed to request irq %s\n", 
riic_irqs[i].name);
> +			return ret;
> +		}
> +	}
> +
> +	adap = &riic->adapter;
> +	i2c_set_adapdata(adap, riic);
> +	strlcpy(adap->name, "Renesas RIIC adapter", sizeof(adap->name));
> +	adap->owner = THIS_MODULE;
> +	adap->algo = &riic_algo;
> +	adap->dev.parent = &pdev->dev;
> +	adap->dev.of_node = pdev->dev.of_node;
> +
> +	init_completion(&riic->msg_done);
> +
> +	of_property_read_u32(np, "clock-frequency", &bus_rate);

As the property is mandatory, shouldn't you check the return value of this 
function ? Another option would be to make the clock-frequency property 
optional and use a default value. What do the other I2C bus drivers usually do 
?

> +	ret = riic_init_hw(riic, bus_rate);
> +	if (ret)
> +		return ret;
> +
> +
> +	ret = i2c_add_adapter(adap);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to add adapter\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, riic);
> +
> +	dev_info(&pdev->dev, "registered with %dHz bus speed\n", bus_rate);
> +	return 0;
> +}
> +
> +static int riic_i2c_remove(struct platform_device *pdev)
> +{
> +	struct riic_dev *riic = platform_get_drvdata(pdev);
> +
> +	writeb(0, riic->base + RIIC_ICIER);
> +	i2c_del_adapter(&riic->adapter);
> +
> +	return 0;
> +}
> +
> +static struct of_device_id riic_i2c_dt_ids[] = {
> +	{ .compatible = "renesas,riic-rz" },
> +	{ /* Sentinel */ },
> +};
> +
> +static struct platform_driver riic_i2c_driver = {
> +	.probe		= riic_i2c_probe,
> +	.remove		= riic_i2c_remove,
> +	.driver		= {
> +		.name	= "i2c-riic",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = riic_i2c_dt_ids,
> +	},
> +};
> +
> +module_platform_driver(riic_i2c_driver);
> +
> +MODULE_DESCRIPTION("Renesas RIIC adapter");
> +MODULE_AUTHOR("Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DEVICE_TABLE(of, riic_i2c_dt_ids);
-- 
Regards,

Laurent Pinchart

Re: [PATCH V2 5/5] i2c: riic: add driver

[Wolfram Sang]

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

Hi Laurent,

thanks for the review!


> > +/*
> > + * This i2c core has a lot of interrupts, namely 8. We use their chaining
> > as
> > + * some kind of state machine.
> 
> I have mixed feelings about this. Wouldn't it be more efficient to have an 
> internal state machine (which you partially have already, using RIIC_INIT_MSG 
> for instance) instead of relying on enabling/disabling interrupts ? The latter 
> has a larger overhead.

I am not sure I get you here. I need the interrupts anyhow. For example,
after the last byte has been written to the 1-byte-FIFO in the
transmission_irq, I need to wait for the transmission_end_irq to ensure
the bits are already on the wire before I mark the message completed.

Polling for that condition is more overhead than just enabling the
proper interrupt (one write to ICIER). I don't need to switch ISR since
all the interrupts are seperate and have dedicated ISR.

> > +static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int
> > num)
> > +{
> > +	struct riic_dev *riic = i2c_get_adapdata(adap);
> > +	int i, ret;
> 
> One of my favorite bikeshedding comments is to ask for unsigned int when the 
> variable can't be negative :-)

OK.

> > +	/*
> > +	 * TODO: Implement formula to calculate the timing values depending on
> > +	 * variable parent clock rate and arbitrary bus speed
> > +	 */
> > +	rate = clk_get_rate(riic->clk);
> > +	if (rate != 33325000) {
> > +		dev_err(&riic->adapter.dev,
> > +			"invalid parent clk (%lu). Must be 33325000Hz\n", rate);
> 
> What about a "goto done;" here and below to avoid repeating the 
> clk_disable_unprepare() call ?

Yeah, can be argued that way. I was fine with both.

> 
> > +		clk_disable_unprepare(riic->clk);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Changing the order of accessing IICRST and ICE may break things! */
> > +	writeb(ICCR1_IICRST | ICCR1_SOWP, riic->base + RIIC_ICCR1);
> > +	riic_clear_set_bit(riic, 0, ICCR1_ICE, RIIC_ICCR1);
> > +
> > +	switch (spd) {
> > +	case 100000:
> > +		writeb(ICMR1_CKS(3), riic->base + RIIC_ICMR1);
> > +		writeb(ICBRH_SP100K, riic->base + RIIC_ICBRH);
> > +		writeb(ICBRL_SP100K, riic->base + RIIC_ICBRL);
> > +		break;
> > +	case 400000:
> > +		writeb(ICMR1_CKS(1), riic->base + RIIC_ICMR1);
> > +		writeb(ICBRH_SP400K, riic->base + RIIC_ICBRH);
> > +		writeb(ICBRL_SP400K, riic->base + RIIC_ICBRL);
> 
> Couldn't you compute the ICMR1, ICBRH and ICBRL values at runtime instead ?

As mentioned in the TODO above, this is scheduled for an incremental
update to this driver.

> > +	of_property_read_u32(np, "clock-frequency", &bus_rate);
> 
> As the property is mandatory, shouldn't you check the return value of this 
> function ? Another option would be to make the clock-frequency property 
> optional and use a default value. What do the other I2C bus drivers usually do 
> ?

bus_rate is initialized to 0 and if read_u32 fails, it will stay this
way. Then, the call to riic_init_hw() will fail and report the error.

There is no standard behaviour (use sane default or fail) yet. It is
somewhere on the I2C todo list :/

Regards,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V2 5/5] i2c: riic: add driver

[Laurent Pinchart]

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

Hi Wolfram,

On Thursday 19 December 2013 13:06:47 Wolfram Sang wrote:
> Hi Laurent,
> 
> thanks for the review!
> 
> > > +/*
> > > + * This i2c core has a lot of interrupts, namely 8. We use their
> > > chaining as some kind of state machine.
> > 
> > I have mixed feelings about this. Wouldn't it be more efficient to have an
> > internal state machine (which you partially have already, using
> > RIIC_INIT_MSG for instance) instead of relying on enabling/disabling
> > interrupts ? The latter has a larger overhead.
> 
> I am not sure I get you here. I need the interrupts anyhow. For example,
> after the last byte has been written to the 1-byte-FIFO in the
> transmission_irq, I need to wait for the transmission_end_irq to ensure the
> bits are already on the wire before I mark the message completed.
> 
> Polling for that condition is more overhead than just enabling the proper
> interrupt (one write to ICIER). I don't need to switch ISR since all the
> interrupts are seperate and have dedicated ISR.

I haven't expressed myself clearly. Polling is of course a bad option. My 
point was that I understood your comment as meaning that you enable and 
disable interrupts at runtime and use that as a state machine, while I was 
wondering whether it wouldn't be simpler to keep all interrupts enabled at all 
time and handle the synchronization explicitly.

Please scratch the comment about the larger overhead though, that was a 
mistake due to reading the code too fast.

> > > +static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> > > int num)
> > > +{
> > > +	struct riic_dev *riic = i2c_get_adapdata(adap);
> > > +	int i, ret;
> > 
> > One of my favorite bikeshedding comments is to ask for unsigned int when
> > the variable can't be negative :-)
> 
> OK.
> 
> > > +	/*
> > > +	 * TODO: Implement formula to calculate the timing values depending 
on
> > > +	 * variable parent clock rate and arbitrary bus speed
> > > +	 */
> > > +	rate = clk_get_rate(riic->clk);
> > > +	if (rate != 33325000) {
> > > +		dev_err(&riic->adapter.dev,
> > > +			"invalid parent clk (%lu). Must be 33325000Hz\n", rate);
> > 
> > What about a "goto done;" here and below to avoid repeating the
> > clk_disable_unprepare() call ?
> 
> Yeah, can be argued that way. I was fine with both.
> 
> > > +		clk_disable_unprepare(riic->clk);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	/* Changing the order of accessing IICRST and ICE may break things! 
*/
> > > +	writeb(ICCR1_IICRST | ICCR1_SOWP, riic->base + RIIC_ICCR1);
> > > +	riic_clear_set_bit(riic, 0, ICCR1_ICE, RIIC_ICCR1);
> > > +
> > > +	switch (spd) {
> > > +	case 100000:
> > > +		writeb(ICMR1_CKS(3), riic->base + RIIC_ICMR1);
> > > +		writeb(ICBRH_SP100K, riic->base + RIIC_ICBRH);
> > > +		writeb(ICBRL_SP100K, riic->base + RIIC_ICBRL);
> > > +		break;
> > > +	case 400000:
> > > +		writeb(ICMR1_CKS(1), riic->base + RIIC_ICMR1);
> > > +		writeb(ICBRH_SP400K, riic->base + RIIC_ICBRH);
> > > +		writeb(ICBRL_SP400K, riic->base + RIIC_ICBRL);
> > 
> > Couldn't you compute the ICMR1, ICBRH and ICBRL values at runtime instead
> > ?
> 
> As mentioned in the TODO above, this is scheduled for an incremental
> update to this driver.

Nice :-)

> > > +	of_property_read_u32(np, "clock-frequency", &bus_rate);
> > 
> > As the property is mandatory, shouldn't you check the return value of this
> > function ? Another option would be to make the clock-frequency property
> > optional and use a default value. What do the other I2C bus drivers
> > usually do ?
> 
> bus_rate is initialized to 0 and if read_u32 fails, it will stay this
> way. Then, the call to riic_init_hw() will fail and report the error.

That's the part I wasn't sure to like, but it will be reworked when making 
clock speed computation dynamic anyway, so we can keep it as-is for now.

> There is no standard behaviour (use sane default or fail) yet. It is
> somewhere on the I2C todo list :/

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH V2 5/5] i2c: riic: add driver

[Wolfram Sang]

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


> disable interrupts at runtime and use that as a state machine, while I was 
> wondering whether it wouldn't be simpler to keep all interrupts enabled at all 
> time and handle the synchronization explicitly.

If I do this, I'll get another "spurious" interrupt telling me the transmit
register is empty again, even after I wrote the last byte. I don't like
this.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]