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

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

[Chen-Yu Tsai]

From: Icenowy Zheng <uwu@icenowy.me>

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@icenowy.me>
Reviewed-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
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);
 
@@ -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);
-- 
2.46.0.76.ge559c4bf1a-goog

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

[Dragan Simic]

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@icenowy.me>
> 
> 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@icenowy.me>
> Reviewed-by: Mark Brown <broonie@kernel.org>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> 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);

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

[Chen-Yu Tsai]

On Mon, Aug 12, 2024 at 9:22 AM Dragan Simic <dsimic@manjaro.org> wrote:
>
> 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@icenowy.me>
> >
> > 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@icenowy.me>
> > Reviewed-by: Mark Brown <broonie@kernel.org>
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> > 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.

I think I understand: move "*ntrips = 0" to after of_node_put() in the
error path, and have the "!trips" branch jump to "out_of_node_put" as
well. That works since of_node_put() checks the pointer first.

I'll wait a bit and see if there are any more comments.

ChenYu

> > @@ -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);

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

[Chen-Yu Tsai]

On Mon, Aug 12, 2024 at 12:46 PM Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> On Mon, Aug 12, 2024 at 9:22 AM Dragan Simic <dsimic@manjaro.org> wrote:
> >
> > 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@icenowy.me>
> > >
> > > 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@icenowy.me>
> > > Reviewed-by: Mark Brown <broonie@kernel.org>
> > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > > ---
> > > 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.
>
> I think I understand: move "*ntrips = 0" to after of_node_put() in the
> error path, and have the "!trips" branch jump to "out_of_node_put" as
> well. That works since of_node_put() checks the pointer first.
>
> I'll wait a bit and see if there are any more comments.

Actually, Krzysztof (+CC) is cleaning up this function using scoped
variables. So it might actually make more sense to move "*ntrips = 0"
to the top once the error path is completely removed.

ChenYu

> ChenYu
>
> > > @@ -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);

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

[Dragan Simic]

Hello Chen-Yu,

On 2024-08-15 06:45, Chen-Yu Tsai wrote:
> On Mon, Aug 12, 2024 at 12:46 PM Chen-Yu Tsai <wenst@chromium.org> 
> wrote:
>> 
>> On Mon, Aug 12, 2024 at 9:22 AM Dragan Simic <dsimic@manjaro.org> 
>> wrote:
>> >
>> > 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@icenowy.me>
>> > >
>> > > 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@icenowy.me>
>> > > Reviewed-by: Mark Brown <broonie@kernel.org>
>> > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
>> > > ---
>> > > 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.
>> 
>> I think I understand: move "*ntrips = 0" to after of_node_put() in the
>> error path, and have the "!trips" branch jump to "out_of_node_put" as
>> well. That works since of_node_put() checks the pointer first.
>> 
>> I'll wait a bit and see if there are any more comments.
> 
> Actually, Krzysztof (+CC) is cleaning up this function using scoped
> variables. So it might actually make more sense to move "*ntrips = 0"
> to the top once the error path is completely removed.

I see, it would make sense to move "*ntrips = 0" to the top, but what
bugs me with that approach a bit is that we's still have another 
instance
of "*ntrips = 0" in the error paths.  Thus, it might be cleaner to have
only one instance of "*ntrips = 0", in the error paths, and use "ret = 
..."
plus "goto ..." pairs instead of single "return ..." statements.

That way, we'd keep "*ntrips = 0" in the error pathso only, which would
clearly show that's part of the error handling only.  Though, I'd be 
also
fine with moving "*ntrips = 0" to the top, if you find that cleaner.

>> > > @@ -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);