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

[Sangbeom Kim <sbkim73@samsung.com>]

On Thu, Jun 10, 2011 at 7:08 PM, Jassi Brar wrote:
> For my convenience, could you please tell how does it differ from my
> original implementation?  Most things look same, except for a few
> variables.
Original code only can support specific buffer size and period count.
New idma driver can work with various buffer size and multiple period.
And Original code is implemented it based on wrapper arch.
But This patch can support driver arch.

> 
> > @@ -16,6 +17,7 @@ obj-$(CONFIG_SND_S3C_I2SV2_SOC) += snd-soc-s3c-i2s-
> v2.o
> >  obj-$(CONFIG_SND_SAMSUNG_SPDIF) += snd-soc-samsung-spdif.o
> >  obj-$(CONFIG_SND_SAMSUNG_PCM) += snd-soc-pcm.o
> >  obj-$(CONFIG_SND_SAMSUNG_I2S) += snd-soc-i2s.o
> > +obj-$(CONFIG_SND_SAMSUNG_I2S) += snd-soc-idma.o
> 
> Please check that building only for s3c64xx doesn't break by this.
If It have building problem, I will modify it in the next version

> 
> > +
> > +static struct idma_info {
> > +       spinlock_t      lock;
> > +       void             __iomem  *regs;
> > +       int             trigger_stat;
> The role of trigger_stat is not necessary.
trigger_stat can be used LP audio mode.
It can be used flag for checking idma operation.


> > +
> > +       /* Start address0 of I2S internal DMA operation. */
> > +       val = readl(idma.regs + I2SSTR0);
> Why read when you immediately overwrite it ?
This is useless code, It will be deleted.
 
> > +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;
> > +
> > +       pr_debug("Entered %s\n", __func__);
> can we have all the pr_debug's converted to dev_debug ?
OK, I will change it.
 
> 
> > +       snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
> > +       runtime->dma_bytes = params_buffer_bytes(params);
> > +       memset(runtime->dma_area, 0, runtime->dma_bytes);
> Is it really needed ?
It can delete useless data in dma buffer.
But It looks not need absolutely.

> > +
> > +       if (iisahb & AHB_LVL0INT)
> > +               val = AHB_CLRLVL0INT;
> > +       else
> > +               val = 0;
>   val = (iisahb & AHB_LVL0INT) ? AHB_CLRLVL0INT : 0;     looks better
OK, I will change
> > +
> > +       if (val) {
> > +               iisahb |= val;
> > +               writel(iisahb, idma.regs + I2SAHB);
> > +
> > +               addr = readl(idma.regs + I2SLVL0ADDR);
> > +               addr += prtd->periodsz;
> > +
> > +               if (addr >= prtd->end)
> > +                       addr = LP_TXBUFF_ADDR;
> This will break if ring buffer is not a multiple of period size.
> Either set the constraint in open or do modulo operation here.
OK,

> > +
> > +static int idma_close(struct snd_pcm_substream *substream)
> > +{
> > +       struct snd_pcm_runtime *runtime = substream->runtime;
> > +       struct idma_ctrl *prtd = runtime->private_data;
> > +
> > +       pr_debug("Entered %s, prtd = %p\n", __func__, prtd);
> > +
> > +       free_irq(IRQ_I2S0, prtd);
> > +
> > +       if (!prtd)
> > +               pr_err("idma_close called with prtd == NULL\n");
> > +
> > +       kfree(prtd);
> 
> Also       runtime->private_data = NULL;
OK,

> > diff --git a/sound/soc/samsung/idma.h b/sound/soc/samsung/idma.h
> > new file mode 100644
> > index 0000000..2b0ac37
> > --- /dev/null
> > +++ b/sound/soc/samsung/idma.h
> > @@ -0,0 +1,28 @@
> > +/*
> > + * idma.h  --  I2S0's Internal Dma driver
> > + *
> > + * Copyright (c) 2010 Samsung Electronics Co. Ltd
> Copyright 2011 ?
OK, I will modify it.
> 
> > +/* idma_state */
> > +#define LPAM_DMA_STOP    0
> > +#define LPAM_DMA_START   1
> These are internal to idma.c, please move them there.
OK, I will move it.

Thanks,