Reviewed-by: Glenn Miles <[email protected]>
Thanks,
Glenn
On Tue, 2026-10-20 at 14:46 +0400, Marc-André Lureau wrote:
> Replace hand-rolled string lookup tables with QAPI enums
> Pca9552LedState and Pca955{2,4}PinState for the led%d and pin%d QOM
> properties. This fixes the incorrect property type ("bool" for LEDs,
> "str" for pins) and 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.
>
> I kept Pca9552PinState & @Pca9554PinState separate types, because I
> don't know if they could diverge. We could eventually merge those.
>
> Signed-off-by: Markus Armbruster <[email protected]>
> Signed-off-by: Marc-André Lureau <[email protected]>
> ---
> hw/gpio/pca9552.c | 82
> +++++++++++++++++++++----------------------------------
> hw/gpio/pca9554.c | 39 ++++++++++----------------
> qapi/machine.json | 46 +++++++++++++++++++++++++++++++
> 3 files changed, 92 insertions(+), 75 deletions(-)
>
> diff --git a/hw/gpio/pca9552.c b/hw/gpio/pca9552.c
> index 719149b7174b..16741c22dea3 100644
> --- a/hw/gpio/pca9552.c
> +++ b/hw/gpio/pca9552.c
> @@ -23,7 +23,8 @@
> #include "hw/core/irq.h"
> #include "migration/vmstate.h"
> #include "qapi/error.h"
> -#include "qapi/visitor.h"
> +#include "qapi/qapi-types-machine.h"
> +#include "qapi/qapi-visit-machine.h"
> #include "trace.h"
> #include "qom/object.h"
>
> @@ -59,16 +60,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(PCA9552_PIN_STATE_LOW != 0x0);
> +QEMU_BUILD_BUG_ON(PCA9552_PIN_STATE_HIGH != 0x1);
>
> static uint8_t pca955x_pin_get_config(PCA955xState *s, int pin)
> {
> @@ -142,24 +142,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] == PCA9552_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;
> @@ -176,7 +176,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] == PCA9552_PIN_STATE_LOW) {
> s->regs[input_reg] &= ~bit_mask;
> } else {
> s->regs[input_reg] |= bit_mask;
> @@ -360,7 +360,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) {
> @@ -378,7 +378,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);
> }
>
> /*
> @@ -397,10 +397,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);
> @@ -413,16 +412,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);
> @@ -437,7 +426,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;
> + Pca9552PinState state;
>
> rc = sscanf(name, "pin%2d", &pin);
> if (rc != 1) {
> @@ -455,7 +445,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_Pca9552PinState(v, name, &state, errp);
> }
>
> static void pca955x_set_pin(Object *obj, Visitor *v, const char *name,
> @@ -464,10 +454,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;
> + Pca9552PinState state;
> + uint8_t config_reg;
>
> - if (!visit_type_str(v, name, &state_str, errp)) {
> + if (!visit_type_Pca9552PinState(v, name, &state, errp)) {
> return;
> }
> rc = sscanf(name, "pin%2d", &pin);
> @@ -480,16 +470,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)) {
> @@ -499,7 +479,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 != PCA9552_PIN_STATE_LOW);
> }
>
> static const VMStateDescription pca9552_vmstate = {
> @@ -529,7 +509,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, PCA9552_PIN_STATE_HIGH, PCA955X_PIN_COUNT_MAX);
> pca955x_update_pin_input(s);
>
> s->pointer = 0xFF;
> @@ -549,7 +529,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, PCA9552_PIN_STATE_HIGH, PCA955X_PIN_COUNT_MAX);
> pca955x_update_pin_input(s);
>
> s->pointer = 0xFF;
> @@ -567,12 +547,12 @@ 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, "bool",
> + object_property_add(obj, name, "Pca9552LedState",
> 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",
> + object_property_add(obj, name, "Pca9552PinState",
> pca955x_get_pin, pca955x_set_pin, NULL,
> NULL);
> }
> g_free(name);
> diff --git a/hw/gpio/pca9554.c b/hw/gpio/pca9554.c
> index 904698cdce85..e4eddc829ef7 100644
> --- a/hw/gpio/pca9554.c
> +++ b/hw/gpio/pca9554.c
> @@ -16,6 +16,8 @@
> #include "hw/core/irq.h"
> #include "migration/vmstate.h"
> #include "qapi/error.h"
> +#include "qapi/qapi-types-machine.h"
> +#include "qapi/qapi-visit-machine.h"
> #include "qapi/visitor.h"
> #include "trace.h"
> #include "qom/object.h"
> @@ -32,10 +34,8 @@ typedef struct PCA9554Class PCA9554Class;
> DECLARE_CLASS_CHECKERS(PCA9554Class, PCA9554,
> TYPE_PCA9554)
>
> -#define PCA9554_PIN_LOW 0x0
> -#define PCA9554_PIN_HIZ 0x1
> -
> -static const char *pin_state[] = {"low", "high"};
> +QEMU_BUILD_BUG_ON(PCA9554_PIN_STATE_LOW != 0x0);
> +QEMU_BUILD_BUG_ON(PCA9554_PIN_STATE_HIGH != 0x1);
>
> static void pca9554_update_pin_input(PCA9554State *s)
> {
> @@ -54,7 +54,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] == PCA9554_PIN_STATE_LOW) {
> s->regs[PCA9554_INPUT] &= ~bit_mask;
> } else {
> s->regs[PCA9554_INPUT] |= bit_mask;
> @@ -156,7 +156,7 @@ static void pca9554_get_pin(Object *obj, Visitor *v,
> const char *name,
> {
> PCA9554State *s = PCA9554(obj);
> int pin, rc;
> - uint8_t state;
> + Pca9554PinState state;
>
> rc = sscanf(name, "pin%2d", &pin);
> if (rc != 1) {
> @@ -175,7 +175,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_Pca9554PinState(v, name, &state, errp);
> }
>
> static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
> @@ -183,10 +183,10 @@ static void pca9554_set_pin(Object *obj, Visitor *v,
> const char *name,
> {
> PCA9554State *s = PCA9554(obj);
> int pin, rc, val;
> - uint8_t state, mask;
> - g_autofree char *state_str = NULL;
> + uint8_t mask;
> + Pca9554PinState state;
>
> - if (!visit_type_str(v, name, &state_str, errp)) {
> + if (!visit_type_Pca9554PinState(v, name, &state, errp)) {
> return;
> }
> rc = sscanf(name, "pin%2d", &pin);
> @@ -199,16 +199,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)) {
> @@ -219,13 +209,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 != PCA9554_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 == PCA9554_PIN_STATE_LOW) {
> val &= ~(mask);
> } else {
> val |= mask;
> @@ -264,7 +254,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, PCA9554_PIN_STATE_HIGH, pc->pin_count);
> pca9554_update_pin_input(s);
>
> s->pointer = 0x0;
> @@ -280,7 +270,8 @@ static void pca9554_initfn(Object *obj)
> char *name;
>
> name = g_strdup_printf("pin%d", pin);
> - object_property_add(obj, name, "str", pca9554_get_pin,
> pca9554_set_pin,
> + object_property_add(obj, name, "Pca9554PinState",
> + pca9554_get_pin, pca9554_set_pin,
> NULL, NULL);
> g_free(name);
> }
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 34be3156270c..46b6935ab0b8 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -416,6 +416,52 @@
> { 'enum': 'LostTickPolicy',
> 'data': ['discard', 'delay', 'slew' ] }
>
> +##
> +# @Pca9552LedState:
> +#
> +# LED state for PCA9552.
> +#
> +# @on: LED on
> +#
> +# @off: LED off
> +#
> +# @pwm0: LED controlled by PWM0
> +#
> +# @pwm1: LED controlled by PWM1
> +#
> +# Since: 11.2
> +##
> +{ 'enum': 'Pca9552LedState',
> + 'data': ['on', 'off', 'pwm0', 'pwm1'] }
> +
> +##
> +# @Pca9552PinState:
> +#
> +# Pin state for PCA9552.
> +#
> +# @low: Pin low
> +#
> +# @high: Pin high
> +#
> +# Since: 11.2
> +##
> +{ 'enum': 'Pca9552PinState',
> + 'data': ['low', 'high'] }
> +
> +##
> +# @Pca9554PinState:
> +#
> +# Pin state for PCA9554.
> +#
> +# @low: Pin low
> +#
> +# @high: Pin high
> +#
> +# Since: 11.2
> +##
> +{ 'enum': 'Pca9554PinState',
> + 'data': ['low', 'high'] }
> +
> ##
> # @inject-nmi:
> # Injects a Non-Maskable Interrupt (machine specific: for example
>
