Re: [Qemu-devel] [PATCH 2/4] hw/misc: add a TMP42{1, 2, 3} device model

[Cédric Le Goater]

On 05/23/2016 05:53 AM, Andrew Jeffery wrote:
> Hi Cédric,
> 
> On Fri, 2016-05-20 at 18:31 +0200, Cédric Le Goater wrote:
>> Largely inspired by the TMP105 temperature sensor, this patch brings
>> to Qemu a model for TMP42{1,2,3} temperature sensors.
>>
>> Specs can be found here :
>>
>>      http://www.ti.com/lit/gpn/tmp421
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>  hw/misc/Makefile.objs |   1 +
>>  hw/misc/tmp421.c      | 395 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 396 insertions(+)
>>  create mode 100644 hw/misc/tmp421.c
>>
>> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
>> index 1faf163c59f3..e50596965b03 100644
>> --- a/hw/misc/Makefile.objs
>> +++ b/hw/misc/Makefile.objs
>> @@ -1,6 +1,7 @@
>>  common-obj-$(CONFIG_APPLESMC) += applesmc.o
>>  common-obj-$(CONFIG_MAX111X) += max111x.o
>>  common-obj-$(CONFIG_TMP105) += tmp105.o
>> +common-obj-$(CONFIG_ASPEED_SOC) += tmp421.o
>>  common-obj-$(CONFIG_ISA_DEBUG) += debugexit.o
>>  common-obj-$(CONFIG_SGA) += sga.o
>>  common-obj-$(CONFIG_ISA_TESTDEV) += pc-testdev.o
>> diff --git a/hw/misc/tmp421.c b/hw/misc/tmp421.c
>> new file mode 100644
>> index 000000000000..e385ce053f5e
>> --- /dev/null
>> +++ b/hw/misc/tmp421.c
>> @@ -0,0 +1,395 @@
>> +/*
>> + * Texas Instruments TMP421 temperature sensor.
>> + *
>> + * Copyright (c) 2016 IBM Corporation.
>> + *
>> + * Largely inspired by :
>> + *
>> + * Texas Instruments TMP105 temperature sensor.
>> + *
>> + * Copyright (C) 2008 Nokia Corporation
>> + * Written by Andrzej Zaborowski <address@hidden>
>> + *
>> + * 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/hw.h"
>> +#include "hw/i2c/i2c.h"
>> +#include "qapi/error.h"
>> +#include "qapi/visitor.h"
>> +
>> +/* Manufacturer / Device ID's */
>> +#define TMP421_MANUFACTURER_ID          0x55
>> +#define TMP421_DEVICE_ID                0x21
>> +#define TMP422_DEVICE_ID                0x22
>> +#define TMP423_DEVICE_ID                0x23
>> +
>> +typedef struct DeviceInfo {
>> +    int model;
>> +    const char *name;
>> +} DeviceInfo;
>> +
>> +static const DeviceInfo devices[] = {
>> +    { TMP421_DEVICE_ID, "tmp421" },
>> +    { TMP422_DEVICE_ID, "tmp422" },
>> +    { TMP423_DEVICE_ID, "tmp423" },
>> +};
>> +
>> +typedef struct TMP421State {
>> +    /*< private >*/
>> +    I2CSlave i2c;
>> +    /*< public >*/
>> +
>> +    int16_t temperature[4];
>> +
>> +    uint8_t status;
>> +    uint8_t config[2];
>> +    uint8_t rate;
>> +
>> +    uint8_t len;
> 
> Given the starting point of the tmp105 code the patch looks okay, but I
> was a bit thrown by the use of the 'len' member as what I'd consider an
> index. For instance we reset len to zero in tmp421_event() after
> populating buf, and then the data in buf is presumably sent out on a
> recv transaction which again starts incrementing len. len is also
> incremented when we don't interact with buf, e.g. when we instead
> assign to pointer. It feels like it could be prone to bugs, and
> 'cb5ef3fa1871 tmp105: Fix I2C protocol bug' suggests that might not be
> an unreasonable feeling.

I will take a look. 

Anyhow I wanted to change this config option :

        +common-obj-$(CONFIG_ASPEED_SOC) += tmp421.o

as there is no reason not to export the device to other platforms.

Thanks

C. 

> But given the code already exists in tmp105 maybe it's fine?
> 
> Cheers,
> 
> Andrew
> 
>> +    uint8_t buf[2];
>> +    uint8_t pointer;
>> +
>> +} TMP421State;
>> +
>> +typedef struct TMP421Class {
>> +    I2CSlaveClass parent_class;
>> +    DeviceInfo *dev;
>> +} TMP421Class;
>> +
>> +#define TYPE_TMP421 "tmp421-generic"
>> +#define TMP421(obj) OBJECT_CHECK(TMP421State, (obj), TYPE_TMP421)
>> +
>> +#define TMP421_CLASS(klass) \
>> +     OBJECT_CLASS_CHECK(TMP421Class, (klass), TYPE_TMP421)
>> +#define TMP421_GET_CLASS(obj) \
>> +     OBJECT_GET_CLASS(TMP421Class, (obj), TYPE_TMP421)
>> +
>> +/* the TMP421 registers */
>> +#define TMP421_STATUS_REG               0x08
>> +#define    TMP421_STATUS_BUSY             (1 << 7)
>> +#define TMP421_CONFIG_REG_1             0x09
>> +#define    TMP421_CONFIG_RANGE            (1 << 2)
>> +#define    TMP421_CONFIG_SHUTDOWN         (1 << 6)
>> +#define TMP421_CONFIG_REG_2             0x0A
>> +#define    TMP421_CONFIG_RC               (1 << 2)
>> +#define    TMP421_CONFIG_LEN              (1 << 3)
>> +#define    TMP421_CONFIG_REN              (1 << 4)
>> +#define    TMP421_CONFIG_REN2             (1 << 5)
>> +#define    TMP421_CONFIG_REN3             (1 << 6)
>> +
>> +#define TMP421_CONVERSION_RATE_REG      0x0B
>> +#define TMP421_ONE_SHOT                 0x0F
>> +
>> +#define TMP421_RESET                    0xFC
>> +#define TMP421_MANUFACTURER_ID_REG      0xFE
>> +#define TMP421_DEVICE_ID_REG            0xFF
>> +
>> +#define TMP421_TEMP_MSB0                0x00
>> +#define TMP421_TEMP_MSB1                0x01
>> +#define TMP421_TEMP_MSB2                0x02
>> +#define TMP421_TEMP_MSB3                0x03
>> +#define TMP421_TEMP_LSB0                0x10
>> +#define TMP421_TEMP_LSB1                0x11
>> +#define TMP421_TEMP_LSB2                0x12
>> +#define TMP421_TEMP_LSB3                0x13
>> +
>> +static const int32_t mins[2] = { -40000, -55000 };
>> +static const int32_t maxs[2] = { 127000, 150000 };
>> +
>> +static void tmp421_get_temperature(Object *obj, Visitor *v, const char 
>> *name,
>> +                                   void *opaque, Error **errp)
>> +{
>> +    TMP421State *s = TMP421(obj);
>> +    bool ext_range = (s->config[0] & TMP421_CONFIG_RANGE);
>> +    int offset = ext_range * 64 * 256;
>> +    int64_t value;
>> +    int tempid;
>> +
>> +    if (sscanf(name, "temperature%d", &tempid) != 1) {
>> +        error_setg(errp, "error reading %s: %m", name);
>> +        return;
>> +    }
>> +
>> +    if (tempid >= 4 || tempid < 0) {
>> +        error_setg(errp, "error reading %s", name);
>> +        return;
>> +    }
>> +
>> +    value = ((s->temperature[tempid] - offset) * 1000 + 128) / 256;
>> +
>> +    visit_type_int(v, name, &value, errp);
>> +}
>> +
>> +/* Units are 0.001 centigrades relative to 0 C.  s->temperature is 8.8
>> + * fixed point, so units are 1/256 centigrades.  A simple ratio will do.
>> + */
>> +static void tmp421_set_temperature(Object *obj, Visitor *v, const char 
>> *name,
>> +                                   void *opaque, Error **errp)
>> +{
>> +    TMP421State *s = TMP421(obj);
>> +    Error *local_err = NULL;
>> +    int64_t temp;
>> +    bool ext_range = (s->config[0] & TMP421_CONFIG_RANGE);
>> +    int offset = ext_range * 64 * 256;
>> +    int tempid;
>> +
>> +    visit_type_int(v, name, &temp, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +
>> +    if (temp >= maxs[ext_range] || temp < mins[ext_range]) {
>> +        error_setg(errp, "value %" PRId64 ".%03" PRIu64 " °C is out of 
>> range",
>> +                   temp / 1000, temp % 1000);
>> +        return;
>> +    }
>> +
>> +    if (sscanf(name, "temperature%d", &tempid) != 1) {
>> +        error_setg(errp, "error reading %s: %m", name);
>> +        return;
>> +    }
>> +
>> +    if (tempid >= 4 || tempid < 0) {
>> +        error_setg(errp, "error reading %s", name);
>> +        return;
>> +    }
>> +
>> +    s->temperature[tempid] = (int16_t) ((temp * 256 - 128) / 1000) + offset;
>> +}
>> +
>> +static void tmp421_read(TMP421State *s)
>> +{
>> +    TMP421Class *sc = TMP421_GET_CLASS(s);
>> +
>> +    s->len = 0;
>> +
>> +    switch (s->pointer) {
>> +    case TMP421_MANUFACTURER_ID_REG:
>> +        s->buf[s->len++] = TMP421_MANUFACTURER_ID;
>> +        break;
>> +    case TMP421_DEVICE_ID_REG:
>> +        s->buf[s->len++] = sc->dev->model;
>> +        break;
>> +    case TMP421_CONFIG_REG_1:
>> +        s->buf[s->len++] = s->config[0];
>> +        break;
>> +    case TMP421_CONFIG_REG_2:
>> +        s->buf[s->len++] = s->config[1];
>> +        break;
>> +    case TMP421_CONVERSION_RATE_REG:
>> +        s->buf[s->len++] = s->rate;
>> +        break;
>> +    case TMP421_STATUS_REG:
>> +        s->buf[s->len++] = s->status;
>> +        break;
>> +
>> +        /* FIXME: check for channel enablement in config registers */
>> +    case TMP421_TEMP_MSB0:
>> +        s->buf[s->len++] = (((uint16_t) s->temperature[0]) >> 8);
>> +        break;
>> +    case TMP421_TEMP_MSB1:
>> +        s->buf[s->len++] = (((uint16_t) s->temperature[1]) >> 8);
>> +        break;
>> +    case TMP421_TEMP_MSB2:
>> +        s->buf[s->len++] = (((uint16_t) s->temperature[2]) >> 8);
>> +        break;
>> +    case TMP421_TEMP_MSB3:
>> +        s->buf[s->len++] = (((uint16_t) s->temperature[3]) >> 8);
>> +        break;
>> +    case TMP421_TEMP_LSB0:
>> +        s->buf[s->len++] = (((uint16_t) s->temperature[0]) >> 0) & 0xf0;
>> +        break;
>> +    case TMP421_TEMP_LSB1:
>> +        s->buf[s->len++] = (((uint16_t) s->temperature[1]) >> 0) & 0xf0;
>> +        break;
>> +    case TMP421_TEMP_LSB2:
>> +        s->buf[s->len++] = (((uint16_t) s->temperature[2]) >> 0) & 0xf0;
>> +        break;
>> +    case TMP421_TEMP_LSB3:
>> +        s->buf[s->len++] = (((uint16_t) s->temperature[3]) >> 0) & 0xf0;
>> +        break;
>> +    }
>> +}
>> +
>> +static void tmp421_reset(I2CSlave *i2c);
>> +
>> +static void tmp421_write(TMP421State *s)
>> +{
>> +    switch (s->pointer) {
>> +    case TMP421_CONVERSION_RATE_REG:
>> +        s->rate = s->buf[0];
>> +        break;
>> +    case TMP421_CONFIG_REG_1:
>> +        s->config[0] = s->buf[0];
>> +        break;
>> +    case TMP421_CONFIG_REG_2:
>> +        s->config[1] = s->buf[0];
>> +        break;
>> +    case TMP421_RESET:
>> +        tmp421_reset(I2C_SLAVE(s));
>> +        break;
>> +    }
>> +}
>> +
>> +static int tmp421_rx(I2CSlave *i2c)
>> +{
>> +    TMP421State *s = TMP421(i2c);
>> +
>> +    if (s->len < 2) {
>> +        return s->buf[s->len++];
>> +    } else {
>> +        return 0xff;
>> +    }
>> +}
>> +
>> +static int tmp421_tx(I2CSlave *i2c, uint8_t data)
>> +{
>> +    TMP421State *s = TMP421(i2c);
>> +
>> +    if (s->len == 0) {
>> +        s->pointer = data;
>> +        s->len++;
>> +    } else {
>> +        if (s->len <= 2) {
>> +            s->buf[s->len - 1] = data;
>> +        }
>> +        s->len++;
>> +        tmp421_write(s);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void tmp421_event(I2CSlave *i2c, enum i2c_event event)
>> +{
>> +    TMP421State *s = TMP421(i2c);
>> +
>> +    if (event == I2C_START_RECV) {
>> +        tmp421_read(s);
>> +    }
>> +
>> +    s->len = 0;
>> +}
>> +
>> +static const VMStateDescription vmstate_tmp421 = {
>> +    .name = "TMP421",
>> +    .version_id = 0,
>> +    .minimum_version_id = 0,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT8(len, TMP421State),
>> +        VMSTATE_UINT8_ARRAY(buf, TMP421State, 2),
>> +        VMSTATE_UINT8(pointer, TMP421State),
>> +        VMSTATE_UINT8_ARRAY(config, TMP421State, 2),
>> +        VMSTATE_UINT8(status, TMP421State),
>> +        VMSTATE_UINT8(rate, TMP421State),
>> +        VMSTATE_INT16_ARRAY(temperature, TMP421State, 4),
>> +        VMSTATE_I2C_SLAVE(i2c, TMP421State),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static void tmp421_reset(I2CSlave *i2c)
>> +{
>> +    TMP421State *s = TMP421(i2c);
>> +    TMP421Class *sc = TMP421_GET_CLASS(s);
>> +
>> +    memset(s->temperature, 0, sizeof(s->temperature));
>> +    s->pointer = 0;
>> +
>> +    s->config[0] = 0; /* TMP421_CONFIG_RANGE */
>> +
>> +     /* resistance correction and channel enablement */
>> +    switch (sc->dev->model) {
>> +    case TMP421_DEVICE_ID:
>> +        s->config[1] = 0x1c;
>> +        break;
>> +    case TMP422_DEVICE_ID:
>> +        s->config[1] = 0x3c;
>> +        break;
>> +    case TMP423_DEVICE_ID:
>> +        s->config[1] = 0x7c;
>> +        break;
>> +    }
>> +
>> +    s->rate = 0x7;       /* 8Hz */
>> +    s->status = 0;
>> +}
>> +
>> +static int tmp421_init(I2CSlave *i2c)
>> +{
>> +    TMP421State *s = TMP421(i2c);
>> +
>> +    tmp421_reset(&s->i2c);
>> +
>> +    return 0;
>> +}
>> +
>> +static void tmp421_initfn(Object *obj)
>> +{
>> +    object_property_add(obj, "temperature0", "int",
>> +                        tmp421_get_temperature,
>> +                        tmp421_set_temperature, NULL, NULL, NULL);
>> +    object_property_add(obj, "temperature1", "int",
>> +                        tmp421_get_temperature,
>> +                        tmp421_set_temperature, NULL, NULL, NULL);
>> +    object_property_add(obj, "temperature2", "int",
>> +                        tmp421_get_temperature,
>> +                        tmp421_set_temperature, NULL, NULL, NULL);
>> +    object_property_add(obj, "temperature3", "int",
>> +                        tmp421_get_temperature,
>> +                        tmp421_set_temperature, NULL, NULL, NULL);
>> +}
>> +
>> +static void tmp421_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
>> +    TMP421Class *sc = TMP421_CLASS(klass);
>> +
>> +    k->init = tmp421_init;
>> +    k->event = tmp421_event;
>> +    k->recv = tmp421_rx;
>> +    k->send = tmp421_tx;
>> +    dc->vmsd = &vmstate_tmp421;
>> +    sc->dev = (DeviceInfo *) data;
>> +}
>> +
>> +static const TypeInfo tmp421_info = {
>> +    .name          = TYPE_TMP421,
>> +    .parent        = TYPE_I2C_SLAVE,
>> +    .instance_size = sizeof(TMP421State),
>> +    .instance_init = tmp421_initfn,
>> +    .class_init    = tmp421_class_init,
>> +};
>> +
>> +static void tmp421_register_types(void)
>> +{
>> +    int i;
>> +
>> +    type_register_static(&tmp421_info);
>> +    for (i = 0; i < ARRAY_SIZE(devices); ++i) {
>> +        TypeInfo ti = {
>> +            .name       = devices[i].name,
>> +            .parent     = TYPE_TMP421,
>> +            .class_init = tmp421_class_init,
>> +            .class_data = (void *) &devices[i],
>> +        };
>> +        type_register(&ti);
>> +    }
>> +}
>> +
>> +type_init(tmp421_register_types)