[alsa-devel] [PATCH V2 2/4] ASoC: SAMSUNG: Add I2S0 internal dma driver

[Jassi Brar]

On Wed, Jun 15, 2011 at 2:13 PM, Sangbeom Kim <sbkim73 at 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