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

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

On Tue, 21 Sept 2021 at 10:41, Kevin Townsend <kevin.townsend@linaro.org> wrote:
>
> This commit adds emulation of the magnetometer on the LSM303DLHC.
> It allows the magnetometer's X, Y and Z outputs to be set via the
> mag-x, mag-y and mag-z properties, as well as the 12-bit
> temperature output via the temperature property.

Thanks; this is generally looking pretty good. I have some review
commenst below.

> Signed-off-by: Kevin Townsend <kevin.townsend@linaro.org>
> ---
>  hw/sensor/Kconfig          |   4 +
>  hw/sensor/lsm303dlhc_mag.c | 754 +++++++++++++++++++++++++++++++++++++
>  hw/sensor/meson.build      |   1 +
>  3 files changed, 759 insertions(+)
>  create mode 100644 hw/sensor/lsm303dlhc_mag.c

> +static void lsm303dlhc_mag_get_x(Object *obj, Visitor *v, const char *name,
> +                                 void *opaque, Error **errp)
> +{
> +    LSM303DLHCMagState *s = LSM303DLHC_MAG(obj);
> +    int64_t value = s->x;
> +
> +    /* Convert to uT where 1000 = 1 uT. Conversion factor depends on gain. */
> +    value *= 1000000;
> +    switch (s->crb >> 5) {
> +        case 1:
> +            /* 11 lsb per uT. */
> +            value /= 11000;
> +            break;
> +        case 2:
> +            /* 8.55 lsb per uT. */
> +            value /= 8550;
> +            break;
> +        case 3:
> +            /* 6.70 lsb per uT. */
> +            value /= 6700;
> +            break;
> +        case 4:
> +            /* 4.50 lsb per uT. */
> +            value /= 4500;
> +            break;
> +        case 5:
> +            /* 4.00 lsb per uT. */
> +            value /= 4000;
> +            break;
> +        case 6:
> +            /* 3.30 lsb per uT. */
> +            value /= 3300;
> +            break;
> +        case 7:
> +            /* 2.30 lsb per uT. */
> +            value /= 2300;
> +            break;
> +        default:
> +            break;
> +    }

This gain conversion code is quite long-winded and duplicated
between the get_x and get_y functions. I think we could reduce it:

/*
 * Conversion factor from Gauss to sensor values for each GN gain setting,
 * in units "lsb per Gauss" (see data sheet table 3). There is no documented
 * behaviour if the GN setting in CRB is incorrectly set to 0b000;
 * we arbitrarily make it the same as 0b001.
 */
uint32_t xy_gain[] = { 1100, 1100, 855, 670, 450, 400, 330, 230 };
uint32_t z_gain[] = { 980, 980, 760, 600, 400, 355, 295, 205 };

static void lsm303dlhc_mag_get_x(Object *obj, Visitor *v, const char *name,
                                 void *opaque, Error **errp)
{
    LSM303DLHCMagState *s = LSM303DLHC_MAG(obj);
    int64_t value;
    int gm = extract32(s->crb, 5, 3);

    /* Convert to uT where 1000 = 1 uT. Conversion factor depends on gain. */
    int64_t value = muldiv64(s->x, 100000, xy_gain[gm]);
    visit_type_int(v, name, &value, errp);
}

static void lsm303dlhc_mag_set_x(Object *obj, Visitor *v, const char *name,
                                 void *opaque, Error **errp)
{
    LSM303DLHCMagState *s = LSM303DLHC_MAG(obj);
    int64_t value;
    int64_t reg;
    int gm = extract32(s->crb, 5, 3);

    if (!visit_type_int(v, name, &value, errp)) {
        return;
    }

    reg = muldiv64(value, xy_gain[gm], 100000);

    /* Make sure we are within a 12-bit limit. */
    if (reg > 2047 || reg < -2048) {
        error_setg(errp, "value %lld out of register's range", value);
        return;
    }

    s->x = (int16_t)reg;
}

Similarly for y and z (z uses z_gain[], obviously).

(muldiv64() is in "qemu/host-utils.h"; it avoids potential overflows
by calculating a * b / c with a higher-precision intermediate value;
we don't need that in the get but we do for the set, and it makes the
two functions clearly the inverse of each other to use it both places.)

> +/*
> + * Callback handler whenever a 'I2C_START_RECV' (read) event is received.
> + */
> +static void lsm303dlhc_mag_read(LSM303DLHCMagState *s)
> +{
> +    s->len = 0;
> +
> +    /*
> +     * 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.
> +     *
> +     * The auto-increment logic is only taken into account in this driver
> +     * for the LSM303DLHC_MAG_REG_OUT_* and LSM303DLHC_MAG_REG_TEMP_OUT_*
> +     * registers, which are the two common uses cases for it. Accessing either
> +     * of these register sets will also populate the rest of the related
> +     * dataset.
> +     */

I thought we'd agreed to implement the whole of the auto-increment
logic, not just for specific registers ?

Could I ask you to write a test case for this new device?
tests/qtest/tmp105-test.c is probably a good model to follow.
It doesn't have to be an exhaustive functionality test, but some
basic tests like:
 * if you set the sensor values via the qom properties and
   read them back do you get the same value you read?
 * if you set the values, change the gain, read back, ditto?
 * does reading the sensor values via the i2c registers
   give the right results?
would help in ensuring this doesn't accidentally regress in future.
(Make the test case a patch 2 in the patchset.)

thanks
-- PMM