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

[John Bonesio <bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>]

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:
>> 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.
> 
>> diff --git a/arch/arm/boot/dts/tegra250.dtsi b/arch/arm/boot/dts/tegra250.dtsi
> 
>> +
>> +	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.

> 
>> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
> 
>> +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?
> 
>> +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.

> 
>> 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!
> 

:-)