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

[Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>]

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.