Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Tue, 27 Sep 2022 12:38:42 +0200 | Subject | Re: [PATCH v5 03/30] thermal/core: Add a generic thermal_zone_set_trip() function |
| |
On Tue, Sep 27, 2022 at 12:11 AM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 26/09/2022 21:25, Rafael J. Wysocki wrote: > > [ ... ] > > >> + if ((t.temperature != trip->temperature) && tz->ops->set_trip_temp) { > > > > The inner parens are not needed here and below. > > > >> + > > > > And the extra empty line is not needed here (and below) too IMO. > > > >> + ret = tz->ops->set_trip_temp(tz, trip_id, trip->temperature); > >> + if (ret) > >> + goto out; > >> + } > > > > Without the parens, the following happens: > > > warning: this ‘if’ clause does not guard... [-Wmisleading-indentation] > 1229 | if ((t.temperature != trip->temperature) && > tz->ops->set_trip_temp) > | ^~ > note: ...this statement, but the latter is misleadingly indented as if > it were guarded by the ‘if’ > 1231 | if (ret) > | ^~
This is about indentation, though, so it looks like white space is mangled somehow.
As a matter of correctness, the inner parens are not needed.
> >> + if ((t.hysteresis != trip->hysteresis) && tz->ops->set_trip_hyst) { > >> + > >> + ret = tz->ops->set_trip_hyst(tz, trip_id, trip->hysteresis); > >> + if (ret) > >> + goto out; > >> + } > >> + > >> + if (((t.temperature != trip->temperature) || > >> + (t.hysteresis != trip->hysteresis)) && tz->trips) > >> + tz->trips[trip_id] = *trip; > > > > I would write this as > > > > if (tz->trips && (t.temperature != trip->temperature || t.hysteresis > > != trip->hysteresis)) > > tz->trips[trip_id] = *trip; > > Ok, sure > > > But > > > > 1. Do we want to copy the trip type here too? > > The function thermal_zone_set_trip() is called from thermal_sysfs.c, it > is the unique call site. However, I think it is a good idea to check the > type of the trip point is not changed, even if it is not possible with > the actual code. > > > 2. If tz->trips is set, do we still want to invoke ->set_trip_temp() > > or ->set_trip_hyst() if they are present? > > No but there are bogus drivers setting the interrupt with these ops > instead of using the set_trips ops (eg. [1][2][3]). So in order to keep > those working ATM, I'm keeping them and when all the drivers will be > changed, I'll wipe out the set_trip_* ops from everywhere.
Do those drivers set tz->trips? If not, the tz->trips check can go before the ops ones.
> [1] drivers/thermal/samsung/exynos_tmu.c > [2] drivers/thermal/qcom/qcom-spmi-temp-alarm.c > [3] drivers/thermal/imx_thermal.c
| |