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 <c...@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 <and...@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)
