Re: [PATCH v2 2/2] hw/ipack: Remove legacy qemu_allocate_irqs() use

[Bernhard Beschow <shentey@gmail.com>]

Am 21. Januar 2025 10:04:43 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 21/1/25 10:24, Bernhard Beschow wrote:
>> 
>> 
>> Am 21. Januar 2025 08:44:52 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>>> No need to dynamically allocate IRQ when we know before hands
>>> how many we'll use. Declare the 2 of them in IPackDevice state
>>> and initialize them in the DeviceRealize handler.
>>> 
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> include/hw/ipack/ipack.h | 7 ++-----
>>> hw/char/ipoctal232.c     | 4 ++--
>>> hw/ipack/ipack.c         | 7 +++----
>>> hw/ipack/tpci200.c       | 6 +++---
>>> 4 files changed, 10 insertions(+), 14 deletions(-)
>>> 
>>> diff --git a/include/hw/ipack/ipack.h b/include/hw/ipack/ipack.h
>>> index cbcdda509d3..00f397fd020 100644
>>> --- a/include/hw/ipack/ipack.h
>>> +++ b/include/hw/ipack/ipack.h
>>> @@ -12,6 +12,7 @@
>>> #define QEMU_IPACK_H
>>> 
>>> #include "hw/qdev-core.h"
>>> +#include "hw/irq.h"
>>> #include "qom/object.h"
>>> 
>>> 
>>> @@ -19,10 +20,8 @@
>>> OBJECT_DECLARE_SIMPLE_TYPE(IPackBus, IPACK_BUS)
>>> 
>>> struct IPackBus {
>>> -    /*< private >*/
>>>      BusState parent_obj;
>>> 
>>> -    /* All fields are private */
>>>      uint8_t n_slots;
>>>      uint8_t free_slot;
>>>      qemu_irq_handler set_irq;
>>> @@ -58,13 +57,11 @@ struct IPackDeviceClass {
>>> };
>>> 
>>> struct IPackDevice {
>>> -    /*< private >*/
>>>      DeviceState parent_obj;
>>> -    /*< public >*/
>>> 
>>>      int32_t slot;
>>>      /* IRQ objects for the IndustryPack INT0# and INT1# */
>>> -    qemu_irq *irq;
>>> +    IRQState irq[2];
>>> };
>>> 
>>> extern const VMStateDescription vmstate_ipack_device;
>>> diff --git a/hw/char/ipoctal232.c b/hw/char/ipoctal232.c
>>> index d1e5f6dad2e..a2879977fb3 100644
>>> --- a/hw/char/ipoctal232.c
>>> +++ b/hw/char/ipoctal232.c
>>> @@ -184,9 +184,9 @@ static void update_irq(IPOctalState *dev, unsigned block)
>>>      unsigned intno = block / 2;
>>> 
>>>      if ((blk0->isr & blk0->imr) || (blk1->isr & blk1->imr)) {
>>> -        qemu_irq_raise(idev->irq[intno]);
>>> +        qemu_irq_raise(&idev->irq[intno]);
>>>      } else {
>>> -        qemu_irq_lower(idev->irq[intno]);
>>> +        qemu_irq_lower(&idev->irq[intno]);
>>>      }
>>> }
>>> 
>>> diff --git a/hw/ipack/ipack.c b/hw/ipack/ipack.c
>>> index ed75f791832..eeb48dd331c 100644
>>> --- a/hw/ipack/ipack.c
>>> +++ b/hw/ipack/ipack.c
>>> @@ -55,22 +55,21 @@ static void ipack_device_realize(DeviceState *dev, Error **errp)
>>>      }
>>>      bus->free_slot = idev->slot + 1;
>>> 
>>> -    idev->irq = qemu_allocate_irqs(bus->set_irq, idev, 2);
>>> +    for (int i = 0; i < ARRAY_SIZE(idev->irq); i++) {
>>> +        qemu_init_irq(&idev->irq[i], bus->set_irq, idev, i);
>> 
>> I wonder if we eventually need a qemu_init_irqs() (note the plural) as we remove further uses of qemu_allocate_irqs().
>
>Yeah I thought about it, but looking at other "Initialize pre-allocated
>QOM object in place", we don't have helpers for arrays. Usually because
>we can set properties, and array initializer doesn't scale then.

Fair enough. I'd call this "addressed".

>
>For now IRQ doesn't have properties, so can do.
>
>> With the above addressed:
>> Reviewed-by: Bernhard Beschow <shentey@gmail.com>
>
>Thanks!