[PATCH 1/5] OMAP4: PM: Use the lowpwrstatechange feature on OMAP4
Rajendra Nayak
rnayak at ti.com
Wed Dec 15 10:17:52 EST 2010
Hi Kevin,
<snip>..
> > if (pwrdm_read_pwrst(pwrdm) < PWRDM_POWER_ON) {
> > + if ((pwrdm_read_pwrst(pwrdm) > state) &&
> > + (pwrdm->flags & PWRDM_HAS_LOWPOWERSTATECHANGE)) {
> > + ret = pwrdm_set_next_pwrst(pwrdm, state);
> > + pwrdm_set_lowpwrstchange(pwrdm);
> > + pwrdm_wait_transition(pwrdm);
> > + pwrdm_state_switch(pwrdm);
> > + return ret;
>
> Personally, I'd prefer if this function flowed through better instead of
> the early return in order to emphasize the common code.
>
> Rather than the return here, can you just set the low-power state change
> bit here (and put the clkdm_wakeup + sleep_switch = 1 into the else
> clause?
>
> Or, does the next state have to be set before the low-power state change
> bit?
Yes, that sequencing is needed.
>
> Basically, what I'm getting at is this should be a single function with
> common flow. The conditional code based on low-power state change
> should be isolated instead of having a special path.
I get your point. See if the below approach looks better.
If it looks fine, I'll do some more testing (currently only tested
on OMAP4430sdp) and repost the 2 patches.
More information about the linux-arm-kernel
mailing list