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

[Philippe Mathieu-Daudé]

On 9/14/20 9:27 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
>> 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).
> 
> Yes.
> 
> You could also use DEFINE_PROP_UINT8(), and use it with your enumeration
> constants.
> 
> I'd be tempted to ditch the enum entirely.  Matter of taste, no big deal
> either way.

Done in v6!

> 
>> 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.
> 
> Purpose?  To be able to define enum properties outside
> qdev-properties.c?

Yes, to avoid pulling in PCI and MAC address properties
into qemu-storage-daemon and linux-user binaries...