lkml.org 
[lkml]   [2014]   [May]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH V3 2/4] cpufreq: add support for intermediate (stable) frequencies
From
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 :)


\
 
 \ /
  Last update: 2014-05-21 07:21    [W:0.057 / U:1.828 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site