[PATCH 0/5] Blackfin ASoC fixing and updates

[PATCH 0/5] Blackfin ASoC fixing and updates

[Bryan Wu]

Hi Mark,

We have some patches for Blackfin ASoC updates.
Please take a look.

Thanks a lot
-Bryan

[PATCH 1/5] ASoC: Blackfin: fix bug - kernel will crash when record and play in bf527-ezkit

[Bryan Wu]

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

set constraint only if the value is not 0, change the configuring way for sport

Signed-off-by: Cliff Cai <cliff.cai@analog.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 sound/soc/blackfin/bf5xx-i2s.c |   25 ++++---------------------
 1 files changed, 4 insertions(+), 21 deletions(-)

diff --git a/sound/soc/blackfin/bf5xx-i2s.c b/sound/soc/blackfin/bf5xx-i2s.c
index d1d95d2..29cd5a4 100644
--- a/sound/soc/blackfin/bf5xx-i2s.c
+++ b/sound/soc/blackfin/bf5xx-i2s.c
@@ -49,7 +49,7 @@ struct bf5xx_i2s_port {
 	u16 rcr1;
 	u16 tcr2;
 	u16 rcr2;
-	int counter;
+	int configured;
 };
 
 static struct bf5xx_i2s_port bf5xx_i2s;
@@ -132,16 +132,6 @@ static int bf5xx_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai,
 	return ret;
 }
 
-static int bf5xx_i2s_startup(struct snd_pcm_substream *substream,
-			     struct snd_soc_dai *dai)
-{
-	pr_debug("%s enter\n", __func__);
-
-	/*this counter is used for counting how many pcm streams are opened*/
-	bf5xx_i2s.counter++;
-	return 0;
-}
-
 static int bf5xx_i2s_hw_params(struct snd_pcm_substream *substream,
 				struct snd_pcm_hw_params *params,
 				struct snd_soc_dai *dai)
@@ -168,7 +158,7 @@ static int bf5xx_i2s_hw_params(struct snd_pcm_substream *substream,
 		break;
 	}
 
-	if (bf5xx_i2s.counter == 1) {
+	if (!bf5xx_i2s.configured) {
 		/*
 		 * TX and RX are not independent,they are enabled at the
 		 * same time, even if only one side is running. So, we
@@ -195,13 +185,6 @@ static int bf5xx_i2s_hw_params(struct snd_pcm_substream *substream,
 	return 0;
 }
 
-static void bf5xx_i2s_shutdown(struct snd_pcm_substream *substream,
-			       struct snd_soc_dai *dai)
-{
-	pr_debug("%s enter\n", __func__);
-	bf5xx_i2s.counter--;
-}
-
 static int bf5xx_i2s_probe(struct platform_device *pdev,
 			   struct snd_soc_dai *dai)
 {
@@ -219,6 +202,8 @@ static int bf5xx_i2s_probe(struct platform_device *pdev,
 		return -ENODEV;
 	}
 
+	bf5xx_i2s.configured = 1;
+
 	return 0;
 }
 
@@ -305,8 +290,6 @@ struct snd_soc_dai bf5xx_i2s_dai = {
 		.rates = BF5XX_I2S_RATES,
 		.formats = BF5XX_I2S_FORMATS,},
 	.ops = {
-		.startup   = bf5xx_i2s_startup,
-		.shutdown  = bf5xx_i2s_shutdown,
 		.hw_params = bf5xx_i2s_hw_params,
 		.set_fmt = bf5xx_i2s_set_dai_fmt,
 	},
-- 
1.5.6.3

[PATCH 1/5] ASoC: Blackfin: fix bug - kernel will crash when record and play in bf527-ezkit

[Bryan Wu]

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

set constraint only if the value is not 0, change the configuring way for sport

Signed-off-by: Cliff Cai <cliff.cai@analog.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 sound/soc/blackfin/bf5xx-i2s.c |   25 ++++---------------------
 1 files changed, 4 insertions(+), 21 deletions(-)

diff --git a/sound/soc/blackfin/bf5xx-i2s.c b/sound/soc/blackfin/bf5xx-i2s.c
index d1d95d2..29cd5a4 100644
--- a/sound/soc/blackfin/bf5xx-i2s.c
+++ b/sound/soc/blackfin/bf5xx-i2s.c
@@ -49,7 +49,7 @@ struct bf5xx_i2s_port {
 	u16 rcr1;
 	u16 tcr2;
 	u16 rcr2;
-	int counter;
+	int configured;
 };
 
 static struct bf5xx_i2s_port bf5xx_i2s;
@@ -132,16 +132,6 @@ static int bf5xx_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai,
 	return ret;
 }
 
-static int bf5xx_i2s_startup(struct snd_pcm_substream *substream,
-			     struct snd_soc_dai *dai)
-{
-	pr_debug("%s enter\n", __func__);
-
-	/*this counter is used for counting how many pcm streams are opened*/
-	bf5xx_i2s.counter++;
-	return 0;
-}
-
 static int bf5xx_i2s_hw_params(struct snd_pcm_substream *substream,
 				struct snd_pcm_hw_params *params,
 				struct snd_soc_dai *dai)
@@ -168,7 +158,7 @@ static int bf5xx_i2s_hw_params(struct snd_pcm_substream *substream,
 		break;
 	}
 
-	if (bf5xx_i2s.counter == 1) {
+	if (!bf5xx_i2s.configured) {
 		/*
 		 * TX and RX are not independent,they are enabled at the
 		 * same time, even if only one side is running. So, we
@@ -195,13 +185,6 @@ static int bf5xx_i2s_hw_params(struct snd_pcm_substream *substream,
 	return 0;
 }
 
-static void bf5xx_i2s_shutdown(struct snd_pcm_substream *substream,
-			       struct snd_soc_dai *dai)
-{
-	pr_debug("%s enter\n", __func__);
-	bf5xx_i2s.counter--;
-}
-
 static int bf5xx_i2s_probe(struct platform_device *pdev,
 			   struct snd_soc_dai *dai)
 {
@@ -219,6 +202,8 @@ static int bf5xx_i2s_probe(struct platform_device *pdev,
 		return -ENODEV;
 	}
 
+	bf5xx_i2s.configured = 1;
+
 	return 0;
 }
 
@@ -305,8 +290,6 @@ struct snd_soc_dai bf5xx_i2s_dai = {
 		.rates = BF5XX_I2S_RATES,
 		.formats = BF5XX_I2S_FORMATS,},
 	.ops = {
-		.startup   = bf5xx_i2s_startup,
-		.shutdown  = bf5xx_i2s_shutdown,
 		.hw_params = bf5xx_i2s_hw_params,
 		.set_fmt = bf5xx_i2s_set_dai_fmt,
 	},
-- 
1.5.6.3

Re: [alsa-devel] [PATCH 1/5] ASoC: Blackfin: fix bug - kernel will crash when record and play in bf527-ezkit

[Mark Brown]

On Fri, Mar 06, 2009 at 03:53:26PM +0800, Bryan Wu wrote:
> From: Cliff Cai <cliff.cai@analog.com>
> 
> set constraint only if the value is not 0, change the configuring way for sport

Hrm.  As far as I can tell the actual effect of this patch is to not do
any of the per-format configuration for the sport if the sport has been
configured once already - as far as I can tell nothing ever resets your
'configured' variable and this is the only place that the data format is
taken into account.  Won't this mean that if a second data format is
played the audio will be mishandled since the hardware will not have
been configured for the new audio format?

If it's really not possible to reconfigure the hardware (I'm assuming
that this is what the actual crash is?) I would expect to see code added
which remembers the format that has been configured and then adds a
constraint in the startup() function enforcing that.

Re: [PATCH 1/5] ASoC: Blackfin: fix bug - kernel will crash when record and play in bf527-ezkit

[Mark Brown]

On Fri, Mar 06, 2009 at 03:53:26PM +0800, Bryan Wu wrote:
> From: Cliff Cai <cliff.cai@analog.com>
> 
> set constraint only if the value is not 0, change the configuring way for sport

Hrm.  As far as I can tell the actual effect of this patch is to not do
any of the per-format configuration for the sport if the sport has been
configured once already - as far as I can tell nothing ever resets your
'configured' variable and this is the only place that the data format is
taken into account.  Won't this mean that if a second data format is
played the audio will be mishandled since the hardware will not have
been configured for the new audio format?

If it's really not possible to reconfigure the hardware (I'm assuming
that this is what the actual crash is?) I would expect to see code added
which remembers the format that has been configured and then adds a
constraint in the startup() function enforcing that.

RE: [alsa-devel] [PATCH 1/5] ASoC: Blackfin: fix bug - kernel willcrash when record and play in bf527-ezkit

[Cai, Cliff]

 
This patch should be named : change the configuring way for sport,
Because I found that the previous way is not reliable sometimes.
Application like "tone" ,will call starup() twice before entering
hw_params() to configure SPORT.  
in this case SPORT won't be configured at all.

Cliff

>-----Original Message-----
>From: Mark Brown [mailto:broonie@sirena.org.uk] 
>Sent: Friday, March 06, 2009 8:02 PM
>To: Bryan Wu
>Cc: Cliff Cai; alsa-devel@alsa-project.org; 
>linux-kernel@vger.kernel.org
>Subject: Re: [alsa-devel] [PATCH 1/5] ASoC: Blackfin: fix bug 
>- kernel willcrash when record and play in bf527-ezkit
>
>On Fri, Mar 06, 2009 at 03:53:26PM +0800, Bryan Wu wrote:
>> From: Cliff Cai <cliff.cai@analog.com>
>> 
>> set constraint only if the value is not 0, change the 
>configuring way 
>> for sport
>
>Hrm.  As far as I can tell the actual effect of this patch is 
>to not do any of the per-format configuration for the sport if 
>the sport has been configured once already - as far as I can 
>tell nothing ever resets your 'configured' variable and this 
>is the only place that the data format is taken into account.  
>Won't this mean that if a second data format is played the 
>audio will be mishandled since the hardware will not have been 
>configured for the new audio format?
>
>If it's really not possible to reconfigure the hardware (I'm 
>assuming that this is what the actual crash is?) I would 
>expect to see code added which remembers the format that has 
>been configured and then adds a constraint in the startup() 
>function enforcing that.
>

Re: [PATCH 1/5] ASoC: Blackfin: fix bug - kernel willcrash when record and play in bf527-ezkit

[Cai, Cliff]

 
This patch should be named : change the configuring way for sport,
Because I found that the previous way is not reliable sometimes.
Application like "tone" ,will call starup() twice before entering
hw_params() to configure SPORT.  
in this case SPORT won't be configured at all.

Cliff

>-----Original Message-----
>From: Mark Brown [mailto:broonie@sirena.org.uk] 
>Sent: Friday, March 06, 2009 8:02 PM
>To: Bryan Wu
>Cc: Cliff Cai; alsa-devel@alsa-project.org; 
>linux-kernel@vger.kernel.org
>Subject: Re: [alsa-devel] [PATCH 1/5] ASoC: Blackfin: fix bug 
>- kernel willcrash when record and play in bf527-ezkit
>
>On Fri, Mar 06, 2009 at 03:53:26PM +0800, Bryan Wu wrote:
>> From: Cliff Cai <cliff.cai@analog.com>
>> 
>> set constraint only if the value is not 0, change the 
>configuring way 
>> for sport
>
>Hrm.  As far as I can tell the actual effect of this patch is 
>to not do any of the per-format configuration for the sport if 
>the sport has been configured once already - as far as I can 
>tell nothing ever resets your 'configured' variable and this 
>is the only place that the data format is taken into account.  
>Won't this mean that if a second data format is played the 
>audio will be mishandled since the hardware will not have been 
>configured for the new audio format?
>
>If it's really not possible to reconfigure the hardware (I'm 
>assuming that this is what the actual crash is?) I would 
>expect to see code added which remembers the format that has 
>been configured and then adds a constraint in the startup() 
>function enforcing that.
>

Re: [alsa-devel] [PATCH 1/5] ASoC: Blackfin: fix bug - kernel willcrash when record and play in bf527-ezkit

[Mark Brown]

On Mon, Mar 09, 2009 at 06:58:19PM +0800, Cai, Cliff wrote:

> This patch should be named : change the configuring way for sport,
> Because I found that the previous way is not reliable sometimes.
> Application like "tone" ,will call starup() twice before entering
> hw_params() to configure SPORT.  
> in this case SPORT won't be configured at all.

This doesn't address the concerns I raised with the patch:

> >Won't this mean that if a second data format is played the 
> >audio will be mishandled since the hardware will not have been 
> >configured for the new audio format?

Re: [PATCH 1/5] ASoC: Blackfin: fix bug - kernel willcrash when record and play in bf527-ezkit

[Mark Brown]

On Mon, Mar 09, 2009 at 06:58:19PM +0800, Cai, Cliff wrote:

> This patch should be named : change the configuring way for sport,
> Because I found that the previous way is not reliable sometimes.
> Application like "tone" ,will call starup() twice before entering
> hw_params() to configure SPORT.  
> in this case SPORT won't be configured at all.

This doesn't address the concerns I raised with the patch:

> >Won't this mean that if a second data format is played the 
> >audio will be mishandled since the hardware will not have been 
> >configured for the new audio format?

RE: [alsa-devel] [PATCH 1/5] ASoC: Blackfin: fix bug - kernelwillcrash when record and play in bf527-ezkit

[Cai, Cliff]

 

>-----Original Message-----
>From: Mark Brown [mailto:broonie@sirena.org.uk] 
>Sent: Monday, March 09, 2009 7:21 PM
>To: Cai, Cliff
>Cc: Bryan Wu; alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org
>Subject: Re: [alsa-devel] [PATCH 1/5] ASoC: Blackfin: fix bug 
>- kernelwillcrash when record and play in bf527-ezkit
>
>On Mon, Mar 09, 2009 at 06:58:19PM +0800, Cai, Cliff wrote:
>
>> This patch should be named : change the configuring way for sport, 
>> Because I found that the previous way is not reliable sometimes.
>> Application like "tone" ,will call starup() twice before entering
>> hw_params() to configure SPORT.  
>> in this case SPORT won't be configured at all.
>
>This doesn't address the concerns I raised with the patch:
>
>> >Won't this mean that if a second data format is played the 
>audio will 
>> >be mishandled since the hardware will not have been configured for 
>> >the new audio format?
>

I think I understand your concerns,I will send a new patch if I'm free.

Thanks

Cliff 

Re: [PATCH 1/5] ASoC: Blackfin: fix bug - kernelwillcrash when record and play in bf527-ezkit

[Cai, Cliff]

 

>-----Original Message-----
>From: Mark Brown [mailto:broonie@sirena.org.uk] 
>Sent: Monday, March 09, 2009 7:21 PM
>To: Cai, Cliff
>Cc: Bryan Wu; alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org
>Subject: Re: [alsa-devel] [PATCH 1/5] ASoC: Blackfin: fix bug 
>- kernelwillcrash when record and play in bf527-ezkit
>
>On Mon, Mar 09, 2009 at 06:58:19PM +0800, Cai, Cliff wrote:
>
>> This patch should be named : change the configuring way for sport, 
>> Because I found that the previous way is not reliable sometimes.
>> Application like "tone" ,will call starup() twice before entering
>> hw_params() to configure SPORT.  
>> in this case SPORT won't be configured at all.
>
>This doesn't address the concerns I raised with the patch:
>
>> >Won't this mean that if a second data format is played the 
>audio will 
>> >be mishandled since the hardware will not have been configured for 
>> >the new audio format?
>

I think I understand your concerns,I will send a new patch if I'm free.

Thanks

Cliff 

[PATCH 2/5] ASoC: ssm2602 codec: fix bug - kernel will crash when record and play in bf527-ezkit

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

set constraint only if the value is not 0, change the configuring way for sport

Signed-off-by: Cliff Cai <cliff.cai@analog.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 sound/soc/codecs/ssm2602.c |   29 ++++++++++++++---------------
 1 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/sound/soc/codecs/ssm2602.c b/sound/soc/codecs/ssm2602.c
index cac3736..3fdba6a 100644
--- a/sound/soc/codecs/ssm2602.c
+++ b/sound/soc/codecs/ssm2602.c
@@ -49,8 +49,8 @@ struct snd_soc_codec_device soc_codec_dev_ssm2602;
 /* codec private data */
 struct ssm2602_priv {
 	unsigned int sysclk;
-	struct snd_pcm_substream *master_substream;
-	struct snd_pcm_substream *slave_substream;
+	unsigned int master_rate;
+	unsigned int master_sample_bits;
 };
 
 /*
@@ -339,31 +339,30 @@ static int ssm2602_startup(struct snd_pcm_substream *substream,
 	struct snd_soc_codec *codec = socdev->codec;
 	struct ssm2602_priv *ssm2602 = codec->private_data;
 	struct i2c_client *i2c = codec->control_data;
-	struct snd_pcm_runtime *master_runtime;
 
 	/* The DAI has shared clocks so if we already have a playback or
 	 * capture going then constrain this substream to match it.
 	 * TODO: the ssm2602 allows pairs of non-matching PB/REC rates
 	 */
-	if (ssm2602->master_substream) {
-		master_runtime = ssm2602->master_substream->runtime;
+	if (ssm2602->master_rate) {
 		dev_dbg(&i2c->dev, "Constraining to %d bits at %dHz\n",
-			master_runtime->sample_bits,
-			master_runtime->rate);
+			ssm2602->master_sample_bits,
+			ssm2602->master_rate);
 
 		snd_pcm_hw_constraint_minmax(substream->runtime,
 					     SNDRV_PCM_HW_PARAM_RATE,
-					     master_runtime->rate,
-					     master_runtime->rate);
-
+ 					     ssm2602->master_rate,
+ 					     ssm2602->master_rate);
+ 	} else
+ 		ssm2602->master_rate = substream->runtime->rate;
+  
+ 	if (ssm2602->master_sample_bits) {
 		snd_pcm_hw_constraint_minmax(substream->runtime,
 					     SNDRV_PCM_HW_PARAM_SAMPLE_BITS,
-					     master_runtime->sample_bits,
-					     master_runtime->sample_bits);
-
-		ssm2602->slave_substream = substream;
+ 					     ssm2602->master_sample_bits,
+ 					     ssm2602->master_sample_bits);
 	} else
-		ssm2602->master_substream = substream;
+ 		ssm2602->master_sample_bits = substream->runtime->sample_bits;
 
 	return 0;
 }
-- 
1.5.6.3

[PATCH 2/5] ASoC: ssm2602 codec: fix bug - kernel will crash when record and play in bf527-ezkit

[Bryan Wu]

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

set constraint only if the value is not 0, change the configuring way for sport

Signed-off-by: Cliff Cai <cliff.cai@analog.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 sound/soc/codecs/ssm2602.c |   29 ++++++++++++++---------------
 1 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/sound/soc/codecs/ssm2602.c b/sound/soc/codecs/ssm2602.c
index cac3736..3fdba6a 100644
--- a/sound/soc/codecs/ssm2602.c
+++ b/sound/soc/codecs/ssm2602.c
@@ -49,8 +49,8 @@ struct snd_soc_codec_device soc_codec_dev_ssm2602;
 /* codec private data */
 struct ssm2602_priv {
 	unsigned int sysclk;
-	struct snd_pcm_substream *master_substream;
-	struct snd_pcm_substream *slave_substream;
+	unsigned int master_rate;
+	unsigned int master_sample_bits;
 };
 
 /*
@@ -339,31 +339,30 @@ static int ssm2602_startup(struct snd_pcm_substream *substream,
 	struct snd_soc_codec *codec = socdev->codec;
 	struct ssm2602_priv *ssm2602 = codec->private_data;
 	struct i2c_client *i2c = codec->control_data;
-	struct snd_pcm_runtime *master_runtime;
 
 	/* The DAI has shared clocks so if we already have a playback or
 	 * capture going then constrain this substream to match it.
 	 * TODO: the ssm2602 allows pairs of non-matching PB/REC rates
 	 */
-	if (ssm2602->master_substream) {
-		master_runtime = ssm2602->master_substream->runtime;
+	if (ssm2602->master_rate) {
 		dev_dbg(&i2c->dev, "Constraining to %d bits at %dHz\n",
-			master_runtime->sample_bits,
-			master_runtime->rate);
+			ssm2602->master_sample_bits,
+			ssm2602->master_rate);
 
 		snd_pcm_hw_constraint_minmax(substream->runtime,
 					     SNDRV_PCM_HW_PARAM_RATE,
-					     master_runtime->rate,
-					     master_runtime->rate);
-
+ 					     ssm2602->master_rate,
+ 					     ssm2602->master_rate);
+ 	} else
+ 		ssm2602->master_rate = substream->runtime->rate;
+  
+ 	if (ssm2602->master_sample_bits) {
 		snd_pcm_hw_constraint_minmax(substream->runtime,
 					     SNDRV_PCM_HW_PARAM_SAMPLE_BITS,
-					     master_runtime->sample_bits,
-					     master_runtime->sample_bits);
-
-		ssm2602->slave_substream = substream;
+ 					     ssm2602->master_sample_bits,
+ 					     ssm2602->master_sample_bits);
 	} else
-		ssm2602->master_substream = substream;
+ 		ssm2602->master_sample_bits = substream->runtime->sample_bits;
 
 	return 0;
 }
-- 
1.5.6.3

Re: [alsa-devel] [PATCH 2/5] ASoC: ssm2602 codec: fix bug - kernel will crash when record and play in bf527-ezkit

[Karl Beldan]

Bryan Wu wrote:
> From: Cliff Cai <cliff.cai@analog.com>
> 
> set constraint only if the value is not 0, change the configuring way for sport
> 
> Signed-off-by: Cliff Cai <cliff.cai@analog.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>
> ---
>  sound/soc/codecs/ssm2602.c |   29 ++++++++++++++---------------
>  1 files changed, 14 insertions(+), 15 deletions(-)
>
What tree is this against ?

-- 
Karl Beldan

Re: [PATCH 2/5] ASoC: ssm2602 codec: fix bug - kernel will crash when record and play in bf527-ezkit

[Karl Beldan]

Bryan Wu wrote:
> From: Cliff Cai <cliff.cai@analog.com>
> 
> set constraint only if the value is not 0, change the configuring way for sport
> 
> Signed-off-by: Cliff Cai <cliff.cai@analog.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>
> ---
>  sound/soc/codecs/ssm2602.c |   29 ++++++++++++++---------------
>  1 files changed, 14 insertions(+), 15 deletions(-)
>
What tree is this against ?

-- 
Karl Beldan

Re: [alsa-devel] [PATCH 2/5] ASoC: ssm2602 codec: fix bug - kernel will crash when record and play in bf527-ezkit

[Mark Brown]

On Fri, Mar 06, 2009 at 03:53:27PM +0800, Bryan Wu wrote:

> set constraint only if the value is not 0, change the configuring way for sport

This changelog doesn't appear to apply to this patch - it looks like
was copied from the previous one?  This patch appears to suffer from a
similar issue where only the first format set by an application can ever
be used, though since this changes the constraints that are set by the
driver there's no possibility that applications will end up trying to
use a sample format that the hardware is not configured for.

Do we really need such a heavy handed fix here - what is the issue
that's being fixed?

Re: [PATCH 2/5] ASoC: ssm2602 codec: fix bug - kernel will crash when record and play in bf527-ezkit

[Mark Brown]

On Fri, Mar 06, 2009 at 03:53:27PM +0800, Bryan Wu wrote:

> set constraint only if the value is not 0, change the configuring way for sport

This changelog doesn't appear to apply to this patch - it looks like
was copied from the previous one?  This patch appears to suffer from a
similar issue where only the first format set by an application can ever
be used, though since this changes the constraints that are set by the
driver there's no possibility that applications will end up trying to
use a sample format that the hardware is not configured for.

Do we really need such a heavy handed fix here - what is the issue
that's being fixed?

RE: [alsa-devel] [PATCH 2/5] ASoC: ssm2602 codec: fix bug - kernelwill crash when record and play in bf527-ezkit

[Cai, Cliff]

 This patch should be named :set constraint only if the value is not 0.
 I found that sometimes substream may be not NULL,but runtime can be
NULL,So the previous way will cause
Kernel to crash.

Cliff

>-----Original Message-----
>From: Mark Brown [mailto:broonie@sirena.org.uk] 
>Sent: Friday, March 06, 2009 8:35 PM
>To: Bryan Wu
>Cc: Cliff Cai; alsa-devel@alsa-project.org; 
>linux-kernel@vger.kernel.org
>Subject: Re: [alsa-devel] [PATCH 2/5] ASoC: ssm2602 codec: fix 
>bug - kernelwill crash when record and play in bf527-ezkit
>
>On Fri, Mar 06, 2009 at 03:53:27PM +0800, Bryan Wu wrote:
>
>> set constraint only if the value is not 0, change the 
>configuring way 
>> for sport
>
>This changelog doesn't appear to apply to this patch - it 
>looks like was copied from the previous one?  This patch 
>appears to suffer from a similar issue where only the first 
>format set by an application can ever be used, though since 
>this changes the constraints that are set by the driver 
>there's no possibility that applications will end up trying to 
>use a sample format that the hardware is not configured for.
>
>Do we really need such a heavy handed fix here - what is the 
>issue that's being fixed?
>

RE: [alsa-devel] [PATCH 2/5] ASoC: ssm2602 codec: fix bug - kernelwill crash when record and play in bf527-ezkit

[Cai, Cliff]

 This patch should be named :set constraint only if the value is not 0.
 I found that sometimes substream may be not NULL,but runtime can be
NULL,So the previous way will cause
Kernel to crash.

Cliff

>-----Original Message-----
>From: Mark Brown [mailto:broonie@sirena.org.uk] 
>Sent: Friday, March 06, 2009 8:35 PM
>To: Bryan Wu
>Cc: Cliff Cai; alsa-devel@alsa-project.org; 
>linux-kernel@vger.kernel.org
>Subject: Re: [alsa-devel] [PATCH 2/5] ASoC: ssm2602 codec: fix 
>bug - kernelwill crash when record and play in bf527-ezkit
>
>On Fri, Mar 06, 2009 at 03:53:27PM +0800, Bryan Wu wrote:
>
>> set constraint only if the value is not 0, change the 
>configuring way 
>> for sport
>
>This changelog doesn't appear to apply to this patch - it 
>looks like was copied from the previous one?  This patch 
>appears to suffer from a similar issue where only the first 
>format set by an application can ever be used, though since 
>this changes the constraints that are set by the driver 
>there's no possibility that applications will end up trying to 
>use a sample format that the hardware is not configured for.
>
>Do we really need such a heavy handed fix here - what is the 
>issue that's being fixed?
>

Re: [alsa-devel] [PATCH 2/5] ASoC: ssm2602 codec: fix bug - kernelwill crash when record and play in bf527-ezkit

[Mark Brown]

On Mon, Mar 09, 2009 at 07:07:26PM +0800, Cai, Cliff wrote:

>  This patch should be named :set constraint only if the value is not 0.
>  I found that sometimes substream may be not NULL,but runtime can be
> NULL,So the previous way will cause
> Kernel to crash.

Again, this doesn't address the concern about the fact that you're only
supporting one time configuration here.

Re: [PATCH 2/5] ASoC: ssm2602 codec: fix bug - kernelwill crash when record and play in bf527-ezkit

[Mark Brown]

On Mon, Mar 09, 2009 at 07:07:26PM +0800, Cai, Cliff wrote:

>  This patch should be named :set constraint only if the value is not 0.
>  I found that sometimes substream may be not NULL,but runtime can be
> NULL,So the previous way will cause
> Kernel to crash.

Again, this doesn't address the concern about the fact that you're only
supporting one time configuration here.

[PATCH 3/5] ASoC: Blackfin: move gpio_err behind the define that is only user of it

[Bryan Wu]

From: Mike Frysinger <vapier.adi@gmail.com>

Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 sound/soc/blackfin/bf5xx-ac97.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sound/soc/blackfin/bf5xx-ac97.c b/sound/soc/blackfin/bf5xx-ac97.c
index 5885702..8a935f2 100644
--- a/sound/soc/blackfin/bf5xx-ac97.c
+++ b/sound/soc/blackfin/bf5xx-ac97.c
@@ -357,8 +357,8 @@ sport_config_err:
 sport_err:
 #ifdef CONFIG_SND_BF5XX_HAVE_COLD_RESET
 	gpio_free(CONFIG_SND_BF5XX_RESET_GPIO_NUM);
-#endif
 gpio_err:
+#endif
 	peripheral_free_list(sport_req[sport_num]);
 peripheral_err:
 	free_page((unsigned long)cmd_count);
-- 
1.5.6.3

Re: [alsa-devel] [PATCH 3/5] ASoC: Blackfin: move gpio_err behind the define that is only user of it

[Mark Brown]

On Fri, Mar 06, 2009 at 03:53:28PM +0800, Bryan Wu wrote:
> From: Mike Frysinger <vapier.adi@gmail.com>
> 
> Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>

Applied, thanks.

Re: [PATCH 3/5] ASoC: Blackfin: move gpio_err behind the define that is only user of it

[Mark Brown]

On Fri, Mar 06, 2009 at 03:53:28PM +0800, Bryan Wu wrote:
> From: Mike Frysinger <vapier.adi@gmail.com>
> 
> Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>

Applied, thanks.

[PATCH 4/5] ASoC: Blackfin: drop pointless casts due to dma updates

[Bryan Wu]

From: Mike Frysinger <vapier.adi@gmail.com>

Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 sound/blackfin/bf53x_sport.c |   55 ++++++++++++++++++-----------------------
 1 files changed, 24 insertions(+), 31 deletions(-)

diff --git a/sound/blackfin/bf53x_sport.c b/sound/blackfin/bf53x_sport.c
index 8dfa694..23ef579 100644
--- a/sound/blackfin/bf53x_sport.c
+++ b/sound/blackfin/bf53x_sport.c
@@ -165,7 +165,7 @@ static void setup_desc(struct dmasg *desc, void *buf, int fragcount,
 	int i;
 
 	for (i=0; i<fragcount; ++i) {
-		desc[i].next_desc_addr  = (unsigned long)&(desc[i + 1]);
+		desc[i].next_desc_addr  = &desc[i + 1];
 		desc[i].start_addr = (unsigned long)buf + i*fragsize;
 		desc[i].cfg = cfg;
 		desc[i].x_count = x_count;
@@ -175,7 +175,7 @@ static void setup_desc(struct dmasg *desc, void *buf, int fragcount,
 	}
 
 	/* make circular */
-	desc[fragcount-1].next_desc_addr = (unsigned long)desc;
+	desc[fragcount-1].next_desc_addr = desc;
 
 	/* printk(KERN_ERR "setup desc: desc0=%p, next0=%lx, desc1=%p,"
 		"next1=%lx\nx_count=%x,y_count=%x,addr=0x%lx,cfs=0x%x\n",
@@ -217,8 +217,7 @@ static inline int sport_hook_rx_dummy(struct bf53x_sport *sport)
 	SPORT_ASSERT(sport->dummy_rx_desc != NULL);
 	SPORT_ASSERT(sport->curr_rx_desc != sport->dummy_rx_desc);
 
-	sport->dummy_rx_desc->next_desc_addr = \
-			(unsigned long)(sport->dummy_rx_desc+1);
+	sport->dummy_rx_desc->next_desc_addr = sport->dummy_rx_desc + 1;
 
 	local_irq_save(flags);
 	desc = (struct dmasg *)get_dma_next_desc_ptr(sport->dma_rx_chan);
@@ -226,12 +225,12 @@ static inline int sport_hook_rx_dummy(struct bf53x_sport *sport)
 	temp_desc = *desc;
 	desc->x_count=0x10;
 	desc->y_count=0;
-	desc->next_desc_addr = (unsigned long)(sport->dummy_rx_desc);
+	desc->next_desc_addr = sport->dummy_rx_desc;
 	local_irq_restore(flags);
 	/* Waiting for dummy buffer descriptor is already hooked*/
 	while ((get_dma_curr_desc_ptr(sport->dma_rx_chan) - \
-			sizeof(struct dmasg)) != \
-			(unsigned long)sport->dummy_rx_desc) {}
+	        sizeof(struct dmasg)) != sport->dummy_rx_desc)
+		continue;
 	sport->curr_rx_desc = sport->dummy_rx_desc;
 	/* Restore the damaged descriptor */
 	*desc = temp_desc;
@@ -244,13 +243,12 @@ static inline int sport_rx_dma_start(struct bf53x_sport *sport, int dummy)
 	struct dma_register *dma = sport->dma_rx;
 
 	if (dummy) {
-		sport->dummy_rx_desc->next_desc_addr = \
-				(unsigned long) sport->dummy_rx_desc;
+		sport->dummy_rx_desc->next_desc_addr = sport->dummy_rx_desc;
 		sport->curr_rx_desc = sport->dummy_rx_desc;
 	} else
 		sport->curr_rx_desc = sport->dma_rx_desc;
 
-	dma->next_desc_ptr = (unsigned long)(sport->curr_rx_desc);
+	dma->next_desc_ptr = sport->curr_rx_desc;
 	dma->cfg           = DMAFLOW_LARGE | NDSIZE_9 | WDSIZE_32 | WNR;
 	dma->x_count       = 0;
 	dma->x_modify      = 0;
@@ -267,13 +265,12 @@ static inline int sport_tx_dma_start(struct bf53x_sport *sport, int dummy)
 	struct dma_register *dma = sport->dma_tx;
 
 	if (dummy) {
-		sport->dummy_tx_desc->next_desc_addr = \
-				(unsigned long) sport->dummy_tx_desc;
+		sport->dummy_tx_desc->next_desc_addr = sport->dummy_tx_desc;
 		sport->curr_tx_desc = sport->dummy_tx_desc;
 	} else
 		sport->curr_tx_desc = sport->dma_tx_desc;
 
-	dma->next_desc_ptr = (unsigned long)(sport->curr_tx_desc);
+	dma->next_desc_ptr = sport->curr_tx_desc;
 	dma->cfg           = DMAFLOW_LARGE | NDSIZE_9 | WDSIZE_32 ;
 	dma->x_count       = 0;
 	dma->x_modify      = 0;
@@ -298,10 +295,9 @@ int bf53x_sport_rx_start(struct bf53x_sport *sport)
 		SPORT_ASSERT(sport->curr_rx_desc == sport->dummy_rx_desc);
 		local_irq_save(flags);
 		while ((get_dma_curr_desc_ptr(sport->dma_rx_chan) - \
-			sizeof(struct dmasg)) != \
-			(unsigned long)sport->dummy_rx_desc) {}
-		sport->dummy_rx_desc->next_desc_addr = \
-				(unsigned long)(sport->dma_rx_desc);
+		        sizeof(struct dmasg)) != sport->dummy_rx_desc)
+			continue;
+		sport->dummy_rx_desc->next_desc_addr = sport->dma_rx_desc;
 		local_irq_restore(flags);
 		sport->curr_rx_desc = sport->dma_rx_desc;
 	} else {
@@ -343,8 +339,7 @@ static inline int sport_hook_tx_dummy(struct bf53x_sport *sport)
 	SPORT_ASSERT(sport->dummy_tx_desc != NULL);
 	SPORT_ASSERT(sport->curr_tx_desc != sport->dummy_tx_desc);
 
-	sport->dummy_tx_desc->next_desc_addr = \
-			(unsigned long)(sport->dummy_tx_desc+1);
+	sport->dummy_tx_desc->next_desc_addr = sport->dummy_tx_desc + 1;
 
 	/* Shorten the time on last normal descriptor */
 	local_irq_save(flags);
@@ -353,12 +348,12 @@ static inline int sport_hook_tx_dummy(struct bf53x_sport *sport)
 	temp_desc = *desc;
 	desc->x_count = 0x10;
 	desc->y_count = 0;
-	desc->next_desc_addr = (unsigned long)(sport->dummy_tx_desc);
+	desc->next_desc_addr = sport->dummy_tx_desc;
 	local_irq_restore(flags);
 	/* Waiting for dummy buffer descriptor is already hooked*/
 	while ((get_dma_curr_desc_ptr(sport->dma_tx_chan) - \
-			sizeof(struct dmasg)) != \
-			(unsigned long)sport->dummy_tx_desc) {}
+	        sizeof(struct dmasg)) != sport->dummy_tx_desc)
+		continue;
 	sport->curr_tx_desc = sport->dummy_tx_desc;
 	/* Restore the damaged descriptor */
 	*desc = temp_desc;
@@ -381,10 +376,9 @@ int bf53x_sport_tx_start(struct bf53x_sport *sport)
 		/* Hook the normal buffer descriptor */
 		local_irq_save(flags);
 		while ((get_dma_curr_desc_ptr(sport->dma_tx_chan) - \
-			sizeof(struct dmasg)) != \
-			(unsigned long)sport->dummy_tx_desc) {}
-		sport->dummy_tx_desc->next_desc_addr = \
-				(unsigned long)(sport->dma_tx_desc);
+		       sizeof(struct dmasg)) != sport->dummy_tx_desc)
+			continue;
+		sport->dummy_tx_desc->next_desc_addr = sport->dma_tx_desc;
 		local_irq_restore(flags);
 		sport->curr_tx_desc = sport->dma_tx_desc;
 	} else {
@@ -587,8 +581,8 @@ static int sport_config_rx_dummy(struct bf53x_sport *sport, size_t size)
 	desc->y_count = 0;
 	desc->y_modify = 0;
 	memcpy(desc+1, desc, sizeof(*desc));
-	desc->next_desc_addr = (unsigned long)(desc+1);
-	desc[1].next_desc_addr = (unsigned long)desc;
+	desc->next_desc_addr = desc + 1;
+	desc[1].next_desc_addr = desc;
 
 	return 0;
 }
@@ -623,8 +617,8 @@ static int sport_config_tx_dummy(struct bf53x_sport *sport, size_t size)
 	desc->y_count = 0;
 	desc->y_modify = 0;
 	memcpy(desc+1, desc, sizeof(*desc));
-	desc->next_desc_addr = (unsigned long)(desc+1);
-	desc[1].next_desc_addr = (unsigned long)desc;
+	desc->next_desc_addr = desc + 1;
+	desc[1].next_desc_addr = desc;
 
 	return 0;
 }
@@ -918,6 +912,5 @@ void bf53x_sport_done(struct bf53x_sport *sport)
 	free_dma(sport->dma_tx_chan);
 	free_irq(sport->err_irq, sport);
 
-
 	kfree(sport);
 }
-- 
1.5.6.3

Re: [alsa-devel] [PATCH 4/5] ASoC: Blackfin: drop pointless casts due to dma updates

[Mark Brown]

On Fri, Mar 06, 2009 at 03:53:29PM +0800, Bryan Wu wrote:
> From: Mike Frysinger <vapier.adi@gmail.com>
> 
> Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>
> ---
>  sound/blackfin/bf53x_sport.c |   55 ++++++++++++++++++-----------------------
>  1 files changed, 24 insertions(+), 31 deletions(-)

Mainline doesn't have a sound/blackfin directory at all as far as I can
tell so this patch doesn't apply?  The only mainline support I can see
for blackfin audio is sound/soc/blackfin which only has a generic
bf5xx-sport file.  A similar cleanup was done there a while ago.

Re: [PATCH 4/5] ASoC: Blackfin: drop pointless casts due to dma updates

[Mark Brown]

On Fri, Mar 06, 2009 at 03:53:29PM +0800, Bryan Wu wrote:
> From: Mike Frysinger <vapier.adi@gmail.com>
> 
> Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>
> ---
>  sound/blackfin/bf53x_sport.c |   55 ++++++++++++++++++-----------------------
>  1 files changed, 24 insertions(+), 31 deletions(-)

Mainline doesn't have a sound/blackfin directory at all as far as I can
tell so this patch doesn't apply?  The only mainline support I can see
for blackfin audio is sound/soc/blackfin which only has a generic
bf5xx-sport file.  A similar cleanup was done there a while ago.

Re: [alsa-devel] [PATCH 4/5] ASoC: Blackfin: drop pointless casts due to dma updates

[Bryan Wu]

On Fri, Mar 6, 2009 at 7:10 PM, Mark Brown <broonie@sirena.org.uk> wrote:
> On Fri, Mar 06, 2009 at 03:53:29PM +0800, Bryan Wu wrote:
>> From: Mike Frysinger <vapier.adi@gmail.com>
>>
>> Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
>> Signed-off-by: Bryan Wu <cooloney@kernel.org>
>> ---
>>  sound/blackfin/bf53x_sport.c |   55 ++++++++++++++++++-----------------------
>>  1 files changed, 24 insertions(+), 31 deletions(-)
>
> Mainline doesn't have a sound/blackfin directory at all as far as I can
> tell so this patch doesn't apply?  The only mainline support I can see
> for blackfin audio is sound/soc/blackfin which only has a generic
> bf5xx-sport file.  A similar cleanup was done there a while ago.
>

Oh, god. Sorry, my script sent out this in a mess. Please ignore this one

Thanks a lot
-Bryan

Re: [PATCH 4/5] ASoC: Blackfin: drop pointless casts due to dma updates

[Bryan Wu]

On Fri, Mar 6, 2009 at 7:10 PM, Mark Brown <broonie@sirena.org.uk> wrote:
> On Fri, Mar 06, 2009 at 03:53:29PM +0800, Bryan Wu wrote:
>> From: Mike Frysinger <vapier.adi@gmail.com>
>>
>> Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
>> Signed-off-by: Bryan Wu <cooloney@kernel.org>
>> ---
>>  sound/blackfin/bf53x_sport.c |   55 ++++++++++++++++++-----------------------
>>  1 files changed, 24 insertions(+), 31 deletions(-)
>
> Mainline doesn't have a sound/blackfin directory at all as far as I can
> tell so this patch doesn't apply?  The only mainline support I can see
> for blackfin audio is sound/soc/blackfin which only has a generic
> bf5xx-sport file.  A similar cleanup was done there a while ago.
>

Oh, god. Sorry, my script sent out this in a mess. Please ignore this one

Thanks a lot
-Bryan

[PATCH 5/5] ASoC: Blackfin: fix typo in MUTE definition

[Bryan Wu]

From: Mike Frysinger <vapier.adi@gmail.com>

Reported-by: Rob Maris <maris.rob@vdi.de>
Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 sound/soc/codecs/ad73311.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sound/soc/codecs/ad73311.h b/sound/soc/codecs/ad73311.h
index 507ce0c..569573d 100644
--- a/sound/soc/codecs/ad73311.h
+++ b/sound/soc/codecs/ad73311.h
@@ -70,7 +70,7 @@
 #define REGD_IGS(x)		(x & 0x7)
 #define REGD_RMOD		(1 << 3)
 #define REGD_OGS(x)		((x & 0x7) << 4)
-#define REGD_MUTE		(x << 7)
+#define REGD_MUTE		(1 << 7)
 
 /* Control register E */
 #define CTRL_REG_E	(4 << 8)
-- 
1.5.6.3

Re: [alsa-devel] [PATCH 5/5] ASoC: Blackfin: fix typo in MUTE definition

[Mark Brown]

On Fri, Mar 06, 2009 at 03:53:30PM +0800, Bryan Wu wrote:
> From: Mike Frysinger <vapier.adi@gmail.com>
> 
> Reported-by: Rob Maris <maris.rob@vdi.de>
> Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>

Applied, thanks.

Re: [PATCH 5/5] ASoC: Blackfin: fix typo in MUTE definition

[Mark Brown]

On Fri, Mar 06, 2009 at 03:53:30PM +0800, Bryan Wu wrote:
> From: Mike Frysinger <vapier.adi@gmail.com>
> 
> Reported-by: Rob Maris <maris.rob@vdi.de>
> Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>

Applied, thanks.