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

[Jassi Brar]

On Mon, Jun 20, 2011 at 1:06 PM, Sangbeom Kim <sbkim73 at samsung.com> wrote:

> +
> +static struct device *dev;
Dear there is more to the first argument of dev_dbg, than to silence
the compiler.
Please extract _real_ device underneath samsung_idma, rather than
using this placeholder.


> +static const struct snd_pcm_hardware idma_hardware = {
> +       .info = SNDRV_PCM_INFO_INTERLEAVED |
> +                   SNDRV_PCM_INFO_BLOCK_TRANSFER |
> +                   SNDRV_PCM_INFO_MMAP |
> +                   SNDRV_PCM_INFO_MMAP_VALID |
> +                   SNDRV_PCM_INFO_PAUSE |
> +                   SNDRV_PCM_INFO_RESUME,
> +       .formats = SNDRV_PCM_FMTBIT_S16_LE |
> +                   SNDRV_PCM_FMTBIT_U16_LE |
> +                   SNDRV_PCM_FMTBIT_S24_LE |
> +                   SNDRV_PCM_FMTBIT_U24_LE |
> +                   SNDRV_PCM_FMTBIT_U8 |
> +                   SNDRV_PCM_FMTBIT_S8,
> +       .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,
How about ouput of aply -v sample.wav ?
Most probably ALSA uses 2 periods of MAX_IDMA_BUFFER/2 bytes each.
Is that what you expect ?  I would want ALSA to always use 1 period
of  MAX_IDMA_PERIOD bytes and ring-buffer of MAX_IDMA_BUFFER bytes.


> +struct idma_ctrl {
> +       spinlock_t      lock;
> +       int             state;
> +       dma_addr_t      start;
> +       dma_addr_t      pos;
> +       dma_addr_t      end;
This 'end' member is useless. Perhaps ringbuffer 'size' will be more useful.

> +       dma_addr_t      period;
> +       dma_addr_t      periodsz;
> +       void            *token;
> +       void            (*cb)(void *dt, int bytes_xfer);
> +};
> +


> +
> +       if (res >= snd_pcm_lib_buffer_bytes(substream)) {
> +                       res = 0;
> +       }
Dropping braces would be nice.


> +               addr += prtd->period;
> +               addr %= prtd->periodsz;
This is wrong.  Perhaps you want...
             addr += prtd->periodsz;
             addr %= prtd->size;
I doubt one could miss this error if the driver was tested.


> +
> +MODULE_AUTHOR("Jaswinder Singh, <jassi.brar at samsung.com>");
Sent emails will bounce from jassi.brar at samsung.com.
Please use jassisinghbrar at gmail.com


> +#define MAX_IDMA_PERIOD (105 * 1024)
> +#define MAX_IDMA_BUFFER (128 * 1024)
IIRC not all SoCs have same size iBuff ?
Maybe passing via platform_data would be good ?