Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Message-ID: <c43c4303-ed59-49e7-940a-62df6ad1b575@linaro.org>
Date:   Tue, 12 Dec 2023 11:47:36 +0000
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC, PATCH v2 1/2] ASoC: qcom: sdw: Add TDM support
Content-Language: en-US
To:     Jianhua Lu <lujianhua000@gmail.com>,
        Banajit Goswami <bgoswami@quicinc.com>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>
Cc:     alsa-devel@alsa-project.org, linux-sound@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20231212095850.26482-1-lujianhua000@gmail.com>
From:   Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
In-Reply-To: <20231212095850.26482-1-lujianhua000@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Thankyou for the patch,


On 12/12/2023 09:58, Jianhua Lu wrote:
> Add qcom TDM setup function to support TDM ports for qcom platform.
> 
> Signed-off-by: Jianhua Lu <lujianhua000@gmail.com>
> ---
> Changes in v2:
>    1. remove EXPORT_SYMBOL_GPL
>    2. remove static modifier
> 
>   sound/soc/qcom/sdw.c | 63 ++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 63 insertions(+)
> 
> diff --git a/sound/soc/qcom/sdw.c b/sound/soc/qcom/sdw.c
> index 77dbe0c28b29..948abbd2e0e3 100644
> --- a/sound/soc/qcom/sdw.c
> +++ b/sound/soc/qcom/sdw.c
> @@ -4,6 +4,7 @@
>   
>   #include <dt-bindings/sound/qcom,q6afe.h>
>   #include <linux/module.h>
> +#include <sound/pcm_params.h>
>   #include <sound/soc.h>
>   #include "sdw.h"
>   
> @@ -101,6 +102,65 @@ int qcom_snd_sdw_prepare(struct snd_pcm_substream *substream,
>   }
>   EXPORT_SYMBOL_GPL(qcom_snd_sdw_prepare);
>   
> +int qcom_snd_tdm_hw_params(struct snd_pcm_substream *substream,
> +			   struct snd_pcm_hw_params *params)
> +{

TBH, this should not be part of sdw.c file, its intended for more of 
soundwire specific helpers, pl consider moving this to common.c for now.
Because, Not all old qcom platforms have soundwire controllers.

> +	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);
> +	unsigned int tdm_slot_offset[8] = { 0, 4, 8, 12, 16, 20, 24, 28 };
> +	int channels, slot_width;
> +	int ret;
> +
> +	switch (params_format(params)) {
> +	case SNDRV_PCM_FORMAT_S16_LE:
> +		slot_width = 16;
> +		break;
> +	case SNDRV_PCM_FORMAT_S24_LE:
> +		slot_width = 24;
> +		break;
> +	case SNDRV_PCM_FORMAT_S32_LE:
> +		slot_width = 32;
> +		break;
> +	default:
> +		dev_err(rtd->dev, "%s: invalid param format 0x%x\n", __func__,
> +			params_format(params));
> +		return -EINVAL;
> +	}
> +
> +	channels = params_channels(params);
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		ret = snd_soc_dai_set_tdm_slot(cpu_dai, 0, 0x3, 8, slot_width);

slot mask is always set to 2 channels in this case, should you not check 
the number of channels to determine the correct one?


These magic number 0, 0x3, 8 seems to make the code unreadable, can you 
do something like this:
snd_soc_dai_set_tdm_slot(cpu_dai, tx_mask, rx_mask, 
ARRAY_SIZE(tdm_slot_offset), slot_width);


> +		if (ret < 0) {
> +			dev_err(rtd->dev, "%s: failed to set tdm slot, err:%d\n",
> +				__func__, ret);
> +			return ret;
> +		}
> +
> +		ret = snd_soc_dai_set_channel_map(cpu_dai, 0, NULL, channels,
> +						  tdm_slot_offset);
> +		if (ret < 0) {
> +			dev_err(rtd->dev, "%s: failed to set channel map, err:%d\n",
> +				__func__, ret);
> +			return ret;
> +		}
> +	} else {
> +		ret = snd_soc_dai_set_tdm_slot(cpu_dai, 0xf, 0, 8, slot_width);
> +		if (ret < 0) {
> +			dev_err(rtd->dev, "%s: failed to set tdm slot, err:%d\n",
> +				__func__, ret);
> +			return ret;
> +		}
> +
> +		ret = snd_soc_dai_set_channel_map(cpu_dai, channels,
> +						  tdm_slot_offset, 0, NULL);
> +		if (ret < 0) {
> +			dev_err(rtd->dev, "%s: failed to set channel map, err:%d\n",
> +				__func__, ret);
> +			return ret;
> +		}
> +	}
Finally  ./sound/soc/qcom/sdm845.c does have exactly same code, can you 
consider removing this and make use of this new helper in that file too.


> +}
> +
>   int qcom_snd_sdw_hw_params(struct snd_pcm_substream *substream,
>   			   struct snd_pcm_hw_params *params,
>   			   struct sdw_stream_runtime **psruntime)
> @@ -125,6 +185,9 @@ int qcom_snd_sdw_hw_params(struct snd_pcm_substream *substream,
>   				*psruntime = sruntime;
>   		}
>   		break;
> +	case PRIMARY_TDM_RX_0...QUINARY_TDM_TX_7:
> +		qcom_snd_tdm_hw_params(substream, params);
> +		break;
>   	}
>   
>   	return 0;