1. Patchew
  2. linux
  3. View series

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

Chen-Yu Tsai posted 1 patch 1 month, 1 week ago
There is a newer version of this series
drivers/thermal/thermal_of.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
[PATCH v3] thermal/of: support thermal zones w/o trips subnode
Posted by Chen-Yu Tsai 1 month, 1 week ago 
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>
---
Changes since v2:
- Stacked on top of Krzysztof's cleanup patches
  - thermal: of: Use scoped memory and OF handling to simplify thermal_of_trips_init() [1]
- Adjusted to account for eliminated error path

[1] https://lore.kernel.org/all/20241010-b4-cleanup-h-of-node-put-thermal-v4-2-bfbe29ad81f4@linaro.org/

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 | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
index 93f7c6f8d06d..be1fa6478c21 100644
--- a/drivers/thermal/thermal_of.c
+++ b/drivers/thermal/thermal_of.c
@@ -99,14 +99,14 @@ static struct thermal_trip *thermal_of_trips_init(struct device_node *np, int *n
 
 	struct device_node *trips __free(device_node) = 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");
-		return ERR_PTR(-EINVAL);
+		pr_debug("No trip point defined\n");
+		return ERR_PTR(-ENXIO);
 	}
 
 	struct thermal_trip *tt __free(kfree) = kzalloc(sizeof(*tt) * count, GFP_KERNEL);
@@ -386,9 +386,15 @@ 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);
-		ret = PTR_ERR(trips);
-		goto out_of_node_put;
+		if (PTR_ERR(trips) != -ENXIO) {
+			pr_err("Failed to find trip points for %pOFn id=%d\n", sensor, id);
+			ret = PTR_ERR(trips);
+			goto out_of_node_put;
+		}
+
+		pr_warn("Failed to find trip points for %pOFn id=%d\n", sensor, id);
+		trips = NULL;
+		ntrips = 0;
 	}
 
 	ret = thermal_of_monitor_init(np, &delay, &pdelay);
-- 
2.47.0.rc1.288.g06298d1525-goog
Re: [PATCH v3] thermal/of: support thermal zones w/o trips subnode
Posted by Rafael J. Wysocki 1 month, 1 week ago 
On Thu, Oct 17, 2024 at 11:05 AM Chen-Yu Tsai <wenst@chromium.org> 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>
> ---
> Changes since v2:
> - Stacked on top of Krzysztof's cleanup patches
>   - thermal: of: Use scoped memory and OF handling to simplify thermal_of_trips_init() [1]
> - Adjusted to account for eliminated error path
>
> [1] https://lore.kernel.org/all/20241010-b4-cleanup-h-of-node-put-thermal-v4-2-bfbe29ad81f4@linaro.org/
>
> 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 | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> index 93f7c6f8d06d..be1fa6478c21 100644
> --- a/drivers/thermal/thermal_of.c
> +++ b/drivers/thermal/thermal_of.c
> @@ -99,14 +99,14 @@ static struct thermal_trip *thermal_of_trips_init(struct device_node *np, int *n
>
>         struct device_node *trips __free(device_node) = 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);

Why not

*ntrips = 0;
return NULL;

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

Is this based on the current mainline code?

>         }
>
>         struct thermal_trip *tt __free(kfree) = kzalloc(sizeof(*tt) * count, GFP_KERNEL);
> @@ -386,9 +386,15 @@ 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);
> -               ret = PTR_ERR(trips);
> -               goto out_of_node_put;
> +               if (PTR_ERR(trips) != -ENXIO) {
> +                       pr_err("Failed to find trip points for %pOFn id=%d\n", sensor, id);
> +                       ret = PTR_ERR(trips);
> +                       goto out_of_node_put;
> +               }
> +
> +               pr_warn("Failed to find trip points for %pOFn id=%d\n", sensor, id);

Wouldn't pr_info() be sufficient for this?

> +               trips = NULL;
> +               ntrips = 0;
>         }
>
>         ret = thermal_of_monitor_init(np, &delay, &pdelay);
> --
Re: [PATCH v3] thermal/of: support thermal zones w/o trips subnode
Posted by Chen-Yu Tsai 1 month, 1 week ago 
On Thu, Oct 17, 2024 at 6:57 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Oct 17, 2024 at 11:05 AM Chen-Yu Tsai <wenst@chromium.org> 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>
> > ---
> > Changes since v2:
> > - Stacked on top of Krzysztof's cleanup patches
> >   - thermal: of: Use scoped memory and OF handling to simplify thermal_of_trips_init() [1]
> > - Adjusted to account for eliminated error path
> >
> > [1] https://lore.kernel.org/all/20241010-b4-cleanup-h-of-node-put-thermal-v4-2-bfbe29ad81f4@linaro.org/
> >
> > 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 | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> > index 93f7c6f8d06d..be1fa6478c21 100644
> > --- a/drivers/thermal/thermal_of.c
> > +++ b/drivers/thermal/thermal_of.c
> > @@ -99,14 +99,14 @@ static struct thermal_trip *thermal_of_trips_init(struct device_node *np, int *n
> >
> >         struct device_node *trips __free(device_node) = 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);
>
> Why not
>
> *ntrips = 0;
> return NULL;

That indeed seems cleaner.

> >         }
> >
> >         count = of_get_child_count(trips);
> >         if (!count) {
> > -               pr_err("No trip point defined\n");
> > -               return ERR_PTR(-EINVAL);
> > +               pr_debug("No trip point defined\n");
> > +               return ERR_PTR(-ENXIO);
>
> Is this based on the current mainline code?

This is based on Krzysztof's scoped memory patch:

  thermal: of: Use scoped memory and OF handling to simplify
thermal_of_trips_init()
  https://lore.kernel.org/all/20241010-b4-cleanup-h-of-node-put-thermal-v4-2-bfbe29ad81f4@linaro.org/

I should have made it more obvious in the footer.

> >         }
> >
> >         struct thermal_trip *tt __free(kfree) = kzalloc(sizeof(*tt) * count, GFP_KERNEL);
> > @@ -386,9 +386,15 @@ 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);
> > -               ret = PTR_ERR(trips);
> > -               goto out_of_node_put;
> > +               if (PTR_ERR(trips) != -ENXIO) {
> > +                       pr_err("Failed to find trip points for %pOFn id=%d\n", sensor, id);
> > +                       ret = PTR_ERR(trips);
> > +                       goto out_of_node_put;
> > +               }
> > +
> > +               pr_warn("Failed to find trip points for %pOFn id=%d\n", sensor, id);
>
> Wouldn't pr_info() be sufficient for this?

Works for me. I should also reword this and the existing error message
to be less confusing.


Thanks
ChenYu


> > +               trips = NULL;
> > +               ntrips = 0;
> >         }
> >
> >         ret = thermal_of_monitor_init(np, &delay, &pdelay);
> > --

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.