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

[Vincent Jardin <vjardin@free.fr>]

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/

> > +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.

> > +/ {
> > +    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.

> > +&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.

> > +    /* 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

> > +    /* 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