Patchwork [v7,04/13] qdev: allow multiple qdev_init_gpio_in() calls

login
register
mail settings
Submitter Peter A. G. Crosthwaite
Date Sept. 24, 2012, 9:18 a.m.
Message ID <f521d51b217517a3d19b37f3e8fe611063453979.1348104012.git.peter.crosthwaite@petalogix.com>
Download mbox | patch
Permalink /patch/186341/
State New
Headers show

Comments

Peter A. G. Crosthwaite - Sept. 24, 2012, 9:18 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>
---
changed since v5:
moved implementation to irq.c as per PMM review

 hw/irq.c  |   17 ++++++++++++++---
 hw/irq.h  |   11 ++++++++++-
 hw/qdev.c |    6 +++---
 3 files changed, 27 insertions(+), 7 deletions(-)
Peter Maydell - Sept. 24, 2012, 9:53 a.m.
On 24 September 2012 10:18, 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.
>
> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
> ---
> changed since v5:
> moved implementation to irq.c as per PMM review

Thanks for this change -- I think it looks much cleaner this way.

>  hw/irq.c  |   17 ++++++++++++++---
>  hw/irq.h  |   11 ++++++++++-
>  hw/qdev.c |    6 +++---
>  3 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/hw/irq.c b/hw/irq.c
> index d413a0b..cd17551 100644
> --- a/hw/irq.c
> +++ b/hw/irq.c
> @@ -38,15 +38,20 @@ void qemu_set_irq(qemu_irq irq, int level)
>      irq->handler(irq->opaque, irq->n, level);
>  }
>
> -qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler, void *opaque, int n)
> +qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
> +                                void *opaque, int n)

Your indent is a little odd here (and below) -- usually the parameters
in the second line are aligned with the first (so the 'v' is in the
same column as the 'q').

>  {
>      qemu_irq *s;
>      struct IRQState *p;
>      int i;
>
> -    s = (qemu_irq *)g_malloc0(sizeof(qemu_irq) * n);
> +    if (!old) {
> +        n_old = 0;
> +    }
> +    s = old ? g_renew(qemu_irq, old, n + n_old) :
> +                    (qemu_irq *)g_new0(qemu_irq, n);

You don't need to cast the return value from g_new0().
Also, this isn't consistent -- either we don't need the
memory zeroed (in which case use g_new rather than g_new0)
or we do (in which case we need to do something about
the new memory g_renew() returns).

>      p = (struct IRQState *)g_malloc0(sizeof(struct IRQState) * n);

qemu_free_irqs() relies on all the struct IRQState *s being
freeable with a single g_free(), so you need to g_renew the
existing array here.

> -    for (i = 0; i < n; i++) {
> +    for (i = n_old; i < n + n_old; i++) {
>          p->handler = handler;
>          p->opaque = opaque;
>          p->n = i;
> @@ -56,6 +61,12 @@ qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler, void *opaque, int n)
>      return s;
>  }
>
> +qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler, void *opaque, int n)
> +{
> +    return qemu_extend_irqs(NULL, 0, handler, opaque, n);
> +}
> +
> +
>  void qemu_free_irqs(qemu_irq *s)
>  {
>      g_free(s[0]);
> diff --git a/hw/irq.h b/hw/irq.h
> index 56c55f0..e640c10 100644
> --- a/hw/irq.h
> +++ b/hw/irq.h
> @@ -23,8 +23,17 @@ static inline void qemu_irq_pulse(qemu_irq irq)
>      qemu_set_irq(irq, 0);
>  }
>
> -/* Returns an array of N IRQs.  */
> +/* Returns an array of N IRQs. Each IRQ is assigned the argument handler and
> + * opaque data.
> + */
>  qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler, void *opaque, int n);
> +
> +/* Extends an Array of IRQs. Old IRQs have their handlers and opaque data
> + * preserved. New IRQs are assigned the argument handler and opaque data.
> + */
> +qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
> +                                void *opaque, int n);
> +
>  void qemu_free_irqs(qemu_irq *s);
>
>  /* Returns a new IRQ with opposite polarity.  */
> diff --git a/hw/qdev.c b/hw/qdev.c
> index b5a52ac..eea9eae 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -291,9 +291,9 @@ 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);
> +    dev->gpio_in = qemu_extend_irqs(dev->gpio_in, dev->num_gpio_in, handler,
> +                                        dev, n);
> +    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 - Sept. 24, 2012, 10:35 a.m.
All changes made.

Thanks
Peter

On Mon, Sep 24, 2012 at 7:53 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 24 September 2012 10:18, 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.
>>
>> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
>> ---
>> changed since v5:
>> moved implementation to irq.c as per PMM review
>
> Thanks for this change -- I think it looks much cleaner this way.
>
>>  hw/irq.c  |   17 ++++++++++++++---
>>  hw/irq.h  |   11 ++++++++++-
>>  hw/qdev.c |    6 +++---
>>  3 files changed, 27 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/irq.c b/hw/irq.c
>> index d413a0b..cd17551 100644
>> --- a/hw/irq.c
>> +++ b/hw/irq.c
>> @@ -38,15 +38,20 @@ void qemu_set_irq(qemu_irq irq, int level)
>>      irq->handler(irq->opaque, irq->n, level);
>>  }
>>
>> -qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler, void *opaque, int n)
>> +qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
>> +                                void *opaque, int n)
>
> Your indent is a little odd here (and below) -- usually the parameters
> in the second line are aligned with the first (so the 'v' is in the
> same column as the 'q').
>
>>  {
>>      qemu_irq *s;
>>      struct IRQState *p;
>>      int i;
>>
>> -    s = (qemu_irq *)g_malloc0(sizeof(qemu_irq) * n);
>> +    if (!old) {
>> +        n_old = 0;
>> +    }
>> +    s = old ? g_renew(qemu_irq, old, n + n_old) :
>> +                    (qemu_irq *)g_new0(qemu_irq, n);
>
> You don't need to cast the return value from g_new0().
> Also, this isn't consistent -- either we don't need the
> memory zeroed (in which case use g_new rather than g_new0)
> or we do (in which case we need to do something about
> the new memory g_renew() returns).
>
>>      p = (struct IRQState *)g_malloc0(sizeof(struct IRQState) * n);
>
> qemu_free_irqs() relies on all the struct IRQState *s being
> freeable with a single g_free(), so you need to g_renew the
> existing array here.
>
>> -    for (i = 0; i < n; i++) {
>> +    for (i = n_old; i < n + n_old; i++) {
>>          p->handler = handler;
>>          p->opaque = opaque;
>>          p->n = i;
>> @@ -56,6 +61,12 @@ qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler, void *opaque, int n)
>>      return s;
>>  }
>>
>> +qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler, void *opaque, int n)
>> +{
>> +    return qemu_extend_irqs(NULL, 0, handler, opaque, n);
>> +}
>> +
>> +
>>  void qemu_free_irqs(qemu_irq *s)
>>  {
>>      g_free(s[0]);
>> diff --git a/hw/irq.h b/hw/irq.h
>> index 56c55f0..e640c10 100644
>> --- a/hw/irq.h
>> +++ b/hw/irq.h
>> @@ -23,8 +23,17 @@ static inline void qemu_irq_pulse(qemu_irq irq)
>>      qemu_set_irq(irq, 0);
>>  }
>>
>> -/* Returns an array of N IRQs.  */
>> +/* Returns an array of N IRQs. Each IRQ is assigned the argument handler and
>> + * opaque data.
>> + */
>>  qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler, void *opaque, int n);
>> +
>> +/* Extends an Array of IRQs. Old IRQs have their handlers and opaque data
>> + * preserved. New IRQs are assigned the argument handler and opaque data.
>> + */
>> +qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
>> +                                void *opaque, int n);
>> +
>>  void qemu_free_irqs(qemu_irq *s);
>>
>>  /* Returns a new IRQ with opposite polarity.  */
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index b5a52ac..eea9eae 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -291,9 +291,9 @@ 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);
>> +    dev->gpio_in = qemu_extend_irqs(dev->gpio_in, dev->num_gpio_in, handler,
>> +                                        dev, n);
>> +    dev->num_gpio_in += n;
>>  }
>>
>>  void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n)
>> --
>> 1.7.0.4
>>
>
>
>
> -- PMM

Patch

diff --git a/hw/irq.c b/hw/irq.c
index d413a0b..cd17551 100644
--- a/hw/irq.c
+++ b/hw/irq.c
@@ -38,15 +38,20 @@  void qemu_set_irq(qemu_irq irq, int level)
     irq->handler(irq->opaque, irq->n, level);
 }
 
-qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler, void *opaque, int n)
+qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
+                                void *opaque, int n)
 {
     qemu_irq *s;
     struct IRQState *p;
     int i;
 
-    s = (qemu_irq *)g_malloc0(sizeof(qemu_irq) * n);
+    if (!old) {
+        n_old = 0;
+    }
+    s = old ? g_renew(qemu_irq, old, n + n_old) :
+                    (qemu_irq *)g_new0(qemu_irq, n);
     p = (struct IRQState *)g_malloc0(sizeof(struct IRQState) * n);
-    for (i = 0; i < n; i++) {
+    for (i = n_old; i < n + n_old; i++) {
         p->handler = handler;
         p->opaque = opaque;
         p->n = i;
@@ -56,6 +61,12 @@  qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler, void *opaque, int n)
     return s;
 }
 
+qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler, void *opaque, int n)
+{
+    return qemu_extend_irqs(NULL, 0, handler, opaque, n);
+}
+
+
 void qemu_free_irqs(qemu_irq *s)
 {
     g_free(s[0]);
diff --git a/hw/irq.h b/hw/irq.h
index 56c55f0..e640c10 100644
--- a/hw/irq.h
+++ b/hw/irq.h
@@ -23,8 +23,17 @@  static inline void qemu_irq_pulse(qemu_irq irq)
     qemu_set_irq(irq, 0);
 }
 
-/* Returns an array of N IRQs.  */
+/* Returns an array of N IRQs. Each IRQ is assigned the argument handler and
+ * opaque data.
+ */
 qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler, void *opaque, int n);
+
+/* Extends an Array of IRQs. Old IRQs have their handlers and opaque data
+ * preserved. New IRQs are assigned the argument handler and opaque data.
+ */
+qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
+                                void *opaque, int n);
+
 void qemu_free_irqs(qemu_irq *s);
 
 /* Returns a new IRQ with opposite polarity.  */
diff --git a/hw/qdev.c b/hw/qdev.c
index b5a52ac..eea9eae 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -291,9 +291,9 @@  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);
+    dev->gpio_in = qemu_extend_irqs(dev->gpio_in, dev->num_gpio_in, handler,
+                                        dev, n);
+    dev->num_gpio_in += n;
 }
 
 void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n)