* [PATCH] leds: Fix warnings when PM is disabled for BD2802
@ 2011-01-21 17:33 Mark Brown
2011-01-21 22:14 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Mark Brown @ 2011-01-21 17:33 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, Richard Purdie, Mark Brown
The suspend and resume functions will only be referenced when PM is
enabled so they generate warnings due to being unreferenced and
unexported.
Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
This is the more usual idiom for fixing this than the patch you added.
drivers/leds/leds-bd2802.c | 30 ++++++++++++++++--------------
1 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/drivers/leds/leds-bd2802.c b/drivers/leds/leds-bd2802.c
index 22152a2..ef0b868 100644
--- a/drivers/leds/leds-bd2802.c
+++ b/drivers/leds/leds-bd2802.c
@@ -319,20 +319,6 @@ static void bd2802_turn_off(struct bd2802_led *led, enum led_ids id,
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,7 @@ static int __exit bd2802_remove(struct i2c_client *client)
return 0;
}
+#ifdef CONFIG_PM_SLEEP
static int bd2802_suspend(struct device *dev)
{
struct i2c_client *client = to_i2c_client(dev);
@@ -771,6 +758,20 @@ static int bd2802_suspend(struct device *dev)
return 0;
}
+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_resume(struct device *dev)
{
struct i2c_client *client = to_i2c_client(dev);
@@ -783,6 +784,7 @@ static int bd2802_resume(struct device *dev)
return 0;
}
+#endif
static SIMPLE_DEV_PM_OPS(bd2802_pm, bd2802_suspend, bd2802_resume);
--
1.7.2.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] leds: Fix warnings when PM is disabled for BD2802
2011-01-21 17:33 [PATCH] leds: Fix warnings when PM is disabled for BD2802 Mark Brown
@ 2011-01-21 22:14 ` Andrew Morton
2011-01-21 23:38 ` Dmitry Torokhov
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2011-01-21 22:14 UTC (permalink / raw)
To: Mark Brown; +Cc: linux-kernel, Richard Purdie, Rafael J. Wysocki
On Fri, 21 Jan 2011 17:33:37 +0000
Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> The suspend and resume functions will only be referenced when PM is
> enabled so they generate warnings due to being unreferenced and
> unexported.
>
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
>
> This is the more usual idiom for fixing this than the patch you added.
>
> drivers/leds/leds-bd2802.c | 30 ++++++++++++++++--------------
> 1 files changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/leds/leds-bd2802.c b/drivers/leds/leds-bd2802.c
> index 22152a2..ef0b868 100644
> --- a/drivers/leds/leds-bd2802.c
> +++ b/drivers/leds/leds-bd2802.c
> @@ -319,20 +319,6 @@ static void bd2802_turn_off(struct bd2802_led *led, enum led_ids id,
> 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,7 @@ static int __exit bd2802_remove(struct i2c_client *client)
> return 0;
> }
>
> +#ifdef CONFIG_PM_SLEEP
> static int bd2802_suspend(struct device *dev)
> {
> struct i2c_client *client = to_i2c_client(dev);
> @@ -771,6 +758,20 @@ static int bd2802_suspend(struct device *dev)
> return 0;
> }
>
> +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_resume(struct device *dev)
> {
> struct i2c_client *client = to_i2c_client(dev);
> @@ -783,6 +784,7 @@ static int bd2802_resume(struct device *dev)
>
> return 0;
> }
> +#endif
>
That rather sucks. It leaves an all-zeroes instance of dev_pm_ops
uselessly bloating the driver. And it leaves
bd2802_i2c_driver.driver.pm pointing at that all-zeroes instance of
dev_pm_ops, which is rather dangerous.
If CONFIG_PM_SLEEP=n, the .driver.pm field shouldn't exist at all.
<looks at Rafael>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] leds: Fix warnings when PM is disabled for BD2802
2011-01-21 22:14 ` Andrew Morton
@ 2011-01-21 23:38 ` Dmitry Torokhov
2011-01-21 23:47 ` Mark Brown
2011-01-22 0:23 ` Andrew Morton
0 siblings, 2 replies; 6+ messages in thread
From: Dmitry Torokhov @ 2011-01-21 23:38 UTC (permalink / raw)
To: Andrew Morton; +Cc: Mark Brown, linux-kernel, Richard Purdie, Rafael J. Wysocki
On Fri, Jan 21, 2011 at 02:14:02PM -0800, Andrew Morton wrote:
> On Fri, 21 Jan 2011 17:33:37 +0000
> Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
>
> > The suspend and resume functions will only be referenced when PM is
> > enabled so they generate warnings due to being unreferenced and
> > unexported.
> >
> > Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> > ---
> >
> > This is the more usual idiom for fixing this than the patch you added.
> >
> > drivers/leds/leds-bd2802.c | 30 ++++++++++++++++--------------
> > 1 files changed, 16 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/leds/leds-bd2802.c b/drivers/leds/leds-bd2802.c
> > index 22152a2..ef0b868 100644
> > --- a/drivers/leds/leds-bd2802.c
> > +++ b/drivers/leds/leds-bd2802.c
> > @@ -319,20 +319,6 @@ static void bd2802_turn_off(struct bd2802_led *led, enum led_ids id,
> > 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,7 @@ static int __exit bd2802_remove(struct i2c_client *client)
> > return 0;
> > }
> >
> > +#ifdef CONFIG_PM_SLEEP
> > static int bd2802_suspend(struct device *dev)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > @@ -771,6 +758,20 @@ static int bd2802_suspend(struct device *dev)
> > return 0;
> > }
> >
> > +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_resume(struct device *dev)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > @@ -783,6 +784,7 @@ static int bd2802_resume(struct device *dev)
> >
> > return 0;
> > }
> > +#endif
> >
>
> That rather sucks. It leaves an all-zeroes instance of dev_pm_ops
> uselessly bloating the driver.
It is per-driver, not per device so do we really care?
> And it leaves
> bd2802_i2c_driver.driver.pm pointing at that all-zeroes instance of
> dev_pm_ops, which is rather dangerous.
Nothing dagerous here - PM core deals with half-filled pm_ops just fine.
>
> If CONFIG_PM_SLEEP=n, the .driver.pm field shouldn't exist at all.
Meh, we have _waaay_ too many config options, I'd rather see CONFIG_PM
and possibly CONFIG_PM_SLEEP go, maybe leaving us with
CONFIG_PM_RUNTIME and maybe not. How many devices out there do not want
PM?
--
Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] leds: Fix warnings when PM is disabled for BD2802
2011-01-21 23:38 ` Dmitry Torokhov
@ 2011-01-21 23:47 ` Mark Brown
2011-01-22 10:21 ` Rafael J. Wysocki
2011-01-22 0:23 ` Andrew Morton
1 sibling, 1 reply; 6+ messages in thread
From: Mark Brown @ 2011-01-21 23:47 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Andrew Morton, linux-kernel, Richard Purdie, Rafael J. Wysocki
On Fri, Jan 21, 2011 at 03:38:37PM -0800, Dmitry Torokhov wrote:
> On Fri, Jan 21, 2011 at 02:14:02PM -0800, Andrew Morton wrote:
> > And it leaves
> > bd2802_i2c_driver.driver.pm pointing at that all-zeroes instance of
> > dev_pm_ops, which is rather dangerous.
> Nothing dagerous here - PM core deals with half-filled pm_ops just fine.
Indeed, all the PM operations are completly optional so there's no
problem there except for the empty dev_pm_ops we leave lying around for
each driver.
> > If CONFIG_PM_SLEEP=n, the .driver.pm field shouldn't exist at all.
> Meh, we have _waaay_ too many config options, I'd rather see CONFIG_PM
> and possibly CONFIG_PM_SLEEP go, maybe leaving us with
> CONFIG_PM_RUNTIME and maybe not. How many devices out there do not want
> PM?
I made the same point earlier; I guess I'll post an RFC patch over the
weekend.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] leds: Fix warnings when PM is disabled for BD2802
2011-01-21 23:38 ` Dmitry Torokhov
2011-01-21 23:47 ` Mark Brown
@ 2011-01-22 0:23 ` Andrew Morton
1 sibling, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2011-01-22 0:23 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Mark Brown, linux-kernel, Richard Purdie, Rafael J. Wysocki
On Fri, 21 Jan 2011 15:38:37 -0800 Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > That rather sucks. It leaves an all-zeroes instance of dev_pm_ops
> > uselessly bloating the driver.
>
> It is per-driver, not per device so do we really care?
We care about everything. If the objective was to make life easier for
ourselves, we'd all be on the golf course.
> > And it leaves
> > bd2802_i2c_driver.driver.pm pointing at that all-zeroes instance of
> > dev_pm_ops, which is rather dangerous.
>
> Nothing dagerous here - PM core deals with half-filled pm_ops just fine.
>
> >
> > If CONFIG_PM_SLEEP=n, the .driver.pm field shouldn't exist at all.
>
> Meh, we have _waaay_ too many config options, I'd rather see CONFIG_PM
> and possibly CONFIG_PM_SLEEP go, maybe leaving us with
> CONFIG_PM_RUNTIME and maybe not. How many devices out there do not want
> PM?
Don't know. How do we determine this?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] leds: Fix warnings when PM is disabled for BD2802
2011-01-21 23:47 ` Mark Brown
@ 2011-01-22 10:21 ` Rafael J. Wysocki
0 siblings, 0 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2011-01-22 10:21 UTC (permalink / raw)
To: Mark Brown; +Cc: Dmitry Torokhov, Andrew Morton, linux-kernel, Richard Purdie
On Saturday, January 22, 2011, Mark Brown wrote:
> On Fri, Jan 21, 2011 at 03:38:37PM -0800, Dmitry Torokhov wrote:
> > On Fri, Jan 21, 2011 at 02:14:02PM -0800, Andrew Morton wrote:
>
> > > And it leaves
> > > bd2802_i2c_driver.driver.pm pointing at that all-zeroes instance of
> > > dev_pm_ops, which is rather dangerous.
>
> > Nothing dagerous here - PM core deals with half-filled pm_ops just fine.
>
> Indeed, all the PM operations are completly optional so there's no
> problem there except for the empty dev_pm_ops we leave lying around for
> each driver.
>
> > > If CONFIG_PM_SLEEP=n, the .driver.pm field shouldn't exist at all.
Not if CONFIG_PM_RUNTIME is set (that field is for both SLEEP and RUNTIME).
> > Meh, we have _waaay_ too many config options, I'd rather see CONFIG_PM
> > and possibly CONFIG_PM_SLEEP go, maybe leaving us with
> > CONFIG_PM_RUNTIME and maybe not. How many devices out there do not want
> > PM?
You'd be surprised.
> I made the same point earlier; I guess I'll post an RFC patch over the
> weekend.
The truth is CONFIG_PM was a mistake, because it's practically meaningless
(it basically is always set), so we could remove it, I think, but that
would require us to modify _many_ drivers.
CONFIG_PM_SLEEP is distinctly about suspend and hibernation which people
tend to switch off sometimes.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-01-22 10:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-21 17:33 [PATCH] leds: Fix warnings when PM is disabled for BD2802 Mark Brown
2011-01-21 22:14 ` Andrew Morton
2011-01-21 23:38 ` Dmitry Torokhov
2011-01-21 23:47 ` Mark Brown
2011-01-22 10:21 ` Rafael J. Wysocki
2011-01-22 0:23 ` Andrew Morton
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.