login
Login
/
Register
Search
Search this site:
Forums
News
Blogs
Features
Site
Home
»
Mailing list archives
»
linux-kernel
»
2008
»
August
»
27
Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
view
thread
Previous message: [
thread
] [
date
] [
author
]
Next message: [
thread
] [
date
] [
author
]
[view in full thread]
From: Kevin Diggs
Subject:
Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
Date: Wednesday, August 27, 2008 - 2:04 pm
Arnd Bergmann wrote:
quoted text
> 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 <>< > >
--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to
majordomo@vger.kernel.org
More majordomo info at
http://vger.kernel.org/majordomo-info.html
Please read the FAQ at
http://www.tux.org/lkml/
Previous message: [
thread
] [
date
] [
author
]
Next message: [
thread
] [
date
] [
author
]
Messages in current thread:
[PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
, Kevin Diggs
, (Mon Aug 25, 3:53 am)
Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
, Arnd Bergmann
, (Mon Aug 25, 4:43 am)
Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
, Kevin Diggs
, (Mon Aug 25, 5:57 pm)
Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
, Arnd Bergmann
, (Tue Aug 26, 4:29 am)
Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
, Kevin Diggs
, (Wed Aug 27, 2:11 am)
Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
, Arnd Bergmann
, (Wed Aug 27, 4:34 am)
Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
, Geert Uytterhoeven
, (Wed Aug 27, 4:40 am)
Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
, Kevin Diggs
, (Wed Aug 27, 2:01 pm)
Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
, Kevin Diggs
, (Wed Aug 27, 2:04 pm)
Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
, Arnd Bergmann
, (Thu Aug 28, 12:46 am)
Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
, Kevin Diggs
, (Thu Aug 28, 9:34 am)
Navigation
Create content
Mailing list archives
Recent posts
Popular discussions
linux-kernel
:
Ingo Molnar
Re: [patch 00/13] Syslets, "Threadlets", generic AIO support, v3
Andi Kleen
[PATCH] [0/35] Some x86 2.6.22 candidate patches for review
Andrew Morton
Re: [PATCH] lazy freeing of memory through MADV_FREE 2/2
Peter Zijlstra
Re: [RFC PATCH 1/2] Marker probes in futex.c
Vivek Goyal
[PATCH] x86_64: Display more intutive error message if kernel is not 2MB aligned
git
:
Felipe Contreras
Re: [kernel.org users] [RFD] On deprecating "git-foo" for builtins
Johannes Schindelin
[PATCH] fetch: refuse to fetch into the current branch in a non-bare repository
Johannes Schindelin
Re: [PATCH] Fix install-doc-quick target
Peter Oberndorfer
Subject: [PATCH] fix stg edit command
Nicolas Pitre
Re: About git and the use of SHA-1
linux-netdev
:
Ursula Braun
[patch 2/8] [PATCH] af_iucv: sync sk shutdown flag if iucv path is quiesced
David Dillow
Re: [PATCH 2.6.30-rc4] r8169: avoid losing MSI interrupts
Andi Kleen
Re: RFC: Nagle latency tuning
Paul E. McKenney
Re: [PATCH 1/3] rcu: Introduce hlist_nulls variant of hlist
Russell King
Re: [BUG] New Kernel Bugs
git-commits-head
:
Linux Kernel Mailing List
sh: Fix compile error by operands(mov.l) in sh3/entry.S
Linux Kernel Mailing List
New device ID for sc92031 [1088:2031]
Linux Kernel Mailing List
e1000e: Expose MDI-X status via ethtool change
Linux Kernel Mailing List
powerpc/kexec: Add support for FSL-BookE
Linux Kernel Mailing List
drivers/acpi: use kasprintf
openbsd-misc
:
Andres Salazar
About priorities in /etc/resolv.conf
Rob Shepherd
x86 hardware for router system
Henning Brauer
Re: Sun Blade 1000?
Mitja Muženič
Re: isakmpd -- NCP IPsec client: peer proposed invalid phase 2 IDs
Damien Miller
Re: Patching a SSH 'Weakness'
Colocation donated by:
Syndicate