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

[Val Packett <val@packett.cool>]

On 5/7/26 5:51 AM, Srinivas Kandagatla wrote:
> 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 ?
ack
>>   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?

The entire driver is "locked" to S16 since 89be3c15a58b ("ASoC: qcom: 
sm8250: explicitly set format in sm8250_be_hw_params_fixup()"). Which 
was due to compressed playback, according to that commit message.. But 
in reality there's also hardcoded MI2S_BCLK_RATE (so I did the same 
thing for TDM for now).

I cooould try unhardcoding that, but this kind of thing is really 
annoying to test with the whole combinatorial explosion of parameters x_x

>> +	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.
ack
>> +			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.

Right, but how would one even define a binding when this is supposed to 
be(?) part of the sndcard and the prefixes themselves are set on codecs' 
own nodes..?

I just copied what's been done for sdm845 so far, even though it doesn't 
seem great to me either.

> This also raises question about the using this generic driver for such
> specific combination. Can these be made specific to compatible ?
Yeah, sure. I don't think we'll see other combinations in practice 
though, for 2 channels regular I2S is used, the only reason for TDM is 
to have 4 channels. Or more but I don't think any device has more than 
4. (And surely no one has 3..)
>> +			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?
By not erroring out here? Yeah
> [..]


Thanks,
~val