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

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

On Sun, Jul 05, 2026 at 08:22:21PM +0000, sashiko-bot@kernel.org wrote:
> 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.

please check this

>
> [ ... ]
> > +&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?

Please check this ?

>
> [ ... ]
> > +&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?

Make sense, please check it.

Frank
>
> [ ... ]
> > 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