Re: [PATCH 3/3] Thermal:core: Handle trips focused on current trip point only.

[Zhang Rui <rui.zhang@intel.com>]

On Tue, 2013-05-21 at 12:40 +0900, jonghwa3.lee@samsung.com wrote:
> On 2013년 05월 21일 01:00, Zhang, Rui wrote:
> 
> > 
> > 
> >> -----Original Message-----
> >> From: Jonghwa Lee [mailto:jonghwa3.lee@samsung.com]
> >> Sent: Saturday, May 18, 2013 5:51 PM
> >> To: linux-pm@vger.kernel.org
> >> Cc: linux-kernel@vger.kernel.org; Zhang, Rui; Eduardo Valentin; Amit
> >> Dinel Kachhap; Jonghwa Lee; MyungJoo Ham
> >> Subject: [PATCH 3/3] Thermal:core: Handle trips focused on current trip
> >> point only.
> >> Importance: High
> >>
> >> When thermal zone device is updated, it doesn't need to check every
> >> trip points and its handling mathod even current temperature doesn't
> >> exceed the trip's temperature. To modify those dissipatve mechanism,
> >> this patch introduces the way to get current thermal trip point to call
> >> only correspond trip point handling.
> >>
> >> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
> >> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> > 
> > NAK.
> > 
> >> ---
> >>  drivers/thermal/thermal_core.c |   28 +++++++++++++++++-----------
> >>  1 file changed, 17 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/thermal/thermal_core.c
> >> b/drivers/thermal/thermal_core.c index ce4384a..1cc4825 100644
> >> --- a/drivers/thermal/thermal_core.c
> >> +++ b/drivers/thermal/thermal_core.c
> >> @@ -333,14 +333,6 @@ static void handle_non_critical_trips(struct
> >> thermal_zone_device *tz,  static void handle_critical_trips(struct
> >> thermal_zone_device *tz,
> >>  				int trip, enum thermal_trip_type trip_type)  {
> >> -	long trip_temp;
> >> -
> >> -	tz->ops->get_trip_temp(tz, trip, &trip_temp);
> >> -
> >> -	/* If we have not crossed the trip_temp, we do not care. */
> >> -	if (tz->temperature < trip_temp)
> >> -		return;
> >> -
> >>  	if (tz->ops->notify)
> >>  		tz->ops->notify(tz, trip, trip_type);
> >>
> >> @@ -437,14 +429,28 @@ static void update_temperature(struct
> >> thermal_zone_device *tz)
> >>  	mutex_unlock(&tz->lock);
> >>  }
> >>
> >> +static int thermal_zone_get_current_trip(struct thermal_zone_device
> >> +*tz) {
> >> +	int trip;
> >> +	long trip_temp;
> >> +
> >> +	for (trip = tz->trips - 1; trip > 0; trip--) {
> >> +		tz->ops->get_trip_temp(tz, trip, &trip_temp);
> >> +		if (tz->temperature > trip_temp)
> >> +			continue;
> >> +	}
> >> +	return trip;
> >> +}
> >> +
> >>  void thermal_zone_device_update(struct thermal_zone_device *tz)  {
> >> -	int count;
> >> +	int trip;
> >>
> >>  	update_temperature(tz);
> >>
> >> -	for (count = 0; count < tz->trips; count++)
> >> -		handle_thermal_trip(tz, count);
> >> +	trip = thermal_zone_get_current_trip(tz);
> >> +
> >> +	handle_thermal_trip(tz, trip);
> > 
> > Say, trip point 1 for thermal zone 0 is 60C,
> > The system is running above 60C for somethime,
> > thus the thermal_instance for this trip point is running at upper_limit.
> > When the temperature suddenly drops below 60C,
> > we still need to handle trip point 1 to deactivate it.
> > 
> 
> 
> Okay, I understood. I missed the point that governor will handle a cooling
> device within certain trip point described in thermal instance.
> But still I don't think this is the best behaviour. Let say we were in trip
> level 2nd and moving to trip level 1st then we should call governor twice for
> applying trip 1 level.

Right.
IMO, the governor is used to get the next proper cooling state for each
referred thermal instance.
So I think it is okay to call it twice.

>  Why don't we just call once? And whenever we call
> handle_thermal_trip() with all trips, monitor_thermal_work() will also be called
> at the same time.

hmm, what is your question about this?

>  I think we can make this work more clearly and intuitively.
> let me think of it more,,,
> 
sure.

thanks,
rui

> Thanks,
> Jonghwa.
> 
> > Thanks,
> > rui
> >>  }
> >>  EXPORT_SYMBOL_GPL(thermal_zone_device_update);
> >>
> >> --
> >> 1.7.9.5
> > 
> > 
> 
>