Re: [PATCH v2 2/3] arm64: dts: freescale: Add support for Variscite DART-MX8M-MINI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Simultaneous assertion/deassertion of Wi-Fi power enable and reset pins in `wifi_pwrseq`, violating typical SDIO Wi-Fi power sequencing requirements.
- [Medium] Bluetooth GPIOs configured in `pinctrl_bt` are not assigned to or claimed by the Bluetooth device node, leaving them as unmanaged floating inputs.
- [Medium] WM8904 codec configuration contains contradictory settings blindly copied from binding examples, likely breaking microphone functionality.
--

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

arm64: dts: freescale: Add support for Variscite DART-MX8M-MINI

Add device tree support for the Variscite DART-MX8MM system on module.
This SOM is designed to be used with various carrier boards. Only
SOM-specific peripherals are enabled by default.

Link: https://variscite.com/system-on-module-som/i-mx-8/i-mx-8m-mini/dart-mx8m-mini/

> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-var-dart.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-var-dart.dtsi
> new file mode 100644
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-var-dart.dtsi

[ ... ]

> +	sound-wm8904 {
> +		compatible = "simple-audio-card";
> +		simple-audio-card,bitclock-master = <&codec_dai>;
> +		simple-audio-card,format = "i2s";
> +		simple-audio-card,frame-master = <&codec_dai>;
> +		simple-audio-card,mclk-fs = <256>;
> +		simple-audio-card,name = "wm8904-audio";
> +		simple-audio-card,routing =
> +			"Headphone Jack", "HPOUTL",
> +			"Headphone Jack", "HPOUTR",
> +			"IN2L", "Line In Jack",
> +			"IN2R", "Line In Jack",
> +			"IN1L", "Microphone Jack",
> +			"IN1R", "Microphone Jack";

[Severity: Medium]
Is this analog microphone routing missing a connection to MICBIAS?

The simple-audio-card,routing property routes the Microphone Jack to the
codec's analog inputs (IN1L/IN1R), but without a connection to the codec's
MICBIAS pin, the analog microphone will likely not receive the required bias
voltage to function.

> +		simple-audio-card,widgets =
> +			"Microphone", "Microphone Jack",
> +			"Headphone", "Headphone Jack",
> +			"Line", "Line In Jack";

[ ... ]

> +	wifi_pwrseq: wifi-pwrseq {
> +		compatible = "mmc-pwrseq-simple";
> +		post-power-on-delay-ms = <100>;
> +		power-off-delay-us = <10000>;
> +		reset-gpios = <&gpio2 10 GPIO_ACTIVE_LOW>, /* WIFI_RESET */
> +			      <&gpio2 20 GPIO_ACTIVE_LOW>; /* WIFI_PWR_EN */
> +	};

[Severity: Medium]
Does grouping both the power enable and reset pins in this single reset-gpios
array violate SDIO Wi-Fi power sequencing requirements?

The mmc-pwrseq-simple driver toggles all GPIOs in the reset-gpios array
simultaneously. During power-on, the Wi-Fi chip's power enable and reset lines
will be driven high at the exact same moment.

Standard Wi-Fi power sequencing typically requires the power supply to be
fully ramped and stable before the reset line is released.

[ ... ]

> +&i2c3 {
> +	clock-frequency = <100000>;
> +	pinctrl-names = "default", "gpio";
> +	pinctrl-0 = <&pinctrl_i2c3>;
> +	pinctrl-1 = <&pinctrl_i2c3_gpio>;
> +	scl-gpios = <&gpio5 18 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> +	sda-gpios = <&gpio5 19 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> +	status = "okay";
> +
> +	wm8904: audio-codec@1a {
> +		compatible = "wlf,wm8904";
> +		reg = <0x1a>;

[ ... ]

> +		/* GPIO1 = DMIC_CLK, don't touch others */
> +		wlf,gpio-cfg = <0x0018>, <0xffff>, <0xffff>, <0xffff>;
> +	};
> +};

[Severity: Medium]
Does this digital microphone clock configuration contradict the analog audio
routing defined above?

Configuring GPIO1 as a digital microphone clock appears to be a direct copy
from the wlf,wm8904 binding example, but it contradicts the analog routing for
the Microphone Jack to IN1L/IN1R.

This might expose unintended side-effects by driving a digital clock out of
GPIO1 when it's not needed for an analog microphone setup.

[ ... ]

> +/* BT module */
> +&uart4 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_uart4>, <&pinctrl_bt>;
> +	assigned-clocks = <&clk IMX8MM_CLK_UART4>;
> +	assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_80M>;
> +	uart-has-rtscts;
> +	status = "okay";
> +
> +	bluetooth_iw61x: bluetooth {
> +		compatible = "nxp,88w8987-bt";
> +	};
> +};

[Severity: Medium]
Are the Bluetooth GPIOs defined in pinctrl_bt missing their assignments here?

The pinctrl_bt group defines GPIO2_IO6 and GPIO5_IO5 with floating pad
configurations (0xc1), and the parent uart4 node applies them. However,
because this bluetooth node doesn't map them to expected driver properties
(such as device-wakeup-gpios or host-wakeup-gpios), they are left as
unmanaged floating inputs.

If one of these acts as an enable or reset pin, floating inputs can cause
erratic behavior due to voltage drift.

> +/* WIFI */
> +&usdhc1 {
> +	pinctrl-names = "default", "state_100mhz", "state_200mhz";
> +	pinctrl-0 = <&pinctrl_usdhc1>, <&pinctrl_wifi>;
> +	pinctrl-1 = <&pinctrl_usdhc1_100mhz>, <&pinctrl_wifi>;
> +	pinctrl-2 = <&pinctrl_usdhc1_200mhz>, <&pinctrl_wifi>;
> +	bus-width = <4>;
> +	keep-power-in-suspend;
> +	mmc-pwrseq = <&wifi_pwrseq>;
> +	non-removable;
> +	status = "okay";
> +};

[Severity: Medium]
Should this node include a vmmc-supply linking to a regulator for the Wi-Fi
power enable?

Modeling WIFI_PWR_EN as a regulator-fixed assigned to vmmc-supply in this
usdhc1 node, while leaving only WIFI_RESET in the mmc-pwrseq-simple node,
would ensure the power sequencing is handled correctly.

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