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

[Philippe Mathieu-Daudé]

On 9/14/20 5:05 PM, Philippe Mathieu-Daudé wrote:
> On 9/14/20 4:03 PM, Eduardo Habkost wrote:
>> 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.
> 
> My guess is that as get_enum()/set_enum()/set_default_value_enum()
> are static, enum properties using it ended in this core file:
> 
> const PropertyInfo qdev_prop_bios_chs_trans = {
>     .name = "BiosAtaTranslation",
>     .description = "Logical CHS translation algorithm, "
>                    "auto/none/lba/large/rechs",
>     .enum_table = &BiosAtaTranslation_lookup,
>     .get = get_enum,
>     .set = set_enum,
>     .set_default_value = set_default_value_enum,
> };
> 
> const PropertyInfo qdev_prop_fdc_drive_type = {
>     .name = "FdcDriveType",
>     .description = "FDC drive type, "
>                    "144/288/120/none/auto",
>     .enum_table = &FloppyDriveType_lookup,
>     .get = get_enum,
>     .set = set_enum,
>     .set_default_value = set_default_value_enum,
> };
> 
> const PropertyInfo qdev_prop_pcie_link_speed = {
>     .name = "PCIELinkSpeed",
>     .description = "2_5/5/8/16",
>     .enum_table = &PCIELinkSpeed_lookup,
>     .get = get_prop_pcielinkspeed,
>     .set = set_prop_pcielinkspeed,
>     .set_default_value = set_default_value_enum,
> };
> 
> Looking at qdev_prop_macaddr[] and qdev_prop_set_macaddr()
> maybe I was remembering incorrectly, it seems this one can
> be extracted easily.

Already done in "user-mode: Prune build dependencies (part 3)"
actually :)
https://www.mail-archive.com/qemu-devel@nongnu.org/msg688867.html