Messages in this thread | | | Date | Wed, 21 May 2014 09:44:53 +0530 | Subject | Re: [PATCH V3 2/4] cpufreq: add support for intermediate (stable) frequencies | From | Viresh Kumar <> |
| |
Doug,
On 20 May 2014 22:18, Doug Anderson <dianders@chromium.org> wrote:
> Is it worth documenting that if we implement target_intermediate() > that target_index() must not fail? That means that any failure-prone > things (like setting a regulator) should happen in target_index().
You meant target_intermediate() is the last line, right ?
Yeah, we can add that..
>> 2. Frequency Table Helpers >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index 9bf12a2..f38f2f2 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -1819,27 +1819,50 @@ EXPORT_SYMBOL(cpufreq_unregister_notifier); >> static int __target_index(struct cpufreq_policy *policy, >> struct cpufreq_frequency_table *freq_table, int index) >> { >> - struct cpufreq_freqs freqs; >> + struct cpufreq_freqs freqs = {.old = policy->cur, .flags = 0}; >> int retval = -EINVAL; >> bool notify; >> >> notify = !(cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION); >> + if (!notify) >> + goto skip_notify; > > I'm personally not a fan of using goto here. All you're trying to do > is avoiding a level of indentation? If it really matters that much > then create a sub function. IMHO goto should generally be reserved > for error handling.
Yeah, I was trying that :) .. Another routine wouldn't be right here as the POST_NOTIFICATION will be handled in this routine only.
I will try again to see if I can write some better code here, but wouldn't promise that :)
>> - if (notify) { >> - freqs.old = policy->cur; >> - freqs.new = freq_table[index].frequency; >> - freqs.flags = 0; >> + /* Handle switching to intermediate frequency */ >> + if (cpufreq_driver->get_intermediate) { >> + freqs.new = cpufreq_driver->get_intermediate(policy, index); > > Would it be worth it to change this to? > > intermediate = 0 > if (cpufreq_driver->get_intermediate) > intermediate = cpufreq_driver->get_intermediate(); > if (intermediate) > > ...the idea being that a driver may use an intermediate frequency for > some transitions but not for all. For instance: on tegra if you > happen to change to the exact clock frequency of the intermediate PLL > it just stays there. There's no need for two notifications in that > case. There may be other systems that can optimize some transitions > to totally skip the intermediate stage (maybe you've got an > non-glitching divider somewhere so you can optimize a transition from > 1.4GHz to 700MHz to go w/ no intermediate jump).
Hmm, will try to fix that as well. Looks like a valid point.
>> - pr_debug("%s: cpu: %d, oldfreq: %u, new freq: %u\n", >> + pr_debug("%s: cpu: %d, switching to intermediate freq: oldfreq: %u, intermediate freq: %u\n", >> __func__, policy->cpu, freqs.old, freqs.new); >> >> cpufreq_freq_transition_begin(policy, &freqs); >> + retval = cpufreq_driver->target_intermediate(policy, freqs.new); > > It feels like you want to pass in "index" here too, just in case. A > driver may need to make decisions about other clocks based on the > eventual final frequency. They could cache it themselves from the > get_intermediate() call, but that seems ugly.
I had index here initially, but then it looked like they may need to perform get_intermediate() again from this routine and so sending the intermediate freq is probably better. So, probably just wait for some drivers which may need index here ?
>> @@ -2361,7 +2384,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data) >> !(driver_data->setpolicy || driver_data->target_index || >> driver_data->target) || >> (driver_data->setpolicy && (driver_data->target_index || >> - driver_data->target))) >> + driver_data->target)) || >> + (!!driver_data->get_intermediate ^ !!driver_data->target_intermediate)) > > I'm OK with the !! trick, but using ^ here seems more confusing. Why > not use "!="? > (!!driver_data->get_intermediate != !!driver_data->target_intermediate))
Will work as well :)
| |