On Tue, 21 Sept 2021 at 10:41, Kevin Townsend <kevin.towns...@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.towns...@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
