[PATCH] ASoC: Intel: sof_rt5682: remove SOF_RT1015_SPEAKER_AMP_100FS flag

[PATCH] ASoC: Intel: sof_rt5682: remove SOF_RT1015_SPEAKER_AMP_100FS flag

[Brent Lu]

This flag could be removed since we now have API to query bclk
fequency setting in the topology. The dai link structure itself also
provides DAI format information instead of figuring it out with fs
number.

Signed-off-by: Brent Lu <brent.lu@intel.com>
---
 sound/soc/intel/boards/sof_realtek_common.c | 86 +++++++++++----------
 sound/soc/intel/boards/sof_realtek_common.h |  2 +-
 sound/soc/intel/boards/sof_rt5682.c         |  6 +-
 3 files changed, 47 insertions(+), 47 deletions(-)

diff --git a/sound/soc/intel/boards/sof_realtek_common.c b/sound/soc/intel/boards/sof_realtek_common.c
index b9643ca2e2f2..ff2851fc8930 100644
--- a/sound/soc/intel/boards/sof_realtek_common.c
+++ b/sound/soc/intel/boards/sof_realtek_common.c
@@ -253,63 +253,70 @@ EXPORT_SYMBOL_NS(sof_rt1015p_codec_conf, SND_SOC_INTEL_SOF_REALTEK_COMMON);
  * RT1015 audio amplifier
  */
 
+static const struct {
+	unsigned int tx;
+	unsigned int rx;
+} rt1015_tdm_mask[] = {
+	{.tx = 0x0, .rx = 0x1},
+	{.tx = 0x0, .rx = 0x2},
+};
+
 static int rt1015_hw_params(struct snd_pcm_substream *substream,
 			    struct snd_pcm_hw_params *params)
 {
 	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+	struct snd_soc_dai_link *dai_link = rtd->dai_link;
 	struct snd_soc_dai *codec_dai;
-	int i, fs = 64, ret;
+	int i, clk_freq, ret;
 
-	for_each_rtd_codec_dais(rtd, i, codec_dai) {
-		ret = snd_soc_dai_set_pll(codec_dai, 0, RT1015_PLL_S_BCLK,
-					  params_rate(params) * fs,
-					  params_rate(params) * 256);
-		if (ret)
-			return ret;
+	clk_freq = sof_dai_get_bclk(rtd);
 
-		ret = snd_soc_dai_set_sysclk(codec_dai, RT1015_SCLK_S_PLL,
-					     params_rate(params) * 256,
-					     SND_SOC_CLOCK_IN);
-		if (ret)
-			return ret;
+	if (clk_freq <= 0) {
+		dev_err(rtd->dev, "fail to get bclk freq, ret %d\n", clk_freq);
+		return -EINVAL;
 	}
 
-	return 0;
-}
-
-static int rt1015_hw_params_pll_and_tdm(struct snd_pcm_substream *substream,
-					 struct snd_pcm_hw_params *params)
-{
-	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
-	struct snd_soc_dai *codec_dai;
-	int i, fs = 100, ret;
-
 	for_each_rtd_codec_dais(rtd, i, codec_dai) {
 		ret = snd_soc_dai_set_pll(codec_dai, 0, RT1015_PLL_S_BCLK,
-					  params_rate(params) * fs,
+					  clk_freq,
 					  params_rate(params) * 256);
-		if (ret)
+		if (ret) {
+			dev_err(codec_dai->dev, "fail to set pll, ret %d\n",
+				ret);
 			return ret;
+		}
 
 		ret = snd_soc_dai_set_sysclk(codec_dai, RT1015_SCLK_S_PLL,
 					     params_rate(params) * 256,
 					     SND_SOC_CLOCK_IN);
-		if (ret)
+		if (ret) {
+			dev_err(codec_dai->dev, "fail to set sysclk, ret %d\n",
+				ret);
 			return ret;
-	}
-	/* rx slot 1 for RT1015_DEV0_NAME */
-	ret = snd_soc_dai_set_tdm_slot(asoc_rtd_to_codec(rtd, 0),
-				       0x0, 0x1, 4, 24);
-	if (ret)
-		return ret;
+		}
 
-	/* rx slot 2 for RT1015_DEV1_NAME */
-	ret = snd_soc_dai_set_tdm_slot(asoc_rtd_to_codec(rtd, 1),
-				       0x0, 0x2, 4, 24);
-	if (ret)
-		return ret;
+		switch (dai_link->dai_fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+		case SND_SOC_DAIFMT_DSP_A:
+		case SND_SOC_DAIFMT_DSP_B:
+			/* 4-slot TDM */
+			ret = snd_soc_dai_set_tdm_slot(codec_dai,
+						       rt1015_tdm_mask[i].tx,
+						       rt1015_tdm_mask[i].rx,
+						       4,
+						       params_width(params));
+			if (ret < 0) {
+				dev_err(codec_dai->dev, "fail to set tdm slot, ret %d\n",
+					ret);
+				return ret;
+			}
+			break;
+		default:
+			dev_dbg(codec_dai->dev, "codec is in I2S mode\n");
+			break;
+		}
+	}
 
-	return 0;
+	return ret;
 }
 
 static struct snd_soc_ops rt1015_ops = {
@@ -351,15 +358,12 @@ void sof_rt1015_codec_conf(struct snd_soc_card *card)
 }
 EXPORT_SYMBOL_NS(sof_rt1015_codec_conf, SND_SOC_INTEL_SOF_REALTEK_COMMON);
 
-void sof_rt1015_dai_link(struct snd_soc_dai_link *link, unsigned int fs)
+void sof_rt1015_dai_link(struct snd_soc_dai_link *link)
 {
 	link->codecs = rt1015_components;
 	link->num_codecs = ARRAY_SIZE(rt1015_components);
 	link->init = speaker_codec_init_lr;
 	link->ops = &rt1015_ops;
-
-	if (fs == 100)
-		rt1015_ops.hw_params = rt1015_hw_params_pll_and_tdm;
 }
 EXPORT_SYMBOL_NS(sof_rt1015_dai_link, SND_SOC_INTEL_SOF_REALTEK_COMMON);
 
diff --git a/sound/soc/intel/boards/sof_realtek_common.h b/sound/soc/intel/boards/sof_realtek_common.h
index 778443421090..3ae99d8239e0 100644
--- a/sound/soc/intel/boards/sof_realtek_common.h
+++ b/sound/soc/intel/boards/sof_realtek_common.h
@@ -32,7 +32,7 @@ void sof_rt1015p_codec_conf(struct snd_soc_card *card);
 #define RT1015_DEV0_NAME	"i2c-10EC1015:00"
 #define RT1015_DEV1_NAME	"i2c-10EC1015:01"
 
-void sof_rt1015_dai_link(struct snd_soc_dai_link *link, unsigned int fs);
+void sof_rt1015_dai_link(struct snd_soc_dai_link *link);
 void sof_rt1015_codec_conf(struct snd_soc_card *card);
 
 #define RT1308_CODEC_DAI	"rt1308-aif"
diff --git a/sound/soc/intel/boards/sof_rt5682.c b/sound/soc/intel/boards/sof_rt5682.c
index 045965312245..7d54b8ed3e83 100644
--- a/sound/soc/intel/boards/sof_rt5682.c
+++ b/sound/soc/intel/boards/sof_rt5682.c
@@ -46,7 +46,6 @@
 	((quirk << SOF_RT5682_NUM_HDMIDEV_SHIFT) & SOF_RT5682_NUM_HDMIDEV_MASK)
 #define SOF_RT1011_SPEAKER_AMP_PRESENT		BIT(13)
 #define SOF_RT1015_SPEAKER_AMP_PRESENT		BIT(14)
-#define SOF_RT1015_SPEAKER_AMP_100FS		BIT(15)
 #define SOF_RT1015P_SPEAKER_AMP_PRESENT		BIT(16)
 #define SOF_MAX98373_SPEAKER_AMP_PRESENT	BIT(17)
 #define SOF_MAX98360A_SPEAKER_AMP_PRESENT	BIT(18)
@@ -132,7 +131,6 @@ static const struct dmi_system_id sof_rt5682_quirk_table[] = {
 					SOF_RT5682_SSP_CODEC(0) |
 					SOF_SPEAKER_AMP_PRESENT |
 					SOF_RT1015_SPEAKER_AMP_PRESENT |
-					SOF_RT1015_SPEAKER_AMP_100FS |
 					SOF_RT5682_SSP_AMP(1)),
 	},
 	{
@@ -739,8 +737,7 @@ static struct snd_soc_dai_link *sof_card_dai_links_create(struct device *dev,
 
 		links[id].id = id;
 		if (sof_rt5682_quirk & SOF_RT1015_SPEAKER_AMP_PRESENT) {
-			sof_rt1015_dai_link(&links[id], (sof_rt5682_quirk &
-					SOF_RT1015_SPEAKER_AMP_100FS) ? 100 : 64);
+			sof_rt1015_dai_link(&links[id]);
 		} else if (sof_rt5682_quirk & SOF_RT1015P_SPEAKER_AMP_PRESENT) {
 			sof_rt1015p_dai_link(&links[id]);
 		} else if (sof_rt5682_quirk & SOF_RT1019_SPEAKER_AMP_PRESENT) {
@@ -1010,7 +1007,6 @@ static const struct platform_device_id board_ids[] = {
 					SOF_RT5682_SSP_CODEC(0) |
 					SOF_SPEAKER_AMP_PRESENT |
 					SOF_RT1015_SPEAKER_AMP_PRESENT |
-					SOF_RT1015_SPEAKER_AMP_100FS |
 					SOF_RT5682_SSP_AMP(1)),
 	},
 	{
-- 
2.25.1

Re: [PATCH] ASoC: Intel: sof_rt5682: remove SOF_RT1015_SPEAKER_AMP_100FS flag

[Pierre-Louis Bossart]

> -static int rt1015_hw_params_pll_and_tdm(struct snd_pcm_substream *substream,
> -					 struct snd_pcm_hw_params *params)
> -{
> -	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> -	struct snd_soc_dai *codec_dai;
> -	int i, fs = 100, ret;
> -
>  	for_each_rtd_codec_dais(rtd, i, codec_dai) {
>  		ret = snd_soc_dai_set_pll(codec_dai, 0, RT1015_PLL_S_BCLK,
> -					  params_rate(params) * fs,
> +					  clk_freq,
>  					  params_rate(params) * 256);
> -		if (ret)
> +		if (ret) {
> +			dev_err(codec_dai->dev, "fail to set pll, ret %d\n",
> +				ret);
>  			return ret;
> +		}
>  
>  		ret = snd_soc_dai_set_sysclk(codec_dai, RT1015_SCLK_S_PLL,
>  					     params_rate(params) * 256,
>  					     SND_SOC_CLOCK_IN);
> -		if (ret)
> +		if (ret) {
> +			dev_err(codec_dai->dev, "fail to set sysclk, ret %d\n",
> +				ret);
>  			return ret;
> -	}
> -	/* rx slot 1 for RT1015_DEV0_NAME */
> -	ret = snd_soc_dai_set_tdm_slot(asoc_rtd_to_codec(rtd, 0),
> -				       0x0, 0x1, 4, 24);
> -	if (ret)
> -		return ret;
> +		}
>  
> -	/* rx slot 2 for RT1015_DEV1_NAME */
> -	ret = snd_soc_dai_set_tdm_slot(asoc_rtd_to_codec(rtd, 1),
> -				       0x0, 0x2, 4, 24);
> -	if (ret)
> -		return ret;
> +		switch (dai_link->dai_fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> +		case SND_SOC_DAIFMT_DSP_A:
> +		case SND_SOC_DAIFMT_DSP_B:
> +			/* 4-slot TDM */
> +			ret = snd_soc_dai_set_tdm_slot(codec_dai,
> +						       rt1015_tdm_mask[i].tx,
> +						       rt1015_tdm_mask[i].rx,
> +						       4,
> +						       params_width(params));

The changes look ok, just wondering if we can avoid hard-coding those 4
values. Can we not get the number of TDM slots from topology and/or
dailink configuration?


> +			if (ret < 0) {
> +				dev_err(codec_dai->dev, "fail to set tdm slot, ret %d\n",
> +					ret);
> +				return ret;
> +			}
> +			break;
> +		default:
> +			dev_dbg(codec_dai->dev, "codec is in I2S mode\n");
> +			break;
> +		}
> +	}
>  
> -	return 0;
> +	return ret;

RE: [PATCH] ASoC: Intel: sof_rt5682: remove SOF_RT1015_SPEAKER_AMP_100FS flag

[Lu, Brent]

> > +		case SND_SOC_DAIFMT_DSP_A:
> > +		case SND_SOC_DAIFMT_DSP_B:
> > +			/* 4-slot TDM */
> > +			ret = snd_soc_dai_set_tdm_slot(codec_dai,
> > +						       rt1015_tdm_mask[i].tx,
> > +						       rt1015_tdm_mask[i].rx,
> > +						       4,
> > +						       params_width(params));
> 
> The changes look ok, just wondering if we can avoid hard-coding those 4 values.
> Can we not get the number of TDM slots from topology and/or dailink
> configuration?
> 
> 

I think TDM slot number is possible but not TX/RX mask. What we have in topology
is union of tx/rx mask of all channels. We don't know the mask of specific channel in
DAI_CONFIG.

DAI_CONFIG(SSP, 0, BOARD_HP_BE_ID, SSP0-Codec,
        SSP_CONFIG(I2S, SSP_CLOCK(mclk, 19200000, codec_mclk_in),
                SSP_CLOCK(bclk, 2400000, codec_slave),
                SSP_CLOCK(fsync, 48000, codec_slave),
                SSP_TDM(2, 25, 3, 3),
                SSP_CONFIG_DATA(SSP, 0, 24, 0, 0, 0, SSP_CC_BCLK_ES)))')

Regards,
Brent

Re: [PATCH] ASoC: Intel: sof_rt5682: remove SOF_RT1015_SPEAKER_AMP_100FS flag

[Pierre-Louis Bossart]

On 9/27/22 15:16, Lu, Brent wrote:
>>> +		case SND_SOC_DAIFMT_DSP_A:
>>> +		case SND_SOC_DAIFMT_DSP_B:
>>> +			/* 4-slot TDM */
>>> +			ret = snd_soc_dai_set_tdm_slot(codec_dai,
>>> +						       rt1015_tdm_mask[i].tx,
>>> +						       rt1015_tdm_mask[i].rx,
>>> +						       4,
>>> +						       params_width(params));
>>
>> The changes look ok, just wondering if we can avoid hard-coding those 4 values.
>> Can we not get the number of TDM slots from topology and/or dailink
>> configuration?
>>
>>
> 
> I think TDM slot number is possible but not TX/RX mask. What we have in topology
> is union of tx/rx mask of all channels. We don't know the mask of specific channel in
> DAI_CONFIG.
> 
> DAI_CONFIG(SSP, 0, BOARD_HP_BE_ID, SSP0-Codec,
>         SSP_CONFIG(I2S, SSP_CLOCK(mclk, 19200000, codec_mclk_in),
>                 SSP_CLOCK(bclk, 2400000, codec_slave),
>                 SSP_CLOCK(fsync, 48000, codec_slave),
>                 SSP_TDM(2, 25, 3, 3),

the 3 3 is precisely the channel mask!

>                 SSP_CONFIG_DATA(SSP, 0, 24, 0, 0, 0, SSP_CC_BCLK_ES)))')
> 
> Regards,
> Brent
> 

RE: [PATCH] ASoC: Intel: sof_rt5682: remove SOF_RT1015_SPEAKER_AMP_100FS flag

[Lu, Brent]

> >
> > I think TDM slot number is possible but not TX/RX mask. What we have
> > in topology is union of tx/rx mask of all channels. We don't know the
> > mask of specific channel in DAI_CONFIG.
> >
> > DAI_CONFIG(SSP, 0, BOARD_HP_BE_ID, SSP0-Codec,
> >         SSP_CONFIG(I2S, SSP_CLOCK(mclk, 19200000, codec_mclk_in),
> >                 SSP_CLOCK(bclk, 2400000, codec_slave),
> >                 SSP_CLOCK(fsync, 48000, codec_slave),
> >                 SSP_TDM(2, 25, 3, 3),
> 
> the 3 3 is precisely the channel mask!
> 

Yes, but what we need in the hw_params() is the mask for individual channel,
like 0x1 for left and 0x2 for right.

Regards,
Brent

Re: [PATCH] ASoC: Intel: sof_rt5682: remove SOF_RT1015_SPEAKER_AMP_100FS flag

[Pierre-Louis Bossart]

On 9/29/22 08:25, Lu, Brent wrote:
>>>
>>> I think TDM slot number is possible but not TX/RX mask. What we have
>>> in topology is union of tx/rx mask of all channels. We don't know the
>>> mask of specific channel in DAI_CONFIG.
>>>
>>> DAI_CONFIG(SSP, 0, BOARD_HP_BE_ID, SSP0-Codec,
>>>         SSP_CONFIG(I2S, SSP_CLOCK(mclk, 19200000, codec_mclk_in),
>>>                 SSP_CLOCK(bclk, 2400000, codec_slave),
>>>                 SSP_CLOCK(fsync, 48000, codec_slave),
>>>                 SSP_TDM(2, 25, 3, 3),
>>
>> the 3 3 is precisely the channel mask!
>>
> 
> Yes, but what we need in the hw_params() is the mask for individual channel,
> like 0x1 for left and 0x2 for right.

Ah yes, you're talking about the mask configuration on the codec side,
sorry I was confused with your reference to DAI_CONFIG. That would be
platform-level information, not DSP topology information indeed.

Sounds good then, thanks for the explanations.

Acked-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Re: [PATCH] ASoC: Intel: sof_rt5682: remove SOF_RT1015_SPEAKER_AMP_100FS flag

[Mark Brown]

On Tue, 13 Sep 2022 15:49:06 +0800, Brent Lu wrote:
> This flag could be removed since we now have API to query bclk
> fequency setting in the topology. The dai link structure itself also
> provides DAI format information instead of figuring it out with fs
> number.
> 
> 

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: Intel: sof_rt5682: remove SOF_RT1015_SPEAKER_AMP_100FS flag
      commit: 4157155df7d34bd91879c06a787944529f0d9a0d

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark