On Wed, Jun 15, 2011 at 2:13 PM, Sangbeom Kim sbkim73@samsung.com wrote:
- .channels_min = 2,
- .channels_max = 2,
- .buffer_bytes_max = MAX_IDMA_BUFFER,
- .period_bytes_min = 128,
- .period_bytes_max = MAX_IDMA_PERIOD,
- .periods_min = 1,
- .periods_max = 2,
+};
The settings here don't ensure ring buffer is always MAX_IDMA_BUFFER. Whereas you assume that in 'iis_irq'.
+void i2sdma_getpos(dma_addr_t *src, struct snd_pcm_substream *substream) +{
- struct idma_ctrl *prtd = substream->runtime->private_data;
- if (prtd && (prtd->state & ST_RUNNING))
- *src = idma.lp_tx_addr +
- (readl(idma.regs + I2STRNCNT) & 0xffffff) * 4;
- else
- *src = idma.lp_tx_addr;
+}
Why do we need this function ?
+static int idma_hw_params(struct snd_pcm_substream *substream,
- struct snd_pcm_hw_params *params)
+{
- struct snd_pcm_runtime *runtime = substream->runtime;
- struct idma_ctrl *prtd = substream->runtime->private_data;
- dev_dbg(dev, "Entered %s\n", __func__);
- snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
- runtime->dma_bytes = params_buffer_bytes(params);
- prtd->start = prtd->pos = runtime->dma_addr;
- prtd->period = params_periods(params);
- prtd->periodsz = params_period_bytes(params);
- prtd->end = idma.lp_tx_addr + runtime->dma_bytes;
prtd->end = runtime->dma_addr + runtime->dma_bytes; makes better sense
+static snd_pcm_uframes_t
- idma_pointer(struct snd_pcm_substream *substream)
+{
- struct snd_pcm_runtime *runtime = substream->runtime;
- struct idma_ctrl *prtd = runtime->private_data;
- dma_addr_t src;
- unsigned long res;
- dev_dbg(dev, "Entered %s\n", __func__);
- spin_lock(&prtd->lock);
- idma_getpos(&src);
- res = src - prtd->start;
- spin_unlock(&prtd->lock);
- dev_dbg(dev, "Pointer %x \n", src);
- if (res >= snd_pcm_lib_buffer_bytes(substream)) {
- if (res == snd_pcm_lib_buffer_bytes(substream))
This second check is redundant.
+static irqreturn_t iis_irq(int irqno, void *dev_id) +{ + struct idma_ctrl *prtd = (struct idma_ctrl *)dev_id; + u32 iiscon, iisahb, val, addr; + + iisahb = readl(idma.regs + I2SAHB); + iiscon = readl(idma.regs + I2SCON); + + val = (iisahb & AHB_LVL0INT) ? AHB_CLRLVL0INT : 0; + + if (val) { + iisahb |= val; + writel(iisahb, idma.regs + I2SAHB); + + addr = readl(idma.regs + I2SLVL0ADDR) - idma.lp_tx_addr; + addr += prtd->period; + addr %= MAX_IDMA_BUFFER; + addr += idma.lp_tx_addr; + + writel(addr, idma.regs + I2SLVL0ADDR); + + if (prtd->cb) { + if (iisahb & AHB_LVL0INT) iisahb will always have the AHB_LVL0INT bit set at this point.
+void idma_reg_init(void *regs) +{
- spin_lock_init(&idma.lock);
- idma.regs = regs;
+}
Why initialize lock twice, here ...
+void idma_addr_init(dma_addr_t addr) +{
- spin_lock_init(&idma.lock);
- idma.lp_tx_addr = addr;
+}
... and here. And we might have races here - there is no mechanism to ensure these functions are called before idma.lp_tx_addr and idma.regs are used. Also, please try to merge them into one function.
+MODULE_DESCRIPTION("Samsung ASoC IDMA Driver"); +MODULE_LICENSE("GPL");
The MODULE_AUTHOR field is missing here.
Thnx, -j