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

[Marc-André Lureau]

Hi

On Fri, Oct 18, 2019 at 5:59 PM Peter Maydell <address@hidden> wrote:
>
> 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.
>

Is there really much difference between:

dev = qdev_create()
qdev_prop_set_ptr(dev, "prop", ptr);
qdev_init_nofail()

and

dev = qdev_create(MYDEV)
MYDEV(dev)->prop = ptr;
qdev_init_nofail()


As "prop" can only be set from code, and those objects are usually
very tightly coupled with the parent/owner.

I don't think it's worth to keep PROP_PTR for this, it just adds complexity.

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


-- 
Marc-André Lureau