[alsa-devel] [PATCH 0/6] 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(-)
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)
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.
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
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.
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
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.
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
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.
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.
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
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.
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
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.
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
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); }
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
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
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...
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_BUSNo, 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_WM8994This 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
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.
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.
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
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...
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
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.
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
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.
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,
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).
participants (4)
-
Arnd Bergmann -
Mark Brown -
Russell King - ARM Linux -
Sangbeom Kim