[riot-notifications] [RIOT-OS/RIOT] cpu/fe310, board/hifive1: SPI support (#10833)

Marian Buschsieweke notifications at github.com
Wed Jan 23 13:06:57 CET 2019


maribu commented on this pull request.

This looks pretty good to me. I have found something to nitpick about, though ;-)

Sadly I'm unable to test this. Maybe I should consider buying a board...

>  USEMODULE += periph_pm
 
+# include common periph drivers
+USEMODULE += periph_common

`periph_common` is added twice

> + * @}
+ */
+
+#include "cpu.h"
+#include "mutex.h"
+#include "assert.h"
+#include "periph/spi.h"
+#include "vendor/platform.h"
+
+#define ENABLE_DEBUG        (0)
+#include "debug.h"
+
+/**
+ * @brief   Allocation device locks
+ */
+static mutex_t lock;

Maybe it would make sharing the code with future versions of the CPU that could potentially have multiple SPI interface easier when you use something like that:

``` C
#define SPI_INTERFACE_NUM         1
static mutex_t lock[SPI_INTERFACE_NUM];
```

(I'm only suggesting this, not insisting on this.)

> +    assert(bus == 0);
+
+    /* initialize the buses lock */
+    mutex_init(&lock);
+
+    /* trigger pin initialization */
+    spi_init_pins(bus);
+
+    /* disable hardware chip select
+       (hardware chip select only supports one-byte transfers...) */
+    SPI1_REG(SPI_REG_CSMODE) = SPI_CSMODE_OFF;
+}
+
+void spi_init_pins(spi_t bus)
+{
+    (void) bus;

Should not needed because of the `assert(bus == 0);` below. (I believe both GCC and clang should still be aware that `bus` was used even when `NDEBUG` is defined, but I'm not 100% sure about that. So please correct me if I'm wrong.)

> +    }
+
+    /* setting the CS high before configuring it as an
+       output should be fine on FE310. */
+    gpio_set(cs);
+
+    if (gpio_init(cs, GPIO_OUT)) {
+        return SPI_NOCS;
+    }
+
+    return SPI_OK;
+}
+
+int spi_acquire(spi_t bus, spi_cs_t cs, spi_mode_t mode, spi_clk_t clk)
+{
+    (void) bus;

I think it would make life easier to also add `assert()`s for `bus` and `cs` here, so that developers are supported in finding their bugs in using the SPI interface.

> +int spi_acquire(spi_t bus, spi_cs_t cs, spi_mode_t mode, spi_clk_t clk)
+{
+    (void) bus;
+    (void) cs;
+
+    mutex_lock(&lock);
+
+    SPI1_REG(SPI_REG_SCKDIV)  = spi_clk_config[clk];
+    SPI1_REG(SPI_REG_SCKMODE) = mode;
+
+    return SPI_OK;
+}
+
+void spi_release(spi_t bus)
+{
+    (void) bus;

Again I would suggest to add an `assert()`

> +    assert(!(SPI1_REG(SPI_REG_TXFIFO) & SPI_TXFIFO_FULL));
+
+    const uint8_t *out = out_;
+    uint8_t *in = in_;
+
+    if (cs != SPI_CS_UNDEF) {
+        gpio_clear(cs);
+    }
+
+    for (size_t i = 0; i < len; i++) {
+        SPI1_REG(SPI_REG_TXFIFO) = out ? out[i] : 0;
+
+        uint32_t rxdata = SPI_RXFIFO_EMPTY;
+        while (rxdata & SPI_RXFIFO_EMPTY) {
+            rxdata = SPI1_REG(SPI_REG_RXFIFO);
+        }

How about

``` C
uint32_t rxdata;
do {
    rxdata = SPI1_REG(SPI_REG_RXFIFO);
} while (rxdata & SPI_RXFIFO_EMPTY);
```

-- 
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/10833#pullrequestreview-195479571
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190123/a01876dd/attachment.html>


More information about the notifications mailing list