Re: [PATCH v1 2/3] ARM: dts: imx7d: add Variscite VAR-SOM-MX7 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] The Ethernet PHY reset pin for `ethphy1` is muxed on the `fec2` node instead of `fec1`, causing PHY detection to fail during MDIO bus probe.
- [Medium] Use of `IRQ_TYPE_NONE` (0) for touchscreen controller interrupts.
- [Low] Duplicate `assigned-clock-rates` property in the `&sai1` node.
- [Low] Attempting to delete a non-existent child node `bcrmf@1` instead of `wifi@1`.
- [Low] Missing required `color` property in the GPIO LED definition.
- [Low] Use of deprecated `reset-gpio` property instead of `reset-gpios` in the PCIe node.
--

--- Patch [2]: [PATCH v1 2/3] ARM: dts: imx7d: add Variscite VAR-SOM-MX7 support ---
commit 910d1859fce3905900b424cb29918719c63275b0
Author: Stefano Radaelli <stefano.r@variscite.com>

ARM: dts: imx7d: add Variscite VAR-SOM-MX7 support

This commit adds device tree support for the Variscite VAR-SOM-MX7 system on
module. It provides the common SoM description along with configurations for
eMMC, NAND, WM8731 audio codec, and the V2 hardware revision.

Link: https://variscite.com/system-on-module-som/i-mx-7/var-som-mx7/

> diff --git a/arch/arm/boot/dts/nxp/imx/imx7d-var-som-mx7customboard.dtsi b/arch/arm/boot/dts/nxp/imx/imx7d-var-som-mx7customboard.dtsi
> index 0000000000000..82814535fb93e 100644
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx7d-var-som-mx7customboard.dtsi
> @@ -0,0 +1,378 @@
[ ... ]
> +	leds {
> +		compatible = "gpio-leds";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_gpio_leds>;
> +
> +		led-d10 {
> +			function = LED_FUNCTION_STATUS;
> +			gpios = <&gpio1 13 GPIO_ACTIVE_HIGH>;
> +			linux,default-trigger = "heartbeat";
> +		};
> +	};

[Severity: Low]
Does this led-d10 node also need a color property? When function is
specified, modern LED bindings generally require the color property to form
sysfs directories in the format color:function.

[ ... ]
> +&i2c4 {
> +	clock-frequency = <100000>;
> +	pinctrl-names = "default", "gpio";
> +	pinctrl-0 = <&pinctrl_i2c4>;
> +	pinctrl-1 = <&pinctrl_i2c4_gpio>;
> +	scl-gpios = <&gpio4 14 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> +	sda-gpios = <&gpio4 15 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> +	status = "okay";
> +
> +	ft5x06_ts: touchscreen@38 {
> +		compatible = "edt,edt-ft5206";
> +		reg = <0x38>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_touch>;
> +		interrupt-parent = <&gpio2>;
> +		interrupts = <30 0>;

[Severity: Medium]
Is it intentional to use 0 (IRQ_TYPE_NONE) for the touchscreen interrupt
trigger type here? This can cause the interrupt to be unconfigured or
incorrectly configured by default, which may lead to missed touch events or
an interrupt storm.

[ ... ]
> +&pcie {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_pcie>;
> +	reset-gpio = <&gpio2 31 GPIO_ACTIVE_LOW>;

[Severity: Low]
Could this use the standard reset-gpios property instead? The reset-gpio
property is deprecated, and dtbs_check validation may emit warnings for it.

> diff --git a/arch/arm/boot/dts/nxp/imx/imx7d-var-som-v2.dtsi b/arch/arm/boot/dts/nxp/imx/imx7d-var-som-v2.dtsi
> index 0000000000000..724aa6fb36de2 100644
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx7d-var-som-v2.dtsi
> @@ -0,0 +1,64 @@
[ ... ]
> +/* WiFi */
> +&usdhc2 {
> +	mmc-pwrseq = <&wifi_pwrseq>;
> +	/delete-node/ bcrmf@1;
> +};

[Severity: Low]
Is there a typo in the node name here? The node defined in the base dtsi is
wifi@1 (with the label brcmf). Trying to delete bcrmf@1 will cause the
device tree compiler to fail since the node doesn't exist.

> diff --git a/arch/arm/boot/dts/nxp/imx/imx7d-var-som-wm8731.dtsi b/arch/arm/boot/dts/nxp/imx/imx7d-var-som-wm8731.dtsi
> index 0000000000000..5fab164a64820 100644
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx7d-var-som-wm8731.dtsi
> @@ -0,0 +1,65 @@
[ ... ]
> +&sai1 {
> +	assigned-clock-rates = <0>, <36864000>;
> +	assigned-clocks = <&clks IMX7D_AUDIO_MCLK_ROOT_SRC>,
> +			  <&clks IMX7D_AUDIO_MCLK_ROOT_DIV>;
> +	assigned-clock-parents = <&clks IMX7D_PLL_AUDIO_POST_DIV>;
> +	assigned-clock-rates = <0>, <12288000>;

[Severity: Low]
Does this node intend to duplicate the assigned-clock-rates property? The
second definition (<0>, <12288000>) will silently overwrite the first one
(<0>, <36864000>), which will cause DTC to emit a duplicate property warning
and the intended 36.864MHz clock rate configuration to be lost.

> +	/delete-property/ fsl,sai-mclk-direction-output;
> +};
> diff --git a/arch/arm/boot/dts/nxp/imx/imx7d-var-som.dtsi b/arch/arm/boot/dts/nxp/imx/imx7d-var-som.dtsi
> index 0000000000000..cbd6252bd18f3 100644
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx7d-var-som.dtsi
> @@ -0,0 +1,633 @@
[ ... ]
> +&ecspi1 {
> +	pinctrl-names = "default", "sleep";
> +	pinctrl-0 = <&pinctrl_ecspi1>;
> +	pinctrl-1 = <&pinctrl_ecspi1_sleep>;
> +	cs-gpios = <&gpio4 19 GPIO_ACTIVE_LOW>;
> +	status = "okay";
> +
> +	/* Resistive touch controller */
> +	ads7846_ts: ads7846@0 {
> +		reg = <0>;
> +		compatible = "ti,ads7846";
> +		pinctrl-names ="default";
> +		pinctrl-0 = <&pinctrl_restouch>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <9 0>;

[Severity: Medium]
Is it intentional to use 0 (IRQ_TYPE_NONE) for this touch controller interrupt
trigger type? This might result in the interrupt being unconfigured by default.

[ ... ]
> +&fec1 {
> +	pinctrl-names = "default","sleep";
> +	pinctrl-0 = <&pinctrl_enet1>, <&pinctrl_enet1_rst>;
[ ... ]
> +	mdio {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		ethphy0: ethernet-phy@0 {
> +			compatible = "ethernet-phy-ieee802.3-c22";
> +			reg = <0>;
> +			reset-gpios = <&gpio5 11 GPIO_ACTIVE_LOW>;
> +			reset-assert-us = <20000>;
> +			reset-deassert-us = <80000>;
> +			vddio-supply = <&reg_phy_vddio>;
> +		};
> +
> +		ethphy1: ethernet-phy@1 {
> +			compatible = "ethernet-phy-ieee802.3-c22";
> +			reg = <1>;
> +			reset-gpios = <&gpio4 3 GPIO_ACTIVE_LOW>;
> +			reset-assert-us = <20000>;
> +			reset-deassert-us = <80000>;
> +			vddio-supply = <&reg_phy_vddio>;
> +		};
> +	};
> +};
> +
> +&fec2 {
> +	pinctrl-names = "default","sleep";
> +	pinctrl-0 = <&pinctrl_enet2>, <&pinctrl_enet2_rst>;

[Severity: High]
Does ethphy1 fail to reset during the MDIO bus probe on fec1?

Since ethphy1 is a child of the fec1 MDIO bus, fec1 will attempt to toggle
its reset GPIO (&gpio4 3) when it probes. However, the pinctrl for this GPIO
(&pinctrl_enet2_rst) is attached to fec2 instead of fec1. Since fec2 has
not probed yet, the reset pin may not be muxed properly, preventing the reset
toggle from reaching the PHY and causing detection to fail.

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