Re: [PATCH 09/10] hw/misc: Add Infineon DPS310 sensor model

[Cédric Le Goater]

On 8/9/21 5:59 PM, Philippe Mathieu-Daudé wrote:
> On 8/9/21 3:15 PM, Cédric Le Goater wrote:
>> From: Joel Stanley <joel@jms.id.au>
>>
>> This contains some hardcoded register values that were obtained from the
>> hardware after reading the temperature.
>>
>> It does enough to test the Linux kernel driver. The FIFO mode, IRQs and
>> operation modes other than the default as used by Linux are not modelled.
>>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> [ clg: Fix sequential reading ]
>> Message-Id: <20210616073358.750472-2-joel@jms.id.au>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> Message-Id: <20210629142336.750058-4-clg@kaod.org>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/misc/dps310.c    | 227 ++++++++++++++++++++++++++++++++++++++++++++
>>  hw/arm/Kconfig      |   1 +
>>  hw/misc/Kconfig     |   4 +
>>  hw/misc/meson.build |   1 +
>>  4 files changed, 233 insertions(+)
>>  create mode 100644 hw/misc/dps310.c
>>
>> diff --git a/hw/misc/dps310.c b/hw/misc/dps310.c
>> new file mode 100644
>> index 000000000000..893521ab8516
>> --- /dev/null
>> +++ b/hw/misc/dps310.c
>> @@ -0,0 +1,227 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright 2017-2021 Joel Stanley <joel@jms.id.au>, IBM Corporation
>> + *
>> + * Infineon DPS310 temperature and humidity sensor
>> + *
>> + * 
>> https://www.infineon.com/cms/en/product/sensor/pressure-sensors/pressure-sensors-for-iot/dps310/
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "hw/hw.h"
>> +#include "hw/i2c/i2c.h"
>> +#include "qapi/error.h"
>> +#include "qapi/visitor.h"
>> +#include "migration/vmstate.h"
>> +
>> +#define NUM_REGISTERS   0x33
>> +
>> +typedef struct DPS310State {
>> +    /*< private >*/
>> +    I2CSlave i2c;
>> +
>> +    /*< public >*/
>> +    uint8_t regs[NUM_REGISTERS];
>> +
>> +    uint8_t len;
>> +    uint8_t pointer;
>> +
>> +} DPS310State;
> 
>> +static void dps310_reset(DeviceState *dev)
>> +{
>> +    DPS310State *s = DPS310(dev);
>> +
>> +    static const uint8_t regs_reset_state[] = {
> 
>        static const uint8_t regs_reset_state[NUM_REGISTERS] = {

How about :

    static const uint8_t regs_reset_state[sizeof(s->regs)] = {

I wonder if that compiles always. I will try with a gitlab CI.

>> +        0xfe, 0x2f, 0xee, 0x02, 0x69, 0xa6, 0x00, 0x80, 0xc7, 0x00, 0x00, 
>> 0x00,
>> +        0x00, 0x10, 0x00, 0x00, 0x0e, 0x1e, 0xdd, 0x13, 0xca, 0x5f, 0x21, 
>> 0x52,
>> +        0xf9, 0xc6, 0x04, 0xd1, 0xdb, 0x47, 0x00, 0x5b, 0xfb, 0x3a, 0x00, 
>> 0x00,
>> +        0x20, 0x49, 0x4e, 0xa5, 0x90, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
>> 0x00,
>> +        0x60, 0x15, 0x02
>> +    };
>> +
>> +    QEMU_BUILD_BUG_ON(sizeof(regs_reset_state) != sizeof(s->regs));
> 
> and drop QEMU_BUILD_BUG_ON?

yes.

thanks,

C.

>> +
>> +    memcpy(s->regs, regs_reset_state, sizeof(s->regs));
>> +    s->pointer = 0;
>> +
>> +    /* TODO: assert these after some timeout ? */
>> +    s->regs[DPS310_MEAS_CFG] = DPS310_COEF_RDY | DPS310_SENSOR_RDY
>> +        | DPS310_TMP_RDY | DPS310_PRS_RDY;
>> +}