Re: [PATCHv4 12/15] vc: omap3: auto_ret / auto_off support

[Tero Kristo <t-kristo@ti.com>]

On Fri, 2011-12-09 at 12:13 -0800, Kevin Hilman wrote:
> Tero Kristo <t-kristo@ti.com> writes:
> 
> > Voltage code will now enable / disable auto_ret / auto_off dynamically
> > according to the voltagedomain usecounts. This is accomplished via
> > the usage of the voltdm callback functions for sleep / wakeup.
> >
> > Signed-off-by: Tero Kristo <t-kristo@ti.com>
> 
> [...]
> 
> > -static void __init omap3_vc_init_channel(struct voltagedomain *voltdm)
> > +static void omap3_set_core_off_timings(struct voltagedomain *voltdm)
> >  {
> > +	u32 tstart, tshut;
> 
> nit: insert blank line here

Okay.

> 
> > +	omap_pm_get_oscillator(&tstart, &tshut);
> > +	omap3_set_clksetup(tstart, voltdm);
> >  	omap3_set_off_timings(voltdm);
> >  }
> >  
> > +static void omap3_vc_channel_sleep(struct voltagedomain *voltdm)
> > +{
> > +	/* Set off timings if entering off */
> > +	if (voltdm->target_state == PWRDM_POWER_OFF)
> > +		omap3_set_off_timings(voltdm);
> > +	else
> > +		omap3_set_i2c_timings(voltdm, 0);
> 
> Comment probably applies more to patch 2 since that's where it was
> introduced, but this is where I got confused, so mentioning it here:
> 
> Calling this 'set_i2c_timings' is a bit confusing IMO because reading
> the code there is a choice between 'i2c' timings and 'off' timings.
> Maybe just calling these 'ret' and 'off' timings will be better for
> readability.

Well, actually, you can use i2c timings when scaling voltage to off
level also. You have following options for voltage scaling on omap3:

- scale to ret level (0.975V) using i2c command
- scale to off level (0.6V) using i2c command
- switch to off (0V) using pmic scripts

I was kind of trying to reflect this here, even though the pmic script
support is missing.

> 
> > +}
> > +
> > +static void omap3_vc_channel_wakeup(struct voltagedomain *voltdm)
> > +{
> > +}
> 
> nit: empty function not needed since code checks for non-NULL function
> pointer.

Yea can drop that away, I left it there because I thought it might need
some beef in it at some point.

> 
> > +static void omap3_vc_core_sleep(struct voltagedomain *voltdm)
> > +{
> > +	u8 mode;
> > +
> > +	switch (voltdm->target_state) {
> > +	case PWRDM_POWER_OFF:
> > +		mode = OMAP3430_AUTO_OFF_MASK;
> > +		break;
> > +	case PWRDM_POWER_RET:
> > +		mode = OMAP3430_AUTO_RET_MASK;
> > +		break;
> > +	default:
> > +		mode = OMAP3430_AUTO_SLEEP_MASK;
> > +		break;
> > +	}
> > +
> > +	if (mode & OMAP3430_AUTO_OFF_MASK)
> 
> AND vs. == ?

Oh yea, some strange logic here for sure, I'll change that to ==. :)

> 
> speaking of which, this function probably needs a comment mentioning
> that these bits are all mutually exclusive (with a TRM reference.)

Can add that too.

-Tero