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.va...@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: off...@denx.de
=====================================================================
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
