Re: [PATCH v3 5/5] powernv:cpufreq: Implement the driver->get() method

[Gautham R Shenoy <ego@linux.vnet.ibm.com>]

On Fri, Mar 21, 2014 at 05:15:34PM +0530, Viresh Kumar wrote:
> On 21 March 2014 16:34, Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:
> > Heh! Well, that wasn't the reason why this was sent out as a separate
> > patch, but never mind. Though I don't understand why it would be
> > difficult to review the patch though.
> 
> Because the initial driver wasn't complete earlier. There were 2-3 patches
> after the first one which are changing what the first patch has added.
> Nothing else :)
> 
> >> > +static void powernv_read_cpu_freq(void *ret_freq)
> >> > +{
> >> > +       unsigned long pmspr_val;
> >> > +       s8 local_pstate_id;
> >> > +       int *cur_freq, freq, pstate_id;
> >> > +
> >> > +       cur_freq = (int *)ret_freq;
> >>
> >> You don't need cur_freq variable at all..
> >
> > I don't like it either. But the compiler complains without this hack
> > :-(
> 
> Why would the compiler warn for doing this?:
> 
> *(int *)ret_freq = freq;

The compiler doesn't complain if I do this.
> 
> >> > +       pmspr_val = get_pmspr(SPRN_PMSR);
> >> > +
> >> > +       /* The local pstate id corresponds bits 48..55 in the PMSR.
> >> > +         * Note: Watch out for the sign! */
> >> > +       local_pstate_id = (pmspr_val >> 48) & 0xFF;
> >> > +       pstate_id = local_pstate_id;
> >>
> >> similarly local_pstate_id
> >
> > well, I am interested in the bits 48..55 of pmspr_val. That's the
> > pstate_id which can be negative. So I'll like to keep
> > local_pstate_id.
> 
> Can you please explain at bit level how getting the value in a s8
> first and then into a s32 will help you here? Instead of getting it
> directly into s32.

Consider the case when pmspr = 0x00feffffffffffff;

We are interested in extracting the value 'fe'. And ensure that when
we store this value into an int, we get the sign extension right.

So the following doesn't work: 

   pstate_id = (pmspr_val >> 48) & 0xFFFFFFFF;

Since we get pstate_id = 0x000000fe, which is not the same as
0xfffffffe.

Of course, we could do the following, but I wasn't sure if
two levels of type casting helped on the readability front.

   pstate_id = (int) ((s8)((pmspr_val >> 48) & 0xFF));

--
Thanks and Regards
gautham.
>