Re: [Qemu-devel] [PATCH v1 13/16] qdev: gpio: Define qdev_pass_gpios()

[Alexander Graf <agraf@suse.de>]

On 15.08.14 07:11, Peter Crosthwaite wrote:
> On Tue, Aug 12, 2014 at 8:55 PM, Alexander Graf <agraf@suse.de> wrote:
>> On 12.08.14 12:48, Peter Crosthwaite wrote:
>>> On Tue, Aug 12, 2014 at 7:24 PM, Alexander Graf <agraf@suse.de> wrote:
>>>> On 04.08.14 03:58, Peter Crosthwaite wrote:
>>>>> Allows a container to take ownership of GPIOs in a contained
>>>>> device and automatically connect them as GPIOs to the container.
>>>>>
>>>>> This prepares for deprecation of the SYSBUS IRQ functionality, which
>>>>> has this feature. We push it up to the device level instead of sysbus
>>>>> level. There's nothing sysbus specific about passing GPIOs to
>>>>> containers so its a legitimate device-level generic feature.
>>>>>
>>>>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>>>> ---
>>>>>
>>>>>     hw/core/qdev.c         | 28 ++++++++++++++++++++++++++++
>>>>>     include/hw/qdev-core.h |  3 +++
>>>>>     2 files changed, 31 insertions(+)
>>>>>
>>>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>>>> index bf2c227..708363f 100644
>>>>> --- a/hw/core/qdev.c
>>>>> +++ b/hw/core/qdev.c
>>>>> @@ -440,6 +440,34 @@ void qdev_connect_gpio_out(DeviceState * dev, int
>>>>> n,
>>>>> qemu_irq pin)
>>>>>         qdev_connect_gpio_out_named(dev, NULL, n, pin);
>>>>>     }
>>>>>     +void qdev_pass_gpios(DeviceState *dev, DeviceState *container,
>>>>> +                     const char *name)
>>>>> +{
>>>>> +    int i;
>>>>> +    NamedGPIOList *ngl = qdev_get_named_gpio_list(dev, name);
>>>>> +
>>>>> +    for (i = 0; i < ngl->num_in; i++) {
>>>>> +        char *propname = g_strdup_printf("%s[%d]",
>>>>> +                                         ngl->name ? ngl->name :
>>>>> +                                                   "unnamed-gpio-in",
>>>>
>>>> Really just a minor nit, but I think the code flow would look a lot nicer
>>>> if
>>>> you did the name check in a separate variable.
>>>>
>>>>     const char *name = ngl->name ? ngl->name : "unnamed-gpio-in";
> Just done it this way. Although have used "nm" instead of "name"
> instead as "name" is already used.
>
>>> I think I may even go an extra step and get it macroified. How about:
>>>
>>> #define QDEV_GPIO_IN_NAME(a) ((a) ? (a) : "unnamed-gpio-io")
>>>
>>> and then you can continue to use it inline without extra variables or
>>> nasty "?:"?
>>
>> The variable will get optimized out, so for the sake of readability I would
>> still vote to have it around. Whether you declare it via a macro or with an
>> a ? a : b macro I don't really care much about :).
>>
>> In fact, it might make more sense to literally have the typical
>>
>>    (a ? a : b)
>>
>> flow macroified in a generic place somewhere.
>>
> Hmm not sure what this wins though? inline "? :" is concise and self
> documenting IMO. You just need to not stuff-up the "()"s (which is
> admittedly quite easy to do.
>
> Anyways, it's a patch for another day.

Yeah, probably not a big win really :).


Alex