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

[Alexander Graf]

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


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