[PATCH] thermal: tegra: add get_trend ops

[PATCH] thermal: tegra: add get_trend ops

[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

Re: [PATCH] thermal: tegra: add get_trend ops

[Thierry Reding]

[-- 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 --]

Re: [PATCH] thermal: tegra: add get_trend ops

[Wei Ni]

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
> 

Re: [PATCH] thermal: tegra: add get_trend ops

[Wei Ni]

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
>>

Re: [PATCH] thermal: tegra: add get_trend ops

[Eduardo Valentin]

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
> > 

Re: [PATCH] thermal: tegra: add get_trend ops

[Wei Ni]

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
>>>

Re: [PATCH] thermal: tegra: add get_trend ops

[Wei Ni]

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
>>>>

Re: [PATCH] thermal: tegra: add get_trend ops

[Wei Ni]

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
>>>>>