Re: [PATCH v1 6/8] ARM: dts: aspeed: System1: GPIO, Fan ctrl, Led

[Krzysztof Kozlowski]

On 12/12/2023 17:40, Ninad Palsule wrote:
> This commit adds following devices to the device tree.
> - GPIO pin assignements, GPIO expansion devices
> - LED brinker devices
> - Fan controllers
> 
> Tested:
>     This board is tested using the simics simulator.
> 
> Signed-off-by: Ninad Palsule <ninad@xxxxxxxxxxxxx>
> ---
>  .../dts/aspeed/aspeed-bmc-ibm-system1.dts     | 547 +++++++++++++++++-

Squash it.

>  1 file changed, 542 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
> index b8e7e52d4600..75562aa63701 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
> @@ -114,6 +114,99 @@ vga_memory: region@bf000000 {
>  		};
>  	};
>  
> +	leds {
> +		compatible = "gpio-leds";
> +
> +		bmc-ready {

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

> +			gpios = <&gpio0 ASPEED_GPIO(L, 7) GPIO_ACTIVE_HIGH>;
> +		};
> +
> +		bmc-hb {

None of these were tested.


>  	/*A0-A7*/	"","","","","","","","",
> -	/*B0-B7*/	"","","","","","","","",
> +	/*B0-B7*/	"","","","","bmc-tpm-reset","","","",

Really? You just added these lines. There is no point in adding a new
line and immediately changing it.

This points how your split is artificial and not helpful.
...


>  &i2c2 {
> @@ -486,6 +744,20 @@ regulator@43 {
>  &i2c6 {
>  	status = "okay";
>  
> +	fan-controller@52 {
> +		compatible = "maxim,max31785a";
> +		reg = <0x52>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;

Why do you need cells?

> +	};
> +
> +	fan-controller@54 {
> +		compatible = "maxim,max31785a";
> +		reg = <0x54>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;

Why do you need cells?

> +	};
> +
>  	i2c-mux@70 {
>  		compatible = "nxp,pca9548";
>  		reg = <0x70>;
> @@ -522,6 +794,48 @@ i2c6mux0chn4: i2c@4 {
>  			#address-cells = <1>;
>  			#size-cells = <0>;
>  			reg = <4>;
> +
> +			led-controller@60 {
> +				compatible = "nxp,pca9551";
> +				reg = <0x60>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +
> +				led@0 {
> +					label = "enclosure-id-led";
> +					reg = <0>;
> +					retain-state-shutdown;
> +					default-state = "keep";
> +					type = <PCA955X_TYPE_LED>;
> +				};
> +
> +				led@1 {
> +					label = "attention-led";
> +					reg = <1>;
> +					retain-state-shutdown;
> +					default-state = "keep";
> +					type = <PCA955X_TYPE_LED>;
> +				};
> +
> +				led@2 {
> +					label = "enclosure-fault-rollup-led";
> +					reg = <2>;
> +					retain-state-shutdown;
> +					default-state = "keep";
> +					type = <PCA955X_TYPE_LED>;
> +				};
> +
> +				led@3 {
> +					label = "power-on-led";
> +					reg = <3>;
> +					retain-state-shutdown;
> +					default-state = "keep";
> +					type = <PCA955X_TYPE_LED>;
> +				};
> +			};
>  		};
>  
>  		i2c6mux0chn5: i2c@5 {
> @@ -542,6 +856,44 @@ i2c6mux0chn7: i2c@7 {
>  			reg = <7>;
>  		};
>  	};
> +
> +	pca3: pca9539@74 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +		compatible = "nxp,pca9539";
> +		reg = <0x74>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +	};
> +
> +	pca4: pca9539@77 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +		compatible = "nxp,pca9539";
> +		reg = <0x77>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +
> +		gpio-line-names =
> +			"PE_NVMED0_EXP_PRSNT_N",
> +			"PE_NVMED1_EXP_PRSNT_N",
> +			"PE_NVMED2_EXP_PRSNT_N",
> +			"PE_NVMED3_EXP_PRSNT_N",
> +			"LED_FAULT_NVMED0",
> +			"LED_FAULT_NVMED1",
> +			"LED_FAULT_NVMED2",
> +			"LED_FAULT_NVMED3",
> +			"FAN0_PRESENCE_R_N",
> +			"FAN1_PRESENCE_R_N",
> +			"FAN2_PRESENCE_R_N",
> +			"FAN3_PRESENCE_R_N",
> +			"FAN4_PRESENCE_R_N",
> +			"FAN5_PRESENCE_N",
> +			"FAN6_PRESENCE_N",
> +			"";
> +	};
>  };
>  
>  &i2c7 {
> @@ -809,6 +1161,191 @@ regulator@41 {
>  		compatible = "infineon,ir38263";
>  		reg = <0x41>;
>  	};
> +
> +	led-controller@61 {
> +		compatible = "nxp,pca9552";
> +		reg = <0x61>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +

...

> +		led@15 {
> +			label = "pe-cp-drv3-perst";
> +			reg = <15>;
> +			retain-state-shutdown;
> +			default-state = "keep";
> +			type = <PCA955X_TYPE_LED>;
> +		};
> +	};
> +
> +	pca1: pca9539@75 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation




Best regards,
Krzysztof