Re: [PATCH v2 4/6] ASoC: qcom: sm8250: add TDM RX support

[Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>]

On 5/6/26 8:33 PM, Val Packett wrote:
> Add support for TDM RX DAIs which are used on some devices to send audio
> data to speaker amplifiers. Channels are assigned based on the codec
> DAI names for a quad-speaker setup such as on the xiaomi-pipa tablet.
> 
> Signed-off-by: Val Packett <val@packett.cool>
> ---
>  sound/soc/qcom/sm8250.c | 141 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 141 insertions(+)
> 
> diff --git a/sound/soc/qcom/sm8250.c b/sound/soc/qcom/sm8250.c
> index a675913da943..b64fd3970ba1 100644
> --- a/sound/soc/qcom/sm8250.c
> +++ b/sound/soc/qcom/sm8250.c
> @@ -17,6 +17,10 @@
>  #include "sdw.h"
>  
>  #define MI2S_BCLK_RATE		1536000
> +#define TDM_BCLK_RATE		6144000
> +#define NUM_TDM_SLOTS		8
> +
> +static unsigned int tdm_slot_offset[8] = {0, 4, 8, 12, 16, 20, 24, 28};
static const ?

>  
>  struct sm8250_snd_data {
>  	bool stream_prepared[AFE_PORT_MAX];
> @@ -55,6 +59,89 @@ static void sm8250_snd_exit(struct snd_soc_pcm_runtime *rtd)
>  
>  }
>  
> +static int sm8250_tdm_snd_hw_params(struct snd_pcm_substream *substream,
> +					struct snd_pcm_hw_params *params)
> +{
> +	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);
> +	struct snd_soc_dai *codec_dai;
> +	unsigned int rx_mask;
> +	int ret = 0, j;
> +	int channels, slot_width;
> +
> +	switch (params_format(params)) {
> +	case SNDRV_PCM_FORMAT_S16_LE:
> +		slot_width = 16;
> +		break;

Any reason only S16?

> +	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,
Please define the magic constants like 0x3 here.

> +				8, slot_width);
> +		if (ret < 0) {
> +			dev_err(rtd->dev, "%s: failed to set tdm slot, err:%d\n",
> +					__func__, ret);
> +			goto end;
> +		}
> +
> +		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);
> +			goto end;
> +		}
> +	} 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);
> +			goto end;
> +		}
> +
> +		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);
> +			goto end;
> +		}
> +	}
> +
> +	for_each_rtd_codec_dais(rtd, j, codec_dai) {
> +		if (strstr(codec_dai->component->name_prefix, "PL")) {
name prefix can be null.

> +			rx_mask = BIT(0);
> +		} else if (strstr(codec_dai->component->name_prefix, "PR")) {
name prefix comparision is fragile design, this will break very soon.
and this is not a binding too.


This also raises question about the using this generic driver for such
specific combination. Can these be made specific to compatible ?


> +			rx_mask = BIT(1);
> +		} else if (strstr(codec_dai->component->name_prefix, "SL")) {
> +			rx_mask = BIT(2);
> +		} else if (strstr(codec_dai->component->name_prefix, "SR")) {
> +			rx_mask = BIT(3);
> +		} else {
> +			rx_mask = 0;
> +			dev_warn(rtd->dev, "%s: codec DAI name '%s' not recognized\n",
> +				__func__, codec_dai->component->name_prefix);
> +		}
> +		ret = snd_soc_dai_set_tdm_slot(codec_dai, 0, rx_mask,
> +				NUM_TDM_SLOTS, slot_width);



> +		if (ret < 0) {
Can we handle -ENOSUPP?

> +			dev_err(rtd->dev, "%s: failed to set TDM slot, err:%d\n",
> +				__func__, ret);
> +			goto end;
> +		}
> +	}
> +
> +end:
> +	return ret;
> +}
> +
>  static int sm8250_be_hw_params_fixup(struct snd_soc_pcm_runtime *rtd,
>  				     struct snd_pcm_hw_params *params)
>  {
> @@ -120,6 +207,41 @@ static int sm8250_snd_startup(struct snd_pcm_substream *substream)
>  		snd_soc_dai_set_fmt(cpu_dai, fmt);
>  		snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt);
>  		break;
> +	case PRIMARY_TDM_RX_0:
> +		codec_dai_fmt |= SND_SOC_DAIFMT_IB_NF | SND_SOC_DAIFMT_DSP_B;
> +		snd_soc_dai_set_sysclk(cpu_dai,
> +			Q6AFE_LPASS_CLK_ID_PRI_TDM_IBIT,
> +			TDM_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
> +		snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt);
> +		break;
> +	case SECONDARY_TDM_RX_0:
> +		codec_dai_fmt |= SND_SOC_DAIFMT_IB_NF | SND_SOC_DAIFMT_DSP_B;
> +		snd_soc_dai_set_sysclk(cpu_dai,
> +			Q6AFE_LPASS_CLK_ID_SEC_TDM_IBIT,
> +			TDM_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
> +		snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt);
> +		break;
> +	case TERTIARY_TDM_RX_0:
> +		codec_dai_fmt |= SND_SOC_DAIFMT_IB_NF | SND_SOC_DAIFMT_DSP_B;
> +		snd_soc_dai_set_sysclk(cpu_dai,
> +			Q6AFE_LPASS_CLK_ID_TER_TDM_IBIT,
> +			TDM_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
> +		snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt);
> +		break;
> +	case QUATERNARY_TDM_RX_0:
> +		codec_dai_fmt |= SND_SOC_DAIFMT_IB_NF | SND_SOC_DAIFMT_DSP_B;
> +		snd_soc_dai_set_sysclk(cpu_dai,
> +			Q6AFE_LPASS_CLK_ID_QUAD_TDM_IBIT,
> +			TDM_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
> +		snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt);
> +		break;
> +	case QUINARY_TDM_RX_0:
> +		codec_dai_fmt |= SND_SOC_DAIFMT_IB_NF | SND_SOC_DAIFMT_DSP_B;
> +		snd_soc_dai_set_sysclk(cpu_dai,
> +			Q6AFE_LPASS_CLK_ID_QUIN_TDM_IBIT,
> +			TDM_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
> +		snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt);
> +		break;
>  	default:
>  		break;
>  	}
> @@ -145,10 +267,29 @@ static int sm8250_snd_hw_free(struct snd_pcm_substream *substream)
>  	return qcom_snd_sdw_hw_free(substream, &data->stream_prepared[cpu_dai->id]);
>  }
>  
> +static int sm8250_snd_hw_params(struct snd_pcm_substream *substream,
> +				struct snd_pcm_hw_params *params)
> +{
> +	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);
> +	int ret = 0;
> +
> +	switch (cpu_dai->id) {
> +	case PRIMARY_TDM_RX_0...QUINARY_TDM_TX_7:
> +		ret = sm8250_tdm_snd_hw_params(substream, params);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
>  static const struct snd_soc_ops sm8250_be_ops = {
>  	.startup = sm8250_snd_startup,
>  	.shutdown = qcom_snd_sdw_shutdown,
>  	.hw_free = sm8250_snd_hw_free,
> +	.hw_params = sm8250_snd_hw_params,
>  	.prepare = sm8250_snd_prepare,
>  };
>