From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45647) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tg0OA-0001E1-Av for qemu-devel@nongnu.org; Tue, 04 Dec 2012 16:50:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tg0O7-0000lL-WE for qemu-devel@nongnu.org; Tue, 04 Dec 2012 16:50:50 -0500 Received: from cantor2.suse.de ([195.135.220.15]:43311 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tg0O7-0000lG-Nn for qemu-devel@nongnu.org; Tue, 04 Dec 2012 16:50:47 -0500 Message-ID: <50BE702B.6060900@suse.de> Date: Tue, 04 Dec 2012 22:50:35 +0100 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1354648993-27263-1-git-send-email-alex.horn@cs.ox.ac.uk> In-Reply-To: <1354648993-27263-1-git-send-email-alex.horn@cs.ox.ac.uk> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/1] tmp105: Create API for TMP105 temperature sensor. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Horn Cc: Peter Maydell , qemu-devel@nongnu.org, Anthony Liguori Am 04.12.2012 20:23, schrieb Alex Horn: > * Define constants for TMP105 registers. > * Move tmp105_set() from I2C to TMP105 header. >=20 > Signed-off-by: Alex Horn CC'ing TMP105 author and ARM maintainer. Comments inline. > --- > hw/i2c.h | 3 --- > hw/tmp105.c | 17 +++++++++-------- > hw/tmp105.h | 34 ++++++++++++++++++++++++++++++++++ > 3 files changed, 43 insertions(+), 11 deletions(-) > create mode 100644 hw/tmp105.h >=20 > diff --git a/hw/i2c.h b/hw/i2c.h > index 0f5682b..883b5c5 100644 > --- a/hw/i2c.h > +++ b/hw/i2c.h > @@ -73,9 +73,6 @@ void *wm8750_dac_buffer(void *opaque, int samples); > void wm8750_dac_commit(void *opaque); > void wm8750_set_bclk_in(void *opaque, int new_hz); > =20 > -/* tmp105.c */ > -void tmp105_set(I2CSlave *i2c, int temp); > - > /* lm832x.c */ > void lm832x_key_event(DeviceState *dev, int key, int state); > =20 > diff --git a/hw/tmp105.c b/hw/tmp105.c > index 8e8dbd9..9c67e64 100644 > --- a/hw/tmp105.c > +++ b/hw/tmp105.c > @@ -20,6 +20,7 @@ > =20 > #include "hw.h" > #include "i2c.h" > +#include "tmp105.h" > =20 > typedef struct { > I2CSlave i2c; > @@ -92,22 +93,22 @@ static void tmp105_read(TMP105State *s) > } > =20 > switch (s->pointer & 3) { > - case 0: /* Temperature */ > + case TMP105_REG_TEMPERATURE: > s->buf[s->len ++] =3D (((uint16_t) s->temperature) >> 8); > s->buf[s->len ++] =3D (((uint16_t) s->temperature) >> 0) & > (0xf0 << ((~s->config >> 5) & 3)); /* R */ > break; > =20 > - case 1: /* Configuration */ > + case TMP105_REG_CONFIG: > s->buf[s->len ++] =3D s->config; > break; > =20 > - case 2: /* T_LOW */ > + case TMP105_REG_T_LOW: > s->buf[s->len ++] =3D ((uint16_t) s->limit[0]) >> 8; > s->buf[s->len ++] =3D ((uint16_t) s->limit[0]) >> 0; > break; > =20 > - case 3: /* T_HIGH */ > + case TMP105_REG_T_HIGH: > s->buf[s->len ++] =3D ((uint16_t) s->limit[1]) >> 8; > s->buf[s->len ++] =3D ((uint16_t) s->limit[1]) >> 0; > break; > @@ -117,10 +118,10 @@ static void tmp105_read(TMP105State *s) > static void tmp105_write(TMP105State *s) > { > switch (s->pointer & 3) { > - case 0: /* Temperature */ > + case TMP105_REG_TEMPERATURE: > break; > =20 > - case 1: /* Configuration */ > + case TMP105_REG_CONFIG: > if (s->buf[0] & ~s->config & (1 << 0)) /* SD */ > printf("%s: TMP105 shutdown\n", __FUNCTION__); > s->config =3D s->buf[0]; > @@ -128,8 +129,8 @@ static void tmp105_write(TMP105State *s) > tmp105_alarm_update(s); > break; > =20 > - case 2: /* T_LOW */ > - case 3: /* T_HIGH */ > + case TMP105_REG_T_LOW: > + case TMP105_REG_T_HIGH: > if (s->len >=3D 3) > s->limit[s->pointer & 1] =3D (int16_t) > ((((uint16_t) s->buf[0]) << 8) | s->buf[1]); > diff --git a/hw/tmp105.h b/hw/tmp105.h > new file mode 100644 > index 0000000..52aa4c9 > --- /dev/null > +++ b/hw/tmp105.h Adding a dedicated header is a good idea. In the future we may want to move TMP105State struct there, too, and add a TYPE_TMP105 constant. However, a new header should get a description and appropriate license, since you are moving a declaration from tmp105.c here that probably means GPLv2+. > @@ -0,0 +1,34 @@ > +#ifndef QEMU_TMP105_H > +#define QEMU_TMP105_H > + > +#include "i2c.h" > + > +/* The following temperature sensors are > + * compatible with the TMP105 registers: > + * > + * adt75 > + * ds1775 > + * ds75 > + * lm75 > + * lm75a > + * max6625 > + * max6626 > + * mcp980x > + * stds75 > + * tcn75 > + * tmp100 > + * tmp101 > + * tmp105 > + * tmp175 > + * tmp275 > + * tmp75 > + */ > +#define TMP105_REG_TEMPERATURE 0 > +#define TMP105_REG_CONFIG 1 > +#define TMP105_REG_T_LOW 2 /* also known as T_hyst (e.g. LM75) *= / > +#define TMP105_REG_T_HIGH 3 /* also known as T_OS (e.g. LM75) */ Might it make sense to use an enum here to improve the gdb experience? > + > +/* See also I2C_SLAVE_FROM_QDEV macro */ This comment did not exist before and I veto it: Instead, people should be using the new I2C_SLAVE() macro, which works for Object just as well as for DeviceState. (git-grep counts six I2C_SLAVE_FROM_QDEV() users only, but I'd rather get my ISA series merged before I send patches for yet another bus.) > +void tmp105_set(I2CSlave *i2c, int temp); Could you add a proper gtk-doc-style comment for this function, documentation its parameters? Like, is @temp in Celsius, Fahrenheit, Kelvin? Any range limits? > + > +#endif Regards, Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg