(no subject)

[Kevin Townsend]

Updates the proposed LSM303DLHC magnetometer device following review by
Philippe Mathieu-Daudé.

This has been tested with Zephyr 2.6.0, as follows:

$ west build -p auto -b mps2_an521 \
  zephyr/samples/sensor/sensor_shell/ \
  -- -DCONFIG_I2C_SHELL=y

$ qemu-system-arm -M mps2-an521 -device loader,file=build/zephyr/zephyr.elf \
  -serial stdio \
  -monitor tcp:localhost:4444,server,nowait \
  -device lsm303dlhc_mag,id=lsm303,address=0x1E

uart:~$ i2c scan I2C_SHIELD1 
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:             -- -- -- -- -- -- -- -- -- -- -- -- 
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- 1e -- 
20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
70: -- -- -- -- -- -- -- --                         
1 devices found on I2C_SHIELD1

Zephyr's LSM303DLHC driver can be enabled in a sample by adding the following
DTS overlay:

/*
 * Copyright (c) 2021 Linaro Limited
 *
 * SPDX-License-Identifier: Apache-2.0
 */

&i2c_shield1 {
	lsm303dlhc-magn@1e {
		compatible = "st,lsm303dlhc-magn";
		reg = <0x1e>;
		label = "LSM303DLHC-MAGN";
	};
};

And the following KConfig settings:

CONFIG_I2C=y
CONFIG_I2C_SHELL=y
CONFIG_SENSOR=y
CONFIG_LSM303DLHC_MAGN=y

When a sample with the above settings is run, the magnetometer can be read
via the shell sensor command:

uart:~$ sensor get LSM303DLHC-MAGN magn_xyz
channel idx=11 magn_xyz x =   0.000000 y =   0.000000 z =   0.000000

Set the y-axis (via human monitor or qmp) to 1100, which equals 1 Gauss
with the default gain settings:

(qemu) qom-set lsm303 mag_y 1100
qom-set lsm303 mag_y 1100

And test again in Zephyr:

uart:~$ sensor get LSM303DLHC-MAGN magn_xyz
channel idx=11 magn_xyz x =   0.000000 y =   1.000000 z =   0.000000

[PATCH v2] hw/sensor: Add lsm303dlhc magnetometer device

[Kevin Townsend]

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.

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

diff --git a/hw/sensor/Kconfig b/hw/sensor/Kconfig
index a2b55a4fdb..f9d0177433 100644
--- a/hw/sensor/Kconfig
+++ b/hw/sensor/Kconfig
@@ -17,3 +17,7 @@ config ADM1272
 config MAX34451
     bool
     depends on I2C
+
+config LSM303DLHC_MAG
+    bool
+    depends on I2C
\ No newline at end of file
diff --git a/hw/sensor/lsm303dlhc_mag.c b/hw/sensor/lsm303dlhc_mag.c
new file mode 100644
index 0000000000..009b6dae6b
--- /dev/null
+++ b/hw/sensor/lsm303dlhc_mag.c
@@ -0,0 +1,502 @@
+/*
+ * LSM303DLHC I2C magnetometer.
+ *
+ * Copyright (C) 2021 Linaro Ltd.
+ * Written by Kevin Townsend <kevin.townsend@linaro.org>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+/*
+ * The I2C address associated with this device is set on the command-line when
+ * initialising the machine, but the following address is standard: 0x1E.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/i2c/i2c.h"
+#include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "qapi/visitor.h"
+#include "qemu/module.h"
+#include "qemu/log.h"
+#include "qemu/bswap.h"
+
+/* Property Names */
+#define LSM303DLHC_MSG_PROP_MAGX ("mag_x")
+#define LSM303DLHC_MSG_PROP_MAGY ("mag_y")
+#define LSM303DLHC_MSG_PROP_MAGZ ("mag_z")
+#define LSM303DLHC_MSG_PROP_TEMP ("temperature")
+
+enum LSM303DLHC_Mag_Reg {
+    LSM303DLHC_MAG_REG_CRA          = 0x00,
+    LSM303DLHC_MAG_REG_CRB          = 0x01,
+    LSM303DLHC_MAG_REG_MR           = 0x02,
+    LSM303DLHC_MAG_REG_OUT_X_H      = 0x03,
+    LSM303DLHC_MAG_REG_OUT_X_L      = 0x04,
+    LSM303DLHC_MAG_REG_OUT_Z_H      = 0x05,
+    LSM303DLHC_MAG_REG_OUT_Z_L      = 0x06,
+    LSM303DLHC_MAG_REG_OUT_Y_H      = 0x07,
+    LSM303DLHC_MAG_REG_OUT_Y_L      = 0x08,
+    LSM303DLHC_MAG_REG_SR           = 0x09,
+    LSM303DLHC_MAG_REG_IRA          = 0x0A,
+    LSM303DLHC_MAG_REG_IRB          = 0x0B,
+    LSM303DLHC_MAG_REG_IRC          = 0x0C,
+    LSM303DLHC_MAG_REG_TEMP_OUT_H   = 0x31,
+    LSM303DLHC_MAG_REG_TEMP_OUT_L   = 0x32
+};
+
+typedef struct LSM303DLHC_Mag_State {
+    I2CSlave parent_obj;
+
+    uint8_t cra;
+    uint8_t crb;
+    uint8_t mr;
+
+    /**
+     * @brief X-axis register value in LSB. Exact relationship to gauss
+     *        varies depending on the current gain settings.
+     */
+    int16_t x;
+
+    /**
+     * @brief Z-axis register value in LSB. Exact relationship to gauss
+     *        varies depending on the current gain settings.
+     */
+    int16_t z;
+
+    /**
+     * @brief Y-axis register value in LSB. Exact relationship to gauss
+     *        varies depending on the current gain settings.
+     */
+    int16_t y;
+
+    uint8_t sr;
+    uint8_t ira;
+    uint8_t irb;
+    uint8_t irc;
+
+    /**
+     * @brief Temperature in LSB where 1 degree C = 8 lsb.
+     */
+    int16_t temperature;
+
+    uint8_t len;
+    uint8_t buf[6];
+    uint8_t pointer;
+} LSM303DLHC_Mag_State;
+
+#define TYPE_LSM303DLHC_MAG "lsm303dlhc_mag"
+OBJECT_DECLARE_SIMPLE_TYPE(LSM303DLHC_Mag_State, LSM303DLHC_MAG)
+
+/**
+ * @brief Get handler for the mag_* property. This will be called
+ *        whenever the public 'mag_*' property is read, such as via
+ *        qom-get in the QEMU monitor.
+ */
+static void lsm303dlhc_mag_get_xyz(Object *obj, Visitor *v, const char *name,
+                                   void *opaque, Error **errp)
+{
+    LSM303DLHC_Mag_State *s = LSM303DLHC_MAG(obj);
+    int64_t value;
+
+    if (strcmp(name, LSM303DLHC_MSG_PROP_MAGX) == 0) {
+        value = s->x;
+    } else if (strcmp(name, LSM303DLHC_MSG_PROP_MAGY) == 0) {
+        value = s->y;
+    } else if (strcmp(name, LSM303DLHC_MSG_PROP_MAGZ) == 0) {
+        value = s->z;
+    } else {
+        error_setg(errp, "unknown property: %s", name);
+        return;
+    }
+
+    visit_type_int(v, name, &value, errp);
+}
+
+/**
+ * @brief Set handler for the mag_* property. This will be called
+ *        whenever the public 'mag_*' property is set, such as via
+ *        qom-set in the QEMU monitor.
+ */
+static void lsm303dlhc_mag_set_xyz(Object *obj, Visitor *v, const char *name,
+                                   void *opaque, Error **errp)
+{
+    LSM303DLHC_Mag_State *s = LSM303DLHC_MAG(obj);
+    int64_t value;
+
+    if (!visit_type_int(v, name, &value, errp)) {
+        return;
+    }
+
+    if (value > 2047 || value < -2048) {
+        error_setg(errp, "value %d lsb is out of range", value);
+        return;
+    }
+
+    if (strcmp(name, LSM303DLHC_MSG_PROP_MAGX) == 0) {
+        s->x = (int16_t)value;
+    } else if (strcmp(name, LSM303DLHC_MSG_PROP_MAGY) == 0) {
+        s->y = (int16_t)value;
+    } else if (strcmp(name, LSM303DLHC_MSG_PROP_MAGZ) == 0) {
+        s->z = (int16_t)value;
+    } else {
+        error_setg(errp, "unknown property: %s", name);
+        return;
+    }
+}
+
+/**
+ * @brief Get handler for the temperature property. This will be called
+ *        whenever the public 'temperature' property is read, such as via
+ *        qom-get in the QEMU monitor.
+ */
+static void lsm303dlhc_mag_get_temperature(Object *obj, Visitor *v,
+                                           const char *name, void *opaque,
+                                           Error **errp)
+{
+    LSM303DLHC_Mag_State *s = LSM303DLHC_MAG(obj);
+    int64_t value;
+
+    value = s->temperature;
+
+    visit_type_int(v, name, &value, errp);
+}
+
+/**
+ * @brief Set handler for the temperature property. This will be called
+ *        whenever the public 'temperature' property is set, such as via
+ *        qom-set in the QEMU monitor.
+ */
+static void lsm303dlhc_mag_set_temperature(Object *obj, Visitor *v,
+                                           const char *name, void *opaque,
+                                           Error **errp)
+{
+    LSM303DLHC_Mag_State *s = LSM303DLHC_MAG(obj);
+    int64_t value;
+
+    if (!visit_type_int(v, name, &value, errp)) {
+        return;
+    }
+
+    if (value > 2047 || value < -2048) {
+        error_setg(errp, "value %d lsb is out of range", value);
+        return;
+    }
+
+    s->temperature = (int16_t)value;
+}
+
+/**
+ * @brief Callback handler whenever a 'I2C_START_RECV' (read) event is received.
+ */
+static void lsm303dlhc_mag_read(LSM303DLHC_Mag_State *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_X_H and LSM303DLHC_MAG_REG_TEMP_OUT_H
+     * registers, which are the two common uses cases for it. Accessing either
+     * of these registers will also populate the rest of the related dataset.
+     */
+
+    switch (s->pointer) {
+    case LSM303DLHC_MAG_REG_CRA:
+        s->buf[s->len++] = s->cra;
+        break;
+    case LSM303DLHC_MAG_REG_CRB:
+        s->buf[s->len++] = s->crb;
+        break;
+    case LSM303DLHC_MAG_REG_MR:
+        s->buf[s->len++] = s->mr;
+        break;
+    case LSM303DLHC_MAG_REG_OUT_X_H:
+        stw_be_p(s->buf, s->x);
+        s->len += sizeof(s->x);
+        stw_be_p(s->buf + 2, s->z);
+        s->len += sizeof(s->z);
+        stw_be_p(s->buf + 4, s->y);
+        s->len += sizeof(s->y);
+        break;
+    case LSM303DLHC_MAG_REG_OUT_X_L:
+        s->buf[s->len++] = (uint8_t)(s->x);
+        break;
+    case LSM303DLHC_MAG_REG_OUT_Z_H:
+        s->buf[s->len++] = (uint8_t)(s->z >> 8);
+        break;
+    case LSM303DLHC_MAG_REG_OUT_Z_L:
+        s->buf[s->len++] = (uint8_t)(s->z);
+        break;
+    case LSM303DLHC_MAG_REG_OUT_Y_H:
+        s->buf[s->len++] = (uint8_t)(s->y >> 8);
+        break;
+    case LSM303DLHC_MAG_REG_OUT_Y_L:
+        s->buf[s->len++] = (uint8_t)(s->y);
+        break;
+    case LSM303DLHC_MAG_REG_SR:
+        s->buf[s->len++] = s->sr;
+        break;
+    case LSM303DLHC_MAG_REG_IRA:
+        s->buf[s->len++] = s->ira;
+        break;
+    case LSM303DLHC_MAG_REG_IRB:
+        s->buf[s->len++] = s->irb;
+        break;
+    case LSM303DLHC_MAG_REG_IRC:
+        s->buf[s->len++] = s->irc;
+        break;
+    case LSM303DLHC_MAG_REG_TEMP_OUT_H:
+        /* Check if the temperature sensor is enabled of not (CRA & 0x80). */
+        if (s->cra & 0x80) {
+            s->buf[s->len++] = (uint8_t)(s->temperature >> 8);
+            s->buf[s->len++] = (uint8_t)(s->temperature & 0xf0);
+        } else {
+            s->buf[s->len++] = 0;
+            s->buf[s->len++] = 0;
+        }
+        break;
+    case LSM303DLHC_MAG_REG_TEMP_OUT_L:
+        if (s->cra & 0x80) {
+            s->buf[s->len++] = (uint8_t)(s->temperature & 0xf0);
+        } else {
+            s->buf[s->len++] = 0;
+        }
+        break;
+    default:
+        s->buf[s->len++] = 0;
+        break;
+    }
+}
+
+/**
+ * @brief Callback handler when a device attempts to write to a register.
+ */
+static void lsm303dlhc_mag_write(LSM303DLHC_Mag_State *s)
+{
+    switch (s->pointer) {
+    case LSM303DLHC_MAG_REG_CRA:
+        s->cra = s->buf[0];
+        break;
+    case LSM303DLHC_MAG_REG_CRB:
+        s->crb = s->buf[0];
+        break;
+    case LSM303DLHC_MAG_REG_MR:
+        s->mr = s->buf[0];
+        break;
+    case LSM303DLHC_MAG_REG_SR:
+        s->sr = s->buf[0];
+        break;
+    case LSM303DLHC_MAG_REG_IRA:
+        s->ira = s->buf[0];
+        break;
+    case LSM303DLHC_MAG_REG_IRB:
+        s->irb = s->buf[0];
+        break;
+    case LSM303DLHC_MAG_REG_IRC:
+        s->irc = s->buf[0];
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "reg is read-only: 0x%02X", s->buf[0]);
+        break;
+    }
+}
+
+/**
+ * @brief Low-level slave-to-master transaction handler.
+ */
+static uint8_t lsm303dlhc_mag_recv(I2CSlave *i2c)
+{
+    LSM303DLHC_Mag_State *s = LSM303DLHC_MAG(i2c);
+
+    if (s->len < 6) {
+        return s->buf[s->len++];
+    } else {
+        return 0xff;
+    }
+}
+
+/**
+ * @brief Low-level master-to-slave transaction handler.
+ */
+static int lsm303dlhc_mag_send(I2CSlave *i2c, uint8_t data)
+{
+    LSM303DLHC_Mag_State *s = LSM303DLHC_MAG(i2c);
+
+    if (s->len == 0) {
+        /* First byte is the reg pointer */
+        s->pointer = data;
+        s->len++;
+    } else if (s->len == 1) {
+        /* Second byte is the new register value. */
+        s->buf[0] = data;
+        lsm303dlhc_mag_write(s);
+    } else {
+        g_assert_not_reached();
+    }
+
+    return 0;
+}
+
+/**
+ * @brief Bus state change handler.
+ */
+static int lsm303dlhc_mag_event(I2CSlave *i2c, enum i2c_event event)
+{
+    LSM303DLHC_Mag_State *s = LSM303DLHC_MAG(i2c);
+
+    switch (event) {
+    case I2C_START_SEND:
+        break;
+    case I2C_START_RECV:
+        lsm303dlhc_mag_read(s);
+        break;
+    case I2C_FINISH:
+        break;
+    case I2C_NACK:
+        break;
+    }
+
+    s->len = 0;
+    return 0;
+}
+
+/**
+ * @brief Device data description using VMSTATE macros.
+ */
+static const VMStateDescription vmstate_lsm303dlhc_mag = {
+    .name = "LSM303DLHC_MAG",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+
+        VMSTATE_I2C_SLAVE(parent_obj, LSM303DLHC_Mag_State),
+        VMSTATE_UINT8(len, LSM303DLHC_Mag_State),
+        VMSTATE_UINT8_ARRAY(buf, LSM303DLHC_Mag_State, 6),
+        VMSTATE_UINT8(pointer, LSM303DLHC_Mag_State),
+        VMSTATE_UINT8(cra, LSM303DLHC_Mag_State),
+        VMSTATE_UINT8(crb, LSM303DLHC_Mag_State),
+        VMSTATE_UINT8(mr, LSM303DLHC_Mag_State),
+        VMSTATE_INT16(x, LSM303DLHC_Mag_State),
+        VMSTATE_INT16(z, LSM303DLHC_Mag_State),
+        VMSTATE_INT16(y, LSM303DLHC_Mag_State),
+        VMSTATE_UINT8(sr, LSM303DLHC_Mag_State),
+        VMSTATE_UINT8(ira, LSM303DLHC_Mag_State),
+        VMSTATE_UINT8(irb, LSM303DLHC_Mag_State),
+        VMSTATE_UINT8(irc, LSM303DLHC_Mag_State),
+        VMSTATE_INT16(temperature, LSM303DLHC_Mag_State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+/**
+ * @brief Put the device into post-reset default state.
+ */
+static void lsm303dlhc_mag_default_cfg(I2CSlave *i2c)
+{
+	LSM303DLHC_Mag_State *s = LSM303DLHC_MAG(i2c);
+
+    /* Set the device into is default reset state. */
+    s->len = 0;
+    s->pointer = 0;         /* Current register. */
+    memset(s->buf, 0, sizeof(s->buf));
+    s->cra = 0x08;          /* Temp Enabled = 0, Data Rate = 3.0 Hz. */
+    s->crb = 0x20;          /* Gain = +/- 1.3 Guas. */
+    s->mr = 0x1;            /* Operating Mode = Single conversion. */
+    s->x = 0;
+    s->z = 0;
+    s->y = 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). */
+}
+
+/**
+ * @brief Callback handler when DeviceState 'reset' is set to true.
+ */
+static void lsm303dlhc_mag_reset(DeviceState *dev)
+{
+    I2CSlave *i2c = I2C_SLAVE(dev);
+    LSM303DLHC_Mag_State *s = LSM303DLHC_MAG(i2c);
+
+	/* Set the device into is default reset state. */
+	lsm303dlhc_mag_default_cfg(&s->parent_obj);
+}
+
+/**
+ * @brief Initialisation of any public properties.
+ *
+ * @note Properties are an object's external interface, and are set before the
+ *       object is started. The 'temperature' property here enables the
+ *       temperature registers to be set by the host OS, for example, or via
+ *       the QEMU monitor interface using commands like 'qom-set' and 'qom-get'.
+ */
+static void lsm303dlhc_mag_initfn(Object *obj)
+{
+    object_property_add(obj, LSM303DLHC_MSG_PROP_MAGX, "int",
+                lsm303dlhc_mag_get_xyz,
+                lsm303dlhc_mag_set_xyz, NULL, NULL);
+
+    object_property_add(obj, LSM303DLHC_MSG_PROP_MAGY, "int",
+                lsm303dlhc_mag_get_xyz,
+                lsm303dlhc_mag_set_xyz, NULL, NULL);
+
+    object_property_add(obj, LSM303DLHC_MSG_PROP_MAGZ, "int",
+                lsm303dlhc_mag_get_xyz,
+                lsm303dlhc_mag_set_xyz, NULL, NULL);
+
+    object_property_add(obj, LSM303DLHC_MSG_PROP_TEMP, "int",
+                lsm303dlhc_mag_get_temperature,
+                lsm303dlhc_mag_set_temperature, NULL, NULL);
+}
+
+/**
+ * @brief Set the virtual method pointers (bus state change, tx/rx, etc.).
+ */
+static void lsm303dlhc_mag_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
+
+    /* DeviceState 'reset' handler. */
+    dc->reset = lsm303dlhc_mag_reset;
+
+    /* VM State description (device data). */
+    dc->vmsd = &vmstate_lsm303dlhc_mag;
+
+    /* Bus state change handler. */
+    k->event = lsm303dlhc_mag_event;
+
+    /* Slave to master handler. */
+    k->recv = lsm303dlhc_mag_recv;
+
+    /* Master to slave handler. */
+    k->send = lsm303dlhc_mag_send;
+}
+
+static const TypeInfo lsm303dlhc_mag_info = {
+    .name = TYPE_LSM303DLHC_MAG,
+    .parent = TYPE_I2C_SLAVE,
+    .instance_size = sizeof(LSM303DLHC_Mag_State),
+    .instance_init = lsm303dlhc_mag_initfn,
+    .class_init = lsm303dlhc_mag_class_init,
+};
+
+static void lsm303dlhc_mag_register_types(void)
+{
+    type_register_static(&lsm303dlhc_mag_info);
+}
+
+type_init(lsm303dlhc_mag_register_types)
diff --git a/hw/sensor/meson.build b/hw/sensor/meson.build
index 034e3e0207..95406abd24 100644
--- a/hw/sensor/meson.build
+++ b/hw/sensor/meson.build
@@ -3,3 +3,4 @@ softmmu_ss.add(when: 'CONFIG_TMP421', if_true: files('tmp421.c'))
 softmmu_ss.add(when: 'CONFIG_EMC141X', if_true: files('emc141x.c'))
 softmmu_ss.add(when: 'CONFIG_ADM1272', if_true: files('adm1272.c'))
 softmmu_ss.add(when: 'CONFIG_MAX34451', if_true: files('max34451.c'))
+softmmu_ss.add(when: 'CONFIG_LSM303DLHC_MAG', if_true: files('lsm303dlhc_mag.c'))
-- 
2.30.1 (Apple Git-130)

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

[Peter Maydell]

On Sun, 15 Aug 2021 at 17:31, 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.
>
> Signed-off-by: Kevin Townsend <kevin.townsend@linaro.org>

Hi; thanks for this patch. I have some code review comments below.

> ---
>  hw/sensor/Kconfig          |   4 +
>  hw/sensor/lsm303dlhc_mag.c | 502 +++++++++++++++++++++++++++++++++++++
>  hw/sensor/meson.build      |   1 +
>  3 files changed, 507 insertions(+)
>  create mode 100644 hw/sensor/lsm303dlhc_mag.c
>
> diff --git a/hw/sensor/Kconfig b/hw/sensor/Kconfig
> index a2b55a4fdb..f9d0177433 100644
> --- a/hw/sensor/Kconfig
> +++ b/hw/sensor/Kconfig
> @@ -17,3 +17,7 @@ config ADM1272
>  config MAX34451
>      bool
>      depends on I2C
> +
> +config LSM303DLHC_MAG
> +    bool
> +    depends on I2C
> \ No newline at end of file

Should have the newline.

> diff --git a/hw/sensor/lsm303dlhc_mag.c b/hw/sensor/lsm303dlhc_mag.c
> new file mode 100644
> index 0000000000..009b6dae6b
> --- /dev/null
> +++ b/hw/sensor/lsm303dlhc_mag.c
> @@ -0,0 +1,502 @@
> +/*
> + * LSM303DLHC I2C magnetometer.
> + *
> + * Copyright (C) 2021 Linaro Ltd.
> + * Written by Kevin Townsend <kevin.townsend@linaro.org>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later

You could add the URL of the datasheet here:
https://www.st.com/resource/en/datasheet/lsm303dlhc.pdf

> + */
> +
> +/*
> + * The I2C address associated with this device is set on the command-line when
> + * initialising the machine, but the following address is standard: 0x1E.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/i2c/i2c.h"
> +#include "migration/vmstate.h"
> +#include "qapi/error.h"
> +#include "qapi/visitor.h"
> +#include "qemu/module.h"
> +#include "qemu/log.h"
> +#include "qemu/bswap.h"
> +
> +/* Property Names */
> +#define LSM303DLHC_MSG_PROP_MAGX ("mag_x")
> +#define LSM303DLHC_MSG_PROP_MAGY ("mag_y")
> +#define LSM303DLHC_MSG_PROP_MAGZ ("mag_z")
> +#define LSM303DLHC_MSG_PROP_TEMP ("temperature")

Is this kind of #define for property names something you've borrowed
from elsewhere in the codebase?

> +enum LSM303DLHC_Mag_Reg {

Our naming convention doesn't have underscores in
type names generally; we prefer CamelCase for those.
(Some older code uses different style.)

> +    LSM303DLHC_MAG_REG_CRA          = 0x00,
> +    LSM303DLHC_MAG_REG_CRB          = 0x01,
> +    LSM303DLHC_MAG_REG_MR           = 0x02,
> +    LSM303DLHC_MAG_REG_OUT_X_H      = 0x03,
> +    LSM303DLHC_MAG_REG_OUT_X_L      = 0x04,
> +    LSM303DLHC_MAG_REG_OUT_Z_H      = 0x05,
> +    LSM303DLHC_MAG_REG_OUT_Z_L      = 0x06,
> +    LSM303DLHC_MAG_REG_OUT_Y_H      = 0x07,
> +    LSM303DLHC_MAG_REG_OUT_Y_L      = 0x08,
> +    LSM303DLHC_MAG_REG_SR           = 0x09,
> +    LSM303DLHC_MAG_REG_IRA          = 0x0A,
> +    LSM303DLHC_MAG_REG_IRB          = 0x0B,
> +    LSM303DLHC_MAG_REG_IRC          = 0x0C,
> +    LSM303DLHC_MAG_REG_TEMP_OUT_H   = 0x31,
> +    LSM303DLHC_MAG_REG_TEMP_OUT_L   = 0x32
> +};

So if I'm reading the datasheet correctly, the LM303DLHC is
really two completely distinct i2c devices in a single
package with different slave addresses; this QEMU device
implements only the magnetometer i2c device, and if we wanted
to add the accelerometer device we'd implement that as a
second separate QEMU i2c device ?

> +
> +typedef struct LSM303DLHC_Mag_State {

Again, CamelCase.

> +    I2CSlave parent_obj;
> +
> +    uint8_t cra;
> +    uint8_t crb;
> +    uint8_t mr;
> +
> +    /**
> +     * @brief X-axis register value in LSB. Exact relationship to gauss
> +     *        varies depending on the current gain settings.
> +     */

This @brief stuff isn't how we do doc comments. For a simple
internal to the device struct like this you don't really need
to mark it up as a doc comment at all; just use a normal
block comment (you could combine these 3 comments, since they're
saying the same thing).

> +    int16_t x;
> +
> +    /**
> +     * @brief Z-axis register value in LSB. Exact relationship to gauss
> +     *        varies depending on the current gain settings.
> +     */
> +    int16_t z;
> +
> +    /**
> +     * @brief Y-axis register value in LSB. Exact relationship to gauss
> +     *        varies depending on the current gain settings.
> +     */
> +    int16_t y;
> +
> +    uint8_t sr;
> +    uint8_t ira;
> +    uint8_t irb;
> +    uint8_t irc;
> +
> +    /**
> +     * @brief Temperature in LSB where 1 degree C = 8 lsb.
> +     */
> +    int16_t temperature;
> +
> +    uint8_t len;
> +    uint8_t buf[6];
> +    uint8_t pointer;
> +} LSM303DLHC_Mag_State;
> +
> +#define TYPE_LSM303DLHC_MAG "lsm303dlhc_mag"
> +OBJECT_DECLARE_SIMPLE_TYPE(LSM303DLHC_Mag_State, LSM303DLHC_MAG)
> +
> +/**
> + * @brief Get handler for the mag_* property. This will be called
> + *        whenever the public 'mag_*' property is read, such as via
> + *        qom-get in the QEMU monitor.
> + */

For a file-internal static function, doc comment formatting is optional
(we only really want it for functions declared in header files).
I would just provide a normal comment here.
(If you do want a doc-comment, QEMU's format for that is not this
@brief style; bitops.h has some examples of what we do, which is
more or less Linux kernel kerneldoc format.)

> +static void lsm303dlhc_mag_get_xyz(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)
> +{
> +    LSM303DLHC_Mag_State *s = LSM303DLHC_MAG(obj);
> +    int64_t value;
> +
> +    if (strcmp(name, LSM303DLHC_MSG_PROP_MAGX) == 0) {
> +        value = s->x;
> +    } else if (strcmp(name, LSM303DLHC_MSG_PROP_MAGY) == 0) {
> +        value = s->y;
> +    } else if (strcmp(name, LSM303DLHC_MSG_PROP_MAGZ) == 0) {
> +        value = s->z;
> +    } else {
> +        error_setg(errp, "unknown property: %s", name);
> +        return;
> +    }
> +
> +    visit_type_int(v, name, &value, errp);
> +}

I think you'd be better off defining different get/set functions
for x,y,z rather than sharing them and then having to do a bunch
of strcmps on the property name to split them apart again.

> +
> +/**
> + * @brief Set handler for the mag_* property. This will be called
> + *        whenever the public 'mag_*' property is set, such as via
> + *        qom-set in the QEMU monitor.
> + */
> +static void lsm303dlhc_mag_set_xyz(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)
> +{
> +    LSM303DLHC_Mag_State *s = LSM303DLHC_MAG(obj);
> +    int64_t value;
> +
> +    if (!visit_type_int(v, name, &value, errp)) {
> +        return;
> +    }
> +
> +    if (value > 2047 || value < -2048) {
> +        error_setg(errp, "value %d lsb is out of range", value);

Why "lsb" ?

> +        return;
> +    }
> +
> +    if (strcmp(name, LSM303DLHC_MSG_PROP_MAGX) == 0) {
> +        s->x = (int16_t)value;
> +    } else if (strcmp(name, LSM303DLHC_MSG_PROP_MAGY) == 0) {
> +        s->y = (int16_t)value;
> +    } else if (strcmp(name, LSM303DLHC_MSG_PROP_MAGZ) == 0) {
> +        s->z = (int16_t)value;
> +    } else {
> +        error_setg(errp, "unknown property: %s", name);
> +        return;
> +    }
> +}
> +
> +/**
> + * @brief Get handler for the temperature property. This will be called
> + *        whenever the public 'temperature' property is read, such as via
> + *        qom-get in the QEMU monitor.
> + */
> +static void lsm303dlhc_mag_get_temperature(Object *obj, Visitor *v,
> +                                           const char *name, void *opaque,
> +                                           Error **errp)
> +{
> +    LSM303DLHC_Mag_State *s = LSM303DLHC_MAG(obj);
> +    int64_t value;
> +
> +    value = s->temperature;
> +
> +    visit_type_int(v, name, &value, errp);
> +}
> +
> +/**
> + * @brief Set handler for the temperature property. This will be called
> + *        whenever the public 'temperature' property is set, such as via
> + *        qom-set in the QEMU monitor.
> + */
> +static void lsm303dlhc_mag_set_temperature(Object *obj, Visitor *v,
> +                                           const char *name, void *opaque,
> +                                           Error **errp)
> +{
> +    LSM303DLHC_Mag_State *s = LSM303DLHC_MAG(obj);
> +    int64_t value;
> +
> +    if (!visit_type_int(v, name, &value, errp)) {
> +        return;
> +    }
> +
> +    if (value > 2047 || value < -2048) {
> +        error_setg(errp, "value %d lsb is out of range", value);
> +        return;
> +    }
> +
> +    s->temperature = (int16_t)value;
> +}
> +
> +/**
> + * @brief Callback handler whenever a 'I2C_START_RECV' (read) event is received.
> + */
> +static void lsm303dlhc_mag_read(LSM303DLHC_Mag_State *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_X_H and LSM303DLHC_MAG_REG_TEMP_OUT_H
> +     * registers, which are the two common uses cases for it. Accessing either
> +     * of these registers will also populate the rest of the related dataset.

How hard would it be to implement the behaviour correctly for all cases?
I find it's usually better to model something correctly from the start:
usually the person writing the code knows the h/w behaviour better than
anybody else coming along later trying to figure out why some other
guest code doesn't work on the model...

> +     */
> +
> +    switch (s->pointer) {
> +    case LSM303DLHC_MAG_REG_CRA:
> +        s->buf[s->len++] = s->cra;
> +        break;
> +    case LSM303DLHC_MAG_REG_CRB:
> +        s->buf[s->len++] = s->crb;
> +        break;
> +    case LSM303DLHC_MAG_REG_MR:
> +        s->buf[s->len++] = s->mr;
> +        break;
> +    case LSM303DLHC_MAG_REG_OUT_X_H:
> +        stw_be_p(s->buf, s->x);
> +        s->len += sizeof(s->x);
> +        stw_be_p(s->buf + 2, s->z);
> +        s->len += sizeof(s->z);
> +        stw_be_p(s->buf + 4, s->y);
> +        s->len += sizeof(s->y);
> +        break;
> +    case LSM303DLHC_MAG_REG_OUT_X_L:
> +        s->buf[s->len++] = (uint8_t)(s->x);
> +        break;
> +    case LSM303DLHC_MAG_REG_OUT_Z_H:
> +        s->buf[s->len++] = (uint8_t)(s->z >> 8);
> +        break;
> +    case LSM303DLHC_MAG_REG_OUT_Z_L:
> +        s->buf[s->len++] = (uint8_t)(s->z);
> +        break;
> +    case LSM303DLHC_MAG_REG_OUT_Y_H:
> +        s->buf[s->len++] = (uint8_t)(s->y >> 8);
> +        break;
> +    case LSM303DLHC_MAG_REG_OUT_Y_L:
> +        s->buf[s->len++] = (uint8_t)(s->y);
> +        break;
> +    case LSM303DLHC_MAG_REG_SR:
> +        s->buf[s->len++] = s->sr;
> +        break;
> +    case LSM303DLHC_MAG_REG_IRA:
> +        s->buf[s->len++] = s->ira;
> +        break;
> +    case LSM303DLHC_MAG_REG_IRB:
> +        s->buf[s->len++] = s->irb;
> +        break;
> +    case LSM303DLHC_MAG_REG_IRC:
> +        s->buf[s->len++] = s->irc;
> +        break;
> +    case LSM303DLHC_MAG_REG_TEMP_OUT_H:
> +        /* Check if the temperature sensor is enabled of not (CRA & 0x80). */

"or not"

> +        if (s->cra & 0x80) {
> +            s->buf[s->len++] = (uint8_t)(s->temperature >> 8);
> +            s->buf[s->len++] = (uint8_t)(s->temperature & 0xf0);
> +        } else {
> +            s->buf[s->len++] = 0;
> +            s->buf[s->len++] = 0;
> +        }
> +        break;
> +    case LSM303DLHC_MAG_REG_TEMP_OUT_L:
> +        if (s->cra & 0x80) {
> +            s->buf[s->len++] = (uint8_t)(s->temperature & 0xf0);
> +        } else {
> +            s->buf[s->len++] = 0;
> +        }
> +        break;
> +    default:
> +        s->buf[s->len++] = 0;
> +        break;
> +    }
> +}
> +
> +/**
> + * @brief Callback handler when a device attempts to write to a register.
> + */
> +static void lsm303dlhc_mag_write(LSM303DLHC_Mag_State *s)
> +{
> +    switch (s->pointer) {
> +    case LSM303DLHC_MAG_REG_CRA:
> +        s->cra = s->buf[0];
> +        break;
> +    case LSM303DLHC_MAG_REG_CRB:
> +        s->crb = s->buf[0];
> +        break;
> +    case LSM303DLHC_MAG_REG_MR:
> +        s->mr = s->buf[0];
> +        break;
> +    case LSM303DLHC_MAG_REG_SR:
> +        s->sr = s->buf[0];
> +        break;
> +    case LSM303DLHC_MAG_REG_IRA:
> +        s->ira = s->buf[0];
> +        break;
> +    case LSM303DLHC_MAG_REG_IRB:
> +        s->irb = s->buf[0];
> +        break;
> +    case LSM303DLHC_MAG_REG_IRC:
> +        s->irc = s->buf[0];
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "reg is read-only: 0x%02X", s->buf[0]);
> +        break;
> +    }
> +}
> +
> +/**
> + * @brief Low-level slave-to-master transaction handler.
> + */
> +static uint8_t lsm303dlhc_mag_recv(I2CSlave *i2c)
> +{
> +    LSM303DLHC_Mag_State *s = LSM303DLHC_MAG(i2c);
> +
> +    if (s->len < 6) {
> +        return s->buf[s->len++];
> +    } else {
> +        return 0xff;
> +    }
> +}
> +
> +/**
> + * @brief Low-level master-to-slave transaction handler.
> + */
> +static int lsm303dlhc_mag_send(I2CSlave *i2c, uint8_t data)
> +{
> +    LSM303DLHC_Mag_State *s = LSM303DLHC_MAG(i2c);
> +
> +    if (s->len == 0) {
> +        /* First byte is the reg pointer */
> +        s->pointer = data;
> +        s->len++;
> +    } else if (s->len == 1) {
> +        /* Second byte is the new register value. */
> +        s->buf[0] = data;
> +        lsm303dlhc_mag_write(s);
> +    } else {
> +        g_assert_not_reached();
> +    }
> +
> +    return 0;
> +}
> +
> +/**
> + * @brief Bus state change handler.
> + */
> +static int lsm303dlhc_mag_event(I2CSlave *i2c, enum i2c_event event)
> +{
> +    LSM303DLHC_Mag_State *s = LSM303DLHC_MAG(i2c);
> +
> +    switch (event) {
> +    case I2C_START_SEND:
> +        break;
> +    case I2C_START_RECV:
> +        lsm303dlhc_mag_read(s);
> +        break;
> +    case I2C_FINISH:
> +        break;
> +    case I2C_NACK:
> +        break;
> +    }
> +
> +    s->len = 0;
> +    return 0;
> +}
> +
> +/**
> + * @brief Device data description using VMSTATE macros.
> + */
> +static const VMStateDescription vmstate_lsm303dlhc_mag = {
> +    .name = "LSM303DLHC_MAG",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .fields = (VMStateField[]) {
> +
> +        VMSTATE_I2C_SLAVE(parent_obj, LSM303DLHC_Mag_State),
> +        VMSTATE_UINT8(len, LSM303DLHC_Mag_State),
> +        VMSTATE_UINT8_ARRAY(buf, LSM303DLHC_Mag_State, 6),
> +        VMSTATE_UINT8(pointer, LSM303DLHC_Mag_State),
> +        VMSTATE_UINT8(cra, LSM303DLHC_Mag_State),
> +        VMSTATE_UINT8(crb, LSM303DLHC_Mag_State),
> +        VMSTATE_UINT8(mr, LSM303DLHC_Mag_State),
> +        VMSTATE_INT16(x, LSM303DLHC_Mag_State),
> +        VMSTATE_INT16(z, LSM303DLHC_Mag_State),
> +        VMSTATE_INT16(y, LSM303DLHC_Mag_State),
> +        VMSTATE_UINT8(sr, LSM303DLHC_Mag_State),
> +        VMSTATE_UINT8(ira, LSM303DLHC_Mag_State),
> +        VMSTATE_UINT8(irb, LSM303DLHC_Mag_State),
> +        VMSTATE_UINT8(irc, LSM303DLHC_Mag_State),
> +        VMSTATE_INT16(temperature, LSM303DLHC_Mag_State),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +/**
> + * @brief Put the device into post-reset default state.
> + */
> +static void lsm303dlhc_mag_default_cfg(I2CSlave *i2c)
> +{
> +       LSM303DLHC_Mag_State *s = LSM303DLHC_MAG(i2c);
> +
> +    /* Set the device into is default reset state. */
> +    s->len = 0;
> +    s->pointer = 0;         /* Current register. */
> +    memset(s->buf, 0, sizeof(s->buf));
> +    s->cra = 0x08;          /* Temp Enabled = 0, Data Rate = 3.0 Hz. */
> +    s->crb = 0x20;          /* Gain = +/- 1.3 Guas. */

"gauss" ?

> +    s->mr = 0x1;            /* Operating Mode = Single conversion. */
> +    s->x = 0;
> +    s->z = 0;
> +    s->y = 0;
> +    s->sr = 0x1;            /* DRDY = 1. */

Do you understand how the DRDY and LOCK bits work ? The datasheet
is unclear. Also, what's the difference between "single-conversion"
and "continuous-conversion" modes ?

> +    s->ira = 0x48;
> +    s->irb = 0x34;
> +    s->irc = 0x33;
> +    s->temperature = 0;     /* Default to 0 degrees C (0/8 lsb = 0 C). */
> +}
> +
> +/**
> + * @brief Callback handler when DeviceState 'reset' is set to true.
> + */
> +static void lsm303dlhc_mag_reset(DeviceState *dev)
> +{
> +    I2CSlave *i2c = I2C_SLAVE(dev);
> +    LSM303DLHC_Mag_State *s = LSM303DLHC_MAG(i2c);
> +
> +       /* Set the device into is default reset state. */
> +       lsm303dlhc_mag_default_cfg(&s->parent_obj);

Misindentation.

Also, don't use the parent_obj field;
always use the QOM cast macro when you need the pointer
to something as a different type. In this case you already
have the I2CSlave*, in 'i2c'. But better would be to make
lsm303dlhc_mag_default_cfg() take a LSM303DLHC_Mag_State*
directly rather than taking an I2CSlave* and casting it
internally.

> +}
> +
> +/**
> + * @brief Initialisation of any public properties.
> + *
> + * @note Properties are an object's external interface, and are set before the
> + *       object is started. The 'temperature' property here enables the
> + *       temperature registers to be set by the host OS, for example, or via
> + *       the QEMU monitor interface using commands like 'qom-set' and 'qom-get'.
> + */

You don't need to say that kind of thing, you can assume the reader
knows what properties are.

> +static void lsm303dlhc_mag_initfn(Object *obj)
> +{
> +    object_property_add(obj, LSM303DLHC_MSG_PROP_MAGX, "int",
> +                lsm303dlhc_mag_get_xyz,
> +                lsm303dlhc_mag_set_xyz, NULL, NULL);
> +
> +    object_property_add(obj, LSM303DLHC_MSG_PROP_MAGY, "int",
> +                lsm303dlhc_mag_get_xyz,
> +                lsm303dlhc_mag_set_xyz, NULL, NULL);
> +
> +    object_property_add(obj, LSM303DLHC_MSG_PROP_MAGZ, "int",
> +                lsm303dlhc_mag_get_xyz,
> +                lsm303dlhc_mag_set_xyz, NULL, NULL);

What units are these in? It looks like your implementation just
uses the property values as the raw -2048..+2048 value that the
X/Y/Z registers read as. Would it be better for the properties to
set the value in Gauss, and then the model to honour the
gain settings in CRB_REG_M.GN{0,1,2} ?  That way guest code that
adjusts the gain will get the results it is expecting.

> +
> +    object_property_add(obj, LSM303DLHC_MSG_PROP_TEMP, "int",
> +                lsm303dlhc_mag_get_temperature,
> +                lsm303dlhc_mag_set_temperature, NULL, NULL);

What units is this in?

> +}
> +
> +/**
> + * @brief Set the virtual method pointers (bus state change, tx/rx, etc.).
> + */
> +static void lsm303dlhc_mag_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
> +
> +    /* DeviceState 'reset' handler. */
> +    dc->reset = lsm303dlhc_mag_reset;
> +
> +    /* VM State description (device data). */
> +    dc->vmsd = &vmstate_lsm303dlhc_mag;
> +
> +    /* Bus state change handler. */
> +    k->event = lsm303dlhc_mag_event;
> +
> +    /* Slave to master handler. */
> +    k->recv = lsm303dlhc_mag_recv;
> +
> +    /* Master to slave handler. */
> +    k->send = lsm303dlhc_mag_send;

These comments are all kind of superfluous too.

> +}
> +
> +static const TypeInfo lsm303dlhc_mag_info = {
> +    .name = TYPE_LSM303DLHC_MAG,
> +    .parent = TYPE_I2C_SLAVE,
> +    .instance_size = sizeof(LSM303DLHC_Mag_State),
> +    .instance_init = lsm303dlhc_mag_initfn,
> +    .class_init = lsm303dlhc_mag_class_init,
> +};
> +
> +static void lsm303dlhc_mag_register_types(void)
> +{
> +    type_register_static(&lsm303dlhc_mag_info);
> +}
> +
> +type_init(lsm303dlhc_mag_register_types)
> diff --git a/hw/sensor/meson.build b/hw/sensor/meson.build
> index 034e3e0207..95406abd24 100644
> --- a/hw/sensor/meson.build
> +++ b/hw/sensor/meson.build
> @@ -3,3 +3,4 @@ softmmu_ss.add(when: 'CONFIG_TMP421', if_true: files('tmp421.c'))
>  softmmu_ss.add(when: 'CONFIG_EMC141X', if_true: files('emc141x.c'))
>  softmmu_ss.add(when: 'CONFIG_ADM1272', if_true: files('adm1272.c'))
>  softmmu_ss.add(when: 'CONFIG_MAX34451', if_true: files('max34451.c'))
> +softmmu_ss.add(when: 'CONFIG_LSM303DLHC_MAG', if_true: files('lsm303dlhc_mag.c'))

You need also a stanza in hw/sensor/Kconfig, like this:

config LSM303DLHC_MAG
    bool
    depends on I2C

Also, unless the Kconfig for some board does 'select LSM303DLHC_MAG'
I don't think this code will ever be compiled in...

thanks
-- PMM

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

[Kevin Townsend]

[-- Attachment #1: Type: text/plain, Size: 7644 bytes --]

Hi Peter,

Thanks for the review, and sorry for the slow reply, just getting back from
holidays myself.

On Thu, 26 Aug 2021 at 17:39, Peter Maydell <peter.maydell@linaro.org>
wrote:

>
> So if I'm reading the datasheet correctly, the LM303DLHC is
> really two completely distinct i2c devices in a single
> package with different slave addresses; this QEMU device
> implements only the magnetometer i2c device, and if we wanted
> to add the accelerometer device we'd implement that as a
> second separate QEMU i2c device ?
>

This is correct. There are two distinct dies in the chip with separate I2C
addresses, etc.,
and this should probably be modelled separately. I chose the magnetometer
since it's
a far simpler device to model than the accelrometer, but still solves the
need for a
more complex I2C sensor that can be used in testing with the I2C bus.

> +    if (value > 2047 || value < -2048) {
> > +        error_setg(errp, "value %d lsb is out of range", value);
>
> Why "lsb" ?
>
>
In my head, using LSB seemed more precise since I know exactly what value
will
be set to the registers, and exactly what I will get back when reading
versus passing
in a float that's needs to be conveted as a 'best-fit' scenario in 0.125°C
steps?

> +     * The auto-increment logic is only taken into account in this driver
> > +     * for the LSM303DLHC_MAG_REG_OUT_X_H and
> LSM303DLHC_MAG_REG_TEMP_OUT_H
> > +     * registers, which are the two common uses cases for it. Accessing
> either
> > +     * of these registers will also populate the rest of the related
> dataset.
>
> How hard would it be to implement the behaviour correctly for all cases?
> I find it's usually better to model something correctly from the start:
> usually the person writing the code knows the h/w behaviour better than
> anybody else coming along later trying to figure out why some other
> guest code doesn't work on the model...
>

I can update this to also take auto-increment into account when you don't
start at
the first record (X-axis), yes, even if that is an uncommon scenario.

> +    s->mr = 0x1;            /* Operating Mode = Single conversion. */
> > +    s->x = 0;
> > +    s->z = 0;
> > +    s->y = 0;
> > +    s->sr = 0x1;            /* DRDY = 1. */
>
> Do you understand how the DRDY and LOCK bits work ? The datasheet
> is unclear. Also, what's the difference between "single-conversion"
> and "continuous-conversion" modes ?
>

DRDY indicates that a set of XYZ samples is available in the data registers,
presumably post reset or when switching modes, and how LOCK works is that
once I read the first byte of the X register, the current sample will be
locked
until it has been fully read to prevent the values from being changed
mid-read
with a high sample rate. LOCK simply indicates this status of the registers
being
read-only until we get to the end of ou 6 byte sample.

Some details are unclear, however, such as what happens if I don't read
all six bytes, and go back to request the first X byte again? I'll need to
hook a
real sensor up to see what happens in those cases.

Single-conversion mode is documented in earlier variations of this chip
family, but
it is used for device calibration. From the LSM303DLH (not 'C' at the end):

"By placing the mode register into single-conversion mode (0x01), two data
acquisition  cycles are made on each magnetic vector. The first acquisition
is a set pulse followed shortly by measurement data of the external field.
The second acquisition has the offset strap excited in the positive bias
mode
to create about  a 0.6 gauss self-test field plus the external field. The
first
acquisition values are subtracted  from the second acquisition, and the net
measurement is placed into the data output  registers."

Given the lack of details in the LSM303DLHC datasheet, however, only
continuous mode should likely be modeled, and the way QEMU works
to model sensors makes this a moot point anyway since the output
registers are only updated when an end-user changes the values manually.
If specific values are expected by the data consumer, such as calibration
data
from single-conversion mode, those values can be set by the user regardless
of the current operating mode.


> > +    s->ira = 0x48;
> > +    s->irb = 0x34;
> > +    s->irc = 0x33;
> > +    s->temperature = 0;     /* Default to 0 degrees C (0/8 lsb = 0 C).
> */
> > +}
> > +
> > +/**
> > + * @brief Callback handler when DeviceState 'reset' is set to true.
> > + */
> > +static void lsm303dlhc_mag_reset(DeviceState *dev)
> > +{
> > +    I2CSlave *i2c = I2C_SLAVE(dev);
> > +    LSM303DLHC_Mag_State *s = LSM303DLHC_MAG(i2c);
> > +
> > +       /* Set the device into is default reset state. */
> > +       lsm303dlhc_mag_default_cfg(&s->parent_obj);
>
> Misindentation.
>
> Also, don't use the parent_obj field;
> always use the QOM cast macro when you need the pointer
> to something as a different type. In this case you already
> have the I2CSlave*, in 'i2c'. But better would be to make
> lsm303dlhc_mag_default_cfg() take a LSM303DLHC_Mag_State*
> directly rather than taking an I2CSlave* and casting it
> internally.
>

Do you have an example, just to be sure I follow? I've changed the code
as follows:

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);
}

static void lsm303dlhc_mag_default_cfg(LSM303DLHCMagState *s)

Is this sufficient?

> +static void lsm303dlhc_mag_initfn(Object *obj)
> > +{
> > +    object_property_add(obj, LSM303DLHC_MSG_PROP_MAGX, "int",
> > +                lsm303dlhc_mag_get_xyz,
> > +                lsm303dlhc_mag_set_xyz, NULL, NULL);
> > +
> > +    object_property_add(obj, LSM303DLHC_MSG_PROP_MAGY, "int",
> > +                lsm303dlhc_mag_get_xyz,
> > +                lsm303dlhc_mag_set_xyz, NULL, NULL);
> > +
> > +    object_property_add(obj, LSM303DLHC_MSG_PROP_MAGZ, "int",
> > +                lsm303dlhc_mag_get_xyz,
> > +                lsm303dlhc_mag_set_xyz, NULL, NULL);
>
> What units are these in? It looks like your implementation just
> uses the property values as the raw -2048..+2048 value that the
> X/Y/Z registers read as. Would it be better for the properties to
> set the value in Gauss, and then the model to honour the
> gain settings in CRB_REG_M.GN{0,1,2} ?  That way guest code that
> adjusts the gain will get the results it is expecting.
>

I guess I found raw units that the sensor uses more intuitive personally,
with no room for unexpected translations and not having to deal with floats,
but if you feel gauss or degrees C is better, I can change these.

In that case, should I accept floating point inputs, however, or stick to
integers?
When dealing with magnetometers the values can be very small, so it's not
the
same as a temp sensor where we can provide 2300 for 23.00C.


> > +
> > +    object_property_add(obj, LSM303DLHC_MSG_PROP_TEMP, "int",
> > +                lsm303dlhc_mag_get_temperature,
> > +                lsm303dlhc_mag_set_temperature, NULL, NULL);
>
> What units is this in?
>

LSB where 1 LSB = 0.125 C, documented elsewhere, but as per the above
I can change this to degrees if you can clarify if this should be in float
or something
integere based with a specific scale factor.

Thanks for the feedback, and sorry again for the slow reply.

-- Kevin

[-- Attachment #2: Type: text/html, Size: 10349 bytes --]

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

[Peter Maydell]

On Mon, 20 Sept 2021 at 14:38, Kevin Townsend <kevin.townsend@linaro.org> wrote:
>
> Hi Peter,
>
> Thanks for the review, and sorry for the slow reply, just getting back from holidays myself.
>
> On Thu, 26 Aug 2021 at 17:39, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>>
>> So if I'm reading the datasheet correctly, the LM303DLHC is
>> really two completely distinct i2c devices in a single
>> package with different slave addresses; this QEMU device
>> implements only the magnetometer i2c device, and if we wanted
>> to add the accelerometer device we'd implement that as a
>> second separate QEMU i2c device ?
>
>
> This is correct. There are two distinct dies in the chip with separate I2C addresses, etc.,
> and this should probably be modelled separately. I chose the magnetometer since it's
> a far simpler device to model than the accelrometer, but still solves the need for a
> more complex I2C sensor that can be used in testing with the I2C bus.
>
>> > +    if (value > 2047 || value < -2048) {
>> > +        error_setg(errp, "value %d lsb is out of range", value);
>>
>> Why "lsb" ?
>>
>
> In my head, using LSB seemed more precise since I know exactly what value will
> be set to the registers, and exactly what I will get back when reading versus passing
> in a float that's needs to be conveted as a 'best-fit' scenario in 0.125°C steps?

My question was really, "what does 'lsb' mean here"?  I would usually
assume "least significant bit", but that makes no sense in this context.


>> > +
>> > +/**
>> > + * @brief Callback handler when DeviceState 'reset' is set to true.
>> > + */
>> > +static void lsm303dlhc_mag_reset(DeviceState *dev)
>> > +{
>> > +    I2CSlave *i2c = I2C_SLAVE(dev);
>> > +    LSM303DLHC_Mag_State *s = LSM303DLHC_MAG(i2c);
>> > +
>> > +       /* Set the device into is default reset state. */
>> > +       lsm303dlhc_mag_default_cfg(&s->parent_obj);
>>
>> Misindentation.
>>
>> Also, don't use the parent_obj field;
>> always use the QOM cast macro when you need the pointer
>> to something as a different type. In this case you already
>> have the I2CSlave*, in 'i2c'. But better would be to make
>> lsm303dlhc_mag_default_cfg() take a LSM303DLHC_Mag_State*
>> directly rather than taking an I2CSlave* and casting it
>> internally.
>
>
> Do you have an example, just to be sure I follow? I've changed the code
> as follows:
>
> 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);
> }
>
> static void lsm303dlhc_mag_default_cfg(LSM303DLHCMagState *s)
>
> Is this sufficient?

Yes, that's right.

>> > +static void lsm303dlhc_mag_initfn(Object *obj)
>> > +{
>> > +    object_property_add(obj, LSM303DLHC_MSG_PROP_MAGX, "int",
>> > +                lsm303dlhc_mag_get_xyz,
>> > +                lsm303dlhc_mag_set_xyz, NULL, NULL);
>> > +
>> > +    object_property_add(obj, LSM303DLHC_MSG_PROP_MAGY, "int",
>> > +                lsm303dlhc_mag_get_xyz,
>> > +                lsm303dlhc_mag_set_xyz, NULL, NULL);
>> > +
>> > +    object_property_add(obj, LSM303DLHC_MSG_PROP_MAGZ, "int",
>> > +                lsm303dlhc_mag_get_xyz,
>> > +                lsm303dlhc_mag_set_xyz, NULL, NULL);
>>
>> What units are these in? It looks like your implementation just
>> uses the property values as the raw -2048..+2048 value that the
>> X/Y/Z registers read as. Would it be better for the properties to
>> set the value in Gauss, and then the model to honour the
>> gain settings in CRB_REG_M.GN{0,1,2} ?  That way guest code that
>> adjusts the gain will get the results it is expecting.
>
>
> I guess I found raw units that the sensor uses more intuitive personally,
> with no room for unexpected translations and not having to deal with floats,
> but if you feel gauss or degrees C is better, I can change these.

Well, given that the device specifically changes the value it
shows the guest based on guest-programmable gain settings,
it does seem to me to be more natural to specify the values
in some way that represents the "real world data" that the
sensor is measuring. Ideally we would then if/when we add more
magnetometer implementations have them all use the same units,
for consistency. This is the first magnetometer we have, so this
is where we get to pick the convention.

> In that case, should I accept floating point inputs, however, or stick to integers?
> When dealing with magnetometers the values can be very small, so it's not the
> same as a temp sensor where we can provide 2300 for 23.00C.

What sort of range and precision requirements are we talking about
here? If we can avoid having to use float that would be nice...

>>
>> > +
>> > +    object_property_add(obj, LSM303DLHC_MSG_PROP_TEMP, "int",
>> > +                lsm303dlhc_mag_get_temperature,
>> > +                lsm303dlhc_mag_set_temperature, NULL, NULL);
>>
>> What units is this in?
>
>
> LSB where 1 LSB = 0.125 C, documented elsewhere, but as per the above
> I can change this to degrees if you can clarify if this should be in float or something
> integere based with a specific scale factor.

Our existing temperature sensors use integer properties whose
value is "temperature in degrees C, units of 0.001 C".
Consistency with that would be best. (We should write these
conventions down somewhere. Not sure where...)

thanks
-- PMM

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

[Kevin Townsend]

[-- Attachment #1: Type: text/plain, Size: 3568 bytes --]

On Mon, 20 Sept 2021 at 15:52, Peter Maydell <peter.maydell@linaro.org>
wrote:

> >> Why "lsb" ?
> >>
> >
> > In my head, using LSB seemed more precise since I know exactly what
> value will
> > be set to the registers, and exactly what I will get back when reading
> versus passing
> > in a float that's needs to be conveted as a 'best-fit' scenario in
> 0.125°C steps?
>
> My question was really, "what does 'lsb' mean here"?  I would usually
> assume "least significant bit", but that makes no sense in this context.
>
> Ha, sorry. Least significant bit, yes. It gets used in sensor and IC
datasheets all
the time and basically means '1 bit', so if the DS says 0.125°C per LSB it
means
that value for 1 bit, or to multiply the integer by the 1 LSB value.

Conversion factors from raw units to standard SI units are almost always
indicated this way, though, such as 'LSB/Gauss', etc.

Well, given that the device specifically changes the value it
> shows the guest based on guest-programmable gain settings,
> it does seem to me to be more natural to specify the values
> in some way that represents the "real world data" that the
> sensor is measuring. Ideally we would then if/when we add more
> magnetometer implementations have them all use the same units,
> for consistency. This is the first magnetometer we have, so this
> is where we get to pick the convention.
>

Sounds reasonable.

We have two options here:

- Gauss (standard SI unit)
- micro Tesla (0.01 Gauss)

They've both widely used; but I think uT might be slightly more common.


> > In that case, should I accept floating point inputs, however, or stick
> to integers?
> > When dealing with magnetometers the values can be very small, so it's
> not the
> > same as a temp sensor where we can provide 2300 for 23.00C.
>
> What sort of range and precision requirements are we talking about
> here? If we can avoid having to use float that would be nice...
>
> Well, taking the LSM303DLHC as an example, we have 1100 LSB per Gauss
at a range of +/- 1.3 Gauss, so 1 bit is: 0.0009090909091 Gauss.

If we use micro Tesla (uT) we get 11 LSB per uT so the smallest value is
0.09090909091 uT ... which we could represent with something like
1000 = 1.000 uT

That gives us +/- 1.3 G = +/- 130 uT = +/- 130,000, for example.

This would require a 32-bit integer, though, to take into account the full
range of +/-8.1 G (+/- 810 uT) = +/- 810,000.

There are devices with a much wider range, like the MLX90393, which can
measure up to +/- 50 mT (50,000 uT), so +/-50,000,000.

That's the largest range I'm aware of personally, with +/- 1-8G (or 100-800
uT)
the most common.

>>
> >> > +
> >> > +    object_property_add(obj, LSM303DLHC_MSG_PROP_TEMP, "int",
> >> > +                lsm303dlhc_mag_get_temperature,
> >> > +                lsm303dlhc_mag_set_temperature, NULL, NULL);
> >>
> >> What units is this in?
> >
> >
> > LSB where 1 LSB = 0.125 C, documented elsewhere, but as per the above
> > I can change this to degrees if you can clarify if this should be in
> float or something
> > integere based with a specific scale factor.
>
> Our existing temperature sensors use integer properties whose
> value is "temperature in degrees C, units of 0.001 C".
> Consistency with that would be best. (We should write these
> conventions down somewhere. Not sure where...)
>

That's similar to what I propose above, based on chosing micro Tesla as the
base unit, and not Gauss, so units of 0.001 uT.

Kevin

[-- Attachment #2: Type: text/html, Size: 5029 bytes --]

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

[Peter Maydell]

On Mon, 20 Sept 2021 at 15:22, Kevin Townsend <kevin.townsend@linaro.org> wrote:
> On Mon, 20 Sept 2021 at 15:52, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> >> Why "lsb" ?
>> >>
>> >
>> > In my head, using LSB seemed more precise since I know exactly what value will
>> > be set to the registers, and exactly what I will get back when reading versus passing
>> > in a float that's needs to be conveted as a 'best-fit' scenario in 0.125°C steps?
>>
>> My question was really, "what does 'lsb' mean here"?  I would usually
>> assume "least significant bit", but that makes no sense in this context.
>>
> Ha, sorry. Least significant bit, yes. It gets used in sensor and IC datasheets all
> the time and basically means '1 bit', so if the DS says 0.125°C per LSB it means
> that value for 1 bit, or to multiply the integer by the 1 LSB value.
>
> Conversion factors from raw units to standard SI units are almost always
> indicated this way, though, such as 'LSB/Gauss', etc.

OK, that's not a convention I've encountered before.

>> Well, given that the device specifically changes the value it
>> shows the guest based on guest-programmable gain settings,
>> it does seem to me to be more natural to specify the values
>> in some way that represents the "real world data" that the
>> sensor is measuring. Ideally we would then if/when we add more
>> magnetometer implementations have them all use the same units,
>> for consistency. This is the first magnetometer we have, so this
>> is where we get to pick the convention.
>
>
> Sounds reasonable.
>
> We have two options here:
>
> - Gauss (standard SI unit)
> - micro Tesla (0.01 Gauss)
>
> They've both widely used; but I think uT might be slightly more common.
>
>>
>> > In that case, should I accept floating point inputs, however, or stick to integers?
>> > When dealing with magnetometers the values can be very small, so it's not the
>> > same as a temp sensor where we can provide 2300 for 23.00C.
>>
>> What sort of range and precision requirements are we talking about
>> here? If we can avoid having to use float that would be nice...
>>
> Well, taking the LSM303DLHC as an example, we have 1100 LSB per Gauss
> at a range of +/- 1.3 Gauss, so 1 bit is: 0.0009090909091 Gauss.
>
> If we use micro Tesla (uT) we get 11 LSB per uT so the smallest value is
> 0.09090909091 uT ... which we could represent with something like
> 1000 = 1.000 uT
>
> That gives us +/- 1.3 G = +/- 130 uT = +/- 130,000, for example.
>
> This would require a 32-bit integer, though, to take into account the full
> range of +/-8.1 G (+/- 810 uT) = +/- 810,000.

That's OK -- our "int" properties are int64_t. So we could easily
fit something like 10000 == 1.0000 uT, in case we might want
the extra precision in future. That would be 1,000,000 == 1 G
(assuming I haven't messed up my arithmetic ;-))

> There are devices with a much wider range, like the MLX90393, which can
> measure up to +/- 50 mT (50,000 uT), so +/-50,000,000.
>
> That's the largest range I'm aware of personally, with +/- 1-8G (or 100-800 uT)
> the most common.

thanks
-- PMM

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

[Kevin Townsend]

[-- Attachment #1: Type: text/plain, Size: 433 bytes --]

On Mon, 20 Sept 2021 at 17:14, Peter Maydell <peter.maydell@linaro.org>
wrote:

> That's OK -- our "int" properties are int64_t. So we could easily
> fit something like 10000 == 1.0000 uT, in case we might want
> the extra precision in future. That would be 1,000,000 == 1 G
> (assuming I haven't messed up my arithmetic ;-))
>
> Is 0.001 uT OK to use as a starting point? I think that's enough for most
sensors I'm aware of.

Kevin

[-- Attachment #2: Type: text/html, Size: 803 bytes --]

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

[Peter Maydell]

On Mon, 20 Sept 2021 at 17:49, Kevin Townsend <kevin.townsend@linaro.org> wrote:
>
> On Mon, 20 Sept 2021 at 17:14, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> That's OK -- our "int" properties are int64_t. So we could easily
>> fit something like 10000 == 1.0000 uT, in case we might want
>> the extra precision in future. That would be 1,000,000 == 1 G
>> (assuming I haven't messed up my arithmetic ;-))
>>
> Is 0.001 uT OK to use as a starting point? I think that's enough for most
> sensors I'm aware of.

The thing is that the starting point is also the finishing point:
once we have released something that uses a particular set of
units, we can't change it in future without breaking backwards
compatibility. So we need to get it right now.

-- PMM

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

[Kevin Townsend]

[-- Attachment #1: Type: text/plain, Size: 779 bytes --]

HI Peter,

On Mon, 20 Sept 2021 at 19:20, Peter Maydell <peter.maydell@linaro.org>
wrote:

> > Is 0.001 uT OK to use as a starting point? I think that's enough for most
> > sensors I'm aware of.
>
> The thing is that the starting point is also the finishing point:
> once we have released something that uses a particular set of
> units, we can't change it in future without breaking backwards
> compatibility. So we need to get it right now.
>
>
I think 1000 = 1 uT is an appropriate value. In theory we could also just
indicate nT, but that's just a weird unit and everyone thinks in either
gauss or uT with magnetometers.

1000000 = 1 uT just feels excessive and outside a sensible
range for any sensors I'm aware of.

That this matches temp sensors is an added bonus.

Kevin

[-- Attachment #2: Type: text/html, Size: 1261 bytes --]

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

[Peter Maydell]

On Sun, 15 Aug 2021 at 17:31, 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.

> +/* Property Names */
> +#define LSM303DLHC_MSG_PROP_MAGX ("mag_x")
> +#define LSM303DLHC_MSG_PROP_MAGY ("mag_y")
> +#define LSM303DLHC_MSG_PROP_MAGZ ("mag_z")
> +#define LSM303DLHC_MSG_PROP_TEMP ("temperature")

Forgot to mention -- for property names, we prefer hyphens
rather than underscores.

-- PMM