Re: [alsa-devel] [PATCH v4 6/8] ASoC: compress: Add support for DAI multicodec

On Tue, Jul 01, 2014 at 09:47:59AM +0200, Benoit Cousson wrote:

Add multicodec support in soc-compress.c

@@ -294,13 +299,19 @@ static int soc_compr_trigger(struct snd_compr_stream *cstream, int cmd) goto out; }

• switch (cmd) {
• case SNDRV_PCM_TRIGGER_START:

• snd_soc_dai_digital_mute(codec_dai, 0, cstream->direction);
  

• break;
  

• case SNDRV_PCM_TRIGGER_STOP:

• snd_soc_dai_digital_mute(codec_dai, 1, cstream->direction);
  

• break;
  

• for (i = 0; i < rtd->num_codecs; i++) {

• struct snd_soc_dai *codec_dai = rtd->codec_dais[i];
  

• switch (cmd) {
  

• case SNDRV_PCM_TRIGGER_START:
  

• 	snd_soc_dai_digital_mute(codec_dai, 0,
  

• 				 cstream->direction);
  

• 	break;
  

• case SNDRV_PCM_TRIGGER_STOP:
  

• 	snd_soc_dai_digital_mute(codec_dai, 1,
  

• 				 cstream->direction);
  

Wouldnt it make sense to fix snd_soc_dai_digital_mute() for multi-codecs. Did you do same thing for pcm?

• 	break;
  

• }
  

  }

out: @@ -622,23 +633,33 @@ int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num) { struct snd_soc_codec *codec = rtd->codec; struct snd_soc_platform *platform = rtd->platform;

• struct snd_soc_dai *codec_dai = rtd->codec_dai; struct snd_soc_dai *cpu_dai = rtd->cpu_dai; struct snd_compr *compr; struct snd_pcm *be_pcm; char new_name[64];
• int ret = 0, direction = 0;
• /* check client and interface hw capabilities */
• snprintf(new_name, sizeof(new_name), "%s %s-%d",

• 	rtd->dai_link->stream_name, codec_dai->name, num);
  

• if (codec_dai->driver->playback.channels_min)

• direction = SND_COMPRESS_PLAYBACK;
  

• else if (codec_dai->driver->capture.channels_min)

• direction = SND_COMPRESS_CAPTURE;
  

• else

• return -EINVAL;
  

• int i, ret = 0, direction = -1;
• for (i = 0; i < rtd->num_codecs; i++) {

• struct snd_soc_dai *codec_dai = rtd->codec_dais[i];
  

• /* check client and interface hw capabilities */
  

• snprintf(new_name, sizeof(new_name), "%s %s-%d",
  

• 	 rtd->dai_link->stream_name, codec_dai->name, num);
  

• if (direction < 0) {
  

• 	if (codec_dai->driver->playback.channels_min)
  

• 		direction = SND_COMPRESS_PLAYBACK;
  

• 	else if (codec_dai->driver->capture.channels_min)
  

• 		direction = SND_COMPRESS_CAPTURE;
  

direction wont change with multiple codecs, so this loop makes no sense here. For simplcity perhaps we can use cpu_dai?

• 	else
  

• 		return -EINVAL;
  

• } else {
  

when will this get executed? You have initialized direction to -1 and if case is always true. I think compiler will simply remove the below sinppet.

• 	if ((codec_dai->driver->playback.channels_min &&
  

• 			direction != SND_COMPRESS_PLAYBACK) ||
  

• 	    (codec_dai->driver->capture.channels_min &&
  

• 			direction != SND_COMPRESS_CAPTURE))
  

and what exactly are we trying to check here?

• 		return -EINVAL;
  

• }
  

• }

  compr = kzalloc(sizeof(*compr), GFP_KERNEL); if (compr == NULL) {

@@ -692,8 +713,11 @@ int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num) rtd->compr = compr; compr->private_data = rtd;

• printk(KERN_INFO "compress asoc: %s <-> %s mapping ok\n", codec_dai->name,

• cpu_dai->name);
  

• for (i = 0; i < rtd->num_codecs; i++) {

• struct snd_soc_dai *codec_dai = rtd->codec_dais[i];
  

• printk(KERN_INFO "compress asoc: %s <-> %s mapping ok\n",
  

•        codec_dai->name, cpu_dai->name);
  

wrong again. You are not creating multiple links with multi-codec. Link is still single.