[riot-notifications] [RIOT-OS/RIOT] periph/spi: add support for printing and testing SPI clock rate (#16727)

Marian Buschsieweke notifications at github.com
Tue Sep 21 15:48:42 CEST 2021


@maribu commented on this pull request.

The AT(X)mega implementations look good to me. I hope I manage to review the remaining soonish.

>      /* internal: band gap */
-    /* Note: the band gap buffer uses a bit of current and is turned off by default,
-     * Set PMC->REGSC |= PMC_REGSC_BGBE_MASK before reading or the input will be floating */
-    [ 9] = { .dev = ADC0, .pin = GPIO_UNDEF, .chan = 27, .avg = ADC_AVG_MAX },
+    /* Note: the band gap buffer uses a bit of current and is turned off
+     * by default, set PMC->REGSC |= PMC_REGSC_BGBE_MASK before reading
+     * or the input will be floating */
+    [9] = {
+        .dev = ADC0,    .pin = GPIO_UNDEF,
+        .chan = 27,     .avg = ADC_AVG_MAX
+    },

I agree that this increases readability. Could you split this change out into a separate PR?

> +    uint8_t shift = 0;
+    while (clk << shift < CLOCK_CORECLOCK / 2) {
+        shift++;
+    }

```suggestion
    uint8_t shift = 0;
    uint32_t spi_clk = CLOCK_CORECLOCK / 2;
    while (spi_clk > clk) {
        shift++;
        spi_clk >>= 1;
    }
```

IMO this is easier to read, as the MCU will divide down the core clock to generate the SPI clock. Also, this will no longer overflow for high values of `clk`.

> +    /* bound divider from 2 to 128 */
+    if (freq > CLOCK_CORECLOCK / 2) {
+        freq = CLOCK_CORECLOCK / 2;
+    }

```suggestion
```

No longer needed if you take the suggestions above.

> +uint32_t spi_clk_info(spi_t bus, spi_clk_t clk)
+{
+    (void)bus;
+    (void)clk;
+    /* in spi_acquire :
+     * CLK2X    PRESCALER
+     * 1        01          ->  ClkPER / 8
+     * All clock prescalers was set to 1 so ClkPER = CLOCK_CORECLOCK */
+    return CLOCK_CORECLOCK / 8;
+}
+

This looks like a leftover to me.

> +     * CLK2X    PRESCALER
+     * 1        01          ->  ClkPER / 8
+     * All clock prescalers was set to 1 so ClkPER = CLOCK_CORECLOCK */
+    return CLOCK_CORECLOCK / 8;
+}
+
+spi_clk_t spi_get_clk(spi_t bus, uint32_t freq)
+{
+    (void)bus;
+    /* bound divider from 2 to 128 */
+    if (freq > CLOCK_CORECLOCK / 2) {
+        freq = CLOCK_CORECLOCK / 2;
+    }
+    assert(freq >= CLOCK_CORECLOCK / 128);
+
+    return _clk_shift(freq);

```suggestion
    uint8_t shift = _clk_shift(freq);
    return ((~shift & 1) << 7) | (shift >> 1)
```

Maybe prepare the value directly in the format that it can be or'ed into the CTRL register?

-- 
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/16727#pullrequestreview-759728658
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210921/b74eb701/attachment.htm>


More information about the notifications mailing list