"Jon Medhurst (Tixy)" <[email protected]> writes: > On Wed, 2015-12-02 at 22:19 +0100, Ben Gamari wrote: >> The frequency units are very confusing in this area as OPPs use Hz >> whereas cpufreq uses kHz. Be explicit about this in variable naming. >> >> Cc: Javier Martinez Canillas <[email protected]> >> Signed-off-by: Ben Gamari <[email protected]> >> --- >> drivers/cpufreq/arm_big_little.c | 20 ++++++++++---------- >> 1 file changed, 10 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/cpufreq/arm_big_little.c >> b/drivers/cpufreq/arm_big_little.c >> index 855599b..2d5761c 100644 >> --- a/drivers/cpufreq/arm_big_little.c >> +++ b/drivers/cpufreq/arm_big_little.c >> @@ -130,14 +130,14 @@ static unsigned int bL_cpufreq_get_rate(unsigned int >> cpu) >> } >> >> static int >> -bL_cpufreq_set_rate_cluster(u32 cpu, u32 cluster, u32 new_rate) >> +bL_cpufreq_set_rate_cluster(u32 cpu, u32 cluster, u32 new_rate_kHz) >> { >> unsigned long volt = 0, volt_old = 0; >> long freq_Hz; >> u32 old_rate; > > IMO variable renaming doesn't seem necessary, if cpufreq uses kHz then > in a cpufreq driver adding 'kHz' to variable seems redundant, especially > if Hz values like freq_Hz above are named especially to signal their > different units. > Correct; it isn't strictly necessary but it would have saved me half an hour of poking around trying work out the intent of this code.
> However, if renaming is going to happen it should at
> least be consistent within the same function i.e. also rename the old
> old_rate variable above.
>
That's a reasonable objection. I'd be happy to do that.
snip
>> static unsigned int
>> -bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
>> +bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate_kHz)
>> {
>> u32 new_rate, prev_rate;
>
> Ditto. Rename these too to add '_kHz' ?
>
Sure.
>> int ret;
>> @@ -209,13 +209,13 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32
>> new_cluster, u32 rate)
>>
>> if (bLs) {
>> prev_rate = per_cpu(cpu_last_req_freq, cpu);
>> - per_cpu(cpu_last_req_freq, cpu) = rate;
>> + per_cpu(cpu_last_req_freq, cpu) = rate_kHz;
>> per_cpu(physical_cluster, cpu) = new_cluster;
>>
>> new_rate = find_cluster_maxfreq(new_cluster);
>> new_rate = ACTUAL_FREQ(new_cluster, new_rate);
>> } else {
>> - new_rate = rate;
>> + new_rate = rate_kHz;
>> }
>>
>> pr_debug("%s: cpu: %d, old cluster: %d, new cluster: %d, freq: %d\n",
>> @@ -236,7 +236,7 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32
>> new_cluster, u32 rate)
>> } else if (ret && bLs) {
>> per_cpu(cpu_last_req_freq, cpu) = prev_rate;
>> per_cpu(physical_cluster, cpu) = old_cluster;
>> - }
>> + }
>
> There's a spurious whitespace change here. I know the space you deleted
> shouldn't have been there, but doing tidyups like that generally isn't
> done in patches that don't otherwise affect the code in question.
>
Alright, I can drop that change.
Cheers,
- Ben
signature.asc
Description: PGP signature
