Re: [PATCH v3 2/2] arm64: dts: freescale: add i.MX95 19x19 FRDM PRO board dts

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The `silent-gpios` for `flexcan1_phy` and `flexcan3_phy` (which use the `nxp,tja1057` CAN transceiver) are defined with the `GPIO_ACTIVE_LOW` polarity. This is incorrect for the TJA1057.
- [High] The pad configuration for the `pcal6416` I2C GPIO expander's interrupt pin (`IMX95_PAD_GPIO_IO34__GPIO5_IO_BIT14`) in `pinctrl_pcal6416` is incorrectly set to `0x31` instead of the expected `0x31e`.
- [Low] The `pcie0` and `pcie1` nodes use the deprecated `reset-gpio` property instead of the standard `reset-gpios`.
- [Low] Device tree node names contain underscores, which violates DT naming specifications.
--

commit e8bda174f6b9f1f5508fbd640dcc59ff092dd5cd
Author: Joseph Guo <qijian.guo@nxp.com>

arm64: dts: freescale: add i.MX95 19x19 FRDM PRO board dts

NXP i.MX95 19x19 FRDM PRO is cost-effective with extensive expansion
capabilities based on the i.MX95 19x19 SoC. Add device tree for this board.

> diff --git a/arch/arm64/boot/dts/freescale/imx95-19x19-frdm-pro.dts b/arch/arm64/boot/dts/freescale/imx95-19x19-frdm-pro.dts
> new file mode 100644
> index 0000000000000..ce638ed8dc0f9
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx95-19x19-frdm-pro.dts
> @@ -0,0 +1,1021 @@

[ ... ]

> +	aliases {
> +		ethernet0 = &enetc_port0;
> +		ethernet1 = &enetc_port1;

[ ... ]

> +		serial0 = &lpuart1;
> +		serial4 = &lpuart5;
> +	};
> +
> +	bt_sco_codec: bt_sco_codec {
> +		compatible = "linux,bt-sco";
> +		#sound-dai-cells = <1>;
> +	};

This isn't a bug, but should this node name use hyphens instead of
underscores to comply with Devicetree naming specifications?

> +
> +	flexcan1_phy: can-phy0 {
> +		compatible = "nxp,tja1057";
> +		#phy-cells = <0>;
> +		max-bitrate = <5000000>;
> +		silent-gpios = <&i2c4_gpio_expander_22 11 GPIO_ACTIVE_LOW>;
> +	};
> +
> +	flexcan3_phy: can-phy2 {
> +		compatible = "nxp,tja1057";
> +		#phy-cells = <0>;
> +		max-bitrate = <5000000>;
> +		silent-gpios = <&i2c4_gpio_expander_22 13 GPIO_ACTIVE_LOW>;
> +	};

Does this incorrectly configure the TJA1057 transceiver?

The hardware's silent pin is active-high, so when the driver deasserts the
silent GPIO to enable transmission (logical 0), setting the polarity to
active-low will drive a physical high level.

This would force the transceiver into silent mode and prevent any CAN
transmissions.

Should the polarity be set to active-high instead?

[ ... ]

> +	sound-mqs {
> +		compatible = "audio-graph-card2";
> +		links = <&sai1_port1>;
> +		label = "mqs-audio";
> +	};
> +
> +	usdhc3_pwrseq: usdhc3_pwrseq {
> +		compatible = "mmc-pwrseq-simple";
> +		reset-gpios = <&i2c4_gpio_expander_22 9 GPIO_ACTIVE_LOW>;
> +	};

This isn't a bug, but should this node name also use hyphens instead of
underscores?

[ ... ]

> +&netcmix_blk_ctrl {
> +	status = "okay";
> +};
> +
> +&pcie0 {
> +	pinctrl-0 = <&pinctrl_pcie0>;
> +	pinctrl-names = "default";
> +	reset-gpio = <&i2c4_gpio_expander_22 19 GPIO_ACTIVE_LOW>;
> +	vpcie-supply = <&reg_m2_mkey_1_pwr>;
> +	status = "okay";
> +};
> +
> +&pcie1 {
> +	pinctrl-0 = <&pinctrl_pcie1>;
> +	pinctrl-names = "default";
> +	reset-gpio = <&i2c3_gpio_expander_20 9 GPIO_ACTIVE_LOW>;
> +	vpcie-supply = <&reg_m2_mkey_2_pwr>;
> +	status = "okay";
> +};

This isn't a bug, but should these properties use the standard reset-gpios
name instead of the deprecated reset-gpio?

Using the singular form violates strict DT schema definitions and might
trigger validation warnings.

[ ... ]

> +	pinctrl_mqs1: mqs1grp {
> +		fsl,pins = <
> +			IMX95_PAD_SAI1_TXFS__AONMIX_TOP_MQS1_LEFT		0x31e
> +			IMX95_PAD_SAI1_RXD0__AONMIX_TOP_MQS1_RIGHT		0x31e
> +		>;
> +	};
> +
> +	pinctrl_pcal6416: pcal6416grp {
> +		fsl,pins = <
> +			IMX95_PAD_GPIO_IO34__GPIO5_IO_BIT14			0x31
> +		>;
> +	};

Is this pad missing the pull-up enable bit?

In i.MX95 pad configurations, the trailing 0xe enables the pull-up resistor.

Since the pcal6416 interrupt output is open-drain and configured as
active-low, leaving it floating could cause an interrupt storm or missed
interrupts.

Should this be 0x31e instead of 0x31?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260509-imx95_frdm_pro-v3-0-96290b471396@nxp.com?part=2