[riot-notifications] [RIOT-OS/RIOT] cpu/sam0_common: SPI: add support for QSPI in SPI mode (#15361)

[Marian Buschsieweke]

@maribu commented on this pull request.

The implementation looks quite good to me. However, I dislike having a `SercomSpi` pointer pointing to an incompatible `Qspi` structure. IMO, it is better to just use `void *`, if the type of the pointer needs to be determined at run time.

> @@ -251,7 +251,25 @@ static const spi_conf_t spi_config[] = {
         .tx_trigger = SERCOM6_DMAC_ID_TX,
         .rx_trigger = SERCOM6_DMAC_ID_RX,
 #endif
-    }
+    },
+#ifdef MODULE_PERIPH_SPI_ON_QSPI
+    {    /* QSPI */
+        .dev      = (SercomSpi*)QSPI,

```suggestion
        .dev      = (SercomSpi *)QSPI,
```

This cast is not safe. E.g. the `struct` member `BAUD` in `Qspi` has an offset of `0x08`, the same member in `SercomSpi` has an offset of `0x0C`. (Only checked for same54.) The members `INTENCLR`, `INTENSET`, and `INTFLAG` have a different size, etc.

It seems like most functions accessing members of `Qspi` or `SercomSpi` are already implemented in two flavors. Why not just use `void *` here and cast it back to `SercomSpi *` or `Qspi *` based on `is_qspi()`? IMO a `void *` comes with an inherit "pay special attention to the actual type" warning. And a `SercomSpi` pointer pointing to an incompatible `Qspi` look a lot like a foot gun to me.

-- 
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/15361#pullrequestreview-532231705
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20201117/67cf8dc7/attachment.htm>