Patchwork [1/1] tmp105: Create API for TMP105 temperature sensor.

login
register
mail settings
Submitter Alex Horn
Date Dec. 4, 2012, 7:23 p.m.
Message ID <1354648993-27263-1-git-send-email-alex.horn@cs.ox.ac.uk>
Download mbox | patch
Permalink /patch/203725/
State New
Headers show

Comments

Alex Horn - Dec. 4, 2012, 7:23 p.m.
* Define constants for TMP105 registers.
* Move tmp105_set() from I2C to TMP105 header.

Signed-off-by: Alex Horn <alex.horn@cs.ox.ac.uk>
---
 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
Andreas Färber - Dec. 4, 2012, 9:50 p.m.
Am 04.12.2012 20:23, schrieb Alex Horn:
> * Define constants for TMP105 registers.
> * Move tmp105_set() from I2C to TMP105 header.
> 
> Signed-off-by: Alex Horn <alex.horn@cs.ox.ac.uk>

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
> 
> 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);
>  
> -/* tmp105.c */
> -void tmp105_set(I2CSlave *i2c, int temp);
> -
>  /* lm832x.c */
>  void lm832x_key_event(DeviceState *dev, int key, int state);
>  
> 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 @@
>  
>  #include "hw.h"
>  #include "i2c.h"
> +#include "tmp105.h"
>  
>  typedef struct {
>      I2CSlave i2c;
> @@ -92,22 +93,22 @@ static void tmp105_read(TMP105State *s)
>      }
>  
>      switch (s->pointer & 3) {
> -    case 0:	/* Temperature */
> +    case TMP105_REG_TEMPERATURE:
>          s->buf[s->len ++] = (((uint16_t) s->temperature) >> 8);
>          s->buf[s->len ++] = (((uint16_t) s->temperature) >> 0) &
>                  (0xf0 << ((~s->config >> 5) & 3));		/* R */
>          break;
>  
> -    case 1:	/* Configuration */
> +    case TMP105_REG_CONFIG:
>          s->buf[s->len ++] = s->config;
>          break;
>  
> -    case 2:	/* T_LOW */
> +    case TMP105_REG_T_LOW:
>          s->buf[s->len ++] = ((uint16_t) s->limit[0]) >> 8;
>          s->buf[s->len ++] = ((uint16_t) s->limit[0]) >> 0;
>          break;
>  
> -    case 3:	/* T_HIGH */
> +    case TMP105_REG_T_HIGH:
>          s->buf[s->len ++] = ((uint16_t) s->limit[1]) >> 8;
>          s->buf[s->len ++] = ((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;
>  
> -    case 1:	/* Configuration */
> +    case TMP105_REG_CONFIG:
>          if (s->buf[0] & ~s->config & (1 << 0))			/* SD */
>              printf("%s: TMP105 shutdown\n", __FUNCTION__);
>          s->config = s->buf[0];
> @@ -128,8 +129,8 @@ static void tmp105_write(TMP105State *s)
>          tmp105_alarm_update(s);
>          break;
>  
> -    case 2:	/* T_LOW */
> -    case 3:	/* T_HIGH */
> +    case TMP105_REG_T_LOW:
> +    case TMP105_REG_T_HIGH:
>          if (s->len >= 3)
>              s->limit[s->pointer & 1] = (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

Patch

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);
 
-/* tmp105.c */
-void tmp105_set(I2CSlave *i2c, int temp);
-
 /* lm832x.c */
 void lm832x_key_event(DeviceState *dev, int key, int state);
 
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 @@ 
 
 #include "hw.h"
 #include "i2c.h"
+#include "tmp105.h"
 
 typedef struct {
     I2CSlave i2c;
@@ -92,22 +93,22 @@  static void tmp105_read(TMP105State *s)
     }
 
     switch (s->pointer & 3) {
-    case 0:	/* Temperature */
+    case TMP105_REG_TEMPERATURE:
         s->buf[s->len ++] = (((uint16_t) s->temperature) >> 8);
         s->buf[s->len ++] = (((uint16_t) s->temperature) >> 0) &
                 (0xf0 << ((~s->config >> 5) & 3));		/* R */
         break;
 
-    case 1:	/* Configuration */
+    case TMP105_REG_CONFIG:
         s->buf[s->len ++] = s->config;
         break;
 
-    case 2:	/* T_LOW */
+    case TMP105_REG_T_LOW:
         s->buf[s->len ++] = ((uint16_t) s->limit[0]) >> 8;
         s->buf[s->len ++] = ((uint16_t) s->limit[0]) >> 0;
         break;
 
-    case 3:	/* T_HIGH */
+    case TMP105_REG_T_HIGH:
         s->buf[s->len ++] = ((uint16_t) s->limit[1]) >> 8;
         s->buf[s->len ++] = ((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;
 
-    case 1:	/* Configuration */
+    case TMP105_REG_CONFIG:
         if (s->buf[0] & ~s->config & (1 << 0))			/* SD */
             printf("%s: TMP105 shutdown\n", __FUNCTION__);
         s->config = s->buf[0];
@@ -128,8 +129,8 @@  static void tmp105_write(TMP105State *s)
         tmp105_alarm_update(s);
         break;
 
-    case 2:	/* T_LOW */
-    case 3:	/* T_HIGH */
+    case TMP105_REG_T_LOW:
+    case TMP105_REG_T_HIGH:
         if (s->len >= 3)
             s->limit[s->pointer & 1] = (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
@@ -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) */
+
+/* See also I2C_SLAVE_FROM_QDEV macro */
+void tmp105_set(I2CSlave *i2c, int temp);
+
+#endif