Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus

[Philby John <pjohn-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>]

Hello Sekhar,

My apologies for the late mail. Had trouble with our mail server and I
seem to have lost mails. Pulling this thread from the list. Comments
inline...


On Mon, Feb 1, 2010 at 11:27 AM, Nori, Sekhar <nsekhar-l0cyMroinI0@public.gmane.org> wrote:
> Hi Philby,
>
> On Wed, Jan 27, 2010 at 05:11:33, Kevin Hilman wrote:
>> From: Philby John <pjohn-k0rHJ+Hhz/SB+jHODAdFcQ@public.gmane.org>
>>
>> Come out of i2c time out condition by following the
>> bus recovery procedure outlined in the i2c protocol v3 spec.
>> The kernel must be robust enough to gracefully recover
>> from i2c bus failure without having to reset the machine.
>> This is done by first NACKing the slave, pulsing the SCL
>> line 9 times and then sending the stop command.
>>
>> This patch has been tested on a DM6446 and DM355
>>
>> Signed-off-by: Philby John <pjohn-k0rHJ+Hhz/SB+jHODAdFcQ@public.gmane.org>
>> Signed-off-by: Srinivasan, Nageswari <nageswari-l0cyMroinI0@public.gmane.org>
>> Acked-by: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
>> ---
>>  drivers/i2c/busses/i2c-davinci.c |   57 +++++++++++++++++++++++++++++++++++--
>>  1 files changed, 53 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
>> index 35f9daa..5459065 100644
>> --- a/drivers/i2c/busses/i2c-davinci.c
>> +++ b/drivers/i2c/busses/i2c-davinci.c
>> @@ -36,6 +36,7 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/io.h>
>>  #include <linux/cpufreq.h>
>> +#include <linux/gpio.h>
>>
>>  #include <mach/hardware.h>
>>  #include <mach/i2c.h>
>> @@ -43,6 +44,7 @@
>>  /* ----- global defines ----------------------------------------------- */
>>
>>  #define DAVINCI_I2C_TIMEOUT  (1*HZ)
>> +#define DAVINCI_I2C_MAX_TRIES        2
>>  #define I2C_DAVINCI_INTR_ALL    (DAVINCI_I2C_IMR_AAS | \
>>                                DAVINCI_I2C_IMR_SCD | \
>>                                DAVINCI_I2C_IMR_ARDY | \
>> @@ -130,6 +132,44 @@ static inline u16 davinci_i2c_read_reg(struct davinci_i2c_dev *i2c_dev, int reg)
>>       return __raw_readw(i2c_dev->base + reg);
>>  }
>>
>> +/* Generate a pulse on the i2c clock pin. */
>> +static void generic_i2c_clock_pulse(unsigned int scl_pin)
>> +{
>> +     u16 i;
>> +
>> +     if (scl_pin) {
>> +             /* Send high and low on the SCL line */
>> +             for (i = 0; i < 9; i++) {
>> +                     gpio_set_value(scl_pin, 0);
>> +                     udelay(20);
>> +                     gpio_set_value(scl_pin, 1);
>> +                     udelay(20);
>> +             }
>
> Before using the pins as GPIO, you would have to set the
> functionality of these pins as GPIO. You had this code in
> previous incarnations of this patch - not sure why it is
> dropped now.
>

Don't seem to remember having the code in the old versions at least
not in generic_i2c_clock_pulse(). The functions disable_i2c_pins() and
enable_i2c_pins() were discarded as the i2c protocol spec. did not
specify the need. Moreover bus recovered without it. (Tested on DM355
and Dm6446).

> Couple of good to haves:
>
> It will be good to do a gpio_request() before using the pins
> as GPIO - though I can see it may have been deemed unnecessary
> - the pins are owned by I2C already - even so it may help catch
> system configuration errors in later platforms.

Yes, I could use gpio_request() in generic_i2c_clock_pulse().

>
> The I2C peripheral on da8xx itself contains a mode where its
> pins could be used as GPIO - so no need for SoC level muxing
> and need for the platform data. This seems to be missing from
> DM355 though. Thankfully there is a revision id within the I2C
> memory map which will help you differentiate the two cases
> (revision 0x5 vs 0x6)

I did not entirely follow your above statement. Will usage of
gpio_request() solve the problem for da8xx and DM355 or does it
require a if else condition? A reminder that the present code will
only work for DM6446 and DM355. I do not have a DA8xx to test specific
functionality if it were to be added.

Regards,
Philby