Re: [PATCH v6 02/12] ARM: dts: aspeed: anacapa: Add EVT1 devicetree and point wrapper to it

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Devicetree ABI breakage due to removal of generic compatible string
- [Medium] Incorrect removal of `p3v3_bmc_aux` fixed-regulator node
- [Low] Included devicetrees use `.dts` extension instead of `.dtsi`
--

--- Patch [2]: [PATCH v6 02/12] ARM: dts: aspeed: anacapa: Add EVT1 devicetree and point wrapper to it ---
commit 9a026f3bc21757ceb40a47713b367e173351589e
Author: Colin Huang <u8813345@gmail.com>

ARM: dts: aspeed: anacapa: Add EVT1 devicetree and point wrapper to it

This commit introduces an EVT1 development-phase devicetree for the
Facebook Anacapa BMC. It updates the top-level wrapper DTS to include
the EVT1 hardware revision file, removes redundant pinctrl names, and
fixes a dt-check-style strict violation.

>       Remove p3v3_bmc_aux node to fix '[unused-lable]' warning.

[Severity: Medium]
Is it correct to remove the entire hardware node here?

If the intention is just to fix an unused label warning, usually only the
label itself (p3v3_bmc_aux:) is removed, preserving the regulator node to
accurately describe the EVT1 hardware.

While follow-up patches in the series (e.g., commit "ARM: dts: aspeed:
anacapa: Add SPI flash VCC supply") add the regulator back into the EVT2
devicetree, the EVT1 devicetree baseline remains permanently stripped of
this regulator node.

> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-anacapa-evt1.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-anacapa-evt1.dts
> new file mode 100644
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-anacapa-evt1.dts
> @@ -0,0 +1,1120 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/dts-v1/;
> +#include "aspeed-g6.dtsi"
> +#include <dt-bindings/gpio/aspeed-gpio.h>
> +#include <dt-bindings/i2c/i2c.h>
> +
> +/ {
> +	compatible = "facebook,anacapa-bmc-evt1", "aspeed,ast2600";

[Severity: Critical]
Does removing the generic "facebook,anacapa-bmc" string from the compatible
property break existing ABI?

Bootloaders, kernel quirks, or userspace utilities (like systemd or OpenBMC)
that rely on matching the generic string will fail to recognize the board
during an upgrade.

Even at the end of the patch series, the generic string is never restored.
Should the wrapper file override this to retain the generic string, or should
it be appended here?

[ ... ]

> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-anacapa.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-anacapa.dts
> --- a/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-anacapa.dts
> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-anacapa.dts
> @@ -1,1079 +1,3 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  
> -/dts-v1/;
> -#include "aspeed-g6.dtsi"

[ ... ]

> -/ {
> -	model = "Facebook Anacapa BMC";
> -	compatible = "facebook,anacapa-bmc", "aspeed,ast2600";

[ ... ]

> +#include "aspeed-bmc-facebook-anacapa-evt1.dts"

[Severity: Low]
Is it intended for this included file to use a .dts extension?

In kernel devicetree conventions, files intended to be included by other
files typically use the .dtsi (Device Tree Source Include) extension. Since
this evt1 file is strictly used as an include base and is not built as a
separate target in the Makefile, would it be better named with a .dtsi
extension?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825-anacapa-devlop-phase-devicetree-v6-0-9059a2a279ce@gmail.com?part=2