Re: [PATCH v2] hw/audio/pcspk: Inline pcspk_init()

[Bernhard Beschow]

Am 19. Oktober 2023 07:33:07 UTC schrieb "Philippe Mathieu-Daudé" 
<philmd@linaro.org>:
>pcspk_init() is a legacy init function, inline and remove it.
>
>Since the device is realized using &error_fatal, use the same
>error for setting the "pit" link.
>
>Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> include/hw/audio/pcspk.h | 10 ----------
> hw/i386/pc.c             |  4 +++-
> hw/isa/i82378.c          |  5 ++++-
> hw/mips/jazz.c           |  5 ++++-
> 4 files changed, 11 insertions(+), 13 deletions(-)
>
>diff --git a/include/hw/audio/pcspk.h b/include/hw/audio/pcspk.h
>index 9506179587..6be75a6b86 100644
>--- a/include/hw/audio/pcspk.h
>+++ b/include/hw/audio/pcspk.h
>@@ -25,16 +25,6 @@
> #ifndef HW_PCSPK_H
> #define HW_PCSPK_H
> 
>-#include "hw/isa/isa.h"
>-#include "hw/qdev-properties.h"
>-#include "qapi/error.h"
>-
> #define TYPE_PC_SPEAKER "isa-pcspk"
> 
>-static inline void pcspk_init(ISADevice *isadev, ISABus *bus, ISADevice *pit)
>-{
>-    object_property_set_link(OBJECT(isadev), "pit", OBJECT(pit), NULL);
>-    isa_realize_and_unref(isadev, bus, &error_fatal);
>-}
>-
> #endif /* HW_PCSPK_H */
>diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>index bb3854d1d0..f7ee638bec 100644
>--- a/hw/i386/pc.c
>+++ b/hw/i386/pc.c
>@@ -1283,7 +1283,9 @@ void pc_basic_device_init(struct PCMachineState *pcms,
>             /* connect PIT to output control line of the HPET */
>             qdev_connect_gpio_out(hpet, 0, qdev_get_gpio_in(DEVICE(pit), 0));
>         }
>-        pcspk_init(pcms->pcspk, isa_bus, pit);
>+        object_property_set_link(OBJECT(pcms->pcspk), "pit",
>+                                 OBJECT(pit), &error_fatal);
>+        isa_realize_and_unref(pcms->pcspk, isa_bus, &error_fatal);
>     }
> 
>     /* Super I/O */
>diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
>index 63e0857208..79ffbb52a0 100644
>--- a/hw/isa/i82378.c
>+++ b/hw/isa/i82378.c
>@@ -67,6 +67,7 @@ static void i82378_realize(PCIDevice *pci, Error **errp)
>     uint8_t *pci_conf;
>     ISABus *isabus;
>     ISADevice *pit;
>+    ISADevice *pcspk;
> 
>     pci_conf = pci->config;
>     pci_set_word(pci_conf + PCI_COMMAND,
>@@ -102,7 +103,9 @@ static void i82378_realize(PCIDevice *pci, Error **errp)
>     pit = i8254_pit_init(isabus, 0x40, 0, NULL);
> 
>     /* speaker */
>-    pcspk_init(isa_new(TYPE_PC_SPEAKER), isabus, pit);
>+    pcspk = isa_new(TYPE_PC_SPEAKER);
>+    object_property_set_link(OBJECT(pcspk), "pit", OBJECT(pit), &error_fatal);
>+    isa_realize_and_unref(pcspk, isabus, &error_fatal);

Why not pass errp here? I think that was Mark's comment in v1.

> 
>     /* 2 82C37 (dma) */
>     isa_create_simple(isabus, "i82374");
>diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
>index c32d2b0b0a..cdc37126c2 100644
>--- a/hw/mips/jazz.c
>+++ b/hw/mips/jazz.c
>@@ -177,6 +177,7 @@ static void mips_jazz_init(MachineState *machine,
>     SysBusDevice *sysbus;
>     ISABus *isa_bus;
>     ISADevice *pit;
>+    ISADevice *pcspk;
>     DriveInfo *fds[MAX_FD];
>     MemoryRegion *bios = g_new(MemoryRegion, 1);
>     MemoryRegion *bios2 = g_new(MemoryRegion, 1);
>@@ -279,7 +280,9 @@ static void mips_jazz_init(MachineState *machine,
>     isa_bus_register_input_irqs(isa_bus, i8259);
>     i8257_dma_init(isa_bus, 0);
>     pit = i8254_pit_init(isa_bus, 0x40, 0, NULL);
>-    pcspk_init(isa_new(TYPE_PC_SPEAKER), isa_bus, pit);
>+    pcspk = isa_new(TYPE_PC_SPEAKER);
>+    object_property_set_link(OBJECT(pcspk), "pit", OBJECT(pit), &error_fatal);
>+    isa_realize_and_unref(pcspk, isa_bus, &error_fatal);
> 
>     /* Video card */
>     switch (jazz_model) {