ASoC: s3c24xx platform: Fix s3c2410_dma_started called at wrong time
Subsystems (4)
PATCH 1/1 ASoC: s3c24xx platform: Fix s3c2410_dma_started called at wrong time
s3c24xx dma has the auto reload feature, when the the trnasfer is done, CURR_TC(DSTAT[19:0], current value of transfer count) reaches 0, and DMA ACK becomes 1, and then, TC(DCON[19:0]) will be loaded into CURR_TC. So the transmission is repeated.
IRQ is issued while auto reload occurs. We change the DISRC and DCON[19:0] in the ISR, but at this time, the auto reload has been performed already. The first block is being re-transmitted by the DMA.
So we need rewrite the DISRC and DCON[19:0] for the next block immediatly after the this block has been started to be transported.
The function s3c2410_dma_started() is for this perpose, which is called in the form of "s3c2410_dma_ctrl(prtd->params->channel, S3C2410_DMAOP_STARTED);" in s3c24xx_pcm_trigger().
But it is not correct. DMA transmission won't start until DMA REQ signal arrived, it is the time s3c24xx_snd_txctrl(1) or s3c24xx_snd_rxctrl(1) is called in s3c24xx_i2s_trigger().
In the current framework, s3c24xx_pcm_trigger() is always called before s3c24xx_pcm_trigger(). So the s3c2410_dma_started() should be called in s3c24xx_pcm_trigger() after s3c24xx_snd_txctrl(1) or s3c24xx_snd_rxctrl(1) is called in this function.
However, s3c2410_dma_started() is dma related, to call this function we should provide the channel number, which is given by substream->runtime->private_data->params->channel. The private_data points to a struct s3c24xx_runtime_data object, which is define in s3c24xx_pcm.c, so s3c2410_dma_started() can't be called in s3c24xx_i2s.c
To move the struct s3c24xx_runtime_data defination to s3c24xx-pcm.h is one solution.
If not moved, the patch shold be like:
----------------------------------------------------------------
sound/soc/s3c24xx/s3c24xx-pcm.h +2 -0
@@ -28,4 +28,6 @@extern struct snd_soc_platform s3c24xx_soc_platform;extern struct snd_ac97_bus_ops s3c24xx_ac97_ops; +void s3c24xx_pcm_dma_started (struct snd_pcm_substream *substream); +#endif
sound/soc/s3c24xx/s3c24xx-pcm.c +22 -1
@@ -66,6 +66,28 @@struct s3c24xx_pcm_dma_params *params; }; +/* s3c24xx_pcm_dma_started +* +* To load the second dma buffer immediately after the +* first dma buffer has been started transferring, so that +* in the dma autoreload situation, the first data block +* won't be re-transferred after the first irq issued. +* +* This function is called in the s3c24xx-i2s-trigger() of +* s3c24xx-i2s.c, just after the dma transfer is really started. +* +* It's ugly here. Need a elegant way to fix this bug. +* FIXME +* +*/ + +void s3c24xx_pcm_dma_started (struct snd_pcm_substream *substream) +{ +struct s3c24xx_runtime_data *prtd = substream->runtime->private_data; +s3c2410_dma_ctrl (prtd->params->channel,S3C2410_DMAOP_STARTED ); + +} +/* s3c24xx_pcm_enqueue * * place a dma buffer onto the queue for the dma system
@@ -255,7 +277,6 @@case SNDRV_PCM_TRIGGER_PAUSE_RELEASE : prtd->state |=ST_RUNNING ;s3c2410_dma_ctrl (prtd->params->channel,S3C2410_DMAOP_START ); -s3c2410_dma_ctrl (prtd->params->channel,S3C2410_DMAOP_STARTED );break ;case SNDRV_PCM_TRIGGER_STOP :
sound/soc/s3c24xx/s3c24xx-i2s.c +3 -0
@@ -296,6 +296,9 @@s3c24xx_snd_rxctrl (1 ); else s3c24xx_snd_txctrl(1); + +/* DMA transfer is really started, equeue the next buffer */ +s3c24xx_pcm_dma_started (substream);break ;case SNDRV_PCM_TRIGGER_STOP :case SNDRV_PCM_TRIGGER_SUSPEND :
Quoted 4 lines · Shine Liu
In the current framework,
s3c24xx_pcm_trigger()is always called befores3c24xx_pcm_trigger(). So thes3c2410_dma_started()should be called ins3c24xx_pcm_trigger()afters3c24xx_snd_txctrl(1)ors3c24xx_snd_rxctrl(1)is called in this function.
I suspect some of the function names in your description here are incorrect :) Another option is to provide a callback in the private data passed to the DMA driver which the DMA driver can call at the appropriate point during setup.
Someone will presumably also need to take care of the same things in the other S3C DAI drivers too, though it should be possible to arrange things so that can be done seperately.
Quoted 11 lines · Mark Brown
Quoted 4 lines · Shine Liu
In the current framework,
s3c24xx_pcm_trigger()is always called befores3c24xx_pcm_trigger(). So thes3c2410_dma_started()should be called ins3c24xx_pcm_trigger()afters3c24xx_snd_txctrl(1)ors3c24xx_snd_rxctrl(1)is called in this function.I suspect some of the function names in your description here are incorrect :) Another option is to provide a callback in the private data passed to the DMA driver which the DMA driver can call at the appropriate point during setup.
Sorry for the mistake. Should be s3c24xx_pcm_trigger() is always called before s3c24xx_i2s_trigger() in current framework :s3c24xx_pcm_trigger() is called via platform->pcm_ops->trigger and s3c24xx_i2s_trigger() is called via cpu_dai->ops->trigger.
The problem is that DMA driver doesn't know when the DMA transfer has been really started. The DMA setup is done in s3c24xx_pcm_trigger(), in which all DMA related registers are set. But it doesn't mean the DMA transfer is started at this point because the DMA REQ signal has not arrived which is generated by the IIS. DMA driver has no way to know when the DMA REQ signal arrived. So the s3c24xx DMA driver provids a s3c2410_dma_started() method to let the user call when he knows the DMA transfer has been started.
Quoted 4 lines
Someone will presumably also need to take care of the same things in the other S3C DAI drivers too, though it should be possible to arrange things so that can be done seperately.
Quoted 10 lines · Shine Liu
Quoted 3 lines · Mark Brown
incorrect :) Another option is to provide a callback in the private data passed to the DMA driver which the DMA driver can call at the appropriate point during setup.
arrived which is generated by the IIS. DMA driver has no way to know when the DMA REQ signal arrived. So the s3c24xx DMA driver provids a
s3c2410_dma_started()method to let the user call when he knows the DMA transfer has been started.
Yes, I understand the problem. The callback I suggest above ought to allow the appropriate sequencing - the DMA driver can call the callback to start the DAI in between starting the DMA and kicking the DMA API to queue the next block. That would avoid the abstraction problems.
Quoted 6 lines · Mark Brown
Yes, I understand the problem. The callback I suggest above ought to allow the appropriate sequencing - the DMA driver can call the callback to start the DAI in between starting the DMA and kicking the DMA API to queue the next block. That would avoid the abstraction problems.
Callback is a elegant way to solve the problem. But I have a question that when should the DMA driver call the callback funtion? There is no event to tell it to call. Periodly checking? It will not only cause latency but also increase the system load.
Thanks.
Quoted 4 lines · Shine Liu
Callback is a elegant way to solve the problem. But I have a question that when should the DMA driver call the callback funtion? There is no event to tell it to call. Periodly checking? It will not only cause latency but also increase the system load.
The callback doesn't need to be set up within the trigger function - if it's done before the trigger functions are called then the DMA trigger can call it. Off the top of my head I'd expect it's possible to set it up when the device is opened.
Quoted 5 lines
The callback doesn't need to be set up within the trigger function - if it's done before the trigger functions are called then the DMA trigger can call it. Off the top of my head I'd expect it's possible to set it up when the device is opened.
Who call the DMA trigger? When? In any of the platform/cpu_dai trigger functions?
Quoted 7 lines · Shine Liu
Quoted 4 lines
The callback doesn't need to be set up within the trigger function - if it's done before the trigger functions are called then the DMA trigger can call it. Off the top of my head I'd expect it's possible to set it up when the device is opened.
Who call the DMA trigger? When? In any of the platform/cpu_dai trigger functions?
It's called ultimately by user space; within ASoC it's always called by the core trigger function in the fixed order that it has. The open() callback would be a safe place to do the setup - that's called before anything else.
Quoted 8 lines
It's called ultimately by user space; within ASoC it's always called by the core trigger function in the fixed order that it has. The
open()callback would be a safe place to do the setup - that's called before anything else.
I've finished a draft patch in the callback manner. Waiting for any suggestion. Thanks.
-----------------------------------------------------------------
arch/arm/plat-s3c24xx/include/plat/dma-plat.h +21 -0
@@ -76,6 +76,27 @@extern int s3c24xx_dma_order_set (struct s3c24xx_dma_order *map); +enum s3c24xx_dma_channel_event { +CHANNEL_EVENT_START , +CHANNEL_EVENT_STOP , +CHANNEL_EVENT_MAX , +}; + +struct s3c24xx_dma_channel_trigger { +unsigned int channel; +/* trigger event */ +enum s3c24xx_dma_channel_event event; +/* pravate date to the callback function */ +void *private_data; +/* callback funtion to generate the DMA REQ signal */ +void (*gen_request)(void *private_data); +/* callback funtion to end the DMA request */ +void (*end_request)(void *private_data); +}; + +/* trigger the DMA engine to start, called from the client */ +extern void s3c24xx_dma_trigger (struct s3c24xx_dma_channel_trigger *client); +/* DMA init code, called from the cpu support code */ extern int s3c2410_dma_init (void );
arch/arm/plat-s3c24xx/dma.c +28 -0
@@ -1008,6 +1008,34 @@EXPORT_SYMBOL (s3c2410_dma_ctrl); +void s3c24xx_dma_trigger (struct s3c24xx_dma_channel_trigger *client) +{ +if (!client || !(client->channel <DMACH_MAX )) { +pr_debug ("%s: Invalid parameter\n" , __func__); +return ; + } + +switch (client->event) { +case CHANNEL_EVENT_START : +s3c2410_dma_ctrl (client->channel,S3C2410_DMAOP_START ); +if (client->gen_request) + client->gen_request (client->private_data); +s3c2410_dma_ctrl (client->channel,S3C2410_DMAOP_STARTED ); +return ; +case CHANNEL_EVENT_STOP : +s3c2410_dma_ctrl (client->channel,S3C2410_DMAOP_STOP ); +if (client->end_request) + client->end_request (client->private_data); +/* should be set to a vaild value in the next request */ + client->channel =DMACH_MAX ; +return ; +default : +pr_debug ("%s: Invalid event\n" , __func__); + } +} + +EXPORT_SYMBOL (s3c24xx_dma_trigger); +/* DMA configuration for each channel * * DISRCC -> source of the DMA (AHB,APB)
sound/soc/s3c24xx/s3c24xx-i2s.c +15 -5
@@ -38,6 +38,7 @@#include <plat/regs-iis.h> +#define __USE_S3C24XX_PCM_RUNTIME#include "s3c24xx-pcm.h" #include "s3c24xx-i2s.h"
@@ -278,6 +279,7 @@static int s3c24xx_i2s_trigger (struct snd_pcm_substream *substream,int cmd,struct snd_soc_dai *dai) { +struct s3c24xx_pcm_runtime_data *prtd = substream->runtime->private_data;int ret =0 ;pr_debug ("Entered %s\n" , __func__);
@@ -292,24 +294,32 @@ goto exit_err; } +spin_lock (&prtd->lock); + prtd->client.private_data= (void *)1 ;if (substream->stream ==SNDRV_PCM_STREAM_CAPTURE ) -s3c24xx_snd_rxctrl (1 ); + prtd->client.gen_request = (void *)s3c24xx_snd_rxctrl;else -s3c24xx_snd_txctrl (1 ); + prtd->client.gen_request = (void *)s3c24xx_snd_txctrl; +spin_unlock (&prtd->lock);break ;case SNDRV_PCM_TRIGGER_STOP :case SNDRV_PCM_TRIGGER_SUSPEND :case SNDRV_PCM_TRIGGER_PAUSE_PUSH : +spin_lock (&prtd->lock); + prtd->client.private_data= (void *)0 ;if (substream->stream ==SNDRV_PCM_STREAM_CAPTURE ) -s3c24xx_snd_rxctrl (0 ); + prtd->client.end_request = (void *)s3c24xx_snd_rxctrl;else -s3c24xx_snd_txctrl (0 ); + prtd->client.end_request = (void *)s3c24xx_snd_txctrl; +spin_unlock (&prtd->lock);break ;default : ret = -EINVAL ; -break ; + goto exit_err; } +s3c24xx_dma_trigger (&prtd->client); + exit_err:return ret; }
sound/soc/s3c24xx/s3c24xx-pcm.h +19 -0
@@ -22,6 +22,25 @@int dma_size;/* Size of the DMA transfer */ }; +#ifdef __USE_S3C24XX_PCM_RUNTIME + +#include <plat/dma-plat.h> + +struct s3c24xx_pcm_runtime_data { +spinlock_t lock; +int state; +unsigned int dma_loaded; +unsigned int dma_limit; +unsigned int dma_period; +dma_addr_t dma_start; +dma_addr_t dma_pos; +dma_addr_t dma_end; +struct s3c24xx_pcm_dma_params *params; +struct s3c24xx_dma_channel_trigger client; +}; + +#endif +#define S3C24XX_DAI_I2S 0/* platform data */
sound/soc/s3c24xx/s3c24xx-pcm.c +16 -25
126 lines not on this page · raw message
Quoted 2 lines · Shine Liu
I've finished a draft patch in the callback manner. Waiting for any suggestion. Thanks.
This isn't quite what I was expecting - what I was expecting was more that the I2S driver would set up a function on startup which would be called by the ASoC DMA driver from its trigger function. It does look like a reasonable approach, though.
The way you've got things at the minute at least all the other S3C ASoC drivers will need to be updated to match the change in the DMA driver.
Quoted 6 lines
This isn't quite what I was expecting - what I was expecting was more that the I2S driver would set up a function on startup which would be called by the ASoC DMA driver from its trigger function. It does look like a reasonable approach, though.
Yes, I understand what you exactly want. I was intend to register the callback function on I2S startup, but finally I found it is not possible because there are two callback functions for the DMA driver:s3c24xx_snd_txctrl() and s3c24xx_snd_rxctrl(), however, there's only one slot for the callback function. So I choose not to setup the callback functions dynamicly, when the task is playback, the callback funtion is s3c24xx_snd_txctrl() and when the task is record, the funtion is s3c24xx_snd_rxctrl().
However, there's another way to archive what you expect: combine the s3c24xx_snd_txctrl() and s3c24xx_snd_rxctrl() into one function. That would be like:
struct s3c24xx_snd_ctrl_data {
int event; //start or stop
int direction; //playback or record
};
static void s3c24xx_snd_ctrl(struct s3c24xx_snd_ctrl_data* data)
{
if(data->direction == SNDRV_PCM_STREAM_PLAYBACK)
s3c24xx_snd_txctrl(data->event);
eles
s3c24xx_snd_rxctrl(data->event);
}Then we can register this callback funtion on I2S startup.
Quoted 4 lines
The way you've got things at the minute at least all the other S3C ASoC drivers will need to be updated to match the change in the DMA driver.
I have posted the DMA driver patch to linux-arm-kernel yesterday:
http://lists.arm.linux.org.uk/lurker/message/20090822.153940.e15e3bd7.en.html
Hope there is no problem to merge the patch.
If the dma patch is accepted, I will post a formal patch here for ASoC S3C24XX.
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 5 lines
...but there's no need for the struct, just pass two arguments to the function, the event and either the substream or the direction. The substream is probably easier since otherwise all the callers will need to parse the direction.
Finally, I decide to place the callback function in struct s3c2410_dma_client instead of add a new struct:
struct s3c2410_dma_client {
....
/* callback funtion from client to generate the DMA REQ signal */
int (*gen_request)(void *);
/* callback funtion from client to end the DMA request */
int (*end_request)(void *);
/* argument for the callback function */
void *private_data;};
I think the callback function should have only one argument. If we have more than one argument to pass, just embbed them into a internal struct known to the callback funtions and pass the pointer to the object of this struct to the callback function.
If not, more filed will be needed in struct s3c2410_dma_client to place the arguments. I think it's redundant for the generic DMA driver.
I've posted five new patches which regist the callback functions either in the I2S/AC97 probe phase or by directly assiged in the source code depended by whether the callback functions and the object of struct s3c2410_dma_client are in the same source file.
Look forward for further comments on these patches. There should be some defects in them.
Thanks.
Quoted 2 lines · Shine Liu
If not, more filed will be needed in struct
s3c2410_dma_clientto place the arguments. I think it's redundant for the generic DMA driver.
But why are you changing the core DMA code in the first place? The ASoC drivers have split front end and DMA code but this is not the normal case for a DMA client.
Quoted 4 lines
But why are you changing the core DMA code in the first place? The ASoC drivers have split front end and DMA code but this is not the normal case for a DMA client.
I think your idea of the callback is very elegant and I appreciate this way to solve the problem so I try to add new API for the core DMA to achive this goal.
Well, if there are two arguments for the callback function passed though struct struct s3c2410_dma_client, it's also reasonable. If there are more arguments to pass, maybe this is not the best solution. Use a internal struct to wrapper more arguments into one object and pass the pointer is just a candidate.
PATCH 1/1 ASoC: S3C platform: Fix s3c2410_dma_started() called at improper time
In reply to Mark BrownHi Mark,
There's a candidate patch for the bug of s3c2410_dma_started()
called at improper time. This patch also covers all S3C I2C/AC97 ASoC drivers affected by this bug.
The previous patches try to fix this bug in the DMA callback way. It
's elegant and should be the proper way to fix the bug but the changes is a little large and need be well discussed and tested.
I think before we fix the bug in the callback way, we can use this
patch provisionally for a while though it seems a little ugly.
This patch is created against linux-2.6.31-rc7, and has been well
tested under s3c2440 platform with I2S DAI.
Best Regards,
Shine
----------------------------------------------------
sound/soc/s3c24xx/s3c24xx-pcm.c +0 -1
@@ -255,7 +255,6 @@case SNDRV_PCM_TRIGGER_PAUSE_RELEASE : prtd->state |=ST_RUNNING ;s3c2410_dma_ctrl (prtd->params->channel,S3C2410_DMAOP_START ); -s3c2410_dma_ctrl (prtd->params->channel,S3C2410_DMAOP_STARTED );break ;case SNDRV_PCM_TRIGGER_STOP :
sound/soc/s3c24xx/s3c24xx-i2s.c +12 -0
@@ -279,9 +279,14 @@ struct snd_soc_dai *dai) {int ret =0 ; +int channel; +struct snd_soc_pcm_runtime *rtd = substream->private_data;pr_debug ("Entered %s\n" , __func__); + channel = ((struct s3c24xx_pcm_dma_params *) + rtd->dai->cpu_dai->dma_data)->channel; +switch (cmd) {case SNDRV_PCM_TRIGGER_START :case SNDRV_PCM_TRIGGER_RESUME :
@@ -296,6 +301,13 @@s3c24xx_snd_rxctrl (1 ); else s3c24xx_snd_txctrl(1); +/* DMA engine has just been started at this point, +* load the next buffer to DMA to meet the reqirement +* of the auto reload mechanism. +* Ugly here, but it is the simplest way. +* Will be fixed if DMA callback API provided. +*/ +s3c2410_dma_ctrl (channel,S3C2410_DMAOP_STARTED );break ;case SNDRV_PCM_TRIGGER_STOP :case SNDRV_PCM_TRIGGER_SUSPEND :
sound/soc/s3c24xx/s3c2443-ac97.c +22 -0
@@ -290,6 +290,9 @@ struct snd_soc_dai *dai) {u32 ac_glbctrl; +struct snd_soc_pcm_runtime *rtd = substream->private_data; +int channel = ((struct s3c24xx_pcm_dma_params *) + rtd->dai->cpu_dai->dma_data)->channel; ac_glbctrl =readl (s3c24xx_ac97.regs +S3C_AC97_GLBCTRL );switch (cmd) {
@@ -312,6 +315,14 @@ }writel (ac_glbctrl, s3c24xx_ac97.regs +S3C_AC97_GLBCTRL ); +/* DMA engine has just been started at this point, +* load the next buffer to DMA to meet the reqirement +* of the auto reload mechanism. +* Ugly here, but it is the simplest way. +* Will be fixed if DMA callback API provided. +*/ +s3c2410_dma_ctrl (channel,S3C2410_DMAOP_STARTED ); +return 0 ; }
@@ -334,6 +345,9 @@ int cmd, struct snd_soc_dai *dai) { u32 ac_glbctrl; +struct snd_soc_pcm_runtime *rtd = substream->private_data; +int channel = ((struct s3c24xx_pcm_dma_params *) + rtd->dai->cpu_dai->dma_data)->channel; ac_glbctrl =readl (s3c24xx_ac97.regs +S3C_AC97_GLBCTRL );switch (cmd) {
@@ -349,6 +363,14 @@ }writel (ac_glbctrl, s3c24xx_ac97.regs +S3C_AC97_GLBCTRL ); +/* DMA engine has just been started at this point, +* load the next buffer to DMA to meet the reqirement +* of the auto reload mechanism. +* Ugly here, but it is the simplest way. +* Will be fixed if DMA callback API provided. +*/ +s3c2410_dma_ctrl (channel,S3C2410_DMAOP_STARTED ); +return 0 ; }
sound/soc/s3c24xx/s3c-i2s-v2.c +14 -0
@@ -387,6 +387,8 @@int capture = (substream->stream ==SNDRV_PCM_STREAM_CAPTURE );unsigned long irqs;int ret =0 ; +int channel = ((struct s3c24xx_pcm_dma_params *) + rtd->dai->cpu_dai->dma_data)->channel;pr_debug ("Entered %s\n" , __func__);
@@ -416,6 +418,18 @@s3c2412_snd_txctrl (i2s,1 );local_irq_restore (irqs); + +/* DMA engine has just been started at this point, +* load the next buffer to DMA to meet the reqirement +* of the auto reload mechanism. +* Ugly here, but it is the simplest way. +* Will be fixed if DMA callback API provided. +* +* S3C64XX doesn't have the auto reload problem, +* only for S3C24XX. This call won't bother S3C64XX. +*/ +s3c2410_dma_ctrl (channel,S3C2410_DMAOP_STARTED ); +break ;case SNDRV_PCM_TRIGGER_STOP :
Re: [PATCH] ASoC: S3C platform: Fix s3c2410_dma_started() called at improper time
Quoted 2 lines · Shine Liu
I think before we fix the bug in the callback way, we can use this
patch provisionally for a while though it seems a little ugly.
I can't prevail on you to do this with a callback within the ASoC code only? This really does seem like something that's created by the way ASoC is structured rather than a general S3C DMA issue so I'm not sure that the arch/arm code needs to be changed at all.
Otherwise the patch looks OK, in fact I'd tone down the comments about it being a hack to do this - ASoC DAI and PCM drivers are supposed to be fairly closely tied to each other.
Re: [PATCH] ASoC: S3C platform: Fix s3c2410_dma_started() called at improper time
Quoted 5 lines
Otherwise the patch looks OK, in fact I'd tone down the comments about it being a hack to do this - ASoC DAI and PCM drivers are supposed to be fairly closely tied to each other.
Yes, I agree with you. I have cut down the unnecessary comments in the patch. Thank you for your appropriate suggestion.
Shine
sound/soc/s3c24xx/s3c24xx-pcm.c +0 -1
@@ -255,7 +255,6 @@case SNDRV_PCM_TRIGGER_PAUSE_RELEASE : prtd->state |=ST_RUNNING ;s3c2410_dma_ctrl (prtd->params->channel,S3C2410_DMAOP_START ); -s3c2410_dma_ctrl (prtd->params->channel,S3C2410_DMAOP_STARTED );break ;case SNDRV_PCM_TRIGGER_STOP :
sound/soc/s3c24xx/s3c2443-ac97.c +10 -0
@@ -290,6 +290,9 @@ struct snd_soc_dai *dai) {u32 ac_glbctrl; +struct snd_soc_pcm_runtime *rtd = substream->private_data; +int channel = ((struct s3c24xx_pcm_dma_params *) + rtd->dai->cpu_dai->dma_data)->channel; ac_glbctrl =readl (s3c24xx_ac97.regs +S3C_AC97_GLBCTRL );switch (cmd) {
@@ -312,6 +315,8 @@ }writel (ac_glbctrl, s3c24xx_ac97.regs +S3C_AC97_GLBCTRL ); +s3c2410_dma_ctrl (channel,S3C2410_DMAOP_STARTED ); +return 0 ; }
@@ -334,6 +339,9 @@ int cmd, struct snd_soc_dai *dai) { u32 ac_glbctrl; +struct snd_soc_pcm_runtime *rtd = substream->private_data; +int channel = ((struct s3c24xx_pcm_dma_params *) + rtd->dai->cpu_dai->dma_data)->channel; ac_glbctrl =readl (s3c24xx_ac97.regs +S3C_AC97_GLBCTRL );switch (cmd) {
@@ -349,6 +357,8 @@ }writel (ac_glbctrl, s3c24xx_ac97.regs +S3C_AC97_GLBCTRL ); +s3c2410_dma_ctrl (channel,S3C2410_DMAOP_STARTED ); +return 0 ; }
sound/soc/s3c24xx/s3c24xx-i2s.c +5 -0
@@ -279,6 +279,9 @@ struct snd_soc_dai *dai) {int ret =0 ; +struct snd_soc_pcm_runtime *rtd = substream->private_data; +int channel = ((struct s3c24xx_pcm_dma_params *) + rtd->dai->cpu_dai->dma_data)->channel;pr_debug ("Entered %s\n" , __func__);
@@ -296,6 +299,8 @@s3c24xx_snd_rxctrl (1 ); else s3c24xx_snd_txctrl(1); + +s3c2410_dma_ctrl (channel,S3C2410_DMAOP_STARTED );break ;case SNDRV_PCM_TRIGGER_STOP :case SNDRV_PCM_TRIGGER_SUSPEND :
sound/soc/s3c24xx/s3c-i2s-v2.c +10 -0
@@ -387,6 +387,8 @@int capture = (substream->stream ==SNDRV_PCM_STREAM_CAPTURE );unsigned long irqs;int ret =0 ; +int channel = ((struct s3c24xx_pcm_dma_params *) + rtd->dai->cpu_dai->dma_data)->channel;pr_debug ("Entered %s\n" , __func__);
@@ -416,6 +418,14 @@s3c2412_snd_txctrl (i2s,1 );local_irq_restore (irqs); + +/* +* Load the next buffer to DMA to meet the reqirement +* of the auto reload mechanism of S3C24XX. +* This call won't bother S3C64XX. +*/ +s3c2410_dma_ctrl (channel,S3C2410_DMAOP_STARTED ); +break ;case SNDRV_PCM_TRIGGER_STOP :
Re: [PATCH] ASoC: S3C platform: Fix s3c2410_dma_started() called at improper time
Quoted 2 lines · Shine Liu
Yes, I agree with you. I have cut down the unnecessary comments in the patch. Thank you for your appropriate suggestion.
I've applied this, taking the commit message from your original e-mail with a slight update to reflect the final solution. Thanks for all your patient work on this.