On Wed, Aug 11, 2010 at 4:31 PM, Amit Arora <amit.ar...@linaro.org> wrote: > On Wed, Aug 11, 2010 at 5:42 PM, Amit Kucheria <amit.kuche...@linaro.org> > wrote: >> Some comments: >> >> 1. Please run scripts/checkpatch.pl on the patches and fixup the >> whitespace warnings. > Sure. > >> 2. In print_cstates(), where you have #ifdef'ed __i386__, the #else >> should also be restricted to __arm__ IMO. > OK. > >> 3. 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. >> 4. 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. > _______________________________________________ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
