Re: [PATCH v2 3/3] ARM: dts: aspeed: yosemite4: Add power module and ADC on Medusa Board

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

Hi Ricky,

On Wed, 2024-09-18 at 17:54 +0800, Delphine CC Chiu wrote:
> From: Ricky CX Wu <ricky.cx.wu.wiwynn@gmail.com>
> 
> Add RTQ6056 as 2nd source ADC sensor on Medusa Board.

Can you unpack why this is related in the commit message? I assume it's
for something like battery monitoring? An explanation would help
though.

> Add power sensors on Medusa board:
> - Add XDP710 as 2nd source HSC to monitor P48V PSU power.
> - Add MP5023 as P12V efuse (Driver exists but un-documented).
> - Add PMBUS sensors as P12V Delta Module.

Generally if you're listing multiple things the change does in the
commit message you should have split the patch up accordingly.

There's some good advice here:

https://docs.kernel.org/process/5.Posting.html#patch-preparation

and here:

https://github.com/axboe/liburing/blob/master/CONTRIBUTING.md?plain=1#L21-L32

> 
> Signed-off-by: Ricky CX Wu <ricky.cx.wu.wiwynn@gmail.com>
> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com>
> ---
>  .../aspeed/aspeed-bmc-facebook-yosemite4.dts  | 45 ++++++++++++++++++-
>  1 file changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-yosemite4.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-yosemite4.dts
> index 98477792aa00..e486b9d78f61 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-yosemite4.dts
> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-yosemite4.dts
> @@ -284,15 +284,25 @@ &i2c10 {
>  &i2c11 {
>  	status = "okay";
>  	power-sensor@10 {
> -		compatible = "adi, adm1272";
> +		compatible = "adi,adm1272";
>  		reg = <0x10>;
>  	};

This was outright busted. The hunk is a fix, as is the second instance
below. Please separate these out into their own patch and add a Fixes:
tag to it.

>  
> +	power-sensor@11 {
> +		compatible = "infineon,xdp710";
> +		reg = <0x11>;
> +	};
> +
>  	power-sensor@12 {
> -		compatible = "adi, adm1272";
> +		compatible = "adi,adm1272";

(i.e. this one also)

>  		reg = <0x12>;
>  	};
>  
> +	power-sensor@13 {
> +		compatible = "infineon,xdp710";
> +		reg = <0x13>;
> +	};
> +
>  	gpio@20 {
>  		compatible = "nxp,pca9555";
>  		reg = <0x20>;
> @@ -321,6 +331,17 @@ gpio@23 {
>  		#gpio-cells = <2>;
>  	};
>  
> +	power-sensor@40 {
> +		compatible = "mps,mp5023";
> +		reg = <0x40>;
> +	};
> +
> +	adc@41 {
> +		compatible = "richtek,rtq6056";
> +		reg = <0x41>;
> +		#io-channel-cells = <1>;
> +	};
> +
>  	temperature-sensor@48 {
>  		compatible = "ti,tmp75";
>  		reg = <0x48>;
> @@ -345,6 +366,26 @@ eeprom@54 {
>  		compatible = "atmel,24c256";
>  		reg = <0x54>;
>  	};
> +
> +	power-sensor@62 {
> +		compatible = "pmbus";
> +		reg = <0x62>;
> +	};
> +
> +	power-sensor@64 {
> +		compatible = "pmbus";
> +		reg = <0x64>;
> +	};
> +
> +	power-sensor@65 {
> +		compatible = "pmbus";
> +		reg = <0x65>;
> +	};
> +
> +	power-sensor@68 {
> +		compatible = "pmbus";
> +		reg = <0x68>;
> +	};

See the discussion on your proposed DT binding document; I expect these
will need to change.

Andrew