[PATCH 0/5] enable I2C on Renesas EMEV2 and KZM9D board

[PATCH 0/5] enable I2C on Renesas EMEV2 and KZM9D board

[Wolfram Sang]

So, I picked up the old series from Ian and reworked it significantly. While
his series was already an improvement from the driver found in the BSP, it was
still not ready for upstream. It should be now :) My idea was to start simple
and improve incrementally, so e.g. clock handling was broken, so clocks are
always on for starters. Major changes include:

* clock handling simplified and adapted to current EMEV clock handling
* switch from wait_event to completion mechanism. The old one was basically
  some kind of completion with custom code around wait_events
* dropped using signals; they always cause trouble with I2C
* simplified the bus free logic. If it turns out to be too simple, we'd need
  to switch to the bus_recovery infrastructure probably. But we'd need a test
  case for that
* error handling improved to match subsystem standards
* removed some BSP angst code
* lots of refactoring to make code simpler, more readable...
* bugfixes

I also added all the glue code to add clocks, disable reset etc which was
missing before. I'd prefer to take the first three patches via i2c myself,
and the latter two via Simon's tree.

A branch for testing can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/emma-i2c

Please test, comment...

Thanks!

   Wolfram


Wolfram Sang (5):
  clk: shmobile: emev2: deassert reset for IIC0/1
  i2c: emev2: add binding documentation
  i2c: emev2: add driver
  ARM: shmobile: emev2: add IIC cores to dtsi
  ARM: shmobile: emev2: kzm9d: enable IIC busses

 .../devicetree/bindings/i2c/i2c-emev2.txt          |  22 ++
 arch/arm/boot/dts/emev2-kzm9d.dts                  |   8 +
 arch/arm/boot/dts/emev2.dtsi                       |  48 +++
 drivers/clk/shmobile/clk-emev2.c                   |   6 +
 drivers/i2c/busses/Kconfig                         |   7 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-emev2.c                     | 334 +++++++++++++++++++++
 7 files changed, 426 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-emev2.txt
 create mode 100644 drivers/i2c/busses/i2c-emev2.c

-- 
2.1.4

[PATCH 0/5] enable I2C on Renesas EMEV2 and KZM9D board

[Wolfram Sang]

So, I picked up the old series from Ian and reworked it significantly. While
his series was already an improvement from the driver found in the BSP, it was
still not ready for upstream. It should be now :) My idea was to start simple
and improve incrementally, so e.g. clock handling was broken, so clocks are
always on for starters. Major changes include:

* clock handling simplified and adapted to current EMEV clock handling
* switch from wait_event to completion mechanism. The old one was basically
  some kind of completion with custom code around wait_events
* dropped using signals; they always cause trouble with I2C
* simplified the bus free logic. If it turns out to be too simple, we'd need
  to switch to the bus_recovery infrastructure probably. But we'd need a test
  case for that
* error handling improved to match subsystem standards
* removed some BSP angst code
* lots of refactoring to make code simpler, more readable...
* bugfixes

I also added all the glue code to add clocks, disable reset etc which was
missing before. I'd prefer to take the first three patches via i2c myself,
and the latter two via Simon's tree.

A branch for testing can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/emma-i2c

Please test, comment...

Thanks!

   Wolfram


Wolfram Sang (5):
  clk: shmobile: emev2: deassert reset for IIC0/1
  i2c: emev2: add binding documentation
  i2c: emev2: add driver
  ARM: shmobile: emev2: add IIC cores to dtsi
  ARM: shmobile: emev2: kzm9d: enable IIC busses

 .../devicetree/bindings/i2c/i2c-emev2.txt          |  22 ++
 arch/arm/boot/dts/emev2-kzm9d.dts                  |   8 +
 arch/arm/boot/dts/emev2.dtsi                       |  48 +++
 drivers/clk/shmobile/clk-emev2.c                   |   6 +
 drivers/i2c/busses/Kconfig                         |   7 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-emev2.c                     | 334 +++++++++++++++++++++
 7 files changed, 426 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-emev2.txt
 create mode 100644 drivers/i2c/busses/i2c-emev2.c

-- 
2.1.4

[PATCH 1/5] clk: shmobile: emev2: deassert reset for IIC0/1

[Wolfram Sang]

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

We have a driver now for IIC, so disable reset for them.

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

If possible, I'd like to take this patch via the I2C tree because the driver
will spit WARNs if the dependency to this patch is not met. OK?

 drivers/clk/shmobile/clk-emev2.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/clk/shmobile/clk-emev2.c b/drivers/clk/shmobile/clk-emev2.c
index 5b60beb7d0ebce..a91825471c79ac 100644
--- a/drivers/clk/shmobile/clk-emev2.c
+++ b/drivers/clk/shmobile/clk-emev2.c
@@ -28,6 +28,8 @@
 #define USIBU1_RSTCTRL 0x0ac
 #define USIBU2_RSTCTRL 0x0b0
 #define USIBU3_RSTCTRL 0x0b4
+#define IIC0_RSTCTRL 0x0dc
+#define IIC1_RSTCTRL 0x0e0
 #define STI_RSTCTRL 0x124
 #define STI_CLKSEL 0x688
 
@@ -66,6 +68,10 @@ static void __init emev2_smu_init(void)
 	emev2_smu_write(2, USIBU1_RSTCTRL);
 	emev2_smu_write(2, USIBU2_RSTCTRL);
 	emev2_smu_write(2, USIBU3_RSTCTRL);
+
+	/* deassert reset for IIC0->IIC1 */
+	emev2_smu_write(1, IIC0_RSTCTRL);
+	emev2_smu_write(1, IIC1_RSTCTRL);
 }
 
 static void __init emev2_smu_clkdiv_init(struct device_node *np)
-- 
2.1.4

[PATCH 1/5] clk: shmobile: emev2: deassert reset for IIC0/1

[Wolfram Sang]

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

We have a driver now for IIC, so disable reset for them.

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

If possible, I'd like to take this patch via the I2C tree because the driver
will spit WARNs if the dependency to this patch is not met. OK?

 drivers/clk/shmobile/clk-emev2.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/clk/shmobile/clk-emev2.c b/drivers/clk/shmobile/clk-emev2.c
index 5b60beb7d0ebce..a91825471c79ac 100644
--- a/drivers/clk/shmobile/clk-emev2.c
+++ b/drivers/clk/shmobile/clk-emev2.c
@@ -28,6 +28,8 @@
 #define USIBU1_RSTCTRL 0x0ac
 #define USIBU2_RSTCTRL 0x0b0
 #define USIBU3_RSTCTRL 0x0b4
+#define IIC0_RSTCTRL 0x0dc
+#define IIC1_RSTCTRL 0x0e0
 #define STI_RSTCTRL 0x124
 #define STI_CLKSEL 0x688
 
@@ -66,6 +68,10 @@ static void __init emev2_smu_init(void)
 	emev2_smu_write(2, USIBU1_RSTCTRL);
 	emev2_smu_write(2, USIBU2_RSTCTRL);
 	emev2_smu_write(2, USIBU3_RSTCTRL);
+
+	/* deassert reset for IIC0->IIC1 */
+	emev2_smu_write(1, IIC0_RSTCTRL);
+	emev2_smu_write(1, IIC1_RSTCTRL);
 }
 
 static void __init emev2_smu_clkdiv_init(struct device_node *np)
-- 
2.1.4

Re: [PATCH 1/5] clk: shmobile: emev2: deassert reset for IIC0/1

[Stephen Boyd]

On 07/06, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> We have a driver now for IIC, so disable reset for them.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---

Acked-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/5] clk: shmobile: emev2: deassert reset for IIC0/1

[Stephen Boyd]

On 07/06, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> We have a driver now for IIC, so disable reset for them.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---

Acked-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/5] i2c: emev2: add binding documentation

[Wolfram Sang]

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

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 .../devicetree/bindings/i2c/i2c-emev2.txt          | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-emev2.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-emev2.txt b/Documentation/devicetree/bindings/i2c/i2c-emev2.txt
new file mode 100644
index 00000000000000..a6740066835056
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-emev2.txt
@@ -0,0 +1,22 @@
+Device tree configuration for Renesas EMEV2 IIC driver
+
+Required properties:
+- compatible      : "renesas,iic-<soctype>". "renesas,iic-emev2" as fallback
+- reg             : address start and address range size of device
+- interrupts      : 1 interrupt
+- clocks          : phandle to the IP core SCLK
+- clock-names     : must be "sclk"
+- #address-cells  : should be <1>
+- #size-cells     : should be <0>
+
+Example:
+
+	iic0: i2c@e0070000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "renesas,iic-emev2";
+		reg = <0xe0070000 0x28>;
+		interrupts = <0 32 IRQ_TYPE_EDGE_RISING>;
+		clocks = <&iic0_sclk>;
+		clock-names = "sclk";
+	};
-- 
2.1.4

[PATCH 2/5] i2c: emev2: add binding documentation

[Wolfram Sang]

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

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 .../devicetree/bindings/i2c/i2c-emev2.txt          | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-emev2.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-emev2.txt b/Documentation/devicetree/bindings/i2c/i2c-emev2.txt
new file mode 100644
index 00000000000000..a6740066835056
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-emev2.txt
@@ -0,0 +1,22 @@
+Device tree configuration for Renesas EMEV2 IIC driver
+
+Required properties:
+- compatible      : "renesas,iic-<soctype>". "renesas,iic-emev2" as fallback
+- reg             : address start and address range size of device
+- interrupts      : 1 interrupt
+- clocks          : phandle to the IP core SCLK
+- clock-names     : must be "sclk"
+- #address-cells  : should be <1>
+- #size-cells     : should be <0>
+
+Example:
+
+	iic0: i2c@e0070000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "renesas,iic-emev2";
+		reg = <0xe0070000 0x28>;
+		interrupts = <0 32 IRQ_TYPE_EDGE_RISING>;
+		clocks = <&iic0_sclk>;
+		clock-names = "sclk";
+	};
-- 
2.1.4

Re: [PATCH 2/5] i2c: emev2: add binding documentation

[Geert Uytterhoeven]

Hi Wolfram,

On Mon, Jul 6, 2015 at 11:46 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-emev2.txt b/Documentation/devicetree/bindings/i2c/i2c-emev2.txt
> new file mode 100644
> index 00000000000000..a6740066835056
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-emev2.txt
> @@ -0,0 +1,22 @@
> +Device tree configuration for Renesas EMEV2 IIC driver
> +
> +Required properties:
> +- compatible      : "renesas,iic-<soctype>". "renesas,iic-emev2" as fallback

What's the "<soctype>"? Isn't that "emev2"?

> +- reg             : address start and address range size of device
> +- interrupts      : 1 interrupt
> +- clocks          : phandle to the IP core SCLK
> +- clock-names     : must be "sclk"
> +- #address-cells  : should be <1>
> +- #size-cells     : should be <0>
> +
> +Example:
> +
> +       iic0: i2c@e0070000 {
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +               compatible = "renesas,iic-emev2";
> +               reg = <0xe0070000 0x28>;
> +               interrupts = <0 32 IRQ_TYPE_EDGE_RISING>;

Are you sure about rising edge? All other emev2 interrupts are
IRQ_TYPE_LEVEL_HIGH.

> +               clocks = <&iic0_sclk>;
> +               clock-names = "sclk";
> +       };


Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH 2/5] i2c: emev2: add binding documentation

[Geert Uytterhoeven]

Hi Wolfram,

On Mon, Jul 6, 2015 at 11:46 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-emev2.txt b/Documentation/devicetree/bindings/i2c/i2c-emev2.txt
> new file mode 100644
> index 00000000000000..a6740066835056
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-emev2.txt
> @@ -0,0 +1,22 @@
> +Device tree configuration for Renesas EMEV2 IIC driver
> +
> +Required properties:
> +- compatible      : "renesas,iic-<soctype>". "renesas,iic-emev2" as fallback

What's the "<soctype>"? Isn't that "emev2"?

> +- reg             : address start and address range size of device
> +- interrupts      : 1 interrupt
> +- clocks          : phandle to the IP core SCLK
> +- clock-names     : must be "sclk"
> +- #address-cells  : should be <1>
> +- #size-cells     : should be <0>
> +
> +Example:
> +
> +       iic0: i2c@e0070000 {
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +               compatible = "renesas,iic-emev2";
> +               reg = <0xe0070000 0x28>;
> +               interrupts = <0 32 IRQ_TYPE_EDGE_RISING>;

Are you sure about rising edge? All other emev2 interrupts are
IRQ_TYPE_LEVEL_HIGH.

> +               clocks = <&iic0_sclk>;
> +               clock-names = "sclk";
> +       };


Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH 2/5] i2c: emev2: add binding documentation

[Wolfram Sang]

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

On Tue, Jul 07, 2015 at 10:41:37AM +0200, Geert Uytterhoeven wrote:
> Hi Wolfram,
> 
> On Mon, Jul 6, 2015 at 11:46 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> > diff --git a/Documentation/devicetree/bindings/i2c/i2c-emev2.txt b/Documentation/devicetree/bindings/i2c/i2c-emev2.txt
> > new file mode 100644
> > index 00000000000000..a6740066835056
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/i2c/i2c-emev2.txt
> > @@ -0,0 +1,22 @@
> > +Device tree configuration for Renesas EMEV2 IIC driver
> > +
> > +Required properties:
> > +- compatible      : "renesas,iic-<soctype>". "renesas,iic-emev2" as fallback
> 
> What's the "<soctype>"? Isn't that "emev2"?

Hmm, I though emev2 was the family but looking again I might be wrong.
But it is still confusing to me. EM (EMMA Mobile) is the family and EV2
is the SoC? I should probably just skip the <soctype> thing above, then
it should be good.

> > +               interrupts = <0 32 IRQ_TYPE_EDGE_RISING>;
> 
> Are you sure about rising edge? All other emev2 interrupts are
> IRQ_TYPE_LEVEL_HIGH.

Yes. R19UH00356EJ0800_1chip.pdf, chapter 7.1.1

64
 IIC0
 IIC0_INT
 IIC #0 interrupt
 Edge
65
 IIC1
 IIC1_INT
 IIC #1 interrupt
 Edge

Thanks,

   Wolfram


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

Re: [PATCH 2/5] i2c: emev2: add binding documentation

[Wolfram Sang]

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

On Tue, Jul 07, 2015 at 10:41:37AM +0200, Geert Uytterhoeven wrote:
> Hi Wolfram,
> 
> On Mon, Jul 6, 2015 at 11:46 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> > diff --git a/Documentation/devicetree/bindings/i2c/i2c-emev2.txt b/Documentation/devicetree/bindings/i2c/i2c-emev2.txt
> > new file mode 100644
> > index 00000000000000..a6740066835056
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/i2c/i2c-emev2.txt
> > @@ -0,0 +1,22 @@
> > +Device tree configuration for Renesas EMEV2 IIC driver
> > +
> > +Required properties:
> > +- compatible      : "renesas,iic-<soctype>". "renesas,iic-emev2" as fallback
> 
> What's the "<soctype>"? Isn't that "emev2"?

Hmm, I though emev2 was the family but looking again I might be wrong.
But it is still confusing to me. EM (EMMA Mobile) is the family and EV2
is the SoC? I should probably just skip the <soctype> thing above, then
it should be good.

> > +               interrupts = <0 32 IRQ_TYPE_EDGE_RISING>;
> 
> Are you sure about rising edge? All other emev2 interrupts are
> IRQ_TYPE_LEVEL_HIGH.

Yes. R19UH00356EJ0800_1chip.pdf, chapter 7.1.1

64
 IIC0
 IIC0_INT
 IIC #0 interrupt
 Edge
65
 IIC1
 IIC1_INT
 IIC #1 interrupt
 Edge

Thanks,

   Wolfram


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

Re: [PATCH 2/5] i2c: emev2: add binding documentation

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, Jul 7, 2015 at 11:17 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> > +               interrupts = <0 32 IRQ_TYPE_EDGE_RISING>;
>>
>> Are you sure about rising edge? All other emev2 interrupts are
>> IRQ_TYPE_LEVEL_HIGH.
>
> Yes. R19UH00356EJ0800_1chip.pdf, chapter 7.1.1
>
> 64
>  IIC0
>  IIC0_INT
>  IIC #0 interrupt
>  Edge
> 65
>  IIC1
>  IIC1_INT
>  IIC #1 interrupt
>  Edge

OK, verified using R19UH0036EJ1600_1chip.pdf (which is newer, Google is
your friend).

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH 2/5] i2c: emev2: add binding documentation

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, Jul 7, 2015 at 11:17 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> > +               interrupts = <0 32 IRQ_TYPE_EDGE_RISING>;
>>
>> Are you sure about rising edge? All other emev2 interrupts are
>> IRQ_TYPE_LEVEL_HIGH.
>
> Yes. R19UH00356EJ0800_1chip.pdf, chapter 7.1.1
>
> 64
>  IIC0
>  IIC0_INT
>  IIC #0 interrupt
>  Edge
> 65
>  IIC1
>  IIC1_INT
>  IIC #1 interrupt
>  Edge

OK, verified using R19UH0036EJ1600_1chip.pdf (which is newer, Google is
your friend).

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH 2/5] i2c: emev2: add binding documentation

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Monday 06 July 2015 23:46:06 Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  .../devicetree/bindings/i2c/i2c-emev2.txt          | 22 ++++++++++++++++++
>  1 file changed, 22 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-emev2.txt
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-emev2.txt
> b/Documentation/devicetree/bindings/i2c/i2c-emev2.txt new file mode 100644
> index 00000000000000..a6740066835056
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-emev2.txt
> @@ -0,0 +1,22 @@
> +Device tree configuration for Renesas EMEV2 IIC driver

s/driver/controller/

> +
> +Required properties:
> +- compatible      : "renesas,iic-<soctype>". "renesas,iic-emev2" as
> fallback
> +- reg             : address start and address range size of device
> +- interrupts      : 1 interrupt

I'd write this as "specifier for the IIC controller interrupt".

> +- clocks          : phandle to the IP core SCLK

The datasheet mentions PCLK and IIC_SCLK but doesn't provide details. Do you 
have more information ?

> +- clock-names     : must be "sclk"
> +- #address-cells  : should be <1>
> +- #size-cells     : should be <0>
> +
> +Example:
> +
> +	iic0: i2c@e0070000 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		compatible = "renesas,iic-emev2";
> +		reg = <0xe0070000 0x28>;
> +		interrupts = <0 32 IRQ_TYPE_EDGE_RISING>;
> +		clocks = <&iic0_sclk>;
> +		clock-names = "sclk";
> +	};

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] i2c: emev2: add binding documentation

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Monday 06 July 2015 23:46:06 Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  .../devicetree/bindings/i2c/i2c-emev2.txt          | 22 ++++++++++++++++++
>  1 file changed, 22 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-emev2.txt
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-emev2.txt
> b/Documentation/devicetree/bindings/i2c/i2c-emev2.txt new file mode 100644
> index 00000000000000..a6740066835056
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-emev2.txt
> @@ -0,0 +1,22 @@
> +Device tree configuration for Renesas EMEV2 IIC driver

s/driver/controller/

> +
> +Required properties:
> +- compatible      : "renesas,iic-<soctype>". "renesas,iic-emev2" as
> fallback
> +- reg             : address start and address range size of device
> +- interrupts      : 1 interrupt

I'd write this as "specifier for the IIC controller interrupt".

> +- clocks          : phandle to the IP core SCLK

The datasheet mentions PCLK and IIC_SCLK but doesn't provide details. Do you 
have more information ?

> +- clock-names     : must be "sclk"
> +- #address-cells  : should be <1>
> +- #size-cells     : should be <0>
> +
> +Example:
> +
> +	iic0: i2c@e0070000 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		compatible = "renesas,iic-emev2";
> +		reg = <0xe0070000 0x28>;
> +		interrupts = <0 32 IRQ_TYPE_EDGE_RISING>;
> +		clocks = <&iic0_sclk>;
> +		clock-names = "sclk";
> +	};

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] i2c: emev2: add binding documentation

[Wolfram Sang]

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

> > +Device tree configuration for Renesas EMEV2 IIC driver
> 
> s/driver/controller/

Yes,

> > +- interrupts      : 1 interrupt
> 
> I'd write this as "specifier for the IIC controller interrupt".

Yes.

> 
> > +- clocks          : phandle to the IP core SCLK
> 
> The datasheet mentions PCLK and IIC_SCLK but doesn't provide details. Do you 
> have more information ?

I follow what is done for the other EMEV2 IP cores currently supported.
All cores I checked have PCLK and SCLK but only SCLK is referenced in
the DTS.


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

Re: [PATCH 2/5] i2c: emev2: add binding documentation

[Wolfram Sang]

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

> > +Device tree configuration for Renesas EMEV2 IIC driver
> 
> s/driver/controller/

Yes,

> > +- interrupts      : 1 interrupt
> 
> I'd write this as "specifier for the IIC controller interrupt".

Yes.

> 
> > +- clocks          : phandle to the IP core SCLK
> 
> The datasheet mentions PCLK and IIC_SCLK but doesn't provide details. Do you 
> have more information ?

I follow what is done for the other EMEV2 IP cores currently supported.
All cores I checked have PCLK and SCLK but only SCLK is referenced in
the DTS.


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

Re: [PATCH 2/5] i2c: emev2: add binding documentation

[Laurent Pinchart]

Hi Wolfram,

On Tuesday 07 July 2015 21:43:13 Wolfram Sang wrote:
> > > +Device tree configuration for Renesas EMEV2 IIC driver
> > 
> > s/driver/controller/
> 
> Yes,
> 
> > > +- interrupts      : 1 interrupt
> > 
> > I'd write this as "specifier for the IIC controller interrupt".
> 
> Yes.
> 
> > > +- clocks          : phandle to the IP core SCLK
> > 
> > The datasheet mentions PCLK and IIC_SCLK but doesn't provide details. Do
> > you have more information ?
> 
> I follow what is done for the other EMEV2 IP cores currently supported.
> All cores I checked have PCLK and SCLK but only SCLK is referenced in
> the DTS.

Could you briefly explain what the two clocks are for ? Or point me to the 
right documentation ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] i2c: emev2: add binding documentation

[Laurent Pinchart]

Hi Wolfram,

On Tuesday 07 July 2015 21:43:13 Wolfram Sang wrote:
> > > +Device tree configuration for Renesas EMEV2 IIC driver
> > 
> > s/driver/controller/
> 
> Yes,
> 
> > > +- interrupts      : 1 interrupt
> > 
> > I'd write this as "specifier for the IIC controller interrupt".
> 
> Yes.
> 
> > > +- clocks          : phandle to the IP core SCLK
> > 
> > The datasheet mentions PCLK and IIC_SCLK but doesn't provide details. Do
> > you have more information ?
> 
> I follow what is done for the other EMEV2 IP cores currently supported.
> All cores I checked have PCLK and SCLK but only SCLK is referenced in
> the DTS.

Could you briefly explain what the two clocks are for ? Or point me to the 
right documentation ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] i2c: emev2: add binding documentation

[Wolfram Sang]

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


> > I follow what is done for the other EMEV2 IP cores currently supported.
> > All cores I checked have PCLK and SCLK but only SCLK is referenced in
> > the DTS.
> 
> Could you briefly explain what the two clocks are for ? Or point me to the 
> right documentation ?

PCLK is the APB clock. See R19UH0037EJ1200_SMU.pdf, chapter 4.13.


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

Re: [PATCH 2/5] i2c: emev2: add binding documentation

[Wolfram Sang]

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


> > I follow what is done for the other EMEV2 IP cores currently supported.
> > All cores I checked have PCLK and SCLK but only SCLK is referenced in
> > the DTS.
> 
> Could you briefly explain what the two clocks are for ? Or point me to the 
> right documentation ?

PCLK is the APB clock. See R19UH0037EJ1200_SMU.pdf, chapter 4.13.


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

Re: [PATCH 2/5] i2c: emev2: add binding documentation

[Laurent Pinchart]

On Tuesday 07 July 2015 21:53:13 Wolfram Sang wrote:
> > > I follow what is done for the other EMEV2 IP cores currently supported.
> > > All cores I checked have PCLK and SCLK but only SCLK is referenced in
> > > the DTS.
> > 
> > Could you briefly explain what the two clocks are for ? Or point me to the
> > right documentation ?
> 
> PCLK is the APB clock. See R19UH0037EJ1200_SMU.pdf, chapter 4.13.

Is that the functional clock, while SCLK is the clock used to control serial 
communication ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] i2c: emev2: add binding documentation

[Laurent Pinchart]

On Tuesday 07 July 2015 21:53:13 Wolfram Sang wrote:
> > > I follow what is done for the other EMEV2 IP cores currently supported.
> > > All cores I checked have PCLK and SCLK but only SCLK is referenced in
> > > the DTS.
> > 
> > Could you briefly explain what the two clocks are for ? Or point me to the
> > right documentation ?
> 
> PCLK is the APB clock. See R19UH0037EJ1200_SMU.pdf, chapter 4.13.

Is that the functional clock, while SCLK is the clock used to control serial 
communication ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] i2c: emev2: add binding documentation

[Wolfram Sang]

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


> Is that the functional clock, while SCLK is the clock used to control serial 
> communication ?

Yes, that's how I read it.


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

Re: [PATCH 2/5] i2c: emev2: add binding documentation

[Wolfram Sang]

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


> Is that the functional clock, while SCLK is the clock used to control serial 
> communication ?

Yes, that's how I read it.


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

[PATCH 4/5] ARM: shmobile: emev2: add IIC cores to dtsi

[Wolfram Sang]

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

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 arch/arm/boot/dts/emev2.dtsi | 48 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/arch/arm/boot/dts/emev2.dtsi b/arch/arm/boot/dts/emev2.dtsi
index bb45694d91bc1e..edad0c4eea3505 100644
--- a/arch/arm/boot/dts/emev2.dtsi
+++ b/arch/arm/boot/dts/emev2.dtsi
@@ -21,6 +21,8 @@
 		gpio2 = &gpio2;
 		gpio3 = &gpio3;
 		gpio4 = &gpio4;
+		i2c0 = &iic0;
+		i2c1 = &iic1;
 	};
 
 	cpus {
@@ -66,6 +68,30 @@
 			clock-frequency = <32768>;
 			#clock-cells = <0>;
 		};
+		iic0_sclkdiv: iic0_sclkdiv {
+			compatible = "renesas,emev2-smu-clkdiv";
+			reg = <0x624 0>;
+			clocks = <&pll3_fo>;
+			#clock-cells = <0>;
+		};
+		iic0_sclk: iic0_sclk {
+			compatible = "renesas,emev2-smu-gclk";
+			reg = <0x48c 1>;
+			clocks = <&iic0_sclkdiv>;
+			#clock-cells = <0>;
+		};
+		iic1_sclkdiv: iic1_sclkdiv {
+			compatible = "renesas,emev2-smu-clkdiv";
+			reg = <0x624 16>;
+			clocks = <&pll3_fo>;
+			#clock-cells = <0>;
+		};
+		iic1_sclk: iic1_sclk {
+			compatible = "renesas,emev2-smu-gclk";
+			reg = <0x490 1>;
+			clocks = <&iic1_sclkdiv>;
+			#clock-cells = <0>;
+		};
 		pll3_fo: pll3_fo {
 			compatible = "fixed-factor-clock";
 			clocks = <&c32ki>;
@@ -234,4 +260,26 @@
 		interrupt-controller;
 		#interrupt-cells = <2>;
 	};
+
+	iic0: i2c@e0070000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "renesas,iic-emev2";
+		reg = <0xe0070000 0x28>;
+		interrupts = <0 32 IRQ_TYPE_EDGE_RISING>;
+		clocks = <&iic0_sclk>;
+		clock-names = "sclk";
+		status = "disabled";
+	};
+
+	iic1: i2c@e10a0000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "renesas,iic-emev2";
+		reg = <0xe10a0000 0x28>;
+		interrupts = <0 33 IRQ_TYPE_EDGE_RISING>;
+		clocks = <&iic1_sclk>;
+		clock-names = "sclk";
+		status = "disabled";
+	};
 };
-- 
2.1.4

[PATCH 4/5] ARM: shmobile: emev2: add IIC cores to dtsi

[Wolfram Sang]

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

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 arch/arm/boot/dts/emev2.dtsi | 48 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/arch/arm/boot/dts/emev2.dtsi b/arch/arm/boot/dts/emev2.dtsi
index bb45694d91bc1e..edad0c4eea3505 100644
--- a/arch/arm/boot/dts/emev2.dtsi
+++ b/arch/arm/boot/dts/emev2.dtsi
@@ -21,6 +21,8 @@
 		gpio2 = &gpio2;
 		gpio3 = &gpio3;
 		gpio4 = &gpio4;
+		i2c0 = &iic0;
+		i2c1 = &iic1;
 	};
 
 	cpus {
@@ -66,6 +68,30 @@
 			clock-frequency = <32768>;
 			#clock-cells = <0>;
 		};
+		iic0_sclkdiv: iic0_sclkdiv {
+			compatible = "renesas,emev2-smu-clkdiv";
+			reg = <0x624 0>;
+			clocks = <&pll3_fo>;
+			#clock-cells = <0>;
+		};
+		iic0_sclk: iic0_sclk {
+			compatible = "renesas,emev2-smu-gclk";
+			reg = <0x48c 1>;
+			clocks = <&iic0_sclkdiv>;
+			#clock-cells = <0>;
+		};
+		iic1_sclkdiv: iic1_sclkdiv {
+			compatible = "renesas,emev2-smu-clkdiv";
+			reg = <0x624 16>;
+			clocks = <&pll3_fo>;
+			#clock-cells = <0>;
+		};
+		iic1_sclk: iic1_sclk {
+			compatible = "renesas,emev2-smu-gclk";
+			reg = <0x490 1>;
+			clocks = <&iic1_sclkdiv>;
+			#clock-cells = <0>;
+		};
 		pll3_fo: pll3_fo {
 			compatible = "fixed-factor-clock";
 			clocks = <&c32ki>;
@@ -234,4 +260,26 @@
 		interrupt-controller;
 		#interrupt-cells = <2>;
 	};
+
+	iic0: i2c@e0070000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "renesas,iic-emev2";
+		reg = <0xe0070000 0x28>;
+		interrupts = <0 32 IRQ_TYPE_EDGE_RISING>;
+		clocks = <&iic0_sclk>;
+		clock-names = "sclk";
+		status = "disabled";
+	};
+
+	iic1: i2c@e10a0000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "renesas,iic-emev2";
+		reg = <0xe10a0000 0x28>;
+		interrupts = <0 33 IRQ_TYPE_EDGE_RISING>;
+		clocks = <&iic1_sclk>;
+		clock-names = "sclk";
+		status = "disabled";
+	};
 };
-- 
2.1.4

[PATCH 3/5] i2c: emev2: add driver

[Wolfram Sang]

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

Add a basic driver for the Renesas EMEV2 SoC. Based on the driver from
the BSP which was first worked on by Ian, and made ready for upstream by
me.

Signed-off-by: Ian Molton <ian.molton@codethink.co.uk>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/Kconfig     |   7 +
 drivers/i2c/busses/Makefile    |   1 +
 drivers/i2c/busses/i2c-emev2.c | 334 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 342 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-emev2.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 2255af23b9c70e..503abb64861f37 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -516,6 +516,13 @@ config I2C_EG20T
 	  ML7213/ML7223/ML7831 is companion chip for Intel Atom E6xx series.
 	  ML7213/ML7223/ML7831 is completely compatible for Intel EG20T PCH.
 
+config I2C_EMEV2
+	tristate "EMMA Mobile series I2C adapter"
+	depends on HAVE_CLK
+	help
+	  If you say yes to this option, support will be included for the
+	  I2C interface on the Renesas Electronics EM/EV family of processors.
+
 config I2C_EXYNOS5
 	tristate "Exynos5 high-speed I2C driver"
 	depends on ARCH_EXYNOS && OF
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index cdf941da91c65e..73f1145da5b4c8 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -48,6 +48,7 @@ i2c-designware-pci-objs := i2c-designware-pcidrv.o
 obj-$(CONFIG_I2C_DIGICOLOR)	+= i2c-digicolor.o
 obj-$(CONFIG_I2C_EFM32)		+= i2c-efm32.o
 obj-$(CONFIG_I2C_EG20T)		+= i2c-eg20t.o
+obj-$(CONFIG_I2C_EMEV2)		+= i2c-emev2.o
 obj-$(CONFIG_I2C_EXYNOS5)	+= i2c-exynos5.o
 obj-$(CONFIG_I2C_GPIO)		+= i2c-gpio.o
 obj-$(CONFIG_I2C_HIGHLANDER)	+= i2c-highlander.o
diff --git a/drivers/i2c/busses/i2c-emev2.c b/drivers/i2c/busses/i2c-emev2.c
new file mode 100644
index 00000000000000..baa88fc5ff05e5
--- /dev/null
+++ b/drivers/i2c/busses/i2c-emev2.c
@@ -0,0 +1,334 @@
+/*
+ * I2C driver for the Renesas EMEV2 SoC
+ *
+ * Copyright (C) 2015 Wolfram Sang <wsa@sang-engineering.com>
+ * Copyright 2013 Codethink Ltd.
+ * Copyright 2010-2015 Renesas Electronics Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/sched.h>
+
+/* I2C Registers */
+#define I2C_OFS_IICACT0		0x00	/* start */
+#define I2C_OFS_IIC0		0x04	/* shift */
+#define I2C_OFS_IICC0		0x08	/* control */
+#define I2C_OFS_SVA0		0x0c	/* slave address */
+#define I2C_OFS_IICCL0		0x10	/* clock select */
+#define I2C_OFS_IICX0		0x14	/* extension */
+#define I2C_OFS_IICS0		0x18	/* status */
+#define I2C_OFS_IICSE0		0x1c	/* status For emulation */
+#define I2C_OFS_IICF0		0x20	/* IIC flag */
+
+/* I2C IICACT0 Masks */
+#define I2C_BIT_IICE0		0x0001
+
+/* I2C IICC0 Masks */
+#define I2C_BIT_LREL0		0x0040
+#define I2C_BIT_WREL0		0x0020
+#define I2C_BIT_SPIE0		0x0010
+#define I2C_BIT_WTIM0		0x0008
+#define I2C_BIT_ACKE0		0x0004
+#define I2C_BIT_STT0		0x0002
+#define I2C_BIT_SPT0		0x0001
+
+/* I2C IICCL0 Masks */
+#define I2C_BIT_SMC0		0x0008
+#define I2C_BIT_DFC0		0x0004
+
+/* I2C IICSE0 Masks */
+#define I2C_BIT_MSTS0		0x0080
+#define I2C_BIT_ALD0		0x0040
+#define I2C_BIT_EXC0		0x0020
+#define I2C_BIT_COI0		0x0010
+#define I2C_BIT_TRC0		0x0008
+#define I2C_BIT_ACKD0		0x0004
+#define I2C_BIT_STD0		0x0002
+#define I2C_BIT_SPD0		0x0001
+
+/* I2C IICF0 Masks */
+#define I2C_BIT_STCF		0x0080
+#define I2C_BIT_IICBSY		0x0040
+#define I2C_BIT_STCEN		0x0002
+#define I2C_BIT_IICRSV		0x0001
+
+struct em_i2c_device {
+	void __iomem *base;
+	struct i2c_adapter adap;
+	struct completion msg_done;
+	struct clk *sclk;
+};
+
+static inline void em_clear_set_bit(struct em_i2c_device *priv, u8 clear, u8 set, u8 reg)
+{
+	writeb((readb(priv->base + reg) & ~clear) | set, priv->base + reg);
+}
+
+static int em_i2c_wait_for_event(struct em_i2c_device *priv)
+{
+	unsigned long time_left;
+	int status;
+
+	reinit_completion(&priv->msg_done);
+
+	time_left = wait_for_completion_timeout(&priv->msg_done, priv->adap.timeout);
+
+	if (!time_left)
+		return -ETIMEDOUT;
+
+	status = readb(priv->base + I2C_OFS_IICSE0);
+	return status & I2C_BIT_ALD0 ? -EAGAIN : status;
+}
+
+static void em_i2c_stop(struct em_i2c_device *priv)
+{
+	/* Send Stop condition */
+	em_clear_set_bit(priv, 0, I2C_BIT_SPT0 | I2C_BIT_SPIE0, I2C_OFS_IICC0);
+
+	/* Wait for stop condition */
+	em_i2c_wait_for_event(priv);
+}
+
+static void em_i2c_reset(struct i2c_adapter *adap)
+{
+	struct em_i2c_device *priv = i2c_get_adapdata(adap);
+	int retr;
+
+	/* If I2C active */
+	if (readb(priv->base + I2C_OFS_IICACT0) & I2C_BIT_IICE0) {
+		/* Disable I2C operation */
+		writeb(0, priv->base + I2C_OFS_IICACT0);
+
+		retr = 1000;
+		while (readb(priv->base + I2C_OFS_IICACT0) = 1 && retr)
+			retr--;
+		WARN_ON(retr = 0);
+	}
+
+	/* Transfer mode set */
+	writeb(I2C_BIT_DFC0, priv->base + I2C_OFS_IICCL0);
+
+	/* Can Issue start without detecting a stop, Reservation disabled. */
+	writeb(I2C_BIT_STCEN | I2C_BIT_IICRSV, priv->base + I2C_OFS_IICF0);
+
+	/* I2C enable, 9 bit interrupt mode */
+	writeb(I2C_BIT_WTIM0, priv->base + I2C_OFS_IICC0);
+
+	/* Enable I2C operation */
+	writeb(I2C_BIT_IICE0, priv->base + I2C_OFS_IICACT0);
+
+	retr = 1000;
+	while (readb(priv->base + I2C_OFS_IICACT0) = 0 && retr)
+		retr--;
+	WARN_ON(retr = 0);
+}
+
+static int __em_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msg,
+				int stop)
+{
+	struct em_i2c_device *priv = i2c_get_adapdata(adap);
+	int count, status;
+
+	/* Send start condition */
+	em_clear_set_bit(priv, 0, I2C_BIT_ACKE0 | I2C_BIT_WTIM0, I2C_OFS_IICC0);
+	em_clear_set_bit(priv, 0, I2C_BIT_STT0, I2C_OFS_IICC0);
+
+	/* Send slave address and R/W type */
+	writeb((msg->addr << 1) | ((msg->flags & I2C_M_RD) ? 1 : 0),
+		priv->base + I2C_OFS_IIC0);
+
+	/* Wait for transaction */
+	status = em_i2c_wait_for_event(priv);
+	if (status < 0)
+		goto out_reset;
+
+	/* Received NACK (result of setting slave address and R/W) */
+	if (!(status & I2C_BIT_ACKD0)) {
+		em_i2c_stop(priv);
+		goto out;
+	}
+
+	/* Extra setup for read transactions */
+	if (!(status & I2C_BIT_TRC0)) {
+		/* 8 bit interrupt mode */
+		em_clear_set_bit(priv, I2C_BIT_WTIM0, I2C_BIT_ACKE0, I2C_OFS_IICC0);
+		em_clear_set_bit(priv, I2C_BIT_WTIM0, I2C_BIT_WREL0, I2C_OFS_IICC0);
+
+		/* Wait for transaction */
+		status = em_i2c_wait_for_event(priv);
+		if (status < 0)
+			goto out_reset;
+	}
+
+	/* Send / receive data */
+	for (count = 0; count < msg->len; count++) {
+		if (!(status & I2C_BIT_TRC0)) { /* Read transaction */
+			msg->buf[count] = readb(priv->base + I2C_OFS_IIC0);
+			em_clear_set_bit(priv, 0, I2C_BIT_WREL0, I2C_OFS_IICC0);
+
+		} else { /* Write transaction */
+			/* Received NACK */
+			if (!(status & I2C_BIT_ACKD0)) {
+				em_i2c_stop(priv);
+				goto out;
+			}
+
+			/* Write data */
+			writeb(msg->buf[count], priv->base + I2C_OFS_IIC0);
+		}
+
+		/* Wait for R/W transaction */
+		status = em_i2c_wait_for_event(priv);
+		if (status < 0)
+			goto out_reset;
+	}
+
+	if (stop)
+		em_i2c_stop(priv);
+
+	return count;
+
+out_reset:
+	em_i2c_reset(adap);
+out:
+	return status < 0 ? status : -ENXIO;
+}
+
+static int em_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
+	int num)
+{
+	struct em_i2c_device *priv = i2c_get_adapdata(adap);
+	int ret, i;
+
+	if (readb(priv->base + I2C_OFS_IICF0) & I2C_BIT_IICBSY)
+		return -EAGAIN;
+
+	for (i = 0; i < num; i++) {
+		ret = __em_i2c_xfer(adap, &msgs[i], (i = (num - 1)));
+		if (ret < 0)
+			return ret;
+	}
+
+	/* I2C transfer completed */
+	return num;
+}
+
+static irqreturn_t em_i2c_irq_handler(int this_irq, void *dev_id)
+{
+	struct em_i2c_device *priv = dev_id;
+
+	complete(&priv->msg_done);
+	return IRQ_HANDLED;
+}
+
+static u32 em_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static struct i2c_algorithm em_i2c_algo = {
+	.master_xfer = em_i2c_xfer,
+	.functionality = em_i2c_func,
+};
+
+static int em_i2c_probe(struct platform_device *pdev)
+{
+	struct em_i2c_device *priv;
+	struct resource *r;
+	int irq, ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(struct em_i2c_device), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	strlcpy(priv->adap.name, "EMEV2 I2C", sizeof(priv->adap.name));
+
+	priv->sclk = devm_clk_get(&pdev->dev, "sclk");
+	if (IS_ERR(priv->sclk))
+		return PTR_ERR(priv->sclk);
+
+	clk_prepare_enable(priv->sclk);
+
+	irq = platform_get_irq(pdev, 0);
+	priv->adap.timeout = msecs_to_jiffies(100);
+	priv->adap.retries = 5;
+	priv->adap.dev.parent = &pdev->dev;
+	priv->adap.algo = &em_i2c_algo;
+	priv->adap.owner = THIS_MODULE;
+	priv->adap.dev.of_node = pdev->dev.of_node;
+
+	init_completion(&priv->msg_done);
+
+	platform_set_drvdata(pdev, priv);
+	i2c_set_adapdata(&priv->adap, priv);
+
+	em_i2c_reset(&priv->adap);
+
+	ret = devm_request_irq(&pdev->dev, irq, em_i2c_irq_handler, 0,
+				"em_i2c", priv);
+	if (ret)
+		goto exit_clk;
+
+	ret = i2c_add_adapter(&priv->adap);
+
+	if (ret)
+		goto exit_clk;
+
+	dev_info(&pdev->dev, "Added i2c controller %d irq %d @ 0x%p\n",
+		priv->adap.nr, irq, priv->base);
+
+	return 0;
+
+exit_clk:
+	clk_disable_unprepare(priv->sclk);
+	return ret;
+}
+
+static int em_i2c_remove(struct platform_device *dev)
+{
+	struct em_i2c_device *priv = platform_get_drvdata(dev);
+
+	i2c_del_adapter(&priv->adap);
+	clk_disable_unprepare(priv->sclk);
+
+	return 0;
+}
+
+static const struct of_device_id em_i2c_ids[] = {
+	{ .compatible = "renesas,iic-emev2", },
+	{ }
+};
+
+static struct platform_driver em_i2c_driver = {
+	.probe = em_i2c_probe,
+	.remove = em_i2c_remove,
+	.driver = {
+		.name = "em-i2c",
+		.of_match_table = em_i2c_ids,
+	}
+};
+module_platform_driver(em_i2c_driver);
+
+MODULE_DESCRIPTION("EMEV2 I2C bus driver");
+MODULE_AUTHOR("Ian Molton and Wolfram Sang <wsa@sang-engineering.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DEVICE_TABLE(of, em_i2c_ids);
-- 
2.1.4

[PATCH 3/5] i2c: emev2: add driver

[Wolfram Sang]

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

Add a basic driver for the Renesas EMEV2 SoC. Based on the driver from
the BSP which was first worked on by Ian, and made ready for upstream by
me.

Signed-off-by: Ian Molton <ian.molton-4yDnlxn2s6sWdaTGBSpHTA@public.gmane.org>
Signed-off-by: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
---
 drivers/i2c/busses/Kconfig     |   7 +
 drivers/i2c/busses/Makefile    |   1 +
 drivers/i2c/busses/i2c-emev2.c | 334 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 342 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-emev2.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 2255af23b9c70e..503abb64861f37 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -516,6 +516,13 @@ config I2C_EG20T
 	  ML7213/ML7223/ML7831 is companion chip for Intel Atom E6xx series.
 	  ML7213/ML7223/ML7831 is completely compatible for Intel EG20T PCH.
 
+config I2C_EMEV2
+	tristate "EMMA Mobile series I2C adapter"
+	depends on HAVE_CLK
+	help
+	  If you say yes to this option, support will be included for the
+	  I2C interface on the Renesas Electronics EM/EV family of processors.
+
 config I2C_EXYNOS5
 	tristate "Exynos5 high-speed I2C driver"
 	depends on ARCH_EXYNOS && OF
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index cdf941da91c65e..73f1145da5b4c8 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -48,6 +48,7 @@ i2c-designware-pci-objs := i2c-designware-pcidrv.o
 obj-$(CONFIG_I2C_DIGICOLOR)	+= i2c-digicolor.o
 obj-$(CONFIG_I2C_EFM32)		+= i2c-efm32.o
 obj-$(CONFIG_I2C_EG20T)		+= i2c-eg20t.o
+obj-$(CONFIG_I2C_EMEV2)		+= i2c-emev2.o
 obj-$(CONFIG_I2C_EXYNOS5)	+= i2c-exynos5.o
 obj-$(CONFIG_I2C_GPIO)		+= i2c-gpio.o
 obj-$(CONFIG_I2C_HIGHLANDER)	+= i2c-highlander.o
diff --git a/drivers/i2c/busses/i2c-emev2.c b/drivers/i2c/busses/i2c-emev2.c
new file mode 100644
index 00000000000000..baa88fc5ff05e5
--- /dev/null
+++ b/drivers/i2c/busses/i2c-emev2.c
@@ -0,0 +1,334 @@
+/*
+ * I2C driver for the Renesas EMEV2 SoC
+ *
+ * Copyright (C) 2015 Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
+ * Copyright 2013 Codethink Ltd.
+ * Copyright 2010-2015 Renesas Electronics Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/sched.h>
+
+/* I2C Registers */
+#define I2C_OFS_IICACT0		0x00	/* start */
+#define I2C_OFS_IIC0		0x04	/* shift */
+#define I2C_OFS_IICC0		0x08	/* control */
+#define I2C_OFS_SVA0		0x0c	/* slave address */
+#define I2C_OFS_IICCL0		0x10	/* clock select */
+#define I2C_OFS_IICX0		0x14	/* extension */
+#define I2C_OFS_IICS0		0x18	/* status */
+#define I2C_OFS_IICSE0		0x1c	/* status For emulation */
+#define I2C_OFS_IICF0		0x20	/* IIC flag */
+
+/* I2C IICACT0 Masks */
+#define I2C_BIT_IICE0		0x0001
+
+/* I2C IICC0 Masks */
+#define I2C_BIT_LREL0		0x0040
+#define I2C_BIT_WREL0		0x0020
+#define I2C_BIT_SPIE0		0x0010
+#define I2C_BIT_WTIM0		0x0008
+#define I2C_BIT_ACKE0		0x0004
+#define I2C_BIT_STT0		0x0002
+#define I2C_BIT_SPT0		0x0001
+
+/* I2C IICCL0 Masks */
+#define I2C_BIT_SMC0		0x0008
+#define I2C_BIT_DFC0		0x0004
+
+/* I2C IICSE0 Masks */
+#define I2C_BIT_MSTS0		0x0080
+#define I2C_BIT_ALD0		0x0040
+#define I2C_BIT_EXC0		0x0020
+#define I2C_BIT_COI0		0x0010
+#define I2C_BIT_TRC0		0x0008
+#define I2C_BIT_ACKD0		0x0004
+#define I2C_BIT_STD0		0x0002
+#define I2C_BIT_SPD0		0x0001
+
+/* I2C IICF0 Masks */
+#define I2C_BIT_STCF		0x0080
+#define I2C_BIT_IICBSY		0x0040
+#define I2C_BIT_STCEN		0x0002
+#define I2C_BIT_IICRSV		0x0001
+
+struct em_i2c_device {
+	void __iomem *base;
+	struct i2c_adapter adap;
+	struct completion msg_done;
+	struct clk *sclk;
+};
+
+static inline void em_clear_set_bit(struct em_i2c_device *priv, u8 clear, u8 set, u8 reg)
+{
+	writeb((readb(priv->base + reg) & ~clear) | set, priv->base + reg);
+}
+
+static int em_i2c_wait_for_event(struct em_i2c_device *priv)
+{
+	unsigned long time_left;
+	int status;
+
+	reinit_completion(&priv->msg_done);
+
+	time_left = wait_for_completion_timeout(&priv->msg_done, priv->adap.timeout);
+
+	if (!time_left)
+		return -ETIMEDOUT;
+
+	status = readb(priv->base + I2C_OFS_IICSE0);
+	return status & I2C_BIT_ALD0 ? -EAGAIN : status;
+}
+
+static void em_i2c_stop(struct em_i2c_device *priv)
+{
+	/* Send Stop condition */
+	em_clear_set_bit(priv, 0, I2C_BIT_SPT0 | I2C_BIT_SPIE0, I2C_OFS_IICC0);
+
+	/* Wait for stop condition */
+	em_i2c_wait_for_event(priv);
+}
+
+static void em_i2c_reset(struct i2c_adapter *adap)
+{
+	struct em_i2c_device *priv = i2c_get_adapdata(adap);
+	int retr;
+
+	/* If I2C active */
+	if (readb(priv->base + I2C_OFS_IICACT0) & I2C_BIT_IICE0) {
+		/* Disable I2C operation */
+		writeb(0, priv->base + I2C_OFS_IICACT0);
+
+		retr = 1000;
+		while (readb(priv->base + I2C_OFS_IICACT0) == 1 && retr)
+			retr--;
+		WARN_ON(retr == 0);
+	}
+
+	/* Transfer mode set */
+	writeb(I2C_BIT_DFC0, priv->base + I2C_OFS_IICCL0);
+
+	/* Can Issue start without detecting a stop, Reservation disabled. */
+	writeb(I2C_BIT_STCEN | I2C_BIT_IICRSV, priv->base + I2C_OFS_IICF0);
+
+	/* I2C enable, 9 bit interrupt mode */
+	writeb(I2C_BIT_WTIM0, priv->base + I2C_OFS_IICC0);
+
+	/* Enable I2C operation */
+	writeb(I2C_BIT_IICE0, priv->base + I2C_OFS_IICACT0);
+
+	retr = 1000;
+	while (readb(priv->base + I2C_OFS_IICACT0) == 0 && retr)
+		retr--;
+	WARN_ON(retr == 0);
+}
+
+static int __em_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msg,
+				int stop)
+{
+	struct em_i2c_device *priv = i2c_get_adapdata(adap);
+	int count, status;
+
+	/* Send start condition */
+	em_clear_set_bit(priv, 0, I2C_BIT_ACKE0 | I2C_BIT_WTIM0, I2C_OFS_IICC0);
+	em_clear_set_bit(priv, 0, I2C_BIT_STT0, I2C_OFS_IICC0);
+
+	/* Send slave address and R/W type */
+	writeb((msg->addr << 1) | ((msg->flags & I2C_M_RD) ? 1 : 0),
+		priv->base + I2C_OFS_IIC0);
+
+	/* Wait for transaction */
+	status = em_i2c_wait_for_event(priv);
+	if (status < 0)
+		goto out_reset;
+
+	/* Received NACK (result of setting slave address and R/W) */
+	if (!(status & I2C_BIT_ACKD0)) {
+		em_i2c_stop(priv);
+		goto out;
+	}
+
+	/* Extra setup for read transactions */
+	if (!(status & I2C_BIT_TRC0)) {
+		/* 8 bit interrupt mode */
+		em_clear_set_bit(priv, I2C_BIT_WTIM0, I2C_BIT_ACKE0, I2C_OFS_IICC0);
+		em_clear_set_bit(priv, I2C_BIT_WTIM0, I2C_BIT_WREL0, I2C_OFS_IICC0);
+
+		/* Wait for transaction */
+		status = em_i2c_wait_for_event(priv);
+		if (status < 0)
+			goto out_reset;
+	}
+
+	/* Send / receive data */
+	for (count = 0; count < msg->len; count++) {
+		if (!(status & I2C_BIT_TRC0)) { /* Read transaction */
+			msg->buf[count] = readb(priv->base + I2C_OFS_IIC0);
+			em_clear_set_bit(priv, 0, I2C_BIT_WREL0, I2C_OFS_IICC0);
+
+		} else { /* Write transaction */
+			/* Received NACK */
+			if (!(status & I2C_BIT_ACKD0)) {
+				em_i2c_stop(priv);
+				goto out;
+			}
+
+			/* Write data */
+			writeb(msg->buf[count], priv->base + I2C_OFS_IIC0);
+		}
+
+		/* Wait for R/W transaction */
+		status = em_i2c_wait_for_event(priv);
+		if (status < 0)
+			goto out_reset;
+	}
+
+	if (stop)
+		em_i2c_stop(priv);
+
+	return count;
+
+out_reset:
+	em_i2c_reset(adap);
+out:
+	return status < 0 ? status : -ENXIO;
+}
+
+static int em_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
+	int num)
+{
+	struct em_i2c_device *priv = i2c_get_adapdata(adap);
+	int ret, i;
+
+	if (readb(priv->base + I2C_OFS_IICF0) & I2C_BIT_IICBSY)
+		return -EAGAIN;
+
+	for (i = 0; i < num; i++) {
+		ret = __em_i2c_xfer(adap, &msgs[i], (i == (num - 1)));
+		if (ret < 0)
+			return ret;
+	}
+
+	/* I2C transfer completed */
+	return num;
+}
+
+static irqreturn_t em_i2c_irq_handler(int this_irq, void *dev_id)
+{
+	struct em_i2c_device *priv = dev_id;
+
+	complete(&priv->msg_done);
+	return IRQ_HANDLED;
+}
+
+static u32 em_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static struct i2c_algorithm em_i2c_algo = {
+	.master_xfer = em_i2c_xfer,
+	.functionality = em_i2c_func,
+};
+
+static int em_i2c_probe(struct platform_device *pdev)
+{
+	struct em_i2c_device *priv;
+	struct resource *r;
+	int irq, ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(struct em_i2c_device), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	strlcpy(priv->adap.name, "EMEV2 I2C", sizeof(priv->adap.name));
+
+	priv->sclk = devm_clk_get(&pdev->dev, "sclk");
+	if (IS_ERR(priv->sclk))
+		return PTR_ERR(priv->sclk);
+
+	clk_prepare_enable(priv->sclk);
+
+	irq = platform_get_irq(pdev, 0);
+	priv->adap.timeout = msecs_to_jiffies(100);
+	priv->adap.retries = 5;
+	priv->adap.dev.parent = &pdev->dev;
+	priv->adap.algo = &em_i2c_algo;
+	priv->adap.owner = THIS_MODULE;
+	priv->adap.dev.of_node = pdev->dev.of_node;
+
+	init_completion(&priv->msg_done);
+
+	platform_set_drvdata(pdev, priv);
+	i2c_set_adapdata(&priv->adap, priv);
+
+	em_i2c_reset(&priv->adap);
+
+	ret = devm_request_irq(&pdev->dev, irq, em_i2c_irq_handler, 0,
+				"em_i2c", priv);
+	if (ret)
+		goto exit_clk;
+
+	ret = i2c_add_adapter(&priv->adap);
+
+	if (ret)
+		goto exit_clk;
+
+	dev_info(&pdev->dev, "Added i2c controller %d irq %d @ 0x%p\n",
+		priv->adap.nr, irq, priv->base);
+
+	return 0;
+
+exit_clk:
+	clk_disable_unprepare(priv->sclk);
+	return ret;
+}
+
+static int em_i2c_remove(struct platform_device *dev)
+{
+	struct em_i2c_device *priv = platform_get_drvdata(dev);
+
+	i2c_del_adapter(&priv->adap);
+	clk_disable_unprepare(priv->sclk);
+
+	return 0;
+}
+
+static const struct of_device_id em_i2c_ids[] = {
+	{ .compatible = "renesas,iic-emev2", },
+	{ }
+};
+
+static struct platform_driver em_i2c_driver = {
+	.probe = em_i2c_probe,
+	.remove = em_i2c_remove,
+	.driver = {
+		.name = "em-i2c",
+		.of_match_table = em_i2c_ids,
+	}
+};
+module_platform_driver(em_i2c_driver);
+
+MODULE_DESCRIPTION("EMEV2 I2C bus driver");
+MODULE_AUTHOR("Ian Molton and Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>");
+MODULE_LICENSE("GPL v2");
+MODULE_DEVICE_TABLE(of, em_i2c_ids);
-- 
2.1.4

Re: [PATCH 3/5] i2c: emev2: add driver

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Monday 06 July 2015 23:46:07 Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Add a basic driver for the Renesas EMEV2 SoC. Based on the driver from
> the BSP which was first worked on by Ian, and made ready for upstream by
> me.
> 
> Signed-off-by: Ian Molton <ian.molton@codethink.co.uk>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/busses/Kconfig     |   7 +
>  drivers/i2c/busses/Makefile    |   1 +
>  drivers/i2c/busses/i2c-emev2.c | 334 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 342 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-emev2.c
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 2255af23b9c70e..503abb64861f37 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -516,6 +516,13 @@ config I2C_EG20T
>  	  ML7213/ML7223/ML7831 is companion chip for Intel Atom E6xx series.
>  	  ML7213/ML7223/ML7831 is completely compatible for Intel EG20T PCH.
> 
> +config I2C_EMEV2
> +	tristate "EMMA Mobile series I2C adapter"
> +	depends on HAVE_CLK
> +	help
> +	  If you say yes to this option, support will be included for the
> +	  I2C interface on the Renesas Electronics EM/EV family of processors.
> +
>  config I2C_EXYNOS5
>  	tristate "Exynos5 high-speed I2C driver"
>  	depends on ARCH_EXYNOS && OF
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index cdf941da91c65e..73f1145da5b4c8 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -48,6 +48,7 @@ i2c-designware-pci-objs := i2c-designware-pcidrv.o
>  obj-$(CONFIG_I2C_DIGICOLOR)	+= i2c-digicolor.o
>  obj-$(CONFIG_I2C_EFM32)		+= i2c-efm32.o
>  obj-$(CONFIG_I2C_EG20T)		+= i2c-eg20t.o
> +obj-$(CONFIG_I2C_EMEV2)		+= i2c-emev2.o
>  obj-$(CONFIG_I2C_EXYNOS5)	+= i2c-exynos5.o
>  obj-$(CONFIG_I2C_GPIO)		+= i2c-gpio.o
>  obj-$(CONFIG_I2C_HIGHLANDER)	+= i2c-highlander.o
> diff --git a/drivers/i2c/busses/i2c-emev2.c b/drivers/i2c/busses/i2c-emev2.c
> new file mode 100644
> index 00000000000000..baa88fc5ff05e5
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-emev2.c
> @@ -0,0 +1,334 @@
> +/*
> + * I2C driver for the Renesas EMEV2 SoC
> + *
> + * Copyright (C) 2015 Wolfram Sang <wsa@sang-engineering.com>
> + * Copyright 2013 Codethink Ltd.
> + * Copyright 2010-2015 Renesas Electronics Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/sched.h>
> +
> +/* I2C Registers */
> +#define I2C_OFS_IICACT0		0x00	/* start */
> +#define I2C_OFS_IIC0		0x04	/* shift */
> +#define I2C_OFS_IICC0		0x08	/* control */
> +#define I2C_OFS_SVA0		0x0c	/* slave address */
> +#define I2C_OFS_IICCL0		0x10	/* clock select */
> +#define I2C_OFS_IICX0		0x14	/* extension */
> +#define I2C_OFS_IICS0		0x18	/* status */
> +#define I2C_OFS_IICSE0		0x1c	/* status For emulation */
> +#define I2C_OFS_IICF0		0x20	/* IIC flag */
> +
> +/* I2C IICACT0 Masks */
> +#define I2C_BIT_IICE0		0x0001
> +
> +/* I2C IICC0 Masks */
> +#define I2C_BIT_LREL0		0x0040
> +#define I2C_BIT_WREL0		0x0020
> +#define I2C_BIT_SPIE0		0x0010
> +#define I2C_BIT_WTIM0		0x0008
> +#define I2C_BIT_ACKE0		0x0004
> +#define I2C_BIT_STT0		0x0002
> +#define I2C_BIT_SPT0		0x0001
> +
> +/* I2C IICCL0 Masks */
> +#define I2C_BIT_SMC0		0x0008
> +#define I2C_BIT_DFC0		0x0004
> +
> +/* I2C IICSE0 Masks */
> +#define I2C_BIT_MSTS0		0x0080
> +#define I2C_BIT_ALD0		0x0040
> +#define I2C_BIT_EXC0		0x0020
> +#define I2C_BIT_COI0		0x0010
> +#define I2C_BIT_TRC0		0x0008
> +#define I2C_BIT_ACKD0		0x0004
> +#define I2C_BIT_STD0		0x0002
> +#define I2C_BIT_SPD0		0x0001
> +
> +/* I2C IICF0 Masks */
> +#define I2C_BIT_STCF		0x0080
> +#define I2C_BIT_IICBSY		0x0040
> +#define I2C_BIT_STCEN		0x0002
> +#define I2C_BIT_IICRSV		0x0001
> +
> +struct em_i2c_device {
> +	void __iomem *base;
> +	struct i2c_adapter adap;
> +	struct completion msg_done;
> +	struct clk *sclk;
> +};
> +
> +static inline void em_clear_set_bit(struct em_i2c_device *priv, u8 clear,
> u8 set, u8 reg)

Maybe em_i2c_clear_set_bit for consistency ? I'd prefer having the reg 
argument before clear and set, but maybe that's just me.

I would have also introduced em_i2c_read and em_i2c_write. That's entirely up 
to you.

> +{
> +	writeb((readb(priv->base + reg) & ~clear) | set, priv->base + reg);
> +}
> +
> +static int em_i2c_wait_for_event(struct em_i2c_device *priv)
> +{
> +	unsigned long time_left;
> +	int status;
> +
> +	reinit_completion(&priv->msg_done);
> +
> +	time_left = wait_for_completion_timeout(&priv->msg_done,
> priv->adap.timeout);
> +
> +	if (!time_left)
> +		return -ETIMEDOUT;
> +
> +	status = readb(priv->base + I2C_OFS_IICSE0);
> +	return status & I2C_BIT_ALD0 ? -EAGAIN : status;
> +}
> +
> +static void em_i2c_stop(struct em_i2c_device *priv)
> +{
> +	/* Send Stop condition */
> +	em_clear_set_bit(priv, 0, I2C_BIT_SPT0 | I2C_BIT_SPIE0, I2C_OFS_IICC0);
> +
> +	/* Wait for stop condition */
> +	em_i2c_wait_for_event(priv);
> +}
> +
> +static void em_i2c_reset(struct i2c_adapter *adap)
> +{
> +	struct em_i2c_device *priv = i2c_get_adapdata(adap);
> +	int retr;
> +
> +	/* If I2C active */
> +	if (readb(priv->base + I2C_OFS_IICACT0) & I2C_BIT_IICE0) {
> +		/* Disable I2C operation */
> +		writeb(0, priv->base + I2C_OFS_IICACT0);
> +
> +		retr = 1000;
> +		while (readb(priv->base + I2C_OFS_IICACT0) = 1 && retr)

How about adding a cpu_relax() here ?

> +			retr--;
> +		WARN_ON(retr = 0);
> +	}
> +
> +	/* Transfer mode set */
> +	writeb(I2C_BIT_DFC0, priv->base + I2C_OFS_IICCL0);
> +
> +	/* Can Issue start without detecting a stop, Reservation disabled. */
> +	writeb(I2C_BIT_STCEN | I2C_BIT_IICRSV, priv->base + I2C_OFS_IICF0);
> +
> +	/* I2C enable, 9 bit interrupt mode */
> +	writeb(I2C_BIT_WTIM0, priv->base + I2C_OFS_IICC0);
> +
> +	/* Enable I2C operation */
> +	writeb(I2C_BIT_IICE0, priv->base + I2C_OFS_IICACT0);
> +
> +	retr = 1000;
> +	while (readb(priv->base + I2C_OFS_IICACT0) = 0 && retr)

And here too.

> +		retr--;
> +	WARN_ON(retr = 0);
> +}
> +
> +static int __em_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msg,
> +				int stop)
> +{
> +	struct em_i2c_device *priv = i2c_get_adapdata(adap);
> +	int count, status;
> +
> +	/* Send start condition */
> +	em_clear_set_bit(priv, 0, I2C_BIT_ACKE0 | I2C_BIT_WTIM0, I2C_OFS_IICC0);
> +	em_clear_set_bit(priv, 0, I2C_BIT_STT0, I2C_OFS_IICC0);
> +
> +	/* Send slave address and R/W type */
> +	writeb((msg->addr << 1) | ((msg->flags & I2C_M_RD) ? 1 : 0),
> +		priv->base + I2C_OFS_IIC0);
> +
> +	/* Wait for transaction */
> +	status = em_i2c_wait_for_event(priv);
> +	if (status < 0)
> +		goto out_reset;
> +
> +	/* Received NACK (result of setting slave address and R/W) */
> +	if (!(status & I2C_BIT_ACKD0)) {
> +		em_i2c_stop(priv);
> +		goto out;
> +	}
> +
> +	/* Extra setup for read transactions */
> +	if (!(status & I2C_BIT_TRC0)) {

How about checking msg->flags & I2C_M_RD here ? It should be equivalent but 
would make the code more readable by not requiring knowledge of the hardware. 
Same for the check in the loop below.

> +		/* 8 bit interrupt mode */
> +		em_clear_set_bit(priv, I2C_BIT_WTIM0, I2C_BIT_ACKE0, I2C_OFS_IICC0);
> +		em_clear_set_bit(priv, I2C_BIT_WTIM0, I2C_BIT_WREL0, I2C_OFS_IICC0);
> +
> +		/* Wait for transaction */
> +		status = em_i2c_wait_for_event(priv);
> +		if (status < 0)
> +			goto out_reset;
> +	}
> +
> +	/* Send / receive data */
> +	for (count = 0; count < msg->len; count++) {
> +		if (!(status & I2C_BIT_TRC0)) { /* Read transaction */
> +			msg->buf[count] = readb(priv->base + I2C_OFS_IIC0);
> +			em_clear_set_bit(priv, 0, I2C_BIT_WREL0, I2C_OFS_IICC0);
> +
> +		} else { /* Write transaction */
> +			/* Received NACK */
> +			if (!(status & I2C_BIT_ACKD0)) {
> +				em_i2c_stop(priv);
> +				goto out;
> +			}
> +
> +			/* Write data */
> +			writeb(msg->buf[count], priv->base + I2C_OFS_IIC0);
> +		}
> +
> +		/* Wait for R/W transaction */
> +		status = em_i2c_wait_for_event(priv);
> +		if (status < 0)
> +			goto out_reset;
> +	}
> +
> +	if (stop)
> +		em_i2c_stop(priv);
> +
> +	return count;
> +
> +out_reset:
> +	em_i2c_reset(adap);
> +out:
> +	return status < 0 ? status : -ENXIO;
> +}
> +
> +static int em_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> +	int num)
> +{
> +	struct em_i2c_device *priv = i2c_get_adapdata(adap);
> +	int ret, i;
> +
> +	if (readb(priv->base + I2C_OFS_IICF0) & I2C_BIT_IICBSY)
> +		return -EAGAIN;
> +
> +	for (i = 0; i < num; i++) {
> +		ret = __em_i2c_xfer(adap, &msgs[i], (i = (num - 1)));
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	/* I2C transfer completed */
> +	return num;
> +}
> +
> +static irqreturn_t em_i2c_irq_handler(int this_irq, void *dev_id)
> +{
> +	struct em_i2c_device *priv = dev_id;
> +
> +	complete(&priv->msg_done);
> +	return IRQ_HANDLED;
> +}
> +
> +static u32 em_i2c_func(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static struct i2c_algorithm em_i2c_algo = {
> +	.master_xfer = em_i2c_xfer,
> +	.functionality = em_i2c_func,
> +};
> +
> +static int em_i2c_probe(struct platform_device *pdev)
> +{
> +	struct em_i2c_device *priv;
> +	struct resource *r;
> +	int irq, ret;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(struct em_i2c_device), 
GFP_KERNEL);

I'd use sizeof(*priv).

> +	if (!priv)
> +		return -ENOMEM;
> +
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->base = devm_ioremap_resource(&pdev->dev, r);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	strlcpy(priv->adap.name, "EMEV2 I2C", sizeof(priv->adap.name));
> +
> +	priv->sclk = devm_clk_get(&pdev->dev, "sclk");
> +	if (IS_ERR(priv->sclk))
> +		return PTR_ERR(priv->sclk);
> +
> +	clk_prepare_enable(priv->sclk);
> +
> +	irq = platform_get_irq(pdev, 0);

I'd move this call right before devm_request_irq() below. devm_request_irq() 
should handle invalid IRQs, but won't print an error message. I'd let you 
decide whether that's a problem.

> +	priv->adap.timeout = msecs_to_jiffies(100);
> +	priv->adap.retries = 5;

Is there a particular reason for setting the number of retries to 5 ?

> +	priv->adap.dev.parent = &pdev->dev;
> +	priv->adap.algo = &em_i2c_algo;
> +	priv->adap.owner = THIS_MODULE;
> +	priv->adap.dev.of_node = pdev->dev.of_node;
> +
> +	init_completion(&priv->msg_done);
> +
> +	platform_set_drvdata(pdev, priv);
> +	i2c_set_adapdata(&priv->adap, priv);
> +
> +	em_i2c_reset(&priv->adap);
> +
> +	ret = devm_request_irq(&pdev->dev, irq, em_i2c_irq_handler, 0,
> +				"em_i2c", priv);
> +	if (ret)
> +		goto exit_clk;

Nitpicking, I'd call this error_clk to show that the label is used in case of 
error only. You could also just call it error as there's no other error-
related label.

> +
> +	ret = i2c_add_adapter(&priv->adap);
> +
> +	if (ret)
> +		goto exit_clk;
> +
> +	dev_info(&pdev->dev, "Added i2c controller %d irq %d @ 0x%p\n",
> +		priv->adap.nr, irq, priv->base);

Is priv->base useful here ? The physical address of the registers block could 
be, but its kernel virtual address doesn't seem very interesting to me.

> +
> +	return 0;
> +
> +exit_clk:
> +	clk_disable_unprepare(priv->sclk);
> +	return ret;
> +}
> +
> +static int em_i2c_remove(struct platform_device *dev)
> +{
> +	struct em_i2c_device *priv = platform_get_drvdata(dev);
> +
> +	i2c_del_adapter(&priv->adap);
> +	clk_disable_unprepare(priv->sclk);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id em_i2c_ids[] = {
> +	{ .compatible = "renesas,iic-emev2", },
> +	{ }
> +};
> +
> +static struct platform_driver em_i2c_driver = {
> +	.probe = em_i2c_probe,
> +	.remove = em_i2c_remove,
> +	.driver = {
> +		.name = "em-i2c",
> +		.of_match_table = em_i2c_ids,
> +	}
> +};
> +module_platform_driver(em_i2c_driver);
> +
> +MODULE_DESCRIPTION("EMEV2 I2C bus driver");
> +MODULE_AUTHOR("Ian Molton and Wolfram Sang <wsa@sang-engineering.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DEVICE_TABLE(of, em_i2c_ids);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/5] i2c: emev2: add driver

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Monday 06 July 2015 23:46:07 Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Add a basic driver for the Renesas EMEV2 SoC. Based on the driver from
> the BSP which was first worked on by Ian, and made ready for upstream by
> me.
> 
> Signed-off-by: Ian Molton <ian.molton@codethink.co.uk>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/busses/Kconfig     |   7 +
>  drivers/i2c/busses/Makefile    |   1 +
>  drivers/i2c/busses/i2c-emev2.c | 334 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 342 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-emev2.c
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 2255af23b9c70e..503abb64861f37 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -516,6 +516,13 @@ config I2C_EG20T
>  	  ML7213/ML7223/ML7831 is companion chip for Intel Atom E6xx series.
>  	  ML7213/ML7223/ML7831 is completely compatible for Intel EG20T PCH.
> 
> +config I2C_EMEV2
> +	tristate "EMMA Mobile series I2C adapter"
> +	depends on HAVE_CLK
> +	help
> +	  If you say yes to this option, support will be included for the
> +	  I2C interface on the Renesas Electronics EM/EV family of processors.
> +
>  config I2C_EXYNOS5
>  	tristate "Exynos5 high-speed I2C driver"
>  	depends on ARCH_EXYNOS && OF
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index cdf941da91c65e..73f1145da5b4c8 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -48,6 +48,7 @@ i2c-designware-pci-objs := i2c-designware-pcidrv.o
>  obj-$(CONFIG_I2C_DIGICOLOR)	+= i2c-digicolor.o
>  obj-$(CONFIG_I2C_EFM32)		+= i2c-efm32.o
>  obj-$(CONFIG_I2C_EG20T)		+= i2c-eg20t.o
> +obj-$(CONFIG_I2C_EMEV2)		+= i2c-emev2.o
>  obj-$(CONFIG_I2C_EXYNOS5)	+= i2c-exynos5.o
>  obj-$(CONFIG_I2C_GPIO)		+= i2c-gpio.o
>  obj-$(CONFIG_I2C_HIGHLANDER)	+= i2c-highlander.o
> diff --git a/drivers/i2c/busses/i2c-emev2.c b/drivers/i2c/busses/i2c-emev2.c
> new file mode 100644
> index 00000000000000..baa88fc5ff05e5
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-emev2.c
> @@ -0,0 +1,334 @@
> +/*
> + * I2C driver for the Renesas EMEV2 SoC
> + *
> + * Copyright (C) 2015 Wolfram Sang <wsa@sang-engineering.com>
> + * Copyright 2013 Codethink Ltd.
> + * Copyright 2010-2015 Renesas Electronics Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/sched.h>
> +
> +/* I2C Registers */
> +#define I2C_OFS_IICACT0		0x00	/* start */
> +#define I2C_OFS_IIC0		0x04	/* shift */
> +#define I2C_OFS_IICC0		0x08	/* control */
> +#define I2C_OFS_SVA0		0x0c	/* slave address */
> +#define I2C_OFS_IICCL0		0x10	/* clock select */
> +#define I2C_OFS_IICX0		0x14	/* extension */
> +#define I2C_OFS_IICS0		0x18	/* status */
> +#define I2C_OFS_IICSE0		0x1c	/* status For emulation */
> +#define I2C_OFS_IICF0		0x20	/* IIC flag */
> +
> +/* I2C IICACT0 Masks */
> +#define I2C_BIT_IICE0		0x0001
> +
> +/* I2C IICC0 Masks */
> +#define I2C_BIT_LREL0		0x0040
> +#define I2C_BIT_WREL0		0x0020
> +#define I2C_BIT_SPIE0		0x0010
> +#define I2C_BIT_WTIM0		0x0008
> +#define I2C_BIT_ACKE0		0x0004
> +#define I2C_BIT_STT0		0x0002
> +#define I2C_BIT_SPT0		0x0001
> +
> +/* I2C IICCL0 Masks */
> +#define I2C_BIT_SMC0		0x0008
> +#define I2C_BIT_DFC0		0x0004
> +
> +/* I2C IICSE0 Masks */
> +#define I2C_BIT_MSTS0		0x0080
> +#define I2C_BIT_ALD0		0x0040
> +#define I2C_BIT_EXC0		0x0020
> +#define I2C_BIT_COI0		0x0010
> +#define I2C_BIT_TRC0		0x0008
> +#define I2C_BIT_ACKD0		0x0004
> +#define I2C_BIT_STD0		0x0002
> +#define I2C_BIT_SPD0		0x0001
> +
> +/* I2C IICF0 Masks */
> +#define I2C_BIT_STCF		0x0080
> +#define I2C_BIT_IICBSY		0x0040
> +#define I2C_BIT_STCEN		0x0002
> +#define I2C_BIT_IICRSV		0x0001
> +
> +struct em_i2c_device {
> +	void __iomem *base;
> +	struct i2c_adapter adap;
> +	struct completion msg_done;
> +	struct clk *sclk;
> +};
> +
> +static inline void em_clear_set_bit(struct em_i2c_device *priv, u8 clear,
> u8 set, u8 reg)

Maybe em_i2c_clear_set_bit for consistency ? I'd prefer having the reg 
argument before clear and set, but maybe that's just me.

I would have also introduced em_i2c_read and em_i2c_write. That's entirely up 
to you.

> +{
> +	writeb((readb(priv->base + reg) & ~clear) | set, priv->base + reg);
> +}
> +
> +static int em_i2c_wait_for_event(struct em_i2c_device *priv)
> +{
> +	unsigned long time_left;
> +	int status;
> +
> +	reinit_completion(&priv->msg_done);
> +
> +	time_left = wait_for_completion_timeout(&priv->msg_done,
> priv->adap.timeout);
> +
> +	if (!time_left)
> +		return -ETIMEDOUT;
> +
> +	status = readb(priv->base + I2C_OFS_IICSE0);
> +	return status & I2C_BIT_ALD0 ? -EAGAIN : status;
> +}
> +
> +static void em_i2c_stop(struct em_i2c_device *priv)
> +{
> +	/* Send Stop condition */
> +	em_clear_set_bit(priv, 0, I2C_BIT_SPT0 | I2C_BIT_SPIE0, I2C_OFS_IICC0);
> +
> +	/* Wait for stop condition */
> +	em_i2c_wait_for_event(priv);
> +}
> +
> +static void em_i2c_reset(struct i2c_adapter *adap)
> +{
> +	struct em_i2c_device *priv = i2c_get_adapdata(adap);
> +	int retr;
> +
> +	/* If I2C active */
> +	if (readb(priv->base + I2C_OFS_IICACT0) & I2C_BIT_IICE0) {
> +		/* Disable I2C operation */
> +		writeb(0, priv->base + I2C_OFS_IICACT0);
> +
> +		retr = 1000;
> +		while (readb(priv->base + I2C_OFS_IICACT0) == 1 && retr)

How about adding a cpu_relax() here ?

> +			retr--;
> +		WARN_ON(retr == 0);
> +	}
> +
> +	/* Transfer mode set */
> +	writeb(I2C_BIT_DFC0, priv->base + I2C_OFS_IICCL0);
> +
> +	/* Can Issue start without detecting a stop, Reservation disabled. */
> +	writeb(I2C_BIT_STCEN | I2C_BIT_IICRSV, priv->base + I2C_OFS_IICF0);
> +
> +	/* I2C enable, 9 bit interrupt mode */
> +	writeb(I2C_BIT_WTIM0, priv->base + I2C_OFS_IICC0);
> +
> +	/* Enable I2C operation */
> +	writeb(I2C_BIT_IICE0, priv->base + I2C_OFS_IICACT0);
> +
> +	retr = 1000;
> +	while (readb(priv->base + I2C_OFS_IICACT0) == 0 && retr)

And here too.

> +		retr--;
> +	WARN_ON(retr == 0);
> +}
> +
> +static int __em_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msg,
> +				int stop)
> +{
> +	struct em_i2c_device *priv = i2c_get_adapdata(adap);
> +	int count, status;
> +
> +	/* Send start condition */
> +	em_clear_set_bit(priv, 0, I2C_BIT_ACKE0 | I2C_BIT_WTIM0, I2C_OFS_IICC0);
> +	em_clear_set_bit(priv, 0, I2C_BIT_STT0, I2C_OFS_IICC0);
> +
> +	/* Send slave address and R/W type */
> +	writeb((msg->addr << 1) | ((msg->flags & I2C_M_RD) ? 1 : 0),
> +		priv->base + I2C_OFS_IIC0);
> +
> +	/* Wait for transaction */
> +	status = em_i2c_wait_for_event(priv);
> +	if (status < 0)
> +		goto out_reset;
> +
> +	/* Received NACK (result of setting slave address and R/W) */
> +	if (!(status & I2C_BIT_ACKD0)) {
> +		em_i2c_stop(priv);
> +		goto out;
> +	}
> +
> +	/* Extra setup for read transactions */
> +	if (!(status & I2C_BIT_TRC0)) {

How about checking msg->flags & I2C_M_RD here ? It should be equivalent but 
would make the code more readable by not requiring knowledge of the hardware. 
Same for the check in the loop below.

> +		/* 8 bit interrupt mode */
> +		em_clear_set_bit(priv, I2C_BIT_WTIM0, I2C_BIT_ACKE0, I2C_OFS_IICC0);
> +		em_clear_set_bit(priv, I2C_BIT_WTIM0, I2C_BIT_WREL0, I2C_OFS_IICC0);
> +
> +		/* Wait for transaction */
> +		status = em_i2c_wait_for_event(priv);
> +		if (status < 0)
> +			goto out_reset;
> +	}
> +
> +	/* Send / receive data */
> +	for (count = 0; count < msg->len; count++) {
> +		if (!(status & I2C_BIT_TRC0)) { /* Read transaction */
> +			msg->buf[count] = readb(priv->base + I2C_OFS_IIC0);
> +			em_clear_set_bit(priv, 0, I2C_BIT_WREL0, I2C_OFS_IICC0);
> +
> +		} else { /* Write transaction */
> +			/* Received NACK */
> +			if (!(status & I2C_BIT_ACKD0)) {
> +				em_i2c_stop(priv);
> +				goto out;
> +			}
> +
> +			/* Write data */
> +			writeb(msg->buf[count], priv->base + I2C_OFS_IIC0);
> +		}
> +
> +		/* Wait for R/W transaction */
> +		status = em_i2c_wait_for_event(priv);
> +		if (status < 0)
> +			goto out_reset;
> +	}
> +
> +	if (stop)
> +		em_i2c_stop(priv);
> +
> +	return count;
> +
> +out_reset:
> +	em_i2c_reset(adap);
> +out:
> +	return status < 0 ? status : -ENXIO;
> +}
> +
> +static int em_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> +	int num)
> +{
> +	struct em_i2c_device *priv = i2c_get_adapdata(adap);
> +	int ret, i;
> +
> +	if (readb(priv->base + I2C_OFS_IICF0) & I2C_BIT_IICBSY)
> +		return -EAGAIN;
> +
> +	for (i = 0; i < num; i++) {
> +		ret = __em_i2c_xfer(adap, &msgs[i], (i == (num - 1)));
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	/* I2C transfer completed */
> +	return num;
> +}
> +
> +static irqreturn_t em_i2c_irq_handler(int this_irq, void *dev_id)
> +{
> +	struct em_i2c_device *priv = dev_id;
> +
> +	complete(&priv->msg_done);
> +	return IRQ_HANDLED;
> +}
> +
> +static u32 em_i2c_func(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static struct i2c_algorithm em_i2c_algo = {
> +	.master_xfer = em_i2c_xfer,
> +	.functionality = em_i2c_func,
> +};
> +
> +static int em_i2c_probe(struct platform_device *pdev)
> +{
> +	struct em_i2c_device *priv;
> +	struct resource *r;
> +	int irq, ret;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(struct em_i2c_device), 
GFP_KERNEL);

I'd use sizeof(*priv).

> +	if (!priv)
> +		return -ENOMEM;
> +
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->base = devm_ioremap_resource(&pdev->dev, r);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	strlcpy(priv->adap.name, "EMEV2 I2C", sizeof(priv->adap.name));
> +
> +	priv->sclk = devm_clk_get(&pdev->dev, "sclk");
> +	if (IS_ERR(priv->sclk))
> +		return PTR_ERR(priv->sclk);
> +
> +	clk_prepare_enable(priv->sclk);
> +
> +	irq = platform_get_irq(pdev, 0);

I'd move this call right before devm_request_irq() below. devm_request_irq() 
should handle invalid IRQs, but won't print an error message. I'd let you 
decide whether that's a problem.

> +	priv->adap.timeout = msecs_to_jiffies(100);
> +	priv->adap.retries = 5;

Is there a particular reason for setting the number of retries to 5 ?

> +	priv->adap.dev.parent = &pdev->dev;
> +	priv->adap.algo = &em_i2c_algo;
> +	priv->adap.owner = THIS_MODULE;
> +	priv->adap.dev.of_node = pdev->dev.of_node;
> +
> +	init_completion(&priv->msg_done);
> +
> +	platform_set_drvdata(pdev, priv);
> +	i2c_set_adapdata(&priv->adap, priv);
> +
> +	em_i2c_reset(&priv->adap);
> +
> +	ret = devm_request_irq(&pdev->dev, irq, em_i2c_irq_handler, 0,
> +				"em_i2c", priv);
> +	if (ret)
> +		goto exit_clk;

Nitpicking, I'd call this error_clk to show that the label is used in case of 
error only. You could also just call it error as there's no other error-
related label.

> +
> +	ret = i2c_add_adapter(&priv->adap);
> +
> +	if (ret)
> +		goto exit_clk;
> +
> +	dev_info(&pdev->dev, "Added i2c controller %d irq %d @ 0x%p\n",
> +		priv->adap.nr, irq, priv->base);

Is priv->base useful here ? The physical address of the registers block could 
be, but its kernel virtual address doesn't seem very interesting to me.

> +
> +	return 0;
> +
> +exit_clk:
> +	clk_disable_unprepare(priv->sclk);
> +	return ret;
> +}
> +
> +static int em_i2c_remove(struct platform_device *dev)
> +{
> +	struct em_i2c_device *priv = platform_get_drvdata(dev);
> +
> +	i2c_del_adapter(&priv->adap);
> +	clk_disable_unprepare(priv->sclk);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id em_i2c_ids[] = {
> +	{ .compatible = "renesas,iic-emev2", },
> +	{ }
> +};
> +
> +static struct platform_driver em_i2c_driver = {
> +	.probe = em_i2c_probe,
> +	.remove = em_i2c_remove,
> +	.driver = {
> +		.name = "em-i2c",
> +		.of_match_table = em_i2c_ids,
> +	}
> +};
> +module_platform_driver(em_i2c_driver);
> +
> +MODULE_DESCRIPTION("EMEV2 I2C bus driver");
> +MODULE_AUTHOR("Ian Molton and Wolfram Sang <wsa@sang-engineering.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DEVICE_TABLE(of, em_i2c_ids);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/5] i2c: emev2: add driver

[Wolfram Sang]

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


> > +static inline void em_clear_set_bit(struct em_i2c_device *priv, u8 clear,
> > u8 set, u8 reg)
> 
> Maybe em_i2c_clear_set_bit for consistency ? I'd prefer having the reg 
> argument before clear and set, but maybe that's just me.

This is such a generic function that I decided to skip 'i2c' in the
function name. Can add it, don't have a strong preference.

> I would have also introduced em_i2c_read and em_i2c_write. That's entirely up 
> to you.

I don't like such wrappers around standard read/write functions. What's
the gain?

> > +		retr = 1000;
> > +		while (readb(priv->base + I2C_OFS_IICACT0) == 1 && retr)
> 
> How about adding a cpu_relax() here ?

Can do, but I think it is overkill.

> 
> > +	/* Extra setup for read transactions */
> > +	if (!(status & I2C_BIT_TRC0)) {
> 
> How about checking msg->flags & I2C_M_RD here ? It should be equivalent but 
> would make the code more readable by not requiring knowledge of the hardware. 

I buy this argument. Will change.

> > +	priv = devm_kzalloc(&pdev->dev, sizeof(struct em_i2c_device), 
> GFP_KERNEL);
> 
> I'd use sizeof(*priv).

Yes, definately.

> > +	irq = platform_get_irq(pdev, 0);
> 
> I'd move this call right before devm_request_irq() below. devm_request_irq() 
> should handle invalid IRQs, but won't print an error message. I'd let you 
> decide whether that's a problem.

Will check.

> 
> > +	priv->adap.timeout = msecs_to_jiffies(100);
> > +	priv->adap.retries = 5;
> 
> Is there a particular reason for setting the number of retries to 5 ?

It should be non-zero at least, so bus access will be retried if -EAGAIN
is returned because of a busy bus.

> > +	if (ret)
> > +		goto exit_clk;
> 
> Nitpicking, I'd call this error_clk to show that the label is used in case of 
> error only. You could also just call it error as there's no other error-
> related label.

Yup.

> > +	dev_info(&pdev->dev, "Added i2c controller %d irq %d @ 0x%p\n",
> > +		priv->adap.nr, irq, priv->base);
> 
> Is priv->base useful here ? The physical address of the registers block could 
> be, but its kernel virtual address doesn't seem very interesting to me.

Agreed.

Thanks for the review!

   Wolfram


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

Re: [PATCH 3/5] i2c: emev2: add driver

[Wolfram Sang]

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


> > +static inline void em_clear_set_bit(struct em_i2c_device *priv, u8 clear,
> > u8 set, u8 reg)
> 
> Maybe em_i2c_clear_set_bit for consistency ? I'd prefer having the reg 
> argument before clear and set, but maybe that's just me.

This is such a generic function that I decided to skip 'i2c' in the
function name. Can add it, don't have a strong preference.

> I would have also introduced em_i2c_read and em_i2c_write. That's entirely up 
> to you.

I don't like such wrappers around standard read/write functions. What's
the gain?

> > +		retr = 1000;
> > +		while (readb(priv->base + I2C_OFS_IICACT0) == 1 && retr)
> 
> How about adding a cpu_relax() here ?

Can do, but I think it is overkill.

> 
> > +	/* Extra setup for read transactions */
> > +	if (!(status & I2C_BIT_TRC0)) {
> 
> How about checking msg->flags & I2C_M_RD here ? It should be equivalent but 
> would make the code more readable by not requiring knowledge of the hardware. 

I buy this argument. Will change.

> > +	priv = devm_kzalloc(&pdev->dev, sizeof(struct em_i2c_device), 
> GFP_KERNEL);
> 
> I'd use sizeof(*priv).

Yes, definately.

> > +	irq = platform_get_irq(pdev, 0);
> 
> I'd move this call right before devm_request_irq() below. devm_request_irq() 
> should handle invalid IRQs, but won't print an error message. I'd let you 
> decide whether that's a problem.

Will check.

> 
> > +	priv->adap.timeout = msecs_to_jiffies(100);
> > +	priv->adap.retries = 5;
> 
> Is there a particular reason for setting the number of retries to 5 ?

It should be non-zero at least, so bus access will be retried if -EAGAIN
is returned because of a busy bus.

> > +	if (ret)
> > +		goto exit_clk;
> 
> Nitpicking, I'd call this error_clk to show that the label is used in case of 
> error only. You could also just call it error as there's no other error-
> related label.

Yup.

> > +	dev_info(&pdev->dev, "Added i2c controller %d irq %d @ 0x%p\n",
> > +		priv->adap.nr, irq, priv->base);
> 
> Is priv->base useful here ? The physical address of the registers block could 
> be, but its kernel virtual address doesn't seem very interesting to me.

Agreed.

Thanks for the review!

   Wolfram


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

[PATCH 5/5] ARM: shmobile: emev2: kzm9d: enable IIC busses

[Wolfram Sang]

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

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 arch/arm/boot/dts/emev2-kzm9d.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/emev2-kzm9d.dts b/arch/arm/boot/dts/emev2-kzm9d.dts
index 0aa1b345f871c9..2e944437287599 100644
--- a/arch/arm/boot/dts/emev2-kzm9d.dts
+++ b/arch/arm/boot/dts/emev2-kzm9d.dts
@@ -95,6 +95,14 @@
 	};
 };
 
+&iic0 {
+	status = "okay";
+};
+
+&iic1 {
+	status = "okay";
+};
+
 &pfc {
 	uart1_pins: serial@e1030000 {
 		renesas,groups = "uart1_ctrl", "uart1_data";
-- 
2.1.4

[PATCH 5/5] ARM: shmobile: emev2: kzm9d: enable IIC busses

[Wolfram Sang]

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

Signed-off-by: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
---
 arch/arm/boot/dts/emev2-kzm9d.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/emev2-kzm9d.dts b/arch/arm/boot/dts/emev2-kzm9d.dts
index 0aa1b345f871c9..2e944437287599 100644
--- a/arch/arm/boot/dts/emev2-kzm9d.dts
+++ b/arch/arm/boot/dts/emev2-kzm9d.dts
@@ -95,6 +95,14 @@
 	};
 };
 
+&iic0 {
+	status = "okay";
+};
+
+&iic1 {
+	status = "okay";
+};
+
 &pfc {
 	uart1_pins: serial@e1030000 {
 		renesas,groups = "uart1_ctrl", "uart1_data";
-- 
2.1.4

Re: [PATCH 0/5] enable I2C on Renesas EMEV2 and KZM9D board

[Geert Uytterhoeven]

Hi Wolfram,

CC Ian

On Mon, Jul 6, 2015 at 11:46 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> So, I picked up the old series from Ian and reworked it significantly. While
> his series was already an improvement from the driver found in the BSP, it was
> still not ready for upstream. It should be now :) My idea was to start simple
> and improve incrementally, so e.g. clock handling was broken, so clocks are
> always on for starters. Major changes include:

Thanks for your series!

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH 0/5] enable I2C on Renesas EMEV2 and KZM9D board

[Geert Uytterhoeven]

Hi Wolfram,

CC Ian

On Mon, Jul 6, 2015 at 11:46 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> So, I picked up the old series from Ian and reworked it significantly. While
> his series was already an improvement from the driver found in the BSP, it was
> still not ready for upstream. It should be now :) My idea was to start simple
> and improve incrementally, so e.g. clock handling was broken, so clocks are
> always on for starters. Major changes include:

Thanks for your series!

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 0/5] enable I2C on Renesas EMEV2 and KZM9D board

[Wolfram Sang]

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

On Tue, Jul 07, 2015 at 10:38:55AM +0200, Geert Uytterhoeven wrote:
> Hi Wolfram,
> 
> CC Ian

Yes, thanks! I wanted to but forgot :(


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

Re: [PATCH 0/5] enable I2C on Renesas EMEV2 and KZM9D board

[Wolfram Sang]

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

On Tue, Jul 07, 2015 at 10:38:55AM +0200, Geert Uytterhoeven wrote:
> Hi Wolfram,
> 
> CC Ian

Yes, thanks! I wanted to but forgot :(


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