| Message ID | 20201016182739.22875-6-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | m48t59: remove legacy init functions | expand |
On 10/16/20 8:27 PM, Mark Cave-Ayland wrote: > Now that all of the callers of this function have been switched to use qdev > properties, this legacy init function can now be removed. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > hw/rtc/m48t59.c | 35 ----------------------------------- > include/hw/rtc/m48t59.h | 4 ---- > 2 files changed, 39 deletions(-) In the PoC I started after your suggestion, I see: #define TYPE_M48T02_SRAM "sysbus-m48t02" #define TYPE_M48T08_SRAM "sysbus-m48t08" #define TYPE_M48T59_SRAM "sysbus-m48t59" static void m48t02_class_init(ObjectClass *oc, void *data) { M48txxSysBusDeviceClass *amc = M48TXX_SYS_BUS_CLASS(oc); amc->model = 2; amc->size = 2 * KiB; }; static void m48t08_class_init(ObjectClass *oc, void *data) { M48txxSysBusDeviceClass *amc = M48TXX_SYS_BUS_CLASS(oc); amc->model = 8; amc->size = 8 * KiB; }; static void m48t59_class_init(ObjectClass *oc, void *data) { M48txxSysBusDeviceClass *amc = M48TXX_SYS_BUS_CLASS(oc); amc->model = 59; amc->size = 8 * KiB; }; static const TypeInfo m48t59_register_types[] = { { .name = TYPE_M48T02_SRAM, .parent = TYPE_M48TXX_SYSBUS, .class_init = m48t02_class_init, }, { .name = TYPE_M48T08_SRAM, .parent = TYPE_M48TXX_SYSBUS, .class_init = m48t08_class_init, }, { .name = TYPE_M48T59_SRAM, .parent = TYPE_M48TXX_SYSBUS, .class_init = m48t59_class_init, }, { .name = TYPE_M48TXX_SYSBUS, .parent = TYPE_SYS_BUS_DEVICE, .instance_size = sizeof(M48txxSysBusState), .instance_init = m48t59_init1, .class_size = sizeof(M48txxSysBusDeviceClass), .class_init = m48txx_sysbus_class_init, .abstract = true, .interfaces = (InterfaceInfo[]) { { TYPE_NVRAM }, { } } } }; and: #define TYPE_M48T59_SRAM "isa-m48t59" static void m48t59_class_init(ObjectClass *oc, void *data) { M48txxISADeviceClass *midc = M48TXX_ISA_CLASS(oc); midc->model = 59; midc->size = 8 * KiB; }; static const TypeInfo m48t59_isa_register_types[] = { { .name = TYPE_M48T59_SRAM, .parent = TYPE_M48TXX_ISA, .class_init = m48t59_class_init, }, { .name = TYPE_M48TXX_ISA, .parent = TYPE_ISA_DEVICE, .instance_size = sizeof(M48txxISAState), .class_size = sizeof(M48txxISADeviceClass), .class_init = m48txx_isa_class_init, .abstract = true, .interfaces = (InterfaceInfo[]) { { TYPE_NVRAM }, { } } } }; I guess I didn't pursue because I wondered what was the best way to have the same model usable by sysbus/isa. IIRC I wanted to proceed as having TYPE_M48T59_SRAM being an abstract qdev parent, and then TYPE_M48TXX_SYSBUS / TYPE_M48TXX_ISA implementing the SYSBUS/ISA interfaces. As it need some thinking I postponed that for after 5.2. Anyhow back to this patch: Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On 17/10/2020 10:53, Philippe Mathieu-Daudé wrote: > On 10/16/20 8:27 PM, Mark Cave-Ayland wrote: >> Now that all of the callers of this function have been switched to use qdev >> properties, this legacy init function can now be removed. >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> hw/rtc/m48t59.c | 35 ----------------------------------- >> include/hw/rtc/m48t59.h | 4 ---- >> 2 files changed, 39 deletions(-) > > In the PoC I started after your suggestion, I see: > > #define TYPE_M48T02_SRAM "sysbus-m48t02" > #define TYPE_M48T08_SRAM "sysbus-m48t08" > #define TYPE_M48T59_SRAM "sysbus-m48t59" > > static void m48t02_class_init(ObjectClass *oc, void *data) > { > M48txxSysBusDeviceClass *amc = M48TXX_SYS_BUS_CLASS(oc); > > amc->model = 2; > amc->size = 2 * KiB; > }; > > static void m48t08_class_init(ObjectClass *oc, void *data) > { > M48txxSysBusDeviceClass *amc = M48TXX_SYS_BUS_CLASS(oc); > > amc->model = 8; > amc->size = 8 * KiB; > }; > > static void m48t59_class_init(ObjectClass *oc, void *data) > { > M48txxSysBusDeviceClass *amc = M48TXX_SYS_BUS_CLASS(oc); > > amc->model = 59; > amc->size = 8 * KiB; > }; > > static const TypeInfo m48t59_register_types[] = { > { > .name = TYPE_M48T02_SRAM, > .parent = TYPE_M48TXX_SYSBUS, > .class_init = m48t02_class_init, > }, { > .name = TYPE_M48T08_SRAM, > .parent = TYPE_M48TXX_SYSBUS, > .class_init = m48t08_class_init, > }, { > .name = TYPE_M48T59_SRAM, > .parent = TYPE_M48TXX_SYSBUS, > .class_init = m48t59_class_init, > }, { > .name = TYPE_M48TXX_SYSBUS, > .parent = TYPE_SYS_BUS_DEVICE, > .instance_size = sizeof(M48txxSysBusState), > .instance_init = m48t59_init1, > .class_size = sizeof(M48txxSysBusDeviceClass), > .class_init = m48txx_sysbus_class_init, > .abstract = true, > .interfaces = (InterfaceInfo[]) { > { TYPE_NVRAM }, > { } > } > } > }; > > and: > > #define TYPE_M48T59_SRAM "isa-m48t59" > > static void m48t59_class_init(ObjectClass *oc, void *data) > { > M48txxISADeviceClass *midc = M48TXX_ISA_CLASS(oc); > > midc->model = 59; > midc->size = 8 * KiB; > }; > > static const TypeInfo m48t59_isa_register_types[] = { > { > .name = TYPE_M48T59_SRAM, > .parent = TYPE_M48TXX_ISA, > .class_init = m48t59_class_init, > }, { > .name = TYPE_M48TXX_ISA, > .parent = TYPE_ISA_DEVICE, > .instance_size = sizeof(M48txxISAState), > .class_size = sizeof(M48txxISADeviceClass), > .class_init = m48txx_isa_class_init, > .abstract = true, > .interfaces = (InterfaceInfo[]) { > { TYPE_NVRAM }, > { } > } > } > }; > > I guess I didn't pursue because I wondered what was the > best way to have the same model usable by sysbus/isa. > > IIRC I wanted to proceed as having TYPE_M48T59_SRAM being > an abstract qdev parent, and then TYPE_M48TXX_SYSBUS / > TYPE_M48TXX_ISA implementing the SYSBUS/ISA interfaces. > > As it need some thinking I postponed that for after 5.2. > > Anyhow back to this patch: > > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Ha indeed, I think you also came to the same conclusion that I did in my previous email :) I'm also not convinced by the dynamic generation of various M48TXX types using class_data - this seems overly complex, and there's nothing there I can see that can't be just as easily handled using qdev properties using an abstract parent as you suggest above. ATB, Mark.
On 10/17/20 1:19 PM, Mark Cave-Ayland wrote: > On 17/10/2020 10:53, Philippe Mathieu-Daudé wrote: > >> On 10/16/20 8:27 PM, Mark Cave-Ayland wrote: >>> Now that all of the callers of this function have been switched to >>> use qdev >>> properties, this legacy init function can now be removed. >>> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>> --- >>> hw/rtc/m48t59.c | 35 ----------------------------------- >>> include/hw/rtc/m48t59.h | 4 ---- >>> 2 files changed, 39 deletions(-) ... >> static void m48t59_class_init(ObjectClass *oc, void *data) >> { >> M48txxISADeviceClass *midc = M48TXX_ISA_CLASS(oc); >> >> midc->model = 59; >> midc->size = 8 * KiB; >> }; >> >> static const TypeInfo m48t59_isa_register_types[] = { >> { >> .name = TYPE_M48T59_SRAM, >> .parent = TYPE_M48TXX_ISA, >> .class_init = m48t59_class_init, >> }, { >> .name = TYPE_M48TXX_ISA, >> .parent = TYPE_ISA_DEVICE, >> .instance_size = sizeof(M48txxISAState), >> .class_size = sizeof(M48txxISADeviceClass), >> .class_init = m48txx_isa_class_init, >> .abstract = true, >> .interfaces = (InterfaceInfo[]) { >> { TYPE_NVRAM }, >> { } >> } >> } >> }; >> >> I guess I didn't pursue because I wondered what was the >> best way to have the same model usable by sysbus/isa. >> >> IIRC I wanted to proceed as having TYPE_M48T59_SRAM being >> an abstract qdev parent, and then TYPE_M48TXX_SYSBUS / >> TYPE_M48TXX_ISA implementing the SYSBUS/ISA interfaces. >> >> As it need some thinking I postponed that for after 5.2. >> >> Anyhow back to this patch: >> >> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > Ha indeed, I think you also came to the same conclusion that I did in my > previous email :) > > I'm also not convinced by the dynamic generation of various M48TXX types > using class_data - this seems overly complex, and there's nothing there > I can see that can't be just as easily handled using qdev properties > using an abstract parent as you suggest above. Yeah, no advantage except having uniform code style that serves as example. > > > ATB, > > Mark. >
diff --git a/hw/rtc/m48t59.c b/hw/rtc/m48t59.c index 6525206976..d54929e861 100644 --- a/hw/rtc/m48t59.c +++ b/hw/rtc/m48t59.c @@ -564,41 +564,6 @@ const MemoryRegionOps m48t59_io_ops = { .endianness = DEVICE_LITTLE_ENDIAN, }; -/* Initialisation routine */ -Nvram *m48t59_init(qemu_irq IRQ, hwaddr mem_base, - uint32_t io_base, uint16_t size, int base_year, - int model) -{ - DeviceState *dev; - SysBusDevice *s; - int i; - - for (i = 0; i < ARRAY_SIZE(m48txx_sysbus_info); i++) { - if (m48txx_sysbus_info[i].size != size || - m48txx_sysbus_info[i].model != model) { - continue; - } - - dev = qdev_new(m48txx_sysbus_info[i].bus_name); - qdev_prop_set_int32(dev, "base-year", base_year); - s = SYS_BUS_DEVICE(dev); - sysbus_realize_and_unref(s, &error_fatal); - sysbus_connect_irq(s, 0, IRQ); - if (io_base != 0) { - memory_region_add_subregion(get_system_io(), io_base, - sysbus_mmio_get_region(s, 1)); - } - if (mem_base != 0) { - sysbus_mmio_map(s, 0, mem_base); - } - - return NVRAM(s); - } - - assert(false); - return NULL; -} - void m48t59_realize_common(M48t59State *s, Error **errp) { s->buffer = g_malloc0(s->size); diff --git a/include/hw/rtc/m48t59.h b/include/hw/rtc/m48t59.h index 9defe578d1..d9b45eb161 100644 --- a/include/hw/rtc/m48t59.h +++ b/include/hw/rtc/m48t59.h @@ -47,8 +47,4 @@ struct NvramClass { void (*toggle_lock)(Nvram *obj, int lock); }; -Nvram *m48t59_init(qemu_irq IRQ, hwaddr mem_base, - uint32_t io_base, uint16_t size, int base_year, - int type); - #endif /* HW_M48T59_H */
Now that all of the callers of this function have been switched to use qdev properties, this legacy init function can now be removed. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- hw/rtc/m48t59.c | 35 ----------------------------------- include/hw/rtc/m48t59.h | 4 ---- 2 files changed, 39 deletions(-)