Post DT API migration review #25251
Description
Describe the bug
Following move to new DT API (#25009 and cie), a systematic code review is needed around impacted code.
First reason is that a lot of code has been changed using scripts which is a usual source of bugs. Most of the time, lot of files where impacted, which is not easy to review, and hence some bugs have been merged (https://github.com/zephyrproject-rtos/zephyr/blame/master/boards/arm/atsamd21_xpro/pinmux.c#L23 -- fixed by #25252)
Second reason is that peripheral instances Kconfig symbols replacement by DT macros is not transparent in all areas. In a driver compiled under CONFIG_SPI control, CONFIG_SPI_1 is equivalent to DT_HAS_NODE_STATUS_OKAY(DT_NODELABEL(spi1)), but this is not valid in other areas.
Under boards/ or soc/, every piece of code under DT_HAS_NODE_STATUS_OKAY(spi1) is compiled in, whatever the status of CONFIG_SPI. So SPI related pins or clocks are activated unconditionally. Since this is the case for all peripherals (SPI, CAN, I2C, ...) this could potentially lead to, among others:
- pin conflicts between peripherals that weren't enabled together until now
- clocks being enabled for disabled peripherals
instances that used to be disabled in Kconfig but DT enables them by default, causing conflicts (eg. with shields).Covered separately by Mitigate changes in peripheral enable state after Kconfig replaced by DT status #24745.
These two cases are typically tricky to detect, and since current code coverage is not sufficient to guarantee all potential bugs introduced will be detected.
So it is safer to systematically review each change and in case of doubt re-introduce a peripheral Kconfig control on newly introduced instance dt macros:
#if DT_HAS_NODE_STATUS_OKAY(DT_NODELABEL(spi1)) && CONFIG_SPI
Expected behavior
No bugs where introduced following move to new DTS API
Impact
Wide spectrum of bugs in v2.3.0
Actions
I propose that each codeowner reviews, in his own area (boards/, soc/, ..) code impacted by new introduced DT macros (DT_HAS_NODE_STATUS_OKAY, DT_NODE_HAS_COMPAT_STATUS, ATMEL_SAM0_DT_SERCOM_CHECK, ..) and take appropriate actions.
In order to make this a task that can be completed in a finite amount of time, I've giving putting here
the various soc maintainers based on CODEOWNERS. I let people tick their area when they consider things are ok (don't hesitate to rework this list if required).
Check boards/ and soc/ for each of the hardware variants below:
- arc/snps_*/ @vonhust @ruuddw
- nios2/ @nashif @wentongwu
- arm/arm/mps2/ ? (@galak)
- arm/atmel_sam/sam*/ @ioannisg, @nandojve, @fallrisk (boards/)
- arm/bcm*/ @sbranden
- arm/infineon_xmc/ @parthitce
- arm/nxp*/ @MaureenHelm (boards: arm: Conditionalize pinmuxes on nxp boards #25525)
- arm/nordic_nrf/ @ioannisg @anangl (boards: arm: nrf5340pdk: fix pin assignments #25533)
- arm/qemu_cortex_a53/ @carlocaione
- arm/st_stm32/ @erwango (boards: stm32: Restore Kconfig control on pinmux files #25288)
- arm/ti_simplelink/ @bwitherspoon, @vanti, @Mani-Sadhasivam
- arm/xilinx_zynqmp/ @stephanosio
- xtensa/intel_s1000/ @sathishkuttan @dcpleung
- x86/appolo_lake @jhedberg
- riscv/rv32m1_vega @MaureenHelm (boards: riscv: Conditionalize pinmuxes on rv32m1_vega board #25540)
Add a dependency to the subsystem Kconfig variable if it was present previously:
#if DT_NODE_UART... && CONFIG_UART
EDIT: Following #25214, DT_HAS_NODE_STATUS_OKAY(spi1) is being replaced by DT_NODE_HAS_STATUS(DT_NODELABEL(spi1), okay)
EDIT: Adding x86 (@andrewboie )