[alsa-devel] [PATCH 0/5] Blackfin ASoC fixing and updates
Hi Mark,
We have some patches for Blackfin ASoC updates. Please take a look.
Thanks a lot -Bryan
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, },
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.
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.
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?
-----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
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; }
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 ?
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?
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?
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.
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);
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.
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); }
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.
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
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)
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.
participants (4)
-
Bryan Wu -
Cai, Cliff -
Karl Beldan -
Mark Brown