Re: [PATCH v2 2/6] hw/gpio/aspeed_sgpio: aspeed: Add QOM property accessors for SGPIO pins

[Yubin Zou <yubinz@google.com>]

[-- Attachment #1: Type: text/plain, Size: 4705 bytes --]

Hi Cédric

Good catch. I will fix these issues.

Yubin

On Tue, Dec 9, 2025 at 6:52 AM Cédric Le Goater <clg@redhat.com> wrote:

> Hi,
>
> The subject needs a fix. Please remove the extra 'aspeed: '.
>
> On 12/9/25 01:01, Yubin Zou wrote:
> > This commit adds QOM property accessors for the Aspeed SGPIO pins.
>
> I think you can drop the above sentence from the commit log. It's
> redundant with the subject.
>
> > The `aspeed_sgpio_get_pin` and `aspeed_sgpio_set_pin` functions are
> > implemented to get and set the level of individual SGPIO pins. These
> > are then exposed as boolean properties on the SGPIO device object.
> >
> > Signed-off-by: Yubin Zou <yubinz@google.com>
> > ---
> >   hw/gpio/aspeed_sgpio.c | 78
> ++++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 78 insertions(+)
> >
> > diff --git a/hw/gpio/aspeed_sgpio.c b/hw/gpio/aspeed_sgpio.c
> > index
> 8676fa7ced134f1f62dc9e30b42c5fe6db3de268..efa7e574abe87e33e58ac88dba5e3469c6702b83
> 100644
> > --- a/hw/gpio/aspeed_sgpio.c
> > +++ b/hw/gpio/aspeed_sgpio.c
> > @@ -91,6 +91,73 @@ static void aspeed_sgpio_2700_write(void *opaque,
> hwaddr offset, uint64_t data,
> >       }
> >   }
> >
> > +static bool aspeed_sgpio_get_pin_level(AspeedSGPIOState *s, int pin)
> > +{
> > +    uint32_t value = s->ctrl_regs[pin >> 1];
> > +    bool is_input = !(pin % 2);
> > +    uint32_t bit_mask = 0;
> > +
> > +    if (is_input) {
> > +        bit_mask = SGPIO_SERIAL_IN_VAL_MASK;
> > +    } else {
> > +        bit_mask = SGPIO_SERIAL_OUT_VAL_MASK;
> > +    }
> > +
> > +    return value & bit_mask;
> > +}
> > +
> > +static void aspeed_sgpio_set_pin_level(AspeedSGPIOState *s, int pin,
> bool level)
> > +{
> > +    uint32_t value = s->ctrl_regs[pin >> 1];
> > +    bool is_input = !(pin % 2);
> > +    uint32_t bit_mask = 0;
> > +
> > +    if (is_input) {
> > +        bit_mask = SGPIO_SERIAL_IN_VAL_MASK;
> > +    } else {
> > +        bit_mask = SGPIO_SERIAL_OUT_VAL_MASK;
> > +    }
> > +
> > +    if (level) {
> > +        value |= bit_mask;
> > +    } else {
> > +        value &= ~bit_mask;
> > +    }
> > +    s->ctrl_regs[pin >> 1] = value;
> > +}
> > +
> > +static void aspeed_sgpio_get_pin(Object *obj, Visitor *v, const char
> *name,
> > +                                void *opaque, Error **errp)
> > +{
> > +    bool level = true;
> > +    int pin = 0xfff;
> > +    AspeedSGPIOState *s = ASPEED_SGPIO(obj);
> > +
> > +    if (sscanf(name, "sgpio%d", &pin) != 1) {
> > +        error_setg(errp, "%s: error reading %s", __func__, name);
> > +        return;
> > +    }
> > +    level = aspeed_sgpio_get_pin_level(s, pin);
> > +    visit_type_bool(v, name, &level, errp);
> > +}
> > +
> > +static void aspeed_sgpio_set_pin(Object *obj, Visitor *v, const char
> *name,
> > +                                void *opaque, Error **errp)
> > +{
> > +    bool level;
> > +    int pin = 0xfff;
> > +    AspeedSGPIOState *s = ASPEED_SGPIO(obj);
> > +
> > +    if (!visit_type_bool(v, name, &level, errp)) {
> > +        return;
> > +    }
> > +    if (sscanf(name, "sgpio%d", &pin) != 1) {
> > +        error_setg(errp, "%s: error reading %s", __func__, name);
> > +        return;
> > +    }
> > +    aspeed_sgpio_set_pin_level(s, pin, level);
> > +}
> > +
> >   static const MemoryRegionOps aspeed_gpio_2700_ops = {
> >     .read       = aspeed_sgpio_2700_read,
> >     .write      = aspeed_sgpio_2700_write,
> > @@ -114,6 +181,16 @@ static void aspeed_sgpio_realize(DeviceState *dev,
> Error **errp)
> >       sysbus_init_mmio(sbd, &s->iomem);
> >   }
> >
> > +static void aspeed_sgpio_init(Object *obj)
> > +{
> > +    for (int i = 0; i < ASPEED_SGPIO_MAX_PIN_PAIR * 2; i++) {
> > +        char *name = g_strdup_printf("sgpio%d", i);
>
> You could use a g_autofree variable and drop the g_free below.
>
> How about using a "%03d" format in the printf and sscanf too ?
>
> C.
>
> > +        object_property_add(obj, name, "bool", aspeed_sgpio_get_pin,
> > +                            aspeed_sgpio_set_pin, NULL, NULL);
> > +        g_free(name);
> > +    }
> > +}
> > +
> >   static void aspeed_sgpio_class_init(ObjectClass *klass, const void
> *data)
> >   {
> >       DeviceClass *dc = DEVICE_CLASS(klass);
> > @@ -143,6 +220,7 @@ static const TypeInfo aspeed_sgpio_ast2700_info = {
> >     .name           = TYPE_ASPEED_SGPIO "-ast2700",
> >     .parent         = TYPE_ASPEED_SGPIO,
> >     .class_init     = aspeed_sgpio_2700_class_init,
> > +  .instance_init  = aspeed_sgpio_init,
> >   };
> >
> >   static void aspeed_sgpio_register_types(void)
> >
>
>

[-- Attachment #2: Type: text/html, Size: 6095 bytes --]