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

[Akshay Adiga]

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.