Re: [PATCH v3 3/7] hw/misc/led: Emit a trace event when LED intensity has changed

[Philippe Mathieu-Daudé]

On 6/21/20 9:23 PM, Richard Henderson wrote:
> On 6/20/20 4:07 PM, Philippe Mathieu-Daudé wrote:
>> Track the LED intensity, and emit a trace event when it changes.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  include/hw/misc/led.h | 1 +
>>  hw/misc/led.c         | 5 +++++
>>  hw/misc/trace-events  | 1 +
>>  3 files changed, 7 insertions(+)
>>
>> diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
>> index 883006bb8f..df5b32a2db 100644
>> --- a/include/hw/misc/led.h
>> +++ b/include/hw/misc/led.h
>> @@ -35,6 +35,7 @@ typedef struct LEDState {
>>      DeviceState parent_obj;
>>      /* Public */
>>  
>> +    uint16_t current_intensity;
>>      qemu_irq irq;
> 
> Why not sort this new field next to the other uint16_t and (partially) fill 
> the
> hole?

>From a reviewer point of view, I prefer to keep the state fields
separated from the properties fields, wasting few bits of RAM.

Anyway I switched to a percent value. What is better to hold
it, an 'unsigned' or 'uint8_t' type?

> 
> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Thanks!

> 
> 
> r~
>