[riot-notifications] [RIOT-OS/RIOT] cpu/nrf52: initial kconfig modeling (no netif) (#16837)

Leandro Lanzieri notifications at github.com
Tue Sep 28 10:17:46 CEST 2021


@leandrolanzieri commented on this pull request.



> @@ -1,5 +1,5 @@
 USEMODULE += boards_common_e104_bt50xxa_tb
 
 # use nrfmin for GNRC as nimble is too large for the board
-include $(RIOTBOARD)/common/nrf52/Makefile.nrfmin.dep
+include $(RIOTCPU)/nrf52/Makefile.nrfmin.dep

The static tests don't like this.

> +if TEST_KCONFIG
+
+rsource "radio/Kconfig"
+rsource "periph/Kconfig"
+rsource "vectors/Kconfig"
+
+endif # TEST_KCONFIG

I know it has the same effect, but could we move the `TEST_KCONFIG` dependency inside each of the files? I think it's easy to forget about it when just looking at the single files, it would help to have the complete picture.

> @@ -0,0 +1,19 @@
+# Copyright (c) 2021 HAW Hamburg
+#
+# This file is subject to the terms and conditions of the GNU Lesser
+# General Public License v2.1. See the file LICENSE in the top level
+# directory for more details.
+#
+
+config MODULE_PERIPH_UART_NONBLOCKING
+    depends on HAS_PERIPH_UART_NONBLOCKING
+    depends on MODULE_PERIPH_UART
+    select MODULE_TSRB
+
+config MODULE_PERIPH_SPI
+    depends on HAS_PERIPH_SPI
+    select MODULE_PERIPH_GPIO_IRQ if CPU_MODEL_NRF52832XXAA

```suggestion
    select MODULE_PERIPH_GPIO_IRQ if CPU_MODEL_NRF52832XXAA && HAS_PERIPH_GPIO_IRQ
```

> +config MODULE_SAUL_NRF_VDDH
+    bool
+    depends on HAS_PERIPH_ADC

Could we add some documentation on this one?

> +config NRF802154
+    bool "nrf802154"
+    imply MODULE_NRF802154
+
+config NRFBLE
+    bool "nrfble"
+    imply MODULE_NRFBLE
+
+config NRFMIN
+    bool "nrfmin"
+    imply MODULE_NRFMIN

Perhaps we can prefix them with `NRF52_RADIO_`?

> @@ -13,4 +13,15 @@ config BOARD_COMMON_NRF52XXXDK
     select HAS_PERIPH_UART
     select HAS_VDD_LC_FILTER_REG1
 
+if TEST_KCONFIG
+
+config MODULE_BOARDS_COMMON_NRF52XXDK
+    bool
+    default y
+    select MODULE_SAUL_GPIO if MODULE_SAUL_DEFAULT && HAS_PERIPH_GPIO

I still think this is not needed.

> +config NRF802154
+    bool "nrf802154"
+    imply MODULE_NRF802154
+
+config NRFBLE
+    bool "nrfble"
+    imply MODULE_NRFBLE
+
+config NRFMIN
+    bool "nrfmin"
+    imply MODULE_NRFMIN

Why do we need the indirection of having the choices `imply`ing modules? Can't we have the modules directly here? That way when dependencies are not met only available options show up.

-- 
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/16837#pullrequestreview-765072027
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210928/d159ef67/attachment-0001.htm>


More information about the notifications mailing list