[PATCH 09/14] i2c: Add Device Tree support to the Nomadik I2C driver

[PATCH 09/14] i2c: Add Device Tree support to the Nomadik I2C driver

[Lee Jones]

Here we move the i2c-nomadik's default settings into the driver
rather than specifying them from platform code. At the time of
this writing we only have one user, the u8500. As new users are
added, it is expected that they will be Device Tree compliant.
If this is the case, we will look up their initialisation values
by compatible entry, then apply them forthwith.

Cc: linux-i2c@vger.kernel.org
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/i2c/busses/i2c-nomadik.c |   40 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index a92440d..1ffdf67 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -23,6 +23,7 @@
 #include <linux/io.h>
 #include <linux/regulator/consumer.h>
 #include <linux/pm_runtime.h>
+#include <linux/of_device.h>
 
 #include <plat/i2c.h>
 
@@ -899,15 +900,51 @@ static const struct i2c_algorithm nmk_i2c_algo = {
 	.functionality	= nmk_i2c_functionality
 };
 
+static struct nmk_i2c_controller u8500_i2c = {
+       /*
+        * slave data setup time, which is
+        * 250 ns,100ns,10ns which is 14,6,2
+        * respectively for a 48 Mhz
+        * i2c clock
+        */
+       .slsu           = 0xe,
+       /* Tx FIFO threshold */
+       .tft            = 1,
+       /* Rx FIFO threshold */
+       .rft            = 8,
+       /* std. mode operation */
+       .clk_freq       = 100000,
+       /* Slave response timeout(ms) */
+       .timeout        = 200,
+       .sm             = I2C_FREQ_MODE_FAST,
+};
+
+
+static const struct of_device_id nmk_gpio_match[] = {
+	{ .compatible = "st,nomadik-i2c", .data = &u8500_i2c, },
+	{}
+};
+
 static int __devinit nmk_i2c_probe(struct platform_device *pdev)
 {
 	int ret = 0;
 	struct resource *res;
-	struct nmk_i2c_controller *pdata =
+	const struct nmk_i2c_controller *pdata =
 			pdev->dev.platform_data;
+        const struct of_device_id *of_id =
+                        of_match_device(nmk_gpio_match, &pdev->dev);
 	struct nmk_i2c_dev	*dev;
 	struct i2c_adapter *adap;
 
+	if (!pdata) {
+		if (of_id && of_id->data)
+			/* Looks like we're booting via Device Tree. */
+			pdata = of_id->data;
+		else
+			/* No i2c configuration found, using the default. */
+			pdata = &u8500_i2c;
+	}
+
 	dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
 	if (!dev) {
 		dev_err(&pdev->dev, "cannot allocate memory\n");
@@ -1043,6 +1080,7 @@ static struct platform_driver nmk_i2c_driver = {
 		.owner = THIS_MODULE,
 		.name = DRIVER_NAME,
 		.pm = &nmk_i2c_pm,
+		.of_match_table = nmk_gpio_match,
 	},
 	.probe = nmk_i2c_probe,
 	.remove = __devexit_p(nmk_i2c_remove),
-- 
1.7.9.5

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

[Wolfram Sang]

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

On Mon, Jun 11, 2012 at 04:25:02PM +0100, Lee Jones wrote:
> Here we move the i2c-nomadik's default settings into the driver
> rather than specifying them from platform code. At the time of
> this writing we only have one user, the u8500. As new users are
> added, it is expected that they will be Device Tree compliant.
> If this is the case, we will look up their initialisation values
> by compatible entry, then apply them forthwith.
> 
> Cc: linux-i2c@vger.kernel.org
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/i2c/busses/i2c-nomadik.c |   40 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> index a92440d..1ffdf67 100644
> --- a/drivers/i2c/busses/i2c-nomadik.c
> +++ b/drivers/i2c/busses/i2c-nomadik.c
> @@ -23,6 +23,7 @@
>  #include <linux/io.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/of_device.h>
>  
>  #include <plat/i2c.h>
>  
> @@ -899,15 +900,51 @@ static const struct i2c_algorithm nmk_i2c_algo = {
>  	.functionality	= nmk_i2c_functionality
>  };
>  
> +static struct nmk_i2c_controller u8500_i2c = {
> +       /*
> +        * slave data setup time, which is
> +        * 250 ns,100ns,10ns which is 14,6,2
> +        * respectively for a 48 Mhz
> +        * i2c clock
> +        */
> +       .slsu           = 0xe,
> +       /* Tx FIFO threshold */

Please put these comments directly after the members they describe.

And make sure you use tabs for indentation all over the patch instead of
spaces. checkpatch.pl will help you to get the formal things right.

> +       .tft            = 1,
> +       /* Rx FIFO threshold */
> +       .rft            = 8,
> +       /* std. mode operation */
> +       .clk_freq       = 100000,
> +       .timeout        = 200,	/* Slave response timeout(ms) */
> +       .sm             = I2C_FREQ_MODE_FAST,
> +};
> +
> +
> +static const struct of_device_id nmk_gpio_match[] = {
> +	{ .compatible = "st,nomadik-i2c", .data = &u8500_i2c, },
> +	{}
> +};
> +
>  static int __devinit nmk_i2c_probe(struct platform_device *pdev)
>  {
>  	int ret = 0;
>  	struct resource *res;
> -	struct nmk_i2c_controller *pdata =
> +	const struct nmk_i2c_controller *pdata =
>  			pdev->dev.platform_data;
> +        const struct of_device_id *of_id =
> +                        of_match_device(nmk_gpio_match, &pdev->dev);
>  	struct nmk_i2c_dev	*dev;
>  	struct i2c_adapter *adap;
>  
> +	if (!pdata) {
> +		if (of_id && of_id->data)
> +			/* Looks like we're booting via Device Tree. */
> +			pdata = of_id->data;
> +		else
> +			/* No i2c configuration found, using the default. */
> +			pdata = &u8500_i2c;
> +	}
> +
>  	dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
>  	if (!dev) {
>  		dev_err(&pdev->dev, "cannot allocate memory\n");
> @@ -1043,6 +1080,7 @@ static struct platform_driver nmk_i2c_driver = {
>  		.owner = THIS_MODULE,
>  		.name = DRIVER_NAME,
>  		.pm = &nmk_i2c_pm,
> +		.of_match_table = nmk_gpio_match,
>  	},
>  	.probe = nmk_i2c_probe,
>  	.remove = __devexit_p(nmk_i2c_remove),
> -- 

Thanks,

   Wolfram

-- 
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 09/14] i2c: Add Device Tree support to the Nomadik I2C driver

[Linus Walleij]

On Mon, Jun 11, 2012 at 5:25 PM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> As new users are
> added, it is expected that they will be Device Tree compliant.
> If this is the case, we will look up their initialisation values
> by compatible entry, then apply them forthwith.
(...)
> +static struct nmk_i2c_controller u8500_i2c = {
> +       /*
> +        * slave data setup time, which is
> +        * 250 ns,100ns,10ns which is 14,6,2
> +        * respectively for a 48 Mhz
> +        * i2c clock
> +        */
> +       .slsu           = 0xe,
> +       /* Tx FIFO threshold */
> +       .tft            = 1,
> +       /* Rx FIFO threshold */
> +       .rft            = 8,
> +       /* std. mode operation */
> +       .clk_freq       = 100000,
> +       /* Slave response timeout(ms) */
> +       .timeout        = 200,
> +       .sm             = I2C_FREQ_MODE_FAST,
> +};

So why don't we create proper device tree bindings for the above platform
data?

For example several driver under
Documentation/devicetree/bindings/i2c/*
define the .clk_freq above as "clock-frequency"

samsung-i2c even has this:
samsung,i2c-sda-delay = <100>;
samsung,i2c-max-bus-freq = <100000>;

Where i2c-sda-delay corresponds to slsu above.

I suspect .sm can be derived from the frequency so it
is "FAST" whenever the frequency > 100000.

This looks to me like a way to push the burden of doing the real
device tree binding for the above to the next user.
(Which will likely be me, so therefore I care a bit ...)

Yours,
Linus Walleij

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

[Lee Jones]

On 11/06/12 20:05, Wolfram Sang wrote:
> On Mon, Jun 11, 2012 at 04:25:02PM +0100, Lee Jones wrote:
>> Here we move the i2c-nomadik's default settings into the driver
>> rather than specifying them from platform code. At the time of
>> this writing we only have one user, the u8500. As new users are
>> added, it is expected that they will be Device Tree compliant.
>> If this is the case, we will look up their initialisation values
>> by compatible entry, then apply them forthwith.
>>
>> Cc: linux-i2c@vger.kernel.org
>> Acked-by: Linus Walleij<linus.walleij@linaro.org>
>> Signed-off-by: Lee Jones<lee.jones@linaro.org>
>> ---
>>   drivers/i2c/busses/i2c-nomadik.c |   40 +++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
>> index a92440d..1ffdf67 100644
>> --- a/drivers/i2c/busses/i2c-nomadik.c
>> +++ b/drivers/i2c/busses/i2c-nomadik.c
>> @@ -23,6 +23,7 @@
>>   #include<linux/io.h>
>>   #include<linux/regulator/consumer.h>
>>   #include<linux/pm_runtime.h>
>> +#include<linux/of_device.h>
>>
>>   #include<plat/i2c.h>
>>
>> @@ -899,15 +900,51 @@ static const struct i2c_algorithm nmk_i2c_algo = {
>>   	.functionality	= nmk_i2c_functionality
>>   };
>>
>> +static struct nmk_i2c_controller u8500_i2c = {
>> +       /*
>> +        * slave data setup time, which is
>> +        * 250 ns,100ns,10ns which is 14,6,2
>> +        * respectively for a 48 Mhz
>> +        * i2c clock
>> +        */
>> +       .slsu           = 0xe,
>> +       /* Tx FIFO threshold */
>
> Please put these comments directly after the members they describe.
>
> And make sure you use tabs for indentation all over the patch instead of
> spaces. checkpatch.pl will help you to get the formal things right.

My apologies. This section of code was a direct copy and paste from 
platform code (arch/arm/mach-ux500/board-mop500.c). It must have lost 
some formatting en route. I'll make the changes you request and re-post.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Lee Jones]

On 11/06/12 21:37, Linus Walleij wrote:
> On Mon, Jun 11, 2012 at 5:25 PM, Lee Jones<lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>  wrote:
>
>> As new users are
>> added, it is expected that they will be Device Tree compliant.
>> If this is the case, we will look up their initialisation values
>> by compatible entry, then apply them forthwith.
> (...)
>> +static struct nmk_i2c_controller u8500_i2c = {
>> +       /*
>> +        * slave data setup time, which is
>> +        * 250 ns,100ns,10ns which is 14,6,2
>> +        * respectively for a 48 Mhz
>> +        * i2c clock
>> +        */
>> +       .slsu           = 0xe,
>> +       /* Tx FIFO threshold */
>> +       .tft            = 1,
>> +       /* Rx FIFO threshold */
>> +       .rft            = 8,
>> +       /* std. mode operation */
>> +       .clk_freq       = 100000,
>> +       /* Slave response timeout(ms) */
>> +       .timeout        = 200,
>> +       .sm             = I2C_FREQ_MODE_FAST,
>> +};
>
> So why don't we create proper device tree bindings for the above platform
> data?
>
> For example several driver under
> Documentation/devicetree/bindings/i2c/*
> define the .clk_freq above as "clock-frequency"
>
> samsung-i2c even has this:
> samsung,i2c-sda-delay =<100>;
> samsung,i2c-max-bus-freq =<100000>;
>
> Where i2c-sda-delay corresponds to slsu above.
>
> I suspect .sm can be derived from the frequency so it
> is "FAST" whenever the frequency>  100000.
>
> This looks to me like a way to push the burden of doing the real
> device tree binding for the above to the next user.
> (Which will likely be me, so therefore I care a bit ...)

On the contrary. This will avoid any added Device Tree complexity and 
ensure that no extra vendor specific bindings are required. When a new 
user wishes to use the driver, all they (you) have to do is create a new 
struct with the platform specific information and add the entry to 
nmk_gpio_match[], simples.

I've even added the logic to extract any information you provide via 
nmk_gpio_match[] for use as platform data. This keeps it both out of 
platform code and the Device Tree binary. Everyone's a winner. :)

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Lee Jones]

From: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Date: Tue, 17 Apr 2012 16:04:13 +0100
Subject: [PATCH 09/14] i2c: Add Device Tree support to the Nomadik I2C driver

Here we move the i2c-nomadik's default settings into the driver
rather than specifying them from platform code. At the time of
this writing we only have one user, the u8500. As new users are
added, it is expected that they will be Device Tree compliant.
If this is the case, we will look up their initialisation values
by compatible entry, then apply them forthwith.

Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Acked-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/i2c/busses/i2c-nomadik.c |   33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index a92440d..23fde19 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -23,6 +23,7 @@
 #include <linux/io.h>
 #include <linux/regulator/consumer.h>
 #include <linux/pm_runtime.h>
+#include <linux/of_device.h>
 
 #include <plat/i2c.h>
 
@@ -899,15 +900,44 @@ static const struct i2c_algorithm nmk_i2c_algo = {
 	.functionality	= nmk_i2c_functionality
 };
 
+static struct nmk_i2c_controller u8500_i2c = {
+	/*
+	 * Slave data setup time; 250ns, 100ns, and 10ns, which
+	 * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
+	 */
+	.slsu           = 0xe,
+	.tft            = 1, 	  /* Tx FIFO threshold */
+	.rft            = 8, 	  /* Rx FIFO threshold */
+	.clk_freq       = 100000, /* std. mode operation */
+	.timeout        = 200, 	  /* Slave response timeout(ms) */
+	.sm             = I2C_FREQ_MODE_FAST,
+};
+
+static const struct of_device_id nmk_gpio_match[] = {
+	{ .compatible = "st,nomadik-i2c", .data = &u8500_i2c, },
+	{}
+};
+
 static int __devinit nmk_i2c_probe(struct platform_device *pdev)
 {
 	int ret = 0;
 	struct resource *res;
-	struct nmk_i2c_controller *pdata =
+	const struct nmk_i2c_controller *pdata =
 			pdev->dev.platform_data;
+        const struct of_device_id *of_id =
+                        of_match_device(nmk_gpio_match, &pdev->dev);
 	struct nmk_i2c_dev	*dev;
 	struct i2c_adapter *adap;
 
+	if (!pdata) {
+		if (of_id && of_id->data)
+			/* Looks like we're booting via Device Tree. */
+			pdata = of_id->data;
+		else
+			/* No i2c configuration found, using the default. */
+			pdata = &u8500_i2c;
+	}
+
 	dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
 	if (!dev) {
 		dev_err(&pdev->dev, "cannot allocate memory\n");
@@ -1043,6 +1073,7 @@ static struct platform_driver nmk_i2c_driver = {
 		.owner = THIS_MODULE,
 		.name = DRIVER_NAME,
 		.pm = &nmk_i2c_pm,
+		.of_match_table = nmk_gpio_match,
 	},
 	.probe = nmk_i2c_probe,
 	.remove = __devexit_p(nmk_i2c_remove),
-- 
1.7.9.5

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

[Wolfram Sang]

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

On Tue, Jun 12, 2012 at 09:52:39AM +0100, Lee Jones wrote:
> From: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Date: Tue, 17 Apr 2012 16:04:13 +0100
> Subject: [PATCH 09/14] i2c: Add Device Tree support to the Nomadik I2C driver
> 
> Here we move the i2c-nomadik's default settings into the driver
> rather than specifying them from platform code. At the time of
> this writing we only have one user, the u8500. As new users are
> added, it is expected that they will be Device Tree compliant.
> If this is the case, we will look up their initialisation values
> by compatible entry, then apply them forthwith.
> 
> Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Acked-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Did you use checkpatch?

============

Applying: i2c: Add Device Tree support to the Nomadik I2C driver
  CHECKPATCH
WARNING: please, no space before tabs
#37: FILE: drivers/i2c/busses/i2c-nomadik.c:909:
+^I.tft            = 1, ^I  /* Tx FIFO threshold */$

WARNING: please, no space before tabs
#38: FILE: drivers/i2c/busses/i2c-nomadik.c:910:
+^I.rft            = 8, ^I  /* Rx FIFO threshold */$

WARNING: please, no space before tabs
#40: FILE: drivers/i2c/busses/i2c-nomadik.c:912:
+^I.timeout        = 200, ^I  /* Slave response timeout(ms) */$

ERROR: code indent should use tabs where possible
#56: FILE: drivers/i2c/busses/i2c-nomadik.c:927:
+        const struct of_device_id *of_id =$

WARNING: please, no spaces at the start of a line
#56: FILE: drivers/i2c/busses/i2c-nomadik.c:927:
+        const struct of_device_id *of_id =$

ERROR: code indent should use tabs where possible
#57: FILE: drivers/i2c/busses/i2c-nomadik.c:928:
+                        of_match_device(nmk_gpio_match, &pdev->dev);$

WARNING: please, no spaces at the start of a line
#57: FILE: drivers/i2c/busses/i2c-nomadik.c:928:
+                        of_match_device(nmk_gpio_match, &pdev->dev);$

total: 2 errors, 5 warnings, 0 checks, 59 lines checked

NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
      scripts/cleanfile

Your patch has style problems, please review.

============

Regards,

   Wolfram

-- 
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 09/14] i2c: Add Device Tree support to the Nomadik I2C driver

[Linus Walleij]

On Tue, Jun 12, 2012 at 9:34 AM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 11/06/12 21:37, Linus Walleij wrote:
>>
>> So why don't we create proper device tree bindings for the above platform
>> data?
>>(...)
>> This looks to me like a way to push the burden of doing the real
>> device tree binding for the above to the next user.
>> (Which will likely be me, so therefore I care a bit ...)
>
>
> On the contrary. This will avoid any added Device Tree complexity and ensure
> that no extra vendor specific bindings are required. When a new user wishes
> to use the driver, all they (you) have to do is create a new struct with the
> platform specific information and add the entry to nmk_gpio_match[],
> simples.
>
> I've even added the logic to extract any information you provide via
> nmk_gpio_match[] for use as platform data. This keeps it both out of
> platform code and the Device Tree binary. Everyone's a winner. :)

No. You assume that the platform data is a chip-specific property,
and that it will be the same for all busses on the chip. But it
isn't.

The above platform data is *board specific*, it depends on
what devices have been connected to each bus.

For example the max frequency: this depends on the maximum
frequency any of the devices connected to one very bus
can support.

The other platforms have put this frequency into their device
trees for a reason, and this is it.

So for the four different i2c busses there may need to be
four different platform data sets. How are you going to
distinguish between the four buses?

This is thus broken and needs to have proper bindings.

Yours,
Linus Walleij

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

[Lee Jones]

On 13/06/12 06:40, Linus Walleij wrote:
> On Tue, Jun 12, 2012 at 9:34 AM, Lee Jones<lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>  wrote:
>> On 11/06/12 21:37, Linus Walleij wrote:
>>>
>>> So why don't we create proper device tree bindings for the above platform
>>> data?
>>> (...)
>>> This looks to me like a way to push the burden of doing the real
>>> device tree binding for the above to the next user.
>>> (Which will likely be me, so therefore I care a bit ...)
>>
>>
>> On the contrary. This will avoid any added Device Tree complexity and ensure
>> that no extra vendor specific bindings are required. When a new user wishes
>> to use the driver, all they (you) have to do is create a new struct with the
>> platform specific information and add the entry to nmk_gpio_match[],
>> simples.
>>
>> I've even added the logic to extract any information you provide via
>> nmk_gpio_match[] for use as platform data. This keeps it both out of
>> platform code and the Device Tree binary. Everyone's a winner. :)
>
> No. You assume that the platform data is a chip-specific property,
> and that it will be the same for all busses on the chip. But it
> isn't.
>
> The above platform data is *board specific*, it depends on
> what devices have been connected to each bus.
>
> For example the max frequency: this depends on the maximum
> frequency any of the devices connected to one very bus
> can support.
>
> The other platforms have put this frequency into their device
> trees for a reason, and this is it.
>
> So for the four different i2c busses there may need to be
> four different platform data sets. How are you going to
> distinguish between the four buses?
>
> This is thus broken and needs to have proper bindings.

Board specific is fine, as the data is protected by a board specific 
property. Do you mean that the properties are *bus specific*? In which 
case I see your point and will apply the correct bindings.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Linus Walleij]

On Wed, Jun 13, 2012 at 9:01 AM, Lee Jones <lee.jones@linaro.org> wrote:

> Board specific is fine, as the data is protected by a board specific
> property. Do you mean that the properties are *bus specific*? In which case
> I see your point and will apply the correct bindings.

I cannot parse this, the board for me is a SoC, busses and a
number of components connected via e.g. I2C.

Can you define what you mean with a "board specific property"?
It seems you are talking about what I would call an
"SoC-specific property", i.e. something out of a .dtsi file for
a certain SoC, whereas the .dts for an entire board is,
well, for a board, a set of components on a PCB.

The arrangement of accelerometers and battery monitors on a
certain board is board-specific, and it is also by definition
bus-specific.

Yours,
Linus Walleij

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

[Lee Jones]

On 13/06/12 09:12, Linus Walleij wrote:
> On Wed, Jun 13, 2012 at 9:01 AM, Lee Jones<lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>  wrote:
>
>> Board specific is fine, as the data is protected by a board specific
>> property. Do you mean that the properties are *bus specific*? In which case
>> I see your point and will apply the correct bindings.
>
> I cannot parse this, the board for me is a SoC, busses and a
> number of components connected via e.g. I2C.
>
> Can you define what you mean with a "board specific property"?
> It seems you are talking about what I would call an
> "SoC-specific property", i.e. something out of a .dtsi file for
> a certain SoC, whereas the .dts for an entire board is,
> well, for a board, a set of components on a PCB.
>
> The arrangement of accelerometers and battery monitors on a
> certain board is board-specific, and it is also by definition
> bus-specific.

Okay, let's put it another way. We can individualise any board, bus, 
platform, machine or system by placing all of the information in a DT 
node so long as we plonk it in the correct place within the Device Tree. 
However, we have just as much control by keeping them in separate 
structs in the C file and selecting the right one using the compatible 
sting.

The real item for discussion is; if we have varying configurations for 
each of the i2c buses residing on the same SoC, do we really want to use 
different compatible strings to identify each one. If the answer is no, 
which I think it is, then I need to write the bindings and have 
everything individually configurable from Device Tree.

While you're mulling this over, I'm just going to write the bindings anyway.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Lee Jones]

From: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Date: Tue, 17 Apr 2012 16:04:13 +0100
Subject: [PATCH 1/3] i2c: 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. We also apply a fall-back
configuration in case either one is not provided, or a required
element is missing from the one supplied.

Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/i2c/busses/i2c-nomadik.c |   92 ++++++++++++++++++++++++++++++++++----
 1 file changed, 84 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index a92440d..b97f52e 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -23,6 +23,7 @@
 #include <linux/io.h>
 #include <linux/regulator/consumer.h>
 #include <linux/pm_runtime.h>
+#include <linux/of.h>
 
 #include <plat/i2c.h>
 
@@ -899,15 +900,86 @@ static const struct i2c_algorithm nmk_i2c_algo = {
 	.functionality	= nmk_i2c_functionality
 };
 
+static struct nmk_i2c_controller u8500_i2c = {
+	/*
+	 * Slave data setup time; 250ns, 100ns, and 10ns, which
+	 * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
+	 */
+	.slsu           = 0xe,
+	.tft            = 1,      /* Tx FIFO threshold */
+	.rft            = 8,      /* Rx FIFO threshold */
+	.clk_freq       = 100000, /* std. mode operation */
+	.timeout        = 200,    /* Slave response timeout(ms) */
+	.sm             = I2C_FREQ_MODE_FAST,
+};
+
+static int __devinit
+nmk_i2c_of_probe(struct device_node *np, struct nmk_i2c_controller *pdata)
+{
+	of_property_read_u32(np, "clock-frequency", (u32*)&pdata->clk_freq);
+	if (!pdata->clk_freq) {
+		pr_warn("%s: Clock frequency not found\n", np->full_name);
+		return -EINVAL;
+	}
+
+	of_property_read_u32(np, "stericsson,slsu", (u32*)&pdata->slsu);
+	if (!pdata->slsu) {
+		pr_warn("%s: Data line delay not found\n", np->full_name);
+		return -EINVAL;
+	}
+
+	of_property_read_u32(np, "stericsson,tft", (u32*)&pdata->tft);
+	if (!pdata->tft) {
+		pr_warn("%s: Tx FIFO threshold not found\n", np->full_name);
+		return -EINVAL;
+	}
+
+	of_property_read_u32(np, "stericsson,rft", (u32*)&pdata->rft);
+	if (!pdata->rft) {
+		pr_warn("%s: Rx FIFO threshold not found\n", np->full_name);
+		return -EINVAL;
+	}
+
+	of_property_read_u32(np, "stericsson,timeout", (u32*)&pdata->timeout);
+	if (!pdata->timeout) {
+		pr_warn("%s: Timeout not found\n", np->full_name);
+		return -EINVAL;
+	}
+
+	if (of_get_property(np, "stericsson,i2c_freq_mode_fast", NULL))
+		pdata->sm = I2C_FREQ_MODE_FAST;
+	else
+		pdata->sm = I2C_FREQ_MODE_STANDARD;
+
+	return 0;
+}
+
 static int __devinit nmk_i2c_probe(struct platform_device *pdev)
 {
 	int ret = 0;
 	struct resource *res;
-	struct nmk_i2c_controller *pdata =
-			pdev->dev.platform_data;
+	struct nmk_i2c_controller *pdata = pdev->dev.platform_data;
+	struct device_node *np = pdev->dev.of_node;
 	struct nmk_i2c_dev	*dev;
 	struct i2c_adapter *adap;
 
+	if (np) {
+		if (!pdata) {
+			pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+			if (!pdata) {
+				ret = -ENOMEM;
+				goto err_no_mem;
+			}
+		}
+		ret = nmk_i2c_of_probe(np, pdata);
+		if (ret)
+			kfree(pdata);
+	}
+
+	if (!pdata)
+		/* No i2c configuration found, using the default. */
+		pdata = &u8500_i2c;
+
 	dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
 	if (!dev) {
 		dev_err(&pdev->dev, "cannot allocate memory\n");
@@ -974,12 +1046,10 @@ static int __devinit nmk_i2c_probe(struct platform_device *pdev)
 
 	/* fetch the controller configuration from machine */
 	dev->cfg.clk_freq = pdata->clk_freq;
-	dev->cfg.slsu	= pdata->slsu;
-	dev->cfg.tft	= pdata->tft;
-	dev->cfg.rft	= pdata->rft;
-	dev->cfg.sm	= pdata->sm;
-
-	i2c_set_adapdata(adap, dev);
+	dev->cfg.slsu	  = pdata->slsu;
+	dev->cfg.tft	  = pdata->tft;
+	dev->cfg.rft	  = pdata->rft;
+	dev->cfg.sm	  = pdata->sm;	i2c_set_adapdata(adap, dev);
 
 	dev_info(&pdev->dev,
 		 "initialize %s on virtual base %p\n",
@@ -1038,11 +1108,17 @@ static int __devexit nmk_i2c_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id nmk_gpio_match[] = {
+	{ .compatible = "st,nomadik-i2c", },
+	{},
+};
+
 static struct platform_driver nmk_i2c_driver = {
 	.driver = {
 		.owner = THIS_MODULE,
 		.name = DRIVER_NAME,
 		.pm = &nmk_i2c_pm,
+		.of_match_table = nmk_gpio_match,
 	},
 	.probe = nmk_i2c_probe,
 	.remove = __devexit_p(nmk_i2c_remove),
-- 
1.7.9.5

[PATCH 2/3] ARM: ux500: Add i2c configurations to the Device Tree for DB8500 based devices

[Lee Jones]

From: Lee Jones <lee.jones@linaro.org>
Date: Wed, 13 Jun 2012 16:21:14 +0100
Subject: [PATCH 2/3] ARM: ux500: Add i2c configurations to the Device Tree
 for DB8500 based devices

For correct operation of the i2c-nomadik driver, it requires some
information surrounding clock-frequencies, delay times and thresholds.
Here we apply those configurations to be used when a platform is
booted via the device Tree.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/db8500.dtsi |   35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/arch/arm/boot/dts/db8500.dtsi b/arch/arm/boot/dts/db8500.dtsi
index 00a4c3e..f3050f1 100644
--- a/arch/arm/boot/dts/db8500.dtsi
+++ b/arch/arm/boot/dts/db8500.dtsi
@@ -455,6 +455,13 @@
 			#address-cells = <1>;
 			#size-cells = <0>;
 			v-i2c-supply = <&db8500_vape_reg>;
+
+			clock-frequency = <100000>;
+			stericsson,slsu = <0xe>;
+			stericsson,tft = <1>;
+			stericsson,rft = <8>;
+			stericsson,timeout = <200>;
+			stericsson,i2c_freq_mode_fast;
 		};
 
 		i2c@80122000 {
@@ -464,6 +471,13 @@
 			#address-cells = <1>;
 			#size-cells = <0>;
 			v-i2c-supply = <&db8500_vape_reg>;
+
+			clock-frequency = <100000>;
+			stericsson,slsu = <0xe>;
+			stericsson,tft = <1>;
+			stericsson,rft = <8>;
+			stericsson,timeout = <200>;
+			stericsson,i2c_freq_mode_fast;
 		};
 
 		i2c@80128000 {
@@ -473,6 +487,13 @@
 			#address-cells = <1>;
 			#size-cells = <0>;
 			v-i2c-supply = <&db8500_vape_reg>;
+
+			clock-frequency = <100000>;
+			stericsson,slsu = <0xe>;
+			stericsson,tft = <1>;
+			stericsson,rft = <8>;
+			stericsson,timeout = <200>;
+			stericsson,i2c_freq_mode_fast;
 		};
 
 		i2c@80110000 {
@@ -482,6 +503,13 @@
 			#address-cells = <1>;
 			#size-cells = <0>;
 			v-i2c-supply = <&db8500_vape_reg>;
+
+			clock-frequency = <100000>;
+			stericsson,slsu = <0xe>;
+			stericsson,tft = <1>;
+			stericsson,rft = <8>;
+			stericsson,timeout = <200>;
+			stericsson,i2c_freq_mode_fast;
 		};
 
 		i2c@8012a000 {
@@ -491,6 +519,13 @@
 			#address-cells = <1>;
 			#size-cells = <0>;
 			v-i2c-supply = <&db8500_vape_reg>;
+
+			clock-frequency = <100000>;
+			stericsson,slsu = <0xe>;
+			stericsson,tft = <1>;
+			stericsson,rft = <8>;
+			stericsson,timeout = <200>;
+			stericsson,i2c_freq_mode_fast;
 		};
 
 		ssp@80002000 {
-- 
1.7.9.5

[PATCH 3/3] Documentation: Device Tree binding information for i2c-nomadik driver

[Lee Jones]

From: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Date: Wed, 13 Jun 2012 17:00:02 +0100
Subject: [PATCH 3/3] Documentation: Device Tree binding information for
 i2c-nomadik driver

This document describes each of the non-standard Device Tree bindings
used in the Nomadik I2C driver.

Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 Documentation/devicetree/bindings/i2c/nomadik.txt |   33 +++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/nomadik.txt

diff --git a/Documentation/devicetree/bindings/i2c/nomadik.txt b/Documentation/devicetree/bindings/i2c/nomadik.txt
new file mode 100644
index 0000000..18cea18
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/nomadik.txt
@@ -0,0 +1,33 @@
+I2C for Nomadik based systems
+
+Required (non-standard) properties:
+ - Nil
+
+Recommended (non-standard) properties:
+ - clock-frequency : Maximum bus clock frequency for the device
+ - stericsson,slsu : Data line delay
+ - stericsson,tft : Tx FIFO threshold
+ - stericsson,rft : Rx FIFO threshold
+ - stericsson,timeout : Slave response timeout(ms)
+ - stericsson,i2c_freq_mode_fast : I2C fast mode (omit for standard mode)
+
+Optional (non-standard) properties:
+ - Nil
+
+Example :
+
+i2c@80004000 {
+        compatible = "stericsson,db8500-i2c", "st,nomadik-i2c";
+        reg = <0x80004000 0x1000>;
+        interrupts = <0 21 0x4>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+        v-i2c-supply = <&db8500_vape_reg>;
+
+        clock-frequency = <100000>;
+        stericsson,slsu = <0xe>;
+        stericsson,tft = <1>;
+        stericsson,rft = <8>;
+        stericsson,timeout = <200>;
+        stericsson,i2c_freq_mode_fast;
+};
-- 
1.7.9.5

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

[Linus Walleij]

On Wed, Jun 13, 2012 at 6:07 PM, 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. We also apply a fall-back
> configuration in case either one is not provided, or a required
> element is missing from the one supplied.
>
> Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

This looks more like I'd expect it, good job! :-D

> +static int __devinit
> +nmk_i2c_of_probe(struct device_node *np, struct nmk_i2c_controller *pdata)
> +{
> +       of_property_read_u32(np, "clock-frequency", (u32*)&pdata->clk_freq);
> +       if (!pdata->clk_freq) {
> +               pr_warn("%s: Clock frequency not found\n", np->full_name);
> +               return -EINVAL;
> +       }
> +
> +       of_property_read_u32(np, "stericsson,slsu", (u32*)&pdata->slsu);
> +       if (!pdata->slsu) {
> +               pr_warn("%s: Data line delay not found\n", np->full_name);
> +               return -EINVAL;
> +       }
> +
> +       of_property_read_u32(np, "stericsson,tft", (u32*)&pdata->tft);
> +       if (!pdata->tft) {
> +               pr_warn("%s: Tx FIFO threshold not found\n", np->full_name);
> +               return -EINVAL;
> +       }
> +
> +       of_property_read_u32(np, "stericsson,rft", (u32*)&pdata->rft);
> +       if (!pdata->rft) {
> +               pr_warn("%s: Rx FIFO threshold not found\n", np->full_name);
> +               return -EINVAL;
> +       }
> +
> +       of_property_read_u32(np, "stericsson,timeout", (u32*)&pdata->timeout);
> +       if (!pdata->timeout) {
> +               pr_warn("%s: Timeout not found\n", np->full_name);
> +               return -EINVAL;
> +       }
> +
> +       if (of_get_property(np, "stericsson,i2c_freq_mode_fast", NULL))
> +               pdata->sm = I2C_FREQ_MODE_FAST;
> +       else
> +               pdata->sm = I2C_FREQ_MODE_STANDARD;
> +
> +       return 0;
> +}

Will this compile fine if CONFIG_OF is not selected?

>        /* fetch the controller configuration from machine */
>        dev->cfg.clk_freq = pdata->clk_freq;
> -       dev->cfg.slsu   = pdata->slsu;
> -       dev->cfg.tft    = pdata->tft;
> -       dev->cfg.rft    = pdata->rft;
> -       dev->cfg.sm     = pdata->sm;
> -
> -       i2c_set_adapdata(adap, dev);
> +       dev->cfg.slsu     = pdata->slsu;
> +       dev->cfg.tft      = pdata->tft;
> +       dev->cfg.rft      = pdata->rft;
> +       dev->cfg.sm       = pdata->sm;  i2c_set_adapdata(adap, dev);

This looks like an unrelated whitespace fix, but OK...

Yours,
Linus Walleij

Re: [PATCH 2/3] ARM: ux500: Add i2c configurations to the Device Tree for DB8500 based devices

[Linus Walleij]

On Wed, Jun 13, 2012 at 6:08 PM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> From: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Date: Wed, 13 Jun 2012 16:21:14 +0100
> Subject: [PATCH 2/3] ARM: ux500: Add i2c configurations to the Device Tree
>  for DB8500 based devices
>
> For correct operation of the i2c-nomadik driver, it requires some
> information surrounding clock-frequencies, delay times and thresholds.
> Here we apply those configurations to be used when a platform is
> booted via the device Tree.
>
> Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

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

Yours,
Linus Walleij

Re: [PATCH 3/3] Documentation: Device Tree binding information for i2c-nomadik driver

[Linus Walleij]

On Wed, Jun 13, 2012 at 6:08 PM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> This document describes each of the non-standard Device Tree bindings
> used in the Nomadik I2C driver.
>
> 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 09/14] i2c: Add Device Tree support to the Nomadik I2C driver

[Mark Brown]

On Wed, Jun 13, 2012 at 01:28:17PM +0100, Lee Jones wrote:

> Device Tree. However, we have just as much control by keeping them
> in separate structs in the C file and selecting the right one using
> the compatible sting.

You're not understanding Linus' point.  The compatible string isn't
useful here because properties like the maximum clock rate of the bus
depend on the board design, not the silicon.  The controller may be
perfectly happy to run at a given rate but other devices on the bus or
the electrical engineering of the PCB itself may restrict this further.

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

[Mark Brown]

On Thu, Jun 14, 2012 at 07:36:36PM +0100, Mark Brown wrote:

> You're not understanding Linus' point.  The compatible string isn't
> useful here because properties like the maximum clock rate of the bus
> depend on the board design, not the silicon.  The controller may be
> perfectly happy to run at a given rate but other devices on the bus or
> the electrical engineering of the PCB itself may restrict this further.

Sorry, I read the next revision and see this was actually resolved OK.

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

[Lee Jones]

On 14/06/12 19:36, Mark Brown wrote:
> On Wed, Jun 13, 2012 at 01:28:17PM +0100, Lee Jones wrote:
>
>> Device Tree. However, we have just as much control by keeping them
>> in separate structs in the C file and selecting the right one using
>> the compatible sting.
>
> You're not understanding Linus' point.  The compatible string isn't
> useful here because properties like the maximum clock rate of the bus
> depend on the board design, not the silicon.  The controller may be
> perfectly happy to run at a given rate but other devices on the bus or
> the electrical engineering of the PCB itself may restrict this further.

And you're not understanding mine. ;)

You can have multiple compatible strings for a single driver. Which one 
you reference from the Device Tree will dictate which group of settings 
are used, including variation of clock rates.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Lee Jones]

On 14/06/12 19:46, Mark Brown wrote:
> On Thu, Jun 14, 2012 at 07:36:36PM +0100, Mark Brown wrote:
>
>> You're not understanding Linus' point.  The compatible string isn't
>> useful here because properties like the maximum clock rate of the bus
>> depend on the board design, not the silicon.  The controller may be
>> perfectly happy to run at a given rate but other devices on the bus or
>> the electrical engineering of the PCB itself may restrict this further.
>
> Sorry, I read the next revision and see this was actually resolved OK.

Yes, I just went ahead created the bindings anyway. I figured it would 
be neater (if no more functional) to keep all the variations in DT. 
Especially if we had devices which only varied by one or two settings, 
which would still require a complete new struct using the previous method.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Lee Jones]

From: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Date: Tue, 17 Apr 2012 16:04:13 +0100
Subject: [PATCH 1/1] i2c: 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. We also apply a fall-back
configuration in case either one is not provided, or a required
element is missing from the one supplied.

Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/i2c/busses/i2c-nomadik.c |   82 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 80 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index a92440d..58e8114 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -23,6 +23,7 @@
 #include <linux/io.h>
 #include <linux/regulator/consumer.h>
 #include <linux/pm_runtime.h>
+#include <linux/of.h>
 
 #include <plat/i2c.h>
 
@@ -899,15 +900,86 @@ static const struct i2c_algorithm nmk_i2c_algo = {
 	.functionality	= nmk_i2c_functionality
 };
 
+static struct nmk_i2c_controller u8500_i2c = {
+	/*
+	 * Slave data setup time; 250ns, 100ns, and 10ns, which
+	 * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
+	 */
+	.slsu           = 0xe,
+	.tft            = 1,      /* Tx FIFO threshold */
+	.rft            = 8,      /* Rx FIFO threshold */
+	.clk_freq       = 100000, /* std. mode operation */
+	.timeout        = 200,    /* Slave response timeout(ms) */
+	.sm             = I2C_FREQ_MODE_FAST,
+};
+
+static int __devinit
+nmk_i2c_of_probe(struct device_node *np, struct nmk_i2c_controller *pdata)
+{
+	of_property_read_u32(np, "clock-frequency", (u32*)&pdata->clk_freq);
+	if (!pdata->clk_freq) {
+		pr_warn("%s: Clock frequency not found\n", np->full_name);
+		return -EINVAL;
+	}
+
+	of_property_read_u32(np, "stericsson,slsu", (u32*)&pdata->slsu);
+	if (!pdata->slsu) {
+		pr_warn("%s: Data line delay not found\n", np->full_name);
+		return -EINVAL;
+	}
+
+	of_property_read_u32(np, "stericsson,tft", (u32*)&pdata->tft);
+	if (!pdata->tft) {
+		pr_warn("%s: Tx FIFO threshold not found\n", np->full_name);
+		return -EINVAL;
+	}
+
+	of_property_read_u32(np, "stericsson,rft", (u32*)&pdata->rft);
+	if (!pdata->rft) {
+		pr_warn("%s: Rx FIFO threshold not found\n", np->full_name);
+		return -EINVAL;
+	}
+
+	of_property_read_u32(np, "stericsson,timeout", (u32*)&pdata->timeout);
+	if (!pdata->timeout) {
+		pr_warn("%s: Timeout not found\n", np->full_name);
+		return -EINVAL;
+	}
+
+	if (of_get_property(np, "stericsson,i2c_freq_mode_fast", NULL))
+		pdata->sm = I2C_FREQ_MODE_FAST;
+	else
+		pdata->sm = I2C_FREQ_MODE_STANDARD;
+
+	return 0;
+}
+
 static int __devinit nmk_i2c_probe(struct platform_device *pdev)
 {
 	int ret = 0;
 	struct resource *res;
-	struct nmk_i2c_controller *pdata =
-			pdev->dev.platform_data;
+	struct nmk_i2c_controller *pdata = pdev->dev.platform_data;
+	struct device_node *np = pdev->dev.of_node;
 	struct nmk_i2c_dev	*dev;
 	struct i2c_adapter *adap;
 
+	if (np) {
+		if (!pdata) {
+			pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+			if (!pdata) {
+				ret = -ENOMEM;
+				goto err_no_mem;
+			}
+		}
+		ret = nmk_i2c_of_probe(np, pdata);
+		if (ret)
+			kfree(pdata);
+	}
+
+	if (!pdata)
+		/* No i2c configuration found, using the default. */
+		pdata = &u8500_i2c;
+
 	dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
 	if (!dev) {
 		dev_err(&pdev->dev, "cannot allocate memory\n");
@@ -1038,11 +1110,17 @@ static int __devexit nmk_i2c_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id nmk_gpio_match[] = {
+	{ .compatible = "st,nomadik-i2c", },
+	{},
+};
+
 static struct platform_driver nmk_i2c_driver = {
 	.driver = {
 		.owner = THIS_MODULE,
 		.name = DRIVER_NAME,
 		.pm = &nmk_i2c_pm,
+		.of_match_table = nmk_gpio_match,
 	},
 	.probe = nmk_i2c_probe,
 	.remove = __devexit_p(nmk_i2c_remove),
-- 
1.7.9.5

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

[Mark Brown]

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

On Thu, Jun 14, 2012 at 07:57:32PM +0100, Lee Jones wrote:
> On 14/06/12 19:36, Mark Brown wrote:

> >You're not understanding Linus' point.  The compatible string isn't
> >useful here because properties like the maximum clock rate of the bus
> >depend on the board design, not the silicon.  The controller may be
> >perfectly happy to run at a given rate but other devices on the bus or
> >the electrical engineering of the PCB itself may restrict this further.

> And you're not understanding mine. ;)

I think we are (or at least I do)...

> You can have multiple compatible strings for a single driver. Which
> one you reference from the Device Tree will dictate which group of
> settings are used, including variation of clock rates.

I'm certainly well aware of that.  The problem is that the compatible
strings should identify a particular IP or piece of silicon and be
unchanging properties of that hardware - for example, working out how
big a FIFO in a device is from the compatible information is totally
sensible and reasonable.  Things like the I2C bus clock speed on the
other hand depend not on the individual device but on how it's been
integrated into the system so you'd end up saying things like
"nomadik-i2c-100khz-bus" in the compatible string which isn't great.  

If there's something like variation in the maximum speed that can be
supported by the different silicon the driver handles then working that
out from compatible and using it as the default is sensible but that's
not the same thing as providing customisation of the system bus
frequency to the user and if there's only one of those two that's
configurable it should be the system bus frequency.

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

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

[Lee Jones]

On 15/06/12 10:32, Mark Brown wrote:
> On Thu, Jun 14, 2012 at 07:57:32PM +0100, Lee Jones wrote:
>> On 14/06/12 19:36, Mark Brown wrote:
>
>>> You're not understanding Linus' point.  The compatible string isn't
>>> useful here because properties like the maximum clock rate of the bus
>>> depend on the board design, not the silicon.  The controller may be
>>> perfectly happy to run at a given rate but other devices on the bus or
>>> the electrical engineering of the PCB itself may restrict this further.
>
>> And you're not understanding mine. ;)
>
> I think we are (or at least I do)...
>
>> You can have multiple compatible strings for a single driver. Which
>> one you reference from the Device Tree will dictate which group of
>> settings are used, including variation of clock rates.
>
> I'm certainly well aware of that.  The problem is that the compatible
> strings should identify a particular IP or piece of silicon and be
> unchanging properties of that hardware - for example, working out how
> big a FIFO in a device is from the compatible information is totally
> sensible and reasonable.  Things like the I2C bus clock speed on the
> other hand depend not on the individual device but on how it's been
> integrated into the system so you'd end up saying things like
> "nomadik-i2c-100khz-bus" in the compatible string which isn't great.
>
> If there's something like variation in the maximum speed that can be
> supported by the different silicon the driver handles then working that
> out from compatible and using it as the default is sensible but that's
> not the same thing as providing customisation of the system bus
> frequency to the user and if there's only one of those two that's
> configurable it should be the system bus frequency.

All agreed. I think we're singing off the same hymn-sheet here. :)

I was just saying that it was possible to pick a selection of 
configurations from a compatible string alone. I also mentioned that on 
reflection it would be neater to specify the properties individually 
from Device Tree in case of small variations in configuration, 
preventing us from using compatible strings like the one you mentioned.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Russell King - ARM Linux]

On Thu, Jun 14, 2012 at 07:57:32PM +0100, Lee Jones wrote:
> On 14/06/12 19:36, Mark Brown wrote:
>> On Wed, Jun 13, 2012 at 01:28:17PM +0100, Lee Jones wrote:
>>
>>> Device Tree. However, we have just as much control by keeping them
>>> in separate structs in the C file and selecting the right one using
>>> the compatible sting.
>>
>> You're not understanding Linus' point.  The compatible string isn't
>> useful here because properties like the maximum clock rate of the bus
>> depend on the board design, not the silicon.  The controller may be
>> perfectly happy to run at a given rate but other devices on the bus or
>> the electrical engineering of the PCB itself may restrict this further.
>
> And you're not understanding mine. ;)
>
> You can have multiple compatible strings for a single driver. Which one  
> you reference from the Device Tree will dictate which group of settings  
> are used, including variation of clock rates.

However, if you list these settings separately, rather than selecting them
based on a compatible string, it means when other board designs come along,
you don't have to modify the kernel code to provide yet another compatible
to settings translation in the code - you can keep all that information
entirely within DT with no kernel code mods.

I thought that was partly the point of DT...

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

[Lee Jones]

On 15/06/12 11:32, Russell King - ARM Linux wrote:
> On Thu, Jun 14, 2012 at 07:57:32PM +0100, Lee Jones wrote:
>> On 14/06/12 19:36, Mark Brown wrote:
>>> On Wed, Jun 13, 2012 at 01:28:17PM +0100, Lee Jones wrote:
>>>
>>>> Device Tree. However, we have just as much control by keeping them
>>>> in separate structs in the C file and selecting the right one using
>>>> the compatible sting.
>>>
>>> You're not understanding Linus' point.  The compatible string isn't
>>> useful here because properties like the maximum clock rate of the bus
>>> depend on the board design, not the silicon.  The controller may be
>>> perfectly happy to run at a given rate but other devices on the bus or
>>> the electrical engineering of the PCB itself may restrict this further.
>>
>> And you're not understanding mine. ;)
>>
>> You can have multiple compatible strings for a single driver. Which one
>> you reference from the Device Tree will dictate which group of settings
>> are used, including variation of clock rates.
>
> However, if you list these settings separately, rather than selecting them
> based on a compatible string, it means when other board designs come along,
> you don't have to modify the kernel code to provide yet another compatible
> to settings translation in the code - you can keep all that information
> entirely within DT with no kernel code mods.
>
> I thought that was partly the point of DT...

Right. See my most recent emails on the subject. :)

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Srinidhi Kasagar]

[...]
> 
> 
> From: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Date: Tue, 17 Apr 2012 16:04:13 +0100
> Subject: [PATCH 1/1] i2c: 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. We also apply a fall-back
> configuration in case either one is not provided, or a required
> element is missing from the one supplied.
> 
> Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-nomadik.c |   82 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 80 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> index a92440d..58e8114 100644
> --- a/drivers/i2c/busses/i2c-nomadik.c
> +++ b/drivers/i2c/busses/i2c-nomadik.c
> @@ -23,6 +23,7 @@
>  #include <linux/io.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/of.h>
> 
>  #include <plat/i2c.h>
> 
> @@ -899,15 +900,86 @@ static const struct i2c_algorithm nmk_i2c_algo = {
>        .functionality  = nmk_i2c_functionality
>  };
> 
> +static struct nmk_i2c_controller u8500_i2c = {
> +       /*
> +        * Slave data setup time; 250ns, 100ns, and 10ns, which
> +        * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
> +        */
> +       .slsu           = 0xe,
> +       .tft            = 1,      /* Tx FIFO threshold */
> +       .rft            = 8,      /* Rx FIFO threshold */
> +       .clk_freq       = 100000, /* std. mode operation */
> +       .timeout        = 200,    /* Slave response timeout(ms) */
> +       .sm             = I2C_FREQ_MODE_FAST,

How is this possible? you are setting clk_freq as 100kb/s and mode
as fast mode which is supposed to be 400kb/s.

> +};
> +
> +static int __devinit
> +nmk_i2c_of_probe(struct device_node *np, struct nmk_i2c_controller *pdata)
> +{
> +       of_property_read_u32(np, "clock-frequency", (u32*)&pdata->clk_freq);
> +       if (!pdata->clk_freq) {
> +               pr_warn("%s: Clock frequency not found\n", np->full_name);
> +               return -EINVAL;
> +       }
> +
> +       of_property_read_u32(np, "stericsson,slsu", (u32*)&pdata->slsu);
> +       if (!pdata->slsu) {
> +               pr_warn("%s: Data line delay not found\n", np->full_name);
> +               return -EINVAL;
> +       }
> +
> +       of_property_read_u32(np, "stericsson,tft", (u32*)&pdata->tft);
> +       if (!pdata->tft) {
> +               pr_warn("%s: Tx FIFO threshold not found\n", np->full_name);
> +               return -EINVAL;
> +       }
> +
> +       of_property_read_u32(np, "stericsson,rft", (u32*)&pdata->rft);
> +       if (!pdata->rft) {
> +               pr_warn("%s: Rx FIFO threshold not found\n", np->full_name);
> +               return -EINVAL;
> +       }
> +
> +       of_property_read_u32(np, "stericsson,timeout", (u32*)&pdata->timeout);
> +       if (!pdata->timeout) {
> +               pr_warn("%s: Timeout not found\n", np->full_name);
> +               return -EINVAL;
> +       }
> +
> +       if (of_get_property(np, "stericsson,i2c_freq_mode_fast", NULL))
> +               pdata->sm = I2C_FREQ_MODE_FAST;
> +       else
> +               pdata->sm = I2C_FREQ_MODE_STANDARD;

The controller has more modes like fast mode plus and high speed mode. You can't make
assumptions like this.

> +
> +       return 0;
> +}
> +
>  static int __devinit nmk_i2c_probe(struct platform_device *pdev)
>  {
>        int ret = 0;
>        struct resource *res;
> -       struct nmk_i2c_controller *pdata =
> -                       pdev->dev.platform_data;
> +       struct nmk_i2c_controller *pdata = pdev->dev.platform_data;
> +       struct device_node *np = pdev->dev.of_node;
>        struct nmk_i2c_dev      *dev;
>        struct i2c_adapter *adap;
> 
> +       if (np) {
> +               if (!pdata) {
> +                       pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +                       if (!pdata) {
> +                               ret = -ENOMEM;
> +                               goto err_no_mem;
> +                       }
> +               }
> +               ret = nmk_i2c_of_probe(np, pdata);
> +               if (ret)
> +                       kfree(pdata);
> +       }
> +
> +       if (!pdata)
> +               /* No i2c configuration found, using the default. */
> +               pdata = &u8500_i2c;

There are redundant fallback configurations exists in the driver. You might need to remove 
those, if you want to fallback here.

> +
>        dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
>        if (!dev) {
>                dev_err(&pdev->dev, "cannot allocate memory\n");
> @@ -1038,11 +1110,17 @@ static int __devexit nmk_i2c_remove(struct platform_device *pdev)
>        return 0;
>  }
> 
> +static const struct of_device_id nmk_gpio_match[] = {
> +       { .compatible = "st,nomadik-i2c", },
> +       {},
> +};
> +
>  static struct platform_driver nmk_i2c_driver = {
>        .driver = {
>                .owner = THIS_MODULE,
>                .name = DRIVER_NAME,
>                .pm = &nmk_i2c_pm,
> +               .of_match_table = nmk_gpio_match,
>        },
>        .probe = nmk_i2c_probe,
>        .remove = __devexit_p(nmk_i2c_remove),
> --
> 1.7.9.5
> 

/srinidhi 

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

[Lee Jones]

On 15/06/12 12:50, Srinidhi Kasagar wrote:
> [...]
>>
>>
>> From: Lee Jones<lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Date: Tue, 17 Apr 2012 16:04:13 +0100
>> Subject: [PATCH 1/1] i2c: 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. We also apply a fall-back
>> configuration in case either one is not provided, or a required
>> element is missing from the one supplied.
>>
>> Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Signed-off-by: Lee Jones<lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>   drivers/i2c/busses/i2c-nomadik.c |   82 +++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 80 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
>> index a92440d..58e8114 100644
>> --- a/drivers/i2c/busses/i2c-nomadik.c
>> +++ b/drivers/i2c/busses/i2c-nomadik.c
>> @@ -23,6 +23,7 @@
>>   #include<linux/io.h>
>>   #include<linux/regulator/consumer.h>
>>   #include<linux/pm_runtime.h>
>> +#include<linux/of.h>
>>
>>   #include<plat/i2c.h>
>>
>> @@ -899,15 +900,86 @@ static const struct i2c_algorithm nmk_i2c_algo = {
>>         .functionality  = nmk_i2c_functionality
>>   };
>>
>> +static struct nmk_i2c_controller u8500_i2c = {
>> +       /*
>> +        * Slave data setup time; 250ns, 100ns, and 10ns, which
>> +        * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
>> +        */
>> +       .slsu           = 0xe,
>> +       .tft            = 1,      /* Tx FIFO threshold */
>> +       .rft            = 8,      /* Rx FIFO threshold */
>> +       .clk_freq       = 100000, /* std. mode operation */
>> +       .timeout        = 200,    /* Slave response timeout(ms) */
>> +       .sm             = I2C_FREQ_MODE_FAST,
>
> How is this possible? you are setting clk_freq as 100kb/s and mode
> as fast mode which is supposed to be 400kb/s.

That's not how I read it:

> 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 */
> };

The operative words here are "up to".

>> +};
>> +
>> +static int __devinit
>> +nmk_i2c_of_probe(struct device_node *np, struct nmk_i2c_controller *pdata)
>> +{
>> +       of_property_read_u32(np, "clock-frequency", (u32*)&pdata->clk_freq);
>> +       if (!pdata->clk_freq) {
>> +               pr_warn("%s: Clock frequency not found\n", np->full_name);
>> +               return -EINVAL;
>> +       }
>> +
>> +       of_property_read_u32(np, "stericsson,slsu", (u32*)&pdata->slsu);
>> +       if (!pdata->slsu) {
>> +               pr_warn("%s: Data line delay not found\n", np->full_name);
>> +               return -EINVAL;
>> +       }
>> +
>> +       of_property_read_u32(np, "stericsson,tft", (u32*)&pdata->tft);
>> +       if (!pdata->tft) {
>> +               pr_warn("%s: Tx FIFO threshold not found\n", np->full_name);
>> +               return -EINVAL;
>> +       }
>> +
>> +       of_property_read_u32(np, "stericsson,rft", (u32*)&pdata->rft);
>> +       if (!pdata->rft) {
>> +               pr_warn("%s: Rx FIFO threshold not found\n", np->full_name);
>> +               return -EINVAL;
>> +       }
>> +
>> +       of_property_read_u32(np, "stericsson,timeout", (u32*)&pdata->timeout);
>> +       if (!pdata->timeout) {
>> +               pr_warn("%s: Timeout not found\n", np->full_name);
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (of_get_property(np, "stericsson,i2c_freq_mode_fast", NULL))
>> +               pdata->sm = I2C_FREQ_MODE_FAST;
>> +       else
>> +               pdata->sm = I2C_FREQ_MODE_STANDARD;
>
> The controller has more modes like fast mode plus and high speed mode. You can't make
> assumptions like this.

These are currently unsupported. Read the comments in the file:

> 	/*
> 	 * set the speed mode. Currently we support
> 	 * only standard and fast mode of operation
> 	 * TODO - support for fast mode plus (up to 1Mb/s)
> 	 * and high speed (up to 3.4 Mb/s)
> 	 */

I can add a similar comment here if it make you feel better?

>> +
>> +       return 0;
>> +}
>> +
>>   static int __devinit nmk_i2c_probe(struct platform_device *pdev)
>>   {
>>         int ret = 0;
>>         struct resource *res;
>> -       struct nmk_i2c_controller *pdata =
>> -                       pdev->dev.platform_data;
>> +       struct nmk_i2c_controller *pdata = pdev->dev.platform_data;
>> +       struct device_node *np = pdev->dev.of_node;
>>         struct nmk_i2c_dev      *dev;
>>         struct i2c_adapter *adap;
>>
>> +       if (np) {
>> +               if (!pdata) {
>> +                       pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>> +                       if (!pdata) {
>> +                               ret = -ENOMEM;
>> +                               goto err_no_mem;
>> +                       }
>> +               }
>> +               ret = nmk_i2c_of_probe(np, pdata);
>> +               if (ret)
>> +                       kfree(pdata);
>> +       }
>> +
>> +       if (!pdata)
>> +               /* No i2c configuration found, using the default. */
>> +               pdata =&u8500_i2c;
>
> There are redundant fallback configurations exists in the driver. You might need to remove
> those, if you want to fallback here.

I only see a fall-back for clk_freq. Are there others?

>> +
>>         dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
>>         if (!dev) {
>>                 dev_err(&pdev->dev, "cannot allocate memory\n");
>> @@ -1038,11 +1110,17 @@ static int __devexit nmk_i2c_remove(struct platform_device *pdev)
>>         return 0;
>>   }
>>
>> +static const struct of_device_id nmk_gpio_match[] = {
>> +       { .compatible = "st,nomadik-i2c", },
>> +       {},
>> +};
>> +
>>   static struct platform_driver nmk_i2c_driver = {
>>         .driver = {
>>                 .owner = THIS_MODULE,
>>                 .name = DRIVER_NAME,
>>                 .pm =&nmk_i2c_pm,
>> +               .of_match_table = nmk_gpio_match,
>>         },
>>         .probe = nmk_i2c_probe,
>>         .remove = __devexit_p(nmk_i2c_remove),
>> --
>> 1.7.9.5
>>

Kind regards,
Lee

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Srinidhi Kasagar]

On Fri, Jun 15, 2012 at 14:45:11 +0200, Lee Jones wrote:
> On 15/06/12 12:50, Srinidhi Kasagar wrote:
> > [...]
> >>
> >>
> >> From: Lee Jones<lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> Date: Tue, 17 Apr 2012 16:04:13 +0100
> >> Subject: [PATCH 1/1] i2c: 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. We also apply a fall-back
> >> configuration in case either one is not provided, or a required
> >> element is missing from the one supplied.
> >>
> >> Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> Signed-off-by: Lee Jones<lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> ---
> >>   drivers/i2c/busses/i2c-nomadik.c |   82 +++++++++++++++++++++++++++++++++++++-
> >>   1 file changed, 80 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> >> index a92440d..58e8114 100644
> >> --- a/drivers/i2c/busses/i2c-nomadik.c
> >> +++ b/drivers/i2c/busses/i2c-nomadik.c
> >> @@ -23,6 +23,7 @@
> >>   #include<linux/io.h>
> >>   #include<linux/regulator/consumer.h>
> >>   #include<linux/pm_runtime.h>
> >> +#include<linux/of.h>
> >>
> >>   #include<plat/i2c.h>
> >>
> >> @@ -899,15 +900,86 @@ static const struct i2c_algorithm nmk_i2c_algo = {
> >>         .functionality  = nmk_i2c_functionality
> >>   };
> >>
> >> +static struct nmk_i2c_controller u8500_i2c = {
> >> +       /*
> >> +        * Slave data setup time; 250ns, 100ns, and 10ns, which
> >> +        * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
> >> +        */
> >> +       .slsu           = 0xe,
> >> +       .tft            = 1,      /* Tx FIFO threshold */
> >> +       .rft            = 8,      /* Rx FIFO threshold */
> >> +       .clk_freq       = 100000, /* std. mode operation */
> >> +       .timeout        = 200,    /* Slave response timeout(ms) */
> >> +       .sm             = I2C_FREQ_MODE_FAST,
> >
> > How is this possible? you are setting clk_freq as 100kb/s and mode
> > as fast mode which is supposed to be 400kb/s.
> 
> That's not how I read it:

But it is not readable. It confuses people.

> 
> > 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 */
> > };
> 
> The operative words here are "up to".

I know, then none of these modes does not make sense.

> 
> >> +};
> >> +
> >> +static int __devinit
> >> +nmk_i2c_of_probe(struct device_node *np, struct nmk_i2c_controller *pdata)
> >> +{
> >> +       of_property_read_u32(np, "clock-frequency", (u32*)&pdata->clk_freq);
> >> +       if (!pdata->clk_freq) {
> >> +               pr_warn("%s: Clock frequency not found\n", np->full_name);
> >> +               return -EINVAL;
> >> +       }
> >> +
> >> +       of_property_read_u32(np, "stericsson,slsu", (u32*)&pdata->slsu);
> >> +       if (!pdata->slsu) {
> >> +               pr_warn("%s: Data line delay not found\n", np->full_name);
> >> +               return -EINVAL;
> >> +       }
> >> +
> >> +       of_property_read_u32(np, "stericsson,tft", (u32*)&pdata->tft);
> >> +       if (!pdata->tft) {
> >> +               pr_warn("%s: Tx FIFO threshold not found\n", np->full_name);
> >> +               return -EINVAL;
> >> +       }
> >> +
> >> +       of_property_read_u32(np, "stericsson,rft", (u32*)&pdata->rft);
> >> +       if (!pdata->rft) {
> >> +               pr_warn("%s: Rx FIFO threshold not found\n", np->full_name);
> >> +               return -EINVAL;
> >> +       }
> >> +
> >> +       of_property_read_u32(np, "stericsson,timeout", (u32*)&pdata->timeout);
> >> +       if (!pdata->timeout) {
> >> +               pr_warn("%s: Timeout not found\n", np->full_name);
> >> +               return -EINVAL;
> >> +       }
> >> +
> >> +       if (of_get_property(np, "stericsson,i2c_freq_mode_fast", NULL))
> >> +               pdata->sm = I2C_FREQ_MODE_FAST;
> >> +       else
> >> +               pdata->sm = I2C_FREQ_MODE_STANDARD;
> >
> > The controller has more modes like fast mode plus and high speed mode. You can't make
> > assumptions like this.
> 
> These are currently unsupported. Read the comments in the file:

I wrote this code, I remember these comments very well :)

> 
> > 	/*
> > 	 * set the speed mode. Currently we support
> > 	 * only standard and fast mode of operation
> > 	 * TODO - support for fast mode plus (up to 1Mb/s)
> > 	 * and high speed (up to 3.4 Mb/s)
> > 	 */
> 
> I can add a similar comment here if it make you feel better?

Please.

> 
> >> +
> >> +       return 0;
> >> +}
> >> +
> >>   static int __devinit nmk_i2c_probe(struct platform_device *pdev)
> >>   {
> >>         int ret = 0;
> >>         struct resource *res;
> >> -       struct nmk_i2c_controller *pdata =
> >> -                       pdev->dev.platform_data;
> >> +       struct nmk_i2c_controller *pdata = pdev->dev.platform_data;
> >> +       struct device_node *np = pdev->dev.of_node;
> >>         struct nmk_i2c_dev      *dev;
> >>         struct i2c_adapter *adap;
> >>
> >> +       if (np) {
> >> +               if (!pdata) {
> >> +                       pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> >> +                       if (!pdata) {
> >> +                               ret = -ENOMEM;
> >> +                               goto err_no_mem;
> >> +                       }
> >> +               }
> >> +               ret = nmk_i2c_of_probe(np, pdata);
> >> +               if (ret)
> >> +                       kfree(pdata);
> >> +       }
> >> +
> >> +       if (!pdata)
> >> +               /* No i2c configuration found, using the default. */
> >> +               pdata =&u8500_i2c;
> >
> > There are redundant fallback configurations exists in the driver. You might need to remove
> > those, if you want to fallback here.
> 
> I only see a fall-back for clk_freq. Are there others?

No, I can see that it only exists in setup_i2c_controller(). Please fix.

> 
> >> +
> >>         dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
> >>         if (!dev) {
> >>                 dev_err(&pdev->dev, "cannot allocate memory\n");
> >> @@ -1038,11 +1110,17 @@ static int __devexit nmk_i2c_remove(struct platform_device *pdev)
> >>         return 0;
> >>   }
> >>
> >> +static const struct of_device_id nmk_gpio_match[] = {
> >> +       { .compatible = "st,nomadik-i2c", },
> >> +       {},
> >> +};
> >> +
> >>   static struct platform_driver nmk_i2c_driver = {
> >>         .driver = {
> >>                 .owner = THIS_MODULE,
> >>                 .name = DRIVER_NAME,
> >>                 .pm =&nmk_i2c_pm,
> >> +               .of_match_table = nmk_gpio_match,
> >>         },
> >>         .probe = nmk_i2c_probe,
> >>         .remove = __devexit_p(nmk_i2c_remove),
> >> --
> >> 1.7.9.5
> >>
> 
> Kind regards,
> Lee
> 
> -- 
> Lee Jones
> Linaro ST-Ericsson Landing Team Lead
> M: +44 77 88 633 515
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

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

[Lee Jones]

On 15/06/12 14:05, Srinidhi Kasagar wrote:
> On Fri, Jun 15, 2012 at 14:45:11 +0200, Lee Jones wrote:
>> On 15/06/12 12:50, Srinidhi Kasagar wrote:
>>> [...]
>>>>
>>>>
>>>> From: Lee Jones<lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>> Date: Tue, 17 Apr 2012 16:04:13 +0100
>>>> Subject: [PATCH 1/1] i2c: 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. We also apply a fall-back
>>>> configuration in case either one is not provided, or a required
>>>> element is missing from the one supplied.
>>>>
>>>> Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>>> Signed-off-by: Lee Jones<lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>> ---
>>>>    drivers/i2c/busses/i2c-nomadik.c |   82 +++++++++++++++++++++++++++++++++++++-
>>>>    1 file changed, 80 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
>>>> index a92440d..58e8114 100644
>>>> --- a/drivers/i2c/busses/i2c-nomadik.c
>>>> +++ b/drivers/i2c/busses/i2c-nomadik.c
>>>> @@ -23,6 +23,7 @@
>>>>    #include<linux/io.h>
>>>>    #include<linux/regulator/consumer.h>
>>>>    #include<linux/pm_runtime.h>
>>>> +#include<linux/of.h>
>>>>
>>>>    #include<plat/i2c.h>
>>>>
>>>> @@ -899,15 +900,86 @@ static const struct i2c_algorithm nmk_i2c_algo = {
>>>>          .functionality  = nmk_i2c_functionality
>>>>    };
>>>>
>>>> +static struct nmk_i2c_controller u8500_i2c = {
>>>> +       /*
>>>> +        * Slave data setup time; 250ns, 100ns, and 10ns, which
>>>> +        * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
>>>> +        */
>>>> +       .slsu           = 0xe,
>>>> +       .tft            = 1,      /* Tx FIFO threshold */
>>>> +       .rft            = 8,      /* Rx FIFO threshold */
>>>> +       .clk_freq       = 100000, /* std. mode operation */
>>>> +       .timeout        = 200,    /* Slave response timeout(ms) */
>>>> +       .sm             = I2C_FREQ_MODE_FAST,
>>>
>>> How is this possible? you are setting clk_freq as 100kb/s and mode
>>> as fast mode which is supposed to be 400kb/s.
>>
>> That's not how I read it:
>
> But it is not readable. It confuses people.

I understood it. :)

If you think it's unclear speak to the author, Linus. He's CC'ed.

>>> 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 */
>>> };
>>
>> The operative words here are "up to".
>
> I know, then none of these modes does not make sense.
 >
>>>> +};
>>>> +
>>>> +static int __devinit
>>>> +nmk_i2c_of_probe(struct device_node *np, struct nmk_i2c_controller *pdata)
>>>> +{
>>>> +       of_property_read_u32(np, "clock-frequency", (u32*)&pdata->clk_freq);
>>>> +       if (!pdata->clk_freq) {
>>>> +               pr_warn("%s: Clock frequency not found\n", np->full_name);
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       of_property_read_u32(np, "stericsson,slsu", (u32*)&pdata->slsu);
>>>> +       if (!pdata->slsu) {
>>>> +               pr_warn("%s: Data line delay not found\n", np->full_name);
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       of_property_read_u32(np, "stericsson,tft", (u32*)&pdata->tft);
>>>> +       if (!pdata->tft) {
>>>> +               pr_warn("%s: Tx FIFO threshold not found\n", np->full_name);
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       of_property_read_u32(np, "stericsson,rft", (u32*)&pdata->rft);
>>>> +       if (!pdata->rft) {
>>>> +               pr_warn("%s: Rx FIFO threshold not found\n", np->full_name);
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       of_property_read_u32(np, "stericsson,timeout", (u32*)&pdata->timeout);
>>>> +       if (!pdata->timeout) {
>>>> +               pr_warn("%s: Timeout not found\n", np->full_name);
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       if (of_get_property(np, "stericsson,i2c_freq_mode_fast", NULL))
>>>> +               pdata->sm = I2C_FREQ_MODE_FAST;
>>>> +       else
>>>> +               pdata->sm = I2C_FREQ_MODE_STANDARD;
>>>
>>> The controller has more modes like fast mode plus and high speed mode. You can't make
>>> assumptions like this.
>>
>> These are currently unsupported. Read the comments in the file:
>
> I wrote this code, I remember these comments very well :)

Yes, I saw. I thought that perhaps you'd forgotten. ;)

>>> 	/*
>>> 	 * set the speed mode. Currently we support
>>> 	 * only standard and fast mode of operation
>>> 	 * TODO - support for fast mode plus (up to 1Mb/s)
>>> 	 * and high speed (up to 3.4 Mb/s)
>>> 	 */
>>
>> I can add a similar comment here if it make you feel better?
>
> Please.

No problem.

>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>>    static int __devinit nmk_i2c_probe(struct platform_device *pdev)
>>>>    {
>>>>          int ret = 0;
>>>>          struct resource *res;
>>>> -       struct nmk_i2c_controller *pdata =
>>>> -                       pdev->dev.platform_data;
>>>> +       struct nmk_i2c_controller *pdata = pdev->dev.platform_data;
>>>> +       struct device_node *np = pdev->dev.of_node;
>>>>          struct nmk_i2c_dev      *dev;
>>>>          struct i2c_adapter *adap;
>>>>
>>>> +       if (np) {
>>>> +               if (!pdata) {
>>>> +                       pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>>>> +                       if (!pdata) {
>>>> +                               ret = -ENOMEM;
>>>> +                               goto err_no_mem;
>>>> +                       }
>>>> +               }
>>>> +               ret = nmk_i2c_of_probe(np, pdata);
>>>> +               if (ret)
>>>> +                       kfree(pdata);
>>>> +       }
>>>> +
>>>> +       if (!pdata)
>>>> +               /* No i2c configuration found, using the default. */
>>>> +               pdata =&u8500_i2c;
>>>
>>> There are redundant fallback configurations exists in the driver. You might need to remove
>>> those, if you want to fallback here.
>>
>> I only see a fall-back for clk_freq. Are there others?
>
> No, I can see that it only exists in setup_i2c_controller(). Please fix.

I can remove it, no problem.

>>>> +
>>>>          dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
>>>>          if (!dev) {
>>>>                  dev_err(&pdev->dev, "cannot allocate memory\n");
>>>> @@ -1038,11 +1110,17 @@ static int __devexit nmk_i2c_remove(struct platform_device *pdev)
>>>>          return 0;
>>>>    }
>>>>
>>>> +static const struct of_device_id nmk_gpio_match[] = {
>>>> +       { .compatible = "st,nomadik-i2c", },
>>>> +       {},
>>>> +};
>>>> +
>>>>    static struct platform_driver nmk_i2c_driver = {
>>>>          .driver = {
>>>>                  .owner = THIS_MODULE,
>>>>                  .name = DRIVER_NAME,
>>>>                  .pm =&nmk_i2c_pm,
>>>> +               .of_match_table = nmk_gpio_match,
>>>>          },
>>>>          .probe = nmk_i2c_probe,
>>>>          .remove = __devexit_p(nmk_i2c_remove),
>>>> --
>>>> 1.7.9.5
>>>>

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Srinidhi Kasagar]

On Fri, Jun 15, 2012 at 15:18:58 +0200, Lee Jones wrote:
> On 15/06/12 14:05, Srinidhi Kasagar wrote:
> > On Fri, Jun 15, 2012 at 14:45:11 +0200, Lee Jones wrote:
> >> On 15/06/12 12:50, Srinidhi Kasagar wrote:
> >>> [...]
> >>>>
> >>>>
> >>>> From: Lee Jones<lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>>> Date: Tue, 17 Apr 2012 16:04:13 +0100
> >>>> Subject: [PATCH 1/1] i2c: 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. We also apply a fall-back
> >>>> configuration in case either one is not provided, or a required
> >>>> element is missing from the one supplied.
> >>>>
> >>>> Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >>>> Signed-off-by: Lee Jones<lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>>> ---
> >>>>    drivers/i2c/busses/i2c-nomadik.c |   82 +++++++++++++++++++++++++++++++++++++-
> >>>>    1 file changed, 80 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> >>>> index a92440d..58e8114 100644
> >>>> --- a/drivers/i2c/busses/i2c-nomadik.c
> >>>> +++ b/drivers/i2c/busses/i2c-nomadik.c
> >>>> @@ -23,6 +23,7 @@
> >>>>    #include<linux/io.h>
> >>>>    #include<linux/regulator/consumer.h>
> >>>>    #include<linux/pm_runtime.h>
> >>>> +#include<linux/of.h>
> >>>>
> >>>>    #include<plat/i2c.h>
> >>>>
> >>>> @@ -899,15 +900,86 @@ static const struct i2c_algorithm nmk_i2c_algo = {
> >>>>          .functionality  = nmk_i2c_functionality
> >>>>    };
> >>>>
> >>>> +static struct nmk_i2c_controller u8500_i2c = {
> >>>> +       /*
> >>>> +        * Slave data setup time; 250ns, 100ns, and 10ns, which
> >>>> +        * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
> >>>> +        */
> >>>> +       .slsu           = 0xe,

And BTW, I forgot to mention: This slsu stuff is not needed at all.
It is required only in case of slave mode operation, which the
driver does not support. You can perhaps consider deprecating
this parameter from device tree list as well as from platform data.

> >>>> +       .tft            = 1,      /* Tx FIFO threshold */
> >>>> +       .rft            = 8,      /* Rx FIFO threshold */
> >>>> +       .clk_freq       = 100000, /* std. mode operation */
> >>>> +       .timeout        = 200,    /* Slave response timeout(ms) */
> >>>> +       .sm             = I2C_FREQ_MODE_FAST,
> >>>
> >>> How is this possible? you are setting clk_freq as 100kb/s and mode
> >>> as fast mode which is supposed to be 400kb/s.
> >>
> >> That's not how I read it:
> >
> > But it is not readable. It confuses people.
> 
> I understood it. :)
> 
> If you think it's unclear speak to the author, Linus. He's CC'ed.

It is!

/srinidhi

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

[Lee Jones]

On 15/06/12 14:37, Srinidhi Kasagar wrote:
> On Fri, Jun 15, 2012 at 15:18:58 +0200, Lee Jones wrote:
>> On 15/06/12 14:05, Srinidhi Kasagar wrote:
>>> On Fri, Jun 15, 2012 at 14:45:11 +0200, Lee Jones wrote:
>>>> On 15/06/12 12:50, Srinidhi Kasagar wrote:
>>>>> [...]
>>>>>>
>>>>>>
>>>>>> From: Lee Jones<lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>>>> Date: Tue, 17 Apr 2012 16:04:13 +0100
>>>>>> Subject: [PATCH 1/1] i2c: 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. We also apply a fall-back
>>>>>> configuration in case either one is not provided, or a required
>>>>>> element is missing from the one supplied.
>>>>>>
>>>>>> Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>>>>> Signed-off-by: Lee Jones<lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>>>> ---
>>>>>>     drivers/i2c/busses/i2c-nomadik.c |   82 +++++++++++++++++++++++++++++++++++++-
>>>>>>     1 file changed, 80 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
>>>>>> index a92440d..58e8114 100644
>>>>>> --- a/drivers/i2c/busses/i2c-nomadik.c
>>>>>> +++ b/drivers/i2c/busses/i2c-nomadik.c
>>>>>> @@ -23,6 +23,7 @@
>>>>>>     #include<linux/io.h>
>>>>>>     #include<linux/regulator/consumer.h>
>>>>>>     #include<linux/pm_runtime.h>
>>>>>> +#include<linux/of.h>
>>>>>>
>>>>>>     #include<plat/i2c.h>
>>>>>>
>>>>>> @@ -899,15 +900,86 @@ static const struct i2c_algorithm nmk_i2c_algo = {
>>>>>>           .functionality  = nmk_i2c_functionality
>>>>>>     };
>>>>>>
>>>>>> +static struct nmk_i2c_controller u8500_i2c = {
>>>>>> +       /*
>>>>>> +        * Slave data setup time; 250ns, 100ns, and 10ns, which
>>>>>> +        * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
>>>>>> +        */
>>>>>> +       .slsu           = 0xe,
>
> And BTW, I forgot to mention: This slsu stuff is not needed at all.
> It is required only in case of slave mode operation, which the
> driver does not support. You can perhaps consider deprecating
> this parameter from device tree list as well as from platform data.

Thanks. I will add it to my TODO list for a latter patch-set.

>>>>>> +       .tft            = 1,      /* Tx FIFO threshold */
>>>>>> +       .rft            = 8,      /* Rx FIFO threshold */
>>>>>> +       .clk_freq       = 100000, /* std. mode operation */
>>>>>> +       .timeout        = 200,    /* Slave response timeout(ms) */
>>>>>> +       .sm             = I2C_FREQ_MODE_FAST,
>>>>>
>>>>> How is this possible? you are setting clk_freq as 100kb/s and mode
>>>>> as fast mode which is supposed to be 400kb/s.
>>>>
>>>> That's not how I read it:
>>>
>>> But it is not readable. It confuses people.
>>
>> I understood it. :)
>>
>> If you think it's unclear speak to the author, Linus. He's CC'ed.
>
> It is!
>
> /srinidhi


-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Linus Walleij]

Gah, what a thread...

On Fri, Jun 15, 2012 at 3:18 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On 15/06/12 14:05, Srinidhi Kasagar wrote:
>> On Fri, Jun 15, 2012 at 14:45:11 +0200, Lee Jones wrote:
>>> On 15/06/12 12:50, Srinidhi Kasagar wrote:

>>>>> +static struct nmk_i2c_controller u8500_i2c = {
>>>>> +       /*
>>>>> +        * Slave data setup time; 250ns, 100ns, and 10ns, which
>>>>> +        * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
>>>>> +        */
>>>>> +       .slsu           = 0xe,
>>>>> +       .tft            = 1,      /* Tx FIFO threshold */
>>>>> +       .rft            = 8,      /* Rx FIFO threshold */
>>>>> +       .clk_freq       = 100000, /* std. mode operation */
>>>>> +       .timeout        = 200,    /* Slave response timeout(ms) */
>>>>> +       .sm             = I2C_FREQ_MODE_FAST,
>>>>
>>>> How is this possible? you are setting clk_freq as 100kb/s and mode
>>>> as fast mode which is supposed to be 400kb/s.
>>>
>>> That's not how I read it:
>>
>>
>> But it is not readable. It confuses people.
>
> I understood it. :)
>
> If you think it's unclear speak to the author, Linus. He's CC'ed.

Author of what? The i2c driver was written by Srinidhi. (The
MODULE_AUTHOR() clause should be a good hint...)

But it's true that board data for the ux500 kernel use
100000 Hz and I2C_FREQ_MODE_FAST, in the board-mop500.c
file, like this:

U8500_I2C_CONTROLLER(0, 0xe, 1, 8, 100000, 200, I2C_FREQ_MODE_FAST);
U8500_I2C_CONTROLLER(1, 0xe, 1, 8, 100000, 200, I2C_FREQ_MODE_FAST);
U8500_I2C_CONTROLLER(2, 0xe, 1, 8, 100000, 200, I2C_FREQ_MODE_FAST);
U8500_I2C_CONTROLLER(3, 0xe, 1, 8, 100000, 200, I2C_FREQ_MODE_FAST);

This is indeed a bit odd. But obviously it works... But reading some
fixes in another tree it seems it should look like this:

/*
 * The board uses 4 i2c controllers, initialize all of
 * them with slave data setup time of 250 ns,
 * Tx & Rx FIFO threshold values as 1 and standard
 * mode of operation
 */
U8500_I2C_CONTROLLER(0, 0xe, 1, 8, 400000, 200, I2C_FREQ_MODE_FAST);
U8500_I2C_CONTROLLER(1, 0xe, 1, 8, 400000, 200, I2C_FREQ_MODE_FAST);
U8500_I2C_CONTROLLER(2, 0xe, 1, 8, 400000, 200, I2C_FREQ_MODE_FAST);
U8500_I2C_CONTROLLER(3, 0xe, 1, 8, 400000, 200, I2C_FREQ_MODE_FAST);

Which makes *much* more sense.

I'll cook a separate patch for this or something.

Thanks,
Linus Walleij

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

[Lee Jones]

On 17/06/12 18:43, Linus Walleij wrote:
> Gah, what a thread...
>
> On Fri, Jun 15, 2012 at 3:18 PM, Lee Jones<lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>  wrote:
>> On 15/06/12 14:05, Srinidhi Kasagar wrote:
>>> On Fri, Jun 15, 2012 at 14:45:11 +0200, Lee Jones wrote:
>>>> On 15/06/12 12:50, Srinidhi Kasagar wrote:
>
>>>>>> +static struct nmk_i2c_controller u8500_i2c = {
>>>>>> +       /*
>>>>>> +        * Slave data setup time; 250ns, 100ns, and 10ns, which
>>>>>> +        * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
>>>>>> +        */
>>>>>> +       .slsu           = 0xe,
>>>>>> +       .tft            = 1,      /* Tx FIFO threshold */
>>>>>> +       .rft            = 8,      /* Rx FIFO threshold */
>>>>>> +       .clk_freq       = 100000, /* std. mode operation */
>>>>>> +       .timeout        = 200,    /* Slave response timeout(ms) */
>>>>>> +       .sm             = I2C_FREQ_MODE_FAST,
>>>>>
>>>>> How is this possible? you are setting clk_freq as 100kb/s and mode
>>>>> as fast mode which is supposed to be 400kb/s.
>>>>
>>>> That's not how I read it:
>>>
>>> But it is not readable. It confuses people.
>>
>> I understood it. :)
>>
>> If you think it's unclear speak to the author, Linus. He's CC'ed.
>
> Author of what? The i2c driver was written by Srinidhi. (The
> MODULE_AUTHOR() clause should be a good hint...)

Actually, we appear to have our wires crossed. You wrote the board data 
stuff below, which changes the mode from STANDARD to FAST, no doubt 
using Srinidhi's struct explanation comments found in 
arch/arm/plat-nomadik/include/plat/i2c.h.

 > I'll cook a separate patch for this or something.

Cool, thanks Linus.

But you're correct in what you say Linus. Srinidhi, the comments which 
are you say are confusing are the bits you are the author of: 
e208c447bd728920e4f3d438a706344ea31249b9?

> But it's true that board data for the ux500 kernel use
> 100000 Hz and I2C_FREQ_MODE_FAST, in the board-mop500.c
> file, like this:
>
> U8500_I2C_CONTROLLER(0, 0xe, 1, 8, 100000, 200, I2C_FREQ_MODE_FAST);
> U8500_I2C_CONTROLLER(1, 0xe, 1, 8, 100000, 200, I2C_FREQ_MODE_FAST);
> U8500_I2C_CONTROLLER(2, 0xe, 1, 8, 100000, 200, I2C_FREQ_MODE_FAST);
> U8500_I2C_CONTROLLER(3, 0xe, 1, 8, 100000, 200, I2C_FREQ_MODE_FAST);
 >
> This is indeed a bit odd. But obviously it works... But reading some
> fixes in another tree it seems it should look like this:
>
> /*
>   * The board uses 4 i2c controllers, initialize all of
>   * them with slave data setup time of 250 ns,
>   * Tx&  Rx FIFO threshold values as 1 and standard
>   * mode of operation
>   */
> U8500_I2C_CONTROLLER(0, 0xe, 1, 8, 400000, 200, I2C_FREQ_MODE_FAST);
> U8500_I2C_CONTROLLER(1, 0xe, 1, 8, 400000, 200, I2C_FREQ_MODE_FAST);
> U8500_I2C_CONTROLLER(2, 0xe, 1, 8, 400000, 200, I2C_FREQ_MODE_FAST);
> U8500_I2C_CONTROLLER(3, 0xe, 1, 8, 400000, 200, I2C_FREQ_MODE_FAST);
>
> Which makes *much* more sense.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Srinidhi Kasagar]

On Mon, Jun 18, 2012 at 09:18:05 +0200, Lee Jones wrote:
> On 17/06/12 18:43, Linus Walleij wrote:
> > Gah, what a thread...
> >
> > On Fri, Jun 15, 2012 at 3:18 PM, Lee Jones<lee.jones@linaro.org>  wrote:
> >> On 15/06/12 14:05, Srinidhi Kasagar wrote:
> >>> On Fri, Jun 15, 2012 at 14:45:11 +0200, Lee Jones wrote:
> >>>> On 15/06/12 12:50, Srinidhi Kasagar wrote:
> >
> >>>>>> +static struct nmk_i2c_controller u8500_i2c = {
> >>>>>> +       /*
> >>>>>> +        * Slave data setup time; 250ns, 100ns, and 10ns, which
> >>>>>> +        * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
> >>>>>> +        */
> >>>>>> +       .slsu           = 0xe,
> >>>>>> +       .tft            = 1,      /* Tx FIFO threshold */
> >>>>>> +       .rft            = 8,      /* Rx FIFO threshold */
> >>>>>> +       .clk_freq       = 100000, /* std. mode operation */
> >>>>>> +       .timeout        = 200,    /* Slave response timeout(ms) */
> >>>>>> +       .sm             = I2C_FREQ_MODE_FAST,
> >>>>>
> >>>>> How is this possible? you are setting clk_freq as 100kb/s and mode
> >>>>> as fast mode which is supposed to be 400kb/s.
> >>>>
> >>>> That's not how I read it:
> >>>
> >>> But it is not readable. It confuses people.
> >>
> >> I understood it. :)
> >>
> >> If you think it's unclear speak to the author, Linus. He's CC'ed.
> >
> > Author of what? The i2c driver was written by Srinidhi. (The
> > MODULE_AUTHOR() clause should be a good hint...)
> 
> Actually, we appear to have our wires crossed. You wrote the board data 
> stuff below, which changes the mode from STANDARD to FAST, no doubt 
> using Srinidhi's struct explanation comments found in 
> arch/arm/plat-nomadik/include/plat/i2c.h.
> 
>  > I'll cook a separate patch for this or something.
> 
> Cool, thanks Linus.
> 
> But you're correct in what you say Linus. Srinidhi, the comments which 
> are you say are confusing are the bits you are the author of: 
> e208c447bd728920e4f3d438a706344ea31249b9?

yes, it is of course evident that it's my commit from git.
I think you are not catching my point. What I mean is:
fast mode devices are downward compatible and can communicate
with the devices with standard only mode. Those comments in that
enum says exactly that (with "up to" keyword). However the code
which configures the bus in fast mode and restrict the frequency
to operate in 100 kb/s without any reason, and this creates
confusion..

Perhaps, I think we need to remove one of these
parameters as configurable options and derive out the
frequency based on the mode of operation..

/srinidhi

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

[Lee Jones]

On 18/06/12 08:58, Srinidhi Kasagar wrote:
> On Mon, Jun 18, 2012 at 09:18:05 +0200, Lee Jones wrote:
>> On 17/06/12 18:43, Linus Walleij wrote:
>>> Gah, what a thread...
>>>
>>> On Fri, Jun 15, 2012 at 3:18 PM, Lee Jones<lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>   wrote:
>>>> On 15/06/12 14:05, Srinidhi Kasagar wrote:
>>>>> On Fri, Jun 15, 2012 at 14:45:11 +0200, Lee Jones wrote:
>>>>>> On 15/06/12 12:50, Srinidhi Kasagar wrote:
>>>
>>>>>>>> +static struct nmk_i2c_controller u8500_i2c = {
>>>>>>>> +       /*
>>>>>>>> +        * Slave data setup time; 250ns, 100ns, and 10ns, which
>>>>>>>> +        * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
>>>>>>>> +        */
>>>>>>>> +       .slsu           = 0xe,
>>>>>>>> +       .tft            = 1,      /* Tx FIFO threshold */
>>>>>>>> +       .rft            = 8,      /* Rx FIFO threshold */
>>>>>>>> +       .clk_freq       = 100000, /* std. mode operation */
>>>>>>>> +       .timeout        = 200,    /* Slave response timeout(ms) */
>>>>>>>> +       .sm             = I2C_FREQ_MODE_FAST,
>>>>>>>
>>>>>>> How is this possible? you are setting clk_freq as 100kb/s and mode
>>>>>>> as fast mode which is supposed to be 400kb/s.
>>>>>>
>>>>>> That's not how I read it:
>>>>>
>>>>> But it is not readable. It confuses people.
>>>>
>>>> I understood it. :)
>>>>
>>>> If you think it's unclear speak to the author, Linus. He's CC'ed.
>>>
>>> Author of what? The i2c driver was written by Srinidhi. (The
>>> MODULE_AUTHOR() clause should be a good hint...)
>>
>> Actually, we appear to have our wires crossed. You wrote the board data
>> stuff below, which changes the mode from STANDARD to FAST, no doubt
>> using Srinidhi's struct explanation comments found in
>> arch/arm/plat-nomadik/include/plat/i2c.h.
>>
>>   >  I'll cook a separate patch for this or something.
>>
>> Cool, thanks Linus.
>>
>> But you're correct in what you say Linus. Srinidhi, the comments which
>> are you say are confusing are the bits you are the author of:
>> e208c447bd728920e4f3d438a706344ea31249b9?
>
> yes, it is of course evident that it's my commit from git.
> I think you are not catching my point. What I mean is:
> fast mode devices are downward compatible and can communicate
> with the devices with standard only mode. Those comments in that
> enum says exactly that (with "up to" keyword). However the code
> which configures the bus in fast mode and restrict the frequency
> to operate in 100 kb/s without any reason, and this creates
> confusion..
>
> Perhaps, I think we need to remove one of these
> parameters as configurable options and derive out the
> frequency based on the mode of operation..

I think Linus is fixing the frequency issues in platform code and I have 
done so from Device Tree, so I think we're hot-to-trot.

Linus, Srinidhi,

If you're happy with the patch now (not withstanding the removal of 
slsu, which I'll do in a separate patch), can you ack it please? By the 
way, which tree will this go through?

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/3] ARM: ux500: Add i2c configurations to the Device Tree for DB8500 based devices

[Linus Walleij]

On Wed, Jun 13, 2012 at 6:08 PM, Lee Jones <lee.jones@linaro.org> wrote:

> +                       clock-frequency = <100000>;
> +                       stericsson,slsu = <0xe>;
> +                       stericsson,tft = <1>;
> +                       stericsson,rft = <8>;
> +                       stericsson,timeout = <200>;
> +                       stericsson,i2c_freq_mode_fast;

In the context of my patch to the board file, we conclude that the
clock-frequency should be set to 400000 instead.

Yours,
Linus Walleij

Re: [PATCH 2/3] ARM: ux500: Add i2c configurations to the Device Tree for DB8500 based devices

[Lee Jones]

On 18/06/12 12:29, Linus Walleij wrote:
> On Wed, Jun 13, 2012 at 6:08 PM, Lee Jones<lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>  wrote:
>
>> +                       clock-frequency =<100000>;
>> +                       stericsson,slsu =<0xe>;
>> +                       stericsson,tft =<1>;
>> +                       stericsson,rft =<8>;
>> +                       stericsson,timeout =<200>;
>> +                       stericsson,i2c_freq_mode_fast;
>
> In the context of my patch to the board file, we conclude that the
> clock-frequency should be set to 400000 instead.

Right, I have already changed that in my patch-set.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog