Re: [PATCH 5/6] hw/isa/piix4: Factor out SM bus initialization from create() function

[Bernhard Beschow <shentey@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3838 bytes --]

On Sat, May 21, 2022 at 10:39 AM Mark Cave-Ayland <
mark.cave-ayland@ilande.co.uk> wrote:

> On 13/05/2022 18:54, Bernhard Beschow wrote:
>
> > Initialize the SM bus just like is done for piix3 which modernizes the
> > code.
> >
> > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > ---
> >   hw/isa/piix4.c                | 15 +++------------
> >   hw/mips/malta.c               |  7 ++++++-
> >   include/hw/southbridge/piix.h |  2 +-
> >   3 files changed, 10 insertions(+), 14 deletions(-)
> >
> > diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> > index 4968c69da9..852e5c4db1 100644
> > --- a/hw/isa/piix4.c
> > +++ b/hw/isa/piix4.c
> > @@ -301,21 +301,12 @@ static void piix4_register_types(void)
> >
> >   type_init(piix4_register_types)
> >
> > -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus)
> > +PCIDevice *piix4_create(PCIBus *pci_bus)
> >   {
> >       PCIDevice *pci;
> > -    DeviceState *dev;
> > -    int devfn = PCI_DEVFN(10, 0);
> >
> > -    pci = pci_create_simple_multifunction(pci_bus, devfn,  true,
> > +    pci = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0),
> true,
> >                                             TYPE_PIIX4_PCI_DEVICE);
> > -    dev = DEVICE(pci);
> >
> > -    if (smbus) {
> > -        *smbus = piix4_pm_init(pci_bus, devfn + 3, 0x1100,
> > -                               qdev_get_gpio_in_named(dev, "isa", 9),
> > -                               NULL, 0, NULL);
> > -    }
> > -
> > -    return dev;
> > +    return pci;
> >   }
>
> I don't think it makes sense to return PCIDevice here: when returning a
> QOM object
> from a function, the general expectation is that for a device you would
> return a
> DeviceState since then it can natively be used by the qdev API. So please
> keep the
> original return type above.
>

Okay, will do.

I've been toying with moving piix4_pm_init() into piix4_realize(), such
that it is created as part of TYPE_PIIX4_PCI_DEVICE - just as the real
hardware. I think I like this solution much better.

>
> > diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> > index e446b25ad0..d4bd3549d0 100644
> > --- a/hw/mips/malta.c
> > +++ b/hw/mips/malta.c
> > @@ -1238,6 +1238,7 @@ void mips_malta_init(MachineState *machine)
> >       int be;
> >       MaltaState *s;
> >       DeviceState *dev;
> > +    PCIDevice *piix4;
> >
> >       s = MIPS_MALTA(qdev_new(TYPE_MIPS_MALTA));
> >       sysbus_realize_and_unref(SYS_BUS_DEVICE(s), &error_fatal);
> > @@ -1399,8 +1400,12 @@ void mips_malta_init(MachineState *machine)
> >       empty_slot_init("GT64120", 0, 0x20000000);
> >
> >       /* Southbridge */
> > -    dev = piix4_create(pci_bus, &smbus);
> > +    piix4 = piix4_create(pci_bus);
> > +    dev = DEVICE(piix4);
> >       isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
> > +    smbus = piix4_pm_init(pci_bus, piix4->devfn + 3, 0x1100,
> > +                          qdev_get_gpio_in_named(dev, "isa", 9),
> > +                          NULL, 0, NULL);
>
> ... then here you can do either "piix4 = PCI_DEVICE(dev)" or perhaps even
> inline it
> directly as PCI_DEVICE(dev)->devfn if it isn't used elsewhere.
>

When instantiating the pm in TYPE_PIIX4_PCI_DEVICE this problem just
disappears magically. So I'd roll with this in v2.

>
> >       /* Interrupt controller */
> >       qdev_connect_gpio_out_named(dev, "intr", 0, i8259_irq);
> > diff --git a/include/hw/southbridge/piix.h
> b/include/hw/southbridge/piix.h
> > index b768109f30..bea3b44551 100644
> > --- a/include/hw/southbridge/piix.h
> > +++ b/include/hw/southbridge/piix.h
> > @@ -74,6 +74,6 @@ DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
> >
> >   PIIX3State *piix3_create(PCIBus *pci_bus);
> >
> > -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus);
> > +PCIDevice *piix4_create(PCIBus *pci_bus);
> >
> >   #endif
>
>
> ATB,
>
> Mark.
>

[-- Attachment #2: Type: text/html, Size: 5344 bytes --]