* [PATCH 0/8] hw/gpio/pca955x: Improvements around props "pin" and "led"
@ 2026-11-06 11:04 Markus Armbruster
2026-11-06 11:04 ` [PATCH 1/8] hw/gpio/pca9552: Convert pin and led properties to class properties Markus Armbruster
` (7 more replies)
0 siblings, 8 replies; 35+ messages in thread
From: Markus Armbruster @ 2026-11-06 11:04 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, milesg, qemu-ppc, qemu-arm, mark.caveayland
Marc-André converted properties "pin" and "led" to QAPI enums in his
"qom/qdev: associate properties with QAPI schema types". I dropped
this part from v8, which has since been merged in commit c5fee4d93fd.
I'm now bringing it back, reworked.
PATCH 1-5 convert these properties to class properties. See commit
7674a1880890 (qom: deprecate use of instance level properties) for
why.
PATCH 6 is Marc-André's "[PATCH v7 48/65] hw/gpio/pca955x: use QAPI
enums for led and pin properties" with the following tweaks:
* I don't change their @type in output of qom-list,
qom-list-properties, qom-list-get, because that what the merged
version of his series did.
* I replaced identical QAPI enums Pca9552PinState and Pca9554PinState
by a single enum GpioPinState.
PATCH 7+8 replace Marc-André's "[PATCH v7 50/65] hw/gpio/pca955x:
convert pin/led property to QAPI-aware enum". Marc-André's used
object_property_add_qapi_enum() (which has since been folded into
object_property_add_enum()), but to be able to use it, he had to bake
the index into getter and the setter functions. He generated them
with the preprocessor. My version sticks to
object_class_property_add(), so we can pass the index to getter and
setter via parameter @opaque. Getter and setter remain slightly more
complex, but don't turn into macros (decorated with backslashes) for
sixteenfold duplication.
Perhaps object_class_property_add_enum() should support passing an
opaque value to getter and setter.
Based-on: <20261105080310.1656968-1-armbru@redhat.com>
Marc-André Lureau (1):
hw/gpio/pca955x: use QAPI enums for led and pin properties
Markus Armbruster (7):
hw/gpio/pca9552: Convert pin and led properties to class properties
hw/gpio/pca9554: Move most code from .h to .c
hw/gpio/pca9554: Simplify QOM type creation
hw/gpio/pca9554: New abstract base "pca9554-base"
hw/gpio/pca9554: Convert pin properties to class properties
hw/gpio/pca955x: Improve @qapi-type reported by qom-list etc.
hw/gpio/pca955x: Simplify pin and led getters and setters
qapi/machine.json | 29 ++++++++
include/hw/gpio/pca9554.h | 26 +------
hw/gpio/pca9552.c | 149 +++++++++++++------------------------
hw/gpio/pca9554.c | 151 +++++++++++++++++++-------------------
4 files changed, 160 insertions(+), 195 deletions(-)
--
2.55.0
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 1/8] hw/gpio/pca9552: Convert pin and led properties to class properties
2026-11-06 11:04 [PATCH 0/8] hw/gpio/pca955x: Improvements around props "pin" and "led" Markus Armbruster
@ 2026-11-06 11:04 ` Markus Armbruster
2026-11-06 20:17 ` Miles Glenn
2026-11-09 8:51 ` Mark Cave-Ayland
2026-11-06 11:04 ` [PATCH 2/8] hw/gpio/pca9554: Move most code from .h to .c Markus Armbruster
` (6 subsequent siblings)
7 siblings, 2 replies; 35+ messages in thread
From: Markus Armbruster @ 2026-11-06 11:04 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, milesg, qemu-ppc, qemu-arm, mark.caveayland
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/gpio/pca9552.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/hw/gpio/pca9552.c b/hw/gpio/pca9552.c
index 90ccf5170e..65a718ef31 100644
--- a/hw/gpio/pca9552.c
+++ b/hw/gpio/pca9552.c
@@ -557,10 +557,8 @@ static void pca9535_reset_hold(Object *obj, ResetType type)
s->len = 0;
}
-static void pca955x_initfn(Object *obj)
+static void pca955x_pin_init(PCA955xClass *k)
{
- PCA955xClass *k = PCA955X_GET_CLASS(obj);
-
assert(k->pin_count <= PCA955X_PIN_COUNT_MAX);
for (int ix = 0; ix < k->pin_count; ix++) {
char *name;
@@ -568,13 +566,17 @@ static void pca955x_initfn(Object *obj)
if (k->has_led_support) {
/* LED variant: expose the LED selector state as led%d. */
name = g_strdup_printf("led%d", ix);
- object_property_add(obj, name, "str", &str_type_info,
- pca955x_get_led, pca955x_set_led, NULL, NULL);
+ object_class_property_add(OBJECT_CLASS(k), name,
+ "str", &str_type_info,
+ pca955x_get_led, pca955x_set_led,
+ NULL, NULL);
} else {
/* GPIO variant: expose the pin logic level as pin%d. */
name = g_strdup_printf("pin%d", ix);
- object_property_add(obj, name, "str", &str_type_info,
- pca955x_get_pin, pca955x_set_pin, NULL, NULL);
+ object_class_property_add(OBJECT_CLASS(k), name,
+ "str", &str_type_info,
+ pca955x_get_pin, pca955x_set_pin,
+ NULL, NULL);
}
g_free(name);
}
@@ -640,6 +642,7 @@ static void pca9552_class_init(ObjectClass *oc, const void *data)
pc->max_reg = PCA9552_LS3;
pc->pin_count = 16;
pc->has_led_support = true;
+ pca955x_pin_init(pc);
}
static void pca95x5_class_init(ObjectClass *oc, const void *data)
@@ -653,13 +656,13 @@ static void pca95x5_class_init(ObjectClass *oc, const void *data)
pc->max_reg = PCA9535_CONFIG1;
pc->pin_count = 16;
pc->has_led_support = false;
+ pca955x_pin_init(pc);
}
static const TypeInfo pca955x_types[] = {
{
.name = TYPE_PCA955X,
.parent = TYPE_I2C_SLAVE,
- .instance_init = pca955x_initfn,
.instance_size = sizeof(PCA955xState),
.class_init = pca955x_class_init,
.class_size = sizeof(PCA955xClass),
--
2.55.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 2/8] hw/gpio/pca9554: Move most code from .h to .c
2026-11-06 11:04 [PATCH 0/8] hw/gpio/pca955x: Improvements around props "pin" and "led" Markus Armbruster
2026-11-06 11:04 ` [PATCH 1/8] hw/gpio/pca9552: Convert pin and led properties to class properties Markus Armbruster
@ 2026-11-06 11:04 ` Markus Armbruster
2026-11-06 20:18 ` Miles Glenn
2026-11-09 8:55 ` Mark Cave-Ayland
2026-11-06 11:04 ` [PATCH 3/8] hw/gpio/pca9554: Simplify QOM type creation Markus Armbruster
` (5 subsequent siblings)
7 siblings, 2 replies; 35+ messages in thread
From: Markus Armbruster @ 2026-11-06 11:04 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, milesg, qemu-ppc, qemu-arm, mark.caveayland
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
include/hw/gpio/pca9554.h | 25 -------------------------
hw/gpio/pca9554.c | 24 ++++++++++++++++++++++++
2 files changed, 24 insertions(+), 25 deletions(-)
diff --git a/include/hw/gpio/pca9554.h b/include/hw/gpio/pca9554.h
index ac835371aa..1b33a8a752 100644
--- a/include/hw/gpio/pca9554.h
+++ b/include/hw/gpio/pca9554.h
@@ -8,32 +8,7 @@
#ifndef PCA9554_H
#define PCA9554_H
-#include "hw/i2c/i2c.h"
-#include "qom/object.h"
-
#define TYPE_PCA9554 "pca9554"
#define TYPE_PCA9536 "pca9536"
-typedef struct PCA9554State PCA9554State;
-DECLARE_INSTANCE_CHECKER(PCA9554State, PCA9554,
- TYPE_PCA9554)
-
-#define PCA9554_NR_REGS 4
-#define PCA9554_PIN_COUNT 8
-#define PCA9536_PIN_COUNT 4
-
-struct PCA9554State {
- /*< private >*/
- I2CSlave i2c;
- /*< public >*/
-
- uint8_t len;
- uint8_t pointer;
-
- uint8_t regs[PCA9554_NR_REGS];
- qemu_irq gpio_out[PCA9554_PIN_COUNT];
- uint8_t ext_state[PCA9554_PIN_COUNT];
- char *description; /* For debugging purpose only */
- bool hw_dir; /* Honor pin direction */
-};
#endif
diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
index 07bb00babf..e5a1932c89 100644
--- a/hw/gpio/pca9554.c
+++ b/hw/gpio/pca9554.c
@@ -13,6 +13,7 @@
#include "hw/core/qdev-properties.h"
#include "hw/gpio/pca9554.h"
#include "hw/gpio/pca9554_regs.h"
+#include "hw/i2c/i2c.h"
#include "hw/core/irq.h"
#include "migration/vmstate.h"
#include "qapi/error.h"
@@ -21,6 +22,29 @@
#include "trace.h"
#include "qom/object.h"
+typedef struct PCA9554State PCA9554State;
+DECLARE_INSTANCE_CHECKER(PCA9554State, PCA9554,
+ TYPE_PCA9554)
+
+#define PCA9554_NR_REGS 4
+#define PCA9554_PIN_COUNT 8
+#define PCA9536_PIN_COUNT 4
+
+struct PCA9554State {
+ /*< private >*/
+ I2CSlave i2c;
+ /*< public >*/
+
+ uint8_t len;
+ uint8_t pointer;
+
+ uint8_t regs[PCA9554_NR_REGS];
+ qemu_irq gpio_out[PCA9554_PIN_COUNT];
+ uint8_t ext_state[PCA9554_PIN_COUNT];
+ char *description; /* For debugging purpose only */
+ bool hw_dir; /* Honor pin direction */
+};
+
struct PCA9554Class {
/*< private >*/
I2CSlaveClass parent_class;
--
2.55.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 3/8] hw/gpio/pca9554: Simplify QOM type creation
2026-11-06 11:04 [PATCH 0/8] hw/gpio/pca955x: Improvements around props "pin" and "led" Markus Armbruster
2026-11-06 11:04 ` [PATCH 1/8] hw/gpio/pca9552: Convert pin and led properties to class properties Markus Armbruster
2026-11-06 11:04 ` [PATCH 2/8] hw/gpio/pca9554: Move most code from .h to .c Markus Armbruster
@ 2026-11-06 11:04 ` Markus Armbruster
2026-11-06 20:19 ` Miles Glenn
2026-11-09 8:56 ` Mark Cave-Ayland
2026-11-06 11:04 ` [PATCH 4/8] hw/gpio/pca9554: New abstract base "pca9554-base" Markus Armbruster
` (4 subsequent siblings)
7 siblings, 2 replies; 35+ messages in thread
From: Markus Armbruster @ 2026-11-06 11:04 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, milesg, qemu-ppc, qemu-arm, mark.caveayland
According to object.h, direct usage of DECLARE_INSTANCE_CHECKERS()
should be avoided, and the complete OBJECT_DECLARE_TYPE macro is
recommended instead. So use that.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/gpio/pca9554.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
index e5a1932c89..caa73f63c9 100644
--- a/hw/gpio/pca9554.c
+++ b/hw/gpio/pca9554.c
@@ -22,14 +22,12 @@
#include "trace.h"
#include "qom/object.h"
-typedef struct PCA9554State PCA9554State;
-DECLARE_INSTANCE_CHECKER(PCA9554State, PCA9554,
- TYPE_PCA9554)
-
#define PCA9554_NR_REGS 4
#define PCA9554_PIN_COUNT 8
#define PCA9536_PIN_COUNT 4
+OBJECT_DECLARE_TYPE(PCA9554State, PCA9554Class, PCA9554)
+
struct PCA9554State {
/*< private >*/
I2CSlave i2c;
@@ -52,10 +50,6 @@ struct PCA9554Class {
uint8_t pin_count;
};
-typedef struct PCA9554Class PCA9554Class;
-
-DECLARE_CLASS_CHECKERS(PCA9554Class, PCA9554,
- TYPE_PCA9554)
#define PCA9554_PIN_LOW 0x0
#define PCA9554_PIN_HIZ 0x1
--
2.55.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 4/8] hw/gpio/pca9554: New abstract base "pca9554-base"
2026-11-06 11:04 [PATCH 0/8] hw/gpio/pca955x: Improvements around props "pin" and "led" Markus Armbruster
` (2 preceding siblings ...)
2026-11-06 11:04 ` [PATCH 3/8] hw/gpio/pca9554: Simplify QOM type creation Markus Armbruster
@ 2026-11-06 11:04 ` Markus Armbruster
2026-11-06 20:19 ` Miles Glenn
2026-11-09 9:02 ` Mark Cave-Ayland
2026-11-06 11:04 ` [PATCH 5/8] hw/gpio/pca9554: Convert pin properties to class properties Markus Armbruster
` (3 subsequent siblings)
7 siblings, 2 replies; 35+ messages in thread
From: Markus Armbruster @ 2026-11-06 11:04 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, milesg, qemu-ppc, qemu-arm, mark.caveayland
Commit 5eceb48bbe86 (hw/gpio: pca9554: add PCA9536 support) made type
"pca9535" a child of concrete type "pca9555". This is inconvenient
for the next commit. Make them both children of new abstract type
"pca9554-base".
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
include/hw/gpio/pca9554.h | 1 +
hw/gpio/pca9554.c | 55 +++++++++++++++++++++++----------------
2 files changed, 33 insertions(+), 23 deletions(-)
diff --git a/include/hw/gpio/pca9554.h b/include/hw/gpio/pca9554.h
index 1b33a8a752..0d841ff017 100644
--- a/include/hw/gpio/pca9554.h
+++ b/include/hw/gpio/pca9554.h
@@ -8,6 +8,7 @@
#ifndef PCA9554_H
#define PCA9554_H
+#define TYPE_PCA9554_BASE "pca9554-base"
#define TYPE_PCA9554 "pca9554"
#define TYPE_PCA9536 "pca9536"
diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
index caa73f63c9..1362b605c2 100644
--- a/hw/gpio/pca9554.c
+++ b/hw/gpio/pca9554.c
@@ -26,7 +26,7 @@
#define PCA9554_PIN_COUNT 8
#define PCA9536_PIN_COUNT 4
-OBJECT_DECLARE_TYPE(PCA9554State, PCA9554Class, PCA9554)
+OBJECT_DECLARE_TYPE(PCA9554State, PCA9554Class, PCA9554_BASE)
struct PCA9554State {
/*< private >*/
@@ -58,7 +58,7 @@ static const char *pin_state[] = {"low", "high"};
static void pca9554_update_pin_input(PCA9554State *s)
{
- PCA9554Class *pc = PCA9554_GET_CLASS(s);
+ PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
int i;
uint8_t config = s->regs[PCA9554_CONFIG];
uint8_t output = s->regs[PCA9554_OUTPUT];
@@ -110,7 +110,7 @@ static uint8_t pca9554_read(PCA9554State *s, uint8_t reg)
static void pca9554_write(PCA9554State *s, uint8_t reg, uint8_t data)
{
- PCA9554Class *pc = PCA9554_GET_CLASS(s);
+ PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
uint8_t pin_mask = (1 << pc->pin_count) - 1;
/* Variants narrower than 8 bits ignore the unimplemented upper pins. */
@@ -134,14 +134,14 @@ static void pca9554_write(PCA9554State *s, uint8_t reg, uint8_t data)
static uint8_t pca9554_recv(I2CSlave *i2c)
{
- PCA9554State *s = PCA9554(i2c);
+ PCA9554State *s = PCA9554_BASE(i2c);
return pca9554_read(s, s->pointer & 0x3);
}
static int pca9554_send(I2CSlave *i2c, uint8_t data)
{
- PCA9554State *s = PCA9554(i2c);
+ PCA9554State *s = PCA9554_BASE(i2c);
/* First byte sent by is the register address */
if (s->len == 0) {
@@ -156,7 +156,7 @@ static int pca9554_send(I2CSlave *i2c, uint8_t data)
static int pca9554_event(I2CSlave *i2c, enum i2c_event event)
{
- PCA9554State *s = PCA9554(i2c);
+ PCA9554State *s = PCA9554_BASE(i2c);
s->len = 0;
return 0;
@@ -173,7 +173,7 @@ static void pca9554_set_ext_state(PCA9554State *s, int pin, int level)
static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
- PCA9554State *s = PCA9554(obj);
+ PCA9554State *s = PCA9554_BASE(obj);
int pin, rc;
uint8_t state;
@@ -182,7 +182,7 @@ static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
error_setg(errp, "%s: error reading %s", __func__, name);
return;
}
- if (pin < 0 || pin >= PCA9554_GET_CLASS(s)->pin_count) {
+ if (pin < 0 || pin >= PCA9554_BASE_GET_CLASS(s)->pin_count) {
error_setg(errp, "%s invalid pin %s", __func__, name);
return;
}
@@ -200,7 +200,7 @@ static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
- PCA9554State *s = PCA9554(obj);
+ PCA9554State *s = PCA9554_BASE(obj);
int pin, rc, val;
uint8_t state, mask;
g_autofree char *state_str = NULL;
@@ -213,7 +213,7 @@ static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
error_setg(errp, "%s: error reading %s", __func__, name);
return;
}
- if (pin < 0 || pin >= PCA9554_GET_CLASS(s)->pin_count) {
+ if (pin < 0 || pin >= PCA9554_BASE_GET_CLASS(s)->pin_count) {
error_setg(errp, "%s invalid pin %s", __func__, name);
return;
}
@@ -274,8 +274,8 @@ static const VMStateDescription pca9554_vmstate = {
static void pca9554_reset(DeviceState *dev)
{
- PCA9554State *s = PCA9554(dev);
- PCA9554Class *pc = PCA9554_GET_CLASS(s);
+ PCA9554State *s = PCA9554_BASE(dev);
+ PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
uint8_t pin_mask = (1 << pc->pin_count) - 1;
s->regs[PCA9554_INPUT] = pin_mask;
@@ -292,7 +292,7 @@ static void pca9554_reset(DeviceState *dev)
static void pca9554_initfn(Object *obj)
{
- PCA9554Class *pc = PCA9554_GET_CLASS(obj);
+ PCA9554Class *pc = PCA9554_BASE_GET_CLASS(obj);
int pin;
for (pin = 0; pin < pc->pin_count; pin++) {
@@ -308,8 +308,8 @@ static void pca9554_initfn(Object *obj)
static void pca9554_gpio_in_handler(void *opaque, int pin, int level)
{
- PCA9554State *s = PCA9554(opaque);
- PCA9554Class *pc = PCA9554_GET_CLASS(s);
+ PCA9554State *s = PCA9554_BASE(opaque);
+ PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
assert((pin >= 0) && (pin < pc->pin_count));
pca9554_set_ext_state(s, pin, level);
@@ -317,8 +317,8 @@ static void pca9554_gpio_in_handler(void *opaque, int pin, int level)
static void pca9554_realize(DeviceState *dev, Error **errp)
{
- PCA9554State *s = PCA9554(dev);
- PCA9554Class *pc = PCA9554_GET_CLASS(s);
+ PCA9554State *s = PCA9554_BASE(dev);
+ PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
if (!s->description) {
s->description = g_strdup(object_get_typename(OBJECT(dev)));
@@ -333,11 +333,10 @@ static const Property pca9554_properties[] = {
DEFINE_PROP_BOOL("hw-dir", PCA9554State, hw_dir, false),
};
-static void pca9554_class_init(ObjectClass *klass, const void *data)
+static void pca9554_base_class_init(ObjectClass *klass, const void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
- PCA9554Class *pc = PCA9554_CLASS(klass);
k->event = pca9554_event;
k->recv = pca9554_recv;
@@ -346,26 +345,36 @@ static void pca9554_class_init(ObjectClass *klass, const void *data)
device_class_set_legacy_reset(dc, pca9554_reset);
dc->vmsd = &pca9554_vmstate;
device_class_set_props(dc, pca9554_properties);
+}
+
+static void pca9554_class_init(ObjectClass *klass, const void *data)
+{
+ PCA9554Class *pc = PCA9554_BASE_CLASS(klass);
pc->pin_count = PCA9554_PIN_COUNT;
}
static void pca9536_class_init(ObjectClass *klass, const void *data)
{
- PCA9554Class *pc = PCA9554_CLASS(klass);
+ PCA9554Class *pc = PCA9554_BASE_CLASS(klass);
pc->pin_count = PCA9536_PIN_COUNT;
}
static const TypeInfo pca9554_types[] = {
{
- .name = TYPE_PCA9554,
+ .name = TYPE_PCA9554_BASE,
.parent = TYPE_I2C_SLAVE,
.instance_init = pca9554_initfn,
.instance_size = sizeof(PCA9554State),
- .class_init = pca9554_class_init,
+ .class_init = pca9554_base_class_init,
.class_size = sizeof(PCA9554Class),
- .abstract = false,
+ .abstract = true,
+ },
+ {
+ .name = TYPE_PCA9554,
+ .parent = TYPE_PCA9554_BASE,
+ .class_init = pca9554_class_init,
},
{
.name = TYPE_PCA9536,
--
2.55.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 5/8] hw/gpio/pca9554: Convert pin properties to class properties
2026-11-06 11:04 [PATCH 0/8] hw/gpio/pca955x: Improvements around props "pin" and "led" Markus Armbruster
` (3 preceding siblings ...)
2026-11-06 11:04 ` [PATCH 4/8] hw/gpio/pca9554: New abstract base "pca9554-base" Markus Armbruster
@ 2026-11-06 11:04 ` Markus Armbruster
2026-11-06 20:20 ` Miles Glenn
2026-11-09 9:03 ` Mark Cave-Ayland
2026-11-06 11:04 ` [PATCH 6/8] hw/gpio/pca955x: use QAPI enums for led and pin properties Markus Armbruster
` (2 subsequent siblings)
7 siblings, 2 replies; 35+ messages in thread
From: Markus Armbruster @ 2026-11-06 11:04 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, milesg, qemu-ppc, qemu-arm, mark.caveayland
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/gpio/pca9554.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
index 1362b605c2..ed304e4639 100644
--- a/hw/gpio/pca9554.c
+++ b/hw/gpio/pca9554.c
@@ -290,18 +290,18 @@ static void pca9554_reset(DeviceState *dev)
s->len = 0;
}
-static void pca9554_initfn(Object *obj)
+static void pca9554_pin_init(PCA9554Class *pc)
{
- PCA9554Class *pc = PCA9554_BASE_GET_CLASS(obj);
int pin;
for (pin = 0; pin < pc->pin_count; pin++) {
char *name;
name = g_strdup_printf("pin%d", pin);
- object_property_add(obj, name, "str", &str_type_info,
- pca9554_get_pin, pca9554_set_pin,
- NULL, NULL);
+ object_class_property_add(OBJECT_CLASS(pc), name,
+ "str", &str_type_info,
+ pca9554_get_pin, pca9554_set_pin,
+ NULL, NULL);
g_free(name);
}
}
@@ -352,6 +352,7 @@ static void pca9554_class_init(ObjectClass *klass, const void *data)
PCA9554Class *pc = PCA9554_BASE_CLASS(klass);
pc->pin_count = PCA9554_PIN_COUNT;
+ pca9554_pin_init(pc);
}
static void pca9536_class_init(ObjectClass *klass, const void *data)
@@ -359,13 +360,13 @@ static void pca9536_class_init(ObjectClass *klass, const void *data)
PCA9554Class *pc = PCA9554_BASE_CLASS(klass);
pc->pin_count = PCA9536_PIN_COUNT;
+ pca9554_pin_init(pc);
}
static const TypeInfo pca9554_types[] = {
{
.name = TYPE_PCA9554_BASE,
.parent = TYPE_I2C_SLAVE,
- .instance_init = pca9554_initfn,
.instance_size = sizeof(PCA9554State),
.class_init = pca9554_base_class_init,
.class_size = sizeof(PCA9554Class),
@@ -378,7 +379,7 @@ static const TypeInfo pca9554_types[] = {
},
{
.name = TYPE_PCA9536,
- .parent = TYPE_PCA9554,
+ .parent = TYPE_PCA9554_BASE,
.class_init = pca9536_class_init,
}
};
--
2.55.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 6/8] hw/gpio/pca955x: use QAPI enums for led and pin properties
2026-11-06 11:04 [PATCH 0/8] hw/gpio/pca955x: Improvements around props "pin" and "led" Markus Armbruster
` (4 preceding siblings ...)
2026-11-06 11:04 ` [PATCH 5/8] hw/gpio/pca9554: Convert pin properties to class properties Markus Armbruster
@ 2026-11-06 11:04 ` Markus Armbruster
2026-11-06 20:20 ` Miles Glenn
2026-11-09 9:05 ` Mark Cave-Ayland
2026-11-06 11:04 ` [PATCH 7/8] hw/gpio/pca955x: Improve @qapi-type reported by qom-list etc Markus Armbruster
2026-11-06 11:04 ` [PATCH 8/8] hw/gpio/pca955x: Simplify pin and led getters and setters Markus Armbruster
7 siblings, 2 replies; 35+ messages in thread
From: Markus Armbruster @ 2026-11-06 11:04 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, milesg, qemu-ppc, qemu-arm, mark.caveayland
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Replace hand-rolled string lookup tables with QAPI enums
Pca9552LedState and GpioPinState for the led%d and pin%d QOM
properties. This lets QAPI handle string-to-enum conversion in the
visitor, removing the manual string matching in the setters.
Add QEMU_BUILD_BUG_ON guards to ensure the QAPI-generated enum values
stay in sync with the hardware register encoding.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
qapi/machine.json | 29 ++++++++++++++++++
hw/gpio/pca9552.c | 76 +++++++++++++++++------------------------------
hw/gpio/pca9554.c | 35 ++++++++--------------
3 files changed, 69 insertions(+), 71 deletions(-)
diff --git a/qapi/machine.json b/qapi/machine.json
index 18d9dce849..c4c8720909 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -416,6 +416,35 @@
{ 'enum': 'LostTickPolicy',
'data': ['discard', 'delay', 'slew' ] }
+##
+# @GpioPinState:
+#
+# @low: Pin low
+#
+# @high: Pin high
+#
+# Since: 12.0
+##
+{ 'enum': 'GpioPinState',
+ 'data': ['low', 'high'] }
+
+##
+# @Pca9552LedState:
+# LED state for PCA9552.
+#
+# @on: LED on
+#
+# @off: LED off
+#
+# @pwm0: LED controlled by PWM0
+#
+# @pwm1: LED controlled by PWM1
+#
+# Since: 12.0
+##
+{ 'enum': 'Pca9552LedState',
+ 'data': ['on', 'off', 'pwm0', 'pwm1'] }
+
##
# @PCSouthBridgeOption:
# South bridge chipset option for PC i440FX machines.
diff --git a/hw/gpio/pca9552.c b/hw/gpio/pca9552.c
index 65a718ef31..99c4e0c0e2 100644
--- a/hw/gpio/pca9552.c
+++ b/hw/gpio/pca9552.c
@@ -24,6 +24,7 @@
#include "migration/vmstate.h"
#include "qapi/error.h"
#include "qapi/qapi-builtin-type-infos.h"
+#include "qapi/qapi-visit-machine.h"
#include "qapi/visitor.h"
#include "trace.h"
#include "qom/object.h"
@@ -60,16 +61,15 @@ struct PCA955xClass {
/*
* Note: The LED_ON and LED_OFF configuration values for the PCA955X
* chips are the reverse of the PCA953X family of chips.
+ *
+ * The QAPI enums must match the hardware register values.
*/
-#define PCA9552_LED_ON 0x0
-#define PCA9552_LED_OFF 0x1
-#define PCA9552_LED_PWM0 0x2
-#define PCA9552_LED_PWM1 0x3
-#define PCA9552_PIN_LOW 0x0
-#define PCA9552_PIN_HIZ 0x1
-
-static const char *led_state[] = {"on", "off", "pwm0", "pwm1"};
-static const char *pin_state[] = {"low", "high"};
+QEMU_BUILD_BUG_ON(PCA9552_LED_STATE_ON != 0x0);
+QEMU_BUILD_BUG_ON(PCA9552_LED_STATE_OFF != 0x1);
+QEMU_BUILD_BUG_ON(PCA9552_LED_STATE_PWM0 != 0x2);
+QEMU_BUILD_BUG_ON(PCA9552_LED_STATE_PWM1 != 0x3);
+QEMU_BUILD_BUG_ON(GPIO_PIN_STATE_LOW != 0x0);
+QEMU_BUILD_BUG_ON(GPIO_PIN_STATE_HIGH != 0x1);
static uint8_t pca955x_pin_get_config(PCA955xState *s, int pin)
{
@@ -143,24 +143,24 @@ static void pca955x_update_pin_input(PCA955xState *s)
uint8_t config = pca955x_pin_get_config(s, i);
switch (config) {
- case PCA9552_LED_ON:
+ case PCA9552_LED_STATE_ON:
/* Pin is set to 0V to turn on LED */
s->regs[input_reg] &= ~bit_mask;
break;
- case PCA9552_LED_OFF:
+ case PCA9552_LED_STATE_OFF:
/*
* Pin is set to Hi-Z to turn off LED and
* pullup sets it to a logical 1 unless
* external device drives it low.
*/
- if (s->ext_state[i] == PCA9552_PIN_LOW) {
+ if (s->ext_state[i] == GPIO_PIN_STATE_LOW) {
s->regs[input_reg] &= ~bit_mask;
} else {
s->regs[input_reg] |= bit_mask;
}
break;
- case PCA9552_LED_PWM0:
- case PCA9552_LED_PWM1:
+ case PCA9552_LED_STATE_PWM0:
+ case PCA9552_LED_STATE_PWM1:
/* TODO */
default:
break;
@@ -177,7 +177,7 @@ static void pca955x_update_pin_input(PCA955xState *s)
*/
if (s->regs[config_reg] & bit_mask) {
/* Input mode - reflect external state */
- if (s->ext_state[i] == PCA9552_PIN_LOW) {
+ if (s->ext_state[i] == GPIO_PIN_STATE_LOW) {
s->regs[input_reg] &= ~bit_mask;
} else {
s->regs[input_reg] |= bit_mask;
@@ -361,7 +361,7 @@ static void pca955x_get_led(Object *obj, Visitor *v, const char *name,
PCA955xClass *k = PCA955X_GET_CLASS(obj);
PCA955xState *s = PCA955X(obj);
int led, rc, reg;
- uint8_t state;
+ Pca9552LedState state;
rc = sscanf(name, "led%2d", &led);
if (rc != 1) {
@@ -379,7 +379,7 @@ static void pca955x_get_led(Object *obj, Visitor *v, const char *name,
*/
reg = PCA9552_LS0 + led / 4;
state = (pca955x_read(s, reg) >> ((led % 4) * 2)) & 0x3;
- visit_type_str(v, name, (char **)&led_state[state], errp);
+ visit_type_Pca9552LedState(v, name, &state, errp);
}
/*
@@ -398,10 +398,9 @@ static void pca955x_set_led(Object *obj, Visitor *v, const char *name,
PCA955xClass *k = PCA955X_GET_CLASS(obj);
PCA955xState *s = PCA955X(obj);
int led, rc, reg, val;
- uint8_t state;
- g_autofree char *state_str = NULL;
+ Pca9552LedState state;
- if (!visit_type_str(v, name, &state_str, errp)) {
+ if (!visit_type_Pca9552LedState(v, name, &state, errp)) {
return;
}
rc = sscanf(name, "led%2d", &led);
@@ -414,16 +413,6 @@ static void pca955x_set_led(Object *obj, Visitor *v, const char *name,
return;
}
- for (state = 0; state < ARRAY_SIZE(led_state); state++) {
- if (!strcmp(state_str, led_state[state])) {
- break;
- }
- }
- if (state >= ARRAY_SIZE(led_state)) {
- error_setg(errp, "%s invalid led state %s", __func__, state_str);
- return;
- }
-
reg = PCA9552_LS0 + led / 4;
val = pca955x_read(s, reg);
val = pca955x_ledsel(val, led % 4, state);
@@ -438,7 +427,8 @@ static void pca955x_get_pin(Object *obj, Visitor *v, const char *name,
PCA955xClass *k = PCA955X_GET_CLASS(obj);
PCA955xState *s = PCA955X(obj);
int pin, rc;
- uint8_t input_reg, state;
+ uint8_t input_reg;
+ GpioPinState state;
rc = sscanf(name, "pin%2d", &pin);
if (rc != 1) {
@@ -456,7 +446,7 @@ static void pca955x_get_pin(Object *obj, Visitor *v, const char *name,
*/
input_reg = PCA9535_INPUT0 + (pin / 8);
state = (s->regs[input_reg] >> (pin % 8)) & 0x1;
- visit_type_str(v, name, (char **)&pin_state[state], errp);
+ visit_type_GpioPinState(v, name, &state, errp);
}
static void pca955x_set_pin(Object *obj, Visitor *v, const char *name,
@@ -465,10 +455,10 @@ static void pca955x_set_pin(Object *obj, Visitor *v, const char *name,
PCA955xClass *k = PCA955X_GET_CLASS(obj);
PCA955xState *s = PCA955X(obj);
int pin, rc;
- uint8_t state, config_reg;
- g_autofree char *state_str = NULL;
+ GpioPinState state;
+ uint8_t config_reg;
- if (!visit_type_str(v, name, &state_str, errp)) {
+ if (!visit_type_GpioPinState(v, name, &state, errp)) {
return;
}
rc = sscanf(name, "pin%2d", &pin);
@@ -481,16 +471,6 @@ static void pca955x_set_pin(Object *obj, Visitor *v, const char *name,
return;
}
- for (state = 0; state < ARRAY_SIZE(pin_state); state++) {
- if (!strcmp(state_str, pin_state[state])) {
- break;
- }
- }
- if (state >= ARRAY_SIZE(pin_state)) {
- error_setg(errp, "%s invalid pin state %s", __func__, state_str);
- return;
- }
-
/* Only input-configured pins can be driven by an external device. */
config_reg = PCA9535_CONFIG0 + (pin / 8);
if (!((s->regs[config_reg] >> (pin % 8)) & 0x1)) {
@@ -500,7 +480,7 @@ static void pca955x_set_pin(Object *obj, Visitor *v, const char *name,
return;
}
- pca955x_set_ext_state(s, pin, state != PCA9552_PIN_LOW);
+ pca955x_set_ext_state(s, pin, state != GPIO_PIN_STATE_LOW);
}
static const VMStateDescription pca9552_vmstate = {
@@ -530,7 +510,7 @@ static void pca9552_reset_hold(Object *obj, ResetType type)
s->regs[PCA9552_LS2] = 0x55;
s->regs[PCA9552_LS3] = 0x55;
- memset(s->ext_state, PCA9552_PIN_HIZ, PCA955X_PIN_COUNT_MAX);
+ memset(s->ext_state, GPIO_PIN_STATE_HIGH, PCA955X_PIN_COUNT_MAX);
pca955x_update_pin_input(s);
s->pointer = 0xFF;
@@ -550,7 +530,7 @@ static void pca9535_reset_hold(Object *obj, ResetType type)
s->regs[PCA9535_CONFIG0] = 0xFF; /* All pins as inputs */
s->regs[PCA9535_CONFIG1] = 0xFF; /* All pins as inputs */
- memset(s->ext_state, PCA9552_PIN_HIZ, PCA955X_PIN_COUNT_MAX);
+ memset(s->ext_state, GPIO_PIN_STATE_HIGH, PCA955X_PIN_COUNT_MAX);
pca955x_update_pin_input(s);
s->pointer = 0xFF;
diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
index ed304e4639..7bbfd5ed5f 100644
--- a/hw/gpio/pca9554.c
+++ b/hw/gpio/pca9554.c
@@ -18,6 +18,7 @@
#include "migration/vmstate.h"
#include "qapi/error.h"
#include "qapi/qapi-builtin-type-infos.h"
+#include "qapi/qapi-visit-machine.h"
#include "qapi/visitor.h"
#include "trace.h"
#include "qom/object.h"
@@ -51,10 +52,8 @@ struct PCA9554Class {
uint8_t pin_count;
};
-#define PCA9554_PIN_LOW 0x0
-#define PCA9554_PIN_HIZ 0x1
-
-static const char *pin_state[] = {"low", "high"};
+QEMU_BUILD_BUG_ON(GPIO_PIN_STATE_LOW != 0x0);
+QEMU_BUILD_BUG_ON(GPIO_PIN_STATE_HIGH != 0x1);
static void pca9554_update_pin_input(PCA9554State *s)
{
@@ -73,7 +72,7 @@ static void pca9554_update_pin_input(PCA9554State *s)
* Input: the pin is Hi-Z with a pull-up, so it reads high
* unless an external device drives it low.
*/
- if (s->ext_state[i] == PCA9554_PIN_LOW) {
+ if (s->ext_state[i] == GPIO_PIN_STATE_LOW) {
s->regs[PCA9554_INPUT] &= ~bit_mask;
} else {
s->regs[PCA9554_INPUT] |= bit_mask;
@@ -175,7 +174,7 @@ static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
{
PCA9554State *s = PCA9554_BASE(obj);
int pin, rc;
- uint8_t state;
+ GpioPinState state;
rc = sscanf(name, "pin%2d", &pin);
if (rc != 1) {
@@ -194,7 +193,7 @@ static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
* holds the wire level regardless of the configured direction.
*/
state = (s->regs[PCA9554_INPUT] >> pin) & 0x1;
- visit_type_str(v, name, (char **)&pin_state[state], errp);
+ visit_type_GpioPinState(v, name, &state, errp);
}
static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
@@ -202,10 +201,10 @@ static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
{
PCA9554State *s = PCA9554_BASE(obj);
int pin, rc, val;
- uint8_t state, mask;
- g_autofree char *state_str = NULL;
+ uint8_t mask;
+ GpioPinState state;
- if (!visit_type_str(v, name, &state_str, errp)) {
+ if (!visit_type_GpioPinState(v, name, &state, errp)) {
return;
}
rc = sscanf(name, "pin%2d", &pin);
@@ -218,16 +217,6 @@ static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
return;
}
- for (state = 0; state < ARRAY_SIZE(pin_state); state++) {
- if (!strcmp(state_str, pin_state[state])) {
- break;
- }
- }
- if (state >= ARRAY_SIZE(pin_state)) {
- error_setg(errp, "%s invalid pin state %s", __func__, state_str);
- return;
- }
-
if (s->hw_dir) {
/* Warn and ignore if the guest has configured this pin as output */
if (!((s->regs[PCA9554_CONFIG] >> pin) & 0x1)) {
@@ -238,13 +227,13 @@ static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
return;
}
/* Drive the external input level */
- pca9554_set_ext_state(s, pin, state != PCA9554_PIN_LOW);
+ pca9554_set_ext_state(s, pin, state != GPIO_PIN_STATE_LOW);
} else {
/* Legacy behavior: force output mode and drive */
/* First, modify the output register bit */
val = pca9554_read(s, PCA9554_OUTPUT);
mask = 0x1 << pin;
- if (state == PCA9554_PIN_LOW) {
+ if (state == GPIO_PIN_STATE_LOW) {
val &= ~(mask);
} else {
val |= mask;
@@ -283,7 +272,7 @@ static void pca9554_reset(DeviceState *dev)
s->regs[PCA9554_POLARITY] = 0x0; /* No pins are inverted */
s->regs[PCA9554_CONFIG] = pin_mask; /* All pins are inputs */
- memset(s->ext_state, PCA9554_PIN_HIZ, pc->pin_count);
+ memset(s->ext_state, GPIO_PIN_STATE_HIGH, pc->pin_count);
pca9554_update_pin_input(s);
s->pointer = 0x0;
--
2.55.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 7/8] hw/gpio/pca955x: Improve @qapi-type reported by qom-list etc.
2026-11-06 11:04 [PATCH 0/8] hw/gpio/pca955x: Improvements around props "pin" and "led" Markus Armbruster
` (5 preceding siblings ...)
2026-11-06 11:04 ` [PATCH 6/8] hw/gpio/pca955x: use QAPI enums for led and pin properties Markus Armbruster
@ 2026-11-06 11:04 ` Markus Armbruster
2026-11-06 20:21 ` Miles Glenn
2026-11-09 9:06 ` Mark Cave-Ayland
2026-11-06 11:04 ` [PATCH 8/8] hw/gpio/pca955x: Simplify pin and led getters and setters Markus Armbruster
7 siblings, 2 replies; 35+ messages in thread
From: Markus Armbruster @ 2026-11-06 11:04 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, milesg, qemu-ppc, qemu-arm, mark.caveayland
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/gpio/pca9552.c | 6 +++---
hw/gpio/pca9554.c | 4 ++--
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/hw/gpio/pca9552.c b/hw/gpio/pca9552.c
index 99c4e0c0e2..f97ad6d083 100644
--- a/hw/gpio/pca9552.c
+++ b/hw/gpio/pca9552.c
@@ -23,7 +23,7 @@
#include "hw/core/irq.h"
#include "migration/vmstate.h"
#include "qapi/error.h"
-#include "qapi/qapi-builtin-type-infos.h"
+#include "qapi/qapi-type-infos-machine.h"
#include "qapi/qapi-visit-machine.h"
#include "qapi/visitor.h"
#include "trace.h"
@@ -547,14 +547,14 @@ static void pca955x_pin_init(PCA955xClass *k)
/* LED variant: expose the LED selector state as led%d. */
name = g_strdup_printf("led%d", ix);
object_class_property_add(OBJECT_CLASS(k), name,
- "str", &str_type_info,
+ "str", &Pca9552LedState_type_info,
pca955x_get_led, pca955x_set_led,
NULL, NULL);
} else {
/* GPIO variant: expose the pin logic level as pin%d. */
name = g_strdup_printf("pin%d", ix);
object_class_property_add(OBJECT_CLASS(k), name,
- "str", &str_type_info,
+ "str", &GpioPinState_type_info,
pca955x_get_pin, pca955x_set_pin,
NULL, NULL);
}
diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
index 7bbfd5ed5f..e2d481866f 100644
--- a/hw/gpio/pca9554.c
+++ b/hw/gpio/pca9554.c
@@ -17,7 +17,7 @@
#include "hw/core/irq.h"
#include "migration/vmstate.h"
#include "qapi/error.h"
-#include "qapi/qapi-builtin-type-infos.h"
+#include "qapi/qapi-type-infos-machine.h"
#include "qapi/qapi-visit-machine.h"
#include "qapi/visitor.h"
#include "trace.h"
@@ -288,7 +288,7 @@ static void pca9554_pin_init(PCA9554Class *pc)
name = g_strdup_printf("pin%d", pin);
object_class_property_add(OBJECT_CLASS(pc), name,
- "str", &str_type_info,
+ "str", &GpioPinState_type_info,
pca9554_get_pin, pca9554_set_pin,
NULL, NULL);
g_free(name);
--
2.55.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 8/8] hw/gpio/pca955x: Simplify pin and led getters and setters
2026-11-06 11:04 [PATCH 0/8] hw/gpio/pca955x: Improvements around props "pin" and "led" Markus Armbruster
` (6 preceding siblings ...)
2026-11-06 11:04 ` [PATCH 7/8] hw/gpio/pca955x: Improve @qapi-type reported by qom-list etc Markus Armbruster
@ 2026-11-06 11:04 ` Markus Armbruster
2026-11-06 20:21 ` Miles Glenn
2026-11-09 9:09 ` Mark Cave-Ayland
7 siblings, 2 replies; 35+ messages in thread
From: Markus Armbruster @ 2026-11-06 11:04 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, milesg, qemu-ppc, qemu-arm, mark.caveayland
Pass the index via object_class_property_add() parameter @opaque
instead extracting it from the property name.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/gpio/pca9552.c | 60 ++++++++++++++---------------------------------
hw/gpio/pca9554.c | 30 +++++++-----------------
2 files changed, 25 insertions(+), 65 deletions(-)
diff --git a/hw/gpio/pca9552.c b/hw/gpio/pca9552.c
index f97ad6d083..c8d0e9a16d 100644
--- a/hw/gpio/pca9552.c
+++ b/hw/gpio/pca9552.c
@@ -360,18 +360,12 @@ static void pca955x_get_led(Object *obj, Visitor *v, const char *name,
{
PCA955xClass *k = PCA955X_GET_CLASS(obj);
PCA955xState *s = PCA955X(obj);
- int led, rc, reg;
+ uintptr_t led = (uintptr_t)opaque;
+ int reg;
Pca9552LedState state;
- rc = sscanf(name, "led%2d", &led);
- if (rc != 1) {
- error_setg(errp, "%s: error reading %s", __func__, name);
- return;
- }
- if (led < 0 || led >= k->pin_count) {
- error_setg(errp, "%s: invalid led %s", __func__, name);
- return;
- }
+ assert(led < k->pin_count);
+
/*
* Get the LSx register as the qom interface should expose the device
* state, not the modeled 'input line' behaviour which would come from
@@ -397,21 +391,15 @@ static void pca955x_set_led(Object *obj, Visitor *v, const char *name,
{
PCA955xClass *k = PCA955X_GET_CLASS(obj);
PCA955xState *s = PCA955X(obj);
- int led, rc, reg, val;
+ uintptr_t led = (uintptr_t)opaque;
+ int reg, val;
Pca9552LedState state;
+ assert(led < k->pin_count);
+
if (!visit_type_Pca9552LedState(v, name, &state, errp)) {
return;
}
- rc = sscanf(name, "led%2d", &led);
- if (rc != 1) {
- error_setg(errp, "%s: error reading %s", __func__, name);
- return;
- }
- if (led < 0 || led >= k->pin_count) {
- error_setg(errp, "%s: invalid led %s", __func__, name);
- return;
- }
reg = PCA9552_LS0 + led / 4;
val = pca955x_read(s, reg);
@@ -426,19 +414,11 @@ static void pca955x_get_pin(Object *obj, Visitor *v, const char *name,
{
PCA955xClass *k = PCA955X_GET_CLASS(obj);
PCA955xState *s = PCA955X(obj);
- int pin, rc;
+ uintptr_t pin = (uintptr_t)opaque;
uint8_t input_reg;
GpioPinState state;
- rc = sscanf(name, "pin%2d", &pin);
- if (rc != 1) {
- error_setg(errp, "%s: error reading %s", __func__, name);
- return;
- }
- if (pin < 0 || pin >= k->pin_count) {
- error_setg(errp, "%s invalid pin %s", __func__, name);
- return;
- }
+ assert(pin < k->pin_count);
/*
* Report the raw pin logic level; polarity inversion is a read-time
@@ -454,28 +434,22 @@ static void pca955x_set_pin(Object *obj, Visitor *v, const char *name,
{
PCA955xClass *k = PCA955X_GET_CLASS(obj);
PCA955xState *s = PCA955X(obj);
- int pin, rc;
+ uintptr_t pin = (uintptr_t)opaque;
GpioPinState state;
uint8_t config_reg;
+ assert(pin < k->pin_count);
+
if (!visit_type_GpioPinState(v, name, &state, errp)) {
return;
}
- rc = sscanf(name, "pin%2d", &pin);
- if (rc != 1) {
- error_setg(errp, "%s: error reading %s", __func__, name);
- return;
- }
- if (pin < 0 || pin >= k->pin_count) {
- error_setg(errp, "%s invalid pin %s", __func__, name);
- return;
- }
/* Only input-configured pins can be driven by an external device. */
config_reg = PCA9535_CONFIG0 + (pin / 8);
if (!((s->regs[config_reg] >> (pin % 8)) & 0x1)) {
qemu_log_mask(LOG_UNIMP,
- "%s: pin %d is configured as output, ignoring set\n",
+ "%s: pin %" PRIdPTR " is configured as output, "
+ "ignoring set\n",
s->description, pin);
return;
}
@@ -549,14 +523,14 @@ static void pca955x_pin_init(PCA955xClass *k)
object_class_property_add(OBJECT_CLASS(k), name,
"str", &Pca9552LedState_type_info,
pca955x_get_led, pca955x_set_led,
- NULL, NULL);
+ NULL, (void *)(uintptr_t)ix);
} else {
/* GPIO variant: expose the pin logic level as pin%d. */
name = g_strdup_printf("pin%d", ix);
object_class_property_add(OBJECT_CLASS(k), name,
"str", &GpioPinState_type_info,
pca955x_get_pin, pca955x_set_pin,
- NULL, NULL);
+ NULL, (void *)(uintptr_t)ix);
}
g_free(name);
}
diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
index e2d481866f..d987b16cd0 100644
--- a/hw/gpio/pca9554.c
+++ b/hw/gpio/pca9554.c
@@ -173,18 +173,10 @@ static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
PCA9554State *s = PCA9554_BASE(obj);
- int pin, rc;
+ uintptr_t pin = (uintptr_t)opaque;
GpioPinState state;
- rc = sscanf(name, "pin%2d", &pin);
- if (rc != 1) {
- error_setg(errp, "%s: error reading %s", __func__, name);
- return;
- }
- if (pin < 0 || pin >= PCA9554_BASE_GET_CLASS(s)->pin_count) {
- error_setg(errp, "%s invalid pin %s", __func__, name);
- return;
- }
+ assert(pin < PCA9554_BASE_GET_CLASS(s)->pin_count);
/*
* Report the physical pin level. The input register is kept in sync by
@@ -200,28 +192,22 @@ static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
PCA9554State *s = PCA9554_BASE(obj);
- int pin, rc, val;
+ uintptr_t pin = (uintptr_t)opaque;
+ int val;
uint8_t mask;
GpioPinState state;
+ assert(pin < PCA9554_BASE_GET_CLASS(s)->pin_count);
+
if (!visit_type_GpioPinState(v, name, &state, errp)) {
return;
}
- rc = sscanf(name, "pin%2d", &pin);
- if (rc != 1) {
- error_setg(errp, "%s: error reading %s", __func__, name);
- return;
- }
- if (pin < 0 || pin >= PCA9554_BASE_GET_CLASS(s)->pin_count) {
- error_setg(errp, "%s invalid pin %s", __func__, name);
- return;
- }
if (s->hw_dir) {
/* Warn and ignore if the guest has configured this pin as output */
if (!((s->regs[PCA9554_CONFIG] >> pin) & 0x1)) {
qemu_log_mask(LOG_UNIMP,
- "%s: pin %d is configured as output, "
+ "%s: pin %" PRIdPTR " is configured as output, "
"ignoring external set\n",
s->description, pin);
return;
@@ -290,7 +276,7 @@ static void pca9554_pin_init(PCA9554Class *pc)
object_class_property_add(OBJECT_CLASS(pc), name,
"str", &GpioPinState_type_info,
pca9554_get_pin, pca9554_set_pin,
- NULL, NULL);
+ NULL, (void *)(uintptr_t)pin);
g_free(name);
}
}
--
2.55.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 1/8] hw/gpio/pca9552: Convert pin and led properties to class properties
2026-11-06 11:04 ` [PATCH 1/8] hw/gpio/pca9552: Convert pin and led properties to class properties Markus Armbruster
@ 2026-11-06 20:17 ` Miles Glenn
2026-11-09 8:51 ` Mark Cave-Ayland
1 sibling, 0 replies; 35+ messages in thread
From: Miles Glenn @ 2026-11-06 20:17 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: marcandre.lureau, qemu-ppc, qemu-arm, mark.caveayland
Reviewed-by: Glenn Miles <milesg@linux.ibm.com>
Thanks,
Glenn
On Fri, 2026-11-06 at 12:04 +0100, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/gpio/pca9552.c | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/hw/gpio/pca9552.c b/hw/gpio/pca9552.c
> index 90ccf5170e..65a718ef31 100644
> --- a/hw/gpio/pca9552.c
> +++ b/hw/gpio/pca9552.c
> @@ -557,10 +557,8 @@ static void pca9535_reset_hold(Object *obj, ResetType type)
> s->len = 0;
> }
>
> -static void pca955x_initfn(Object *obj)
> +static void pca955x_pin_init(PCA955xClass *k)
> {
> - PCA955xClass *k = PCA955X_GET_CLASS(obj);
> -
> assert(k->pin_count <= PCA955X_PIN_COUNT_MAX);
> for (int ix = 0; ix < k->pin_count; ix++) {
> char *name;
> @@ -568,13 +566,17 @@ static void pca955x_initfn(Object *obj)
> if (k->has_led_support) {
> /* LED variant: expose the LED selector state as led%d. */
> name = g_strdup_printf("led%d", ix);
> - object_property_add(obj, name, "str", &str_type_info,
> - pca955x_get_led, pca955x_set_led, NULL, NULL);
> + object_class_property_add(OBJECT_CLASS(k), name,
> + "str", &str_type_info,
> + pca955x_get_led, pca955x_set_led,
> + NULL, NULL);
> } else {
> /* GPIO variant: expose the pin logic level as pin%d. */
> name = g_strdup_printf("pin%d", ix);
> - object_property_add(obj, name, "str", &str_type_info,
> - pca955x_get_pin, pca955x_set_pin, NULL, NULL);
> + object_class_property_add(OBJECT_CLASS(k), name,
> + "str", &str_type_info,
> + pca955x_get_pin, pca955x_set_pin,
> + NULL, NULL);
> }
> g_free(name);
> }
> @@ -640,6 +642,7 @@ static void pca9552_class_init(ObjectClass *oc, const void *data)
> pc->max_reg = PCA9552_LS3;
> pc->pin_count = 16;
> pc->has_led_support = true;
> + pca955x_pin_init(pc);
> }
>
> static void pca95x5_class_init(ObjectClass *oc, const void *data)
> @@ -653,13 +656,13 @@ static void pca95x5_class_init(ObjectClass *oc, const void *data)
> pc->max_reg = PCA9535_CONFIG1;
> pc->pin_count = 16;
> pc->has_led_support = false;
> + pca955x_pin_init(pc);
> }
>
> static const TypeInfo pca955x_types[] = {
> {
> .name = TYPE_PCA955X,
> .parent = TYPE_I2C_SLAVE,
> - .instance_init = pca955x_initfn,
> .instance_size = sizeof(PCA955xState),
> .class_init = pca955x_class_init,
> .class_size = sizeof(PCA955xClass),
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/8] hw/gpio/pca9554: Move most code from .h to .c
2026-11-06 11:04 ` [PATCH 2/8] hw/gpio/pca9554: Move most code from .h to .c Markus Armbruster
@ 2026-11-06 20:18 ` Miles Glenn
2026-11-09 8:55 ` Mark Cave-Ayland
1 sibling, 0 replies; 35+ messages in thread
From: Miles Glenn @ 2026-11-06 20:18 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: marcandre.lureau, qemu-ppc, qemu-arm, mark.caveayland
Reviewed-by: Glenn Miles <milesg@linux.ibm.com>
Thanks,
Glenn
On Fri, 2026-11-06 at 12:04 +0100, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> include/hw/gpio/pca9554.h | 25 -------------------------
> hw/gpio/pca9554.c | 24 ++++++++++++++++++++++++
> 2 files changed, 24 insertions(+), 25 deletions(-)
>
> diff --git a/include/hw/gpio/pca9554.h b/include/hw/gpio/pca9554.h
> index ac835371aa..1b33a8a752 100644
> --- a/include/hw/gpio/pca9554.h
> +++ b/include/hw/gpio/pca9554.h
> @@ -8,32 +8,7 @@
> #ifndef PCA9554_H
> #define PCA9554_H
>
> -#include "hw/i2c/i2c.h"
> -#include "qom/object.h"
> -
> #define TYPE_PCA9554 "pca9554"
> #define TYPE_PCA9536 "pca9536"
> -typedef struct PCA9554State PCA9554State;
> -DECLARE_INSTANCE_CHECKER(PCA9554State, PCA9554,
> - TYPE_PCA9554)
> -
> -#define PCA9554_NR_REGS 4
> -#define PCA9554_PIN_COUNT 8
> -#define PCA9536_PIN_COUNT 4
> -
> -struct PCA9554State {
> - /*< private >*/
> - I2CSlave i2c;
> - /*< public >*/
> -
> - uint8_t len;
> - uint8_t pointer;
> -
> - uint8_t regs[PCA9554_NR_REGS];
> - qemu_irq gpio_out[PCA9554_PIN_COUNT];
> - uint8_t ext_state[PCA9554_PIN_COUNT];
> - char *description; /* For debugging purpose only */
> - bool hw_dir; /* Honor pin direction */
> -};
>
> #endif
> diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
> index 07bb00babf..e5a1932c89 100644
> --- a/hw/gpio/pca9554.c
> +++ b/hw/gpio/pca9554.c
> @@ -13,6 +13,7 @@
> #include "hw/core/qdev-properties.h"
> #include "hw/gpio/pca9554.h"
> #include "hw/gpio/pca9554_regs.h"
> +#include "hw/i2c/i2c.h"
> #include "hw/core/irq.h"
> #include "migration/vmstate.h"
> #include "qapi/error.h"
> @@ -21,6 +22,29 @@
> #include "trace.h"
> #include "qom/object.h"
>
> +typedef struct PCA9554State PCA9554State;
> +DECLARE_INSTANCE_CHECKER(PCA9554State, PCA9554,
> + TYPE_PCA9554)
> +
> +#define PCA9554_NR_REGS 4
> +#define PCA9554_PIN_COUNT 8
> +#define PCA9536_PIN_COUNT 4
> +
> +struct PCA9554State {
> + /*< private >*/
> + I2CSlave i2c;
> + /*< public >*/
> +
> + uint8_t len;
> + uint8_t pointer;
> +
> + uint8_t regs[PCA9554_NR_REGS];
> + qemu_irq gpio_out[PCA9554_PIN_COUNT];
> + uint8_t ext_state[PCA9554_PIN_COUNT];
> + char *description; /* For debugging purpose only */
> + bool hw_dir; /* Honor pin direction */
> +};
> +
> struct PCA9554Class {
> /*< private >*/
> I2CSlaveClass parent_class;
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/8] hw/gpio/pca9554: Simplify QOM type creation
2026-11-06 11:04 ` [PATCH 3/8] hw/gpio/pca9554: Simplify QOM type creation Markus Armbruster
@ 2026-11-06 20:19 ` Miles Glenn
2026-11-09 8:56 ` Mark Cave-Ayland
1 sibling, 0 replies; 35+ messages in thread
From: Miles Glenn @ 2026-11-06 20:19 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: marcandre.lureau, qemu-ppc, qemu-arm, mark.caveayland
Reviewed-by: Glenn Miles <milesg@linux.ibm.com>
Thanks,
Glenn
On Fri, 2026-11-06 at 12:04 +0100, Markus Armbruster wrote:
> According to object.h, direct usage of DECLARE_INSTANCE_CHECKERS()
> should be avoided, and the complete OBJECT_DECLARE_TYPE macro is
> recommended instead. So use that.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/gpio/pca9554.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
> index e5a1932c89..caa73f63c9 100644
> --- a/hw/gpio/pca9554.c
> +++ b/hw/gpio/pca9554.c
> @@ -22,14 +22,12 @@
> #include "trace.h"
> #include "qom/object.h"
>
> -typedef struct PCA9554State PCA9554State;
> -DECLARE_INSTANCE_CHECKER(PCA9554State, PCA9554,
> - TYPE_PCA9554)
> -
> #define PCA9554_NR_REGS 4
> #define PCA9554_PIN_COUNT 8
> #define PCA9536_PIN_COUNT 4
>
> +OBJECT_DECLARE_TYPE(PCA9554State, PCA9554Class, PCA9554)
> +
> struct PCA9554State {
> /*< private >*/
> I2CSlave i2c;
> @@ -52,10 +50,6 @@ struct PCA9554Class {
>
> uint8_t pin_count;
> };
> -typedef struct PCA9554Class PCA9554Class;
> -
> -DECLARE_CLASS_CHECKERS(PCA9554Class, PCA9554,
> - TYPE_PCA9554)
>
> #define PCA9554_PIN_LOW 0x0
> #define PCA9554_PIN_HIZ 0x1
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 4/8] hw/gpio/pca9554: New abstract base "pca9554-base"
2026-11-06 11:04 ` [PATCH 4/8] hw/gpio/pca9554: New abstract base "pca9554-base" Markus Armbruster
@ 2026-11-06 20:19 ` Miles Glenn
2026-11-09 9:02 ` Mark Cave-Ayland
1 sibling, 0 replies; 35+ messages in thread
From: Miles Glenn @ 2026-11-06 20:19 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: marcandre.lureau, qemu-ppc, qemu-arm, mark.caveayland
Reviewed-by: Glenn Miles <milesg@linux.ibm.com>
Thanks,
Glenn
On Fri, 2026-11-06 at 12:04 +0100, Markus Armbruster wrote:
> Commit 5eceb48bbe86 (hw/gpio: pca9554: add PCA9536 support) made type
> "pca9535" a child of concrete type "pca9555". This is inconvenient
> for the next commit. Make them both children of new abstract type
> "pca9554-base".
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> include/hw/gpio/pca9554.h | 1 +
> hw/gpio/pca9554.c | 55 +++++++++++++++++++++++----------------
> 2 files changed, 33 insertions(+), 23 deletions(-)
>
> diff --git a/include/hw/gpio/pca9554.h b/include/hw/gpio/pca9554.h
> index 1b33a8a752..0d841ff017 100644
> --- a/include/hw/gpio/pca9554.h
> +++ b/include/hw/gpio/pca9554.h
> @@ -8,6 +8,7 @@
> #ifndef PCA9554_H
> #define PCA9554_H
>
> +#define TYPE_PCA9554_BASE "pca9554-base"
> #define TYPE_PCA9554 "pca9554"
> #define TYPE_PCA9536 "pca9536"
>
> diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
> index caa73f63c9..1362b605c2 100644
> --- a/hw/gpio/pca9554.c
> +++ b/hw/gpio/pca9554.c
> @@ -26,7 +26,7 @@
> #define PCA9554_PIN_COUNT 8
> #define PCA9536_PIN_COUNT 4
>
> -OBJECT_DECLARE_TYPE(PCA9554State, PCA9554Class, PCA9554)
> +OBJECT_DECLARE_TYPE(PCA9554State, PCA9554Class, PCA9554_BASE)
>
> struct PCA9554State {
> /*< private >*/
> @@ -58,7 +58,7 @@ static const char *pin_state[] = {"low", "high"};
>
> static void pca9554_update_pin_input(PCA9554State *s)
> {
> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
> int i;
> uint8_t config = s->regs[PCA9554_CONFIG];
> uint8_t output = s->regs[PCA9554_OUTPUT];
> @@ -110,7 +110,7 @@ static uint8_t pca9554_read(PCA9554State *s, uint8_t reg)
>
> static void pca9554_write(PCA9554State *s, uint8_t reg, uint8_t data)
> {
> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
> uint8_t pin_mask = (1 << pc->pin_count) - 1;
>
> /* Variants narrower than 8 bits ignore the unimplemented upper pins. */
> @@ -134,14 +134,14 @@ static void pca9554_write(PCA9554State *s, uint8_t reg, uint8_t data)
>
> static uint8_t pca9554_recv(I2CSlave *i2c)
> {
> - PCA9554State *s = PCA9554(i2c);
> + PCA9554State *s = PCA9554_BASE(i2c);
>
> return pca9554_read(s, s->pointer & 0x3);
> }
>
> static int pca9554_send(I2CSlave *i2c, uint8_t data)
> {
> - PCA9554State *s = PCA9554(i2c);
> + PCA9554State *s = PCA9554_BASE(i2c);
>
> /* First byte sent by is the register address */
> if (s->len == 0) {
> @@ -156,7 +156,7 @@ static int pca9554_send(I2CSlave *i2c, uint8_t data)
>
> static int pca9554_event(I2CSlave *i2c, enum i2c_event event)
> {
> - PCA9554State *s = PCA9554(i2c);
> + PCA9554State *s = PCA9554_BASE(i2c);
>
> s->len = 0;
> return 0;
> @@ -173,7 +173,7 @@ static void pca9554_set_ext_state(PCA9554State *s, int pin, int level)
> static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> - PCA9554State *s = PCA9554(obj);
> + PCA9554State *s = PCA9554_BASE(obj);
> int pin, rc;
> uint8_t state;
>
> @@ -182,7 +182,7 @@ static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
> error_setg(errp, "%s: error reading %s", __func__, name);
> return;
> }
> - if (pin < 0 || pin >= PCA9554_GET_CLASS(s)->pin_count) {
> + if (pin < 0 || pin >= PCA9554_BASE_GET_CLASS(s)->pin_count) {
> error_setg(errp, "%s invalid pin %s", __func__, name);
> return;
> }
> @@ -200,7 +200,7 @@ static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
> static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> - PCA9554State *s = PCA9554(obj);
> + PCA9554State *s = PCA9554_BASE(obj);
> int pin, rc, val;
> uint8_t state, mask;
> g_autofree char *state_str = NULL;
> @@ -213,7 +213,7 @@ static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
> error_setg(errp, "%s: error reading %s", __func__, name);
> return;
> }
> - if (pin < 0 || pin >= PCA9554_GET_CLASS(s)->pin_count) {
> + if (pin < 0 || pin >= PCA9554_BASE_GET_CLASS(s)->pin_count) {
> error_setg(errp, "%s invalid pin %s", __func__, name);
> return;
> }
> @@ -274,8 +274,8 @@ static const VMStateDescription pca9554_vmstate = {
>
> static void pca9554_reset(DeviceState *dev)
> {
> - PCA9554State *s = PCA9554(dev);
> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
> + PCA9554State *s = PCA9554_BASE(dev);
> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
> uint8_t pin_mask = (1 << pc->pin_count) - 1;
>
> s->regs[PCA9554_INPUT] = pin_mask;
> @@ -292,7 +292,7 @@ static void pca9554_reset(DeviceState *dev)
>
> static void pca9554_initfn(Object *obj)
> {
> - PCA9554Class *pc = PCA9554_GET_CLASS(obj);
> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(obj);
> int pin;
>
> for (pin = 0; pin < pc->pin_count; pin++) {
> @@ -308,8 +308,8 @@ static void pca9554_initfn(Object *obj)
>
> static void pca9554_gpio_in_handler(void *opaque, int pin, int level)
> {
> - PCA9554State *s = PCA9554(opaque);
> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
> + PCA9554State *s = PCA9554_BASE(opaque);
> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
>
> assert((pin >= 0) && (pin < pc->pin_count));
> pca9554_set_ext_state(s, pin, level);
> @@ -317,8 +317,8 @@ static void pca9554_gpio_in_handler(void *opaque, int pin, int level)
>
> static void pca9554_realize(DeviceState *dev, Error **errp)
> {
> - PCA9554State *s = PCA9554(dev);
> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
> + PCA9554State *s = PCA9554_BASE(dev);
> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
>
> if (!s->description) {
> s->description = g_strdup(object_get_typename(OBJECT(dev)));
> @@ -333,11 +333,10 @@ static const Property pca9554_properties[] = {
> DEFINE_PROP_BOOL("hw-dir", PCA9554State, hw_dir, false),
> };
>
> -static void pca9554_class_init(ObjectClass *klass, const void *data)
> +static void pca9554_base_class_init(ObjectClass *klass, const void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
> - PCA9554Class *pc = PCA9554_CLASS(klass);
>
> k->event = pca9554_event;
> k->recv = pca9554_recv;
> @@ -346,26 +345,36 @@ static void pca9554_class_init(ObjectClass *klass, const void *data)
> device_class_set_legacy_reset(dc, pca9554_reset);
> dc->vmsd = &pca9554_vmstate;
> device_class_set_props(dc, pca9554_properties);
> +}
> +
> +static void pca9554_class_init(ObjectClass *klass, const void *data)
> +{
> + PCA9554Class *pc = PCA9554_BASE_CLASS(klass);
>
> pc->pin_count = PCA9554_PIN_COUNT;
> }
>
> static void pca9536_class_init(ObjectClass *klass, const void *data)
> {
> - PCA9554Class *pc = PCA9554_CLASS(klass);
> + PCA9554Class *pc = PCA9554_BASE_CLASS(klass);
>
> pc->pin_count = PCA9536_PIN_COUNT;
> }
>
> static const TypeInfo pca9554_types[] = {
> {
> - .name = TYPE_PCA9554,
> + .name = TYPE_PCA9554_BASE,
> .parent = TYPE_I2C_SLAVE,
> .instance_init = pca9554_initfn,
> .instance_size = sizeof(PCA9554State),
> - .class_init = pca9554_class_init,
> + .class_init = pca9554_base_class_init,
> .class_size = sizeof(PCA9554Class),
> - .abstract = false,
> + .abstract = true,
> + },
> + {
> + .name = TYPE_PCA9554,
> + .parent = TYPE_PCA9554_BASE,
> + .class_init = pca9554_class_init,
> },
> {
> .name = TYPE_PCA9536,
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 5/8] hw/gpio/pca9554: Convert pin properties to class properties
2026-11-06 11:04 ` [PATCH 5/8] hw/gpio/pca9554: Convert pin properties to class properties Markus Armbruster
@ 2026-11-06 20:20 ` Miles Glenn
2026-11-09 9:03 ` Mark Cave-Ayland
1 sibling, 0 replies; 35+ messages in thread
From: Miles Glenn @ 2026-11-06 20:20 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: marcandre.lureau, qemu-ppc, qemu-arm, mark.caveayland
Reviewed-by: Glenn Miles <milesg@linux.ibm.com>
Thanks,
Glenn
On Fri, 2026-11-06 at 12:04 +0100, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/gpio/pca9554.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
> index 1362b605c2..ed304e4639 100644
> --- a/hw/gpio/pca9554.c
> +++ b/hw/gpio/pca9554.c
> @@ -290,18 +290,18 @@ static void pca9554_reset(DeviceState *dev)
> s->len = 0;
> }
>
> -static void pca9554_initfn(Object *obj)
> +static void pca9554_pin_init(PCA9554Class *pc)
> {
> - PCA9554Class *pc = PCA9554_BASE_GET_CLASS(obj);
> int pin;
>
> for (pin = 0; pin < pc->pin_count; pin++) {
> char *name;
>
> name = g_strdup_printf("pin%d", pin);
> - object_property_add(obj, name, "str", &str_type_info,
> - pca9554_get_pin, pca9554_set_pin,
> - NULL, NULL);
> + object_class_property_add(OBJECT_CLASS(pc), name,
> + "str", &str_type_info,
> + pca9554_get_pin, pca9554_set_pin,
> + NULL, NULL);
> g_free(name);
> }
> }
> @@ -352,6 +352,7 @@ static void pca9554_class_init(ObjectClass *klass, const void *data)
> PCA9554Class *pc = PCA9554_BASE_CLASS(klass);
>
> pc->pin_count = PCA9554_PIN_COUNT;
> + pca9554_pin_init(pc);
> }
>
> static void pca9536_class_init(ObjectClass *klass, const void *data)
> @@ -359,13 +360,13 @@ static void pca9536_class_init(ObjectClass *klass, const void *data)
> PCA9554Class *pc = PCA9554_BASE_CLASS(klass);
>
> pc->pin_count = PCA9536_PIN_COUNT;
> + pca9554_pin_init(pc);
> }
>
> static const TypeInfo pca9554_types[] = {
> {
> .name = TYPE_PCA9554_BASE,
> .parent = TYPE_I2C_SLAVE,
> - .instance_init = pca9554_initfn,
> .instance_size = sizeof(PCA9554State),
> .class_init = pca9554_base_class_init,
> .class_size = sizeof(PCA9554Class),
> @@ -378,7 +379,7 @@ static const TypeInfo pca9554_types[] = {
> },
> {
> .name = TYPE_PCA9536,
> - .parent = TYPE_PCA9554,
> + .parent = TYPE_PCA9554_BASE,
> .class_init = pca9536_class_init,
> }
> };
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 6/8] hw/gpio/pca955x: use QAPI enums for led and pin properties
2026-11-06 11:04 ` [PATCH 6/8] hw/gpio/pca955x: use QAPI enums for led and pin properties Markus Armbruster
@ 2026-11-06 20:20 ` Miles Glenn
2026-11-09 9:05 ` Mark Cave-Ayland
1 sibling, 0 replies; 35+ messages in thread
From: Miles Glenn @ 2026-11-06 20:20 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: marcandre.lureau, qemu-ppc, qemu-arm, mark.caveayland
Reviewed-by: Glenn Miles <milesg@linux.ibm.com>
Thanks,
Glenn
On Fri, 2026-11-06 at 12:04 +0100, Markus Armbruster wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Replace hand-rolled string lookup tables with QAPI enums
> Pca9552LedState and GpioPinState for the led%d and pin%d QOM
> properties. This lets QAPI handle string-to-enum conversion in the
> visitor, removing the manual string matching in the setters.
>
> Add QEMU_BUILD_BUG_ON guards to ensure the QAPI-generated enum values
> stay in sync with the hardware register encoding.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> qapi/machine.json | 29 ++++++++++++++++++
> hw/gpio/pca9552.c | 76 +++++++++++++++++------------------------------
> hw/gpio/pca9554.c | 35 ++++++++--------------
> 3 files changed, 69 insertions(+), 71 deletions(-)
>
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 18d9dce849..c4c8720909 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -416,6 +416,35 @@
> { 'enum': 'LostTickPolicy',
> 'data': ['discard', 'delay', 'slew' ] }
>
> +##
> +# @GpioPinState:
> +#
> +# @low: Pin low
> +#
> +# @high: Pin high
> +#
> +# Since: 12.0
> +##
> +{ 'enum': 'GpioPinState',
> + 'data': ['low', 'high'] }
> +
> +##
> +# @Pca9552LedState:
> +# LED state for PCA9552.
> +#
> +# @on: LED on
> +#
> +# @off: LED off
> +#
> +# @pwm0: LED controlled by PWM0
> +#
> +# @pwm1: LED controlled by PWM1
> +#
> +# Since: 12.0
> +##
> +{ 'enum': 'Pca9552LedState',
> + 'data': ['on', 'off', 'pwm0', 'pwm1'] }
> +
> ##
> # @PCSouthBridgeOption:
> # South bridge chipset option for PC i440FX machines.
> diff --git a/hw/gpio/pca9552.c b/hw/gpio/pca9552.c
> index 65a718ef31..99c4e0c0e2 100644
> --- a/hw/gpio/pca9552.c
> +++ b/hw/gpio/pca9552.c
> @@ -24,6 +24,7 @@
> #include "migration/vmstate.h"
> #include "qapi/error.h"
> #include "qapi/qapi-builtin-type-infos.h"
> +#include "qapi/qapi-visit-machine.h"
> #include "qapi/visitor.h"
> #include "trace.h"
> #include "qom/object.h"
> @@ -60,16 +61,15 @@ struct PCA955xClass {
> /*
> * Note: The LED_ON and LED_OFF configuration values for the PCA955X
> * chips are the reverse of the PCA953X family of chips.
> + *
> + * The QAPI enums must match the hardware register values.
> */
> -#define PCA9552_LED_ON 0x0
> -#define PCA9552_LED_OFF 0x1
> -#define PCA9552_LED_PWM0 0x2
> -#define PCA9552_LED_PWM1 0x3
> -#define PCA9552_PIN_LOW 0x0
> -#define PCA9552_PIN_HIZ 0x1
> -
> -static const char *led_state[] = {"on", "off", "pwm0", "pwm1"};
> -static const char *pin_state[] = {"low", "high"};
> +QEMU_BUILD_BUG_ON(PCA9552_LED_STATE_ON != 0x0);
> +QEMU_BUILD_BUG_ON(PCA9552_LED_STATE_OFF != 0x1);
> +QEMU_BUILD_BUG_ON(PCA9552_LED_STATE_PWM0 != 0x2);
> +QEMU_BUILD_BUG_ON(PCA9552_LED_STATE_PWM1 != 0x3);
> +QEMU_BUILD_BUG_ON(GPIO_PIN_STATE_LOW != 0x0);
> +QEMU_BUILD_BUG_ON(GPIO_PIN_STATE_HIGH != 0x1);
>
> static uint8_t pca955x_pin_get_config(PCA955xState *s, int pin)
> {
> @@ -143,24 +143,24 @@ static void pca955x_update_pin_input(PCA955xState *s)
> uint8_t config = pca955x_pin_get_config(s, i);
>
> switch (config) {
> - case PCA9552_LED_ON:
> + case PCA9552_LED_STATE_ON:
> /* Pin is set to 0V to turn on LED */
> s->regs[input_reg] &= ~bit_mask;
> break;
> - case PCA9552_LED_OFF:
> + case PCA9552_LED_STATE_OFF:
> /*
> * Pin is set to Hi-Z to turn off LED and
> * pullup sets it to a logical 1 unless
> * external device drives it low.
> */
> - if (s->ext_state[i] == PCA9552_PIN_LOW) {
> + if (s->ext_state[i] == GPIO_PIN_STATE_LOW) {
> s->regs[input_reg] &= ~bit_mask;
> } else {
> s->regs[input_reg] |= bit_mask;
> }
> break;
> - case PCA9552_LED_PWM0:
> - case PCA9552_LED_PWM1:
> + case PCA9552_LED_STATE_PWM0:
> + case PCA9552_LED_STATE_PWM1:
> /* TODO */
> default:
> break;
> @@ -177,7 +177,7 @@ static void pca955x_update_pin_input(PCA955xState *s)
> */
> if (s->regs[config_reg] & bit_mask) {
> /* Input mode - reflect external state */
> - if (s->ext_state[i] == PCA9552_PIN_LOW) {
> + if (s->ext_state[i] == GPIO_PIN_STATE_LOW) {
> s->regs[input_reg] &= ~bit_mask;
> } else {
> s->regs[input_reg] |= bit_mask;
> @@ -361,7 +361,7 @@ static void pca955x_get_led(Object *obj, Visitor *v, const char *name,
> PCA955xClass *k = PCA955X_GET_CLASS(obj);
> PCA955xState *s = PCA955X(obj);
> int led, rc, reg;
> - uint8_t state;
> + Pca9552LedState state;
>
> rc = sscanf(name, "led%2d", &led);
> if (rc != 1) {
> @@ -379,7 +379,7 @@ static void pca955x_get_led(Object *obj, Visitor *v, const char *name,
> */
> reg = PCA9552_LS0 + led / 4;
> state = (pca955x_read(s, reg) >> ((led % 4) * 2)) & 0x3;
> - visit_type_str(v, name, (char **)&led_state[state], errp);
> + visit_type_Pca9552LedState(v, name, &state, errp);
> }
>
> /*
> @@ -398,10 +398,9 @@ static void pca955x_set_led(Object *obj, Visitor *v, const char *name,
> PCA955xClass *k = PCA955X_GET_CLASS(obj);
> PCA955xState *s = PCA955X(obj);
> int led, rc, reg, val;
> - uint8_t state;
> - g_autofree char *state_str = NULL;
> + Pca9552LedState state;
>
> - if (!visit_type_str(v, name, &state_str, errp)) {
> + if (!visit_type_Pca9552LedState(v, name, &state, errp)) {
> return;
> }
> rc = sscanf(name, "led%2d", &led);
> @@ -414,16 +413,6 @@ static void pca955x_set_led(Object *obj, Visitor *v, const char *name,
> return;
> }
>
> - for (state = 0; state < ARRAY_SIZE(led_state); state++) {
> - if (!strcmp(state_str, led_state[state])) {
> - break;
> - }
> - }
> - if (state >= ARRAY_SIZE(led_state)) {
> - error_setg(errp, "%s invalid led state %s", __func__, state_str);
> - return;
> - }
> -
> reg = PCA9552_LS0 + led / 4;
> val = pca955x_read(s, reg);
> val = pca955x_ledsel(val, led % 4, state);
> @@ -438,7 +427,8 @@ static void pca955x_get_pin(Object *obj, Visitor *v, const char *name,
> PCA955xClass *k = PCA955X_GET_CLASS(obj);
> PCA955xState *s = PCA955X(obj);
> int pin, rc;
> - uint8_t input_reg, state;
> + uint8_t input_reg;
> + GpioPinState state;
>
> rc = sscanf(name, "pin%2d", &pin);
> if (rc != 1) {
> @@ -456,7 +446,7 @@ static void pca955x_get_pin(Object *obj, Visitor *v, const char *name,
> */
> input_reg = PCA9535_INPUT0 + (pin / 8);
> state = (s->regs[input_reg] >> (pin % 8)) & 0x1;
> - visit_type_str(v, name, (char **)&pin_state[state], errp);
> + visit_type_GpioPinState(v, name, &state, errp);
> }
>
> static void pca955x_set_pin(Object *obj, Visitor *v, const char *name,
> @@ -465,10 +455,10 @@ static void pca955x_set_pin(Object *obj, Visitor *v, const char *name,
> PCA955xClass *k = PCA955X_GET_CLASS(obj);
> PCA955xState *s = PCA955X(obj);
> int pin, rc;
> - uint8_t state, config_reg;
> - g_autofree char *state_str = NULL;
> + GpioPinState state;
> + uint8_t config_reg;
>
> - if (!visit_type_str(v, name, &state_str, errp)) {
> + if (!visit_type_GpioPinState(v, name, &state, errp)) {
> return;
> }
> rc = sscanf(name, "pin%2d", &pin);
> @@ -481,16 +471,6 @@ static void pca955x_set_pin(Object *obj, Visitor *v, const char *name,
> return;
> }
>
> - for (state = 0; state < ARRAY_SIZE(pin_state); state++) {
> - if (!strcmp(state_str, pin_state[state])) {
> - break;
> - }
> - }
> - if (state >= ARRAY_SIZE(pin_state)) {
> - error_setg(errp, "%s invalid pin state %s", __func__, state_str);
> - return;
> - }
> -
> /* Only input-configured pins can be driven by an external device. */
> config_reg = PCA9535_CONFIG0 + (pin / 8);
> if (!((s->regs[config_reg] >> (pin % 8)) & 0x1)) {
> @@ -500,7 +480,7 @@ static void pca955x_set_pin(Object *obj, Visitor *v, const char *name,
> return;
> }
>
> - pca955x_set_ext_state(s, pin, state != PCA9552_PIN_LOW);
> + pca955x_set_ext_state(s, pin, state != GPIO_PIN_STATE_LOW);
> }
>
> static const VMStateDescription pca9552_vmstate = {
> @@ -530,7 +510,7 @@ static void pca9552_reset_hold(Object *obj, ResetType type)
> s->regs[PCA9552_LS2] = 0x55;
> s->regs[PCA9552_LS3] = 0x55;
>
> - memset(s->ext_state, PCA9552_PIN_HIZ, PCA955X_PIN_COUNT_MAX);
> + memset(s->ext_state, GPIO_PIN_STATE_HIGH, PCA955X_PIN_COUNT_MAX);
> pca955x_update_pin_input(s);
>
> s->pointer = 0xFF;
> @@ -550,7 +530,7 @@ static void pca9535_reset_hold(Object *obj, ResetType type)
> s->regs[PCA9535_CONFIG0] = 0xFF; /* All pins as inputs */
> s->regs[PCA9535_CONFIG1] = 0xFF; /* All pins as inputs */
>
> - memset(s->ext_state, PCA9552_PIN_HIZ, PCA955X_PIN_COUNT_MAX);
> + memset(s->ext_state, GPIO_PIN_STATE_HIGH, PCA955X_PIN_COUNT_MAX);
> pca955x_update_pin_input(s);
>
> s->pointer = 0xFF;
> diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
> index ed304e4639..7bbfd5ed5f 100644
> --- a/hw/gpio/pca9554.c
> +++ b/hw/gpio/pca9554.c
> @@ -18,6 +18,7 @@
> #include "migration/vmstate.h"
> #include "qapi/error.h"
> #include "qapi/qapi-builtin-type-infos.h"
> +#include "qapi/qapi-visit-machine.h"
> #include "qapi/visitor.h"
> #include "trace.h"
> #include "qom/object.h"
> @@ -51,10 +52,8 @@ struct PCA9554Class {
> uint8_t pin_count;
> };
>
> -#define PCA9554_PIN_LOW 0x0
> -#define PCA9554_PIN_HIZ 0x1
> -
> -static const char *pin_state[] = {"low", "high"};
> +QEMU_BUILD_BUG_ON(GPIO_PIN_STATE_LOW != 0x0);
> +QEMU_BUILD_BUG_ON(GPIO_PIN_STATE_HIGH != 0x1);
>
> static void pca9554_update_pin_input(PCA9554State *s)
> {
> @@ -73,7 +72,7 @@ static void pca9554_update_pin_input(PCA9554State *s)
> * Input: the pin is Hi-Z with a pull-up, so it reads high
> * unless an external device drives it low.
> */
> - if (s->ext_state[i] == PCA9554_PIN_LOW) {
> + if (s->ext_state[i] == GPIO_PIN_STATE_LOW) {
> s->regs[PCA9554_INPUT] &= ~bit_mask;
> } else {
> s->regs[PCA9554_INPUT] |= bit_mask;
> @@ -175,7 +174,7 @@ static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
> {
> PCA9554State *s = PCA9554_BASE(obj);
> int pin, rc;
> - uint8_t state;
> + GpioPinState state;
>
> rc = sscanf(name, "pin%2d", &pin);
> if (rc != 1) {
> @@ -194,7 +193,7 @@ static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
> * holds the wire level regardless of the configured direction.
> */
> state = (s->regs[PCA9554_INPUT] >> pin) & 0x1;
> - visit_type_str(v, name, (char **)&pin_state[state], errp);
> + visit_type_GpioPinState(v, name, &state, errp);
> }
>
> static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
> @@ -202,10 +201,10 @@ static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
> {
> PCA9554State *s = PCA9554_BASE(obj);
> int pin, rc, val;
> - uint8_t state, mask;
> - g_autofree char *state_str = NULL;
> + uint8_t mask;
> + GpioPinState state;
>
> - if (!visit_type_str(v, name, &state_str, errp)) {
> + if (!visit_type_GpioPinState(v, name, &state, errp)) {
> return;
> }
> rc = sscanf(name, "pin%2d", &pin);
> @@ -218,16 +217,6 @@ static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
> return;
> }
>
> - for (state = 0; state < ARRAY_SIZE(pin_state); state++) {
> - if (!strcmp(state_str, pin_state[state])) {
> - break;
> - }
> - }
> - if (state >= ARRAY_SIZE(pin_state)) {
> - error_setg(errp, "%s invalid pin state %s", __func__, state_str);
> - return;
> - }
> -
> if (s->hw_dir) {
> /* Warn and ignore if the guest has configured this pin as output */
> if (!((s->regs[PCA9554_CONFIG] >> pin) & 0x1)) {
> @@ -238,13 +227,13 @@ static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
> return;
> }
> /* Drive the external input level */
> - pca9554_set_ext_state(s, pin, state != PCA9554_PIN_LOW);
> + pca9554_set_ext_state(s, pin, state != GPIO_PIN_STATE_LOW);
> } else {
> /* Legacy behavior: force output mode and drive */
> /* First, modify the output register bit */
> val = pca9554_read(s, PCA9554_OUTPUT);
> mask = 0x1 << pin;
> - if (state == PCA9554_PIN_LOW) {
> + if (state == GPIO_PIN_STATE_LOW) {
> val &= ~(mask);
> } else {
> val |= mask;
> @@ -283,7 +272,7 @@ static void pca9554_reset(DeviceState *dev)
> s->regs[PCA9554_POLARITY] = 0x0; /* No pins are inverted */
> s->regs[PCA9554_CONFIG] = pin_mask; /* All pins are inputs */
>
> - memset(s->ext_state, PCA9554_PIN_HIZ, pc->pin_count);
> + memset(s->ext_state, GPIO_PIN_STATE_HIGH, pc->pin_count);
> pca9554_update_pin_input(s);
>
> s->pointer = 0x0;
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 7/8] hw/gpio/pca955x: Improve @qapi-type reported by qom-list etc.
2026-11-06 11:04 ` [PATCH 7/8] hw/gpio/pca955x: Improve @qapi-type reported by qom-list etc Markus Armbruster
@ 2026-11-06 20:21 ` Miles Glenn
2026-11-09 9:06 ` Mark Cave-Ayland
1 sibling, 0 replies; 35+ messages in thread
From: Miles Glenn @ 2026-11-06 20:21 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: marcandre.lureau, qemu-ppc, qemu-arm, mark.caveayland
Reviewed-by: Glenn Miles <milesg@linux.ibm.com>
Thanks,
Glenn
On Fri, 2026-11-06 at 12:04 +0100, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/gpio/pca9552.c | 6 +++---
> hw/gpio/pca9554.c | 4 ++--
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/gpio/pca9552.c b/hw/gpio/pca9552.c
> index 99c4e0c0e2..f97ad6d083 100644
> --- a/hw/gpio/pca9552.c
> +++ b/hw/gpio/pca9552.c
> @@ -23,7 +23,7 @@
> #include "hw/core/irq.h"
> #include "migration/vmstate.h"
> #include "qapi/error.h"
> -#include "qapi/qapi-builtin-type-infos.h"
> +#include "qapi/qapi-type-infos-machine.h"
> #include "qapi/qapi-visit-machine.h"
> #include "qapi/visitor.h"
> #include "trace.h"
> @@ -547,14 +547,14 @@ static void pca955x_pin_init(PCA955xClass *k)
> /* LED variant: expose the LED selector state as led%d. */
> name = g_strdup_printf("led%d", ix);
> object_class_property_add(OBJECT_CLASS(k), name,
> - "str", &str_type_info,
> + "str", &Pca9552LedState_type_info,
> pca955x_get_led, pca955x_set_led,
> NULL, NULL);
> } else {
> /* GPIO variant: expose the pin logic level as pin%d. */
> name = g_strdup_printf("pin%d", ix);
> object_class_property_add(OBJECT_CLASS(k), name,
> - "str", &str_type_info,
> + "str", &GpioPinState_type_info,
> pca955x_get_pin, pca955x_set_pin,
> NULL, NULL);
> }
> diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
> index 7bbfd5ed5f..e2d481866f 100644
> --- a/hw/gpio/pca9554.c
> +++ b/hw/gpio/pca9554.c
> @@ -17,7 +17,7 @@
> #include "hw/core/irq.h"
> #include "migration/vmstate.h"
> #include "qapi/error.h"
> -#include "qapi/qapi-builtin-type-infos.h"
> +#include "qapi/qapi-type-infos-machine.h"
> #include "qapi/qapi-visit-machine.h"
> #include "qapi/visitor.h"
> #include "trace.h"
> @@ -288,7 +288,7 @@ static void pca9554_pin_init(PCA9554Class *pc)
>
> name = g_strdup_printf("pin%d", pin);
> object_class_property_add(OBJECT_CLASS(pc), name,
> - "str", &str_type_info,
> + "str", &GpioPinState_type_info,
> pca9554_get_pin, pca9554_set_pin,
> NULL, NULL);
> g_free(name);
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 8/8] hw/gpio/pca955x: Simplify pin and led getters and setters
2026-11-06 11:04 ` [PATCH 8/8] hw/gpio/pca955x: Simplify pin and led getters and setters Markus Armbruster
@ 2026-11-06 20:21 ` Miles Glenn
2026-11-09 9:09 ` Mark Cave-Ayland
1 sibling, 0 replies; 35+ messages in thread
From: Miles Glenn @ 2026-11-06 20:21 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: marcandre.lureau, qemu-ppc, qemu-arm, mark.caveayland
Reviewed-by: Glenn Miles <milesg@linux.ibm.com>
Thanks,
Glenn
On Fri, 2026-11-06 at 12:04 +0100, Markus Armbruster wrote:
> Pass the index via object_class_property_add() parameter @opaque
> instead extracting it from the property name.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/gpio/pca9552.c | 60 ++++++++++++++---------------------------------
> hw/gpio/pca9554.c | 30 +++++++-----------------
> 2 files changed, 25 insertions(+), 65 deletions(-)
>
> diff --git a/hw/gpio/pca9552.c b/hw/gpio/pca9552.c
> index f97ad6d083..c8d0e9a16d 100644
> --- a/hw/gpio/pca9552.c
> +++ b/hw/gpio/pca9552.c
> @@ -360,18 +360,12 @@ static void pca955x_get_led(Object *obj, Visitor *v, const char *name,
> {
> PCA955xClass *k = PCA955X_GET_CLASS(obj);
> PCA955xState *s = PCA955X(obj);
> - int led, rc, reg;
> + uintptr_t led = (uintptr_t)opaque;
> + int reg;
> Pca9552LedState state;
>
> - rc = sscanf(name, "led%2d", &led);
> - if (rc != 1) {
> - error_setg(errp, "%s: error reading %s", __func__, name);
> - return;
> - }
> - if (led < 0 || led >= k->pin_count) {
> - error_setg(errp, "%s: invalid led %s", __func__, name);
> - return;
> - }
> + assert(led < k->pin_count);
> +
> /*
> * Get the LSx register as the qom interface should expose the device
> * state, not the modeled 'input line' behaviour which would come from
> @@ -397,21 +391,15 @@ static void pca955x_set_led(Object *obj, Visitor *v, const char *name,
> {
> PCA955xClass *k = PCA955X_GET_CLASS(obj);
> PCA955xState *s = PCA955X(obj);
> - int led, rc, reg, val;
> + uintptr_t led = (uintptr_t)opaque;
> + int reg, val;
> Pca9552LedState state;
>
> + assert(led < k->pin_count);
> +
> if (!visit_type_Pca9552LedState(v, name, &state, errp)) {
> return;
> }
> - rc = sscanf(name, "led%2d", &led);
> - if (rc != 1) {
> - error_setg(errp, "%s: error reading %s", __func__, name);
> - return;
> - }
> - if (led < 0 || led >= k->pin_count) {
> - error_setg(errp, "%s: invalid led %s", __func__, name);
> - return;
> - }
>
> reg = PCA9552_LS0 + led / 4;
> val = pca955x_read(s, reg);
> @@ -426,19 +414,11 @@ static void pca955x_get_pin(Object *obj, Visitor *v, const char *name,
> {
> PCA955xClass *k = PCA955X_GET_CLASS(obj);
> PCA955xState *s = PCA955X(obj);
> - int pin, rc;
> + uintptr_t pin = (uintptr_t)opaque;
> uint8_t input_reg;
> GpioPinState state;
>
> - rc = sscanf(name, "pin%2d", &pin);
> - if (rc != 1) {
> - error_setg(errp, "%s: error reading %s", __func__, name);
> - return;
> - }
> - if (pin < 0 || pin >= k->pin_count) {
> - error_setg(errp, "%s invalid pin %s", __func__, name);
> - return;
> - }
> + assert(pin < k->pin_count);
>
> /*
> * Report the raw pin logic level; polarity inversion is a read-time
> @@ -454,28 +434,22 @@ static void pca955x_set_pin(Object *obj, Visitor *v, const char *name,
> {
> PCA955xClass *k = PCA955X_GET_CLASS(obj);
> PCA955xState *s = PCA955X(obj);
> - int pin, rc;
> + uintptr_t pin = (uintptr_t)opaque;
> GpioPinState state;
> uint8_t config_reg;
>
> + assert(pin < k->pin_count);
> +
> if (!visit_type_GpioPinState(v, name, &state, errp)) {
> return;
> }
> - rc = sscanf(name, "pin%2d", &pin);
> - if (rc != 1) {
> - error_setg(errp, "%s: error reading %s", __func__, name);
> - return;
> - }
> - if (pin < 0 || pin >= k->pin_count) {
> - error_setg(errp, "%s invalid pin %s", __func__, name);
> - return;
> - }
>
> /* Only input-configured pins can be driven by an external device. */
> config_reg = PCA9535_CONFIG0 + (pin / 8);
> if (!((s->regs[config_reg] >> (pin % 8)) & 0x1)) {
> qemu_log_mask(LOG_UNIMP,
> - "%s: pin %d is configured as output, ignoring set\n",
> + "%s: pin %" PRIdPTR " is configured as output, "
> + "ignoring set\n",
> s->description, pin);
> return;
> }
> @@ -549,14 +523,14 @@ static void pca955x_pin_init(PCA955xClass *k)
> object_class_property_add(OBJECT_CLASS(k), name,
> "str", &Pca9552LedState_type_info,
> pca955x_get_led, pca955x_set_led,
> - NULL, NULL);
> + NULL, (void *)(uintptr_t)ix);
> } else {
> /* GPIO variant: expose the pin logic level as pin%d. */
> name = g_strdup_printf("pin%d", ix);
> object_class_property_add(OBJECT_CLASS(k), name,
> "str", &GpioPinState_type_info,
> pca955x_get_pin, pca955x_set_pin,
> - NULL, NULL);
> + NULL, (void *)(uintptr_t)ix);
> }
> g_free(name);
> }
> diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
> index e2d481866f..d987b16cd0 100644
> --- a/hw/gpio/pca9554.c
> +++ b/hw/gpio/pca9554.c
> @@ -173,18 +173,10 @@ static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> PCA9554State *s = PCA9554_BASE(obj);
> - int pin, rc;
> + uintptr_t pin = (uintptr_t)opaque;
> GpioPinState state;
>
> - rc = sscanf(name, "pin%2d", &pin);
> - if (rc != 1) {
> - error_setg(errp, "%s: error reading %s", __func__, name);
> - return;
> - }
> - if (pin < 0 || pin >= PCA9554_BASE_GET_CLASS(s)->pin_count) {
> - error_setg(errp, "%s invalid pin %s", __func__, name);
> - return;
> - }
> + assert(pin < PCA9554_BASE_GET_CLASS(s)->pin_count);
>
> /*
> * Report the physical pin level. The input register is kept in sync by
> @@ -200,28 +192,22 @@ static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> PCA9554State *s = PCA9554_BASE(obj);
> - int pin, rc, val;
> + uintptr_t pin = (uintptr_t)opaque;
> + int val;
> uint8_t mask;
> GpioPinState state;
>
> + assert(pin < PCA9554_BASE_GET_CLASS(s)->pin_count);
> +
> if (!visit_type_GpioPinState(v, name, &state, errp)) {
> return;
> }
> - rc = sscanf(name, "pin%2d", &pin);
> - if (rc != 1) {
> - error_setg(errp, "%s: error reading %s", __func__, name);
> - return;
> - }
> - if (pin < 0 || pin >= PCA9554_BASE_GET_CLASS(s)->pin_count) {
> - error_setg(errp, "%s invalid pin %s", __func__, name);
> - return;
> - }
>
> if (s->hw_dir) {
> /* Warn and ignore if the guest has configured this pin as output */
> if (!((s->regs[PCA9554_CONFIG] >> pin) & 0x1)) {
> qemu_log_mask(LOG_UNIMP,
> - "%s: pin %d is configured as output, "
> + "%s: pin %" PRIdPTR " is configured as output, "
> "ignoring external set\n",
> s->description, pin);
> return;
> @@ -290,7 +276,7 @@ static void pca9554_pin_init(PCA9554Class *pc)
> object_class_property_add(OBJECT_CLASS(pc), name,
> "str", &GpioPinState_type_info,
> pca9554_get_pin, pca9554_set_pin,
> - NULL, NULL);
> + NULL, (void *)(uintptr_t)pin);
> g_free(name);
> }
> }
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/8] hw/gpio/pca9552: Convert pin and led properties to class properties
2026-11-06 11:04 ` [PATCH 1/8] hw/gpio/pca9552: Convert pin and led properties to class properties Markus Armbruster
2026-11-06 20:17 ` Miles Glenn
@ 2026-11-09 8:51 ` Mark Cave-Ayland
1 sibling, 0 replies; 35+ messages in thread
From: Mark Cave-Ayland @ 2026-11-09 8:51 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: marcandre.lureau, milesg, qemu-ppc, qemu-arm
On 06/11/2026 11:04, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/gpio/pca9552.c | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/hw/gpio/pca9552.c b/hw/gpio/pca9552.c
> index 90ccf5170e..65a718ef31 100644
> --- a/hw/gpio/pca9552.c
> +++ b/hw/gpio/pca9552.c
> @@ -557,10 +557,8 @@ static void pca9535_reset_hold(Object *obj, ResetType type)
> s->len = 0;
> }
>
> -static void pca955x_initfn(Object *obj)
> +static void pca955x_pin_init(PCA955xClass *k)
> {
> - PCA955xClass *k = PCA955X_GET_CLASS(obj);
> -
> assert(k->pin_count <= PCA955X_PIN_COUNT_MAX);
> for (int ix = 0; ix < k->pin_count; ix++) {
> char *name;
> @@ -568,13 +566,17 @@ static void pca955x_initfn(Object *obj)
> if (k->has_led_support) {
> /* LED variant: expose the LED selector state as led%d. */
> name = g_strdup_printf("led%d", ix);
> - object_property_add(obj, name, "str", &str_type_info,
> - pca955x_get_led, pca955x_set_led, NULL, NULL);
> + object_class_property_add(OBJECT_CLASS(k), name,
> + "str", &str_type_info,
> + pca955x_get_led, pca955x_set_led,
> + NULL, NULL);
> } else {
> /* GPIO variant: expose the pin logic level as pin%d. */
> name = g_strdup_printf("pin%d", ix);
> - object_property_add(obj, name, "str", &str_type_info,
> - pca955x_get_pin, pca955x_set_pin, NULL, NULL);
> + object_class_property_add(OBJECT_CLASS(k), name,
> + "str", &str_type_info,
> + pca955x_get_pin, pca955x_set_pin,
> + NULL, NULL);
> }
> g_free(name);
> }
> @@ -640,6 +642,7 @@ static void pca9552_class_init(ObjectClass *oc, const void *data)
> pc->max_reg = PCA9552_LS3;
> pc->pin_count = 16;
> pc->has_led_support = true;
> + pca955x_pin_init(pc);
> }
>
> static void pca95x5_class_init(ObjectClass *oc, const void *data)
> @@ -653,13 +656,13 @@ static void pca95x5_class_init(ObjectClass *oc, const void *data)
> pc->max_reg = PCA9535_CONFIG1;
> pc->pin_count = 16;
> pc->has_led_support = false;
> + pca955x_pin_init(pc);
> }
>
> static const TypeInfo pca955x_types[] = {
> {
> .name = TYPE_PCA955X,
> .parent = TYPE_I2C_SLAVE,
> - .instance_init = pca955x_initfn,
> .instance_size = sizeof(PCA955xState),
> .class_init = pca955x_class_init,
> .class_size = sizeof(PCA955xClass),
Reviewed-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
ATB,
Mark.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/8] hw/gpio/pca9554: Move most code from .h to .c
2026-11-06 11:04 ` [PATCH 2/8] hw/gpio/pca9554: Move most code from .h to .c Markus Armbruster
2026-11-06 20:18 ` Miles Glenn
@ 2026-11-09 8:55 ` Mark Cave-Ayland
2026-11-09 9:40 ` Markus Armbruster
1 sibling, 1 reply; 35+ messages in thread
From: Mark Cave-Ayland @ 2026-11-09 8:55 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: marcandre.lureau, milesg, qemu-ppc, qemu-arm
On 06/11/2026 11:04, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> include/hw/gpio/pca9554.h | 25 -------------------------
> hw/gpio/pca9554.c | 24 ++++++++++++++++++++++++
> 2 files changed, 24 insertions(+), 25 deletions(-)
>
> diff --git a/include/hw/gpio/pca9554.h b/include/hw/gpio/pca9554.h
> index ac835371aa..1b33a8a752 100644
> --- a/include/hw/gpio/pca9554.h
> +++ b/include/hw/gpio/pca9554.h
> @@ -8,32 +8,7 @@
> #ifndef PCA9554_H
> #define PCA9554_H
>
> -#include "hw/i2c/i2c.h"
> -#include "qom/object.h"
> -
> #define TYPE_PCA9554 "pca9554"
> #define TYPE_PCA9536 "pca9536"
> -typedef struct PCA9554State PCA9554State;
> -DECLARE_INSTANCE_CHECKER(PCA9554State, PCA9554,
> - TYPE_PCA9554)
> -
> -#define PCA9554_NR_REGS 4
> -#define PCA9554_PIN_COUNT 8
> -#define PCA9536_PIN_COUNT 4
> -
> -struct PCA9554State {
> - /*< private >*/
> - I2CSlave i2c;
> - /*< public >*/
> -
> - uint8_t len;
> - uint8_t pointer;
> -
> - uint8_t regs[PCA9554_NR_REGS];
> - qemu_irq gpio_out[PCA9554_PIN_COUNT];
> - uint8_t ext_state[PCA9554_PIN_COUNT];
> - char *description; /* For debugging purpose only */
> - bool hw_dir; /* Honor pin direction */
> -};
>
> #endif
Is there anything left in pca9554.h at this point?
> diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
> index 07bb00babf..e5a1932c89 100644
> --- a/hw/gpio/pca9554.c
> +++ b/hw/gpio/pca9554.c
> @@ -13,6 +13,7 @@
> #include "hw/core/qdev-properties.h"
> #include "hw/gpio/pca9554.h"
> #include "hw/gpio/pca9554_regs.h"
> +#include "hw/i2c/i2c.h"
> #include "hw/core/irq.h"
> #include "migration/vmstate.h"
> #include "qapi/error.h"
> @@ -21,6 +22,29 @@
> #include "trace.h"
> #include "qom/object.h"
>
> +typedef struct PCA9554State PCA9554State;
> +DECLARE_INSTANCE_CHECKER(PCA9554State, PCA9554,
> + TYPE_PCA9554)
> +
> +#define PCA9554_NR_REGS 4
> +#define PCA9554_PIN_COUNT 8
> +#define PCA9536_PIN_COUNT 4
> +
> +struct PCA9554State {
> + /*< private >*/
> + I2CSlave i2c;
> + /*< public >*/
> +
> + uint8_t len;
> + uint8_t pointer;
> +
> + uint8_t regs[PCA9554_NR_REGS];
> + qemu_irq gpio_out[PCA9554_PIN_COUNT];
> + uint8_t ext_state[PCA9554_PIN_COUNT];
> + char *description; /* For debugging purpose only */
> + bool hw_dir; /* Honor pin direction */
> +};
> +
> struct PCA9554Class {
> /*< private >*/
> I2CSlaveClass parent_class;
Otherwise:
Reviewed-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
ATB,
Mark.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/8] hw/gpio/pca9554: Simplify QOM type creation
2026-11-06 11:04 ` [PATCH 3/8] hw/gpio/pca9554: Simplify QOM type creation Markus Armbruster
2026-11-06 20:19 ` Miles Glenn
@ 2026-11-09 8:56 ` Mark Cave-Ayland
1 sibling, 0 replies; 35+ messages in thread
From: Mark Cave-Ayland @ 2026-11-09 8:56 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: marcandre.lureau, milesg, qemu-ppc, qemu-arm
On 06/11/2026 11:04, Markus Armbruster wrote:
> According to object.h, direct usage of DECLARE_INSTANCE_CHECKERS()
> should be avoided, and the complete OBJECT_DECLARE_TYPE macro is
> recommended instead. So use that.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/gpio/pca9554.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
> index e5a1932c89..caa73f63c9 100644
> --- a/hw/gpio/pca9554.c
> +++ b/hw/gpio/pca9554.c
> @@ -22,14 +22,12 @@
> #include "trace.h"
> #include "qom/object.h"
>
> -typedef struct PCA9554State PCA9554State;
> -DECLARE_INSTANCE_CHECKER(PCA9554State, PCA9554,
> - TYPE_PCA9554)
> -
> #define PCA9554_NR_REGS 4
> #define PCA9554_PIN_COUNT 8
> #define PCA9536_PIN_COUNT 4
>
> +OBJECT_DECLARE_TYPE(PCA9554State, PCA9554Class, PCA9554)
> +
> struct PCA9554State {
> /*< private >*/
> I2CSlave i2c;
> @@ -52,10 +50,6 @@ struct PCA9554Class {
>
> uint8_t pin_count;
> };
> -typedef struct PCA9554Class PCA9554Class;
> -
> -DECLARE_CLASS_CHECKERS(PCA9554Class, PCA9554,
> - TYPE_PCA9554)
>
> #define PCA9554_PIN_LOW 0x0
> #define PCA9554_PIN_HIZ 0x1
Reviewed-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
ATB,
Mark.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 4/8] hw/gpio/pca9554: New abstract base "pca9554-base"
2026-11-06 11:04 ` [PATCH 4/8] hw/gpio/pca9554: New abstract base "pca9554-base" Markus Armbruster
2026-11-06 20:19 ` Miles Glenn
@ 2026-11-09 9:02 ` Mark Cave-Ayland
2026-11-09 10:17 ` Mark Cave-Ayland
2026-11-09 10:24 ` Markus Armbruster
1 sibling, 2 replies; 35+ messages in thread
From: Mark Cave-Ayland @ 2026-11-09 9:02 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: marcandre.lureau, milesg, qemu-ppc, qemu-arm
On 06/11/2026 11:04, Markus Armbruster wrote:
> Commit 5eceb48bbe86 (hw/gpio: pca9554: add PCA9536 support) made type
> "pca9535" a child of concrete type "pca9555". This is inconvenient
> for the next commit. Make them both children of new abstract type
> "pca9554-base".
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> include/hw/gpio/pca9554.h | 1 +
> hw/gpio/pca9554.c | 55 +++++++++++++++++++++++----------------
> 2 files changed, 33 insertions(+), 23 deletions(-)
>
> diff --git a/include/hw/gpio/pca9554.h b/include/hw/gpio/pca9554.h
> index 1b33a8a752..0d841ff017 100644
> --- a/include/hw/gpio/pca9554.h
> +++ b/include/hw/gpio/pca9554.h
> @@ -8,6 +8,7 @@
> #ifndef PCA9554_H
> #define PCA9554_H
>
> +#define TYPE_PCA9554_BASE "pca9554-base"
> #define TYPE_PCA9554 "pca9554"
> #define TYPE_PCA9536 "pca9536"
>
> diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
> index caa73f63c9..1362b605c2 100644
> --- a/hw/gpio/pca9554.c
> +++ b/hw/gpio/pca9554.c
> @@ -26,7 +26,7 @@
> #define PCA9554_PIN_COUNT 8
> #define PCA9536_PIN_COUNT 4
>
> -OBJECT_DECLARE_TYPE(PCA9554State, PCA9554Class, PCA9554)
> +OBJECT_DECLARE_TYPE(PCA9554State, PCA9554Class, PCA9554_BASE)
>
> struct PCA9554State {
> /*< private >*/
> @@ -58,7 +58,7 @@ static const char *pin_state[] = {"low", "high"};
>
> static void pca9554_update_pin_input(PCA9554State *s)
> {
> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
> int i;
> uint8_t config = s->regs[PCA9554_CONFIG];
> uint8_t output = s->regs[PCA9554_OUTPUT];
> @@ -110,7 +110,7 @@ static uint8_t pca9554_read(PCA9554State *s, uint8_t reg)
>
> static void pca9554_write(PCA9554State *s, uint8_t reg, uint8_t data)
> {
> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
> uint8_t pin_mask = (1 << pc->pin_count) - 1;
>
> /* Variants narrower than 8 bits ignore the unimplemented upper pins. */
> @@ -134,14 +134,14 @@ static void pca9554_write(PCA9554State *s, uint8_t reg, uint8_t data)
>
> static uint8_t pca9554_recv(I2CSlave *i2c)
> {
> - PCA9554State *s = PCA9554(i2c);
> + PCA9554State *s = PCA9554_BASE(i2c);
>
> return pca9554_read(s, s->pointer & 0x3);
> }
>
> static int pca9554_send(I2CSlave *i2c, uint8_t data)
> {
> - PCA9554State *s = PCA9554(i2c);
> + PCA9554State *s = PCA9554_BASE(i2c);
>
> /* First byte sent by is the register address */
> if (s->len == 0) {
> @@ -156,7 +156,7 @@ static int pca9554_send(I2CSlave *i2c, uint8_t data)
>
> static int pca9554_event(I2CSlave *i2c, enum i2c_event event)
> {
> - PCA9554State *s = PCA9554(i2c);
> + PCA9554State *s = PCA9554_BASE(i2c);
>
> s->len = 0;
> return 0;
> @@ -173,7 +173,7 @@ static void pca9554_set_ext_state(PCA9554State *s, int pin, int level)
> static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> - PCA9554State *s = PCA9554(obj);
> + PCA9554State *s = PCA9554_BASE(obj);
> int pin, rc;
> uint8_t state;
>
> @@ -182,7 +182,7 @@ static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
> error_setg(errp, "%s: error reading %s", __func__, name);
> return;
> }
> - if (pin < 0 || pin >= PCA9554_GET_CLASS(s)->pin_count) {
> + if (pin < 0 || pin >= PCA9554_BASE_GET_CLASS(s)->pin_count) {
> error_setg(errp, "%s invalid pin %s", __func__, name);
> return;
> }
> @@ -200,7 +200,7 @@ static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
> static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> - PCA9554State *s = PCA9554(obj);
> + PCA9554State *s = PCA9554_BASE(obj);
> int pin, rc, val;
> uint8_t state, mask;
> g_autofree char *state_str = NULL;
> @@ -213,7 +213,7 @@ static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
> error_setg(errp, "%s: error reading %s", __func__, name);
> return;
> }
> - if (pin < 0 || pin >= PCA9554_GET_CLASS(s)->pin_count) {
> + if (pin < 0 || pin >= PCA9554_BASE_GET_CLASS(s)->pin_count) {
> error_setg(errp, "%s invalid pin %s", __func__, name);
> return;
> }
> @@ -274,8 +274,8 @@ static const VMStateDescription pca9554_vmstate = {
>
> static void pca9554_reset(DeviceState *dev)
> {
> - PCA9554State *s = PCA9554(dev);
> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
> + PCA9554State *s = PCA9554_BASE(dev);
> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
> uint8_t pin_mask = (1 << pc->pin_count) - 1;
>
> s->regs[PCA9554_INPUT] = pin_mask;
> @@ -292,7 +292,7 @@ static void pca9554_reset(DeviceState *dev)
>
> static void pca9554_initfn(Object *obj)
> {
> - PCA9554Class *pc = PCA9554_GET_CLASS(obj);
> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(obj);
> int pin;
>
> for (pin = 0; pin < pc->pin_count; pin++) {
> @@ -308,8 +308,8 @@ static void pca9554_initfn(Object *obj)
>
> static void pca9554_gpio_in_handler(void *opaque, int pin, int level)
> {
> - PCA9554State *s = PCA9554(opaque);
> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
> + PCA9554State *s = PCA9554_BASE(opaque);
> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
>
> assert((pin >= 0) && (pin < pc->pin_count));
> pca9554_set_ext_state(s, pin, level);
> @@ -317,8 +317,8 @@ static void pca9554_gpio_in_handler(void *opaque, int pin, int level)
>
> static void pca9554_realize(DeviceState *dev, Error **errp)
> {
> - PCA9554State *s = PCA9554(dev);
> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
> + PCA9554State *s = PCA9554_BASE(dev);
> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
>
> if (!s->description) {
> s->description = g_strdup(object_get_typename(OBJECT(dev)));
> @@ -333,11 +333,10 @@ static const Property pca9554_properties[] = {
> DEFINE_PROP_BOOL("hw-dir", PCA9554State, hw_dir, false),
> };
>
> -static void pca9554_class_init(ObjectClass *klass, const void *data)
> +static void pca9554_base_class_init(ObjectClass *klass, const void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
> - PCA9554Class *pc = PCA9554_CLASS(klass);
>
> k->event = pca9554_event;
> k->recv = pca9554_recv;
> @@ -346,26 +345,36 @@ static void pca9554_class_init(ObjectClass *klass, const void *data)
> device_class_set_legacy_reset(dc, pca9554_reset);
> dc->vmsd = &pca9554_vmstate;
> device_class_set_props(dc, pca9554_properties);
> +}
> +
> +static void pca9554_class_init(ObjectClass *klass, const void *data)
> +{
> + PCA9554Class *pc = PCA9554_BASE_CLASS(klass);
>
> pc->pin_count = PCA9554_PIN_COUNT;
> }
>
> static void pca9536_class_init(ObjectClass *klass, const void *data)
> {
> - PCA9554Class *pc = PCA9554_CLASS(klass);
> + PCA9554Class *pc = PCA9554_BASE_CLASS(klass);
>
> pc->pin_count = PCA9536_PIN_COUNT;
> }
>
> static const TypeInfo pca9554_types[] = {
> {
> - .name = TYPE_PCA9554,
> + .name = TYPE_PCA9554_BASE,
> .parent = TYPE_I2C_SLAVE,
> .instance_init = pca9554_initfn,
> .instance_size = sizeof(PCA9554State),
> - .class_init = pca9554_class_init,
> + .class_init = pca9554_base_class_init,
> .class_size = sizeof(PCA9554Class),
> - .abstract = false,
> + .abstract = true,
> + },
> + {
> + .name = TYPE_PCA9554,
> + .parent = TYPE_PCA9554_BASE,
> + .class_init = pca9554_class_init,
> },
> {
> .name = TYPE_PCA9536,
The basic change looks good to me, however I found it tricky to follow
given that PCA9554Class is now related to PCA9554_BASE_CLASS() and
PCA9554State is now related to PCA9554_BASE().
Would it make sense to rename PCA9554Class to PCA9554BaseClass and
PCA9554State to PCA9554BaseState here? I think it would help make the
hierarchy easier to follow.
ATB,
Mark.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 5/8] hw/gpio/pca9554: Convert pin properties to class properties
2026-11-06 11:04 ` [PATCH 5/8] hw/gpio/pca9554: Convert pin properties to class properties Markus Armbruster
2026-11-06 20:20 ` Miles Glenn
@ 2026-11-09 9:03 ` Mark Cave-Ayland
1 sibling, 0 replies; 35+ messages in thread
From: Mark Cave-Ayland @ 2026-11-09 9:03 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: marcandre.lureau, milesg, qemu-ppc, qemu-arm
On 06/11/2026 11:04, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/gpio/pca9554.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
> index 1362b605c2..ed304e4639 100644
> --- a/hw/gpio/pca9554.c
> +++ b/hw/gpio/pca9554.c
> @@ -290,18 +290,18 @@ static void pca9554_reset(DeviceState *dev)
> s->len = 0;
> }
>
> -static void pca9554_initfn(Object *obj)
> +static void pca9554_pin_init(PCA9554Class *pc)
> {
> - PCA9554Class *pc = PCA9554_BASE_GET_CLASS(obj);
> int pin;
>
> for (pin = 0; pin < pc->pin_count; pin++) {
> char *name;
>
> name = g_strdup_printf("pin%d", pin);
> - object_property_add(obj, name, "str", &str_type_info,
> - pca9554_get_pin, pca9554_set_pin,
> - NULL, NULL);
> + object_class_property_add(OBJECT_CLASS(pc), name,
> + "str", &str_type_info,
> + pca9554_get_pin, pca9554_set_pin,
> + NULL, NULL);
> g_free(name);
> }
> }
> @@ -352,6 +352,7 @@ static void pca9554_class_init(ObjectClass *klass, const void *data)
> PCA9554Class *pc = PCA9554_BASE_CLASS(klass);
>
> pc->pin_count = PCA9554_PIN_COUNT;
> + pca9554_pin_init(pc);
> }
>
> static void pca9536_class_init(ObjectClass *klass, const void *data)
> @@ -359,13 +360,13 @@ static void pca9536_class_init(ObjectClass *klass, const void *data)
> PCA9554Class *pc = PCA9554_BASE_CLASS(klass);
>
> pc->pin_count = PCA9536_PIN_COUNT;
> + pca9554_pin_init(pc);
> }
>
> static const TypeInfo pca9554_types[] = {
> {
> .name = TYPE_PCA9554_BASE,
> .parent = TYPE_I2C_SLAVE,
> - .instance_init = pca9554_initfn,
> .instance_size = sizeof(PCA9554State),
> .class_init = pca9554_base_class_init,
> .class_size = sizeof(PCA9554Class),
> @@ -378,7 +379,7 @@ static const TypeInfo pca9554_types[] = {
> },
> {
> .name = TYPE_PCA9536,
> - .parent = TYPE_PCA9554,
> + .parent = TYPE_PCA9554_BASE,
> .class_init = pca9536_class_init,
> }
> };
Reviewed-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
ATB,
Mark.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 6/8] hw/gpio/pca955x: use QAPI enums for led and pin properties
2026-11-06 11:04 ` [PATCH 6/8] hw/gpio/pca955x: use QAPI enums for led and pin properties Markus Armbruster
2026-11-06 20:20 ` Miles Glenn
@ 2026-11-09 9:05 ` Mark Cave-Ayland
2026-11-09 10:20 ` Markus Armbruster
1 sibling, 1 reply; 35+ messages in thread
From: Mark Cave-Ayland @ 2026-11-09 9:05 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: marcandre.lureau, milesg, qemu-ppc, qemu-arm
On 06/11/2026 11:04, Markus Armbruster wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Replace hand-rolled string lookup tables with QAPI enums
> Pca9552LedState and GpioPinState for the led%d and pin%d QOM
> properties. This lets QAPI handle string-to-enum conversion in the
> visitor, removing the manual string matching in the setters.
>
> Add QEMU_BUILD_BUG_ON guards to ensure the QAPI-generated enum values
> stay in sync with the hardware register encoding.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> qapi/machine.json | 29 ++++++++++++++++++
> hw/gpio/pca9552.c | 76 +++++++++++++++++------------------------------
> hw/gpio/pca9554.c | 35 ++++++++--------------
> 3 files changed, 69 insertions(+), 71 deletions(-)
>
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 18d9dce849..c4c8720909 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -416,6 +416,35 @@
> { 'enum': 'LostTickPolicy',
> 'data': ['discard', 'delay', 'slew' ] }
>
> +##
> +# @GpioPinState:
> +#
> +# @low: Pin low
> +#
> +# @high: Pin high
> +#
> +# Since: 12.0
> +##
> +{ 'enum': 'GpioPinState',
> + 'data': ['low', 'high'] }
> +
> +##
> +# @Pca9552LedState:
> +# LED state for PCA9552.
> +#
> +# @on: LED on
> +#
> +# @off: LED off
> +#
> +# @pwm0: LED controlled by PWM0
> +#
> +# @pwm1: LED controlled by PWM1
> +#
> +# Since: 12.0
> +##
> +{ 'enum': 'Pca9552LedState',
> + 'data': ['on', 'off', 'pwm0', 'pwm1'] }
> +
> ##
> # @PCSouthBridgeOption:
> # South bridge chipset option for PC i440FX machines.
> diff --git a/hw/gpio/pca9552.c b/hw/gpio/pca9552.c
> index 65a718ef31..99c4e0c0e2 100644
> --- a/hw/gpio/pca9552.c
> +++ b/hw/gpio/pca9552.c
> @@ -24,6 +24,7 @@
> #include "migration/vmstate.h"
> #include "qapi/error.h"
> #include "qapi/qapi-builtin-type-infos.h"
> +#include "qapi/qapi-visit-machine.h"
> #include "qapi/visitor.h"
> #include "trace.h"
> #include "qom/object.h"
> @@ -60,16 +61,15 @@ struct PCA955xClass {
> /*
> * Note: The LED_ON and LED_OFF configuration values for the PCA955X
> * chips are the reverse of the PCA953X family of chips.
> + *
> + * The QAPI enums must match the hardware register values.
> */
> -#define PCA9552_LED_ON 0x0
> -#define PCA9552_LED_OFF 0x1
> -#define PCA9552_LED_PWM0 0x2
> -#define PCA9552_LED_PWM1 0x3
> -#define PCA9552_PIN_LOW 0x0
> -#define PCA9552_PIN_HIZ 0x1
> -
> -static const char *led_state[] = {"on", "off", "pwm0", "pwm1"};
> -static const char *pin_state[] = {"low", "high"};
> +QEMU_BUILD_BUG_ON(PCA9552_LED_STATE_ON != 0x0);
> +QEMU_BUILD_BUG_ON(PCA9552_LED_STATE_OFF != 0x1);
> +QEMU_BUILD_BUG_ON(PCA9552_LED_STATE_PWM0 != 0x2);
> +QEMU_BUILD_BUG_ON(PCA9552_LED_STATE_PWM1 != 0x3);
> +QEMU_BUILD_BUG_ON(GPIO_PIN_STATE_LOW != 0x0);
> +QEMU_BUILD_BUG_ON(GPIO_PIN_STATE_HIGH != 0x1);
>
> static uint8_t pca955x_pin_get_config(PCA955xState *s, int pin)
> {
> @@ -143,24 +143,24 @@ static void pca955x_update_pin_input(PCA955xState *s)
> uint8_t config = pca955x_pin_get_config(s, i);
>
> switch (config) {
> - case PCA9552_LED_ON:
> + case PCA9552_LED_STATE_ON:
> /* Pin is set to 0V to turn on LED */
> s->regs[input_reg] &= ~bit_mask;
> break;
> - case PCA9552_LED_OFF:
> + case PCA9552_LED_STATE_OFF:
> /*
> * Pin is set to Hi-Z to turn off LED and
> * pullup sets it to a logical 1 unless
> * external device drives it low.
> */
> - if (s->ext_state[i] == PCA9552_PIN_LOW) {
> + if (s->ext_state[i] == GPIO_PIN_STATE_LOW) {
> s->regs[input_reg] &= ~bit_mask;
> } else {
> s->regs[input_reg] |= bit_mask;
> }
> break;
> - case PCA9552_LED_PWM0:
> - case PCA9552_LED_PWM1:
> + case PCA9552_LED_STATE_PWM0:
> + case PCA9552_LED_STATE_PWM1:
> /* TODO */
> default:
> break;
> @@ -177,7 +177,7 @@ static void pca955x_update_pin_input(PCA955xState *s)
> */
> if (s->regs[config_reg] & bit_mask) {
> /* Input mode - reflect external state */
> - if (s->ext_state[i] == PCA9552_PIN_LOW) {
> + if (s->ext_state[i] == GPIO_PIN_STATE_LOW) {
> s->regs[input_reg] &= ~bit_mask;
> } else {
> s->regs[input_reg] |= bit_mask;
> @@ -361,7 +361,7 @@ static void pca955x_get_led(Object *obj, Visitor *v, const char *name,
> PCA955xClass *k = PCA955X_GET_CLASS(obj);
> PCA955xState *s = PCA955X(obj);
> int led, rc, reg;
> - uint8_t state;
> + Pca9552LedState state;
>
> rc = sscanf(name, "led%2d", &led);
> if (rc != 1) {
> @@ -379,7 +379,7 @@ static void pca955x_get_led(Object *obj, Visitor *v, const char *name,
> */
> reg = PCA9552_LS0 + led / 4;
> state = (pca955x_read(s, reg) >> ((led % 4) * 2)) & 0x3;
> - visit_type_str(v, name, (char **)&led_state[state], errp);
> + visit_type_Pca9552LedState(v, name, &state, errp);
> }
>
> /*
> @@ -398,10 +398,9 @@ static void pca955x_set_led(Object *obj, Visitor *v, const char *name,
> PCA955xClass *k = PCA955X_GET_CLASS(obj);
> PCA955xState *s = PCA955X(obj);
> int led, rc, reg, val;
> - uint8_t state;
> - g_autofree char *state_str = NULL;
> + Pca9552LedState state;
>
> - if (!visit_type_str(v, name, &state_str, errp)) {
> + if (!visit_type_Pca9552LedState(v, name, &state, errp)) {
> return;
> }
> rc = sscanf(name, "led%2d", &led);
> @@ -414,16 +413,6 @@ static void pca955x_set_led(Object *obj, Visitor *v, const char *name,
> return;
> }
>
> - for (state = 0; state < ARRAY_SIZE(led_state); state++) {
> - if (!strcmp(state_str, led_state[state])) {
> - break;
> - }
> - }
> - if (state >= ARRAY_SIZE(led_state)) {
> - error_setg(errp, "%s invalid led state %s", __func__, state_str);
> - return;
> - }
> -
> reg = PCA9552_LS0 + led / 4;
> val = pca955x_read(s, reg);
> val = pca955x_ledsel(val, led % 4, state);
> @@ -438,7 +427,8 @@ static void pca955x_get_pin(Object *obj, Visitor *v, const char *name,
> PCA955xClass *k = PCA955X_GET_CLASS(obj);
> PCA955xState *s = PCA955X(obj);
> int pin, rc;
> - uint8_t input_reg, state;
> + uint8_t input_reg;
> + GpioPinState state;
>
> rc = sscanf(name, "pin%2d", &pin);
> if (rc != 1) {
> @@ -456,7 +446,7 @@ static void pca955x_get_pin(Object *obj, Visitor *v, const char *name,
> */
> input_reg = PCA9535_INPUT0 + (pin / 8);
> state = (s->regs[input_reg] >> (pin % 8)) & 0x1;
> - visit_type_str(v, name, (char **)&pin_state[state], errp);
> + visit_type_GpioPinState(v, name, &state, errp);
> }
>
> static void pca955x_set_pin(Object *obj, Visitor *v, const char *name,
> @@ -465,10 +455,10 @@ static void pca955x_set_pin(Object *obj, Visitor *v, const char *name,
> PCA955xClass *k = PCA955X_GET_CLASS(obj);
> PCA955xState *s = PCA955X(obj);
> int pin, rc;
> - uint8_t state, config_reg;
> - g_autofree char *state_str = NULL;
> + GpioPinState state;
> + uint8_t config_reg;
>
> - if (!visit_type_str(v, name, &state_str, errp)) {
> + if (!visit_type_GpioPinState(v, name, &state, errp)) {
> return;
> }
> rc = sscanf(name, "pin%2d", &pin);
> @@ -481,16 +471,6 @@ static void pca955x_set_pin(Object *obj, Visitor *v, const char *name,
> return;
> }
>
> - for (state = 0; state < ARRAY_SIZE(pin_state); state++) {
> - if (!strcmp(state_str, pin_state[state])) {
> - break;
> - }
> - }
> - if (state >= ARRAY_SIZE(pin_state)) {
> - error_setg(errp, "%s invalid pin state %s", __func__, state_str);
> - return;
> - }
> -
> /* Only input-configured pins can be driven by an external device. */
> config_reg = PCA9535_CONFIG0 + (pin / 8);
> if (!((s->regs[config_reg] >> (pin % 8)) & 0x1)) {
> @@ -500,7 +480,7 @@ static void pca955x_set_pin(Object *obj, Visitor *v, const char *name,
> return;
> }
>
> - pca955x_set_ext_state(s, pin, state != PCA9552_PIN_LOW);
> + pca955x_set_ext_state(s, pin, state != GPIO_PIN_STATE_LOW);
> }
>
> static const VMStateDescription pca9552_vmstate = {
> @@ -530,7 +510,7 @@ static void pca9552_reset_hold(Object *obj, ResetType type)
> s->regs[PCA9552_LS2] = 0x55;
> s->regs[PCA9552_LS3] = 0x55;
>
> - memset(s->ext_state, PCA9552_PIN_HIZ, PCA955X_PIN_COUNT_MAX);
> + memset(s->ext_state, GPIO_PIN_STATE_HIGH, PCA955X_PIN_COUNT_MAX);
> pca955x_update_pin_input(s);
>
> s->pointer = 0xFF;
> @@ -550,7 +530,7 @@ static void pca9535_reset_hold(Object *obj, ResetType type)
> s->regs[PCA9535_CONFIG0] = 0xFF; /* All pins as inputs */
> s->regs[PCA9535_CONFIG1] = 0xFF; /* All pins as inputs */
>
> - memset(s->ext_state, PCA9552_PIN_HIZ, PCA955X_PIN_COUNT_MAX);
> + memset(s->ext_state, GPIO_PIN_STATE_HIGH, PCA955X_PIN_COUNT_MAX);
> pca955x_update_pin_input(s);
>
> s->pointer = 0xFF;
> diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
> index ed304e4639..7bbfd5ed5f 100644
> --- a/hw/gpio/pca9554.c
> +++ b/hw/gpio/pca9554.c
> @@ -18,6 +18,7 @@
> #include "migration/vmstate.h"
> #include "qapi/error.h"
> #include "qapi/qapi-builtin-type-infos.h"
> +#include "qapi/qapi-visit-machine.h"
> #include "qapi/visitor.h"
> #include "trace.h"
> #include "qom/object.h"
> @@ -51,10 +52,8 @@ struct PCA9554Class {
> uint8_t pin_count;
> };
>
> -#define PCA9554_PIN_LOW 0x0
> -#define PCA9554_PIN_HIZ 0x1
> -
> -static const char *pin_state[] = {"low", "high"};
> +QEMU_BUILD_BUG_ON(GPIO_PIN_STATE_LOW != 0x0);
> +QEMU_BUILD_BUG_ON(GPIO_PIN_STATE_HIGH != 0x1);
>
> static void pca9554_update_pin_input(PCA9554State *s)
> {
> @@ -73,7 +72,7 @@ static void pca9554_update_pin_input(PCA9554State *s)
> * Input: the pin is Hi-Z with a pull-up, so it reads high
> * unless an external device drives it low.
> */
> - if (s->ext_state[i] == PCA9554_PIN_LOW) {
> + if (s->ext_state[i] == GPIO_PIN_STATE_LOW) {
> s->regs[PCA9554_INPUT] &= ~bit_mask;
> } else {
> s->regs[PCA9554_INPUT] |= bit_mask;
> @@ -175,7 +174,7 @@ static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
> {
> PCA9554State *s = PCA9554_BASE(obj);
> int pin, rc;
> - uint8_t state;
> + GpioPinState state;
>
> rc = sscanf(name, "pin%2d", &pin);
> if (rc != 1) {
> @@ -194,7 +193,7 @@ static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
> * holds the wire level regardless of the configured direction.
> */
> state = (s->regs[PCA9554_INPUT] >> pin) & 0x1;
> - visit_type_str(v, name, (char **)&pin_state[state], errp);
> + visit_type_GpioPinState(v, name, &state, errp);
> }
>
> static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
> @@ -202,10 +201,10 @@ static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
> {
> PCA9554State *s = PCA9554_BASE(obj);
> int pin, rc, val;
> - uint8_t state, mask;
> - g_autofree char *state_str = NULL;
> + uint8_t mask;
> + GpioPinState state;
>
> - if (!visit_type_str(v, name, &state_str, errp)) {
> + if (!visit_type_GpioPinState(v, name, &state, errp)) {
> return;
> }
> rc = sscanf(name, "pin%2d", &pin);
> @@ -218,16 +217,6 @@ static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
> return;
> }
>
> - for (state = 0; state < ARRAY_SIZE(pin_state); state++) {
> - if (!strcmp(state_str, pin_state[state])) {
> - break;
> - }
> - }
> - if (state >= ARRAY_SIZE(pin_state)) {
> - error_setg(errp, "%s invalid pin state %s", __func__, state_str);
> - return;
> - }
> -
> if (s->hw_dir) {
> /* Warn and ignore if the guest has configured this pin as output */
> if (!((s->regs[PCA9554_CONFIG] >> pin) & 0x1)) {
> @@ -238,13 +227,13 @@ static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
> return;
> }
> /* Drive the external input level */
> - pca9554_set_ext_state(s, pin, state != PCA9554_PIN_LOW);
> + pca9554_set_ext_state(s, pin, state != GPIO_PIN_STATE_LOW);
> } else {
> /* Legacy behavior: force output mode and drive */
> /* First, modify the output register bit */
> val = pca9554_read(s, PCA9554_OUTPUT);
> mask = 0x1 << pin;
> - if (state == PCA9554_PIN_LOW) {
> + if (state == GPIO_PIN_STATE_LOW) {
> val &= ~(mask);
> } else {
> val |= mask;
> @@ -283,7 +272,7 @@ static void pca9554_reset(DeviceState *dev)
> s->regs[PCA9554_POLARITY] = 0x0; /* No pins are inverted */
> s->regs[PCA9554_CONFIG] = pin_mask; /* All pins are inputs */
>
> - memset(s->ext_state, PCA9554_PIN_HIZ, pc->pin_count);
> + memset(s->ext_state, GPIO_PIN_STATE_HIGH, pc->pin_count);
> pca9554_update_pin_input(s);
>
> s->pointer = 0x0;
I'm less confident with the QAPI side with all the recent changes to
give an R-B, however it seems sensible to me. Perhaps an Acked-by if you
feel you need it? Otherwise LGTM.
ATB,
Mark.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 7/8] hw/gpio/pca955x: Improve @qapi-type reported by qom-list etc.
2026-11-06 11:04 ` [PATCH 7/8] hw/gpio/pca955x: Improve @qapi-type reported by qom-list etc Markus Armbruster
2026-11-06 20:21 ` Miles Glenn
@ 2026-11-09 9:06 ` Mark Cave-Ayland
1 sibling, 0 replies; 35+ messages in thread
From: Mark Cave-Ayland @ 2026-11-09 9:06 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: marcandre.lureau, milesg, qemu-ppc, qemu-arm
On 06/11/2026 11:04, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/gpio/pca9552.c | 6 +++---
> hw/gpio/pca9554.c | 4 ++--
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/gpio/pca9552.c b/hw/gpio/pca9552.c
> index 99c4e0c0e2..f97ad6d083 100644
> --- a/hw/gpio/pca9552.c
> +++ b/hw/gpio/pca9552.c
> @@ -23,7 +23,7 @@
> #include "hw/core/irq.h"
> #include "migration/vmstate.h"
> #include "qapi/error.h"
> -#include "qapi/qapi-builtin-type-infos.h"
> +#include "qapi/qapi-type-infos-machine.h"
> #include "qapi/qapi-visit-machine.h"
> #include "qapi/visitor.h"
> #include "trace.h"
> @@ -547,14 +547,14 @@ static void pca955x_pin_init(PCA955xClass *k)
> /* LED variant: expose the LED selector state as led%d. */
> name = g_strdup_printf("led%d", ix);
> object_class_property_add(OBJECT_CLASS(k), name,
> - "str", &str_type_info,
> + "str", &Pca9552LedState_type_info,
> pca955x_get_led, pca955x_set_led,
> NULL, NULL);
> } else {
> /* GPIO variant: expose the pin logic level as pin%d. */
> name = g_strdup_printf("pin%d", ix);
> object_class_property_add(OBJECT_CLASS(k), name,
> - "str", &str_type_info,
> + "str", &GpioPinState_type_info,
> pca955x_get_pin, pca955x_set_pin,
> NULL, NULL);
> }
> diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
> index 7bbfd5ed5f..e2d481866f 100644
> --- a/hw/gpio/pca9554.c
> +++ b/hw/gpio/pca9554.c
> @@ -17,7 +17,7 @@
> #include "hw/core/irq.h"
> #include "migration/vmstate.h"
> #include "qapi/error.h"
> -#include "qapi/qapi-builtin-type-infos.h"
> +#include "qapi/qapi-type-infos-machine.h"
> #include "qapi/qapi-visit-machine.h"
> #include "qapi/visitor.h"
> #include "trace.h"
> @@ -288,7 +288,7 @@ static void pca9554_pin_init(PCA9554Class *pc)
>
> name = g_strdup_printf("pin%d", pin);
> object_class_property_add(OBJECT_CLASS(pc), name,
> - "str", &str_type_info,
> + "str", &GpioPinState_type_info,
> pca9554_get_pin, pca9554_set_pin,
> NULL, NULL);
> g_free(name);
Reviewed-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
ATB,
Mark.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 8/8] hw/gpio/pca955x: Simplify pin and led getters and setters
2026-11-06 11:04 ` [PATCH 8/8] hw/gpio/pca955x: Simplify pin and led getters and setters Markus Armbruster
2026-11-06 20:21 ` Miles Glenn
@ 2026-11-09 9:09 ` Mark Cave-Ayland
2026-11-09 10:19 ` Markus Armbruster
1 sibling, 1 reply; 35+ messages in thread
From: Mark Cave-Ayland @ 2026-11-09 9:09 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: marcandre.lureau, milesg, qemu-ppc, qemu-arm
On 06/11/2026 11:04, Markus Armbruster wrote:
> Pass the index via object_class_property_add() parameter @opaque
> instead extracting it from the property name.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/gpio/pca9552.c | 60 ++++++++++++++---------------------------------
> hw/gpio/pca9554.c | 30 +++++++-----------------
> 2 files changed, 25 insertions(+), 65 deletions(-)
>
> diff --git a/hw/gpio/pca9552.c b/hw/gpio/pca9552.c
> index f97ad6d083..c8d0e9a16d 100644
> --- a/hw/gpio/pca9552.c
> +++ b/hw/gpio/pca9552.c
> @@ -360,18 +360,12 @@ static void pca955x_get_led(Object *obj, Visitor *v, const char *name,
> {
> PCA955xClass *k = PCA955X_GET_CLASS(obj);
> PCA955xState *s = PCA955X(obj);
> - int led, rc, reg;
> + uintptr_t led = (uintptr_t)opaque;
> + int reg;
> Pca9552LedState state;
>
> - rc = sscanf(name, "led%2d", &led);
> - if (rc != 1) {
> - error_setg(errp, "%s: error reading %s", __func__, name);
> - return;
> - }
> - if (led < 0 || led >= k->pin_count) {
> - error_setg(errp, "%s: invalid led %s", __func__, name);
> - return;
> - }
> + assert(led < k->pin_count);
> +
> /*
> * Get the LSx register as the qom interface should expose the device
> * state, not the modeled 'input line' behaviour which would come from
> @@ -397,21 +391,15 @@ static void pca955x_set_led(Object *obj, Visitor *v, const char *name,
> {
> PCA955xClass *k = PCA955X_GET_CLASS(obj);
> PCA955xState *s = PCA955X(obj);
> - int led, rc, reg, val;
> + uintptr_t led = (uintptr_t)opaque;
> + int reg, val;
> Pca9552LedState state;
>
> + assert(led < k->pin_count);
> +
> if (!visit_type_Pca9552LedState(v, name, &state, errp)) {
> return;
> }
> - rc = sscanf(name, "led%2d", &led);
> - if (rc != 1) {
> - error_setg(errp, "%s: error reading %s", __func__, name);
> - return;
> - }
> - if (led < 0 || led >= k->pin_count) {
> - error_setg(errp, "%s: invalid led %s", __func__, name);
> - return;
> - }
>
> reg = PCA9552_LS0 + led / 4;
> val = pca955x_read(s, reg);
> @@ -426,19 +414,11 @@ static void pca955x_get_pin(Object *obj, Visitor *v, const char *name,
> {
> PCA955xClass *k = PCA955X_GET_CLASS(obj);
> PCA955xState *s = PCA955X(obj);
> - int pin, rc;
> + uintptr_t pin = (uintptr_t)opaque;
> uint8_t input_reg;
> GpioPinState state;
>
> - rc = sscanf(name, "pin%2d", &pin);
> - if (rc != 1) {
> - error_setg(errp, "%s: error reading %s", __func__, name);
> - return;
> - }
> - if (pin < 0 || pin >= k->pin_count) {
> - error_setg(errp, "%s invalid pin %s", __func__, name);
> - return;
> - }
> + assert(pin < k->pin_count);
>
> /*
> * Report the raw pin logic level; polarity inversion is a read-time
> @@ -454,28 +434,22 @@ static void pca955x_set_pin(Object *obj, Visitor *v, const char *name,
> {
> PCA955xClass *k = PCA955X_GET_CLASS(obj);
> PCA955xState *s = PCA955X(obj);
> - int pin, rc;
> + uintptr_t pin = (uintptr_t)opaque;
> GpioPinState state;
> uint8_t config_reg;
>
> + assert(pin < k->pin_count);
> +
> if (!visit_type_GpioPinState(v, name, &state, errp)) {
> return;
> }
> - rc = sscanf(name, "pin%2d", &pin);
> - if (rc != 1) {
> - error_setg(errp, "%s: error reading %s", __func__, name);
> - return;
> - }
> - if (pin < 0 || pin >= k->pin_count) {
> - error_setg(errp, "%s invalid pin %s", __func__, name);
> - return;
> - }
>
> /* Only input-configured pins can be driven by an external device. */
> config_reg = PCA9535_CONFIG0 + (pin / 8);
> if (!((s->regs[config_reg] >> (pin % 8)) & 0x1)) {
> qemu_log_mask(LOG_UNIMP,
> - "%s: pin %d is configured as output, ignoring set\n",
> + "%s: pin %" PRIdPTR " is configured as output, "
> + "ignoring set\n",
> s->description, pin);
> return;
> }
> @@ -549,14 +523,14 @@ static void pca955x_pin_init(PCA955xClass *k)
> object_class_property_add(OBJECT_CLASS(k), name,
> "str", &Pca9552LedState_type_info,
> pca955x_get_led, pca955x_set_led,
> - NULL, NULL);
> + NULL, (void *)(uintptr_t)ix);
> } else {
> /* GPIO variant: expose the pin logic level as pin%d. */
> name = g_strdup_printf("pin%d", ix);
> object_class_property_add(OBJECT_CLASS(k), name,
> "str", &GpioPinState_type_info,
> pca955x_get_pin, pca955x_set_pin,
> - NULL, NULL);
> + NULL, (void *)(uintptr_t)ix);
> }
> g_free(name);
> }
> diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
> index e2d481866f..d987b16cd0 100644
> --- a/hw/gpio/pca9554.c
> +++ b/hw/gpio/pca9554.c
> @@ -173,18 +173,10 @@ static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> PCA9554State *s = PCA9554_BASE(obj);
> - int pin, rc;
> + uintptr_t pin = (uintptr_t)opaque;
> GpioPinState state;
>
> - rc = sscanf(name, "pin%2d", &pin);
> - if (rc != 1) {
> - error_setg(errp, "%s: error reading %s", __func__, name);
> - return;
> - }
> - if (pin < 0 || pin >= PCA9554_BASE_GET_CLASS(s)->pin_count) {
> - error_setg(errp, "%s invalid pin %s", __func__, name);
> - return;
> - }
> + assert(pin < PCA9554_BASE_GET_CLASS(s)->pin_count);
>
> /*
> * Report the physical pin level. The input register is kept in sync by
> @@ -200,28 +192,22 @@ static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> PCA9554State *s = PCA9554_BASE(obj);
> - int pin, rc, val;
> + uintptr_t pin = (uintptr_t)opaque;
> + int val;
> uint8_t mask;
> GpioPinState state;
>
> + assert(pin < PCA9554_BASE_GET_CLASS(s)->pin_count);
> +
> if (!visit_type_GpioPinState(v, name, &state, errp)) {
> return;
> }
> - rc = sscanf(name, "pin%2d", &pin);
> - if (rc != 1) {
> - error_setg(errp, "%s: error reading %s", __func__, name);
> - return;
> - }
> - if (pin < 0 || pin >= PCA9554_BASE_GET_CLASS(s)->pin_count) {
> - error_setg(errp, "%s invalid pin %s", __func__, name);
> - return;
> - }
>
> if (s->hw_dir) {
> /* Warn and ignore if the guest has configured this pin as output */
> if (!((s->regs[PCA9554_CONFIG] >> pin) & 0x1)) {
> qemu_log_mask(LOG_UNIMP,
> - "%s: pin %d is configured as output, "
> + "%s: pin %" PRIdPTR " is configured as output, "
> "ignoring external set\n",
> s->description, pin);
> return;
> @@ -290,7 +276,7 @@ static void pca9554_pin_init(PCA9554Class *pc)
> object_class_property_add(OBJECT_CLASS(pc), name,
> "str", &GpioPinState_type_info,
> pca9554_get_pin, pca9554_set_pin,
> - NULL, NULL);
> + NULL, (void *)(uintptr_t)pin);
> g_free(name);
> }
> }
Interesting. I think I had a similar issue trying to get the index of a
gpio when converting one of the hw/adc devices. No objection here, but
we should try and be consistent about how we obtain the index in these
cases as I can see this coming up frequently.
ATB,
Mark.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/8] hw/gpio/pca9554: Move most code from .h to .c
2026-11-09 8:55 ` Mark Cave-Ayland
@ 2026-11-09 9:40 ` Markus Armbruster
0 siblings, 0 replies; 35+ messages in thread
From: Markus Armbruster @ 2026-11-09 9:40 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, marcandre.lureau, milesg, qemu-ppc, qemu-arm
Mark Cave-Ayland <mark.caveayland@nutanix.com> writes:
> On 06/11/2026 11:04, Markus Armbruster wrote:
>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> include/hw/gpio/pca9554.h | 25 -------------------------
>> hw/gpio/pca9554.c | 24 ++++++++++++++++++++++++
>> 2 files changed, 24 insertions(+), 25 deletions(-)
>> diff --git a/include/hw/gpio/pca9554.h b/include/hw/gpio/pca9554.h
>> index ac835371aa..1b33a8a752 100644
>> --- a/include/hw/gpio/pca9554.h
>> +++ b/include/hw/gpio/pca9554.h
>> @@ -8,32 +8,7 @@
>> #ifndef PCA9554_H
>> #define PCA9554_H
>> -#include "hw/i2c/i2c.h"
>> -#include "qom/object.h"
>> -
>> #define TYPE_PCA9554 "pca9554"
>> #define TYPE_PCA9536 "pca9536"
>> -typedef struct PCA9554State PCA9554State;
>> -DECLARE_INSTANCE_CHECKER(PCA9554State, PCA9554,
>> - TYPE_PCA9554)
>> -
>> -#define PCA9554_NR_REGS 4
>> -#define PCA9554_PIN_COUNT 8
>> -#define PCA9536_PIN_COUNT 4
>> -
>> -struct PCA9554State {
>> - /*< private >*/
>> - I2CSlave i2c;
>> - /*< public >*/
>> -
>> - uint8_t len;
>> - uint8_t pointer;
>> -
>> - uint8_t regs[PCA9554_NR_REGS];
>> - qemu_irq gpio_out[PCA9554_PIN_COUNT];
>> - uint8_t ext_state[PCA9554_PIN_COUNT];
>> - char *description; /* For debugging purpose only */
>> - bool hw_dir; /* Honor pin direction */
>> -};
>> #endif
>
> Is there anything left in pca9554.h at this point?
Same as pca9552.h: just the #define TYPE_... Visible above if you
squint :)
[...]
> Otherwise:
>
> Reviewed-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
Thank you!
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 4/8] hw/gpio/pca9554: New abstract base "pca9554-base"
2026-11-09 9:02 ` Mark Cave-Ayland
@ 2026-11-09 10:17 ` Mark Cave-Ayland
2026-11-09 11:56 ` Markus Armbruster
2026-11-09 10:24 ` Markus Armbruster
1 sibling, 1 reply; 35+ messages in thread
From: Mark Cave-Ayland @ 2026-11-09 10:17 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: marcandre.lureau, milesg, qemu-ppc, qemu-arm
On 09/11/2026 09:02, Mark Cave-Ayland wrote:
> On 06/11/2026 11:04, Markus Armbruster wrote:
>
>> Commit 5eceb48bbe86 (hw/gpio: pca9554: add PCA9536 support) made type
>> "pca9535" a child of concrete type "pca9555". This is inconvenient
>> for the next commit. Make them both children of new abstract type
>> "pca9554-base".
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> include/hw/gpio/pca9554.h | 1 +
>> hw/gpio/pca9554.c | 55 +++++++++++++++++++++++----------------
>> 2 files changed, 33 insertions(+), 23 deletions(-)
>>
>> diff --git a/include/hw/gpio/pca9554.h b/include/hw/gpio/pca9554.h
>> index 1b33a8a752..0d841ff017 100644
>> --- a/include/hw/gpio/pca9554.h
>> +++ b/include/hw/gpio/pca9554.h
>> @@ -8,6 +8,7 @@
>> #ifndef PCA9554_H
>> #define PCA9554_H
>> +#define TYPE_PCA9554_BASE "pca9554-base"
>> #define TYPE_PCA9554 "pca9554"
>> #define TYPE_PCA9536 "pca9536"
>> diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
>> index caa73f63c9..1362b605c2 100644
>> --- a/hw/gpio/pca9554.c
>> +++ b/hw/gpio/pca9554.c
>> @@ -26,7 +26,7 @@
>> #define PCA9554_PIN_COUNT 8
>> #define PCA9536_PIN_COUNT 4
>> -OBJECT_DECLARE_TYPE(PCA9554State, PCA9554Class, PCA9554)
>> +OBJECT_DECLARE_TYPE(PCA9554State, PCA9554Class, PCA9554_BASE)
>> struct PCA9554State {
>> /*< private >*/
>> @@ -58,7 +58,7 @@ static const char *pin_state[] = {"low", "high"};
>> static void pca9554_update_pin_input(PCA9554State *s)
>> {
>> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
>> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
>> int i;
>> uint8_t config = s->regs[PCA9554_CONFIG];
>> uint8_t output = s->regs[PCA9554_OUTPUT];
>> @@ -110,7 +110,7 @@ static uint8_t pca9554_read(PCA9554State *s,
>> uint8_t reg)
>> static void pca9554_write(PCA9554State *s, uint8_t reg, uint8_t data)
>> {
>> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
>> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
>> uint8_t pin_mask = (1 << pc->pin_count) - 1;
>> /* Variants narrower than 8 bits ignore the unimplemented upper
>> pins. */
>> @@ -134,14 +134,14 @@ static void pca9554_write(PCA9554State *s,
>> uint8_t reg, uint8_t data)
>> static uint8_t pca9554_recv(I2CSlave *i2c)
>> {
>> - PCA9554State *s = PCA9554(i2c);
>> + PCA9554State *s = PCA9554_BASE(i2c);
>> return pca9554_read(s, s->pointer & 0x3);
>> }
>> static int pca9554_send(I2CSlave *i2c, uint8_t data)
>> {
>> - PCA9554State *s = PCA9554(i2c);
>> + PCA9554State *s = PCA9554_BASE(i2c);
>> /* First byte sent by is the register address */
>> if (s->len == 0) {
>> @@ -156,7 +156,7 @@ static int pca9554_send(I2CSlave *i2c, uint8_t data)
>> static int pca9554_event(I2CSlave *i2c, enum i2c_event event)
>> {
>> - PCA9554State *s = PCA9554(i2c);
>> + PCA9554State *s = PCA9554_BASE(i2c);
>> s->len = 0;
>> return 0;
>> @@ -173,7 +173,7 @@ static void pca9554_set_ext_state(PCA9554State *s,
>> int pin, int level)
>> static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
>> void *opaque, Error **errp)
>> {
>> - PCA9554State *s = PCA9554(obj);
>> + PCA9554State *s = PCA9554_BASE(obj);
>> int pin, rc;
>> uint8_t state;
>> @@ -182,7 +182,7 @@ static void pca9554_get_pin(Object *obj, Visitor
>> *v, const char *name,
>> error_setg(errp, "%s: error reading %s", __func__, name);
>> return;
>> }
>> - if (pin < 0 || pin >= PCA9554_GET_CLASS(s)->pin_count) {
>> + if (pin < 0 || pin >= PCA9554_BASE_GET_CLASS(s)->pin_count) {
>> error_setg(errp, "%s invalid pin %s", __func__, name);
>> return;
>> }
>> @@ -200,7 +200,7 @@ static void pca9554_get_pin(Object *obj, Visitor
>> *v, const char *name,
>> static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
>> void *opaque, Error **errp)
>> {
>> - PCA9554State *s = PCA9554(obj);
>> + PCA9554State *s = PCA9554_BASE(obj);
>> int pin, rc, val;
>> uint8_t state, mask;
>> g_autofree char *state_str = NULL;
>> @@ -213,7 +213,7 @@ static void pca9554_set_pin(Object *obj, Visitor
>> *v, const char *name,
>> error_setg(errp, "%s: error reading %s", __func__, name);
>> return;
>> }
>> - if (pin < 0 || pin >= PCA9554_GET_CLASS(s)->pin_count) {
>> + if (pin < 0 || pin >= PCA9554_BASE_GET_CLASS(s)->pin_count) {
>> error_setg(errp, "%s invalid pin %s", __func__, name);
>> return;
>> }
>> @@ -274,8 +274,8 @@ static const VMStateDescription pca9554_vmstate = {
>> static void pca9554_reset(DeviceState *dev)
>> {
>> - PCA9554State *s = PCA9554(dev);
>> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
>> + PCA9554State *s = PCA9554_BASE(dev);
>> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
>> uint8_t pin_mask = (1 << pc->pin_count) - 1;
>> s->regs[PCA9554_INPUT] = pin_mask;
>> @@ -292,7 +292,7 @@ static void pca9554_reset(DeviceState *dev)
>> static void pca9554_initfn(Object *obj)
>> {
>> - PCA9554Class *pc = PCA9554_GET_CLASS(obj);
>> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(obj);
>> int pin;
>> for (pin = 0; pin < pc->pin_count; pin++) {
>> @@ -308,8 +308,8 @@ static void pca9554_initfn(Object *obj)
>> static void pca9554_gpio_in_handler(void *opaque, int pin, int level)
>> {
>> - PCA9554State *s = PCA9554(opaque);
>> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
>> + PCA9554State *s = PCA9554_BASE(opaque);
>> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
>> assert((pin >= 0) && (pin < pc->pin_count));
>> pca9554_set_ext_state(s, pin, level);
>> @@ -317,8 +317,8 @@ static void pca9554_gpio_in_handler(void *opaque,
>> int pin, int level)
>> static void pca9554_realize(DeviceState *dev, Error **errp)
>> {
>> - PCA9554State *s = PCA9554(dev);
>> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
>> + PCA9554State *s = PCA9554_BASE(dev);
>> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
>> if (!s->description) {
>> s->description = g_strdup(object_get_typename(OBJECT(dev)));
>> @@ -333,11 +333,10 @@ static const Property pca9554_properties[] = {
>> DEFINE_PROP_BOOL("hw-dir", PCA9554State, hw_dir, false),
>> };
>> -static void pca9554_class_init(ObjectClass *klass, const void *data)
>> +static void pca9554_base_class_init(ObjectClass *klass, const void
>> *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(klass);
>> I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
>> - PCA9554Class *pc = PCA9554_CLASS(klass);
>> k->event = pca9554_event;
>> k->recv = pca9554_recv;
>> @@ -346,26 +345,36 @@ static void pca9554_class_init(ObjectClass
>> *klass, const void *data)
>> device_class_set_legacy_reset(dc, pca9554_reset);
>> dc->vmsd = &pca9554_vmstate;
>> device_class_set_props(dc, pca9554_properties);
>> +}
>> +
>> +static void pca9554_class_init(ObjectClass *klass, const void *data)
>> +{
>> + PCA9554Class *pc = PCA9554_BASE_CLASS(klass);
>> pc->pin_count = PCA9554_PIN_COUNT;
>> }
>> static void pca9536_class_init(ObjectClass *klass, const void *data)
>> {
>> - PCA9554Class *pc = PCA9554_CLASS(klass);
>> + PCA9554Class *pc = PCA9554_BASE_CLASS(klass);
>> pc->pin_count = PCA9536_PIN_COUNT;
>> }
>> static const TypeInfo pca9554_types[] = {
>> {
>> - .name = TYPE_PCA9554,
>> + .name = TYPE_PCA9554_BASE,
>> .parent = TYPE_I2C_SLAVE,
>> .instance_init = pca9554_initfn,
>> .instance_size = sizeof(PCA9554State),
>> - .class_init = pca9554_class_init,
>> + .class_init = pca9554_base_class_init,
>> .class_size = sizeof(PCA9554Class),
>> - .abstract = false,
>> + .abstract = true,
>> + },
>> + {
>> + .name = TYPE_PCA9554,
>> + .parent = TYPE_PCA9554_BASE,
>> + .class_init = pca9554_class_init,
>> },
>> {
>> .name = TYPE_PCA9536,
>
> The basic change looks good to me, however I found it tricky to follow
> given that PCA9554Class is now related to PCA9554_BASE_CLASS() and
> PCA9554State is now related to PCA9554_BASE().
>
> Would it make sense to rename PCA9554Class to PCA9554BaseClass and
> PCA9554State to PCA9554BaseState here? I think it would help make the
> hierarchy easier to follow.
Another follow-up question: how is PCA9554BaseState related to
PCA955xState? The naming suggests that the latter is already assuming
this role.
ATB,
Mark.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 8/8] hw/gpio/pca955x: Simplify pin and led getters and setters
2026-11-09 9:09 ` Mark Cave-Ayland
@ 2026-11-09 10:19 ` Markus Armbruster
0 siblings, 0 replies; 35+ messages in thread
From: Markus Armbruster @ 2026-11-09 10:19 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, marcandre.lureau, milesg, qemu-ppc, qemu-arm
Mark Cave-Ayland <mark.caveayland@nutanix.com> writes:
> On 06/11/2026 11:04, Markus Armbruster wrote:
>
>> Pass the index via object_class_property_add() parameter @opaque
>> instead extracting it from the property name.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
[...]
> Interesting. I think I had a similar issue trying to get the index of a
> gpio when converting one of the hw/adc devices. No objection here, but
> we should try and be consistent about how we obtain the index in these
> cases as I can see this coming up frequently.
Yes, consistency is desirable.
Permit me a digression.
QOM is designed to be maximally flexible, at the cost of extra
boilerplate. It also enables different solutions to similar problems.
Consider properties. Property accessors get passed the object and a
void *opaque. Maximally flexible: the object is necessary, and @opaque
can be anything. In practice, this pushes you towards writing one
bespoke getter/setter pair per property, ignoring @opaque.
When that becomes too cumbersome, people use @opaque however they see
fit. For instance, the object_property_add_T_ptr() convenience
functions use it to pass a member offset. But @opaque can be anything,
and this lets people get creative.
Qdev is different: accessors get passed a property descriptor (struct
Property), which (among other things) includes an @offset. This pushes
you towards writing a single getter/setter pair per kind of property,
using @offset to access the object memory backing the property. Less
flexible, less freedom, less boilerplate, more consistency.
Since qdev was rebased onto QOM, it uses @opaque to pass the property
descriptor through QOM to qdev accessors.
End of digression.
We should use @opaque for its intended purpose: pass whatever additional
information the getter/setter needs. We should not abuse the property
name to pass such information.
object_class_property_add_enum() doesn't support @opaque. Maybe it
should.
We don't have an object_class_property_add_enum_ptr(). Maybe we should.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 6/8] hw/gpio/pca955x: use QAPI enums for led and pin properties
2026-11-09 9:05 ` Mark Cave-Ayland
@ 2026-11-09 10:20 ` Markus Armbruster
0 siblings, 0 replies; 35+ messages in thread
From: Markus Armbruster @ 2026-11-09 10:20 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, marcandre.lureau, milesg, qemu-ppc, qemu-arm
Mark Cave-Ayland <mark.caveayland@nutanix.com> writes:
> On 06/11/2026 11:04, Markus Armbruster wrote:
>
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Replace hand-rolled string lookup tables with QAPI enums
>> Pca9552LedState and GpioPinState for the led%d and pin%d QOM
>> properties. This lets QAPI handle string-to-enum conversion in the
>> visitor, removing the manual string matching in the setters.
>> Add QEMU_BUILD_BUG_ON guards to ensure the QAPI-generated enum values
>> stay in sync with the hardware register encoding.
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
[...]
> I'm less confident with the QAPI side with all the recent changes to give an R-B, however it seems sensible to me. Perhaps an Acked-by if you feel you need it? Otherwise LGTM.
Acked-by makes sense then, doesn't it?
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 4/8] hw/gpio/pca9554: New abstract base "pca9554-base"
2026-11-09 9:02 ` Mark Cave-Ayland
2026-11-09 10:17 ` Mark Cave-Ayland
@ 2026-11-09 10:24 ` Markus Armbruster
2026-11-09 13:52 ` BALATON Zoltan
1 sibling, 1 reply; 35+ messages in thread
From: Markus Armbruster @ 2026-11-09 10:24 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, marcandre.lureau, milesg, qemu-ppc, qemu-arm
Mark Cave-Ayland <mark.caveayland@nutanix.com> writes:
> On 06/11/2026 11:04, Markus Armbruster wrote:
>
>> Commit 5eceb48bbe86 (hw/gpio: pca9554: add PCA9536 support) made type
>> "pca9535" a child of concrete type "pca9555". This is inconvenient
>> for the next commit. Make them both children of new abstract type
>> "pca9554-base".
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> include/hw/gpio/pca9554.h | 1 +
>> hw/gpio/pca9554.c | 55 +++++++++++++++++++++++----------------
>> 2 files changed, 33 insertions(+), 23 deletions(-)
>>
>> diff --git a/include/hw/gpio/pca9554.h b/include/hw/gpio/pca9554.h
>> index 1b33a8a752..0d841ff017 100644
>> --- a/include/hw/gpio/pca9554.h
>> +++ b/include/hw/gpio/pca9554.h
>> @@ -8,6 +8,7 @@
>> #ifndef PCA9554_H
>> #define PCA9554_H
>>
>> +#define TYPE_PCA9554_BASE "pca9554-base"
>> #define TYPE_PCA9554 "pca9554"
>> #define TYPE_PCA9536 "pca9536"
>>
>> diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
>> index caa73f63c9..1362b605c2 100644
>> --- a/hw/gpio/pca9554.c
>> +++ b/hw/gpio/pca9554.c
>> @@ -26,7 +26,7 @@
>> #define PCA9554_PIN_COUNT 8
>> #define PCA9536_PIN_COUNT 4
>>
>> -OBJECT_DECLARE_TYPE(PCA9554State, PCA9554Class, PCA9554)
>> +OBJECT_DECLARE_TYPE(PCA9554State, PCA9554Class, PCA9554_BASE)
>>
>> struct PCA9554State {
>> /*< private >*/
>> @@ -58,7 +58,7 @@ static const char *pin_state[] = {"low", "high"};
>>
>> static void pca9554_update_pin_input(PCA9554State *s)
>> {
>> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
>> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
>> int i;
>> uint8_t config = s->regs[PCA9554_CONFIG];
>> uint8_t output = s->regs[PCA9554_OUTPUT];
>> @@ -110,7 +110,7 @@ static uint8_t pca9554_read(PCA9554State *s, uint8_t reg)
>>
>> static void pca9554_write(PCA9554State *s, uint8_t reg, uint8_t data)
>> {
>> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
>> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
>> uint8_t pin_mask = (1 << pc->pin_count) - 1;
>>
>> /* Variants narrower than 8 bits ignore the unimplemented upper pins. */
>> @@ -134,14 +134,14 @@ static void pca9554_write(PCA9554State *s, uint8_t reg, uint8_t data)
>>
>> static uint8_t pca9554_recv(I2CSlave *i2c)
>> {
>> - PCA9554State *s = PCA9554(i2c);
>> + PCA9554State *s = PCA9554_BASE(i2c);
>>
>> return pca9554_read(s, s->pointer & 0x3);
>> }
>>
>> static int pca9554_send(I2CSlave *i2c, uint8_t data)
>> {
>> - PCA9554State *s = PCA9554(i2c);
>> + PCA9554State *s = PCA9554_BASE(i2c);
>>
>> /* First byte sent by is the register address */
>> if (s->len == 0) {
>> @@ -156,7 +156,7 @@ static int pca9554_send(I2CSlave *i2c, uint8_t data)
>>
>> static int pca9554_event(I2CSlave *i2c, enum i2c_event event)
>> {
>> - PCA9554State *s = PCA9554(i2c);
>> + PCA9554State *s = PCA9554_BASE(i2c);
>>
>> s->len = 0;
>> return 0;
>> @@ -173,7 +173,7 @@ static void pca9554_set_ext_state(PCA9554State *s, int pin, int level)
>> static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
>> void *opaque, Error **errp)
>> {
>> - PCA9554State *s = PCA9554(obj);
>> + PCA9554State *s = PCA9554_BASE(obj);
>> int pin, rc;
>> uint8_t state;
>>
>> @@ -182,7 +182,7 @@ static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
>> error_setg(errp, "%s: error reading %s", __func__, name);
>> return;
>> }
>> - if (pin < 0 || pin >= PCA9554_GET_CLASS(s)->pin_count) {
>> + if (pin < 0 || pin >= PCA9554_BASE_GET_CLASS(s)->pin_count) {
>> error_setg(errp, "%s invalid pin %s", __func__, name);
>> return;
>> }
>> @@ -200,7 +200,7 @@ static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
>> static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
>> void *opaque, Error **errp)
>> {
>> - PCA9554State *s = PCA9554(obj);
>> + PCA9554State *s = PCA9554_BASE(obj);
>> int pin, rc, val;
>> uint8_t state, mask;
>> g_autofree char *state_str = NULL;
>> @@ -213,7 +213,7 @@ static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
>> error_setg(errp, "%s: error reading %s", __func__, name);
>> return;
>> }
>> - if (pin < 0 || pin >= PCA9554_GET_CLASS(s)->pin_count) {
>> + if (pin < 0 || pin >= PCA9554_BASE_GET_CLASS(s)->pin_count) {
>> error_setg(errp, "%s invalid pin %s", __func__, name);
>> return;
>> }
>> @@ -274,8 +274,8 @@ static const VMStateDescription pca9554_vmstate = {
>>
>> static void pca9554_reset(DeviceState *dev)
>> {
>> - PCA9554State *s = PCA9554(dev);
>> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
>> + PCA9554State *s = PCA9554_BASE(dev);
>> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
>> uint8_t pin_mask = (1 << pc->pin_count) - 1;
>>
>> s->regs[PCA9554_INPUT] = pin_mask;
>> @@ -292,7 +292,7 @@ static void pca9554_reset(DeviceState *dev)
>>
>> static void pca9554_initfn(Object *obj)
>> {
>> - PCA9554Class *pc = PCA9554_GET_CLASS(obj);
>> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(obj);
>> int pin;
>>
>> for (pin = 0; pin < pc->pin_count; pin++) {
>> @@ -308,8 +308,8 @@ static void pca9554_initfn(Object *obj)
>>
>> static void pca9554_gpio_in_handler(void *opaque, int pin, int level)
>> {
>> - PCA9554State *s = PCA9554(opaque);
>> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
>> + PCA9554State *s = PCA9554_BASE(opaque);
>> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
>>
>> assert((pin >= 0) && (pin < pc->pin_count));
>> pca9554_set_ext_state(s, pin, level);
>> @@ -317,8 +317,8 @@ static void pca9554_gpio_in_handler(void *opaque, int pin, int level)
>>
>> static void pca9554_realize(DeviceState *dev, Error **errp)
>> {
>> - PCA9554State *s = PCA9554(dev);
>> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
>> + PCA9554State *s = PCA9554_BASE(dev);
>> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
>>
>> if (!s->description) {
>> s->description = g_strdup(object_get_typename(OBJECT(dev)));
>> @@ -333,11 +333,10 @@ static const Property pca9554_properties[] = {
>> DEFINE_PROP_BOOL("hw-dir", PCA9554State, hw_dir, false),
>> };
>>
>> -static void pca9554_class_init(ObjectClass *klass, const void *data)
>> +static void pca9554_base_class_init(ObjectClass *klass, const void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(klass);
>> I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
>> - PCA9554Class *pc = PCA9554_CLASS(klass);
>>
>> k->event = pca9554_event;
>> k->recv = pca9554_recv;
>> @@ -346,26 +345,36 @@ static void pca9554_class_init(ObjectClass *klass, const void *data)
>> device_class_set_legacy_reset(dc, pca9554_reset);
>> dc->vmsd = &pca9554_vmstate;
>> device_class_set_props(dc, pca9554_properties);
>> +}
>> +
>> +static void pca9554_class_init(ObjectClass *klass, const void *data)
>> +{
>> + PCA9554Class *pc = PCA9554_BASE_CLASS(klass);
>>
>> pc->pin_count = PCA9554_PIN_COUNT;
>> }
>>
>> static void pca9536_class_init(ObjectClass *klass, const void *data)
>> {
>> - PCA9554Class *pc = PCA9554_CLASS(klass);
>> + PCA9554Class *pc = PCA9554_BASE_CLASS(klass);
>>
>> pc->pin_count = PCA9536_PIN_COUNT;
>> }
>>
>> static const TypeInfo pca9554_types[] = {
>> {
>> - .name = TYPE_PCA9554,
>> + .name = TYPE_PCA9554_BASE,
>> .parent = TYPE_I2C_SLAVE,
>> .instance_init = pca9554_initfn,
>> .instance_size = sizeof(PCA9554State),
>> - .class_init = pca9554_class_init,
>> + .class_init = pca9554_base_class_init,
>> .class_size = sizeof(PCA9554Class),
>> - .abstract = false,
>> + .abstract = true,
>> + },
>> + {
>> + .name = TYPE_PCA9554,
>> + .parent = TYPE_PCA9554_BASE,
>> + .class_init = pca9554_class_init,
>> },
>> {
>> .name = TYPE_PCA9536,
>
> The basic change looks good to me, however I found it tricky to follow
> given that PCA9554Class is now related to PCA9554_BASE_CLASS() and
> PCA9554State is now related to PCA9554_BASE().
>
> Would it make sense to rename PCA9554Class to PCA9554BaseClass and
> PCA9554State to PCA9554BaseState here? I think it would help make the
> hierarchy easier to follow.
I considered it when I wrote this patch, then decided not to, to
minimize churn and keep the identifiers short. I'm fine either way.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 4/8] hw/gpio/pca9554: New abstract base "pca9554-base"
2026-11-09 10:17 ` Mark Cave-Ayland
@ 2026-11-09 11:56 ` Markus Armbruster
2026-11-09 14:20 ` Mark Cave-Ayland
0 siblings, 1 reply; 35+ messages in thread
From: Markus Armbruster @ 2026-11-09 11:56 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, marcandre.lureau, milesg, qemu-ppc, qemu-arm
Mark Cave-Ayland <mark.caveayland@nutanix.com> writes:
> On 09/11/2026 09:02, Mark Cave-Ayland wrote:
>
>> On 06/11/2026 11:04, Markus Armbruster wrote:
>>
>>> Commit 5eceb48bbe86 (hw/gpio: pca9554: add PCA9536 support) made type
>>> "pca9535" a child of concrete type "pca9555". This is inconvenient
>>> for the next commit. Make them both children of new abstract type
>>> "pca9554-base".
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>> include/hw/gpio/pca9554.h | 1 +
>>> hw/gpio/pca9554.c | 55 +++++++++++++++++++++++----------------
>>> 2 files changed, 33 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/include/hw/gpio/pca9554.h b/include/hw/gpio/pca9554.h
>>> index 1b33a8a752..0d841ff017 100644
>>> --- a/include/hw/gpio/pca9554.h
>>> +++ b/include/hw/gpio/pca9554.h
>>> @@ -8,6 +8,7 @@
>>> #ifndef PCA9554_H
>>> #define PCA9554_H
>>>
>>> +#define TYPE_PCA9554_BASE "pca9554-base"
>>> #define TYPE_PCA9554 "pca9554"
>>> #define TYPE_PCA9536 "pca9536"
>>>
>>> diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
>>> index caa73f63c9..1362b605c2 100644
>>> --- a/hw/gpio/pca9554.c
>>> +++ b/hw/gpio/pca9554.c
>>> @@ -26,7 +26,7 @@
>>> #define PCA9554_PIN_COUNT 8
>>> #define PCA9536_PIN_COUNT 4
>>>
>>> -OBJECT_DECLARE_TYPE(PCA9554State, PCA9554Class, PCA9554)
>>> +OBJECT_DECLARE_TYPE(PCA9554State, PCA9554Class, PCA9554_BASE)
>>>
>>> struct PCA9554State {
>>> /*< private >*/
[...]
>> The basic change looks good to me, however I found it tricky to follow given that PCA9554Class is now related to PCA9554_BASE_CLASS() and PCA9554State is now related to PCA9554_BASE().
>> Would it make sense to rename PCA9554Class to PCA9554BaseClass and PCA9554State to PCA9554BaseState here? I think it would help make the hierarchy easier to follow.
> Another follow-up question: how is PCA9554BaseState related to PCA955xState? The naming suggests that the latter is already assuming this role.
There is no PCA9554BaseState.
Have a look at the OBJECT_DECLARE_TYPE(), and its definition in
qom/object.h. It does the following:
* typedef struct PCA9554State PCA9554State
* typedef struct PCA9554Class PCA9554Class
* Make g_autoptr(PCA9554State) work
* Define PCA9554_BASE(), PCA9554_BASE_CLASS(), PCA9554_GET_BASE_CLASS()
Or let's approach this from another angle.
Initially, we had just one type ("pca9554"). It had its own class and
instance struct (PCA9554Class and PCA9554State), which came with the
usual macros PCI9554(), PCA9554_CLASS(), PCA9554_GET_CLASS().
Then we added a second type ("pca9536") as a subtype of the first.
Instead of giving it its own class and instance struct, we reused the
existing ones, and we didn't define any macros.
This patch adds a third type ("pca9554-base"). It is abstract. It
again reuses the existing class and instance struct. However, because
this third type is the supertype of the existing two, we can't cleanly
reuse the existing macros. So we *replace* them by PCA9554_BASE(),
PCA9554_BASE_CLASS(), PCA9554_BASE_GET_CLASS(). Except this is actually
a *rename*, because we still use them same class and instance struct.
Summary:
* "pca9536" ceases to be a subtype of "pca9554"
* "pca9554" and "pca9536" become subtypes of "pca9554-base"
* Their class and instance structs remain the same: PCA9554Class and
PCA9554State. There are no new C types.
* Macros PCA9554(), PCA9554_CLASS(), PCA9554_GET_CLASS() are renamed to
PCA9554_BASE(), PCA9554_BASE_CLASS(), PCA9554_BASE_GET_CLASS().
Questions?
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 4/8] hw/gpio/pca9554: New abstract base "pca9554-base"
2026-11-09 10:24 ` Markus Armbruster
@ 2026-11-09 13:52 ` BALATON Zoltan
2026-11-09 14:17 ` Markus Armbruster
0 siblings, 1 reply; 35+ messages in thread
From: BALATON Zoltan @ 2026-11-09 13:52 UTC (permalink / raw)
To: Markus Armbruster
Cc: Mark Cave-Ayland, qemu-devel, marcandre.lureau, milesg, qemu-ppc,
qemu-arm
On Mon, 9 Nov 2026, Markus Armbruster wrote:
> Mark Cave-Ayland <mark.caveayland@nutanix.com> writes:
>
>> On 06/11/2026 11:04, Markus Armbruster wrote:
>>
>>> Commit 5eceb48bbe86 (hw/gpio: pca9554: add PCA9536 support) made type
>>> "pca9535" a child of concrete type "pca9555". This is inconvenient
>>> for the next commit. Make them both children of new abstract type
>>> "pca9554-base".
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>> include/hw/gpio/pca9554.h | 1 +
>>> hw/gpio/pca9554.c | 55 +++++++++++++++++++++++----------------
>>> 2 files changed, 33 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/include/hw/gpio/pca9554.h b/include/hw/gpio/pca9554.h
>>> index 1b33a8a752..0d841ff017 100644
>>> --- a/include/hw/gpio/pca9554.h
>>> +++ b/include/hw/gpio/pca9554.h
>>> @@ -8,6 +8,7 @@
>>> #ifndef PCA9554_H
>>> #define PCA9554_H
>>>
>>> +#define TYPE_PCA9554_BASE "pca9554-base"
>>> #define TYPE_PCA9554 "pca9554"
>>> #define TYPE_PCA9536 "pca9536"
>>>
>>> diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
>>> index caa73f63c9..1362b605c2 100644
>>> --- a/hw/gpio/pca9554.c
>>> +++ b/hw/gpio/pca9554.c
>>> @@ -26,7 +26,7 @@
>>> #define PCA9554_PIN_COUNT 8
>>> #define PCA9536_PIN_COUNT 4
>>>
>>> -OBJECT_DECLARE_TYPE(PCA9554State, PCA9554Class, PCA9554)
>>> +OBJECT_DECLARE_TYPE(PCA9554State, PCA9554Class, PCA9554_BASE)
>>>
>>> struct PCA9554State {
>>> /*< private >*/
>>> @@ -58,7 +58,7 @@ static const char *pin_state[] = {"low", "high"};
>>>
>>> static void pca9554_update_pin_input(PCA9554State *s)
>>> {
>>> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
>>> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
>>> int i;
>>> uint8_t config = s->regs[PCA9554_CONFIG];
>>> uint8_t output = s->regs[PCA9554_OUTPUT];
>>> @@ -110,7 +110,7 @@ static uint8_t pca9554_read(PCA9554State *s, uint8_t reg)
>>>
>>> static void pca9554_write(PCA9554State *s, uint8_t reg, uint8_t data)
>>> {
>>> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
>>> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
>>> uint8_t pin_mask = (1 << pc->pin_count) - 1;
>>>
>>> /* Variants narrower than 8 bits ignore the unimplemented upper pins. */
>>> @@ -134,14 +134,14 @@ static void pca9554_write(PCA9554State *s, uint8_t reg, uint8_t data)
>>>
>>> static uint8_t pca9554_recv(I2CSlave *i2c)
>>> {
>>> - PCA9554State *s = PCA9554(i2c);
>>> + PCA9554State *s = PCA9554_BASE(i2c);
>>>
>>> return pca9554_read(s, s->pointer & 0x3);
>>> }
>>>
>>> static int pca9554_send(I2CSlave *i2c, uint8_t data)
>>> {
>>> - PCA9554State *s = PCA9554(i2c);
>>> + PCA9554State *s = PCA9554_BASE(i2c);
>>>
>>> /* First byte sent by is the register address */
>>> if (s->len == 0) {
>>> @@ -156,7 +156,7 @@ static int pca9554_send(I2CSlave *i2c, uint8_t data)
>>>
>>> static int pca9554_event(I2CSlave *i2c, enum i2c_event event)
>>> {
>>> - PCA9554State *s = PCA9554(i2c);
>>> + PCA9554State *s = PCA9554_BASE(i2c);
>>>
>>> s->len = 0;
>>> return 0;
>>> @@ -173,7 +173,7 @@ static void pca9554_set_ext_state(PCA9554State *s, int pin, int level)
>>> static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
>>> void *opaque, Error **errp)
>>> {
>>> - PCA9554State *s = PCA9554(obj);
>>> + PCA9554State *s = PCA9554_BASE(obj);
>>> int pin, rc;
>>> uint8_t state;
>>>
>>> @@ -182,7 +182,7 @@ static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
>>> error_setg(errp, "%s: error reading %s", __func__, name);
>>> return;
>>> }
>>> - if (pin < 0 || pin >= PCA9554_GET_CLASS(s)->pin_count) {
>>> + if (pin < 0 || pin >= PCA9554_BASE_GET_CLASS(s)->pin_count) {
>>> error_setg(errp, "%s invalid pin %s", __func__, name);
>>> return;
>>> }
>>> @@ -200,7 +200,7 @@ static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
>>> static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
>>> void *opaque, Error **errp)
>>> {
>>> - PCA9554State *s = PCA9554(obj);
>>> + PCA9554State *s = PCA9554_BASE(obj);
>>> int pin, rc, val;
>>> uint8_t state, mask;
>>> g_autofree char *state_str = NULL;
>>> @@ -213,7 +213,7 @@ static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
>>> error_setg(errp, "%s: error reading %s", __func__, name);
>>> return;
>>> }
>>> - if (pin < 0 || pin >= PCA9554_GET_CLASS(s)->pin_count) {
>>> + if (pin < 0 || pin >= PCA9554_BASE_GET_CLASS(s)->pin_count) {
>>> error_setg(errp, "%s invalid pin %s", __func__, name);
>>> return;
>>> }
>>> @@ -274,8 +274,8 @@ static const VMStateDescription pca9554_vmstate = {
>>>
>>> static void pca9554_reset(DeviceState *dev)
>>> {
>>> - PCA9554State *s = PCA9554(dev);
>>> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
>>> + PCA9554State *s = PCA9554_BASE(dev);
>>> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
>>> uint8_t pin_mask = (1 << pc->pin_count) - 1;
>>>
>>> s->regs[PCA9554_INPUT] = pin_mask;
>>> @@ -292,7 +292,7 @@ static void pca9554_reset(DeviceState *dev)
>>>
>>> static void pca9554_initfn(Object *obj)
>>> {
>>> - PCA9554Class *pc = PCA9554_GET_CLASS(obj);
>>> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(obj);
>>> int pin;
>>>
>>> for (pin = 0; pin < pc->pin_count; pin++) {
>>> @@ -308,8 +308,8 @@ static void pca9554_initfn(Object *obj)
>>>
>>> static void pca9554_gpio_in_handler(void *opaque, int pin, int level)
>>> {
>>> - PCA9554State *s = PCA9554(opaque);
>>> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
>>> + PCA9554State *s = PCA9554_BASE(opaque);
>>> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
>>>
>>> assert((pin >= 0) && (pin < pc->pin_count));
>>> pca9554_set_ext_state(s, pin, level);
>>> @@ -317,8 +317,8 @@ static void pca9554_gpio_in_handler(void *opaque, int pin, int level)
>>>
>>> static void pca9554_realize(DeviceState *dev, Error **errp)
>>> {
>>> - PCA9554State *s = PCA9554(dev);
>>> - PCA9554Class *pc = PCA9554_GET_CLASS(s);
>>> + PCA9554State *s = PCA9554_BASE(dev);
>>> + PCA9554Class *pc = PCA9554_BASE_GET_CLASS(s);
>>>
>>> if (!s->description) {
>>> s->description = g_strdup(object_get_typename(OBJECT(dev)));
>>> @@ -333,11 +333,10 @@ static const Property pca9554_properties[] = {
>>> DEFINE_PROP_BOOL("hw-dir", PCA9554State, hw_dir, false),
>>> };
>>>
>>> -static void pca9554_class_init(ObjectClass *klass, const void *data)
>>> +static void pca9554_base_class_init(ObjectClass *klass, const void *data)
>>> {
>>> DeviceClass *dc = DEVICE_CLASS(klass);
>>> I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
>>> - PCA9554Class *pc = PCA9554_CLASS(klass);
>>>
>>> k->event = pca9554_event;
>>> k->recv = pca9554_recv;
>>> @@ -346,26 +345,36 @@ static void pca9554_class_init(ObjectClass *klass, const void *data)
>>> device_class_set_legacy_reset(dc, pca9554_reset);
>>> dc->vmsd = &pca9554_vmstate;
>>> device_class_set_props(dc, pca9554_properties);
>>> +}
>>> +
>>> +static void pca9554_class_init(ObjectClass *klass, const void *data)
>>> +{
>>> + PCA9554Class *pc = PCA9554_BASE_CLASS(klass);
>>>
>>> pc->pin_count = PCA9554_PIN_COUNT;
>>> }
>>>
>>> static void pca9536_class_init(ObjectClass *klass, const void *data)
>>> {
>>> - PCA9554Class *pc = PCA9554_CLASS(klass);
>>> + PCA9554Class *pc = PCA9554_BASE_CLASS(klass);
>>>
>>> pc->pin_count = PCA9536_PIN_COUNT;
>>> }
>>>
>>> static const TypeInfo pca9554_types[] = {
>>> {
>>> - .name = TYPE_PCA9554,
>>> + .name = TYPE_PCA9554_BASE,
>>> .parent = TYPE_I2C_SLAVE,
>>> .instance_init = pca9554_initfn,
>>> .instance_size = sizeof(PCA9554State),
>>> - .class_init = pca9554_class_init,
>>> + .class_init = pca9554_base_class_init,
>>> .class_size = sizeof(PCA9554Class),
>>> - .abstract = false,
>>> + .abstract = true,
>>> + },
>>> + {
>>> + .name = TYPE_PCA9554,
>>> + .parent = TYPE_PCA9554_BASE,
>>> + .class_init = pca9554_class_init,
>>> },
>>> {
>>> .name = TYPE_PCA9536,
>>
>> The basic change looks good to me, however I found it tricky to follow
>> given that PCA9554Class is now related to PCA9554_BASE_CLASS() and
>> PCA9554State is now related to PCA9554_BASE().
>>
>> Would it make sense to rename PCA9554Class to PCA9554BaseClass and
>> PCA9554State to PCA9554BaseState here? I think it would help make the
>> hierarchy easier to follow.
>
> I considered it when I wrote this patch, then decided not to, to
> minimize churn and keep the identifiers short. I'm fine either way.
How about calling the base class PCA95xx or similar to keep it both short
and show it belongs to both child classes?
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 4/8] hw/gpio/pca9554: New abstract base "pca9554-base"
2026-11-09 13:52 ` BALATON Zoltan
@ 2026-11-09 14:17 ` Markus Armbruster
0 siblings, 0 replies; 35+ messages in thread
From: Markus Armbruster @ 2026-11-09 14:17 UTC (permalink / raw)
To: BALATON Zoltan
Cc: Mark Cave-Ayland, qemu-devel, marcandre.lureau, milesg, qemu-ppc,
qemu-arm
BALATON Zoltan <balaton@eik.bme.hu> writes:
> On Mon, 9 Nov 2026, Markus Armbruster wrote:
>> Mark Cave-Ayland <mark.caveayland@nutanix.com> writes:
>>
>>> On 06/11/2026 11:04, Markus Armbruster wrote:
>>>
>>>> Commit 5eceb48bbe86 (hw/gpio: pca9554: add PCA9536 support) made type
>>>> "pca9535" a child of concrete type "pca9555". This is inconvenient
>>>> for the next commit. Make them both children of new abstract type
>>>> "pca9554-base".
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
[...]
>>> The basic change looks good to me, however I found it tricky to follow
>>> given that PCA9554Class is now related to PCA9554_BASE_CLASS() and
>>> PCA9554State is now related to PCA9554_BASE().
>>>
>>> Would it make sense to rename PCA9554Class to PCA9554BaseClass and
>>> PCA9554State to PCA9554BaseState here? I think it would help make the
>>> hierarchy easier to follow.
>>
>> I considered it when I wrote this patch, then decided not to, to
>> minimize churn and keep the identifiers short. I'm fine either way.
>
> How about calling the base class PCA95xx or similar to keep it both short and show it belongs to both child classes?
Existing types in this area:
* Abstract "pca955x" with concrete subtypes "pca9552", "pca9535",
"pca9555".
* Abstract type whose name we're debating with concrete subtypes
"pca9554" and "pca9536".
I fear naming it "pca95xx" would be confusing.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 4/8] hw/gpio/pca9554: New abstract base "pca9554-base"
2026-11-09 11:56 ` Markus Armbruster
@ 2026-11-09 14:20 ` Mark Cave-Ayland
2026-11-09 14:44 ` Markus Armbruster
0 siblings, 1 reply; 35+ messages in thread
From: Mark Cave-Ayland @ 2026-11-09 14:20 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, marcandre.lureau, milesg, qemu-ppc, qemu-arm
On 09/11/2026 11:56, Markus Armbruster wrote:
> Mark Cave-Ayland <mark.caveayland@nutanix.com> writes:
>
>> On 09/11/2026 09:02, Mark Cave-Ayland wrote:
>>
>>> On 06/11/2026 11:04, Markus Armbruster wrote:
>>>
>>>> Commit 5eceb48bbe86 (hw/gpio: pca9554: add PCA9536 support) made type
>>>> "pca9535" a child of concrete type "pca9555". This is inconvenient
>>>> for the next commit. Make them both children of new abstract type
>>>> "pca9554-base".
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>> include/hw/gpio/pca9554.h | 1 +
>>>> hw/gpio/pca9554.c | 55 +++++++++++++++++++++++----------------
>>>> 2 files changed, 33 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/include/hw/gpio/pca9554.h b/include/hw/gpio/pca9554.h
>>>> index 1b33a8a752..0d841ff017 100644
>>>> --- a/include/hw/gpio/pca9554.h
>>>> +++ b/include/hw/gpio/pca9554.h
>>>> @@ -8,6 +8,7 @@
>>>> #ifndef PCA9554_H
>>>> #define PCA9554_H
>>>>
>>>> +#define TYPE_PCA9554_BASE "pca9554-base"
>>>> #define TYPE_PCA9554 "pca9554"
>>>> #define TYPE_PCA9536 "pca9536"
>>>>
>>>> diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
>>>> index caa73f63c9..1362b605c2 100644
>>>> --- a/hw/gpio/pca9554.c
>>>> +++ b/hw/gpio/pca9554.c
>>>> @@ -26,7 +26,7 @@
>>>> #define PCA9554_PIN_COUNT 8
>>>> #define PCA9536_PIN_COUNT 4
>>>>
>>>> -OBJECT_DECLARE_TYPE(PCA9554State, PCA9554Class, PCA9554)
>>>> +OBJECT_DECLARE_TYPE(PCA9554State, PCA9554Class, PCA9554_BASE)
>>>>
>>>> struct PCA9554State {
>>>> /*< private >*/
>
> [...]
>
>>> The basic change looks good to me, however I found it tricky to follow given that PCA9554Class is now related to PCA9554_BASE_CLASS() and PCA9554State is now related to PCA9554_BASE().
>>> Would it make sense to rename PCA9554Class to PCA9554BaseClass and PCA9554State to PCA9554BaseState here? I think it would help make the hierarchy easier to follow.
>> Another follow-up question: how is PCA9554BaseState related to PCA955xState? The naming suggests that the latter is already assuming this role.
>
> There is no PCA9554BaseState.
>
> Have a look at the OBJECT_DECLARE_TYPE(), and its definition in
> qom/object.h. It does the following:
>
> * typedef struct PCA9554State PCA9554State
>
> * typedef struct PCA9554Class PCA9554Class
>
> * Make g_autoptr(PCA9554State) work
>
> * Define PCA9554_BASE(), PCA9554_BASE_CLASS(), PCA9554_GET_BASE_CLASS()
>
> Or let's approach this from another angle.
>
> Initially, we had just one type ("pca9554"). It had its own class and
> instance struct (PCA9554Class and PCA9554State), which came with the
> usual macros PCI9554(), PCA9554_CLASS(), PCA9554_GET_CLASS().
>
> Then we added a second type ("pca9536") as a subtype of the first.
> Instead of giving it its own class and instance struct, we reused the
> existing ones, and we didn't define any macros.
>
> This patch adds a third type ("pca9554-base"). It is abstract. It
> again reuses the existing class and instance struct. However, because
> this third type is the supertype of the existing two, we can't cleanly
> reuse the existing macros. So we *replace* them by PCA9554_BASE(),
> PCA9554_BASE_CLASS(), PCA9554_BASE_GET_CLASS(). Except this is actually
> a *rename*, because we still use them same class and instance struct.
>
> Summary:
>
> * "pca9536" ceases to be a subtype of "pca9554"
>
> * "pca9554" and "pca9536" become subtypes of "pca9554-base"
>
> * Their class and instance structs remain the same: PCA9554Class and
> PCA9554State. There are no new C types.
>
> * Macros PCA9554(), PCA9554_CLASS(), PCA9554_GET_CLASS() are renamed to
> PCA9554_BASE(), PCA9554_BASE_CLASS(), PCA9554_BASE_GET_CLASS().
>
> Questions?
Well just the one :) The point I was making above was really about
PCA955xState already existing in hw/gpio/pca9552.c: it seems that the
common parent PCA955xState (and also parent class PCA955xClass) already
exists for this family of chips, and in fact the real problem is that
hw/gpio/pca9554.c doesn't make use of this.
ATB,
Mark.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 4/8] hw/gpio/pca9554: New abstract base "pca9554-base"
2026-11-09 14:20 ` Mark Cave-Ayland
@ 2026-11-09 14:44 ` Markus Armbruster
0 siblings, 0 replies; 35+ messages in thread
From: Markus Armbruster @ 2026-11-09 14:44 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, marcandre.lureau, milesg, qemu-ppc, qemu-arm
Mark Cave-Ayland <mark.caveayland@nutanix.com> writes:
> Well just the one :) The point I was making above was really about PCA955xState already existing in hw/gpio/pca9552.c: it seems that the common parent PCA955xState (and also parent class PCA955xClass) already exists for this family of chips, and in fact the real problem is that hw/gpio/pca9554.c doesn't make use of this.
Ah, I completely misunderstood your question!
PCA955xState is the instance struct of abstract type "pca955x" and its
concrete subtypes "pca9552", "pca9535", "pca9555". These live in
pca9552.c.
We also have "pca9554" and "pca9536" in pca9554.c. It's an entirely
separate implementation. I have no idea whether this is approproate.
Do you think they should be subtypes of "pca955x", and share common
code?
If yes: I can't get us there, sorry. All I'm trying to do in this
series is to bring back Marc-André's work I dropped. I didn't feel like
leaving the properties non-class when I touch them anyway, so I
converted them to class properties first. That may have been unwise :)
^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2026-11-09 14:46 UTC | newest]
Thread overview: 35+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-11-06 11:04 [PATCH 0/8] hw/gpio/pca955x: Improvements around props "pin" and "led" Markus Armbruster
2026-11-06 11:04 ` [PATCH 1/8] hw/gpio/pca9552: Convert pin and led properties to class properties Markus Armbruster
2026-11-06 20:17 ` Miles Glenn
2026-11-09 8:51 ` Mark Cave-Ayland
2026-11-06 11:04 ` [PATCH 2/8] hw/gpio/pca9554: Move most code from .h to .c Markus Armbruster
2026-11-06 20:18 ` Miles Glenn
2026-11-09 8:55 ` Mark Cave-Ayland
2026-11-09 9:40 ` Markus Armbruster
2026-11-06 11:04 ` [PATCH 3/8] hw/gpio/pca9554: Simplify QOM type creation Markus Armbruster
2026-11-06 20:19 ` Miles Glenn
2026-11-09 8:56 ` Mark Cave-Ayland
2026-11-06 11:04 ` [PATCH 4/8] hw/gpio/pca9554: New abstract base "pca9554-base" Markus Armbruster
2026-11-06 20:19 ` Miles Glenn
2026-11-09 9:02 ` Mark Cave-Ayland
2026-11-09 10:17 ` Mark Cave-Ayland
2026-11-09 11:56 ` Markus Armbruster
2026-11-09 14:20 ` Mark Cave-Ayland
2026-11-09 14:44 ` Markus Armbruster
2026-11-09 10:24 ` Markus Armbruster
2026-11-09 13:52 ` BALATON Zoltan
2026-11-09 14:17 ` Markus Armbruster
2026-11-06 11:04 ` [PATCH 5/8] hw/gpio/pca9554: Convert pin properties to class properties Markus Armbruster
2026-11-06 20:20 ` Miles Glenn
2026-11-09 9:03 ` Mark Cave-Ayland
2026-11-06 11:04 ` [PATCH 6/8] hw/gpio/pca955x: use QAPI enums for led and pin properties Markus Armbruster
2026-11-06 20:20 ` Miles Glenn
2026-11-09 9:05 ` Mark Cave-Ayland
2026-11-09 10:20 ` Markus Armbruster
2026-11-06 11:04 ` [PATCH 7/8] hw/gpio/pca955x: Improve @qapi-type reported by qom-list etc Markus Armbruster
2026-11-06 20:21 ` Miles Glenn
2026-11-09 9:06 ` Mark Cave-Ayland
2026-11-06 11:04 ` [PATCH 8/8] hw/gpio/pca955x: Simplify pin and led getters and setters Markus Armbruster
2026-11-06 20:21 ` Miles Glenn
2026-11-09 9:09 ` Mark Cave-Ayland
2026-11-09 10:19 ` Markus Armbruster
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.