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

[Shawn Guo <shengchao.guo@oss.qualcomm.com>]

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

  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.

>  #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?

> +		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.

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

Stray semicolon

> +
> +	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?

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
> 
>