[U-Boot] [PATCH] mx1: add mx1/l support for mxc_i2c

[Eric Jarrige]

Hi Stefano,

On 19 sept. 2011, at 09:26, Stefano Babic wrote:

> On 09/19/2011 08:57 AM, Marek Vasut wrote:
>> On Monday, August 22, 2011 10:56:43 PM Eric Jarrige wrote:
>>> Signed-off-by: Eric Jarrige <eric.jarrige at armadeus.org>
>>> Cc: Stefano Babic <sbabic at denx.de>
>>> Cc: Heiko Schocher <hs at denx.de>
>>> @@ -94,6 +98,8 @@ void i2c_init(int speed, int unused)
>>> 	/* start the required I2C clock */
>>> 	writel(readl(&sc_regs->cgr0) | (3 << I2C_CLK_OFFSET),
>>> 		&sc_regs->cgr0);
>>> +#elif defined(CONFIG_IMX)
>>> +	freq = get_HCLK();
>>> #else
>>> 	freq = mxc_get_clock(MXC_IPG_PERCLK);
>>> #endif
>> 
>> Please, no more ifdefs -- can't you actually modify your header files or add 
>> some which would define mxc_get_clock() ? That'd make this way much more clean.
> 
> In principl, I agree completely with Marek, and we have tried to get rid
> as much of possible of #ifdef in drivers. I know we have already
> discussed about the opportunity to fix the whole IMX stuff, and I agreed
> with you that this processor is obsolete and make no sense to invest a
> lot of time for it. However, what about to define mxc_get_clock() as
> simple macro ? Maybe in imx-regs.h, as exceptiion for this processor
> (normally the imx-regs.h contains only defines and structures for the
> internal registers) ? Something like:
> 
> 	#define mxc_get_clock(a)	(get_HCLK())

I agree with you to get rid of all these #ifdef and my current patch does not help at all.
I can update the imx-regs.h with the change you propose but that won't help for the
future iMX CPU.
IMHO the problem comes from all the hard coded constants in the driver such as
mxc_get_clock(MXC_IPG_PERCLK)  and others I2C_BASE...
Every chipset use a different root clock for its i2c controller.

Why not changing the line
	freq = mxc_get_clock(MXC_IPG_PERCLK);

by something more generic such as  a macro:

	freq = MXC_GET_I2C_CLOCK(); /* or GET_MXC_I2C_CLOCK() */


that can be defined in clock.h with the correct definition for each chipset?
Using a macro would clarify that is chipset dependent as each CPU use
its own clock.h

For the mx1, I can add a clock.h with the correct implementation of the
macro:
#define MXC_GET_I2C_CLOCK() get_HCLK()
or any other solution that avoid any new ifdef.

Does it makes sense?

With all these constants moved to header files it would be easier and faster to support
new CPU as the final hardware abstraction layer moved to header file specific for each
CPU.

Best regards,
Eric