[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v6 1/9] hw/i2c/core: Add i2c_try_create_slave() and i2c_reali
From: |
Peter Maydell |
Subject: |
Re: [PATCH v6 1/9] hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref() |
Date: |
Fri, 26 Jun 2020 19:23:02 +0100 |
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 ?
> --- 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 ?
thanks
-- PMM
- [PATCH v6 0/9] hw/misc/pca9552: Trace GPIO change events, Philippe Mathieu-Daudé, 2020/06/23
- [PATCH v6 1/9] hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref(), Philippe Mathieu-Daudé, 2020/06/23
- [PATCH v6 2/9] hw/misc/pca9552: Rename 'nr_leds' as 'pin_count', Philippe Mathieu-Daudé, 2020/06/23
- [PATCH v6 3/9] hw/misc/pca9552: Rename generic code as pca955x, Philippe Mathieu-Daudé, 2020/06/23
- [PATCH v6 4/9] hw/misc/pca9552: Add generic PCA955xClass, parent of TYPE_PCA9552, Philippe Mathieu-Daudé, 2020/06/23
- [PATCH v6 5/9] hw/misc/pca9552: Add a 'description' property for debugging purpose, Philippe Mathieu-Daudé, 2020/06/23
- [PATCH v6 6/9] hw/misc/pca9552: Trace GPIO High/Low events, Philippe Mathieu-Daudé, 2020/06/23
- [PATCH v6 7/9] hw/arm/aspeed: Describe each PCA9552 device, Philippe Mathieu-Daudé, 2020/06/23
- [PATCH v6 8/9] hw/misc/pca9552: Trace GPIO change events, Philippe Mathieu-Daudé, 2020/06/23