Re: [PATCH 3/3] arm64: dts: freescale: add Nodebox v3 CPU Module

Hi M. Bot,

Thanks for the review. Addressed in v2:
  https://lore.kernel.org/all/20260907-for-upstream-nbxv3-dts-v2-0-6b4afd36180b@free.fr/ (local)

quoted

+DTC_FLAGS_fsl-lx2160a-nbxv3 := -Wno-interrupt_map
+dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-lx2160a-nbxv3.dtb
 DTC_FLAGS_fsl-lx2162a-clearfog := -Wno-interrupt_map

[Severity: Low]
Does this break the alphabetical ordering of the dtbs? It looks like
fsl-lx2160a-nbxv3.dtb is being inserted after fsl-lx2160a-rdb.dtb instead of
before fsl-lx2160a-qds.dtb.

Yes. Fixed in v2: fsl-lx2160a-nbxv3.dtb before fsl-lx2160a-qds.dtb.

quoted

+/ {
+    model = "Free Mobile Nodebox v3 CPU Module (LX2160A)";
+    compatible = "freemobile,nbxv3", "fsl,lx2160a";
+};

[Severity: Low]
Is the "freemobile,nbxv3" compatible string documented in the device tree
bindings? Missing documentation here might trigger warnings during a
make dtbs_check run.

It is.

patch 2 of this series, "dt-bindings: arm: fsl: add freemobile,nbxv3 board", 
and patch 1 adds the vendor prefix.

Both precede the DTS, so dtbs_check should be ok.

quoted

+&fspi {
+    pinctrl-names = "default";
+    pinctrl-0 = <&fspi_dqs_sck_cs10_pins>, <&fspi_data30_pins>,
+            <&gpio1_31_28_pins>;
+};

[Severity: Medium]
Does assigning the pinctrl group for GPIOs 28-31 to the fspi device create
a conflict with the gpio-hogs defined in the gpio1 controller?

If the pinmux controller enforces strict muxing, the gpio-hog nodes for
the ethernet switch and flash resets will fail to claim the pins because
they have already been claimed by fspi's pinctrl here. Furthermore, tying
unrelated system resets to the fspi peripheral's pinctrl lifecycle means
that if the fspi driver fails to probe, or is unbound, these components
will lose their configuration.

In v2 the group is applied by &gpio1 itself:
the controller that owns those lines and their hogs, and fspi's pinctrl-0
lists only its own pins.

quoted

+    /* Ethernet switch reset, released: electrical high */
+    ethsw-reset-hog {
+        gpio-hog;
+        gpios = <31 GPIO_ACTIVE_HIGH>;
+        output-high;
+        line-name = "ethsw-reset#";
+    };
+};

[Severity: Low]
Is this active-low hardware reset signal defined with the correct semantic
polarity? Defining an active-low reset as GPIO_ACTIVE_HIGH and manually
forcing the electrical state with output-high bypasses the device tree
coding standards, which expect the hardware's active state to be accurately
reflected in the bindings.

Agreed. done with v2

quoted

+    /* Low enables the carrier-side EMDIO2 level translator */
+    ethsw-mdio-en-hog {
+        gpio-hog;
+        gpios = <25 GPIO_ACTIVE_HIGH>;
+        output-low;
+        line-name = "ethsw-mdio-en";
+    };
+};

[Severity: Low]
Similarly, this active-low enable signal is defined as GPIO_ACTIVE_HIGH
while using output-low to force the electrical low. Should this be defined
as GPIO_ACTIVE_LOW instead to reflect its true semantic polarity?

Agreed, see v2.

Thanks,
  Vincent