Re: [RFC PATCH 1/3] hw/i2c/smbus_eeprom: Set QOM parent

[BALATON Zoltan]

On Fri, 26 Jun 2020, BALATON Zoltan wrote:
> On Fri, 26 Jun 2020, Philippe Mathieu-Daudé wrote:
> > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> > Aspeed change pending latest ARM pull-request, so meanwhile sending
> > as RFC.
> > ---
> > include/hw/i2c/smbus_eeprom.h |  9 ++++++---
> > hw/i2c/smbus_eeprom.c         | 13 ++++++++++---
> > hw/mips/fuloong2e.c           |  2 +-
> > hw/ppc/sam460ex.c             |  2 +-
> > 4 files changed, 18 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/hw/i2c/smbus_eeprom.h b/include/hw/i2c/smbus_eeprom.h
> > index 68b0063ab6..037612bbbb 100644
> > --- a/include/hw/i2c/smbus_eeprom.h
> > +++ b/include/hw/i2c/smbus_eeprom.h
> > @@ -26,9 +26,12 @@
> > #include "exec/cpu-common.h"
> > #include "hw/i2c/i2c.h"
> >
> > -void smbus_eeprom_init_one(I2CBus *bus, uint8_t address, uint8_t 
> > *eeprom_buf);
> > -void smbus_eeprom_init(I2CBus *bus, int nb_eeprom,
> > -                       const uint8_t *eeprom_spd, int size);
> > +void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
> > +                           I2CBus *smbus, uint8_t address,
> > +                           uint8_t *eeprom_buf);
> > +void smbus_eeprom_init(Object *parent_obj, const char *child_name_prefix,
> > +                       I2CBus *smbus, int nb_eeprom,
> > +                       const uint8_t *eeprom_spd, int eeprom_spd_size);
>
> Keeping I2CBus *smbus and uint8_t address as first parameters before 
> parent_obj and name looks better to me. These functions still operate on an 
> I2Cbus so could be regarded as methods of I2CBus therefore first parameter 
> should be that.

Also isn't parent_obj is the I2Cbus itself? Why is that need to be passed? 
The i2c_init_bus() also takes parent and name params so both I2Cbus and 
it's parent should be available as parents of the new I2C device here 
without more parameters. What am I missing here?

> Regards,
> BALATON Zoltan
>
> > enum sdram_type { SDR = 0x4, DDR = 0x7, DDR2 = 0x8 };
> > uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size);
> > diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> > index b7def9eeb8..879fd7c416 100644
> > --- a/hw/i2c/smbus_eeprom.c
> > +++ b/hw/i2c/smbus_eeprom.c
> > @@ -165,7 +165,9 @@ static void smbus_eeprom_register_types(void)
> >
> > type_init(smbus_eeprom_register_types)
> >
> > -void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t 
> > *eeprom_buf)
> > +void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
> > +                           I2CBus *smbus, uint8_t address,
> > +                           uint8_t *eeprom_buf)
> > {
> >     DeviceState *dev;
> >
> > @@ -173,10 +175,12 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t 
> > address, uint8_t *eeprom_buf)
> >     qdev_prop_set_uint8(dev, "address", address);
> >     /* FIXME: use an array of byte or block backend property? */
> >     SMBUS_EEPROM(dev)->init_data = eeprom_buf;
> > +    object_property_add_child(parent_obj, child_name, OBJECT(dev));
> >     qdev_realize_and_unref(dev, (BusState *)smbus, &error_fatal);
> > }
> >
> > -void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
> > +void smbus_eeprom_init(Object *parent_obj, const char *child_name_prefix,
> > +                       I2CBus *smbus, int nb_eeprom,
> >                        const uint8_t *eeprom_spd, int eeprom_spd_size)
> > {
> >     int i;
> > @@ -189,8 +193,11 @@ void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
> >     }
> >
> >     for (i = 0; i < nb_eeprom; i++) {
> > -        smbus_eeprom_init_one(smbus, 0x50 + i,
> > +        char *name = g_strdup_printf("%s-%d", child_name_prefix, i);
> > +
> > +        smbus_eeprom_init_one(parent_obj, name, smbus, 0x50 + i,
> >                               eeprom_buf + (i * SMBUS_EEPROM_SIZE));
> > +        g_free(name);
> >     }
> > }
> >
> > diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
> > index 8ca31e5162..304a096c6a 100644
> > --- a/hw/mips/fuloong2e.c
> > +++ b/hw/mips/fuloong2e.c
> > @@ -377,7 +377,7 @@ static void mips_fuloong2e_init(MachineState *machine)
> >
> >     /* Populate SPD eeprom data */
> >     spd_data = spd_data_generate(DDR, machine->ram_size);
> > -    smbus_eeprom_init_one(smbus, 0x50, spd_data);
> > +    smbus_eeprom_init_one(OBJECT(machine->ram), "spd", smbus, 0x50, 
> > spd_data);
> >
> >     mc146818_rtc_init(isa_bus, 2000, NULL);
> >
> > diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> > index 1a106a68de..064d07f4e2 100644
> > --- a/hw/ppc/sam460ex.c
> > +++ b/hw/ppc/sam460ex.c
> > @@ -337,7 +337,7 @@ static void sam460ex_init(MachineState *machine)
> >     spd_data = spd_data_generate(ram_sizes[0] < 128 * MiB ? DDR : DDR2,
> >                                  ram_sizes[0]);
> >     spd_data[20] = 4; /* SO-DIMM module */
> > -    smbus_eeprom_init_one(i2c, 0x50, spd_data);
> > +    smbus_eeprom_init_one(OBJECT(machine->ram), "spd", i2c, 0x50, 
> > spd_data);
> >     /* RTC */
> >     i2c_create_slave(i2c, "m41t80", 0x68);