| Message ID | 1506264466-28252-7-git-send-email-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | mac_dbdma: tidy-up and QOMify | expand |
On Sun, Sep 24, 2017 at 03:47:45PM +0100, Mark Cave-Ayland wrote: > Using this we can change the MACIO_IDE instance to register the channel > itself via a type method instead of requiring a separate > DBDMA_register_channel() function. > > As a consequence of this it is now possible to remove the old external > macio_ide_register_dma() function. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Ok, two concerns about this. First, you've added the function pointer to the instance, not to the class, which is not how a QOM method would normally be done. More generally, though, why is a method preferable to a plain function? AFAICT it's not plausible that there will ever be more than one implementation of the method. Same comments apply to patch 7/7. > --- > hw/ide/macio.c | 12 ++++++------ > hw/misc/macio/mac_dbdma.c | 9 +++++---- > hw/misc/macio/macio.c | 1 - > include/hw/ppc/mac_dbdma.h | 9 ++++----- > 4 files changed, 15 insertions(+), 16 deletions(-) > > diff --git a/hw/ide/macio.c b/hw/ide/macio.c > index ce194c6..b296017 100644 > --- a/hw/ide/macio.c > +++ b/hw/ide/macio.c > @@ -411,12 +411,18 @@ static const IDEDMAOps dbdma_ops = { > static void macio_ide_realizefn(DeviceState *dev, Error **errp) > { > MACIOIDEState *s = MACIO_IDE(dev); > + DBDMAState *dbdma; > > ide_init2(&s->bus, s->ide_irq); > > /* Register DMA callbacks */ > s->dma.ops = &dbdma_ops; > s->bus.dma = &s->dma; > + > + /* Register DBDMA channel */ > + dbdma = MAC_DBDMA(object_property_get_link(OBJECT(dev), "dbdma", errp)); > + dbdma->register_channel(dbdma, s->channel, s->dma_irq, > + pmac_ide_transfer, pmac_ide_flush, s); > } > > static void pmac_ide_irq(void *opaque, int n, int level) > @@ -497,10 +503,4 @@ void macio_ide_init_drives(MACIOIDEState *s, DriveInfo **hd_table) > } > } > > -void macio_ide_register_dma(MACIOIDEState *s) > -{ > - DBDMA_register_channel(s->dbdma, s->channel, s->dma_irq, > - pmac_ide_transfer, pmac_ide_flush, s); > -} > - > type_init(macio_ide_register_types) > diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c > index 0eddf2e..addb97d 100644 > --- a/hw/misc/macio/mac_dbdma.c > +++ b/hw/misc/macio/mac_dbdma.c > @@ -557,11 +557,10 @@ void DBDMA_kick(DBDMAState *dbdma) > qemu_bh_schedule(dbdma->bh); > } > > -void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq, > - DBDMA_rw rw, DBDMA_flush flush, > - void *opaque) > +static void > +dbdma_register_channel(DBDMAState *s, int nchan, qemu_irq irq, > + DBDMA_rw rw, DBDMA_flush flush, void *opaque) > { > - DBDMAState *s = dbdma; > DBDMA_channel *ch = &s->channels[nchan]; > > DBDMA_DPRINTFCH(ch, "DBDMA_register_channel 0x%x\n", nchan); > @@ -903,6 +902,8 @@ static void mac_dbdma_init(Object *obj) > > memory_region_init_io(&s->mem, obj, &dbdma_ops, s, "dbdma", 0x1000); > sysbus_init_mmio(sbd, &s->mem); > + > + s->register_channel = dbdma_register_channel; > } > > static void mac_dbdma_realize(DeviceState *dev, Error **errp) > diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c > index 9aa7e75..533331a 100644 > --- a/hw/misc/macio/macio.c > +++ b/hw/misc/macio/macio.c > @@ -161,7 +161,6 @@ static void macio_realize_ide(MacIOState *s, MACIOIDEState *ide, > sysbus_connect_irq(sysbus_dev, 1, irq1); > qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid); > object_property_set_link(OBJECT(ide), OBJECT(s->dbdma), "dbdma", errp); > - macio_ide_register_dma(ide); > > object_property_set_bool(OBJECT(ide), true, "realized", errp); > } > diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h > index 26cc469..d6a38c5 100644 > --- a/include/hw/ppc/mac_dbdma.h > +++ b/include/hw/ppc/mac_dbdma.h > @@ -160,19 +160,18 @@ typedef struct DBDMA_channel { > dbdma_cmd current; > } DBDMA_channel; > > -typedef struct { > +typedef struct DBDMAState { > SysBusDevice parent_obj; > > MemoryRegion mem; > DBDMA_channel channels[DBDMA_CHANNELS]; > QEMUBH *bh; > + > + void (*register_channel)(struct DBDMAState *s, int nchan, qemu_irq irq, > + DBDMA_rw rw, DBDMA_flush flush, void *opaque); > } DBDMAState; > > /* Externally callable functions */ > - > -void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq, > - DBDMA_rw rw, DBDMA_flush flush, > - void *opaque); > void DBDMA_kick(DBDMAState *dbdma); > > #define TYPE_MAC_DBDMA "mac-dbdma"
On 26/09/17 04:47, David Gibson wrote: > On Sun, Sep 24, 2017 at 03:47:45PM +0100, Mark Cave-Ayland wrote: >> Using this we can change the MACIO_IDE instance to register the channel >> itself via a type method instead of requiring a separate >> DBDMA_register_channel() function. >> >> As a consequence of this it is now possible to remove the old external >> macio_ide_register_dma() function. >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > Ok, two concerns about this. > > First, you've added the function pointer to the instance, not to the > class, which is not how a QOM method would normally be done. Yeah I did think about whether I needed to create a full class but was torn since as you say there is only one implementation. > More generally, though, why is a method preferable to a plain > function? AFAICT it's not plausible that there will ever be more than > one implementation of the method. > > Same comments apply to patch 7/7. For me it's really for encapsulation. It seems a little odd requiring a global function to configure a QOM object to which I already have a reference. If I were to redo the last 2 patches using a proper class, would you accept them? ATB, Mark.
diff --git a/hw/ide/macio.c b/hw/ide/macio.c index ce194c6..b296017 100644 --- a/hw/ide/macio.c +++ b/hw/ide/macio.c @@ -411,12 +411,18 @@ static const IDEDMAOps dbdma_ops = { static void macio_ide_realizefn(DeviceState *dev, Error **errp) { MACIOIDEState *s = MACIO_IDE(dev); + DBDMAState *dbdma; ide_init2(&s->bus, s->ide_irq); /* Register DMA callbacks */ s->dma.ops = &dbdma_ops; s->bus.dma = &s->dma; + + /* Register DBDMA channel */ + dbdma = MAC_DBDMA(object_property_get_link(OBJECT(dev), "dbdma", errp)); + dbdma->register_channel(dbdma, s->channel, s->dma_irq, + pmac_ide_transfer, pmac_ide_flush, s); } static void pmac_ide_irq(void *opaque, int n, int level) @@ -497,10 +503,4 @@ void macio_ide_init_drives(MACIOIDEState *s, DriveInfo **hd_table) } } -void macio_ide_register_dma(MACIOIDEState *s) -{ - DBDMA_register_channel(s->dbdma, s->channel, s->dma_irq, - pmac_ide_transfer, pmac_ide_flush, s); -} - type_init(macio_ide_register_types) diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c index 0eddf2e..addb97d 100644 --- a/hw/misc/macio/mac_dbdma.c +++ b/hw/misc/macio/mac_dbdma.c @@ -557,11 +557,10 @@ void DBDMA_kick(DBDMAState *dbdma) qemu_bh_schedule(dbdma->bh); } -void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq, - DBDMA_rw rw, DBDMA_flush flush, - void *opaque) +static void +dbdma_register_channel(DBDMAState *s, int nchan, qemu_irq irq, + DBDMA_rw rw, DBDMA_flush flush, void *opaque) { - DBDMAState *s = dbdma; DBDMA_channel *ch = &s->channels[nchan]; DBDMA_DPRINTFCH(ch, "DBDMA_register_channel 0x%x\n", nchan); @@ -903,6 +902,8 @@ static void mac_dbdma_init(Object *obj) memory_region_init_io(&s->mem, obj, &dbdma_ops, s, "dbdma", 0x1000); sysbus_init_mmio(sbd, &s->mem); + + s->register_channel = dbdma_register_channel; } static void mac_dbdma_realize(DeviceState *dev, Error **errp) diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c index 9aa7e75..533331a 100644 --- a/hw/misc/macio/macio.c +++ b/hw/misc/macio/macio.c @@ -161,7 +161,6 @@ static void macio_realize_ide(MacIOState *s, MACIOIDEState *ide, sysbus_connect_irq(sysbus_dev, 1, irq1); qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid); object_property_set_link(OBJECT(ide), OBJECT(s->dbdma), "dbdma", errp); - macio_ide_register_dma(ide); object_property_set_bool(OBJECT(ide), true, "realized", errp); } diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h index 26cc469..d6a38c5 100644 --- a/include/hw/ppc/mac_dbdma.h +++ b/include/hw/ppc/mac_dbdma.h @@ -160,19 +160,18 @@ typedef struct DBDMA_channel { dbdma_cmd current; } DBDMA_channel; -typedef struct { +typedef struct DBDMAState { SysBusDevice parent_obj; MemoryRegion mem; DBDMA_channel channels[DBDMA_CHANNELS]; QEMUBH *bh; + + void (*register_channel)(struct DBDMAState *s, int nchan, qemu_irq irq, + DBDMA_rw rw, DBDMA_flush flush, void *opaque); } DBDMAState; /* Externally callable functions */ - -void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq, - DBDMA_rw rw, DBDMA_flush flush, - void *opaque); void DBDMA_kick(DBDMAState *dbdma); #define TYPE_MAC_DBDMA "mac-dbdma"
Using this we can change the MACIO_IDE instance to register the channel itself via a type method instead of requiring a separate DBDMA_register_channel() function. As a consequence of this it is now possible to remove the old external macio_ide_register_dma() function. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- hw/ide/macio.c | 12 ++++++------ hw/misc/macio/mac_dbdma.c | 9 +++++---- hw/misc/macio/macio.c | 1 - include/hw/ppc/mac_dbdma.h | 9 ++++----- 4 files changed, 15 insertions(+), 16 deletions(-)