* Now one has to explicitly pass the device used if there are multiple EEPROM available. This changes quite a bit the existing API and will require user code to be updated.

Like for every other RIOT-API. Citing the Guide for writing a device driver in RIOT:

Additionally towards ease of use, all device drivers in RIOT should provide a similar 'look and feel'.

• To remove the need of the extra eeprom_t, how about only use one EEPROM at a time ? An external one being used instead of the CPU one if present ?

Citing again from the RIOT driver guide:

Third, it should always be possible, to handle more than a single device of one kind. Drivers and their interfaces are thus designed to keep their state information in a parameterized location instead of driver defined global variables.

So this is against RIOT's policy.

* Another solution could be to "merge" all available EEPROM into a single global memory ?

This is super complex. Image one has three EEPROM devices. Now the order of the initialization is changed --> the memory layout is messed up. Or one of the three fails to initialize. What will the memory layout look like? There more I think about it the more pitfall and corner cases pop up.

Citing the Guide for writing a device driver in RIOT

Make sense. I just wanted to discuss possible alternatives.
The current design makes it little more complex for the calling code which has to deal with available EEPROM source.

What's the verdict on this one?

I'm fine with this API.
If the concern is that this adds too much overhead/unneeded indirection because nobody will ever use more than one EEPROM at a time, I can also draft a header-only api (with some common support code) like what we have for the watchdog.

I also provide a PR as fall back wrapper for the existing API, that simple uses the first EEPROM device available - that way external applications could work without being updated.

This could also include a fast path to directly (or with a thin wrapper) call the periph_eeprom functions if no external EEPROM modules are used.

OK. I split out initialization of this PR entirely and added a function void eeprom_register(const eeprom_driver_t *driver, void *handle, eeprom_off_t size); to registers devices to the common API instead. I think this could be a good compromise.

I like the current approach.

I still don't really like the approach of registration. Usually in RIOT all hardware components are defined by static initializers. Why not use the same for the configuration of existing EEPROM devices? If _eeprom_devs[] would be declared by the board or the application like following

static const eeprom_dev_t _eeprom_devs[] = {
    {
      .driver = abc_eeprom_driver,
      .handle = &abc_eeprom,
      .params = &abc_eeprom_params,
      .size = ABC_EEPROM_SIZE
    },
    {
      .driver = xyz_eeprom_driver,
      .handle = &xyz_eeprom,
      .params = &xyz_eeprom_params,
      .size = XYZ_EEPROM_SIZE
    },
};

the init function of the API could iterate over all EEPROM devices and could call

    _eeprom_driver(dev)->init(_eeprom_handle(dev), _eeprom_params(dev));

The init function could be called in sys/auto_init.

This approach would have the advantages

• that initialization of all EEPROMs would be done automatically
• the application has not take care what functions to call and when
• the EEPROM_NUM_OF would be simply `ARRAY_SIZE(_eeprom_devs)

FYI: Some EEPROMs provide an EUI-48 or EUI-64.

I still don't really like the approach of registration.

Well, this is basically the trade-off we have to make. Either we pull in implementation details of every implementation to allow for static initialization, or we keep the API completely distinct from those details. Recently, you and @aabadie convinced me that the latter is to be preferred in order to trade initialization overhead for better maintainability and sound architectural design.

Usually in RIOT all hardware components are defined by static initializers.

E.g. with SAUL and network device drivers we have the registration for the same reason.

If _eeprom_devs[] would be declared by the board or the application

This would have its own disadvantage; namely that board configurations get longer and that end users plugging external stuff (not included in their board) would have more effort to configure.

I have however no strong feeling about this decision. Maybe @aabadie and @benpicco could give their opinion on whether they prefer registration or static initializers? I would than just do as the majority says. (Luckily there are three of you, so there should be no draw 😄)

If the registration approach helps keep the board files cleaner I'm all for it.
And if we manage to resist the temptation to compare EEPROM_NUMOF in processor macros we can even populate it automatically:

#define EEPROM_NUMOF (MCU_EEPROM + AT24XXX_NUMOF + AT25XXX_NUMOF …)

@gschorcht What about EEPROMs that are integrated into the MCU? It would be quite bad if every board would have to declare them.

@gschorcht What about EEPROMs that are integrated into the MCU? It would be quite bad if every board would have to declare them.

For boards that have same properties, we use boards/common/*/board_common.h for the definitions.

But that's not my point. All hardware configurations in RIOT are done as static const definitions during the compile time. There is only one exception, the netif. However, we are still looking for an approach on how to get the static definition of GNRC_NETIF_NUMOF and the dynamic netif instantiation consistent.

This will be the same here. We have the static definition of EEPROM_NUMF and the dynamic registration of the devices that are really there. Also your macro hack wouldn't solve this problem since it doesn't cover the case that a board could have multiple eeproms of the same type.

The attempt of a dynamic instantiation without having a real dynamic registry just has its limits 😟

To be clear, I'm not against the approach. It just wanted to tell my concerns. There are a lot of other use case where such a registration would help, e.g., for GPIO extenders.

For boards that have same properties, we use boards/common/*/board_common.h for the definitions.

That doesn't help with your static array example.
Say you have an ATmega board with an additional I2C EEPROM.
You can't have a split array between common code and the board, so either you have to declare both EEPROMs in the board code (meh) or you do something like

#define EXTERNAL_EEPROMS            \
    {                               \
      .driver = i2c_eeprom_driver,  \
      .params = &i2c_eeprom_params, \ 
      .size = I2C_EEPROM_SIZE       \
    },

…

#define INTERNAL_EEPROMS                \
    {                                   \
      .driver = atmega_eeprom_driver,   \
      .params = NULL,                   \
      .size = ATMEGA_EEPROM_SIZE        \
    },

…

static const eeprom_dev_t _eeprom_devs[] = {
    INTERNAL_EEPROMS 
    EXTERNAL_EEPROMS 
};

Or just have a separate struct for external EEPROMs - that should work too.

But on top of that you also need

#define AT24XXXX_PARAMS         {           \
        .i2c = AT24XXXX_PARAM_I2C,          \
        .dev_addr = AT24XXXX_PARAM_ADDR,    \
        .pin_wp = AT24XXXX_PARAM_ADDR,      \
        .eeprom_size = AT24XXXX_PARAM_EEPROM_SIZE \
}

for the driver itself.

Also your macro hack wouldn't solve this problem since it doesn't cover the case that a board could have multiple eeproms of same type.

If you have

#define AT24XXX_NUMOF ARRAY_SIZE(at25xxx_params)

it should work.

I'm wondering: Why not just use the existing MTD API for EEPROMs?

Any idea why MTD exposes the concept of sectors and pages? The API seems not to be using this. IMHO it would be nice to get rid of these details, so that MTD could be applied more broadly. But to me it seems that indeed that API should work just fine.

Some work would be needed to update users like the EEPROM Registry, so that they could be used with external EEPROM. (And any other memory that has an MTD API.)

Btw: Wouldn't storage would be the correct term rather than memory? To me, storage is persistent, while memory is volatile.

Just using the MTD API is clearly the better approach. No need to reinvent the wheel.