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

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

On Wednesday, July 25, 2012, Zhang Rui wrote:
> On 二, 2012-07-24 at 11:27 +0200, Rafael J. Wysocki wrote:
> > 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. :-)
> > 
> sure.
> 
> > 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).
> > 
> sorry I still do not get it.
> the generic thermal layer is the caller of this callback.

I'm not talking about the callback, but of the static function with the
same name you've introduced into drivers/thermal/thermal_sys.c in the
previous patch. :-)

The only caller of this function seems to be thermal_zone_device_passive()
and my point is that it would be better to simply put the code from that
function into thermal_zone_device_passive() directly, unless there are
more callers added by the subsequent patches, which I'm not sure about.

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