Re: [PATCH v4 5/7] ARM: dts: aspeed: yosemite5: Correct power monitor shunt resistor

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

Hi Kevin,

On Fri, 2026-01-30 at 16:20 +0800, Kevin Tung wrote:
> The shunt resistor value defined in the DTS was incorrect and did not
> reflect the resistor value populated on the board. As a result, power
> and current readings derived from this value were inaccurate.

Okay, but how did it come to be incorrect in the devicetree? Was the
documentation wrong and so someone entered the wrong value? Was the
documentation correct but there was a misunderstanding? Did the
hardware design change and the devicetree is now just inappropriate for
your needs?

Essentially, what's preventing this from happening again?

> 
> This change 
> 

I'd rather you rephrase this to avoid using "This change ...".
Hopefully the commit message always refers to its change.

Generally, please write in the imperative mood (see [1])

[1]: https://docs.kernel.org/process/submitting-patches.html#describe-your-changes

> updates the DTS to use the correct shunt resistor value
> according to the hardware design, restoring accurate power and current
> measurements.
> 
> Signed-off-by: Kevin Tung <kevin.tung.openbmc@gmail.com>
> ---
>  arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-yosemite5.dts | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-yosemite5.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-yosemite5.dts
> index 84d3731b17f7c7c87338672bbcc859de2b89b722..524597a81365ef10cd03b67d35eeb88a965cbe0a 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-yosemite5.dts
> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-yosemite5.dts
> @@ -415,7 +415,7 @@ power-sensor@42 {
>  	power-monitor@43 {
>  		compatible = "lltc,ltc4287";
>  		reg = <0x43>;
> -		shunt-resistor-micro-ohms = <250>;
> +		shunt-resistor-micro-ohms = <100>;
>  	};
>  
>  	power-sensor@44 {
> @@ -461,25 +461,25 @@ eeprom@57 {
>  	power-monitor@58 {
>  		compatible = "renesas,isl28022";
>  		reg = <0x58>;
> -		shunt-resistor-micro-ohms = <1000>;
> +		shunt-resistor-micro-ohms = <10000>;
>  	};
>  
>  	power-monitor@59 {
>  		compatible = "renesas,isl28022";
>  		reg = <0x59>;
> -		shunt-resistor-micro-ohms = <1000>;
> +		shunt-resistor-micro-ohms = <10000>;
>  	};
>  
>  	power-monitor@5a {
>  		compatible = "renesas,isl28022";
>  		reg = <0x5a>;
> -		shunt-resistor-micro-ohms = <1000>;
> +		shunt-resistor-micro-ohms = <10000>;
>  	};
>  
>  	power-monitor@5b {
>  		compatible = "renesas,isl28022";
>  		reg = <0x5b>;
> -		shunt-resistor-micro-ohms = <1000>;
> +		shunt-resistor-micro-ohms = <10000>;
>  	};
>  
>  	psu@5c {
> @@ -723,13 +723,13 @@ gpio-expander@21 {
>  	power-sensor@40 {
>  		compatible = "ti,ina230";
>  		reg = <0x40>;
> -		shunt-resistor = <2000>;
> +		shunt-resistor = <1000>;
>  	};
>  
>  	power-sensor@41 {
>  		compatible = "ti,ina230";
>  		reg = <0x41>;
> -		shunt-resistor = <2000>;
> +		shunt-resistor = <1000>;
>  	};
>  
>  	power-sensor@42 {

The way the commit message is written suggests it was just one shunt
resistor, but that's clearly not the case.

Andrew