[U-Boot] [PATCH v3 01/10] dm: i2c: Add a uclass for I2C

[Simon Glass]

Hi Masahiro,

On 3 December 2014 at 21:36, Masahiro Yamada <yamada.m at 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 at 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