From mboxrd@z Thu Jan 1 00:00:00 1970 From: Liam Girdwood Subject: Re: [RFC PATCH v2 1/3] ep93xx i2s audio driver Date: Wed, 26 May 2010 12:07:21 +0100 Message-ID: <1274872041.3240.158.camel@odin> References: <1274850588-30460-1-git-send-email-ryan@bluewatersys.com> <1274850588-30460-2-git-send-email-ryan@bluewatersys.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wy0-f179.google.com (mail-wy0-f179.google.com [74.125.82.179]) by alsa0.perex.cz (Postfix) with ESMTP id 455A910381A for ; Wed, 26 May 2010 13:07:25 +0200 (CEST) Received: by wyb33 with SMTP id 33so1022848wyb.38 for ; Wed, 26 May 2010 04:07:24 -0700 (PDT) In-Reply-To: <1274850588-30460-2-git-send-email-ryan@bluewatersys.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: alsa-devel-bounces@alsa-project.org Errors-To: alsa-devel-bounces@alsa-project.org To: Ryan Mallon Cc: john.cooper@third-harmonic.com, alsa-devel@alsa-project.org, broonie@opensource.wolfsonmicro.com, chasedouglas@gmail.com, buytenh@wantstofly.org, r&d4@dave-tech.it, hartleys@visionengravers.com, linux-arm-kernel@lists.infradead.org, dhuggins@cs.cmu.edu List-Id: alsa-devel@alsa-project.org On Wed, 2010-05-26 at 17:09 +1200, Ryan Mallon wrote: > Add ep93xx i2s audio driver > > Signed-off-by: Ryan Mallon > --- > 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