On Wed, Aug 11, 2010 at 4:31 PM, Amit Arora amit.arora@linaro.org wrote:
On Wed, Aug 11, 2010 at 5:42 PM, Amit Kucheria amit.kucheria@linaro.org wrote:
Some comments:
- Please run scripts/checkpatch.pl on the patches and fixup the
whitespace warnings.
Sure.
- In print_cstates(), where you have #ifdef'ed __i386__, the #else
should also be restricted to __arm__ IMO.
OK.
- On my quad core desktop, it doesn't show C1 as a supported C-state
on startup. (Supported C-states : C2 C3)
That could be because print_cstates() shows only those C states which support MWAIT (have mwait in "desc" file for that particular C state). Should this be different for ARM ? Should we show all the states (whether they have mwait or not in "desc") ?
Atleast on OMAP, /sys/devices/system/cpu/cpu0/cpuidle/stateN/desc is <null>.
On a second thought, print_cstates() for intel makes sense, since they want to tell which states are supported by BIOS and which are supported by the CPU (using, cpuid). But, for ARM, we have only one set of supported states and these will be same as what will be shown in the next screen. Thats why I have kept the second patch separate, since we can remove that if its really not required. Thoughts ?
I think MWAIT is x86-specific. So we don't care about that on ARM.
- Works on Beagleboard.
Nice.
Could you please try and post this to the powertop list this week?
Sure. Will wait for one more day for others to comment and then post it.