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

[Frank Li <Frank.li@oss.nxp.com>]

On Thu, Sep 17, 2026 at 06:35:12AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> - [High] dts: pcal6524 interrupt polarity is incorrectly set to LEVEL_HIGH
> - [Medium] dts: Ethernet PHY reset-gpios configured with wrong polarity
> - [Medium] dts: eMMC reset GPIO defined but unused in usdhc1
> - [Medium] dts: reg_panel_vcc regulator missing enable-active-high property
> - [Medium] dts: M.2 Key-E (usdhc3) slot missing power regulator
> - [Low] dts: tas2552 codec uses deprecated enable-gpio instead of enable-gpios
> --
>
> commit 5792bafb068af6288c67560ae9fb90a4c34326af
> Author: Marek Vasut <marex@nabladev.com>
> arm64: dts: imx95: Add support for Data Modul i.MX95 eDM SBC
>
> This commit adds support for the Data Modul i.MX95 eDM SBC evaluation board,
> enabling various peripherals like serial console, ethernet, eMMC, SD,
> SPI NOR, USB, 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..0181262eb8a98
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/freescale/imx95-data-modul-edm-sbc.dts
>
> [ ... ]
>
> > +	reg_panel_vcc: regulator-panel-vcc {
> > +		compatible = "regulator-fixed";
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +		regulator-name = "PANEL_VCC";
> > +		gpios = <&i2c2_gpio_expander_20 17 0>;

gpio third arg 0, means GPIO_ACTIVE_HIGH

	so need enable-active-high

if your boards have invertor should use GPIO_ACTIVE_LOW


> > +		/* Disabled by default, unless display board plugged in. */
> > +		status = "disabled";
> > +	};
>
> [Severity: Medium]
> Does reg_panel_vcc need an enable-active-high property? Without it, the
> regulator defaults to active-low logic, which will drive the panel VCC low
> when the regulator is enabled.
>
> [ ... ]
>
> > +	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: High]
> Will triggering interrupts-extended on IRQ_TYPE_LEVEL_HIGH cause an interrupt
> storm? The PCAL6524 INT output is an open-drain, active-low signal, so the
> line will idle high.

Yes, suppose it should LOW to trigger irq.


>
> [ ... ]
>
> > +	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: High]
> Could this IRQ_TYPE_LEVEL_HIGH configuration also lead to an interrupt
> storm for i2c2_gpio_expander_21?
>
> [ ... ]
>
> > +	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: High]
> Does i2c2_gpio_expander_22 similarly need to be configured for an active-low
> interrupt instead of IRQ_TYPE_LEVEL_HIGH?
>
> [ ... ]
>
> > +	tas2552: audio-codec@40 {
> > +		compatible = "ti,tas2552";
> > +		reg = <0x40>;
> > +		#sound-dai-cells = <0>;
> > +		avdd-supply = <&reg_1p8v>;
> > +		iovdd-supply = <&reg_3p3v>;
> > +		vbat-supply = <&reg_5p0v>;
> > +		enable-gpio = <&i2c2_gpio_expander_20 23 GPIO_ACTIVE_HIGH>;
>
> [Severity: Low]
> This isn't a bug, but should tas2552 be using the enable-gpios property name
> instead of the deprecated enable-gpio? The patch changelog mentions replacing
> gpio with gpios DT properties.

Please fix this one.

>
> [ ... ]
>
> > +	ethphy0: ethernet-phy@1 {	/* KSZ9131RNXI U20 */
> > +		compatible = "ethernet-phy-id0022.1640";
> > +		reg = <1>;
> > +		interrupts-extended = <&gpio1 10 IRQ_TYPE_LEVEL_LOW>;
> > +		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]
> Will ethphy0 be held in reset permanently? The KSZ9131RNXI RESET_N signal is
> active-low, so defining reset-gpios with GPIO_ACTIVE_HIGH will drive the line
> high to assert reset.
>
> > +	};
> > +
> > +	ethphy1: ethernet-phy@2 {	/* KSZ9131RNXI U22 */
> > +		compatible = "ethernet-phy-id0022.1640";
> > +		reg = <2>;
> > +		interrupts-extended = <&gpio1 10 IRQ_TYPE_LEVEL_LOW>;
> > +		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]
> Does ethphy1 also suffer from the incorrect GPIO_ACTIVE_HIGH reset polarity?

Is it correct?

>
> [ ... ]
>
> > +&usdhc1 {	/* eMMC */
> > +	pinctrl-names = "default", "state_100mhz", "state_200mhz", "sleep";
> > +	pinctrl-0 = <&pinctrl_usdhc1>;
>
> [Severity: Medium]
> Could usdhc1 be missing its reset sequence configuration? The hardware
> provides an eMMC_RESET line on the I2C GPIO expander, but it isn't consumed
> via mmc-pwrseq or reset-gpios. This might cause the eMMC to fail to initialize
> during warm reboots.

Please check this

Frank

>
> [ ... ]
>
> > +&usdhc3 {	/* M.2 Key-E */
> > +	pinctrl-names = "default", "state_100mhz", "state_200mhz", "sleep";
> > +	pinctrl-0 = <&pinctrl_usdhc3>;
>
> [Severity: Medium]
> Does the usdhc3 node need a vmmc-supply? The VDD_3V3_M2-E_EN pin is defined on
> the expander but there is no fixed regulator defined to power the M.2 Key-E
> slot, which might leave peripherals undetectable.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260917062543.534416-1-marex@nabladev.com?part=3