[v2,3/5] ASoC: amd: acp: Refactor i2s clocks programming sequence

Commit Message

Venkata Prasad Potturu Jan. 9, 2023, 1:21 p.m. UTC

Refactor i2s clock values based on hw_params and use srate and
bclk_ratio variables in clock enable API instead hard-code values.

Signed-off-by: Venkata Prasad Potturu <[e-mail]>
---
 sound/soc/amd/acp/acp-mach-common.c | 169 ++++++++++++++++------------
 1 file changed, 94 insertions(+), 75 deletions(-)

Comments

Pierre-Louis Bossart Jan. 25, 2023, 5:15 p.m. UTC | #1

This patch adds new Sparse warnings [1]:

sound/soc/amd/acp/acp-mach-common.c:189:35: error: restricted
snd_pcm_format_t degrades to integer
sound/soc/amd/acp/acp-mach-common.c:333:35: error: restricted
snd_pcm_format_t degrades to integer
sound/soc/amd/acp/acp-mach-common.c:478:35: error: restricted
snd_pcm_format_t degrades to integer
sound/soc/amd/acp/acp-mach-common.c:619:35: error: restricted
snd_pcm_format_t degrades to integer

> @@ -193,7 +163,11 @@ static int acp_card_rt5682_hw_params(struct snd_pcm_substream *substream,
>  	struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
>  	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
>  	int ret;
> -	unsigned int fmt;
> +	unsigned int fmt, srate, ch, format;
> +
> +	srate = params_rate(params);
> +	ch = params_channels(params);
> +	format = 8 * params_format(params);

This last line looks suspicious, no? format-to-physical-size conversions
should be using existing macros.

Should it be

format = params_physical_width(params);

?


> +	/* Set tdm/i2s1 master bclk ratio */
> +	ret = snd_soc_dai_set_bclk_ratio(codec_dai, ch * format);
> +	if (ret < 0) {
> +		dev_err(rtd->dev, "Failed to set rt5682 tdm bclk ratio: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (!drvdata->soc_mclk) {
> +		ret = acp_clk_enable(drvdata, srate, ch * format);
> +		if (ret < 0) {
> +			dev_err(rtd->card->dev, "Failed to enable HS clk: %d\n", ret);
> +			return ret;
> +		}

[1]
https://github.com/thesofproject/linux/actions/runs/4005001249/jobs/6874834205

Peter Ujfalusi Jan. 26, 2023, 7:54 a.m. UTC | #2

On 25/01/2023 19:15, Pierre-Louis Bossart wrote:
> This patch adds new Sparse warnings [1]:
> 
> sound/soc/amd/acp/acp-mach-common.c:189:35: error: restricted
> snd_pcm_format_t degrades to integer
> sound/soc/amd/acp/acp-mach-common.c:333:35: error: restricted
> snd_pcm_format_t degrades to integer
> sound/soc/amd/acp/acp-mach-common.c:478:35: error: restricted
> snd_pcm_format_t degrades to integer
> sound/soc/amd/acp/acp-mach-common.c:619:35: error: restricted
> snd_pcm_format_t degrades to integer
> 
>> @@ -193,7 +163,11 @@ static int acp_card_rt5682_hw_params(struct snd_pcm_substream *substream,
>>  	struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
>>  	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
>>  	int ret;
>> -	unsigned int fmt;
>> +	unsigned int fmt, srate, ch, format;
>> +
>> +	srate = params_rate(params);
>> +	ch = params_channels(params);
>> +	format = 8 * params_format(params);
> 
> This last line looks suspicious, no? format-to-physical-size conversions
> should be using existing macros.

not only that, but the "8 * params_format(params)" is certainly not
correct for the purpose.

params_format() returns SNDRV_PCM_FORMAT_*, which is basically an enum:
SNDRV_PCM_FORMAT_S8		0
SNDRV_PCM_FORMAT_S16_LE		2
SNDRV_PCM_FORMAT_S24_LE		6
SNDRV_PCM_FORMAT_S24_3LE	32
SNDRV_PCM_FORMAT_S32_LE		10

include/uapi/sound/asound.h

> Should it be
> 
> format = params_physical_width(params);

I believe this was the intention.

> 
> ?
> 
> 
>> +	/* Set tdm/i2s1 master bclk ratio */
>> +	ret = snd_soc_dai_set_bclk_ratio(codec_dai, ch * format);
>> +	if (ret < 0) {
>> +		dev_err(rtd->dev, "Failed to set rt5682 tdm bclk ratio: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	if (!drvdata->soc_mclk) {
>> +		ret = acp_clk_enable(drvdata, srate, ch * format);
>> +		if (ret < 0) {
>> +			dev_err(rtd->card->dev, "Failed to enable HS clk: %d\n", ret);
>> +			return ret;
>> +		}
> 
> [1]
> https://github.com/thesofproject/linux/actions/runs/4005001249/jobs/6874834205

Venkata Prasad Potturu Jan. 30, 2023, 5:56 a.m. UTC | #3

On 1/25/23 22:45, Pierre-Louis Bossart wrote:
Thanks for your time Louis.
> This patch adds new Sparse warnings [1]:
>
> sound/soc/amd/acp/acp-mach-common.c:189:35: error: restricted
> snd_pcm_format_t degrades to integer
> sound/soc/amd/acp/acp-mach-common.c:333:35: error: restricted
> snd_pcm_format_t degrades to integer
> sound/soc/amd/acp/acp-mach-common.c:478:35: error: restricted
> snd_pcm_format_t degrades to integer
> sound/soc/amd/acp/acp-mach-common.c:619:35: error: restricted
> snd_pcm_format_t degrades to integer
>
>> @@ -193,7 +163,11 @@ static int acp_card_rt5682_hw_params(struct snd_pcm_substream *substream,
>>   	struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
>>   	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
>>   	int ret;
>> -	unsigned int fmt;
>> +	unsigned int fmt, srate, ch, format;
>> +
>> +	srate = params_rate(params);
>> +	ch = params_channels(params);
>> +	format = 8 * params_format(params);
> This last line looks suspicious, no? format-to-physical-size conversions
> should be using existing macros.
>
> Should it be
>
> format = params_physical_width(params);
>
> ?

Yes, we will use params_physical_width(params) and update it in the next 
patch.

Thanks for the information.

>
>
>> +	/* Set tdm/i2s1 master bclk ratio */
>> +	ret = snd_soc_dai_set_bclk_ratio(codec_dai, ch * format);
>> +	if (ret < 0) {
>> +		dev_err(rtd->dev, "Failed to set rt5682 tdm bclk ratio: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	if (!drvdata->soc_mclk) {
>> +		ret = acp_clk_enable(drvdata, srate, ch * format);
>> +		if (ret < 0) {
>> +			dev_err(rtd->card->dev, "Failed to enable HS clk: %d\n", ret);
>> +			return ret;
>> +		}
> [1]
> https://github.com/thesofproject/linux/actions/runs/4005001249/jobs/6874834205

Patch

diff --git a/sound/soc/amd/acp/acp-mach-common.c b/sound/soc/amd/acp/acp-mach-common.c
index 75beb6e41e99..88c194902157 100644
--- a/sound/soc/amd/acp/acp-mach-common.c
+++ b/sound/soc/amd/acp/acp-mach-common.c
@@ -54,10 +54,11 @@  static const struct snd_pcm_hw_constraint_list constraints_channels = {
 	.mask = 0,
 };
 
-static int acp_clk_enable(struct acp_card_drvdata *drvdata)
+static int acp_clk_enable(struct acp_card_drvdata *drvdata,
+			  unsigned int srate, unsigned int bclk_ratio)
 {
-	clk_set_rate(drvdata->wclk, 48000);
-	clk_set_rate(drvdata->bclk, 48000 * 64);
+	clk_set_rate(drvdata->wclk, srate);
+	clk_set_rate(drvdata->bclk, srate * bclk_ratio);
 
 	return clk_prepare_enable(drvdata->wclk);
 }
@@ -86,27 +87,6 @@  static int acp_card_rt5682_init(struct snd_soc_pcm_runtime *rtd)
 	if (drvdata->hs_codec_id != RT5682)
 		return -EINVAL;
 
-	ret = snd_soc_dai_set_pll(codec_dai, RT5682_PLL2, RT5682_PLL2_S_MCLK,
-				  PCO_PLAT_CLK, RT5682_PLL_FREQ);
-	if (ret < 0) {
-		dev_err(rtd->dev, "Failed to set codec PLL: %d\n", ret);
-		return ret;
-	}
-
-	ret = snd_soc_dai_set_sysclk(codec_dai, RT5682_SCLK_S_PLL2,
-				     RT5682_PLL_FREQ, SND_SOC_CLOCK_IN);
-	if (ret < 0) {
-		dev_err(rtd->dev, "Failed to set codec SYSCLK: %d\n", ret);
-		return ret;
-	}
-
-	/* Set tdm/i2s1 master bclk ratio */
-	ret = snd_soc_dai_set_bclk_ratio(codec_dai, 64);
-	if (ret < 0) {
-		dev_err(rtd->dev, "Failed to set rt5682 tdm bclk ratio: %d\n", ret);
-		return ret;
-	}
-
 	drvdata->wclk = clk_get(component->dev, "rt5682-dai-wclk");
 	drvdata->bclk = clk_get(component->dev, "rt5682-dai-bclk");
 
@@ -161,16 +141,6 @@  static int acp_card_hs_startup(struct snd_pcm_substream *substream)
 	snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_RATE,
 				      &constraints_rates);
 
-	if (strcmp(codec_dai->name, "rt5682s-aif1") && strcmp(codec_dai->name, "rt5682s-aif2")) {
-		if (!drvdata->soc_mclk) {
-			ret = acp_clk_enable(drvdata);
-			if (ret < 0) {
-				dev_err(rtd->card->dev, "Failed to enable HS clk: %d\n", ret);
-				return ret;
-			}
-		}
-	}
-
 	return ret;
 }
 
@@ -193,7 +163,11 @@  static int acp_card_rt5682_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
 	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
 	int ret;
-	unsigned int fmt;
+	unsigned int fmt, srate, ch, format;
+
+	srate = params_rate(params);
+	ch = params_channels(params);
+	format = 8 * params_format(params);
 
 	if (drvdata->soc_mclk)
 		fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBC_CFC;
@@ -212,6 +186,35 @@  static int acp_card_rt5682_hw_params(struct snd_pcm_substream *substream,
 		return ret;
 	}
 
+	ret = snd_soc_dai_set_pll(codec_dai, RT5682_PLL2, RT5682_PLL2_S_MCLK,
+				  PCO_PLAT_CLK, RT5682_PLL_FREQ);
+	if (ret < 0) {
+		dev_err(rtd->dev, "Failed to set codec PLL: %d\n", ret);
+		return ret;
+	}
+
+	ret = snd_soc_dai_set_sysclk(codec_dai, RT5682_SCLK_S_PLL2,
+				     RT5682_PLL_FREQ, SND_SOC_CLOCK_IN);
+	if (ret < 0) {
+		dev_err(rtd->dev, "Failed to set codec SYSCLK: %d\n", ret);
+		return ret;
+	}
+
+	/* Set tdm/i2s1 master bclk ratio */
+	ret = snd_soc_dai_set_bclk_ratio(codec_dai, ch * format);
+	if (ret < 0) {
+		dev_err(rtd->dev, "Failed to set rt5682 tdm bclk ratio: %d\n", ret);
+		return ret;
+	}
+
+	if (!drvdata->soc_mclk) {
+		ret = acp_clk_enable(drvdata, srate, ch * format);
+		if (ret < 0) {
+			dev_err(rtd->card->dev, "Failed to enable HS clk: %d\n", ret);
+			return ret;
+		}
+	}
+
 	return 0;
 }
 
@@ -244,27 +247,6 @@  static int acp_card_rt5682s_init(struct snd_soc_pcm_runtime *rtd)
 	if (drvdata->hs_codec_id != RT5682S)
 		return -EINVAL;
 
-	ret = snd_soc_dai_set_pll(codec_dai, RT5682S_PLL2, RT5682S_PLL_S_MCLK,
-				  PCO_PLAT_CLK, RT5682_PLL_FREQ);
-	if (ret < 0) {
-		dev_err(rtd->dev, "Failed to set codec PLL: %d\n", ret);
-		return ret;
-	}
-
-	ret = snd_soc_dai_set_sysclk(codec_dai, RT5682S_SCLK_S_PLL2,
-				     RT5682_PLL_FREQ, SND_SOC_CLOCK_IN);
-	if (ret < 0) {
-		dev_err(rtd->dev, "Failed to set codec SYSCLK: %d\n", ret);
-		return ret;
-	}
-
-	/* Set tdm/i2s1 master bclk ratio */
-	ret = snd_soc_dai_set_bclk_ratio(codec_dai, 64);
-	if (ret < 0) {
-		dev_err(rtd->dev, "Failed to set rt5682 tdm bclk ratio: %d\n", ret);
-		return ret;
-	}
-
 	if (!drvdata->soc_mclk) {
 		drvdata->wclk = clk_get(component->dev, "rt5682-dai-wclk");
 		drvdata->bclk = clk_get(component->dev, "rt5682-dai-bclk");
@@ -303,7 +285,11 @@  static int acp_card_rt5682s_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
 	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
 	int ret;
-	unsigned int fmt;
+	unsigned int fmt, srate, ch, format;
+
+	srate = params_rate(params);
+	ch = params_channels(params);
+	format = 8 * params_format(params);
 
 	if (drvdata->soc_mclk)
 		fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBC_CFC;
@@ -322,6 +308,30 @@  static int acp_card_rt5682s_hw_params(struct snd_pcm_substream *substream,
 		return ret;
 	}
 
+	ret = snd_soc_dai_set_pll(codec_dai, RT5682S_PLL2, RT5682S_PLL_S_MCLK,
+				  PCO_PLAT_CLK, RT5682_PLL_FREQ);
+	if (ret < 0) {
+		dev_err(rtd->dev, "Failed to set codec PLL: %d\n", ret);
+		return ret;
+	}
+
+	ret = snd_soc_dai_set_sysclk(codec_dai, RT5682S_SCLK_S_PLL2,
+				     RT5682_PLL_FREQ, SND_SOC_CLOCK_IN);
+	if (ret < 0) {
+		dev_err(rtd->dev, "Failed to set codec SYSCLK: %d\n", ret);
+		return ret;
+	}
+
+	/* Set tdm/i2s1 master bclk ratio */
+	ret = snd_soc_dai_set_bclk_ratio(codec_dai, ch * format);
+	if (ret < 0) {
+		dev_err(rtd->dev, "Failed to set rt5682 tdm bclk ratio: %d\n", ret);
+		return ret;
+	}
+
+	clk_set_rate(drvdata->wclk, srate);
+	clk_set_rate(drvdata->bclk, srate * ch * format);
+
 	return 0;
 }
 
@@ -397,10 +407,12 @@  static int acp_card_rt1019_hw_params(struct snd_pcm_substream *substream,
 	struct acp_card_drvdata *drvdata = card->drvdata;
 	struct snd_soc_dai *codec_dai;
 	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
-	int srate, i, ret = 0;
-	unsigned int fmt;
+	int i, ret = 0;
+	unsigned int fmt, srate, ch, format;
 
 	srate = params_rate(params);
+	ch = params_channels(params);
+	format = 8 * params_format(params);
 
 	if (drvdata->amp_codec_id != RT1019)
 		return -EINVAL;
@@ -421,7 +433,7 @@  static int acp_card_rt1019_hw_params(struct snd_pcm_substream *substream,
 			continue;
 
 		ret = snd_soc_dai_set_pll(codec_dai, 0, RT1019_PLL_S_BCLK,
-					  64 * srate, 256 * srate);
+					  ch * format * srate, 256 * srate);
 		if (ret < 0)
 			return ret;
 
@@ -431,16 +443,20 @@  static int acp_card_rt1019_hw_params(struct snd_pcm_substream *substream,
 			return ret;
 	}
 
+	if (!drvdata->soc_mclk) {
+		ret = acp_clk_enable(drvdata, srate, ch * format);
+		if (ret < 0) {
+			dev_err(rtd->card->dev, "Failed to enable AMP clk: %d\n", ret);
+			return ret;
+		}
+	}
+
 	return 0;
 }
 
 static int acp_card_amp_startup(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
-	struct snd_soc_card *card = rtd->card;
-	struct acp_card_drvdata *drvdata = card->drvdata;
-	int ret = 0;
 
 	runtime->hw.channels_max = DUAL_CHANNEL;
 	snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS,
@@ -448,14 +464,7 @@  static int acp_card_amp_startup(struct snd_pcm_substream *substream)
 	snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_RATE,
 				      &constraints_rates);
 
-	if (!drvdata->soc_mclk) {
-		ret = acp_clk_enable(drvdata);
-		if (ret < 0) {
-			dev_err(rtd->card->dev, "Failed to enable AMP clk: %d\n", ret);
-			return ret;
-		}
-	}
-	return ret;
+	return 0;
 }
 
 static const struct snd_soc_ops acp_card_rt1019_ops = {
@@ -491,9 +500,13 @@  static int acp_card_maxim_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_card *card = rtd->card;
 	struct acp_card_drvdata *drvdata = card->drvdata;
 	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
-	unsigned int fmt;
+	unsigned int fmt, srate, ch, format;
 	int ret;
 
+	srate = params_rate(params);
+	ch = params_channels(params);
+	format = 8 * params_format(params);
+
 	if (drvdata->soc_mclk)
 		fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBC_CFC;
 	else
@@ -504,6 +517,14 @@  static int acp_card_maxim_hw_params(struct snd_pcm_substream *substream,
 		dev_err(rtd->dev, "Failed to set dai fmt: %d\n", ret);
 		return ret;
 	}
+
+	if (!drvdata->soc_mclk) {
+		ret = acp_clk_enable(drvdata, srate, ch * format);
+		if (ret < 0) {
+			dev_err(rtd->card->dev, "Failed to enable AMP clk: %d\n", ret);
+			return ret;
+		}
+	}
 	return 0;
 }
 
@@ -692,8 +713,6 @@  static int acp_rtk_set_bias_level(struct snd_soc_card *card,
 	switch (level) {
 	case SND_SOC_BIAS_STANDBY:
 		if (snd_soc_dapm_get_bias_level(dapm) == SND_SOC_BIAS_OFF) {
-			clk_set_rate(drvdata->wclk, 48000);
-			clk_set_rate(drvdata->bclk, 48000 * 64);
 
 			/* Increase bclk's enable_count */
 			ret = clk_prepare_enable(drvdata->bclk);