Re: [PATCH] cpufreq: powernv: Replacing pstate_id with frequency table index

[Akshay Adiga <akshay.adiga@linux.vnet.ibm.com>]

Hi viresh,

My apologies. I realize that i have messed it up a quite a few places. Surely with the checkpatch as well. I will send a v2 with corrections.

On 06/27/2016 12:00 PM, Viresh Kumar wrote:

> Hi Akshay,
>
> Did you try running checkpatch for this?
>
> On 24-06-16, 19:33, Akshay Adiga wrote:
>> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
>> index b29c5c2..f6ce6f0 100644
>> --- a/drivers/cpufreq/powernv-cpufreq.c
>> +++ b/drivers/cpufreq/powernv-cpufreq.c
>> @@ -43,6 +43,7 @@
>>   #define PMSR_SPR_EM_DISABLE	(1UL << 31)
>>   #define PMSR_MAX(x)		((x >> 32) & 0xFF)
>>   
>> +
> ?
>
>>   #define MAX_RAMP_DOWN_TIME				5120
>>   /*
>>    * On an idle system we want the global pstate to ramp-down from max value to
>> @@ -124,20 +125,29 @@ static int nr_chips;
>>   static DEFINE_PER_CPU(struct chip *, chip_info);
>>   
>>   /*
>> - * Note: The set of pstates consists of contiguous integers, the
>> - * smallest of which is indicated by powernv_pstate_info.min, the
>> - * largest of which is indicated by powernv_pstate_info.max.
>> + * Note: The set of pstates consists of contiguous integers,
>> + *
>> + * powernv_pstate_info stores the index of the frequency table
>> + *  instead of pstate itself for each of the pstates referred
>>    *
>>    * The nominal pstate is the highest non-turbo pstate in this
>>    * platform. This is indicated by powernv_pstate_info.nominal.
>>    */
>>   static struct powernv_pstate_info {
>> -	int min;
>> -	int max;
>> -	int nominal;
>> -	int nr_pstates;
>> +	unsigned int min;
>> +	unsigned int max;
>> +	unsigned int nominal;
>> +	unsigned int nr_pstates;
>>   } powernv_pstate_info;
>>   
>> +/* Use following macros for conversions between pstate_id and index */
>> +static inline int get_pstate(unsigned int i) {
> Read coding-styles please on how to write functions.
>
>> +	return powernv_freqs[i].driver_data;
>> +}
> Add a blank line here please.
>
>> +static inline unsigned int get_index(int pstate) {
>> +	return abs(pstate - get_pstate(powernv_pstate_info.max));
>> +}
>> +
>>   static inline void reset_gpstates(struct cpufreq_policy *policy)
>>   {
>>   	struct global_pstate_info *gpstates = policy->driver_data;
>> @@ -208,23 +218,28 @@ static int init_powernv_pstates(void)
>>   		return -ENODEV;
>>   	}
>>   
>> +	powernv_pstate_info.nr_pstates = nr_pstates;
>>   	pr_debug("NR PStates %d\n", nr_pstates);
>>   	for (i = 0; i < nr_pstates; i++) {
>>   		u32 id = be32_to_cpu(pstate_ids[i]);
>>   		u32 freq = be32_to_cpu(pstate_freqs[i]);
>>   
>> -		pr_debug("PState id %d freq %d MHz\n", id, freq);
> ?
>
>>   		powernv_freqs[i].frequency = freq * 1000; /* kHz */
>>   		powernv_freqs[i].driver_data = id;
> Will it be possible for Shilpa who was earlier on this to review this patch? As
> we don't really have great knowledge of the internals of this driver.
>