Re: [PATCH v8 7/7] 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 5 potential issue(s) to consider:
- [High] The `sai5clk` clock provider uses the `bus` clock instead of the intended 24MHz root clock to generate MCLK, which will break the SGTL5000 audio codec.
- [High] The shared `.dtsi` file applies i.MX8MM-specific UART overrides to the i.MX8MP baseboard, breaking onboard Bluetooth.
- [Medium] The overlays create a new `clock-controller` node at the same unit address as the existing `sai` node, violating Device Tree uniqueness rules.
- [Low] The commit message inaccurately claims the overlay provides a PWM Beeper, but the beeper node and PWM configuration are missing.
- [Low] Dead pinctrl code and inaccurate commit message regarding 'additional UARTs'.
--

--- Patch [7]: [PATCH v8 7/7] arm64: dts: imx8mm: imx8mp: Add FIO1-Audio DTOs for Data Modul i.MX8M Mini and Plus eDM SBC ---
commit ffa908f72aa972e17c5f27427f835a5125cba175
Author: Marek Vasut <marex@nabladev.com>
Subject: 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. This adapter can be optionally populated onto the eDM SBC.

[Severity: Low]
Does this overlay actually provide a PWM Beeper and multiple additional UARTs?

Looking at the implementation below, uart1 is never enabled, leaving only
uart2 as the single additional UART. Additionally, the pin typically used
for the PWM Beeper is explicitly repurposed as a GPIO hog, and no PWM nodes
are instantiated in the audio 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
> new file mode 100644
> index 0000000000000..8b38b5c228e0a
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-edm-mod-imx8mm-fio1-audio.dtsi

[ ... ]

> +&uart2 {	/* RS422 J12 */
> +	linux,rs485-enabled-at-boot-time;
> +	uart-has-rtscts;
> +	status = "okay";
> +};
> +
> +/* UART4 is blocked by RDC and used as CM4 console UART */
> +&uart4 {	/* UART to 1-Wire J5 */
> +	status = "disabled";
> +};

[Severity: High]
Will this unconditionally disable onboard Bluetooth when this overlay is
applied to the i.MX8MP baseboard?

Since this shared dtsi file is also included by the i.MX8MP overlay, disabling
uart4 here conflicts with the i.MX8MP baseboard, which actively uses uart4 for
the onboard Infineon CYW55572 Bluetooth chip.

> 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..88fa705ac2b67
> --- /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]
Does this create an overlapping duplicate node at the 30050000 unit address?

The base i.MX8MM devicetree already defines sai5: sai@30050000. Declaring
a new clock-controller@30050000 node under spba2 instead of extending the
existing node with &sai5 violates the Devicetree specification requiring
unique unit addresses for sibling nodes.

> +		compatible = "fsl,imx8mm-sai-clock", "fsl,imx8mq-sai-clock";
> +		reg = <0x30050000 0x10000>;
> +		#clock-cells = <1>;
> +		clocks = <&clk IMX8MM_CLK_SAI5_IPG>,
> +			 <&clk IMX8MM_CLK_SAI5_ROOT>;

[Severity: High]
Will this derive the MCLK from the bus clock instead of the intended 24MHz
root clock?

The clock driver at drivers/clk/clk-fsl-sai.c hardcodes index 0 as the parent
for generated clocks. Because the bus clock (IPG) is listed at index 0 here,
the Common Clock Framework will use it as the parent. Since the bus clock
typically runs around 133MHz, the SGTL5000 codec will receive an out-of-spec
MCLK (it strictly requires 8-27 MHz) and audio playback will fail to initialize.

> +		clock-names = "bus", "mclk1";

[ ... ]

> 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..4e2112355350f
> --- /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: Low]
Is this pinctrl group dead code?

The uart1-grp is defined here, but the uart1 node is never enabled or
referenced in this overlay or the shared dtsi.

> +&sai2 {
> +	assigned-clocks = <&clk IMX8MP_CLK_SAI2>;
> +	assigned-clock-parents = <&clk IMX8MP_AUDIO_PLL2_OUT>;
> +};
> +
> +&spba5 {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	sai5clk: clock-controller@30c50000 {

[Severity: Medium]
Does this create a duplicate node at the 30c50000 unit address?

Similar to the i.MX8MM overlay, the i.MX8MP base devicetree already defines
sai5: sai@30c50000. Extending it via &sai5 would avoid overlapping unit
addresses.

> +		compatible = "fsl,imx8mp-sai-clock", "fsl,imx8mq-sai-clock";
> +		reg = <0x30c50000 0x10000>;
> +		#clock-cells = <1>;
> +		clocks = <&audio_blk_ctrl IMX8MP_CLK_AUDIOMIX_SAI5_IPG>,
> +			 <&audio_blk_ctrl IMX8MP_CLK_AUDIOMIX_SAI5_MCLK1>;

[Severity: High]
Does listing the IPG clock at index 0 cause the MCLK to be generated from the
bus clock instead of the MCLK1 root clock, as mentioned for the i.MX8MM overlay?

> +		clock-names = "bus", "mclk1";

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