ARM: EXYNOS4: Enable WM8994 MFD on I2S selection

Message ID 4DDD425A.1070202@samsung.com
State New
Headers show

Commit Message

Kukjin Kim May 25, 2011, 5:54 p.m.
On 05/24/11 21:39, Angus Ainslie wrote:
> From: Giridhar Maruthy<giridhar.maruthy@linaro.org>
>
> Whenever I2S is selected, select MFD for WM8994 and I2C
> to avoid manual enabling of the codec and I2C.
>
> Signed-off-by: Giridhar Maruthy<giridhar.maruthy@linaro.org>
> Signed-off-by: Angus Ainslie<angus.ainslie@linaro.org>
> ---
>   sound/soc/samsung/Kconfig |    3 +++
>   1 files changed, 3 insertions(+), 0 deletions(-)
>

Hi Angus :)

Hmm...first of all, should add proper maintainers in Cc. In this case, 
Jassi, Liam, and Mark should be added in Cc. (Cc'ed them)

> diff --git a/sound/soc/samsung/Kconfig b/sound/soc/samsung/Kconfig
> index a3fdfb6..a80eeb1 100644
> --- a/sound/soc/samsung/Kconfig
> +++ b/sound/soc/samsung/Kconfig
> @@ -65,6 +65,9 @@ config SND_SOC_SAMSUNG_SMDK_WM8994
>   	tristate "SoC I2S Audio support for WM8994 on SMDK"
>   	depends on SND_SOC_SAMSUNG&&  (MACH_SMDKV310 || MACH_SMDKC210)
>   	select SND_SOC_WM8994
> +	select MFD_SUPPORT
> +	select I2C_S3C2410
> +	select MFD_WM8994

I'm not sure about MFD configs, but if "select I2C_S3C2410" is required, 
it should be moved into regarding Kconfig of arch/arm, because it is 
related to dependency of ARCH. Maybe in this case, 
arch/arm/mach-exynos4/Kconfig?

>   	select SND_SAMSUNG_I2S
>   	help
>   		Say Y if you want to add support for SoC audio on the SMDKs.

Comments

Jassi Brar May 25, 2011, 6:15 p.m. | #1
>> diff --git a/sound/soc/samsung/Kconfig b/sound/soc/samsung/Kconfig
>> index a3fdfb6..a80eeb1 100644
>> --- a/sound/soc/samsung/Kconfig
>> +++ b/sound/soc/samsung/Kconfig
>> @@ -65,6 +65,9 @@ config SND_SOC_SAMSUNG_SMDK_WM8994
>>        tristate "SoC I2S Audio support for WM8994 on SMDK"
>>        depends on SND_SOC_SAMSUNG&&  (MACH_SMDKV310 || MACH_SMDKC210)
>>        select SND_SOC_WM8994
>> +       select MFD_SUPPORT
>> +       select I2C_S3C2410
>> +       select MFD_WM8994
>
> I'm not sure about MFD configs, but if "select I2C_S3C2410" is required, it
> should be moved into regarding Kconfig of arch/arm, because it is related to
> dependency of ARCH. Maybe in this case, arch/arm/mach-exynos4/Kconfig?

I2C here is a SMDK specific requirement and even if most devices need I2C,
selecting I2C for every EXYNOS4210 based device is not ideal. IMO it'd better
be tied to some SMDK specific config option.

Cheer!
Jassi
Kukjin Kim May 25, 2011, 6:48 p.m. | #2
On 05/25/11 11:15, Jassi Brar wrote:
>>> diff --git a/sound/soc/samsung/Kconfig b/sound/soc/samsung/Kconfig
>>> index a3fdfb6..a80eeb1 100644
>>> --- a/sound/soc/samsung/Kconfig
>>> +++ b/sound/soc/samsung/Kconfig
>>> @@ -65,6 +65,9 @@ config SND_SOC_SAMSUNG_SMDK_WM8994
>>>         tristate "SoC I2S Audio support for WM8994 on SMDK"
>>>         depends on SND_SOC_SAMSUNG&&    (MACH_SMDKV310 || MACH_SMDKC210)
>>>         select SND_SOC_WM8994
>>> +       select MFD_SUPPORT
>>> +       select I2C_S3C2410
>>> +       select MFD_WM8994
>>
>> I'm not sure about MFD configs, but if "select I2C_S3C2410" is required, it
>> should be moved into regarding Kconfig of arch/arm, because it is related to
>> dependency of ARCH. Maybe in this case, arch/arm/mach-exynos4/Kconfig?
>
> I2C here is a SMDK specific requirement and even if most devices need I2C,
> selecting I2C for every EXYNOS4210 based device is not ideal. IMO it'd better
> be tied to some SMDK specific config option.
>

Hi Jassi :)

How are you?

Hmm...need to think again because as you know, basically all of boards 
can be compiled with one exynos4_defconfig.

Ok, so you agreed it should be moved into some Kconfig of arch/arm/ :)
I will think again which one is more reasonable then let you know.
Mark Brown May 26, 2011, 2:24 p.m. | #3
On Wed, May 25, 2011 at 11:45:37PM +0530, Jassi Brar wrote:

> I2C here is a SMDK specific requirement and even if most devices need I2C,
> selecting I2C for every EXYNOS4210 based device is not ideal. IMO it'd better
> be tied to some SMDK specific config option.

Given that it's the audio machine driver the dependency on I2C doesn't
seem unreasonable - the CODEC will generally be I2C connected so I2C is
going to be required for the driver to do anything useful.  Might be
better to just add a dependency on the WM8994 MFD, though, which does
roughly the same thing.
Jassi Brar May 26, 2011, 3:02 p.m. | #4
On Thu, May 26, 2011 at 7:54 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, May 25, 2011 at 11:45:37PM +0530, Jassi Brar wrote:
>
>> I2C here is a SMDK specific requirement and even if most devices need I2C,
>> selecting I2C for every EXYNOS4210 based device is not ideal. IMO it'd better
>> be tied to some SMDK specific config option.
>
> Given that it's the audio machine driver the dependency on I2C doesn't
> seem unreasonable
Yup, actually I am not dead against the submitted patch... it's just
that historically
Samsung ASoC doesn't include I2C dependency, so why now.

> - the CODEC will generally be I2C connected so I2C is
> going to be required for the driver to do anything useful.  Might be
> better to just add a dependency on the WM8994 MFD, though, which does
> roughly the same thing.
Should do too.

Patch

diff --git a/arch/arm/mach-exynos4/Kconfig b/arch/arm/mach-exynos4/Kconfig
index e849f67..30b716e 100644
--- a/arch/arm/mach-exynos4/Kconfig
+++ b/arch/arm/mach-exynos4/Kconfig
@@ -12,6 +12,7 @@  if ARCH_EXYNOS4
  config CPU_EXYNOS4210
         bool
         select S3C_PL330_DMA
+       select I2C_S3C2410
         help
           Enable EXYNOS4210 CPU support