Re: [PATCH v6 1/9] hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref()

[Philippe Mathieu-Daudé]

On Fri, Jun 26, 2020 at 8:27 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> On Tue, 23 Jun 2020 at 08:27, Philippe Mathieu-Daudé <f4bug@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 <cminyard@mvista.com>
> > Reviewed-by: Cédric Le Goater <clg@kaod.org>
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@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