| Message ID | 20210815162738.75461-2-kevin.townsend@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] hw/sensor: Add lsm303dlhc magnetometer device | expand |
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
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
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 <div dir="ltr"><div dir="ltr"><div>Hi Peter,</div><div><br></div><div>Thanks for the review, and sorry for the slow reply, just getting back from holidays myself.<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 26 Aug 2021 at 17:39, Peter Maydell <<a href="mailto:peter.maydell@linaro.org">peter.maydell@linaro.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br> So if I'm reading the datasheet correctly, the LM303DLHC is<br> really two completely distinct i2c devices in a single<br> package with different slave addresses; this QEMU device<br> implements only the magnetometer i2c device, and if we wanted<br> to add the accelerometer device we'd implement that as a<br> second separate QEMU i2c device ?<br></blockquote><div><br></div><div>This is correct. There are two distinct dies in the chip with separate I2C addresses, etc.,</div><div>and this should probably be modelled separately. I chose the magnetometer since it's</div><div>a far simpler device to model than the accelrometer, but still solves the need for a</div><div>more complex I2C sensor that can be used in testing with the I2C bus.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">> + if (value > 2047 || value < -2048) {<br> > + error_setg(errp, "value %d lsb is out of range", value);<br> <br> Why "lsb" ?<br> <br></blockquote><div><br></div><div>In my head, using LSB seemed more precise since I know exactly what value will</div><div>be set to the registers, and exactly what I will get back when reading versus passing</div><div>in a float that's needs to be conveted as a 'best-fit' scenario in 0.125°C steps?</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + * The auto-increment logic is only taken into account in this driver<br> > + * for the LSM303DLHC_MAG_REG_OUT_X_H and LSM303DLHC_MAG_REG_TEMP_OUT_H<br> > + * registers, which are the two common uses cases for it. Accessing either<br> > + * of these registers will also populate the rest of the related dataset.<br> <br> How hard would it be to implement the behaviour correctly for all cases?<br> I find it's usually better to model something correctly from the start:<br> usually the person writing the code knows the h/w behaviour better than<br> anybody else coming along later trying to figure out why some other<br> guest code doesn't work on the model...<br></blockquote><div><br></div><div>I can update this to also take auto-increment into account when you don't start at</div><div>the first record (X-axis), yes, even if that is an uncommon scenario.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">> + s->mr = 0x1; /* Operating Mode = Single conversion. */<br> > + s->x = 0;<br> > + s->z = 0;<br> > + s->y = 0;<br> > + s->sr = 0x1; /* DRDY = 1. */<br> <br> Do you understand how the DRDY and LOCK bits work ? The datasheet<br> is unclear. Also, what's the difference between "single-conversion"<br> and "continuous-conversion" modes ?<br></blockquote><div><br></div><div>DRDY indicates that a set of XYZ samples is available in the data registers,</div><div>presumably post reset or when switching modes, and how LOCK works is that</div><div>once I read the first byte of the X register, the current sample will be locked</div><div>until it has been fully read to prevent the values from being changed mid-read</div><div>with a high sample rate. LOCK simply indicates this status of the registers being</div><div>read-only until we get to the end of ou 6 byte sample.<br></div><div><br></div><div>Some details are unclear, however, such as what happens if I don't read</div><div>all six bytes, and go back to request the first X byte again? I'll need to hook a</div><div>real sensor up to see what happens in those cases.</div><div><br></div><div>Single-conversion mode is documented in earlier variations of this chip family, but</div><div>it is used for device calibration. From the LSM303DLH (not 'C' at the end):</div><div><br></div><div>"By placing the mode register into single-conversion mode (0x01), two data</div><div>acquisition cycles are made on each magnetic vector. The first acquisition</div><div>is a set pulse followed shortly by measurement data of the external field.</div><div>The second acquisition has the offset strap excited in the positive bias mode</div><div>to create about a 0.6 gauss self-test field plus the external field. The first</div><div>acquisition values are subtracted from the second acquisition, and the net</div><div>measurement is placed into the data output registers."</div><div><br> </div><div>Given the lack of details in the LSM303DLHC datasheet, however, only</div><div>continuous mode should likely be modeled, and the way QEMU works</div><div>to model sensors makes this a moot point anyway since the output</div><div>registers are only updated when an end-user changes the values manually.</div><div>If specific values are expected by the data consumer, such as calibration data</div><div>from single-conversion mode, those values can be set by the user regardless</div><div>of the current operating mode.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> > + s->ira = 0x48;<br> > + s->irb = 0x34;<br> > + s->irc = 0x33;<br> > + s->temperature = 0; /* Default to 0 degrees C (0/8 lsb = 0 C). */<br> > +}<br> > +<br> > +/**<br> > + * @brief Callback handler when DeviceState 'reset' is set to true.<br> > + */<br> > +static void lsm303dlhc_mag_reset(DeviceState *dev)<br> > +{<br> > + I2CSlave *i2c = I2C_SLAVE(dev);<br> > + LSM303DLHC_Mag_State *s = LSM303DLHC_MAG(i2c);<br> > +<br> > + /* Set the device into is default reset state. */<br> > + lsm303dlhc_mag_default_cfg(&s->parent_obj);<br> <br> Misindentation.<br> <br> Also, don't use the parent_obj field;<br> always use the QOM cast macro when you need the pointer<br> to something as a different type. In this case you already<br> have the I2CSlave*, in 'i2c'. But better would be to make<br> lsm303dlhc_mag_default_cfg() take a LSM303DLHC_Mag_State*<br> directly rather than taking an I2CSlave* and casting it<br> internally.<br></blockquote><div><br></div><div>Do you have an example, just to be sure I follow? I've changed the code</div><div>as follows:</div><div><br></div><div>static void lsm303dlhc_mag_reset(DeviceState *dev)<br>{<br> I2CSlave *i2c = I2C_SLAVE(dev);<br> LSM303DLHCMagState *s = LSM303DLHC_MAG(i2c);<br><br> /* Set the device into its default reset state. */<br> lsm303dlhc_mag_default_cfg(s);<br>}</div><div><br></div><div>static void lsm303dlhc_mag_default_cfg(LSM303DLHCMagState *s)</div><div><br></div><div>Is this sufficient?</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > +static void lsm303dlhc_mag_initfn(Object *obj)<br> > +{<br> > + object_property_add(obj, LSM303DLHC_MSG_PROP_MAGX, "int",<br> > + lsm303dlhc_mag_get_xyz,<br> > + lsm303dlhc_mag_set_xyz, NULL, NULL);<br> > +<br> > + object_property_add(obj, LSM303DLHC_MSG_PROP_MAGY, "int",<br> > + lsm303dlhc_mag_get_xyz,<br> > + lsm303dlhc_mag_set_xyz, NULL, NULL);<br> > +<br> > + object_property_add(obj, LSM303DLHC_MSG_PROP_MAGZ, "int",<br> > + lsm303dlhc_mag_get_xyz,<br> > + lsm303dlhc_mag_set_xyz, NULL, NULL);<br> <br> What units are these in? It looks like your implementation just<br> uses the property values as the raw -2048..+2048 value that the<br> X/Y/Z registers read as. Would it be better for the properties to<br> set the value in Gauss, and then the model to honour the<br> gain settings in <a href="http://CRB_REG_M.GN" rel="noreferrer" target="_blank">CRB_REG_M.GN</a>{0,1,2} ? That way guest code that<br> adjusts the gain will get the results it is expecting.<br></blockquote><div><br></div><div>I guess I found raw units that the sensor uses more intuitive personally,</div><div>with no room for unexpected translations and not having to deal with floats,</div><div>but if you feel gauss or degrees C is better, I can change these.</div><div><br></div><div>In that case, should I accept floating point inputs, however, or stick to integers?</div><div>When dealing with magnetometers the values can be very small, so it's not the</div><div>same as a temp sensor where we can provide 2300 for 23.00C.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> > +<br> > + object_property_add(obj, LSM303DLHC_MSG_PROP_TEMP, "int",<br> > + lsm303dlhc_mag_get_temperature,<br> > + lsm303dlhc_mag_set_temperature, NULL, NULL);<br> <br> What units is this in?<br></blockquote><div><br></div><div>LSB where 1 LSB = 0.125 C, documented elsewhere, but as per the above</div><div>I can change this to degrees if you can clarify if this should be in float or something</div><div>integere based with a specific scale factor.</div></div><div class="gmail_quote"><br></div><div class="gmail_quote">Thanks for the feedback, and sorry again for the slow reply.</div><div class="gmail_quote"><br></div><div class="gmail_quote">-- Kevin<br></div></div>
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
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 <div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 20 Sept 2021 at 15:52, Peter Maydell <<a href="mailto:peter.maydell@linaro.org">peter.maydell@linaro.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">>> Why "lsb" ?<br> >><br> ><br> > In my head, using LSB seemed more precise since I know exactly what value will<br> > be set to the registers, and exactly what I will get back when reading versus passing<br> > in a float that's needs to be conveted as a 'best-fit' scenario in 0.125°C steps?<br> <br> My question was really, "what does 'lsb' mean here"? I would usually<br> assume "least significant bit", but that makes no sense in this context.<br> <br></blockquote><div>Ha, sorry. Least significant bit, yes. It gets used in sensor and IC datasheets all</div><div>the time and basically means '1 bit', so if the DS says 0.125°C per LSB it means</div><div>that value for 1 bit, or to multiply the integer by the 1 LSB value.</div><div><br></div><div>Conversion factors from raw units to standard SI units are almost always</div><div>indicated this way, though, such as 'LSB/Gauss', etc.<br></div><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Well, given that the device specifically changes the value it<br> shows the guest based on guest-programmable gain settings,<br> it does seem to me to be more natural to specify the values<br> in some way that represents the "real world data" that the<br> sensor is measuring. Ideally we would then if/when we add more<br> magnetometer implementations have them all use the same units,<br> for consistency. This is the first magnetometer we have, so this<br> is where we get to pick the convention.<br></blockquote><div><br></div><div>Sounds reasonable.</div><div><br></div><div>We have two options here:</div><div><br></div><div>- Gauss (standard SI unit)<br></div><div>- micro Tesla (0.01 Gauss)<br></div><div><br></div><div>They've both widely used; but I think uT might be slightly more common.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> > In that case, should I accept floating point inputs, however, or stick to integers?<br> > When dealing with magnetometers the values can be very small, so it's not the<br> > same as a temp sensor where we can provide 2300 for 23.00C.<br> <br> What sort of range and precision requirements are we talking about<br> here? If we can avoid having to use float that would be nice...<br> <br></blockquote><div>Well, taking the LSM303DLHC as an example, we have 1100 LSB per Gauss</div><div>at a range of +/- 1.3 Gauss, so 1 bit is: 0.0009090909091 Gauss.<br></div><div><br></div><div>If we use micro Tesla (uT) we get 11 LSB per uT so the smallest value is</div><div>0.09090909091 uT ... which we could represent with something like</div><div>1000 = 1.000 uT</div><div><br></div><div>That gives us +/- 1.3 G = +/- 130 uT = +/- 130,000, for example.<br></div><div><br></div><div>This would require a 32-bit integer, though, to take into account the full</div><div>range of +/-8.1 G (+/- 810 uT) = +/- 810,000.<br></div><div> <br></div><div>There are devices with a much wider range, like the MLX90393, which can</div><div>measure up to +/- 50 mT (50,000 uT), so +/-50,000,000.</div><div><br></div><div>That's the largest range I'm aware of personally, with +/- 1-8G (or 100-800 uT)</div><div>the most common.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> >><br> >> > +<br> >> > + object_property_add(obj, LSM303DLHC_MSG_PROP_TEMP, "int",<br> >> > + lsm303dlhc_mag_get_temperature,<br> >> > + lsm303dlhc_mag_set_temperature, NULL, NULL);<br> >><br> >> What units is this in?<br> ><br> ><br> > LSB where 1 LSB = 0.125 C, documented elsewhere, but as per the above<br> > I can change this to degrees if you can clarify if this should be in float or something<br> > integere based with a specific scale factor.<br> <br> Our existing temperature sensors use integer properties whose<br> value is "temperature in degrees C, units of 0.001 C".<br> Consistency with that would be best. (We should write these<br> conventions down somewhere. Not sure where...)<br></blockquote><div><br></div><div>That's similar to what I propose above, based on chosing micro Tesla as the</div><div>base unit, and not Gauss, so units of 0.001 uT.<br></div><div><br></div><div>Kevin<br> </div></div></div>
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
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 <div dir="ltr"><div dir="ltr" class="gmail_attr">On Mon, 20 Sept 2021 at 17:14, Peter Maydell <<a href="mailto:peter.maydell@linaro.org">peter.maydell@linaro.org</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> That's OK -- our "int" properties are int64_t. So we could easily<br> fit something like 10000 == 1.0000 uT, in case we might want<br> the extra precision in future. That would be 1,000,000 == 1 G<br> (assuming I haven't messed up my arithmetic ;-))<br> <br></blockquote><div>Is 0.001 uT OK to use as a starting point? I think that's enough for most</div><div>sensors I'm aware of.</div><div><br></div><div>Kevin<br></div></div></div>
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
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 <div dir="ltr"><div>HI Peter,<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 20 Sept 2021 at 19:20, Peter Maydell <<a href="mailto:peter.maydell@linaro.org">peter.maydell@linaro.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">> Is 0.001 uT OK to use as a starting point? I think that's enough for most<br> > sensors I'm aware of.<br> <br> The thing is that the starting point is also the finishing point:<br> once we have released something that uses a particular set of<br> units, we can't change it in future without breaking backwards<br> compatibility. So we need to get it right now.<br> <br></blockquote><div><br></div><div>I think 1000 = 1 uT is an appropriate value. In theory we could also just</div><div>indicate nT, but that's just a weird unit and everyone thinks in either</div><div>gauss or uT with magnetometers.</div><div><br></div><div>1000000 = 1 uT just feels excessive and outside a sensible</div><div>range for any sensors I'm aware of.</div><div><br></div><div>That this matches temp sensors is an added bonus.<br></div><div><br></div><div>Kevin<br></div></div></div>
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'))
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 -- 2.30.1 (Apple Git-130)