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

[Val Packett]

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@xxxxxxxxxxxx>
> > ---
> >   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