| Message ID | 1342679480-5336-8-git-send-email-rui.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)