Re: [PATCH v1 05/11] ARM: dts: imx6ul: Add Variscite storage and wireless files

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] dts: BRCM Wi-Fi fails to enumerate due to missing non-removable property
- [High] dts: IW61X Wi-Fi power enable pin lacks pinctrl configuration
- [Low] dts: unnecessary address and size cells in IW61X usdhc node
--

commit 7bf945c5fafb3aee26ff8892a479bb36b844b74e
Author: Stefano Radaelli <stefano.r@variscite.com>
ARM: dts: imx6ul: Add Variscite storage and wireless files

This patch adds reusable DTSI files for eMMC, NAND, Broadcom Wi-Fi, and
NXP IW61x options on VAR-SOM-6UL and DART-6UL modules. Keeping the module
options independent of the carrier board allows each board DTS to select
its assembled storage and wireless devices.

> diff --git a/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-dart-wifi-iw61x.dtsi b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-dart-wifi-iw61x.dtsi
> new file mode 100644
> index 0000000000000..4d47cea982ec4
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-dart-wifi-iw61x.dtsi
[ ... ]
> +	iw61x_pwrseq: mmc-pwrseq {
> +		compatible = "mmc-pwrseq-simple";
> +		post-power-on-delay-ms = <100>;
> +		power-off-delay-us = <10000>;
> +		reset-gpios = <&gpio5 6 GPIO_ACTIVE_LOW>, /* WIFI_RESET */
> +			      <&gpio5 2 GPIO_ACTIVE_LOW>; /* WIFI_PWR_EN */
> +	};

[Severity: High]
This references &gpio5 2 as the power enable pin (WIFI_PWR_EN), but is the
corresponding pinmux configuration missing from the pinctrl_wifi group?

Since pinctrl_wifi (defined in imx6ul-var-som.dtsi and imx6ul-var-dart.dtsi)
only configures GPIO5_IO04 and GPIO5_IO06, GPIO5_IO02 appears to be omitted
from the pinmux configuration.

Will this leave the pad in its default hardware reset state, preventing the
GPIO from toggling the Wi-Fi power enable line and leaving the module powered
off?

[ ... ]
> +&usdhc1 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;

[Severity: Low]
Are these address and size cells necessary here?

Unlike the BRCM equivalent which defines a child wifi node, there are no child
nodes under this SDIO controller in the IW61X implementation. Defining address
and size cells without any children will trigger avoid_unnecessary_addr_size
devicetree warnings when compiled with dtc.

> +	pinctrl-names = "default", "state_100mhz", "state_200mhz";
> +	pinctrl-0 = <&pinctrl_usdhc1>, <&pinctrl_wifi>;
[ ... ]
> diff --git a/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-som-wifi-brcm.dtsi b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-som-wifi-brcm.dtsi
> new file mode 100644
> index 0000000000000..d7952233a1ee9
> --- /dev/null
> +++ b/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>;
> +	pinctrl-1 = <&pinctrl_usdhc1_100mhz>, <&pinctrl_32k_clk>, <&pinctrl_wifi>;
> +	pinctrl-2 = <&pinctrl_usdhc1_200mhz>, <&pinctrl_32k_clk>, <&pinctrl_wifi>;
> +	keep-power-in-suspend;
> +	vmmc-supply = <&reg_sd1_vmmc>;
> +	status = "okay";

[Severity: High]
Does this node need the non-removable property to properly enumerate the
Broadcom Wi-Fi module?

Since the module is permanently soldered onto the SOM and does not have a
physical card detect mechanism, omitting non-removable causes the kernel
MMC core to wait indefinitely for a card detect interrupt. Consequently,
the SDIO bus probe might never complete and the Wi-Fi hardware could remain
unusable.

> +
> +	brcmf: wifi@1 {
> +		compatible = "brcm,bcm4329-fmac";
> +		reg = <1>;
> +	};
> +};
[ ... ]

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