[PATCH 1/5] ASoC codec: SSM2602: remove unsupported sample rates

[PATCH 1/5] ASoC codec: SSM2602: remove unsupported sample rates

[Mike Frysinger]

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

Signed-off-by: Cliff Cai <cliff.cai@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 sound/soc/codecs/ssm2602.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/sound/soc/codecs/ssm2602.c b/sound/soc/codecs/ssm2602.c
index 87f606c..d6af069 100644
--- a/sound/soc/codecs/ssm2602.c
+++ b/sound/soc/codecs/ssm2602.c
@@ -497,11 +497,9 @@ static int ssm2602_set_bias_level(struct snd_soc_codec *codec,
 	return 0;
 }
 
-#define SSM2602_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 |\
-		SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 |\
-		SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |\
-		SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 |\
-		SNDRV_PCM_RATE_96000)
+#define SSM2602_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_32000 |\
+		SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |\
+		SNDRV_PCM_RATE_88200 | SNDRV_PCM_RATE_96000)
 
 #define SSM2602_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE |\
 		SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE)
-- 
1.6.3.1

[PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Mike Frysinger]

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

Fixes crash when shutting down.

Signed-off-by: Cliff Cai <cliff.cai@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 sound/soc/codecs/ssm2602.c |   25 ++++++++++++++++---------
 1 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/sound/soc/codecs/ssm2602.c b/sound/soc/codecs/ssm2602.c
index d6af069..1fc4c8e 100644
--- a/sound/soc/codecs/ssm2602.c
+++ b/sound/soc/codecs/ssm2602.c
@@ -336,15 +336,17 @@ static int ssm2602_startup(struct snd_pcm_substream *substream,
 			master_runtime->sample_bits,
 			master_runtime->rate);
 
-		snd_pcm_hw_constraint_minmax(substream->runtime,
-					     SNDRV_PCM_HW_PARAM_RATE,
-					     master_runtime->rate,
-					     master_runtime->rate);
-
-		snd_pcm_hw_constraint_minmax(substream->runtime,
-					     SNDRV_PCM_HW_PARAM_SAMPLE_BITS,
-					     master_runtime->sample_bits,
-					     master_runtime->sample_bits);
+		if (master_runtime->rate != 0)
+			snd_pcm_hw_constraint_minmax(substream->runtime,
+						     SNDRV_PCM_HW_PARAM_RATE,
+						     master_runtime->rate,
+						     master_runtime->rate);
+
+		if (master_runtime->sample_bits != 0)
+			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;
 	} else
@@ -372,6 +374,11 @@ static void ssm2602_shutdown(struct snd_pcm_substream *substream,
 	struct snd_soc_device *socdev = rtd->socdev;
 	struct snd_soc_codec *codec = socdev->card->codec;
 	struct ssm2602_priv *ssm2602 = codec->private_data;
+
+	if (ssm2602->master_substream == substream)
+		ssm2602->master_substream = ssm2602->slave_substream;
+
+	ssm2602->slave_substream = NULL;
 	/* deactivate */
 	if (!codec->active)
 		ssm2602_write(codec, SSM2602_ACTIVE, 0);
-- 
1.6.3.1

[PATCH 3/5] ASoC: Blackfin: tweak how we initialize the SPORT

[Mike Frysinger]

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

Treat the configured field as a boolean rather than using a counter where
we only check one value so the process is more reliable.

Signed-off-by: Cliff Cai <cliff.cai@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 sound/soc/blackfin/bf5xx-i2s.c |   18 ++++--------------
 1 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/sound/soc/blackfin/bf5xx-i2s.c b/sound/soc/blackfin/bf5xx-i2s.c
index 9648244..1c07696 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
@@ -190,6 +180,7 @@ static int bf5xx_i2s_hw_params(struct snd_pcm_substream *substream,
 			pr_err("SPORT is busy!\n");
 			return -EBUSY;
 		}
+		bf5xx_i2s.configured = 1;
 	}
 
 	return 0;
@@ -199,7 +190,7 @@ static void bf5xx_i2s_shutdown(struct snd_pcm_substream *substream,
 			       struct snd_soc_dai *dai)
 {
 	pr_debug("%s enter\n", __func__);
-	bf5xx_i2s.counter--;
+	bf5xx_i2s.configured = 0;
 }
 
 static int bf5xx_i2s_probe(struct platform_device *pdev,
@@ -288,7 +279,6 @@ static int bf5xx_i2s_resume(struct platform_device *pdev,
 	SNDRV_PCM_FMTBIT_S32_LE)
 
 static struct snd_soc_dai_ops bf5xx_i2s_dai_ops = {
-	.startup	= bf5xx_i2s_startup,
 	.shutdown	= bf5xx_i2s_shutdown,
 	.hw_params	= bf5xx_i2s_hw_params,
 	.set_fmt	= bf5xx_i2s_set_dai_fmt,
-- 
1.6.3.1

[PATCH 4/5] ASoC: Blackfin: set the transfer size according the ac97_frame size

[Mike Frysinger]

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

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

diff --git a/sound/soc/blackfin/bf5xx-sport.c b/sound/soc/blackfin/bf5xx-sport.c
index b7953c8..469ce7f 100644
--- a/sound/soc/blackfin/bf5xx-sport.c
+++ b/sound/soc/blackfin/bf5xx-sport.c
@@ -190,7 +190,7 @@ static inline int sport_hook_rx_dummy(struct sport_device *sport)
 	desc = get_dma_next_desc_ptr(sport->dma_rx_chan);
 	/* Copy the descriptor which will be damaged to backup */
 	temp_desc = *desc;
-	desc->x_count = 0xa;
+	desc->x_count = sport->dummy_count / 2;
 	desc->y_count = 0;
 	desc->next_desc_addr = sport->dummy_rx_desc;
 	local_irq_restore(flags);
@@ -309,7 +309,7 @@ static inline int sport_hook_tx_dummy(struct sport_device *sport)
 	desc = get_dma_next_desc_ptr(sport->dma_tx_chan);
 	/* Store the descriptor which will be damaged */
 	temp_desc = *desc;
-	desc->x_count = 0xa;
+	desc->x_count = sport->dummy_count / 2;
 	desc->y_count = 0;
 	desc->next_desc_addr = sport->dummy_tx_desc;
 	local_irq_restore(flags);
-- 
1.6.3.1

[PATCH 5/5] ASoC: Blackfin: document how anomaly 05000250 is handled

[Mike Frysinger]

From: Sonic Zhang <sonic.zhang@analog.com>

Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 sound/soc/blackfin/bf5xx-ac97.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/sound/soc/blackfin/bf5xx-ac97.c b/sound/soc/blackfin/bf5xx-ac97.c
index 8a935f2..b1ed423 100644
--- a/sound/soc/blackfin/bf5xx-ac97.c
+++ b/sound/soc/blackfin/bf5xx-ac97.c
@@ -31,6 +31,15 @@
 #include "bf5xx-sport.h"
 #include "bf5xx-ac97.h"
 
+/* Anomaly notes:
+ *  05000250 -	AD1980 is running in TDM mode and RFS/TFS are generated by SPORT
+ *		contrtoller. But, RFSDIV and TFSDIV are always set to 16*16-1,
+ *		while the max AC97 data size is 13*16. The DIV is always larger
+ *		than data size. AD73311 and ad2602 are not running in TDM mode.
+ *		AD1836 and AD73322 depend on external RFS/TFS only. So, this
+ *		anomaly does not affect blackfin sound drivers.
+*/
+
 static int *cmd_count;
 static int sport_num = CONFIG_SND_BF5XX_SPORT_NUM;
 
-- 
1.6.3.1

Re: [PATCH 1/5] ASoC codec: SSM2602: remove unsupported sample rates

[Mark Brown]

On Tue, Jun 02, 2009 at 12:18:53AM -0400, Mike Frysinger wrote:
> From: Cliff Cai <cliff.cai@analog.com>
> 
> Signed-off-by: Cliff Cai <cliff.cai@analog.com>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

Applied, thanks.

Re: [PATCH 3/5] ASoC: Blackfin: tweak how we initialize the SPORT

[Mark Brown]

On Tue, Jun 02, 2009 at 12:18:55AM -0400, Mike Frysinger wrote:

> @@ -199,7 +190,7 @@ static void bf5xx_i2s_shutdown(struct snd_pcm_substream *substream,
>  			       struct snd_soc_dai *dai)
>  {
>  	pr_debug("%s enter\n", __func__);
> -	bf5xx_i2s.counter--;
> +	bf5xx_i2s.configured = 0;
>  }

Are you sure this works properly with simultaneous record and playback?
If one stops while the other is running then the configured flag will be
cleared, meaning that the stopped stream can be restarted and used to
reconfigure the DAI which appears to be what the code is trying to
prevent.

This should be using constraints to stop applications trying to
reconfigure active streams - for the sample rate you just need to set
symmetric_rates in the DAI.

Re: [PATCH 4/5] ASoC: Blackfin: set the transfer size according the ac97_frame size

[Mark Brown]

On Tue, Jun 02, 2009 at 12:18:56AM -0400, Mike Frysinger wrote:
> From: Cliff Cai <cliff.cai@analog.com>
> 
> Signed-off-by: Cliff Cai <cliff.cai@analog.com>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>

Applied, thanks.

Re: [PATCH 5/5] ASoC: Blackfin: document how anomaly 05000250 is handled

[Mark Brown]

On Tue, Jun 02, 2009 at 12:18:57AM -0400, Mike Frysinger wrote:
> From: Sonic Zhang <sonic.zhang@analog.com>
> 
> Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

Applied, thanks.

Re: [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Mark Brown]

On Tue, Jun 02, 2009 at 12:18:54AM -0400, Mike Frysinger wrote:

> -		snd_pcm_hw_constraint_minmax(substream->runtime,
> -					     SNDRV_PCM_HW_PARAM_RATE,
> -					     master_runtime->rate,
> -					     master_runtime->rate);
> -
> -		snd_pcm_hw_constraint_minmax(substream->runtime,
> -					     SNDRV_PCM_HW_PARAM_SAMPLE_BITS,
> -					     master_runtime->sample_bits,
> -					     master_runtime->sample_bits);
> +		if (master_runtime->rate != 0)
> +			snd_pcm_hw_constraint_minmax(substream->runtime,
> +						     SNDRV_PCM_HW_PARAM_RATE,
> +						     master_runtime->rate,
> +						     master_runtime->rate);

> +
> +		if (master_runtime->sample_bits != 0)
> +			snd_pcm_hw_constraint_minmax(substream->runtime,
> +						     SNDRV_PCM_HW_PARAM_SAMPLE_BITS,
> +						     master_runtime->sample_bits,
> +						     master_runtime->sample_bits);

This change is entirely unrelated to your changelog and should have been
submitted as a separate patch.  It's preventing the use of the
configuration from an unconfigured master which isn't what the patch is
about).  It's also going over the 80 column limit.

For the rate constraint you should switch to using the symmetric_rates 
flag in the DAI which implements the equivalent constraints with no
per-driver code.

However, given that this fix has missed two kernel releases due to the
very slow rate of responses to review comments I've applied it so users
get the benefit of the bug fix.  Things would go more smoothly if you
could get new versions of patches submitted faster after the original
review.

Re: [PATCH 1/5] ASoC codec: SSM2602: remove unsupported sample rates

[Karl Beldan]

Mark Brown wrote:
> On Tue, Jun 02, 2009 at 12:18:53AM -0400, Mike Frysinger wrote:
>> From: Cliff Cai <cliff.cai@analog.com>
>>
>> Signed-off-by: Cliff Cai <cliff.cai@analog.com>
>> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> 
> Applied, thanks.

These samples rates are supported by the codec, updating coeff_div would be a better move.

-- 
Karl

Re: [PATCH 1/5] ASoC codec: SSM2602: remove unsupported sample rates

[Mark Brown]

On Wed, Jun 03, 2009 at 02:11:36PM +0200, Karl Beldan wrote:

> These samples rates are supported by the codec, updating coeff_div
> would be a better move.

It looks like the driver only supports a limited subset of the chip
features as it is - there's also comments saying it supports asymmetric
configurations but currently the driver only does symmetric record and
playback setups.

Re: [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Mike Frysinger]

On Wed, Jun 3, 2009 at 06:52, Mark Brown wrote:
> However, given that this fix has missed two kernel releases due to the
> very slow rate of responses to review comments I've applied it so users
> get the benefit of the bug fix.

not sure what you're referring to

> Things would go more smoothly if you
> could get new versions of patches submitted faster after the original
> review.

the previous Blackfin kernel maintainer changed jobs so we've been
transitioning.  reality is, this doesnt happen over nite.
-mike
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/5] ASoC codec: SSM2602: remove unsupported sample rates

[Karl Beldan]

Mark Brown wrote:
> On Wed, Jun 03, 2009 at 02:11:36PM +0200, Karl Beldan wrote:
> 
>> These samples rates are supported by the codec, updating coeff_div
>> would be a better move.
> 
> It looks like the driver only supports a limited subset of the chip
> features as it is - there's also comments saying it supports asymmetric
> configurations but currently the driver only does symmetric record and
> playback setups.

True. The comment was from me.
The codec supports these rates and the driver only needs little
modification to support them (just update an array - coeff_div).
Though this commit is better than nothing, updating coeff_div seems IMO
the thing to do. This change being trivial, the commit feels like a
feature cut.

-- 
Karl

Re: [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Mark Brown]

On Wed, Jun 03, 2009 at 08:37:54AM -0400, Mike Frysinger wrote:
> On Wed, Jun 3, 2009 at 06:52, Mark Brown wrote:

> > However, given that this fix has missed two kernel releases due to the
> > very slow rate of responses to review comments I've applied it so users
> > get the benefit of the bug fix.

> not sure what you're referring to

This looks like a respin of a fix which was originally submitted back in
2.6.29 era or so.  IIRC there's been two or three versions, each of
which needed fixes.

> > Things would go more smoothly if you
> > could get new versions of patches submitted faster after the original
> > review.

> the previous Blackfin kernel maintainer changed jobs so we've been
> transitioning.  reality is, this doesnt happen over nite.

I appreciate that Brian leaving won't have helped here but this was an
issue before then, it feels like there's some sort of general process
issue going on.  I'm not sure what it is - IIRC there tends to be about
one set of Blackfin patches per kernel release.

It's not specifically causing me problems, it's more of a concern from
the point of view of getting stuff to users.

Re: [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Mike Frysinger]

On Wed, Jun 3, 2009 at 10:53, Mark Brown wrote:
> On Wed, Jun 03, 2009 at 08:37:54AM -0400, Mike Frysinger wrote:
>> On Wed, Jun 3, 2009 at 06:52, Mark Brown wrote:
>> > However, given that this fix has missed two kernel releases due to the
>> > very slow rate of responses to review comments I've applied it so users
>> > get the benefit of the bug fix.
>> not sure what you're referring to
>
> This looks like a respin of a fix which was originally submitted back in
> 2.6.29 era or so.  IIRC there's been two or three versions, each of
> which needed fixes.

it could be.  i was going by the changes that werent in 2.6.30.  i
thought i looked for previous submissions, but i must have missed
previous attempts.  i'm not familiar with alsa stuff, so i'm not
always sure when i need to bang on Cliff to get things fixed up
(changes split / better comments / etc...).

>> > Things would go more smoothly if you
>> > could get new versions of patches submitted faster after the original
>> > review.
>
>> the previous Blackfin kernel maintainer changed jobs so we've been
>> transitioning.  reality is, this doesnt happen over nite.
>
> I appreciate that Brian leaving won't have helped here but this was an
> issue before then, it feels like there's some sort of general process
> issue going on.  I'm not sure what it is - IIRC there tends to be about
> one set of Blackfin patches per kernel release.
>
> It's not specifically causing me problems, it's more of a concern from
> the point of view of getting stuff to users.

if fixes miss a merge window, then that's an issue.  but in general,
users only see fixes per release, so sending things more quickly there
wouldnt really help ?
-mike
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 3/5] ASoC: Blackfin: tweak how weinitialize the SPORT

[Cai, Cliff]

 

>-----Original Message-----
>From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] 
>Sent: Wednesday, June 03, 2009 6:29 PM
>To: Mike Frysinger
>Cc: alsa-devel@alsa-project.org; 
>uclinux-dist-devel@blackfin.uclinux.org; Cai, Cliff
>Subject: Re: [alsa-devel] [PATCH 3/5] ASoC: Blackfin: tweak 
>how weinitialize the SPORT
>
>On Tue, Jun 02, 2009 at 12:18:55AM -0400, Mike Frysinger wrote:
>
>> @@ -199,7 +190,7 @@ static void bf5xx_i2s_shutdown(struct 
>snd_pcm_substream *substream,
>>  			       struct snd_soc_dai *dai)
>>  {
>>  	pr_debug("%s enter\n", __func__);
>> -	bf5xx_i2s.counter--;
>> +	bf5xx_i2s.configured = 0;
>>  }
>
>Are you sure this works properly with simultaneous record and playback?
>If one stops while the other is running then the configured 
>flag will be cleared, meaning that the stopped stream can be 
>restarted and used to reconfigure the DAI which appears to be 
>what the code is trying to prevent.
>
>This should be using constraints to stop applications trying 
>to reconfigure active streams - for the sample rate you just 
>need to set symmetric_rates in the DAI.
>


This is not a problem of rate but format,I think combine the two flags
will solve all the problems.
The reason I write this patch is that I found when some oss-based
applications open soundcard ,the startup callbacks will be called
several times.
So using the counter to record opened pcm streams is not reliable.

Thanks

Cliff 

Re: [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Karl Beldan]

Mike Frysinger wrote:
> On Wed, Jun 3, 2009 at 10:53, Mark Brown wrote:
>> On Wed, Jun 03, 2009 at 08:37:54AM -0400, Mike Frysinger wrote:
>>> On Wed, Jun 3, 2009 at 06:52, Mark Brown wrote:
>>>> However, given that this fix has missed two kernel releases due to the
>>>> very slow rate of responses to review comments I've applied it so users
>>>> get the benefit of the bug fix.
>>> not sure what you're referring to
>> This looks like a respin of a fix which was originally submitted back in
>> 2.6.29 era or so.  IIRC there's been two or three versions, each of
>> which needed fixes.
> 
> it could be.  i was going by the changes that werent in 2.6.30.  i
> thought i looked for previous submissions, but i must have missed
> previous attempts.  i'm not familiar with alsa stuff, so i'm not
> always sure when i need to bang on Cliff to get things fixed up
> (changes split / better comments / etc...).
> 

Anyways, this commit has been trying to fix something already fixed since the
first submission and if you look at its effect on the current tree you will see
duplicate code.

-- 
Karl

Re: [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Mike Frysinger]

On Thu, Jun 4, 2009 at 04:17, Karl Beldan wrote:
> Mike Frysinger wrote:
>> On Wed, Jun 3, 2009 at 10:53, Mark Brown wrote:
>>> On Wed, Jun 03, 2009 at 08:37:54AM -0400, Mike Frysinger wrote:
>>>> On Wed, Jun 3, 2009 at 06:52, Mark Brown wrote:
>>>>> However, given that this fix has missed two kernel releases due to the
>>>>> very slow rate of responses to review comments I've applied it so users
>>>>> get the benefit of the bug fix.
>>>> not sure what you're referring to
>>> This looks like a respin of a fix which was originally submitted back in
>>> 2.6.29 era or so.  IIRC there's been two or three versions, each of
>>> which needed fixes.
>>
>> it could be.  i was going by the changes that werent in 2.6.30.  i
>> thought i looked for previous submissions, but i must have missed
>> previous attempts.  i'm not familiar with alsa stuff, so i'm not
>> always sure when i need to bang on Cliff to get things fixed up
>> (changes split / better comments / etc...).
>
> Anyways, this commit has been trying to fix something already fixed since the
> first submission and if you look at its effect on the current tree you will see
> duplicate code.

you're going to have to be a bit more specific.  i dont know alsa, so
when i looked at the change history, i saw framework conversions.
about the only thing that might be related is faab5a32.  is that what
you're talking about ?
-mike
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Cai, Cliff]

 

>-----Original Message-----
>From: Mike Frysinger [mailto:vapier.adi@gmail.com] 
>Sent: Thursday, June 04, 2009 4:23 PM
>To: Karl Beldan
>Cc: Mark Brown; uclinux-dist-devel@blackfin.uclinux.org; 
>alsa-devel@alsa-project.org; Cai, Cliff
>Subject: Re: [alsa-devel] [Uclinux-dist-devel] [PATCH 2/5] 
>ASoC codec: SSM2602: assign last substream to the master when 
>shutting down
>
>On Thu, Jun 4, 2009 at 04:17, Karl Beldan wrote:
>> Mike Frysinger wrote:
>>> On Wed, Jun 3, 2009 at 10:53, Mark Brown wrote:
>>>> On Wed, Jun 03, 2009 at 08:37:54AM -0400, Mike Frysinger wrote:
>>>>> On Wed, Jun 3, 2009 at 06:52, Mark Brown wrote:
>>>>>> However, given that this fix has missed two kernel 
>releases due to 
>>>>>> the very slow rate of responses to review comments I've 
>applied it 
>>>>>> so users get the benefit of the bug fix.
>>>>> not sure what you're referring to
>>>> This looks like a respin of a fix which was originally submitted 
>>>> back in
>>>> 2.6.29 era or so.  IIRC there's been two or three 
>versions, each of 
>>>> which needed fixes.
>>>
>>> it could be.  i was going by the changes that werent in 2.6.30.  i 
>>> thought i looked for previous submissions, but i must have missed 
>>> previous attempts.  i'm not familiar with alsa stuff, so i'm not 
>>> always sure when i need to bang on Cliff to get things fixed up 
>>> (changes split / better comments / etc...).
>>
>> Anyways, this commit has been trying to fix something already fixed 
>> since the first submission and if you look at its effect on the 
>> current tree you will see duplicate code.
>
>you're going to have to be a bit more specific.  i dont know 
>alsa, so when i looked at the change history, i saw framework 
>conversions.
>about the only thing that might be related is faab5a32.  is 
>that what you're talking about ?
>-mike
>

The blackfin tree is quite different from the latest mainline tree now,I need to make fixes against the mainline tree
And resend the patches

Cliff

Re: [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Karl Beldan]

Mike Frysinger wrote:
> On Thu, Jun 4, 2009 at 04:17, Karl Beldan wrote:
>> Mike Frysinger wrote:
>>> On Wed, Jun 3, 2009 at 10:53, Mark Brown wrote:
>>>> On Wed, Jun 03, 2009 at 08:37:54AM -0400, Mike Frysinger wrote:
>>>>> On Wed, Jun 3, 2009 at 06:52, Mark Brown wrote:
>>>>>> However, given that this fix has missed two kernel releases due to the
>>>>>> very slow rate of responses to review comments I've applied it so users
>>>>>> get the benefit of the bug fix.
>>>>> not sure what you're referring to
>>>> This looks like a respin of a fix which was originally submitted back in
>>>> 2.6.29 era or so.  IIRC there's been two or three versions, each of
>>>> which needed fixes.
>>> it could be.  i was going by the changes that werent in 2.6.30.  i
>>> thought i looked for previous submissions, but i must have missed
>>> previous attempts.  i'm not familiar with alsa stuff, so i'm not
>>> always sure when i need to bang on Cliff to get things fixed up
>>> (changes split / better comments / etc...).
>> Anyways, this commit has been trying to fix something already fixed since the
>> first submission and if you look at its effect on the current tree you will see
>> duplicate code.
> 
> you're going to have to be a bit more specific.  i dont know alsa, so
> when i looked at the change history, i saw framework conversions.
> about the only thing that might be related is faab5a32.  is that what
> you're talking about ?

Indeed.
The commit duplicates some code, IMO this should be discussed before going further.

-- 
Karl

Re: [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Mike Frysinger]

On Thu, Jun 4, 2009 at 04:31, Cai, Cliff wrote:
> The blackfin tree is quite different from the latest mainline tree now,I need to make fixes against the mainline tree
> And resend the patches

i wouldnt bother.  2.6.30 is going to be out shortly and i'll update
our tree to that soon after.
-mike

Re: [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Cai, Cliff]

 

>-----Original Message-----
>From: Karl Beldan [mailto:karl.beldan@gmail.com] 
>Sent: Thursday, June 04, 2009 4:41 PM
>To: Mike Frysinger
>Cc: Mark Brown; uclinux-dist-devel@blackfin.uclinux.org; 
>alsa-devel@alsa-project.org; Cai, Cliff
>Subject: Re: [alsa-devel] [Uclinux-dist-devel] [PATCH 2/5] 
>ASoC codec: SSM2602: assign last substream to the master when 
>shutting down
>
>Mike Frysinger wrote:
>> On Thu, Jun 4, 2009 at 04:17, Karl Beldan wrote:
>>> Mike Frysinger wrote:
>>>> On Wed, Jun 3, 2009 at 10:53, Mark Brown wrote:
>>>>> On Wed, Jun 03, 2009 at 08:37:54AM -0400, Mike Frysinger wrote:
>>>>>> On Wed, Jun 3, 2009 at 06:52, Mark Brown wrote:
>>>>>>> However, given that this fix has missed two kernel releases due 
>>>>>>> to the very slow rate of responses to review comments I've 
>>>>>>> applied it so users get the benefit of the bug fix.
>>>>>> not sure what you're referring to
>>>>> This looks like a respin of a fix which was originally submitted 
>>>>> back in
>>>>> 2.6.29 era or so.  IIRC there's been two or three 
>versions, each of 
>>>>> which needed fixes.
>>>> it could be.  i was going by the changes that werent in 2.6.30.  i 
>>>> thought i looked for previous submissions, but i must have missed 
>>>> previous attempts.  i'm not familiar with alsa stuff, so i'm not 
>>>> always sure when i need to bang on Cliff to get things fixed up 
>>>> (changes split / better comments / etc...).
>>> Anyways, this commit has been trying to fix something already fixed 
>>> since the first submission and if you look at its effect on the 
>>> current tree you will see duplicate code.
>> 
>> you're going to have to be a bit more specific.  i dont know 
>alsa, so 
>> when i looked at the change history, i saw framework conversions.
>> about the only thing that might be related is faab5a32.  is 
>that what 
>> you're talking about ?
>
>Indeed.
>The commit duplicates some code, IMO this should be discussed 
>before going further.

Could you point it out,I don't know much about the changes of alsa in
recent months.

Thanks a lot.

Cliff

Re: [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Mike Frysinger]

On Thu, Jun 4, 2009 at 04:50, Cai, Cliff wrote:
> Could you point it out,I don't know much about the changes of alsa in
> recent months.

he is talking about this commit:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=faab5a32f4d0784d6bde57963267be0453be3546

which presumably makes the second change of your patch obsolete.
which leaves the first change and i think Mark is still awaiting an
explanation for it as it appears to be unrelated to the latter change.
-mike

Re: [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Cai, Cliff]

 

>-----Original Message-----
>From: Mike Frysinger [mailto:vapier.adi@gmail.com] 
>Sent: Thursday, June 04, 2009 4:46 PM
>To: Cai, Cliff
>Cc: Karl Beldan; Mark Brown; 
>uclinux-dist-devel@blackfin.uclinux.org; alsa-devel@alsa-project.org
>Subject: Re: [alsa-devel] [Uclinux-dist-devel] [PATCH 2/5] 
>ASoC codec: SSM2602: assign last substream to the master when 
>shutting down
>
>On Thu, Jun 4, 2009 at 04:31, Cai, Cliff wrote:
>> The blackfin tree is quite different from the latest mainline tree 
>> now,I need to make fixes against the mainline tree And resend the 
>> patches
>
>i wouldnt bother.  2.6.30 is going to be out shortly and i'll 
>update our tree to that soon after.


Then,should I re-make the patches against the mainline tree right
now(I've cloned the mainline tree locally)
 or wait untill you've updated blackfin tree?

Cliff  

Re: [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Mark Brown]

On Wed, Jun 03, 2009 at 09:26:34PM -0400, Mike Frysinger wrote:

> it could be.  i was going by the changes that werent in 2.6.30.  i
> thought i looked for previous submissions, but i must have missed
> previous attempts.  i'm not familiar with alsa stuff, so i'm not

The patch hadn't previously been sent in that form but it looks like
it's attempting to fix a bug which had had some previous goes at fixing
it that required updates.

> always sure when i need to bang on Cliff to get things fixed up
> (changes split / better comments / etc...).

If you're not sure there's a case for better changelogs at least! :)

> if fixes miss a merge window, then that's an issue.  but in general,
> users only see fixes per release, so sending things more quickly there
> wouldnt really help ?

The issue is that things are missing merge windows - they're getting
sent in but if there's any review comments a revised version of the
patch isn't getting posted until the next batch of submissions.  It's
these revisions rather than the initial submissions that I'm concerned
about.

Re: [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Mike Frysinger]

On Thu, Jun 4, 2009 at 04:59, Mark Brown wrote:
> On Wed, Jun 03, 2009 at 09:26:34PM -0400, Mike Frysinger wrote:
>> always sure when i need to bang on Cliff to get things fixed up
>> (changes split / better comments / etc...).
>
> If you're not sure there's a case for better changelogs at least! :)

meh, ive read some of the alsa changelogs and looked through the code
and none of it really mattered.  i'm just happy sound comes out of my
machine at all (including mixing).

>> if fixes miss a merge window, then that's an issue.  but in general,
>> users only see fixes per release, so sending things more quickly there
>> wouldnt really help ?
>
> The issue is that things are missing merge windows - they're getting
> sent in but if there's any review comments a revised version of the
> patch isn't getting posted until the next batch of submissions.  It's
> these revisions rather than the initial submissions that I'm concerned
> about.

well i can say i'm addressing that, but really nothing i say matters
as this will only be proven over time.
-mike
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Karl Beldan]

Cai, Cliff wrote:
>  
> 
>> -----Original Message-----
>> From: Mike Frysinger [mailto:vapier.adi@gmail.com] 
>> Sent: Thursday, June 04, 2009 4:46 PM
>> To: Cai, Cliff
>> Cc: Karl Beldan; Mark Brown; 
>> uclinux-dist-devel@blackfin.uclinux.org; alsa-devel@alsa-project.org
>> Subject: Re: [alsa-devel] [Uclinux-dist-devel] [PATCH 2/5] 
>> ASoC codec: SSM2602: assign last substream to the master when 
>> shutting down
>>
>> On Thu, Jun 4, 2009 at 04:31, Cai, Cliff wrote:
>>> The blackfin tree is quite different from the latest mainline tree 
>>> now,I need to make fixes against the mainline tree And resend the 
>>> patches
>> i wouldnt bother.  2.6.30 is going to be out shortly and i'll 
>> update our tree to that soon after.
> 
> 
> Then,should I re-make the patches against the mainline tree right
> now(I've cloned the mainline tree locally)
>  or wait untill you've updated blackfin tree?
> 

I suggest you test the blackfin tree once it has been updated, see if it got rid
 of the problems you encountered back then.


-- 
Karl

Re: [PATCH 3/5] ASoC: Blackfin: tweak how weinitialize the SPORT

[Mark Brown]

On Thu, Jun 04, 2009 at 10:36:48AM +0800, Cai, Cliff wrote:

> This is not a problem of rate but format,I think combine the two flags
> will solve all the problems.

Ah, yes - I was remembering the ssm2602 driver which does do rate
constraints.

> The reason I write this patch is that I found when some oss-based
> applications open soundcard ,the startup callbacks will be called
> several times.

All OSS applications do this, OSS has a separate ioctl for each
configuration option so you get lots of configuration calls.

> So using the counter to record opened pcm streams is not reliable.

It's only hw_params() that get called repeatedly.

I could be wrong but looking at the code I suspect you may just need to
make the reconfiguration conditional on if it's a capture or playback
stream and you'll be fine - at the minute you're configuring both RX and
TX on every hw_params() but there are separate tcr2 and rcr2 registers
that you write to.  If you only wrote to the appropriate one then you
wouldn't need to worry about interfering with another stream for that
configuration.  There is the wdsize parameter for the SPORT as well but
a brief glance at the code for that suggests that there are actually
separate RX and TX configurations there too.

If not then adding constraint code would help a lot.

Re: [PATCH 3/5] ASoC: Blackfin: tweak howweinitialize the SPORT

[Cai, Cliff]

 

>-----Original Message-----
>From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] 
>Sent: Thursday, June 04, 2009 6:51 PM
>To: Cai, Cliff
>Cc: Mike Frysinger; uclinux-dist-devel@blackfin.uclinux.org; 
>alsa-devel@alsa-project.org
>Subject: Re: [alsa-devel] [PATCH 3/5] ASoC: Blackfin: tweak 
>howweinitialize the SPORT
>
>On Thu, Jun 04, 2009 at 10:36:48AM +0800, Cai, Cliff wrote:
>
>> This is not a problem of rate but format,I think combine the 
>two flags 
>> will solve all the problems.
>
>Ah, yes - I was remembering the ssm2602 driver which does do 
>rate constraints.
>
>> The reason I write this patch is that I found when some oss-based 
>> applications open soundcard ,the startup callbacks will be called 
>> several times.
>
>All OSS applications do this, OSS has a separate ioctl for 
>each configuration option so you get lots of configuration calls.
>
>> So using the counter to record opened pcm streams is not reliable.
>
>It's only hw_params() that get called repeatedly.
>
>I could be wrong but looking at the code I suspect you may 
>just need to make the reconfiguration conditional on if it's a 
>capture or playback stream and you'll be fine - at the minute 
>you're configuring both RX and TX on every hw_params() but 
>there are separate tcr2 and rcr2 registers that you write to.  
>If you only wrote to the appropriate one then you wouldn't 
>need to worry about interfering with another stream for that 
>configuration.  There is the wdsize parameter for the SPORT as 
>well but a brief glance at the code for that suggests that 
>there are actually separate RX and TX configurations there too.
>
>If not then adding constraint code would help a lot.

Yes,if RX and TX can be configured separately ,it would be much
easier...
But for full-duplex consideration,we now don't run RX and TX
independently,
I've tested the combining of "counter" and "configured" together to
sovle the problem
You mentioned by clearing "configured" when counter is 0.Once blackfin
tree has been
Updated to the head I will send you the patch.

Thanks a lot!

Cliff

Re: [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Karl Beldan]

On Thu, Jun 4, 2009 at 10:59 AM, Mark
Brown<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Jun 03, 2009 at 09:26:34PM -0400, Mike Frysinger wrote:
>
>> it could be.  i was going by the changes that werent in 2.6.30.  i
>> thought i looked for previous submissions, but i must have missed
>> previous attempts.  i'm not familiar with alsa stuff, so i'm not
>
> The patch hadn't previously been sent in that form but it looks like
> it's attempting to fix a bug which had had some previous goes at fixing
> it that required updates.
>
>> always sure when i need to bang on Cliff to get things fixed up
>> (changes split / better comments / etc...).
>
> If you're not sure there's a case for better changelogs at least! :)
>
>> if fixes miss a merge window, then that's an issue.  but in general,
>> users only see fixes per release, so sending things more quickly there
>> wouldnt really help ?
>
> The issue is that things are missing merge windows - they're getting
> sent in but if there's any review comments a revised version of the
> patch isn't getting posted until the next batch of submissions.  It's
> these revisions rather than the initial submissions that I'm concerned
> about.
>

This - f692fce - should not have reached upstream, how come ?

Re: [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Mike Frysinger]

On Sat, Jun 13, 2009 at 05:37, Karl Beldan wrote:
> On Thu, Jun 4, 2009 at 10:59 AM, Mark
> Brown<broonie@opensource.wolfsonmicro.com> wrote:
>> On Wed, Jun 03, 2009 at 09:26:34PM -0400, Mike Frysinger wrote:
>>
>>> it could be.  i was going by the changes that werent in 2.6.30.  i
>>> thought i looked for previous submissions, but i must have missed
>>> previous attempts.  i'm not familiar with alsa stuff, so i'm not
>>
>> The patch hadn't previously been sent in that form but it looks like
>> it's attempting to fix a bug which had had some previous goes at fixing
>> it that required updates.
>>
>>> always sure when i need to bang on Cliff to get things fixed up
>>> (changes split / better comments / etc...).
>>
>> If you're not sure there's a case for better changelogs at least! :)
>>
>>> if fixes miss a merge window, then that's an issue.  but in general,
>>> users only see fixes per release, so sending things more quickly there
>>> wouldnt really help ?
>>
>> The issue is that things are missing merge windows - they're getting
>> sent in but if there's any review comments a revised version of the
>> patch isn't getting posted until the next batch of submissions.  It's
>> these revisions rather than the initial submissions that I'm concerned
>> about.
>
> This - f692fce - should not have reached upstream, how come ?

i was going to send a revert for that and 2552a71
-mike
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Mark Brown]

On Sat, Jun 13, 2009 at 11:37:33AM +0200, Karl Beldan wrote:

> This - f692fce - should not have reached upstream, how come ?

The entire discussion of the patch occurred in response to my saying I'd
applied it, with most of the discussion being a couple of days later
after it'd have already gone into ALSA.  Since nobody asked to have it
reverted or indicated that there was any actual problem other than a
repitition of code it wasn't reverted.  For the most part repitition
in ASoC would just be a no-op and therefore a code quality rather
than behavioural issue.  As I indicated when accepting the patch I've
lowered my standards there for blackfin due to the past issues with
iteration speed (which Mike has already said will be being addressed
now).

I've now reverted the shutdown hunk of the patch where duplication will
actually introduce a bug.

Re: [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Mike Frysinger]

On Sat, Jun 13, 2009 at 06:44, Mark Brown wrote:
> On Sat, Jun 13, 2009 at 11:37:33AM +0200, Karl Beldan wrote:
>> This - f692fce - should not have reached upstream, how come ?
>
> The entire discussion of the patch occurred in response to my saying I'd
> applied it, with most of the discussion being a couple of days later
> after it'd have already gone into ALSA.  Since nobody asked to have it
> reverted or indicated that there was any actual problem other than a
> repitition of code it wasn't reverted.  For the most part repitition
> in ASoC would just be a no-op and therefore a code quality rather
> than behavioural issue.  As I indicated when accepting the patch I've
> lowered my standards there for blackfin due to the past issues with
> iteration speed (which Mike has already said will be being addressed
> now).
>
> I've now reverted the shutdown hunk of the patch where duplication will
> actually introduce a bug.

i think Cliff verified that the other part of the change was no longer
needed in his testing, so reverting the whole patch is fine.  we've
done that in the Blackfin tree.
-mike
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Mark Brown]

On Sat, Jun 13, 2009 at 06:47:17AM -0400, Mike Frysinger wrote:

> i think Cliff verified that the other part of the change was no longer
> needed in his testing, so reverting the whole patch is fine.  we've
> done that in the Blackfin tree.

So, the first part of the patch was applying constraints.  You definitely
want to do that with your current implementation - the hw_params()
function will completely ignore the parameters set for the second
substream so unless you use constraints the second application will get
a configuration it didn't ask for which isn't going to work so well.

Re: [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Mike Frysinger]

On Sat, Jun 13, 2009 at 06:59, Mark Brown wrote:
> On Sat, Jun 13, 2009 at 06:47:17AM -0400, Mike Frysinger wrote:
>> i think Cliff verified that the other part of the change was no longer
>> needed in his testing, so reverting the whole patch is fine.  we've
>> done that in the Blackfin tree.
>
> So, the first part of the patch was applying constraints.  You definitely
> want to do that with your current implementation - the hw_params()
> function will completely ignore the parameters set for the second
> substream so unless you use constraints the second application will get
> a configuration it didn't ask for which isn't going to work so well.

the first part conditionalized the constraints.  ignoring whitespace
changes, this is the diff:
+if (master_runtime->rate != 0)
    snd_pcm_hw_constraint_minmax(substream->runtime,
    ......
+if (master_runtime->sample_bits != 0)
    snd_pcm_hw_constraint_minmax(substream->runtime,
    ......

or maybe that is what you're saying and i dont know alsa
-mike
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Mark Brown]

On Sat, Jun 13, 2009 at 07:03:12AM -0400, Mike Frysinger wrote:

> the first part conditionalized the constraints.  ignoring whitespace
> changes, this is the diff:
> +if (master_runtime->rate != 0)
>     snd_pcm_hw_constraint_minmax(substream->runtime,
>     ......
> +if (master_runtime->sample_bits != 0)
>     snd_pcm_hw_constraint_minmax(substream->runtime,
>     ......

> or maybe that is what you're saying and i dont know alsa

That's not a bad change - there's a thin race condition if you
simultaneously start up two applications where the primary substream
could be there but not yet configured since stream startup is separate
to the configuration of the stream.  It's hard but not impossible to
reproduce.

Re: [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Takashi Iwai]

At Sat, 13 Jun 2009 12:06:07 +0100,
Mark Brown wrote:
> 
> On Sat, Jun 13, 2009 at 07:03:12AM -0400, Mike Frysinger wrote:
> 
> > the first part conditionalized the constraints.  ignoring whitespace
> > changes, this is the diff:
> > +if (master_runtime->rate != 0)
> >     snd_pcm_hw_constraint_minmax(substream->runtime,
> >     ......
> > +if (master_runtime->sample_bits != 0)
> >     snd_pcm_hw_constraint_minmax(substream->runtime,
> >     ......
> 
> > or maybe that is what you're saying and i dont know alsa
> 
> That's not a bad change - there's a thin race condition if you
> simultaneously start up two applications where the primary substream
> could be there but not yet configured since stream startup is separate
> to the configuration of the stream.  It's hard but not impossible to
> reproduce.

An absolute race-free hw-constraint for multiple substreams is
actually difficult to implement.  The best practice would be to give
an error and refuse the change when any inconsistency occurs, IMO.


Takashi

Re: [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down

[Cai, Cliff]

 

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai@suse.de] 
>Sent: Sunday, June 14, 2009 4:16 PM
>To: Mark Brown
>Cc: Mike Frysinger; Karl Beldan; 
>uclinux-dist-devel@blackfin.uclinux.org; 
>alsa-devel@alsa-project.org; Cai, Cliff
>Subject: Re: [alsa-devel] [Uclinux-dist-devel] [PATCH 2/5] 
>ASoC codec: SSM2602: assign last substream to the master when 
>shutting down
>
>At Sat, 13 Jun 2009 12:06:07 +0100,
>Mark Brown wrote:
>> 
>> On Sat, Jun 13, 2009 at 07:03:12AM -0400, Mike Frysinger wrote:
>> 
>> > the first part conditionalized the constraints.  ignoring 
>whitespace 
>> > changes, this is the diff:
>> > +if (master_runtime->rate != 0)
>> >     snd_pcm_hw_constraint_minmax(substream->runtime,
>> >     ......
>> > +if (master_runtime->sample_bits != 0)
>> >     snd_pcm_hw_constraint_minmax(substream->runtime,
>> >     ......
>> 
>> > or maybe that is what you're saying and i dont know alsa
>> 
>> That's not a bad change - there's a thin race condition if you 
>> simultaneously start up two applications where the primary substream 
>> could be there but not yet configured since stream startup 
>is separate 
>> to the configuration of the stream.  It's hard but not impossible to 
>> reproduce.
>
>An absolute race-free hw-constraint for multiple substreams is 
>actually difficult to implement.  The best practice would be 
>to give an error and refuse the change when any inconsistency 
>occurs, IMO.
>


Without this patch, Oss-based applications (such as "tone","vplay" etc.)
won't work properly.  

Cliff

[PATCH] ASoC: Blackfin: keep better track of SPORT configuration state

[Mike Frysinger]

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

Do not let the SPORT be reconfigured until there are no more active
streams.  Then we can let the system reprogram the SPORT state.

Signed-off-by: Cliff Cai <cliff.cai@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 sound/soc/blackfin/bf5xx-i2s.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/sound/soc/blackfin/bf5xx-i2s.c b/sound/soc/blackfin/bf5xx-i2s.c
index 9648244..0e8af48 100644
--- a/sound/soc/blackfin/bf5xx-i2s.c
+++ b/sound/soc/blackfin/bf5xx-i2s.c
@@ -50,6 +50,7 @@ struct bf5xx_i2s_port {
 	u16 tcr2;
 	u16 rcr2;
 	int counter;
+	int configured;
 };
 
 static struct bf5xx_i2s_port bf5xx_i2s;
@@ -168,7 +169,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
@@ -177,6 +178,7 @@ static int bf5xx_i2s_hw_params(struct snd_pcm_substream *substream,
 		 *
 		 * CPU DAI:slave mode.
 		 */
+		bf5xx_i2s.configured = 1;
 		ret = sport_config_rx(sport_handle, bf5xx_i2s.rcr1,
 				      bf5xx_i2s.rcr2, 0, 0);
 		if (ret) {
@@ -200,6 +202,9 @@ static void bf5xx_i2s_shutdown(struct snd_pcm_substream *substream,
 {
 	pr_debug("%s enter\n", __func__);
 	bf5xx_i2s.counter--;
+	/* No active stream, SPORT is allowed to be configured again. */
+	if (!bf5xx_i2s.counter)
+		bf5xx_i2s.configured = 0;
 }
 
 static int bf5xx_i2s_probe(struct platform_device *pdev,
-- 
1.6.3.1

Re: [PATCH] ASoC: Blackfin: keep better track of SPORT configuration state

[Mark Brown]

On Sat, Jun 20, 2009 at 11:29:05AM -0400, Mike Frysinger wrote:
> From: Cliff Cai <cliff.cai@analog.com>
> 
> Do not let the SPORT be reconfigured until there are no more active
> streams.  Then we can let the system reprogram the SPORT state.

Applied.

Please remember to CC maintainers on patches, especially if not starting
a new thread for them.