On Mon, Jun 6, 2016 at 6:25 PM, Viresh Kumar viresh.kumar@linaro.org wrote:
On 6 June 2016 at 18:27, Rafael J. Wysocki rafael@kernel.org wrote:
On Mon, Jun 6, 2016 at 2:24 PM, Viresh Kumar viresh.kumar@linaro.org wrote:
On 6 June 2016 at 17:40, Rafael J. Wysocki rjw@rjwysocki.net wrote:
On Monday, June 06, 2016 09:22:31 AM Viresh Kumar wrote:
I agree with that, though that requires larger changes across multiple sites.
What changes and where?
s/larger/some :)
So we can change all the callers of cpufreq_frequency_table_target(),
But why?
It just works as a static inline wrapper around cpufreq_find_index_l() for the code in question after this patch, doesn't it?
So if the caller knows it will always ask for RELATION_L, why bother with using the wrapper?
Sorry, I got a bit confused. Are you saying that we should do that change right in the patch?
Because I am also saying that yes, there is no point calling the wrapper.
OK
I can update this patch to make direct calls to the relation specific routines if you want.
I'm not sure if I like this patch at all in the first place.
Also I'm wondering about the cpufreq_for_each_valid_entry() used all over. Can't the things be arranged so all of the entries are valid?
Yeah, there would be multiple opportunities available to optimize code after this series is in. The policy->table after this series is all sorted properly and all the entries are valid as well.
But surely that should be done in a separate series
So I'm reading this as "I will add overhead to that code now, but I can remove it later" which makes me go "What?!" right away.
Moreover, you seem to be saying something like "all of the entries are valid now, but I'm using cpufreq_for_each_valid_entry() to walk freq tables anyway, so that I can get rid of it in a future patch" which makes me go "What?!" again.
Since you are adding new code, you can write it so it doesn't do unnecessary checks from the start.
While at it, the "if ((freq < policy->min) || (freq > policy->max))" checks in cpufreq_find_index_l() and cpufreq_find_index_h() don't look good to me, because they very well may cause those function to return -EINVAL even when there's a valid table and that may cause acpi_cpufreq_fast_switch() to do bad things.
Also, if you are going to return an index, why don't you iterate over indexes and avoid using pointer subtractions to compute the return value?
With that, cpufreq_find_index_l() would look like
static inline int cpufreq_find_index_l(struct cpufreq_policy *policy, unsigned int target_freq) { struct cpufreq_frequency_table *table = policy->freq_table; int i;
for (i = 0; table[i].frequency != CPUFREQ_TABLE_END; i++) if (table[i].frequency >= target_freq) return i;
return i > 0 ? --i : 0; }
and that can go into cpufreq.h IMO (ie. no need for the new header file).