Patchwork [v5,05/15] qdev: allow multiple qdev_init_gpio_in() calls

login
register
mail settings
Submitter Peter A. G. Crosthwaite
Date Aug. 6, 2012, 2:16 a.m.
Message ID <1edafd8e8b99a7fa09cdb122f240fb181fe5c928.1344218410.git.peter.crosthwaite@petalogix.com>
Download mbox | patch
Permalink /patch/175231/
State New
Headers show

Comments

Peter A. G. Crosthwaite - Aug. 6, 2012, 2:16 a.m.
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(-)
Peter Maydell - Aug. 6, 2012, 9:38 a.m.
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
Peter A. G. Crosthwaite - Aug. 7, 2012, 12:12 a.m.
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
Peter Maydell - Aug. 7, 2012, 8:03 a.m.
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

Patch

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)