Re: [PATCH v2] thermal/of: support thermal zones w/o trips subnode

From: Dragan Simic
Date: Sun Aug 11 2024 - 21:22:21 EST


Hello Chen-Yu,

Thanks for the patch. Please see one comment below.

On 2024-08-09 09:08, Chen-Yu Tsai wrote:
From: Icenowy Zheng <uwu@xxxxxxxxxx>

Although the current device tree binding of thermal zones require the
trips subnode, the binding in kernel v5.15 does not require it, and many
device trees shipped with the kernel, for example,
allwinner/sun50i-a64.dtsi and mediatek/mt8183-kukui.dtsi in ARM64, still
comply to the old binding and contain no trips subnode.

Allow the code to successfully register thermal zones w/o trips subnode
for DT binding compatibility now.

Furtherly, the inconsistency between DTs and bindings should be resolved
by either adding empty trips subnode or dropping the trips subnode
requirement.

Fixes: d0c75fa2c17f ("thermal/of: Initialize trip points separately")
Signed-off-by: Icenowy Zheng <uwu@xxxxxxxxxx>
Reviewed-by: Mark Brown <broonie@xxxxxxxxxx>
Signed-off-by: Chen-Yu Tsai <wenst@xxxxxxxxxxxx>
---
Resurrecting this patch specifically for MediaTek MT8183 Kukui devices.

Changes since v1:
- set *ntrips at beginning of thermal_of_trips_init()
- Keep goto out_of_node_put in of_get_child_count(trips) == 0 branch
- Check return value of thermal_of_trips_init(), if it is -ENXIO, print
warning and clear |trips| pointer
- Drop |mask| change, as the variable was removed

I kept Mark's reviewed-by since the changes are more stylish than
functional.
---
drivers/thermal/thermal_of.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
index aa34b6e82e26..f237e74c92fc 100644
--- a/drivers/thermal/thermal_of.c
+++ b/drivers/thermal/thermal_of.c
@@ -128,16 +128,17 @@ static struct thermal_trip
*thermal_of_trips_init(struct device_node *np, int *n
struct device_node *trips, *trip;
int ret, count;

+ *ntrips = 0;
trips = of_get_child_by_name(np, "trips");
if (!trips) {
- pr_err("Failed to find 'trips' node\n");
- return ERR_PTR(-EINVAL);
+ pr_debug("Failed to find 'trips' node\n");
+ return ERR_PTR(-ENXIO);
}

count = of_get_child_count(trips);
if (!count) {
- pr_err("No trip point defined\n");
- ret = -EINVAL;
+ pr_debug("No trip point defined\n");
+ ret = -ENXIO;
goto out_of_node_put;
}

@@ -162,7 +163,6 @@ static struct thermal_trip
*thermal_of_trips_init(struct device_node *np, int *n

out_kfree:
kfree(tt);
- *ntrips = 0;
out_of_node_put:
of_node_put(trips);

It might be a bit cleaner to keep the "*ntrips = 0" assignment
in the error handling path(s) only, with the positions of the goto
labels adjusted a bit, and then assign -ENXIO to "ret" and jump
to the right label when of_get_child_by_name(np, "trips") fails,
instead of returning from there.

If it's unclear what I'm talking about, please let me know and
I'll send back the proposed hunk.

@@ -490,8 +490,13 @@ static struct thermal_zone_device
*thermal_of_zone_register(struct device_node *

trips = thermal_of_trips_init(np, &ntrips);
if (IS_ERR(trips)) {
- pr_err("Failed to find trip points for %pOFn id=%d\n", sensor, id);
- return ERR_CAST(trips);
+ if (PTR_ERR(trips) != -ENXIO) {
+ pr_err("Failed to find trip points for %pOFn id=%d\n", sensor, id);
+ return ERR_CAST(trips);
+ }
+
+ pr_warn("Failed to find trip points for %pOFn id=%d\n", sensor, id);
+ trips = NULL;
}

ret = thermal_of_monitor_init(np, &delay, &pdelay);