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

["Rafael J. Wysocki" <rjw@sisk.pl>]

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