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

[Philippe Mathieu-Daudé]

Hi Kevin,

On 30/1/22 10:50, Kevin Townsend 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. Sensor can be
> enabled with 'CONFIG_LSM303DLHC_MAG=y'.
>
> Signed-off-by: Kevin Townsend <kevin.townsend@linaro.org>
> ---
>   hw/sensor/Kconfig                 |   4 +
>   hw/sensor/lsm303dlhc_mag.c        | 556 ++++++++++++++++++++++++++++++
>   hw/sensor/meson.build             |   1 +
>   tests/qtest/lsm303dlhc-mag-test.c | 148 ++++++++
>   tests/qtest/meson.build           |   1 +
>   5 files changed, 710 insertions(+)
>   create mode 100644 hw/sensor/lsm303dlhc_mag.c
>   create mode 100644 tests/qtest/lsm303dlhc-mag-test.c

> +static void lsm303dlhc_mag_set_z(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);

Is it worth using qemu_log_mask(GUEST_ERROR) if gm == 0 here?

> +    if (!visit_type_int(v, name, &value, errp)) {
> +        return;
> +    }
> +
> +    reg = muldiv64(value, z_gain[gm], 100000);
> +
> +    /* Make sure we are within a 12-bit limit. */
> +    if (reg > 2047 || reg < -2048) {
> +        error_setg(errp, "value %" PRId64 " out of register's range", value);
> +        return;
> +    }
> +
> +    s->z = (int16_t)reg;
> +}
...

> +/*
> + * Put the device into post-reset default state.
> + */
> +static void lsm303dlhc_mag_default_cfg(LSM303DLHCMagState *s)
> +{
> +    /* Set the device into is default reset state. */
> +    s->len = 0;
> +    s->pointer = 0;         /* Current register. */
> +    s->buf = 0;             /* Shared buffer. */
> +    s->cra = 0x10;          /* Temp Enabled = 0, Data Rate = 15.0 Hz. */
> +    s->crb = 0x20;          /* Gain = +/- 1.3 Gauss. */
> +    s->mr = 0x3;            /* Operating Mode = Sleep. */
> +    s->x = 0;
> +    s->z = 0;
> +    s->y = 0;
> +    s->x_lock = 0;
> +    s->z_lock = 0;
> +    s->y_lock = 0;
> +    s->sr = 0x1;            /* DRDY = 1. */
> +    s->ira = 0x48;
> +    s->irb = 0x34;
> +    s->irc = 0x33;
> +    s->temperature = 0;     /* Default to 0 degrees C (0/8 lsb = 0 C). */
> +    s->temperature_lock = 0;
> +}
> +
> +/*
> + * Callback handler when DeviceState 'reset' is set to true.
> + */
> +static void lsm303dlhc_mag_reset(DeviceState *dev)
> +{
> +    I2CSlave *i2c = I2C_SLAVE(dev);
> +    LSM303DLHCMagState *s = LSM303DLHC_MAG(i2c);
> +
> +    /* Set the device into its default reset state. */
> +    lsm303dlhc_mag_default_cfg(s);

This follow what other I2C devices do, so OK. Now I2C devices often
are on the same machine board, but their nRESET line aren't always
wired to the CPU & co. logic. IOW the I2C chipsets are reset at board
cold power-on, but not at CPU hot reset; and a CPU soft reset could
check the device is in coherent state and keep reading sampled data.

Anyhow, just thinking loudly.

For this patch:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>