Date: Mon, 14 Nov 2016 12:34:02 +0530
From: Viresh Kumar <viresh.kumar@linaro.org>
To: Stratos Karafotis <stratosk@semaphore.gr>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] cpufreq: conservative: Decrease frequency faster when
 the update deferred
Message-ID: <20161114070402.GC4178@vireshk-i7>
References: <691e286d-249b-e450-2df1-8421d83e6a46@semaphore.gr>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <691e286d-249b-e450-2df1-8421d83e6a46@semaphore.gr>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1224
Lines: 40

On 12-11-16, 23:04, Stratos Karafotis wrote:
> diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
> index fa5ece3..d787772 100644
> --- a/drivers/cpufreq/cpufreq_conservative.c
> +++ b/drivers/cpufreq/cpufreq_conservative.c
> @@ -73,7 +73,19 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
>  	 */
>  	if (cs_tuners->freq_step == 0)
>  		goto out;
> -
> +	/*
> +	 * Decrease requested_freq for each idle period that we didn't
> +	 * update the frequency

Add a full stop (.) here.

> +	 */

I am wondering if we should be adding this code after the below 'if' block where
we check if the policy->min/max have changed ?

> +	if (policy_dbs->idle_periods < UINT_MAX) {
> +		unsigned int freq_target = policy_dbs->idle_periods *
> +				get_freq_target(cs_tuners, policy);

I get confused every time I look at this routine (get_freq_target()). I have
sent an update to this file just now to get that fixed. If Rafael applies that
one, please rebase over it.

> +		if (requested_freq > freq_target)
> +			requested_freq -= freq_target;
> +		else
> +			requested_freq = policy->min;
> +		policy_dbs->idle_periods = UINT_MAX;
> +	}

Need a blank line here.

-- 
viresh