Patchwork [1/3] mfd: Fix compile errors and warnings when !CONFIG_AB8500_BM

login
register
mail settings
Submitter Lee Jones
Date Jan. 9, 2013, 10:06 a.m.
Message ID <1357725965-27342-1-git-send-email-lee.jones@linaro.org>
Download mbox | patch
Permalink /patch/210680/
State New
Headers show

Comments

Lee Jones - Jan. 9, 2013, 10:06 a.m.
drivers/mfd/ab8500-core.c:1015:21: error: ‘ab8500_bm_data’ undeclared here

include/linux/mfd/abx500/ab8500-bm.h:445:13: warning: ‘ab8500_fg_reinit’ defined but not used
include/linux/mfd/abx500/ab8500-bm.h:448:13: warning: ‘ab8500_charger_usb_state_changed’ defined but not used
include/linux/mfd/abx500/ab8500-bm.h:451:29: warning: ‘ab8500_btemp_get’ defined but not used
include/linux/mfd/abx500/ab8500-bm.h:455:12: warning: ‘ab8500_btemp_get_batctrl_temp’ defined but not used
include/linux/mfd/abx500/ab8500-bm.h:463:12: warning: ‘ab8500_fg_inst_curr_blocking’ defined but not used
include/linux/mfd/abx500/ab8500-bm.h:442:12: warning: ‘ab8500_fg_inst_curr_done’ defined but not used
include/linux/mfd/abx500/ab8500-bm.h:447:26: warning: ‘ab8500_fg_get’ defined but not used

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mfd/ab8500-core.c            |    1 +
 include/linux/mfd/abx500.h           |    2 --
 include/linux/mfd/abx500/ab8500-bm.h |   29 ++++-------------------------
 3 files changed, 5 insertions(+), 27 deletions(-)
Arnd Bergmann - Jan. 9, 2013, 10:18 a.m.
On Wednesday 09 January 2013, Lee Jones wrote:
> diff --git a/arch/arm/mach-ux500/Kconfig b/arch/arm/mach-ux500/Kconfig
> index 5dea906..0f25b07 100644
> --- a/arch/arm/mach-ux500/Kconfig
> +++ b/arch/arm/mach-ux500/Kconfig
> @@ -43,6 +43,8 @@ config MACH_HREFV60
>  config MACH_SNOWBALL
>         bool "U8500 Snowball platform"
>         select MACH_MOP500
> +       select LEDS_TRIGGERS
> +       select LEDS_TRIGGER_HEARTBEAT
>         help
>           Include support for the snowball development platform.

Be careful with "select" statements like this when there are other
dependencies. I think LEDS_TRIGGERS depends on LEDS_CLASS, so this
should probably be written as

	select LEDS_TRIGGERS if LEDS_CLASS

	Arnd
Russell King - ARM Linux - Jan. 9, 2013, 10:35 a.m.
On Wed, Jan 09, 2013 at 10:18:54AM +0000, Arnd Bergmann wrote:
> On Wednesday 09 January 2013, Lee Jones wrote:
> > diff --git a/arch/arm/mach-ux500/Kconfig b/arch/arm/mach-ux500/Kconfig
> > index 5dea906..0f25b07 100644
> > --- a/arch/arm/mach-ux500/Kconfig
> > +++ b/arch/arm/mach-ux500/Kconfig
> > @@ -43,6 +43,8 @@ config MACH_HREFV60
> >  config MACH_SNOWBALL
> >         bool "U8500 Snowball platform"
> >         select MACH_MOP500
> > +       select LEDS_TRIGGERS
> > +       select LEDS_TRIGGER_HEARTBEAT
> >         help
> >           Include support for the snowball development platform.
> 
> Be careful with "select" statements like this when there are other
> dependencies. I think LEDS_TRIGGERS depends on LEDS_CLASS, so this
> should probably be written as
> 
> 	select LEDS_TRIGGERS if LEDS_CLASS

Umm, this is also wrong for another reason.  LEDS is an optional feature.
It's not required for the platform to be functional.  So why the hell would
anyone want to _force_ such an optional feature to be mandatory?

This is something that the default config published for the platform should
be handling.
Lee Jones - Jan. 9, 2013, 11:08 a.m.
On Wed, 09 Jan 2013, Russell King - ARM Linux wrote:

> On Wed, Jan 09, 2013 at 10:18:54AM +0000, Arnd Bergmann wrote:
> > On Wednesday 09 January 2013, Lee Jones wrote:
> > > diff --git a/arch/arm/mach-ux500/Kconfig b/arch/arm/mach-ux500/Kconfig
> > > index 5dea906..0f25b07 100644
> > > --- a/arch/arm/mach-ux500/Kconfig
> > > +++ b/arch/arm/mach-ux500/Kconfig
> > > @@ -43,6 +43,8 @@ config MACH_HREFV60
> > >  config MACH_SNOWBALL
> > >         bool "U8500 Snowball platform"
> > >         select MACH_MOP500
> > > +       select LEDS_TRIGGERS
> > > +       select LEDS_TRIGGER_HEARTBEAT
> > >         help
> > >           Include support for the snowball development platform.
> > 
> > Be careful with "select" statements like this when there are other
> > dependencies. I think LEDS_TRIGGERS depends on LEDS_CLASS, so this
> > should probably be written as
> > 
> > 	select LEDS_TRIGGERS if LEDS_CLASS
> 
> Umm, this is also wrong for another reason.  LEDS is an optional feature.
> It's not required for the platform to be functional.  So why the hell would
> anyone want to _force_ such an optional feature to be mandatory?
> 
> This is something that the default config published for the platform should
> be handling.

Do you mean <config_dir>/u8500_defconfig?

If so, that's wrong, as it covers many more boards than just Snowball
and this is only applicable on the Snowball board. So how else can
I enable this feature?
Russell King - ARM Linux - Jan. 9, 2013, 11:12 a.m.
On Wed, Jan 09, 2013 at 11:08:40AM +0000, Lee Jones wrote:
> On Wed, 09 Jan 2013, Russell King - ARM Linux wrote:
> 
> > On Wed, Jan 09, 2013 at 10:18:54AM +0000, Arnd Bergmann wrote:
> > > On Wednesday 09 January 2013, Lee Jones wrote:
> > > > diff --git a/arch/arm/mach-ux500/Kconfig b/arch/arm/mach-ux500/Kconfig
> > > > index 5dea906..0f25b07 100644
> > > > --- a/arch/arm/mach-ux500/Kconfig
> > > > +++ b/arch/arm/mach-ux500/Kconfig
> > > > @@ -43,6 +43,8 @@ config MACH_HREFV60
> > > >  config MACH_SNOWBALL
> > > >         bool "U8500 Snowball platform"
> > > >         select MACH_MOP500
> > > > +       select LEDS_TRIGGERS
> > > > +       select LEDS_TRIGGER_HEARTBEAT
> > > >         help
> > > >           Include support for the snowball development platform.
> > > 
> > > Be careful with "select" statements like this when there are other
> > > dependencies. I think LEDS_TRIGGERS depends on LEDS_CLASS, so this
> > > should probably be written as
> > > 
> > > 	select LEDS_TRIGGERS if LEDS_CLASS
> > 
> > Umm, this is also wrong for another reason.  LEDS is an optional feature.
> > It's not required for the platform to be functional.  So why the hell would
> > anyone want to _force_ such an optional feature to be mandatory?
> > 
> > This is something that the default config published for the platform should
> > be handling.
> 
> Do you mean <config_dir>/u8500_defconfig?
> 
> If so, that's wrong, as it covers many more boards than just Snowball
> and this is only applicable on the Snowball board. So how else can
> I enable this feature?

And why is it wrong to enable an optional feature in a defconfig which some
boards using that defconfig may want?

It's _less_ wrong than forcing user selectable options to be mandatorily
selected.
Arnd Bergmann - Jan. 9, 2013, 11:56 a.m.
On Wednesday 09 January 2013, Russell King - ARM Linux wrote:
> > If so, that's wrong, as it covers many more boards than just Snowball
> > and this is only applicable on the Snowball board. So how else can
> > I enable this feature?
> 
> And why is it wrong to enable an optional feature in a defconfig which some
> boards using that defconfig may want?
> 
> It's less wrong than forcing user selectable options to be mandatorily
> selected.

Right. The change I suggested fixes the build errors but doesn't actually
make the intended behavior correct. Just using a configuration file is the
right solution here.

It may be a good use case for the "suggest" Kconfig statement I brought
up before: Rather than having snowball hard enable the LEDS subsystem,
it could 'suggest LEDS_CORE' in order to change the default for that
option, but still let users disable it if they want to.

	Arnd
Russell King - ARM Linux - Jan. 9, 2013, 11:59 a.m.
On Wed, Jan 09, 2013 at 11:56:55AM +0000, Arnd Bergmann wrote:
> On Wednesday 09 January 2013, Russell King - ARM Linux wrote:
> > > If so, that's wrong, as it covers many more boards than just Snowball
> > > and this is only applicable on the Snowball board. So how else can
> > > I enable this feature?
> > 
> > And why is it wrong to enable an optional feature in a defconfig which some
> > boards using that defconfig may want?
> > 
> > It's less wrong than forcing user selectable options to be mandatorily
> > selected.
> 
> Right. The change I suggested fixes the build errors but doesn't actually
> make the intended behavior correct. Just using a configuration file is the
> right solution here.

Hang on a moment: if the lack of the LEDs support leads to build errors,
doesn't that mean there's missing conditional compilation?
Arnd Bergmann - Jan. 9, 2013, 12:02 p.m.
On Wednesday 09 January 2013, Russell King - ARM Linux wrote:
> On Wed, Jan 09, 2013 at 11:56:55AM +0000, Arnd Bergmann wrote:

> > Right. The change I suggested fixes the build errors but doesn't actually
> > make the intended behavior correct. Just using a configuration file is the
> > right solution here.
> 
> Hang on a moment: if the lack of the LEDs support leads to build errors,
> doesn't that mean there's missing conditional compilation?

The build error would have come only from selecting LEDS_TRIGGERS without
also selecting NEW_LEDS and LEDS_CLASS. I haven't actually tried if that
results in a hard error, but you'd get at least a Kconfig warning.

	Arnd
Lee Jones - Jan. 9, 2013, 12:23 p.m.
On Wed, 09 Jan 2013, Russell King - ARM Linux wrote:

> On Wed, Jan 09, 2013 at 11:08:40AM +0000, Lee Jones wrote:
> > On Wed, 09 Jan 2013, Russell King - ARM Linux wrote:
> > 
> > > On Wed, Jan 09, 2013 at 10:18:54AM +0000, Arnd Bergmann wrote:
> > > > On Wednesday 09 January 2013, Lee Jones wrote:
> > > > > diff --git a/arch/arm/mach-ux500/Kconfig b/arch/arm/mach-ux500/Kconfig
> > > > > index 5dea906..0f25b07 100644
> > > > > --- a/arch/arm/mach-ux500/Kconfig
> > > > > +++ b/arch/arm/mach-ux500/Kconfig
> > > > > @@ -43,6 +43,8 @@ config MACH_HREFV60
> > > > >  config MACH_SNOWBALL
> > > > >         bool "U8500 Snowball platform"
> > > > >         select MACH_MOP500
> > > > > +       select LEDS_TRIGGERS
> > > > > +       select LEDS_TRIGGER_HEARTBEAT
> > > > >         help
> > > > >           Include support for the snowball development platform.
> > > > 
> > > > Be careful with "select" statements like this when there are other
> > > > dependencies. I think LEDS_TRIGGERS depends on LEDS_CLASS, so this
> > > > should probably be written as
> > > > 
> > > > 	select LEDS_TRIGGERS if LEDS_CLASS
> > > 
> > > Umm, this is also wrong for another reason.  LEDS is an optional feature.
> > > It's not required for the platform to be functional.  So why the hell would
> > > anyone want to _force_ such an optional feature to be mandatory?
> > > 
> > > This is something that the default config published for the platform should
> > > be handling.
> > 
> > Do you mean <config_dir>/u8500_defconfig?
> > 
> > If so, that's wrong, as it covers many more boards than just Snowball
> > and this is only applicable on the Snowball board. So how else can
> > I enable this feature?
> 
> And why is it wrong to enable an optional feature in a defconfig which some
> boards using that defconfig may want?
> 
> It's _less_ wrong than forcing user selectable options to be mandatorily
> selected.

Excuse my ignorance, but I'm a little confused by this.

What's the difference between 'select <OPTION>' in the Kconfig and
'CONFIG_<OPTION>=y' in the defconfig; besides the fact that if we
do it in the Kconfig file, we can be more selective with regards to
which platform it gets enabled on?
Russell King - ARM Linux - Jan. 9, 2013, 12:38 p.m.
On Wed, Jan 09, 2013 at 12:23:23PM +0000, Lee Jones wrote:
> Excuse my ignorance, but I'm a little confused by this.
> 
> What's the difference between 'select <OPTION>' in the Kconfig and
> 'CONFIG_<OPTION>=y' in the defconfig; besides the fact that if we
> do it in the Kconfig file, we can be more selective with regards to
> which platform it gets enabled on?

Take this in Kconfig:

config FOO
	bool "FOO option"
	select BAR

config BAR
	bool "BAR option"

Now, irrespective of the default configuration file being used:
- if you don't enable FOO, then you can enable _and_ _disable_ BAR according
  to your needs.
- if you enable FOO, then BAR will be _forcefully_ enabled and you can't
  turn it off without first disabling FOO.

The default configuration file will specify the _default_ values for these
options, but if FOO ends up being enabled, BAR will be forcefully enabled
irrespective of what's in the configuration file.

With this instead:

config FOO
	bool "FOO option"

config BAR
	bool "BAR option"

Then, the two options are independent.  They can be enabled and disabled
by the configuration completely independently.  However, their default
values come from the default configuration file.  So, if the config file
has:

CONFIG_FOO=y
CONFIG_BAR=y

and you do a 'make oldconfig' then they will remain set.  If you use one
of the configuration editing tools, you'll be presented with them already
enabled, and you can turn them off independently.

So, putting this stuff in the default configuration file allows
_non-mandatory_ options to be disabled should the user desire without the
user having to edit the configuration files.

If a user has to edit the configuration files in order to configure the
kernel as they desire, then the configuration system has failed - or we
have failed to properly think out how to represent the allowable
configurations.
Lee Jones - Jan. 9, 2013, 1 p.m.
> > Excuse my ignorance, but I'm a little confused by this.
> > 
> > What's the difference between 'select <OPTION>' in the Kconfig and
> > 'CONFIG_<OPTION>=y' in the defconfig; besides the fact that if we
> > do it in the Kconfig file, we can be more selective with regards to
> > which platform it gets enabled on?
> 
> Take this in Kconfig:
> 
> config FOO
> 	bool "FOO option"
> 	select BAR
> 
> config BAR
> 	bool "BAR option"
> 
> Now, irrespective of the default configuration file being used:
> - if you don't enable FOO, then you can enable _and_ _disable_ BAR according
>   to your needs.
> - if you enable FOO, then BAR will be _forcefully_ enabled and you can't
>   turn it off without first disabling FOO.
> 
> The default configuration file will specify the _default_ values for these
> options, but if FOO ends up being enabled, BAR will be forcefully enabled
> irrespective of what's in the configuration file.
> 
> With this instead:
> 
> config FOO
> 	bool "FOO option"
> 
> config BAR
> 	bool "BAR option"
> 
> Then, the two options are independent.  They can be enabled and disabled
> by the configuration completely independently.  However, their default
> values come from the default configuration file.  So, if the config file
> has:
> 
> CONFIG_FOO=y
> CONFIG_BAR=y
> 
> and you do a 'make oldconfig' then they will remain set.  If you use one
> of the configuration editing tools, you'll be presented with them already
> enabled, and you can turn them off independently.
> 
> So, putting this stuff in the default configuration file allows
> _non-mandatory_ options to be disabled should the user desire without the
> user having to edit the configuration files.
> 
> If a user has to edit the configuration files in order to configure the
> kernel as they desire, then the configuration system has failed - or we
> have failed to properly think out how to represent the allowable
> configurations.

Understood. Thanks for the explanation.

Linus, is it okay to put these in the defconfig instead?

If so, I'll fixup.
Linus Walleij - Jan. 17, 2013, 10:22 a.m.
On Wed, Jan 9, 2013 at 2:00 PM, Lee Jones <lee.jones@linaro.org> wrote:

>> If a user has to edit the configuration files in order to configure the
>> kernel as they desire, then the configuration system has failed - or we
>> have failed to properly think out how to represent the allowable
>> configurations.
>
> Understood. Thanks for the explanation.
>
> Linus, is it okay to put these in the defconfig instead?
>
> If so, I'll fixup.

Sure, go ahead (I bet I already have a patch higher up
in my mailbox).

Yours,
Linus Walleij
Samuel Ortiz - Jan. 22, 2013, 3:26 a.m.
Hi Lee,

On Wed, Jan 09, 2013 at 10:06:03AM +0000, Lee Jones wrote:
> drivers/mfd/ab8500-core.c:1015:21: error: ‘ab8500_bm_data’ undeclared here
> 
> include/linux/mfd/abx500/ab8500-bm.h:445:13: warning: ‘ab8500_fg_reinit’ defined but not used
> include/linux/mfd/abx500/ab8500-bm.h:448:13: warning: ‘ab8500_charger_usb_state_changed’ defined but not used
> include/linux/mfd/abx500/ab8500-bm.h:451:29: warning: ‘ab8500_btemp_get’ defined but not used
> include/linux/mfd/abx500/ab8500-bm.h:455:12: warning: ‘ab8500_btemp_get_batctrl_temp’ defined but not used
> include/linux/mfd/abx500/ab8500-bm.h:463:12: warning: ‘ab8500_fg_inst_curr_blocking’ defined but not used
> include/linux/mfd/abx500/ab8500-bm.h:442:12: warning: ‘ab8500_fg_inst_curr_done’ defined but not used
> include/linux/mfd/abx500/ab8500-bm.h:447:26: warning: ‘ab8500_fg_get’ defined but not used
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/mfd/ab8500-core.c            |    1 +
>  include/linux/mfd/abx500.h           |    2 --
>  include/linux/mfd/abx500/ab8500-bm.h |   29 ++++-------------------------
>  3 files changed, 5 insertions(+), 27 deletions(-)
Applied to my for-linus branch, thanks.

Cheers,
Samuel.
Linus Walleij - Jan. 23, 2013, 9:36 a.m.
On Wed, Jan 9, 2013 at 11:06 AM, Lee Jones <lee.jones@linaro.org> wrote:

> Here we setup the GPIO pin responsible for illuminating the user
> LED on the Snowball low-cost development board.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Patch applied to my ux500 tree, branch ux500-pinctrl.

Yours,
Linus Walleij
Lee Jones - Feb. 26, 2013, 10:32 a.m.
On Wed, 23 Jan 2013, Linus Walleij wrote:

> On Wed, Jan 9, 2013 at 11:06 AM, Lee Jones <lee.jones@linaro.org> wrote:
> 
> > Here we setup the GPIO pin responsible for illuminating the user
> > LED on the Snowball low-cost development board.
> >
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> Patch applied to my ux500 tree, branch ux500-pinctrl.

Perhaps you can apply this to the for-next branch you pulled from me?

Patch

diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
index e1650ba..4778bb1 100644
--- a/drivers/mfd/ab8500-core.c
+++ b/drivers/mfd/ab8500-core.c
@@ -19,6 +19,7 @@ 
 #include <linux/mfd/core.h>
 #include <linux/mfd/abx500.h>
 #include <linux/mfd/abx500/ab8500.h>
+#include <linux/mfd/abx500/ab8500-bm.h>
 #include <linux/mfd/dbx500-prcmu.h>
 #include <linux/regulator/ab8500.h>
 #include <linux/of.h>
diff --git a/include/linux/mfd/abx500.h b/include/linux/mfd/abx500.h
index 2138bd3..e53dcfe 100644
--- a/include/linux/mfd/abx500.h
+++ b/include/linux/mfd/abx500.h
@@ -272,8 +272,6 @@  struct abx500_bm_data {
 	const struct abx500_fg_parameters *fg_params;
 };
 
-extern struct abx500_bm_data ab8500_bm_data;
-
 enum {
 	NTC_EXTERNAL = 0,
 	NTC_INTERNAL,
diff --git a/include/linux/mfd/abx500/ab8500-bm.h b/include/linux/mfd/abx500/ab8500-bm.h
index 44310c9..9bd037d 100644
--- a/include/linux/mfd/abx500/ab8500-bm.h
+++ b/include/linux/mfd/abx500/ab8500-bm.h
@@ -422,7 +422,10 @@  struct ab8500_chargalg_platform_data {
 struct ab8500_btemp;
 struct ab8500_gpadc;
 struct ab8500_fg;
+
 #ifdef CONFIG_AB8500_BM
+extern struct abx500_bm_data ab8500_bm_data;
+
 void ab8500_fg_reinit(void);
 void ab8500_charger_usb_state_changed(u8 bm_usb_state, u16 mA);
 struct ab8500_btemp *ab8500_btemp_get(void);
@@ -434,31 +437,7 @@  int ab8500_fg_inst_curr_finalize(struct ab8500_fg *di, int *res);
 int ab8500_fg_inst_curr_done(struct ab8500_fg *di);
 
 #else
-int ab8500_fg_inst_curr_done(struct ab8500_fg *di)
-{
-}
-static void ab8500_fg_reinit(void)
-{
-}
-static void ab8500_charger_usb_state_changed(u8 bm_usb_state, u16 mA)
-{
-}
-static struct ab8500_btemp *ab8500_btemp_get(void)
-{
-	return NULL;
-}
-static int ab8500_btemp_get_batctrl_temp(struct ab8500_btemp *btemp)
-{
-	return 0;
-}
-struct ab8500_fg *ab8500_fg_get(void)
-{
-	return NULL;
-}
-static int ab8500_fg_inst_curr_blocking(struct ab8500_fg *dev)
-{
-	return -ENODEV;
-}
+static struct abx500_bm_data ab8500_bm_data;
 
 static inline int ab8500_fg_inst_curr_start(struct ab8500_fg *di)
 {