Re: [PATCH v4 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] The SAI2 interface is incorrectly configured with `fsl,sai-asynchronous` and an invalid `fsl,sai-bit-clock-swap` property, leading to audio capture and playback failure.
- [Medium] The overlays instantiate new `clock-controller` nodes at the exact same unit addresses as the existing `sai5` nodes, causing DTC warnings and resource conflicts.
- [Medium] Missing interrupt override for the PCA9554 GPIO expander in the i.MX8MP overlay causes the device to listen on a dead interrupt line.
- [Medium] The commit message claims features that are not implemented in the patch.
--

commit 1e6ee57031100e82b436dac299b0eed194860d5b
Author: Marek Vasut <marex@nabladev.com>

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

This commit adds device tree overlays for the eDM-MOD-iMX8Mm-FIO1-Audio
feature connector expansion module. It provides support for the SGTL5000
codec, CAN, I2C, SPI, and other peripherals on the i.MX8M Mini and Plus
boards.

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

[Severity: Medium]
Is the commit message accurate regarding the features provided? 

The FIO1-Audio overlay only seems to enable a single UART (uart2, while uart4
is disabled) and doesn't contain a pwm-beeper node or PWM pin configuration.

Could the commit message be updated to reflect only the features actually 
implemented in this variant?

> 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
> --- /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: Medium]
Since this file is shared between i.MX8MM and i.MX8MP overlays, does the
interrupt configuration need to be overridden for i.MX8MP?

On the i.MX8MP baseboard, this feature connector signal seems to be physically
routed to GPIO1_IO08 instead of GPIO5_IO04. The i.MX8MP overlay doesn't
currently override these properties for gpio_feature, which might cause the
expander to listen on a dead interrupt line.

[ ... ]
> +&sai2 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_sai2>;
> +	assigned-clock-rates = <24576000>;
> +	fsl,sai-asynchronous;

[Severity: High]
Does disabling internal synchronization between the TX and RX halves of the
SAI block break audio?

The SGTL5000 codec only has a single set of I2S clocks, and the pinmux
settings only route one clock pair. This leaves the other half of the SAI
physically disconnected from any clock source.

[ ... ]
> 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
[ ... ]
> +&sai2 {
> +	assigned-clocks = <&clk IMX8MM_CLK_SAI2>;
> +	assigned-clock-parents = <&clk IMX8MM_AUDIO_PLL1_OUT>;
> +	fsl,sai-bit-clock-swap;

[Severity: High]
Is fsl,sai-bit-clock-swap intended to fix the asynchronous clocking issue?

This property is a downstream vendor leftover that does not exist in
mainline device tree bindings and is ignored by the upstream fsl_sai.c
driver. Should fsl,sai-asynchronous be removed from the shared .dtsi
instead so the SAI can operate in synchronous mode?

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

[Severity: Medium]
Does declaring a new node here cause a duplicate unit-address warning and
potential resource conflicts?

The base SoC .dtsi files already define sai5: sai@30050000 at this exact
address. Because the node names differ (clock-controller vs sai), DTC will
create two distinct sibling nodes rather than merging them.

If the baseboard enables the existing &sai5 node, both drivers could attempt
to map the identical physical MMIO region using devm_platform_ioremap_resource,
causing a -EBUSY error. Should this overlay reference the existing &sai5 node
by label 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
[ ... ]
> +&spba5 {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	sai5clk: clock-controller@30c50000 {

[Severity: Medium]
Similarly, does this cause a duplicate unit-address warning on i.MX8MP? 

The base SoC defines sai5: sai@30c50000. It seems the overlay should
reference the existing &sai5 node by label here as well.

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