Re: [RFC 2/2] ARM: Tegra: Device Tree Support: Add i2c devices

My comments are inline below.

- John

On 05/10/2011 03:10 PM, Stephen Warren wrote:

John Bonesio wrote at Tuesday, May 10, 2011 2:15 PM:

quoted

This patch initializes i2c controller devices in board-dt.c. The i2c
controller
is added to tegra250.dtsi so later on-board i2c devices can be found and
initialized based on the device tree information.

quoted

diff --git a/arch/arm/boot/dts/tegra250.dtsi b/arch/arm/boot/dts/tegra250.dtsi

quoted

+
+	i2c@7000C000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra250-i2c";
+		reg = <0x7000C000 0x100>;
+		interrupts = < 70 >;
+	};

Could we add 'status = "disabled"' for all of these in the core Tegra SoC
file. Then, the board-specific files (e.g. tegra-harmony.dts) will set
'status = "ok"' for just the particular controllers that are actually used
by the board. The SDHCI controllers are setup this way now per my recent
patches.

Good idea.

quoted

diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c

quoted

+static struct tegra_i2c_platform_data harmony_i2c1_platform_data = {
+	.bus_clk_rate   = 400000,
+};

Similar to my comments on the SDHCI patch, shouldn't the platform data
come only from devicetree?

quoted

+static void __init harmony_i2c_init(void)

It'd be nice not to name stuff in board-dt.c after "harmony", since the
idea is for it to work on arbitrary boards given the appropriate
devicetree.

Yep. I'll fix this in the next revision.

quoted

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c

Since the driver and board files go to separate subsystems, shouldn't
the changes to this file be a separate patch, to aid eventual
upstreaming?

Maybe. Let's see what others think.

BTW, thanks for working on Tegra!

:-)