Re: [PATCH v13 2/2] arm: dts: aspeed: ventura: add Meta Ventura BMC

Hi P.K.

On Tue, 2026-04-07 at 16:17 +0800, P.K. Lee wrote:
> Add Linux device tree related to Meta (Facebook) Ventura specific
> devices connected to the BMC (AST2600) SoC. The purpose of Ventura is to
> detect liquid leakage from all compute trays, switch trays and rack
> sensors within the rack, log the events, and take necessary actions
> accordingly.
> 
> Signed-off-by: P.K. Lee <[email protected]>
> ---
>  arch/arm/boot/dts/aspeed/Makefile             |    1 +
>  .../aspeed/aspeed-bmc-facebook-ventura.dts    | 1636 +++++++++++++++++
>  2 files changed, 1637 insertions(+)
>  create mode 100644 arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-ventura.dts
> 
> diff --git a/arch/arm/boot/dts/aspeed/Makefile b/arch/arm/boot/dts/aspeed/Makefile
> index 0f0b5b707654..f5ac72d5933c 100644
> --- a/arch/arm/boot/dts/aspeed/Makefile
> +++ b/arch/arm/boot/dts/aspeed/Makefile
> @@ -32,6 +32,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>  	aspeed-bmc-facebook-minipack.dtb \
>  	aspeed-bmc-facebook-santabarbara.dtb \
>  	aspeed-bmc-facebook-tiogapass.dtb \
> +	aspeed-bmc-facebook-ventura.dtb \
>  	aspeed-bmc-facebook-wedge40.dtb \
>  	aspeed-bmc-facebook-wedge100.dtb \
>  	aspeed-bmc-facebook-wedge400-data64.dtb \
> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-ventura.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-ventura.dts
> new file mode 100644
> index 000000000000..6ce6201f7755
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-ventura.dts
> @@ -0,0 +1,1636 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +// Copyright (c) 2023 Facebook Inc.
> +/dts-v1/;
> +
> +#include "aspeed-g6.dtsi"
> +#include <dt-bindings/i2c/i2c.h>
> +#include <dt-bindings/gpio/aspeed-gpio.h>
> +
> +/ {
> +	model = "Facebook ventura RMC";

I suggest capitalising 'Ventura'.

> +	compatible = "facebook,ventura-rmc", "aspeed,ast2600";
> +
> +	aliases {
> +		serial4 = &uart5;
> +		i2c16 = &i2c3mux0ch3;
> +		i2c17 = &i2c3mux0ch4;
> +		i2c18 = &i2c3mux0ch5;
> +		i2c19 = &i2c3mux0ch6;
> +		i2c20 = &i2c3mux0ch0;
> +		i2c21 = &i2c3mux0ch1;
> +		i2c22 = &i2c3mux0ch2;
> +		i2c23 = &i2c3mux0ch7;
> +		i2c24 = &i2c0mux0ch0;
> +		i2c25 = &i2c0mux0ch1;
> +		i2c26 = &i2c0mux0ch2;
> +		i2c27 = &i2c0mux0ch3;
> +		i2c28 = &i2c0mux0ch4;
> +		i2c29 = &i2c0mux0ch5;
> +		i2c30 = &i2c0mux0ch6;
> +		i2c31 = &i2c0mux0ch7;
> +		i2c32 = &i2c1mux0ch0;
> +		i2c33 = &i2c1mux0ch1;
> +		i2c34 = &i2c1mux0ch2;
> +		i2c35 = &i2c1mux0ch3;
> +		i2c36 = &i2c1mux0ch4;
> +		i2c37 = &i2c1mux0ch5;
> +		i2c38 = &i2c1mux0ch6;
> +		i2c39 = &i2c1mux0ch7;
> +		i2c40 = &i2c2mux0ch0;
> +		i2c41 = &i2c2mux0ch1;
> +		i2c42 = &i2c2mux0ch2;
> +		i2c43 = &i2c2mux0ch3;
> +		i2c44 = &i2c2mux0ch4;
> +		i2c45 = &i2c2mux0ch5;
> +		i2c46 = &i2c2mux0ch6;
> +		i2c47 = &i2c2mux0ch7;

Many of the buses aliased here don't have any devices described below
them. Can you add some commentary about why it's necessary to enable
and alias each of these?

> +	};
> +
> +	chosen {
> +		stdout-path = "serial4:57600n8";
> +	};
> +
> +	iio-hwmon {
> +		compatible = "iio-hwmon";
> +		io-channels = <&adc0 0>, <&adc0 1>, <&adc0 2>, <&adc0 3>,
> +			<&adc0 4>, <&adc0 5>, <&adc0 6>, <&adc0 7>,
> +			<&adc1 2>;
> +	};
> +

...

> +		i2c3mux0ch4: i2c@4 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <4>;
> +
> +			adc@1f {
> +				compatible = "ti,adc128d818";
> +				reg = <0x1f>;
> +				ti,mode = /bits/ 8 <1>;
> +			};
> +
> +			fan_leds_g2_gpio: gpio@21 {
> +				compatible = "nxp,pca9555";
> +				reg = <0x21>;
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +
> +				gpio-line-names =
> +				"", "",
> +				"", "",
> +				"", "",
> +				"", "",
> +				"FAN2_PRSNT", "FAN3_PRSNT",
> +				"", "",
> +				"", "",
> +				"", "";
> +			};
> +
> +			adc@35 {
> +				compatible = "maxim,max11617";
> +				reg = <0x35>;
> +			};
> +
> +			// Fan Board 1 FRU

I'd rather we pick one commenting style (/* */). Can you please fix
that throughout?

> +			eeprom@56 {
> +				compatible = "atmel,24c128";
> +				reg = <0x56>;
> +			};
> +		};
> +
> +		i2c3mux0ch5: i2c@5 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <5>;
> +
> +			pwm@20 {
> +				compatible = "maxim,max31790";
> +				reg = <0x20>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				channel@2 {
> +					reg = <2>;
> +					sensor-type = "TACH";
> +				};
> +				channel@5 {
> +					reg = <5>;
> +					sensor-type = "TACH";
> +				};
> +			};
> +
> +			hwmon: hwmon@23 {
> +				compatible = "nuvoton,nct7363";
> +				reg = <0x23>;
> +				#pwm-cells = <2>;
> +
> +				//fan 0 IL

Can you please add a space between the comment marker and the comment
itself? This needs fixing throughout.

> +				fan-0 {
> +					pwms = <&hwmon 0 20000>;
> +					tach-ch = /bits/ 8 <0x09>;
> +				};
> +
> +				//fan 0 OL
> +				fan-1 {
> +					pwms = <&hwmon 0 20000>;
> +					tach-ch = /bits/ 8 <0x0B>;
> +				};
> +
> +				//fan 1 IL
> +				fan-2 {
> +					pwms = <&hwmon 4 20000>;
> +					tach-ch = /bits/ 8 <0x0A>;
> +				};
> +
> +				//fan 1 OL
> +				fan-3 {
> +					pwms = <&hwmon 4 20000>;
> +					tach-ch = /bits/ 8 <0x0D>;
> +				};
> +
> +				//fan 2 IL
> +				fan-4 {
> +					pwms = <&hwmon 6 20000>;
> +					tach-ch = /bits/ 8 <0x0F>;
> +				};
> +
> +				//fan 2 OL
> +				fan-5 {
> +					pwms = <&hwmon 6 20000>;
> +					tach-ch = /bits/ 8 <0x01>;
> +				};
> +
> +				//fan 3 IL
> +				fan-6 {
> +					pwms = <&hwmon 10 20000>;
> +					tach-ch = /bits/ 8 <0x00>;
> +				};
> +
> +				//fan 3 OL
> +				fan-7 {
> +					pwms = <&hwmon 10 20000>;
> +					tach-ch = /bits/ 8 <0x03>;
> +				};
> +			};
> +		};
> +
> 

...

> +
> +&mdio0 {
> +	status = "okay";
> +	/* * Intentionally left empty.

The comment is a bit busted here. Can you please fix it?

Andrew

> +	 * Enabled to allow user-space tools (e.g., mdio)
> +	 * to access the unmanaged Marvell switch registers.
> +	 */
> +};
> +