[U-Boot] [PATCH 3/8] ARMV7: Modify i2c driver for more reliable operation on OMAP4

Steve Sakoman
2010-07-23T16:33:10Z

On Wed, Jul 21, 2010 at 4:53 PM, Nishanth Menon
<menon.nishanth@gmail.com> wrote:

>>>> diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c
>>>> index 3256133..a91fe55 100644
>>>> --- a/drivers/i2c/omap24xx_i2c.c
>>>> +++ b/drivers/i2c/omap24xx_i2c.c
>>>> @@ -41,6 +41,7 @@ void i2c_init (int speed, int slaveadd)
>>>> ? ? ? int psc, fsscll, fssclh;
>>>> ? ? ? int hsscll = 0, hssclh = 0;
>>>> ? ? ? u32 scll, sclh;
>>>> + ? ? int reset_timeout = 10;
>>>
>>> I am guessing we ?can life with a u8, we timeout in 10ms.. would asking
>>> for a #define I2C_TIMEOUT_RESET is too much here?
>>
>> I can't imagine why we would ever want to make this a config option!
>>
>> In normal operation the loop is only traversed one time. ?I only added
>> the timeout to prevent the possibility of an infinite loop.
>
> not as a config option, but a #define in the C file so that it is readable,
> the 10 makes not much sense at a glance.. but as I mentioned, it is just a
> nitpick.. it just helps to quickly figure out which are the timeouts used in
> the driver.. I mean we cant proceed with i2c_init or use it as i2c master
> (aka no point in using the driver) if that happens in the system rt?

OK, I will submit a version 2 with a #define for the timeout/retry count.

>>>> - ? ? ? ? ? ? printf ("I2C read: addr len %d not supported\n", alen);
>>>> + ? ? ? ? ? ? printf("I2C write: addr len %d not supported\n", alen);
>>>
>>> err.. do we need this change?
>>
>> I think so -- otherwise the i2c_write function is printing messages
>> claiming to come from i2c_read! ?Same for the other messages you
>> flagged.
>>
>
> Apologies on not being clear - I dont have a problem with the print as such,
> BUT, do we need a diff here which changes printf ("... to printf("... ?
> makes no reason why I need that for reliable OMAP4 operation :D..

The message is clearly wrong, but your point is well taken: an
erroneous error message fix doesn't really belong in a patch for
reliable operation on OMAP4.

Version 2 of this patch will address only the reliability issues and
your request for a timeout #define.

I will then do a subsequent patch (not part of this series) to fix
erroneous error messages, potential infinite loops, and a variety of
whitespace and style issues.

Regards,

Steve