Re: [PATCH v5 2/7] hw/misc/led: Allow connecting from GPIO output

[Philippe Mathieu-Daudé]

Eduardo is already in Cc, adding Markus.

On 9/12/20 12:44 AM, Richard Henderson wrote:
> On 9/10/20 1:54 PM, Philippe Mathieu-Daudé wrote:
>> Some devices expose GPIO lines.
>>
>> Add a GPIO qdev input to our LED device, so we can
>> connect a GPIO output using qdev_connect_gpio_out().
>>
>> When used with GPIOs, the intensity can only be either
>> minium or maximum. This depends of the polarity of the
>> GPIO (which can be inverted).
>> Declare the GpioPolarity type to model the polarity.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  include/hw/misc/led.h  |  8 ++++++++
>>  include/hw/qdev-core.h |  8 ++++++++
>>  hw/misc/led.c          | 17 ++++++++++++++++-
>>  3 files changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
>> index f5afaa34bfb..71c9b8c59bf 100644
>> --- a/include/hw/misc/led.h
>> +++ b/include/hw/misc/led.h
>> @@ -38,10 +38,16 @@ typedef struct LEDState {
>>      /* Public */
>>  
>>      uint8_t intensity_percent;
>> +    qemu_irq irq;
>>  
>>      /* Properties */
>>      char *description;
>>      char *color;
>> +    /*
>> +     * When used with GPIO, the intensity at reset is related
>> +     * to the GPIO polarity.
>> +     */
>> +    bool inverted_polarity;
> 
> Why are you not using the GpioPolarity enum that you added?

Because it is migrated...

Using DEFINE_PROP_BOOL() is simpler that adding hardware specific
enum visitor in hw/core/qdev-properties.c (which is included in
user-mode builds because pulled by the CPU type).

A sane cleanup would be to make get_enum(), set_enum()
and set_default_value_enum() public (prefixed with 'qdev_')
in include/hw/qdev-properties.h.

Out of the scope of this series, but might be worth it.

Eduardo, Markus, what do you think?

Thanks Richard for reviewing this series!

Phil.