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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The ads7846 touchscreen specifies an active-high GPIO for pendown, which conflicts with its active-low interrupt and breaks touch detection.
- [High] The V1 hardware WiFi regulator (`reg_vmmc_wl`) lacks a `gpio` property and relies on a userspace script to enable WiFi power, breaking automatic device probing.
- [Medium] The `wm8904` codec's `IN1L` pin is configured as a digital microphone input but is simultaneously routed to an analog 'Microphone Jack' in the DAPM topology.
- [Low] Non-generic node names and invalid characters are used for the touchscreen, audio codec devices, and regulators.
--

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

This commit adds device tree support for the Variscite VAR-SOM-MX7 system
on module based on the NXP i.MX7Dual SoC. It includes the common SoM
description along with support 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
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx7d-var-som-mx7customboard.dtsi
> @@ -0,0 +1,382 @@
[ ... ]
> +	reg_hsic_hub_connect: regulator_hsic_hub_connect {

[Severity: Low]
Are there invalid characters in this node name? The Devicetree Specification
prohibits the use of underscores in node names, which should use hyphens
instead.

> 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
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx7d-var-som-wm8731.dtsi
> @@ -0,0 +1,65 @@
[ ... ]
> +&i2c2 {
> +	wm8731: wm8731@1a {

[Severity: Low]
Does this node name follow the Devicetree Specification? Node names should be
generic and reflect the function of the device rather than its precise part
number. Should this be named audio-codec@1a instead?

> diff --git a/arch/arm/boot/dts/nxp/imx/imx7d-var-som.dtsi b/arch/arm/boot/dts/nxp/imx/imx7d-var-som.dtsi
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx7d-var-som.dtsi
> @@ -0,0 +1,635 @@
[ ... ]
> +	codec_wm8904: sound {
> +		compatible = "simple-audio-card";
> +		simple-audio-card,bitclock-master = <&dailink_master>;
> +		simple-audio-card,format = "i2s";
> +		simple-audio-card,frame-master = <&dailink_master>;
> +		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",
[ ... ]
> +	wm8904: audio-codec@1a {
> +		compatible = "wlf,wm8904";
> +		reg = <0x1a>;
[ ... ]
> +		/* DMIC is connected to IN1L */
> +		wlf,in1l-as-dmicdat1;
> +	};

[Severity: Medium]
Is this DAPM routing correct? This maps IN1L to an analog "Microphone Jack",
but later in the wm8904 node, wlf,in1l-as-dmicdat1 explicitly configures it
as a digital microphone input.

Can this topological contradiction cause the DAPM subsystem to incorrectly
power up analog components for a digital microphone, or present invalid analog
paths to userspace?

[ ... ]
> +	/* Resistive touch controller */
> +	ads7846_ts: ads7846@0 {

[Severity: Low]
Should this node use a generic name like touchscreen@0 instead of the
specific part number to align with Devicetree Specification requirements?

> +		reg = <0>;
> +		compatible = "ti,ads7846";
> +		pinctrl-names ="default";
> +		pinctrl-0 = <&pinctrl_restouch>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <9 IRQ_TYPE_EDGE_FALLING>;
> +		spi-max-frequency = <1500000>;
> +		pendown-gpio = <&gpio1 9 0>;

[Severity: High]
Does this configuration break touch detection? The interrupt is defined as
active-low (IRQ_TYPE_EDGE_FALLING), but the GPIO flag here is 0 (active-high).

When the interrupt handler fires, it will read the physical low signal as
a logical false, causing it to immediately abort without processing the event.
Should this use GPIO_ACTIVE_LOW instead?

[ ... ]
> +	reg_vmmc_wl: regulator-vmmc-wl {
> +		compatible = "regulator-fixed";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		regulator-name = "wlreg_on";
> +	};
[ ... ]
> +&usdhc2 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	pinctrl-names = "default", "state_100mhz", "state_200mhz","sleep";
> +	pinctrl-0 = <&pinctrl_usdhc2>, <&pinctrl_wlan>;
> +	pinctrl-1 = <&pinctrl_usdhc2_100mhz>, <&pinctrl_wlan>;
> +	pinctrl-2 = <&pinctrl_usdhc2_200mhz>, <&pinctrl_wlan>;
> +	pinctrl-3 = <&pinctrl_usdhc2_sleep>, <&pinctrl_wlan_sleep>;
> +	keep-power-in-suspend;
> +	non-removable;
> +	vmmc-supply = <&reg_vmmc_wl>;
> +	status = "okay";
[ ... ]
> +	/* GPIO Used in User Space init script */
> +	pinctrl_wlan: wlangrp {
> +		fsl,pins = <
> +			MX7D_PAD_LPSR_GPIO1_IO04__GPIO1_IO4		0x09
> +		>;
> +	};

[Severity: High]
Will this cause the SDIO bus probe to fail permanently during boot?

The usdhc2 node is marked non-removable and bound to reg_vmmc_wl. Because
this regulator lacks a gpio property to physically power the chip, the MMC
core will attempt to enumerate the bus while the WiFi chip is unpowered.

Since the bus is non-removable, it will not retry when the userspace
script later toggles the GPIO via pinctrl_wlan. Should the gpio property
be moved to the regulator-fixed node so the kernel can properly probe it?

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