[riot-notifications] [RIOT-OS/RIOT] nrf52: Implement EasyDMA-based SPI peripheral implemenation (#14057)

benpicco notifications at github.com
Mon May 18 18:05:02 CEST 2020


@benpicco commented on this pull request.

Looks good to me - just some minor nits.
You can squash directly.

> @@ -54,6 +62,12 @@ extern "C" {
  */
 #define ADC_NUMOF           (9U)
 
+/**
+ * @brief   SPI temporary buffer size for storing const data in RAM before
+ *          initiating DMA transfer
+ */
+#define SPI_MBUF_SIZE       64

```suggestion
#ifndef SPI_MBUF_SIZE
#define SPI_MBUF_SIZE       64
#endif
```

Always useful being able to overwrite these. 

> +    dev(bus)->FREQUENCY = clk;
+    /* enable the bus */
+    dev(bus)->ENABLE = SPIM_ENABLE_ENABLE_Enabled;
+
+    return SPI_OK;
+}
+
+void spi_release(spi_t bus)
+{
+    /* power off everything */
+    dev(bus)->ENABLE = 0;
+    mutex_unlock(&locks[bus]);
+}
+
+static size_t _transfer(spi_t bus, const uint8_t *out_buf, uint8_t *in_buf,
+                      size_t remaining_len)

```suggestion
                        size_t remaining_len)
```

minor nit

> +    dev(bus)->ENABLE = SPIM_ENABLE_ENABLE_Enabled;
+
+    return SPI_OK;
+}
+
+void spi_release(spi_t bus)
+{
+    /* power off everything */
+    dev(bus)->ENABLE = 0;
+    mutex_unlock(&locks[bus]);
+}
+
+static size_t _transfer(spi_t bus, const uint8_t *out_buf, uint8_t *in_buf,
+                      size_t remaining_len)
+{
+    uint8_t transfer_len = remaining_len > UINT8_MAX ? UINT8_MAX : remaining_len;

```suggestion
    uint8_t transfer_len = remaining_len & 0xFF;
```

> +    dev(bus)->ENABLE = SPIM_ENABLE_ENABLE_Enabled;
+
+    return SPI_OK;
+}
+
+void spi_release(spi_t bus)
+{
+    /* power off everything */
+    dev(bus)->ENABLE = 0;
+    mutex_unlock(&locks[bus]);
+}
+
+static size_t _transfer(spi_t bus, const uint8_t *out_buf, uint8_t *in_buf,
+                      size_t remaining_len)
+{
+    uint8_t transfer_len = remaining_len > UINT8_MAX ? UINT8_MAX : remaining_len;

I thought the compiler would optimize it to the same, but turns out this saves an instruction. 

> +}
+
+static size_t _transfer(spi_t bus, const uint8_t *out_buf, uint8_t *in_buf,
+                      size_t remaining_len)
+{
+    uint8_t transfer_len = remaining_len > UINT8_MAX ? UINT8_MAX : remaining_len;
+    const uint8_t *out_mbuf = out_buf;
+
+    /**
+     * Copy the out buffer in case it resides in flash, EasyDMA only works from
+     * RAM
+     */
+    if (out_buf && !_in_ram(out_buf)) {
+        /* The SPI MBUF can be smaller than UINT8_MAX */
+        transfer_len = transfer_len > SPI_MBUF_SIZE ?
+            SPI_MBUF_SIZE : transfer_len;

```suggestion
                       SPI_MBUF_SIZE : transfer_len;
```

minor nit.

```suggestion
        transfer_len = transfer_len > SPI_MBUF_SIZE
                     ? SPI_MBUF_SIZE : transfer_len;
```
Is visually even more pleasing, but that's personal taste :wink: 

-- 
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/14057#pullrequestreview-413726086
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20200518/72a4d735/attachment-0001.htm>


More information about the notifications mailing list