Re: [PATCH v3 06/12] 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] mmc: pwrseq: simultaneous toggle of power and reset violates hardware sequencing
- [High] dts: brcm: missing power and reset GPIO configuration for Broadcom Wi-Fi
- [High] dts: brcm: completely missing Bluetooth UART node
- [Medium] mmc: missing non-removable property for soldered Broadcom Wi-Fi
- [Low] mtd: nand: partitions defined directly under controller violate modern bindings
--

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

    ARM: dts: imx6ul: Add Variscite storage and wireless files
    
    Add shared DTSI files for eMMC, NAND, Broadcom Wi-Fi and NXP
    IW61x options on VAR-SOM-6UL and DART-6UL. These options have the
    same descriptions on both modules, so use one file for each.

> diff --git a/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-nand.dtsi b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-nand.dtsi
> new file mode 100644
> index 0000000000000..1522b5d7e0924
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-nand.dtsi
> @@ -0,0 +1,63 @@
[ ... ]
> +&gpmi {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_gpmi_nand_1>;
> +	status = "okay";
> +
> +	partitions {
> +		compatible = "fixed-partitions";
> +		#address-cells = <1>;
> +		#size-cells = <1>;

[Severity: Low]
Should these partitions be placed under a NAND chip subnode (e.g., nand@0)
representing the physical chip, rather than directly under the NAND
controller (&gpmi)?

Modern NAND bindings (nand-controller.yaml) require this, and defining them
here might cause devicetree validation warnings and relies on deprecated
legacy behavior.

[ ... ]
> diff --git a/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-wifi-brcm.dtsi b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-wifi-brcm.dtsi
> new file mode 100644
> index 0000000000000..d7952233a1ee9
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-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]
Is the gpio property missing from this regulator?

The Broadcom Wi-Fi module requires its power enable and reset GPIOs to be
driven to function, but there is no gpio property here and the file
completely lacks an mmc-pwrseq node to release the reset. Will the chip
remain permanently powered off and in reset?

> +};
> +
> +&tsc {
> +	status = "disabled";
> +};
> +
> +&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: Medium]
Is the non-removable property missing from this node?

Without it, the MMC core incorrectly treats the hard-wired SDIO Wi-Fi chip
as removable, which can cause inefficient polling or failed probing if no
Card Detect (CD) GPIO is present. The companion var-wifi-iw61x.dtsi correctly
sets this property.

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

[Severity: High]
Is an &uart2 node missing from this file?

The Broadcom combo module requires a Bluetooth child node under its UART
(e.g., compatible = "brcm,bcm43438-bt") to initialize the BT functionality.
Will the Bluetooth portion remain uninitialized and unusable without it?

[ ... ]
> diff --git a/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-wifi-iw61x.dtsi b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-wifi-iw61x.dtsi
> new file mode 100644
> index 0000000000000..d2f116bc8d70f
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-wifi-iw61x.dtsi
> @@ -0,0 +1,43 @@
[ ... ]
> +/ {
> +	reg_sd1_vmmc: regulator-sd1-vmmc {
> +		compatible = "regulator-fixed";
> +		regulator-name = "VSD_3V3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +	};
> +
> +	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]
Does placing both the power enable (WIFI_PWR_EN) and reset (WIFI_RESET) pins
in this reset-gpios array cause them to transition state at the exact same
instant?

The mmc_pwrseq_simple driver toggles all pins in this array simultaneously
using gpiod_multi_set_value_cansleep() in drivers/mmc/core/pwrseq_simple.c,
which violates the hardware requirement that power must be stable before the
reset line is released. Could this lead to unreliable or failed Wi-Fi
initialization?

> +	};
> +};

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