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