Re: [PATCH RFC 19/23] ASoC: qcom: sc8280xp: allow using the sc8280xp card with lpass-cpu

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] ASoC: qcom: sc8280xp: DAI ID collision in
  sc8280xp_snd_hw_params()
- [Medium] ASoC: qcom: sc8280xp: DAI ID collision in
  sc8280xp_be_hw_params_fixup()
- [Medium] ASoC: qcom: sc8280xp: missing lpass-cpu jack DAI IDs in
  sc8280xp_snd_init()
--

commit 3e03c7d28b8733d89a72bc91c60e7c4086af115e
Author: Neil Armstrong <neil.armstrong@linaro.org>

ASoC: qcom: sc8280xp: allow using the sc8280xp card with lpass-cpu

This commit adds LPASS Codec DMA DAI IDs and attaches DAI link callbacks
unconditionally so the sc8280xp machine driver can be used with
lpass-cpu DAI links.

> diff --git a/sound/soc/qcom/sc8280xp.c b/sound/soc/qcom/sc8280xp.c
> index 8cc75a7ad866d..ff57fb9051a26 100644
> --- a/sound/soc/qcom/sc8280xp.c
> +++ b/sound/soc/qcom/sc8280xp.c

[ ... ]

> @@ -242,6 +243,8 @@ static int sc8280xp_snd_init(struct snd_soc_pcm_runtime *rtd)
>  	switch (cpu_dai->id) {
>  	case WSA_CODEC_DMA_RX_0:
>  	case WSA_CODEC_DMA_RX_1:
> +	case LPASS_CDC_DMA_WSA_RX0:
> +	case LPASS_CDC_DMA_WSA_RX1:
>  		/*
>  		 * Set limit of -3 dB on Digital Volume and 0 dB on PA Volume
>  		 * to reduce the risk of speaker damage until we have active

[Severity: Medium]
Does switching on cpu_dai->id here without checking rtd->dai_link->no_pcm
collide with Q6DSP TDM DAI IDs, while leaving DisplayPort and WCD headset
jack setup broken on lpass-cpu links?

LPASS_CDC_DMA_WSA_RX0 and LPASS_CDC_DMA_WSA_RX1 (36 and 37) have the same
values as PRIMARY_TDM_RX_6 and PRIMARY_TDM_TX_6 on AudioReach DPCM links.
Meanwhile, for lpass-cpu links, LPASS_DP_RX is missing from the
DisplayPort cases below (skipping qcom_snd_dp_jack_setup()), and
qcom_snd_wcd_jack_setup() only attaches the jack to the codec component
for LPI_MI2S_RX_0 and TX_CODEC_DMA_TX_0..3, ignoring
LPASS_CDC_DMA_TX0..3.

> @@ -323,6 +326,10 @@ static int sc8280xp_be_hw_params_fixup(struct snd_soc_pcm_runtime *rtd,
>  		}
>  		break;
>  	}
> +	case LPASS_CDC_DMA_TX0:
> +	case LPASS_CDC_DMA_TX1:
> +	case LPASS_CDC_DMA_TX2:
> +	case LPASS_CDC_DMA_TX3:
>  	case TX_CODEC_DMA_TX_0:
>  	case TX_CODEC_DMA_TX_1:
>  	case TX_CODEC_DMA_TX_2:

[Severity: Medium]
Does adding LPASS_CDC_DMA_TX0..LPASS_CDC_DMA_TX3 to
sc8280xp_be_hw_params_fixup() alter PRIMARY_MI2S and SECONDARY_MI2S DPCM
backends instead of affecting lpass-cpu links?

In sc8280xp_add_be_ops(), sc8280xp_be_hw_params_fixup() is only wired up
when link->no_pcm == 1 (AudioReach DPCM backend links), so it never runs
on non-DPCM lpass-cpu links.  On DPCM links where it does run,
LPASS_CDC_DMA_TX0..LPASS_CDC_DMA_TX3 (16..19) alias PRIMARY_MI2S_RX,
PRIMARY_MI2S_TX, SECONDARY_MI2S_RX and SECONDARY_MI2S_TX (16..19),
changing their minimum channel constraint from 2 to 1.

> @@ -533,12 +540,11 @@ static void sc8280xp_add_be_ops(struct snd_soc_card *card)
>  	int i;
>  
>  	for_each_card_prelinks(card, i, link) {
> -		if (link->no_pcm == 1) {
> -			link->init = sc8280xp_snd_init;
> -			link->exit = sc8280xp_snd_exit;
> +		link->init = sc8280xp_snd_init;
> +		link->exit = sc8280xp_snd_exit;
> +		link->ops = &sc8280xp_be_ops;
> +		if (link->no_pcm == 1)
>  			link->be_hw_params_fixup = sc8280xp_be_hw_params_fixup;
> -			link->ops = &sc8280xp_be_ops;
> -		}
>  	}
>  }

[Severity: High]
When sc8280xp_be_ops is attached to non-DPCM lpass-cpu links, does
sc8280xp_snd_hw_params() misidentify LPASS_CDC_DMA_TX0..7 as MI2S DAIs
and fail stream setup with -ENOTSUPP?

sound/soc/qcom/sc8280xp.c:sc8280xp_snd_hw_params() {
    ...
	switch (cpu_dai->id) {
	case PRIMARY_MI2S_RX ... QUATERNARY_MI2S_TX:
    ...
		if (mi2s_mclk_enable) {
			ret = snd_soc_dai_set_sysclk(cpu_dai,
						     LPAIF_MI2S_MCLK, mclk_freq,
						     SND_SOC_CLOCK_OUT);
			if (ret)
				return ret;
		}
    ...
	case PRIMARY_TDM_RX_0 ... QUINARY_TDM_TX_7:
	case AIF_TDM_RX_0 ... AIF_TDM_TX_12:
		return sc8280xp_tdm_hw_params(substream, params);
    ...
}

LPASS_CDC_DMA_TX0..LPASS_CDC_DMA_TX7 (16..23) share the same numeric IDs
as PRIMARY_MI2S_RX..QUATERNARY_MI2S_TX (16..23).  On platforms such as
sm8650 where mi2s_mclk_enable and mi2s_bclk_enable are set in
sm8650_priv_data, starting a capture stream on LPASS_CDC_DMA_TX0..7 calls
snd_soc_dai_set_sysclk() on the LPASS Codec DMA DAI, which does not
implement .set_sysclk and returns -ENOTSUPP.

Conversely, actual lpass-cpu MI2S DAI IDs do not match the Q6DSP MI2S
ranges: MI2S_PRIMARY..MI2S_QUINARY (0..4) hit the default case and skip
format and clock setup, while MI2S_SENARY..MI2S_SEPTENARY (34..35) fall
into PRIMARY_TDM_RX_0..QUINARY_TDM_TX_7 (24..103) and are routed to
sc8280xp_tdm_hw_params().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-topic-sm8650-el2-v1-0-5183f6aab9fc@linaro.org?part=19