Re: [PATCH 4/8] hw/gpio/pca9554: New abstract base "pca9554-base"

[Markus Armbruster <armbru@redhat.com>]

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.