Re: [PATCH v3 1/2] hw/misc: add an EMC141{3,4} device model

[Philippe Mathieu-Daudé]

On 10/25/20 2:14 PM, John Wang wrote:
> Largely inspired by the TMP421 temperature sensor, here is a model for
> the EMC1413/EMC1414 temperature sensors.
>
> Specs can be found here :
>    http://ww1.microchip.com/downloads/en/DeviceDoc/20005274A.pdf
>
> Signed-off-by: John Wang <wangzhiqiang.bj@bytedance.com>
> ---
> v3:
>    - update the link to the spec
>    - Rename emc1413.c to emc141x.c
>    - Add sensors_count in EMC141XClass
>    - Make emc1413_read/write easier to review :)

Thanks for the update.

> v2:
>    - Remove DeviceInfo
>    - commit message: TMP423 -> TMP421
> ---
>   hw/arm/Kconfig      |   1 +
>   hw/misc/Kconfig     |   4 +
>   hw/misc/emc141x.c   | 347 ++++++++++++++++++++++++++++++++++++++++++++
>   hw/misc/meson.build |   1 +
>   4 files changed, 353 insertions(+)
>   create mode 100644 hw/misc/emc141x.c
>
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 7d040827af..3dd651a236 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -403,6 +403,7 @@ config ASPEED_SOC
>       select SSI_M25P80
>       select TMP105
>       select TMP421
> +    select EMC141X
>       select UNIMP
>   
>   config MPS2
> diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
> index 3185456110..169d087d3d 100644
> --- a/hw/misc/Kconfig
> +++ b/hw/misc/Kconfig
> @@ -13,6 +13,10 @@ config TMP421
>       bool
>       depends on I2C
>   
> +config EMC141X
> +    bool
> +    depends on I2C
> +
>   config ISA_DEBUG
>       bool
>       depends on ISA_BUS
> diff --git a/hw/misc/emc141x.c b/hw/misc/emc141x.c
> new file mode 100644
> index 0000000000..a2bce74719
> --- /dev/null
> +++ b/hw/misc/emc141x.c
> @@ -0,0 +1,347 @@
> +/*
> + * SMSC EMC141X temperature sensor.
> + *
> + * Copyright (c) 2020 Bytedance Corporation
> + * Written by John Wang <wangzhiqiang.bj@bytedance.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 or
> + * (at your option) version 3 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#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 "qom/object.h"
> +
> +#define EMC1413_DEVICE_ID                0x21
> +#define EMC1414_DEVICE_ID                0x25
> +#define MANUFACTURER_ID                  0x5d
> +#define REVISION                         0x04
> +
> +#define SENSORS_COUNT_MAX    4
> +
> +struct EMC141XState {
> +    I2CSlave i2c;

The QOM style name is 'parent_obj':

       I2CSlave parent_obj;

> +    uint8_t temperature[SENSORS_COUNT_MAX];

What is the unit?

> +    uint8_t min[SENSORS_COUNT_MAX];
> +    uint8_t max[SENSORS_COUNT_MAX];

min/max what? Temperature?

What about (if the unit is milli Celsius):

       struct {
           uint8_t temp_min_mC;
           uint8_t temp_current_mC;
           uint8_t temp_max_mC;
       } sensor[SENSORS_COUNT_MAX];

Looking at the datasheet chapter 6.8 "Temperature Measurement
Results  and  Data", it isn't in milli Celsius. See below in
emc141x_get_temperature().

So maybe:

       struct {
           uint8_t raw_temp_min;
           uint8_t raw_temp_current;
           uint8_t raw_temp_max;
       } sensor[SENSORS_COUNT_MAX];

> +    uint8_t len;
> +    uint8_t data;
> +    uint8_t pointer;
> +};
> +
> +struct EMC141XClass {
> +    I2CSlaveClass parent_class;
> +    uint8_t model;
> +    unsigned sensors_count;
> +};
> +
> +#define TYPE_EMC141X "emc141x"
> +OBJECT_DECLARE_TYPE(EMC141XState, EMC141XClass, EMC141X)
> +
> +
> +/* the EMC141X registers */
> +#define EMC141X_TEMP_HIGH0               0x00
> +#define EMC141X_TEMP_HIGH1               0x01
> +#define EMC141X_TEMP_HIGH2               0x23
> +#define EMC141X_TEMP_HIGH3               0x2a
> +#define EMC141X_TEMP_MAX_HIGH0           0x05
> +#define EMC141X_TEMP_MIN_HIGH0           0x06
> +#define EMC141X_TEMP_MAX_HIGH1           0x07
> +#define EMC141X_TEMP_MIN_HIGH1           0x08
> +#define EMC141X_TEMP_MAX_HIGH2           0x15
> +#define EMC141X_TEMP_MIN_HIGH2           0x16
> +#define EMC141X_TEMP_MAX_HIGH3           0x2c
> +#define EMC141X_TEMP_MIN_HIGH3           0x2d
> +#define EMC141X_DEVICE_ID                0xfd
> +#define EMC141X_MANUFACTURER_ID          0xfe
> +#define EMC141X_REVISION                 0xff
> +
> +static void emc141x_get_temperature(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)

Nitpicking, align is off ;)

> +{
> +    EMC141XState *s = EMC141X(obj);
> +    EMC141XClass *sc = EMC141X_GET_CLASS(s);
> +    int64_t value;
> +    int tempid;

Shouldn't it be unsigned?

> +
> +    if (sscanf(name, "temperature%d", &tempid) != 1) {

Format doesn't match. Using 'unsigned' is probably easier (%u).

> +        error_setg(errp, "error reading %s: %s", name, g_strerror(errno));
> +        return;
> +    }
> +
> +    if (tempid >= sc->sensors_count || tempid < 0) {
> +        error_setg(errp, "error reading %s", name);
> +        return;
> +    }
> +
> +    value = s->temperature[tempid] * 1000;

This is not what is described in "Table 6.2: Temperature Data Format",
there is an offset and a step multiplier. Which makes me wonder how
are you testing this?
We have very simple tests for the TMP105, see tests/qtest/tmp105-test.c,
please add a similar test.

> +
> +    visit_type_int(v, name, &value, errp);
> +}
> +
...