| Message ID | 1357725965-27342-2-git-send-email-lee.jones@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| 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
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.
As Snowball is a community board it should have its default LED trigger set in 'heartbeat' mode, meaning that it double flashes ~1HZ. This provides an excellent debugging tool when community members are hacking. Signed-off-by: Lee Jones <lee.jones@linaro.org> --- arch/arm/mach-ux500/Kconfig | 2 ++ 1 file changed, 2 insertions(+)