Doug,
On 20 May 2014 22:18, Doug Anderson <diand...@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 :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
