[PATCH 0/6] ASoC fixes from ARM randconfig builds

[PATCH 0/6] ASoC fixes from ARM randconfig builds

[Arnd Bergmann]

Hi Mark,

As promised, here are the remaining ASoC patches from my
randconfig series. I hope I managed to get the submission
right this time. I've left out the ones that you already
took or NAKed, so this is shorter than I expected.

	Arnd

Arnd Bergmann (6):
  ASoC: codecs/wm8682: use __devexit_p
  ASoC: codecs: AK4641 depends on GPIOLIB
  ASoC: imx: eukrea_tlv320 needs i2c
  ASoC: sh: use correct __iomem annotations
  ASoC: samsung: fix Kconfig dependencies
  ASoC: samsung: add missing __devexit_p() annotations

 sound/soc/codecs/Kconfig  |    2 +-
 sound/soc/codecs/wm8782.c |    2 +-
 sound/soc/imx/Kconfig     |    1 +
 sound/soc/samsung/Kconfig |    4 ++++
 sound/soc/samsung/ac97.c  |    2 +-
 sound/soc/samsung/i2s.c   |    2 +-
 sound/soc/sh/fsi.c        |   10 +++++-----
 7 files changed, 14 insertions(+), 9 deletions(-)

-- 
1.7.5.4

[PATCH 1/6] ASoC: codecs/wm8682: use __devexit_p

[Arnd Bergmann]

This fixes a build error when CONFIG_HOTPLUG is disabled.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Ian Lartey <ian@opensource.wolfsonmicro.com>
Cc: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
---
 sound/soc/codecs/wm8782.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sound/soc/codecs/wm8782.c b/sound/soc/codecs/wm8782.c
index a2a09f8..f2ced71 100644
--- a/sound/soc/codecs/wm8782.c
+++ b/sound/soc/codecs/wm8782.c
@@ -60,7 +60,7 @@ static struct platform_driver wm8782_codec_driver = {
 		.owner = THIS_MODULE,
 	},
 	.probe = wm8782_probe,
-	.remove = wm8782_remove,
+	.remove = __devexit_p(wm8782_remove),
 };
 
 static int __init wm8782_init(void)
-- 
1.7.5.4

[PATCH 1/6] ASoC: codecs/wm8682: use __devexit_p

[Mark Brown]

On Sun, Oct 02, 2011 at 10:27:59PM +0200, Arnd Bergmann wrote:
> This fixes a build error when CONFIG_HOTPLUG is disabled.

Axel Lin sent a patch for this recently, it's already been merged.

>  sound/soc/codecs/wm8782.c |    2 +-

Your subject line doesn't match the file you're changing.

[PATCH 2/6] ASoC: codecs: AK4641 depends on GPIOLIB

[Arnd Bergmann]

This driver only builds correctly on platforms that use
GPIOLIB. Disable it otherwise.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Dmitry Artamonow <mad_soft@inbox.ru>
---
 sound/soc/codecs/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 665d924..4d41447 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -21,7 +21,7 @@ config SND_SOC_ALL_CODECS
 	select SND_SOC_ADS117X
 	select SND_SOC_AK4104 if SPI_MASTER
 	select SND_SOC_AK4535 if I2C
-	select SND_SOC_AK4641 if I2C
+	select SND_SOC_AK4641 if I2C && GPIOLIB
 	select SND_SOC_AK4642 if I2C
 	select SND_SOC_AK4671 if I2C
 	select SND_SOC_ALC5623 if I2C
-- 
1.7.5.4

[PATCH 2/6] ASoC: codecs: AK4641 depends on GPIOLIB

[Mark Brown]

On Sun, Oct 02, 2011 at 10:28:00PM +0200, Arnd Bergmann wrote:
> This driver only builds correctly on platforms that use
> GPIOLIB. Disable it otherwise.

No, gpiolib is one implementation of the GPIO API but if platforms want
to go and define their own that's currently OK (personally I think at
this point we should just be converting all the stragglers over to
gpiolib).  As things stand we shouldn't have dependencies on a
particular implementation of the API.

[PATCH 2/6] ASoC: codecs: AK4641 depends on GPIOLIB

[Arnd Bergmann]

On Sunday 02 October 2011 21:41:07 Mark Brown wrote:
> On Sun, Oct 02, 2011 at 10:28:00PM +0200, Arnd Bergmann wrote:
> > This driver only builds correctly on platforms that use
> > GPIOLIB. Disable it otherwise.
> 
> No, gpiolib is one implementation of the GPIO API but if platforms want
> to go and define their own that's currently OK (personally I think at
> this point we should just be converting all the stragglers over to
> gpiolib).  As things stand we shouldn't have dependencies on a
> particular implementation of the API.

Thanks for the explanation!

Is there any other symbol that I can test then?

I noticed that a lot of places use 'depends on GPIOLIB' or
'#ifdef CONFIG_GPIOLIB', are those usually wrong, too?

	Arnd

[PATCH 2/6] ASoC: codecs: AK4641 depends on GPIOLIB

[Mark Brown]

On Sun, Oct 02, 2011 at 10:53:24PM +0200, Arnd Bergmann wrote:
> On Sunday 02 October 2011 21:41:07 Mark Brown wrote:

> > No, gpiolib is one implementation of the GPIO API but if platforms want
> > to go and define their own that's currently OK (personally I think at

> Thanks for the explanation!

> Is there any other symbol that I can test then?

You shouldn't be testing anything - the client side GPIO API (which is
what this driver is using) is supposed to stub itself out when not in
use so drivers should just be able to use it without worrying about
dependencies.  You didn't report the problem but I'd expect that
whatever you saw will be an issue in whatever platform you were trying
to build for (I'm guessing it hasn't provided gpio_request_one()),
though it could be a problem in the gpiolib stubs if that's being used.

> I noticed that a lot of places use 'depends on GPIOLIB' or
> '#ifdef CONFIG_GPIOLIB', are those usually wrong, too?

Checks for gpiolib in drivers providing GPIOs are sensible, if a
platform hasn't used gpiolib then it's generally not even got an
interface for drivers to provide GPIOs.

On the user side these are usually due to people making the sort of
changes you're making here due to a random build coverage issue - it
seems unfortunately common for people to just shove a dependency in
Kconfig when they run into a build coverage issue without looking at
what's going on.  For a lot of the stuff you see on PCs it's going to
make sense but for some of the "service" APIs like GPIOs that are more
commonly used only in embedded contexts the use of the API is usually
completely optional (eg, in this case the driver is controlling power
and reset lines which could easily just be strapped in the hardware with
no soft control and are supplied as optional platform data) so for many
systems the driver is going to work completely happily without doing
anything with GPIOs.

Adding dependencies to all the users needlessly restricts which systems
can use the drivers.  Adding ifdefs to the drivers is repetitive and
isn't great for legiblity, having the header stub itself out is simpler
and easier to use on the driver side.

[PATCH 2/6] ASoC: codecs: AK4641 depends on GPIOLIB

[Arnd Bergmann]

On Sunday 02 October 2011 22:27:11 Mark Brown wrote:
> > Is there any other symbol that I can test then?
> 
> You shouldn't be testing anything - the client side GPIO API (which is
> what this driver is using) is supposed to stub itself out when not in
> use so drivers should just be able to use it without worrying about
> dependencies.  You didn't report the problem but I'd expect that
> whatever you saw will be an issue in whatever platform you were trying
> to build for (I'm guessing it hasn't provided gpio_request_one()),
> though it could be a problem in the gpiolib stubs if that's being used.

I don't remember where I first saw it. If the problem comes back,
I'll do a full bug report. I've verified now that it works on
various platforms with and without GPIOLIB.

I didn't know how the GPIO bits fit together, so I ended up doing
something that made the problem go away, whatever it was. This
is of course a problem with the randconfig fixing: One really needs
to understand every possible corner of the kernel to get it right,
and if you don't you end up with a patch that avoids the symptom
without fixing the underlying bug and then you make it harder to
find.

I really appreciate you doing the thorough review of the patches
to make sure we find the actual bugs, which is one of the main
things I want to achieve here anyway.

> > I noticed that a lot of places use 'depends on GPIOLIB' or
> > '#ifdef CONFIG_GPIOLIB', are those usually wrong, too?
> 
> Checks for gpiolib in drivers providing GPIOs are sensible, if a
> platform hasn't used gpiolib then it's generally not even got an
> interface for drivers to provide GPIOs.
> 
> On the user side these are usually due to people making the sort of
> changes you're making here due to a random build coverage issue - it
> seems unfortunately common for people to just shove a dependency in
> Kconfig when they run into a build coverage issue without looking at
> what's going on.  For a lot of the stuff you see on PCs it's going to
> make sense but for some of the "service" APIs like GPIOs that are more
> commonly used only in embedded contexts the use of the API is usually
> completely optional (eg, in this case the driver is controlling power
> and reset lines which could easily just be strapped in the hardware with
> no soft control and are supplied as optional platform data) so for many
> systems the driver is going to work completely happily without doing
> anything with GPIOs.
> 
> Adding dependencies to all the users needlessly restricts which systems
> can use the drivers.  Adding ifdefs to the drivers is repetitive and
> isn't great for legiblity, having the header stub itself out is simpler
> and easier to use on the driver side.

Ok, makes sense. Thanks for the background information!

	Arnd

[PATCH 2/6] ASoC: codecs: AK4641 depends on GPIOLIB

[Russell King - ARM Linux]

On Sun, Oct 02, 2011 at 09:41:07PM +0100, Mark Brown wrote:
> On Sun, Oct 02, 2011 at 10:28:00PM +0200, Arnd Bergmann wrote:
> > This driver only builds correctly on platforms that use
> > GPIOLIB. Disable it otherwise.
> 
> No, gpiolib is one implementation of the GPIO API but if platforms want
> to go and define their own that's currently OK (personally I think at
> this point we should just be converting all the stragglers over to
> gpiolib).  As things stand we shouldn't have dependencies on a
> particular implementation of the API.

Then it should depend on GENERIC_GPIO (not to be confused with GPIO_GENERIC,
the generic gpiolib driver), which is the symbol meaning that the GPIO API
is provided by something.

[alsa-devel] [PATCH 2/6] ASoC: codecs: AK4641 depends on GPIOLIB

[Mark Brown]

On Mon, Oct 03, 2011 at 02:45:27PM +0100, Russell King - ARM Linux wrote:
> On Sun, Oct 02, 2011 at 09:41:07PM +0100, Mark Brown wrote:

> > No, gpiolib is one implementation of the GPIO API but if platforms want
> > to go and define their own that's currently OK (personally I think at
> > this point we should just be converting all the stragglers over to
> > gpiolib).  As things stand we shouldn't have dependencies on a
> > particular implementation of the API.

> Then it should depend on GENERIC_GPIO (not to be confused with GPIO_GENERIC,
> the generic gpiolib driver), which is the symbol meaning that the GPIO API
> is provided by something.

Not for devices like this where the GPIOs are an optional thing the
driver can use, a dependency is far too strong.  Devices like that
should be able to rely on the stubs.

[alsa-devel] [PATCH 2/6] ASoC: codecs: AK4641 depends on GPIOLIB

[Arnd Bergmann]

On Monday 03 October 2011 15:35:53 Mark Brown wrote:
> On Mon, Oct 03, 2011 at 02:45:27PM +0100, Russell King - ARM Linux wrote:
> > On Sun, Oct 02, 2011 at 09:41:07PM +0100, Mark Brown wrote:
> 
> > > No, gpiolib is one implementation of the GPIO API but if platforms want
> > > to go and define their own that's currently OK (personally I think at
> > > this point we should just be converting all the stragglers over to
> > > gpiolib).  As things stand we shouldn't have dependencies on a
> > > particular implementation of the API.
> 
> > Then it should depend on GENERIC_GPIO (not to be confused with GPIO_GENERIC,
> > the generic gpiolib driver), which is the symbol meaning that the GPIO API
> > is provided by something.
> 
> Not for devices like this where the GPIOs are an optional thing the
> driver can use, a dependency is far too strong.  Devices like that
> should be able to rely on the stubs.

FWIW, while trying to reproduce this (I still could not), I stumbled over a
different build error with

CONFIG_ARCH_PRIMA2=y
CONFIG_SND_SOC_ALL_CODECS=m
CONFIG_SND_SOC_WM1250_EV1=m

sound/soc/codecs/wm1250-ev1.c:32:14: error: array type has incomplete element type
sound/soc/codecs/wm1250-ev1.c: In function 'wm1250_ev1_pdata':
sound/soc/codecs/wm1250-ev1.c:126:87: error: negative width in bit-field '<anonymous>'
sound/soc/codecs/wm1250-ev1.c:134:111: error: negative width in bit-field '<anonymous>'
sound/soc/codecs/wm1250-ev1.c: In function 'wm1250_ev1_free':
sound/soc/codecs/wm1250-ev1.c:155:103: error: negative width in bit-field '<anonymous>'

	Arnd

[alsa-devel] [PATCH 2/6] ASoC: codecs: AK4641 depends on GPIOLIB

[Mark Brown]

On Mon, Oct 03, 2011 at 04:47:07PM +0200, Arnd Bergmann wrote:

> sound/soc/codecs/wm1250-ev1.c:32:14: error: array type has incomplete element type

OK, that's the gpio_request_ stuff not being implemented thing that I
have seen some reports of.  Looks like that's not been stubbed out, but
for Prima2 the best fix is just to turn on gpiolib since there's no
excuse for a new platform to not use it.

[alsa-devel] [PATCH 2/6] ASoC: codecs: AK4641 depends on GPIOLIB

[Arnd Bergmann]

On Monday 03 October 2011 16:20:02 Mark Brown wrote:
> On Mon, Oct 03, 2011 at 04:47:07PM +0200, Arnd Bergmann wrote:
> 
> > sound/soc/codecs/wm1250-ev1.c:32:14: error: array type has incomplete element type
> 
> OK, that's the gpio_request_ stuff not being implemented thing that I
> have seen some reports of.  Looks like that's not been stubbed out, but
> for Prima2 the best fix is just to turn on gpiolib since there's no
> excuse for a new platform to not use it.

With the latest changes that Russell did in this direction, we can probably
set ARCH_WANT_OPTIONAL_GPIOLIB on ARM for all platforms that don't
provide their own gpio implementation or already require gpiolib.

However, I see no reason to force-enable gpiolib on platforms that
don't actually have any GPIO. On those, you would still get the same
problem with this code in the wm1250-ev1 driver:

        for (i = 0; i < ARRAY_SIZE(wm1250->gpios); i++) {
                wm1250->gpios[i].gpio = pdata->gpios[i];
                wm1250->gpios[i].label = wm1250_gpio_names[i];
                wm1250->gpios[i].flags = GPIOF_OUT_INIT_LOW;
        }

When the GPIO API is stubbed out, the definition of struct gpio
is empty, so you cannot access the members, which seems to be
intentional behavior.

In order to make that work, I think we need one of the two patches
below.

	Arnd

--- a/include/linux/gpio.h
+++ b/include/linux/gpio.h
@@ -24,7 +24,11 @@
 #include <linux/errno.h>
 
 struct device;
-struct gpio;
+struct gpio {
+       unsigned        gpio;
+       unsigned long   flags;
+       const char      *label;
+};
 struct gpio_chip;
 
 /*


--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -58,7 +58,7 @@ config SND_SOC_ALL_CODECS
 	select SND_SOC_UDA134X
 	select SND_SOC_UDA1380 if I2C
 	select SND_SOC_WL1273 if MFD_WL1273_CORE
-	select SND_SOC_WM1250_EV1 if I2C
+	select SND_SOC_WM1250_EV1 if I2C && GENERIC_GPIO
 	select SND_SOC_WM2000 if I2C
 	select SND_SOC_WM5100 if I2C
 	select SND_SOC_WM8350 if MFD_WM8350

[alsa-devel] [PATCH 2/6] ASoC: codecs: AK4641 depends on GPIOLIB

[Mark Brown]

On Mon, Oct 03, 2011 at 06:19:48PM +0200, Arnd Bergmann wrote:

> When the GPIO API is stubbed out, the definition of struct gpio
> is empty, so you cannot access the members, which seems to be
> intentional behavior.

That doesn't seem terribly helpful, it means drivers either need to
ifdef or not use the bulk functions.

>  struct device;
> -struct gpio;
> +struct gpio {
> +       unsigned        gpio;
> +       unsigned long   flags;
> +       const char      *label;
> +};

This looks much more sensible to me.

>  	select SND_SOC_WL1273 if MFD_WL1273_CORE
> -	select SND_SOC_WM1250_EV1 if I2C
> +	select SND_SOC_WM1250_EV1 if I2C && GENERIC_GPIO

Or ifdefs in the driver.

[PATCH 3/6] ASoC: imx: eukrea_tlv320 needs i2c

[Arnd Bergmann]

Add a missing dependency that is required for random configurations.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: "Eric B?nard" <eric@eukrea.com>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
---
 sound/soc/imx/Kconfig |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/sound/soc/imx/Kconfig b/sound/soc/imx/Kconfig
index bb699bb..dcd954b 100644
--- a/sound/soc/imx/Kconfig
+++ b/sound/soc/imx/Kconfig
@@ -50,6 +50,7 @@ config SND_SOC_EUKREA_TLV320
 		|| MACH_EUKREA_MBIMXSD25_BASEBOARD \
 		|| MACH_EUKREA_MBIMXSD35_BASEBOARD \
 		|| MACH_EUKREA_MBIMXSD51_BASEBOARD
+	depends on I2C
 	select SND_SOC_TLV320AIC23
 	select SND_MXC_SOC_FIQ
 	help
-- 
1.7.5.4

[PATCH 3/6] ASoC: imx: eukrea_tlv320 needs i2c

[Mark Brown]

On Sun, Oct 02, 2011 at 10:28:01PM +0200, Arnd Bergmann wrote:
> Add a missing dependency that is required for random configurations.

Applied, thanks.

[PATCH 4/6] ASoC: sh: use correct __iomem annotations

[Arnd Bergmann]

This removes a few unnecessary type casts and avoids
sparse warnings.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Cc: Paul Mundt <lethal@linux-sh.org>
---
 sound/soc/sh/fsi.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c
index 8e112cc..916b9f9 100644
--- a/sound/soc/sh/fsi.c
+++ b/sound/soc/sh/fsi.c
@@ -210,7 +210,7 @@ struct fsi_master {
  *		basic read write function
  */
 
-static void __fsi_reg_write(u32 reg, u32 data)
+static void __fsi_reg_write(u32 __iomem *reg, u32 data)
 {
 	/* valid data area is 24bit */
 	data &= 0x00ffffff;
@@ -218,12 +218,12 @@ static void __fsi_reg_write(u32 reg, u32 data)
 	__raw_writel(data, reg);
 }
 
-static u32 __fsi_reg_read(u32 reg)
+static u32 __fsi_reg_read(u32 __iomem *reg)
 {
 	return __raw_readl(reg);
 }
 
-static void __fsi_reg_mask_set(u32 reg, u32 mask, u32 data)
+static void __fsi_reg_mask_set(u32 __iomem *reg, u32 mask, u32 data)
 {
 	u32 val = __fsi_reg_read(reg);
 
@@ -250,7 +250,7 @@ static u32 _fsi_master_read(struct fsi_master *master, u32 reg)
 	unsigned long flags;
 
 	spin_lock_irqsave(&master->lock, flags);
-	ret = __fsi_reg_read((u32)(master->base + reg));
+	ret = __fsi_reg_read(master->base + reg);
 	spin_unlock_irqrestore(&master->lock, flags);
 
 	return ret;
@@ -264,7 +264,7 @@ static void _fsi_master_mask_set(struct fsi_master *master,
 	unsigned long flags;
 
 	spin_lock_irqsave(&master->lock, flags);
-	__fsi_reg_mask_set((u32)(master->base + reg), mask, data);
+	__fsi_reg_mask_set(master->base + reg, mask, data);
 	spin_unlock_irqrestore(&master->lock, flags);
 }
 
-- 
1.7.5.4

[PATCH 4/6] ASoC: sh: use correct __iomem annotations

[Mark Brown]

On Sun, Oct 02, 2011 at 10:28:02PM +0200, Arnd Bergmann wrote:
> This removes a few unnecessary type casts and avoids
> sparse warnings.

Applied, thanks.

[PATCH 4/6] ASoC: sh: use correct __iomem annotations

[Arnd Bergmann]

On Sunday 02 October 2011 21:50:09 Mark Brown wrote:
> On Sun, Oct 02, 2011 at 10:28:02PM +0200, Arnd Bergmann wrote:
> > This removes a few unnecessary type casts and avoids
> > sparse warnings.
> 
> Applied, thanks.

Thank you very much for the realtime feedback and patch applying.

	Arnd

[PATCH 5/6] ASoC: samsung: fix Kconfig dependencies

[Arnd Bergmann]

SND_SOC_WM8994 can only be selected if the respective MFD driver is
present.

SND_SAMSUNG_AC97 must not be builtin if SND_SOC_ALL_CODECS is
set to building all drivers as modules.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Jassi Brar <jassisinghbrar@gmail.com>
Cc: Sangbeom Kim <sbkim73@samsung.com>
---
 sound/soc/samsung/Kconfig |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/sound/soc/samsung/Kconfig b/sound/soc/samsung/Kconfig
index 65f980e..cb63cd0 100644
--- a/sound/soc/samsung/Kconfig
+++ b/sound/soc/samsung/Kconfig
@@ -25,6 +25,7 @@ config SND_SAMSUNG_PCM
 
 config SND_SAMSUNG_AC97
 	tristate
+	depends on SND_SOC_ALL_CODECS=n || SND_SOC_ALL_CODECS
 	select SND_SOC_AC97_BUS
 
 config SND_SAMSUNG_SPDIF
@@ -64,6 +65,7 @@ config SND_SOC_SAMSUNG_SMDK_WM8580
 config SND_SOC_SAMSUNG_SMDK_WM8994
 	tristate "SoC I2S Audio support for WM8994 on SMDK"
 	depends on SND_SOC_SAMSUNG && (MACH_SMDKV310 || MACH_SMDKC210)
+	depends on MFD_WM8994
 	select SND_SOC_WM8994
 	select SND_SAMSUNG_I2S
 	help
@@ -151,6 +153,7 @@ config SND_SOC_GONI_AQUILA_WM8994
 	tristate "SoC I2S Audio support for AQUILA/GONI - WM8994"
 	depends on SND_SOC_SAMSUNG && (MACH_GONI || MACH_AQUILA)
 	select SND_SAMSUNG_I2S
+	depends on MFD_WM8994
 	select SND_SOC_WM8994
 	help
 	  Say Y if you want to add support for SoC audio on goni or aquila
@@ -174,6 +177,7 @@ config SND_SOC_SMDK_WM8580_PCM
 config SND_SOC_SMDK_WM8994_PCM
 	tristate "SoC PCM Audio support for WM8994 on SMDK"
 	depends on SND_SOC_SAMSUNG && (MACH_SMDKC210 || MACH_SMDKV310)
+	depends on MFD_WM8994
 	select SND_SOC_WM8994
 	select SND_SAMSUNG_PCM
 	help
-- 
1.7.5.4

[PATCH 5/6] ASoC: samsung: fix Kconfig dependencies

[Mark Brown]

On Sun, Oct 02, 2011 at 10:28:03PM +0200, Arnd Bergmann wrote:

>  config SND_SAMSUNG_AC97
>  	tristate
> +	depends on SND_SOC_ALL_CODECS=n || SND_SOC_ALL_CODECS
>  	select SND_SOC_AC97_BUS

No, I'm not sure what the problem you're trying to fix here but this
looks pretty terrible.  SND_SOC_ALL_CODECS is a debugging tool for build
coverage, we shouldn't be restricting actual useful drivers based on it.
What is the intention of this change and why does it only apply to the
Samsung platform?  It all looks very magic.

>  config SND_SOC_SAMSUNG_SMDK_WM8994
>  	tristate "SoC I2S Audio support for WM8994 on SMDK"
>  	depends on SND_SOC_SAMSUNG && (MACH_SMDKV310 || MACH_SMDKC210)
> +	depends on MFD_WM8994
>  	select SND_SOC_WM8994

This is non-idiomatic - we always select the CODEC drivers rather than
depending on them for usability.

Please also split the two changes you're doing here into separate
patches.  I might've applied the wm8994 bit but it's squashed in with
the AC'97 change...

[PATCH 5/6] ASoC: samsung: fix Kconfig dependencies

[Arnd Bergmann]

On Sunday 02 October 2011 21:47:30 Mark Brown wrote:
> On Sun, Oct 02, 2011 at 10:28:03PM +0200, Arnd Bergmann wrote:
> 
> >  config SND_SAMSUNG_AC97
> >       tristate
> > +     depends on SND_SOC_ALL_CODECS=n || SND_SOC_ALL_CODECS
> >       select SND_SOC_AC97_BUS
> 
> No, I'm not sure what the problem you're trying to fix here but this
> looks pretty terrible.  SND_SOC_ALL_CODECS is a debugging tool for build
> coverage, we shouldn't be restricting actual useful drivers based on it.
> What is the intention of this change and why does it only apply to the
> Samsung platform?  It all looks very magic.

It's a bug that I only observed on exynos4, and it could be that
this patch didn't actually solve it in the end. I'll drop it for
now and do a better report when the problem comes back.

I remember that I never fully understood what was going on either,
and I suspected a problem with Kconfig resulting in some builtin
ac97 code referencing symbols that are enabled in a module when
SND_SOC_ALL_CODECS=m.

> >  config SND_SOC_SAMSUNG_SMDK_WM8994
> >       tristate "SoC I2S Audio support for WM8994 on SMDK"
> >       depends on SND_SOC_SAMSUNG && (MACH_SMDKV310 || MACH_SMDKC210)
> > +     depends on MFD_WM8994
> >       select SND_SOC_WM8994
> 
> This is non-idiomatic - we always select the CODEC drivers rather than
> depending on them for usability.

Ok. I did this patch before the MFD_SUPPORT option was removed, so I did
not want to add both 'select MFD_WM8994' and 'select MFD_SUPPORT' here.

So should SND_SOC_WM8994 instead select MFD_WM8994? That would mean adding
the select only in one place.

	Arnd

8<---
ASoC: codecs: SND_SOC_WM8994 requires MFD_WM8994

The samsung SMDK platform can select SND_SOC_WM8994 while the
necessary MFD driver is not present. Always select MFD_WM8994
now in order to satisfy the build dependencies.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 4d41447..b7b9ddc 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -92,7 +92,7 @@ config SND_SOC_ALL_CODECS
 	select SND_SOC_WM8990 if I2C
 	select SND_SOC_WM8991 if I2C
 	select SND_SOC_WM8993 if I2C
-	select SND_SOC_WM8994 if MFD_WM8994
+	select SND_SOC_WM8994
 	select SND_SOC_WM8995 if SND_SOC_I2C_AND_SPI
 	select SND_SOC_WM8996 if I2C
 	select SND_SOC_WM9081 if I2C
@@ -373,6 +373,7 @@ config SND_SOC_WM8993
 
 config SND_SOC_WM8994
 	tristate
+	select MFD_WM8994
 
 config SND_SOC_WM8995
 	tristate

[PATCH 5/6] ASoC: samsung: fix Kconfig dependencies

[Mark Brown]

On Sun, Oct 02, 2011 at 11:14:55PM +0200, Arnd Bergmann wrote:

> So should SND_SOC_WM8994 instead select MFD_WM8994? That would mean adding
> the select only in one place.

select ignores the dependencies of the things it selects which isn't
enormously helpful for this sort of stuff.

[alsa-devel] [PATCH 5/6] ASoC: samsung: fix Kconfig dependencies

[Sangbeom Kim]

Hi,
On Mon, Oct 03, 2011 at 6:15:03AM , Arnd Bergmann wrote:
> It's a bug that I only observed on exynos4, and it could be that
> this patch didn't actually solve it in the end. I'll drop it for
> now and do a better report when the problem comes back.

Could you please explain problem which observed on exynos4?
If possible, I want to check it too.

Thanks

Sangbeom.

[alsa-devel] [PATCH 5/6] ASoC: samsung: fix Kconfig dependencies

[Arnd Bergmann]

On Monday 03 October 2011 15:48:54 Sangbeom Kim wrote:
> On Mon, Oct 03, 2011 at 6:15:03AM , Arnd Bergmann wrote:
> > It's a bug that I only observed on exynos4, and it could be that
> > this patch didn't actually solve it in the end. I'll drop it for
> > now and do a better report when the problem comes back.
> 
> Could you please explain problem which observed on exynos4?
> If possible, I want to check it too.

Sorry, I can't reproduce it any more. It could have been something
unrelated like a Kconfig or toolchain problem that got fixed in
the meantime. I really should have dropped that one hunk from
the patch. The other problem in this patch is still there, with
CONFIG_SND_SOC_SMDK_WM8994_PCM=y and CONFIG_MFD_WM8994=n:

sound/built-in.o: In function `wm8994_resume':
last.c:(.text+0x1f600): undefined reference to `wm8994_reg_read'
sound/built-in.o: In function `wm8994_read':
last.c:(.text+0x20160): undefined reference to `wm8994_reg_read'
sound/built-in.o: In function `wm8994_codec_probe':
last.c:(.text+0x2029c): undefined reference to `wm8994_reg_read'
last.c:(.text+0x20480): undefined reference to `wm8994_reg_read'
last.c:(.text+0x204b4): undefined reference to `wm8994_reg_read'
sound/built-in.o: In function `wm8994_write':
last.c:(.text+0x20e68): undefined reference to `wm8994_reg_write'
last.c:(.text+0x20ea4): undefined reference to `wm8994_reg_write'
sound/built-in.o: In function `wm8958_dsp2_fw':
last.c:(.text+0x2159c): undefined reference to `wm8994_bulk_write'

	Arnd

[alsa-devel] [PATCH 5/6] ASoC: samsung: fix Kconfig dependencies

[Mark Brown]

On Mon, Oct 03, 2011 at 01:50:08PM +0200, Arnd Bergmann wrote:

> the patch. The other problem in this patch is still there, with
> CONFIG_SND_SOC_SMDK_WM8994_PCM=y and CONFIG_MFD_WM8994=n:

You've not sent a seprate patch for that one yet...

[PATCH 5/6] ASoC: samsung: wm8994 depends on mfd_wm8994

[Arnd Bergmann]

Any driver that selects SND_SOC_WM8994 should also make sure that
MFD_WM8994 is set, since the codec relies on the mfd code:

  sound/built-in.o: In function `wm8994_read':
  last.c:(.text+0x20160): undefined reference to `wm8994_reg_read'
  sound/built-in.o: In function `wm8994_write':
  last.c:(.text+0x20e68): undefined reference to `wm8994_reg_write'

This adds the missing 'depends' statements to Kconfig, as found
during ARM randconfig tests.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 sound/soc/samsung/Kconfig |    3 +++
 1 file changed, 3 insertions(+)

On Monday 03 October 2011 13:07:41 Mark Brown wrote:
> On Mon, Oct 03, 2011 at 01:50:08PM +0200, Arnd Bergmann wrote:
> 
> > the patch. The other problem in this patch is still there, with
> > CONFIG_SND_SOC_SMDK_WM8994_PCM=y and CONFIG_MFD_WM8994=n:
> 
> You've not sent a seprate patch for that one yet...

I wasn't sure any more which version you wanted. Is this this right one?

--- a/sound/soc/samsung/Kconfig
+++ b/sound/soc/samsung/Kconfig
@@ -65,6 +65,7 @@ config SND_SOC_SAMSUNG_SMDK_WM8580
 config SND_SOC_SAMSUNG_SMDK_WM8994
 	tristate "SoC I2S Audio support for WM8994 on SMDK"
 	depends on SND_SOC_SAMSUNG && (MACH_SMDKV310 || MACH_SMDKC210)
+	depends on MFD_WM8994
 	select SND_SOC_WM8994
 	select SND_SAMSUNG_I2S
 	help
@@ -152,6 +153,7 @@ config SND_SOC_GONI_AQUILA_WM8994
 	tristate "SoC I2S Audio support for AQUILA/GONI - WM8994"
 	depends on SND_SOC_SAMSUNG && (MACH_GONI || MACH_AQUILA)
 	select SND_SAMSUNG_I2S
+	depends on MFD_WM8994
 	select SND_SOC_WM8994
 	help
 	  Say Y if you want to add support for SoC audio on goni or aquila
@@ -175,6 +177,7 @@ config SND_SOC_SMDK_WM8580_PCM
 config SND_SOC_SMDK_WM8994_PCM
 	tristate "SoC PCM Audio support for WM8994 on SMDK"
 	depends on SND_SOC_SAMSUNG && (MACH_SMDKC210 || MACH_SMDKV310)
+	depends on MFD_WM8994
 	select SND_SOC_WM8994
 	select SND_SAMSUNG_PCM
 	help

[PATCH 5/6] ASoC: samsung: wm8994 depends on mfd_wm8994

[Mark Brown]

On Mon, Oct 03, 2011 at 03:43:10PM +0200, Arnd Bergmann wrote:

> > You've not sent a seprate patch for that one yet...

> I wasn't sure any more which version you wanted. Is this this right one?

I was expecting something that selects the core.  I was also expecting
something that can be applied against -next which this patch can't be,
this doesn't feel like something we should be rushing into 3.1.

[PATCH v3] ASoC: samsung: wm8994 depends on mfd_wm8994

[Arnd Bergmann]

Any driver that selects SND_SOC_WM8994 should also make sure that
MFD_WM8994 is set, since the codec relies on the mfd code:

  sound/built-in.o: In function `wm8994_read':
  last.c:(.text+0x20160): undefined reference to `wm8994_reg_read'
  sound/built-in.o: In function `wm8994_write':
  last.c:(.text+0x20e68): undefined reference to `wm8994_reg_write'

This solves the problem by selecting the MFD driver directly
and adding extra 'depends on' statements to make sure that we
respect the dependencies of that driver.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
On Monday 03 October 2011 15:08:24 Mark Brown wrote:
> On Mon, Oct 03, 2011 at 03:43:10PM +0200, Arnd Bergmann wrote:
> 
> > > You've not sent a seprate patch for that one yet...
> 
> > I wasn't sure any more which version you wanted. Is this this right one?
> 
> I was expecting something that selects the core.  I was also expecting
> something that can be applied against -next which this patch can't be,

I'm still confused, I thought you had NAKed the patch that selects the
core because it ignores the dependencies. Here comes another variant,
this time for your branch, and selecting the MFD driver directly while
respecting its dependencies.

> this doesn't feel like something we should be rushing into 3.1.

No, certainly not. This patch by itself helps nobody, the only reason
for doing it is to make randconfig work and that requires lots of
other patches.

	Arnd

--- a/sound/soc/samsung/Kconfig
+++ b/sound/soc/samsung/Kconfig
@@ -64,6 +64,8 @@ config SND_SOC_SAMSUNG_SMDK_WM8580
 config SND_SOC_SAMSUNG_SMDK_WM8994
 	tristate "SoC I2S Audio support for WM8994 on SMDK"
 	depends on SND_SOC_SAMSUNG && (MACH_SMDKV310 || MACH_SMDKC210 || MACH_SMDK4212)
+	depends on I2C=y && GENERIC_HARDIRQS
+	select MFD_WM8994
 	select SND_SOC_WM8994
 	select SND_SAMSUNG_I2S
 	help
@@ -150,7 +152,9 @@ config SND_SOC_SMARTQ
 config SND_SOC_GONI_AQUILA_WM8994
 	tristate "SoC I2S Audio support for AQUILA/GONI - WM8994"
 	depends on SND_SOC_SAMSUNG && (MACH_GONI || MACH_AQUILA)
+	depends on I2C=y && GENERIC_HARDIRQS
 	select SND_SAMSUNG_I2S
+	select MFD_WM8994
 	select SND_SOC_WM8994
 	help
 	  Say Y if you want to add support for SoC audio on goni or aquila
@@ -174,6 +178,8 @@ config SND_SOC_SMDK_WM8580_PCM
 config SND_SOC_SMDK_WM8994_PCM
 	tristate "SoC PCM Audio support for WM8994 on SMDK"
 	depends on SND_SOC_SAMSUNG && (MACH_SMDKC210 || MACH_SMDKV310 || MACH_SMDK4212)
+	depends on I2C=y && GENERIC_HARDIRQS
+	select MFD_WM8994
 	select SND_SOC_WM8994
 	select SND_SAMSUNG_PCM
 	help

[PATCH v3] ASoC: samsung: wm8994 depends on mfd_wm8994

[Mark Brown]

On Mon, Oct 03, 2011 at 04:35:46PM +0200, Arnd Bergmann wrote:

> I'm still confused, I thought you had NAKed the patch that selects the
> core because it ignores the dependencies. Here comes another variant,
> this time for your branch, and selecting the MFD driver directly while
> respecting its dependencies.

The problem with that one was that you had the select on the Kconfig for
the CODEC driver rather than on the machine drivers so you ended up with
the select being done from a symbol that is itself selected.

Applied this version, thanks.

[PATCH 6/6] ASoC: samsung: add missing __devexit_p() annotations

[Arnd Bergmann]

Drivers that refer to a __devexit function in an operations
structure need to annotate that pointer with __devexit_p so
replace it with a NULL pointer when the section gets discarded.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Jassi Brar <jassisinghbrar@gmail.com>
Cc: Sangbeom Kim <sbkim73@samsung.com>
---
 sound/soc/samsung/ac97.c |    2 +-
 sound/soc/samsung/i2s.c  |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/samsung/ac97.c b/sound/soc/samsung/ac97.c
index f97110e..65ea538 100644
--- a/sound/soc/samsung/ac97.c
+++ b/sound/soc/samsung/ac97.c
@@ -495,7 +495,7 @@ static __devexit int s3c_ac97_remove(struct platform_device *pdev)
 
 static struct platform_driver s3c_ac97_driver = {
 	.probe  = s3c_ac97_probe,
-	.remove = s3c_ac97_remove,
+	.remove = __devexit_p(s3c_ac97_remove),
 	.driver = {
 		.name = "samsung-ac97",
 		.owner = THIS_MODULE,
diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c
index c086b78..0c9ac20 100644
--- a/sound/soc/samsung/i2s.c
+++ b/sound/soc/samsung/i2s.c
@@ -1136,7 +1136,7 @@ static __devexit int samsung_i2s_remove(struct platform_device *pdev)
 
 static struct platform_driver samsung_i2s_driver = {
 	.probe  = samsung_i2s_probe,
-	.remove = samsung_i2s_remove,
+	.remove = __devexit_p(samsung_i2s_remove),
 	.driver = {
 		.name = "samsung-i2s",
 		.owner = THIS_MODULE,
-- 
1.7.5.4

[PATCH 6/6] ASoC: samsung: add missing __devexit_p() annotations

[Mark Brown]

On Sun, Oct 02, 2011 at 10:28:04PM +0200, Arnd Bergmann wrote:
> Drivers that refer to a __devexit function in an operations
> structure need to annotate that pointer with __devexit_p so
> replace it with a NULL pointer when the section gets discarded.

Again Axel already fixed these (and the other Samsung CPU drivers).