Hi Wolfram, Sorry for the late review.
On Sun, 8 Nov 2009 21:14:57 +0100, Wolfram Sang wrote: > Writes may take some time on EEPROMs, so for consecutive writes, we already > have a loop waiting for the EEPROM to become ready. Use such a loop for reads, > too, in case somebody wants to immediately read after a write. Detailed bug > report and test case can be found here: > > http://article.gmane.org/gmane.linux.drivers.i2c/4660 > > Reported-by: Aleksandar Ivanov <[email protected]> > Signed-off-by: Wolfram Sang <[email protected]> > Cc: David Brownell <[email protected]> > Cc: Jean Delvare <[email protected]> > --- > > I could reproduce the errornous behaviour and this patch fixes it for me. Looks overall good, with just one comment: > > drivers/misc/eeprom/at24.c | 78 ++++++++++++++++++++++++++----------------- > 1 files changed, 47 insertions(+), 31 deletions(-) > > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c > index db39f4a..78aa46c 100644 > --- a/drivers/misc/eeprom/at24.c > +++ b/drivers/misc/eeprom/at24.c > @@ -158,6 +158,7 @@ static ssize_t at24_eeprom_read(struct at24_data *at24, > char *buf, > struct i2c_msg msg[2]; > u8 msgbuf[2]; > struct i2c_client *client; > + unsigned long timeout, read_time; > int status, i; > > memset(msg, 0, sizeof(msg)); > @@ -183,47 +184,62 @@ static ssize_t at24_eeprom_read(struct at24_data *at24, > char *buf, > if (count > io_limit) > count = io_limit; > > - /* Smaller eeproms can work given some SMBus extension calls */ > if (at24->use_smbus) { > + /* Smaller eeproms can work given some SMBus extension calls */ > if (count > I2C_SMBUS_BLOCK_MAX) > count = I2C_SMBUS_BLOCK_MAX; > - status = i2c_smbus_read_i2c_block_data(client, offset, > - count, buf); > - dev_dbg(&client->dev, "smbus read %...@%d --> %d\n", > - count, offset, status); > - return (status < 0) ? -EIO : status; > + } else { > + /* > + * When we have a better choice than SMBus calls, use a > + * combined I2C message. Write address; then read up to > + * io_limit data bytes. Note that read page rollover helps us > + * here (unlike writes). msgbuf is u8 and will cast to our > + * needs. > + */ > + i = 0; > + if (at24->chip.flags & AT24_FLAG_ADDR16) > + msgbuf[i++] = offset >> 8; > + msgbuf[i++] = offset; > + > + msg[0].addr = client->addr; > + msg[0].buf = msgbuf; > + msg[0].len = i; > + > + msg[1].addr = client->addr; > + msg[1].flags = I2C_M_RD; > + msg[1].buf = buf; > + msg[1].len = count; > } > > /* > - * When we have a better choice than SMBus calls, use a combined > - * I2C message. Write address; then read up to io_limit data bytes. > - * Note that read page rollover helps us here (unlike writes). > - * msgbuf is u8 and will cast to our needs. > + * Reads fail if the previous write didn't complete yet. We may > + * loop a few times until this one succeeds, waiting at least > + * long enough for one entire page write to work. > */ > - i = 0; > - if (at24->chip.flags & AT24_FLAG_ADDR16) > - msgbuf[i++] = offset >> 8; > - msgbuf[i++] = offset; > - > - msg[0].addr = client->addr; > - msg[0].buf = msgbuf; > - msg[0].len = i; > + timeout = jiffies + msecs_to_jiffies(write_timeout); > + do { > + read_time = jiffies; > + if (at24->use_smbus) { > + status = i2c_smbus_read_i2c_block_data(client, offset, > + count, buf); > + if (status == 0) > + status = count; I don't think this is needed. i2c_smbus_read_i2c_block_data() returns the number of bytes read, or a negative error code. I don't think it can ever return 0 (and if it did, it would not mean success.) Thus returning status without additional processing should be fine. > + } else { > + status = i2c_transfer(client->adapter, msg, 2); > + if (status == 2) > + status = count; > + } > + dev_dbg(&client->dev, "read %...@%d --> %zd (%ld)\n", > + count, offset, status, jiffies); > > - msg[1].addr = client->addr; > - msg[1].flags = I2C_M_RD; > - msg[1].buf = buf; > - msg[1].len = count; > + if (status == count) > + return count; > > - status = i2c_transfer(client->adapter, msg, 2); > - dev_dbg(&client->dev, "i2c read %...@%d --> %d\n", > - count, offset, status); > + /* REVISIT: at HZ=100, this is sloooow */ > + msleep(1); > + } while (time_before(read_time, timeout)); > > - if (status == 2) > - return count; > - else if (status >= 0) > - return -EIO; > - else > - return status; > + return -ETIMEDOUT; > } > > static ssize_t at24_read(struct at24_data *at24, Other than that this looks good. If the above change is OK with you then I can push this fix to Linus quickly. -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
