[PATCH 0/5] i2c: rcar: Device Tree support and clock improvements

[PATCH 0/5] i2c: rcar: Device Tree support and clock improvements

[Guennadi Liakhovetski]

Device Tree support for i2c-rcar isn't too complex, only usual or standard
properties are used. Apart from it several clock handling improvements are
included in this patch series.

Developed on top of -next of 09.09.2013

Cc: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>

Guennadi Liakhovetski (5):
  i2c: rcar: (cosmetic) remove superfluous parenthesis
  i2c: rcar: get clock rate only once and simplify calculation
  i2c: rcar: add Device Tree support
  i2c: rcar: fix clk_get() error handling
  i2c: rcar: use per-device clock

 Documentation/devicetree/bindings/i2c/i2c-rcar.txt |   22 +++++++++
 drivers/i2c/busses/i2c-rcar.c                      |   50 ++++++++++++++------
 2 files changed, 58 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-rcar.txt

-- 
1.7.2.5

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

[PATCH 1/5] i2c: rcar: (cosmetic) remove superfluous parenthesis

[Guennadi Liakhovetski]

A recent patch added even more superfluous parenthesis to those, which
already were there. Remove them again.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
---
 drivers/i2c/busses/i2c-rcar.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index d2fe11d..15eef94 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -306,7 +306,7 @@ scgd_find:
 	/*
 	 * keep icccr value
 	 */
-	priv->icccr = (scgd << (cdf_width) | cdf);
+	priv->icccr = scgd << cdf_width | cdf;
 
 	return 0;
 }
-- 
1.7.2.5

[PATCH 2/5] i2c: rcar: get clock rate only once and simplify calculation

[Guennadi Liakhovetski]

There is no need to repeatedly query clock frequency, where it is not
expected to change. The complete loop can also trivially be replaced with
a simple division. A further loop below the one, being simplified, could
also be replaced, but that would get more complicated.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-rcar.c |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 15eef94..9325db4 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -231,6 +231,7 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv,
 	u32 round, ick;
 	u32 scl;
 	u32 cdf_width;
+	unsigned long rate;
 
 	if (!clkp) {
 		dev_err(dev, "there is no peripheral_clk\n");
@@ -264,15 +265,14 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv,
 	 * clkp : peripheral_clk
 	 * F[]  : integer up-valuation
 	 */
-	for (cdf = 0; cdf < (1 << cdf_width); cdf++) {
-		ick = clk_get_rate(clkp) / (1 + cdf);
-		if (ick < 20000000)
-			goto ick_find;
+	rate = clk_get_rate(clkp);
+	cdf = rate / 20000000;
+	if (cdf >= 1 << cdf_width) {
+		dev_err(dev, "Input clock %lu too high\n", rate);
+		return -EIO;
 	}
-	dev_err(dev, "there is no best CDF\n");
-	return -EIO;
+	ick = rate / (cdf + 1);
 
-ick_find:
 	/*
 	 * it is impossible to calculate large scale
 	 * number on u32. separate it
@@ -290,6 +290,12 @@ ick_find:
 	 *
 	 * Calculation result (= SCL) should be less than
 	 * bus_speed for hardware safety
+	 *
+	 * We could use something along the lines of
+	 *	div = ick / (bus_speed + 1) + 1;
+	 *	scgd = (div - 20 - round + 7) / 8;
+	 *	scl = ick / (20 + (scgd * 8) + round);
+	 * (not fully verified) but that would get pretty involved
 	 */
 	for (scgd = 0; scgd < 0x40; scgd++) {
 		scl = ick / (20 + (scgd * 8) + round);
-- 
1.7.2.5

[PATCH 3/5] i2c: rcar: add Device Tree support

[Guennadi Liakhovetski]

This patch adds Device Tree support to the i2c-rcar driver and respective
documentation.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
---
 Documentation/devicetree/bindings/i2c/i2c-rcar.txt |   22 ++++++++++++++++++++
 drivers/i2c/busses/i2c-rcar.c                      |   20 ++++++++++++++++-
 2 files changed, 40 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-rcar.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-rcar.txt b/Documentation/devicetree/bindings/i2c/i2c-rcar.txt
new file mode 100644
index 0000000..b3c030b
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-rcar.txt
@@ -0,0 +1,22 @@
+I2C for R-Car platforms
+
+Required properties:
+- compatible: Must be one of
+	"renesas,i2c-rcar"
+	"renesas,i2c-rcar-h1"
+	"renesas,i2c-rcar-h2"
+- reg: physical base address of the controller and length of memory mapped
+  region.
+- interrupts: interrupt specifier.
+
+Optional properties:
+- clock-frequency: desired I2C bus clock frequency in Hz. The absence of this
+  propoerty indicates the default frequency 100 kHz.
+
+Examples :
+
+i2c0: i2c@e6500000 {
+	compatible = "renesas,i2c-rcar-h2";
+	reg = <0 0xe6500000 0 0x428>;
+	interrupts = <0 174 0x4>;
+};
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 9325db4..be801b9 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -33,6 +33,7 @@
 #include <linux/i2c/i2c-rcar.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
@@ -638,6 +639,14 @@ static const struct i2c_algorithm rcar_i2c_algo = {
 	.functionality	= rcar_i2c_func,
 };
 
+static const struct of_device_id rcar_i2c_dt_ids[] = {
+	{ .compatible = "renesas,i2c-rcar", .data = (void *)I2C_RCAR_H1 },
+	{ .compatible = "renesas,i2c-rcar-h1", .data = (void *)I2C_RCAR_H1 },
+	{ .compatible = "renesas,i2c-rcar-h2", .data = (void *)I2C_RCAR_H2 },
+	{},
+};
+MODULE_DEVICE_TABLE(of, rcar_i2c_dt_ids);
+
 static int rcar_i2c_probe(struct platform_device *pdev)
 {
 	struct i2c_rcar_platform_data *pdata = dev_get_platdata(&pdev->dev);
@@ -655,10 +664,15 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 	}
 
 	bus_speed = 100000; /* default 100 kHz */
-	if (pdata && pdata->bus_speed)
+	ret = of_property_read_u32(dev->of_node, "clock-frequency", &bus_speed);
+	if (ret < 0 && pdata && pdata->bus_speed)
 		bus_speed = pdata->bus_speed;
 
-	priv->devtype = platform_get_device_id(pdev)->driver_data;
+	if (pdev->dev.of_node)
+		priv->devtype = (long)of_match_device(rcar_i2c_dt_ids,
+						      dev)->data;
+	else
+		priv->devtype = platform_get_device_id(pdev)->driver_data;
 
 	ret = rcar_i2c_clock_calculate(priv, bus_speed, dev);
 	if (ret < 0)
@@ -679,6 +693,7 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 	adap->class		= I2C_CLASS_HWMON | I2C_CLASS_SPD;
 	adap->retries		= 3;
 	adap->dev.parent	= dev;
+	adap->dev.of_node	= dev->of_node;
 	i2c_set_adapdata(adap, priv);
 	strlcpy(adap->name, pdev->name, sizeof(adap->name));
 
@@ -726,6 +741,7 @@ static struct platform_driver rcar_i2c_driver = {
 	.driver	= {
 		.name	= "i2c-rcar",
 		.owner	= THIS_MODULE,
+		.of_match_table = rcar_i2c_dt_ids,
 	},
 	.probe		= rcar_i2c_probe,
 	.remove		= rcar_i2c_remove,
-- 
1.7.2.5

[PATCH 4/5] i2c: rcar: fix clk_get() error handling

[Guennadi Liakhovetski]

When clk_get() fails, it returns an error code, not a NULL. This patch
fixes such an error handling bug.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
---
 drivers/i2c/busses/i2c-rcar.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index be801b9..7e71cf4 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -234,9 +234,9 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv,
 	u32 cdf_width;
 	unsigned long rate;
 
-	if (!clkp) {
-		dev_err(dev, "there is no peripheral_clk\n");
-		return -EIO;
+	if (IS_ERR(clkp)) {
+		dev_err(dev, "couldn't get clock\n");
+		return PTR_ERR(clkp);
 	}
 
 	switch (priv->devtype) {
-- 
1.7.2.5

[PATCH 5/5] i2c: rcar: use per-device clock

[Guennadi Liakhovetski]

Using the same clock for all device instances is non-portable and obtaining
clock references by an ID without using a device pointer is discouraged.
This is also not needed, because on platforms, where this driver is used,
suitable clocks are available for the I2C controllers, that are children of
the peripheral clock and just pass its rate 1-to-1 to controllers. This
patch switches the driver to obtain references to correct clocks.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-rcar.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 7e71cf4..7b986cb 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -227,7 +227,7 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv,
 				    u32 bus_speed,
 				    struct device *dev)
 {
-	struct clk *clkp = clk_get(NULL, "peripheral_clk");
+	struct clk *clkp = clk_get(dev, NULL);
 	u32 scgd, cdf;
 	u32 round, ick;
 	u32 scl;
-- 
1.7.2.5

Re: [PATCH 2/5] i2c: rcar: get clock rate only once and simplify calculation

[Magnus Damm]

Hi Guennadi,

On Tue, Sep 10, 2013 at 12:55 AM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> There is no need to repeatedly query clock frequency, where it is not
> expected to change. The complete loop can also trivially be replaced with
> a simple division. A further loop below the one, being simplified, could
> also be replaced, but that would get more complicated.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>

Thanks for your efforts. What makes you think that you should assume
that the clock frequency doesn't change?

As you already know, we want drivers to be reusable across multiple
SoCs. Assuming it that it would be fixed based on one particular SoC
doesn't guarantee that that's the case on other SoCs. Which SoCs did
you take into consideration?

Thanks,

/ magnus

Re: [PATCH 3/5] i2c: rcar: add Device Tree support

[Magnus Damm]

Hi Guennadi,

[CC Simon, Laurent]

On Tue, Sep 10, 2013 at 12:55 AM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> This patch adds Device Tree support to the i2c-rcar driver and respective
> documentation.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-rcar.txt |   22 ++++++++++++++++++++
>  drivers/i2c/busses/i2c-rcar.c                      |   20 ++++++++++++++++-
>  2 files changed, 40 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-rcar.txt
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-rcar.txt b/Documentation/devicetree/bindings/i2c/i2c-rcar.txt
> new file mode 100644
> index 0000000..b3c030b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-rcar.txt
> @@ -0,0 +1,22 @@
> +I2C for R-Car platforms
> +
> +Required properties:
> +- compatible: Must be one of
> +       "renesas,i2c-rcar"
> +       "renesas,i2c-rcar-h1"
> +       "renesas,i2c-rcar-h2"

Is this following the same style as other DT patches? It looks to me
that you may want to use r8a7779 and r8a7790 instead of h1 and h2.

Simon, Laurent, what do you think about this?

/ magnus

Re: [PATCH 5/5] i2c: rcar: use per-device clock

[Magnus Damm]

Hi Guennadi,

On Tue, Sep 10, 2013 at 12:55 AM, Guennadi Liakhovetski
<g.liakhovetski-Mmb7MZpHnFY@public.gmane.org> wrote:
> Using the same clock for all device instances is non-portable and obtaining
> clock references by an ID without using a device pointer is discouraged.
> This is also not needed, because on platforms, where this driver is used,
> suitable clocks are available for the I2C controllers, that are children of
> the peripheral clock and just pass its rate 1-to-1 to controllers. This
> patch switches the driver to obtain references to correct clocks.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-rcar.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index 7e71cf4..7b986cb 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -227,7 +227,7 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv,
>                                     u32 bus_speed,
>                                     struct device *dev)
>  {
> -       struct clk *clkp = clk_get(NULL, "peripheral_clk");
> +       struct clk *clkp = clk_get(dev, NULL);
>         u32 scgd, cdf;
>         u32 round, ick;
>         u32 scl;

I agree that passing struct device to clk_get() is a good idea. Thanks
for spotting this.

What are the run-time dependencies for this patch? Do all affected I2C
controllers have MSTP bits with proper parents already, or will this
patch cause breakage for some SoCs?

Thanks,

/ magnus

Re: [PATCH 3/5] i2c: rcar: add Device Tree support

[Laurent Pinchart]

Hi Magnus,

On Wednesday 11 September 2013 07:40:54 Magnus Damm wrote:
> Hi Guennadi,
> 
> [CC Simon, Laurent]
> 
> On Tue, Sep 10, 2013 at 12:55 AM, Guennadi Liakhovetski wrote:
> > This patch adds Device Tree support to the i2c-rcar driver and respective
> > documentation.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> > 
> >  Documentation/devicetree/bindings/i2c/i2c-rcar.txt |   22 +++++++++++++++
> >  drivers/i2c/busses/i2c-rcar.c                      |   20 ++++++++++++++-
> >  2 files changed, 40 insertions(+), 2 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-rcar.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/i2c/i2c-rcar.txt
> > b/Documentation/devicetree/bindings/i2c/i2c-rcar.txt new file mode 100644
> > index 0000000..b3c030b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/i2c/i2c-rcar.txt
> > @@ -0,0 +1,22 @@
> > +I2C for R-Car platforms
> > +
> > +Required properties:
> > +- compatible: Must be one of
> > +       "renesas,i2c-rcar"
> > +       "renesas,i2c-rcar-h1"
> > +       "renesas,i2c-rcar-h2"
> 
> Is this following the same style as other DT patches? It looks to me
> that you may want to use r8a7779 and r8a7790 instead of h1 and h2.
> 
> Simon, Laurent, what do you think about this?

I think that should be "renesas,i2c-r8a7779" and "renesas,i2c-r8a7790", yes.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/5] i2c: rcar: add Device Tree support

[Magnus Damm]

Hi Laurent,

On Wed, Sep 11, 2013 at 7:46 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> On Wednesday 11 September 2013 07:40:54 Magnus Damm wrote:
>> Hi Guennadi,
>>
>> [CC Simon, Laurent]
>>
>> On Tue, Sep 10, 2013 at 12:55 AM, Guennadi Liakhovetski wrote:
>> > This patch adds Device Tree support to the i2c-rcar driver and respective
>> > documentation.
>> >
>> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
>> > ---
>> >
>> >  Documentation/devicetree/bindings/i2c/i2c-rcar.txt |   22 +++++++++++++++
>> >  drivers/i2c/busses/i2c-rcar.c                      |   20 ++++++++++++++-
>> >  2 files changed, 40 insertions(+), 2 deletions(-)
>> >  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-rcar.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/i2c/i2c-rcar.txt
>> > b/Documentation/devicetree/bindings/i2c/i2c-rcar.txt new file mode 100644
>> > index 0000000..b3c030b
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/i2c/i2c-rcar.txt
>> > @@ -0,0 +1,22 @@
>> > +I2C for R-Car platforms
>> > +
>> > +Required properties:
>> > +- compatible: Must be one of
>> > +       "renesas,i2c-rcar"
>> > +       "renesas,i2c-rcar-h1"
>> > +       "renesas,i2c-rcar-h2"
>>
>> Is this following the same style as other DT patches? It looks to me
>> that you may want to use r8a7779 and r8a7790 instead of h1 and h2.
>>
>> Simon, Laurent, what do you think about this?
>
> I think that should be "renesas,i2c-r8a7779" and "renesas,i2c-r8a7790", yes.

Thanks for your comments. I recall that r8a7790 has a bunch of
channels and at least two types of i2c controllers. I wonder what the
proper naming would be then...

/ magnus

Re: [PATCH 2/5] i2c: rcar: get clock rate only once and simplify calculation

[Guennadi Liakhovetski]

Hi Magnus

Thanks for your comments

On Wed, 11 Sep 2013, Magnus Damm wrote:

> Hi Guennadi,
> 
> On Tue, Sep 10, 2013 at 12:55 AM, Guennadi Liakhovetski
> <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org> wrote:
> > There is no need to repeatedly query clock frequency, where it is not
> > expected to change. The complete loop can also trivially be replaced with
> > a simple division. A further loop below the one, being simplified, could
> > also be replaced, but that would get more complicated.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> Thanks for your efforts. What makes you think that you should assume
> that the clock frequency doesn't change?

Sorry, maybe I didn't explain well enough in the patch description. 
Please, have a look at the patch. It removes repeated clock rate readings 
inside a tight loop. I'm aware that the clock rate can change, but there 
should be a way to make sure, that beginning now and until a certain later 
point of time the rate doesn't change. Presumably this should be done by 
locking the clock. I'm not sure this is currently supported by the clock 
API. Neither clk_get(), nor clk_prepare() nor clk_enable() seem to have 
the semantics of locking the clock's frequency. Maybe the one who actually 
prepared the clock should become its owner, but I don't think this is 
currently the expected behaviour. In either case, I don't think calling 
clk_get_rate() repeatedly inside _that_ loop makes sense - the rate can 
anyway be changed after the loop, so, it doesn't add any security.

Thanks
Guennadi

> As you already know, we want drivers to be reusable across multiple
> SoCs. Assuming it that it would be fixed based on one particular SoC
> doesn't guarantee that that's the case on other SoCs. Which SoCs did
> you take into consideration?

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 5/5] i2c: rcar: use per-device clock

[Guennadi Liakhovetski]

Hi Magnus,

On Wed, 11 Sep 2013, Magnus Damm wrote:

> Hi Guennadi,
> 
> On Tue, Sep 10, 2013 at 12:55 AM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > Using the same clock for all device instances is non-portable and obtaining
> > clock references by an ID without using a device pointer is discouraged.
> > This is also not needed, because on platforms, where this driver is used,
> > suitable clocks are available for the I2C controllers, that are children of
> > the peripheral clock and just pass its rate 1-to-1 to controllers. This
> > patch switches the driver to obtain references to correct clocks.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
> > ---
> >  drivers/i2c/busses/i2c-rcar.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> > index 7e71cf4..7b986cb 100644
> > --- a/drivers/i2c/busses/i2c-rcar.c
> > +++ b/drivers/i2c/busses/i2c-rcar.c
> > @@ -227,7 +227,7 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv,
> >                                     u32 bus_speed,
> >                                     struct device *dev)
> >  {
> > -       struct clk *clkp = clk_get(NULL, "peripheral_clk");
> > +       struct clk *clkp = clk_get(dev, NULL);
> >         u32 scgd, cdf;
> >         u32 round, ick;
> >         u32 scl;
> 
> I agree that passing struct device to clk_get() is a good idea. Thanks
> for spotting this.
> 
> What are the run-time dependencies for this patch? Do all affected I2C
> controllers have MSTP bits with proper parents already, or will this
> patch cause breakage for some SoCs?

Currently (Simon's devel branch of 5 days ago as of commit 5cbe867) I only 
see r8a7778 and r8a7779 using i2c-rcar. They both define clocks like

	CLKDEV_DEV_ID("i2c-rcar.0", &mstp_clks[MSTP030]), /* I2C0 */

and similarly for I2C1 - I2C3. So, it looks like it should work, but would 
be nice to test.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 3/5] i2c: rcar: add Device Tree support

[Guennadi Liakhovetski]

Hi Magnus, Laurent

On Wed, 11 Sep 2013, Magnus Damm wrote:

> Hi Laurent,
> 
> On Wed, Sep 11, 2013 at 7:46 AM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > Hi Magnus,
> >
> > On Wednesday 11 September 2013 07:40:54 Magnus Damm wrote:
> >> Hi Guennadi,
> >>
> >> [CC Simon, Laurent]
> >>
> >> On Tue, Sep 10, 2013 at 12:55 AM, Guennadi Liakhovetski wrote:
> >> > This patch adds Device Tree support to the i2c-rcar driver and respective
> >> > documentation.
> >> >
> >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
> >> > ---
> >> >
> >> >  Documentation/devicetree/bindings/i2c/i2c-rcar.txt |   22 +++++++++++++++
> >> >  drivers/i2c/busses/i2c-rcar.c                      |   20 ++++++++++++++-
> >> >  2 files changed, 40 insertions(+), 2 deletions(-)
> >> >  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-rcar.txt
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/i2c/i2c-rcar.txt
> >> > b/Documentation/devicetree/bindings/i2c/i2c-rcar.txt new file mode 100644
> >> > index 0000000..b3c030b
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/i2c/i2c-rcar.txt
> >> > @@ -0,0 +1,22 @@
> >> > +I2C for R-Car platforms
> >> > +
> >> > +Required properties:
> >> > +- compatible: Must be one of
> >> > +       "renesas,i2c-rcar"
> >> > +       "renesas,i2c-rcar-h1"
> >> > +       "renesas,i2c-rcar-h2"
> >>
> >> Is this following the same style as other DT patches? It looks to me
> >> that you may want to use r8a7779 and r8a7790 instead of h1 and h2.
> >>
> >> Simon, Laurent, what do you think about this?
> >
> > I think that should be "renesas,i2c-r8a7779" and "renesas,i2c-r8a7790", yes.

Yes, agree, those names look better. I modelled my names after the 
respective strings in the struct platform_device_id table in the driver, 
looks like that wasn't a very good idea. Yes, I realise that those 
platform device ID don't matter much - they aren't a part of any ABI 
(fortunately), can freely be changed at any time, but maybe it would be 
good to also try to create them according to a certain pattern... But yes, 
that's unrelated.

> Thanks for your comments. I recall that r8a7790 has a bunch of
> channels and at least two types of i2c controllers. I wonder what the
> proper naming would be then...

You're right. r8a7790 has 4 I2C interfaces, each of them can be used with 
either an i2c-sh_mobile - compatible or an i2c-rcar - compatible 
controller instance. Some of those interfaces can also be configured to 
use one of several pin groups.

The i2c-sh_mobile driver already has DT support and it's compatibility 
string, as added by your patch "i2c: sh_mobile: add device tree support" 
of almost 1.5 years ago, is "renesas,rmobile-iic", so, it wouldn't clash 
with the proposed "renesas,i2c-<soc>" naming scheme for i2c-rcar.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/