[PATCH] ARM: OMAP: drop "select MACH_NOKIA_RM696"

[PATCH] ARM: OMAP: drop "select MACH_NOKIA_RM696"

[Paul Bolle]

When support was added for Nokia N9 (RM-696), with commit
63fc5f3bb3d0ca9ab4767a801b518aa6335f87ad ("ARM: OMAP: add minimal
support for Nokia RM-696"), a select statement for MACH_NOKIA_RM696 was
added to the tree. But there's no Kconfig symbol with that name. That
symbol would be superfluous, since support for that machine piggybacks
on MACH_NOKIA_RM680. So drop that select.

Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
---
0) Tested with "git grep".

1) Some searching on the web didn't return a "config MACH_NOKIA_RM696".
So apparently there's not even a development tree that uses this symbol.

 arch/arm/mach-omap2/Kconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
index 49ac3df..5cafa77 100644
--- a/arch/arm/mach-omap2/Kconfig
+++ b/arch/arm/mach-omap2/Kconfig
@@ -296,7 +296,6 @@ config MACH_NOKIA_RM680
 	bool "Nokia N950 (RM-680) / N9 (RM-696) phones"
 	depends on ARCH_OMAP3
 	default y
-	select MACH_NOKIA_RM696
 	select OMAP_PACKAGE_CBB
 
 config MACH_NOKIA_RX51
-- 
1.7.11.7

Re: [PATCH] ARM: OMAP: drop "select MACH_NOKIA_RM696"

[Aaro Koskinen]

On Fri, Mar 08, 2013 at 11:29:56AM +0100, Paul Bolle wrote:
> When support was added for Nokia N9 (RM-696), with commit
> 63fc5f3bb3d0ca9ab4767a801b518aa6335f87ad ("ARM: OMAP: add minimal
> support for Nokia RM-696"), a select statement for MACH_NOKIA_RM696 was
> added to the tree. But there's no Kconfig symbol with that name. That
> symbol would be superfluous, since support for that machine piggybacks
> on MACH_NOKIA_RM680. So drop that select.

This is needed because of arch/arm/tools/mach-types. See
include/generated/mach-types.h.

If you have just CONFIG_MACH_NOKIA_RM680 and run the kernel on RM-696,
then machine_is_nokia_rm696() will return false. If I rememeber correctly,
this broke at least early printk / uncompressor output at the time.

I guess people may still want to use machine_is_... macros e.g. for
debugging.

> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> ---
> 0) Tested with "git grep".

git grep won't search generated source files.

> 1) Some searching on the web didn't return a "config MACH_NOKIA_RM696".
> So apparently there's not even a development tree that uses this symbol.

You can see machine_is_nokia_rm696() used in the public kernel source
for Nokia N9 product. :-)

A.

Re: [PATCH] ARM: OMAP: drop "select MACH_NOKIA_RM696"

[Tony Lindgren]

* Aaro Koskinen <aaro.koskinen@iki.fi> [130308 08:16]:
> On Fri, Mar 08, 2013 at 11:29:56AM +0100, Paul Bolle wrote:
> > When support was added for Nokia N9 (RM-696), with commit
> > 63fc5f3bb3d0ca9ab4767a801b518aa6335f87ad ("ARM: OMAP: add minimal
> > support for Nokia RM-696"), a select statement for MACH_NOKIA_RM696 was
> > added to the tree. But there's no Kconfig symbol with that name. That
> > symbol would be superfluous, since support for that machine piggybacks
> > on MACH_NOKIA_RM680. So drop that select.
> 
> This is needed because of arch/arm/tools/mach-types. See
> include/generated/mach-types.h.
> 
> If you have just CONFIG_MACH_NOKIA_RM680 and run the kernel on RM-696,
> then machine_is_nokia_rm696() will return false. If I rememeber correctly,
> this broke at least early printk / uncompressor output at the time.
> 
> I guess people may still want to use machine_is_... macros e.g. for
> debugging.

I think the righ fix is to just add

config MACH_NOKIA_RM680
	bool

to the mach-omap2/Kconfig like we have for n8x0 also.

Regards,

Tony

Re: [PATCH] ARM: OMAP: drop "select MACH_NOKIA_RM696"

[Paul Bolle]

On Fri, 2013-03-08 at 08:35 -0800, Tony Lindgren wrote:
> I think the righ fix is to just add
> 
> config MACH_NOKIA_RM680
> 	bool

I guess you meant MACH_NOKIA_RM696.

> to the mach-omap2/Kconfig like we have for n8x0 also.

Should I draft a patch?


Paul Bolle

Re: [PATCH] ARM: OMAP: drop "select MACH_NOKIA_RM696"

[Tony Lindgren]

* Paul Bolle <pebolle@tiscali.nl> [130308 09:24]:
> On Fri, 2013-03-08 at 08:35 -0800, Tony Lindgren wrote:
> > I think the righ fix is to just add
> > 
> > config MACH_NOKIA_RM680
> > 	bool
> 
> I guess you meant MACH_NOKIA_RM696.

Ooops yes.
 
> > to the mach-omap2/Kconfig like we have for n8x0 also.
> 
> Should I draft a patch?

Sure that would be nice.

Tony

Re: [PATCH] ARM: OMAP: drop "select MACH_NOKIA_RM696"

[Paul Bolle]

On Fri, 2013-03-08 at 09:55 -0800, Tony Lindgren wrote:
> * Paul Bolle <pebolle@tiscali.nl> [130308 09:24]:
> > Should I draft a patch?
> 
> Sure that would be nice.

One thing I couldn't determine is how the generated mach-types.h header
handles multiple CONFIG_MACH_* macros.

If both CONFIG_MACH_FOO and CONFIG_MACH_BAR are defined, and these both
have a line in */mach-types, will the machine_is_foo() and
machine_is_bar() macros both behave as one would expect?


Paul Bolle

Re: [PATCH] ARM: OMAP: drop "select MACH_NOKIA_RM696"

[Tony Lindgren]

* Paul Bolle <pebolle@tiscali.nl> [130308 10:06]:
> On Fri, 2013-03-08 at 09:55 -0800, Tony Lindgren wrote:
> > * Paul Bolle <pebolle@tiscali.nl> [130308 09:24]:
> > > Should I draft a patch?
> > 
> > Sure that would be nice.
> 
> One thing I couldn't determine is how the generated mach-types.h header
> handles multiple CONFIG_MACH_* macros.
> 
> If both CONFIG_MACH_FOO and CONFIG_MACH_BAR are defined, and these both
> have a line in */mach-types, will the machine_is_foo() and
> machine_is_bar() macros both behave as one would expect?

Yes they do, for the selected ones the macro becomes
machine_arch_type == MACH_TYPE_XYZ instead of 0.

Regards,

Tony

Re: [PATCH] ARM: OMAP: drop "select MACH_NOKIA_RM696"

[Russell King - ARM Linux]

On Fri, Mar 08, 2013 at 07:02:44PM +0100, Paul Bolle wrote:
> On Fri, 2013-03-08 at 09:55 -0800, Tony Lindgren wrote:
> > * Paul Bolle <pebolle@tiscali.nl> [130308 09:24]:
> > > Should I draft a patch?
> > 
> > Sure that would be nice.
> 
> One thing I couldn't determine is how the generated mach-types.h header
> handles multiple CONFIG_MACH_* macros.
> 
> If both CONFIG_MACH_FOO and CONFIG_MACH_BAR are defined, and these both
> have a line in */mach-types, will the machine_is_foo() and
> machine_is_bar() macros both behave as one would expect?

It's actually quite clever.  There's two levels to it.

The first is that CONFIG_MACH_xxx result in their machine_is_xxx() macros
being defined to constant zero if the CONFIG option is not enabled.  That
allows the compiler to throw away code for disabled platforms because
the expression is always false.

Otherwise, they end up as (machine_arch_type == MACH_TYPE_xxx).

The second is the magic which happens when two CONFIG_MACH_xxx are
selected.  If only one is selected, then machine_arch_type is defined
to the appropriate MACH_TYPE_xxx.  This means that the above expression
becomes constant-true, and the conditional is eliminated.

If more than one is selected, then machine_arch_type is defined to a
variable which is appropriately set to one of the MACH_TYPE_xxx values.

So, the result is that:
- de-selected platforms have their if (machine_is_xxx()) { } optimised
  out of the kernel.
- for a kernel built targetting one platform, all the
  if (machine_is_xxx()) tests are optimised away, leaving only the
  relevant code behind.
- otherwise, we get the _appropriate_ conditional code for the
  configuration generated.

However, going back to that MACH_NOKIA_RM696.  If there exists only a
select of this symbol and no "config MACH_NOKIA_RM696" entry, then the
symbol will never be generated in the output .config file.

I too can find no trace of any use of machine_is_nokia_rm696 in the
mainline kernel.  So, if there's nothing using the machine_is_()
symbol, and nothing using the CONFIG_MACH_NOKIA_RM696 symbol, then
any select of that is entirely superfluous.

Well, I did this:

$ git grep -i nokia_rm696
arch/arm/mach-omap2/Kconfig:    select MACH_NOKIA_RM696
arch/arm/mach-omap2/board-rm680.c:MACHINE_START(NOKIA_RM696, "Nokia RM-696 board")
arch/arm/tools/mach-types:nokia_rm696           MACH_NOKIA_RM696        NOKIA_RM696             3522

So, there exists platform support for this device, provided by the RM680
support, but there's no use of the machine_is_xxx() symbol - and if there
was, it would always be false.

My conclusion is... it's a mess.

Re: [PATCH] ARM: OMAP: drop "select MACH_NOKIA_RM696"

[Paul Bolle]

On Sat, 2013-03-09 at 00:01 +0000, Russell King - ARM Linux wrote:
> It's actually quite clever.  There's two levels to it.
> 
> The first is that CONFIG_MACH_xxx result in their machine_is_xxx() macros
> being defined to constant zero if the CONFIG option is not enabled.  That
> allows the compiler to throw away code for disabled platforms because
> the expression is always false.
> 
> Otherwise, they end up as (machine_arch_type == MACH_TYPE_xxx).
> 
> The second is the magic which happens when two CONFIG_MACH_xxx are
> selected.  If only one is selected, then machine_arch_type is defined
> to the appropriate MACH_TYPE_xxx.  This means that the above expression
> becomes constant-true, and the conditional is eliminated.
> 
> If more than one is selected, then machine_arch_type is defined to a
> variable which is appropriately set to one of the MACH_TYPE_xxx values.

At boot?

> So, the result is that:
> - de-selected platforms have their if (machine_is_xxx()) { } optimised
>   out of the kernel.
> - for a kernel built targetting one platform, all the
>   if (machine_is_xxx()) tests are optimised away, leaving only the
>   relevant code behind.
> - otherwise, we get the _appropriate_ conditional code for the
>   configuration generated.

Thanks for clarifying this. Quite clever indeed. 

> However, going back to that MACH_NOKIA_RM696.  If there exists only a
> select of this symbol and no "config MACH_NOKIA_RM696" entry, then the
> symbol will never be generated in the output .config file.
> 
>[...]
> 
> My conclusion is... it's a mess.

That mess can only be fully cleaned up if the code for the RM-696 that
now is maintained in some unknown to me repository gets merged into
mainline, can't it?

In the meantime, how do you prefer I solve the (trivial) issue of an
useless select for MACH_NOKIA_RM696? Drop that select or add an (equally
useless) config entry for MACH_NOKIA_RM696? Or should I try to ignore it
for the time being?


Paul Bolle

Re: [PATCH] ARM: OMAP: drop "select MACH_NOKIA_RM696"

[Tony Lindgren]

* Paul Bolle <pebolle@tiscali.nl> [130309 11:52]:
> On Sat, 2013-03-09 at 00:01 +0000, Russell King - ARM Linux wrote:
> 
> > However, going back to that MACH_NOKIA_RM696.  If there exists only a
> > select of this symbol and no "config MACH_NOKIA_RM696" entry, then the
> > symbol will never be generated in the output .config file.
> > 
> >[...]
> > 
> > My conclusion is... it's a mess.
> 
> That mess can only be fully cleaned up if the code for the RM-696 that
> now is maintained in some unknown to me repository gets merged into
> mainline, can't it?
>
> In the meantime, how do you prefer I solve the (trivial) issue of an
> useless select for MACH_NOKIA_RM696? Drop that select or add an (equally
> useless) config entry for MACH_NOKIA_RM696? Or should I try to ignore it
> for the time being?

Just adding the config MACH_NOKIA_RM696 to Kconfig as bool should fix
this unless I'm missing something here.

Regards,

Tony

Re: [PATCH] ARM: OMAP: drop "select MACH_NOKIA_RM696"

[Paul Bolle]

On Mon, 2013-03-11 at 09:33 -0700, Tony Lindgren wrote:
> * Paul Bolle <pebolle@tiscali.nl> [130309 11:52]:
> > In the meantime, how do you prefer I solve the (trivial) issue of an
> > useless select for MACH_NOKIA_RM696? Drop that select or add an (equally
> > useless) config entry for MACH_NOKIA_RM696? Or should I try to ignore it
> > for the time being?
> 
> Just adding the config MACH_NOKIA_RM696 to Kconfig as bool should fix
> this unless I'm missing something here.

Yes, that should do. I already promised to draft that small patch last
week. I hope to actually do that shortly.


Paul Bolle

Re: [PATCH] ARM: OMAP: drop "select MACH_NOKIA_RM696"

[Paul Bolle]

On Fri, 2013-03-08 at 18:11 +0200, Aaro Koskinen wrote:
> On Fri, Mar 08, 2013 at 11:29:56AM +0100, Paul Bolle wrote:
> > When support was added for Nokia N9 (RM-696), with commit
> > 63fc5f3bb3d0ca9ab4767a801b518aa6335f87ad ("ARM: OMAP: add minimal
> > support for Nokia RM-696"), a select statement for MACH_NOKIA_RM696 was
> > added to the tree. But there's no Kconfig symbol with that name. That
> > symbol would be superfluous, since support for that machine piggybacks
> > on MACH_NOKIA_RM680. So drop that select.
> 
> This is needed because of arch/arm/tools/mach-types. See
> include/generated/mach-types.h.

How does that file actually need this select statement?

> If you have just CONFIG_MACH_NOKIA_RM680 and run the kernel on RM-696,
> then machine_is_nokia_rm696() will return false. If I rememeber correctly,
> this broke at least early printk / uncompressor output at the time.
> 
> I guess people may still want to use machine_is_... macros e.g. for
> debugging.

What in the current mainline kernel cares about a
machine_is_nokia_rm696() macro?
    $ git grep -n machine_is_nokia v3.9-rc1
    v3.9-rc1:arch/arm/mach-omap2/board-n8x0.c:349:  if (machine_is_nokia_n800() || slot == 0)
    v3.9-rc1:arch/arm/mach-omap2/board-n8x0.c:395:  if (machine_is_nokia_n800()) {
    v3.9-rc1:arch/arm/mach-omap2/board-n8x0.c:428:  if (machine_is_nokia_n800())
    v3.9-rc1:arch/arm/mach-omap2/board-n8x0.c:451:  if (machine_is_nokia_n800()) {
    v3.9-rc1:arch/arm/mach-omap2/board-n8x0.c:478:  if (machine_is_nokia_n800())
    v3.9-rc1:arch/arm/mach-omap2/board-n8x0.c:493:  if (machine_is_nokia_n810()) {
    v3.9-rc1:arch/arm/mach-omap2/board-n8x0.c:544:  if (machine_is_nokia_n810()) {
    v3.9-rc1:arch/arm/mach-omap2/board-n8x0.c:562:  if (machine_is_nokia_n810()) {
    v3.9-rc1:arch/arm/mach-omap2/board-n8x0.c:714:  if (machine_is_nokia_n810())
    v3.9-rc1:arch/arm/mach-omap2/board-rx51-video.c:71:     if (!machine_is_nokia_rx51())
    v3.9-rc1:drivers/usb/host/ohci-omap.c:267:      } else if (machine_is_nokia770()) {
    v3.9-rc1:sound/soc/omap/n810.c:309:     if (!(machine_is_nokia_n810() || machine_is_nokia_n810_wimax()))
    v3.9-rc1:sound/soc/omap/rx51.c:400:     if (!machine_is_nokia_rx51())

    $ git grep -n rm696 v3.9-rc1
    v3.9-rc1:arch/arm/tools/mach-types:589:nokia_rm696		MACH_NOKIA_RM696	NOKIA_RM696		3522

I don't find any actual usage of that macro. Are its uses also
generated? 

> > 0) Tested with "git grep".
> 
> git grep won't search generated source files.

Of course. That's one of the reasons why I explicitly state that I only
tested it that way.

> > 1) Some searching on the web didn't return a "config MACH_NOKIA_RM696".
> > So apparently there's not even a development tree that uses this symbol.
> 
> You can see machine_is_nokia_rm696() used in the public kernel source
> for Nokia N9 product. :-)

Where would that be? And does that tree actually have a Kconfig entry
MACH_NOKIA_RM696?


Paul Bolle