Re: [PATCH 6/9] vt82c686: Support machine-default audiodev with fallback

[BALATON Zoltan]

On Fri, 22 Sep 2023, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/isa/vt82c686.c   |  2 ++
> hw/mips/fuloong2e.c | 12 +++++++++---
> hw/ppc/pegasos2.c   | 10 ++++++++--
> hw/ppc/sam460ex.c   |  2 ++
> 4 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 57bdfb4e78c..3ec8e43708a 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -578,6 +578,8 @@ static void via_isa_init(Object *obj)
>     object_initialize_child(obj, "uhci2", &s->uhci[1], TYPE_VT82C686B_USB_UHCI);
>     object_initialize_child(obj, "ac97", &s->ac97, TYPE_VIA_AC97);
>     object_initialize_child(obj, "mc97", &s->mc97, TYPE_VIA_MC97);
> +
> +    object_property_add_alias(obj, "audiodev", OBJECT(&s->ac97), "audiodev");
> }
>
> static const TypeInfo via_isa_info = {
> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
> index c827f615f3b..d0671824f94 100644
> --- a/hw/mips/fuloong2e.c
> +++ b/hw/mips/fuloong2e.c
> @@ -41,6 +41,7 @@
> #include "sysemu/reset.h"
> #include "sysemu/sysemu.h"
> #include "qemu/error-report.h"
> +#include "audio/audio.h"
>
> #define ENVP_PADDR              0x2000
> #define ENVP_VADDR              cpu_mips_phys_to_kseg0(NULL, ENVP_PADDR)
> @@ -295,9 +296,12 @@ static void mips_fuloong2e_init(MachineState *machine)
>     pci_bus = bonito_init((qemu_irq *)&(env->irq[2]));
>
>     /* South bridge -> IP5 */
> -    pci_dev = pci_create_simple_multifunction(pci_bus,
> -                                              PCI_DEVFN(FULOONG2E_VIA_SLOT, 0),
> -                                              TYPE_VT82C686B_ISA);
> +    pci_dev = pci_new_multifunction(PCI_DEVFN(FULOONG2E_VIA_SLOT, 0),
> +                                    TYPE_VT82C686B_ISA);
> +    qdev_prop_set_string(DEVICE(pci_dev), "audiodev",
> +                         audio_maybe_init_dummy("fuloong2e.defaudio"));
> +    pci_realize_and_unref(pci_dev, pci_bus, &error_abort);
> +
>     object_property_add_alias(OBJECT(machine), "rtc-time",
>                               object_resolve_path_component(OBJECT(pci_dev),
>                                                             "rtc"),
> @@ -337,6 +341,8 @@ static void mips_fuloong2e_machine_init(MachineClass *mc)
>     mc->default_ram_size = 256 * MiB;
>     mc->default_ram_id = "fuloong2e.ram";
>     mc->minimum_page_bits = 14;
> +
> +    machine_add_audiodev_property(mc);
> }
>
> DEFINE_MACHINE("fuloong2e", mips_fuloong2e_machine_init)
> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> index bd397cf2b5c..45b94cab73a 100644
> --- a/hw/ppc/pegasos2.c
> +++ b/hw/ppc/pegasos2.c
> @@ -37,6 +37,7 @@
> #include "qemu/datadir.h"
> #include "sysemu/device_tree.h"
> #include "hw/ppc/vof.h"
> +#include "audio/audio.h"
>
> #include <libfdt.h>
>
> @@ -180,8 +181,11 @@ static void pegasos2_init(MachineState *machine)
>     pci_bus_irqs(pci_bus, pegasos2_pci_irq, pm, PCI_NUM_PINS);
>
>     /* VIA VT8231 South Bridge (multifunction PCI device) */
> -    via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, 0),
> -                                                 TYPE_VT8231_ISA));
> +    via = OBJECT(pci_new_multifunction(PCI_DEVFN(12, 0), TYPE_VT8231_ISA));
> +    qdev_prop_set_string(DEVICE(via), "audiodev",
> +                         audio_maybe_init_dummy("pegasos2.defaudio"));
> +    pci_realize_and_unref(PCI_DEVICE(via), pci_bus, &error_abort);
> +

Do not add blank line here, the rest is still part of via init.

>     for (i = 0; i < PCI_NUM_PINS; i++) {
>         pm->via_pirq[i] = qdev_get_gpio_in_named(DEVICE(via), "pirq", i);
>     }
> @@ -564,6 +568,8 @@ static void pegasos2_machine_class_init(ObjectClass *oc, 
> void *data)
>     vhc->encode_hpt_for_kvm_pr = vhyp_encode_hpt_for_kvm_pr;
>
>     vmc->setprop = pegasos2_setprop;
> +
> +    machine_add_audiodev_property(mc);
> }
>
> static const TypeInfo pegasos2_machine_info = {
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 1e615b8d355..8c3abc67131 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -530,6 +530,8 @@ static void sam460ex_machine_init(MachineClass *mc)
>     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("460exb");
>     mc->default_ram_size = 512 * MiB;
>     mc->default_ram_id = "ppc4xx.sdram";
> +
> +    machine_add_audiodev_property(mc);

This hunk has nothing to do with vt82686 so probably should be in 
previoius patch. Also sam460ex embedded sound part is not emulated and can 
only use PCI sound cards. What this line is needed for? If every machine 
now needs this call, can it be added in some generic machine init func in 
hw/core/machine.c instead?

I'm not sure about this whole series. Looks like it gets rid of 71 lines 
but adding 158 and makes the users' life harder by requireing them to 
specify drivers they may not even know about. What's the point? Is there 
still a default to use the normally used audiodev for the platform or 
people will now get errors and no sound unless they change their command 
lines?

Regards,
BALATON Zoltan

> }
>
> DEFINE_MACHINE("sam460ex", sam460ex_machine_init)