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

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Hardware Interrupt Routing Mismatch causes PCA9554 GPIO expander to conflict with CAN1 on i.MX8MP.
- [High] The `fsl,sai-asynchronous` property breaks audio capture (and playback) on both i.MX8MM and i.MX8MP due to missing clock routing.
- [High] The i.MX8MP overlay fails to configure the clock parent and rate for `sai5clk`, resulting in an invalid MCLK for the SGTL5000 codec.
- [High] Duplicate `unique_unit_address` causes devres memory region conflict between `sai5clk` and `sai5` drivers.
- [High] The shared `.dtsi` unconditionally disables `uart4`, breaking onboard Bluetooth on the i.MX8MP SBC.
- [Low] Commit message inaccurately claims the overlay provides a "PWM Beeper" and "additional UARTs".
--

--- Patch [7]: [PATCH v7 7/7] arm64: dts: imx8mm: imx8mp: Add FIO1-Audio DTOs for Data Modul i.MX8M Mini and Plus eDM SBC ---
commit 43246870359af0cd7d3d40f8db5eaaff8fad43a1
Author: Marek Vasut <[email protected]>

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.

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

[Severity: Low]
Is this description accurate? The commit message mentions a PWM Beeper and
additional UARTs, but the code below disables uart4 and repurposes the
SPDIF_TX pin used for the beeper as a GPIO instead.

[ ... ]

> 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..01cf5c89dbff4
> --- /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]
Does this interrupt routing conflict on the i.MX8MP baseboard?

The shared overlay hardcodes the PCA9554 interrupt to GPIO5_IO04, which is
correct for i.MX8MM. However, on i.MX8MP, this pad (SPDIF_RX) is physically
routed and configured for CAN1_RX.

Could this cause the GPIO expander interrupts to fail and CAN traffic to
trigger spurious GPIO interrupts when loaded on the i.MX8MP? Would it be
better to override this interrupt in the i.MX8MP overlay?

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

[Severity: High]
Could this fsl,sai-asynchronous property break audio capture?

It forces the SAI block to use independent clocks for TX and RX. However, the
pinmux routing on i.MX8MM only provides RX_BCLK and TX_SYNC, meaning RX
lacks a SYNC clock. On i.MX8MP, only TX clocks are routed, leaving RX without
both BCLK and SYNC clocks.

[ ... ]
> +/* UART4 is blocked by RDC and used as CM4 console UART */
> +&uart4 {	/* UART to 1-Wire J5 */
> +	status = "disabled";
> +};

[Severity: High]
Will disabling uart4 here break the onboard Bluetooth on the i.MX8MP SBC?

This file is shared between the i.MX8MM and i.MX8MP overlays, but on the
i.MX8MP SBC, uart4 is actively used for the infineon,cyw55572-bt module.

[ ... ]
> 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
[ ... ]
> +	sai5clk: clock-controller@30050000 {	/* SAI5 */
> +		compatible = "fsl,imx8mm-sai-clock", "fsl,imx8mq-sai-clock";
> +		reg = <0x30050000 0x10000>;

[Severity: High]
Can this cause a devres memory region conflict during driver probe?

This overlay adds a new clock-controller node with the exact same unit address
and register range as the existing sai@30050000 audio node.

Since both drivers will use devm_platform_ioremap_resource() to request the
region exclusively, whichever probes second might receive -EBUSY and fail.

[ ... ]
> 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
[ ... ]
> +	sai5clk: clock-controller@30c50000 {
> +		compatible = "fsl,imx8mp-sai-clock", "fsl,imx8mq-sai-clock";
> +		reg = <0x30c50000 0x10000>;

[Severity: High]
Similar to the i.MX8MM overlay, can this cause a memory region conflict with
the existing sai@30c50000 audio node?

[ ... ]
> +		#clock-cells = <1>;
> +		clocks = <&audio_blk_ctrl IMX8MP_CLK_AUDIOMIX_SAI5_IPG>,
> +			 <&audio_blk_ctrl IMX8MP_CLK_AUDIOMIX_SAI5_MCLK1>;
> +		clock-names = "bus", "mclk1";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_codec_mclk>;
> +		status = "okay";
> +	};
> +};

[Severity: High]
Does this node also need assigned-clocks and assigned-clock-rates configured?

Unlike the i.MX8MM variant, this node leaves the MCLK unconfigured. The
SGTL5000 driver and simple-audio-card do not dynamically set this clock rate
via clk_set_rate() unless mclk-fs is defined.

Could this leave the clock at 0 Hz and prevent the codec from functioning on
the i.MX8MP?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7