[PATCH 14/15] ASoC: Samsung: Update DMA interface

On Wed, Jul 27, 2011 at 11:01 AM, Boojin Kim [off-list ref] wrote:

?static void dma_enqueue(struct snd_pcm_substream *substream)
?{
? ? ? ?struct runtime_data *prtd = substream->runtime->private_data;
? ? ? ?dma_addr_t pos = prtd->dma_pos;
? ? ? ?unsigned int limit;
- ? ? ? int ret;
+ ? ? ? struct samsung_dma_prep_info dma_info;

? ? ? ?pr_debug("Entered %s\n", __func__);

- ? ? ? if (s3c_dma_has_circular())
- ? ? ? ? ? ? ? limit = (prtd->dma_end - prtd->dma_start) / prtd->dma_period;
- ? ? ? else
- ? ? ? ? ? ? ? limit = prtd->dma_limit;
+ ? ? ? limit = (prtd->dma_end - prtd->dma_start) / prtd->dma_period;

'dma_limit' is rendered useless, so you might want to remove it from
'struct runtime_data'
as well.

? ? ? ?pr_debug("%s: loaded %d, limit %d\n",
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?__func__, prtd->dma_loaded, limit);

- ? ? ? while (prtd->dma_loaded < limit) {
- ? ? ? ? ? ? ? unsigned long len = prtd->dma_period;
+ ? ? ? dma_info.cap = (samsung_dma_has_circular() ? DMA_CYCLIC : DMA_SLAVE);
+ ? ? ? dma_info.direction =
+ ? ? ? ? ? ? ? (substream->stream == SNDRV_PCM_STREAM_PLAYBACK
+ ? ? ? ? ? ? ? ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
+ ? ? ? dma_info.fp = audio_buffdone;
+ ? ? ? dma_info.fp_param = substream;
+ ? ? ? dma_info.period = prtd->dma_period;
+ ? ? ? dma_info.len = prtd->dma_period*limit;

+ ? ? ? while (prtd->dma_loaded < limit) {
? ? ? ? ? ? ? ?pr_debug("dma_loaded: %d\n", prtd->dma_loaded);

- ? ? ? ? ? ? ? if ((pos + len) > prtd->dma_end) {
- ? ? ? ? ? ? ? ? ? ? ? len ?= prtd->dma_end - pos;
- ? ? ? ? ? ? ? ? ? ? ? pr_debug("%s: corrected dma len %ld\n", __func__, len);
+ ? ? ? ? ? ? ? if ((pos + dma_info.period) > prtd->dma_end) {
+ ? ? ? ? ? ? ? ? ? ? ? dma_info.period ?= prtd->dma_end - pos;
+ ? ? ? ? ? ? ? ? ? ? ? pr_debug("%s: corrected dma len %ld\n",
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? __func__, dma_info.period);
? ? ? ? ? ? ? ?}

- ? ? ? ? ? ? ? ret = s3c2410_dma_enqueue(prtd->params->channel,
- ? ? ? ? ? ? ? ? ? ? ? substream, pos, len);
+ ? ? ? ? ? ? ? dma_info.buf = pos;
+ ? ? ? ? ? ? ? prtd->params->ops->prepare(prtd->params->ch, &dma_info);

- ? ? ? ? ? ? ? if (ret == 0) {
- ? ? ? ? ? ? ? ? ? ? ? prtd->dma_loaded++;
- ? ? ? ? ? ? ? ? ? ? ? pos += prtd->dma_period;
- ? ? ? ? ? ? ? ? ? ? ? if (pos >= prtd->dma_end)
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pos = prtd->dma_start;
- ? ? ? ? ? ? ? } else
- ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? ? ? ? ? prtd->dma_loaded++;
+ ? ? ? ? ? ? ? pos += prtd->dma_period;
+ ? ? ? ? ? ? ? if (pos >= prtd->dma_end)
+ ? ? ? ? ? ? ? ? ? ? ? pos = prtd->dma_start;
? ? ? ?}

? ? ? ?prtd->dma_pos = pos;
?}

-static void audio_buffdone(struct s3c2410_dma_chan *channel,
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? void *dev_id, int size,
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? enum s3c2410_dma_buffresult result)
+static void audio_buffdone(void *data)
?{
- ? ? ? struct snd_pcm_substream *substream = dev_id;
- ? ? ? struct runtime_data *prtd;
+ ? ? ? struct snd_pcm_substream *substream = data;
+ ? ? ? struct runtime_data *prtd = substream->runtime->private_data;

? ? ? ?pr_debug("Entered %s\n", __func__);

- ? ? ? if (result == S3C2410_RES_ABORT || result == S3C2410_RES_ERR)
- ? ? ? ? ? ? ? return;
-
- ? ? ? prtd = substream->runtime->private_data;
+ ? ? ? if (prtd->state & ST_RUNNING) {
+ ? ? ? ? ? ? ? prtd->dma_pos += prtd->dma_period;
+ ? ? ? ? ? ? ? if (prtd->dma_pos >= prtd->dma_end)
+ ? ? ? ? ? ? ? ? ? ? ? prtd->dma_pos = prtd->dma_start;

- ? ? ? if (substream)
- ? ? ? ? ? ? ? snd_pcm_period_elapsed(substream);
+ ? ? ? ? ? ? ? if (substream)
+ ? ? ? ? ? ? ? ? ? ? ? snd_pcm_period_elapsed(substream);

- ? ? ? spin_lock(&prtd->lock);
- ? ? ? if (prtd->state & ST_RUNNING && !s3c_dma_has_circular()) {
- ? ? ? ? ? ? ? prtd->dma_loaded--;
- ? ? ? ? ? ? ? dma_enqueue(substream);
+ ? ? ? ? ? ? ? spin_lock(&prtd->lock);
+ ? ? ? ? ? ? ? if (!samsung_dma_has_circular()) {
+ ? ? ? ? ? ? ? ? ? ? ? prtd->dma_loaded--;
+ ? ? ? ? ? ? ? ? ? ? ? dma_enqueue(substream);
+ ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? spin_unlock(&prtd->lock);
? ? ? ?}
-
- ? ? ? spin_unlock(&prtd->lock);
?}

Since we set integer-constraint on number of periods, you could also
discard bothering fractional boundaries. That would make things a lot
simpler.

I wish you agreed and used
       prtd->params->ops->cmd(ch, enum some_cmd_options)
rather than having 4 separate callbacks
      prtd->params->ops->trigger(prtd->params->ch)
      prtd->params->ops->stop(prtd->params->ch)
     prtd->params->ops->flush(prtd->params->ch)
     prtd->params->ops->started(prtd->params->ch)

Isn't this a regression ?
dma_pointer() doesn't really return actual location of DMA activity anymore.
Now it simply tells the last period done.
This would affect latencies in a bad way.