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

[stefano babic]

Am 19/09/2011 22:34, schrieb Eric Jarrige:
> Hi Stefano,
>
> On 19 sept. 2011, at 09:26, Stefano Babic wrote:
>
Hi Eric,

>> 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)
...I see the clock name is not so generic, and it is not related to the
interface..

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

We have already this mechanism - this is the goal of the mxc_get_clock()
function: a common way to get clocks for all i.MX processors.
We have for example MXC_CSPI_CLK (SPI), MXC_UART_CLK, and so on. The
line should become:

freq = mxc_get_clock(MXC_I2C_CLK)

and then each processor can return its own value. This already works for the other interfaces (FEC, UART, SPI,..)



>
> that can be defined in clock.h with the correct definition for each chipset?
We need only to extend the mxc_clock enumeration type. The
implementation of mxc_get_clock() is SOC specific.
> 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?

Yes, it makes sense to change, check my proposal 

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================