Reviewed-by: Glenn Miles <[email protected]> Thanks,
Glenn On Fri, 2026-11-06 at 12:04 +0100, Markus Armbruster wrote: > From: Marc-André Lureau <[email protected]> > > 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 <[email protected]> > --- > 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;
