* [PATCH] thermal: tegra: add get_trend ops @ 2018-11-20 9:11 Wei Ni 2018-11-20 15:38 ` Thierry Reding 0 siblings, 1 reply; 8+ messages in thread From: Wei Ni @ 2018-11-20 9:11 UTC (permalink / raw) To: thierry.reding, daniel.lezcano, linux-tegra Cc: rui.zhang, edubezval, linux-kernel, Wei Ni Add support for get_trend ops that allows soctherm sensors to be used with the step-wise governor. Signed-off-by: Wei Ni <wni@nvidia.com> --- 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; +} + static const struct thermal_zone_of_device_ops tegra_of_thermal_ops = { .get_temp = tegra_thermctl_get_temp, .set_trip_temp = tegra_thermctl_set_trip_temp, + .get_trend = tegra_thermctl_get_trend, }; static int get_hot_temp(struct thermal_zone_device *tz, int *trip, int *temp) -- 2.7.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] thermal: tegra: add get_trend ops 2018-11-20 9:11 [PATCH] thermal: tegra: add get_trend ops Wei Ni @ 2018-11-20 15:38 ` Thierry Reding 2018-11-21 6:36 ` Wei Ni 0 siblings, 1 reply; 8+ messages in thread From: Thierry Reding @ 2018-11-20 15:38 UTC (permalink / raw) To: Wei Ni; +Cc: daniel.lezcano, linux-tegra, rui.zhang, edubezval, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2168 bytes --] 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@nvidia.com> > --- > 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. 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(). Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] thermal: tegra: add get_trend ops 2018-11-20 15:38 ` Thierry Reding @ 2018-11-21 6:36 ` Wei Ni 2018-11-28 9:41 ` Wei Ni 2018-11-29 17:01 ` Eduardo Valentin 0 siblings, 2 replies; 8+ messages in thread From: Wei Ni @ 2018-11-21 6:36 UTC (permalink / raw) To: Thierry Reding Cc: daniel.lezcano, linux-tegra, rui.zhang, edubezval, linux-kernel 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@nvidia.com> >> --- >> 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] thermal: tegra: add get_trend ops 2018-11-21 6:36 ` Wei Ni @ 2018-11-28 9:41 ` Wei Ni 2018-11-29 17:01 ` Eduardo Valentin 1 sibling, 0 replies; 8+ messages in thread From: Wei Ni @ 2018-11-28 9:41 UTC (permalink / raw) To: Thierry Reding Cc: daniel.lezcano, linux-tegra, rui.zhang, edubezval, linux-kernel Hi all, Does there have any more comments for this patch? Thanks. Wei. On 21/11/2018 2:36 PM, Wei Ni wrote: > > > 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@nvidia.com> >>> --- >>> 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 >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] thermal: tegra: add get_trend ops 2018-11-21 6:36 ` Wei Ni 2018-11-28 9:41 ` Wei Ni @ 2018-11-29 17:01 ` Eduardo Valentin 2018-11-30 3:07 ` Wei Ni 1 sibling, 1 reply; 8+ messages in thread From: Eduardo Valentin @ 2018-11-29 17:01 UTC (permalink / raw) To: Wei Ni Cc: Thierry Reding, daniel.lezcano, linux-tegra, rui.zhang, linux-kernel On Wed, Nov 21, 2018 at 02:36:10PM +0800, Wei Ni wrote: > > > 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@nvidia.com> > >> --- > >> 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(). The idea of the helper is to provide a trend in case drivers dont have a specific way of doing so. > > 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. > Yeah, if you are computing trend towards a trip, then yes, that is different and this patch is needed. > > > > 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 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] thermal: tegra: add get_trend ops 2018-11-29 17:01 ` Eduardo Valentin @ 2018-11-30 3:07 ` Wei Ni 2018-12-05 8:30 ` Wei Ni 0 siblings, 1 reply; 8+ messages in thread From: Wei Ni @ 2018-11-30 3:07 UTC (permalink / raw) To: Eduardo Valentin Cc: Thierry Reding, daniel.lezcano, linux-tegra, rui.zhang, linux-kernel On 30/11/2018 1:01 AM, Eduardo Valentin wrote: > On Wed, Nov 21, 2018 at 02:36:10PM +0800, Wei Ni wrote: >> >> >> 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@nvidia.com> >>>> --- >>>> 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(). > > The idea of the helper is to provide a trend in case drivers dont have > a specific way of doing so. Yes, thanks for your explanation. > >> >> 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. >> > > Yeah, if you are computing trend towards a trip, then yes, that is > different and this patch is needed. > >>> >>> 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 >>> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] thermal: tegra: add get_trend ops 2018-11-30 3:07 ` Wei Ni @ 2018-12-05 8:30 ` Wei Ni 2018-12-11 6:20 ` Wei Ni 0 siblings, 1 reply; 8+ messages in thread From: Wei Ni @ 2018-12-05 8:30 UTC (permalink / raw) To: Eduardo Valentin Cc: Thierry Reding, daniel.lezcano, linux-tegra, rui.zhang, linux-kernel Hi Daniel, It seems no more comments, could this patch be approved? Thanks. Wei. On 30/11/2018 11:07 AM, Wei Ni wrote: > > > On 30/11/2018 1:01 AM, Eduardo Valentin wrote: >> On Wed, Nov 21, 2018 at 02:36:10PM +0800, Wei Ni wrote: >>> >>> >>> 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@nvidia.com> >>>>> --- >>>>> 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(). >> >> The idea of the helper is to provide a trend in case drivers dont have >> a specific way of doing so. > > Yes, thanks for your explanation. > >> >>> >>> 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. >>> >> >> Yeah, if you are computing trend towards a trip, then yes, that is >> different and this patch is needed. >> >>>> >>>> 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 >>>> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] thermal: tegra: add get_trend ops 2018-12-05 8:30 ` Wei Ni @ 2018-12-11 6:20 ` Wei Ni 0 siblings, 0 replies; 8+ messages in thread From: Wei Ni @ 2018-12-11 6:20 UTC (permalink / raw) To: Eduardo Valentin, daniel.lezcano, rui.zhang Cc: Thierry Reding, linux-tegra, linux-kernel Hi Rui & Eduardo, Could you please take this patch? Thanks. Wei. On 5/12/2018 4:30 PM, Wei Ni wrote: > Hi Daniel, > It seems no more comments, could this patch be approved? > > Thanks. > Wei. > > On 30/11/2018 11:07 AM, Wei Ni wrote: >> >> >> On 30/11/2018 1:01 AM, Eduardo Valentin wrote: >>> On Wed, Nov 21, 2018 at 02:36:10PM +0800, Wei Ni wrote: >>>> >>>> >>>> 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@nvidia.com> >>>>>> --- >>>>>> 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(). >>> >>> The idea of the helper is to provide a trend in case drivers dont have >>> a specific way of doing so. >> >> Yes, thanks for your explanation. >> >>> >>>> >>>> 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. >>>> >>> >>> Yeah, if you are computing trend towards a trip, then yes, that is >>> different and this patch is needed. >>> >>>>> >>>>> 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 >>>>> ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-12-11 6:20 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-20 9:11 [PATCH] thermal: tegra: add get_trend ops Wei Ni 2018-11-20 15:38 ` Thierry Reding 2018-11-21 6:36 ` Wei Ni 2018-11-28 9:41 ` Wei Ni 2018-11-29 17:01 ` Eduardo Valentin 2018-11-30 3:07 ` Wei Ni 2018-12-05 8:30 ` Wei Ni 2018-12-11 6:20 ` Wei Ni
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).