[riot-notifications] [RIOT-OS/RIOT] cpu/qn908x: Implement blocking SPI support (#15689)

iosabi notifications at github.com
Sun Jan 24 19:38:27 CET 2021


@iosabi commented on this pull request.



> +This driver uses the [OSHA SPI Signal Names](
+https://www.oshwa.org/a-resolution-to-redefine-spi-signal-names/) and while it
+only implements the Controller mode, the hardware is capable of operating in
+Peripheral mode as well so we use the COPI/CIPO names.

Let's clarify this a bit more. There are two interfaces at play here:

 1. First is the `periph/spi.h` interface that this driver implements. This interface uses miso/mosi names in just two functions when `MODULE_PERIPH_SPI_RECONFIGURE` is set and it must be consistent across all cpus because they implement those. I would agree here with a RIOT-wide change where you #define the old names to the new names, update all the cpu modules to use the new names and declare the old names deprecated but keep them around for a few releases. Here's there's no discussion about what to do.

 2. The other interface is the board configuration interface between the cpu module and the board module *for that specific cpu* as defined in the `periph_cpu.h` of each CPU. So first I want to note that the board is for a specific cpu, and each CPU will define their `spi_conf_t` a bit differently because each SPI driver will need or offer different configuration due to hardware differences, this is to say that when writing a board/ you *need* to look at the cpu's periph_cpu.h to write you board configuration, so consistency here is not a strong argument. Look for example at the Chip Select line, it is also called Slave Select and indeed is called SSEL in this datasheet, but no cpu in RIOT uses "ss_pin" or "ssel_pin", my cursory search shows some "cs_pin", "pin_cs" and "cs" used. I'm not saying this as "hey look it is already not consistent elsewhere so I have the right to be even more inconsistent" but instead to point out that consistency in this case is not the most important thing to optimize for because it doesn't bring much benefit. Moreover, what if the datasheet for this CPU was written in more modern times and it used COPI/CIPO names, would you still want to use MISO/MOSI here for consistency with other older cpus? What if you buy a board from Sparkfun today which now uses COPI/CIPO on the board documentation and board silkscreen, would you still name those MISO/MOSI in your board config because of consistency with other unrelated boards? Changing the periph_cpu.h definition in the future is not free because users who build their custom boards/ would need to update them... and the definition of "board" in RIOT implies that something as simple as plugging a sensor breakout board to some GPIOs on a dev board is a new "board", if that new board means multiplexing some pins to be used as SPI instead of GPIO, you would be dealing with this spi_conf_t. I think it is more sensible to write new code using the new conventions  eventually having most of your boards with the new conventions than requiring users to update the config on their boards.

Finally, I have a hard time reconciling the idea of sticking to slavery analogies and the guidelines from the code of conduct in this project. I understand that slavery analogies were the norm in SPI signals since forever and until recently, but so was slavery in the old times and nowadays is not ok. The industry is moving out of those signal names because they are not ok and we can do better here. For this specific pull request I don't see a downside on using copi and cipo in the periph_cpu.h of this cpu.

All that said, you are maintainers here, so if this is the direction you want for your project I would have to accept it.

-- 
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/15689#discussion_r563331865
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210124/5adc9431/attachment-0001.htm>


More information about the notifications mailing list