From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49579) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b4ilD-0004LL-Sk for qemu-devel@nongnu.org; Mon, 23 May 2016 01:50:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b4il7-0001bW-Sx for qemu-devel@nongnu.org; Mon, 23 May 2016 01:50:38 -0400 Received: from 19.mo3.mail-out.ovh.net ([178.32.98.231]:51895) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b4il7-0001b6-8Q for qemu-devel@nongnu.org; Mon, 23 May 2016 01:50:33 -0400 Received: from player772.ha.ovh.net (b7.ovh.net [213.186.33.57]) by mo3.mail-out.ovh.net (Postfix) with ESMTP id 36542FFB211 for ; Mon, 23 May 2016 07:50:31 +0200 (CEST) References: <1463761907-15439-1-git-send-email-clg@kaod.org> <1463975622.2703.262.camel@aj.id.au> From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Message-ID: <57429A15.5000606@kaod.org> Date: Mon, 23 May 2016 07:50:13 +0200 MIME-Version: 1.0 In-Reply-To: <1463975622.2703.262.camel@aj.id.au> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/4] hw/misc: add a TMP42{1, 2, 3} device model List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: andrew@aj.id.au, Peter Maydell Cc: qemu-devel@nongnu.org On 05/23/2016 05:53 AM, Andrew Jeffery wrote: > Hi C=C3=A9dric, >=20 > On Fri, 2016-05-20 at 18:31 +0200, C=C3=A9dric 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=C3=A9dric Le Goater >> --- >> 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) +=3D applesmc.o >> common-obj-$(CONFIG_MAX111X) +=3D max111x.o >> common-obj-$(CONFIG_TMP105) +=3D tmp105.o >> +common-obj-$(CONFIG_ASPEED_SOC) +=3D tmp421.o >> common-obj-$(CONFIG_ISA_DEBUG) +=3D debugexit.o >> common-obj-$(CONFIG_SGA) +=3D sga.o >> common-obj-$(CONFIG_ISA_TESTDEV) +=3D 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 >> + * >> + * 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 . >> + */ >> + >> +#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[] =3D { >> + { 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; >=20 > 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.=20 Anyhow I wanted to change this config option : +common-obj-$(CONFIG_ASPEED_SOC) +=3D tmp421.o as there is no reason not to export the device to other platforms. Thanks C.=20 > But given the code already exists in tmp105 maybe it's fine? >=20 > Cheers, >=20 > Andrew >=20 >> + 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] =3D { -40000, -55000 }; >> +static const int32_t maxs[2] =3D { 127000, 150000 }; >> + >> +static void tmp421_get_temperature(Object *obj, Visitor *v, const cha= r *name, >> + void *opaque, Error **errp) >> +{ >> + TMP421State *s =3D TMP421(obj); >> + bool ext_range =3D (s->config[0] & TMP421_CONFIG_RANGE); >> + int offset =3D ext_range * 64 * 256; >> + int64_t value; >> + int tempid; >> + >> + if (sscanf(name, "temperature%d", &tempid) !=3D 1) { >> + error_setg(errp, "error reading %s: %m", name); >> + return; >> + } >> + >> + if (tempid >=3D 4 || tempid < 0) { >> + error_setg(errp, "error reading %s", name); >> + return; >> + } >> + >> + value =3D ((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 cha= r *name, >> + void *opaque, Error **errp) >> +{ >> + TMP421State *s =3D TMP421(obj); >> + Error *local_err =3D NULL; >> + int64_t temp; >> + bool ext_range =3D (s->config[0] & TMP421_CONFIG_RANGE); >> + int offset =3D 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 >=3D maxs[ext_range] || temp < mins[ext_range]) { >> + error_setg(errp, "value %" PRId64 ".%03" PRIu64 " =C2=B0C is = out of range", >> + temp / 1000, temp % 1000); >> + return; >> + } >> + >> + if (sscanf(name, "temperature%d", &tempid) !=3D 1) { >> + error_setg(errp, "error reading %s: %m", name); >> + return; >> + } >> + >> + if (tempid >=3D 4 || tempid < 0) { >> + error_setg(errp, "error reading %s", name); >> + return; >> + } >> + >> + s->temperature[tempid] =3D (int16_t) ((temp * 256 - 128) / 1000) = + offset; >> +} >> + >> +static void tmp421_read(TMP421State *s) >> +{ >> + TMP421Class *sc =3D TMP421_GET_CLASS(s); >> + >> + s->len =3D 0; >> + >> + switch (s->pointer) { >> + case TMP421_MANUFACTURER_ID_REG: >> + s->buf[s->len++] =3D TMP421_MANUFACTURER_ID; >> + break; >> + case TMP421_DEVICE_ID_REG: >> + s->buf[s->len++] =3D sc->dev->model; >> + break; >> + case TMP421_CONFIG_REG_1: >> + s->buf[s->len++] =3D s->config[0]; >> + break; >> + case TMP421_CONFIG_REG_2: >> + s->buf[s->len++] =3D s->config[1]; >> + break; >> + case TMP421_CONVERSION_RATE_REG: >> + s->buf[s->len++] =3D s->rate; >> + break; >> + case TMP421_STATUS_REG: >> + s->buf[s->len++] =3D s->status; >> + break; >> + >> + /* FIXME: check for channel enablement in config registers */ >> + case TMP421_TEMP_MSB0: >> + s->buf[s->len++] =3D (((uint16_t) s->temperature[0]) >> 8); >> + break; >> + case TMP421_TEMP_MSB1: >> + s->buf[s->len++] =3D (((uint16_t) s->temperature[1]) >> 8); >> + break; >> + case TMP421_TEMP_MSB2: >> + s->buf[s->len++] =3D (((uint16_t) s->temperature[2]) >> 8); >> + break; >> + case TMP421_TEMP_MSB3: >> + s->buf[s->len++] =3D (((uint16_t) s->temperature[3]) >> 8); >> + break; >> + case TMP421_TEMP_LSB0: >> + s->buf[s->len++] =3D (((uint16_t) s->temperature[0]) >> 0) & = 0xf0; >> + break; >> + case TMP421_TEMP_LSB1: >> + s->buf[s->len++] =3D (((uint16_t) s->temperature[1]) >> 0) & = 0xf0; >> + break; >> + case TMP421_TEMP_LSB2: >> + s->buf[s->len++] =3D (((uint16_t) s->temperature[2]) >> 0) & = 0xf0; >> + break; >> + case TMP421_TEMP_LSB3: >> + s->buf[s->len++] =3D (((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 =3D s->buf[0]; >> + break; >> + case TMP421_CONFIG_REG_1: >> + s->config[0] =3D s->buf[0]; >> + break; >> + case TMP421_CONFIG_REG_2: >> + s->config[1] =3D s->buf[0]; >> + break; >> + case TMP421_RESET: >> + tmp421_reset(I2C_SLAVE(s)); >> + break; >> + } >> +} >> + >> +static int tmp421_rx(I2CSlave *i2c) >> +{ >> + TMP421State *s =3D 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 =3D TMP421(i2c); >> + >> + if (s->len =3D=3D 0) { >> + s->pointer =3D data; >> + s->len++; >> + } else { >> + if (s->len <=3D 2) { >> + s->buf[s->len - 1] =3D data; >> + } >> + s->len++; >> + tmp421_write(s); >> + } >> + >> + return 0; >> +} >> + >> +static void tmp421_event(I2CSlave *i2c, enum i2c_event event) >> +{ >> + TMP421State *s =3D TMP421(i2c); >> + >> + if (event =3D=3D I2C_START_RECV) { >> + tmp421_read(s); >> + } >> + >> + s->len =3D 0; >> +} >> + >> +static const VMStateDescription vmstate_tmp421 =3D { >> + .name =3D "TMP421", >> + .version_id =3D 0, >> + .minimum_version_id =3D 0, >> + .fields =3D (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 =3D TMP421(i2c); >> + TMP421Class *sc =3D TMP421_GET_CLASS(s); >> + >> + memset(s->temperature, 0, sizeof(s->temperature)); >> + s->pointer =3D 0; >> + >> + s->config[0] =3D 0; /* TMP421_CONFIG_RANGE */ >> + >> + /* resistance correction and channel enablement */ >> + switch (sc->dev->model) { >> + case TMP421_DEVICE_ID: >> + s->config[1] =3D 0x1c; >> + break; >> + case TMP422_DEVICE_ID: >> + s->config[1] =3D 0x3c; >> + break; >> + case TMP423_DEVICE_ID: >> + s->config[1] =3D 0x7c; >> + break; >> + } >> + >> + s->rate =3D 0x7; /* 8Hz */ >> + s->status =3D 0; >> +} >> + >> +static int tmp421_init(I2CSlave *i2c) >> +{ >> + TMP421State *s =3D 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 =3D DEVICE_CLASS(klass); >> + I2CSlaveClass *k =3D I2C_SLAVE_CLASS(klass); >> + TMP421Class *sc =3D TMP421_CLASS(klass); >> + >> + k->init =3D tmp421_init; >> + k->event =3D tmp421_event; >> + k->recv =3D tmp421_rx; >> + k->send =3D tmp421_tx; >> + dc->vmsd =3D &vmstate_tmp421; >> + sc->dev =3D (DeviceInfo *) data; >> +} >> + >> +static const TypeInfo tmp421_info =3D { >> + .name =3D TYPE_TMP421, >> + .parent =3D TYPE_I2C_SLAVE, >> + .instance_size =3D sizeof(TMP421State), >> + .instance_init =3D tmp421_initfn, >> + .class_init =3D tmp421_class_init, >> +}; >> + >> +static void tmp421_register_types(void) >> +{ >> + int i; >> + >> + type_register_static(&tmp421_info); >> + for (i =3D 0; i < ARRAY_SIZE(devices); ++i) { >> + TypeInfo ti =3D { >> + .name =3D devices[i].name, >> + .parent =3D TYPE_TMP421, >> + .class_init =3D tmp421_class_init, >> + .class_data =3D (void *) &devices[i], >> + }; >> + type_register(&ti); >> + } >> +} >> + >> +type_init(tmp421_register_types)