Re: [PATCH 1/6] hw/nvram/eeprom_at24c: Add header w/ init helper

[Peter Delevoryas]

On Mon, Jan 16, 2023 at 01:23:01PM +0100, Philippe Mathieu-Daudé wrote:
> On 14/1/23 18:01, Peter Delevoryas wrote:
> > Signed-off-by: Peter Delevoryas <peter@pjd.dev>
> > ---
> >   hw/nvram/eeprom_at24c.c         | 10 ++++++++++
> >   include/hw/nvram/eeprom_at24c.h | 10 ++++++++++
> >   2 files changed, 20 insertions(+)
> >   create mode 100644 include/hw/nvram/eeprom_at24c.h
> 
> > +void at24c_eeprom_init(I2CBus *bus, uint8_t address, uint32_t rom_size)
> > +{
> > +    I2CSlave *i2c_dev = i2c_slave_new("at24c-eeprom", address);
> 
> Please use the type definition: TYPE_AT24C_EE.
> 
> > +    DeviceState *dev = DEVICE(i2c_dev);
> > +
> > +    qdev_prop_set_uint32(dev, "rom-size", rom_size);
> > +    i2c_slave_realize_and_unref(i2c_dev, bus, &error_abort);
> 
> Although the allocated object is somehow reachable from the i2c bus
> object, it would be simpler to deallocate allowing the parent to keep
> a reference to it. So consider this prototype instead:
> 
>   I2CSlave *at24c_eeprom_create(I2CBus *bus, uint8_t address,
>                                 uint32_t rom_size);
> 

Oh ok, yeah that sounds good. In this case, if I let the parent keep a
reference, maybe I shouldn't use i2c_slave_realize_and_unref, and just use
qdev_realize/etc (to avoid the unref?). I'll try just returning the pointer
from the function to start with though.

> > +}