Re: [Qemu-devel] [PATCH 6/7] mac_dbdma: change DBDMA_register_channel to a MAC_DBDMA type method

2017-09-28 Thread Mark Cave-Ayland 
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 
> 
> 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.

Re: [Qemu-devel] [PATCH 6/7] mac_dbdma: change DBDMA_register_channel to a MAC_DBDMA type method

2017-09-25 Thread David Gibson 
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 

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(>bus, s->ide_irq);
>  
>  /* Register DMA callbacks */
>  s->dma.ops = _ops;
>  s->bus.dma = >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 = >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(>mem, obj, _ops, s, "dbdma", 0x1000);
>  sysbus_init_mmio(sbd, >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..51a 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


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH 6/7] mac_dbdma: change DBDMA_register_channel to a MAC_DBDMA type method

2017-09-24 Thread Mark Cave-Ayland 
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 
---
 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(>bus, s->ide_irq);
 
 /* Register DMA callbacks */
 s->dma.ops = _ops;
 s->bus.dma = >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 = >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(>mem, obj, _ops, s, "dbdma", 0x1000);
 sysbus_init_mmio(sbd, >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..51a 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