Subject: [E3-hacking] [PATCH 2.6.37-rc1] ASoC: OMAP: fix OMAP1 compilation problem

From: Janusz Krzysztofik <[email-masked]>
Date: Tue, 2 Nov 2010 15:50:32 +0100

In the new code introduced with commit cf4c87abe238ec17cd0255b4e21abd949d7f811e,
"OMAP: McBSP: implement McBSP CLKR and FSR signal muxing via mach-omap2/mcbsp.c",
the way omap1 build is supposed to bypass omap2 specific functionality doesn't 
optimize out all omap2 specific stuff. This breaks linking phase for omap1 
machines, giving "undefined reference to `omap2_mcbsp1_mux_clkr_src'" 
and "undefined reference to `omap2_mcbsp1_mux_fsr_src'" errors. Fix it.

Created and tested against linux-2.6.37-rc1.

Signed-off-by: Janusz Krzysztofik <[email-masked]>
---

 sound/soc/omap/omap-mcbsp.c |    8 ++++++++
 1 file changed, 8 insertions(+)

--- linux-2.6.37-rc1/sound/soc/omap/omap-mcbsp.c.orig	2010-11-01 22:43:16.000000000 +0100
+++ linux-2.6.37-rc1/sound/soc/omap/omap-mcbsp.c	2010-11-02 15:41:42.000000000 +0100
@@ -644,15 +644,23 @@ static int omap_mcbsp_dai_set_dai_sysclk

 	case OMAP_MCBSP_CLKR_SRC_CLKR:
+		if (cpu_class_is_omap1())
+			break;
 		omap2_mcbsp1_mux_clkr_src(CLKR_SRC_CLKR);
 		break;
 	case OMAP_MCBSP_CLKR_SRC_CLKX:
+		if (cpu_class_is_omap1())
+			break;
 		omap2_mcbsp1_mux_clkr_src(CLKR_SRC_CLKX);
 		break;
 	case OMAP_MCBSP_FSR_SRC_FSR:
+		if (cpu_class_is_omap1())
+			break;
 		omap2_mcbsp1_mux_fsr_src(FSR_SRC_FSR);
 		break;
 	case OMAP_MCBSP_FSR_SRC_FSX:
+		if (cpu_class_is_omap1())
+			break;
 		omap2_mcbsp1_mux_fsr_src(FSR_SRC_FSX);
 		break;
 	default:

---

From: Mark Brown <[email-masked]>
Date: Tue, 2 Nov 2010 10:56:18 -0400

> and "undefined reference to `omap2_mcbsp1_mux_fsr_src'" errors. Fix it.

Acked-by: Mark Brown <[email-masked]>

>  		omap2_mcbsp1_mux_clkr_src(CLKR_SRC_CLKR);

I guess this will still fail with optimisation diabled, but I'm not sure
anyone ever builds the kernel like that.

---

From: Paul Walmsley <[email-masked]>
Date: Tue, 2 Nov 2010 10:58:59 -0600 (MDT)

> Signed-off-by: Janusz Krzysztofik <[email-masked]>

Acked-by: Paul Walmsley <[email-masked]>

Thanks Janusz.

- Paul

---

From: Jarkko Nikula <[email-masked]>
Date: Wed, 3 Nov 2010 10:20:28 +0200

On Tue, 2 Nov 2010 10:58:59 -0600 (MDT)

> Acked-by: Paul Walmsley <[email-masked]>
Yep, this must go to 2.6.37.

Acked-by: Jarkko Nikula <[email-masked]>

---

From: Liam Girdwood <[email-masked]>
Date: Wed, 03 Nov 2010 14:16:32 +0000

> Acked-by: Jarkko Nikula <[email-masked]>

Applied.

Thanks

Liam
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

---

From: Paul Walmsley <[email-masked]>
Date: Mon, 22 Nov 2010 17:48:24 -0700 (MST)

Hello Janusz, Mark,

> Signed-off-by: Janusz Krzysztofik <[email-masked]>

Thanks for fixing this.  What do you think about the following patch 
instead?  It should avoid any compiler issues.

- Paul

From: Paul Walmsley <[email-masked]>
Subject: [PATCH] ASoC: OMAP: McBSP: fix build breakage on OMAP1

After commits d13586574d373ef40acd4725c9a269daa355e412 ("OMAP: McBSP:
implement functional clock switching via clock framework") and
cf4c87abe238ec17cd0255b4e21abd949d7f811e ("OMAP: McBSP: implement
McBSP CLKR and FSR signal muxing via mach-omap2/mcbsp.c"), any OMAP1
board (such as the AMS Delta) that uses the ASoC McBSP driver will no
longer build:

sound/built-in.o: In function `omap_mcbsp_dai_set_dai_sysclk':
last.c:(.text+0x24ff8): undefined reference to `omap2_mcbsp1_mux_clkr_src'
last.c:(.text+0x2500c): undefined reference to `omap2_mcbsp1_mux_fsr_src'
make: *** [vmlinux] Error 1

Fix by defining three OMAP1-only dummy functions for
omap2_mcbsp1_mux_clkr_src(), omap2_mcbsp1_mux_fsr_src(), and
omap2_mcbsp_set_clks_src().

Normally, code that is OMAP SoC-revision-specific like this should go
under the arch/arm/*omap* directories, and get abstracted away from
drivers via struct platform_data function pointers.  This doesn't work
in this case since there doesn't appear to be any convenient way to access
struct platform_data (or something like it) in the current design of
the sound/soc/omap/omap-mcbsp.c driver.

Reported by Janusz Krzysztofik <[email-masked]> and Tony Lindgren
<[email-masked]>.  Janusz also posted a patch to fix this at:

   http://www.spinics.net/lists/linux-omap/msg39560.html

(among other places), but the following approach seems less dependent
on compiler behavior.

This patch passes build tests for ams_delta_defconfig and omap2plus_defconfig,
but since I don't have an AMS Delta here, I can't boot test it on that
platform.

Signed-off-by: Paul Walmsley <[email-masked]>
Cc: Janusz Krzysztofik <[email-masked]>
Cc: Tony Lindgren <[email-masked]>
Cc: Jarkko Nikula <[email-masked]>
Cc: Peter Ujfalusi <[email-masked]>
Cc: Mark Brown <[email-masked]>
Cc: Liam Girdwood <[email-masked]>
---
 arch/arm/plat-omap/mcbsp.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
index fdecd33..1fbfcf3 100644
--- a/arch/arm/plat-omap/mcbsp.c
+++ b/arch/arm/plat-omap/mcbsp.c
@@ -1466,6 +1466,33 @@ void omap_mcbsp_set_spi_mode(unsigned int id,
 }
 EXPORT_SYMBOL(omap_mcbsp_set_spi_mode);
 
+/*
+ * The following functions are only required on an OMAP1-only build.
+ * mach-omap2/mcbsp.c contains the real functions
+ */
+#ifndef CONFIG_ARCH_OMAP2PLUS
+int omap2_mcbsp_set_clks_src(u8 id, u8 fck_src_id)
+{
+	WARN(1, "%s: should never be called on an OMAP1-only kernel\n",
+	     __func__);
+	return -EINVAL;
+}
+
+void omap2_mcbsp1_mux_clkr_src(u8 mux)
+{
+	WARN(1, "%s: should never be called on an OMAP1-only kernel\n",
+	     __func__);
+	return;
+}
+
+void omap2_mcbsp1_mux_fsr_src(u8 mux)
+{
+	WARN(1, "%s: should never be called on an OMAP1-only kernel\n",
+	     __func__);
+	return;
+}
+#endif
+
 #ifdef CONFIG_ARCH_OMAP3
 #define max_thres(m)			(mcbsp->pdata->buffer_size)
 #define valid_threshold(m, val)		((val) <= max_thres(m))
-- 
1.7.2.3

---

From: Paul Walmsley <[email-masked]>
Date: Mon, 22 Nov 2010 17:50:54 -0700 (MST)

By the way, ASoC and OMAP audio-hackers, I deeply regret how much trouble 
that SCM cleanup series has caused.  It does not meet my personal 
standards for a patch-series...

- Paul

---

From: Jarkko Nikula <[email-masked]>
Date: Tue, 23 Nov 2010 09:26:30 +0200

On Mon, 22 Nov 2010 17:48:24 -0700 (MST)

> instead?  It should avoid any compiler issues.
Hmm.. looks like Janusz's patch is still in Liam's for-2.6.37
branch only.

> +int omap2_mcbsp_set_clks_src(u8 id, u8 fck_src_id)

Would that create a new problem if we are able to compile some day
omap1 and omap2 support into same kernel? I agree with you that passing
these via platform_data sounds the right solution.

-- 
Jarkko

---

From: Jarkko Nikula <[email-masked]>
Date: Tue, 23 Nov 2010 09:38:43 +0200

On Mon, 22 Nov 2010 17:50:54 -0700 (MST)

> standards for a patch-series...
Hey, small compile breaks happens to everyone and word trouble doesn't
come to my mind when speaking about your set.

Your set actually did one major step forward with ASoC OMAP drivers -
now it's possible to have modular build with them so your work has lot
of benefits to developers and distro builders.

-- 
Jarkko

---

From: Liam Girdwood <[email-masked]>
Date: Tue, 23 Nov 2010 10:32:39 +0000

> Hmm.. looks like Janusz's patch is still in Liam's for-2.6.37

Sorry, this pull request was waiting on Takashi's return from vacation.
I'll be doing a pull request shortly.

Thanks

Liam

---

From: Tony Lindgren <[email-masked]>
Date: Wed, 24 Nov 2010 15:35:11 -0800

> these via platform_data sounds the right solution.

This should not be a problem because there's no ifdef else and
in that case the functions would be there.

In general, the way to avoid problems like this is to make all
code under drivers arch independent and pass the platform data
from arch specific code.

In this case the ASoC platform data should be passed from
arch/arm/*omap*/ board-*.c files to make the drivers generic.

We are going to get rid of cpu_is_omapxxxx macros in the drivers,
so please consider this when patching the ASoC drivers.

Anyways, Paul's patch should be merged during the -rc cycle
because it is currently blocking Amstrad delta being built:

http://armlinux.simtec.co.uk/kautobuild/

Acked-by: Tony Lindgren <[email-masked]>

---

From: Tony Lindgren <[email-masked]>
Date: Wed, 24 Nov 2010 15:53:42 -0800

> http://armlinux.simtec.co.uk/kautobuild/

Turns out I'm several hours behind.. The build is
working with Janusz commit 233538501f707b0176f09af7039fec1e3fcac6e7
now in mainline.

I guess no need to merge Paul's fix then?

Tony

---

From: Jarkko Nikula <[email-masked]>
Date: Thu, 25 Nov 2010 09:02:01 +0200

On Wed, 24 Nov 2010 15:53:42 -0800

> I guess no need to merge Paul's fix then?
Not necessary I think if compilers are happy with Janusz's fix. If not,
then very strong reasong to start looking for platform data solution
as soon as possible :-)

-- 
Jarkko