Re: [Qemu-devel] [PATCH 2/4] hw/misc: add a TMP42{1, 2, 3} device model

[Andrew Jeffery]

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