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

[Alexander Graf <agraf@suse.de>]

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";
> 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.

>
>>    for (i = 0; ...) {
>>      char *propname = g_strdup_printf("%s[%d]", name, i);
>>      ....
>>    }
>>
>> Also I don't fully grasp what the naming scheme is supposed to be here. Who
>> sets the name and why is there only a single global name for all GPIOs?
>>
> Ideally, the instantiating device sets the names. There's not a global
> name for all GPIOs, just an over-used default. The intention is that
> qdev_init_gpio_in is phased out in favor of qdev_init_gpio_in_named.
> If NULL name is given or qdev_init_gpio_in is used, it defaults to
> this single global name here. All sysbus IRQs also share a  single
> name (seperate from the qdev default) but that sharing is implemented
> on the sysbus level and transparent to qdev. The need for a default
> name is to appease QOM, which needs a valid string for canonical path.

Ah, I missed the fact that qdev_get_named_gpio_list() returns a list of 
elements with exactly the name you searched for in the first place.


Alex