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é <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.
Already done in "user-mode: Prune build dependencies (part 3)"
actually :)
https://www.mail-archive.com/qemu-devel@nongnu.org/msg688867.html
