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

["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.

For now IRQ doesn't have properties, so can do.

> With the above addressed:
> Reviewed-by: Bernhard Beschow <shentey@gmail.com>

Thanks!