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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
