Re: [PATCHv6 9/9] OMAP3: PM: Added support for suspending to INACTIVE state

[Kevin Hilman <khilman@deeprootsystems.com>]

Tero Kristo <tero.kristo@nokia.com> writes:

> From: Tero Kristo <tero.kristo@nokia.com>
>
> With the new support functions this is now possible. Suspending to INACTIVE
> is useful for testing purposes.
>
> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
> ---
>  arch/arm/mach-omap2/pm34xx.c |   11 ++++++-----
>  1 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index cdbedcf..41d6cc3 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -633,11 +633,12 @@ int set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
>  	if (pwrdm == NULL || IS_ERR(pwrdm))
>  		return -EINVAL;
>  
> -	while (!(pwrdm->pwrsts & (1 << state))) {
> -		if (state == PWRDM_POWER_OFF)
> -			return ret;
> -		state--;
> -	}

The comment above set_pwrdm_state() says only ON & RET are supported.
Please update that comment as well.


> +	if (state != PWRDM_POWER_INACTIVE)
> +		while (!(pwrdm->pwrsts & (1 << state))) {
> +			if (state == PWRDM_POWER_OFF)
> +				return ret;
> +			state--;
> +		}

I think all powerdomains can be inactive right?

I think it would be cleaner to just have all the pwrdm->pwrsts fields
include intactive as a valid option.

Something like the patch below.  IIRC, you did something like this in
one of the earlier versions of the patch.

Kevin

diff --git a/arch/arm/plat-omap/include/plat/powerdomain.h b/arch/arm/plat-omap/include/plat/powerdomain.h
index a1ecd47..c692472 100644
--- a/arch/arm/plat-omap/include/plat/powerdomain.h
+++ b/arch/arm/plat-omap/include/plat/powerdomain.h
@@ -31,17 +31,17 @@
 #define PWRDM_MAX_PWRSTS	4
 
 /* Powerdomain allowable state bitfields */
-#define PWRSTS_OFF_ON		((1 << PWRDM_POWER_OFF) | \
+#define PWRSTS_ON		((1 << PWRDM_POWER_INACTIVE) | \
 				 (1 << PWRDM_POWER_ON))
 
+#define PWRSTS_OFF_ON		((1 << PWRDM_POWER_OFF) | PWRSTS_ON)
+
 #define PWRSTS_OFF_RET		((1 << PWRDM_POWER_OFF) | \
 				 (1 << PWRDM_POWER_RET))
 
-#define PWRSTS_RET_ON		((1 << PWRDM_POWER_RET) | \
-				 (1 << PWRDM_POWER_ON))
-
-#define PWRSTS_OFF_RET_ON	(PWRSTS_OFF_RET | (1 << PWRDM_POWER_ON))
+#define PWRSTS_RET_ON		((1 << PWRDM_POWER_RET) | PWRSTS_ON)
 
+#define PWRSTS_OFF_RET_ON	(PWRSTS_OFF_RET | PWRSTS_ON)
 
 /* Powerdomain flags */
 #define PWRDM_HAS_HDWR_SAR	(1 << 0) /* hardware save-and-restore support */