[U-Boot] [PATCH] I2C: mxc_i2c rework

[Stefano Babic <sbabic@denx.de>]

On 07/13/2011 11:53 AM, Marek Vasut wrote:
> Rewrite the mxc_i2c driver.
>  * This version is much closer to Linux implementation.
>  * Fixes IPG_PERCLK being incorrectly used as clock source
>  * Fixes behaviour of the driver on iMX51
>  * Clean up coding style a bit ;-)
> 
> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> ---

Hi Marek,

I have added Heiko in CC. He is the Maintainer for I2C.

>  #define I2C_MAX_TIMEOUT		10000
> -#define I2C_MAX_RETRIES		3
>  
> -static u16 div[] = { 30, 32, 36, 42, 48, 52, 60, 72, 80, 88, 104, 128, 144,
> -	             160, 192, 240, 288, 320, 384, 480, 576, 640, 768, 960,
> -	             1152, 1280, 1536, 1920, 2304, 2560, 3072, 3840};
> +static u16 i2c_clk_div[50][2] = {
> +	{ 22,	0x20 }, { 24,	0x21 }, { 26,	0x22 }, { 28,	0x23 },
> +	{ 30,	0x00 }, { 32,	0x24 }, { 36,	0x25 }, { 40,	0x26 },
> +	{ 42,	0x03 }, { 44,	0x27 }, { 48,	0x28 }, { 52,	0x05 },
> +	{ 56,	0x29 }, { 60,	0x06 }, { 64,	0x2A }, { 72,	0x2B },
> +	{ 80,	0x2C }, { 88,	0x09 }, { 96,	0x2D }, { 104,	0x0A },
> +	{ 112,	0x2E }, { 128,	0x2F }, { 144,	0x0C }, { 160,	0x30 },
> +	{ 192,	0x31 }, { 224,	0x32 }, { 240,	0x0F }, { 256,	0x33 },
> +	{ 288,	0x10 }, { 320,	0x34 }, { 384,	0x35 }, { 448,	0x36 },
> +	{ 480,	0x13 }, { 512,	0x37 }, { 576,	0x14 }, { 640,	0x38 },
> +	{ 768,	0x39 }, { 896,	0x3A }, { 960,	0x17 }, { 1024,	0x3B },
> +	{ 1152,	0x18 }, { 1280,	0x3C }, { 1536,	0x3D }, { 1792,	0x3E },
> +	{ 1920,	0x1B }, { 2048,	0x3F }, { 2304,	0x1C }, { 2560,	0x1D },
> +	{ 3072,	0x1E }, { 3840,	0x1F }
> +};

You have added an array with fixed values as indicated in the
Freescale's manual (Table 26-7 for i.MX31, Table 40-7 for MX51, Table
41-12 for MX53). What about to add also some comments about these changes ?


> -void i2c_init(int speed, int unused)
> +/*
> + * Calculate and set proper clock divider
> + *
> + * FIXME: remove #ifdefs !
> + */

Agree. I have prepared a patch to get rid of this mx31_ specialties. I
will post soon. Then we can use mxc_get_clock(MXC_IPG_CLK) for all i.MX
processors.


> +	div = (i2c_clk_rate + rate - 1) / rate;
> +	if (div < i2c_clk_div[0][0])
> +		i = 0;
> +	else if (div > i2c_clk_div[ARRAY_SIZE(i2c_clk_div) - 1][0])
> +		i = ARRAY_SIZE(i2c_clk_div) - 1;
> +	else
> +		for (i = 0; i2c_clk_div[i][0] < div; i++);
> +
> +	/* Store divider value */
> +	writeb(div, I2C_BASE + IFDR);
> +	clk_idx = div;

It seems to me ok - you replaced a computed value, that does not obtain
exactly the value indicated in the manual, with the closest value of the
table.

> +int i2c_imx_bus_busy(int for_busy)
>  {
> +	unsigned int temp;
> +
>  	int timeout = I2C_MAX_TIMEOUT;
>  
> -	while ((readw(I2C_BASE + I2SR) & I2SR_IBB) && --timeout) {
> -		writew(0, I2C_BASE + I2SR);
> +	while (timeout--) {
> +		temp = readb(I2C_BASE + I2SR);
> +
> +		if (for_busy && (temp & I2SR_IBB))
> +			return 0;
> +		if (!for_busy && !(temp & I2SR_IBB))
> +			return 0;
> +
>  		udelay(1);
>  	}
> -	return timeout ? timeout : (!(readw(I2C_BASE + I2SR) & I2SR_IBB));
> +
> +	return 1;
>  }

It is not clear to me why you add a way to go out from the function. If
it is busy, should we not wait at least until the timeout variable
becomes zero ?


>  
> -static int wait_busy(void)
> +/*
> + * Wait for transaction to complete
> + */
> +int i2c_imx_trx_complete(void)
>  {
>  	int timeout = I2C_MAX_TIMEOUT;
>  
> -	while (!(readw(I2C_BASE + I2SR) & I2SR_IBB) && --timeout)
> +	while (timeout--) {
> +		if (readb(I2C_BASE + I2SR) & I2SR_IIF) {

If we wait for completion, should we not check the ICF bit instead of
IIF, as done before your patch ?

> +/*
> + * Start the controller
> + */
> +int i2c_imx_start(void)
> +{
> +	unsigned int temp = 0;
> +	int result;
>  
> -	writew(0, I2C_BASE + I2SR);	/* clear interrupt */
> +	writeb(clk_idx, I2C_BASE + IFDR);

Well, as you talk about cleaning up the code, what about to replace the
direct access to the registers with a C structure, as part of your clean
up ?


> +/*
> + * Write register address
> + */
> +int i2c_imx_set_reg_addr(uint addr, int alen)
>  {
> -	int i, retry = 0;
> -	for (retry = 0; retry < 3; retry++) {
> -		if (wait_idle())
> +	int ret;
> +	int i;
> +

mmmhh...it seems to me you change completely the logic here. Heiko, waht
do you think about ?

Best regards,
Stefano Babic

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