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

[BALATON Zoltan]

On Wed, 18 Jan 2023, Philippe Mathieu-Daudé wrote:
> On 17/1/23 22:17, BALATON Zoltan wrote:
> > 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.
>
> Difference of mindset I suppose, as you clearly type linearly :)
>
> I consider logical blocks of hardware, and the southbridge is one of
> them. So I thought moving components connected to the 'machine' via
> the southbridge in a separate function would be clearer for the
> overall community (this file is not exclusively used by you, and
> can potentially used as example to build a machine).

I understand but you cannot clearly move all of it out to one function but 
proposed to introduce two small functions with leaving the spd data 
creation in board code. In my view only the line creating the 'via' 
instance is one unit, he rest connects the instance to the machine (the 
rtc alias adds property to the _machine_, pci_ide_create_devs(dev) is 
usually part of boards and the spd data is also not part of the 'via' just 
uses the i2c bus it provides). So for me it's more logical to have these 
in the board init func than in a via_init func as it's not initialises the 
'via' itself just uses the one created before. Creating the 'via' instance 
is now just one line after moving all the subfunctions in the via model. 
It's still in one block separated by comments so i think adding functions 
to this would not make it clearer and legacy init functions are also 
deprecated in favour of plain QOM in board init lately.

> Anyhow I don't mind much.

In that case I'd leave it as it is with the small readability improvement 
I've sent a v2 of fixing the typos and adding your R-b, unless others 
think splitting some part of this in separate functions would be better.

Regards,
BALATON Zoltan