Final (large) batch of Snowball Device Tree Enablement

The kernel now forces IRQs to be ONESHOT if no IRQ handler is passed.

Cc Alessandro Zummo <[email protected]>

Cc rtc-linux at googlegroups.com

Signed-off-by Lee Jones <[email protected]>

Quoted 5 lines · Lee Jones

The kernel now forces IRQs to be ONESHOT if no IRQ handler is passed.

Cc Alessandro Zummo<[email protected]>

Cc rtc-linux at googlegroups.com

Signed-off-by Lee Jones<[email protected]>

Isn't this a bug fix that should be applied to the stable kernels too?

Quoted 8 lines · Arnd Bergmann

Quoted 5 lines · Lee Jones

The kernel now forces IRQs to be ONESHOT if no IRQ handler is passed.

Cc Alessandro Zummo<[email protected]>

Cc rtc-linux at googlegroups.com

Signed-off-by Lee Jones<[email protected]>

Isn't this a bug fix that should be applied to the stable kernels too?

Possibly. Should we alert Greg (CC'ed - alerted)?

Quoted 11 lines · Lee Jones

Quoted 8 lines · Arnd Bergmann

Quoted 5 lines · Lee Jones

The kernel now forces IRQs to be ONESHOT if no IRQ handler is passed.

Cc Alessandro Zummo<[email protected]>

Cc rtc-linux at googlegroups.com

Signed-off-by Lee Jones<[email protected]>

Isn't this a bug fix that should be applied to the stable kernels too?

Possibly. Should we alert Greg (CC'ed - alerted)?

<formletter>

This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read Documentation/stable_kernel_rules.txt for how to do this properly.

</formletter>

I wasn't submitting, merely asking if it would be suitable for inclusion. :)

If it is, I'd be happy to _properly_ submit this and its ab8500-gpadc counterpart.

The kernel now forces IRQs to be ONESHOT if no IRQ handler is passed.

Cc stable at vger.kernel.org

Quoted 1 line · On Tue, 12 Jun 2012 09:57:37 +0100 Lee Jones<[email protected]> wrote:

The kernel now forces IRQs to be ONESHOT if no IRQ handler is passed.

What does this mean. The kernel crashes? The registration fails? A warning is emitted?

When fixing a bug, please fully describe the consequences of that bug.

Quoted 1 line

Cc stable at vger.kernel.org

Especially when suggesting a -stable backport.

Quoted 12 lines · Andrew Morton

Quoted 1 line · On Tue, 12 Jun 2012 09:57:37 +0100 Lee Jones<[email protected]> wrote:

The kernel now forces IRQs to be ONESHOT if no IRQ handler is passed.

What does this mean. The kernel crashes? The registration fails? A warning is emitted?

When fixing a bug, please fully describe the consequences of that bug.

Quoted 1 line

Cc [email protected]

Quoted 1 line

Especially when suggesting a -stable backport.

No problem. I'll try to remember that for next time.

I see that you've taken the patch into your tree however. So do you want me to update the commit log and resubmit, or are you going to 'let this one slide'?

Quoted 17 lines · On Thu, 14 Jun 2012 08:14:49 +0100 Lee Jones<[email protected]> wrote:

Quoted 5 lines · Andrew Morton

Quoted 1 line · On Tue, 12 Jun 2012 09:57:37 +0100 Lee Jones<[email protected]> wrote:

The kernel now forces IRQs to be ONESHOT if no IRQ handler is passed.

Quoted 8 lines

What does this mean. The kernel crashes? The registration fails? A warning is emitted?

When fixing a bug, please fully describe the consequences of that bug.

Quoted 1 line

Cc stable at vger.kernel.org

Especially when suggesting a -stable backport.

No problem. I'll try to remember that for next time.

Quoted 3 lines

I see that you've taken the patch into your tree however. So do you want me to update the commit log and resubmit, or are you going to 'let this one slide'?

Please read my emails - they're very nice! "What does this mean. The kernel crashes? The registration fails? A warning is emitted?". You answer that, I copy-n-paste into changelog and we're done.

__setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) {

	if (new->flags & IRQF_ONESHOT) {

} else if (new->handler == irq_default_primary_handler) {
/*
* The interrupt was requested with handler = NULL, so
* we use the default primary handler for it. But it
* does not have the oneshot flag set. In combination
* with level interrupts this is deadly, because the
* default primary handler just wakes the thread, then
* the irq lines is reenabled, but the device still
* has the level irq asserted. Rinse and repeat....
*
* While this works for edge type interrupts, we play
* it safe and reject unconditionally because we can't
* say for sure which type this interrupt really
* has. The type flags are unreliable as the
* underlying chip implementation can override them.
*/
pr_err("Threaded irq requested with handler=NULL and !ONESHOT for irq %d\n", irq); ret = -EINVAL;
goto out_mask;
}

Without this patch, if Device Tree is enabled the AB8500 RTC wouldn't get probed at all, as there is no reference to it from platform code. This patch ensures the driver is probed during normal DT start-up.

Cc Alessandro Zummo <[email protected]>

Cc rtc-linux at googlegroups.com

Signed-off-by Lee Jones <[email protected]>

Without this patch, if Device Tree is enabled the AB8500 RTC wouldn't get probed at all, as there is no reference to it from platform code. This patch ensures the driver is probed during normal DT start-up.

Cc Alessandro Zummo <[email protected]>

Cc rtc-linux at googlegroups.com

Signed-off-by Lee Jones <[email protected]>

---
  drivers/rtc/rtc-ab8500.c |    8 +++++++-
  1 file changed, 7 insertions(+), 1 deletion(-)

The kernel now forces IRQs to be ONESHOT if no IRQ handler is passed.

Cc Samuel Ortiz <[email protected]>

Signed-off-by Lee Jones <[email protected]>

During ab8500-core clean-up the Kconfig entry for AB8500_I2C_CORE was left remnant. This patch simply removes it.

Cc Samuel Ortiz <[email protected]>

Signed-off-by Lee Jones <[email protected]>

This patch adds the correct compatible string for use during Device Tree population. Without it the DB8500 PRCMU will not be probed.

Cc Samuel Ortiz <[email protected]>

Signed-off-by Lee Jones <[email protected]>

This patch adds the correct compatible string for use during Device Tree population. Without it the DB8500 PRCMU will not be probed.

Cc Samuel Ortiz <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/mfd/db8500-prcmu.c |    5 +++++
 1 file changed, 5 insertions(+)

Now the AB8500 has its own IRQ domain

it needs to be initialised earlier
in the boot sequence. As the AB8500 relies on the DB8500 PRCMU we need to reflect this change for the PRCMU driver too.

Quoted 1 line · Lee Jones

Now the AB8500 has its own IRQ domain

But that does not appear until patch 8 in this series? Are the patches in the wrong order? Or does this need rewording?

Quoted 3 lines

it needs to be initialised earlier
in the boot sequence. As the AB8500 relies on the DB8500 PRCMU we need to reflect this change for the PRCMU driver too.

Hm what shall we do when we run out of initlevels? I think this was the kind of thing that deferred probe should solve. Usually changing this kind of thing has side effects so I'm a bit hesitant.

Quoted 1 line · Lee Jones

Now the AB8500 has its own IRQ domain

But that does not appear until patch 8 in this series? Are the patches in the wrong order? Or does this need rewording?

Quoted 3 lines

it needs to be initialised earlier
in the boot sequence. As the AB8500 relies on the DB8500 PRCMU we need to reflect this change for the PRCMU driver too.

Hm what shall we do when we run out of initlevels? I think this was the kind of thing that deferred probe should solve. Usually changing this kind of thing has side effects so I'm a bit hesitant.

Quoted 3 lines · Linus Walleij

Hm what shall we do when we run out of initlevels? I think this was the kind of thing that deferred probe should solve. Usually changing this kind of thing has side effects so I'm a bit hesitant.

Ah yes, I remember now. The IRQ domain needs to be in place before the Device Tree is parsed by the Open Firmware subsystem. If it's not the error "no irq domain found" is triggered and the IRQs are never mapped.

I'd be happy to take a second opinion, but I believe this (and the other core_initcall patch) is required.

Quoted 9 lines · Lee Jones

Quoted 3 lines · Linus Walleij

Hm what shall we do when we run out of initlevels? I think this was the kind of thing that deferred probe should solve. Usually changing this kind of thing has side effects so I'm a bit hesitant.

Ah yes, I remember now. The IRQ domain needs to be in place before the Device Tree is parsed by the Open Firmware subsystem. If it's not the error "no irq domain found" is triggered and the IRQs are never mapped.

Quoted 2 lines

I'd be happy to take a second opinion, but I believe this (and the other core_initcall patch) is required.

It's still just a hack. The real solution that we discussed last time it came up is to defer the translation of irq numbers until device driver probe time, and bail out with -EPROBE_DEFER if you try to use a device whose interrupt is not available yet.

/*
* FIXME: Should we set up the GPIO domain here?
*
* The problem is that we cannot put the interrupt resources into the platform

* device until the irqdomain has been added. Right now, we set the GIC interrupt
* domain from init_irq(), then load the gpio driver from
* core_initcall(nmk_gpio_init) and add the platform devices from
* arch_initcall(customize_machine).
*
* This feels fragile because it depends on the gpio device getting probed
* _before_ any device uses the gpio interrupts.
*/

But your suggestion above involves some heavy refactoring of how Device Tree works. Meanwhile the ab8500 will not work as an IRQ controller until we either complete the work or use the same solution as we did for the Nomadik GPIO controller. I think for now at least we should continue to work on enablement and leave the 'nice to have's until last. We are doing nothing new here, just bringing the ab8500 IRQ controller into line with the Nomadik one.

Quoted 7 lines · Lee Jones

But your suggestion above involves some heavy refactoring of how Device Tree works. Meanwhile the ab8500 will not work as an IRQ controller until we either complete the work or use the same solution as we did for the Nomadik GPIO controller. I think for now at least we should continue to work on enablement and leave the 'nice to have's until last. We are doing nothing new here, just bringing the ab8500 IRQ controller into line with the Nomadik one.

Right, I didn't say we should not take your patch, but we should be aware that it's not the solution we want to use in the long run.

Quoted 7 lines · Lee Jones

But your suggestion above involves some heavy refactoring of how Device Tree works. Meanwhile the ab8500 will not work as an IRQ controller until we either complete the work or use the same solution as we did for the Nomadik GPIO controller. I think for now at least we should continue to work on enablement and leave the 'nice to have's until last. We are doing nothing new here, just bringing the ab8500 IRQ controller into line with the Nomadik one.

Right, I didn't say we should not take your patch, but we should be aware that it's not the solution we want to use in the long run.

Now the AB8500 has its own IRQ domain it needs to be initialised earlier in the boot sequence. As the AB8500 relies on the DB8500 PRCMU we need to reflect this change for the PRCMU driver too.

Cc Samuel Ortiz <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/mfd/db8500-prcmu.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

The AB8500 is soon to have its own IRQ domain. For this to be useful the driver needs to be initialised earlier in the boot sequence. Here we move initialisation forward from arch_initcall to core_initcall time.

Quoted 3 lines · Lee Jones

The AB8500 is soon to have its own IRQ domain. For this to be useful the driver needs to be initialised earlier in the boot sequence. Here we move initialisation forward from arch_initcall to core_initcall time.

I don't understand why using IRQ domain makes it necessary to initialize the driver earlier in the init process. Please explain to me, I need tutoring here.

Quoted 3 lines · Lee Jones

The AB8500 is soon to have its own IRQ domain. For this to be useful the driver needs to be initialised earlier in the boot sequence. Here we move initialisation forward from arch_initcall to core_initcall time.

I don't understand why using IRQ domain makes it necessary to initialize the driver earlier in the init process. Please explain to me, I need tutoring here.

The AB8500 is soon to have its own IRQ domain. For this to be useful the driver needs to be initialised earlier in the boot sequence. Here we move initialisation forward from arch_initcall to core_initcall time.

Cc Samuel Ortiz <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/mfd/ab8500-core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

As the AB8500 is an IRQ controller in its own right, here we provide the AB8500 driver with IRQ domain support. This is required if we wish to reference any of its IRQs from a platform's Device Tree.

drivers/mfd/ab8500-core.c +0 -0

+ *
+ * @ab8500: ab8500_irq controller to operate on.
+ * @irq: index of the interrupt requested in the chip IRQs
+ */
+int ab8500_irq_get_virq(struct ab8500 *ab8500, int irq) ?{
- ? ? ? int base = ab8500->irq_base;
- ? ? ? int irq;
- ? ? ? int num_irqs;
+ ? ? ? if (!ab8500)
+ ? ? ? ? ? ? ? return -EINVAL;

- ? ? ? if (is_ab9540(ab8500))
- ? ? ? ? ? ? ? num_irqs = AB9540_NR_IRQS;
- ? ? ? else if (is_ab8505(ab8500))
- ? ? ? ? ? ? ? num_irqs = AB8505_NR_IRQS;
- ? ? ? else
- ? ? ? ? ? ? ? num_irqs = AB8500_NR_IRQS;
+ ? ? ? return irq_create_mapping(ab8500->domain, irq);

drivers/mfd/ab8500-core.c +2 -0

+}
+EXPORT_SYMBOL_GPL(ab8500_irq_get_virq);

include/linux/mfd/abx500/ab8500.h +0 -0

index 91dd3ef..48f126c 100644
@@ -227,6 +227,7 @@ enum ab8500_version {

?* @irq_lock: genirq bus lock
?* @transfer_ongoing: 0 if no transfer ongoing ?* @irq: irq line
+ * @irq_domain: irq domain
?* @version: chip version id (e.g. ab8500 or ab9540) ?* @chip_id: chip revision id
?* @write: register write

Quoted 3 lines · Lee Jones

As the AB8500 is an IRQ controller in its own right, here we provide the AB8500 driver with IRQ domain support. This is required if we wish to reference any of its IRQs from a platform's Device Tree.

OK..

Quoted 1 line

drivers/mfd/ab8500-core.c +0 -0

(...)

drivers/mfd/ab8500-core.c static irqreturn_t ab8500_irq(int irq, void *dev) · line 517

Quoted 5 lines

drivers/mfd/ab8500-core.c +4 -1

-static int ab8500_irq_init(struct ab8500 *ab8500)
+/**
+ * ab8500_irq_get_virq(): Map an interrupt on a chip to a virtual IRQ
+ *
+ * Useful for drivers to request their own IRQs.

Check style against Documentation/kernel-doc-nano-HOWTO.txt verbos explanation follows argument documentation.

Quoted 21 lines

+ *
+ * @ab8500: ab8500_irq controller to operate on.
+ * @irq: index of the interrupt requested in the chip IRQs
+ */

drivers/mfd/ab8500-core.c +6 -9

+int ab8500_irq_get_virq(struct ab8500 *ab8500, int irq)
  {
-       int base = ab8500->irq_base;
-       int irq;
-       int num_irqs;
+       if (!ab8500)
+               return -EINVAL;
 
-       if (is_ab9540(ab8500))
-               num_irqs = AB9540_NR_IRQS;
-       else if (is_ab8505(ab8500))
-               num_irqs = AB8505_NR_IRQS;
-       else
-               num_irqs = AB8500_NR_IRQS;
+       return irq_create_mapping(ab8500->domain, irq);
+}
+EXPORT_SYMBOL_GPL(ab8500_irq_get_virq);

(...)

Quoted 2 lines

drivers/mfd/ab8500-core.c +0 -0

@@ -1233,11 +1256,9 @@ static int __devinit ab8500_probe(struct platform_device *pdev)
 

drivers/mfd/ab8500-core.c static int __devinit ab8500_probe(struct platform_device *pdev)

Quoted 4 lines

drivers/mfd/ab8500-core.c +0 -2

        if (plat)
                ab8500->irq_base = plat->irq_base;
-       else if (np)
-               ret = of_property_read_u32(np, "stericsson,irq-base",&ab8500->irq_base);

So if we're not using the irq base thing anymore, should you also delete it from the binding document too? (If there is no binding doc something is wrong and you need to create it I guess...)

Quoted 13 lines

include/linux/mfd/abx500/ab8500.h +1 -0

index 91dd3ef..48f126c 100644
@@ -227,6 +227,7 @@ enum ab8500_version {
  * @irq_lock: genirq bus lock
  * @transfer_ongoing: 0 if no transfer ongoing
  * @irq: irq line
+ * @irq_domain: irq domain
  * @version: chip version id (e.g. ab8500 or ab9540)
  * @chip_id: chip revision id
  * @write: register write

@@ -247,6 +248,7 @@ struct ab8500 {

include/linux/mfd/abx500/ab8500.h struct ab8500 { · line 251

Quoted 4 lines

include/linux/mfd/abx500/ab8500.h +1 -0

        atomic_t        transfer_ongoing;
        int             irq_base;
        int             irq;
+       struct irq_domain  *domain;

Don't you need to forward-declare struct irq_domain? I think you're just lucky to have it compiling... (Something else included<linux/irqdomain.h> on the way here.)

drivers/mfd/ab8500-core.c static void ab8500_irq_remove(struct ab8500 *ab8500) · line 576

Quoted 9 lines · Lee Jones

drivers/mfd/ab8500-core.c +9 -0

+	if (np) {
+		ab8500->domain = irq_domain_add_linear(
+			np, num_irqs,&ab8500_irq_ops, ab8500);
+	}
+	else {
+		ab8500->domain = irq_domain_add_legacy(
+			NULL, num_irqs, ab8500->irq_base,
+			0,&ab8500_irq_ops, ab8500);
+	}

This is odd, why are you forcing non-DT systems to use a legacy mapping? The more usual approach is to use a legacy mapping if and only if a base for the legacy range has been provided, otherwise the system will needlessly fail to initialise the device...

drivers/mfd/ab8500-core.c static int __devinit ab8500_probe(struct platform_device *pdev) · line 1261

Quoted 2 lines

drivers/mfd/ab8500-core.c +2 -0

+	if (!(ab8500->irq_base || np)) {
+		dev_info(&pdev->dev, "couldn't find irq-base and not doing DT boot\n");

...like this. See regmap_irq for an example.

Do this look better to you Mark?

Review

Quoted 1 line

(quoted diff) +1 -0

+	int offset = data->irq;

Are you sure this is right? I thought irq_data->irq was the Linux IRQ number, irq_data->hwirq was the interrupt number inside the domain.

Quoted 4 lines · Russell King - ARM Linux

Review

Quoted 1 line

(quoted diff) +1 -0

+	int offset = data->irq;

Are you sure this is right? I thought irq_data->irq was the Linux IRQ number, irq_data->hwirq was the interrupt number inside the domain.

Yes, that's correct. I'm surprised the code runs without crashing...

Quoted 2 lines · Mark Brown

Yes, that's correct. ?I'm surprised the code runs without crashing...

Oddly the code ran perfectly.

Quoted 5 lines · Lee Jones

Quoted 2 lines · Mark Brown

Yes, that's correct. I'm surprised the code runs without crashing...

Oddly the code ran perfectly.

So how did you test it?

The only way to generate an IRQ on the AB8500 with the mainline code is to push the power button, that should generate an event from the driver in drivers/input/misc/ab8500-ponkey.c that you can listen to by doing cat on the proper /dev/input/eventN file.

Please test this scenario before and after the patch and you have some indication that the IRQdomain is working.

CPU0 CPU1

  6:          0          2    ab8500  ab8500-ponkey-dbf
  7:          0          2    ab8500  ab8500-ponkey-dbr

Review static void ab8500_irq_sync_unlock(struct irq_data *data) · line 365

Quoted 6 lines · Lee Jones

(quoted diff) +1 -1

@@ -361,7 +362,7 @@ static void ab8500_irq_sync_unlock(struct irq_data *data)
 ?static void ab8500_irq_mask(struct irq_data *data)
 ?{
 ? ? ? struct ab8500 *ab8500 = irq_data_get_irq_chip_data(data);
- ? ? int offset = data->irq - ab8500->irq_base;
+ ? ? int offset = data->irq;

Are you sure this is right? ?I thought irq_data->irq was the Linux IRQ number, irq_data->hwirq was the interrupt number inside the domain.

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 at vger.kernel.org

Acked-by Linus Walleij <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/i2c/busses/i2c-nomadik.c |   40 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

drivers/i2c/busses/i2c-nomadik.c +1 -0

index a92440d..1ffdf67 100644
@@ -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
 };

drivers/i2c/busses/i2c-nomadik.c +27 -1

+       .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");

Quoted 29 lines · 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 at vger.kernel.org

Acked-by Linus Walleij<[email protected]>

Signed-off-by Lee Jones<[email protected]>

---
  drivers/i2c/busses/i2c-nomadik.c |   40 +++++++++++++++++++++++++++++++++++++-
  1 file changed, 39 insertions(+), 1 deletion(-)

drivers/i2c/busses/i2c-nomadik.c +1 -0

index a92440d..1ffdf67 100644
@@ -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
  };

drivers/i2c/busses/i2c-nomadik.c static const struct i2c_algorithm nmk_i2c_algo = { · line 911

Quoted 10 lines

drivers/i2c/busses/i2c-nomadik.c +9 -0

 
+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.

From: Lee Jones <[email protected]>
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 at vger.kernel.org

Acked-by Linus Walleij <[email protected]>

Signed-off-by Lee Jones <[email protected]>

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.

(quoted diff) +8 -0

+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,

Quoted 4 lines · Lee Jones

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.

(...)

Quoted 8 lines

drivers/i2c/busses/i2c-nomadik.c +8 -0

+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,

drivers/i2c/busses/i2c-nomadik.c static const struct i2c_algorithm nmk_i2c_algo = { · line 920

Quoted 10 lines

drivers/i2c/busses/i2c-nomadik.c +10 -0

+       /* 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 ...)

Quoted 7 lines · Linus Walleij

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. :)

Quoted 16 lines · Lee Jones

Quoted 7 lines · Linus Walleij

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.

Quoted 3 lines

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.

Quoted 3 lines · Lee Jones

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.

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.

Quoted 5 lines · Mark Brown

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.

Quoted 3 lines · Lee Jones

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.

Quoted 5 lines · Mark Brown

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.

Quoted 9 lines · Lee Jones

Quoted 5 lines · Mark Brown

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)...

Quoted 3 lines

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.

Quoted 11 lines · Mark Brown

Quoted 3 lines · Lee Jones

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.

Quoted 13 lines · Lee Jones

Quoted 11 lines · Mark Brown

Quoted 3 lines · Lee Jones

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.

Quoted 5 lines

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...

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 at vger.kernel.org

Signed-off-by Lee Jones <[email protected]>

(quoted diff) +30 -0

+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);

? ? ? ?/* 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);

From: Lee Jones <[email protected]>
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 at vger.kernel.org

Signed-off-by Lee Jones <[email protected]>

---
 drivers/i2c/busses/i2c-nomadik.c |   82 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 80 insertions(+), 2 deletions(-)

drivers/i2c/busses/i2c-nomadik.c +2 -0

index a92440d..58e8114 100644
@@ -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 = {

drivers/i2c/busses/i2c-nomadik.c +29 -0

+};
+
+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;
+       }
+

drivers/i2c/busses/i2c-nomadik.c +12 -2

+
+       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;

drivers/i2c/busses/i2c-nomadik.c +7 -1

+
       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

[...]

Quoted 33 lines

From: Lee Jones<[email protected]>
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 at vger.kernel.org

Signed-off-by Lee Jones<[email protected]>

---
  drivers/i2c/busses/i2c-nomadik.c |   82 +++++++++++++++++++++++++++++++++++++-
  1 file changed, 80 insertions(+), 2 deletions(-)

drivers/i2c/busses/i2c-nomadik.c +1 -0

index a92440d..58e8114 100644
@@ -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
  };
 

drivers/i2c/busses/i2c-nomadik.c static const struct i2c_algorithm nmk_i2c_algo = { · line 913

Quoted 11 lines

drivers/i2c/busses/i2c-nomadik.c +11 -0

+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.

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

};

Quoted 34 lines

drivers/i2c/busses/i2c-nomadik.c +34 -0

+};
+
+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;
+       }

drivers/i2c/busses/i2c-nomadik.c static const struct i2c_algorithm nmk_i2c_algo = { · line 952

Quoted 5 lines

drivers/i2c/busses/i2c-nomadik.c +5 -0

+
+       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.

/*
* 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)
*/

Quoted 26 lines

drivers/i2c/busses/i2c-nomadik.c +17 -2

+
+       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);

drivers/i2c/busses/i2c-nomadik.c static const struct i2c_algorithm nmk_i2c_algo = {

Quoted 5 lines

drivers/i2c/busses/i2c-nomadik.c +5 -0

+       }
+
+       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.

Quoted 17 lines

drivers/i2c/busses/i2c-nomadik.c +6 -0

+
        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,

Quoted 8 lines

drivers/i2c/busses/i2c-nomadik.c +1 -1

                .pm =&nmk_i2c_pm,
+               .of_match_table = nmk_gpio_match,
        },
        .probe = nmk_i2c_probe,
        .remove = __devexit_p(nmk_i2c_remove),
--

1.7.9.5

Quoted 34 lines · Srinidhi Kasagar

[...]

Quoted 33 lines

From: Lee Jones<[email protected]>
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 at vger.kernel.org

Signed-off-by Lee Jones<[email protected]>

---
  drivers/i2c/busses/i2c-nomadik.c |   82 +++++++++++++++++++++++++++++++++++++-
  1 file changed, 80 insertions(+), 2 deletions(-)

drivers/i2c/busses/i2c-nomadik.c +1 -0

index a92440d..58e8114 100644
@@ -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
  };
 

Quoted 14 lines

drivers/i2c/busses/i2c-nomadik.c static const struct i2c_algorithm nmk_i2c_algo = { · line 913

Quoted 11 lines

drivers/i2c/busses/i2c-nomadik.c +11 -0

+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:

Quoted 7 lines

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".

Quoted 35 lines

Quoted 35 lines

drivers/i2c/busses/i2c-nomadik.c +34 -0

+};
+
+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;
+       }

Quoted 8 lines

drivers/i2c/busses/i2c-nomadik.c static const struct i2c_algorithm nmk_i2c_algo = { · line 952

Quoted 5 lines

drivers/i2c/busses/i2c-nomadik.c +5 -0

+
+       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:

Quoted 7 lines

/*
* 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?

Quoted 27 lines

Quoted 27 lines

drivers/i2c/busses/i2c-nomadik.c +17 -2

+
+       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);

Quoted 8 lines

drivers/i2c/busses/i2c-nomadik.c static const struct i2c_algorithm nmk_i2c_algo = {

Quoted 5 lines

drivers/i2c/busses/i2c-nomadik.c +5 -0

+       }
+
+       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?

Quoted 18 lines

Quoted 18 lines

drivers/i2c/busses/i2c-nomadik.c +6 -0

+
        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,

Quoted 8 lines

Quoted 8 lines

drivers/i2c/busses/i2c-nomadik.c +1 -1

                .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

Signature · 5 lines

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

Quoted 50 lines · Lee Jones

Quoted 48 lines · Srinidhi Kasagar

[...]

Quoted 34 lines

From: Lee Jones<[email protected]>
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 at vger.kernel.org

Signed-off-by Lee Jones<[email protected]>

---
   drivers/i2c/busses/i2c-nomadik.c |   82 +++++++++++++++++++++++++++++++++++++-
   1 file changed, 80 insertions(+), 2 deletions(-)

drivers/i2c/busses/i2c-nomadik.c +2 -0

index a92440d..58e8114 100644
@@ -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 = {

drivers/i2c/busses/i2c-nomadik.c static const struct i2c_algorithm nmk_i2c_algo = { · line 913

Quoted 10 lines

drivers/i2c/busses/i2c-nomadik.c +10 -0

+       /*
+        * 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.

Quoted 1 line

That's not how I read it:

But it is not readable. It confuses people.

Quoted 8 lines

Quoted 6 lines

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.

Quoted 43 lines

Quoted 42 lines

Quoted 29 lines

drivers/i2c/busses/i2c-nomadik.c +29 -0

+};
+
+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;
+       }
+

drivers/i2c/busses/i2c-nomadik.c static const struct i2c_algorithm nmk_i2c_algo = { · line 952

Quoted 10 lines

drivers/i2c/busses/i2c-nomadik.c +10 -0

+       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.

Quoted 1 line

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

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

Quoted 8 lines

Quoted 6 lines

/*
* 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.

Quoted 35 lines

Quoted 34 lines

Quoted 21 lines

drivers/i2c/busses/i2c-nomadik.c +12 -2

+
+       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;

drivers/i2c/busses/i2c-nomadik.c static const struct i2c_algorithm nmk_i2c_algo = {

Quoted 10 lines

drivers/i2c/busses/i2c-nomadik.c +10 -0

+                       }
+               }
+               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.

Quoted 1 line

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.

Quoted 25 lines

Quoted 25 lines

Quoted 25 lines

drivers/i2c/busses/i2c-nomadik.c +7 -1

+
         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

Quoted 36 lines · Srinidhi Kasagar

Quoted 35 lines · Lee Jones

Quoted 34 lines · Srinidhi Kasagar

[...]

Quoted 33 lines

From: Lee Jones<[email protected]>
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 at vger.kernel.org

Signed-off-by Lee Jones<[email protected]>

---
   drivers/i2c/busses/i2c-nomadik.c |   82 +++++++++++++++++++++++++++++++++++++-
   1 file changed, 80 insertions(+), 2 deletions(-)

drivers/i2c/busses/i2c-nomadik.c +1 -0

index a92440d..58e8114 100644
@@ -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
   };
 

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

drivers/i2c/busses/i2c-nomadik.c static const struct i2c_algorithm nmk_i2c_algo = { · line 908

Quoted 6 lines

drivers/i2c/busses/i2c-nomadik.c +6 -0

+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,

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

drivers/i2c/busses/i2c-nomadik.c static const struct i2c_algorithm nmk_i2c_algo = { · line 913

Quoted 5 lines

drivers/i2c/busses/i2c-nomadik.c +5 -0

+       .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.

Quoted 3 lines

Quoted 1 line

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.

Quoted 37 lines · Lee Jones

Quoted 36 lines · Srinidhi Kasagar

Quoted 35 lines · Lee Jones

Quoted 34 lines · Srinidhi Kasagar

[...]

Quoted 33 lines

From: Lee Jones<[email protected]>
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 at vger.kernel.org

Signed-off-by Lee Jones<[email protected]>

---
    drivers/i2c/busses/i2c-nomadik.c |   82 +++++++++++++++++++++++++++++++++++++-
    1 file changed, 80 insertions(+), 2 deletions(-)

drivers/i2c/busses/i2c-nomadik.c +1 -0

index a92440d..58e8114 100644
@@ -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
    };
 

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

drivers/i2c/busses/i2c-nomadik.c static const struct i2c_algorithm nmk_i2c_algo = { · line 908

Quoted 6 lines

drivers/i2c/busses/i2c-nomadik.c +6 -0

+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.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

drivers/i2c/busses/i2c-nomadik.c static const struct i2c_algorithm nmk_i2c_algo = { · line 913

Quoted 5 lines

drivers/i2c/busses/i2c-nomadik.c +5 -0

+       .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.

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

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

Quoted 18 lines · Srinidhi Kasagar

Quoted 17 lines · Lee Jones

Quoted 14 lines · Srinidhi Kasagar

Quoted 11 lines

+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.

Quoted 4 lines

Quoted 1 line

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.

Gah, what a thread...

Quoted 21 lines · Lee Jones

Quoted 20 lines · Srinidhi Kasagar

Quoted 18 lines · Lee Jones

Quoted 12 lines · Srinidhi Kasagar

Review

Quoted 11 lines

(quoted diff) +11 -0

+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,

Quoted 2 lines

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:

Quoted 5 lines

Quoted 1 line

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...)

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

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.

Quoted 24 lines · Linus Walleij

Gah, what a thread...

Quoted 21 lines · Lee Jones

Quoted 20 lines · Srinidhi Kasagar

Quoted 18 lines · Lee Jones

Quoted 12 lines · Srinidhi Kasagar

Review

Quoted 11 lines

(quoted diff) +11 -0

+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,

Quoted 2 lines

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:

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

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.

Quoted 1 line

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?

Quoted 39 lines · Lee Jones

Quoted 24 lines · Linus Walleij

Gah, what a thread...

Quoted 21 lines · Lee Jones

Quoted 20 lines · Srinidhi Kasagar

Quoted 18 lines · Lee Jones

Quoted 12 lines · Srinidhi Kasagar

Review

Quoted 11 lines

(quoted diff) +11 -0

+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,

Quoted 2 lines

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:

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

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.

Quoted 7 lines

Quoted 1 line

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..

From: Lee Jones <[email protected]>
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 <[email protected]>

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

Review

Quoted 6 lines · Lee Jones

(quoted diff) +6 -0

+                       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.

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

Signed-off-by Lee Jones <[email protected]>