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(-)
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"
--
1.7.10.4
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"
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
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.
On Thu, Sep 28, 2017 at 07:40:18AM +0100, Mark Cave-Ayland wrote: > 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. Instead you're using the method which is defined in a global type definition. I don't think it really makes any different to encapsulation. > If I were to redo the last 2 patches using a proper class, would you > accept them? > > > ATB, > > Mark. > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
© 2016 - 2026 Red Hat, Inc.