| Message ID | 20240809070822.2835371-1-wenst@chromium.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| 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);
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);
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);