LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] leds: Convert bd2802 driver to dev_pm_ops
@ 2011-01-20 21:36 Mark Brown
  2011-01-20 23:12 ` Andrew Morton
  0 siblings, 1 reply; 5+ messages in thread
From: Mark Brown @ 2011-01-20 21:36 UTC (permalink / raw)
  To: Kim Kyuwon, Kim Kyuwon, Richard Purdie, Andrew Morton
  Cc: linux-kernel, Mark Brown

There is a move to deprecate bus-specific PM operations and move to
using dev_pm_ops instead in order to reduce the amount of boilerplate
code in buses and facilitiate updates to the PM core. Do this move for
the bs2802 driver.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/leds/leds-bd2802.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/leds/leds-bd2802.c b/drivers/leds/leds-bd2802.c
index 19dc4b6..22152a2 100644
--- a/drivers/leds/leds-bd2802.c
+++ b/drivers/leds/leds-bd2802.c
@@ -19,7 +19,7 @@
 #include <linux/leds.h>
 #include <linux/leds-bd2802.h>
 #include <linux/slab.h>
-
+#include <linux/pm.h>
 
 #define LED_CTL(rgb2en, rgb1en) ((rgb2en) << 4 | ((rgb1en) << 0))
 
@@ -761,8 +761,9 @@ static int __exit bd2802_remove(struct i2c_client *client)
 	return 0;
 }
 
-static int bd2802_suspend(struct i2c_client *client, pm_message_t mesg)
+static int bd2802_suspend(struct device *dev)
 {
+	struct i2c_client *client = to_i2c_client(dev);
 	struct bd2802_led *led = i2c_get_clientdata(client);
 
 	gpio_set_value(led->pdata->reset_gpio, 0);
@@ -770,8 +771,9 @@ static int bd2802_suspend(struct i2c_client *client, pm_message_t mesg)
 	return 0;
 }
 
-static int bd2802_resume(struct i2c_client *client)
+static int bd2802_resume(struct device *dev)
 {
+	struct i2c_client *client = to_i2c_client(dev);
 	struct bd2802_led *led = i2c_get_clientdata(client);
 
 	if (!bd2802_is_all_off(led) || led->adf_on) {
@@ -782,6 +784,8 @@ static int bd2802_resume(struct i2c_client *client)
 	return 0;
 }
 
+static SIMPLE_DEV_PM_OPS(bd2802_pm, bd2802_suspend, bd2802_resume);
+
 static const struct i2c_device_id bd2802_id[] = {
 	{ "BD2802", 0 },
 	{ }
@@ -791,11 +795,10 @@ MODULE_DEVICE_TABLE(i2c, bd2802_id);
 static struct i2c_driver bd2802_i2c_driver = {
 	.driver	= {
 		.name	= "BD2802",
+		.pm	= &bd2802_pm,
 	},
 	.probe		= bd2802_probe,
 	.remove		= __exit_p(bd2802_remove),
-	.suspend	= bd2802_suspend,
-	.resume		= bd2802_resume,
 	.id_table	= bd2802_id,
 };
 
-- 
1.7.2.3


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] leds: Convert bd2802 driver to dev_pm_ops
  2011-01-20 21:36 [PATCH] leds: Convert bd2802 driver to dev_pm_ops Mark Brown
@ 2011-01-20 23:12 ` Andrew Morton
  2011-01-20 23:24   ` Mark Brown
  0 siblings, 1 reply; 5+ messages in thread
From: Andrew Morton @ 2011-01-20 23:12 UTC (permalink / raw)
  To: Mark Brown; +Cc: Kim Kyuwon, Kim Kyuwon, Richard Purdie, linux-kernel

On Thu, 20 Jan 2011 21:36:35 +0000
Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> There is a move to deprecate bus-specific PM operations and move to
> using dev_pm_ops instead in order to reduce the amount of boilerplate
> code in buses and facilitiate updates to the PM core. Do this move for
> the bs2802 driver.
> 

CONFIG_PM=n:

drivers/leds/leds-bd2802.c:765: warning: 'bd2802_suspend' defined but not used
drivers/leds/leds-bd2802.c:775: warning: 'bd2802_resume' defined but not used

It would be nice to fix all this via automagic within the
SIMPLE_DEV_PM_OPS() implementation but I can't see a way of doing that :(



--- a/drivers/leds/leds-bd2802.c~leds-convert-bd2802-driver-to-dev_pm_ops-fix
+++ a/drivers/leds/leds-bd2802.c
@@ -319,20 +319,6 @@ static void bd2802_turn_off(struct bd280
 	bd2802_update_state(led, id, color, BD2802_OFF);
 }
 
-static void bd2802_restore_state(struct bd2802_led *led)
-{
-	int i;
-
-	for (i = 0; i < LED_NUM; i++) {
-		if (led->led[i].r)
-			bd2802_turn_on(led, i, RED, led->led[i].r);
-		if (led->led[i].g)
-			bd2802_turn_on(led, i, GREEN, led->led[i].g);
-		if (led->led[i].b)
-			bd2802_turn_on(led, i, BLUE, led->led[i].b);
-	}
-}
-
 #define BD2802_SET_REGISTER(reg_addr, reg_name)				\
 static ssize_t bd2802_store_reg##reg_addr(struct device *dev,		\
 	struct device_attribute *attr, const char *buf, size_t count)	\
@@ -761,6 +747,22 @@ static int __exit bd2802_remove(struct i
 	return 0;
 }
 
+#ifdef CONFIG_PM
+
+static void bd2802_restore_state(struct bd2802_led *led)
+{
+	int i;
+
+	for (i = 0; i < LED_NUM; i++) {
+		if (led->led[i].r)
+			bd2802_turn_on(led, i, RED, led->led[i].r);
+		if (led->led[i].g)
+			bd2802_turn_on(led, i, GREEN, led->led[i].g);
+		if (led->led[i].b)
+			bd2802_turn_on(led, i, BLUE, led->led[i].b);
+	}
+}
+
 static int bd2802_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
@@ -785,6 +787,10 @@ static int bd2802_resume(struct device *
 }
 
 static SIMPLE_DEV_PM_OPS(bd2802_pm, bd2802_suspend, bd2802_resume);
+#define BD2802_PM (&bd2802_pm)
+#else		/* CONFIG_PM */
+#define BD2802_PM NULL
+#endif
 
 static const struct i2c_device_id bd2802_id[] = {
 	{ "BD2802", 0 },
@@ -795,7 +801,7 @@ MODULE_DEVICE_TABLE(i2c, bd2802_id);
 static struct i2c_driver bd2802_i2c_driver = {
 	.driver	= {
 		.name	= "BD2802",
-		.pm	= &bd2802_pm,
+		.pm	= BD2802_PM,
 	},
 	.probe		= bd2802_probe,
 	.remove		= __exit_p(bd2802_remove),
_


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] leds: Convert bd2802 driver to dev_pm_ops
  2011-01-20 23:12 ` Andrew Morton
@ 2011-01-20 23:24   ` Mark Brown
  2011-01-20 23:35     ` Andrew Morton
  0 siblings, 1 reply; 5+ messages in thread
From: Mark Brown @ 2011-01-20 23:24 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Kim Kyuwon, Kim Kyuwon, Richard Purdie, linux-kernel

On Thu, Jan 20, 2011 at 03:12:01PM -0800, Andrew Morton wrote:

> CONFIG_PM=n:

To be honest I've been forming the opinion that this is just cruft these
days - it's hard to see a modern Linux system where you're sufficiently
space constrained to want to turn it off without also being sufficiently
power constrained to want to turn it on and it's hassle to maintain it.
That said...

> It would be nice to fix all this via automagic within the
> SIMPLE_DEV_PM_OPS() implementation but I can't see a way of doing that :(

...the problem here is that the macro is doing roughly the right magic
but the original driver wasn't ifdefing the suspend and resume stuff at
all.  If the were only defining the suspend and resume functions under
CONFIG_PM_SLEEP it should build cleanly.  Since the original driver
didn't have the ifdefs I didn't add or update them.

This means the pm_ops can be unconditionally defined which seems to be
the preferred idiom for this stuff.  If SIMPLE_DEV_PM_OPS() didn't do
the stuff it's doing then the warnings would vanish in the same way they
did originally, by virtue of the functions being unconditionally
referenced from the vtable.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] leds: Convert bd2802 driver to dev_pm_ops
  2011-01-20 23:24   ` Mark Brown
@ 2011-01-20 23:35     ` Andrew Morton
  2011-01-21 12:13       ` Mark Brown
  0 siblings, 1 reply; 5+ messages in thread
From: Andrew Morton @ 2011-01-20 23:35 UTC (permalink / raw)
  To: Mark Brown; +Cc: Kim Kyuwon, Kim Kyuwon, Richard Purdie, linux-kernel

On Thu, 20 Jan 2011 23:24:09 +0000
Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> On Thu, Jan 20, 2011 at 03:12:01PM -0800, Andrew Morton wrote:
> 
> > CONFIG_PM=n:
> 
> To be honest I've been forming the opinion that this is just cruft these
> days - it's hard to see a modern Linux system where you're sufficiently
> space constrained to want to turn it off without also being sufficiently
> power constrained to want to turn it on and it's hassle to maintain it.

Could be.  It's hard to think of a machine which is small enough to
care about ~10k of kernel text but which doesn't care about power
consumption.

> That said...
> 
> > It would be nice to fix all this via automagic within the
> > SIMPLE_DEV_PM_OPS() implementation but I can't see a way of doing that :(
> 
> ...the problem here is that the macro is doing roughly the right magic
> but the original driver wasn't ifdefing the suspend and resume stuff at
> all.  If the were only defining the suspend and resume functions under
> CONFIG_PM_SLEEP it should build cleanly.  Since the original driver
> didn't have the ifdefs I didn't add or update them.
> 
> This means the pm_ops can be unconditionally defined which seems to be
> the preferred idiom for this stuff.  If SIMPLE_DEV_PM_OPS() didn't do
> the stuff it's doing then the warnings would vanish in the same way they
> did originally, by virtue of the functions being unconditionally
> referenced from the vtable.

It's tricky to get the compiler/linker to discard code and data without
triggering an unused-var warning.  cpu_notifier() does it by emitting a
reference:

static void foo(..)
{
	...
}

bar()
{	
	...
	do { (void)(foo); } while (0)
	...
}

but this is only possible because cpu_notifier() is "called" within a
function.  Whereas SIMPLE_DEV_PM_OPS() is a static initialisation
thingy.

We could emit a dummy function but then we get a warning about the
unused dummy function.

Can the dummy function refer to itself and thus squish the warning?

akpm:/home/akpm> cat t.c
static void foo(void)
{
	        do { (void)(foo); } while (0);
}
akpm:/home/akpm> gcc -c -Wall t.c
t.c:1: warning: 'foo' defined but not used

Nope.


This works.  lol.

akpm:/home/akpm> cat t.c
static void bar(void);
static void foo(void)
{
        do { (void)(bar); } while (0);
}
static void bar(void)
{
        do { (void)(foo); } while (0);
}
akpm:/home/akpm> gcc -O2 -c -Wall t.c
akpm:/home/akpm> size t.o
   text    data     bss     dec     hex filename
      0       0       0       0       0 t.o


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] leds: Convert bd2802 driver to dev_pm_ops
  2011-01-20 23:35     ` Andrew Morton
@ 2011-01-21 12:13       ` Mark Brown
  0 siblings, 0 replies; 5+ messages in thread
From: Mark Brown @ 2011-01-21 12:13 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Kim Kyuwon, Kim Kyuwon, Richard Purdie, linux-kernel

On Thu, Jan 20, 2011 at 03:35:28PM -0800, Andrew Morton wrote:

> It's tricky to get the compiler/linker to discard code and data without
> triggering an unused-var warning.  cpu_notifier() does it by emitting a
> reference:

SIMPLE_DEV_PM_OPS() is being a bit more brute force than that - it
just defines out the reference if compiled without PM_SLEEP.  You still
need the ifdefs around the function definitions, but not their uses.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2011-01-21 12:13 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-20 21:36 [PATCH] leds: Convert bd2802 driver to dev_pm_ops Mark Brown
2011-01-20 23:12 ` Andrew Morton
2011-01-20 23:24   ` Mark Brown
2011-01-20 23:35     ` Andrew Morton
2011-01-21 12:13       ` Mark Brown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).