Driver for AT24CXXX EEPROM

[fabian18]

Contribution description

This contribution adds a driver to support the external EEPROM modules
AT24C128 and ATC256.
It allows integration with the existing EEPROM registry, while it includes
a pseudomodule that provides wrappers for the expected EEPROM API.

Testing procedure

Development and testing has been done with the bluepill board.
There is a test application in tests/driver_at24cxxx.

You can also run the existing test for the EEPROM registry.
USEMODULE=at24cxxx_eepreg_wrapper make BOARD=bluepill flash
make BOARD=bluepill term

Issues/PRs references

None

[benpicco]

I think you can make the driver much simpler by using some existing i2c helper functions.

[maribu]

Btw: If you want to fix a bug in commit with ID , instead of normal commit you can use git commit --fixup=<FOO>. When later the reviewer asks to squash, you can then use git commit --autosquash -i <COMMIT_ID> and git will sort the fixup-commit to be integrated into the commits they fix. This can safe you some work, but has the disadvantage that the commit messages of the fixes no longer make sense. (But eventually those commits will be integrated into the one they fix anyway.)

[maribu]

Two remarks from me, see inline

[benpicco]

I'd suggest you drop the 'C' from the name - this family of EEPROMs is commonly known as AT24xxx.
The SPI version will then be AT25xxx.

Linux just calls them at24 & at25.

[maribu]

I pointed out some minor issues inline.

This is by the way a very nice PR, especially since it is your first :-)

[fabian18]

I have just seen that there are many different at24xxx. They also vary in page size, so I am going to add the page size to the configuration parameters.
The AT24C1024 has more then 64kB of memory but also uses 2 address bytes, where the device I2C address is different for the lower 64kB and the higher 64kB. So I think the AT24C1024 will not be fully functioning with this driver.

[maribu]

To me it would also be fine to limit this PR to a subset of devices with the same page size. The AT24C256 and the AT24C128 are the most popular ones anyway, so maybe no one will ever need support for the other chips. In any case, I'm personally a fan of limiting the problem space a PR wants to solve, so that reviewing is easier: Support for more device could be added in a follow-up PR.

[maribu]

Oh, almost forgot: Could you add at24c256 and at24c128 as (empty) pseudo modules that depend on the driver? That would ease the use as one could add the name of the actually used device.

(Or maybe even at24c256% to match both at24c256 and at24c256c?)

Edit: Here is a backup of the old AT24C256 data sheet, here is the new AT24C256C version on the official Microchip Homepage. The wildcard in at24c256% would likely match also future versions, which likely will remain backward compatible (and therefore compatible with the driver).

[maribu]

I'd suggest you drop the 'C' from the name - this family of EEPROMs is commonly known as AT24xxx.
The SPI version will then be AT25xxx.

Linux just calls them at24 & at25.

@benpicco: I only just read your comment. Do you still prefer at24xxx? The devices supported by the driver have AT24C as common prefix with three digits following, so the common RIOT naming scheme would be at24cxxx for the driver. Are you fine with at24cxxx?

[benpicco]

@maribu I'm fine with either. All devices in the kernel follow the AT24Cxxx naming scheme too.
The naming is indicative of capacity/page size and other vendors just produce compatible modules.

Linux also has handling for devices with multiple addresses but I agree that this is out of the scope of this PR since I assume neither of us has the hardware to test this.

[maribu]

OK. One last thing came to my mind: The indentation is different from RIOT's standard. You can use the command line tool uncrustify with the config in the root of the repo to get code formatting consistent. After that, I'd say squash to three commits: One for the base driver, one for the test and one for the wrapper.

[maribu]

@fabian18: When I test with no device connected to the I2C bus, it seems the test/driver hangs instead of returning with an error code. Can you reproduce that?

[maribu]

See inline comments. I'm not fully sure how to do proper error handling. Maybe it is the best to give up on having the API compatible with the EEPROM API for now, as the API prevents any reasonable error handling :-/

[fabian18]

@maribu
Ain´t your requests for changes fulfilled?

[benpicco]

Thank you for the update! Good to have extended device support.
I just ran this on the AT24MAC402 (I know I know, it's not supported by the driver, but that's what I had at hand), but I got

2020-02-17 16:39:44,116 #  main(): This is RIOT! (Version: 2020.04-devel-522-gc58f8-driver-at24cxxx-eeprom)
2020-02-17 16:39:44,122 # Starting tests for module at24cxxx
2020-02-17 16:39:44,132 # [SUCCESS] at24cxxx_init
2020-02-17 16:39:44,134 # [SUCCESS] at24cxxx_write_byte
2020-02-17 16:39:44,135 # [SUCCESS] at24cxxx_read_byte
2020-02-17 16:39:44,136 # [SUCCESS] write_byte/read_byte
2020-02-17 16:39:44,137 # [SUCCESS] at24cxxx_write
2020-02-17 16:39:44,140 # [SUCCESS] at24cxxx_read
2020-02-17 16:39:44,144 # [FAILURE] write/read: (4FREE != BEER4FREE)

at24mac.patch

diff --git a/drivers/Makefile.dep b/drivers/Makefile.dep
index 423319b245..0b60b0e52c 100644
--- a/drivers/Makefile.dep
+++ b/drivers/Makefile.dep
@@ -36,16 +36,12 @@ ifneq (,$(filter at,$(USEMODULE)))
   USEMODULE += isrpipe_read_timeout
 endif
 
-ifneq (,$(filter at24c%,$(USEMODULE)))
+ifneq (,$(filter at24mac at24c%,$(USEMODULE)))
   FEATURES_REQUIRED += periph_i2c
   USEMODULE += xtimer
   USEMODULE += at24cxxx
 endif
 
-ifneq (,$(filter at24mac,$(USEMODULE)))
-  FEATURES_REQUIRED += periph_i2c
-endif
-
 ifneq (,$(filter at30tse75x,$(USEMODULE)))
   USEMODULE += xtimer
   FEATURES_REQUIRED += periph_i2c
diff --git a/drivers/at24cxxx/include/at24cxxx_defines.h b/drivers/at24cxxx/include/at24cxxx_defines.h
index 312ba9d50d..4d2c0e715a 100644
--- a/drivers/at24cxxx/include/at24cxxx_defines.h
+++ b/drivers/at24cxxx/include/at24cxxx_defines.h
@@ -299,6 +299,29 @@ extern "C" {
                                          / AT24CXXX_POLL_DELAY_US))
 /** @} */
 
+/**
+ * @name AT24MAC402/602 constants
+ * @{
+ */
+/**
+ * @brief   2048 byte memory
+ */
+#define AT24MAC_EEPROM_SIZE             (2048U)
+/**
+ * @brief   256 pages of 8 bytes
+ */
+#define AT24MAC_PAGE_SIZE               (8U)
+/**
+ * @brief   Delay to complete write operation
+ */
+#define AT24MAC_PAGE_WRITE_DELAY_US     (5000U)
+/**
+ * @brief   Number of poll attempts
+ */
+#define AT24MAC_MAX_POLLS               (1 + (AT24MAC_PAGE_WRITE_DELAY_US \
+                                         / AT24CXXX_POLL_DELAY_US))
+/** @} */
+
 /**
  * @name Set constants depending on module
  * @{
@@ -347,6 +370,10 @@ extern "C" {
 #define AT24CXXX_EEPROM_SIZE            (AT24C01A_EEPROM_SIZE)
 #define AT24CXXX_PAGE_SIZE              (AT24C01A_PAGE_SIZE)
 #define AT24CXXX_MAX_POLLS              (AT24C01A_MAX_POLLS)
+#elif IS_USED(MODULE_AT24MAC)
+#define AT24CXXX_EEPROM_SIZE            (AT24MAC_EEPROM_SIZE)
+#define AT24CXXX_PAGE_SIZE              (AT24MAC_PAGE_SIZE)
+#define AT24CXXX_MAX_POLLS              (AT24MAC_MAX_POLLS)
 #else /* minimal */
 #define AT24CXXX_EEPROM_SIZE            (128U)  /**< EEPROM size */
 #define AT24CXXX_PAGE_SIZE              (4U)    /**< page size */

same54-xpro.patch

diff --git a/boards/same54-xpro/include/board.h b/boards/same54-xpro/include/board.h
index cde35f1a69..a80f6ebde3 100644
--- a/boards/same54-xpro/include/board.h
+++ b/boards/same54-xpro/include/board.h
@@ -33,6 +33,7 @@ extern "C" {
 #define AT24MAC_PARAM_I2C_DEV   I2C_DEV(0)
 #define AT24MAC_PARAM_I2C_ADDR  (0x5E)
 #define AT24MAC_PARAM_TYPE      AT24MAC4XX
+#define AT24CXXX_PARAM_ADDR     (0x56)
 /** @} */
 
 /**

[fabian18]

The AT24MAC402 has 2048 Kbit (not Kbyte).
And if I understand correctly, according to the datasheet (p. 8) the memory layout is 16 pages of 16 bytes each. Only the positions ([0] - [127], [144] - [151], [A0] - [FF]) are R/W.
The test writes at position 444, which is out of bounds but it does not complain because you specified the EEPROM size with 2048 bytes.

So maybe try:

/**
 * @name AT24MAC402/602 constants
 * @{
 */
/**
 * @brief   256 byte memory
 */
#define AT24MAC_EEPROM_SIZE             (256U)
/**
 * @brief   16 pages of 16 bytes
 */
#define AT24MAC_PAGE_SIZE               (16U)
/**
 * @brief   Delay to complete write operation
 */
#define AT24MAC_PAGE_WRITE_DELAY_US     (5000U)
/**
 * @brief   Number of poll attempts
 */
#define AT24MAC_MAX_POLLS               (1 + (AT24MAC_PAGE_WRITE_DELAY_US \
                                         / AT24CXXX_POLL_DELAY_US))
/** @} */

And change the positions in the test so that you stay in the memory range.

[fabian18]

Should I make the positions in the tests configurable too?
Like...

#ifndef WRITE_POSITION
#define WRITE_POSITION (444U)
#endif

[benpicco]

The AT24MAC402 has 2048 Kbit (not Kbyte).
And if I understand correctly, according to the datasheet (p. 8) the memory layout is 16 pages of 16

You are of course right! Now with that and AT24CDEV also changed to at24mac the test is passing.

Should I make the positions in the tests configurable too?

No - we know the parameters of the EEPROM at compile time, so select some good values for testing, e.g.

#define WRITE_POSITION          (AT24CXXX_EEPROM_SIZE - 3 * AT24CXXX_PAGE_SIZE - 4)

will make sure that we always test writing over the page boundary.

[fabian18]

@benpicco Could you please test again? Then I can squash, make Murdock green and maybe we can finally finish this?

[benpicco]

Tried it again, works as expected - please squash.

same54-xpro with at24mac

2020-02-24 11:12:19,109 # Starting tests for module at24cxxx
2020-02-24 11:12:19,110 # EEPROM size: 256 byte
2020-02-24 11:12:19,112 # Page size  : 16 byte
2020-02-24 11:12:19,114 # [SUCCESS] at24cxxx_init
2020-02-24 11:12:19,121 # [SUCCESS] at24cxxx_write_byte
2020-02-24 11:12:19,122 # [SUCCESS] at24cxxx_read_byte
2020-02-24 11:12:19,123 # [SUCCESS] write_byte/read_byte
2020-02-24 11:12:19,128 # [SUCCESS] at24cxxx_write
2020-02-24 11:12:19,136 # [SUCCESS] at24cxxx_read
2020-02-24 11:12:19,137 # [SUCCESS] write/read
2020-02-24 11:12:19,142 # [SUCCESS] at24cxxx_set
2020-02-24 11:12:19,146 # [SUCCESS] set/read
2020-02-24 11:12:19,149 # Finished tests for module at24cxxx

[benpicco]

Code looks good and works.
We can adapt this to the common EEPROM API later - and finalizing the API might be easier with some existing code to make use of it.

[benpicco]

Mind giving #11945 a look?

[fabian18]

I will look at #11945 but I cannot test because I don´t have an SPI EEPROM.

[fabian18]

What´s wrong with Murdock?:
--- FAILED build outputs: --- build output of app tests/thread_flags for board native:gnu (raw, runtime=15.6s):
I don´t think I touched this.

[kaspar030]

Pausing for #13456

Comments have been addressed.

[fabian18]

Thank you @benpicco ☺️

[maribu]

Nice :-) @fabian18: Thanks for your contribution and @benpicco thanks for reviewing!