| Message ID | 20230114170151.87833-2-peter@pjd.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/nvram/eeprom_at24c: Cleanup + FRUID EEPROM init example | expand |
On 1/14/23 18:01, Peter Delevoryas wrote: > Signed-off-by: Peter Delevoryas <peter@pjd.dev> Please add some short commit log explaining how the helper could be useful. Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C. > --- > 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 > > diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c > index 2d4d8b952f38..0c27eae2b354 100644 > --- a/hw/nvram/eeprom_at24c.c > +++ b/hw/nvram/eeprom_at24c.c > @@ -12,6 +12,7 @@ > #include "qapi/error.h" > #include "qemu/module.h" > #include "hw/i2c/i2c.h" > +#include "hw/nvram/eeprom_at24c.h" > #include "hw/qdev-properties.h" > #include "hw/qdev-properties-system.h" > #include "sysemu/block-backend.h" > @@ -128,6 +129,15 @@ int at24c_eeprom_send(I2CSlave *s, uint8_t data) > return 0; > } > > +void at24c_eeprom_init(I2CBus *bus, uint8_t address, uint32_t rom_size) > +{ > + I2CSlave *i2c_dev = i2c_slave_new("at24c-eeprom", address); > + DeviceState *dev = DEVICE(i2c_dev); > + > + qdev_prop_set_uint32(dev, "rom-size", rom_size); > + i2c_slave_realize_and_unref(i2c_dev, bus, &error_abort); > +} > + > static void at24c_eeprom_realize(DeviceState *dev, Error **errp) > { > EEPROMState *ee = AT24C_EE(dev); > diff --git a/include/hw/nvram/eeprom_at24c.h b/include/hw/nvram/eeprom_at24c.h > new file mode 100644 > index 000000000000..9d9cf212757c > --- /dev/null > +++ b/include/hw/nvram/eeprom_at24c.h > @@ -0,0 +1,10 @@ > +/* Copyright (c) Meta Platforms, Inc. and affiliates. */ > + > +#ifndef EEPROM_AT24C_H > +#define EEPROM_AT24C_H > + > +#include "hw/i2c/i2c.h" > + > +void at24c_eeprom_init(I2CBus *bus, uint8_t address, uint32_t rom_size); > + > +#endif
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); > +}
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. > > +}
diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c index 2d4d8b952f38..0c27eae2b354 100644 --- a/hw/nvram/eeprom_at24c.c +++ b/hw/nvram/eeprom_at24c.c @@ -12,6 +12,7 @@ #include "qapi/error.h" #include "qemu/module.h" #include "hw/i2c/i2c.h" +#include "hw/nvram/eeprom_at24c.h" #include "hw/qdev-properties.h" #include "hw/qdev-properties-system.h" #include "sysemu/block-backend.h" @@ -128,6 +129,15 @@ int at24c_eeprom_send(I2CSlave *s, uint8_t data) return 0; } +void at24c_eeprom_init(I2CBus *bus, uint8_t address, uint32_t rom_size) +{ + I2CSlave *i2c_dev = i2c_slave_new("at24c-eeprom", address); + DeviceState *dev = DEVICE(i2c_dev); + + qdev_prop_set_uint32(dev, "rom-size", rom_size); + i2c_slave_realize_and_unref(i2c_dev, bus, &error_abort); +} + static void at24c_eeprom_realize(DeviceState *dev, Error **errp) { EEPROMState *ee = AT24C_EE(dev); diff --git a/include/hw/nvram/eeprom_at24c.h b/include/hw/nvram/eeprom_at24c.h new file mode 100644 index 000000000000..9d9cf212757c --- /dev/null +++ b/include/hw/nvram/eeprom_at24c.h @@ -0,0 +1,10 @@ +/* Copyright (c) Meta Platforms, Inc. and affiliates. */ + +#ifndef EEPROM_AT24C_H +#define EEPROM_AT24C_H + +#include "hw/i2c/i2c.h" + +void at24c_eeprom_init(I2CBus *bus, uint8_t address, uint32_t rom_size); + +#endif
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