Re: [PATCH 04/14] etraxfs: remove PROP_PTR usage

[Peter Maydell]

On Fri, 18 Oct 2019 at 16:42, Marc-André Lureau
<address@hidden> wrote:
>
> etraxfs_dma_client are not Object, so can't be exposed to user with
> QOM path. Let's remove property usage and move the constructor to the
> .c unit, simplifying some code on the way.
>
> Signed-off-by: Marc-André Lureau <address@hidden>

> +
> +/* Instantiate an ETRAXFS Ethernet MAC.  */
> +DeviceState *
> +etraxfs_eth_init(NICInfo *nd, hwaddr base, int phyaddr,
> +                 struct etraxfs_dma_client *dma_out,
> +                 struct etraxfs_dma_client *dma_in)
> +{
> +    DeviceState *dev;
> +    qemu_check_nic_model(nd, "fseth");
> +
> +    dev = qdev_create(NULL, "etraxfs-eth");
> +    qdev_set_nic_properties(dev, nd);
> +    qdev_prop_set_uint32(dev, "phyaddr", phyaddr);
> +    ETRAX_FS_ETH(dev)->dma_out = dma_out;
> +    ETRAX_FS_ETH(dev)->dma_in = dma_in;
> +    qdev_init_nofail(dev);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> +
> +    return dev;
> +}
> +
>  static const TypeInfo etraxfs_eth_info = {
>      .name          = TYPE_ETRAX_FS_ETH,
>      .parent        = TYPE_SYS_BUS_DEVICE,
> diff --git a/include/hw/cris/etraxfs.h b/include/hw/cris/etraxfs.h
> index aa146a2cd8..403e7f95e6 100644
> --- a/include/hw/cris/etraxfs.h
> +++ b/include/hw/cris/etraxfs.h
> @@ -30,23 +30,9 @@
>  #include "hw/qdev-properties.h"
>  #include "hw/sysbus.h"
>
> -/* Instantiate an ETRAXFS Ethernet MAC.  */
> -static inline DeviceState *
> -etraxfs_eth_init(NICInfo *nd, hwaddr base, int phyaddr,
> -                 void *dma_out, void *dma_in)
> -{
> -    DeviceState *dev;
> -    qemu_check_nic_model(nd, "fseth");
> -
> -    dev = qdev_create(NULL, "etraxfs-eth");
> -    qdev_set_nic_properties(dev, nd);
> -    qdev_prop_set_uint32(dev, "phyaddr", phyaddr);
> -    qdev_prop_set_ptr(dev, "dma_out", dma_out);
> -    qdev_prop_set_ptr(dev, "dma_in", dma_in);
> -    qdev_init_nofail(dev);
> -    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> -    return dev;
> -}
> +DeviceState *etraxfs_eth_init(NICInfo *nd, hwaddr base, int phyaddr,
> +                              struct etraxfs_dma_client *dma_out,
> +                              struct etraxfs_dma_client *dma_in);


I don't think this is an improvement -- it's taking a step
back in the direction of "you need to call a funny _init
function to initialize a device". You should be able to
create devices using generic qdev functions.

What we're actually connecting here is 'etraxfs_dma_client'
struct pointers between the devices like this ethernet
device and the DMA controller. The connection is currently
done via a pointer property because we don't have a more
QOM-like way to do it, but if we want to get rid of the
pointer property we need to actually implement the more
QOM-like mechanism, not go backwards from having devices
connected via properties.

(Similar comments apply for the omap clock connections.
In that case the answer might be Damien's clock framework
API, eventually.)

thanks
-- PMM