| Message ID | 20241017090503.1006068-1-wenst@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] thermal/of: support thermal zones w/o trips subnode | expand |
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); > --
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); > > --
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);