[02/10] clk: samsung: s3c2410: Use generic helper for handling suspend/resume

Message ID 20180829155046.29359-3-m.szyprowski@samsung.com
State Superseded
Headers show
Series
  • Cleanup suspend/resume code in Samsung clock drivers
Related show

Commit Message

Marek Szyprowski Aug. 29, 2018, 3:50 p.m.
Replace common suspend/resume handling code by generic helper.
No functional change.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

---
 drivers/clk/samsung/clk-s3c2410.c | 43 ++-----------------------------
 1 file changed, 2 insertions(+), 41 deletions(-)

-- 
2.17.1

Comments

Chanwoo Choi Aug. 30, 2018, 1:10 a.m. | #1
Hi Marek,

On 2018년 08월 30일 00:50, Marek Szyprowski wrote:
> Replace common suspend/resume handling code by generic helper.

> No functional change.

> 

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---

>  drivers/clk/samsung/clk-s3c2410.c | 43 ++-----------------------------

>  1 file changed, 2 insertions(+), 41 deletions(-)

> 


Looks good to me.
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>



> diff --git a/drivers/clk/samsung/clk-s3c2410.c b/drivers/clk/samsung/clk-s3c2410.c

> index a9c887475054..8cb868f06257 100644

> --- a/drivers/clk/samsung/clk-s3c2410.c

> +++ b/drivers/clk/samsung/clk-s3c2410.c

> @@ -11,7 +11,6 @@

>  #include <linux/clk-provider.h>

>  #include <linux/of.h>

>  #include <linux/of_address.h>

> -#include <linux/syscore_ops.h>

>  

>  #include <dt-bindings/clock/s3c2410.h>

>  

> @@ -40,9 +39,6 @@ enum s3c2410_plls {

>  

>  static void __iomem *reg_base;

>  

> -#ifdef CONFIG_PM_SLEEP

> -static struct samsung_clk_reg_dump *s3c2410_save;

> -

>  /*

>   * list of controller registers to be saved and restored during a

>   * suspend/resume cycle.

> @@ -57,42 +53,6 @@ static unsigned long s3c2410_clk_regs[] __initdata = {

>  	CAMDIVN,

>  };

>  

> -static int s3c2410_clk_suspend(void)

> -{

> -	samsung_clk_save(reg_base, s3c2410_save,

> -				ARRAY_SIZE(s3c2410_clk_regs));

> -

> -	return 0;

> -}

> -

> -static void s3c2410_clk_resume(void)

> -{

> -	samsung_clk_restore(reg_base, s3c2410_save,

> -				ARRAY_SIZE(s3c2410_clk_regs));

> -}

> -

> -static struct syscore_ops s3c2410_clk_syscore_ops = {

> -	.suspend = s3c2410_clk_suspend,

> -	.resume = s3c2410_clk_resume,

> -};

> -

> -static void __init s3c2410_clk_sleep_init(void)

> -{

> -	s3c2410_save = samsung_clk_alloc_reg_dump(s3c2410_clk_regs,

> -						ARRAY_SIZE(s3c2410_clk_regs));

> -	if (!s3c2410_save) {

> -		pr_warn("%s: failed to allocate sleep save data, no sleep support!\n",

> -			__func__);

> -		return;

> -	}

> -

> -	register_syscore_ops(&s3c2410_clk_syscore_ops);

> -	return;

> -}

> -#else

> -static void __init s3c2410_clk_sleep_init(void) {}

> -#endif

> -

>  PNAME(fclk_p) = { "mpll", "div_slow" };

>  

>  static struct samsung_mux_clock s3c2410_common_muxes[] __initdata = {

> @@ -461,7 +421,8 @@ void __init s3c2410_common_clk_init(struct device_node *np, unsigned long xti_f,

>  			ARRAY_SIZE(s3c244x_common_aliases));

>  	}

>  

> -	s3c2410_clk_sleep_init();

> +	samsung_clk_sleep_init(reg_base, s3c2410_clk_regs,

> +			       ARRAY_SIZE(s3c2410_clk_regs));

>  

>  	samsung_clk_of_add_provider(np, ctx);

>  }

> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics
Krzysztof Kozlowski Aug. 30, 2018, 6:39 a.m. | #2
On Wed, 29 Aug 2018 at 17:51, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>

> Replace common suspend/resume handling code by generic helper.

> No functional change.


There is functional change in case alloc fails (kzalloc(),
samsung_clk_alloc_reg_dump()). Previously it would be warned and
ignored. Now it will panic. You could mention this but anyway the
change looks nice! Thanks!

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>


Best regards,
Krzysztof
Marek Szyprowski Aug. 30, 2018, 11:26 a.m. | #3
Hi Krzysztof,

On 2018-08-30 08:39, Krzysztof Kozlowski wrote:
> On Wed, 29 Aug 2018 at 17:51, Marek Szyprowski <m.szyprowski@samsung.com> wrote:

>> Replace common suspend/resume handling code by generic helper.

>> No functional change.

> There is functional change in case alloc fails (kzalloc(),

> samsung_clk_alloc_reg_dump()). Previously it would be warned and

> ignored. Now it will panic. You could mention this but anyway the

> change looks nice! Thanks!


No problem to replace panic() in samsung_clk_sleep_init() with a pair
of warn and return error as this is probably now a preferred behavior.
This is still truly hypothetical discussion because system, which
fails to allocate memory at boot is useless anyway.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Krzysztof Kozlowski Aug. 31, 2018, 6:34 a.m. | #4
On Thu, 30 Aug 2018 at 13:26, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>

> Hi Krzysztof,

>

> On 2018-08-30 08:39, Krzysztof Kozlowski wrote:

> > On Wed, 29 Aug 2018 at 17:51, Marek Szyprowski <m.szyprowski@samsung.com> wrote:

> >> Replace common suspend/resume handling code by generic helper.

> >> No functional change.

> > There is functional change in case alloc fails (kzalloc(),

> > samsung_clk_alloc_reg_dump()). Previously it would be warned and

> > ignored. Now it will panic. You could mention this but anyway the

> > change looks nice! Thanks!

>

> No problem to replace panic() in samsung_clk_sleep_init() with a pair

> of warn and return error as this is probably now a preferred behavior.

> This is still truly hypothetical discussion because system, which

> fails to allocate memory at boot is useless anyway.


I do not mind keeping this as is with small notice in commit msg about
change in case of error path. Indeed it is truly hypothetical case so
there is no point to rewrite samsung_clk_sleep_init for this.

Best regards,
Krzysztof

Patch

diff --git a/drivers/clk/samsung/clk-s3c2410.c b/drivers/clk/samsung/clk-s3c2410.c
index a9c887475054..8cb868f06257 100644
--- a/drivers/clk/samsung/clk-s3c2410.c
+++ b/drivers/clk/samsung/clk-s3c2410.c
@@ -11,7 +11,6 @@ 
 #include <linux/clk-provider.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
-#include <linux/syscore_ops.h>
 
 #include <dt-bindings/clock/s3c2410.h>
 
@@ -40,9 +39,6 @@  enum s3c2410_plls {
 
 static void __iomem *reg_base;
 
-#ifdef CONFIG_PM_SLEEP
-static struct samsung_clk_reg_dump *s3c2410_save;
-
 /*
  * list of controller registers to be saved and restored during a
  * suspend/resume cycle.
@@ -57,42 +53,6 @@  static unsigned long s3c2410_clk_regs[] __initdata = {
 	CAMDIVN,
 };
 
-static int s3c2410_clk_suspend(void)
-{
-	samsung_clk_save(reg_base, s3c2410_save,
-				ARRAY_SIZE(s3c2410_clk_regs));
-
-	return 0;
-}
-
-static void s3c2410_clk_resume(void)
-{
-	samsung_clk_restore(reg_base, s3c2410_save,
-				ARRAY_SIZE(s3c2410_clk_regs));
-}
-
-static struct syscore_ops s3c2410_clk_syscore_ops = {
-	.suspend = s3c2410_clk_suspend,
-	.resume = s3c2410_clk_resume,
-};
-
-static void __init s3c2410_clk_sleep_init(void)
-{
-	s3c2410_save = samsung_clk_alloc_reg_dump(s3c2410_clk_regs,
-						ARRAY_SIZE(s3c2410_clk_regs));
-	if (!s3c2410_save) {
-		pr_warn("%s: failed to allocate sleep save data, no sleep support!\n",
-			__func__);
-		return;
-	}
-
-	register_syscore_ops(&s3c2410_clk_syscore_ops);
-	return;
-}
-#else
-static void __init s3c2410_clk_sleep_init(void) {}
-#endif
-
 PNAME(fclk_p) = { "mpll", "div_slow" };
 
 static struct samsung_mux_clock s3c2410_common_muxes[] __initdata = {
@@ -461,7 +421,8 @@  void __init s3c2410_common_clk_init(struct device_node *np, unsigned long xti_f,
 			ARRAY_SIZE(s3c244x_common_aliases));
 	}
 
-	s3c2410_clk_sleep_init();
+	samsung_clk_sleep_init(reg_base, s3c2410_clk_regs,
+			       ARRAY_SIZE(s3c2410_clk_regs));
 
 	samsung_clk_of_add_provider(np, ctx);
 }