Hi Simon, Sorry for my late reply.
On Fri, 5 Dec 2014 07:41:50 -0700 Simon Glass sjg@chromium.org wrote: n -EADDRNOTAVAIL;
I notice i2c_{read|write}_bytewise checks the return code of this function, but the normal one does not.
I think it seems a little bit strange.
Instead of the code above, can we put this here?
if (chip->offset_len > I2C_MAX_OFFSET_LEN) return -EADDRNOTAVAIL;And then, the normal i2c_{read|write} should also check the return code of this.
if (!chip->offset_len) return -EADDRNOTAVAIL;is specific to *_bytewise ones, so it can be moved to there.
We are not trying to check that the offset length is too large - that has already been done in ic_set_chip_offset_len(). I will add an assert and a comment. The purpose of the return value is to indicate whether an offset was added at all. In the case of a chip with a zero offset length, when doing a read, we want to omit the 'write' cycle entirely since there is no offset to write. The return value makes that possible.
I missed this. OK, I agreed that we should keep this as is.
offset_len = chip->offset_len;while (offset_len--)*offset_buf++ = offset >> (8 * offset_len);We should be very careful about this code because buffer overrun happens if too big offset_len is given.
So, we should make sure that offset_len is no larger than I2C_MAX_OFFSET_LEN. (Or, you can pass the length of offset_buf[] to this function.)
I know you implemented a similar check check in i2c_set_chip_offset_len() function.
But users can skip i2c_set_chip_offset_len() and directly invoke i2c_read() or i2c_write(). This is very dangerous.
If they skip it, then the value will be 1, the default. I've added an assert() but I don't really want to bloat the code which duplicate checks. Of course assert() is only useful for debugging.
You are right, thanks.
Best Regards Masahiro Yamada