Re: [PATCH 09/16] Thermal: Introduce thermal_zone_trip_update()

From: "Rafael J. Wysocki" <rjw@sisk.pl>
To: Zhang Rui <rui.zhang@intel.com>
Cc: Amit Kachhap <amit.kachhap@linaro.org>,
	linux-acpi@vger.kernel.org, linux-pm@vger.kernel.org,
	Matthew Garrett <mjg@redhat.com>, Len Brown <lenb@kernel.org>,
	R Durgadoss <durgadoss.r@intel.com>, Wei Ni <wni@nvidia.com>
Subject: Re: [PATCH 09/16] Thermal: Introduce thermal_zone_trip_update()
Date: Tue, 24 Jul 2012 11:27:42 +0200	[thread overview]
Message-ID: <201207241127.43086.rjw@sisk.pl> (raw)
In-Reply-To: <1343094471.1682.354.camel@rui.sh.intel.com>

On Tuesday, July 24, 2012, Zhang Rui wrote:
> On 四, 2012-07-19 at 23:19 +0200, Rafael J. Wysocki wrote:
> > On Thursday, July 19, 2012, Zhang Rui wrote:
> > > This function is used to update the cooling state of
> > > all the cooling devices that are binded to an active trip point.
> > 
> > s/binded/bound/
> > 
> got it.
> 
> > > This will be used for passive cooling as well, in the future patches.
> > > as both active and passive cooling can share the same algorithm,
> > > which is
> > > 
> > > 1. if the temperature is higher than a trip point,
> > >    a. if the trend is THERMAL_TREND_RAISING, use higher cooling
> > >       state for this trip point
> > >    b. if the trend is THERMAL_TREND_DROPPING, use lower cooling
> > >       state for this trip point
> > > 
> > > 2. if the temperature is lower than a trip point, use lower
> > >    cooling state for this trip point.
> > > 
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > ---
> > >  drivers/acpi/thermal.c        |    7 +++-
> > >  drivers/thermal/thermal_sys.c |   91 +++++++++++++++++++++++++++++------------
> > >  2 files changed, 71 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> > > index 73e335f..14c4879 100644
> > > --- a/drivers/acpi/thermal.c
> > > +++ b/drivers/acpi/thermal.c
> > > @@ -715,7 +715,12 @@ static int thermal_get_trend(struct thermal_zone_device *thermal,
> > >  	if (thermal_get_trip_type(thermal, trip, &type))
> > >  		return -EINVAL;
> > >  
> > > -	/* Only PASSIVE trip points need TREND */
> > > +	if (type == THERMAL_TRIP_ACTIVE) {
> > > +		/* aggressive active cooling */
> > > +		*trend = THERMAL_TREND_RAISING;
> > > +		return 0;
> > 
> > Please move that into thermal_zone_trip_update() directly, unless you
> > need it elsewhere.
> > 
> IMO, I can say that ACPI thermal wants aggressive active cooling, as it
> will never spin down the fan when the temperature is higher than the
> trip point.

I meant the code organization, not the functionality. :-)

Since thermal_get_trend() is static, it is not used outside of this file,
so you can move the "if (type == THERMAL_TRIP_ACTIVE)" conditional from it
directly into the caller (unless there are more callers, which I'm not sure
about without reading the whole code again).

That would make the code cleaner, because right now the condition is
hidden in thermal_get_trend() and it may not be clear to the reader of
thermal_zone_trip_update() that it is actually checked.

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