Re: [Qemu-devel] [PATCH v5 05/15] qdev: allow multiple qdev_init_gpio_in() calls

[Peter Maydell]

On 6 August 2012 03:16, Peter A. G. Crosthwaite
<address@hidden> wrote:
> Allow multiple qdev_init_gpio_in() calls for the one device. The first call 
> will
> define GPIOs 0-N-1, the next GPIOs N- ... . Allows different GPIOs to be 
> handled
> with different handlers. Needed when two levels of the QOM class heirachy both
> define GPIO functionality, as a single GPIO handler with an index selecter is
> not possible.

I generally like this idea and I think there are a few devices in the
existing codebase that could profitably use this rather than trying
to sort things out in the handler function. (Long term we would ideally
replace the single gpio array with a set of named arrays of Pins but
this is useful in the meantime.)

> Signed-off-by: Peter A. G. Crosthwaite <address@hidden>
> ---
>  hw/qdev.c |   16 +++++++++++++---
>  1 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index b5b74b9..ce91a72 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -293,9 +293,19 @@ BusState *qdev_get_parent_bus(DeviceState *dev)
>
>  void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n)
>  {
> -    assert(dev->num_gpio_in == 0);
> -    dev->num_gpio_in = n;
> -    dev->gpio_in = qemu_allocate_irqs(handler, dev, n);
> +    qemu_irq *new_irqs = qemu_allocate_irqs(handler, dev, n);
> +
> +    if (dev->num_gpio_in == 0) {
> +        dev->gpio_in = qemu_allocate_irqs(handler, dev, n);

In this case we've just called qemu_allocate_irqs() twice and leaked
the copy in new_irqs.

> +    } else {
> +        qemu_irq *all_irqs = g_new(qemu_irq, n + dev->num_gpio_in);
> +        memcpy(all_irqs, dev->gpio_in, sizeof(*all_irqs) * dev->num_gpio_in);
> +        g_free(dev->gpio_in);
> +        memcpy(&all_irqs[dev->num_gpio_in], new_irqs, sizeof(*all_irqs) * n),
> +        g_free(new_irqs);

I think this is rather looking into private details of what qemu_allocate_irqs
does, and it would be better to define a new qemu_reallocate_irqs() in irq.c
so we can use it here. (when doing so, consider whether g_renew() might help
in producing nice looking code)


> +        dev->gpio_in = all_irqs;
> +    }
> +    dev->num_gpio_in += n;
>  }
>
>  void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n)
> --
> 1.7.0.4
>


-- PMM