From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Mack Subject: Re: [PATCH] davinci-mcasp: Add support for multichannel playback Date: Tue, 26 Feb 2013 23:49:37 +0100 Message-ID: <512D3C01.3060400@gmail.com> References: <1361895658-27256-1-git-send-email-michal.bachraty@streamunlimited.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-bk0-f53.google.com (mail-bk0-f53.google.com [209.85.214.53]) by alsa0.perex.cz (Postfix) with ESMTP id 019C82652FB for ; Tue, 26 Feb 2013 23:49:40 +0100 (CET) Received: by mail-bk0-f53.google.com with SMTP id j10so2105903bkw.26 for ; Tue, 26 Feb 2013 14:49:40 -0800 (PST) In-Reply-To: <1361895658-27256-1-git-send-email-michal.bachraty@streamunlimited.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Michal Bachraty Cc: alsa-devel@alsa-project.org, Mark Brown , Liam Girdwood List-Id: alsa-devel@alsa-project.org Hi Michal, (+ Mark and Liam - the ASoC maintainers) many thanks for the patch. I couldn't try it yet on real hardware, but will do so soon. Meanwhile, below are some comments on locations I stumbled over ... Best regards, Daniel On 26.02.2013 17:20, Michal Bachraty wrote: > Davinci McASP has support for I2S multichannel playback. > For I2S playback/receive, each serializer is capable to play 2 channels > (L/R) audio data.Serializer function (Playback-receive-none) is configured > in DT, depending on hardware specification. It is possible to play less > channels than configured in DT. For that purpose,only specific number of > active serializers are enabled. McASP FIFO need to have DMA transfer Bcnt > set to number of enabled serializers, otherwise no data are transfered to > McASP and Alsa generates "DMA/IRQ playback write error (DMA or IRQ trouble?)" > error. > > Signed-off-by: Michal Bachraty > --- > sound/soc/davinci/davinci-mcasp.c | 49 +++++++++++++++++++++++++++++++------ > sound/soc/davinci/davinci-pcm.c | 15 ++++++------ > sound/soc/davinci/davinci-pcm.h | 1 + > 3 files changed, 51 insertions(+), 14 deletions(-) > > diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c > index d2ca682..ebd6ee0 100644 > --- a/sound/soc/davinci/davinci-mcasp.c > +++ b/sound/soc/davinci/davinci-mcasp.c > @@ -657,12 +657,14 @@ static int davinci_config_channel_size(struct davinci_audio_dev *dev, > return 0; > } > > -static void davinci_hw_common_param(struct davinci_audio_dev *dev, int stream) > +static int davinci_hw_common_param(struct davinci_audio_dev *dev, int stream, > + int channels) Stray change of of the return value here? At least, you don't return anything, which gives a build warning. But you don't care for the return value either, so this is probably just unintentional. > { > int i; > u8 tx_ser = 0; > u8 rx_ser = 0; > - > + u8 ser; > + u8 stream_serializers = channels/2 + channels%2; I think you can write that as '(channels + 1) / 2', which is more obvious. Nit: I'd rather call the variable num_serializers or something like that ... > /* Default configuration */ > mcasp_set_bits(dev->base + DAVINCI_MCASP_PWREMUMGT_REG, MCASP_SOFT); > > @@ -680,16 +682,44 @@ static void davinci_hw_common_param(struct davinci_audio_dev *dev, int stream) > } > > for (i = 0; i < dev->num_serializer; i++) { > + if (dev->serial_dir[i] == TX_MODE) > + tx_ser++; > + if (dev->serial_dir[i] == RX_MODE) > + rx_ser++; > + } > + > + if (stream == SNDRV_PCM_STREAM_PLAYBACK) { > + ser = tx_ser; > + } else { > + ser = rx_ser; > + } No curly brackets needed here. > + > + if (ser < stream_serializers) { > + printk(KERN_WARNING "davinci-mcasp: stream has more channels " > + "(%d) than are enabled in mcasp (%d)", > + channels, ser * 2); Use dev_warn(), and check the indentation. > + return -EINVAL; > + } > + > + tx_ser = 0; > + rx_ser = 0; > + > + for (i = 0; i < dev->num_serializer; i++) { > mcasp_set_bits(dev->base + DAVINCI_MCASP_XRSRCTL_REG(i), > dev->serial_dir[i]); > - if (dev->serial_dir[i] == TX_MODE) { > + if (dev->serial_dir[i] == TX_MODE && > + tx_ser < stream_serializers) { > mcasp_set_bits(dev->base + DAVINCI_MCASP_PDIR_REG, > AXR(i)); > tx_ser++; > - } else if (dev->serial_dir[i] == RX_MODE) { > + } else if (dev->serial_dir[i] == RX_MODE && > + rx_ser < stream_serializers) { > mcasp_clr_bits(dev->base + DAVINCI_MCASP_PDIR_REG, > AXR(i)); > rx_ser++; > + } else { > + mcasp_clr_bits(dev->base + DAVINCI_MCASP_XRSRCTL_REG(i), > + 3); Can the magic '3' go into a macro definition? > } > } > > @@ -812,8 +842,12 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream, > &dev->dma_params[substream->stream]; > int word_length; > u8 fifo_level; > + int channels; > + struct snd_interval *pcm_channels = hw_param_interval(params, > + SNDRV_PCM_HW_PARAM_CHANNELS); > + channels = pcm_channels->min; > > - davinci_hw_common_param(dev, substream->stream); > + davinci_hw_common_param(dev, substream->stream, channels); > if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) > fifo_level = dev->txnumevt; > else > @@ -862,6 +896,7 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream, > dma_params->acnt = dma_params->data_type; > > dma_params->fifo_level = fifo_level; > + dma_params->active_serializers = channels/2 + channels%2; See above. > davinci_config_channel_size(dev, word_length); > > return 0; > @@ -936,13 +971,13 @@ static struct snd_soc_dai_driver davinci_mcasp_dai[] = { > .name = "davinci-mcasp.0", > .playback = { > .channels_min = 2, > - .channels_max = 2, > + .channels_max = 8, > .rates = DAVINCI_MCASP_RATES, > .formats = DAVINCI_MCASP_PCM_FMTS, > }, > .capture = { > .channels_min = 2, > - .channels_max = 2, > + .channels_max = 8, > .rates = DAVINCI_MCASP_RATES, > .formats = DAVINCI_MCASP_PCM_FMTS, > }, > diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c > index bb57552..9951973 100644 > --- a/sound/soc/davinci/davinci-pcm.c > +++ b/sound/soc/davinci/davinci-pcm.c > @@ -182,6 +182,7 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream) > unsigned short acnt; > unsigned int count; > unsigned int fifo_level; > + unsigned char serializers = prtd->params->active_serializers; > > period_size = snd_pcm_lib_period_bytes(substream); > dma_offset = prtd->period * period_size; > @@ -193,7 +194,7 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream) > period_size); > > data_type = prtd->params->data_type; > - count = period_size / data_type; > + count = period_size / (data_type * serializers); > if (fifo_level) > count /= fifo_level; > > @@ -201,8 +202,8 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream) > src = dma_pos; > dst = prtd->params->dma_addr; > src_bidx = data_type; > - dst_bidx = 0; > - src_cidx = data_type * fifo_level; > + dst_bidx = 4; > + src_cidx = data_type * fifo_level * serializers; > dst_cidx = 0; > } else { > src = prtd->params->dma_addr; > @@ -210,7 +211,7 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream) > src_bidx = 0; > dst_bidx = data_type; > src_cidx = 0; > - dst_cidx = data_type * fifo_level; > + dst_cidx = data_type * fifo_level * serializers; > } > > acnt = prtd->params->acnt; > @@ -224,9 +225,9 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream) > edma_set_transfer_params(prtd->asp_link[0], acnt, count, 1, 0, > ASYNC); > else > - edma_set_transfer_params(prtd->asp_link[0], acnt, fifo_level, > - count, fifo_level, > - ABSYNC); > + edma_set_transfer_params(prtd->asp_link[0], acnt, serializers, > + count, fifo_level * serializers, > + ABSYNC); > } > > static void davinci_pcm_dma_irq(unsigned link, u16 ch_status, void *data) > diff --git a/sound/soc/davinci/davinci-pcm.h b/sound/soc/davinci/davinci-pcm.h > index fbb710c..0d84d32 100644 > --- a/sound/soc/davinci/davinci-pcm.h > +++ b/sound/soc/davinci/davinci-pcm.h > @@ -27,6 +27,7 @@ struct davinci_pcm_dma_params { > unsigned char data_type; /* xfer data type */ > unsigned char convert_mono_stereo; > unsigned int fifo_level; > + unsigned char active_serializers; /* num. of active audio serializers */ > }; > > int davinci_soc_platform_register(struct device *dev); >