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

[Kevin Diggs]

Arnd Bergmann wrote:
> On Wednesday 27 August 2008, Kevin Diggs wrote:
>
> > Arnd Bergmann wrote:
>
>
> > > 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.
>
>
> The reasoning on Linux is that you can tell from the declaration
> whether something is global or automatic. In fact, functions should
> be so short that you can always see all automatic declarations
> when you look at some code.
>
> If you use nonstandard variable naming, people will never stop
> asking you about that, so it's easier to just to the same as
> everyone else.
>  
>
> > > 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?
>
>
> Module parameter names should be short, so just "minmax" would
> be a good name, but better put the module_param() line right
> after that. If it's a bool type, I would just leave out the
> initialization.
>
>
> > > > ..._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.
>
>
> But you have marked as read/write in the module_param line.
>
> I would prefer to just have the module parameter and have
> a tool to modify that one.
>
> If a module parameter only makes sense as read-only, you
> should mark it as 0444 instead of 0644, but this one looks
> like it can be writable.
>
>
> > > 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?
>
>
> No, I was wrong about the 603. The machine I was thinking of is actually
> 604e.
>
>
> > > 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.
>
>
> I think the module_exit() function should leave the frequency in a
> well-defined state, so the easiest way to get there is probably
> to delete the timer, and then manually set the frequency.
>
> 	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/