| Message ID | 1edafd8e8b99a7fa09cdb122f240fb181fe5c928.1344218410.git.peter.crosthwaite@petalogix.com |
|---|---|
| State | New |
| Headers | show |
On 6 August 2012 03:16, Peter A. G. Crosthwaite <peter.crosthwaite@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.crosthwaite@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); > + > + 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
On Mon, Aug 6, 2012 at 7:38 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 6 August 2012 03:16, Peter A. G. Crosthwaite > <peter.crosthwaite@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.crosthwaite@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
On 7 August 2012 01:12, Peter Crosthwaite <peter.crosthwaite@petalogix.com> wrote: > On Mon, Aug 6, 2012 at 7:38 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 6 August 2012 03:16, Peter A. G. Crosthwaite >> <peter.crosthwaite@petalogix.com> wrote: >>> + 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. Yes, long term this will all disappear, but I wouldn't hold your breath (and indeed it's because I don't think the Pin reworking will hit before next release that I think this patch makes sense at all). But it will be around for a fair while, which is why the code should be in the right place. It's only adding a five or ten line function to irq.c. -- PMM
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); + } 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); + dev->gpio_in = all_irqs; + } + dev->num_gpio_in += n; } void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n)
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. Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com> --- hw/qdev.c | 16 +++++++++++++--- 1 files changed, 13 insertions(+), 3 deletions(-)