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

[Masahiro Yamada]

Hi Simon,
Sorry for my late reply.

On Fri, 5 Dec 2014 07:41:50 -0700
Simon Glass <sjg at 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