On Fri, Jun 26, 2020 at 8:27 PM Peter Maydell <peter.mayd...@linaro.org> wrote: > On Tue, 23 Jun 2020 at 08:27, Philippe Mathieu-Daudé <f4...@amsat.org> wrote: > > Extract i2c_try_create_slave() and i2c_realize_and_unref() > > from i2c_create_slave(). > > We can now set properties on a I2CSlave before it is realized. > > > > This is in line with the recent qdev/QOM changes merged > > in commit 6675a653d2e. > > > > Reviewed-by: Corey Minyard <cminy...@mvista.com> > > Reviewed-by: Cédric Le Goater <c...@kaod.org> > > Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org> > > Couple of things I belatedly noticed on this patch, which I don't > think are important enough for me to drop it from my pullreq, > but which I think it would be nice to address in a followup: > > > diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h > > index 4117211565..d6e3d85faf 100644 > > --- a/include/hw/i2c/i2c.h > > +++ b/include/hw/i2c/i2c.h > > @@ -80,6 +80,8 @@ int i2c_send(I2CBus *bus, uint8_t data); > > uint8_t i2c_recv(I2CBus *bus); > > > > DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr); > > +DeviceState *i2c_try_create_slave(const char *name, uint8_t addr); > > +bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp); > > Can we have doc-comments for new global-scope functions, please ?
Sure. > > > --- a/hw/i2c/core.c > > +++ b/hw/i2c/core.c > > @@ -267,13 +267,27 @@ const VMStateDescription vmstate_i2c_slave = { > > } > > }; > > > > -DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr) > > +DeviceState *i2c_try_create_slave(const char *name, uint8_t addr) > > { > > DeviceState *dev; > > > > dev = qdev_new(name); > > qdev_prop_set_uint8(dev, "address", addr); > > - qdev_realize_and_unref(dev, &bus->qbus, &error_fatal); > > + return dev; > > +} > > + > > +bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp) > > +{ > > + return qdev_realize_and_unref(dev, &bus->qbus, errp); > > +} > > + > > +DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr) > > +{ > > + DeviceState *dev; > > + > > + dev = i2c_try_create_slave(name, addr); > > + i2c_realize_and_unref(dev, bus, &error_fatal); > > + > > We now have a _try_ function which isn't "same behaviour as > the non-try function, but give me back an error status rather > than just killing QEMU". That seems confusing -- is there a > better name we can use ? Yes, Markus asked me to change that, but as it could be done on top, he didn't block the series. I'll address that next week. > thanks > -- PMM