Re: [PATCH v2 3/3] ASoC: qcom: sc8280xp: ASoC: qcom: sc8280xp: enhance machine driver for board-specific config

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `sc8280xp_get_mclk_freq()` incorrectly compares integer sample rate values in Hz with ALSA `SNDRV_PCM_RATE_*` bitmask constants.
- [Medium] The `codec_sysclk_set` flag incorrectly applies the sysclk configuration to the CPU DAI instead of the CODEC DAI.
- [Low] Returning `-ENOMEM` when `of_device_get_match_data()` returns NULL.
--

commit 66516d9230e47a6cd893799b841db83183cc7051
Author: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>

ASoC: qcom: sc8280xp: ASoC: qcom: sc8280xp: enhance machine driver for board-specific config

This commit enhances the sc8280xp machine driver to support board-specific
configurations. It allows each board variant to provide its own DAPM widgets
and routes, and enables MI2S MCLK programming for external codecs that
require a stable master clock.

> diff --git a/sound/soc/qcom/sc8280xp.c b/sound/soc/qcom/sc8280xp.c
> index 7925aa3f63ba0..1f3afc6d015cf 100644
> --- a/sound/soc/qcom/sc8280xp.c
> +++ b/sound/soc/qcom/sc8280xp.c
[ ... ]
> @@ -12,17 +12,77 @@
[ ... ]
> +static inline int sc8280xp_get_mclk_freq(struct snd_pcm_hw_params *params)
> +{
> +	int rate = params_rate(params);
> +
> +	switch (rate) {
> +	case SNDRV_PCM_RATE_11025:
> +	case SNDRV_PCM_RATE_44100:
> +	case SNDRV_PCM_RATE_88200:

[Severity: High]
Does this switch statement compare integer frequencies in Hz against ALSA rate
bitmask macros?

The function params_rate(params) returns the actual sample rate in Hz
(e.g., 44100), but macros like SNDRV_PCM_RATE_44100 evaluate to bitmasks
(e.g., 1U << 6, or 64). Because an integer frequency will never equal its
bitmask value, it seems this switch will always fall through to the default
case.

This will accidentally yield the correct multiplier for 44100 Hz streams, but
will incorrectly calculate the MCLK rate for 11025 Hz and 88200 Hz streams.

> +		return I2S_MCLK_RATE(44100);
> +	default:
> +		break;
> +	}
> +
> +	return I2S_MCLK_RATE(rate);
> +}
[ ... ]
> @@ -96,6 +152,47 @@ static int sc8280xp_be_hw_params_fixup(struct snd_soc_pcm_runtime *rtd,
[ ... ]
> +static int sc8280xp_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 *codec_dai = snd_soc_rtd_to_codec(rtd, 0);
> +	struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
[ ... ]
> +		if (data->snd_soc_common_priv->codec_sysclk_set)
> +			snd_soc_dai_set_sysclk(cpu_dai,
> +					       0, mclk_freq,
> +					       SND_SOC_CLOCK_IN);

[Severity: Medium]
Should this call configure the system clock for the codec_dai instead of the
cpu_dai?

When the codec_sysclk_set flag is true, it appears the intent is to configure
the external codec's clock. However, applying it to the cpu_dai pointer might
leave the external codec unconfigured while inadvertently assigning an
unintended clock configuration to the CPU DAI.

> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
[ ... ]
> @@ -145,37 +243,116 @@ static int sc8280xp_platform_probe(struct platform_device *pdev)
[ ... ]
> +	data->snd_soc_common_priv = (struct snd_soc_common *)of_device_get_match_data(dev);
> +	if (!data->snd_soc_common_priv)
> +		return -ENOMEM;

[Severity: Low]
Would it be better to return -EINVAL or -ENODEV here instead of -ENOMEM?

A missing device tree match or absent platform data does not seem like a memory
allocation failure, and returning -ENOMEM might create confusing kernel logs
during debugging.

> +
> +	card->owner = THIS_MODULE;
>  	card->dev = dev;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608023011.942228-1-mohammad.rafi.shaik@oss.qualcomm.com?part=3