Re: [PATCH v3 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 3 potential issue(s) to consider:
- [High] net: incorrect Ethernet PHY reset polarity permanently holds PHYs in reset
- [High] gpio: PCAL6524 interrupt polarity mismatch causes interrupt storm
- [Medium] regulator: reg_panel_vcc missing enable-active-high property inverts enable logic
--

commit 89d12be71b001da7aa525de48a7d723a37bc3741
Author: Marek Vasut <marex@nabladev.com>

arm64: dts: imx95: Add support for Data Modul i.MX95 eDM SBC

Add support for Data Modul i.MX95 eDM SBC board. This is an
evaluation board for various custom display units. Currently
supported are serial console, 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
> --- /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";
> +		gpio = <&i2c2_gpio_expander_20 17 0>;
> +		/* Disabled by default, unless display board plugged in. */
> +		status = "disabled";
> +	};

[Severity: Medium]
Will this configuration cause the generic GPIO framework to force an
active-low polarity on the V_LCD_EN signal?

The gpio property omits enable-active-high. The gpiolib framework
(of_gpio_quirk_polarity in drivers/gpio/gpiolib-of.c) enforces an active-low
polarity for regulator-fixed if enable-active-high isn't explicitly set.
This seems like it could drive the panel VCC regulator low when it should be
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 configuring the shared PCAL6524 interrupt line with IRQ_TYPE_LEVEL_HIGH
result in an interrupt storm?

The PCAL6524 features an open-drain, active-low interrupt output pin that
rests at a high logic level. Since the interrupt controller is configured to
trigger on the high level, won't it continuously trigger on the idle state?

[ ... ]

> +	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]
Does this expander have the same active-low interrupt output pin behavior,
and should this also avoid IRQ_TYPE_LEVEL_HIGH?

[ ... ]

> +	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 this expander also need to be updated to match the active-low interrupt
output?

[ ... ]

> +&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>;
> +		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: High]
Is GPIO_ACTIVE_HIGH the correct polarity for the KSZ9131RNXI PHY reset?

The PHY uses an active-low physical reset pin. Because the framework drives
the active-low pin low during deassertion, configuring this as
GPIO_ACTIVE_HIGH appears to permanently trap the ethphy0 interface in 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: High]
Should this reset-gpios polarity also be updated to active-low to properly
deassert reset for the ethphy1 interface?

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