[riot-notifications] [RIOT-OS/RIOT] 16-bits register addressing support (#16711)

benpicco notifications at github.com
Thu Aug 5 12:30:21 CEST 2021


@benpicco commented on this pull request.

Thank you for the patch!
Looks good. The use of VLAs is unfortunate but it's already present in the existing driver.

Maybe we can get rid of that. 

>      /* the nrf52's TWI device does not support to do two consecutive transfers
      * without a repeated start condition in between. So we have to put all data
      * to be transferred into a temporary buffer
      *
      * CAUTION: this might become critical when transferring large blocks of
      *          data as the temporary buffer is allocated on the stack... */
-    uint8_t buf_tmp[len + 1];

urgh so this was incidentally putting up to 256 bytes of memory on the stack and using VLA on top?
I know this was not introduced by you, but to avoid nasty surprises I would make this a 

```C
    static uint8_t tx_buf[256];
```

> +        buf_tmp[0] = reg>>8; /* AddrH in the first byte  */
+        buf_tmp[1] = reg&0xFF; /* AddrL in the second byte  */

```suggestion
        buf_tmp[0] = reg >> 8; /* AddrH in the first byte  */
        buf_tmp[1] = reg & 0xFF; /* AddrL in the second byte  */
```

> +        memcpy(&buf_tmp[reg_addr_len], data, len);
+        return i2c_write_bytes(dev, addr, buf_tmp, (reg_addr_len + len), flags);

looks like you can pull this out of the if block 

> +        buf_regs[0] = reg>>8; /* AddrH in the first byte  */
+        buf_regs[1] = reg&0xFF; /* AddrL in the second byte  */
+        bus(dev)->TXD.PTR = (uint32_t)buf_regs;
+        bus(dev)->TXD.MAXCNT = 2;
+    }
+    else {
+        bus(dev)->TXD.PTR = (uint32_t)®
+        bus(dev)->TXD.MAXCNT = 1;
+    }

Won't this suffice?

```suggestion
        reg = htons(reg);
        bus(dev)->TXD.MAXCNT = 2;
    }
    else {
        bus(dev)->TXD.MAXCNT = 1;
    }
    bus(dev)->TXD.PTR = (uint32_t)®
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/16711#pullrequestreview-723189687
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210805/8b28b471/attachment.htm>


More information about the notifications mailing list