Re: [PATCH 2/5] ASoC: qcom: sm8250: add Senary MI2S RX support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] sound: missing audio format configuration for secondary codec DAIs
- [Medium] sound: unhandled return values from DAI hardware configuration functions
--

commit a955f60b4e954bf749d62fe6837b52a492472249
Author: Val Packett <val@packett.cool>
ASoC: qcom: sm8250: add Senary MI2S RX support

This patch adds support for the SENARY_MI2S_RX DAI to allow sending audio
data to speaker amplifiers on some devices.

> diff --git a/sound/soc/qcom/sm8250.c b/sound/soc/qcom/sm8250.c
> index 76dc3a3f4a312..2d49b04cb1f69 100644
> --- a/sound/soc/qcom/sm8250.c
> +++ b/sound/soc/qcom/sm8250.c
> @@ -112,6 +112,14 @@ 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 SENARY_MI2S_RX:
> +		codec_dai_fmt |= SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_I2S;
> +		snd_soc_dai_set_sysclk(cpu_dai,
> +			Q6AFE_LPASS_CLK_ID_SEN_MI2S_IBIT,
> +			MI2S_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);

[Severity: Medium]
Can these hardware configuration functions fail silently?

The newly added code calls snd_soc_dai_set_sysclk() and snd_soc_dai_set_fmt()
for both the CPU and codec DAIs, but it does not check or propagate their
return values.

If these operations return an error like -EINVAL or -ENOTSUPP, the
initialization sequence will silently continue, potentially leaving the
hardware in a misconfigured state and causing difficult-to-debug audio
failures later.

> +		snd_soc_dai_set_fmt(cpu_dai, fmt);
> +		snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt);

[Severity: High]
Does this code miss configuring the audio format for secondary codec DAIs?

In sm8250_snd_startup(), codec_dai is retrieved earlier in the function
using:

	struct snd_soc_dai *codec_dai = snd_soc_rtd_to_codec(rtd, 0);

This only fetches the first codec on the runtime link. Since the
SENARY_MI2S_RX link is intended for speaker amplifiers, there may be
multiple speaker codecs (such as left and right channels) on this link.

Because the driver applies the I2S format only to this single codec_dai, any
additional amplifiers will not receive the required format configuration.
Could this result in silent playback on one channel? Would it be better to
iterate over all codec DAIs using for_each_rtd_codec_dais() instead?

> +		break;
>  	case LPI_MI2S_RX_0:
>  		codec_dai_fmt |= SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_I2S;
>  		snd_soc_dai_set_sysclk(cpu_dai,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016073157.611956-1-val@packett.cool?part=2