Re: [PATCH] aspeed/i2c: QOMify AspeedI2CBus

[Cédric Le Goater]

On 9/7/21 9:40 AM, Philippe Mathieu-Daudé wrote:
> On 9/7/21 9:30 AM, Cédric Le Goater wrote:
>> The AST2600 SoC realize routine needs to be adapted when connecting
>> the I2C bus IRQs because the bus IRQs have moved to the AspeedI2CBus
>> SysBus device, one level below the Aspeed I2C controller SysBus
>> device.
> 
> When did they move?

With this patch. I will rephrase.

> 
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  include/hw/i2c/aspeed_i2c.h |   8 ++-
>>  hw/arm/aspeed_ast2600.c     |   7 +--
>>  hw/i2c/aspeed_i2c.c         | 103 +++++++++++++++++++++++++++++-------
>>  3 files changed, 93 insertions(+), 25 deletions(-)
> 
>> +static void aspeed_i2c_bus_realize(DeviceState *dev, Error **errp)
>> +{
>> +    AspeedI2CBus *s = ASPEED_I2C_BUS(dev);
>> +    char name[32];
>> +    AspeedI2CClass *aic;
>> +
>> +    if (!s->controller) {
>> +        error_setg(errp, TYPE_ASPEED_I2C_BUS ": 'controller' link not set");
>> +        return;
>> +    }
>> +
>> +    aic = ASPEED_I2C_GET_CLASS(s->controller);
>> +
>> +    snprintf(name, sizeof(name), TYPE_ASPEED_I2C_BUS ".%d", s->id);
> 
> Even if this particular case is safe, it is better to use:
> 
>   g_autofree char *name = g_strdup_printf(TYPE_ASPEED_I2C_BUS ".%d",
>                                           s->id);
> 
> So future developer copying your device code will be safe if they
> modify the format :)


Yeah. I kind of like snprintf but I agree g_strdup_printf() is as good
with the g_autofree variables. I will change the patches.

Thanks,

C. 


>> +
>> +    sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
>> +
>> +    s->bus = i2c_init_bus(dev, name);
>> +
>> +    memory_region_init_io(&s->mr, OBJECT(s), &aspeed_i2c_bus_ops,
>> +                          s, name, aic->reg_size);
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mr);
>> +}