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

[Paolo Bonzini <pbonzini@redhat.com>]

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