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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] dts: missing hardware power and reset sequence for Broadcom Wi-Fi
- [High] dts: missing 'non-removable' property for Broadcom Wi-Fi SDIO interface
- [Low] dt-bindings: unnecessary #address-cells and #size-cells properties
- [Low] dts: 100% code duplication for DART and SOM DTSI variants
- [Low] dts: missing <dt-bindings/gpio/gpio.h> for GPIO_ACTIVE_LOW macro
--

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

    ARM: dts: imx6ul: Add Variscite storage and wireless files
    
    Add reusable DTSI files for eMMC, NAND, Broadcom Wi-Fi and NXP IW61x
    options on VAR-SOM-6UL and DART-6UL modules.
    
    Keep the module options independent of the carrier board so each
    board DTS can select its assembled storage and wireless devices.

> diff --git a/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-dart-emmc.dtsi b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-dart-emmc.dtsi
> new file mode 100644
> index 0000000000000..26c8ca8f21a0c
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-dart-emmc.dtsi
> @@ -0,0 +1,32 @@
> +// SPDX-License-Identifier: GPL-2.0+

[Severity: Low]
Does this file need to be separate? It is a 100% exact duplicate of
imx6ul-imx6ull-var-som-emmc.dtsi.

Could these variants be extracted into a shared file like
imx6ul-imx6ull-var-common-emmc.dtsi to avoid duplication?

[ ... ]

> diff --git a/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-dart-wifi-brcm.dtsi b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-dart-wifi-brcm.dtsi
> new file mode 100644
> index 0000000000000..d7952233a1ee9
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-dart-wifi-brcm.dtsi
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2026 Variscite Ltd. - https://www.variscite.com
> + */
> +
> +/ {
> +	reg_sd1_vmmc: regulator-sd1-vmmc {
> +		compatible = "regulator-fixed";
> +		regulator-name = "VSD_3V3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +	};

[Severity: High]
Does this regulator need a gpio property to drive WIFI_PWR_EN?

Without asserting WIFI_PWR_EN and providing an mmc-pwrseq to release
WIFI_RESET, the SDIO interface might fail to detect the Broadcom hardware.

Also, is the bluetooth node intended to be omitted here compared to the
IW61X configurations?

[ ... ]

> +&usdhc1 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	pinctrl-names = "default", "state_100mhz", "state_200mhz";

[Severity: High]
Should this node include the 'non-removable' property?

Since the Broadcom Wi-Fi chip is physically soldered onto the module, does
the SDIO host controller need this property to properly initialize without
falling back to spurious polling?

[ ... ]

> 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
> @@ -0,0 +1,45 @@
> +// SPDX-License-Identifier: GPL-2.0+

[ ... ]

> +	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: Low]
Does this file need to explicitly include <dt-bindings/gpio/gpio.h>?

Since the GPIO_ACTIVE_LOW macro is used here, missing the header might cause
dtc build failures if a parent file does not implicitly include it.

[ ... ]

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

[Severity: Low]
Are these properties necessary here?

Since there are no child nodes declared under usdhc1 for the iw61x variant,
having these properties might trigger the avoid_unnecessary_addr_size dtc
warning.

[ ... ]

> 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
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0+

[Severity: Low]
Does this file need to be separate? It is a 100% exact duplicate of
imx6ul-imx6ull-var-dart-wifi-brcm.dtsi.

Could these variants be extracted into a single shared file to avoid
code duplication?

[ ... ]

> +&usdhc1 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	pinctrl-names = "default", "state_100mhz", "state_200mhz";

[Severity: High]
Should this node include the 'mmc-pwrseq' assignment to properly drive
WIFI_RESET, as well as the 'non-removable' property?

Since the Broadcom Wi-Fi chip is physically soldered onto the module,
omitting these might prevent proper initialization and hardware probing.

[ ... ]

> diff --git a/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-som-wifi-iw61x.dtsi b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-som-wifi-iw61x.dtsi
> new file mode 100644
> index 0000000000000..4d47cea982ec4
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-som-wifi-iw61x.dtsi
> @@ -0,0 +1,45 @@
> +// SPDX-License-Identifier: GPL-2.0+

[ ... ]

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

[Severity: Low]
Are these properties needed?

Without child nodes under this usdhc1 block, this might generate an
avoid_unnecessary_addr_size dtc warning.

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