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

[BALATON Zoltan]

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.

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);