Fwd: [PATCH 4/8] ASoC: amd: acp: Add generic machine driver support for ACP cards

[Ajit Kumar Pandey]

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