Re: [PATCH v5 6/6] arm64: dts: imx8mm: imx8mp: Add FIO1-Audio DTOs for Data Modul i.MX8M Mini and Plus eDM SBC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Incorrect GPIO interrupt pin mapped for PCA9554 IO Expander on i.MX8MP
- [High] Broken RS485 UART configuration and pinmux RTS conflict on i.MX8MP
- [Medium] Device Tree node collision and duplicate unit address for SAI5 clock controller
- [Low] Missing PWM Beeper feature implementation contrary to commit message
--

--- Patch [6]: [PATCH v5 6/6] arm64: dts: imx8mm: imx8mp: Add FIO1-Audio DTOs for Data Modul i.MX8M Mini and Plus eDM SBC ---
commit 44a6d32ca342c6e20d4887379001b4de1904a3f8
Author: Marek Vasut <marex@nabladev.com>

arm64: dts: imx8mm: imx8mp: Add FIO1-Audio DTOs for Data Modul i.MX8M Mini and Plus eDM SBC

Add DT overlay for feature connector expansion module eDM-MOD-iMX8Mm-FIO1-Audio
providing additional UARTs, CAN, PWM Beeper, I2C, SPI, GPIO breakout and
SGTL5000 codec. This adapter can be optionally populated onto the eDM SBC.

> providing additional UARTs, CAN, PWM Beeper, I2C, SPI, GPIO breakout and
> SGTL5000 codec.

[Severity: Low]
The commit message states that the overlay provides a PWM Beeper, but I do not
see a pwm-beeper compatible node or PWM configuration added in these new files.

Is this feature missing from the patch, or should the commit message be
updated?

> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-edm-mod-imx8mm-fio1-audio.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-edm-mod-imx8mm-fio1-audio.dtsi
> new file mode 100644
> index 0000000000000..c622213c12747
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-edm-mod-imx8mm-fio1-audio.dtsi

[ ... ]

> +	gpio_feature: io-expander@20 {
> +		compatible = "nxp,pca9554";
> +		reg = <0x20>;
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		#interrupt-cells = <2>;
> +		interrupt-controller;
> +		interrupt-parent = <&gpio5>;
> +		interrupts = <4 IRQ_TYPE_LEVEL_LOW>;

[Severity: High]
This hardcodes the PCA9554 interrupt to &gpio5 4 for both platforms. While this
routes correctly on i.MX8MM, doesn't the i.MX8MP SOM route this external
connector pin to &gpio1 8?

Since the i.MX8MP dtso doesn't override these properties, will the IO expander
listen on the wrong pin and miss interrupts on the i.MX8MP?

[ ... ]

> +&uart2 {	/* RS422 J12 */
> +	linux,rs485-enabled-at-boot-time;
> +	uart-has-rtscts;
> +	status = "okay";
> +};

[Severity: High]
Applying the RS485 properties to &uart2 works for i.MX8MM, but doesn't i.MX8MP
use &uart1 for its RS485 port?

Could this leave &uart1 unconfigured on i.MX8MP while applying invalid
properties to &uart2?

> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-edm-mod-imx8mm-fio1-audio.dtso b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-edm-mod-imx8mm-fio1-audio.dtso
> new file mode 100644
> index 0000000000000..4a02bd026ac16
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-edm-mod-imx8mm-fio1-audio.dtso

[ ... ]

> +&spba2 {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	sai5clk: clock-controller@30050000 {	/* SAI5 */

[Severity: Medium]
Is it intentional to create a new clock-controller node here?

This unit address overlaps with the existing sai@30050000 node in the base
i.MX8MM dtsi. Because the node names differ, dtc will create two duplicate
nodes, which can cause an -EBUSY resource conflict during probe.

Should this modify the existing &sai5 node instead?

> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc-overlay-edm-mod-imx8mm-fio1-audio.dtso b/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc-overlay-edm-mod-imx8mm-fio1-audio.dtso
> new file mode 100644
> index 0000000000000..90c2dd1e67248
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc-overlay-edm-mod-imx8mm-fio1-audio.dtso

[ ... ]

> +	uart1-grp {
> +		fsl,pins = <
> +			MX8MP_IOMUXC_SD1_CLK__UART1_DCE_TX		0x49
> +			MX8MP_IOMUXC_SD1_CMD__UART1_DCE_RX		0x49
> +			MX8MP_IOMUXC_SD1_DATA1__UART1_DCE_CTS		0x49
> +		>;
> +	};

[Severity: High]
The base i.MX8MP board uses SAI2_RXD0 for the UART1_DCE_RTS pin. Since this
audio overlay reclaims SAI2_RXD0 for audio, it drops the RTS pin from this
uart1-grp.

Without the RTS pin to control transceiver direction, will the RS485 hardware
on i.MX8MP still be functional?

[ ... ]

> +&spba5 {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	sai5clk: clock-controller@30c50000 {

[Severity: Medium]
Similar to the i.MX8MM overlay, this creates a new node that overlaps with the
existing sai@30c50000 node in the i.MX8MP base tree.

Could this cause duplicate nodes to be generated and lead to resource conflicts?

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