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

[Peter Crosthwaite]

On Tue, Aug 12, 2014 at 7:24 PM, Alexander Graf <address@hidden> 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 <address@hidden>
>> ---
>>
>>   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 "?:"?

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

Regards,
Peter

>
> Alex
>
>
>> +                                         i);
>> +        object_property_add_alias(OBJECT(container), propname,
>> +                                  OBJECT(dev), propname,
>> +                                  &error_abort);
>> +    }
>> +    for (i = 0; i < ngl->num_out; i++) {
>> +        char *propname = g_strdup_printf("%s[%d]",
>> +                                         ngl->name ? ngl->name :
>> +                                                     "unnamed-gpio-in",
>> +                                         i);
>> +        object_property_add_alias(OBJECT(container), propname,
>> +                                  OBJECT(dev), propname,
>> +                                  &error_abort);
>> +    }
>> +    QLIST_REMOVE(ngl, node);
>> +    QLIST_INSERT_HEAD(&container->gpios, ngl, node);
>> +}
>> +
>>   BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
>>   {
>>       BusState *bus;
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index d3326b1..08dafda 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -289,6 +289,9 @@ void qdev_init_gpio_in_named(DeviceState *dev,
>> qemu_irq_handler handler,
>>   void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
>>                                 const char *name, int n);
>>   +void qdev_pass_gpios(DeviceState *dev, DeviceState *container,
>> +                     const char *name);
>> +
>>   BusState *qdev_get_parent_bus(DeviceState *dev);
>>     /*** BUS API. ***/
>
>
>