Hi Masahiro,
On 3 December 2014 at 21:36, Masahiro Yamada yamada.m@jp.panasonic.com wrote:
Hi Simon,
I am testing this series on my board and found some bugs.
On Mon, 24 Nov 2014 11:57:15 -0700 Simon Glass sjg@chromium.org wrote:
+static bool i2c_setup_offset(struct dm_i2c_chip *chip, uint offset,
uint8_t offset_buf[], struct i2c_msg *msg)+{
if (!chip->offset_len)return false;msg->addr = chip->chip_addr;msg->flags = chip->flags;msg->len = chip->offset_len;msg->buf = offset_buf;offset_buf[0] = offset;offset_buf[1] = offset >> 8;offset_buf[2] = offset >> 16;offset_buf[3] = offset >> 24;return true;+}
The i2c_setup_offset() function includes two bugs.
[1] Even if chip->offset_len is zero, it should not return immediately.
struct i2c_msg *msg has not been initialized. msg->addr and msg->len include unexpected values and they are passed to the driver. It results in an enexpected behavior.[2] The endian of offset_buf[] should be big endian, not little endian.
So, if I rewrote this function locally as follows, it is working file:
static bool i2c_setup_offset(struct dm_i2c_chip *chip, uint offset, uint8_t offset_buf[], struct i2c_msg *msg) { int offset_len;
msg->addr = chip->chip_addr; msg->flags = chip->flags; msg->len = chip->offset_len; msg->buf = offset_buf; offset_len = chip->offset_len; while(offset_len--) *offset_buf++ = offset >> (8 * offset_len); return true;}
Thanks. I am about half-way through finishing the unit tests and have found several other bugs. I never did get around to finishing the tests when I put the original patches together. But don't worry, I will not merge this until we have reasonable test coverage. I will add tests for your bugs also - I had not noticed the offset endian problem!
Regards, Simon