diff mbox

[07/16] Thermal: Introduce .get_trend() callback.

Message ID 1342679480-5336-8-git-send-email-rui.zhang@intel.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Zhang Rui July 19, 2012, 6:31 a.m. UTC
tc1 and tc2 are used by OSPM to anticipate the temperature trends.
But they are ACPI platform specific concepts.

Introduce .get_trend() as a more general solution.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/thermal.c        |   30 ++++++++++++++++++++++++++++++
 drivers/thermal/thermal_sys.c |   19 +++++++++++++++++--
 include/linux/thermal.h       |    9 +++++++++
 3 files changed, 56 insertions(+), 2 deletions(-)

Comments

Rafael Wysocki July 19, 2012, 9:13 p.m. UTC | #1
On Thursday, July 19, 2012, Zhang Rui wrote:
> tc1 and tc2 are used by OSPM to anticipate the temperature trends.
> But they are ACPI platform specific concepts.
> 
> Introduce .get_trend() as a more general solution.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/acpi/thermal.c        |   30 ++++++++++++++++++++++++++++++
>  drivers/thermal/thermal_sys.c |   19 +++++++++++++++++--
>  include/linux/thermal.h       |    9 +++++++++
>  3 files changed, 56 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> index a7c97f5..b345646 100644
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -704,6 +704,35 @@ static int thermal_get_crit_temp(struct thermal_zone_device *thermal,
>  		return -EINVAL;
>  }
>  
> +static int thermal_get_trend(struct thermal_zone_device *thermal,
> +				int trip, enum thermal_trend *trend)
> +{
> +	struct acpi_thermal *tz = thermal->devdata;
> +	enum thermal_trip_type type;
> +	unsigned long trip_temp;
> +	int i;
> +
> +	if (thermal_get_trip_type(thermal, trip, &type))
> +		return -EINVAL;
> +
> +	/* Only PASSIVE trip points need TREND */
> +	if (type != THERMAL_TRIP_PASSIVE)
> +		return -EINVAL;
> +
> +	/*
> +	 * tz->temperature has already been updated by generic thermal layer,
> +	 * before this callback being invoked
> +	 */
> +	i = (tz->trips.passive.tc1 * (tz->temperature - tz->last_temperature))
> +		+ (tz->trips.passive.tc2
> +		* (tz->temperature - tz->trips.passive.temperature));
> +
> +	*trend = i > 0 ? THERMAL_TREND_RAISING :
> +		(i < 0 ? THERMAL_TREND_DROPPING : THERMAL_TREND_STABLE);

I'd use if (...) / else if (...) / else here.  It would be _way_ more readable.

> +	return 0;
> +}
> +
> +
>  static int thermal_notify(struct thermal_zone_device *thermal, int trip,
>  			   enum thermal_trip_type trip_type)
>  {
> @@ -832,6 +861,7 @@ static const struct thermal_zone_device_ops acpi_thermal_zone_ops = {
>  	.get_trip_type = thermal_get_trip_type,
>  	.get_trip_temp = thermal_get_trip_temp,
>  	.get_crit_temp = thermal_get_crit_temp,
> +	.get_trend = thermal_get_trend,
>  	.notify = thermal_notify,
>  };
>  
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index db35300..29b6dba 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -698,6 +698,18 @@ thermal_remove_hwmon_sysfs(struct thermal_zone_device *tz)
>  }
>  #endif
>  
> +static void thermal_get_trend(struct thermal_zone_device *tz,
> +		int trip, enum thermal_trend *trend)

The fact that this function has the same name as the ACPI one above is kind of
disturbing.  Any chance to call it differently?

> +{
> +	if (tz->ops->get_trend)
> +		if (!tz->ops->get_trend(tz, trip, trend))
> +			return;

What about:

+	if (tz->ops->get_trend && !tz->ops->get_trend(tz, trip, trend))
+		return;

And since the error code returned by .get_trend() apparently doesn't matter,
shouldn't it return a bool?

> +
> +	*trend = tz->temperature >= tz->last_temperature ?
> +		 THERMAL_TREND_RAISING : THERMAL_TREND_DROPPING;
> +	return;
> +}
> +
>  static void thermal_zone_device_set_polling(struct thermal_zone_device *tz,
>  					    int delay)
>  {
> @@ -732,6 +744,8 @@ static void thermal_zone_device_passive(struct thermal_zone_device *tz,
>  	if (temp >= trip_temp) {
>  		tz->passive = true;
>  
> +		thermal_get_trend(tz, trip, (enum thermal_trend *)&trend);

What's wrong with the data type of 'trend' here?

> +
>  		trend = (tz->tc1 * (temp - tz->last_temperature)) +
>  			(tz->tc2 * (temp - trip_temp));
>  
> @@ -1090,6 +1104,9 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
>  		goto leave;
>  	}
>  
> +	tz->last_temperature = tz->temperature;
> +	tz->temperature = temp;
> +
>  	for (count = 0; count < tz->trips; count++) {
>  		tz->ops->get_trip_type(tz, count, &trip_type);
>  		tz->ops->get_trip_temp(tz, count, &trip_temp);
> @@ -1149,8 +1166,6 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
>  		thermal_zone_device_passive(tz, temp, tz->forced_passive,
>  					    THERMAL_TRIPS_NONE);
>  
> -	tz->last_temperature = temp;
> -

I'm not sure if this is correct.  It seems to change the behavior of
thermal_zone_device_passive() in a subtle way, but I'm not sure that really
matters.  Does it?

>  leave:
>  	if (tz->passive)
>  		thermal_zone_device_set_polling(tz, tz->passive_delay);
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index b355c62..ab656d6 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -44,6 +44,12 @@ enum thermal_trip_type {
>  	THERMAL_TRIP_CRITICAL,
>  };
>  
> +enum thermal_trend {
> +	THERMAL_TREND_STABLE, /* temperature is stable */
> +	THERMAL_TREND_RAISING, /* temperature is raising */
> +	THERMAL_TREND_DROPPING, /* temperature is dropping */
> +};
> +
>  struct thermal_zone_device_ops {
>  	int (*bind) (struct thermal_zone_device *,
>  		     struct thermal_cooling_device *);
> @@ -65,6 +71,8 @@ struct thermal_zone_device_ops {
>  	int (*set_trip_hyst) (struct thermal_zone_device *, int,
>  			      unsigned long);
>  	int (*get_crit_temp) (struct thermal_zone_device *, unsigned long *);
> +	int (*get_trend) (struct thermal_zone_device *, int,
> +			  enum thermal_trend *);
>  	int (*notify) (struct thermal_zone_device *, int,
>  		       enum thermal_trip_type);
>  };
> @@ -109,6 +117,7 @@ struct thermal_zone_device {
>  	int tc2;
>  	int passive_delay;
>  	int polling_delay;
> +	int temperature;
>  	int last_temperature;
>  	bool passive;
>  	unsigned int forced_passive;
> 

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jacob Pan July 19, 2012, 10:09 p.m. UTC | #2
On Thu, 19 Jul 2012 14:31:11 +0800
Zhang Rui <rui.zhang@intel.com> wrote:

> tc1 and tc2 are used by OSPM to anticipate the temperature trends.
> But they are ACPI platform specific concepts.
> 
> Introduce .get_trend() as a more general solution.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/acpi/thermal.c        |   30 ++++++++++++++++++++++++++++++
>  drivers/thermal/thermal_sys.c |   19 +++++++++++++++++--
>  include/linux/thermal.h       |    9 +++++++++
>  3 files changed, 56 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> index a7c97f5..b345646 100644
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -704,6 +704,35 @@ static int thermal_get_crit_temp(struct
> thermal_zone_device *thermal, return -EINVAL;
>  }
>  
> +static int thermal_get_trend(struct thermal_zone_device *thermal,
> +				int trip, enum thermal_trend *trend)
> +{
> +	struct acpi_thermal *tz = thermal->devdata;
> +	enum thermal_trip_type type;
> +	unsigned long trip_temp;
> +	int i;
> +
> +	if (thermal_get_trip_type(thermal, trip, &type))
> +		return -EINVAL;
> +
> +	/* Only PASSIVE trip points need TREND */
> +	if (type != THERMAL_TRIP_PASSIVE)
> +		return -EINVAL;
> +
> +	/*
> +	 * tz->temperature has already been updated by generic
> thermal layer,
> +	 * before this callback being invoked
> +	 */
> +	i = (tz->trips.passive.tc1 * (tz->temperature -
> tz->last_temperature))
> +		+ (tz->trips.passive.tc2
> +		* (tz->temperature - tz->trips.passive.temperature));
> +
> +	*trend = i > 0 ? THERMAL_TREND_RAISING :
> +		(i < 0 ? THERMAL_TREND_DROPPING :
> THERMAL_TREND_STABLE);
> +	return 0;
> +}
> +
> +
>  static int thermal_notify(struct thermal_zone_device *thermal, int
> trip, enum thermal_trip_type trip_type)
>  {
> @@ -832,6 +861,7 @@ static const struct thermal_zone_device_ops
> acpi_thermal_zone_ops = { .get_trip_type = thermal_get_trip_type,
>  	.get_trip_temp = thermal_get_trip_temp,
>  	.get_crit_temp = thermal_get_crit_temp,
> +	.get_trend = thermal_get_trend,
>  	.notify = thermal_notify,
>  };
>  
> diff --git a/drivers/thermal/thermal_sys.c
> b/drivers/thermal/thermal_sys.c index db35300..29b6dba 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -698,6 +698,18 @@ thermal_remove_hwmon_sysfs(struct
> thermal_zone_device *tz) }
>  #endif
>  
> +static void thermal_get_trend(struct thermal_zone_device *tz,
> +		int trip, enum thermal_trend *trend)
> +{
> +	if (tz->ops->get_trend)
> +		if (!tz->ops->get_trend(tz, trip, trend))
> +			return;
> +
> +	*trend = tz->temperature >= tz->last_temperature ?
> +		 THERMAL_TREND_RAISING : THERMAL_TREND_DROPPING;
I think you are mixing things a little here. ACPI passive cooling
formula generates a performance trend to guide thermal management,
which is not a simple temperature trend.

In case of no .get_trend() present, caller can easily figure out
temperature trend explicitly instead of getting uninformed information.

> +	return;
> +}
> +
>  static void thermal_zone_device_set_polling(struct
> thermal_zone_device *tz, int delay)
>  {
> @@ -732,6 +744,8 @@ static void thermal_zone_device_passive(struct
> thermal_zone_device *tz, if (temp >= trip_temp) {
>  		tz->passive = true;
>  
> +		thermal_get_trend(tz, trip, (enum thermal_trend
> *)&trend); +
>  		trend = (tz->tc1 * (temp - tz->last_temperature)) +
>  			(tz->tc2 * (temp - trip_temp));
>  
> @@ -1090,6 +1104,9 @@ void thermal_zone_device_update(struct
> thermal_zone_device *tz) goto leave;
>  	}
>  
> +	tz->last_temperature = tz->temperature;
> +	tz->temperature = temp;
> +
>  	for (count = 0; count < tz->trips; count++) {
>  		tz->ops->get_trip_type(tz, count, &trip_type);
>  		tz->ops->get_trip_temp(tz, count, &trip_temp);
> @@ -1149,8 +1166,6 @@ void thermal_zone_device_update(struct
> thermal_zone_device *tz) thermal_zone_device_passive(tz, temp,
> tz->forced_passive, THERMAL_TRIPS_NONE);
>  
> -	tz->last_temperature = temp;
> -
>  leave:
>  	if (tz->passive)
>  		thermal_zone_device_set_polling(tz,
> tz->passive_delay); diff --git a/include/linux/thermal.h
> b/include/linux/thermal.h index b355c62..ab656d6 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -44,6 +44,12 @@ enum thermal_trip_type {
>  	THERMAL_TRIP_CRITICAL,
>  };
>  
> +enum thermal_trend {
> +	THERMAL_TREND_STABLE, /* temperature is stable */
> +	THERMAL_TREND_RAISING, /* temperature is raising */
> +	THERMAL_TREND_DROPPING, /* temperature is dropping */
> +};
> +
>  struct thermal_zone_device_ops {
>  	int (*bind) (struct thermal_zone_device *,
>  		     struct thermal_cooling_device *);
> @@ -65,6 +71,8 @@ struct thermal_zone_device_ops {
>  	int (*set_trip_hyst) (struct thermal_zone_device *, int,
>  			      unsigned long);
>  	int (*get_crit_temp) (struct thermal_zone_device *, unsigned
> long *);
> +	int (*get_trend) (struct thermal_zone_device *, int,
> +			  enum thermal_trend *);
>  	int (*notify) (struct thermal_zone_device *, int,
>  		       enum thermal_trip_type);
>  };
> @@ -109,6 +117,7 @@ struct thermal_zone_device {
>  	int tc2;
>  	int passive_delay;
>  	int polling_delay;
> +	int temperature;
>  	int last_temperature;
>  	bool passive;
>  	unsigned int forced_passive;
Rafael Wysocki July 20, 2012, 9:53 a.m. UTC | #3
On Friday, July 20, 2012, Jacob Pan wrote:
> On Thu, 19 Jul 2012 14:31:11 +0800
> Zhang Rui <rui.zhang@intel.com> wrote:
> 
> > tc1 and tc2 are used by OSPM to anticipate the temperature trends.
> > But they are ACPI platform specific concepts.
> > 
> > Introduce .get_trend() as a more general solution.
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/acpi/thermal.c        |   30 ++++++++++++++++++++++++++++++
> >  drivers/thermal/thermal_sys.c |   19 +++++++++++++++++--
> >  include/linux/thermal.h       |    9 +++++++++
> >  3 files changed, 56 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> > index a7c97f5..b345646 100644
> > --- a/drivers/acpi/thermal.c
> > +++ b/drivers/acpi/thermal.c
> > @@ -704,6 +704,35 @@ static int thermal_get_crit_temp(struct
> > thermal_zone_device *thermal, return -EINVAL;
> >  }
> >  
> > +static int thermal_get_trend(struct thermal_zone_device *thermal,
> > +				int trip, enum thermal_trend *trend)
> > +{
> > +	struct acpi_thermal *tz = thermal->devdata;
> > +	enum thermal_trip_type type;
> > +	unsigned long trip_temp;
> > +	int i;
> > +
> > +	if (thermal_get_trip_type(thermal, trip, &type))
> > +		return -EINVAL;
> > +
> > +	/* Only PASSIVE trip points need TREND */
> > +	if (type != THERMAL_TRIP_PASSIVE)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * tz->temperature has already been updated by generic
> > thermal layer,
> > +	 * before this callback being invoked
> > +	 */
> > +	i = (tz->trips.passive.tc1 * (tz->temperature -
> > tz->last_temperature))
> > +		+ (tz->trips.passive.tc2
> > +		* (tz->temperature - tz->trips.passive.temperature));
> > +
> > +	*trend = i > 0 ? THERMAL_TREND_RAISING :
> > +		(i < 0 ? THERMAL_TREND_DROPPING :
> > THERMAL_TREND_STABLE);
> > +	return 0;
> > +}
> > +
> > +
> >  static int thermal_notify(struct thermal_zone_device *thermal, int
> > trip, enum thermal_trip_type trip_type)
> >  {
> > @@ -832,6 +861,7 @@ static const struct thermal_zone_device_ops
> > acpi_thermal_zone_ops = { .get_trip_type = thermal_get_trip_type,
> >  	.get_trip_temp = thermal_get_trip_temp,
> >  	.get_crit_temp = thermal_get_crit_temp,
> > +	.get_trend = thermal_get_trend,
> >  	.notify = thermal_notify,
> >  };
> >  
> > diff --git a/drivers/thermal/thermal_sys.c
> > b/drivers/thermal/thermal_sys.c index db35300..29b6dba 100644
> > --- a/drivers/thermal/thermal_sys.c
> > +++ b/drivers/thermal/thermal_sys.c
> > @@ -698,6 +698,18 @@ thermal_remove_hwmon_sysfs(struct
> > thermal_zone_device *tz) }
> >  #endif
> >  
> > +static void thermal_get_trend(struct thermal_zone_device *tz,
> > +		int trip, enum thermal_trend *trend)
> > +{
> > +	if (tz->ops->get_trend)
> > +		if (!tz->ops->get_trend(tz, trip, trend))
> > +			return;
> > +
> > +	*trend = tz->temperature >= tz->last_temperature ?
> > +		 THERMAL_TREND_RAISING : THERMAL_TREND_DROPPING;
> I think you are mixing things a little here. ACPI passive cooling
> formula generates a performance trend to guide thermal management,
> which is not a simple temperature trend.
> 
> In case of no .get_trend() present, caller can easily figure out
> temperature trend explicitly instead of getting uninformed information.

thermal_zone_device_passive() (below) is the caller and I don't think it
can really do that.

That said, since thermal_zone_device_passive() seems to me the only caller of
thermal_get_trend(), it might be better to move the code to that function
directly.

Thanks,
Rafael


> > +	return;
> > +}
> > +
> >  static void thermal_zone_device_set_polling(struct
> > thermal_zone_device *tz, int delay)
> >  {
> > @@ -732,6 +744,8 @@ static void thermal_zone_device_passive(struct
> > thermal_zone_device *tz, if (temp >= trip_temp) {
> >  		tz->passive = true;
> >  
> > +		thermal_get_trend(tz, trip, (enum thermal_trend
> > *)&trend); +
> >  		trend = (tz->tc1 * (temp - tz->last_temperature)) +
> >  			(tz->tc2 * (temp - trip_temp));
> >  
> > @@ -1090,6 +1104,9 @@ void thermal_zone_device_update(struct
> > thermal_zone_device *tz) goto leave;
> >  	}
> >  
> > +	tz->last_temperature = tz->temperature;
> > +	tz->temperature = temp;
> > +
> >  	for (count = 0; count < tz->trips; count++) {
> >  		tz->ops->get_trip_type(tz, count, &trip_type);
> >  		tz->ops->get_trip_temp(tz, count, &trip_temp);
> > @@ -1149,8 +1166,6 @@ void thermal_zone_device_update(struct
> > thermal_zone_device *tz) thermal_zone_device_passive(tz, temp,
> > tz->forced_passive, THERMAL_TRIPS_NONE);
> >  
> > -	tz->last_temperature = temp;
> > -
> >  leave:
> >  	if (tz->passive)
> >  		thermal_zone_device_set_polling(tz,
> > tz->passive_delay); diff --git a/include/linux/thermal.h
> > b/include/linux/thermal.h index b355c62..ab656d6 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -44,6 +44,12 @@ enum thermal_trip_type {
> >  	THERMAL_TRIP_CRITICAL,
> >  };
> >  
> > +enum thermal_trend {
> > +	THERMAL_TREND_STABLE, /* temperature is stable */
> > +	THERMAL_TREND_RAISING, /* temperature is raising */
> > +	THERMAL_TREND_DROPPING, /* temperature is dropping */
> > +};
> > +
> >  struct thermal_zone_device_ops {
> >  	int (*bind) (struct thermal_zone_device *,
> >  		     struct thermal_cooling_device *);
> > @@ -65,6 +71,8 @@ struct thermal_zone_device_ops {
> >  	int (*set_trip_hyst) (struct thermal_zone_device *, int,
> >  			      unsigned long);
> >  	int (*get_crit_temp) (struct thermal_zone_device *, unsigned
> > long *);
> > +	int (*get_trend) (struct thermal_zone_device *, int,
> > +			  enum thermal_trend *);
> >  	int (*notify) (struct thermal_zone_device *, int,
> >  		       enum thermal_trip_type);
> >  };
> > @@ -109,6 +117,7 @@ struct thermal_zone_device {
> >  	int tc2;
> >  	int passive_delay;
> >  	int polling_delay;
> > +	int temperature;
> >  	int last_temperature;
> >  	bool passive;
> >  	unsigned int forced_passive;
> 
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jacob Pan July 20, 2012, 4:12 p.m. UTC | #4
On Fri, 20 Jul 2012 11:53:41 +0200
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> On Friday, July 20, 2012, Jacob Pan wrote:
> > On Thu, 19 Jul 2012 14:31:11 +0800
> > Zhang Rui <rui.zhang@intel.com> wrote:
> > 
> > > tc1 and tc2 are used by OSPM to anticipate the temperature trends.
> > > But they are ACPI platform specific concepts.
> > > 
> > > Introduce .get_trend() as a more general solution.
> > > 
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > ---
> > >  drivers/acpi/thermal.c        |   30
> > > ++++++++++++++++++++++++++++++ drivers/thermal/thermal_sys.c |
> > > 19 +++++++++++++++++-- include/linux/thermal.h       |    9
> > > +++++++++ 3 files changed, 56 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> > > index a7c97f5..b345646 100644
> > > --- a/drivers/acpi/thermal.c
> > > +++ b/drivers/acpi/thermal.c
> > > @@ -704,6 +704,35 @@ static int thermal_get_crit_temp(struct
> > > thermal_zone_device *thermal, return -EINVAL;
> > >  }
> > >  
> > > +static int thermal_get_trend(struct thermal_zone_device *thermal,
> > > +				int trip, enum thermal_trend
> > > *trend) +{
> > > +	struct acpi_thermal *tz = thermal->devdata;
> > > +	enum thermal_trip_type type;
> > > +	unsigned long trip_temp;
> > > +	int i;
> > > +
> > > +	if (thermal_get_trip_type(thermal, trip, &type))
> > > +		return -EINVAL;
> > > +
> > > +	/* Only PASSIVE trip points need TREND */
> > > +	if (type != THERMAL_TRIP_PASSIVE)
> > > +		return -EINVAL;
> > > +
> > > +	/*
> > > +	 * tz->temperature has already been updated by generic
> > > thermal layer,
> > > +	 * before this callback being invoked
> > > +	 */
> > > +	i = (tz->trips.passive.tc1 * (tz->temperature -
> > > tz->last_temperature))
> > > +		+ (tz->trips.passive.tc2
> > > +		* (tz->temperature -
> > > tz->trips.passive.temperature)); +
> > > +	*trend = i > 0 ? THERMAL_TREND_RAISING :
> > > +		(i < 0 ? THERMAL_TREND_DROPPING :
> > > THERMAL_TREND_STABLE);
> > > +	return 0;
> > > +}
> > > +
> > > +
> > >  static int thermal_notify(struct thermal_zone_device *thermal,
> > > int trip, enum thermal_trip_type trip_type)
> > >  {
> > > @@ -832,6 +861,7 @@ static const struct thermal_zone_device_ops
> > > acpi_thermal_zone_ops = { .get_trip_type = thermal_get_trip_type,
> > >  	.get_trip_temp = thermal_get_trip_temp,
> > >  	.get_crit_temp = thermal_get_crit_temp,
> > > +	.get_trend = thermal_get_trend,
> > >  	.notify = thermal_notify,
> > >  };
> > >  
> > > diff --git a/drivers/thermal/thermal_sys.c
> > > b/drivers/thermal/thermal_sys.c index db35300..29b6dba 100644
> > > --- a/drivers/thermal/thermal_sys.c
> > > +++ b/drivers/thermal/thermal_sys.c
> > > @@ -698,6 +698,18 @@ thermal_remove_hwmon_sysfs(struct
> > > thermal_zone_device *tz) }
> > >  #endif
> > >  
> > > +static void thermal_get_trend(struct thermal_zone_device *tz,
> > > +		int trip, enum thermal_trend *trend)
> > > +{
> > > +	if (tz->ops->get_trend)
> > > +		if (!tz->ops->get_trend(tz, trip, trend))
> > > +			return;
> > > +
> > > +	*trend = tz->temperature >= tz->last_temperature ?
> > > +		 THERMAL_TREND_RAISING : THERMAL_TREND_DROPPING;
> > I think you are mixing things a little here. ACPI passive cooling
> > formula generates a performance trend to guide thermal management,
> > which is not a simple temperature trend.
> > 
> > In case of no .get_trend() present, caller can easily figure out
> > temperature trend explicitly instead of getting uninformed
> > information.
> 
> thermal_zone_device_passive() (below) is the caller and I don't think
> it can really do that.
> 
perhaps i did not make it clear, i meant in case caller wants to know
the simple temperature trend, it can keep the last reading and
timestamp to figure out trend.

Going back to the original point, even though TC1 TC2 are ACPI specific
time constants but they represents a generic characteristic of the
platform thermal dynamics. These are useful for userspace or governors
to do thermal control in general. perhaps, we can generalize TCx
paramenters instead. i.e. allow a set of thermal constants exposed from
ACPI/BIOS and treat ACPI passive formular as a simple governor. In the
future, more constants can be added to have a better control logic.
e.g. I have written a PID controller (userspace) to manage passive
cooling, it shows better result than the ACPI passive formula in terms
of responsiveness, overshoot etc. In case of PID, we need three
constants which i have to tune manually at this point.

> That said, since thermal_zone_device_passive() seems to me the only
> caller of thermal_get_trend(), it might be better to move the code to
> that function directly.
> 
> Thanks,
> Rafael
> 
> 
> > > +	return;
> > > +}
> > > +
> > >  static void thermal_zone_device_set_polling(struct
> > > thermal_zone_device *tz, int delay)
> > >  {
> > > @@ -732,6 +744,8 @@ static void thermal_zone_device_passive(struct
> > > thermal_zone_device *tz, if (temp >= trip_temp) {
> > >  		tz->passive = true;
> > >  
> > > +		thermal_get_trend(tz, trip, (enum thermal_trend
> > > *)&trend); +
> > >  		trend = (tz->tc1 * (temp -
> > > tz->last_temperature)) + (tz->tc2 * (temp - trip_temp));
> > >  
> > > @@ -1090,6 +1104,9 @@ void thermal_zone_device_update(struct
> > > thermal_zone_device *tz) goto leave;
> > >  	}
> > >  
> > > +	tz->last_temperature = tz->temperature;
> > > +	tz->temperature = temp;
> > > +
> > >  	for (count = 0; count < tz->trips; count++) {
> > >  		tz->ops->get_trip_type(tz, count, &trip_type);
> > >  		tz->ops->get_trip_temp(tz, count, &trip_temp);
> > > @@ -1149,8 +1166,6 @@ void thermal_zone_device_update(struct
> > > thermal_zone_device *tz) thermal_zone_device_passive(tz, temp,
> > > tz->forced_passive, THERMAL_TRIPS_NONE);
> > >  
> > > -	tz->last_temperature = temp;
> > > -
> > >  leave:
> > >  	if (tz->passive)
> > >  		thermal_zone_device_set_polling(tz,
> > > tz->passive_delay); diff --git a/include/linux/thermal.h
> > > b/include/linux/thermal.h index b355c62..ab656d6 100644
> > > --- a/include/linux/thermal.h
> > > +++ b/include/linux/thermal.h
> > > @@ -44,6 +44,12 @@ enum thermal_trip_type {
> > >  	THERMAL_TRIP_CRITICAL,
> > >  };
> > >  
> > > +enum thermal_trend {
> > > +	THERMAL_TREND_STABLE, /* temperature is stable */
> > > +	THERMAL_TREND_RAISING, /* temperature is raising */
> > > +	THERMAL_TREND_DROPPING, /* temperature is dropping */
> > > +};
> > > +
> > >  struct thermal_zone_device_ops {
> > >  	int (*bind) (struct thermal_zone_device *,
> > >  		     struct thermal_cooling_device *);
> > > @@ -65,6 +71,8 @@ struct thermal_zone_device_ops {
> > >  	int (*set_trip_hyst) (struct thermal_zone_device *, int,
> > >  			      unsigned long);
> > >  	int (*get_crit_temp) (struct thermal_zone_device *,
> > > unsigned long *);
> > > +	int (*get_trend) (struct thermal_zone_device *, int,
> > > +			  enum thermal_trend *);
> > >  	int (*notify) (struct thermal_zone_device *, int,
> > >  		       enum thermal_trip_type);
> > >  };
> > > @@ -109,6 +117,7 @@ struct thermal_zone_device {
> > >  	int tc2;
> > >  	int passive_delay;
> > >  	int polling_delay;
> > > +	int temperature;
> > >  	int last_temperature;
> > >  	bool passive;
> > >  	unsigned int forced_passive;
> > 
> > 
> > 
> > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi"
> in the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhang Rui July 24, 2012, 1:42 a.m. UTC | #5
On ?, 2012-07-19 at 23:13 +0200, Rafael J. Wysocki wrote:
> On Thursday, July 19, 2012, Zhang Rui wrote:
> > tc1 and tc2 are used by OSPM to anticipate the temperature trends.
> > But they are ACPI platform specific concepts.
> > 
> > Introduce .get_trend() as a more general solution.
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/acpi/thermal.c        |   30 ++++++++++++++++++++++++++++++
> >  drivers/thermal/thermal_sys.c |   19 +++++++++++++++++--
> >  include/linux/thermal.h       |    9 +++++++++
> >  3 files changed, 56 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> > index a7c97f5..b345646 100644
> > --- a/drivers/acpi/thermal.c
> > +++ b/drivers/acpi/thermal.c
> > @@ -704,6 +704,35 @@ static int thermal_get_crit_temp(struct thermal_zone_device *thermal,
> >  		return -EINVAL;
> >  }
> >  
> > +static int thermal_get_trend(struct thermal_zone_device *thermal,
> > +				int trip, enum thermal_trend *trend)
> > +{
> > +	struct acpi_thermal *tz = thermal->devdata;
> > +	enum thermal_trip_type type;
> > +	unsigned long trip_temp;
> > +	int i;
> > +
> > +	if (thermal_get_trip_type(thermal, trip, &type))
> > +		return -EINVAL;
> > +
> > +	/* Only PASSIVE trip points need TREND */
> > +	if (type != THERMAL_TRIP_PASSIVE)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * tz->temperature has already been updated by generic thermal layer,
> > +	 * before this callback being invoked
> > +	 */
> > +	i = (tz->trips.passive.tc1 * (tz->temperature - tz->last_temperature))
> > +		+ (tz->trips.passive.tc2
> > +		* (tz->temperature - tz->trips.passive.temperature));
> > +
> > +	*trend = i > 0 ? THERMAL_TREND_RAISING :
> > +		(i < 0 ? THERMAL_TREND_DROPPING : THERMAL_TREND_STABLE);
> 
> I'd use if (...) / else if (...) / else here.  It would be _way_ more readable.
> 
agreed.

> > +	return 0;
> > +}
> > +
> > +
> >  static int thermal_notify(struct thermal_zone_device *thermal, int trip,
> >  			   enum thermal_trip_type trip_type)
> >  {
> > @@ -832,6 +861,7 @@ static const struct thermal_zone_device_ops acpi_thermal_zone_ops = {
> >  	.get_trip_type = thermal_get_trip_type,
> >  	.get_trip_temp = thermal_get_trip_temp,
> >  	.get_crit_temp = thermal_get_crit_temp,
> > +	.get_trend = thermal_get_trend,
> >  	.notify = thermal_notify,
> >  };
> >  
> > diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> > index db35300..29b6dba 100644
> > --- a/drivers/thermal/thermal_sys.c
> > +++ b/drivers/thermal/thermal_sys.c
> > @@ -698,6 +698,18 @@ thermal_remove_hwmon_sysfs(struct thermal_zone_device *tz)
> >  }
> >  #endif
> >  
> > +static void thermal_get_trend(struct thermal_zone_device *tz,
> > +		int trip, enum thermal_trend *trend)
> 
> The fact that this function has the same name as the ACPI one above is kind of
> disturbing.  Any chance to call it differently?
> 
> > +{
> > +	if (tz->ops->get_trend)
> > +		if (!tz->ops->get_trend(tz, trip, trend))
> > +			return;
> 
> What about:
> 
> +	if (tz->ops->get_trend && !tz->ops->get_trend(tz, trip, trend))
> +		return;
> 
> And since the error code returned by .get_trend() apparently doesn't matter,
> shouldn't it return a bool?
> 
agreed.
> > +
> > +	*trend = tz->temperature >= tz->last_temperature ?
> > +		 THERMAL_TREND_RAISING : THERMAL_TREND_DROPPING;
> > +	return;
> > +}
> > +
> >  static void thermal_zone_device_set_polling(struct thermal_zone_device *tz,
> >  					    int delay)
> >  {
> > @@ -732,6 +744,8 @@ static void thermal_zone_device_passive(struct thermal_zone_device *tz,
> >  	if (temp >= trip_temp) {
> >  		tz->passive = true;
> >  
> > +		thermal_get_trend(tz, trip, (enum thermal_trend *)&trend);
> 
> What's wrong with the data type of 'trend' here?
> 
this is no functional changes in this patch.
the trend value returned by thermal_get_trend first is overridden by
tc1/tc2 formula below, so trend is still an integer at this time.

But you remind me that I should redefine trend as enum thermal_trend in
the next patch.

> > +
> >  		trend = (tz->tc1 * (temp - tz->last_temperature)) +
> >  			(tz->tc2 * (temp - trip_temp));
> >  

> > @@ -1090,6 +1104,9 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
> >  		goto leave;
> >  	}
> >  
Say, when the temperature is changed from 60C to 65C,

without this patch, tz->last_temperature is 60C because it is updated in
the previous thermal_zone_device_update() call.
with this patch, tz->temperature is 60C because it is the previous
"current" temperature.

> > +	tz->last_temperature = tz->temperature;
> > +	tz->temperature = temp;
> > +
we update here them to reflect the truth.

> >  	for (count = 0; count < tz->trips; count++) {
> >  		tz->ops->get_trip_type(tz, count, &trip_type);
> >  		tz->ops->get_trip_temp(tz, count, &trip_temp);
> > @@ -1149,8 +1166,6 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
> >  		thermal_zone_device_passive(tz, temp, tz->forced_passive,
> >  					    THERMAL_TRIPS_NONE);
> >  
> > -	tz->last_temperature = temp;
> > -
without this patch, tz->last_temperature is updated to 65C
with this patch, tz->last_temperature is still 60C and tz->temperature
is 65C.
that's why we need to update them in the beginning of
thermal_zone_device_update().
> 
> I'm not sure if this is correct.  It seems to change the behavior of
> thermal_zone_device_passive() in a subtle way, but I'm not sure that really
> matters.  Does it?
> 
so I think this will not change the behavior of
thermal_zone_device_passive.

thanks,
rui

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael Wysocki July 24, 2012, 9:22 a.m. UTC | #6
On Tuesday, July 24, 2012, Zhang Rui wrote:
> On ?, 2012-07-19 at 23:13 +0200, Rafael J. Wysocki wrote:
> > On Thursday, July 19, 2012, Zhang Rui wrote:
> > > tc1 and tc2 are used by OSPM to anticipate the temperature trends.
> > > But they are ACPI platform specific concepts.
> > > 
> > > Introduce .get_trend() as a more general solution.
> > > 
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > ---
> > >  drivers/acpi/thermal.c        |   30 ++++++++++++++++++++++++++++++
> > >  drivers/thermal/thermal_sys.c |   19 +++++++++++++++++--
> > >  include/linux/thermal.h       |    9 +++++++++
> > >  3 files changed, 56 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> > > index a7c97f5..b345646 100644
> > > --- a/drivers/acpi/thermal.c
> > > +++ b/drivers/acpi/thermal.c
> > > @@ -704,6 +704,35 @@ static int thermal_get_crit_temp(struct thermal_zone_device *thermal,
> > >  		return -EINVAL;
> > >  }
> > >  
> > > +static int thermal_get_trend(struct thermal_zone_device *thermal,
> > > +				int trip, enum thermal_trend *trend)
> > > +{
> > > +	struct acpi_thermal *tz = thermal->devdata;
> > > +	enum thermal_trip_type type;
> > > +	unsigned long trip_temp;
> > > +	int i;
> > > +
> > > +	if (thermal_get_trip_type(thermal, trip, &type))
> > > +		return -EINVAL;
> > > +
> > > +	/* Only PASSIVE trip points need TREND */
> > > +	if (type != THERMAL_TRIP_PASSIVE)
> > > +		return -EINVAL;
> > > +
> > > +	/*
> > > +	 * tz->temperature has already been updated by generic thermal layer,
> > > +	 * before this callback being invoked
> > > +	 */
> > > +	i = (tz->trips.passive.tc1 * (tz->temperature - tz->last_temperature))
> > > +		+ (tz->trips.passive.tc2
> > > +		* (tz->temperature - tz->trips.passive.temperature));
> > > +
> > > +	*trend = i > 0 ? THERMAL_TREND_RAISING :
> > > +		(i < 0 ? THERMAL_TREND_DROPPING : THERMAL_TREND_STABLE);
> > 
> > I'd use if (...) / else if (...) / else here.  It would be _way_ more readable.
> > 
> agreed.
> 
> > > +	return 0;
> > > +}
> > > +
> > > +
> > >  static int thermal_notify(struct thermal_zone_device *thermal, int trip,
> > >  			   enum thermal_trip_type trip_type)
> > >  {
> > > @@ -832,6 +861,7 @@ static const struct thermal_zone_device_ops acpi_thermal_zone_ops = {
> > >  	.get_trip_type = thermal_get_trip_type,
> > >  	.get_trip_temp = thermal_get_trip_temp,
> > >  	.get_crit_temp = thermal_get_crit_temp,
> > > +	.get_trend = thermal_get_trend,
> > >  	.notify = thermal_notify,
> > >  };
> > >  
> > > diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> > > index db35300..29b6dba 100644
> > > --- a/drivers/thermal/thermal_sys.c
> > > +++ b/drivers/thermal/thermal_sys.c
> > > @@ -698,6 +698,18 @@ thermal_remove_hwmon_sysfs(struct thermal_zone_device *tz)
> > >  }
> > >  #endif
> > >  
> > > +static void thermal_get_trend(struct thermal_zone_device *tz,
> > > +		int trip, enum thermal_trend *trend)
> > 
> > The fact that this function has the same name as the ACPI one above is kind of
> > disturbing.  Any chance to call it differently?
> > 
> > > +{
> > > +	if (tz->ops->get_trend)
> > > +		if (!tz->ops->get_trend(tz, trip, trend))
> > > +			return;
> > 
> > What about:
> > 
> > +	if (tz->ops->get_trend && !tz->ops->get_trend(tz, trip, trend))
> > +		return;
> > 
> > And since the error code returned by .get_trend() apparently doesn't matter,
> > shouldn't it return a bool?
> > 
> agreed.
> > > +
> > > +	*trend = tz->temperature >= tz->last_temperature ?
> > > +		 THERMAL_TREND_RAISING : THERMAL_TREND_DROPPING;
> > > +	return;
> > > +}
> > > +
> > >  static void thermal_zone_device_set_polling(struct thermal_zone_device *tz,
> > >  					    int delay)
> > >  {
> > > @@ -732,6 +744,8 @@ static void thermal_zone_device_passive(struct thermal_zone_device *tz,
> > >  	if (temp >= trip_temp) {
> > >  		tz->passive = true;
> > >  
> > > +		thermal_get_trend(tz, trip, (enum thermal_trend *)&trend);
> > 
> > What's wrong with the data type of 'trend' here?
> > 
> this is no functional changes in this patch.

Sure.

I was wondering if the explicit cast could be avoided.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index a7c97f5..b345646 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -704,6 +704,35 @@  static int thermal_get_crit_temp(struct thermal_zone_device *thermal,
 		return -EINVAL;
 }
 
+static int thermal_get_trend(struct thermal_zone_device *thermal,
+				int trip, enum thermal_trend *trend)
+{
+	struct acpi_thermal *tz = thermal->devdata;
+	enum thermal_trip_type type;
+	unsigned long trip_temp;
+	int i;
+
+	if (thermal_get_trip_type(thermal, trip, &type))
+		return -EINVAL;
+
+	/* Only PASSIVE trip points need TREND */
+	if (type != THERMAL_TRIP_PASSIVE)
+		return -EINVAL;
+
+	/*
+	 * tz->temperature has already been updated by generic thermal layer,
+	 * before this callback being invoked
+	 */
+	i = (tz->trips.passive.tc1 * (tz->temperature - tz->last_temperature))
+		+ (tz->trips.passive.tc2
+		* (tz->temperature - tz->trips.passive.temperature));
+
+	*trend = i > 0 ? THERMAL_TREND_RAISING :
+		(i < 0 ? THERMAL_TREND_DROPPING : THERMAL_TREND_STABLE);
+	return 0;
+}
+
+
 static int thermal_notify(struct thermal_zone_device *thermal, int trip,
 			   enum thermal_trip_type trip_type)
 {
@@ -832,6 +861,7 @@  static const struct thermal_zone_device_ops acpi_thermal_zone_ops = {
 	.get_trip_type = thermal_get_trip_type,
 	.get_trip_temp = thermal_get_trip_temp,
 	.get_crit_temp = thermal_get_crit_temp,
+	.get_trend = thermal_get_trend,
 	.notify = thermal_notify,
 };
 
diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index db35300..29b6dba 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -698,6 +698,18 @@  thermal_remove_hwmon_sysfs(struct thermal_zone_device *tz)
 }
 #endif
 
+static void thermal_get_trend(struct thermal_zone_device *tz,
+		int trip, enum thermal_trend *trend)
+{
+	if (tz->ops->get_trend)
+		if (!tz->ops->get_trend(tz, trip, trend))
+			return;
+
+	*trend = tz->temperature >= tz->last_temperature ?
+		 THERMAL_TREND_RAISING : THERMAL_TREND_DROPPING;
+	return;
+}
+
 static void thermal_zone_device_set_polling(struct thermal_zone_device *tz,
 					    int delay)
 {
@@ -732,6 +744,8 @@  static void thermal_zone_device_passive(struct thermal_zone_device *tz,
 	if (temp >= trip_temp) {
 		tz->passive = true;
 
+		thermal_get_trend(tz, trip, (enum thermal_trend *)&trend);
+
 		trend = (tz->tc1 * (temp - tz->last_temperature)) +
 			(tz->tc2 * (temp - trip_temp));
 
@@ -1090,6 +1104,9 @@  void thermal_zone_device_update(struct thermal_zone_device *tz)
 		goto leave;
 	}
 
+	tz->last_temperature = tz->temperature;
+	tz->temperature = temp;
+
 	for (count = 0; count < tz->trips; count++) {
 		tz->ops->get_trip_type(tz, count, &trip_type);
 		tz->ops->get_trip_temp(tz, count, &trip_temp);
@@ -1149,8 +1166,6 @@  void thermal_zone_device_update(struct thermal_zone_device *tz)
 		thermal_zone_device_passive(tz, temp, tz->forced_passive,
 					    THERMAL_TRIPS_NONE);
 
-	tz->last_temperature = temp;
-
 leave:
 	if (tz->passive)
 		thermal_zone_device_set_polling(tz, tz->passive_delay);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index b355c62..ab656d6 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -44,6 +44,12 @@  enum thermal_trip_type {
 	THERMAL_TRIP_CRITICAL,
 };
 
+enum thermal_trend {
+	THERMAL_TREND_STABLE, /* temperature is stable */
+	THERMAL_TREND_RAISING, /* temperature is raising */
+	THERMAL_TREND_DROPPING, /* temperature is dropping */
+};
+
 struct thermal_zone_device_ops {
 	int (*bind) (struct thermal_zone_device *,
 		     struct thermal_cooling_device *);
@@ -65,6 +71,8 @@  struct thermal_zone_device_ops {
 	int (*set_trip_hyst) (struct thermal_zone_device *, int,
 			      unsigned long);
 	int (*get_crit_temp) (struct thermal_zone_device *, unsigned long *);
+	int (*get_trend) (struct thermal_zone_device *, int,
+			  enum thermal_trend *);
 	int (*notify) (struct thermal_zone_device *, int,
 		       enum thermal_trip_type);
 };
@@ -109,6 +117,7 @@  struct thermal_zone_device {
 	int tc2;
 	int passive_delay;
 	int polling_delay;
+	int temperature;
 	int last_temperature;
 	bool passive;
 	unsigned int forced_passive;