Received-SPF: pass (google.com: domain of linux-kernel+bounces-171430-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <738045d2-a445-4f93-abfd-203348a538d1@linaro.org>
Date: Tue, 7 May 2024 14:20:41 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 4/4] ASoC: qcom: x1e80100: Correct channel mapping
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
 James Schulman <james.schulman@cirrus.com>,
 David Rhodes <david.rhodes@cirrus.com>,
 Richard Fitzgerald <rf@opensource.cirrus.com>,
 Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
 Liam Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>,
 Lars-Peter Clausen <lars@metafoo.de>, =?UTF-8?Q?Nuno_S=C3=A1?=
 <nuno.sa@analog.com>, Banajit Goswami <bgoswami@quicinc.com>
Cc: alsa-devel@alsa-project.org, patches@opensource.cirrus.com,
 linux-sound@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20240507-asoc-x1e80100-4-channel-mapping-v1-0-b12c13e0a55d@linaro.org>
 <20240507-asoc-x1e80100-4-channel-mapping-v1-4-b12c13e0a55d@linaro.org>
Content-Language: en-US
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
In-Reply-To: <20240507-asoc-x1e80100-4-channel-mapping-v1-4-b12c13e0a55d@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Thanks Krzystof for the patch.

On 07/05/2024 11:27, Krzysztof Kozlowski wrote:
> X1E80100 CRD board comes with four speakers arranged as left front+back
> and then right front+back.  Using default channel mapping causes front
> right speaker to play left back stream.
> 
> Adjust the channel maps for frontend DAIs to fix stereo and four-channel
> playback.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>   sound/soc/qcom/x1e80100.c | 37 +++++++++++++++++++++++++++++++++++--
>   1 file changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/qcom/x1e80100.c b/sound/soc/qcom/x1e80100.c
> index c3c8bf7ffb5b..e90c68815b5c 100644
> --- a/sound/soc/qcom/x1e80100.c
> +++ b/sound/soc/qcom/x1e80100.c
> @@ -12,6 +12,7 @@
>   
>   #include "common.h"
>   #include "qdsp6/q6afe.h"
> +#include "qdsp6/q6dsp-common.h"
>   #include "sdw.h"
>   
>   struct x1e80100_snd_data {
> @@ -74,7 +75,7 @@ static int x1e80100_snd_hw_params(struct snd_pcm_substream *substream,
>   	return qcom_snd_sdw_hw_params(substream, params, &data->sruntime[cpu_dai->id]);
>   }
>   
> -static int x1e80100_snd_prepare(struct snd_pcm_substream *substream)
> +static int x1e80100_snd_be_prepare(struct snd_pcm_substream *substream)
>   {
>   	struct snd_soc_pcm_runtime *rtd = substream->private_data;
>   	struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
> @@ -96,12 +97,34 @@ static int x1e80100_snd_hw_free(struct snd_pcm_substream *substream)
>   				    &data->stream_prepared[cpu_dai->id]);
>   }
>   
> +static int x1e80100_snd_fe_prepare(struct snd_pcm_substream *substream)
> +{
> +	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
> +	struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		const unsigned int rx_slot[4] = { PCM_CHANNEL_FL,
> +						  PCM_CHANNEL_LB,
> +						  PCM_CHANNEL_FR,
> +						  PCM_CHANNEL_RB };
> +
> +		snd_soc_dai_set_channel_map(cpu_dai, 0, NULL, ARRAY_SIZE(rx_slot),
> +					    rx_slot);

Channel mapping are specific to backend dais rather than front end pcm dais.

This will set all the playback pcms with this channel maps, which is a 
problem.

example the 2 channel headset we will endup with data of front channel 
and zeros on the right channel, however a speaker might work as you have 
4 speakers in your system.


So No for this approach.


--srini

> +	}
> +
> +	return 0;
> +}
> +
>   static const struct snd_soc_ops x1e80100_be_ops = {
>   	.startup = qcom_snd_sdw_startup,
>   	.shutdown = x1e80100_snd_shutdown,
>   	.hw_params = x1e80100_snd_hw_params,
>   	.hw_free = x1e80100_snd_hw_free,
> -	.prepare = x1e80100_snd_prepare,
> +	.prepare = x1e80100_snd_be_prepare,
> +};
> +
> +static const struct snd_soc_ops x1e80100_fe_ops = {
> +	.prepare = x1e80100_snd_fe_prepare,
>   };
>   
>   static void x1e80100_add_be_ops(struct snd_soc_card *card)
> @@ -118,6 +141,15 @@ static void x1e80100_add_be_ops(struct snd_soc_card *card)
>   	}
>   }
>   
> +static int x1e80100_add_dai_link(struct snd_soc_card *card, struct snd_soc_dai_link *link)
> +{
> +	/* Add ops for Frontend DAIs coming from Topology */
> +	if (link->dynamic && !link->no_pcm && !link->ops)
> +		link->ops = &x1e80100_fe_ops;
> +
> +	return 0;
> +}
> +
>   static int x1e80100_platform_probe(struct platform_device *pdev)
>   {
>   	struct snd_soc_card *card;
> @@ -135,6 +167,7 @@ static int x1e80100_platform_probe(struct platform_device *pdev)
>   
>   	card->owner = THIS_MODULE;
>   	card->dev = dev;
> +	card->add_dai_link = x1e80100_add_dai_link;
>   	dev_set_drvdata(dev, card);
>   	snd_soc_card_set_drvdata(card, data);
>   
>