Re: [PATCH v4 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 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>;
> +		/* 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.

[ ... ]

> +	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.

[ ... ]

> +	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?

[ ... ]

> +&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.

[ ... ]

> +&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