Re: [PATCH 1/9] hw/isa/vt82c686: QOM'ify Super I/O creation

[BALATON Zoltan]

On Tue, 23 Aug 2022, Bernhard Beschow wrote:
> The object creation now happens in chip-specific init methods which
> allows the realize methods to be consolidated into one method. Shifting
> the logic into the init methods has the addidional advantage that the
> parent object's init methods are called implicitly.

This and later patches titled "QOM'ify" don't do what I understand on 
QOMifying which is turining an old device model into a QOM object. These 
devices are already QOMified, what's really done here is that they are 
moved to the ViaISAState or embedded into it and created as part of the 
south bridge rather then individually by the boards. Either my 
understanding of what QOMify means is wrong or these patches are misnamed.

Technically via_isa_realize() is the realize method of the abstract 
TYPE_VIA_ISA class which were overriden by the subclasses but since QOM 
does not call overridden methods implicitly this had to be explicitly 
called in the overriding realize methods of the subclasses. Now that we 
don't have to ovverride the method maybe it could be set once on the 
VIA_ISA class then it may work that way but as it's done here is also OK 
maybe as a reminder that this super class method should be called by any 
overriding method if one's added in the future for some reason.

Regards,
BALATON Zoltan

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/isa/vt82c686.c | 33 ++++++++++++++++++---------------
> 1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 8f656251b8..0217c98fe4 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -544,7 +544,7 @@ struct ViaISAState {
>     qemu_irq cpu_intr;
>     qemu_irq *isa_irqs;
>     ISABus *isa_bus;
> -    ViaSuperIOState *via_sio;
> +    ViaSuperIOState via_sio;
> };
>
> static const VMStateDescription vmstate_via = {
> @@ -602,6 +602,11 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>             d->wmask[i] = 0;
>         }
>     }
> +
> +    /* Super I/O */
> +    if (!qdev_realize(DEVICE(&s->via_sio), BUS(s->isa_bus), errp)) {
> +        return;
> +    }
> }
>
> /* TYPE_VT82C686B_ISA */
> @@ -615,7 +620,7 @@ static void vt82c686b_write_config(PCIDevice *d, uint32_t 
> addr,
>     pci_default_write_config(d, addr, val, len);
>     if (addr == 0x85) {
>         /* BIT(1): enable or disable superio config io ports */
> -        via_superio_io_enable(s->via_sio, val & BIT(1));
> +        via_superio_io_enable(&s->via_sio, val & BIT(1));
>     }
> }
>
> @@ -639,13 +644,11 @@ static void vt82c686b_isa_reset(DeviceState *dev)
>     pci_conf[0x77] = 0x10; /* GPIO Control 1/2/3/4 */
> }
>
> -static void vt82c686b_realize(PCIDevice *d, Error **errp)
> +static void vt82c686b_init(Object *obj)
> {
> -    ViaISAState *s = VIA_ISA(d);
> +    ViaISAState *s = VIA_ISA(obj);
>
> -    via_isa_realize(d, errp);
> -    s->via_sio = VIA_SUPERIO(isa_create_simple(s->isa_bus,
> -                                               TYPE_VT82C686B_SUPERIO));
> +    object_initialize_child(obj, "sio", &s->via_sio, TYPE_VT82C686B_SUPERIO);
> }
>
> static void vt82c686b_class_init(ObjectClass *klass, void *data)
> @@ -653,7 +656,7 @@ static void vt82c686b_class_init(ObjectClass *klass, void 
> *data)
>     DeviceClass *dc = DEVICE_CLASS(klass);
>     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
> -    k->realize = vt82c686b_realize;
> +    k->realize = via_isa_realize;
>     k->config_write = vt82c686b_write_config;
>     k->vendor_id = PCI_VENDOR_ID_VIA;
>     k->device_id = PCI_DEVICE_ID_VIA_82C686B_ISA;
> @@ -670,6 +673,7 @@ static const TypeInfo vt82c686b_isa_info = {
>     .name          = TYPE_VT82C686B_ISA,
>     .parent        = TYPE_VIA_ISA,
>     .instance_size = sizeof(ViaISAState),
> +    .instance_init = vt82c686b_init,
>     .class_init    = vt82c686b_class_init,
> };
>
> @@ -684,7 +688,7 @@ static void vt8231_write_config(PCIDevice *d, uint32_t addr,
>     pci_default_write_config(d, addr, val, len);
>     if (addr == 0x50) {
>         /* BIT(2): enable or disable superio config io ports */
> -        via_superio_io_enable(s->via_sio, val & BIT(2));
> +        via_superio_io_enable(&s->via_sio, val & BIT(2));
>     }
> }
>
> @@ -703,13 +707,11 @@ static void vt8231_isa_reset(DeviceState *dev)
>     pci_conf[0x6b] = 0x01; /* Fast IR I/O Base */
> }
>
> -static void vt8231_realize(PCIDevice *d, Error **errp)
> +static void vt8231_init(Object *obj)
> {
> -    ViaISAState *s = VIA_ISA(d);
> +    ViaISAState *s = VIA_ISA(obj);
>
> -    via_isa_realize(d, errp);
> -    s->via_sio = VIA_SUPERIO(isa_create_simple(s->isa_bus,
> -                                               TYPE_VT8231_SUPERIO));
> +    object_initialize_child(obj, "sio", &s->via_sio, TYPE_VT8231_SUPERIO);
> }
>
> static void vt8231_class_init(ObjectClass *klass, void *data)
> @@ -717,7 +719,7 @@ static void vt8231_class_init(ObjectClass *klass, void 
> *data)
>     DeviceClass *dc = DEVICE_CLASS(klass);
>     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
> -    k->realize = vt8231_realize;
> +    k->realize = via_isa_realize;
>     k->config_write = vt8231_write_config;
>     k->vendor_id = PCI_VENDOR_ID_VIA;
>     k->device_id = PCI_DEVICE_ID_VIA_8231_ISA;
> @@ -734,6 +736,7 @@ static const TypeInfo vt8231_isa_info = {
>     .name          = TYPE_VT8231_ISA,
>     .parent        = TYPE_VIA_ISA,
>     .instance_size = sizeof(ViaISAState),
> +    .instance_init = vt8231_init,
>     .class_init    = vt8231_class_init,
> };