ASoC fixes from ARM randconfig builds

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

This fixes a build error when CONFIG_HOTPLUG is disabled.

---
 sound/soc/codecs/wm8782.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index a2a09f8..f2ced71 100644
@@ -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

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

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

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

---
 sound/soc/codecs/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index 665d924..4d41447 100644
@@ -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

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

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.

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.

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.

Ok, makes sense. Thanks for the background information!

Arnd

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

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

@@ -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;
 
 /*

@@ -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

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

Add a missing dependency that is required for random configurations.

---
 sound/soc/imx/Kconfig |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

index bb699bb..dcd954b 100644
@@ -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

Applied, thanks.

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

---
 sound/soc/sh/fsi.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

index 8e112cc..916b9f9 100644
@@ -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

Applied, thanks.

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

Arnd

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.

---
 sound/soc/samsung/Kconfig |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

index 65f980e..cb63cd0 100644
@@ -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

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.

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.

index 4d41447..b7b9ddc 100644
@@ -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

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

Hi,

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

Thanks

Sangbeom.

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

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

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.

---
 sound/soc/samsung/Kconfig |    3 +++
 1 file changed, 3 insertions(+)

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

@@ -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

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.

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.

---

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.

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

@@ -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

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.

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.

---
 sound/soc/samsung/ac97.c |    2 +-
 sound/soc/samsung/i2s.c  |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

index f97110e..65ea538 100644
@@ -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,

index c086b78..0c9ac20 100644
@@ -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

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