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

[Kevin Diggs <kevdig@hypersurf.com>]

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 <><
> 
>