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

[Peter Maydell <peter.maydell@linaro.org>]

On Tue, 28 Sept 2021 at 11:36, Kevin Townsend <kevin.townsend@linaro.org> wrote:
>
> Hi Peter,
>
> 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 ?
>
>
> Thanks again for the feedback. Dealing with one register value at a time
> (versus a buffer of response values) does simplify the code flow.
>
> The following code appears to handle multi-byte reads correctly. I just
> wanted to confirm this is what you were looking for before moving on to
> the test code?
>
> /*
>  * Callback handler whenever a 'I2C_START_RECV' (read) event is received.
>  */
> static void lsm303dlhc_mag_read(LSM303DLHCMagState *s)
> {
>     /*
>      * Set the LOCK bit whenever a new read attempt is made. This will be
>      * cleared in I2C_FINISH. Note that DRDY is always set to 1 in this driver.
>      */
>     s->sr = 0x3;
> }
>
> /*
>  * Callback handler whenever a 'I2C_FINISH' event is received.
>  */
> static void lsm303dlhc_mag_finish(LSM303DLHCMagState *s)
> {
>     /*
>      * Clear the LOCK bit when the read attempt terminates.
>      * This bit is initially set in the I2C_START_RECV handler.
>      */
>     s->sr = 0x1;
> }

I would just inline these in the event lsm303dlhc_mag_event()
function. You might also #define some constants for the
register bits.

>
> /*
>  * Low-level slave-to-master transaction handler (read attempts).
>  */
> static uint8_t lsm303dlhc_mag_recv(I2CSlave *i2c)
> {
>     LSM303DLHCMagState *s = LSM303DLHC_MAG(i2c);
>
>     switch (s->pointer) {
>     case LSM303DLHC_MAG_REG_CRA:
>         s->buf = s->cra;
>         break;
>     case LSM303DLHC_MAG_REG_CRB:
>         s->buf = s->crb;
>         break;
>     case LSM303DLHC_MAG_REG_MR:
>         s->buf = s->mr;
>         break;
>     case LSM303DLHC_MAG_REG_OUT_X_H:
>         s->buf = (uint8_t)(s->x >> 8);
>         break;
>     case LSM303DLHC_MAG_REG_OUT_X_L:
>         s->buf = (uint8_t)(s->x);
>         break;
>     case LSM303DLHC_MAG_REG_OUT_Z_H:
>         s->buf = (uint8_t)(s->z >> 8);
>         break;
>     case LSM303DLHC_MAG_REG_OUT_Z_L:
>         s->buf = (uint8_t)(s->z);
>         break;
>     case LSM303DLHC_MAG_REG_OUT_Y_H:
>         s->buf = (uint8_t)(s->y >> 8);
>         break;
>     case LSM303DLHC_MAG_REG_OUT_Y_L:
>         s->buf = (uint8_t)(s->y);
>         break;
>     case LSM303DLHC_MAG_REG_SR:
>         s->buf = s->sr;
>         break;
>     case LSM303DLHC_MAG_REG_IRA:
>         s->buf = s->ira;
>         break;
>     case LSM303DLHC_MAG_REG_IRB:
>         s->buf = s->irb;
>         break;
>     case LSM303DLHC_MAG_REG_IRC:
>         s->buf = s->irc;
>         break;
>     case LSM303DLHC_MAG_REG_TEMP_OUT_H:
>         /* Check if the temperature sensor is enabled or not (CRA & 0x80). */
>         if (s->cra & 0x80) {
>             s->buf = (uint8_t)(s->temperature >> 8);
>         } else {
>             s->buf = 0;
>         }
>         break;
>     case LSM303DLHC_MAG_REG_TEMP_OUT_L:
>         if (s->cra & 0x80) {
>             s->buf = (uint8_t)(s->temperature & 0xf0);
>         } else {
>             s->buf = 0;
>         }
>         break;
>     default:
>         s->buf = 0;
>         break;
>     }
>
>     /*
>      * The address pointer on the LSM303DLHC auto-increments whenever a byte
>      * is read, without the master device having to request the next address.
>      *
>      * The auto-increment process has the following logic:
>      *
>      *   - if (s->pointer == 8) then s->pointer = 3
>      *   - else: if (s->pointer >= 12) then s->pointer = 0
>      *   - else: s->pointer += 1
>      *
>      * Reading an invalid address return 0.
>      */
>     if (s->pointer == LSM303DLHC_MAG_REG_OUT_Y_L) {
>         s->pointer = LSM303DLHC_MAG_REG_OUT_X_H;
>     } else if (s->pointer >= LSM303DLHC_MAG_REG_IRC) {
>         s->pointer = LSM303DLHC_MAG_REG_CRA;
>     } else {
>         s->pointer++;
>     }
>
>     return s->buf;

I think you don't need to write the value to s->buf, you can just
use a local variable and return that. Nothing should be able to read
the value back out of s->buf later. I think you should also implement
the actual lock part, to avoid wrong values in the case of
 * read starts, reads X_H
 * s->x updated via the QOM property setter
 * read continues, reads X_L
Basically just capture x,y,z,temp at the point of lock, and then
return those values in the recv function.

> }

thanks
-- PMM