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

[Masahiro Yamada]

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;
}



Best Regards
Masahiro Yamada