| Message ID | 1357725965-27342-1-git-send-email-lee.jones@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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.
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?
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.
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
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?
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
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?
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.
> > 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.
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
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.
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
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?
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) {
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(-)