Re: [PATCH v7 7/7] ASoC: qcom: sc8280xp: add TDM hw_params support

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] sc8280xp_tdm_hw_params() fails to ignore -ENOTSUPP errors from snd_soc_dai_set_fmt() and snd_soc_dai_set_sysclk(), causing unconditional stream failures for TDM DAIs.
- [Medium] The return value of snd_soc_tdm_params_to_bclk() is incorrectly stored in an unsigned integer, causing potential negative error codes to bypass the error check and be used as huge clock frequencies.
- [Medium] sc8280xp_tdm_hw_params() sets the audio format for the CPU DAI but fails to configure the format for the codec DAIs.
--

--- Patch [7]: [PATCH v7 7/7] ASoC: qcom: sc8280xp: add TDM hw_params support ---
commit f81d8adf5e4da9f01c411fd511ee11515191c223
Author: Prasad Kumpatla <[email protected]>

ASoC: qcom: sc8280xp: add TDM hw_params support

This commit adds TDM backend handling to the sc8280xp machine driver.
It parses standard DAI TDM slot properties and derives the LPASS backend
bit clock from the runtime parameters.

> diff --git a/sound/soc/qcom/sc8280xp.c b/sound/soc/qcom/sc8280xp.c
> index c3ce3e05b2608..597c0d887d2f4 100644
> --- a/sound/soc/qcom/sc8280xp.c
> +++ b/sound/soc/qcom/sc8280xp.c
[ ... ]
> @@ -106,6 +106,63 @@ static inline int sc8280xp_get_bclk_freq(struct snd_pcm_hw_params *params)
> +static int sc8280xp_tdm_hw_params(struct snd_pcm_substream *substream,
> +				  struct snd_pcm_hw_params *params)
> +{
[ ... ]
> +	if (!cpu_cfg.slots)
> +		return 0;
> +
> +	ret = snd_soc_dai_set_fmt(cpu_dai, SND_SOC_DAIFMT_BP_FP);
> +	if (ret)
> +		return ret;

[Severity: High]
Does this strictly require the CPU DAI to support set_fmt? Since the QCOM DSP
TDM DAIs (q6tdm_ops) do not implement the set_fmt callback, 
snd_soc_dai_set_fmt() will return -ENOTSUPP. Should this explicitly ignore
-ENOTSUPP, similar to how it is handled in the MI2S path, to avoid failing
stream initialization unconditionally?

[Severity: Medium]
Additionally, this sets the audio format for the CPU DAI, but does it omit
configuring the format for the codec DAIs? Without setting the format for
the codec DAIs (like data->priv->codec_dai_fmt used in the MI2S path), will
the codec remain in its default format and cause a mismatch with the CPU DAI?

> +
> +	ret = qcom_snd_apply_dai_tdm_slots_cfg(rtd, &cpu_cfg, &codec_cfg);
> +	if (ret)
> +		return ret;
> +
> +	bclk_freq = snd_soc_tdm_params_to_bclk(params, cpu_cfg.slot_width, cpu_cfg.slots, 1);
> +	if (!bclk_freq)
> +		return -EINVAL;

[Severity: Medium]
Can snd_soc_tdm_params_to_bclk() return a negative error code like -EINVAL
here? Since bclk_freq is an unsigned int, a negative return value would
underflow into a large positive integer. This would bypass the !bclk_freq
check and potentially pass an invalid massive frequency to the system clock
provider.

[ ... ]
> +	if (data->priv->codec_sysclk_set) {
> +		for_each_rtd_codec_dais(rtd, i, codec_dai) {
> +			ret = snd_soc_dai_set_sysclk(codec_dai, 0, bclk_freq,
> +						     SND_SOC_CLOCK_IN);
> +			if (ret) {
> +				dev_err(rtd->dev, "%s: failed to set codec sysclk on %s: %d\n",
> +					__func__, codec_dai->name, ret);
> +				return ret;
> +			}
> +		}
> +	}

[Severity: High]
Will this codec sysclk setup loop fail stream initialization if any codec in
the link does not support setting sysclk? Similar to the set_fmt call above,
if snd_soc_dai_set_sysclk() returns -ENOTSUPP, should that error be explicitly
ignored rather than treated as a fatal error?

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7