On 20/11/2018 11:38 PM, Thierry Reding wrote: > On Tue, Nov 20, 2018 at 05:11:17PM +0800, Wei Ni wrote: >> Add support for get_trend ops that allows soctherm >> sensors to be used with the step-wise governor. >> >> Signed-off-by: Wei Ni <wni@xxxxxxxxxx> >> --- >> drivers/thermal/tegra/soctherm.c | 34 ++++++++++++++++++++++++++++++++++ >> 1 file changed, 34 insertions(+) >> >> diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c >> index ed28110a3535..d2951fbe2b7c 100644 >> --- a/drivers/thermal/tegra/soctherm.c >> +++ b/drivers/thermal/tegra/soctherm.c >> @@ -488,9 +488,43 @@ static int tegra_thermctl_set_trip_temp(void *data, int trip, int temp) >> return 0; >> } >> >> +static int tegra_thermctl_get_trend(void *data, int trip, >> + enum thermal_trend *trend) >> +{ >> + struct tegra_thermctl_zone *zone = data; >> + struct thermal_zone_device *tz = zone->tz; >> + int trip_temp, temp, last_temp, ret; >> + >> + if (!tz) >> + return -EINVAL; >> + >> + ret = tz->ops->get_trip_temp(zone->tz, trip, &trip_temp); >> + if (ret) >> + return ret; >> + >> + mutex_lock(&tz->lock); >> + temp = tz->temperature; >> + last_temp = tz->last_temperature; >> + mutex_unlock(&tz->lock); >> + >> + if (temp > trip_temp) { >> + if (temp >= last_temp) >> + *trend = THERMAL_TREND_RAISING; >> + else >> + *trend = THERMAL_TREND_STABLE; >> + } else if (temp < trip_temp) { >> + *trend = THERMAL_TREND_DROPPING; >> + } else { >> + *trend = THERMAL_TREND_STABLE; >> + } >> + >> + return 0; >> +} > > This looks like a reimplementation of the get_tz_trend() helper. Is > seems like that helper already has everything we need. Perhaps this > isn't working because of-thermal installs of_thermal_get_trend(), a > function that returns -EINVAL if the driver doesn't implement the > ->get_trend() callback. 1. The get_tz_trend() helper can work, because it has: if (tz->emul_temperature || !tz->ops->get_trend || tz->ops->get_trend(tz, trip, &trend)) { ... } the tz->ops->get_trend is of_thermal_get_trend(). If without special get_trend(), it will return -EINVAL, so it will implement the if block to get the "trend". If we have the special get_trend(), then the of_thermal_get_trend() will return 0, so this helper will not implement the if block, it will get the "trend" from the special get_trend(). 2. There has a little difference between the helper and our special callback. The tegra_thermctl_get_trend() consider the trip_temp, but the get_tz_trend() helper didn't. > > Perhaps a better way would be to do something like this in > thermal_zone_of_add_sensor(): > > if (ops->get_trend) > tzd->ops->get_trend = of_thermal_get_trend; > > That's similar to how ->set_trips() and ->set_emul_temp() are set up > and should make sure that get_tz_trend() will do the right thing for > all drivers that don't implement a special ->get_trend(). As above description, I think the of_thermal_get_trend() already can handle this case, doesn't need to change. Wei. > > Thierry >