Re: [PATCH v2 06/11] ARM: dts: imx6ul: Add Variscite carrier board files

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] dt: pinctrl configures SNVS pin in wrong iomuxc on i.MX6ULL breaking extcon
- [Medium] dt: SD card fixed regulator reg_sd1_vmmc is unreferenced by usdhc1
- [Low] dt: interrupts use IRQ_TYPE_NONE or 0 instead of hardware trigger type
- [Low] dt: ineffective backlight node disable on i.MX6ULZ Concerto board
--

commit 0f5f420751205e116ebd8d4705179e8c32c39131
Author: Stefano Radaelli <stefano.r@variscite.com>
ARM: dts: imx6ul: Add Variscite carrier board files

This commit adds DTSI files for the Concerto-Board, Symphony-Board, and
VAR-6ULCustomBoard carriers used with VAR-SOM-6UL and DART-6UL. It shares
common carrier wiring across SoC variants and keeps SoC-specific connections
in separate wrappers.

> diff --git a/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-dart-6ulcustomboard-sd-card.dtsi b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-dart-6ulcustomboard-sd-card.dtsi
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-dart-6ulcustomboard-sd-card.dtsi

[ ... ]

> +	reg_sd1_vmmc: regulator-sd1-vmmc {
> +		compatible = "regulator-fixed";
> +		regulator-name = "VSD_3V3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +	};
> +};
> +
> +&usdhc1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_usdhc1>, <&pinctrl_usdhc1_gpio>;
> +	cd-gpios = <&gpio4 19 GPIO_ACTIVE_LOW>;
> +	no-1-8-v;
> +	keep-power-in-suspend;
> +	wakeup-source;
> +	status = "okay";
> +};

[Severity: Medium]
Should this node reference the regulator via a vmmc-supply = <&reg_sd1_vmmc>;
property? Without it, the MMC subsystem is unaware of the slot's operating
voltage, and the regulator framework might leave it uninitialized or disabled
since it appears unreferenced.

> diff --git a/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-dart-6ulcustomboard.dtsi b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-dart-6ulcustomboard.dtsi
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-dart-6ulcustomboard.dtsi

[ ... ]

> +	touchscreen@38 {
> +		compatible = "edt,edt-ft5406";
> +		reg = <0x38>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_ft5x06_ts_gpio>;
> +		interrupt-parent = <&gpio3>;
> +		interrupts = <4 0>;

[Severity: Low]
Does this properly configure the hardware trigger type? Specifying 0 instead
of a physical active state (like IRQ_TYPE_EDGE_FALLING or
IRQ_TYPE_LEVEL_LOW) causes the kernel to rely on bootloader remnants or
driver-level hardcoded overrides.

> diff --git a/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-som-concerto-board-sd-card.dtsi b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-som-concerto-board-sd-card.dtsi
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-som-concerto-board-sd-card.dtsi

[ ... ]

> +&usdhc1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_usdhc1>, <&pinctrl_usdhc1_gpio>;
> +	cd-gpios = <&gpio1 0 GPIO_ACTIVE_LOW>;
> +	no-1-8-v;
> +	keep-power-in-suspend;
> +	wakeup-source;
> +	status = "okay";
> +};

[Severity: Medium]
Similarly here, should this node link the 3.3V fixed regulator using
vmmc-supply = <&reg_sd1_vmmc>; to ensure it is properly managed?

> diff --git a/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-som-concerto-board.dtsi b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-som-concerto-board.dtsi
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-som-concerto-board.dtsi

[ ... ]

> +	touchscreen@38 {
> +		compatible = "edt,edt-ft5406";
> +		reg = <0x38>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_ft5x06_ts_gpio>;
> +		interrupt-parent = <&gpio5>;
> +		interrupts = <9 0>;

[Severity: Low]
Is the interrupt trigger type intentionally set to 0 here as well? Providing
the explicit trigger type described in the device specification and schematic
is generally preferred.

> diff --git a/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-som-symphony-board.dtsi b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-som-symphony-board.dtsi
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6ul-imx6ull-var-som-symphony-board.dtsi

[ ... ]

> +	/* USB Type-C Controller */
> +	extcon_ptn5150: typec@3d {
> +		compatible = "nxp,ptn5150";
> +		reg = <0x3d>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_extcon>;
> +		interrupt-parent = <&gpio5>;
> +		interrupts = <3 IRQ_TYPE_NONE>;

[Severity: Low]
Does this correctly describe the physical interrupt pin? IRQ_TYPE_NONE
often relies on driver-level fallbacks rather than strictly defining the
expected hardware state in the devicetree.

[ ... ]

> +&iomuxc {
> +	pinctrl_ft5x06_ts_gpio: ft5x06-ts-gpio-grp {

[ ... ]

> +	pinctrl_extcon: extcon-grp {
> +		fsl,pins = <
> +			MX6UL_PAD_SNVS_TAMPER3__GPIO5_IO03		0x1b0b0
> +		>;
> +	};

[Severity: High]
Will this clobber the JTAG TDO pad on i.MX6ULL/ULZ variants? On i.MX6UL,
SNVS tamper pads are configured via the main iomuxc controller, which works
correctly. However, on i.MX6ULL and i.MX6ULZ, SNVS tamper pads were moved to
a completely separate iomuxc_snvs controller with different base addresses.

Since this is defined inside the shared &iomuxc node, booting an i.MX6ULL
will incorrectly apply the SNVS pin offset (0x0028) to the main iomuxc
base, breaking the Type-C extcon interrupt. Should this be moved or
overridden in the i.MX6ULL specific file using &iomuxc_snvs?

> diff --git a/arch/arm/boot/dts/nxp/imx/imx6ul-var-som-symphony-board.dtsi b/arch/arm/boot/dts/nxp/imx/imx6ul-var-som-symphony-board.dtsi
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6ul-var-som-symphony-board.dtsi

[ ... ]

> +	touchscreen@38 {
> +		compatible = "edt,edt-ft5406";
> +		reg = <0x38>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_ft5x06_ts_gpio>;
> +		interrupt-parent = <&gpio3>;
> +		interrupts = <6 0>;

[Severity: Low]
Like the other touchscreen definitions, should this specify the exact
hardware interrupt trigger instead of 0?

> diff --git a/arch/arm/boot/dts/nxp/imx/imx6ulz-var-som-concerto-board.dtsi b/arch/arm/boot/dts/nxp/imx/imx6ulz-var-som-concerto-board.dtsi
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6ulz-var-som-concerto-board.dtsi

[ ... ]

> +#include "imx6ul-imx6ull-var-som-concerto-board.dtsi"
> +
> +/ {
> +	backlight {
> +		status = "disabled";
> +	};
> +};

[Severity: Low]
Will this actually disable the backlight? The common inclusion file
imx6ul-imx6ull-var-som-concerto-board.dtsi defines the node with the name
lcd-backlight rather than backlight. 

Because the node names mismatch, this will likely leave the lcd-backlight
node enabled and create a dead node named backlight. The kernel might
unexpectedly attempt to probe the PWM backlight driver. Should this be
&lcd_backlight { status = "disabled"; }; instead?

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