[PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Lee Jones]

Here we apply the bindings required for successful Device Tree
probing of the i2c-nomadik driver.

Cc: linux-i2c@vger.kernel.org
Acked-by: srinidhi kasagar <srinidhi.kasagar@stericsson.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/i2c/busses/i2c-nomadik.c |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 61b00ed..8168389 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -25,6 +25,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/pm_runtime.h>
 #include <linux/platform_data/i2c-nomadik.h>
+#include <linux/of.h>
 
 #define DRIVER_NAME "nmk-i2c"
 
@@ -920,15 +921,42 @@ static struct nmk_i2c_controller u8500_i2c = {
 	.sm             = I2C_FREQ_MODE_FAST,
 };
 
+static void nmk_i2c_of_probe(struct device_node *np,
+			struct nmk_i2c_controller *pdata)
+{
+	/* Provide the default configuration as a base. */
+	pdata = &u8500_i2c;
+
+	of_property_read_u32(np, "clock-frequency", (u32*)&pdata->clk_freq);
+
+	/* This driver only supports 'standard' and 'fast' modes of operation. */
+	if (pdata->clk_freq <= 100000)
+		pdata->sm = I2C_FREQ_MODE_STANDARD;
+	else
+		pdata->sm = I2C_FREQ_MODE_FAST;
+}
+
 static atomic_t adapter_id = ATOMIC_INIT(0);
 
 static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 {
 	int ret = 0;
 	struct nmk_i2c_controller *pdata = adev->dev.platform_data;
+	struct device_node *np = adev->dev.of_node;
 	struct nmk_i2c_dev	*dev;
 	struct i2c_adapter *adap;
 
+	if (np) {
+		if (!pdata) {
+			pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL);
+			if (!pdata) {
+				ret = -ENOMEM;
+				goto err_no_mem;
+			}
+		}
+		nmk_i2c_of_probe(np, pdata);
+	}
+
 	if (!pdata)
 		/* No i2c configuration found, using the default. */
 		pdata = &u8500_i2c;
-- 
1.7.9.5

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Linus Walleij]

On Thu, Aug 23, 2012 at 8:01 AM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> Here we apply the bindings required for successful Device Tree
> probing of the i2c-nomadik driver.
>
> Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Acked-by: srinidhi kasagar <srinidhi.kasagar-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>
> Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Acked-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Yours,
Linus Walleij

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Lee Jones]

On Mon, Aug 27, 2012 at 04:42:49PM -0700, Linus Walleij wrote:
> On Thu, Aug 23, 2012 at 8:01 AM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> 
> > Here we apply the bindings required for successful Device Tree
> > probing of the i2c-nomadik driver.
> >
> > Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Acked-by: srinidhi kasagar <srinidhi.kasagar-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>
> > Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> 
> Acked-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

I'm assuming I still need Wolfram's Ack on this?

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 2938 bytes --]

On Thu, Aug 23, 2012 at 04:01:27PM +0100, Lee Jones wrote:
> Here we apply the bindings required for successful Device Tree
> probing of the i2c-nomadik driver.
> 
> Cc: linux-i2c@vger.kernel.org
> Acked-by: srinidhi kasagar <srinidhi.kasagar@stericsson.com>

Two acks? I'd think this cannot work for multiple reasons.

BTW, patch 2 and 3 should be merged. It is a lot easier to review the
code with the binding description together.

Is there some dependency other than updating the dts files? If not, I'd
like to pick up the patch via I2C.

> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/i2c/busses/i2c-nomadik.c |   28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> index 61b00ed..8168389 100644
> --- a/drivers/i2c/busses/i2c-nomadik.c
> +++ b/drivers/i2c/busses/i2c-nomadik.c
> @@ -25,6 +25,7 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/platform_data/i2c-nomadik.h>
> +#include <linux/of.h>
>  
>  #define DRIVER_NAME "nmk-i2c"
>  
> @@ -920,15 +921,42 @@ static struct nmk_i2c_controller u8500_i2c = {
>  	.sm             = I2C_FREQ_MODE_FAST,
>  };
>  
> +static void nmk_i2c_of_probe(struct device_node *np,
> +			struct nmk_i2c_controller *pdata)
> +{
> +	/* Provide the default configuration as a base. */
> +	pdata = &u8500_i2c;

?????? I wonder how that could work... have you tested the patch?

> +
> +	of_property_read_u32(np, "clock-frequency", (u32*)&pdata->clk_freq);

Might be worth changing clk_freq to u32?

> +
> +	/* This driver only supports 'standard' and 'fast' modes of operation. */
> +	if (pdata->clk_freq <= 100000)
> +		pdata->sm = I2C_FREQ_MODE_STANDARD;

Is standard == 100000 Hz?


> +	else
> +		pdata->sm = I2C_FREQ_MODE_FAST;

If those two are fixed frequencies, you should omit a warning if the
devicetree has a different frequency set and report which one is going
to be used actually.


> +}
> +
>  static atomic_t adapter_id = ATOMIC_INIT(0);
>  
>  static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
>  {
>  	int ret = 0;
>  	struct nmk_i2c_controller *pdata = adev->dev.platform_data;
> +	struct device_node *np = adev->dev.of_node;
>  	struct nmk_i2c_dev	*dev;
>  	struct i2c_adapter *adap;
>  
> +	if (np) {
> +		if (!pdata) {
> +			pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL);
> +			if (!pdata) {
> +				ret = -ENOMEM;
> +				goto err_no_mem;
> +			}
> +		}
> +		nmk_i2c_of_probe(np, pdata);
> +	}
> +
>  	if (!pdata)
>  		/* No i2c configuration found, using the default. */
>  		pdata = &u8500_i2c;
> -- 
> 1.7.9.5
> 

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Lee Jones]

> Is there some dependency other than updating the dts files? If not, I'd
> like to pick up the patch via I2C.

There's no other dependency. Feel free to take them into your tree.

> > +static void nmk_i2c_of_probe(struct device_node *np,
> > +			struct nmk_i2c_controller *pdata)
> > +{
> > +	/* Provide the default configuration as a base. */
> > +	pdata = &u8500_i2c;
> 
> ?????? I wonder how that could work... have you tested the patch?

Wow, that's a great spot!

I have tested the patch, but I don't have any i2c devices, so can't
test full functionality. I, wrongly it seems, assumed there would be
a complaint from the I2C subsystem if any of the values seemed wrong.
What I will do before next submission is print out the entire pdata
structure to ensure it's populated in the correct way.

> > +
> > +	of_property_read_u32(np, "clock-frequency", (u32*)&pdata->clk_freq);
> 
> Might be worth changing clk_freq to u32?

Yes, makes sense. 

> > +
> > +	/* This driver only supports 'standard' and 'fast' modes of operation. */
> > +	if (pdata->clk_freq <= 100000)
> > +		pdata->sm = I2C_FREQ_MODE_STANDARD;
> 
> Is standard == 100000 Hz?

Well it depends on how you interpret the comments in:
     include/linux/platform_data/i2c-nomadik.h

enum i2c_freq_mode {
        I2C_FREQ_MODE_STANDARD,         /* up to 100 Kb/s */
        I2C_FREQ_MODE_FAST,             /* up to 400 Kb/s */
        I2C_FREQ_MODE_HIGH_SPEED,       /* up to 3.4 Mb/s */
        I2C_FREQ_MODE_FAST_PLUS,        /* up to 1 Mb/s */
};

I guess your guess would be better than mine.

> > +	else
> > +		pdata->sm = I2C_FREQ_MODE_FAST;
> 
> If those two are fixed frequencies, you should omit a warning if the
> devicetree has a different frequency set and report which one is going
> to be used actually.

Well, again by the comments above I would say that in between values
were valid, but I'm willing to bow down to your knowledge if you think
they are fixed values?

Thanks for reviewing.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Lee Jones]

Hopefully this is more to your liking:

Author: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Date:   Mon Aug 6 11:09:57 2012 +0100

    i2c: nomadik: Add Device Tree support to the Nomadik I2C driver
    
    Here we apply the bindings required for successful Device Tree
    probing of the i2c-nomadik driver.
    
    Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
    Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 61b00ed..2c85de1 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -25,6 +25,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/pm_runtime.h>
 #include <linux/platform_data/i2c-nomadik.h>
+#include <linux/of.h>
 
 #define DRIVER_NAME "nmk-i2c"
 
@@ -920,15 +921,41 @@ static struct nmk_i2c_controller u8500_i2c = {
        .sm             = I2C_FREQ_MODE_FAST,
 };
 
+static void nmk_i2c_of_probe(struct device_node *np,
+                       struct nmk_i2c_controller *pdata)
+{
+       of_property_read_u32(np, "clock-frequency", &pdata->clk_freq);
+
+       /* This driver only supports 'standard' and 'fast' modes of operation. */
+       if (pdata->clk_freq <= 100000)
+               pdata->sm = I2C_FREQ_MODE_STANDARD;
+       else
+               pdata->sm = I2C_FREQ_MODE_FAST;
+}
+
 static atomic_t adapter_id = ATOMIC_INIT(0);
 
 static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 {
        int ret = 0;
        struct nmk_i2c_controller *pdata = adev->dev.platform_data;
+       struct device_node *np = adev->dev.of_node;
        struct nmk_i2c_dev      *dev;
        struct i2c_adapter *adap;
 
+       if (np) {
+               if (!pdata) {
+                       pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL);
+                       if (!pdata) {
+                               ret = -ENOMEM;
+                               goto err_no_mem;
+                       }
+               }
+               /* Provide the default configuration as a base. */
+               pdata = &u8500_i2c;
+               nmk_i2c_of_probe(np, pdata);
+       }
+
        if (!pdata)
                /* No i2c configuration found, using the default. */
                pdata = &u8500_i2c;
diff --git a/include/linux/platform_data/i2c-nomadik.h b/include/linux/platform_data/i2c-nomadik.h
index c2303c3..3a8be9c 100644
--- a/include/linux/platform_data/i2c-nomadik.h
+++ b/include/linux/platform_data/i2c-nomadik.h
@@ -28,7 +28,7 @@ enum i2c_freq_mode {
  * @sm:                speed mode
  */
 struct nmk_i2c_controller {
-       unsigned long   clk_freq;
+       u32             clk_freq;
        unsigned short  slsu;
        unsigned char   tft;
        unsigned char   rft;

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Linus Walleij]

On Fri, Aug 31, 2012 at 2:23 PM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

(...)
>  static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
>  {
>         int ret = 0;
>         struct nmk_i2c_controller *pdata = adev->dev.platform_data;
> +       struct device_node *np = adev->dev.of_node;
>         struct nmk_i2c_dev      *dev;
>         struct i2c_adapter *adap;
>
> +       if (np) {
> +               if (!pdata) {

So, if no pdata is provided, we go on to allocate some ...

> +                       pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL);
> +                       if (!pdata) {
> +                               ret = -ENOMEM;
> +                               goto err_no_mem;
> +                       }
> +               }
> +               /* Provide the default configuration as a base. */
> +               pdata = &u8500_i2c;

Then you just override that pointer with a pointer to the local config.

> +               nmk_i2c_of_probe(np, pdata);
> +       }
> +
>         if (!pdata)
>                 /* No i2c configuration found, using the default. */
>                 pdata = &u8500_i2c;

This in it's entirety does not look sound. I *think* this is what you
want to do,
replace all of the above codde (including the last if (!pdata) clause) with:

if (!pdata) {
    /* If no platform data passed in, use the default configuration as
a base. */
    pdata = &u8500_i2c;
    if (np)
        /* Further, if we have a DT node, override the default with this */
        nmk_i2c_of_probe(np, pdata);
}

This makes any passed pdata take precedence, else default pdata
complemented with DT info. Which is what we want.

Yours,
Linus Walleij

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Wolfram Sang]

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

On Mon, Sep 03, 2012 at 11:22:28AM +0200, Linus Walleij wrote:
> On Fri, Aug 31, 2012 at 2:23 PM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> 
> (...)
> >  static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
> >  {
> >         int ret = 0;
> >         struct nmk_i2c_controller *pdata = adev->dev.platform_data;
> > +       struct device_node *np = adev->dev.of_node;
> >         struct nmk_i2c_dev      *dev;
> >         struct i2c_adapter *adap;
> >
> > +       if (np) {
> > +               if (!pdata) {
> 
> So, if no pdata is provided, we go on to allocate some ...
> 
> > +                       pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL);
> > +                       if (!pdata) {
> > +                               ret = -ENOMEM;
> > +                               goto err_no_mem;
> > +                       }
> > +               }
> > +               /* Provide the default configuration as a base. */
> > +               pdata = &u8500_i2c;
> 
> Then you just override that pointer with a pointer to the local config.
> 
> > +               nmk_i2c_of_probe(np, pdata);
> > +       }
> > +
> >         if (!pdata)
> >                 /* No i2c configuration found, using the default. */
> >                 pdata = &u8500_i2c;
> 
> This in it's entirety does not look sound. I *think* this is what you
> want to do,
> replace all of the above codde (including the last if (!pdata) clause) with:
> 
> if (!pdata) {
>     /* If no platform data passed in, use the default configuration as
> a base. */
>     pdata = &u8500_i2c;
>     if (np)
>         /* Further, if we have a DT node, override the default with this */
>         nmk_i2c_of_probe(np, pdata);
> }
> 
> This makes any passed pdata take precedence, else default pdata
> complemented with DT info. Which is what we want.

No. of_probe modifies pdata which in this case the default config which
might already be in use. So, you will get problems if you have two
instances with different configuration. So, we need to allocate memory
but copy the content of the default data. The patch above just copies
the pointer which is bogus.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Lee Jones]

On Mon, Sep 03, 2012 at 11:44:48AM +0200, Wolfram Sang wrote:
> On Mon, Sep 03, 2012 at 11:22:28AM +0200, Linus Walleij wrote:
> > On Fri, Aug 31, 2012 at 2:23 PM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > 
> > (...)
> > >  static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
> > >  {
> > >         int ret = 0;
> > >         struct nmk_i2c_controller *pdata = adev->dev.platform_data;
> > > +       struct device_node *np = adev->dev.of_node;
> > >         struct nmk_i2c_dev      *dev;
> > >         struct i2c_adapter *adap;
> > >
> > > +       if (np) {
> > > +               if (!pdata) {
> > 
> > So, if no pdata is provided, we go on to allocate some ...
> > 
> > > +                       pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL);
> > > +                       if (!pdata) {
> > > +                               ret = -ENOMEM;
> > > +                               goto err_no_mem;
> > > +                       }
> > > +               }
> > > +               /* Provide the default configuration as a base. */
> > > +               pdata = &u8500_i2c;
> > 
> > Then you just override that pointer with a pointer to the local config.
> > 
> > > +               nmk_i2c_of_probe(np, pdata);
> > > +       }
> > > +
> > >         if (!pdata)
> > >                 /* No i2c configuration found, using the default. */
> > >                 pdata = &u8500_i2c;
> > 
> > This in it's entirety does not look sound. I *think* this is what you
> > want to do,
> > replace all of the above codde (including the last if (!pdata) clause) with:
> > 
> > if (!pdata) {
> >     /* If no platform data passed in, use the default configuration as
> > a base. */
> >     pdata = &u8500_i2c;
> >     if (np)
> >         /* Further, if we have a DT node, override the default with this */
> >         nmk_i2c_of_probe(np, pdata);
> > }
> > 
> > This makes any passed pdata take precedence, else default pdata
> > complemented with DT info. Which is what we want.
> 
> No. of_probe modifies pdata which in this case the default config which
> might already be in use. So, you will get problems if you have two
> instances with different configuration. So, we need to allocate memory
> but copy the content of the default data. The patch above just copies
> the pointer which is bogus.

Agreed. I'll fixup.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Lee Jones]

Author: Lee Jones <lee.jones@linaro.org>
Date:   Mon Aug 6 11:09:57 2012 +0100

    i2c: nomadik: Add Device Tree support to the Nomadik I2C driver
    
    Here we apply the bindings required for successful Device Tree
    probing of the i2c-nomadik driver.
    
    Cc: linux-i2c@vger.kernel.org
    Signed-off-by: Lee Jones <lee.jones@linaro.org>

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 61b00ed..c2da711 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -25,6 +25,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/pm_runtime.h>
 #include <linux/platform_data/i2c-nomadik.h>
+#include <linux/of.h>
 
 #define DRIVER_NAME "nmk-i2c"
 
@@ -920,15 +921,42 @@ static struct nmk_i2c_controller u8500_i2c = {
        .sm             = I2C_FREQ_MODE_FAST,
 };
 
+static void nmk_i2c_of_probe(struct device_node *np,
+                       struct nmk_i2c_controller *pdata)
+{
+       of_property_read_u32(np, "clock-frequency", &pdata->clk_freq);
+
+       /* This driver only supports 'standard' and 'fast' modes of operation. */
+       if (pdata->clk_freq <= 100000)
+               pdata->sm = I2C_FREQ_MODE_STANDARD;
+       else
+               pdata->sm = I2C_FREQ_MODE_FAST;
+}
+
 static atomic_t adapter_id = ATOMIC_INIT(0);
 
 static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 {
        int ret = 0;
        struct nmk_i2c_controller *pdata = adev->dev.platform_data;
+       struct device_node *np = adev->dev.of_node;
        struct nmk_i2c_dev      *dev;
        struct i2c_adapter *adap;
 
+       if (np) {
+               if (!pdata) {
+                       pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL);
+                       if (!pdata) {
+                               ret = -ENOMEM;
+                               goto err_no_mem;
+                       }
+               }
+               /* Provide the default configuration as a base. */
+               memcpy(pdata, &u8500_i2c, sizeof(struct nmk_i2c_controller));
+
+               nmk_i2c_of_probe(np, pdata);
+       }
+
        if (!pdata)
                /* No i2c configuration found, using the default. */
                pdata = &u8500_i2c;
diff --git a/include/linux/platform_data/i2c-nomadik.h b/include/linux/platform_data/i2c-nomadik.h
index c2303c3..3a8be9c 100644
--- a/include/linux/platform_data/i2c-nomadik.h
+++ b/include/linux/platform_data/i2c-nomadik.h
@@ -28,7 +28,7 @@ enum i2c_freq_mode {
  * @sm:                speed mode
  */
 struct nmk_i2c_controller {
-       unsigned long   clk_freq;
+       u32             clk_freq;
        unsigned short  slsu;
        unsigned char   tft;
        unsigned char   rft;

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Linus Walleij]

On Mon, Sep 3, 2012 at 12:07 PM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

(...)
> +       if (np) {
> +               if (!pdata) {
> +                       pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL);
> +                       if (!pdata) {
> +                               ret = -ENOMEM;
> +                               goto err_no_mem;
> +                       }
> +               }
> +               /* Provide the default configuration as a base. */
> +               memcpy(pdata, &u8500_i2c, sizeof(struct nmk_i2c_controller));

Here you blank out any pdata passed from say a board file or
whatever if pdata != NULL.

> +
> +               nmk_i2c_of_probe(np, pdata);
> +       }
> +
>         if (!pdata)
>                 /* No i2c configuration found, using the default. */
>                 pdata = &u8500_i2c;

So this is still wrong, if pdata is passed to the driver it will
not override the DT, you have the semantics the other way
around, DT overrides pdata.

Look at the switch statement in my previous comment,
just add the allocations and a memcpy() and it still holds:

if (!pdata) {
    if (np) {
              pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL);
                      if (!pdata) {
                              ret = -ENOMEM;
                              goto err_no_mem;
                      }
              }
              /* Provide the default configuration as a base. */
              memcpy(pdata, &u8500_i2c, sizeof(struct nmk_i2c_controller));
              nmk_i2c_of_probe(np, pdata);
    } else
         /* Just use the static pdata */
         pdata = &u8500_i2c;
}

Yours,
Linus Walleij

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Lee Jones]

[-- Attachment #1.1: Type: text/plain, Size: 2225 bytes --]

You're right. I rushed it to get it out the door, as I'm working on
something else.

Leave it with me. I'll spend more time on the semantics before posting
again.

On 3 September 2012 12:07, Linus Walleij <linus.walleij@linaro.org> wrote:

> On Mon, Sep 3, 2012 at 12:07 PM, Lee Jones <lee.jones@linaro.org> wrote:
>
> (...)
> > +       if (np) {
> > +               if (!pdata) {
> > +                       pdata = devm_kzalloc(&adev->dev, sizeof(*pdata),
> GFP_KERNEL);
> > +                       if (!pdata) {
> > +                               ret = -ENOMEM;
> > +                               goto err_no_mem;
> > +                       }
> > +               }
> > +               /* Provide the default configuration as a base. */
> > +               memcpy(pdata, &u8500_i2c, sizeof(struct
> nmk_i2c_controller));
>
> Here you blank out any pdata passed from say a board file or
> whatever if pdata != NULL.
>
> > +
> > +               nmk_i2c_of_probe(np, pdata);
> > +       }
> > +
> >         if (!pdata)
> >                 /* No i2c configuration found, using the default. */
> >                 pdata = &u8500_i2c;
>
> So this is still wrong, if pdata is passed to the driver it will
> not override the DT, you have the semantics the other way
> around, DT overrides pdata.
>
> Look at the switch statement in my previous comment,
> just add the allocations and a memcpy() and it still holds:
>
> if (!pdata) {
>     if (np) {
>               pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL);
>                       if (!pdata) {
>                               ret = -ENOMEM;
>                               goto err_no_mem;
>                       }
>               }
>               /* Provide the default configuration as a base. */
>               memcpy(pdata, &u8500_i2c, sizeof(struct nmk_i2c_controller));
>               nmk_i2c_of_probe(np, pdata);
>     } else
>          /* Just use the static pdata */
>          pdata = &u8500_i2c;
> }
>
> Yours,
> Linus Walleij
>



-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[-- Attachment #1.2: Type: text/html, Size: 3120 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Lee Jones]

[-- Attachment #1.1: Type: text/plain, Size: 2429 bytes --]

Looking at this again ...

On 3 September 2012 12:07, Linus Walleij <linus.walleij@linaro.org> wrote:

> On Mon, Sep 3, 2012 at 12:07 PM, Lee Jones <lee.jones@linaro.org> wrote:
>
> (...)
> > +       if (np) {
> > +               if (!pdata) {
> > +                       pdata = devm_kzalloc(&adev->dev, sizeof(*pdata),
> GFP_KERNEL);
> > +                       if (!pdata) {
> > +                               ret = -ENOMEM;
> > +                               goto err_no_mem;
> > +                       }
> > +               }
> > +               /* Provide the default configuration as a base. */
> > +               memcpy(pdata, &u8500_i2c, sizeof(struct
> nmk_i2c_controller));
>
> Here you blank out any pdata passed from say a board file or
> whatever if pdata != NULL.
>

Right, this should be in 'if (!pdata) {}' . I'll correct this now.


>
> > +
> > +               nmk_i2c_of_probe(np, pdata);
> > +       }
> > +
> >         if (!pdata)
> >                 /* No i2c configuration found, using the default. */
> >                 pdata = &u8500_i2c;
>
> So this is still wrong, if pdata is passed to the driver it will
> not override the DT, you have the semantics the other way
> around, DT overrides pdata.
>

This is correct, however. If there is DT, it should override any platform
data.

Actually, if there is DT then no platform data should be passed in the
first place.


> Look at the switch statement in my previous comment,
> just add the allocations and a memcpy() and it still holds:
>
> if (!pdata) {
>     if (np) {
>               pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL);
>                       if (!pdata) {
>                               ret = -ENOMEM;
>                               goto err_no_mem;
>                       }
>               }
>               /* Provide the default configuration as a base. */
>               memcpy(pdata, &u8500_i2c, sizeof(struct nmk_i2c_controller));
>               nmk_i2c_of_probe(np, pdata);
>     } else
>          /* Just use the static pdata */
>          pdata = &u8500_i2c;
> }
>
>
No, this is wrong. Platform data should not override DT.

If DT is enabled and passed, it should have highest priority.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[-- Attachment #1.2: Type: text/html, Size: 3632 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Linus Walleij]

On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> No, this is wrong. Platform data should not override DT.
>
> If DT is enabled and passed, it should have highest priority.

Oh is that so.

Rob: do we have a clear consensus on this? Then we should document
it in Documentation/devicetree/usage-model.txt.

(That document isn't part of the binding I believe, so we could define
Linux-specific behaviours in it.)

I always thought it was the other way around, that pdata took priority.

Usecase: hardcoded bootloader passer erroneous DT to a platform.
No way out. What to do? Override with pdata.

Yours,
Linus Walleij

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Lee Jones]

On Mon, Sep 03, 2012 at 01:58:04PM +0200, Linus Walleij wrote:
> On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> 
> > No, this is wrong. Platform data should not override DT.
> >
> > If DT is enabled and passed, it should have highest priority.
> 
> Oh is that so.

:)

> Rob: do we have a clear consensus on this? Then we should document
> it in Documentation/devicetree/usage-model.txt.
> 
> (That document isn't part of the binding I believe, so we could define
> Linux-specific behaviours in it.)
> 
> I always thought it was the other way around, that pdata took priority.
> 
> Usecase: hardcoded bootloader passer erroneous DT to a platform.
> No way out. What to do? Override with pdata.
> 
> Yours,
> Linus Walleij

Hmmm... I see your point, but this won't work.

When booting DT booting take a different path and no platform data
is passed. We can't boot DT AND register devices with platform data
or else we will double probe every device. The only way to pass
pdata when booting with DT is with AUX_DATA() and that's a hack to
get around things we don't have support for yet. Up until now that
has been DMA bindings, clock and pinctrl names and call-backs.

If DT is corrupt or missing the kernel will boot using platform 
data, but np will always be NULL, so we don't have the problem you
were alluding to above.

Let me know if I didn't explain that well enough and I will have
another go.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Linus Walleij]

On Mon, Sep 3, 2012 at 2:34 PM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> When booting DT booting take a different path and no platform data
> is passed. We can't boot DT AND register devices with platform data
> or else we will double probe every device. The only way to pass
> pdata when booting with DT is with AUX_DATA() and that's a hack to
> get around things we don't have support for yet. Up until now that
> has been DMA bindings, clock and pinctrl names and call-backs.

So if we pass some augmented platform data using AUX_DATA()
that appears as pdata in this case, and gets discarded.

Thus we cannot use AUX_DATA() to override a broken, as in
"the interrupt number is wrong" device tree.

> If DT is corrupt or missing the kernel will boot using platform
> data, but np will always be NULL, so we don't have the problem you
> were alluding to above.

That was not the problem I had in mind.

I had a valid, but incorrect device tree in mind. I.e the device
is there, but with wrong base address, or wrong IRQ number.

If pdata takes precedence, we can use AUX_DATA() to
override such errors from the platform, since drivers/of/platform.c
helpfully pokes in the auxdata as the platform data.
I thought this was one of the reasons why auxdata exist
at all.

Or is the proper solution to runtime-patch the device tree
per se in such cases? How is that actually done then?

Yours,
Linus Walleij

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Lee Jones]

On Mon, Sep 03, 2012 at 03:19:13PM +0200, Linus Walleij wrote:
> On Mon, Sep 3, 2012 at 2:34 PM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> 
> > When booting DT booting take a different path and no platform data
> > is passed. We can't boot DT AND register devices with platform data
> > or else we will double probe every device. The only way to pass
> > pdata when booting with DT is with AUX_DATA() and that's a hack to
> > get around things we don't have support for yet. Up until now that
> > has been DMA bindings, clock and pinctrl names and call-backs.
> 
> So if we pass some augmented platform data using AUX_DATA()
> that appears as pdata in this case, and gets discarded.
> 
> Thus we cannot use AUX_DATA() to override a broken, as in
> "the interrupt number is wrong" device tree.

No, you cannot to that. You'd have to fix it in DT (which is easier).

> > If DT is corrupt or missing the kernel will boot using platform
> > data, but np will always be NULL, so we don't have the problem you
> > were alluding to above.
> 
> That was not the problem I had in mind.
> 
> I had a valid, but incorrect device tree in mind. I.e the device
> is there, but with wrong base address, or wrong IRQ number.
> 
> If pdata takes precedence, we can use AUX_DATA() to
> override such errors from the platform, since drivers/of/platform.c
> helpfully pokes in the auxdata as the platform data.

Yes, that's what happens.

> I thought this was one of the reasons why auxdata exist
> at all.

I don't think so. I've been told that AUXDATA is just a hack. My
aim is to rid the boardfile of all AUXDATA entries.

> Or is the proper solution to runtime-patch the device tree
> per se in such cases? How is that actually done then?

No, you can't do that either. DT is only read once at boot-time.

The correct solution would be to fix the broken DT.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Stephen Warren]

On 09/03/2012 05:58 AM, Linus Walleij wrote:
> On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones <lee.jones@linaro.org> wrote:
> 
>> No, this is wrong. Platform data should not override DT.
>>
>> If DT is enabled and passed, it should have highest priority.

No, that's wrong. If platform data is specified, it overrides DT, so
that if the DT needs any fixup, it can be provided using platform data.

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Linus Walleij]

On Mon, Sep 3, 2012 at 4:33 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> On 09/03/2012 05:58 AM, Linus Walleij wrote:
>> On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>
>>> No, this is wrong. Platform data should not override DT.
>>>
>>> If DT is enabled and passed, it should have highest priority.
>
> No, that's wrong. If platform data is specified, it overrides DT, so
> that if the DT needs any fixup, it can be provided using platform data.

Thanks Stephen, now there are two of us saying this, Lee please
follow this design pattern.

(Unless Rob/Grant start shouting counter-orders...)

Yours,
Linus Walleij

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Rob Herring]

On 09/03/2012 09:35 AM, Linus Walleij wrote:
> On Mon, Sep 3, 2012 at 4:33 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> On 09/03/2012 05:58 AM, Linus Walleij wrote:
>>> On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>
>>>> No, this is wrong. Platform data should not override DT.
>>>>
>>>> If DT is enabled and passed, it should have highest priority.
>>
>> No, that's wrong. If platform data is specified, it overrides DT, so
>> that if the DT needs any fixup, it can be provided using platform data.
> 
> Thanks Stephen, now there are two of us saying this, Lee please
> follow this design pattern.
> 
> (Unless Rob/Grant start shouting counter-orders...)

Ideally, you only use DT or platform_data and you override DT with a new
DTB. Hopefully we can ultimately remove platform_data or all but parts
that can't be described in DT (i.e. function callouts).

But if you are handling both, then I agree that platform_data should
override DT.

Rob

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Lee Jones]

On Mon, Sep 03, 2012 at 10:09:34AM -0500, Rob Herring wrote:
> On 09/03/2012 09:35 AM, Linus Walleij wrote:
> > On Mon, Sep 3, 2012 at 4:33 PM, Stephen Warren <swarren@nvidia.com> wrote:
> >> On 09/03/2012 05:58 AM, Linus Walleij wrote:
> >>> On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >>>
> >>>> No, this is wrong. Platform data should not override DT.
> >>>>
> >>>> If DT is enabled and passed, it should have highest priority.
> >>
> >> No, that's wrong. If platform data is specified, it overrides DT, so
> >> that if the DT needs any fixup, it can be provided using platform data.
> > 
> > Thanks Stephen, now there are two of us saying this, Lee please
> > follow this design pattern.
> > 
> > (Unless Rob/Grant start shouting counter-orders...)
> 
> Ideally, you only use DT or platform_data and you override DT with a new
> DTB. Hopefully we can ultimately remove platform_data or all but parts
> that can't be described in DT (i.e. function callouts).

Exactly. I don't believe that AUX_DATA() should be used as a facility to
override DT settings from platform_data.

> But if you are handling both, then I agree that platform_data should
> override DT.

I do agree with this, but I haven't stumbled over such a use-case yet.
I have only provided; clock names, DMA settings and call-back information
via AUX_DATA() thus far, and those are being removed too when a) the 
correct bindings are mainlined and b) I have the time.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Arnd Bergmann]

On Monday 03 September 2012, Lee Jones wrote:
> > But if you are handling both, then I agree that platform_data should
> > override DT.
> 
> I do agree with this, but I haven't stumbled over such a use-case yet.
> I have only provided; clock names, DMA settings and call-back information
> via AUX_DATA() thus far, and those are being removed too when a) the 
> correct bindings are mainlined and b) I have the time.

I'd prefer if you just disallow the case where pdata and DT have conflicting
information. We don't seem to have a clear rule that is enforced over the
kernel, so I don't think we can rely on either one taking precedence over
the other in general.

In this particular case, we don't have a single board file providing a
struct nmk_i2c_controller definition for platform data, so the best way
to handle this IMHO is to remove the header file with the platform
data definition, and just encode the defaults in the driver.

	Arnd

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Linus Walleij]

On Tue, Sep 4, 2012 at 4:28 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:

> In this particular case, we don't have a single board file providing a
> struct nmk_i2c_controller definition for platform data, so the best way
> to handle this IMHO is to remove the header file with the platform
> data definition, and just encode the defaults in the driver.

Alessandro Rubini is actively working on bridging this (and
other amba_device primecells) to PCI, that is the reason why it
was recently converted to an amba_device. How is he then supposed to
get the proper parameters into the driver? Note that the PCI ID
is no help at all since the parameters depend on what is connected
to the I2C bus, not on what it itself is connected to. Isn't platform data
used in such cases?

Yours,
Linus Walleij

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Lee Jones]

On Tue, Sep 04, 2012 at 07:27:10PM +0200, Linus Walleij wrote:
> On Tue, Sep 4, 2012 at 4:28 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> 
> > In this particular case, we don't have a single board file providing a
> > struct nmk_i2c_controller definition for platform data, so the best way
> > to handle this IMHO is to remove the header file with the platform
> > data definition, and just encode the defaults in the driver.
> 
> Alessandro Rubini is actively working on bridging this (and
> other amba_device primecells) to PCI, that is the reason why it
> was recently converted to an amba_device. How is he then supposed to
> get the proper parameters into the driver? Note that the PCI ID
> is no help at all since the parameters depend on what is connected
> to the I2C bus, not on what it itself is connected to. Isn't platform data
> used in such cases?

So why can't Alessandro continue to use Platform Data in the normal way?

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Linus Walleij]

On Wed, Sep 5, 2012 at 8:41 AM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Sep 04, 2012 at 07:27:10PM +0200, Linus Walleij wrote:
>> On Tue, Sep 4, 2012 at 4:28 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>>
>> > In this particular case, we don't have a single board file providing a
>> > struct nmk_i2c_controller definition for platform data, so the best way
>> > to handle this IMHO is to remove the header file with the platform
>> > data definition, and just encode the defaults in the driver.
>>
>> Alessandro Rubini is actively working on bridging this (and
>> other amba_device primecells) to PCI, that is the reason why it
>> was recently converted to an amba_device. How is he then supposed to
>> get the proper parameters into the driver? Note that the PCI ID
>> is no help at all since the parameters depend on what is connected
>> to the I2C bus, not on what it itself is connected to. Isn't platform data
>> used in such cases?
>
> So why can't Alessandro continue to use Platform Data in the normal way?

He probably can, this is not an argument about that, what I am worried
about is Arnd's suggestion to delete the platform data header if there
are potential users of it.

Yours,
Linus Walleij

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Alessandro Rubini]

> Alessandro Rubini is actively working on bridging this (and
> other amba_device primecells) to PCI, that is the reason why it
> was recently converted to an amba_device.

Yes, I've been inactive for a while but I'm on it right now.

> How is he then supposed to get the proper parameters into the
> driver? Note that the PCI ID is no help at all since the parameters
> depend on what is connected to the I2C bus, not on what it itself is
> connected to. Isn't platform data used in such cases?

I'm using platform data currently, but Davide Ciminaghi is actively
working to convert the configuration to device-tree: the way we pass
platform data to the pci device (and thus amba) is not considered
acceptable by Peter Anvin.

I'm thus asking Davide if he's happy to remove the platform data
configuration path right now (I personally wouldn't be very happy, but
I acknowledge it should happen, sooner or later).

/alessandro

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Lee Jones]

Author: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Date:   Mon Aug 6 11:09:57 2012 +0100

    i2c: nomadik: Add Device Tree support to the Nomadik I2C driver
    
    Here we apply the bindings required for successful Device Tree
    probing of the i2c-nomadik driver.
    
    Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
    Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 61b00ed..5d1a970 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -25,6 +25,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/pm_runtime.h>
 #include <linux/platform_data/i2c-nomadik.h>
+#include <linux/of.h>
 
 #define DRIVER_NAME "nmk-i2c"
 
@@ -920,18 +921,42 @@ static struct nmk_i2c_controller u8500_i2c = {
        .sm             = I2C_FREQ_MODE_FAST,
 };
 
+static void nmk_i2c_of_probe(struct device_node *np,
+                       struct nmk_i2c_controller *pdata)
+{
+       of_property_read_u32(np, "clock-frequency", &pdata->clk_freq);
+
+       /* This driver only supports 'standard' and 'fast' modes of operation. */
+       if (pdata->clk_freq <= 100000)
+               pdata->sm = I2C_FREQ_MODE_STANDARD;
+       else
+               pdata->sm = I2C_FREQ_MODE_FAST;
+}
+
 static atomic_t adapter_id = ATOMIC_INIT(0);
 
 static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 {
        int ret = 0;
        struct nmk_i2c_controller *pdata = adev->dev.platform_data;
+       struct device_node *np = adev->dev.of_node;
        struct nmk_i2c_dev      *dev;
        struct i2c_adapter *adap;
 
-       if (!pdata)
-               /* No i2c configuration found, using the default. */
-               pdata = &u8500_i2c;
+       if (!pdata) {
+               if (np) {
+                       pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL);
+                       if (!pdata) {
+                               ret = -ENOMEM;
+                               goto err_no_mem;
+                       }
+                       /* Provide the default configuration as a base. */
+                       memcpy(pdata, &u8500_i2c, sizeof(struct nmk_i2c_controller));
+                       nmk_i2c_of_probe(np, pdata);
+               } else
+                       /* No i2c configuration found, using the default. */
+                       pdata = &u8500_i2c;
+       }
 
        dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
        if (!dev) {
diff --git a/include/linux/platform_data/i2c-nomadik.h b/include/linux/platform_data/i2c-nomadik.h
index c2303c3..3a8be9c 100644
--- a/include/linux/platform_data/i2c-nomadik.h
+++ b/include/linux/platform_data/i2c-nomadik.h
@@ -28,7 +28,7 @@ enum i2c_freq_mode {
  * @sm:                speed mode
  */
 struct nmk_i2c_controller {
-       unsigned long   clk_freq;
+       u32             clk_freq;
        unsigned short  slsu;
        unsigned char   tft;
        unsigned char   rft;

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Linus Walleij]

On Wed, Sep 5, 2012 at 9:33 AM, Lee Jones <lee.jones@linaro.org> wrote:

> Author: Lee Jones <lee.jones@linaro.org>
> Date:   Mon Aug 6 11:09:57 2012 +0100
>
>     i2c: nomadik: Add Device Tree support to the Nomadik I2C driver
>
>     Here we apply the bindings required for successful Device Tree
>     probing of the i2c-nomadik driver.
>
>     Cc: linux-i2c@vger.kernel.org
>     Signed-off-by: Lee Jones <lee.jones@linaro.org>

Excellent :-)
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Wolfram are you picking this up?

Yours,
Linus Walleij