diff mbox series

[5/5] m48t59: remove legacy m48t59_init() function

Message ID 20201016182739.22875-6-mark.cave-ayland@ilande.co.uk
State New
Headers show
Series m48t59: remove legacy init functions | expand

Commit Message

Mark Cave-Ayland Oct. 16, 2020, 6:27 p.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé Oct. 17, 2020, 9:53 a.m. UTC | #1
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>
Mark Cave-Ayland Oct. 17, 2020, 11:19 a.m. UTC | #2
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.
Philippe Mathieu-Daudé Oct. 17, 2020, 1:57 p.m. UTC | #3
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 mbox series

Patch

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 */