On Wed, Aug 24, 2022 at 1:36 AM BALATON Zoltan <bala...@eik.bme.hu> wrote:
> On Tue, 23 Aug 2022, Bernhard Beschow wrote: > > On Tue, Aug 23, 2022 at 2:35 AM BALATON Zoltan <bala...@eik.bme.hu> > wrote: > > > >> 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. > >> > > > > I think your understanding is correct. Peter refers to it as the > > embed-the-device-struct style [1] which I can take as inspiration for > > renaming my patches in v2. > > > > 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. > >> > > > > Right. This would involve moving some code around and creating a class > > struct. Do you think it's worth it? > > You mean a class_init func to assign realize as the class struct > via_isa_info already exists. Ah yes, of course. > But yes this would need to be moved after > via_isa_realize then. As I wrote above I don't think this is worth it, > especially becuase if in the future a realize method was re-added to the > subclasses then it's easy to forget to revert this and call the superclass > method so assigning via_isa_realize to the subclass as done here is OK and > I can think of it as a reminder to call this method when overriding it. > Also with the added class_init func it's shorter this way even if slightly > mixing different objects. So in the end I'm OK with this as it is. > Okay, I'll keep it as it is. Thanks, Bernhard > > Regards, > BALATON Zoltan > > > Best regards, > > Bernhard > > > > [1] > > > http://patchwork.ozlabs.org/project/qemu-devel/patch/20220205175913.31738-2-shen...@gmail.com/ > > > > Regards, > >> BALATON Zoltan > >> > >>> Signed-off-by: Bernhard Beschow <shen...@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, > >>> }; > >>> > >>> > >> > > >
