Re: [PATCH 2/2] hw/ppc/pegasos2: Extract via_vt8231_create() helper

[BALATON Zoltan]

On Tue, 17 Jan 2023, Philippe Mathieu-Daudé wrote:
> Simplify a bit pegasos2_init() by extracting the VIA southbridge
> creation code into a new via_vt8231_create() helper.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> hw/ppc/pegasos2.c | 33 +++++++++++++++++++++------------
> 1 file changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> index ac69aee099..445cb5ef31 100644
> --- a/hw/ppc/pegasos2.c
> +++ b/hw/ppc/pegasos2.c
> @@ -96,6 +96,25 @@ static void pegasos2_cpu_reset(void *opaque)
>     }
> }
>
> +static PCIDevice *via_vt8231_create(MachineState *machine, PCIBus *pci_bus)
> +{
> +    Pegasos2MachineState *pm = PEGASOS2_MACHINE(machine);
> +    PCIDevice *dev, *via;
> +
> +    via = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, 0),
> +                                          true, TYPE_VT8231_ISA);
> +    object_property_add_alias(OBJECT(machine), "rtc-time",
> +                              object_resolve_path_component(OBJECT(via), 
> "rtc"),
> +                              "date");
> +    qdev_connect_gpio_out(DEVICE(via), 0,
> +                          qdev_get_gpio_in_named(pm->mv, "gpp", 31));
> +
> +    dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "ide"));
> +    pci_ide_create_devs(dev);
> +
> +    return via;
> +}
> +
> static I2CBus *via_i2c_bus(PCIDevice *via)
> {
>     PCIDevice *dev;
> @@ -110,7 +129,7 @@ static void pegasos2_init(MachineState *machine)
>     CPUPPCState *env;
>     MemoryRegion *rom = g_new(MemoryRegion, 1);
>     PCIBus *pci_bus;
> -    PCIDevice *dev, *via;
> +    PCIDevice *via;
>     const char *fwname = machine->firmware ?: PROM_FILENAME;
>     char *filename;
>     int sz;
> @@ -166,17 +185,7 @@ static void pegasos2_init(MachineState *machine)
>     pci_bus = mv64361_get_pci_bus(pm->mv, 1);
>
>     /* VIA VT8231 South Bridge (multifunction PCI device) */
> -    via = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, 0), true,
> -                                          TYPE_VT8231_ISA);
> -    object_property_add_alias(OBJECT(machine), "rtc-time",
> -                              object_resolve_path_component(OBJECT(via),
> -                                                            "rtc"),

Is this series to help any later patches or is it proposed on its own? In 
the latter case I don't see how this would improve it much. The only 
useful change in the series is removing the unnecessary line break before 
"rtc"); here, otherwise moving patts of this init routine to separate 
functions does not make it simpler just makes you jump around instead of 
being able to read it linearly. So if this makes it possible for later 
patches to move some of it elsewhere then OK otherwise I'd say I'm OK with 
how it is now, it's just the normal unreadable QOM stuff you see 
everywhere after removing legacy init functions.

Regards,
BALATON Zoltan

> -                              "date");
> -    qdev_connect_gpio_out(DEVICE(via), 0,
> -                          qdev_get_gpio_in_named(pm->mv, "gpp", 31));
> -
> -    dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "ide"));
> -    pci_ide_create_devs(dev);
> +    via = via_vt8231_create(machine, pci_bus);
>
>     spd_data = spd_data_generate(DDR, machine->ram_size);
>     smbus_eeprom_init_one(via_i2c_bus(via), 0x57, spd_data);