Re: [PATCH v5 2/2] ARM: dts: aspeed: ventura2: Add Meta ventura2 BMC

[Andrew Jeffery <andrew@codeconstruct.com.au>]

Hi Kyle,

On Mon, 2026-06-08 at 10:42 +0800, Kyle Hsieh wrote:
> Add linux device tree entry related to the Meta(Facebook) rmc-node.

This is redundant as it is evident from the patch itself. Can you
please remove it?

> The system use an AT2600 BMC.
> This node is named "ventura2".

Can you provide some more detail about the platform in the commit
message? What's it's purpose? Can you describe some interesting
features or details about its design?

> 
> Signed-off-by: Kyle Hsieh <kylehsieh1995@gmail.com>
> ---
>  arch/arm/boot/dts/aspeed/Makefile                  |    1 +
>  .../dts/aspeed/aspeed-bmc-facebook-ventura2.dts    | 2888 ++++++++++++++++++++
>  2 files changed, 2889 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/aspeed/Makefile b/arch/arm/boot/dts/aspeed/Makefile
> index 9adf9278dc94..6b96997629d4 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-ventura2.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-ventura2.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-ventura2.dts
> new file mode 100644
> index 000000000000..9bf7d6e52e40
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-ventura2.dts
> @@ -0,0 +1,2888 @@
> +// 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 Ventura2 RMC";
> +	compatible = "facebook,ventura2-rmc", "aspeed,ast2600";
> 

...

> +};
> +

...

> +&gpio1 {
> +	gpio-line-names =
> +	/*18A0-18A7*/	"","","","","","","","",
> +	/*18B0-18B7*/	"","","","",
> +			"FM_BOARD_BMC_REV_ID0","FM_BOARD_BMC_REV_ID1",
> +			"FM_BOARD_BMC_REV_ID2","",
> +	/*18C0-18C7*/	"SPI_BMC_BIOS_ROM_IRQ0_R_N","","","","","","","",
> +	/*18D0-18D7*/	"","","","","","","","",
> +	/*18E0-18E3*/	"FM_BMC_PROT_LS_EN","AC_PWR_BMC_BTN_R_N","","";
> +};
> +
> +&i2c0 {
> +	status = "okay";
> +
> +	i2c-mux@77 {
> +		compatible = "nxp,pca9548";
> +		reg = <0x77>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		i2c-mux-idle-disconnect;
> +
> +		i2c0mux0ch0: i2c@0 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <0>;
> +		};
> +
> +		i2c0mux0ch1: i2c@1 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <1>;
> +		};
> +
> +		i2c0mux0ch2: i2c@2 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <2>;
> +		};
> +
> +		i2c0mux0ch3: i2c@3 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <3>;
> +			status = "okay";
> +		};
> +
> +		i2c0mux0ch4: i2c@4 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <4>;
> +			status = "okay";
> +		};
> +
> +		i2c0mux0ch5: i2c@5 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <5>;
> +			status = "okay";
> +
> +			eeprom@56 {
> +				compatible = "atmel,24c128";
> +				reg = <0x56>;
> +			};
> +		};
> +
> +		i2c0mux0ch6: i2c@6 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <6>;
> +
> +			eeprom@56 {
> +				compatible = "atmel,24c128";
> +				reg = <0x56>;
> +			};
> +
> +			fan_io_expander0: gpio@20 {
> +				compatible = "nxp,pca9555";
> +				reg = <0x20>;
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +			};
> +
> +			fan_io_expander1: gpio@21 {
> +				compatible = "nxp,pca9555";
> +				reg = <0x21>;
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +			};
> +
> +			adc@1d {
> +				compatible = "ti,adc128d818";
> +				reg = <0x1d>;
> +				ti,mode = /bits/ 8 <1>;
> +			};
> +
> +			adc@35 {
> +				compatible = "maxim,max11617";
> +				reg = <0x35>;
> +			};
> +		};
> +
> +		i2c0mux0ch7: i2c@7 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <7>;
> +
> +			fanctl0: fan-controller@20 {
> +				compatible = "maxim,max31790";
> +				reg = <0x20>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				channel@2 {

Can you make sure that you consistently use a blank line to separate
child nodes from each other and from properties in their parent?

Please fix throughout.

> +					reg = <2>;
> +					sensor-type = "TACH";
> +				};
> +				channel@5 {
> +					reg = <5>;
> +					sensor-type = "TACH";
> +				};
> +			};
> +
> +			fanctl1: fan-controller@23 {
> +				compatible = "nuvoton,nct7363";
> +				reg = <0x23>;
> +				#pwm-cells = <2>;
> +
> +				fan-9 {
> +					pwms = <&fanctl1 0 20000>;
> +					tach-ch = /bits/ 8 <0x09>;
> +				};
> +
> +				fan-11 {
> +					pwms = <&fanctl1 0 20000>;
> +					tach-ch = /bits/ 8 <0x0B>;
> +				};
> +
> +				fan-10 {
> +					pwms = <&fanctl1 4 20000>;
> +					tach-ch = /bits/ 8 <0x0A>;
> +				};
> +
> +				fan-13 {
> +					pwms = <&fanctl1 4 20000>;
> +					tach-ch = /bits/ 8 <0x0D>;
> +				};
> +
> +				fan-15 {
> +					pwms = <&fanctl1 6 20000>;
> +					tach-ch = /bits/ 8 <0x0F>;
> +				};
> +
> +				fan-1 {

Can you please sort the fan nodes in ascending order?

> +					pwms = <&fanctl1 6 20000>;
> +					tach-ch = /bits/ 8 <0x01>;
> +				};
> +
> +				fan-0 {
> +					pwms = <&fanctl1 10 20000>;
> +					tach-ch = /bits/ 8 <0x00>;
> +				};
> +
> +				fan-3 {
> +					pwms = <&fanctl1 10 20000>;
> +					tach-ch = /bits/ 8 <0x03>;
> +				};
> +			};
> +		};
> +	};
> +};
> 

...

> +
> +	// Marvell 88E6393X EEPROM

Please try to be consistent with the comment style (prefer /* */).

> +	eeprom@50 {
> +		compatible = "atmel,24c64";
> +		reg = <0x50>;
> +	};
> +
> +	rtc@51 {
> +		compatible = "nxp,pcf8563";
> +		reg = <0x51>;
> +	};
> +};
> +