diff mbox

[U-Boot] ARM: phytec: pcm051: select board revision by Kconfig

Message ID 1433171351-20418-1-git-send-email-poeschel@lemonage.de
State Superseded
Headers show

Commit Message

Lars Poeschel June 1, 2015, 3:09 p.m. UTC 
From: Lars Poeschel <poeschel@lemonage.de>

This add a Kconfig entry that allows to set the board revision in
menuconfig. So the deprecated CONFIG_SYS_EXTRA_OPTIONS is no longer
needed for this boad.

Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
---
 board/phytec/pcm051/Kconfig   | 19 +++++++++++++++++++
 configs/pcm051_rev1_defconfig |  2 +-
 configs/pcm051_rev3_defconfig |  2 +-
 3 files changed, 21 insertions(+), 2 deletions(-)

Comments

Tom Rini June 2, 2015, 2:34 p.m. UTC | #1 
On Mon, Jun 01, 2015 at 05:09:11PM +0200, poeschel@lemonage.de wrote:

> From: Lars Poeschel <poeschel@lemonage.de>
> 
> This add a Kconfig entry that allows to set the board revision in
> menuconfig. So the deprecated CONFIG_SYS_EXTRA_OPTIONS is no longer
> needed for this boad.
> 
> Signed-off-by: Lars Poeschel <poeschel@lemonage.de>

I like the concept but CONFIG_REVx is way too generic.  Can we maybe
re-work things as CONFIG_TARGET_PCM051_REV1 / CONFIG_TARGET_PCM051_REV3
(and those select CONFIG_TARGET_PCM051) ?  Masahiro?  Thanks!

> ---
>  board/phytec/pcm051/Kconfig   | 19 +++++++++++++++++++
>  configs/pcm051_rev1_defconfig |  2 +-
>  configs/pcm051_rev3_defconfig |  2 +-
>  3 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/board/phytec/pcm051/Kconfig b/board/phytec/pcm051/Kconfig
> index 2cc0d88..c1071c6 100644
> --- a/board/phytec/pcm051/Kconfig
> +++ b/board/phytec/pcm051/Kconfig
> @@ -12,4 +12,23 @@ config SYS_SOC
>  config SYS_CONFIG_NAME
>  	default "pcm051"
>  
> +choice
> +prompt "pcm051 revision select"
> +default REV3
> +
> +config REV1
> +	bool "pcm051 revision 1 or 2"
> +	help
> +	  If you have 1358.1 written on the pcb of your pcm051, you
> +	  have a revision 1 board. Likewise if you have 1358.2 on your
> +	  board, it is a revision 2 board and this entry is for you.
> +
> +config REV3
> +	bool "pcm051 revision 3"
> +	help
> +	  If you have 1358.3 written on the pcb of your pcm051, you
> +	  have a revision 3 board and you have to select this entry.
> +
> +endchoice
> +
>  endif
> diff --git a/configs/pcm051_rev1_defconfig b/configs/pcm051_rev1_defconfig
> index af02b2f..0a28195 100644
> --- a/configs/pcm051_rev1_defconfig
> +++ b/configs/pcm051_rev1_defconfig
> @@ -1,4 +1,4 @@
>  CONFIG_ARM=y
>  CONFIG_TARGET_PCM051=y
>  CONFIG_SPL=y
> -CONFIG_SYS_EXTRA_OPTIONS="REV1"
> +CONFIG_REV1=y
> diff --git a/configs/pcm051_rev3_defconfig b/configs/pcm051_rev3_defconfig
> index 2a907d7..4ad49df 100644
> --- a/configs/pcm051_rev3_defconfig
> +++ b/configs/pcm051_rev3_defconfig
> @@ -1,4 +1,4 @@
>  CONFIG_ARM=y
>  CONFIG_TARGET_PCM051=y
>  CONFIG_SPL=y
> -CONFIG_SYS_EXTRA_OPTIONS="REV3"
> +CONFIG_REV3=y
> -- 
> 2.1.4
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Lars Poeschel June 3, 2015, 2:36 p.m. UTC | #2 
On Tue, Jun 02, 2015 at 10:34:34AM -0400, Tom Rini wrote:
> On Mon, Jun 01, 2015 at 05:09:11PM +0200, poeschel@lemonage.de wrote:
> 
> > From: Lars Poeschel <poeschel@lemonage.de>
> > 
> > This add a Kconfig entry that allows to set the board revision in
> > menuconfig. So the deprecated CONFIG_SYS_EXTRA_OPTIONS is no longer
> > needed for this boad.
> > 
> > Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
> 
> I like the concept but CONFIG_REVx is way too generic.  Can we maybe
> re-work things as CONFIG_TARGET_PCM051_REV1 / CONFIG_TARGET_PCM051_REV3
> (and those select CONFIG_TARGET_PCM051) ?  Masahiro?  Thanks!

Agree: CONFIG_REVx is too generic. I will send a version 2 of the patch,
but I don't understand why you want CONFIG_TARGET_PCM051_REV1 /
CONFIG_TARGET_PCM051_REV3 to select CONFIG_TARGET_PCM051. The
CONFIG_TARGET_PCM051_REVx's are inside an

if TARGET_PCM051
...
endif

That means, that CONFIG_TARGET_PCM051 must already be selected to make
the *_REVx's visible and selectable.

> > ---
> >  board/phytec/pcm051/Kconfig   | 19 +++++++++++++++++++
> >  configs/pcm051_rev1_defconfig |  2 +-
> >  configs/pcm051_rev3_defconfig |  2 +-
> >  3 files changed, 21 insertions(+), 2 deletions(-)
> > 
> > diff --git a/board/phytec/pcm051/Kconfig b/board/phytec/pcm051/Kconfig
> > index 2cc0d88..c1071c6 100644
> > --- a/board/phytec/pcm051/Kconfig
> > +++ b/board/phytec/pcm051/Kconfig
> > @@ -12,4 +12,23 @@ config SYS_SOC
> >  config SYS_CONFIG_NAME
> >  	default "pcm051"
> >  
> > +choice
> > +prompt "pcm051 revision select"
> > +default REV3
> > +
> > +config REV1
> > +	bool "pcm051 revision 1 or 2"
> > +	help
> > +	  If you have 1358.1 written on the pcb of your pcm051, you
> > +	  have a revision 1 board. Likewise if you have 1358.2 on your
> > +	  board, it is a revision 2 board and this entry is for you.
> > +
> > +config REV3
> > +	bool "pcm051 revision 3"
> > +	help
> > +	  If you have 1358.3 written on the pcb of your pcm051, you
> > +	  have a revision 3 board and you have to select this entry.
> > +
> > +endchoice
> > +
> >  endif
> > diff --git a/configs/pcm051_rev1_defconfig b/configs/pcm051_rev1_defconfig
> > index af02b2f..0a28195 100644
> > --- a/configs/pcm051_rev1_defconfig
> > +++ b/configs/pcm051_rev1_defconfig
> > @@ -1,4 +1,4 @@
> >  CONFIG_ARM=y
> >  CONFIG_TARGET_PCM051=y
> >  CONFIG_SPL=y
> > -CONFIG_SYS_EXTRA_OPTIONS="REV1"
> > +CONFIG_REV1=y
> > diff --git a/configs/pcm051_rev3_defconfig b/configs/pcm051_rev3_defconfig
> > index 2a907d7..4ad49df 100644
> > --- a/configs/pcm051_rev3_defconfig
> > +++ b/configs/pcm051_rev3_defconfig
> > @@ -1,4 +1,4 @@
> >  CONFIG_ARM=y
> >  CONFIG_TARGET_PCM051=y
> >  CONFIG_SPL=y
> > -CONFIG_SYS_EXTRA_OPTIONS="REV3"
> > +CONFIG_REV3=y
> > -- 
> > 2.1.4
> > 
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot@lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
> 
> -- 
> Tom

Regards,
Lars
Tom Rini June 3, 2015, 3:20 p.m. UTC | #3 
On Wed, Jun 03, 2015 at 04:36:06PM +0200, Lars Poeschel wrote:
> On Tue, Jun 02, 2015 at 10:34:34AM -0400, Tom Rini wrote:
> > On Mon, Jun 01, 2015 at 05:09:11PM +0200, poeschel@lemonage.de wrote:
> > 
> > > From: Lars Poeschel <poeschel@lemonage.de>
> > > 
> > > This add a Kconfig entry that allows to set the board revision in
> > > menuconfig. So the deprecated CONFIG_SYS_EXTRA_OPTIONS is no longer
> > > needed for this boad.
> > > 
> > > Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
> > 
> > I like the concept but CONFIG_REVx is way too generic.  Can we maybe
> > re-work things as CONFIG_TARGET_PCM051_REV1 / CONFIG_TARGET_PCM051_REV3
> > (and those select CONFIG_TARGET_PCM051) ?  Masahiro?  Thanks!
> 
> Agree: CONFIG_REVx is too generic. I will send a version 2 of the patch,
> but I don't understand why you want CONFIG_TARGET_PCM051_REV1 /
> CONFIG_TARGET_PCM051_REV3 to select CONFIG_TARGET_PCM051. The
> CONFIG_TARGET_PCM051_REVx's are inside an
> 
> if TARGET_PCM051
> ...
> endif
> 
> That means, that CONFIG_TARGET_PCM051 must already be selected to make
> the *_REVx's visible and selectable.

Right.  I mean since we must select one of these boards at build-time,
why not just ask about them up-front in arch/arm/Kconfig as rev1/rev3,
and then have the main symbol be a hidden one, ie roughly:

config TARGET_PCM051
  bool

config TARGET_PCM051_REV1
  bool "Enable pcm051 rev1"
  select TARGET_PCM051
  help
    ...

config TARGET_PCM051_REV3
  bool "Enable pcm051 rev3"
  select TARGET_PCM051
  help
    ...
Lars Poeschel June 4, 2015, 8:07 a.m. UTC | #4 
On Wed, Jun 03, 2015 at 11:20:25AM -0400, Tom Rini wrote:
> On Wed, Jun 03, 2015 at 04:36:06PM +0200, Lars Poeschel wrote:
> > On Tue, Jun 02, 2015 at 10:34:34AM -0400, Tom Rini wrote:
> > > On Mon, Jun 01, 2015 at 05:09:11PM +0200, poeschel@lemonage.de wrote:
> > > 
> > > > From: Lars Poeschel <poeschel@lemonage.de>
> > > > 
> > > > This add a Kconfig entry that allows to set the board revision in
> > > > menuconfig. So the deprecated CONFIG_SYS_EXTRA_OPTIONS is no longer
> > > > needed for this boad.
> > > > 
> > > > Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
> > > 
> > > I like the concept but CONFIG_REVx is way too generic.  Can we maybe
> > > re-work things as CONFIG_TARGET_PCM051_REV1 / CONFIG_TARGET_PCM051_REV3
> > > (and those select CONFIG_TARGET_PCM051) ?  Masahiro?  Thanks!
> > 
> > Agree: CONFIG_REVx is too generic. I will send a version 2 of the patch,
> > but I don't understand why you want CONFIG_TARGET_PCM051_REV1 /
> > CONFIG_TARGET_PCM051_REV3 to select CONFIG_TARGET_PCM051. The
> > CONFIG_TARGET_PCM051_REVx's are inside an
> > 
> > if TARGET_PCM051
> > ...
> > endif
> > 
> > That means, that CONFIG_TARGET_PCM051 must already be selected to make
> > the *_REVx's visible and selectable.
> 
> Right.  I mean since we must select one of these boards at build-time,
> why not just ask about them up-front in arch/arm/Kconfig as rev1/rev3,

I wanted the initial list you choose your board from not to grow too
big. The (unsorted) list you scroll in menuconfig is already huge! I
like the aproach "Atmel AT91" takes more, and I'd propably even go
one step further an select a manufacturer as a first step and then
in a second step select a specific board from this manufacturer.
But if you prefer the other way - no problem. I tried that:

> and then have the main symbol be a hidden one, ie roughly:
> 
> config TARGET_PCM051
>   bool
> 
> config TARGET_PCM051_REV1
>   bool "Enable pcm051 rev1"
>   select TARGET_PCM051
>   help
>     ...
> 
> config TARGET_PCM051_REV3
>   bool "Enable pcm051 rev3"
>   select TARGET_PCM051
>   help
>     ...

I could not get this approach to work. It seems Kconfig does not support
hidden config options. I took a slightly different approach. I'll send a
PATCH V2 in a second. Please have a look, if that is what what you mean.

Lars
Masahiro Yamada June 4, 2015, 5:13 p.m. UTC | #5 
Hi.

Sorry for my late reply.

2015-06-04 17:07 GMT+09:00 Lars Poeschel <poeschel@lemonage.de>:
> On Wed, Jun 03, 2015 at 11:20:25AM -0400, Tom Rini wrote:
>> On Wed, Jun 03, 2015 at 04:36:06PM +0200, Lars Poeschel wrote:
>> > On Tue, Jun 02, 2015 at 10:34:34AM -0400, Tom Rini wrote:
>> > > On Mon, Jun 01, 2015 at 05:09:11PM +0200, poeschel@lemonage.de wrote:
>> > >
>> > > > From: Lars Poeschel <poeschel@lemonage.de>
>> > > >
>> > > > This add a Kconfig entry that allows to set the board revision in
>> > > > menuconfig. So the deprecated CONFIG_SYS_EXTRA_OPTIONS is no longer
>> > > > needed for this boad.
>> > > >
>> > > > Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
>> > >
>> > > I like the concept but CONFIG_REVx is way too generic.  Can we maybe
>> > > re-work things as CONFIG_TARGET_PCM051_REV1 / CONFIG_TARGET_PCM051_REV3
>> > > (and those select CONFIG_TARGET_PCM051) ?  Masahiro?  Thanks!
>> >
>> > Agree: CONFIG_REVx is too generic. I will send a version 2 of the patch,
>> > but I don't understand why you want CONFIG_TARGET_PCM051_REV1 /
>> > CONFIG_TARGET_PCM051_REV3 to select CONFIG_TARGET_PCM051. The
>> > CONFIG_TARGET_PCM051_REVx's are inside an
>> >
>> > if TARGET_PCM051
>> > ...
>> > endif
>> >
>> > That means, that CONFIG_TARGET_PCM051 must already be selected to make
>> > the *_REVx's visible and selectable.
>>
>> Right.  I mean since we must select one of these boards at build-time,
>> why not just ask about them up-front in arch/arm/Kconfig as rev1/rev3,
>
> I wanted the initial list you choose your board from not to grow too
> big. The (unsorted) list you scroll in menuconfig is already huge! I
> like the aproach "Atmel AT91" takes more, and I'd propably even go
> one step further an select a manufacturer as a first step and then
> in a second step select a specific board from this manufacturer.

Right.
Refactoring arch/arm/Kconfig is still under way.

I think the biggest part of the remainder is Freescale boards.


> But if you prefer the other way - no problem. I tried that:
>
>> and then have the main symbol be a hidden one, ie roughly:
>>
>> config TARGET_PCM051
>>   bool
>>
>> config TARGET_PCM051_REV1
>>   bool "Enable pcm051 rev1"
>>   select TARGET_PCM051
>>   help
>>     ...
>>
>> config TARGET_PCM051_REV3
>>   bool "Enable pcm051 rev3"
>>   select TARGET_PCM051
>>   help
>>     ...
>
> I could not get this approach to work. It seems Kconfig does not support
> hidden config options. I took a slightly different approach. I'll send a
> PATCH V2 in a second. Please have a look, if that is what what you mean.


Why do we need to "select" the board?

The problem here is that CONFIG_REV1/_REV3 are too generic,
so I think just renaming them into CONFIG_PCM051_REV1/_REV3 is enough
and better.



choice
prompt "pcm051 revision select"
default REV3

config PCM051_REV1
       bool "pcm051 revision 1 or 2"
       help
         If you have 1358.1 written on the pcb of your pcm051, you
         have a revision 1 board. Likewise if you have 1358.2 on your
         board, it is a revision 2 board and this entry is for you.

config PCM051_REV3
       bool "pcm051 revision 3"
       help
         If you have 1358.3 written on the pcb of your pcm051, you
         have a revision 3 board and you have to select this entry.

endchoice
diff mbox

Patch

diff --git a/board/phytec/pcm051/Kconfig b/board/phytec/pcm051/Kconfig
index 2cc0d88..c1071c6 100644
--- a/board/phytec/pcm051/Kconfig
+++ b/board/phytec/pcm051/Kconfig
@@ -12,4 +12,23 @@  config SYS_SOC
 config SYS_CONFIG_NAME
 	default "pcm051"
 
+choice
+prompt "pcm051 revision select"
+default REV3
+
+config REV1
+	bool "pcm051 revision 1 or 2"
+	help
+	  If you have 1358.1 written on the pcb of your pcm051, you
+	  have a revision 1 board. Likewise if you have 1358.2 on your
+	  board, it is a revision 2 board and this entry is for you.
+
+config REV3
+	bool "pcm051 revision 3"
+	help
+	  If you have 1358.3 written on the pcb of your pcm051, you
+	  have a revision 3 board and you have to select this entry.
+
+endchoice
+
 endif
diff --git a/configs/pcm051_rev1_defconfig b/configs/pcm051_rev1_defconfig
index af02b2f..0a28195 100644
--- a/configs/pcm051_rev1_defconfig
+++ b/configs/pcm051_rev1_defconfig
@@ -1,4 +1,4 @@ 
 CONFIG_ARM=y
 CONFIG_TARGET_PCM051=y
 CONFIG_SPL=y
-CONFIG_SYS_EXTRA_OPTIONS="REV1"
+CONFIG_REV1=y
diff --git a/configs/pcm051_rev3_defconfig b/configs/pcm051_rev3_defconfig
index 2a907d7..4ad49df 100644
--- a/configs/pcm051_rev3_defconfig
+++ b/configs/pcm051_rev3_defconfig
@@ -1,4 +1,4 @@ 
 CONFIG_ARM=y
 CONFIG_TARGET_PCM051=y
 CONFIG_SPL=y
-CONFIG_SYS_EXTRA_OPTIONS="REV3"
+CONFIG_REV3=y