Merge #19270

19270: drivers/at24cxxx: implement _mtd_at24cxxx_read_page r=benpicco a=HendrikVE ### Contribution description The function `read_page` was missing which lead to (from a user perspective) undefined behavior on the MTD layer. ### Testing procedure Any application using MTD in conjunction with a board with an at24cxxx. Co-authored-by: Hendrik van Essen <[email protected]>
RIOT-OS · Feb 15, 2023 · 2dd133d · 2dd133d
2 parents 1472a76 + 3837536
commit 2dd133d
Show file tree

Hide file tree

Showing 4 changed files with 43 additions and 20 deletions.
diff --git a/cpu/sam0_common/include/periph_cpu_common.h b/cpu/sam0_common/include/periph_cpu_common.h
@@ -56,6 +56,11 @@ extern "C" {
 #define PERIPH_I2C_NEED_WRITE_REGS
 /** @} */
 
+/**
+ * @brief   Maximum bytes per frame for I2C operations
+ */
+#define PERIPH_I2C_MAX_BYTES_PER_FRAME 256
+
 /**
  * @brief   Override GPIO type
  * @{

diff --git a/drivers/at24cxxx/at24cxxx.c b/drivers/at24cxxx/at24cxxx.c
@@ -97,35 +97,60 @@ int _read(const at24cxxx_t *dev, uint32_t pos, void *data, size_t len)
         }
         xtimer_usleep(AT24CXXX_POLL_DELAY_US);
     }
+
     DEBUG("[at24cxxx] i2c_read_regs(): %d; polls: %d\n", check, polls);
+
     return check;
 }
 
+static int _read_max(const at24cxxx_t *dev, uint32_t pos, void *data, size_t len)
+{
+#ifdef PERIPH_I2C_MAX_BYTES_PER_FRAME
+    uint8_t *data_p = data;
+
+    while (len) {
+        size_t clen = MIN(len, PERIPH_I2C_MAX_BYTES_PER_FRAME);
+
+        if (_read(dev, pos, data_p, clen) == AT24CXXX_OK) {
+            len -= clen;
+            pos += clen;
+            data_p += clen;
+        }
+        else {
+            return -EIO;
+        }
+    }
+
+    return AT24CXXX_OK;
+#else
+    return _read(dev, pos, data, len);
+#endif
+}
+
 static
 int _write_page(const at24cxxx_t *dev, uint32_t pos, const void *data, size_t len)
 {
     int check;
     uint8_t polls = DEV_MAX_POLLS;
     uint8_t dev_addr;
-    uint16_t _pos;
     uint8_t flags = 0;
 
     if (DEV_EEPROM_SIZE > 2048) {
         /* 2 bytes word address length if more than 11 bits are
            used for addressing */
         /* append page address bits to device address (if any) */
         dev_addr  = (DEV_I2C_ADDR | ((pos & 0xFF0000) >> 16));
-        _pos = (pos & 0xFFFF);
+        pos &= 0xFFFF;
         flags = I2C_REG16;
     }
     else {
         /* append page address bits to device address (if any) */
         dev_addr = (DEV_I2C_ADDR | ((pos & 0xFF00) >> 8));
-        _pos = pos & 0xFF;
+        pos &= 0xFF;
     }
 
     while (-ENXIO == (check = i2c_write_regs(DEV_I2C_BUS, dev_addr,
-                                             _pos, data, len, flags))) {
+                                             pos, data, len, flags))) {
         if (--polls == 0) {
             break;
         }
@@ -209,8 +234,7 @@ int at24cxxx_read_byte(const at24cxxx_t *dev, uint32_t pos, void *dest)
     return check;
 }
 
-int at24cxxx_read(const at24cxxx_t *dev, uint32_t pos, void *data,
-                      size_t len)
+int at24cxxx_read(const at24cxxx_t *dev, uint32_t pos, void *data, size_t len)
 {
     if (pos + len > DEV_EEPROM_SIZE) {
         return -ERANGE;
@@ -219,9 +243,10 @@ int at24cxxx_read(const at24cxxx_t *dev, uint32_t pos, void *data,
     int check = AT24CXXX_OK;
     if (len) {
         i2c_acquire(DEV_I2C_BUS);
-        check = _read(dev, pos, data, len);
+        check = _read_max(dev, pos, data, len);
         i2c_release(DEV_I2C_BUS);
     }
+
     return check;
 }
 

diff --git a/drivers/at24cxxx/mtd/mtd.c b/drivers/at24cxxx/mtd/mtd.c
@@ -45,16 +45,11 @@ static int _mtd_at24cxxx_init(mtd_dev_t *mtd)
     return 0;
 }
 
-static int _mtd_at24cxxx_read(mtd_dev_t *mtd, void *dest, uint32_t addr,
-                              uint32_t size)
-{
-    return at24cxxx_read(DEV(mtd), addr, dest, size) == AT24CXXX_OK ? 0 : -EIO;
-}
-
-static int _mtd_at24cxxx_write(mtd_dev_t *mtd, const void *src, uint32_t addr,
-                               uint32_t size)
+static int _mtd_at24cxxx_read_page(mtd_dev_t *mtd, void *dest, uint32_t page,
+                                   uint32_t offset, uint32_t size)
 {
-    return at24cxxx_write(DEV(mtd), addr, src, size) == AT24CXXX_OK ? 0 : -EIO;
+    int rc = at24cxxx_read(DEV(mtd), page * mtd->page_size + offset, dest, size);
+    return rc == AT24CXXX_OK ? (int)size : rc;
 }
 
 static int mtd_at24cxxx_write_page(mtd_dev_t *mtd, const void *src, uint32_t page,
@@ -77,8 +72,7 @@ static int _mtd_at24cxxx_power(mtd_dev_t *mtd, enum mtd_power_state power)
 
 const mtd_desc_t mtd_at24cxxx_driver = {
     .init = _mtd_at24cxxx_init,
-    .read = _mtd_at24cxxx_read,
-    .write = _mtd_at24cxxx_write,
+    .read_page = _mtd_at24cxxx_read_page,
     .write_page = mtd_at24cxxx_write_page,
     .erase = _mtd_at24cxxx_erase,
     .power = _mtd_at24cxxx_power,

diff --git a/drivers/include/at24cxxx.h b/drivers/include/at24cxxx.h
@@ -96,8 +96,7 @@ int at24cxxx_read_byte(const at24cxxx_t *dev, uint32_t pos, void *dest);
  * @return          -ERANGE if @p pos + @p len is out of bounds
  * @return          @see i2c_read_regs
  */
-int at24cxxx_read(const at24cxxx_t *dev, uint32_t pos, void *data,
-                  size_t len);
+int at24cxxx_read(const at24cxxx_t *dev, uint32_t pos, void *data, size_t len);
 
 /**
  * @brief   Write a byte at a given position @p pos