Re: [PATCH 3/3] ARM: dts: sun9i-a80: Add thermal sensor and thermal zones

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The thermal zones for the CPU clusters are incorrectly named relative to their mapped hardware sensors, risking incorrect cooling mitigations.
- [High] The A80 thermal sensor driver lacks a `.calibrate` callback, resulting in a NULL pointer dereference if `nvmem` calibration data is ever provided.
- [Medium] The critical trip point temperature does not match the driver's emergency shutdown threshold as explicitly claimed in the commit message.
- [Low] The 'ths' node is not sorted properly by unit address.
--

--- Patch [3]: [PATCH 3/3] ARM: dts: sun9i-a80: Add thermal sensor and thermal zones ---
commit cb2cbf274b6f042626c2998af64fc2030a9af3ea
Author: Philipp Rossak <embed3d@gmail.com>

ARM: dts: sun9i-a80: Add thermal sensor and thermal zones

This commit adds the device node for the A80 thermal sensor and the thermal
zones to the device tree. It maps the sensors to the big CPU cluster, DRAM,
GPU, and little CPU cluster.

> [soeren: rebased from the 2018 series, fixed the interrupt number
> (SPI 115 as in the vendor BSP; verified on hardware - SPI 31 stays
> silent), added critical trip points matching the driver's emergency
> shutdown threshold]

[Severity: Medium]
Does the trip point temperature match the driver's emergency shutdown
threshold as claimed in the commit message?

The commit message states that the patch adds trip points matching the
driver's emergency shutdown threshold, which is around 105C. However, the
device tree source sets the temperature to 100C (100000). Could the commit
message be more precise about this difference?

> diff --git a/arch/arm/boot/dts/allwinner/sun9i-a80.dtsi b/arch/arm/boot/dts/allwinner/sun9i-a80.dtsi
> index a1ae0929cec9d..8e7f8cd943d2e 100644
> --- a/arch/arm/boot/dts/allwinner/sun9i-a80.dtsi
> +++ b/arch/arm/boot/dts/allwinner/sun9i-a80.dtsi
> @@ -941,6 +941,16 @@ ccu: clock@6000000 {
>  			#reset-cells = <1>;
>  		};
>  
> +		ths: thermal-sensor@6004c00 {

[Severity: Low]
Is this node sorted properly by unit address?

This thermal-sensor@6004c00 node is inserted between clock@6000000 and
timer@6000c00. Since 0x06004c00 is numerically greater than 0x06000c00, it
appears to break the standard ascending order sorting of unit addresses.

> +			compatible = "allwinner,sun9i-a80-ths";

[Severity: High]
Does the sun9i-a80-ths driver lack a calibrate callback?

If an nvmem calibration cell is provided in the device tree, the driver
unconditionally calls the calibrate callback:

drivers/thermal/sun8i_thermal.c:sun8i_ths_calibrate() {
    ...
    tmdev->chip->calibrate(tmdev, caldata, callen);
    ...
}

Will this cause a NULL pointer dereference during driver probe if calibration
data is ever provided, since sun9i-a80-ths omits this function pointer?

> +			reg = <0x06004c00 0x100>;
> +			interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&ccu CLK_BUS_GPADC>, <&ccu CLK_GPADC>;
> +			clock-names = "bus", "mod";
> +			resets = <&ccu RST_BUS_GPADC>;
> +			#thermal-sensor-cells = <1>;
> +		};
> +
>  		timer@6000c00 {

[ ... ]

> @@ -1250,4 +1260,62 @@ r_rsb: rsb@8003400 {
>  			#size-cells = <0>;
>  		};
>  	};
> +
> +	thermal-zones {
> +		cpu0_thermal: cpu0-thermal {
> +			polling-delay-passive = <1000>;
> +			polling-delay = <5000>;
> +			thermal-sensors = <&ths 0>;

[Severity: High]
Are the thermal zones for the CPU clusters incorrectly named relative to their
mapped hardware sensors?

The commit message notes that sensor 0 is located beside the big CPU cluster
(cpu4-7), and sensor 3 is beside the little CPU cluster (cpu0-3).

Here cpu0-thermal is mapped to sensor 0 (the big cluster). Will this cause
userspace daemons or future device tree cooling maps to incorrectly throttle
the little cluster when the big cluster overheats?

[ ... ]

> +		cpu2_thermal: cpu2-thermal {
> +			polling-delay-passive = <1000>;
> +			polling-delay = <5000>;
> +			thermal-sensors = <&ths 3>;

[Severity: High]
Similar to the question above, cpu2-thermal targets sensor 3, which monitors
the little cluster. Is this misleadingly named, risking incorrect cooling
mitigations?

> +
> +			trips {
> +				cpu2_crit: cpu2-crit {
> +					temperature = <100000>;
> +					hysteresis = <2000>;
> +					type = "critical";
> +				};
> +			};
> +		};
> +	};
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722203341.2917852-1-fugininsane@googlemail.com?part=3