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

[Philippe Mathieu-Daudé]

On 9/12/20 10:50 AM, Philippe Mathieu-Daudé wrote:
> 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...

Luc made me realize we don't need to keep the enum in the device
state, a 'is_gpio_polarity_high' boolean is enough, so I don't
need to worry about migrating the enum.

https://www.mail-archive.com/qemu-devel@nongnu.org/msg739790.html

> 
> 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?

This question still stands however :)

> 
> Thanks Richard for reviewing this series!
> 
> Phil.
>