[PATCH] backlight: pwm_bl: Use SIMPLE_DEV_PM_OPS()

Subsystems: backlight class/subsystem, framebuffer layer, the rest

STALE4508d

5 messages, 3 authors, 2014-04-01 · open the first message on its own page

[PATCH] backlight: pwm_bl: Use SIMPLE_DEV_PM_OPS()

From: Alexander Shiyan <hidden>
Date: 2014-03-29 10:11:11

Use the SIMPLE_DEV_PM_OPS() macro to declare the driver's pm_ops.

Signed-off-by: Alexander Shiyan <redacted>
---
 drivers/video/backlight/pwm_bl.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index b75201f..d5e1f5b 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -359,8 +359,7 @@ static int pwm_backlight_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int pwm_backlight_suspend(struct device *dev)
+static int __maybe_unused pwm_bl_suspend(struct device *dev)
 {
 	struct backlight_device *bl = dev_get_drvdata(dev);
 	struct pwm_bl_data *pb = bl_get_data(bl);
@@ -376,7 +375,7 @@ static int pwm_backlight_suspend(struct device *dev)
 	return 0;
 }
 
-static int pwm_backlight_resume(struct device *dev)
+static int __maybe_unused pwm_bl_resume(struct device *dev)
 {
 	struct backlight_device *bl = dev_get_drvdata(dev);
 
@@ -384,22 +383,14 @@ static int pwm_backlight_resume(struct device *dev)
 
 	return 0;
 }
-#endif
 
-static const struct dev_pm_ops pwm_backlight_pm_ops = {
-#ifdef CONFIG_PM_SLEEP
-	.suspend = pwm_backlight_suspend,
-	.resume = pwm_backlight_resume,
-	.poweroff = pwm_backlight_suspend,
-	.restore = pwm_backlight_resume,
-#endif
-};
+static SIMPLE_DEV_PM_OPS(pwm_bl_pm_ops, pwm_bl_suspend, pwm_bl_resume);
 
 static struct platform_driver pwm_backlight_driver = {
 	.driver		= {
 		.name		= "pwm-backlight",
 		.owner		= THIS_MODULE,
-		.pm		= &pwm_backlight_pm_ops,
+		.pm		= &pwm_bl_pm_ops,
 		.of_match_table	= of_match_ptr(pwm_backlight_of_match),
 	},
 	.probe		= pwm_backlight_probe,
-- 
1.8.3.2

Re: [PATCH] backlight: pwm_bl: Use SIMPLE_DEV_PM_OPS()

From: Lee Jones <hidden>
Date: 2014-03-31 07:38:48

quoted hunk
Use the SIMPLE_DEV_PM_OPS() macro to declare the driver's pm_ops.

Signed-off-by: Alexander Shiyan <redacted>
---
 drivers/video/backlight/pwm_bl.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index b75201f..d5e1f5b 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
[...]
-static int pwm_backlight_resume(struct device *dev)
+static int __maybe_unused pwm_bl_resume(struct device *dev)
What's the __maybe_unused attribute for?

In include/linux/compiler-gcc.h it redefines the attribute as 'unused': 

  #define __maybe_unused                  __attribute__((unused))

... are you sure this is what you want?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] backlight: pwm_bl: Use SIMPLE_DEV_PM_OPS()

From: Lee Jones <hidden>
Date: 2014-03-31 08:16:38

quoted
quoted
Use the SIMPLE_DEV_PM_OPS() macro to declare the driver's pm_ops.

Signed-off-by: Alexander Shiyan <redacted>
---
 drivers/video/backlight/pwm_bl.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index b75201f..d5e1f5b 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
[...]
quoted
-static int pwm_backlight_resume(struct device *dev)
+static int __maybe_unused pwm_bl_resume(struct device *dev)
What's the __maybe_unused attribute for?

In include/linux/compiler-gcc.h it redefines the attribute as 'unused': 

  #define __maybe_unused                  __attribute__((unused))

... are you sure this is what you want?
Yes. This avoids compiler warnings if CONFIG_PM is unset.
What are the advantages of this over the config option? Besides 2
lines of code?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] backlight: pwm_bl: Use SIMPLE_DEV_PM_OPS()

From: Lee Jones <hidden>
Date: 2014-03-31 08:50:47

quoted
quoted
quoted
quoted
Use the SIMPLE_DEV_PM_OPS() macro to declare the driver's pm_ops.

Signed-off-by: Alexander Shiyan <redacted>
---
 drivers/video/backlight/pwm_bl.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index b75201f..d5e1f5b 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
[...]
quoted
-static int pwm_backlight_resume(struct device *dev)
+static int __maybe_unused pwm_bl_resume(struct device *dev)
What's the __maybe_unused attribute for?

In include/linux/compiler-gcc.h it redefines the attribute as 'unused': 

  #define __maybe_unused                  __attribute__((unused))

... are you sure this is what you want?
Yes. This avoids compiler warnings if CONFIG_PM is unset.
What are the advantages of this over the config option? Besides 2
lines of code?
Just an increase compile coverage.
What does this mean? I replied to another one of your patches with the
same question.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] backlight: pwm_bl: Use SIMPLE_DEV_PM_OPS()

From: Jingoo Han <hidden>
Date: 2014-04-01 00:08:33

On Monday, March 31, 2014 7:37 PM, Lee Jones wrote:
Mon, 31 Mar 2014 09:50:47 +0100 от Lee Jones [off-list ref]:
quoted
quoted
quoted
quoted
quoted
quoted
Use the SIMPLE_DEV_PM_OPS() macro to declare the driver's pm_ops.

Signed-off-by: Alexander Shiyan <redacted>
---
 drivers/video/backlight/pwm_bl.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index b75201f..d5e1f5b 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
[...]
quoted
-static int pwm_backlight_resume(struct device *dev)
+static int __maybe_unused pwm_bl_resume(struct device *dev)
What's the __maybe_unused attribute for?

In include/linux/compiler-gcc.h it redefines the attribute as 'unused':

  #define __maybe_unused                  __attribute__((unused))

... are you sure this is what you want?
Yes. This avoids compiler warnings if CONFIG_PM is unset.
What are the advantages of this over the config option? Besides 2
lines of code?
Just an increase compile coverage.
What does this mean? I replied to another one of your patches with the
same question.
Code parts for suspend() and resume() will be compiled regardless of
CONFIG_PM option and will be discarded by linker if CONFIG_PM is not set.
(+cc Joe Perches, Dan Carpenter)

Original patch is 
http://www.spinics.net/lists/linux-fbdev/msg14177.html

I don't think that __maybe_unused is good.
Currently, SIMPLE_DEV_PM_OPS is using "#ifdef CONFIG_PM_SLEEP" as below.
So, suspend_fn, resume_fn should be protected by "#ifdef CONFIG_PM_SLEEP".
Maybe, it is necessary to fix SET_SYSTEM_SLEEP_PM_OPS define,
in order to increase compile coverage.

./include/linux/pm.h
#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
const struct dev_pm_ops name = { \
        SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
}

#ifdef CONFIG_PM_SLEEP
#define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
        .suspend = suspend_fn, \
        .resume = resume_fn, \
        .freeze = suspend_fn, \
        .thaw = resume_fn, \
        .poweroff = suspend_fn, \
        .restore = resume_fn,
#else
#define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
#endif


Please, don’t remove "#ifdef CONFIG_PM_SLEEP".
Your patch can be modified as below:
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -386,14 +386,7 @@ static int pwm_backlight_resume(struct device *dev)
 }
 #endif

-static const struct dev_pm_ops pwm_backlight_pm_ops = {
-#ifdef CONFIG_PM_SLEEP
-       .suspend = pwm_backlight_suspend,
-       .resume = pwm_backlight_resume,
-       .poweroff = pwm_backlight_suspend,
-       .restore = pwm_backlight_resume,
-#endif
-};
+static SIMPLE_DEV_PM_OPS(pwm_bl_pm_ops, pwm_bl_suspend, pwm_bl_resume);
Best regards,
Jingoo Han
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help