[RFC PATCH 1/5] OMAP3:I2C: Add device tree nodes for beagle board

[RFC PATCH 1/5] OMAP3:I2C: Add device tree nodes for beagle board

[Tony Lindgren]

Hi,

Few comments on the .dts data layout below.

* G, Manjunath Kondaiah <manjugk@ti.com> [110630 02:44]:
> --- a/arch/arm/boot/dts/omap3-beagle-nunchuck.dts
> +++ b/arch/arm/boot/dts/omap3-beagle-nunchuck.dts
> @@ -2,11 +2,6 @@
>  
>  / {
>  	i2c at 48072000 {
> -		compatible = "ti,omap3-i2c";
> -		reg = <0x48072000 0x80>;
> -		#address-cells = <1>;
> -		#size-cells = <0>;
> -
>  		eeprom at 50 {
>  			compatible = "at,at24c01";
>  			reg = < 0x50 >;

The board .dts file should include the omap3 SoC .dts file.

The omap3 SoC .dts file should have the devices mapped to L3 and L4
busses, and the then i2c at 1 would just contain the bus offset.

Then the i2c at 1 entry would be repeated in the board specific
.dts and tell that the i2c at 1 is enabled.

> --- a/arch/arm/boot/dts/omap3-beagle.dts
> +++ b/arch/arm/boot/dts/omap3-beagle.dts
> @@ -4,4 +4,46 @@
>  / {
>  	model = "TI OMAP3 BeagleBoard";
>  	compatible = "ti,omap3-beagle";
> +	interrupt-parent = <&gic>;
> +
> +	gic: interrupt-controller at 48241000 {
> +		compatible = "ti,omap-gic", "arm,gic";
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +		reg = <0x48200000 0x1000>;
> +	};

There's no GIC on omap3, that's only on Cortex A9 systems.

Regards,

Tony

[RFC PATCH 1/5] OMAP3:I2C: Add device tree nodes for beagle board

[Grant Likely]

On Thu, Jun 30, 2011 at 07:27:02AM -0700, Tony Lindgren wrote:
> Hi,
> 
> Few comments on the .dts data layout below.
> 
> * G, Manjunath Kondaiah <manjugk@ti.com> [110630 02:44]:
> > --- a/arch/arm/boot/dts/omap3-beagle-nunchuck.dts
> > +++ b/arch/arm/boot/dts/omap3-beagle-nunchuck.dts
> > @@ -2,11 +2,6 @@
> >  
> >  / {
> >  	i2c at 48072000 {
> > -		compatible = "ti,omap3-i2c";
> > -		reg = <0x48072000 0x80>;
> > -		#address-cells = <1>;
> > -		#size-cells = <0>;
> > -
> >  		eeprom at 50 {
> >  			compatible = "at,at24c01";
> >  			reg = < 0x50 >;
> 
> The board .dts file should include the omap3 SoC .dts file.
> 
> The omap3 SoC .dts file should have the devices mapped to L3 and L4
> busses, and the then i2c at 1 would just contain the bus offset.
> 
> Then the i2c at 1 entry would be repeated in the board specific
> .dts and tell that the i2c at 1 is enabled.

yup.

[RFC PATCH 1/5] OMAP3:I2C: Add device tree nodes for beagle board

[Grant Likely]

On Thu, Jun 30, 2011 at 03:07:23PM +0500, G, Manjunath Kondaiah wrote:
> 
> Add I2C and it's child device nodes for beagle board.
> The I2C1 controller child devices are not populated and it
> should be handled along with OMAP clock changes.
> 
> Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com>
> ---
>  arch/arm/boot/dts/omap3-beagle-nunchuck.dts |    5 ---
>  arch/arm/boot/dts/omap3-beagle.dts          |   42 +++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/omap3-beagle-nunchuck.dts b/arch/arm/boot/dts/omap3-beagle-nunchuck.dts
> index 2607be5..479be11 100644
> --- a/arch/arm/boot/dts/omap3-beagle-nunchuck.dts
> +++ b/arch/arm/boot/dts/omap3-beagle-nunchuck.dts

This hunk is of course only for my tree since I'm the only one who
actually has this modified beagleboard.  :-)

> @@ -2,11 +2,6 @@
>  
>  / {
>  	i2c at 48072000 {
> -		compatible = "ti,omap3-i2c";
> -		reg = <0x48072000 0x80>;
> -		#address-cells = <1>;
> -		#size-cells = <0>;
> -
>  		eeprom at 50 {
>  			compatible = "at,at24c01";
>  			reg = < 0x50 >;
> diff --git a/arch/arm/boot/dts/omap3-beagle.dts b/arch/arm/boot/dts/omap3-beagle.dts
> index 4439466..491ee2b 100644
> --- a/arch/arm/boot/dts/omap3-beagle.dts
> +++ b/arch/arm/boot/dts/omap3-beagle.dts
> @@ -4,4 +4,46 @@
>  / {
>  	model = "TI OMAP3 BeagleBoard";
>  	compatible = "ti,omap3-beagle";
> +	interrupt-parent = <&gic>;
> +
> +	gic: interrupt-controller at 48241000 {
> +		compatible = "ti,omap-gic", "arm,gic";
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +		reg = <0x48200000 0x1000>;
> +	};
> +
> +	i2c at 48070000 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		compatible = "ti,omap_i2c";

ti,omap3-i2c

Use '-' not '_'. and the specific silicon implementation should be
specified (omap3 vs. omap).

> +		reg = <0x48070000 0x100>;
> +		interrupts = < 88 >;
> +		interrupt-parent = <&gic>;

interrupt-parent isn't needed because it is inherited from the root node.

> +		clock-frequency = <2600>;
> +		status = "disabled";

Drop 'status' when you move this node definition to
arch/arm/boot/dts/omap3.dtsi.  Board overlay files that include the
omap3.dtsi should explicitly disable any devices that it does not use
(which is opposite to what tegra currently does, but I'm going to
change that).

> +	};
> +
> +	i2c at 48072000 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		compatible = "ti,omap_i2c";
> +		reg = <0x48072000 0x100>;
> +		interrupts = < 89 >;
> +		interrupt-parent = <&gic>;
> +		clock-frequency = <400>;
> +		status = "ok";

Okay is spelled 'okay'.  :-)  The kernel does accept 'ok', but I
discourage its usage... just because I'm a nitpick about stuff like
that.

Actually, if the device is enabled, the status property can be dropped
entirely because the default behaviour is to enable.

> +	};
> +
> +	i2c at 48060000 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		compatible = "ti,omap_i2c";
> +		reg = <0x48060000 0x100>;
> +		interrupts = < 93 >;
> +		interrupt-parent = <&gic>;
> +		clock-frequency = <100>;
> +		status = "ok";
> +	};
> +
>  };
> -- 
> 1.7.4.1
> 

[RFC PATCH 2/5] OMAP4:I2C: Add device tree nodes for panda board

[Grant Likely]

On Thu, Jun 30, 2011 at 03:07:24PM +0500, G, Manjunath Kondaiah wrote:
> 
> Add I2C and it's child device nodes for beagle board.
> The I2C1 controller child devices are not populated and it
> should be handled along with OMAP clock changes.
> 
> Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com>
> ---
>  arch/arm/boot/dts/omap4-panda.dts |   57 +++++++++++++++++++++++++++++++++++++
>  1 files changed, 57 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/omap4-panda.dts b/arch/arm/boot/dts/omap4-panda.dts
> index 58909e9..f9af72f 100644
> --- a/arch/arm/boot/dts/omap4-panda.dts
> +++ b/arch/arm/boot/dts/omap4-panda.dts
> @@ -8,4 +8,61 @@
>  / {
>  	model = "TI OMAP4 PandaBoard";
>  	compatible = "ti,omap4-panda", "ti,omap4430";
> +	interrupt-parent = <&gic>;
> +
> +	gic: interrupt-controller at 48241000 {
> +		compatible = "ti,omap-gic", "arm,gic";

ti,omap4-gic

> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +		reg = <0x48241000 0x1000>,
> +		      <0X48240100 0x0200>;
> +	};
> +
> +	i2c at 48070000 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		compatible = "ti,omap_i2c";

ti,omap4-i2c.  Actually, it is quite possibly appropriate to do:

	compatible = "ti,omap4-i2c", "ti,omap3-i2c";

> +		reg = <0x48070000 0x100>;
> +		interrupts = < 88 >;
> +		interrupt-parent = <&gic>;
> +		clock-frequency = <400>;

Frequency should be a Hz value, not kHz or MHz.

> +		status = "disabled";
> +	};
> +
> +	i2c at 48072000 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		compatible = "ti,omap_i2c";
> +		reg = <0x48072000 0x100>;
> +		interrupts = < 89 >;
> +		interrupt-parent = <&gic>;
> +		clock-frequency = <400>;
> +		status = "ok";
> +	};
> +
> +	i2c at 48060000 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		compatible = "ti,omap_i2c";
> +		reg = <0x48060000 0x100>;
> +		interrupts = < 93 >;
> +		interrupt-parent = <&gic>;
> +		clock-frequency = <100>;
> +		status = "ok";
> +		eeprom at 50 {
> +			compatible = "at,at24c01";
> +			reg = < 0x50 >;
> +		};
> +	};
> +
> +	i2c at 48350000 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		compatible = "ti,omap_i2c";
> +		reg = <0x48350000 0x100>;
> +		interrupts = < 94 >;
> +		interrupt-parent = <&gic>;
> +		clock-frequency = <400>;
> +		status = "ok";
> +	};
>  };
> -- 
> 1.7.4.1
> 

[RFC PATCH 3/5] OMAP3: Beagle: Update beagle board file to use DT

[Grant Likely]

On Thu, Jun 30, 2011 at 03:07:25PM +0500, G, Manjunath Kondaiah wrote:
> 
> The beagle board file is updated to use i2c nodes from device
> tree data structures.
> 
> Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com>
> ---
>  arch/arm/mach-omap2/board-omap3beagle.c |   24 ++++++++++++++++++++----
>  1 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/board-omap3beagle.c b/arch/arm/mach-omap2/board-omap3beagle.c
> index 213c4cd..db494aa 100644
> --- a/arch/arm/mach-omap2/board-omap3beagle.c
> +++ b/arch/arm/mach-omap2/board-omap3beagle.c
> @@ -20,6 +20,7 @@
>  #include <linux/clk.h>
>  #include <linux/io.h>
>  #include <linux/of_platform.h>
> +#include <linux/of_address.h>
>  #include <linux/leds.h>
>  #include <linux/gpio.h>
>  #include <linux/input.h>
> @@ -33,6 +34,7 @@
>  
>  #include <linux/regulator/machine.h>
>  #include <linux/i2c/twl.h>
> +#include <linux/irq.h>
>  
>  #include <mach/hardware.h>
>  #include <asm/mach-types.h>
> @@ -421,8 +423,8 @@ static int __init omap3_beagle_i2c_init(void)
>  	omap3_pmic_init("twl4030", &beagle_twldata);
>  	/* Bus 3 is attached to the DVI port where devices like the pico DLP
>  	 * projector don't work reliably with 400kHz */
> +#if !defined(CONFIG_OF)
>  	omap_register_i2c_bus(3, 100, beagle_i2c_eeprom, ARRAY_SIZE(beagle_i2c_eeprom));
> -#ifdef CONFIG_OF
>  	omap_register_i2c_bus(2, 100, NULL, 0);
>  #endif /* CONFIG_OF */
>  	return 0;
> @@ -565,11 +567,24 @@ static void __init beagle_opp_init(void)
>  	return;
>  }
>  
> +static struct of_device_id omap_dt_gic_match[] __initdata = {
> +	{ .compatible = "ti,omap-gic", },
> +	{}
> +};
> +
> +static struct of_device_id omap_dt_match_table[] __initdata = {
> +	{ .compatible = "ti,omap3-beagle", },
> +	{}
> +};
> +
>  static void __init omap3_beagle_init(void)
>  {
> -#ifdef CONFIG_OF
> -	of_platform_prepare(NULL, NULL);
> -#endif /* CONFIG_OF */
> +	struct device_node *node;
> +
> +	node = of_find_matching_node_by_address(NULL, omap_dt_gic_match,
> +						OMAP34XX_IC_BASE);
> +	if (node)
> +		irq_domain_add_simple(node, 0);
>  
>  	omap3_mux_init(board_mux, OMAP_PACKAGE_CBB);
>  	omap3_beagle_init_rev();
> @@ -597,6 +612,7 @@ static void __init omap3_beagle_init(void)
>  
>  	beagle_display_init();
>  	beagle_opp_init();
> +	of_platform_populate(NULL, omap_dt_match_table, NULL, NULL);

This is probably a loosing approach.  Until the DT stuff is stable on
omap, you should avoid doing anything that might break existing board
ports.  Instead, I recommend creating an board-omap3-dt.c board file,
and pull in only the static beagle device registrations that you need
to get up and running, then you can pull them back out as the beagle
DT board support matures.

g.

>  }
>  
>  static const char *omap3_beagle_dt_match[] __initdata = {
> -- 
> 1.7.4.1
> 

[RFC PATCH 4/5] OMAP4: Panda: Update panda board file to use DT

[Grant Likely]

On Thu, Jun 30, 2011 at 03:07:26PM +0500, G, Manjunath Kondaiah wrote:
> 
> The OMAP4 panda board file is updated to use i2c nodes from device
> tree data structures.
> 
> Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com>
> ---
>  arch/arm/mach-omap2/board-omap4panda.c |   22 ++++++++++++++++++++++
>  1 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/board-omap4panda.c b/arch/arm/mach-omap2/board-omap4panda.c
> index c9d1e13..b3151a0 100644
> --- a/arch/arm/mach-omap2/board-omap4panda.c
> +++ b/arch/arm/mach-omap2/board-omap4panda.c
> @@ -22,12 +22,15 @@
>  #include <linux/clk.h>
>  #include <linux/io.h>
>  #include <linux/leds.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
>  #include <linux/gpio.h>
>  #include <linux/usb/otg.h>
>  #include <linux/i2c/twl.h>
>  #include <linux/regulator/machine.h>
>  #include <linux/regulator/fixed.h>
>  #include <linux/wl12xx.h>
> +#include <linux/irq.h>
>  
>  #include <mach/hardware.h>
>  #include <mach/omap4-common.h>
> @@ -410,6 +413,7 @@ static struct i2c_board_info __initdata panda_i2c_eeprom[] = {
>  static int __init omap4_panda_i2c_init(void)
>  {
>  	omap4_pmic_init("twl6030", &omap4_panda_twldata);
> +#if !defined(CONFIG_OF)
>  	omap_register_i2c_bus(2, 400, NULL, 0);
>  	/*
>  	 * Bus 3 is attached to the DVI port where devices like the pico DLP
> @@ -418,6 +422,7 @@ static int __init omap4_panda_i2c_init(void)
>  	omap_register_i2c_bus(3, 100, panda_i2c_eeprom,
>  					ARRAY_SIZE(panda_i2c_eeprom));
>  	omap_register_i2c_bus(4, 400, NULL, 0);
> +#endif

Yeah, don't do this.  Anything we merge to mainline we want to make
sure that a kernel image will still support both DT and non-DT boots.
That won't work if you #ifdef out blocks of code.

>  	return 0;
>  }
>  
> @@ -681,9 +686,25 @@ void omap4_panda_display_init(void)
>  	omap_display_init(&omap4_panda_dss_data);
>  }
>  
> +static struct of_device_id omap_dt_gic_match[] __initdata = {
> +	{ .compatible = "ti,omap-gic", },
> +	{}
> +};
> +
> +static struct of_device_id omap_dt_match_table[] __initdata = {
> +	{ .compatible = "ti,omap4-panda", },
> +	{}
> +};
> +
>  static void __init omap4_panda_init(void)
>  {
>  	int package = OMAP_PACKAGE_CBS;
> +	struct device_node *node;
> +
> +	node = of_find_matching_node_by_address(NULL, omap_dt_gic_match,
> +						OMAP44XX_GIC_DIST_BASE);
> +	if (node)
> +		irq_domain_add_simple(node, 0);
>  
>  	if (omap_rev() == OMAP4430_REV_ES1_0)
>  		package = OMAP_PACKAGE_CBL;
> @@ -700,6 +721,7 @@ static void __init omap4_panda_init(void)
>  	omap4_ehci_init();
>  	usb_musb_init(&musb_board_data);
>  	omap4_panda_display_init();
> +	of_platform_populate(NULL, omap_dt_match_table, NULL, NULL);
>  }
>  
>  static void __init omap4_panda_map_io(void)
> -- 
> 1.7.4.1
> 

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

[Grant Likely]

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@ti.com>

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

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

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

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

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

>  static struct dev_pm_ops omap_i2c_pm_ops = {
>  	.suspend = omap_i2c_suspend,
>  	.resume = omap_i2c_resume,
> @@ -1178,6 +1205,7 @@ static struct platform_driver omap_i2c_driver = {
>  		.name	= "omap_i2c",
>  		.owner	= THIS_MODULE,
>  		.pm	= OMAP_I2C_PM_OPS,
> +		.of_match_table = omap_i2c_of_match,
>  	},
>  };
>  
> -- 
> 1.7.4.1
> 

[RFC PATCH 1/5] OMAP3:I2C: Add device tree nodes for beagle board

[Stephen Warren]

Grant Likely wrote at Wednesday, July 06, 2011 12:56 PM:
> On Thu, Jun 30, 2011 at 03:07:23PM +0500, G, Manjunath Kondaiah wrote:
> > Add I2C and it's child device nodes for beagle board.
> > The I2C1 controller child devices are not populated and it
> > should be handled along with OMAP clock changes.
...
> > diff --git a/arch/arm/boot/dts/omap3-beagle.dts b/arch/arm/boot/dts/omap3-beagle.dts
...
> > +	i2c at 48070000 {
...
> > +		clock-frequency = <2600>;
> > +		status = "disabled";
> 
> Drop 'status' when you move this node definition to
> arch/arm/boot/dts/omap3.dtsi.  Board overlay files that include the
> omap3.dtsi should explicitly disable any devices that it does not use
> (which is opposite to what tegra currently does, but I'm going to
> change that).

Purely out of idle curiosity: What's the benefit of one way over the other?

-- 
nvpublic

[RFC PATCH 1/5] OMAP3:I2C: Add device tree nodes for beagle board

[Grant Likely]

On Wed, Jul 6, 2011 at 5:26 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Grant Likely wrote at Wednesday, July 06, 2011 12:56 PM:
>> On Thu, Jun 30, 2011 at 03:07:23PM +0500, G, Manjunath Kondaiah wrote:
>> > Add I2C and it's child device nodes for beagle board.
>> > The I2C1 controller child devices are not populated and it
>> > should be handled along with OMAP clock changes.
> ...
>> > diff --git a/arch/arm/boot/dts/omap3-beagle.dts b/arch/arm/boot/dts/omap3-beagle.dts
> ...
>> > + ? i2c at 48070000 {
> ...
>> > + ? ? ? ? ? clock-frequency = <2600>;
>> > + ? ? ? ? ? status = "disabled";
>>
>> Drop 'status' when you move this node definition to
>> arch/arm/boot/dts/omap3.dtsi. ?Board overlay files that include the
>> omap3.dtsi should explicitly disable any devices that it does not use
>> (which is opposite to what tegra currently does, but I'm going to
>> change that).
>
> Purely out of idle curiosity: What's the benefit of one way over the other?

Mostly consistency.  Most of the experience we have with the flattened
device tree up to this point hasn't bothered with the 'status'
property.  It is only when AMP and hypervisors cam online that it
became important to use a status property, and that only when the
kernel needs to be told that the device does indeed exist, but it must
not be touched.  I'd like to continue that pattern for new DT users
with the default assumption that a device is enabled unless the board
.dts explicitly disables it.

g.

[RFC PATCH 2/5] OMAP4:I2C: Add device tree nodes for panda board

[G, Manjunath Kondaiah]

On Wed, Jul 06, 2011 at 12:57:12PM -0600, Grant Likely wrote:
> On Thu, Jun 30, 2011 at 03:07:24PM +0500, G, Manjunath Kondaiah wrote:
> > 
> > Add I2C and it's child device nodes for beagle board.
> > The I2C1 controller child devices are not populated and it
> > should be handled along with OMAP clock changes.
> > 
> > Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com>
> > ---
> >  arch/arm/boot/dts/omap4-panda.dts |   57 +++++++++++++++++++++++++++++++++++++
> >  1 files changed, 57 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/omap4-panda.dts b/arch/arm/boot/dts/omap4-panda.dts
> > index 58909e9..f9af72f 100644
> > --- a/arch/arm/boot/dts/omap4-panda.dts
> > +++ b/arch/arm/boot/dts/omap4-panda.dts
> > @@ -8,4 +8,61 @@
> >  / {
> >  	model = "TI OMAP4 PandaBoard";
> >  	compatible = "ti,omap4-panda", "ti,omap4430";
> > +	interrupt-parent = <&gic>;
> > +
> > +	gic: interrupt-controller at 48241000 {
> > +		compatible = "ti,omap-gic", "arm,gic";
> 
> ti,omap4-gic
> 
> > +		interrupt-controller;
> > +		#interrupt-cells = <1>;
> > +		reg = <0x48241000 0x1000>,
> > +		      <0X48240100 0x0200>;
> > +	};
> > +
> > +	i2c at 48070000 {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +		compatible = "ti,omap_i2c";
> 
> ti,omap4-i2c.  Actually, it is quite possibly appropriate to do:
> 
> 	compatible = "ti,omap4-i2c", "ti,omap3-i2c";
ok. can also have "ti,omap2-i2c"
> 
> > +		reg = <0x48070000 0x100>;
> > +		interrupts = < 88 >;
> > +		interrupt-parent = <&gic>;
> > +		clock-frequency = <400>;
> 
> Frequency should be a Hz value, not kHz or MHz.
current i2c-omap driver expects value to be in KHz. Will change it in driver
and pass value in Hz here.

-Manjunath

[RFC PATCH 3/5] OMAP3: Beagle: Update beagle board file to use DT

[G, Manjunath Kondaiah]

On Wed, Jul 06, 2011 at 01:00:22PM -0600, Grant Likely wrote:
> On Thu, Jun 30, 2011 at 03:07:25PM +0500, G, Manjunath Kondaiah wrote:
> > 
> > The beagle board file is updated to use i2c nodes from device
> > tree data structures.
> > 
> > Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com>
> > ---
> >  arch/arm/mach-omap2/board-omap3beagle.c |   24 ++++++++++++++++++++----
> >  1 files changed, 20 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm/mach-omap2/board-omap3beagle.c b/arch/arm/mach-omap2/board-omap3beagle.c
> > index 213c4cd..db494aa 100644
> > --- a/arch/arm/mach-omap2/board-omap3beagle.c
> > +++ b/arch/arm/mach-omap2/board-omap3beagle.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/clk.h>
> >  #include <linux/io.h>
> >  #include <linux/of_platform.h>
> > +#include <linux/of_address.h>
> >  #include <linux/leds.h>
> >  #include <linux/gpio.h>
> >  #include <linux/input.h>
> > @@ -33,6 +34,7 @@
> >  
> >  #include <linux/regulator/machine.h>
> >  #include <linux/i2c/twl.h>
> > +#include <linux/irq.h>
> >  
> >  #include <mach/hardware.h>
> >  #include <asm/mach-types.h>
> > @@ -421,8 +423,8 @@ static int __init omap3_beagle_i2c_init(void)
> >  	omap3_pmic_init("twl4030", &beagle_twldata);
> >  	/* Bus 3 is attached to the DVI port where devices like the pico DLP
> >  	 * projector don't work reliably with 400kHz */
> > +#if !defined(CONFIG_OF)
> >  	omap_register_i2c_bus(3, 100, beagle_i2c_eeprom, ARRAY_SIZE(beagle_i2c_eeprom));
> > -#ifdef CONFIG_OF
> >  	omap_register_i2c_bus(2, 100, NULL, 0);
> >  #endif /* CONFIG_OF */
> >  	return 0;
> > @@ -565,11 +567,24 @@ static void __init beagle_opp_init(void)
> >  	return;
> >  }
> >  
> > +static struct of_device_id omap_dt_gic_match[] __initdata = {
> > +	{ .compatible = "ti,omap-gic", },
> > +	{}
> > +};
> > +
> > +static struct of_device_id omap_dt_match_table[] __initdata = {
> > +	{ .compatible = "ti,omap3-beagle", },
> > +	{}
> > +};
> > +
> >  static void __init omap3_beagle_init(void)
> >  {
> > -#ifdef CONFIG_OF
> > -	of_platform_prepare(NULL, NULL);
> > -#endif /* CONFIG_OF */
> > +	struct device_node *node;
> > +
> > +	node = of_find_matching_node_by_address(NULL, omap_dt_gic_match,
> > +						OMAP34XX_IC_BASE);
> > +	if (node)
> > +		irq_domain_add_simple(node, 0);
> >  
> >  	omap3_mux_init(board_mux, OMAP_PACKAGE_CBB);
> >  	omap3_beagle_init_rev();
> > @@ -597,6 +612,7 @@ static void __init omap3_beagle_init(void)
> >  
> >  	beagle_display_init();
> >  	beagle_opp_init();
> > +	of_platform_populate(NULL, omap_dt_match_table, NULL, NULL);
> 
> This is probably a loosing approach.  Until the DT stuff is stable on
> omap, you should avoid doing anything that might break existing board
> ports.  Instead, I recommend creating an board-omap3-dt.c board file,
> and pull in only the static beagle device registrations that you need
> to get up and running, then you can pull them back out as the beagle
> DT board support matures.
yes. that is the plan. I have mentioned the same in PATCH 0/5

-Manjunath

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

[G, Manjunath Kondaiah]

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@ti.com>
> 
> 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

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

[Grant Likely]

On Thu, Jul 07, 2011 at 10:43:49PM +0530, G, Manjunath Kondaiah wrote:
> 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@ti.com>
> > 
> > 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.

You should be able to sidestep this issue in the short term.  As long
as you can get the of_platform_populate() function to create
omap_devices that look identical to the current statically allocated
omap_devices.  If you need to extend the auxdata structure to do so,
then I'm okay with that.  You could add special handling for devices
compatible with "ti,omap3-device" or "ti,omap4-device".

I'm keen to see this done, and if you can put it together quickly,
then I think I still have time to queue it up for v3.1 (especially
since it would be a new block of code that should not break any
existing boards).

g.

[RFC PATCH 1/5] OMAP3:I2C: Add device tree nodes for beagle board

[Shawn Guo]

On Wed, Jul 06, 2011 at 06:12:49PM -0600, Grant Likely wrote:
> On Wed, Jul 6, 2011 at 5:26 PM, Stephen Warren <swarren@nvidia.com> wrote:
> > Grant Likely wrote at Wednesday, July 06, 2011 12:56 PM:
> >> On Thu, Jun 30, 2011 at 03:07:23PM +0500, G, Manjunath Kondaiah wrote:
> >> > Add I2C and it's child device nodes for beagle board.
> >> > The I2C1 controller child devices are not populated and it
> >> > should be handled along with OMAP clock changes.
> > ...
> >> > diff --git a/arch/arm/boot/dts/omap3-beagle.dts b/arch/arm/boot/dts/omap3-beagle.dts
> > ...
> >> > + ? i2c at 48070000 {
> > ...
> >> > + ? ? ? ? ? clock-frequency = <2600>;
> >> > + ? ? ? ? ? status = "disabled";
> >>
> >> Drop 'status' when you move this node definition to
> >> arch/arm/boot/dts/omap3.dtsi. ?Board overlay files that include the
> >> omap3.dtsi should explicitly disable any devices that it does not use
> >> (which is opposite to what tegra currently does, but I'm going to
> >> change that).
> >
> > Purely out of idle curiosity: What's the benefit of one way over the other?
> 
> Mostly consistency.  Most of the experience we have with the flattened
> device tree up to this point hasn't bothered with the 'status'
> property.  It is only when AMP and hypervisors cam online that it
> became important to use a status property, and that only when the
> kernel needs to be told that the device does indeed exist, but it must
> not be touched.  I'd like to continue that pattern for new DT users
> with the default assumption that a device is enabled unless the board
> .dts explicitly disables it.
> 
When going this way with my i.mx53 related dts files, I feel I like the
opposite way.

i.mx53 has many number of peripherals inside, while individual board
might only use small portion there.  For example, i.mx53 has 5 uart
controller instances inside, while quick start (aka loco) board only
uses one.  With the way you suggest here, we will have the following
in imx53.dtsi.

uart0: uart at ... {
	...
};

uart1: uart at ... {
	...
};

uart2: uart at ... {
	...
};

uart3: uart at ... {
	...
};

uart4: uart at ... {
	...
};

And we will have to tell the 4 we do not use on quick start board in
imx53-qs.dtsi


uart1: uart at ... {
	...
	status = "disabled";
};

uart2: uart at ... {
	...
	status = "disabled";
};

uart3: uart at ... {
	...
	status = "disabled";
};

uart4: uart at ... {
	...
	status = "disabled";
};

Besides the bothering that we have to list so many unused controllers
in individual board dts file, it's also hard to tell which controllers
are actually available on the board.  People have to look at imx53.dts
to get a full list and then exclude the ones in imx53-<board>.dts as
"disabled".

And if we go the way opposite, adding "disabled" status for everyone
in imx53.dts, we will only need to specify the peripherals that are
actually available on board with "okay" status in imx53-<board>.dts.
And it's much more clear for people to see what peripherals are
available on individual board.

So I'm going the way than you suggested.  Please let me know if you
strongly dislikes it.

-- 
Regards,
Shawn

[RFC PATCH 1/5] OMAP3:I2C: Add device tree nodes for beagle board

[Grant Likely]

On Wed, Jul 20, 2011 at 07:04:20PM +0800, Shawn Guo wrote:
> > Mostly consistency.  Most of the experience we have with the flattened
> > device tree up to this point hasn't bothered with the 'status'
> > property.  It is only when AMP and hypervisors cam online that it
> > became important to use a status property, and that only when the
> > kernel needs to be told that the device does indeed exist, but it must
> > not be touched.  I'd like to continue that pattern for new DT users
> > with the default assumption that a device is enabled unless the board
> > .dts explicitly disables it.
[...]
> Besides the bothering that we have to list so many unused controllers
> in individual board dts file, it's also hard to tell which controllers
> are actually available on the board.  People have to look at imx53.dts
> to get a full list and then exclude the ones in imx53-<board>.dts as
> "disabled".
> 
> And if we go the way opposite, adding "disabled" status for everyone
> in imx53.dts, we will only need to specify the peripherals that are
> actually available on board with "okay" status in imx53-<board>.dts.
> And it's much more clear for people to see what peripherals are
> available on individual board.
> 
> So I'm going the way than you suggested.  Please let me know if you
> strongly dislikes it.

Yes, I strongly dislike it.  I understand the concern, but at this
early stage with converting to device tree I think consistency between
platforms is more important.  We can talk about the issue at Linaro
Connect in 2 weeks, but in the mean time please use the
enabled-by-default/explicitly-disabled pattern.

g.

[RFC PATCH 1/5] OMAP3:I2C: Add device tree nodes for beagle board

[Shawn Guo]

On Wed, Jul 20, 2011 at 12:55:13PM -0600, Grant Likely wrote:
> On Wed, Jul 20, 2011 at 07:04:20PM +0800, Shawn Guo wrote:
> > > Mostly consistency.  Most of the experience we have with the flattened
> > > device tree up to this point hasn't bothered with the 'status'
> > > property.  It is only when AMP and hypervisors cam online that it
> > > became important to use a status property, and that only when the
> > > kernel needs to be told that the device does indeed exist, but it must
> > > not be touched.  I'd like to continue that pattern for new DT users
> > > with the default assumption that a device is enabled unless the board
> > > .dts explicitly disables it.
> [...]
> > Besides the bothering that we have to list so many unused controllers
> > in individual board dts file, it's also hard to tell which controllers
> > are actually available on the board.  People have to look at imx53.dts
> > to get a full list and then exclude the ones in imx53-<board>.dts as
> > "disabled".
> > 
> > And if we go the way opposite, adding "disabled" status for everyone
> > in imx53.dts, we will only need to specify the peripherals that are
> > actually available on board with "okay" status in imx53-<board>.dts.
> > And it's much more clear for people to see what peripherals are
> > available on individual board.
> > 
> > So I'm going the way than you suggested.  Please let me know if you
> > strongly dislikes it.
> 
> Yes, I strongly dislike it.  I understand the concern, but at this
> early stage with converting to device tree I think consistency between
> platforms is more important.  We can talk about the issue at Linaro
> Connect in 2 weeks, but in the mean time please use the
> enabled-by-default/explicitly-disabled pattern.
> 
Okay, hope you will not ask me to use the opposite pattern when you
actually see the patch :)

-- 
Regards,
Shawn

[RFC PATCH 1/5] OMAP3:I2C: Add device tree nodes for beagle board

[Grant Likely]

On Wed, Jul 20, 2011 at 4:33 PM, Shawn Guo <shawn.guo@freescale.com> wrote:
> On Wed, Jul 20, 2011 at 12:55:13PM -0600, Grant Likely wrote:
>> On Wed, Jul 20, 2011 at 07:04:20PM +0800, Shawn Guo wrote:
>> > > Mostly consistency. ?Most of the experience we have with the flattened
>> > > device tree up to this point hasn't bothered with the 'status'
>> > > property. ?It is only when AMP and hypervisors cam online that it
>> > > became important to use a status property, and that only when the
>> > > kernel needs to be told that the device does indeed exist, but it must
>> > > not be touched. ?I'd like to continue that pattern for new DT users
>> > > with the default assumption that a device is enabled unless the board
>> > > .dts explicitly disables it.
>> [...]
>> > Besides the bothering that we have to list so many unused controllers
>> > in individual board dts file, it's also hard to tell which controllers
>> > are actually available on the board. ?People have to look at imx53.dts
>> > to get a full list and then exclude the ones in imx53-<board>.dts as
>> > "disabled".
>> >
>> > And if we go the way opposite, adding "disabled" status for everyone
>> > in imx53.dts, we will only need to specify the peripherals that are
>> > actually available on board with "okay" status in imx53-<board>.dts.
>> > And it's much more clear for people to see what peripherals are
>> > available on individual board.
>> >
>> > So I'm going the way than you suggested. ?Please let me know if you
>> > strongly dislikes it.
>>
>> Yes, I strongly dislike it. ?I understand the concern, but at this
>> early stage with converting to device tree I think consistency between
>> platforms is more important. ?We can talk about the issue at Linaro
>> Connect in 2 weeks, but in the mean time please use the
>> enabled-by-default/explicitly-disabled pattern.
>>
> Okay, hope you will not ask me to use the opposite pattern when you
> actually see the patch :)

:-)

g.