On Wed, Aug 11, 2010 at 10:10 AM, Amit Arora amit.arora@linaro.org wrote:
What to review:
Once you checkout the "linaro" branch, as suggested above. You can review the two patches with commit id "09049b42e3" and "783a3e6bbe".
What to test:
Some of the things which could be tested is, o Compile and test on any ARM board that you may have. You can also test on any x86 / x86_64 systems, to check if nothing has been broken. o Compare the output from "master" and "linaro" branches ("-d" dump option may help, if you wish to take a diff) o Check if the number of C states is correct (/sys/devices/system/cpu/cpu0/cpuidle/) o Check if the number of P states is correct (/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_frequencies) o Check if the % values makes sense (say, if total doesn't add to 100! Or, if its a very huge value [1] ). o If number of Wakeups-per-second [1] looks reasonable. o Top causes for wakeups is shown properly o Suggestions are shown properly. Some of the suggestions may show a hot key on the status bar (at bottom).
[1] There is a known issue of %age values and Wakeups-per-sec number being shown very high (in the order of thousands), if some key is hit (say, 'r' to refresh or any other key). This problem can be seen in "master" branch (upstream) too, and hence is not introduced by the new patches. Since the goal of these patches is different, I haven't addressed this issue here.
Some comments:
1. Please run scripts/checkpatch.pl on the patches and fixup the whitespace warnings. 2. In print_cstates(), where you have #ifdef'ed __i386__, the #else should also be restricted to __arm__ IMO. 3. On my quad core desktop, it doesn't show C1 as a supported C-state on startup. (Supported C-states : C2 C3) 4. Works on Beagleboard.
Could you please try and post this to the powertop list this week?
Regards, Amit