Re: [alsa-devel] [PATCH 3/4] ASOC: WM8731 codec: add SPI support as well as I2C

Previous thread: Frustrated with capabilities.. by Markku Savela on Wednesday, August 27, 2008 - 2:31 am. (12 messages)

Next thread: [PATCH] Disable partition scan by Hannes Reinecke on Wednesday, August 27, 2008 - 3:31 am. (3 messages)
From: Bryan Wu
Date: Wednesday, August 27, 2008 - 2:39 am

Hi folks,

It's very happy to release this ASOC Blackfin ports. Please review our drivers
and feel free to ask us update it.

In this patchset, we post ASOC Blackfin supports, SSM2602 audio codec driver
and other 2 ALSA/ASOC patches.

Thanks a lot
-Bryan
--

From: Bryan Wu
Date: Wednesday, August 27, 2008 - 2:39 am

From: Cliff Cai <cliff.cai@analog.com>

Signed-off-by: Cliff Cai <cliff.cai@analog.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 sound/soc/codecs/Kconfig  |    5 +++
 sound/soc/codecs/wm8731.c |   71 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 73 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 67ed3f2..21bb277 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -21,6 +21,11 @@ config SND_SOC_SSM2602
 config SND_SOC_WM8731
 	tristate
 
+config SND_SOC_WM8731_SPI
+	bool "Control interface: Use SPI instead of I2C"
+	depends on SND_SOC_WM8731
+	default n
+
 config SND_SOC_WM8750
 	tristate
 
diff --git a/sound/soc/codecs/wm8731.c b/sound/soc/codecs/wm8731.c
index 9402fca..56a0ff1 100644
--- a/sound/soc/codecs/wm8731.c
+++ b/sound/soc/codecs/wm8731.c
@@ -19,6 +19,7 @@
 #include <linux/pm.h>
 #include <linux/i2c.h>
 #include <linux/platform_device.h>
+#include <linux/spi/spi.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
@@ -562,7 +563,7 @@ pcm_err:
 
 static struct snd_soc_device *wm8731_socdev;
 
-#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
+#if defined (CONFIG_I2C) || defined (CONFIG_I2C_MODULE) && !defined (CONFIG_SND_SOC_WM8731_SPI)
 
 /*
  * WM8731 2 wire address is determined by GPIO5
@@ -652,6 +653,62 @@ static struct i2c_client client_template = {
 };
 #endif
 
+#if defined(CONFIG_SPI_MASTER) && defined(CONFIG_SND_SOC_WM8731_SPI)
+static int __devinit wm8731_spi_probe(struct spi_device *spi)
+{
+	struct snd_soc_device *socdev = wm8731_socdev;
+	struct snd_soc_codec *codec = socdev->codec;
+	int ret;
+
+	codec->control_data = spi;
+
+	ret = wm8731_init(socdev);
+	if (ret < 0) {
+		err("failed to initialise WM8731\n");
+	}
+
+	return ret;
+}
+
+static int __devexit wm8731_spi_remove(struct spi_device *spi)
+{
+	return 0;
+}
+
+static struct spi_driver wm8731_spi_driver = {
+	.driver = ...
From: Mark Brown
Date: Wednesday, August 27, 2008 - 4:01 am

On Wed, Aug 27, 2008 at 05:39:27PM +0800, Bryan Wu wrote:


...as for the SSM2602 it'd be good if it were possible to build a kernel
which supports both I2C and SPI.  If you like I could do this for you -
there's likely to be merge issues due to the update to the new I2C API
anyway?
--

From: Bryan Wu
Date: Wednesday, August 27, 2008 - 8:46 pm

Oh, do you mean SSM2602 or WM8731? I also noticed the i2c API updates
patch from Jean and I'd like to use the new one

Thanks
-Bryan
--

From: Mark Brown
Date: Thursday, August 28, 2008 - 3:14 am

Sorry, that could've been clearer - I meant wm8731.  Obviously if you
want to update that too then that would be great but I figured it'd make
life easier for you if I were to ensure that one gets merged OK.  Which
way do you prefer?
--

From: Takashi Iwai
Date: Thursday, August 28, 2008 - 5:47 am

At Thu, 28 Aug 2008 11:14:35 +0100,

For me, it's easier when they come from you all together ;)

I still postponed to merge Jean's latest patches.  Care to check that
don't conflict your work and send them together?


thanks,

Takashi
--

From: Mark Brown
Date: Thursday, August 28, 2008 - 6:01 am

No problem from my point of view, though all I'll do is construct a
patch series with Jean's changes at the start so from that point of view
you may as well merge them directly - it doesn't change the work that
needs to be done.
--

From: Alan Horstmann
Date: Monday, September 1, 2008 - 6:52 am

We have just started trying this out as an alternative to i2c, and picked out 
the following:



in order to force use of SPI when (CONFIG_SND_SOC_WM8731_SPI) and (CONFIG_I2C) 
are both defined.  Or have we misunderstood the intention?  This is in 2 
other places also in the patch.

Alan

--

From: Liam Girdwood
Date: Monday, September 1, 2008 - 7:00 am

It may be simpler to only have CONFIG_SND_SOC_WM8731_SPI and
CONFIG_SND_SOC_WM8731_I2C definitions for all the codec drivers. These
would be set by machine Kconfig. 

Liam

--

From: Mark Brown
Date: Monday, September 1, 2008 - 7:02 am

I've reworked the patch to allow a single kernel image to support both
I2C and SPI which sidesteps this problem.  I'll post this later today.
--

From: Bryan Wu
Date: Wednesday, August 27, 2008 - 2:39 am

From: Cliff Cai <cliff.cai@analog.com>

Signed-off-by: Cliff Cai <cliff.cai@analog.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 include/linux/i2c-id.h     |    1 +
 sound/soc/codecs/Kconfig   |    3 +
 sound/soc/codecs/Makefile  |    2 +
 sound/soc/codecs/ssm2602.c |  837 ++++++++++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/ssm2602.h |  132 +++++++
 5 files changed, 975 insertions(+), 0 deletions(-)
 create mode 100644 sound/soc/codecs/ssm2602.c
 create mode 100644 sound/soc/codecs/ssm2602.h

diff --git a/include/linux/i2c-id.h b/include/linux/i2c-id.h
index e1b46bc..aeb221b 100644
--- a/include/linux/i2c-id.h
+++ b/include/linux/i2c-id.h
@@ -82,6 +82,7 @@
 #define I2C_DRIVERID_CS4270	94	/* Cirrus Logic 4270 audio codec */
 #define I2C_DRIVERID_M52790 	95      /* Mitsubishi M52790SP/FP AV switch */
 #define I2C_DRIVERID_CS5345	96	/* cs5345 audio processor	*/
+#define I2C_DRIVERID_SSM2602	97	/* BF52xC built in audio codec */
 
 #define I2C_DRIVERID_OV7670 1048	/* Omnivision 7670 camera */
 #define I2C_DRIVERID_AD5252 1049
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 221e1da..67ed3f2 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -15,6 +15,9 @@ config SND_SOC_AD1980
 	tristate
 	depends on SND_SOC
 
+config SND_SOC_SSM2602
+	tristate
+
 config SND_SOC_WM8731
 	tristate
 
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index 65e2a7d..29bcea6 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -1,6 +1,7 @@
 snd-soc-ac97-objs := ac97.o
 snd-soc-ad1980-objs := ad1980.o
 snd-soc-ak4535-objs := ak4535.o
+snd-soc-ssm2602-objs := ssm2602.o
 snd-soc-uda1380-objs := uda1380.o
 snd-soc-wm8510-objs := wm8510.o
 snd-soc-wm8731-objs := wm8731.o
@@ -15,6 +16,7 @@ snd-soc-tlv320aic3x-objs := tlv320aic3x.o
 obj-$(CONFIG_SND_SOC_AC97_CODEC)	+= snd-soc-ac97.o
 obj-$(CONFIG_SND_SOC_AD1980)	+= snd-soc-ad1980.o
 obj-$(CONFIG_SND_SOC_AK4535)	+= ...
From: Mark Brown
Date: Wednesday, August 27, 2008 - 3:54 am

On Wed, Aug 27, 2008 at 05:39:26PM +0800, Bryan Wu wrote:

This looks basically good, thanks - I've picked up a few things below
but they are mostly either minor or reflect the fact that the patch
looks like it's been developed against current release kernels but


Current kernels have a Kconfig option SND_SOC_ALL_CODECS which should
have your codec added - this allows codec drivers to be built without

Please convert these to use the standard pr_ macros (or ideally the dev_

Please keep the lines under 80 characters where reasonable.  A little
more while space (blank lines and at the start and end of comments)

This should be converted to an array of struct snd_soc_dapm_route -
there's a new bulk registration API been added which saves everything

There's now a snd_soc_dapm_new_controls() for registering DAPM controls



This won't compile with current kernels.  The dapm_event() interface has
been replaced by the very similar but hopefully clearer set_bias_level()

That *should* be redundant - ALSA should stop the playback streams

Hrm.  Why does the SPI support disable the I2C support?  I'd expect to
be able to build a kernel with the chip supported on both (for use on

Jean Delvare is currently in the process of convering all the ASoC codec
drivers to new style I2C drivers so we probably shouldn't add any more
old style I2C codec drivers.  The update is fairly straightforward - see

You could add a flag in the device data to identify if the device is on
SPI here (that's what the check for i2c_address is intended for).
--

From: Bryan Wu
Date: Wednesday, August 27, 2008 - 10:55 pm

Is this option in alsa git tree or in the mainline? I fail to find it
in upstream mainline.


Yes, I will fix this issue by running checkpatch.pl. And for other API
conflicts and I2C interface upgrading stuffs,
I will leave them to Cliff.

Thanks
-Bryan
--

From: Mark Brown
Date: Thursday, August 28, 2008 - 3:06 am

It's in ALSA git, queued for the 2.6.28 merge window.  See the
topic/asoc branch of:

        git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git

for the current ASoC merge queue.  The other API changes will all be
queued there too.

There's quite a bit of churn in ASoC at the minute but additional API
changes will be taken care of by whoever does them once your driver is
merged so it should save you some work if you're able to get this in

OK.  Jean might be willing to do the I2C API conversion for you (I've
CCed him in).
--

From: Jean Delvare
Date: Thursday, August 28, 2008 - 4:44 am

Hi Bryan, Mark,


Sure, I can help with that if needed. OTOH, I've posted patches
converting 2 drivers already [1] [2], and apparently all the codec
drivers follow the same model so it shouldn't be too difficult for
driver authors to do the same on their code. But anyway I'll convert
the remaining drivers as my time permits. The plan is to have
everything converted for kernel 2.6.28 (at which point the old I2C API
will be deprecated.)

[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2008-August/010198.html
[2] http://mailman.alsa-project.org/pipermail/alsa-devel/2008-August/010199.html

-- 
Jean Delvare
--

From: Bryan Wu
Date: Wednesday, August 27, 2008 - 2:39 am

From: Cliff Cai <cliff.cai@analog.com>

Signed-off-by: Cliff Cai <cliff.cai@analog.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 sound/core/pcm_native.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index c49b9d9..cb202a1 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -3391,6 +3391,12 @@ out:
 }
 #endif /* CONFIG_SND_SUPPORT_OLD_API */
 
+unsigned long dummy_get_unmapped_area(struct file *file, unsigned long addr,
+				      unsigned long len, unsigned long pgoff, unsigned long flags)
+{
+	return 0;
+}
+
 /*
  *  Register section
  */
@@ -3407,6 +3413,7 @@ const struct file_operations snd_pcm_f_ops[2] = {
 		.compat_ioctl = 	snd_pcm_ioctl_compat,
 		.mmap =			snd_pcm_mmap,
 		.fasync =		snd_pcm_fasync,
+		.get_unmapped_area =	dummy_get_unmapped_area,
 	},
 	{
 		.owner =		THIS_MODULE,
@@ -3419,5 +3426,6 @@ const struct file_operations snd_pcm_f_ops[2] = {
 		.compat_ioctl = 	snd_pcm_ioctl_compat,
 		.mmap =			snd_pcm_mmap,
 		.fasync =		snd_pcm_fasync,
+		.get_unmapped_area =	dummy_get_unmapped_area,
 	}
 };
-- 
1.5.6
--

From: Takashi Iwai
Date: Thursday, August 28, 2008 - 5:44 am

At Wed, 27 Aug 2008 17:39:28 +0800,


I don't think adding this dummy get_unmapped_area unconditionally for
every driver is good.  This overrides the default
mm->get_unmaped_area.


Takashi
--

From: Cai, Cliff
Date: Monday, September 1, 2008 - 8:16 pm

-----Original Message-----
From: Takashi Iwai [mailto:tiwai@suse.de] 
Sent: Thursday, August 28, 2008 8:45 PM
To: Bryan Wu
Cc: perex@perex.cz; liam.girdwood@wolfsonmicro.com;
broonie@opensource.wolfsonmicro.com; alsa-devel@alsa-project.org;
linux-kernel@vger.kernel.org; Cliff Cai
Subject: Re: [PATCH 4/4] ALSA: add dummy function to support shared mmap
in nommu Blackfin arch

At Wed, 27 Aug 2008 17:39:28 +0800,
But without this dummy function,shared mmap on nommu arch would
fail,refer to validate_mmap_request() in mm/nommu.c.
May be we can add a kernel config item to include or not include this
function depending on non-mmu or mmu arch.  

Cliff
--

From: Takashi Iwai
Date: Tuesday, September 2, 2008 - 2:41 am

At Tue, 2 Sep 2008 11:16:37 +0800,

Yes, I know it's needed for nommu.  That's why I wrote

Checking CONFIG_MMU may suffice?


Takashi
--

From: Bryan Wu
Date: Tuesday, September 2, 2008 - 2:59 am

Right, I post a v2 patch including this checking.

Thanks
-Bryan
--

From: Bryan Wu
Date: Wednesday, August 27, 2008 - 2:39 am

From: Cliff Cai <cliff.cai@analog.com>

Signed-off-by: Cliff Cai <cliff.cai@analog.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 sound/soc/Kconfig                   |    1 +
 sound/soc/Makefile                  |    2 +-
 sound/soc/blackfin/Kconfig          |   85 +++
 sound/soc/blackfin/Makefile         |   20 +
 sound/soc/blackfin/bf5xx-ac97-pcm.c |  424 +++++++++++++++
 sound/soc/blackfin/bf5xx-ac97-pcm.h |   29 +
 sound/soc/blackfin/bf5xx-ac97.c     |  417 ++++++++++++++
 sound/soc/blackfin/bf5xx-ac97.h     |   36 ++
 sound/soc/blackfin/bf5xx-ad1980.c   |  116 ++++
 sound/soc/blackfin/bf5xx-i2s-pcm.c  |  293 ++++++++++
 sound/soc/blackfin/bf5xx-i2s-pcm.h  |   29 +
 sound/soc/blackfin/bf5xx-i2s.c      |  183 +++++++
 sound/soc/blackfin/bf5xx-i2s.h      |   14 +
 sound/soc/blackfin/bf5xx-sport.c    | 1018 +++++++++++++++++++++++++++++++++++
 sound/soc/blackfin/bf5xx-sport.h    |  192 +++++++
 sound/soc/blackfin/bf5xx-ssm2602.c  |  195 +++++++
 16 files changed, 3053 insertions(+), 1 deletions(-)
 create mode 100644 sound/soc/blackfin/Kconfig
 create mode 100644 sound/soc/blackfin/Makefile
 create mode 100644 sound/soc/blackfin/bf5xx-ac97-pcm.c
 create mode 100644 sound/soc/blackfin/bf5xx-ac97-pcm.h
 create mode 100644 sound/soc/blackfin/bf5xx-ac97.c
 create mode 100644 sound/soc/blackfin/bf5xx-ac97.h
 create mode 100644 sound/soc/blackfin/bf5xx-ad1980.c
 create mode 100644 sound/soc/blackfin/bf5xx-i2s-pcm.c
 create mode 100644 sound/soc/blackfin/bf5xx-i2s-pcm.h
 create mode 100644 sound/soc/blackfin/bf5xx-i2s.c
 create mode 100644 sound/soc/blackfin/bf5xx-i2s.h
 create mode 100644 sound/soc/blackfin/bf5xx-sport.c
 create mode 100644 sound/soc/blackfin/bf5xx-sport.h
 create mode 100644 sound/soc/blackfin/bf5xx-ssm2602.c

diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig
index f743530..82e4d79 100644
--- a/sound/soc/Kconfig
+++ b/sound/soc/Kconfig
@@ -25,6 +25,7 @@ config SND_SOC_AC97_BUS
 source "sound/soc/at32/Kconfig"
 source "sound/soc/at91/Kconfig"
 ...
From: Mark Brown
Date: Wednesday, August 27, 2008 - 6:48 am

As with the other patches in the series this looks good but it has been
affected by changes in the ASoC APIs and needs to be updated to reflect
current ALSA.  The topic/asoc branch of Takashi's tree:

    git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git

is what's currently queued for 2.6.28.

Other than that, checkpatch-style things and a few more minor issues the
main things are the register write interface in /proc and the hardware
parameters for the I2S driver.  I've not flagged all the checkpatch
stuff here.

Can I also suggest splitting this up into a series of patches for easier
review - for example, adding the SPORT support first, followed by the
AC97 and I2S drivers and then the machine drivers?  I'd certainly have
appreciated having seen the SPORT support (which appears at the end of

Your spelling of AC97 isn't consistent here.  The kernel seems to prefer


Checkpatch picks up on this and quite a few other places being over 80

This looks wrong - there's rather more options for cmd, for example, and




Might be nice to say what to report and who the author is - by itself

struct snd_soc_cpu_dai and struct snd_soc_codec_dai have been merged

Shouldn't this undo the previous sport_confix_rx() on error?

Also, the use of blank lines is a bit funny here - I'd expect the blank

I think it's probably better to drop this from mainline - it doesn't
seem like something that we should be supporting long term and obviously
it could do damage if used.

If you do want to put it in mainline then it ought to go in debugfs

Again, odd blank lines and presumably there needs to be something to
o> + * Description:  Driver for SSM2602 sound chip built in ADSP-BF52xC




Same comment as for AC97.  Shouldn't more of this code be shared with



Are the RX and TX clocks independant?  If not you should probably tell
user space about the constraint in startup(), forcing the second stream
that's opened to use the same configuration as the first - take ...
From: Cai, Cliff
Date: Tuesday, September 2, 2008 - 9:09 pm

-----Original Message-----
From: Mark Brown [mailto:broonie@sirena.org.uk] 
Sent: Wednesday, August 27, 2008 9:49 PM
To: Bryan Wu
Cc: perex@perex.cz; lrg@kernel.org; alsa-devel@alsa-project.org;
linux-kernel@vger.kernel.org; Cliff Cai
Subject: Re: [PATCH 1/4] ASOC: Blackfin driver for ALSA SoC framework


As with the other patches in the series this looks good but it has been
affected by changes in the ASoC APIs and needs to be updated to reflect
current ALSA.  The topic/asoc branch of Takashi's tree:

    git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git

is what's currently queued for 2.6.28.

Other than that, checkpatch-style things and a few more minor issues the
main things are the register write interface in /proc and the hardware
parameters for the I2S driver.  I've not flagged all the checkpatch
stuff here.

Can I also suggest splitting this up into a series of patches for easier
review - for example, adding the SPORT support first, followed by the
AC97 and I2S drivers and then the machine drivers?  I'd certainly have
appreciated having seen the SPORT support (which appears at the end of

Your spelling of AC97 isn't consistent here.  The kernel seems to prefer


Checkpatch picks up on this and quite a few other places being over 80

This looks wrong - there's rather more options for cmd, for example, and

undo things on error?

t's not necessary to do anything else on error, these functions only set
user space about the constraint in startup(), forcing the second stream
the wm8903 driver for examples of how to do this.

TX and RX Clocks are also not independent,currently,in order to
implement full duplex,we have to enable both RX and TX side even if
there is only a steam is opened,
And make the other side running on a dummy buffer,so all registers
cann't be configured any more. when the second stream is opened we just
but then it looks like the driver only support slave mode ATM?  That's
what the machine driver is
 >using.  There ...
From: Mark Brown
Date: Wednesday, September 3, 2008 - 3:38 am

On Wed, Sep 03, 2008 at 12:09:49PM +0800, Cai, Cliff wrote:

Cliff, please fix your mail client configuration - the formatting of
your reply makes little (if any) distinction between the text of my

And none of these settings will leave things so that the port looks busy

Right, in that case your driver should use constraints to stop
applications trying to configure the capture and playback sides

It shouldn't just be a dummy - it should reject DAI formats other than

This is normal (and generally trying would result in audio artifacts if
you try), but normally there is at least some configuration that can be
done before the stream actually starts running.
--

From: Cliff Cai
Date: Wednesday, September 3, 2008 - 8:54 pm

_________________________________________________________________
一边聊天一边快速搜索,并把结果共享给好友,立刻试试!
http://im.live.cn/Share/18.htm
--

Previous thread: Frustrated with capabilities.. by Markku Savela on Wednesday, August 27, 2008 - 2:31 am. (12 messages)

Next thread: [PATCH] Disable partition scan by Hannes Reinecke on Wednesday, August 27, 2008 - 3:31 am. (3 messages)