Hi Masahiro,
On 3 December 2014 at 21:36, Masahiro Yamada <yamad...@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 <s...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
