Re: [RFC PATCH 5/5] OMAP: I2C: Convert I2C driver to use device tree

On Wed, Jul 06, 2011 at 01:08:03PM -0600, Grant Likely wrote:

On Thu, Jun 30, 2011 at 03:07:27PM +0500, G, Manjunath Kondaiah wrote:

quoted

The OMAP I2C driver is modified to use platform_device data from
device tree data structures.

Signed-off-by: G, Manjunath Kondaiah <redacted>

Mostly looks good, but a few things that need to be fixed.  You can
probably even get this change merged for v3.1

Thanks. I will fix the review comments and we can have these changes with
board-omapx-dt.c file so that dt and not dt builds can co exist.

For complete functionality with dt build, omap hwmod needs to be handled
through DT framework. I am waiting for comments from omap hwmod maintainers.

quoted

---
 drivers/i2c/busses/i2c-omap.c |   30 +++++++++++++++++++++++++++++-
 1 files changed, 29 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index ae1545b..d4ccc52 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c

@@ -38,9 +38,13 @@
 #include <linux/clk.h>
 #include <linux/io.h>
 #include <linux/of_i2c.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
 #include <linux/slab.h>
 #include <linux/i2c-omap.h>
 #include <linux/pm_runtime.h>
+#include <plat/i2c.h>
 
 /* I2C controller revisions */
 #define OMAP_I2C_REV_2			0x20

@@ -972,6 +976,7 @@ static const struct i2c_algorithm omap_i2c_algo = {
 	.functionality	= omap_i2c_func,
 };
 
+static const struct of_device_id omap_i2c_of_match[];
 static int __devinit
 omap_i2c_probe(struct platform_device *pdev)
 {

@@ -979,10 +984,15 @@ omap_i2c_probe(struct platform_device *pdev)
 	struct i2c_adapter	*adap;
 	struct resource		*mem, *irq, *ioarea;
 	struct omap_i2c_bus_platform_data *pdata = pdev->dev.platform_data;
+	const struct of_device_id *match;
 	irq_handler_t isr;
 	int r;
 	u32 speed = 0;
 
+	match = of_match_device(omap_i2c_of_match, &pdev->dev);
+	if (!match)
+		dev_err(&pdev->dev, "DT: i2c device match not found.......\n");
+

This isn't an error.  It just mean that the device wasn't registered
from the device tree, and it needs to get its configuration from
pdata.

In fact, you don't even need this block since the driver never uses
the match entry returned by of_match_device()

ok. I will remove this.

quoted

 	/* NOTE: driver uses the static register mapping */
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!mem) {

@@ -1007,10 +1017,19 @@ omap_i2c_probe(struct platform_device *pdev)
 		r = -ENOMEM;
 		goto err_release_region;
 	}
-

Don't drop the empty line.

quoted

+	/* I2C device without DT might use this */

No need for the comment.

ok

quoted

 	if (pdata != NULL) {
 		speed = pdata->clkrate;
 		dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
+	} else if (pdev->dev.of_node) {
+		const unsigned int *prop;
+		prop = of_get_property(pdev->dev.of_node,
+				"clock-frequency", NULL);
+		if (prop)
+			speed = be32_to_cpup(prop);
+		else
+			speed = 100;

There is a new function that makes this easier.  Do this instead:

	speed = 100;
	if (of_property_read_u32(pdev->dev.of_node, "clock-frequency", &prop)
		speed = prop / 1000000; /* speed in Hz, this might be wrong */

These patches got merged after posting this series. I will take care of it in
the next version.

BTW, I have posted seperate patch to use above API for code optimization.

quoted

+		dev->set_mpu_wkup_lat = NULL;

dev is kzalloced, which means set_mpu_wkup_lat is already NULL.

quoted

 	} else {
 		speed = 100;	/* Default speed */
 		dev->set_mpu_wkup_lat = NULL;

@@ -1045,6 +1064,7 @@ omap_i2c_probe(struct platform_device *pdev)
 
 	dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;
 
+

Drop the unrelated whitespace change.

quoted

 	if (dev->rev <= OMAP_I2C_REV_ON_3430)
 		dev->errata |= I2C_OMAP3_1P153;
 

@@ -1073,6 +1093,7 @@ omap_i2c_probe(struct platform_device *pdev)
 				       (1000 * speed / 8);
 	}
 
+

Ditto.

quoted

 	/* reset ASAP, clearing any IRQs */
 	omap_i2c_init(dev);
 

@@ -1162,6 +1183,12 @@ static int omap_i2c_resume(struct device *dev)
 	return 0;
 }
 
+static const struct of_device_id omap_i2c_of_match[] = {
+	{.compatible = "ti,omap_i2c", },
+	{},
+}
+MODULE_DEVICE_TABLE(of, omap_i2c_of_match);
+

This needs to be protected with #ifdef CONFIG_OF/#else/#endif.  Don't
break non-dt builds.

ok. Thanks.

-Manjunath