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

[Eduardo Habkost]

On Mon, Sep 14, 2020 at 09:48:45AM +0200, Philippe Mathieu-Daudé wrote:
> 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.
> > 

Where and how exactly do you expect those functions to be used?
Having additional PropertyInfo structs defined manually would not
be desirable (especially if they are outside qdev*.c).  Having a
DEFINE_PROP_ENUM macro that does the enum magic behind the scenes
would be nice.

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

I don't understand what enum functions have to do with PCI and
MAC address properties.

-- 
Eduardo