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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] DT Node Collision and I2C Address Conflict for `io-expander@20` on i.MX8M Plus.
- [Medium] Unreliable CAN controller initialization due to missing/floating reset pin configuration.
--

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

Add DT overlay for feature connector expansion module eDM-MOD-iMX8Mm-FIO1
providing additional UARTs, CAN, PWM Beeper, I2C, SPI and GPIO breakout.
This adapter can be optionally populated onto the eDM SBC.

> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-edm-mod-imx8mm-fio1.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-edm-mod-imx8mm-fio1.dtsi
> new file mode 100644
> index 0000000000000..905db21d57c21
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-edm-mod-imx8mm-fio1.dtsi

[ ... ]

> +&ecspi2 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	status = "okay";
> +
> +	can_fio: can@0 {
> +		compatible = "microchip,mcp2515";
> +		reg = <0>;

[Severity: Medium]
The MCP2515 binding doesn't use reset-gpios. Does this require a gpio-hog to
take the device out of reset?

> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_can>;
> +		clocks = <&can_osc>;
> +		spi-max-frequency = <5000000>;
> +	};
> +};
> +
> +&i2c_feature {	/* Feature connector I2C */
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	gpio_feature: io-expander@20 {
> +		compatible = "nxp,pca9554";
> +		reg = <0x20>;

[Severity: High]
On the i.MX8M Plus base board, i2c_feature is an alias to i2c2, which already
has an LVDS GPIO expander at address 0x20 (gpiolvds: io-expander@20).

When this overlay is applied on the i.MX8M Plus baseboard, the DT compiler
will merge gpio_feature into gpiolvds. Will this silently overwrite critical
properties like gpio-line-names and interrupts, breaking LVDS panel power
control?

Is there a hardware I2C address collision between the LVDS expander and the
FIO1 expander on the Plus board?

> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_gpio_expander>;
> +		#gpio-cells = <2>;
> +		gpio-controller;

[ ... ]

> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-edm-mod-imx8mm-fio1.dtso b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-edm-mod-imx8mm-fio1.dtso
> new file mode 100644
> index 0000000000000..ad410db5f5b76
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-edm-mod-imx8mm-fio1.dtso

[ ... ]

> +&iomuxc {
> +	pinctrl_can: can-feature-grp {
> +		fsl,pins = <
> +			/* CAN_INT# */
> +			MX8MM_IOMUXC_SAI2_TXC_GPIO4_IO25		0x400000d6
> +			/* CAN_RST# */
> +			MX8MM_IOMUXC_SAI2_TXD0_GPIO4_IO26		0x6
> +		>;
> +	};

[Severity: Medium]
The CAN_RST# pin is muxed but lacks a gpio-hog to drive it as an output.

Will this leave the line floating or weakly pulled, leading to unreliable
CAN controller initialization?

> +
> +	pinctrl_gpio_expander: gpio-expander-feature-grp {
> +		fsl,pins = <
> +			/* GPIO4_IO27 */
> +			MX8MM_IOMUXC_SAI2_MCLK_GPIO4_IO27		0x6
> +		>;
> +	};

[ ... ]

> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc-overlay-edm-mod-imx8mm-fio1.dtso b/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc-overlay-edm-mod-imx8mm-fio1.dtso
> new file mode 100644
> index 0000000000000..0eccb7f7c0a8c
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc-overlay-edm-mod-imx8mm-fio1.dtso

[ ... ]

> +&iomuxc {
> +	pinctrl_can: can-feature-grp {
> +		fsl,pins = <
> +			/* CAN_INT# */
> +			MX8MP_IOMUXC_SD1_RESET_B__GPIO2_IO10		0x400000d6
> +		>;
> +	};

[Severity: Medium]
The CAN_RST# pin configuration appears to be completely missing here.

Is the reset pin state undefined on the IMX8MP base board when this overlay
is applied?

> +
> +	pinctrl_gpio_expander: gpio-expander-feature-grp {
> +		fsl,pins = <
> +			/* GPIO4_IO27 */
> +			MX8MP_IOMUXC_SAI2_MCLK__GPIO4_IO27		0x6
> +		>;
> +	};

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