On Mon, Aug 6, 2012 at 7:38 PM, Peter Maydell <peter.mayd...@linaro.org> wrote: > On 6 August 2012 03:16, Peter A. G. Crosthwaite > <peter.crosthwa...@petalogix.com> 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 <peter.crosthwa...@petalogix.com> >> --- >> 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);
RE comment below, this should be in the else condition, ill move it. >> + >> + 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) My understanding is Anthony is doing major refactoring in the GPIO area anyways. Long term, will this code in qdev.c/irq.c even going to exist? In this patch, I took something of a path of least resistance to just make this series work, as I suspect this patch in its current form will be short lived due to continued QOM development. cc Andreas and Anthony. Regards, Peter > > >> + 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