[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/4] hw/misc: add a TMP42{1, 2, 3} device model
From: |
Andrew Jeffery |
Subject: |
Re: [Qemu-devel] [PATCH 2/4] hw/misc: add a TMP42{1, 2, 3} device model |
Date: |
Tue, 15 Nov 2016 11:39:11 +1030 |
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