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: 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 d418b34a8643..0e0d85d0a76d 100644 --- a/qapi/machine.json +++ b/qapi/machine.json @@ -429,6 +429,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: # -- 2.55.0.543.g5ebe2ebe4ea8
