Re: [PATCH] leds: Fix warnings when PM is disabled for BD2802

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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