Re: [PATCH v3] hw/sensor: Add lsm303dlhc magnetometer device

[Peter Maydell]

On Mon, 27 Sept 2021 at 18:47, Kevin Townsend <kevin.townsend@linaro.org> wrote:
>
> Hi Peter,
>
> Thanks for the updated review.
>
> On Mon, 27 Sept 2021 at 18:39, Peter Maydell <peter.maydell@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