Re: [PATCH 3/3] arm64: dts: imx95: Add support for Data Modul i.MX95 eDM SBC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] dts: imx95: I2C expanders stuck in reset due to gpio-hog mismatch
- [Medium] dts: imx95: Incorrect PCAL6524 interrupt polarity
- [Medium] dts: imx95: Ethernet PHY reset held active due to wrong polarity
- [Medium] dts: imx95: Missing pinctrl for Ethernet PHY interrupt
- [Medium] dts: imx95: M.2 Key-E (usdhc3) slot left unpowered
- [Medium] dts: imx95: reg_panel_vcc inverted polarity missing enable-active-high
- [Low] dts: imx95: reg_pcie1 misnamed as Key-E instead of Key-M
--

commit 79360138b5f32bc771e33534a4b368370f38afbf
Author: Marek Vasut <marex@nabladev.com>
Subject: arm64: dts: imx95: Add support for Data Modul i.MX95 eDM SBC

This commit adds device tree support for the Data Modul i.MX95 eDM SBC. It
configures essential board features including serial consoles, ethernet, eMMC,
SD, SPI NOR, USB host, and PCIe.

> diff --git a/arch/arm64/boot/dts/freescale/imx95-data-modul-edm-sbc.dts b/arch/arm64/boot/dts/freescale/imx95-data-modul-edm-sbc.dts
> new file mode 100644
> index 0000000000000..01272ab6ae0b2
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx95-data-modul-edm-sbc.dts
> @@ -0,0 +1,1111 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (C) 2025-2026 Marek Vasut <marex@nabladev.com>
> + */

[ ... ]

> +	reg_panel_vcc: regulator-panel-vcc {
> +		compatible = "regulator-fixed";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		regulator-name = "PANEL_VCC";
> +		gpio = <&i2c2_gpio_expander_20 17 0>;

[Severity: Medium]
Does this regulator need an enable-active-high property? Without it, and
using 0 for the GPIO flag, the fixed-regulator driver will interpret this
as active-low, which might drive the power enable pin low when requested
to turn on.

> +		/* Disabled by default, unless display board plugged in. */
> +		status = "disabled";
> +	};
> +
> +	reg_pcie0: regulator-pcie0 {
> +		compatible = "regulator-fixed";
> +		regulator-max-microvolt = <3300000>;
> +		regulator-min-microvolt = <3300000>;
> +		regulator-name = "PCIE_M2-B_EN";
> +		gpio = <&i2c2_gpio_expander_20 6 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +	};
> +
> +	reg_pcie1: regulator-pcie1 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "PCIE_M2-E_EN";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		gpio = <&i2c2_gpio_expander_21 7 GPIO_ACTIVE_HIGH>;

[Severity: Low]
Is this regulator name correct? The GPIO targets expander 21 pin 7, which
is labeled "VDD_3V3_M2-M_EN" in the pcal6524 line names, but the regulator
name here is "PCIE_M2-E_EN". Should this be named "PCIE_M2-M_EN"?

> +		enable-active-high;
> +	};

[ ... ]

> +&gpio5 {
> +	pcal6524-reset-hog {
> +		gpio-hog;
> +		gpios = <5 0>;

[Severity: High]
Will this correctly release the I2C expanders from reset? The hog drives
GPIO5 bit 5, but the physical reset line appears to be tied to bit 10
according to the pinctrl muxing for IMX95_PAD_XSPI1_SS0_B__GPIO5_IO_BIT10
below. If this targets the wrong pin, all secondary peripherals routed
through the expanders could remain stuck in reset.

> +		output-high;
> +		line-name = "IO_XPNDR_RST_1V8";
> +	};

[ ... ]

> +	i2c2_gpio_expander_20: gpio@20 {	/* U12 */
> +		compatible = "nxp,pcal6524";
> +		reg = <0x20>;
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		#interrupt-cells = <2>;
> +		interrupt-controller;
> +		interrupts-extended = <&gpio5 11 IRQ_TYPE_LEVEL_HIGH>;

[Severity: Medium]
Is IRQ_TYPE_LEVEL_HIGH the correct polarity here? The PCAL6524 INT pin is
architecturally an open-drain, active-low output. Using a high-level
trigger might cause the expander interrupts to fail to trigger or result
in an interrupt storm.

> +		vcc-supply = <&reg_3p3v>;

[ ... ]

> +	i2c2_gpio_expander_21: gpio@21 {	/* U14 */
> +		compatible = "nxp,pcal6524";
> +		reg = <0x21>;
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		#interrupt-cells = <2>;
> +		interrupt-controller;
> +		interrupts-extended = <&gpio5 11 IRQ_TYPE_LEVEL_HIGH>;

[Severity: Medium]
Does this also need to be IRQ_TYPE_LEVEL_LOW for the active-low INT pin?

> +		vcc-supply = <&reg_3p3v>;

[ ... ]

> +	i2c2_gpio_expander_22: gpio@22 {	/* U13 */
> +		compatible = "nxp,pcal6524";
> +		reg = <0x22>;
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		#interrupt-cells = <2>;
> +		interrupt-controller;
> +		interrupts-extended = <&gpio5 11 IRQ_TYPE_LEVEL_HIGH>;

[Severity: Medium]
Does this also need to be IRQ_TYPE_LEVEL_LOW for the active-low INT pin?

> +		vcc-supply = <&reg_3p3v>;

[ ... ]

> +&netc_emdio {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_emdio>;
> +	status = "okay";
> +
> +	ethphy0: ethernet-phy@1 {	/* KSZ9131RNXI U20 */
> +		compatible = "ethernet-phy-id0022.1640";
> +		reg = <1>;
> +		interrupts-extended = <&gpio1 10 IRQ_TYPE_LEVEL_LOW>;

[Severity: Medium]
Is there a missing pinctrl configuration for this GPIO interrupt line?
gpio1 10 (IMX95_PAD_SYS_WAKEUP) doesn't seem to have a pinctrl entry
configuring it to operate as a GPIO, which might prevent Ethernet PHY
link state interrupts from being delivered.

> +		reset-assert-us = <1000>;
> +		/* RESET_N signal rise time ~100ms */
> +		reset-deassert-us = <120000>;
> +		reset-gpios = <&i2c2_gpio_expander_22 4 GPIO_ACTIVE_HIGH>;

[Severity: Medium]
Could this hold the Ethernet PHY in reset? The KSZ9131 RESET_N pin is
active-low. With GPIO_ACTIVE_HIGH, deasserting the reset will drive the
physical pin low, improperly holding the chip in reset. Should this be
GPIO_ACTIVE_LOW?

> +	};
> +
> +	ethphy1: ethernet-phy@2 {	/* KSZ9131RNXI U22 */
> +		compatible = "ethernet-phy-id0022.1640";
> +		reg = <2>;
> +		interrupts-extended = <&gpio1 10 IRQ_TYPE_LEVEL_LOW>;

[Severity: Medium]
Same question here regarding the missing pinctrl for gpio1 10.

> +		reset-assert-us = <1000>;
> +		/* RESET_N signal rise time ~100ms */
> +		reset-deassert-us = <120000>;
> +		reset-gpios = <&i2c2_gpio_expander_22 5 GPIO_ACTIVE_HIGH>;

[Severity: Medium]
Should this also be GPIO_ACTIVE_LOW for the active-low reset pin?

> +	};
> +};

[ ... ]

> +&usdhc3 {	/* M.2 Key-E */
> +	pinctrl-names = "default", "state_100mhz", "state_200mhz", "sleep";
> +	pinctrl-0 = <&pinctrl_usdhc3>;
> +	pinctrl-1 = <&pinctrl_usdhc3_100mhz>;
> +	pinctrl-2 = <&pinctrl_usdhc3_200mhz>;
> +	pinctrl-3 = <&pinctrl_usdhc3>;
> +	bus-width = <4>;

[Severity: Medium]
Is this M.2 Key-E slot missing a vmmc-supply? It appears VDD_3V3_M2-E_EN
(expander 20, pin 5) is meant to supply power, but this GPIO is unmodeled
and not attached here, which might leave the slot unpowered.

> +	status = "okay";
> +};
> +
> +&wdog3 {
> +	fsl,ext-reset-output;
> +	status = "okay";
> +};
> +
> +&scmi_iomuxc {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_hog>;
> +
> +	pinctrl_hog: hoggrp {
> +		fsl,pins = <
> +			/* PCIe 0 PEWAKE */
> +			IMX95_PAD_GPIO_IO34__GPIO5_IO_BIT14			0x4000031e
> +			/* PCIe 1 PEWAKE */
> +			IMX95_PAD_GPIO_IO37__GPIO5_IO_BIT17			0x4000031e
> +			/* BOOT_MODE2 */
> +			IMX95_PAD_SAI1_TXFS__AONMIX_TOP_GPIO1_IO_BIT11		0x4000031e
> +			/* IO_XPNDR_RST_1V8 shared by multiple expanders */
> +			IMX95_PAD_XSPI1_SS0_B__GPIO5_IO_BIT10			0x40000b9e

[Severity: High]
This pinctrl explicitly configures GPIO5 bit 10 for the expander reset
line, which exposes the mismatch with the pcal6524-reset-hog targeting
GPIO5 bit 5.

> +			/* IO_XPNDR_INT_1V8 shared by multiple expanders */
> +			IMX95_PAD_XSPI1_SS1_B__GPIO5_IO_BIT11			0x40000b9e
> +		>;
> +	};

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917010906.424049-1-marex@nabladev.com?part=3