[PATCH] ASoC: soc-compress: Add support for not memory mapped DSPs

[PATCH] ASoC: soc-compress: Add support for not memory mapped DSPs

[Charles Keepax]

The ASoC compressed API did not implement the copy callback in its
compressed ops which is required for DSPs that are not memory mapped.
This patch adds a second set of compressed ops which does implement the
copy callback and uses that when copy is defined in the platform
compressed ops, ie. when the DSP is not memory mapped.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 sound/soc/soc-compress.c |   36 +++++++++++++++++++++++++++++++++++-
 1 files changed, 35 insertions(+), 1 deletions(-)

diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index 3ea7956..f9fb112 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -302,6 +302,22 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream,
 	return 0;
 }
 
+static int soc_compr_copy(struct snd_compr_stream *cstream,
+			  const char __user *buf, size_t count)
+{
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct snd_soc_platform *platform = rtd->platform;
+	int ret = 0;
+
+	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+
+	if (platform->driver->compr_ops && platform->driver->compr_ops->copy)
+		ret = platform->driver->compr_ops->copy(cstream, buf, count);
+
+	mutex_unlock(&rtd->pcm_mutex);
+	return ret;
+}
+
 /* ASoC Compress operations */
 static struct snd_compr_ops soc_compr_ops = {
 	.open		= soc_compr_open,
@@ -315,10 +331,25 @@ static struct snd_compr_ops soc_compr_ops = {
 	.get_codec_caps = soc_compr_get_codec_caps
 };
 
+/* ASoC Compress operations for non memory mapped DSPs */
+static struct snd_compr_ops soc_compr_nomap_ops = {
+	.open		= soc_compr_open,
+	.free		= soc_compr_free,
+	.set_params	= soc_compr_set_params,
+	.get_params	= soc_compr_get_params,
+	.trigger	= soc_compr_trigger,
+	.pointer	= soc_compr_pointer,
+	.copy		= soc_compr_copy,
+	.ack		= soc_compr_ack,
+	.get_caps	= soc_compr_get_caps,
+	.get_codec_caps = soc_compr_get_codec_caps
+};
+
 /* create a new compress */
 int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
 {
 	struct snd_soc_codec *codec = rtd->codec;
+	struct snd_soc_platform *platform = rtd->platform;
 	struct snd_soc_dai *codec_dai = rtd->codec_dai;
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
 	struct snd_compr *compr;
@@ -335,7 +366,10 @@ int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
 		return -ENOMEM;
 	}
 
-	compr->ops = &soc_compr_ops;
+	if (platform->driver->compr_ops && platform->driver->compr_ops->copy)
+		compr->ops = &soc_compr_nomap_ops;
+	else
+		compr->ops = &soc_compr_ops;
 	mutex_init(&compr->lock);
 	ret = snd_compress_new(rtd->card->snd_card, num, direction, compr);
 	if (ret < 0) {
-- 
1.7.2.5

Re: [PATCH] ASoC: soc-compress: Add support for not memory mapped DSPs

[Vinod Koul]

On Fri, Jan 25, 2013 at 09:00:00AM +0000, Charles Keepax wrote:
> The ASoC compressed API did not implement the copy callback in its
> compressed ops which is required for DSPs that are not memory mapped.
> This patch adds a second set of compressed ops which does implement the
> copy callback and uses that when copy is defined in the platform
> compressed ops, ie. when the DSP is not memory mapped.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> +static struct snd_compr_ops soc_compr_nomap_ops = {
> +	.open		= soc_compr_open,
> +	.free		= soc_compr_free,
> +	.set_params	= soc_compr_set_params,
> +	.get_params	= soc_compr_get_params,
> +	.trigger	= soc_compr_trigger,
> +	.pointer	= soc_compr_pointer,
> +	.copy		= soc_compr_copy,
> +	.ack		= soc_compr_ack,
> +	.get_caps	= soc_compr_get_caps,
> +	.get_codec_caps = soc_compr_get_codec_caps
> +};
Somehow I dont like the idea of doing one more ops for this.
> +
>  /* create a new compress */
>  int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
>  {
>  	struct snd_soc_codec *codec = rtd->codec;
> +	struct snd_soc_platform *platform = rtd->platform;
>  	struct snd_soc_dai *codec_dai = rtd->codec_dai;
>  	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>  	struct snd_compr *compr;
> @@ -335,7 +366,10 @@ int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
>  		return -ENOMEM;
>  	}
>  
> -	compr->ops = &soc_compr_ops;
> +	if (platform->driver->compr_ops && platform->driver->compr_ops->copy)
> +		compr->ops = &soc_compr_nomap_ops;
> +	else
> +		compr->ops = &soc_compr_ops;
How about:
	compr->ops = &soc_compr_ops;
	if (platform->driver->compr_ops && !platform->driver->compr_ops->copy)
		compr->ops->copy = NULL;

That way you set it to null for those who doent implement this and would called
for drivers who implement this.

--
~Vinod

Re: [PATCH] ASoC: soc-compress: Add support for not memory mapped DSPs

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 410 bytes --]

On Sun, Jan 27, 2013 at 08:04:16PM -0800, Vinod Koul wrote:

> How about:
> 	compr->ops = &soc_compr_ops;
> 	if (platform->driver->compr_ops && !platform->driver->compr_ops->copy)
> 		compr->ops->copy = NULL;

> That way you set it to null for those who doent implement this and would called
> for drivers who implement this.

We'd need to take a copy of it in case we have a system with a mix of
both styles.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: soc-compress: Add support for not memory mapped DSPs

[Vinod Koul]

On Mon, Jan 28, 2013 at 12:56:13PM +0800, Mark Brown wrote:
> On Sun, Jan 27, 2013 at 08:04:16PM -0800, Vinod Koul wrote:
> 
> > How about:
> > 	compr->ops = &soc_compr_ops;
> > 	if (platform->driver->compr_ops && !platform->driver->compr_ops->copy)
> > 		compr->ops->copy = NULL;
> 
> > That way you set it to null for those who doent implement this and would called
> > for drivers who implement this.
> 
> We'd need to take a copy of it in case we have a system with a mix of
> both styles.
For the same device?

Shouldnt you have two different device nodes and then use copy method on one
which supports and memory-mapped for the one which doesn't?

--
~Vinod

Re: [PATCH] ASoC: soc-compress: Add support for not memory mapped DSPs

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 728 bytes --]

On Sun, Jan 27, 2013 at 10:09:14PM -0800, Vinod Koul wrote:
> On Mon, Jan 28, 2013 at 12:56:13PM +0800, Mark Brown wrote:

> > > How about:
> > > 	compr->ops = &soc_compr_ops;
> > > 	if (platform->driver->compr_ops && !platform->driver->compr_ops->copy)
> > > 		compr->ops->copy = NULL;

> > > That way you set it to null for those who doent implement this and would called
> > > for drivers who implement this.

> > We'd need to take a copy of it in case we have a system with a mix of
> > both styles.

> For the same device?

No, not for the same device - just in the system.  compr->ops has been
set to point to soc_compr_ops which is a static shared by all drivers in
the system (unless I've misread what you wrote above).

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: soc-compress: Add support for not memory mapped DSPs

[Vinod Koul]

On Mon, Jan 28, 2013 at 02:44:42PM +0800, Mark Brown wrote:
> > > We'd need to take a copy of it in case we have a system with a mix of
> > > both styles.
Yes that would be the right thing to do..

> > For the same device?
> 
> No, not for the same device - just in the system.  compr->ops has been
> set to point to soc_compr_ops which is a static shared by all drivers in
> the system (unless I've misread what you wrote above).
No you read it right. We need to change a little and yes take a copy of it.
That way you can modify the copy locally for your device rather that updating
the system while avoiding two defines :)

--
~Vinod

Re: [PATCH] ASoC: soc-compress: Add support for not memory mapped DSPs

[Charles Keepax]

On Mon, Jan 28, 2013 at 01:34:49AM -0800, Vinod Koul wrote:
> On Mon, Jan 28, 2013 at 02:44:42PM +0800, Mark Brown wrote:
> No you read it right. We need to change a little and yes take a copy of it.
> That way you can modify the copy locally for your device rather that updating
> the system while avoiding two defines :)

I can update the patch to create a new copy for each device
however, I am not sure I prefer this approach. It seems highly
likely that presence or not of copy will be the only difference
been devices that is ever present. So making dynamic copies per
device for what will only ever be two distinct items feels
excessive compared to the cost of carrying one extra static
struct.

Charles

[PATCH v2] ASoC: soc-compress: Add support for not memory mapped DSPs

[Charles Keepax]

The ASoC compressed API did not implement the copy callback in its
compressed ops which is required for DSPs that are not memory mapped.
This patch adds a second set of compressed ops which does implement the
copy callback and uses that when copy is defined in the platform
compressed ops, ie. when the DSP is not memory mapped.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---


This version of the patch dynamically copies the compressed ops
as requested. Personally, I would select the first version but
both achieve the result I am after so I don't really mind which
version gets applied.

Thanks,
Charles


 sound/soc/soc-compress.c |   38 +++++++++++++++++++++++++++++++++++---
 1 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index 3ea7956..c0699fe 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -302,6 +302,22 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream,
 	return 0;
 }
 
+static int soc_compr_copy(struct snd_compr_stream *cstream,
+			  const char __user *buf, size_t count)
+{
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct snd_soc_platform *platform = rtd->platform;
+	int ret = 0;
+
+	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+
+	if (platform->driver->compr_ops && platform->driver->compr_ops->copy)
+		ret = platform->driver->compr_ops->copy(cstream, buf, count);
+
+	mutex_unlock(&rtd->pcm_mutex);
+	return ret;
+}
+
 /* ASoC Compress operations */
 static struct snd_compr_ops soc_compr_ops = {
 	.open		= soc_compr_open,
@@ -319,6 +335,7 @@ static struct snd_compr_ops soc_compr_ops = {
 int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
 {
 	struct snd_soc_codec *codec = rtd->codec;
+	struct snd_soc_platform *platform = rtd->platform;
 	struct snd_soc_dai *codec_dai = rtd->codec_dai;
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
 	struct snd_compr *compr;
@@ -335,14 +352,23 @@ int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
 		return -ENOMEM;
 	}
 
-	compr->ops = &soc_compr_ops;
+	compr->ops = kmemdup(&soc_compr_ops, sizeof(soc_compr_ops), GFP_KERNEL);
+	if (compr->ops == NULL) {
+		dev_err(rtd->card->dev, "Cannot allocate compressed ops\n");
+		ret = -ENOMEM;
+		goto compr_err;
+	}
+
+	/* Add copy callback for not memory mapped DSPs */
+	if (platform->driver->compr_ops && platform->driver->compr_ops->copy)
+		compr->ops->copy = soc_compr_copy;
+
 	mutex_init(&compr->lock);
 	ret = snd_compress_new(rtd->card->snd_card, num, direction, compr);
 	if (ret < 0) {
 		pr_err("compress asoc: can't create compress for codec %s\n",
 			codec->name);
-		kfree(compr);
-		return ret;
+		goto compr_ops_err;
 	}
 
 	/* DAPM dai link stream work */
@@ -354,4 +380,10 @@ int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
 	printk(KERN_INFO "compress asoc: %s <-> %s mapping ok\n", codec_dai->name,
 		cpu_dai->name);
 	return ret;
+
+compr_ops_err:
+	kfree(compr->ops);
+compr_err:
+	kfree(compr);
+	return ret;
 }
-- 
1.7.2.5

Re: [PATCH v2] ASoC: soc-compress: Add support for not memory mapped DSPs

[Vinod Koul]

On Tue, Jan 29, 2013 at 06:51:16PM +0000, Charles Keepax wrote:
> The ASoC compressed API did not implement the copy callback in its
> compressed ops which is required for DSPs that are not memory mapped.
> This patch adds a second set of compressed ops which does implement the
> copy callback and uses that when copy is defined in the platform
> compressed ops, ie. when the DSP is not memory mapped.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
> @@ -335,14 +352,23 @@ int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
>  		return -ENOMEM;
>  	}
>  
> -	compr->ops = &soc_compr_ops;
> +	compr->ops = kmemdup(&soc_compr_ops, sizeof(soc_compr_ops), GFP_KERNEL);
You forgot to free this memory when device is destoryed

--
~Vinod

[PATCH v3] ASoC: soc-compress: Add support for not memory mapped DSPs

[Charles Keepax]

The ASoC compressed API did not implement the copy callback in its
compressed ops which is required for DSPs that are not memory mapped.

This patch creates a local copy of the compress ops for each runtime and
modifies them with a copy callback as appropriate.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 sound/soc/soc-compress.c |   38 +++++++++++++++++++++++++++++++++++---
 1 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index 3ea7956..c81aeec 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -302,6 +302,22 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream,
 	return 0;
 }
 
+static int soc_compr_copy(struct snd_compr_stream *cstream,
+			  const char __user *buf, size_t count)
+{
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct snd_soc_platform *platform = rtd->platform;
+	int ret = 0;
+
+	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+
+	if (platform->driver->compr_ops && platform->driver->compr_ops->copy)
+		ret = platform->driver->compr_ops->copy(cstream, buf, count);
+
+	mutex_unlock(&rtd->pcm_mutex);
+	return ret;
+}
+
 /* ASoC Compress operations */
 static struct snd_compr_ops soc_compr_ops = {
 	.open		= soc_compr_open,
@@ -319,6 +335,7 @@ static struct snd_compr_ops soc_compr_ops = {
 int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
 {
 	struct snd_soc_codec *codec = rtd->codec;
+	struct snd_soc_platform *platform = rtd->platform;
 	struct snd_soc_dai *codec_dai = rtd->codec_dai;
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
 	struct snd_compr *compr;
@@ -335,14 +352,25 @@ int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
 		return -ENOMEM;
 	}
 
-	compr->ops = &soc_compr_ops;
+	compr->ops = devm_kzalloc(rtd->card->dev, sizeof(soc_compr_ops),
+				  GFP_KERNEL);
+	if (compr->ops == NULL) {
+		dev_err(rtd->card->dev, "Cannot allocate compressed ops\n");
+		ret = -ENOMEM;
+		goto compr_err;
+	}
+	memcpy(compr->ops, &soc_compr_ops, sizeof(soc_compr_ops));
+
+	/* Add copy callback for not memory mapped DSPs */
+	if (platform->driver->compr_ops && platform->driver->compr_ops->copy)
+		compr->ops->copy = soc_compr_copy;
+
 	mutex_init(&compr->lock);
 	ret = snd_compress_new(rtd->card->snd_card, num, direction, compr);
 	if (ret < 0) {
 		pr_err("compress asoc: can't create compress for codec %s\n",
 			codec->name);
-		kfree(compr);
-		return ret;
+		goto compr_err;
 	}
 
 	/* DAPM dai link stream work */
@@ -354,4 +382,8 @@ int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
 	printk(KERN_INFO "compress asoc: %s <-> %s mapping ok\n", codec_dai->name,
 		cpu_dai->name);
 	return ret;
+
+compr_err:
+	kfree(compr);
+	return ret;
 }
-- 
1.7.2.5

Re: [PATCH v3] ASoC: soc-compress: Add support for not memory mapped DSPs

[Vinod Koul]

On Tue, Feb 05, 2013 at 10:41:47AM +0000, Charles Keepax wrote:
> The ASoC compressed API did not implement the copy callback in its
> compressed ops which is required for DSPs that are not memory mapped.
> 
> This patch creates a local copy of the compress ops for each runtime and
> modifies them with a copy callback as appropriate.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Acked-by: Vinod Koul <vinod.koul@intel.com>

--
~Vinod

Re: [PATCH v3] ASoC: soc-compress: Add support for not memory mapped DSPs

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 223 bytes --]

On Tue, Feb 05, 2013 at 10:41:47AM +0000, Charles Keepax wrote:
> The ASoC compressed API did not implement the copy callback in its
> compressed ops which is required for DSPs that are not memory mapped.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]