> -----Original Message----- > From: Santosh Shilimkar [mailto:santosh.shilimkar@xxxxxx] > Sent: Monday, February 21, 2011 3:57 PM > To: Jean Pihet > Cc: linux-omap@xxxxxxxxxxxxxxx; Kevin Hilman; linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx; Rajendra Nayak > Subject: RE: [PATCH 14/17] omap4: cpuidle: Add MPUSS RET OFF states > [...] > > I think the piece of code below is rather difficult to read and > > understand. Based on the patch description and the comment here > > below > > I do not see the relation with the code. > > > There is relation. The comments are as per the conditions. And > The hardware constraint is back-ground behind the conditions. > > > " On OMAP4 because of hardware constraints, no low power states > are > > targeted when both CPUs are online and in SMP mode. The low power > > states are attempted only when secondary CPU gets offline to OFF > > through hotplug infrastructure. " > > The test below does not seem to match this comment. > > > > > + /* > > > + * Special hardware/software considerations: > > > + * 1. Do only WFI for secondary CPU(non-boot - CPU1). > > > + * Secondary cores are taken down only via hotplug > > path. > > The comment looks contradictory. Which one is taken OFF using this > > code, which one from hotplug? > > Does this correspond to the condition '(dev->cpu)' in the test > > below? > > Yes. > > > > > + * 2. Do only a WFI as long as in SMP mode. > > Does this correspond to the condition '(num_online_cpus() > 1)' in > > the > > test below? If so it this one triggering the low power mode for > > cpu0? > yes > > > > > + * 3. Continue to do only WFI till CPU1 hits OFF state. > > > + * This is necessary to honour hardware > recommondation > > > + * of triggeing all the possible low power modes once > > CPU1 is > > > + * out of coherency and in OFF mode. > > Does this correspond to the condition '(cpu1_state != > > PWRDM_POWER_OFF)' in the test below? > > > Yes > > > > + * Update dev->last_state so that governor stats > reflects > > right > > > + * data. > > > + */ > > > + cpu1_state = pwrdm_read_pwrst(cpu1_pd); > > > + if ((dev->cpu) || (num_online_cpus() > 1) || > > > + (cpu1_state != PWRDM_POWER_OFF)) { > > Are '||' correct here? > Yes. > > > Sorry if the code behaves correctly, the remarks are about > > readability especially in the comments. > > > You got all three correct. Instead of having three If's doing > same thing I have merged them and added comments above. > > And you got all of them correctly. > Will add the code conditions in comments as well so that it becomes mere readable. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
