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

[Mohammad Rafi Shaik]

On 6/8/2026 5:38 PM, Mark Brown wrote:
> On Mon, Jun 08, 2026 at 08:00:11AM +0530, Mohammad Rafi Shaik wrote:
> > The sc8280xp machine driver is currently written with a largely
> > SoC-centric view and assumes a uniform audio topology across all boards.
>
> > +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:
>
> rate is in Hz but these are bitmasks.

Yes right, thanks for pointing out.

Comparing the sampling rate value from params_rate(params) against 
bitmasks like SNDRV_PCM_RATE_44100 is an error. I will update the 
switch-case to use literal frequency values (e.g., 44100, 88200) or 
appropriate rate constants to ensure the sampling rate is correctly 
identified.

> > +		return I2S_MCLK_RATE(44100);
> > +	default:
> > +		break;
> > +	}
>
> The function only works since it ignores invalid values.

Ack, will fix in next version.

> > +static int sc8280xp_snd_hw_params(struct snd_pcm_substream *substream,
> > +				  struct snd_pcm_hw_params *params)
> > +{
>
> > +
> > +		if (data->snd_soc_common_priv->codec_dai_fmt)
> > +			snd_soc_dai_set_fmt(codec_dai,
> > +					    data->snd_soc_common_priv->codec_dai_fmt);
>
> Should we error check the functions we call here?

Agreed. I will add proper return value checks for snd_soc_dai_set_fmt 
and snd_soc_dai_set_sysclk to ensure any configuration failures are 
caught and reported during hw_params.

> > +		if (data->snd_soc_common_priv->mi2s_mclk_enable)
> > +			snd_soc_dai_set_sysclk(cpu_dai,
> > +					       LPAIF_MI2S_MCLK, mclk_freq,
> > +					       SND_SOC_CLOCK_IN);
> > +
> > +		if (data->snd_soc_common_priv->mi2s_bclk_enable)
> > +			snd_soc_dai_set_sysclk(cpu_dai,
> > +					       LPAIF_MI2S_BCLK, bclk_freq,
> > +					       SND_SOC_CLOCK_IN);
>
> Is SND_SOC_CLK_IN right here?  The flag sounds like it's enabling the
> clock on the DAI but this is configuring the DAI to consume a clock.

Good catch. Since the CPU DAI is providing the MCLK/BCLK to the codec in 
this configuration (Master mode), it should be configured as 
SND_SOC_CLOCK_OUT. I will change these to SND_SOC_CLOCK_OUT to correctly 
reflect that the DAI is the clock source.

> > +		if (data->snd_soc_common_priv->codec_sysclk_set)
> > +			snd_soc_dai_set_sysclk(cpu_dai,
> > +					       0, mclk_freq,
> > +					       SND_SOC_CLOCK_IN);
>
> This is configuring the CPU not CODEC.

will fix typo error. The call should indeed target the codec_dai when 
the codec_sysclk_set flag is enabled. I will fix the function call to 
use the correct DAI pointer.

Thanks & Regards,
Rafi.