Re: [PATCH] davinci-mcasp: Add support for multichannel playback

[Daniel Mack <zonque@gmail.com>]

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 <michal.bachraty@streamunlimited.com>
> ---
>  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);
>