Re: [PATCH 44/44] cpufreq: pass suitable index instead of freq to cpufreq_driver->target()

On 10 August 2013 12:14, Viresh Kumar viresh.kumar@linaro.org wrote:

This tries to remove code redundancy from cpufreq driver by moving some common part of them to the core. Each driver calls cpufreq_frequency_table_target() to get a suitable index for a target frequency and then works on it. Its better to do this at core level before calling cpufreq driver and hence passing "index" instead of "target_freq and relation" to cpufreq_driver->target() routine.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 06f8671..4bf023d 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1628,7 +1628,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy, unsigned int target_freq, unsigned int relation) {

•   int retval = -EINVAL;
  

•   int retval = -EINVAL, index;
    unsigned int old_target_freq = target_freq;
  
    if (cpufreq_disabled())
  

@@ -1645,11 +1645,35 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy, pr_debug("target for CPU %u: %u kHz, relation %u, requested %u kHz\n", policy->cpu, target_freq, relation, old_target_freq);

•   /*
  

•    * This might look like a redundant call as we are checking it again
  

•    * after finding index. But it is left intentionally for cases where
  

•    * same freq is called again and so we can save on few function calls.
  

•    */
    if (target_freq == policy->cur)
            return 0;
  
  

•   if (cpufreq_driver->target)
  

•           retval = cpufreq_driver->target(policy, target_freq, relation);
  

•   if (cpufreq_driver->target) {
  

•           struct cpufreq_frequency_table *freq_table;
  

•           freq_table = cpufreq_frequency_get_table(policy->cpu);
  

•           if (unlikely(!freq_table)) {
  

•                   pr_err("%s: Unable to find freq_table\n", __func__);
  

•                   return retval;
  

•           }
  

•           retval = cpufreq_frequency_table_target(policy, freq_table,
  

•                           target_freq, relation, &index);
  

•           if (unlikely(retval)) {
  

•                   pr_err("%s: Unable to find matching freq\n", __func__);
  

•                   return retval;
  

•           }
  

•           if (freq_table[index].frequency == policy->cur)
  

•                   return 0;
  

•           retval = cpufreq_driver->target(policy, index, relation);
  

•   }
  
    return retval;
  

}

Hi Rafael,

This was sent earlier by mistake but lets take advantage of that mistake now :)

I wanted to discuss what's the right way to get this patch in..

Idea: was to simplify ->target() routines of drivers by finding the right index prior to calling ->target().. And so the parameters would be changed to:

int (*target) (struct cpufreq_policy *policy, - unsigned int target_freq, - unsigned int relation); + unsigned int index)

Problem: Some drivers that implement ->target() doesn't have a freq table with them and so they never call cpufreq_frequency_table_target to get index and so passing Index for them is irrelevant.

What can we do here? Solution 1: Define two types of ->target() one with above mentioned prototype and other with the older style, And so drivers can initialize one of them..

Issues: Two pointers for same work, doesn't look clean enough

Solution 2: Change prototype to something like this: int (*target) (struct cpufreq_policy *policy, unsigned int target_freq, + unsigned int index, unsigned int relation);

Here, - target_freq: will have the freq requested (not the freq from table) - index: index of table suitable for this freq (for drivers exposing freq_table) and will be -1 for others, like: at32, pcc, unicore2.. - relation will stay as it is..

Issues: Not all parameters are useful for everybody.. Most of the drivers wouldn't use target_freq or relation..

Which one do you like?

-- viresh