Re: [PATCH 2/2] arm64: dts: imx8mp: add Olimex i.MX8MP-SOM-EVB kit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] dt: PMIC interrupt misconfigured as edge-rising instead of level-low
- [Medium] dt: EVB carrier features incorrectly enabled in SOM dtsi
- [Medium] dt: unused CAN standby pinctrl groups leave transceivers disabled
- [Medium] dt: missing pinctrl-names in PMIC node prevents pinmux application
- [Low] dt: duplicate pinctrl_usb1_vbus label causes build error
- [Low] dt: rohm,dh2228fv compatible string abused for generic SPI header
- [Low] dt: deprecated reset-gpio property used in PCIe node
--

commit b3a5c80438428e973d66a598e54426ad5ece4f26
Author: Zoltan HERPAI <wigyori@uid0.hu>

arm64: dts: imx8mp: add Olimex i.MX8MP-SOM-EVB kit

This adds support for an i.MX8-based system-on-module + EVB board. The
product pages are here [1] and here [2] respectively.
There are two new files, one dts for the EVB and one dtsi for the SoM.

> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-olimex-som-evb.dts b/arch/arm64/boot/dts/freescale/imx8mp-olimex-som-evb.dts
> new file mode 100644
> index 0000000000000..26fee72d50c90
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mp-olimex-som-evb.dts
[ ... ]
> +&ecspi2 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	cs-gpios = <&gpio5 13 GPIO_ACTIVE_LOW>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_ecspi2 &pinctrl_ecspi2_cs>;
> +
> +	status = "okay";
> +
> +	spidev1: spi@0 {
> +		reg = <0>;
> +		compatible = "rohm,dh2228fv";
> +		spi-max-frequency = <500000>;
> +	};
> +};

[Severity: Low]
Is it appropriate to use the rohm,dh2228fv compatible string here for a
generic SPI header? This appears to be forcing the spidev driver to bind
and silences DTC warnings, which goes against the device tree compatible
string mechanism.

[ ... ]
> +&flexcan1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_flexcan1>;
> +
> +	status = "okay";
> +};

[Severity: Medium]
Will this configuration leave the CAN1 transceiver disabled? The flexcan1
node fails to reference the pinctrl_flexcan1_reg standby pins or a regulator
for them.

[ ... ]
> +	pinctrl_flexcan1_reg: flexcan1reggrp {
> +		fsl,pins = <
> +			MX8MP_IOMUXC_SPDIF_EXT_CLK__GPIO5_IO05	0x154	/* CAN1_STBY */
> +		>;
> +	};

Because these standby pins are configured but never used, the physical CAN
transceivers will likely remain disabled.

[ ... ]
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-olimex-som.dtsi b/arch/arm64/boot/dts/freescale/imx8mp-olimex-som.dtsi
> new file mode 100644
> index 0000000000000..9876d3e7b81a3
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mp-olimex-som.dtsi
[ ... ]
> +&i2c1 {
> +	clock-frequency = <400000>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_i2c1>;
> +
> +	status = "okay";
> +
> +	pmic: pmic@25 {
> +		reg = <0x25>;
> +		compatible = "nxp,pca9450c";
> +		/* PMIC PCA9450 PMIC_nINT GPIO1_IO3 */
> +		pinctrl-0 = <&pinctrl_pmic>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <3 GPIO_ACTIVE_LOW>;

[Severity: Medium]
Does the pmic@25 node need a pinctrl-names = "default"; property here? Without
it, the kernel's pinctrl core will not automatically apply the pinctrl-0 state
during probe, potentially leaving the interrupt pin floating.

[Severity: High]
Does this interrupt configuration break the delivery of PMIC events?
The GPIO_ACTIVE_LOW flag resolves to 1, which corresponds to
IRQ_TYPE_EDGE_RISING. Since the PCA9450's INTb line is active low, the host
expecting a rising edge will miss the interrupts. Should this be
IRQ_TYPE_LEVEL_LOW instead?

[ ... ]
> +&pcie {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_pcie0>;
> +	reset-gpio = <&gpio2 7 GPIO_ACTIVE_LOW>;
> +	vpcie-supply = <&reg_pcie0>;
> +
> +	status = "okay";
> +};

[Severity: Medium]
Is it correct to enable EVB carrier features like PCIe inside the shared
SOM .dtsi file? Hardcoding baseboard-specific peripheral enablement here
causes pin conflicts and prevents this .dtsi from being reused for other
carrier board designs.

[Severity: Low]
Does this node use a deprecated property? The reset-gpio property is
deprecated by standard PCI bindings which require the generic reset-gpios
plural format.

[ ... ]
> +&usdhc2 {
> +	assigned-clocks = <&clk IMX8MP_CLK_USDHC2>;
> +	assigned-clock-rates = <400000000>;
> +	bus-width = <4>;
> +	cd-gpios = <&gpio2 12 GPIO_ACTIVE_LOW>;
> +	pinctrl-names = "default", "state_100mhz", "state_200mhz";
> +	pinctrl-0 = <&pinctrl_usdhc2>, <&pinctrl_usdhc2_gpio>;
> +	pinctrl-1 = <&pinctrl_usdhc2_100mhz>, <&pinctrl_usdhc2_gpio>;
> +	pinctrl-2 = <&pinctrl_usdhc2_200mhz>, <&pinctrl_usdhc2_gpio>;
> +	vmmc-supply = <&reg_usdhc2_vmmc>;
> +
> +	status = "okay";
> +};

[Severity: Medium]
Should the usdhc2 (microSD slot) be enabled in the SOM .dtsi? Similar to
PCIe, enabling baseboard-specific features in the shared SoM file breaks
device tree abstraction and forces other carrier boards to inherit this
configuration.

[ ... ]
> +	pinctrl_usb1_vbus: usb1grp {
> +		fsl,pins = <
> +			MX8MP_IOMUXC_GPIO1_IO14__GPIO1_IO14		0x19
> +		>;
> +	};
> +
> +	pinctrl_usdhc2: usdhc2grp {
[ ... ]
> +	pinctrl_usdhc3_200mhz: usdhc3-200mhzgrp {
> +		fsl,pins = <
> +			MX8MP_IOMUXC_NAND_WE_B__USDHC3_CLK	0x196
> +			MX8MP_IOMUXC_NAND_WP_B__USDHC3_CMD	0x1d6
> +			MX8MP_IOMUXC_NAND_DATA04__USDHC3_DATA0	0x1d6
> +			MX8MP_IOMUXC_NAND_DATA05__USDHC3_DATA1	0x1d6
> +			MX8MP_IOMUXC_NAND_DATA06__USDHC3_DATA2	0x1d6
> +			MX8MP_IOMUXC_NAND_DATA07__USDHC3_DATA3	0x1d6
> +			MX8MP_IOMUXC_NAND_RE_B__USDHC3_DATA4	0x1d6
> +			MX8MP_IOMUXC_NAND_CE2_B__USDHC3_DATA5	0x1d6
> +			MX8MP_IOMUXC_NAND_CE3_B__USDHC3_DATA6	0x1d6
> +			MX8MP_IOMUXC_NAND_CLE__USDHC3_DATA7	0x1d6
> +			MX8MP_IOMUXC_NAND_CE1_B__USDHC3_STROBE	0x196
> +		>;
> +	};
> +
> +	pinctrl_usb1_vbus: usb1grp {
> +		fsl,pins = <
> +			MX8MP_IOMUXC_GPIO1_IO14__GPIO1_IO14	0x19
> +		>;
> +	};

[Severity: Low]
Does this code duplicate the pinctrl_usb1_vbus label? The Device Tree
Compiler (DTC) prohibits identically named nodes and duplicate labels at the
same scope level, and this will cause a build failure.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918195559.130443-1-wigyori@uid0.hu?part=2