Re: [RFC PATCH v2 1/3] ep93xx i2s audio driver

[Liam Girdwood <lrg@slimlogic.co.uk>]

On Wed, 2010-05-26 at 17:09 +1200, Ryan Mallon wrote:
> Add ep93xx i2s audio driver
> 
> Signed-off-by: Ryan Mallon <ryan@bluewatersys.com>
> ---
>  sound/soc/Kconfig             |    1 +
>  sound/soc/Makefile            |    1 +
>  sound/soc/ep93xx/Kconfig      |    9 +
>  sound/soc/ep93xx/Makefile     |    8 +
>  sound/soc/ep93xx/ep93xx-i2s.c |  489 +++++++++++++++++++++++++++++++++++++++++
>  sound/soc/ep93xx/ep93xx-i2s.h |   25 ++
>  sound/soc/ep93xx/ep93xx-pcm.c |  323 +++++++++++++++++++++++++++
>  sound/soc/ep93xx/ep93xx-pcm.h |   22 ++
>  8 files changed, 878 insertions(+), 0 deletions(-)
>  create mode 100644 sound/soc/ep93xx/Kconfig
>  create mode 100644 sound/soc/ep93xx/Makefile
>  create mode 100644 sound/soc/ep93xx/ep93xx-i2s.c
>  create mode 100644 sound/soc/ep93xx/ep93xx-i2s.h
>  create mode 100644 sound/soc/ep93xx/ep93xx-pcm.c
>  create mode 100644 sound/soc/ep93xx/ep93xx-pcm.h
> 

Overall looks OK, just some comments below.

> +static void ep93xx_i2s_enable(struct ep93xx_i2s_info *info)
> +{
> +	int i;
> +
> +	/* Enable clocks */
> +	clk_enable(info->mclk);
> +	clk_enable(info->sclk);
> +	clk_enable(info->lrclk);
> +
> +	/* Enable i2s */
> +	ep93xx_i2s_write_reg(info, EP93XX_I2S_GLCTRL, 1);
> +
> +	/* Enable fifos */
> +	for (i = 0; i < 3; i++) {
> +		ep93xx_i2s_write_reg(info, EP93XX_I2S_RX0EN + (i * 4), 1);
> +		ep93xx_i2s_write_reg(info, EP93XX_I2S_TX0EN + (i * 4), 1);

Just curious, do we always need to enable both the RX and TX FIFOs ? Is
there unnecessary system overhead here when we have playback only or
capture only use cases ?


> +static void ep93xx_pcm_buffer_started(void *cookie,
> +				      struct ep93xx_dma_buffer *buf)
> +{
> +}

Do we need this ?



> +
> +static int ep93xx_pcm_prepare(struct snd_pcm_substream *substream)
> +{
> +	return 0;
> +}

This can be removed

Thanks

Liam

-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk