All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 2/4] hw/misc: add a TMP42{1,2,3} device model
@ 2016-05-20 16:31 Cédric Le Goater
  2016-05-23  3:53 ` [Qemu-devel] [PATCH 2/4] hw/misc: add a TMP42{1, 2, 3} " Andrew Jeffery
  0 siblings, 1 reply; 5+ messages in thread
From: Cédric Le Goater @ 2016-05-20 16:31 UTC (permalink / raw)
  To: Peter Maydell; +Cc: qemu-devel, Andrew Jeffery, Cédric Le Goater

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 <clg@kaod.org>
---
 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 <andrew@openedhand.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/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;
+    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)
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [Qemu-devel] [PATCH 2/4] hw/misc: add a TMP42{1, 2, 3} device model
  2016-05-20 16:31 [Qemu-devel] [PATCH 2/4] hw/misc: add a TMP42{1,2,3} device model Cédric Le Goater
@ 2016-05-23  3:53 ` Andrew Jeffery
  2016-05-23  5:50   ` Cédric Le Goater
  2016-11-14  7:14   ` Cédric Le Goater
  0 siblings, 2 replies; 5+ messages in thread
From: Andrew Jeffery @ 2016-05-23  3:53 UTC (permalink / raw)
  To: Cédric Le Goater, Peter Maydell; +Cc: qemu-devel

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

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 <clg@kaod.org>
> ---
>  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 <andrew@openedhand.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/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.

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)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Qemu-devel] [PATCH 2/4] hw/misc: add a TMP42{1, 2, 3} device model
  2016-05-23  3:53 ` [Qemu-devel] [PATCH 2/4] hw/misc: add a TMP42{1, 2, 3} " Andrew Jeffery
@ 2016-05-23  5:50   ` Cédric Le Goater
  2016-11-14  7:14   ` Cédric Le Goater
  1 sibling, 0 replies; 5+ messages in thread
From: Cédric Le Goater @ 2016-05-23  5:50 UTC (permalink / raw)
  To: andrew, Peter Maydell; +Cc: qemu-devel

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 <clg@kaod.org>
>> ---
>>  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 <andrew@openedhand.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/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)

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Qemu-devel] [PATCH 2/4] hw/misc: add a TMP42{1, 2, 3} device model
  2016-05-23  3:53 ` [Qemu-devel] [PATCH 2/4] hw/misc: add a TMP42{1, 2, 3} " Andrew Jeffery
  2016-05-23  5:50   ` Cédric Le Goater
@ 2016-11-14  7:14   ` Cédric Le Goater
  2016-11-15  1:09     ` Andrew Jeffery
  1 sibling, 1 reply; 5+ messages in thread
From: Cédric Le Goater @ 2016-11-14  7:14 UTC (permalink / raw)
  To: andrew, Peter Maydell; +Cc: qemu-devel

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 <clg@kaod.org>
>> ---
>>  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 <andrew@openedhand.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/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.
> 
> But given the code already exists in tmp105 maybe it's fine?

So, I took my time to check this but yes, I think the code is fine.

However, tmp421 does not need to support 2 bytes writes so we can
simplify tmp421_tx() :

	static int tmp421_tx(I2CSlave *i2c, uint8_t data)
	{
	    TMP421State *s = TMP421(i2c);
	
	    if (s->len == 0) {
	        /* first byte is the register pointer for a read or write
	         * operation */
	        s->pointer = data;
	        s->len++;
	    } else if (s->len == 1) {
	        /* second byte is the data to write. The device only supports
	         * one byte writes */
	        s->buf[0] = data;
	        tmp421_write(s);
	    }
	
	    return 0;
	}

and tmp421 needs to support 2 bytes reads, so we need to extend a bit 
tmp421_read() when the temperatures are are. Linux does not use it 
so I guess we should use a command line tool to test.

I will send an updated patch for the TMP42{1,2,3} device with a larger 
patchset I am working on for Aspeed support. That is for 2.9.

Thanks,

C. 

 

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Qemu-devel] [PATCH 2/4] hw/misc: add a TMP42{1, 2, 3} device model
  2016-11-14  7:14   ` Cédric Le Goater
@ 2016-11-15  1:09     ` Andrew Jeffery
  0 siblings, 0 replies; 5+ messages in thread
From: Andrew Jeffery @ 2016-11-15  1:09 UTC (permalink / raw)
  To: Cédric Le Goater, Peter Maydell; +Cc: qemu-devel

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

On Mon, 2016-11-14 at 08:14 +0100, Cédric Le Goater wrote:
> > 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.
> > 
> > But given the code already exists in tmp105 maybe it's fine?
> 
> So, I took my time to check this but yes, I think the code is fine.

Yes, from memory it was okay, just not as obvious as I'd hoped it to
be.

> 
> However, tmp421 does not need to support 2 bytes writes so we can
> simplify tmp421_tx() :
> 
>         static int tmp421_tx(I2CSlave *i2c, uint8_t data)
>         {
>             TMP421State *s = TMP421(i2c);
>         
>             if (s->len == 0) {
>                 /* first byte is the register pointer for a read or write
>                  * operation */
>                 s->pointer = data;
>                 s->len++;
>             } else if (s->len == 1) {
>                 /* second byte is the data to write. The device only supports
>                  * one byte writes */
>                 s->buf[0] = data;
>                 tmp421_write(s);
>             }
>         
>             return 0;
>         }
> 
> and tmp421 needs to support 2 bytes reads, so we need to extend a bit 
> tmp421_read() when the temperatures are are. Linux does not use it 
> so I guess we should use a command line tool to test.

Okay.

> 
> I will send an updated patch for the TMP42{1,2,3} device with a larger 
> patchset I am working on for Aspeed support. That is for 2.9.
> 

Okay, I'll review again then.

Thanks,

Andrew

> Thanks,
> 
> C. 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2016-11-15  1:09 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-20 16:31 [Qemu-devel] [PATCH 2/4] hw/misc: add a TMP42{1,2,3} device model Cédric Le Goater
2016-05-23  3:53 ` [Qemu-devel] [PATCH 2/4] hw/misc: add a TMP42{1, 2, 3} " Andrew Jeffery
2016-05-23  5:50   ` Cédric Le Goater
2016-11-14  7:14   ` Cédric Le Goater
2016-11-15  1:09     ` Andrew Jeffery

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.