On Mon, 2016-11-14 at 08:14 +0100, Cédric Le Goater wrote: > > Given the starting point of the tmp105 code the patch looks okay, but I > > was a bit thrown by the use of the 'len' member as what I'd consider an > > index. For instance we reset len to zero in tmp421_event() after > > populating buf, and then the data in buf is presumably sent out on a > > recv transaction which again starts incrementing len. len is also > > incremented when we don't interact with buf, e.g. when we instead > > assign to pointer. It feels like it could be prone to bugs, and > > 'cb5ef3fa1871 tmp105: Fix I2C protocol bug' suggests that might not be > > an unreasonable feeling. > > > > But given the code already exists in tmp105 maybe it's fine? > > So, I took my time to check this but yes, I think the code is fine.
Yes, from memory it was okay, just not as obvious as I'd hoped it to be. > > However, tmp421 does not need to support 2 bytes writes so we can > simplify tmp421_tx() : > > static int tmp421_tx(I2CSlave *i2c, uint8_t data) > { > TMP421State *s = TMP421(i2c); > > if (s->len == 0) { > /* first byte is the register pointer for a read or write > * operation */ > s->pointer = data; > s->len++; > } else if (s->len == 1) { > /* second byte is the data to write. The device only supports > * one byte writes */ > s->buf[0] = data; > tmp421_write(s); > } > > return 0; > } > > and tmp421 needs to support 2 bytes reads, so we need to extend a bit > tmp421_read() when the temperatures are are. Linux does not use it > so I guess we should use a command line tool to test. Okay. > > I will send an updated patch for the TMP42{1,2,3} device with a larger > patchset I am working on for Aspeed support. That is for 2.9. > Okay, I'll review again then. Thanks, Andrew > Thanks, > > C.
signature.asc
Description: This is a digitally signed message part