Re: [PATCH v1 3/3] arm64: dts: imx8mp-var-dart: Add support for Variscite Sonata board

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Shared open-drain interrupts for PCA6408 GPIO expanders are incorrectly configured as edge-triggered.
- [Medium] The `model` property incorrectly identifies the board as 'DART-MX93' instead of 'DART-MX8M-PLUS'.
- [Low] Regulator pinmux configuration is incorrectly placed in the consumer node.
- [Low] PTN5150 Type-C controller interrupt trigger level is incorrectly specified as active-high.
--

commit 95bc444bf3e6a9d1e620e1258513ee415d198b04
Author: Stefano Radaelli <stefano.r@variscite.com>
Subject: arm64: dts: imx8mp-var-dart: Add support for Variscite Sonata board

This commit adds device tree support for the Variscite Sonata carrier board
equipped with the DART-MX8M-PLUS system on module. It includes configurations
for peripherals such as Ethernet, USB, HDMI, and CAN.

Link: https://variscite.com/carrier-boards/sonata-board/

> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-var-dart-sonata.dts b/arch/arm64/boot/dts/freescale/imx8mp-var-dart-sonata.dts
> new file mode 100644
> index 0000000000000..4aa1545377191
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mp-var-dart-sonata.dts
> @@ -0,0 +1,716 @@
[ ... ]
> +/ {
> +	model = "Variscite DART-MX93 on Sonata-Board";

[Severity: Medium]
Should this model string be "DART-MX8M-PLUS" instead of "DART-MX93"?

The compatible strings, file name, and commit message all indicate this is an
i.MX8M Plus based board. Exposing "DART-MX93" to userspace via
/sys/firmware/devicetree/base/model could break provisioning scripts.

> +	compatible = "variscite,var-dart-mx8mp-sonata",
> +		     "variscite,var-dart-mx8mp",
> +		     "fsl,imx8mp";
[ ... ]
> +	reg_usdhc2_vmmc: regulator-vmmc-usdhc2 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "VSD_3V3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		gpio = <&gpio2 19 GPIO_ACTIVE_HIGH>;

[Severity: Low]
Should the pinmux for gpio2 19 be placed in this regulator node instead of the
usdhc2 consumer node?

The reg_usdhc2_vmmc fixed regulator uses gpio2 19, but its pinmux
configuration (MX8MP_IOMUXC_SD2_RESET_B__GPIO2_IO19) is placed in
pinctrl_usdhc2_gpio under the &usdhc2 node.

While the execution order might happen to mask this issue, the pin multiplexing
configuration for a GPIO should reside in the device node that requests it.

> +		enable-active-high;
> +		startup-delay-us = <100>;
> +		off-on-delay-us = <12000>;
> +	};
[ ... ]
> +	typec@3d {
> +		compatible = "nxp,ptn5150";
> +		reg = <0x3d>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_extcon>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <10 IRQ_TYPE_LEVEL_HIGH>;

[Severity: Low]
Should this interrupt trigger level be active-low (IRQ_TYPE_LEVEL_LOW) rather
than active-high?

The PTN5150 uses an active-low, open-drain interrupt line (INTB). Configuring
it as active-high level-sensitive in the device tree contradicts the hardware
specification, even if the driver currently overrides it by requesting
IRQF_TRIGGER_FALLING explicitly.

> +
> +		port {
> +			typec_dr_sw: endpoint {
> +				remote-endpoint = <&usb3_drd_sw>;
> +			};
> +		};
> +	};
[ ... ]
> +	pca6408_1: gpio@20 {
> +		compatible = "nxp,pcal6408";
> +		reg = <0x20>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_pca6408>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <5 IRQ_TYPE_EDGE_FALLING>;
> +	};
> +
> +	pca6408_2: gpio@21 {
> +		compatible = "nxp,pcal6408";
> +		reg = <0x21>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <5 IRQ_TYPE_EDGE_FALLING>;

[Severity: High]
Should these shared open-drain interrupts be configured as IRQ_TYPE_LEVEL_LOW
instead of IRQ_TYPE_EDGE_FALLING?

Both pca6408_1 and pca6408_2 share the same interrupt line (gpio1 5). The
PCA6408 has an active-low open-drain interrupt output.

When multiple devices share an open-drain interrupt line, the controller must
be level-sensitive. If configured as edge-triggered, and the second chip
asserts its interrupt while the first chip holds the line low, the second
interrupt could be missed entirely.

> +	};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1780910435.git.stefano.r@variscite.com?part=3