qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Philippe Mathieu-Daudé
Subject: Re: [PATCH v5 2/7] hw/misc/led: Allow connecting from GPIO output
Date: Mon, 14 Sep 2020 17:05:24 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0

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.

Regards,

Phil.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]