On Wed, Jun 15, 2011 at 2:13 PM, Sangbeom Kim <sbkim73@xxxxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
