Apologies for corrupted reply format, some issue with thunderbird.

We will update all below review comments in v2 patch series

On Thu, Sep 30, 2021 at 06:54:14PM +0530, Ajit Kumar Pandey wrote:

A couple of things here, most of these are probably fine for now other than the EXPORT_SYMBOL but I think you're likely to run into issues going forward and need to refactor.

...

• switch (drvdata->hs_codec_id) {
• case RT5682:

• pll_id = RT5682_PLL2;
  

• pll_src = RT5682_PLL2_S_MCLK;
  

• freq_in = PCO_PLAT_CLK;
  

• freq_out = RT5682_PLL_FREQ;
  

• clk_id = RT5682_SCLK_S_PLL2;
  

• clk_freq = RT5682_PLL_FREQ;
  

• wclk_name = "rt5682-dai-wclk";
  

• bclk_name = "rt5682-dai-bclk";
  

• drvdata->dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF
  

• 		   | SND_SOC_DAIFMT_CBP_CFP;
  

• snd_soc_dapm_add_routes(&rtd->card->dapm, rt5682_map, ARRAY_SIZE(rt5682_map));
  

• break;
  

It feels like this is going to run into scaling issues going forward and you're likely to need separate operations for different CODECs rather than just different IDs. Similar issues apply for the amps, it feels like you want to be passing separate ops in rather than having these switch statements.

Ok will change to use separate ops in v2 patch series instead of switch statements.

...

• /* Do nothing for dummy codec */
• if (!drvdata->hs_codec_id && drvdata->amp_codec_id)

• return;
  

Wha the test seems to say is do nothing if there's no CODEC but there is an amp...

...

• clk_disable_unprepare(drvdata->wclk);

+}

...though I'd expect that given that the clock API accepts NULL clocks you could just remove these checks and unconditionally use the clocks.

Ok will remove this check in v2 patch series.

...

• return 0;

+} +EXPORT_SYMBOL_NS(acp_legacy_dai_links_create, SND_SOC_AMD_MACH);

EXPORT_SYMBOL_GPL_NS() - ASoC is all EXPORT_SYMBOL_GPL

sure will update to use NS_GPL() in v2 patch chain