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:
> >
> > The OMAP I2C driver is modified to use platform_device data from
> > device tree data structures.
> >
> > Signed-off-by: G, Manjunath Kondaiah <manjugk@xxxxxx>
>
> 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.
>
> > ---
> > 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.
>
> > /* 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.
>
> > + /* I2C device without DT might use this */
>
> No need for the comment.
ok
>
> > 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.
>
> > + dev->set_mpu_wkup_lat = NULL;
>
> dev is kzalloced, which means set_mpu_wkup_lat is already NULL.
>
> > } 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.
>
> > 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.
>
> > /* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
