Re: [PATCH v1 5/5] ASoC: qcom: sc8280xp: Add Nord Ride sound card support

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

On 9/9/2026 9:07 PM, Shawn Guo wrote:
> On Mon, Sep 07, 2026 at 11:39:46PM +0530, Mohammad Rafi Shaik wrote:
>> Add support for the Nord Ride sound card by introducing a new
>> compatible string and associated platform private data.
>>
>> Nord uses external audio codecs with interface requirements that
>> differ from the generic SC8280XP platforms. Add a dedicated
>> hw_params callback to configure the appropriate DAI format for
>> playback and capture streams and to program the codec system clock
>> configuration required by the board.
>>
>> The callback configures the PCM1681 playback path to operate in I2S
>> mode and the ADAU1979 capture path to operate in DSP_A mode. For
>> capture, the ADAU1979 system clock is sourced from LRCLK as
>> required by the hardware design.
>>
>> Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
>> ---
>>   sound/soc/qcom/sc8280xp.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 57 insertions(+)
>>
>> diff --git a/sound/soc/qcom/sc8280xp.c b/sound/soc/qcom/sc8280xp.c
>> index 4d48e1012cd4..4d8eafca4b7f 100644
>> --- a/sound/soc/qcom/sc8280xp.c
>> +++ b/sound/soc/qcom/sc8280xp.c
>> @@ -18,6 +18,7 @@
>>   #include "common.h"
>>   #include "sdw.h"
>>   
>> +#define LRCLK_SYSCLK 1
> 
> Rather than open-coding the value, please move
> 
>    enum adau1977_clk_id;
>    enum adau1977_sysclk_src;
> 
> out of sound/soc/codecs/adau1977.h into include/sound/adau1977.h so
> machine drivers can include them, and write the call as
>


Ack, thanks for pointing this out. I will rework this in the next revision.

There is no need to introduce a new include/sound/adau1977.h header for 
this. Instead, I will define the required enums locally in the machine 
driver and use them accordingly.


>    snd_soc_component_set_sysclk(codec_dai->component,
>                                 ADAU1977_SYSCLK, ADAU1977_SYSCLK_SRC_LRCLK,
>                                 rate, SND_SOC_CLOCK_IN);
> 
> That documents both slots and turns a mix-up into a compile error.
> 

ACk, will check and update in next revision,


>>   #define I2S_MCLKFS 256
>>   
>>   #define I2S_MCLK_RATE(rate) \
>> @@ -72,6 +73,8 @@ struct qcom_snd_soc_common {
>>   	bool mi2s_bclk_enable;
>>   	bool wcd_jack;
>>   	int (*snd_prepare)(struct snd_pcm_substream *substream);
>> +	int (*snd_hw_params)(struct snd_pcm_substream *substream,
>> +			     struct snd_pcm_hw_params *params);
>>   };
>>   
>>   struct sc8280xp_snd_data {
>> @@ -244,6 +247,47 @@ static int sc8280xp_be_hw_params_fixup(struct snd_soc_pcm_runtime *rtd,
>>   	return 0;
>>   }
>>   
>> +static int nord_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);
>> +	int rate = params_rate(params);
>> +	int ret;
>> +
>> +	switch (cpu_dai->id) {
>> +	case TERTIARY_MI2S_RX:
>> +		ret = snd_soc_dai_set_fmt(codec_dai,
>> +					  SND_SOC_DAIFMT_CBC_CFC |
>> +					  SND_SOC_DAIFMT_NB_NF |
>> +					  SND_SOC_DAIFMT_I2S);
>> +		if (ret && ret != -ENOTSUPP)
>> +			return ret;
>> +
>> +		break;
>> +	case TERTIARY_TDM_TX_7:
>> +		ret = snd_soc_dai_set_fmt(codec_dai,
>> +					  SND_SOC_DAIFMT_CBC_CFC |
>> +					  SND_SOC_DAIFMT_NB_NF |
>> +					  SND_SOC_DAIFMT_DSP_A);
>> +		if (ret && ret != -ENOTSUPP)
>> +			return ret;
>> +
>> +		/* adau1979 MCLK sourced from LRCLK */
> 
> The comment says MCLK is sourced from LRCLK, but the adau1977_set_sysclk()
> call selects the PLL input (ADAU1977_PLL_CLK_S) -- MCLK is the other choice.
> Something like "PLL clocked from LRCLK, no external MCLK" would match
> the register write?
> 

yes your right,

will update the comment something like below.

"adau1979 PLL clock sourced from LRCLK, no external MCLK".


>> +		ret = snd_soc_component_set_sysclk(codec_dai->component,
>> +						   0, LRCLK_SYSCLK,
>> +						   rate, SND_SOC_CLOCK_IN);
> 
> There seems to be two problems, one functional and one cosmetic.
> 
> First, I guess this is called too late to have the intended effect. On
> the ADAU1977 side, set_sysclk() doesn't only pick the clock source, it
> also computes the rate constraint mask:
> 
>    } else if (source == ADAU1977_SYSCLK_SRC_LRCLK) {
>            mask = ADAU1977_RATE_CONSTRAINT_MASK_LRCLK;
>    }
>    ...
>    adau1977->constraints.mask = mask;
> 
> and that mask is consumed in adau1977_startup():
> 
>    snd_pcm_hw_constraint_list(substream->runtime, 0,
>            SNDRV_PCM_HW_PARAM_RATE, &adau1977->constraints);
> 
> startup() runs before hw_params(), so on the first capture open after
> boot the mask is still 0 from probe and the rate constraint is not what
> the LRCLK-sourced configuration requires. Setting the source once from a
> dai_link init (or from a startup callback) rather than per-hw_params
> would get the ordering right and also avoid reprogramming the PLL source
> on every stream open. Could you confirm what the first arecord after
> ot negotiates? I suspect it only appears to work because the rate you
> test with is permitted by the unconstrained list anyway.
> 
> Second, on the naming. The prototype is
> 
>    int snd_soc_component_set_sysclk(struct snd_soc_component *component,
>                                     int clk_id, int source,
>                                     unsigned int freq, int dir);
> 
> so in the call above the literal 0 is the clk_id (ADAU1977_SYSCLK) and
> LRCLK_SYSCLK is the source (ADAU1977_SYSCLK_SRC_LRCLK). The macro name
> reads like a clk_id, which is the slot it is *not* in, while the argument
> that really is a clk_id is an unexplained 0.
> 


Thanks for pointing this out.

I'll verify the first capture-open behaviour and revisit the 
initialization sequence.

The current set_sysclk() call is in hw_params() because ADAU1979 is 
configured with LRCLK as its source, and the LRCLK frequency depends on 
the selected sample rate rather than being fixed.

I'll investigate whether the source selection can be initialized earlier 
while maintaining the runtime rate-dependent configuration.


>> +		if (ret && ret != -ENOTSUPP)
>> +			return ret;
>> +		break;
>> +	default:
>> +		break;
>> +	};
> 
> Stray semicolon
>

Will fix in next revision.


>> +
>> +	return 0;
>> +}
>> +
>>   static int sc8280xp_snd_hw_params(struct snd_pcm_substream *substream,
>>   				  struct snd_pcm_hw_params *params)
>>   {
>> @@ -255,6 +299,12 @@ static int sc8280xp_snd_hw_params(struct snd_pcm_substream *substream,
>>   	int bclk_freq = sc8280xp_get_bclk_freq(params);
>>   	int ret;
>>   
>> +	if (data->priv->snd_hw_params) {
>> +		ret = data->priv->snd_hw_params(substream, params);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>   	switch (cpu_dai->id) {
>>   	case PRIMARY_MI2S_RX ... QUATERNARY_MI2S_TX:
>>   	case QUINARY_MI2S_RX ... QUINARY_MI2S_TX:
>> @@ -461,6 +511,12 @@ static const struct qcom_snd_soc_common kaanapali_priv_data = {
>>   	.wcd_jack = true,
>>   };
>>   
>> +static const struct qcom_snd_soc_common nord_ride_priv_data = {
> 
> Again, can we drop "ride" to make it nord specific?
> 

Ack, will use the qcom,nord-sndcard.

Thanks & Regards,
Rafi.

> Shawn
> 
>> +	.driver_name = "nord",
>> +	.mi2s_bclk_enable = true,
>> +	.snd_hw_params = nord_snd_hw_params,
>> +};
>> +
>>   static const struct qcom_snd_soc_common qcs9100_priv_data = {
>>   	.driver_name = "sa8775p",
>>   	.dapm_widgets = sc8280xp_dapm_widgets,
>> @@ -564,6 +620,7 @@ static const struct of_device_id snd_sc8280xp_dt_match[] = {
>>   	{ .compatible = "qcom,hawi-sndcard", .data = &hawi_priv_data },
>>   	{ .compatible = "qcom,kaanapali-sndcard", .data = &kaanapali_priv_data },
>>   	{ .compatible = "qcom,maili-sndcard", .data = &hawi_priv_data },
>> +	{ .compatible = "qcom,nord-ride-sndcard", .data = &nord_ride_priv_data },
>>   	{ .compatible = "qcom,qcm6490-idp-sndcard", .data = &qcm6490_priv_data },
>>   	{ .compatible = "qcom,qcs615-sndcard", .data = &qcs615_priv_data },
>>   	{ .compatible = "qcom,qcs6490-rb3gen2-sndcard", .data = &qcs6490_priv_data },
>>
>> -- 
>> 2.34.1
>>
>>