On 10 August 2013 12:14, Viresh Kumar <viresh.kumar@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
