On 4 October 2012 13:30, Uwe Kleine-König
<u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> On Fri, Sep 28, 2012 at 04:58:43PM +0530, Viresh Kumar wrote:
>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> +static inline void set_scl_value(struct i2c_adapter *adap, int val)
>> +{
>> + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>> +
>> + if (bri->is_gpio_recovery)
>> + gpio_set_value(bri->scl_gpio, val);
>> + else
>> + bri->set_scl(adap, val);
> I would have done this in a different way (with function pointers
> instead of an if for each bang). Well, I don't care much.
That would be better. Will be fixed :)
>> +static int i2c_recover_bus(struct i2c_adapter *adap)
>> +{
>> + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>> + unsigned long delay = 1000000;
>> + int i, ret, val = 1;
>> +
>> + if (bri->is_gpio_recovery) {
>> + ret = i2c_get_gpios_for_recovery(adap);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + delay = DIV_ROUND_UP(delay, bri->clock_rate_khz * 2);
>> +
>> + for (i = 0; i < bri->clock_cnt * 2; i++, val = !val) {
>> + set_scl_value(adap, val);
>> + ndelay(delay);
>> +
>> + /* break if sda got high, check only when scl line is high */
>> + if (!bri->skip_sda_polling && val)
>> + if (unlikely(get_sda_value(adap)))
>> + break;
>> + }
> With clock_cnt usually being 9 (and skipping sda polling) assume a
> device pulls down SDA because it was just addressed but the last clock
> pulse to release SDA didn't occur:
>
> SDAout ¯\_/¯¯¯\___/¯¯¯\___________________/¯¯¯¯¯¯
> SCL ¯¯\_/¯\_/¯\_/¯\_/¯\_/¯\_/¯\_/¯\_/¯\_/¯¯¯¯¯
> S 1 2 3 4 5 6 7 8 9
>
> This adresses an eeprom with address 0x50. When SCL is released for the
> 9th clock the eeprom pulls down SDA to ack being addressed. After that
> the eeprom expects the master to write a byte.
>
> Now you do write 0xff:
>
> i 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17
> SDAin ___/¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯\______
> SCL ¯¯\_/¯\_/¯\_/¯\_/¯\_/¯¯\__/¯¯\__/¯¯\__/¯¯\______/
> 9 1 2 3 4 5 6 7 8
>
> (assuming the SCL line goes up some time later to its idle state).
> That is you leave the bus in exactly the same state as before: The 9th
> clock isn't complete and the eeprom asserts SDA low to ack the byte just
> written. So the bus is still stalled. The problem is BTW the exact same
> one I introduced first in my bus clear routine (for barebox though).
> Even though you count to 2*9 you only do 8 real clock pulses because you
> have a half cycle at both the start and the end.
Fantastic explanation!!
So, we actually need to do "Low-High" 9 times instead of "High-Low".
So, initializing val to 0 should fix it?
>> + * @scl_gpio_flags: flag for gpio_request_one of scl_gpio. 0 implies
>> + * GPIOF_OUT_INIT_LOW.
>> + * @sda_gpio_flags: flag for gpio_request_one of sda_gpio. 0 implies
>> + * GPIOF_OUT_INIT_LOW.
> Didn't you want to change this to GPIOF_OUT_INIT_HIGH |
> GPIOF_OPEN_DRAIN? Hmm, I just wonder how to distinguish
> GPIOF_OUT_INIT_LOW from unset. Hmm, maybe assume unset because _LOW is
> wrong?
Hmmm.... Hmmmm... Hmmm... :)
Two things:
- Why should we default it to GPIOF_OPEN_DRAIN? Open drain would mean,
gpio_set_value() will not write one for it, but would put it in input mode.
I don't think that would be correct, as an generic case.
- Obviously _LOW seems to be incorrect. So we can actually do something as
simple as: pass (sda_gpio_flags | GPIOF_OUT_INIT_HIGH). That will work
in both cases, user passed a value or not.
--
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
