[PATCH v3 1/4] common: Add generic function for reading serial number

[Simon Glass]

Hi Artur,

On Wed, 4 Oct 2023 at 06:47, Artur Rojek <artur at conclusive.pl> wrote:
>
> >Hi Artur,
> >
> >On Mon, 2 Oct 2023 at 06:42, Artur Rojek <artur at conclusive.pl> wrote:
> >>
> >> Provide a generic way for boards to read their serial number from EEPROM
> >> at init.
> >>
> >> If CONFIG_ID_EEPROM is set, the new serial_read_from_eeprom() function
> >> will now be called during the post-relocation part of the board init.
> >>
> >> Provided is the tlv eeprom implementation of the above function, making
> >> use of the existing, yet never utilized, populate_serial_number().
> >> Boards which use custom logic for interaction with their EEPROMs need to
> >> supply their own implementation.
> >>
> >> Signed-off-by: Artur Rojek <artur at conclusive.pl>
> >> ---
> >>
> >> v3: - restore original function name and make it static
> >>     - provide a generic function for reading EEPROM serial number and
> >>       wrap it around the existing tlv logic
> >>     - move the env var check out of populate_serial_number() and into
> >>       the new serial_read_from_eeprom() in order to stay consistent with
> >>       the documentation
> >>
> >> v2: - rename the function
> >>     - move function documentation from .c file to the prototype location
> >>
> >>  cmd/tlv_eeprom.c | 25 +++++++++----------------
> >>  common/board_r.c |  8 ++++++++
> >>  include/init.h   | 14 ++++++++++++++
> >>  3 files changed, 31 insertions(+), 16 deletions(-)
> >
> >Can you please use events for this? Something like EVT_SETTINGS_R ?
> >
> >See the one recently added for how to do this:
> >
> >INITCALL_EVENT(EVT_LAST_STAGE_INIT),
>
> I like this approach, but just to be clear with your intention - you
> want me to move both serial_read_from_eeprom AND mac_read_from_eeprom
> into a separate function, defined for each affected board? To do this
> for mac_read_from_eeprom becomes slightly cumbersome, because there are
> this many of them, depending on the current scheme:
>
> $ grep -r "ID_EEPROM" ./configs/ | wc -l
> 55
>
> If each of them needs to contain something like this:
>
> >static int settings_r(void)
> >{
> >#if defined(CONFIG_ID_EEPROM)
> >       mac_read_from_eeprom();
> >#endif
> >       return 0;
> >}
> >EVENT_SPY_SIMPLE(EVT_SETTINGS_R, settings_r);
>
> then this strays very far from the original intention of this series,
> which is to add support for a single board :)
> Unless you only care about serial_read_from_eeprom, then I don't need to
> modify any of the existing boards.

As per irc, you don't need to adjust the mac_read_from_eeprom(). Just
start your own thing (read_settings() or whatever) - i.e. don't make
it any worse. People do appreciate cleanup, but sometimes it can be a
big task

Regards,
Simon