On Wed, Jun 15, 2011 at 2:13 PM, Sangbeom Kim <sbki...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html