On 08/04/2011 10:45 AM, Rob Herring wrote:

Ben,

On 08/04/2011 04:12 AM, Ben Dooks wrote:

quoted

On Wed, Aug 03, 2011 at 03:04:23PM -0500, Rob Herring wrote:

quoted

From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>

Add of_match_table and DT style i2c registration to designware i2c
driver.

Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
Cc: Ben Dooks <redacted>
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 Documentation/devicetree/bindings/i2c/dw-i2c.txt |   23 ++++++++++++++++++++++
 drivers/i2c/busses/i2c-designware.c              |   13 ++++++++++++
 2 files changed, 36 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/dw-i2c.txt

diff --git a/Documentation/devicetree/bindings/i2c/dw-i2c.txt b/Documentation/devicetree/bindings/i2c/dw-i2c.txt
new file mode 100644
index 0000000..cbcb404
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/dw-i2c.txt

@@ -0,0 +1,23 @@
+* Synopsys DesignWare I2C
+
+Required properties :
+
+ - compatible : should be "snps,designware-i2c"
+ - reg : Offset and length of the register set for the device
+ - interrupts : <IRQ> where IRQ is the interrupt number.
+
+Recommended properties :
+
+ - clock-frequency : desired I2C bus clock frequency in Hz.
+
+Example :
+
+	i2c@f0000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "snps,designware-i2c";
+		reg = <0xf0000 0x1000>;
+		interrupts = <11>;
+		clock-frequency = <400000>;
+	};
+

looks good to me.

quoted

diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
index b7a51c4..2911a49 100644
--- a/drivers/i2c/busses/i2c-designware.c
+++ b/drivers/i2c/busses/i2c-designware.c

@@ -37,6 +37,7 @@
 #include <linux/platform_device.h>
 #include <linux/io.h>
 #include <linux/slab.h>
+#include <linux/of_i2c.h>
 
 /*
  * Registers offset

@@ -770,12 +771,17 @@ static int __devinit dw_i2c_probe(struct platform_device *pdev)
 	adap->algo = &i2c_dw_algo;
 	adap->dev.parent = &pdev->dev;
 
+#ifdef CONFIG_OF
+	r = i2c_add_adapter(adap);
+#else
 	adap->nr = pdev->id;
 	r = i2c_add_numbered_adapter(adap);
+#endif

I would say that doing the #ifdef CONFIG_OF is dangerous here when we
are in a mixed OF/platform enviromnent as we're depending on compile
time selection.

I'm also wondering whether we have an of helper macro which takes
a pdev and gives you an adapter number either given on pdev->id or
-1 for the case when we're using the OF bindings.

It might be worth talking to Grant about setting pdev->id to -1 if we
are using an OF device.

As Grant said, that's already done and this hunk is not needed.

quoted

quoted

 	if (r) {
 		dev_err(&pdev->dev, "failure adding adapter\n");
 		goto err_free_irq;
 	}
+	of_i2c_register_devices(adap);

If we did that, we could add a of_i2c_register_adapter() call which
would take the platform device and then do the of_i2c_register_devices()
and do these steps.

Better yet, how about putting of_i2c_register_devices into
i2c_register_adapter? Everywhere that calls of_i2c_register_devices is
preceded by a call to i2c_add_numbered_adapter or i2c_add_adapter. It
seems logical to put it with i2c_scan_static_board_info. I'll prepare a
patch to add that and remove all the other callers unless you think
that's a bad idea.

Nevermind. That would be undoing this commit:

    of/i2c: Fix module load order issue caused by of_i2c.c

    Commit 959e85f7, "i2c: add OF-style registration and binding" caused a
    module dependency loop where of_i2c.c calls functions in i2c-core, and
    i2c-core calls of_i2c_register_devices() in of_i2c.  This means that
    when i2c support is built as a module when CONFIG_OF is set, then
    neither i2c_core nor of_i2c are able to be loaded.

    This patch fixes the problem by moving the of_i2c_register_devices()
    calls back into the device drivers.  Device drivers already
    specifically request the core code to parse the device tree for
    devices anyway by setting the of_node pointer, so it isn't a big
    deal to also call the registration function.  The drivers just become
    slightly more verbose.

    Signed-off-by: Grant Likely [off-list ref]
    Signed-off-by: Jean Delvare [off-list ref]

Rob

quoted

quoted

 	return 0;
 

@@ -819,6 +825,12 @@ static int __devexit dw_i2c_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id dw_i2c_of_match[] = {
+	{ .compatible = "snps,designware-i2c", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
+
 /* work with hotplug and coldplug */
 MODULE_ALIAS("platform:i2c_designware");
 

@@ -827,6 +839,7 @@ static struct platform_driver dw_i2c_driver = {
 	.driver		= {
 		.name	= "i2c_designware",
 		.owner	= THIS_MODULE,
+		.of_match_table = dw_i2c_of_match,

If my patch for of_match_ptr() is accepted, it will be needed here
otherwise you will need to do something about remvoing the of table
above if not config of.

There's really not much harm with having the table. If you match the
device in the non-DT way, the table is not used. Drivers should never
directly access the table either, but use the helpers to get their data.

Rob