Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <2bc19b09-ea9b-46f6-dd11-27eadb7cd5da@arm.com>
Date:   Mon, 10 Oct 2022 09:49:26 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH 1/2] cpufreq: Change macro for store scaling min/max
 frequency
Content-Language: en-US
To:     Viresh Kumar <viresh.kumar@linaro.org>
Cc:     linux-kernel@vger.kernel.org, rafael@kernel.org,
        linux-pm@vger.kernel.org, Dietmar.Eggemann@arm.com
References: <20220930094821.31665-1-lukasz.luba@arm.com>
 <20221010053614.zu2f3btjlh7woxbj@vireshk-i7>
From:   Lukasz Luba <lukasz.luba@arm.com>
In-Reply-To: <20221010053614.zu2f3btjlh7woxbj@vireshk-i7>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Viresh,


On 10/10/22 06:36, Viresh Kumar wrote:
> On 30-09-22, 10:48, Lukasz Luba wrote:
>> In order to prepare extension to the store_scaling_max_freq() remove
>> the macro and use two normal functions. The set value for max frequency
>> is important for the task scheduler.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>>   drivers/cpufreq/cpufreq.c | 47 ++++++++++++++++++++++-----------------
>>   1 file changed, 27 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 69b3d61852ac..1f8b93f42c76 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -715,26 +715,33 @@ static ssize_t show_scaling_cur_freq(struct cpufreq_policy *policy, char *buf)
>>   	return ret;
>>   }
>>   
>> -/*
>> - * cpufreq_per_cpu_attr_write() / store_##file_name() - sysfs write access
>> - */
>> -#define store_one(file_name, object)			\
>> -static ssize_t store_##file_name					\
>> -(struct cpufreq_policy *policy, const char *buf, size_t count)		\
>> -{									\
>> -	unsigned long val;						\
>> -	int ret;							\
>> -									\
>> -	ret = sscanf(buf, "%lu", &val);					\
>> -	if (ret != 1)							\
>> -		return -EINVAL;						\
>> -									\
>> -	ret = freq_qos_update_request(policy->object##_freq_req, val);\
>> -	return ret >= 0 ? count : ret;					\
>> -}
>> -
>> -store_one(scaling_min_freq, min);
>> -store_one(scaling_max_freq, max);
>> +static ssize_t store_scaling_max_freq
>> +(struct cpufreq_policy *policy, const char *buf, size_t count)
> 
> I thought we can't break right after function's name according to
> kernel's coding guidelines. Did you run checkpatch on this ?

The checkpatch didn't capture this, but you are right. I'll change this.

-----------------------------------------------
/tmp/cpufreq//0001-cpufreq-Change-macro-for-store-scaling-min-max-frequ.patch
WARNING: Prefer kstrto<type> to single variable sscanf
#49: FILE: drivers/cpufreq/cpufreq.c:724:
+	ret = sscanf(buf, "%lu", &val);

WARNING: Prefer kstrto<type> to single variable sscanf
#63: FILE: drivers/cpufreq/cpufreq.c:738:
+	ret = sscanf(buf, "%lu", &val);

total: 0 errors, 2 warnings, 53 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
       mechanically convert to the typical style using --fix or 
--fix-inplace.

/tmp/cpufreq//0001-cpufreq-Change-macro-for-store-scaling-min-max-frequ.patch 
has style problems, please review.

NOTE: If any of the errors are false positives, please report
       them to the maintainer, see CHECKPATCH in MAINTAINERS.
==============================================================
/tmp/cpufreq//0002-cpufreq-Update-CPU-capacity-reduction-in-store_scali.patch
total: 0 errors, 0 warnings, 36 lines checked

/tmp/cpufreq//0002-cpufreq-Update-CPU-capacity-reduction-in-store_scali.patch 
has no obvious style problems and is ready for submission.
==============================================================

-----------------------------------------------