[PATCH v6 0/9] hw/misc/pca9552: Trace GPIO change events

[PATCH v6 0/9] hw/misc/pca9552: Trace GPIO change events

[Philippe Mathieu-Daudé]

This series add trace events to better display GPIO changes.
We'll continue in the following series by connecting LEDs to
these GPIOs.

This helps me to work on a generic LED device, see:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg711917.html

Since v5, addressed Cédric review comment:
- Move pin_count check from realize() to instance_init()

Since v4: Addressed Cédric review comments
- Extract PCA955xClass
- Add/use pca955x_pins_get_status() method instead of keeping
  cached value in PCA955xState

Example when booting an obmc-phosphor-image, we can see the LED #14
(front-power LED) starting to blink.

- ASCII LED bar view:

  $ qemu-system-arm -M witherspoon-bmc -trace pca955x_gpio_status
  1592689902.327837:pca955x_gpio_status pca-unspecified GPIOs 0-15 [*...............]
  1592689902.329934:pca955x_gpio_status pca-unspecified GPIOs 0-15 [**..............]
  1592689902.330717:pca955x_gpio_status pca-unspecified GPIOs 0-15 [***.............]
  1592689902.331431:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****............]
  1592689902.332163:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........*..]
  1592689902.332888:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........**.]
  1592689902.333629:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
  1592690032.793289:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]
  1592690033.303163:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
  1592690033.812962:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]
  1592690034.323234:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
  1592690034.832922:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]

- Only display GPIOs which status changes:

  $ qemu-system-arm -M witherspoon-bmc -trace pca955x_gpio_change
  1592690552.687372:pca955x_gpio_change pca1 GPIO id:0 status: 0 -> 1
  1592690552.690169:pca955x_gpio_change pca1 GPIO id:1 status: 0 -> 1
  1592690552.691673:pca955x_gpio_change pca1 GPIO id:2 status: 0 -> 1
  1592690552.696886:pca955x_gpio_change pca1 GPIO id:3 status: 0 -> 1
  1592690552.698614:pca955x_gpio_change pca1 GPIO id:13 status: 0 -> 1
  1592690552.699833:pca955x_gpio_change pca1 GPIO id:14 status: 0 -> 1
  1592690552.700842:pca955x_gpio_change pca1 GPIO id:15 status: 0 -> 1
  1592690683.841921:pca955x_gpio_change pca1 GPIO id:14 status: 1 -> 0
  1592690683.861660:pca955x_gpio_change pca1 GPIO id:14 status: 0 -> 1
  1592690684.371460:pca955x_gpio_change pca1 GPIO id:14 status: 1 -> 0
  1592690684.882115:pca955x_gpio_change pca1 GPIO id:14 status: 0 -> 1
  1592690685.391411:pca955x_gpio_change pca1 GPIO id:14 status: 1 -> 0
  1592690685.901391:pca955x_gpio_change pca1 GPIO id:14 status: 0 -> 1

For information about how to test the obmc-phosphor-image, see:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg712911.html

$ git backport-diff -u v5
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/9:[----] [--] 'hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref()'
002/9:[----] [--] 'hw/misc/pca9552: Rename 'nr_leds' as 'pin_count''
003/9:[----] [--] 'hw/misc/pca9552: Rename generic code as pca955x'
004/9:[0010] [FC] 'hw/misc/pca9552: Add generic PCA955xClass, parent of TYPE_PCA9552'
005/9:[0007] [FC] 'hw/misc/pca9552: Add a 'description' property for debugging purpose'
006/9:[----] [--] 'hw/misc/pca9552: Trace GPIO High/Low events'
007/9:[----] [-C] 'hw/arm/aspeed: Describe each PCA9552 device'
008/9:[----] [--] 'hw/misc/pca9552: Trace GPIO change events'
009/9:[0003] [FC] 'hw/misc/pca9552: Model qdev output GPIOs'

Based-on: <20200623072132.2868-1-f4bug@amsat.org>

Philippe Mathieu-Daudé (9):
  hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref()
  hw/misc/pca9552: Rename 'nr_leds' as 'pin_count'
  hw/misc/pca9552: Rename generic code as pca955x
  hw/misc/pca9552: Add generic PCA955xClass, parent of TYPE_PCA9552
  hw/misc/pca9552: Add a 'description' property for debugging purpose
  hw/misc/pca9552: Trace GPIO High/Low events
  hw/arm/aspeed: Describe each PCA9552 device
  hw/misc/pca9552: Trace GPIO change events
  hw/misc/pca9552: Model qdev output GPIOs

 include/hw/i2c/i2c.h      |   2 +
 include/hw/misc/pca9552.h |  16 +--
 hw/arm/aspeed.c           |  13 ++-
 hw/i2c/core.c             |  18 +++-
 hw/misc/pca9552.c         | 216 ++++++++++++++++++++++++++++----------
 hw/misc/trace-events      |   4 +
 6 files changed, 202 insertions(+), 67 deletions(-)

-- 
2.21.3

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

[Philippe Mathieu-Daudé]

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>
---
Cc: Markus Armbruster <armbru@redhat.com>
---
 include/hw/i2c/i2c.h |  2 ++
 hw/i2c/core.c        | 18 ++++++++++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

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);
 
 /* lm832x.c */
 void lm832x_key_event(DeviceState *dev, int key, int state);
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 1aac457a2a..acf34a12d6 100644
--- 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);
+
     return dev;
 }
 
-- 
2.21.3

[PATCH v6 2/9] hw/misc/pca9552: Rename 'nr_leds' as 'pin_count'

[Philippe Mathieu-Daudé]

The PCA9552 device does not expose LEDs, but simple pins
to connnect LEDs to. To be clearer with the device model,
rename 'nr_leds' as 'pin_count'.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/pca9552.h |  2 +-
 hw/misc/pca9552.c         | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
index ebb43c63fe..bc5ed31087 100644
--- a/include/hw/misc/pca9552.h
+++ b/include/hw/misc/pca9552.h
@@ -26,7 +26,7 @@ typedef struct PCA9552State {
 
     uint8_t regs[PCA9552_NR_REGS];
     uint8_t max_reg;
-    uint8_t nr_leds;
+    uint8_t pin_count;
 } PCA9552State;
 
 #endif
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index cac729e35a..81da757a7e 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -37,7 +37,7 @@ static void pca9552_update_pin_input(PCA9552State *s)
 {
     int i;
 
-    for (i = 0; i < s->nr_leds; i++) {
+    for (i = 0; i < s->pin_count; i++) {
         uint8_t input_reg = PCA9552_INPUT0 + (i / 8);
         uint8_t input_shift = (i % 8);
         uint8_t config = pca9552_pin_get_config(s, i);
@@ -185,7 +185,7 @@ static void pca9552_get_led(Object *obj, Visitor *v, const char *name,
         error_setg(errp, "%s: error reading %s", __func__, name);
         return;
     }
-    if (led < 0 || led > s->nr_leds) {
+    if (led < 0 || led > s->pin_count) {
         error_setg(errp, "%s invalid led %s", __func__, name);
         return;
     }
@@ -228,7 +228,7 @@ static void pca9552_set_led(Object *obj, Visitor *v, const char *name,
         error_setg(errp, "%s: error reading %s", __func__, name);
         return;
     }
-    if (led < 0 || led > s->nr_leds) {
+    if (led < 0 || led > s->pin_count) {
         error_setg(errp, "%s invalid led %s", __func__, name);
         return;
     }
@@ -291,9 +291,9 @@ static void pca9552_initfn(Object *obj)
      * PCA955X device
      */
     s->max_reg = PCA9552_LS3;
-    s->nr_leds = 16;
+    s->pin_count = 16;
 
-    for (led = 0; led < s->nr_leds; led++) {
+    for (led = 0; led < s->pin_count; led++) {
         char *name;
 
         name = g_strdup_printf("led%d", led);
-- 
2.21.3

[PATCH v6 3/9] hw/misc/pca9552: Rename generic code as pca955x

[Philippe Mathieu-Daudé]

Various code from the PCA9552 device model is generic to the
PCA955X family. We'll split the generic code in a base class
in the next commit. To ease review, first do a dumb renaming.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/pca9552.h | 10 ++---
 hw/misc/pca9552.c         | 80 +++++++++++++++++++--------------------
 2 files changed, 45 insertions(+), 45 deletions(-)

diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
index bc5ed31087..db527595a3 100644
--- a/include/hw/misc/pca9552.h
+++ b/include/hw/misc/pca9552.h
@@ -12,11 +12,11 @@
 #include "hw/i2c/i2c.h"
 
 #define TYPE_PCA9552 "pca9552"
-#define PCA9552(obj) OBJECT_CHECK(PCA9552State, (obj), TYPE_PCA9552)
+#define PCA955X(obj) OBJECT_CHECK(PCA955xState, (obj), TYPE_PCA9552)
 
-#define PCA9552_NR_REGS 10
+#define PCA955X_NR_REGS 10
 
-typedef struct PCA9552State {
+typedef struct PCA955xState {
     /*< private >*/
     I2CSlave i2c;
     /*< public >*/
@@ -24,9 +24,9 @@ typedef struct PCA9552State {
     uint8_t len;
     uint8_t pointer;
 
-    uint8_t regs[PCA9552_NR_REGS];
+    uint8_t regs[PCA955X_NR_REGS];
     uint8_t max_reg;
     uint8_t pin_count;
-} PCA9552State;
+} PCA955xState;
 
 #endif
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index 81da757a7e..5681ff3b22 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -25,7 +25,7 @@
 
 static const char *led_state[] = {"on", "off", "pwm0", "pwm1"};
 
-static uint8_t pca9552_pin_get_config(PCA9552State *s, int pin)
+static uint8_t pca955x_pin_get_config(PCA955xState *s, int pin)
 {
     uint8_t reg   = PCA9552_LS0 + (pin / 4);
     uint8_t shift = (pin % 4) << 1;
@@ -33,14 +33,14 @@ static uint8_t pca9552_pin_get_config(PCA9552State *s, int pin)
     return extract32(s->regs[reg], shift, 2);
 }
 
-static void pca9552_update_pin_input(PCA9552State *s)
+static void pca955x_update_pin_input(PCA955xState *s)
 {
     int i;
 
     for (i = 0; i < s->pin_count; i++) {
         uint8_t input_reg = PCA9552_INPUT0 + (i / 8);
         uint8_t input_shift = (i % 8);
-        uint8_t config = pca9552_pin_get_config(s, i);
+        uint8_t config = pca955x_pin_get_config(s, i);
 
         switch (config) {
         case PCA9552_LED_ON:
@@ -58,7 +58,7 @@ static void pca9552_update_pin_input(PCA9552State *s)
     }
 }
 
-static uint8_t pca9552_read(PCA9552State *s, uint8_t reg)
+static uint8_t pca955x_read(PCA955xState *s, uint8_t reg)
 {
     switch (reg) {
     case PCA9552_INPUT0:
@@ -79,7 +79,7 @@ static uint8_t pca9552_read(PCA9552State *s, uint8_t reg)
     }
 }
 
-static void pca9552_write(PCA9552State *s, uint8_t reg, uint8_t data)
+static void pca955x_write(PCA955xState *s, uint8_t reg, uint8_t data)
 {
     switch (reg) {
     case PCA9552_PSC0:
@@ -94,7 +94,7 @@ static void pca9552_write(PCA9552State *s, uint8_t reg, uint8_t data)
     case PCA9552_LS2:
     case PCA9552_LS3:
         s->regs[reg] = data;
-        pca9552_update_pin_input(s);
+        pca955x_update_pin_input(s);
         break;
 
     case PCA9552_INPUT0:
@@ -110,7 +110,7 @@ static void pca9552_write(PCA9552State *s, uint8_t reg, uint8_t data)
  * after each byte is sent to or received by the device. The index
  * rollovers to 0 when the maximum register address is reached.
  */
-static void pca9552_autoinc(PCA9552State *s)
+static void pca955x_autoinc(PCA955xState *s)
 {
     if (s->pointer != 0xFF && s->pointer & PCA9552_AUTOINC) {
         uint8_t reg = s->pointer & 0xf;
@@ -120,12 +120,12 @@ static void pca9552_autoinc(PCA9552State *s)
     }
 }
 
-static uint8_t pca9552_recv(I2CSlave *i2c)
+static uint8_t pca955x_recv(I2CSlave *i2c)
 {
-    PCA9552State *s = PCA9552(i2c);
+    PCA955xState *s = PCA955X(i2c);
     uint8_t ret;
 
-    ret = pca9552_read(s, s->pointer & 0xf);
+    ret = pca955x_read(s, s->pointer & 0xf);
 
     /*
      * From the Specs:
@@ -143,40 +143,40 @@ static uint8_t pca9552_recv(I2CSlave *i2c)
                       __func__);
     }
 
-    pca9552_autoinc(s);
+    pca955x_autoinc(s);
 
     return ret;
 }
 
-static int pca9552_send(I2CSlave *i2c, uint8_t data)
+static int pca955x_send(I2CSlave *i2c, uint8_t data)
 {
-    PCA9552State *s = PCA9552(i2c);
+    PCA955xState *s = PCA955X(i2c);
 
     /* First byte sent by is the register address */
     if (s->len == 0) {
         s->pointer = data;
         s->len++;
     } else {
-        pca9552_write(s, s->pointer & 0xf, data);
+        pca955x_write(s, s->pointer & 0xf, data);
 
-        pca9552_autoinc(s);
+        pca955x_autoinc(s);
     }
 
     return 0;
 }
 
-static int pca9552_event(I2CSlave *i2c, enum i2c_event event)
+static int pca955x_event(I2CSlave *i2c, enum i2c_event event)
 {
-    PCA9552State *s = PCA9552(i2c);
+    PCA955xState *s = PCA955X(i2c);
 
     s->len = 0;
     return 0;
 }
 
-static void pca9552_get_led(Object *obj, Visitor *v, const char *name,
+static void pca955x_get_led(Object *obj, Visitor *v, const char *name,
                             void *opaque, Error **errp)
 {
-    PCA9552State *s = PCA9552(obj);
+    PCA955xState *s = PCA955X(obj);
     int led, rc, reg;
     uint8_t state;
 
@@ -195,7 +195,7 @@ static void pca9552_get_led(Object *obj, Visitor *v, const char *name,
      * reading the INPUTx reg
      */
     reg = PCA9552_LS0 + led / 4;
-    state = (pca9552_read(s, reg) >> (led % 8)) & 0x3;
+    state = (pca955x_read(s, reg) >> (led % 8)) & 0x3;
     visit_type_str(v, name, (char **)&led_state[state], errp);
 }
 
@@ -209,10 +209,10 @@ static inline uint8_t pca955x_ledsel(uint8_t oldval, int led_num, int state)
                 ((state & 0x3) << (led_num << 1));
 }
 
-static void pca9552_set_led(Object *obj, Visitor *v, const char *name,
+static void pca955x_set_led(Object *obj, Visitor *v, const char *name,
                             void *opaque, Error **errp)
 {
-    PCA9552State *s = PCA9552(obj);
+    PCA955xState *s = PCA955X(obj);
     Error *local_err = NULL;
     int led, rc, reg, val;
     uint8_t state;
@@ -244,9 +244,9 @@ static void pca9552_set_led(Object *obj, Visitor *v, const char *name,
     }
 
     reg = PCA9552_LS0 + led / 4;
-    val = pca9552_read(s, reg);
+    val = pca955x_read(s, reg);
     val = pca955x_ledsel(val, led % 4, state);
-    pca9552_write(s, reg, val);
+    pca955x_write(s, reg, val);
 }
 
 static const VMStateDescription pca9552_vmstate = {
@@ -254,17 +254,17 @@ static const VMStateDescription pca9552_vmstate = {
     .version_id = 0,
     .minimum_version_id = 0,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT8(len, PCA9552State),
-        VMSTATE_UINT8(pointer, PCA9552State),
-        VMSTATE_UINT8_ARRAY(regs, PCA9552State, PCA9552_NR_REGS),
-        VMSTATE_I2C_SLAVE(i2c, PCA9552State),
+        VMSTATE_UINT8(len, PCA955xState),
+        VMSTATE_UINT8(pointer, PCA955xState),
+        VMSTATE_UINT8_ARRAY(regs, PCA955xState, PCA955X_NR_REGS),
+        VMSTATE_I2C_SLAVE(i2c, PCA955xState),
         VMSTATE_END_OF_LIST()
     }
 };
 
 static void pca9552_reset(DeviceState *dev)
 {
-    PCA9552State *s = PCA9552(dev);
+    PCA955xState *s = PCA955X(dev);
 
     s->regs[PCA9552_PSC0] = 0xFF;
     s->regs[PCA9552_PWM0] = 0x80;
@@ -275,15 +275,15 @@ static void pca9552_reset(DeviceState *dev)
     s->regs[PCA9552_LS2] = 0x55;
     s->regs[PCA9552_LS3] = 0x55;
 
-    pca9552_update_pin_input(s);
+    pca955x_update_pin_input(s);
 
     s->pointer = 0xFF;
     s->len = 0;
 }
 
-static void pca9552_initfn(Object *obj)
+static void pca955x_initfn(Object *obj)
 {
-    PCA9552State *s = PCA9552(obj);
+    PCA955xState *s = PCA955X(obj);
     int led;
 
     /* If support for the other PCA955X devices are implemented, these
@@ -297,7 +297,7 @@ static void pca9552_initfn(Object *obj)
         char *name;
 
         name = g_strdup_printf("led%d", led);
-        object_property_add(obj, name, "bool", pca9552_get_led, pca9552_set_led,
+        object_property_add(obj, name, "bool", pca955x_get_led, pca955x_set_led,
                             NULL, NULL);
         g_free(name);
     }
@@ -308,9 +308,9 @@ static void pca9552_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
 
-    k->event = pca9552_event;
-    k->recv = pca9552_recv;
-    k->send = pca9552_send;
+    k->event = pca955x_event;
+    k->recv = pca955x_recv;
+    k->send = pca955x_send;
     dc->reset = pca9552_reset;
     dc->vmsd = &pca9552_vmstate;
 }
@@ -318,14 +318,14 @@ static void pca9552_class_init(ObjectClass *klass, void *data)
 static const TypeInfo pca9552_info = {
     .name          = TYPE_PCA9552,
     .parent        = TYPE_I2C_SLAVE,
-    .instance_init = pca9552_initfn,
-    .instance_size = sizeof(PCA9552State),
+    .instance_init = pca955x_initfn,
+    .instance_size = sizeof(PCA955xState),
     .class_init    = pca9552_class_init,
 };
 
-static void pca9552_register_types(void)
+static void pca955x_register_types(void)
 {
     type_register_static(&pca9552_info);
 }
 
-type_init(pca9552_register_types)
+type_init(pca955x_register_types)
-- 
2.21.3

[PATCH v6 4/9] hw/misc/pca9552: Add generic PCA955xClass, parent of TYPE_PCA9552

[Philippe Mathieu-Daudé]

Extract the code common to the PCA955x family in PCA955xClass,
keeping the PCA9552 specific parts into pca9552_class_init().
Remove the 'TODO' comment added in commit 5141d4158cf.

Suggested-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/pca9552.h |  6 ++--
 hw/misc/pca9552.c         | 66 ++++++++++++++++++++++++++++-----------
 2 files changed, 51 insertions(+), 21 deletions(-)

diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
index db527595a3..90843b03b8 100644
--- a/include/hw/misc/pca9552.h
+++ b/include/hw/misc/pca9552.h
@@ -12,9 +12,11 @@
 #include "hw/i2c/i2c.h"
 
 #define TYPE_PCA9552 "pca9552"
-#define PCA955X(obj) OBJECT_CHECK(PCA955xState, (obj), TYPE_PCA9552)
+#define TYPE_PCA955X "pca955x"
+#define PCA955X(obj) OBJECT_CHECK(PCA955xState, (obj), TYPE_PCA955X)
 
 #define PCA955X_NR_REGS 10
+#define PCA955X_PIN_COUNT_MAX 16
 
 typedef struct PCA955xState {
     /*< private >*/
@@ -25,8 +27,6 @@ typedef struct PCA955xState {
     uint8_t pointer;
 
     uint8_t regs[PCA955X_NR_REGS];
-    uint8_t max_reg;
-    uint8_t pin_count;
 } PCA955xState;
 
 #endif
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index 5681ff3b22..4de57dbe2e 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -4,6 +4,7 @@
  *     https://www.nxp.com/docs/en/application-note/AN264.pdf
  *
  * Copyright (c) 2017-2018, IBM Corporation.
+ * Copyright (c) 2020 Philippe Mathieu-Daudé
  *
  * This work is licensed under the terms of the GNU GPL, version 2 or
  * later. See the COPYING file in the top-level directory.
@@ -18,6 +19,20 @@
 #include "qapi/error.h"
 #include "qapi/visitor.h"
 
+typedef struct PCA955xClass {
+    /*< private >*/
+    I2CSlaveClass parent_class;
+    /*< public >*/
+
+    uint8_t pin_count;
+    uint8_t max_reg;
+} PCA955xClass;
+
+#define PCA955X_CLASS(klass) \
+    OBJECT_CLASS_CHECK(PCA955xClass, (klass), TYPE_PCA955X)
+#define PCA955X_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(PCA955xClass, (obj), TYPE_PCA955X)
+
 #define PCA9552_LED_ON   0x0
 #define PCA9552_LED_OFF  0x1
 #define PCA9552_LED_PWM0 0x2
@@ -35,9 +50,10 @@ static uint8_t pca955x_pin_get_config(PCA955xState *s, int pin)
 
 static void pca955x_update_pin_input(PCA955xState *s)
 {
+    PCA955xClass *k = PCA955X_GET_CLASS(s);
     int i;
 
-    for (i = 0; i < s->pin_count; i++) {
+    for (i = 0; i < k->pin_count; i++) {
         uint8_t input_reg = PCA9552_INPUT0 + (i / 8);
         uint8_t input_shift = (i % 8);
         uint8_t config = pca955x_pin_get_config(s, i);
@@ -112,10 +128,12 @@ static void pca955x_write(PCA955xState *s, uint8_t reg, uint8_t data)
  */
 static void pca955x_autoinc(PCA955xState *s)
 {
+    PCA955xClass *k = PCA955X_GET_CLASS(s);
+
     if (s->pointer != 0xFF && s->pointer & PCA9552_AUTOINC) {
         uint8_t reg = s->pointer & 0xf;
 
-        reg = (reg + 1) % (s->max_reg + 1);
+        reg = (reg + 1) % (k->max_reg + 1);
         s->pointer = reg | PCA9552_AUTOINC;
     }
 }
@@ -176,6 +194,7 @@ static int pca955x_event(I2CSlave *i2c, enum i2c_event event)
 static void pca955x_get_led(Object *obj, Visitor *v, const char *name,
                             void *opaque, Error **errp)
 {
+    PCA955xClass *k = PCA955X_GET_CLASS(obj);
     PCA955xState *s = PCA955X(obj);
     int led, rc, reg;
     uint8_t state;
@@ -185,7 +204,7 @@ static void pca955x_get_led(Object *obj, Visitor *v, const char *name,
         error_setg(errp, "%s: error reading %s", __func__, name);
         return;
     }
-    if (led < 0 || led > s->pin_count) {
+    if (led < 0 || led > k->pin_count) {
         error_setg(errp, "%s invalid led %s", __func__, name);
         return;
     }
@@ -212,6 +231,7 @@ static inline uint8_t pca955x_ledsel(uint8_t oldval, int led_num, int state)
 static void pca955x_set_led(Object *obj, Visitor *v, const char *name,
                             void *opaque, Error **errp)
 {
+    PCA955xClass *k = PCA955X_GET_CLASS(obj);
     PCA955xState *s = PCA955X(obj);
     Error *local_err = NULL;
     int led, rc, reg, val;
@@ -228,7 +248,7 @@ static void pca955x_set_led(Object *obj, Visitor *v, const char *name,
         error_setg(errp, "%s: error reading %s", __func__, name);
         return;
     }
-    if (led < 0 || led > s->pin_count) {
+    if (led < 0 || led > k->pin_count) {
         error_setg(errp, "%s invalid led %s", __func__, name);
         return;
     }
@@ -283,17 +303,11 @@ static void pca9552_reset(DeviceState *dev)
 
 static void pca955x_initfn(Object *obj)
 {
-    PCA955xState *s = PCA955X(obj);
+    PCA955xClass *k = PCA955X_GET_CLASS(obj);
     int led;
 
-    /* If support for the other PCA955X devices are implemented, these
-     * constant values might be part of class structure describing the
-     * PCA955X device
-     */
-    s->max_reg = PCA9552_LS3;
-    s->pin_count = 16;
-
-    for (led = 0; led < s->pin_count; led++) {
+    assert(k->pin_count <= PCA955X_PIN_COUNT_MAX);
+    for (led = 0; led < k->pin_count; led++) {
         char *name;
 
         name = g_strdup_printf("led%d", led);
@@ -303,28 +317,44 @@ static void pca955x_initfn(Object *obj)
     }
 }
 
-static void pca9552_class_init(ObjectClass *klass, void *data)
+static void pca955x_class_init(ObjectClass *klass, void *data)
 {
-    DeviceClass *dc = DEVICE_CLASS(klass);
     I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
 
     k->event = pca955x_event;
     k->recv = pca955x_recv;
     k->send = pca955x_send;
+}
+
+static const TypeInfo pca955x_info = {
+    .name          = TYPE_PCA955X,
+    .parent        = TYPE_I2C_SLAVE,
+    .instance_init = pca955x_initfn,
+    .instance_size = sizeof(PCA955xState),
+    .class_init    = pca955x_class_init,
+    .abstract      = true,
+};
+
+static void pca9552_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    PCA955xClass *pc = PCA955X_CLASS(oc);
+
     dc->reset = pca9552_reset;
     dc->vmsd = &pca9552_vmstate;
+    pc->max_reg = PCA9552_LS3;
+    pc->pin_count = 16;
 }
 
 static const TypeInfo pca9552_info = {
     .name          = TYPE_PCA9552,
-    .parent        = TYPE_I2C_SLAVE,
-    .instance_init = pca955x_initfn,
-    .instance_size = sizeof(PCA955xState),
+    .parent        = TYPE_PCA955X,
     .class_init    = pca9552_class_init,
 };
 
 static void pca955x_register_types(void)
 {
+    type_register_static(&pca955x_info);
     type_register_static(&pca9552_info);
 }
 
-- 
2.21.3

[PATCH v6 5/9] hw/misc/pca9552: Add a 'description' property for debugging purpose

[Philippe Mathieu-Daudé]

Add a description field to distinguish between multiple devices.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/pca9552.h |  1 +
 hw/misc/pca9552.c         | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
index 90843b03b8..bf1a589137 100644
--- a/include/hw/misc/pca9552.h
+++ b/include/hw/misc/pca9552.h
@@ -27,6 +27,7 @@ typedef struct PCA955xState {
     uint8_t pointer;
 
     uint8_t regs[PCA955X_NR_REGS];
+    char *description; /* For debugging purpose only */
 } PCA955xState;
 
 #endif
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index 4de57dbe2e..2cc52b0205 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -13,6 +13,7 @@
 #include "qemu/osdep.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "hw/qdev-properties.h"
 #include "hw/misc/pca9552.h"
 #include "hw/misc/pca9552_regs.h"
 #include "migration/vmstate.h"
@@ -317,13 +318,30 @@ static void pca955x_initfn(Object *obj)
     }
 }
 
+static void pca955x_realize(DeviceState *dev, Error **errp)
+{
+    PCA955xState *s = PCA955X(dev);
+
+    if (!s->description) {
+        s->description = g_strdup("pca-unspecified");
+    }
+}
+
+static Property pca955x_properties[] = {
+    DEFINE_PROP_STRING("description", PCA955xState, description),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void pca955x_class_init(ObjectClass *klass, void *data)
 {
+    DeviceClass *dc = DEVICE_CLASS(klass);
     I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
 
     k->event = pca955x_event;
     k->recv = pca955x_recv;
     k->send = pca955x_send;
+    dc->realize = pca955x_realize;
+    device_class_set_props(dc, pca955x_properties);
 }
 
 static const TypeInfo pca955x_info = {
-- 
2.21.3

[PATCH v6 6/9] hw/misc/pca9552: Trace GPIO High/Low events

[Philippe Mathieu-Daudé]

Add a trivial representation of the PCA9552 GPIOs.

Example booting obmc-phosphor-image:

  $ qemu-system-arm -M witherspoon-bmc -trace pca955x_gpio_status
  1592689902.327837:pca955x_gpio_status pca-unspecified GPIOs 0-15 [*...............]
  1592689902.329934:pca955x_gpio_status pca-unspecified GPIOs 0-15 [**..............]
  1592689902.330717:pca955x_gpio_status pca-unspecified GPIOs 0-15 [***.............]
  1592689902.331431:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****............]
  1592689902.332163:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........*..]
  1592689902.332888:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........**.]
  1592689902.333629:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
  1592690032.793289:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]
  1592690033.303163:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
  1592690033.812962:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]
  1592690034.323234:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
  1592690034.832922:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]

We notice the GPIO #14 (front-power LED) starts to blink.

This LED is described in the witherspoon device-tree [*]:

  front-power {
      retain-state-shutdown;
      default-state = "keep";
      gpios = <&pca0 14 GPIO_ACTIVE_LOW>;
  };

[*] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts?id=b1f9be9392f0#n140

Suggested-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/misc/pca9552.c    | 39 +++++++++++++++++++++++++++++++++++++++
 hw/misc/trace-events |  3 +++
 2 files changed, 42 insertions(+)

diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index 2cc52b0205..41f8ad213d 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -13,12 +13,14 @@
 #include "qemu/osdep.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "qemu/bitops.h"
 #include "hw/qdev-properties.h"
 #include "hw/misc/pca9552.h"
 #include "hw/misc/pca9552_regs.h"
 #include "migration/vmstate.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
+#include "trace.h"
 
 typedef struct PCA955xClass {
     /*< private >*/
@@ -49,6 +51,39 @@ static uint8_t pca955x_pin_get_config(PCA955xState *s, int pin)
     return extract32(s->regs[reg], shift, 2);
 }
 
+/* Return INPUT status (bit #N belongs to GPIO #N) */
+static uint16_t pca955x_pins_get_status(PCA955xState *s)
+{
+    return (s->regs[PCA9552_INPUT1] << 8) | s->regs[PCA9552_INPUT0];
+}
+
+static void pca955x_display_pins_status(PCA955xState *s,
+                                        uint16_t previous_pins_status)
+{
+    PCA955xClass *k = PCA955X_GET_CLASS(s);
+    uint16_t pins_status, pins_changed;
+    int i;
+
+    pins_status = pca955x_pins_get_status(s);
+    pins_changed = previous_pins_status ^ pins_status;
+    if (!pins_changed) {
+        return;
+    }
+    if (trace_event_get_state_backends(TRACE_PCA955X_GPIO_STATUS)) {
+        char *buf = g_newa(char, k->pin_count + 1);
+
+        for (i = 0; i < k->pin_count; i++) {
+            if (extract32(pins_status, i, 1)) {
+                buf[i] = '*';
+            } else {
+                buf[i] = '.';
+            }
+        }
+        buf[i] = '\0';
+        trace_pca955x_gpio_status(s->description, buf);
+    }
+}
+
 static void pca955x_update_pin_input(PCA955xState *s)
 {
     PCA955xClass *k = PCA955X_GET_CLASS(s);
@@ -98,6 +133,8 @@ static uint8_t pca955x_read(PCA955xState *s, uint8_t reg)
 
 static void pca955x_write(PCA955xState *s, uint8_t reg, uint8_t data)
 {
+    uint16_t pins_status;
+
     switch (reg) {
     case PCA9552_PSC0:
     case PCA9552_PWM0:
@@ -110,8 +147,10 @@ static void pca955x_write(PCA955xState *s, uint8_t reg, uint8_t data)
     case PCA9552_LS1:
     case PCA9552_LS2:
     case PCA9552_LS3:
+        pins_status = pca955x_pins_get_status(s);
         s->regs[reg] = data;
         pca955x_update_pin_input(s);
+        pca955x_display_pins_status(s, pins_status);
         break;
 
     case PCA9552_INPUT0:
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 5561746866..9282c60dd9 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -206,3 +206,6 @@ via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "secto
 # grlib_ahb_apb_pnp.c
 grlib_ahb_pnp_read(uint64_t addr, uint32_t value) "AHB PnP read addr:0x%03"PRIx64" data:0x%08x"
 grlib_apb_pnp_read(uint64_t addr, uint32_t value) "APB PnP read addr:0x%03"PRIx64" data:0x%08x"
+
+# pca9552.c
+pca955x_gpio_status(const char *description, const char *buf) "%s GPIOs 0-15 [%s]"
-- 
2.21.3

[PATCH v6 7/9] hw/arm/aspeed: Describe each PCA9552 device

[Philippe Mathieu-Daudé]

We have 2 distinct PCA9552 devices. Set their description
to distinguish them when looking at the trace events.

Description name taken from:
https://github.com/open-power/witherspoon-xml/blob/master/witherspoon.xml

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Corey Minyard <cminyard@mvista.com>
---
 hw/arm/aspeed.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index ccf127b328..307dba5065 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -508,12 +508,15 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
 {
     AspeedSoCState *soc = &bmc->soc;
     uint8_t *eeprom_buf = g_malloc0(8 * 1024);
+    DeviceState *dev;
 
     /* Bus 3: TODO bmp280@77 */
     /* Bus 3: TODO max31785@52 */
     /* Bus 3: TODO dps310@76 */
-    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3), TYPE_PCA9552,
-                     0x60);
+    dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
+    qdev_prop_set_string(dev, "description", "pca1");
+    i2c_realize_and_unref(dev, aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3),
+                          &error_fatal);
 
     i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 4), "tmp423", 0x4c);
     i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 5), "tmp423", 0x4c);
@@ -528,8 +531,10 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
 
     smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), 0x51,
                           eeprom_buf);
-    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), TYPE_PCA9552,
-                     0x60);
+    dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
+    qdev_prop_set_string(dev, "description", "pca0");
+    i2c_realize_and_unref(dev, aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11),
+                          &error_fatal);
     /* Bus 11: TODO ucd90160@64 */
 }
 
-- 
2.21.3

[PATCH v6 8/9] hw/misc/pca9552: Trace GPIO change events

[Philippe Mathieu-Daudé]

Emit a trace event when a GPIO change its state.

Example booting obmc-phosphor-image:

  $ qemu-system-arm -M witherspoon-bmc -trace pca955x_gpio_change
  1592690552.687372:pca955x_gpio_change pca1 GPIO id:0 status: 0 -> 1
  1592690552.690169:pca955x_gpio_change pca1 GPIO id:1 status: 0 -> 1
  1592690552.691673:pca955x_gpio_change pca1 GPIO id:2 status: 0 -> 1
  1592690552.696886:pca955x_gpio_change pca1 GPIO id:3 status: 0 -> 1
  1592690552.698614:pca955x_gpio_change pca1 GPIO id:13 status: 0 -> 1
  1592690552.699833:pca955x_gpio_change pca1 GPIO id:14 status: 0 -> 1
  1592690552.700842:pca955x_gpio_change pca1 GPIO id:15 status: 0 -> 1
  1592690683.841921:pca955x_gpio_change pca1 GPIO id:14 status: 1 -> 0
  1592690683.861660:pca955x_gpio_change pca1 GPIO id:14 status: 0 -> 1
  1592690684.371460:pca955x_gpio_change pca1 GPIO id:14 status: 1 -> 0
  1592690684.882115:pca955x_gpio_change pca1 GPIO id:14 status: 0 -> 1
  1592690685.391411:pca955x_gpio_change pca1 GPIO id:14 status: 1 -> 0
  1592690685.901391:pca955x_gpio_change pca1 GPIO id:14 status: 0 -> 1
  1592690686.411678:pca955x_gpio_change pca1 GPIO id:14 status: 1 -> 0
  1592690686.921279:pca955x_gpio_change pca1 GPIO id:14 status: 0 -> 1

We notice the GPIO #14 (front-power LED) starts to blink.

This LED is described in the witherspoon device-tree [*]:

  front-power {
      retain-state-shutdown;
      default-state = "keep";
      gpios = <&pca0 14 GPIO_ACTIVE_LOW>;
  };

[*] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts?id=b1f9be9392f0#n140

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/misc/pca9552.c    | 15 +++++++++++++++
 hw/misc/trace-events |  1 +
 2 files changed, 16 insertions(+)

diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index 41f8ad213d..1c3ad57432 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -82,6 +82,21 @@ static void pca955x_display_pins_status(PCA955xState *s,
         buf[i] = '\0';
         trace_pca955x_gpio_status(s->description, buf);
     }
+    if (trace_event_get_state_backends(TRACE_PCA955X_GPIO_CHANGE)) {
+        for (i = 0; i < k->pin_count; i++) {
+            if (extract32(pins_changed, i, 1)) {
+                unsigned new_state = extract32(pins_status, i, 1);
+
+                /*
+                 * We display the state using the PCA logic ("active-high").
+                 * This is not the state of the LED, which signal might be
+                 * wired "active-low" on the board.
+                 */
+                trace_pca955x_gpio_change(s->description, i,
+                                          !new_state, new_state);
+            }
+        }
+    }
 }
 
 static void pca955x_update_pin_input(PCA955xState *s)
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 9282c60dd9..7ccf683dd1 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -209,3 +209,4 @@ grlib_apb_pnp_read(uint64_t addr, uint32_t value) "APB PnP read addr:0x%03"PRIx6
 
 # pca9552.c
 pca955x_gpio_status(const char *description, const char *buf) "%s GPIOs 0-15 [%s]"
+pca955x_gpio_change(const char *description, unsigned id, unsigned prev_state, unsigned current_state) "%s GPIO id:%u status: %u -> %u"
-- 
2.21.3

[PATCH v6 9/9] hw/misc/pca9552: Model qdev output GPIOs

[Philippe Mathieu-Daudé]

The PCA9552 has 16 GPIOs which can be used as input,
output or PWM mode. QEMU models the output GPIO with
the qemu_irq type. Let the device expose the 16 GPIOs
to allow us to later connect LEDs to these outputs.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/pca9552.h | 1 +
 hw/misc/pca9552.c         | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
index bf1a589137..600356fbf9 100644
--- a/include/hw/misc/pca9552.h
+++ b/include/hw/misc/pca9552.h
@@ -27,6 +27,7 @@ typedef struct PCA955xState {
     uint8_t pointer;
 
     uint8_t regs[PCA955X_NR_REGS];
+    qemu_irq gpio[PCA955X_PIN_COUNT_MAX];
     char *description; /* For debugging purpose only */
 } PCA955xState;
 
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index 1c3ad57432..80caa9ec8f 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -17,6 +17,7 @@
 #include "hw/qdev-properties.h"
 #include "hw/misc/pca9552.h"
 #include "hw/misc/pca9552_regs.h"
+#include "hw/irq.h"
 #include "migration/vmstate.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
@@ -111,9 +112,11 @@ static void pca955x_update_pin_input(PCA955xState *s)
 
         switch (config) {
         case PCA9552_LED_ON:
+            qemu_set_irq(s->gpio[i], 1);
             s->regs[input_reg] |= 1 << input_shift;
             break;
         case PCA9552_LED_OFF:
+            qemu_set_irq(s->gpio[i], 0);
             s->regs[input_reg] &= ~(1 << input_shift);
             break;
         case PCA9552_LED_PWM0:
@@ -374,11 +377,14 @@ static void pca955x_initfn(Object *obj)
 
 static void pca955x_realize(DeviceState *dev, Error **errp)
 {
+    PCA955xClass *k = PCA955X_GET_CLASS(dev);
     PCA955xState *s = PCA955X(dev);
 
     if (!s->description) {
         s->description = g_strdup("pca-unspecified");
     }
+
+    qdev_init_gpio_out(dev, s->gpio, k->pin_count);
 }
 
 static Property pca955x_properties[] = {
-- 
2.21.3

Re: [PATCH v6 0/9] hw/misc/pca9552: Trace GPIO change events

[Cédric Le Goater]

On 6/23/20 9:27 AM, Philippe Mathieu-Daudé wrote:
> This series add trace events to better display GPIO changes.
> We'll continue in the following series by connecting LEDs to
> these GPIOs.
> 
> This helps me to work on a generic LED device, see:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg711917.html


Tested-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C. 

> 
> Since v5, addressed Cédric review comment:
> - Move pin_count check from realize() to instance_init()
> 
> Since v4: Addressed Cédric review comments
> - Extract PCA955xClass
> - Add/use pca955x_pins_get_status() method instead of keeping
>   cached value in PCA955xState
> 
> Example when booting an obmc-phosphor-image, we can see the LED #14
> (front-power LED) starting to blink.
> 
> - ASCII LED bar view:
> 
>   $ qemu-system-arm -M witherspoon-bmc -trace pca955x_gpio_status
>   1592689902.327837:pca955x_gpio_status pca-unspecified GPIOs 0-15 [*...............]
>   1592689902.329934:pca955x_gpio_status pca-unspecified GPIOs 0-15 [**..............]
>   1592689902.330717:pca955x_gpio_status pca-unspecified GPIOs 0-15 [***.............]
>   1592689902.331431:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****............]
>   1592689902.332163:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........*..]
>   1592689902.332888:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........**.]
>   1592689902.333629:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
>   1592690032.793289:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]
>   1592690033.303163:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
>   1592690033.812962:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]
>   1592690034.323234:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
>   1592690034.832922:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]
> 
> - Only display GPIOs which status changes:
> 
>   $ qemu-system-arm -M witherspoon-bmc -trace pca955x_gpio_change
>   1592690552.687372:pca955x_gpio_change pca1 GPIO id:0 status: 0 -> 1
>   1592690552.690169:pca955x_gpio_change pca1 GPIO id:1 status: 0 -> 1
>   1592690552.691673:pca955x_gpio_change pca1 GPIO id:2 status: 0 -> 1
>   1592690552.696886:pca955x_gpio_change pca1 GPIO id:3 status: 0 -> 1
>   1592690552.698614:pca955x_gpio_change pca1 GPIO id:13 status: 0 -> 1
>   1592690552.699833:pca955x_gpio_change pca1 GPIO id:14 status: 0 -> 1
>   1592690552.700842:pca955x_gpio_change pca1 GPIO id:15 status: 0 -> 1
>   1592690683.841921:pca955x_gpio_change pca1 GPIO id:14 status: 1 -> 0
>   1592690683.861660:pca955x_gpio_change pca1 GPIO id:14 status: 0 -> 1
>   1592690684.371460:pca955x_gpio_change pca1 GPIO id:14 status: 1 -> 0
>   1592690684.882115:pca955x_gpio_change pca1 GPIO id:14 status: 0 -> 1
>   1592690685.391411:pca955x_gpio_change pca1 GPIO id:14 status: 1 -> 0
>   1592690685.901391:pca955x_gpio_change pca1 GPIO id:14 status: 0 -> 1
> 
> For information about how to test the obmc-phosphor-image, see:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg712911.html
> 
> $ git backport-diff -u v5
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> 
> 001/9:[----] [--] 'hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref()'
> 002/9:[----] [--] 'hw/misc/pca9552: Rename 'nr_leds' as 'pin_count''
> 003/9:[----] [--] 'hw/misc/pca9552: Rename generic code as pca955x'
> 004/9:[0010] [FC] 'hw/misc/pca9552: Add generic PCA955xClass, parent of TYPE_PCA9552'
> 005/9:[0007] [FC] 'hw/misc/pca9552: Add a 'description' property for debugging purpose'
> 006/9:[----] [--] 'hw/misc/pca9552: Trace GPIO High/Low events'
> 007/9:[----] [-C] 'hw/arm/aspeed: Describe each PCA9552 device'
> 008/9:[----] [--] 'hw/misc/pca9552: Trace GPIO change events'
> 009/9:[0003] [FC] 'hw/misc/pca9552: Model qdev output GPIOs'
> 
> Based-on: <20200623072132.2868-1-f4bug@amsat.org>
> 
> Philippe Mathieu-Daudé (9):
>   hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref()
>   hw/misc/pca9552: Rename 'nr_leds' as 'pin_count'
>   hw/misc/pca9552: Rename generic code as pca955x
>   hw/misc/pca9552: Add generic PCA955xClass, parent of TYPE_PCA9552
>   hw/misc/pca9552: Add a 'description' property for debugging purpose
>   hw/misc/pca9552: Trace GPIO High/Low events
>   hw/arm/aspeed: Describe each PCA9552 device
>   hw/misc/pca9552: Trace GPIO change events
>   hw/misc/pca9552: Model qdev output GPIOs
> 
>  include/hw/i2c/i2c.h      |   2 +
>  include/hw/misc/pca9552.h |  16 +--
>  hw/arm/aspeed.c           |  13 ++-
>  hw/i2c/core.c             |  18 +++-
>  hw/misc/pca9552.c         | 216 ++++++++++++++++++++++++++++----------
>  hw/misc/trace-events      |   4 +
>  6 files changed, 202 insertions(+), 67 deletions(-)
> 

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

[Philippe Mathieu-Daudé]

On 6/23/20 9:27 AM, Philippe Mathieu-Daudé 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>

Collecting Markus review on v5:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg07060.html

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Cc: Markus Armbruster <armbru@redhat.com>
> ---
>  include/hw/i2c/i2c.h |  2 ++
>  hw/i2c/core.c        | 18 ++++++++++++++++--
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> 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);
>  
>  /* lm832x.c */
>  void lm832x_key_event(DeviceState *dev, int key, int state);
> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index 1aac457a2a..acf34a12d6 100644
> --- 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);
> +
>      return dev;
>  }
>  
> 

Re: [PATCH v6 7/9] hw/arm/aspeed: Describe each PCA9552 device

[Philippe Mathieu-Daudé]

On 6/23/20 9:27 AM, Philippe Mathieu-Daudé wrote:
> We have 2 distinct PCA9552 devices. Set their description
> to distinguish them when looking at the trace events.
> 
> Description name taken from:
> https://github.com/open-power/witherspoon-xml/blob/master/witherspoon.xml
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Collecting Markus review on v5:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg07078.html

> ---
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Corey Minyard <cminyard@mvista.com>
> ---
>  hw/arm/aspeed.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index ccf127b328..307dba5065 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -508,12 +508,15 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
>  {
>      AspeedSoCState *soc = &bmc->soc;
>      uint8_t *eeprom_buf = g_malloc0(8 * 1024);
> +    DeviceState *dev;
>  
>      /* Bus 3: TODO bmp280@77 */
>      /* Bus 3: TODO max31785@52 */
>      /* Bus 3: TODO dps310@76 */
> -    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3), TYPE_PCA9552,
> -                     0x60);
> +    dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
> +    qdev_prop_set_string(dev, "description", "pca1");
> +    i2c_realize_and_unref(dev, aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3),
> +                          &error_fatal);
>  
>      i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 4), "tmp423", 0x4c);
>      i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 5), "tmp423", 0x4c);
> @@ -528,8 +531,10 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
>  
>      smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), 0x51,
>                            eeprom_buf);
> -    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), TYPE_PCA9552,
> -                     0x60);
> +    dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
> +    qdev_prop_set_string(dev, "description", "pca0");
> +    i2c_realize_and_unref(dev, aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11),
> +                          &error_fatal);
>      /* Bus 11: TODO ucd90160@64 */
>  }
>  
> 

Re: [PATCH v6 0/9] hw/misc/pca9552: Trace GPIO change events

[Philippe Mathieu-Daudé]

On 6/23/20 10:14 AM, Cédric Le Goater wrote:
> On 6/23/20 9:27 AM, Philippe Mathieu-Daudé wrote:
>> This series add trace events to better display GPIO changes.
>> We'll continue in the following series by connecting LEDs to
>> these GPIOs.
>>
>> This helps me to work on a generic LED device, see:
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg711917.html
> 
> 
> Tested-by: Cédric Le Goater <clg@kaod.org>

Thanks :)

Note to the maintainer, this series is now fully reviewed/tested.

>>
>> Based-on: <20200623072132.2868-1-f4bug@amsat.org>
>>
>> Philippe Mathieu-Daudé (9):
>>   hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref()
>>   hw/misc/pca9552: Rename 'nr_leds' as 'pin_count'
>>   hw/misc/pca9552: Rename generic code as pca955x
>>   hw/misc/pca9552: Add generic PCA955xClass, parent of TYPE_PCA9552
>>   hw/misc/pca9552: Add a 'description' property for debugging purpose
>>   hw/misc/pca9552: Trace GPIO High/Low events
>>   hw/arm/aspeed: Describe each PCA9552 device
>>   hw/misc/pca9552: Trace GPIO change events
>>   hw/misc/pca9552: Model qdev output GPIOs
>>
>>  include/hw/i2c/i2c.h      |   2 +
>>  include/hw/misc/pca9552.h |  16 +--
>>  hw/arm/aspeed.c           |  13 ++-
>>  hw/i2c/core.c             |  18 +++-
>>  hw/misc/pca9552.c         | 216 ++++++++++++++++++++++++++++----------
>>  hw/misc/trace-events      |   4 +
>>  6 files changed, 202 insertions(+), 67 deletions(-)

Re: [PATCH v6 7/9] hw/arm/aspeed: Describe each PCA9552 device

[Corey Minyard]

On Tue, Jun 23, 2020 at 09:27:21AM +0200, Philippe Mathieu-Daudé wrote:
> We have 2 distinct PCA9552 devices. Set their description
> to distinguish them when looking at the trace events.
> 
> Description name taken from:
> https://github.com/open-power/witherspoon-xml/blob/master/witherspoon.xml

I forgot to respond to this earlier.

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>
> ---
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Corey Minyard <cminyard@mvista.com>
> ---
>  hw/arm/aspeed.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index ccf127b328..307dba5065 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -508,12 +508,15 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
>  {
>      AspeedSoCState *soc = &bmc->soc;
>      uint8_t *eeprom_buf = g_malloc0(8 * 1024);
> +    DeviceState *dev;
>  
>      /* Bus 3: TODO bmp280@77 */
>      /* Bus 3: TODO max31785@52 */
>      /* Bus 3: TODO dps310@76 */
> -    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3), TYPE_PCA9552,
> -                     0x60);
> +    dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
> +    qdev_prop_set_string(dev, "description", "pca1");
> +    i2c_realize_and_unref(dev, aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3),
> +                          &error_fatal);
>  
>      i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 4), "tmp423", 0x4c);
>      i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 5), "tmp423", 0x4c);
> @@ -528,8 +531,10 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
>  
>      smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), 0x51,
>                            eeprom_buf);
> -    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), TYPE_PCA9552,
> -                     0x60);
> +    dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
> +    qdev_prop_set_string(dev, "description", "pca0");
> +    i2c_realize_and_unref(dev, aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11),
> +                          &error_fatal);
>      /* Bus 11: TODO ucd90160@64 */
>  }
>  
> -- 
> 2.21.3
> 

Re: [PATCH v6 0/9] hw/misc/pca9552: Trace GPIO change events

[Peter Maydell]

On Tue, 23 Jun 2020 at 12:09, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 6/23/20 10:14 AM, Cédric Le Goater wrote:
> > On 6/23/20 9:27 AM, Philippe Mathieu-Daudé wrote:
> >> This series add trace events to better display GPIO changes.
> >> We'll continue in the following series by connecting LEDs to
> >> these GPIOs.
> >>
> >> This helps me to work on a generic LED device, see:
> >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg711917.html
> >
> >
> > Tested-by: Cédric Le Goater <clg@kaod.org>
>
> Thanks :)
>
> Note to the maintainer, this series is now fully reviewed/tested.



Applied to target-arm.next, thanks.

-- PMM

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

[Peter Maydell]

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

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