Hi Cédric Good catch. I will fix these issues.
Yubin On Tue, Dec 9, 2025 at 6:52 AM Cédric Le Goater <[email protected]> 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 <[email protected]> > > --- > > 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) > > > >
