Re: [PATCH v1 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 4 potential issue(s) to consider:
- [High] Contradictory GPIO polarity in fixed regulator `reg_eth_phy` between `enable-active-high` and `GPIO_ACTIVE_LOW`.
- [High] Simultaneous deassertion of power enable and reset in `wifi_pwrseq` violates hardware sequencing requirements.
- [Medium] The audio card hardcodes carrier-board-specific jacks in the SOM .dtsi, violating the module separation described in the commit message.
- [Low] The `bluetooth` node is missing the necessary GPIO property assignments (`device-wakeup-gpios`, etc.) required to control the Bluetooth chip, despite the pins being configured in `pinctrl_bt`.
--

commit 9e70fb8b6cc043ee758ca5e7801e30846eec594b
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.

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
> @@ -0,0 +1,559 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Common dtsi for Variscite DART-MX8M-MINI
[ ... ]
> +	reg_eth_phy: regulator-eth-phy {
> +		compatible = "regulator-fixed";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_reg_eth_phy>;
> +		regulator-name = "eth_phy_pwr";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		regulator-enable-ramp-delay = <20000>;
> +		gpio = <&gpio1 7 GPIO_ACTIVE_LOW>;
> +		enable-active-high;
> +	};

[Severity: High]
Does the PHY power switch require an active-low or active-high signal?

The reg_eth_phy node uses GPIO_ACTIVE_LOW but also specifies
enable-active-high. Under the regulator framework, enable-active-high forces
the GPIO to be treated as active-high, overriding the GPIO_ACTIVE_LOW flag.

If the power switch requires an active-low signal, it might fail to enable
the PHY. If it requires an active-high signal, the GPIO_ACTIVE_LOW flag
seems misleading.

[ ... ]
> +	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";
> +		simple-audio-card,widgets =
> +			"Microphone", "Microphone Jack",
> +			"Headphone", "Headphone Jack",
> +			"Line", "Line In Jack";

[Severity: Medium]
Should this routing and widget configuration be moved to the carrier board
device tree instead?

The commit message states that carrier board specific interfaces are left
disabled to be enabled in respective carrier board device trees. However,
this sound-wm8904 node hardcodes routing for physical jacks (Headphone, Line
In, Microphone) which normally reside on the carrier board.

Including this directly in the module's .dtsi forces all carrier boards to
inherit this exact jack topology.

[ ... ]
> +	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: High]
Does toggling these GPIOs simultaneously violate the hardware sequencing
requirements for the Wi-Fi chip?

Grouping both WIFI_RESET and WIFI_PWR_EN in the reset-gpios property of
mmc-pwrseq-simple causes the driver to toggle both lines at the exact same
moment.

Would it be safer to model the power enable signal as a vmmc-supply with a
startup-delay-us? This would allow the power rail to fully stabilize before
the reset line is released.

[ ... ]
> +/* 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";
> +	};
> +};

[ ... ]
> +&iomuxc {
> +	pinctrl_bt: btgrp {
> +		fsl,pins = <
> +			MX8MM_IOMUXC_SD1_DATA4_GPIO2_IO6		0xc1
> +			MX8MM_IOMUXC_SPDIF_EXT_CLK_GPIO5_IO5		0xc1
> +		>;
> +	};

[Severity: Low]
Are we missing driver property assignments for these Bluetooth pins?

The pinctrl_bt group configures GPIO2_IO6 and GPIO5_IO5 with pull-ups
disabled (0xc1), and assigns them to the uart4 node. However, the
bluetooth_iw61x node does not assign these pins to any driver properties
(like device-wakeup-gpios).

Because the driver cannot manage them, does this leave the pins physically
floating, which could cause spurious wakeups or increased power consumption?

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