Re: [PATCH v4 3/5] hw/nvram/eeprom_at24c: Add init_rom field and at24c_eeprom_init_rom helper

[Peter Delevoryas]

On Fri, Jan 27, 2023 at 08:42:40AM +0100, Cédric Le Goater wrote:
> > > >   I2CSlave *at24c_eeprom_init(I2CBus *bus, uint8_t address, uint32_t 
> > > > rom_size)
> > > >   {
> > > > -    I2CSlave *i2c_dev = i2c_slave_new(TYPE_AT24C_EE, address);
> > > > -    DeviceState *dev = DEVICE(i2c_dev);
> > > > +    return at24c_eeprom_init_rom(bus, address, rom_size, NULL, 0);
> > > > +}
> > > > +
> > > > +I2CSlave *at24c_eeprom_init_rom(I2CBus *bus, uint8_t address, uint32_t 
> > > > rom_size,
> > > > +                                const uint8_t *init_rom, uint32_t 
> > > > init_rom_size)
> > > > +{
> > > > +    EEPROMState *s;
> > > > +
> > > > +    s = AT24C_EE(qdev_new(TYPE_AT24C_EE));
> > > > +
> > > > +    qdev_prop_set_uint8(DEVICE(s), "address", address);
> > > 
> > > Why did you switch from using i2c_slave_new()?  Using it is more
> > > documentation and future-proofing than convenience.
> > 
> > Oh, yeah that's my bad. I was probably doing it so that all the 
> > qdev_prop_set
> > calls to the object are in the same place, but I probably should have just 
> > kept
> > i2c_slave_new() and initialized only the at24c-eeprom properties here, 
> > instead
> > of initializing the I2CSlave ones too.
> 
> Will you send a v5 ?

Oh! Yeah sure: I thought I had already missed the boat because I thought you
sent a pull request, but I see that it hasn't been pulled yet.

- Peter

> 
> Thanks,
> 
> C.