On Mon, 27 Sept 2021 at 18:47, Kevin Townsend <kevin.towns...@linaro.org> wrote: > > Hi Peter, > > Thanks for the updated review. > > On Mon, 27 Sept 2021 at 18:39, Peter Maydell <peter.mayd...@linaro.org> wrote: >> >> I thought we'd agreed to implement the whole of the auto-increment >> logic, not just for specific registers ? > > > The problem I have here is ... how many bytes are we willing to buffer? > There's no > reason I can't request 512 registers, for example. Should we limit the buffer > length > to a single 'full' set of register values?
I think the underlying reason this seems awkward is the way this i2c device has been implemented as "at START_RECV create the whole buffer that we're going to read, and then for every call to the recv callback, read out one byte from that buffer". That's how the other hw/sensor/ i2c devices seem to have been written, and it's kinda OK if the device doesn't support reading more than one register at once, but it's a bit awkward if they can handle multiple-register big reads. (Also, I suspect a lot of those other sensors just copied the code structure from the original tmp105 implementation -- which is now 13 years old -- and maybe even for those devices it's not the best approach.) Anyway you don't have to write it that way: you can have the action at START_RECV be "capture the sensor readings" (AIUI this is what the h/w does, and it sets the LOCK bit here), and then the action on the recv callback is "return the right byte for the current address-pointer value, which might be a register value or might be the captured sensor data, and auto-increment the address-pointer". (It's not entirely clear to me from the datasheet when exactly the device captures and locks mag and temperature readings, and when it then lets go of that locked data and lets you read a fresh set, but https://electronics.stackexchange.com/a/265561 is a random person with some info suggesting that the values read are locked for the duration of the read transaction and not re-captured. If that's so then "capture once at START_RECV" would DTRT. The event for "end of transaction" is I2C_FINISH, if we need/want to do something at that point.) -- PMM
