Re: [PATCH v2 07/11] ARM: dts: imx6ul: Reuse Variscite DTSI files for Concerto

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] mmc: missing non-removable property breaks SDIO Wi-Fi
- [High] clk: invalid audio clock rate for SAI2
- [Medium] dt: removal of hardware description for userspace policy
- [Medium] dt: invalid interrupt trigger type for touchscreen
--

commit 4652c48dc5d4a5447b97f46a89cbede055fb6211
Author: Stefano Radaelli <stefano.r@variscite.com>

ARM: dts: imx6ul: Reuse Variscite DTSI files for Concerto

This commit converts the existing i.MX6UL and i.MX6ULL Concerto boards
to use shared module, option, and carrier DTSI files. It also removes
the old kernel-managed Wi-Fi power sequence and Bluetooth nodes in
favor of user-space initialization.

> The old Wi-Fi power sequence and Bluetooth nodes modeled enable and
> reset control in the kernel. On these SoMs those controls are handled
> in userspace, so use the shared Broadcom Wi-Fi description without the
> old kernel-managed nodes.

[Severity: Medium]
Does this removal of hardware description to defer to userspace initialization
violate Devicetree principles?

The kernel will not automatically initialize the Bluetooth and Wi-Fi modules
natively. Devicetree principles require describing the physical hardware
regardless of the OS policy. 

In arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-som-wifi-brcm.dtsi:
        brcmf: wifi@1 {
                compatible = "brcm,bcm4329-fmac";
                reg = <1>;
        };

Without the mmc-pwrseq, how is the reset line physically released for the
Wi-Fi card when it was removed from the DT?

[ ... ]

> diff --git a/arch/arm/boot/dts/nxp/imx/imx6ul-var-som-concerto-full.dts b/arch/arm/boot/dts/nxp/imx/imx6ul-var-som-concerto-full.dts
> index 725f34d6b7ee9..5a3e3fb4c5a5b 100644
> --- a/arch/arm/boot/dts/nxp/imx/imx6ul-var-som-concerto-full.dts
> +++ b/arch/arm/boot/dts/nxp/imx/imx6ul-var-som-concerto-full.dts
> @@ -8,15 +8,16 @@
>  
>  /dts-v1/;
>  
> +#include "imx6ul.dtsi"
>  #include "imx6ul-var-som.dtsi"
> -#include "imx6ul-var-som-concerto-common.dtsi"
> -#include "imx6ul-var-som-wifi.dtsi"
> -#include "imx6ul-var-som-enet2.dtsi"
> -#include "imx6ul-var-som-enet1.dtsi"
> -#include "imx6ul-var-som-audio.dtsi"
> -#include "imx6ul-var-som-lvds-panel.dtsi"
> +#include "imx6ul-imx6ull-var-som-emmc.dtsi"
> +#include "imx6ul-imx6ull-var-som-wifi-brcm.dtsi"

[Severity: High]
Does this shared Wi-Fi configuration omit the non-removable property for a
hardwired SDIO Wi-Fi module?

The MMC subsystem will treat it as a removable slot and, lacking a card-detect
signal, will fail to detect the module. The omission of no-1-8-v may also
cause erroneous UHS voltage switch attempts on a fixed 3.3V supply.

In arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-som-wifi-brcm.dtsi:
&usdhc1 {
        #address-cells = <1>;
        #size-cells = <0>;
        pinctrl-names = "default", "state_100mhz", "state_200mhz";
        pinctrl-0 = <&pinctrl_usdhc1>, <&pinctrl_32k_clk>, <&pinctrl_wifi>;
        ...
        keep-power-in-suspend;
        vmmc-supply = <&reg_sd1_vmmc>;
        status = "okay";

Can this cause a complete loss of Wi-Fi functionality when the MMC driver
probes the interface during boot?

> +#include "imx6ul-imx6ull-var-wm8904.dtsi"

[Severity: High]
Does this file assign a mathematically incorrect and non-standard audio clock
frequency to the SAI2 master clock?

In arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-wm8904.dtsi:
&sai2 {
        assigned-clock-rates = <0>, <11565177>;
        ...

The SAI2 master clock is assigned to 11565177 Hz instead of the standard
12288000 Hz (which cleanly divides the 786432000 Hz PLL and provides exactly
256 * 48kHz). Can this cause audio playback or capture to be pitch-shifted, or
make the codec reject the clock entirely?

> +#include "imx6ul-var-som-concerto-board.dtsi"

[Severity: Medium]
Does this specify an undefined interrupt trigger (0) for the external
touchscreen controller?

In arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-som-concerto-board.dtsi:
touchscreen@38 {
        ...
        interrupt-parent = <&gpio5>;
        interrupts = <9 0>;
        ...

This specifies an undefined interrupt trigger (0, IRQ_TYPE_NONE) for the
edt-ft5406 touchscreen, which is an anti-pattern. Can this cause the
touchscreen driver to fail to receive interrupts and silently drop them,
rendering the touchscreen non-functional if the bootloader hasn't
pre-configured the GPIO interrupt trigger type?

>  
>  / {
>  	model = "Variscite VAR-SOM-6UL Concerto Board (6UL CPU)";

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790413032.git.stefano.r@variscite.com?part=7