On Wednesday 27 August 2008, Kevin Diggs wrote:

Arnd Bergmann wrote:

quoted

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.
 

quoted

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.

quoted

quoted

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

quoted

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.

quoted

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