[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