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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <11a0ae09bc722b1f21ae76df99bd8c1d885c85d1.camel@codeconstruct.com.au>
Date: Thu, 19 Sep 2024 10:52:24 +0930
From: Andrew Jeffery <andrew@...econstruct.com.au>
To: Delphine CC Chiu <Delphine_CC_Chiu@...ynn.com>, patrick@...cx.xyz, Rob
 Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor
 Dooley <conor+dt@...nel.org>, Joel Stanley <joel@....id.au>
Cc: Ricky CX Wu <ricky.cx.wu.wiwynn@...il.com>, Guenter Roeck
 <linux@...ck-us.net>, Noah Wang <noahwang.wang@...look.com>, Peter Yin
 <peteryin.openbmc@...il.com>, Javier Carrasco
 <javier.carrasco.cruz@...il.com>,  Lukas Wunner <lukas@...ner.de>, Laurent
 Pinchart <laurent.pinchart@...asonboard.com>,  devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org,  linux-arm-kernel@...ts.infradead.org,
 linux-aspeed@...ts.ozlabs.org
Subject: Re: [PATCH v2 3/3] ARM: dts: aspeed: yosemite4: Add power module
 and ADC on Medusa Board

Hi Ricky,

On Wed, 2024-09-18 at 17:54 +0800, Delphine CC Chiu wrote:
> From: Ricky CX Wu <ricky.cx.wu.wiwynn@...il.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@...il.com>
> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@...ynn.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