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é <f4...@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é <f4...@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.
