Re: [RFC patch 1/2] ARM: at91: cpuidle: encapsulate the standby code

[Nicolas Ferre <nicolas.ferre@atmel.com>]

Hi Daniel,

On 04/15/2013 03:29 PM, Daniel Lezcano :
> In order to split the pm code from the cpuidle driver, add an ops for the
> standby function which will be initialized by the pm init functions directly,
> thus no need of the SoC specific headers.
> 
> Cleanup also the headers included in this file as they are no longer needed.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  arch/arm/mach-at91/cpuidle.c |   19 ++++---------------
>  arch/arm/mach-at91/pm.c      |    8 +++++++-
>  2 files changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c
> index 48f1228..b2bec92 100644
> --- a/arch/arm/mach-at91/cpuidle.c
> +++ b/arch/arm/mach-at91/cpuidle.c
> @@ -13,32 +13,21 @@
>   * #2 wait-for-interrupt and RAM self refresh
>   */
>  
> -#include <linux/kernel.h>
> +#include <linux/module.h>
>  #include <linux/init.h>
> -#include <linux/platform_device.h>
>  #include <linux/cpuidle.h>
> -#include <linux/io.h>
> -#include <linux/export.h>
> -#include <asm/proc-fns.h>
>  #include <asm/cpuidle.h>
> -#include <mach/cpu.h>
> -
> -#include "pm.h"
>  
>  #define AT91_MAX_STATES	2
>  
> +extern void (*at91_standby_ops)(void);
> +
>  /* Actual code that puts the SoC in different idle states */
>  static int at91_enter_idle(struct cpuidle_device *dev,
>  			struct cpuidle_driver *drv,
>  			       int index)
>  {
> -	if (cpu_is_at91rm9200())
> -		at91rm9200_standby();
> -	else if (cpu_is_at91sam9g45())
> -		at91sam9g45_standby();
> -	else
> -		at91sam9_standby();
> -
> +	at91_standby_ops();
>  	return index;
>  }
>  
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
> index 73f1f25..f456e86 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -39,6 +39,8 @@
>  #include "at91_rstc.h"
>  #include "at91_shdwc.h"
>  
> +void (*at91_standby_ops)(void);

Is this a common pattern to have such a floating function pointer in the
pm code?

> +
>  static void __init show_reset_status(void)
>  {
>  	static char reset[] __initdata = "reset";
> @@ -321,8 +323,12 @@ static int __init at91_pm_init(void)
>  	pr_info("AT91: Power Management%s\n", (slow_clock ? " (with slow clock mode)" : ""));
>  
>  	/* AT91RM9200 SDRAM low-power mode cannot be used with self-refresh. */
> -	if (cpu_is_at91rm9200())
> +	if (cpu_is_at91rm9200()) {
>  		at91_ramc_write(0, AT91RM9200_SDRAMC_LPR, 0);
> +		at91_standby_ops = at91rm9200_standby;
> +	} else if (cpu_is_at91sam9g45())

CodingStyle: ending "{" is missing.

> +		at91_standby_ops= at91sam9g45_standby;

CodingStyle: " " is missing

> +	else at91_standby_ops = at91sam9_standby;

CodingStyle: not on the same line + "{}" missing

>  
>  	suspend_set_ops(&at91_pm_ops);

I am in favor for the move. But please rewrite a new series.

Bye,
-- 
Nicolas Ferre