Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX

[Kevin Diggs]

Arnd Bergmann wrote:
> On Tuesday 26 August 2008, Kevin Diggs wrote:
>
> > Arnd Bergmann wrote:
> >
> > > On Monday 25 August 2008, Kevin Diggs wrote:
>
>
> > > Most people list their email address here as well
> >
> > For reasons I'd rather not go into, my email address is not likely
> > to remain valid for much longer.
>
>
> Maybe you should get a freemail account somewhere then.
> It's better if people have a way to Cc the author of
> a file when they make changes to it.
That won't really help either.
> > > drop the _t here, or make explicit what is meant by it.
> >
> > Now that I look at it the _t is supposed to be at the end. It is
> > meant to indicate that this is a structure tag or type. I'll
> > remove it.
>
>
> Ok, thanks for the explanation. I now saw that you also
> use '_v' for variables (I guess). These should probably
> go the same way.
Actually the _v means global variable. I would prefer to keep it.
> > > > +static DECLARE_COMPLETION(cf750gx_v_exit_completion);
> > > > +
> > > > +static unsigned int override_min_core = 0;
> > > > +static unsigned int override_max_core = 0;
> > > > +static unsigned int minmaxmode = 0;
> > > > +
> > > > +static unsigned int cf750gx_v_min_core = 0;
> > > > +static unsigned int cf750gx_v_max_core = 0;
> > > > +static int cf750gx_v_idle_pll_off = 0;
> > > > +static int cf750gx_v_min_max_mode = 0;
> > > > +static unsigned long cf750gx_v_state_bits = 0;
> > >
> > >
> > > Is 0 a meaningful value for these? If it just means 'uninitialized',
> > > then better don't initialize them in the first place, for clarity.
> >
> > The first 3 are module parameters. For the first 2, 0 means
> > that they were not set. minmaxmode is a boolean. 0 is the
> > default of disabled.
>
>
> Then at least for the first two, the common coding style would
> be to leave out the '= 0'. For minmaxmode, the most expressive
> way would be to define an enum, like
>
> enum {
> 	MODE_NORMAL,
> 	MODE_MINMAX,
> };
>
> int minmaxmode = MODE_NORMAL;
Since this is a boolean parameter I don't know? What if I change the
name to enable_minmax. And actually use the "bool" module parameter
type?
> > ..._min_max_mode is a boolean to hold the state of
> > minmaxmode. Seems to be only used to print the current
> > value.
>
>
> this looks like a duplicate then. why would you need both?
> It seems really confusing to have both a cpufreq attribute
> and a module attribute with the same name, writing to
> different variables. 
I probably don't need both? I kinda treat the variables tied to module
parameters as read only.
> Are there even SMP boards based on a 750? I thought only 74xx
> and 603/604 were SMP capable.
Not that I have heard of. I thought it was lacking some hardware that
was needed to do SMP? Can the 603 do SMP?
> > > > +static int cf750gx_pll_switch_cb(struct notifier_block *nb, unsigned long
> > > > +       action, void *data)
> > > > +{
> > > > +struct cf750gx_t_call_data *cd;
> > > > +unsigned int idle_pll;
> > > > +unsigned int pll_off_cmd;
> > > > +unsigned int new_pll;
> > >
> > >
> > > The whitespace appears damaged here.
> >
> > Just a coding style thing. I put declarations (or definitions -
> > I get the two confused?) on the same indent as the block they are
> > in. Is this a 15 yard illegal procedure penalty?
>
>
> I've never seen that style before. Better do what everyone
> else does here, e.g. using 'indent -kr -i8'.
Ok, I'll fix this.
> > > > +       dprintk(__FILE__">%s()-%d:  Modifying PLL:  0x%x\n", __func__, __LINE__,
> > > > +               new_pll);
> > >
> > >
> > > Please go through all your dprintk and see if you really really need all of them.
> > > Usually they are useful while you are doing the initial code, but only get in the
> > > way as soon as it starts working.
> >
> > This from a code readability standpoint? Or an efficiency one?
> > I think the cpufreq stuff has a debug configure option that
> > disables compilation of these unless enabled.
>
>
> It's more about readability.
I removed a few. Let me know if I should whack some more (and which ones).
> > > > +       result = pllif_register_pll_switch_cb(&cf750gx_v_pll_switch_nb);
> > > > +
> > > > +       cf750gx_v_pll_lock_nb.notifier_call = cf750gx_pll_lock_cb;
> > > > +       cf750gx_v_pll_lock_nb.next =
> > > > +               (struct notifier_block *)&cf750gx_v_lock_call_data;
> > >
> > >
> > > These casts look wrong, cf750gx_v_lock_call_data is not a notifier_block.
> > > What are you trying to do here?
> >
> > Just a little sneaky. I should document in the kernel doc though.
>
>
> No, better avoid such hacks instead of documenting them. I think I
> now understand what you do there, and if I got it right, you should
> just pass two arguments to pllif_register_pll_switch_cb.
I'll fix this.
> > > > +static int cf750gx_cpu_exit(struct cpufreq_policy *policy)
> > > > +{
> > > > +       dprintk("%s()\n", __func__);
> > > > +
> > > > +       /*
> > > > +        * Wait for any active requests to ripple through before exiting
> > > > +        */
> > > > +       wait_for_completion(&cf750gx_v_exit_completion);
> > >
> > >
> > > This "wait for anything" use of wait_for_completion looks wrong, 
> > > because once any other function has done the 'complete', you won't
> > > wait here any more.
> > >
> > > What exactly are you trying to accomplish with this?
> >
> > Originall I had something like:
> >
> > 	while(some_bit_is_still_set)
> > 		sleep
> >
> > I think you suggested I use a completion because it is in
> > fact simpler than a sleep. Now that I think about it also seems
> > intuitive to give the system a hint as to when something will
> > be done. 'complete' just means there is no timer pending (unless,
> > of course, I screwed up the code).
>
>
> The completion would certainly be better than the sleep here, but
> I think you shouldn't need that in the first place. AFAICT, you
> have three different kinds of events that you need to protect
> against, when some other code can call into your module:
>
> 1. timer function,
> 2. cpufreq notifier, and
> 3. sysfs attribute.
>
> In each case, the problem is a race between two threads that you
> need to close. In case of the timer, you need to call del_timer_sync
> because the timers already have this method builtin. For the other
> two, you already unregister the interfaces, which ought to be enough.
The choice I made here was to wait for the timer to fire rather than
to delete it. I think it is easier to just wait for it than to delete
it and try to get things back in order. Though since this is in a
module exit routine it probably does not matter. Also I would have to
check whether there was an analogous HRTimer call and call the right
one.
> 	Arnd <><

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/