[Qemu-devel] [PATCH] tmp105: read temperature in milli-celsius

[Qemu-devel] [PATCH] tmp105: read temperature in milli-celsius

[Paolo Bonzini]

Right now, the temperature property must be written in milli-celsius, but it
reads back the value in 8.8 fixed point.  Fix this by letting the property
read back the original value (possibly rounded).  Also simplify the code that
does the conversion.

Before:

    (QEMU) qom-set path=/machine/peripheral/sensor property=temperature value=20000
    {u'return': {}}
    (QEMU) qom-get path=sensor property=temperature
    {u'return': 5120}

After:

    (QEMU) qom-set path=/machine/peripheral/sensor property=temperature value=20000
    {u'return': {}}
    (QEMU) qom-get path=sensor property=temperature
    {u'return': 20000}

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/misc/tmp105.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/misc/tmp105.c b/hw/misc/tmp105.c
index 155e03d..63aa3d6 100644
--- a/hw/misc/tmp105.c
+++ b/hw/misc/tmp105.c
@@ -56,12 +56,14 @@ static void tmp105_get_temperature(Object *obj, Visitor *v, void *opaque,
                                    const char *name, Error **errp)
 {
     TMP105State *s = TMP105(obj);
-    int64_t value = s->temperature;
+    int64_t value = s->temperature * 1000 / 256;
 
     visit_type_int(v, &value, name, errp);
 }
 
-/* Units are 0.001 centigrades relative to 0 C.  */
+/* 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 tmp105_set_temperature(Object *obj, Visitor *v, void *opaque,
                                    const char *name, Error **errp)
 {
@@ -78,7 +80,7 @@ static void tmp105_set_temperature(Object *obj, Visitor *v, void *opaque,
         return;
     }
 
-    s->temperature = ((int16_t) (temp * 0x800 / 128000)) << 4;
+    s->temperature = (int16_t) (temp * 256 / 1000);
 
     tmp105_alarm_update(s);
 }
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH] tmp105: read temperature in milli-celsius

[Andreas Färber]

Am 17.02.2014 11:57, schrieb Paolo Bonzini:
> Right now, the temperature property must be written in milli-celsius, but it
> reads back the value in 8.8 fixed point.  Fix this by letting the property
> read back the original value (possibly rounded).  Also simplify the code that
> does the conversion.
> 
> Before:
> 
>     (QEMU) qom-set path=/machine/peripheral/sensor property=temperature value=20000
>     {u'return': {}}
>     (QEMU) qom-get path=sensor property=temperature
>     {u'return': 5120}
> 
> After:
> 
>     (QEMU) qom-set path=/machine/peripheral/sensor property=temperature value=20000
>     {u'return': {}}
>     (QEMU) qom-get path=sensor property=temperature
>     {u'return': 20000}
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/misc/tmp105.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/misc/tmp105.c b/hw/misc/tmp105.c
> index 155e03d..63aa3d6 100644
> --- a/hw/misc/tmp105.c
> +++ b/hw/misc/tmp105.c
> @@ -56,12 +56,14 @@ static void tmp105_get_temperature(Object *obj, Visitor *v, void *opaque,
>                                     const char *name, Error **errp)
>  {
>      TMP105State *s = TMP105(obj);
> -    int64_t value = s->temperature;
> +    int64_t value = s->temperature * 1000 / 256;

Hmm, I'll have to check history, but I guess the setter was there and I
wrongly added the getter. That would be easier to ack of course if I
didn't have to think about a complete new formula in both places... ;)

>  
>      visit_type_int(v, &value, name, errp);
>  }
>  
> -/* Units are 0.001 centigrades relative to 0 C.  */
> +/* 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 tmp105_set_temperature(Object *obj, Visitor *v, void *opaque,
>                                     const char *name, Error **errp)
>  {
> @@ -78,7 +80,7 @@ static void tmp105_set_temperature(Object *obj, Visitor *v, void *opaque,
>          return;
>      }
>  
> -    s->temperature = ((int16_t) (temp * 0x800 / 128000)) << 4;
> +    s->temperature = (int16_t) (temp * 256 / 1000);

Did you check whether those magic 4 bits shift were for some other
purpose such as flags possibly? CC'ing Alex Horn.

Since we do have a tmp105-test, we should also add a regression test for
the getter bug.

Regards,
Andreas

>  
>      tmp105_alarm_update(s);
>  }
> 

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] tmp105: read temperature in milli-celsius

[Paolo Bonzini]

> > @@ -78,7 +80,7 @@ static void tmp105_set_temperature(Object *obj, Visitor
> > *v, void *opaque,
> >          return;
> >      }
> >  
> > -    s->temperature = ((int16_t) (temp * 0x800 / 128000)) << 4;
> > +    s->temperature = (int16_t) (temp * 256 / 1000);
> 
> Did you check whether those magic 4 bits shift were for some other
> purpose such as flags possibly? CC'ing Alex Horn.

>From the data sheet, the temperature register is 9-12 bits, depending
on the configuration.  So the low 4 bits will always read back as zero.
However, they are already masked away when reading the temperature in
tmp105_read:

        s->buf[s->len ++] = (((uint16_t) s->temperature) >> 8);
        s->buf[s->len ++] = (((uint16_t) s->temperature) >> 0) &
                (0xf0 << ((~s->config >> 5) & 3));              /* R */

The equivalence of the formula can be proved as follows:

   ((int16_t) (temp * 0x800 / 128000)) << 4
          = (int16_t) (temp * 0x8000 / 128000) & ~15
          = (int16_t) (temp * (0x100 * 0x80) / (1000 * 0x80)) & ~15
          = (int16_t) (temp * 0x100 / 1000) & ~15

and the AND can be removed as mentioned above.

> Since we do have a tmp105-test, we should also add a regression test for
> the getter bug.

Yeah, if only tmp105-test already tested the setter. :)

Paolo

Re: [Qemu-devel] [PATCH] tmp105: read temperature in milli-celsius

[Paolo Bonzini]

Il 17/02/2014 13:08, Andreas Färber ha scritto:
> Am 17.02.2014 11:57, schrieb Paolo Bonzini:
>> Right now, the temperature property must be written in milli-celsius, but it
>> reads back the value in 8.8 fixed point.  Fix this by letting the property
>> read back the original value (possibly rounded).  Also simplify the code that
>> does the conversion.
>>
>> Before:
>>
>>     (QEMU) qom-set path=/machine/peripheral/sensor property=temperature value=20000
>>     {u'return': {}}
>>     (QEMU) qom-get path=sensor property=temperature
>>     {u'return': 5120}
>>
>> After:
>>
>>     (QEMU) qom-set path=/machine/peripheral/sensor property=temperature value=20000
>>     {u'return': {}}
>>     (QEMU) qom-get path=sensor property=temperature
>>     {u'return': 20000}
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/misc/tmp105.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/misc/tmp105.c b/hw/misc/tmp105.c
>> index 155e03d..63aa3d6 100644
>> --- a/hw/misc/tmp105.c
>> +++ b/hw/misc/tmp105.c
>> @@ -56,12 +56,14 @@ static void tmp105_get_temperature(Object *obj, Visitor *v, void *opaque,
>>                                     const char *name, Error **errp)
>>  {
>>      TMP105State *s = TMP105(obj);
>> -    int64_t value = s->temperature;
>> +    int64_t value = s->temperature * 1000 / 256;
>
> Hmm, I'll have to check history, but I guess the setter was there and I
> wrongly added the getter. That would be easier to ack of course if I
> didn't have to think about a complete new formula in both places... ;)
>
>>
>>      visit_type_int(v, &value, name, errp);
>>  }
>>
>> -/* Units are 0.001 centigrades relative to 0 C.  */
>> +/* 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 tmp105_set_temperature(Object *obj, Visitor *v, void *opaque,
>>                                     const char *name, Error **errp)
>>  {
>> @@ -78,7 +80,7 @@ static void tmp105_set_temperature(Object *obj, Visitor *v, void *opaque,
>>          return;
>>      }
>>
>> -    s->temperature = ((int16_t) (temp * 0x800 / 128000)) << 4;
>> +    s->temperature = (int16_t) (temp * 256 / 1000);
>
> Did you check whether those magic 4 bits shift were for some other
> purpose such as flags possibly? CC'ing Alex Horn.
>
> Since we do have a tmp105-test, we should also add a regression test for
> the getter bug.

This is quite hard to do because the tmp105 is under /machine/unattached 
and thus does not have a stable QOM path.  The other way to test it 
would be to add a libqos driver for the i386 smbus device, add tmp105 to 
qemu-system-x86_64, and write another tmp105 test.

Given this, is this patch okay for 2.0?

Paolo