All of lore.kernel.org
 help / color / mirror / Atom feed
From: Frank Li <Frank.li@oss.nxp.com>
To: sashiko-reviews@lists.linux.dev
Cc: Marek Vasut <marex@nabladev.com>,
	Frank.Li@kernel.org, imx@lists.linux.dev, robh@kernel.org,
	conor+dt@kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH v7 7/7] arm64: dts: imx8mm: imx8mp: Add FIO1-Audio DTOs for Data Modul i.MX8M Mini and Plus eDM SBC
Date: Thu, 30 Jul 2026 10:17:13 -0500	[thread overview]
Message-ID: <amtq-Ukw_dyvavxJ@SMW015318> (raw)
In-Reply-To: <20260729221033.3CDB41F000E9@smtp.kernel.org>

On Wed, Jul 29, 2026 at 10:10:32PM +0000, sashiko-bot@kernel.org wrote:
> 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 <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.
>
> > 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?

Yes, I check flexcan1 use this pad at i.MX8MP. you need disable flexcan1
at i.mx8mp's dtso file.

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

at imx8mm.dts, 30050000 is SAI5, here you change to to use clock-controller

Suppose address here is wrong.

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
> > 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/20260729214723.557461-1-marex@nabladev.com?part=7

  reply	other threads:[~2026-07-30 15:17 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-29 21:47 [PATCH v7 1/7] arm64: dts: imx8mp: Add DT label to SPBA5 bus Marek Vasut
2026-07-29 21:47 ` [PATCH v7 2/7] arm64: dts: imx8mm: imx8mp: Add LVDS DTOs for Data Modul i.MX8M Mini and Plus eDM SBC Marek Vasut
2026-07-29 22:06   ` sashiko-bot
2026-07-30 15:31     ` Frank Li
2026-07-30 19:14       ` Marek Vasut
2026-07-29 21:47 ` [PATCH v7 3/7] arm64: dts: imx8mm: imx8mp: Add legacy board " Marek Vasut
2026-07-29 21:47 ` [PATCH v7 4/7] arm64: dts: imx8mm: imx8mp: Add HDMI " Marek Vasut
2026-07-29 21:47 ` [PATCH v7 5/7] arm64: dts: imx8mm: imx8mp: Add CM " Marek Vasut
2026-07-29 21:47 ` [PATCH v7 6/7] arm64: dts: imx8mm: imx8mp: Add FIO1 " Marek Vasut
2026-07-29 21:47 ` [PATCH v7 7/7] arm64: dts: imx8mm: imx8mp: Add FIO1-Audio " Marek Vasut
2026-07-29 22:10   ` sashiko-bot
2026-07-30 15:17     ` Frank Li [this message]
2026-07-30 19:34       ` Marek Vasut
2026-07-31 15:23         ` Frank Li
2026-07-31 15:56           ` Marek Vasut

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=amtq-Ukw_dyvavxJ@SMW015318 \
    --to=frank.li@oss.nxp.com \
    --cc=Frank.Li@kernel.org \
    --cc=conor+dt@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=imx@lists.linux.dev \
    --cc=marex@nabladev.com \
    --cc=robh@kernel.org \
    --cc=sashiko-reviews@lists.linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.