Re: [PATCH v2 1/5] hw/arm: Extract at24c_eeprom_init helper from Aspeed and Nuvoton boards

[Peter Delevoryas]

On Tue, Jan 17, 2023 at 10:32:15AM -0800, Peter Delevoryas wrote:
> On Tue, Jan 17, 2023 at 09:00:34AM +0100, Philippe Mathieu-Daudé wrote:
> > On 17/1/23 00:56, Peter Delevoryas wrote:
> > > This helper is useful in board initialization because lets users 
> > > initialize and
> > > realize an EEPROM on an I2C bus with a single function call.
> > > 
> > > Signed-off-by: Peter Delevoryas <peter@pjd.dev>
> > > ---
> > >   hw/arm/aspeed.c                 | 10 +---------
> > >   hw/arm/npcm7xx_boards.c         | 20 +++++---------------
> > >   hw/nvram/eeprom_at24c.c         | 12 ++++++++++++
> > >   include/hw/nvram/eeprom_at24c.h | 10 ++++++++++
> > >   4 files changed, 28 insertions(+), 24 deletions(-)
> > >   create mode 100644 include/hw/nvram/eeprom_at24c.h
> > 
> > > diff --git a/include/hw/nvram/eeprom_at24c.h 
> > > b/include/hw/nvram/eeprom_at24c.h
> > > new file mode 100644
> > > index 000000000000..79a36b53ca87
> > > --- /dev/null
> > > +++ b/include/hw/nvram/eeprom_at24c.h
> > > @@ -0,0 +1,10 @@
> > > +/* Copyright (c) Meta Platforms, Inc. and affiliates. */
> > 
> > What license for this copyright?
> 
> Erg, yeah, thanks for calling this out, I did this wrong. Meta has some new
> licensing guidelines and I misinterpreted them. Contributors are just supposed
> to use whatever license the open-source project has, so I'll just change this
> to say it's under GPL2, like the one I used in hw/arm/fby35.c
> 
> > 
> > > +#ifndef EEPROM_AT24C_H
> > > +#define EEPROM_AT24C_H
> > > +
> > > +#include "hw/i2c/i2c.h"
> > 
> >  /**
> >   * Create and realize an AT24C EEPROM device on the heap.
> >   * @bus: I2C bus to put it on
> >   * @addr: I2C address of the EEPROM slave when put on a bus
> >   * @rom_size: size of the EEPROM
> >   *
> >   * Create the device state structure, initialize it, put it on
> >   * the specified @bus, and drop the reference to it (the device
> >   * is realized).
> >   */
> >  I2CSlave *at24c_eeprom_create_simple(I2CBus *bus, uint8_t addr,
> >                                       size_t rom_size);
> 
> +1, I'll include this comment

Oh, to clarify though: I'm not going to include the rename to the function,
maybe we could do that separately? I kinda want to avoid touching all the
at24c_eeprom_init calls unless I really need to. I know it's just a simple sed,
but also, smbus_eeprom_init is using the "init" suffix, so I'm not sure it's
consistent, although "create_simple" probably _is_ more consistent with devices
in general in QEMU. But anyways, main point, I just want to avoid making any
unnecessary refactoring here, and renaming it completely seems unnecessary.

> 
> > 
> > > +I2CSlave *at24c_eeprom_init(I2CBus *bus, uint8_t address, uint32_t 
> > > rom_size);
> > > +
> > > +#endif
> > 
>