Re: [Qemu-devel] [PATCH v4 1/2] qdev: Implement named GPIOs

[Peter Maydell]

On 12 May 2014 02:03, Peter Crosthwaite <address@hidden> wrote:
> Implement named GPIOs on the Device layer. Listifies the existing GPIOs
> stuff using string keys. Legacy un-named GPIOs are preserved by using
> a NULL name string - they are just a single matchable element in the
> name list.
>
> Signed-off-by: Peter Crosthwaite <address@hidden>

Two very minor nits involving comments below, but otherwise

Reviewed-by: Peter Maydell <address@hidden>

and I would be happy for these patches to be committed
(through the QOM tree?)

> @@ -854,10 +907,19 @@ static void device_post_init(Object *obj)
>  /* Unlink device from bus and free the structure.  */
>  static void device_finalize(Object *obj)
>  {
> +    NamedGPIOList *ngl, *next;
> +
>      DeviceState *dev = DEVICE(obj);
>      if (dev->opts) {
>          qemu_opts_del(dev->opts);
>      }
> +
> +    QLIST_FOREACH_SAFE(ngl, &dev->gpios, node, next) {
> +        QLIST_REMOVE(ngl, node);
> +        qemu_free_irqs(ngl->in);
> +        g_free(ngl->name);
> +        g_free(ngl);

I think we could use a comment along the lines of
    /* ngl->out irqs are owned by the other end and should not be freed here */

to stop somebody later on wondering whether it's a bug
that we don't clean up that field in NamedGPIOList.

> @@ -253,10 +254,18 @@ static void qtest_process_command(CharDriverState *chr, 
> gchar **words)
>             return;
>          }
>
> -        if (words[0][14] == 'o') {
> -            qemu_irq_intercept_out(&dev->gpio_out, qtest_irq_handler, 
> dev->num_gpio_out);
> -        } else {
> -            qemu_irq_intercept_in(dev->gpio_in, qtest_irq_handler, 
> dev->num_gpio_in);
> +        QLIST_FOREACH(ngl, &dev->gpios, node) {
> +            /* We dont support intercept of named GPIOs yet */

"don't".

thanks
-- PMM