From: ben@smart-cactus.org (Ben Gamari)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 11/12] cpufreq: arm-big-little: clarify frequency units
Date: Thu, 03 Dec 2015 15:37:44 +0100 [thread overview]
Message-ID: <87egf338ef.fsf@smart-cactus.org> (raw)
In-Reply-To: <1449152534.2817.26.camel@linaro.org>
"Jon Medhurst (Tixy)" <tixy@linaro.org> 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 <javier@osg.samsung.com>
>> Signed-off-by: Ben Gamari <ben@smart-cactus.org>
>> ---
>> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 472 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151203/f9c0b9e2/attachment.sig>
next prev parent reply other threads:[~2015-12-03 14:37 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-02 21:19 [PATCH v5 0/12] cpufreq: Add support for Exynos 5800, 5420, and 5422 Ben Gamari
2015-12-02 21:19 ` [PATCH 01/12] cpufreq: arm_big_little: add cluster regulator support Ben Gamari
2015-12-03 4:41 ` Anand Moon
2015-12-02 21:19 ` [PATCH 02/12] clk: samsung: exynos5420: add cpu clock configuration data and instantiate cpu clock Ben Gamari
2015-12-03 6:08 ` Krzysztof Kozlowski
2015-12-03 10:30 ` Ben Gamari
2015-12-04 2:25 ` Krzysztof Kozlowski
2015-12-02 21:19 ` [PATCH 03/12] ARM: dts: Exynos5420: add CPU OPP and regulator supply property Ben Gamari
2015-12-02 21:19 ` [PATCH 04/12] ARM: Exynos: use generic cpufreq driver for Exynos5420 Ben Gamari
2015-12-02 21:19 ` [PATCH 05/12] clk: samsung: exynos5800: fix cpu clock configuration data Ben Gamari
2015-12-02 21:19 ` [PATCH 06/12] ARM: dts: Exynos5800: fix CPU OPP Ben Gamari
2015-12-02 21:19 ` [PATCH 07/12] ARM: dts: Exynos5422: fix OPP tables Ben Gamari
2015-12-02 21:19 ` [PATCH 08/12] ARM: Exynos: use generic cpufreq driver for Exynos5800 Ben Gamari
2015-12-02 21:19 ` [PATCH 09/12] ARM: dts: Exynos5420/5800: add cluster regulator supply properties Ben Gamari
2015-12-02 21:19 ` [PATCH 10/12] cpufreq: arm-big-little: accept operating-points-v2 nodes Ben Gamari
2015-12-02 21:19 ` [PATCH 11/12] cpufreq: arm-big-little: clarify frequency units Ben Gamari
2015-12-03 14:22 ` Jon Medhurst (Tixy)
2015-12-03 14:37 ` Ben Gamari [this message]
2015-12-02 21:19 ` [PATCH 12/12] cpufreq: arm-big-little: warn on invalid regulator Ben Gamari
2015-12-03 6:05 ` [PATCH v5 0/12] cpufreq: Add support for Exynos 5800, 5420, and 5422 Viresh Kumar
2015-12-03 10:26 ` Ben Gamari
2015-12-03 10:37 ` Viresh Kumar
2015-12-03 11:21 ` Ben Gamari
2015-12-03 11:25 ` Viresh Kumar
2015-12-07 21:19 ` Ben Gamari
2015-12-03 11:05 ` Sudeep Holla
2015-12-03 11:24 ` Viresh Kumar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87egf338ef.fsf@smart-cactus.org \
--to=ben@smart-cactus.org \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).