From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark Brown Subject: Re: [PATCH 3/3 v2] sound/soc/lapis: add platform driver for ML7213 IOH I2S Date: Thu, 22 Dec 2011 00:58:16 +0000 Message-ID: <20111222005816.GB27144@opensource.wolfsonmicro.com> References: <1324349144-12784-1-git-send-email-tomoya.rohm@gmail.com> <1324349144-12784-3-git-send-email-tomoya.rohm@gmail.com> <20111220132314.GQ2866@opensource.wolfsonmicro.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Tomoya MORINAGA Cc: Liam Girdwood , Jaroslav Kysela , Takashi Iwai , Lars-Peter Clausen , Dimitris Papastamos , Mike Frysinger , Daniel Mack , alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, qi.wang@intel.com, yong.y.wang@intel.com, joel.clark@intel.com, kok.howg.ewe@intel.com List-Id: alsa-devel@alsa-project.org On Wed, Dec 21, 2011 at 08:22:56PM +0900, Tomoya MORINAGA wrote: > 2011/12/20 Mark Brown : > > On Tue, Dec 20, 2011 at 11:45:44AM +0900, Tomoya MORINAGA wrote: > >> + =A0 =A0 iowrite32(I2S_TX_FINT | I2S_TX_AFINT | I2S_TX_EINT | I2S= _TX_AEINT, > >> + =A0 =A0 =A0 =A0 =A0 =A0 i2s_data->iobase + I2SISTTX_OFFSET + off= set); > > This appears to unconditionally acknowledge all interrupts, general= ly > > you should only acknowledge interrupts that have been handled. > > Otherwise it's possible that interrupts may be dropped if they're > > flagged between the status read and acknowledgement write. > I can understand your saying. > If following your saying, only called by i2s_dma_tx_complete is enoug= h. > However I think adding clearing interrupt status at both before and > after i2s communicating start/finish is better. No, the issue is that you're acknowleding a hard coded set of interrupts, not the interrupts that were actually handled. The orderin= g isn't really the issue, the issue is that you could acknowledge things that weren't handled. For example: 1. ISR runs, reads status A 2. Condition B happens 3. ISR handles condition A. 4. ISR acknowledges both A and B. would mean that B would just get ignored. > >> +static bool filter(struct dma_chan *chan, void *slave) > > This needs a better name. =A0It's not namespaced and it's not clear= what > > it's filtering. > In case of using Linux standard DMA API, function "filter" is the mos= t > popular name. > Almost drivers which use standard use "filter". I suspect they may have the filter immediately next to the code that uses it, which certainly isn't the case here. > >> +ioh_i2s_write(struct snd_pcm_substream *substream, const void *da= ta, int len) > >> +{ > >> + =A0 =A0 int rem1; > >> + =A0 =A0 int rem2; > > What is this supposed to do? =A0There's an awful lot of it, and it = looks > > like it's supposed to be rewriting the data format for some reason = which > > isn't something that a driver ought to be doing (apart from anythin= g > > else it does rather defeat the point of DMA). > This driver has buffer which is for preventing noisy sound by jitter > So, this buffer is variable. You're implementing some sort of custom buffering in your driver? That sounds terribly unidiomatic - pretty much all DMA drivers are very thin and manage to work well, I'd expect this is masking some problems in th= e code rather than anything else. Can you provide more detail on what this is working around? > >> + =A0 =A0 num =3D ((int)dma->tx_avail) / (I2S_AEMPTY_THRESH * 4); > > That case looks *very* suspect. > This is correct. > Why do you think so ? I can't tell what it's supposed to do, and casting that isn't super clear especially casting to integer types tends to be a big red flag.