diff mbox series

[14/15] ASoC: rockchip: i2s: Add support for 'rockchip, clk-trcm' property

Message ID 1629716076-21465-5-git-send-email-sugar.zhang@rock-chips.com (mailing list archive)
State New, archived
Headers show
Series Patches to update for rockchip i2s | expand

Commit Message

Sugar Zhang Aug. 23, 2021, 10:54 a.m. UTC 
From: Xing Zheng <zhengxing@rock-chips.com>

If there is only one lrck (tx or rx) by hardware, we need to
use 'rockchip,clk-trcm' to specify which lrck can be used.

Change-Id: I3bf8d87a6bc8c45e183040012d87d8be21a4c133
Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com>
---
 sound/soc/rockchip/rockchip_i2s.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

Comments

Nicolas Frattaroli Aug. 23, 2021, 11:47 a.m. UTC | #1 
On Montag, 23. August 2021 12:54:35 CEST Sugar Zhang wrote:
> From: Xing Zheng <zhengxing@rock-chips.com>
> 
> If there is only one lrck (tx or rx) by hardware, we need to
> use 'rockchip,clk-trcm' to specify which lrck can be used.
> 
> Change-Id: I3bf8d87a6bc8c45e183040012d87d8be21a4c133
> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
> Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com>
> ---
>  sound/soc/rockchip/rockchip_i2s.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/sound/soc/rockchip/rockchip_i2s.c
> b/sound/soc/rockchip/rockchip_i2s.c index 6ccb62e..b9d9c88 100644
> --- a/sound/soc/rockchip/rockchip_i2s.c
> +++ b/sound/soc/rockchip/rockchip_i2s.c
> @@ -54,6 +54,7 @@ struct rk_i2s_dev {
>  	bool is_master_mode;
>  	const struct rk_i2s_pins *pins;
>  	unsigned int bclk_ratio;
> +	unsigned int clk_trcm;
>  };
> 
>  /* tx/rx ctrl lock */
> @@ -321,7 +322,6 @@ static int rockchip_i2s_hw_params(struct
> snd_pcm_substream *substream, struct snd_soc_dai *dai)
>  {
>  	struct rk_i2s_dev *i2s = to_info(dai);
> -	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
>  	unsigned int val = 0;
>  	unsigned int mclk_rate, bclk_rate, div_bclk, div_lrck;
> 
> @@ -421,13 +421,6 @@ static int rockchip_i2s_hw_params(struct
> snd_pcm_substream *substream, regmap_update_bits(i2s->regmap, I2S_DMACR,
> I2S_DMACR_RDL_MASK,
>  			   I2S_DMACR_RDL(16));
> 
> -	val = I2S_CKR_TRCM_TXRX;
> -	if (dai->driver->symmetric_rate && rtd->dai_link->symmetric_rate)
> -		val = I2S_CKR_TRCM_TXONLY;
> -
> -	regmap_update_bits(i2s->regmap, I2S_CKR,
> -			   I2S_CKR_TRCM_MASK,
> -			   val);
>  	return 0;
>  }
> 
> @@ -531,7 +524,6 @@ static struct snd_soc_dai_driver rockchip_i2s_dai = {
>  			    SNDRV_PCM_FMTBIT_S32_LE),
>  	},
>  	.ops = &rockchip_i2s_dai_ops,
> -	.symmetric_rate = 1,
>  };
> 
>  static const struct snd_soc_component_driver rockchip_i2s_component = {
> @@ -750,6 +742,18 @@ static int rockchip_i2s_probe(struct platform_device
> *pdev) else if (of_property_read_bool(node, "rockchip,capture-only"))
>  		soc_dai->playback.channels_min = 0;
> 
> +	i2s->clk_trcm = I2S_CKR_TRCM_TXRX;
> +	if (!of_property_read_u32(node, "rockchip,clk-trcm", &val)) {
> +		if (val >= 0 && val <= 2) {
> +			i2s->clk_trcm = val << I2S_CKR_TRCM_SHIFT;
> +			if (i2s->clk_trcm)
> +				soc_dai->symmetric_rate = 1;
> +		}
> +	}
> +
> +	regmap_update_bits(i2s->regmap, I2S_CKR,
> +			   I2S_CKR_TRCM_MASK, i2s->clk_trcm);
> +
>  	ret = devm_snd_soc_register_component(&pdev->dev,
>  					      &rockchip_i2s_component,
>  					      soc_dai, 1);

Hello,

I recommend doing the same thing with clk-trcm that I'm going to do in v3 of 
my i2s-tdm driver, as per Robin Murphy's suggestion:

Have tx-only and rx-only be two boolean properties. I named them
rockchip,trcm-sync-tx-only and rockchip,trcm-sync-rx-only.

I also recommend only shifting the value when writing it to registers, and
storing it in its unshifted state for debug reasons.

My probe function looks like this:

	i2s_tdm->clk_trcm = TRCM_TXRX;
	if (of_property_read_bool(node, "rockchip,trcm-sync-tx-only"))
		i2s_tdm->clk_trcm = TRCM_TX;
	if (of_property_read_bool(node, "rockchip,trcm-sync-rx-only")) {
		if (i2s_tdm->clk_trcm) {
			dev_err(i2s_tdm->dev, "invalid trcm-sync 
configuration\n");
			return -EINVAL;
		}
		i2s_tdm->clk_trcm = TRCM_RX;
	}
	if (i2s_tdm->clk_trcm != TRCM_TXRX)
		i2s_tdm_dai.symmetric_rate = 1;

When writing clk_trcm to a register, I then just do:

	regmap_update_bits(i2s_tdm->regmap, I2S_CKR, I2S_CKR_TRCM_MASK,
			   i2s_tdm->clk_trcm << I2S_CKR_TRCM_SHIFT);

This way if I need to add an error message or debug print somewhere, then 
clk_trcm is still either 0, 1 or 2.

In general, we should look into supporting both i2s and i2s-tdm controllers in 
the same driver if possible. This way we don't need to duplicate work like 
this. Do you think this is feasible to do? When I looked at the register maps 
I saw that the bits I2S/TDM uses were reserved in the I2S version of the 
controller, so I think it should work.

Regards,
Nicolas Frattaroli
Sugar Zhang Aug. 24, 2021, 1:41 a.m. UTC | #2 
Hi Nicolas,

On 2021/8/23 19:47, Nicolas Frattaroli wrote:
> On Montag, 23. August 2021 12:54:35 CEST Sugar Zhang wrote:
>> From: Xing Zheng <zhengxing@rock-chips.com>
>>
>> If there is only one lrck (tx or rx) by hardware, we need to
>> use 'rockchip,clk-trcm' to specify which lrck can be used.
>>
>> Change-Id: I3bf8d87a6bc8c45e183040012d87d8be21a4c133
>> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
>> Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com>
>> ---
>>   sound/soc/rockchip/rockchip_i2s.c | 22 +++++++++++++---------
>>   1 file changed, 13 insertions(+), 9 deletions(-)
>>
>> diff --git a/sound/soc/rockchip/rockchip_i2s.c
>> b/sound/soc/rockchip/rockchip_i2s.c index 6ccb62e..b9d9c88 100644
>> --- a/sound/soc/rockchip/rockchip_i2s.c
>> +++ b/sound/soc/rockchip/rockchip_i2s.c
>> @@ -54,6 +54,7 @@ struct rk_i2s_dev {
>>   	bool is_master_mode;
>>   	const struct rk_i2s_pins *pins;
>>   	unsigned int bclk_ratio;
>> +	unsigned int clk_trcm;
>>   };
>>
>>   /* tx/rx ctrl lock */
>> @@ -321,7 +322,6 @@ static int rockchip_i2s_hw_params(struct
>> snd_pcm_substream *substream, struct snd_soc_dai *dai)
>>   {
>>   	struct rk_i2s_dev *i2s = to_info(dai);
>> -	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
>>   	unsigned int val = 0;
>>   	unsigned int mclk_rate, bclk_rate, div_bclk, div_lrck;
>>
>> @@ -421,13 +421,6 @@ static int rockchip_i2s_hw_params(struct
>> snd_pcm_substream *substream, regmap_update_bits(i2s->regmap, I2S_DMACR,
>> I2S_DMACR_RDL_MASK,
>>   			   I2S_DMACR_RDL(16));
>>
>> -	val = I2S_CKR_TRCM_TXRX;
>> -	if (dai->driver->symmetric_rate && rtd->dai_link->symmetric_rate)
>> -		val = I2S_CKR_TRCM_TXONLY;
>> -
>> -	regmap_update_bits(i2s->regmap, I2S_CKR,
>> -			   I2S_CKR_TRCM_MASK,
>> -			   val);
>>   	return 0;
>>   }
>>
>> @@ -531,7 +524,6 @@ static struct snd_soc_dai_driver rockchip_i2s_dai = {
>>   			    SNDRV_PCM_FMTBIT_S32_LE),
>>   	},
>>   	.ops = &rockchip_i2s_dai_ops,
>> -	.symmetric_rate = 1,
>>   };
>>
>>   static const struct snd_soc_component_driver rockchip_i2s_component = {
>> @@ -750,6 +742,18 @@ static int rockchip_i2s_probe(struct platform_device
>> *pdev) else if (of_property_read_bool(node, "rockchip,capture-only"))
>>   		soc_dai->playback.channels_min = 0;
>>
>> +	i2s->clk_trcm = I2S_CKR_TRCM_TXRX;
>> +	if (!of_property_read_u32(node, "rockchip,clk-trcm", &val)) {
>> +		if (val >= 0 && val <= 2) {
>> +			i2s->clk_trcm = val << I2S_CKR_TRCM_SHIFT;
>> +			if (i2s->clk_trcm)
>> +				soc_dai->symmetric_rate = 1;
>> +		}
>> +	}
>> +
>> +	regmap_update_bits(i2s->regmap, I2S_CKR,
>> +			   I2S_CKR_TRCM_MASK, i2s->clk_trcm);
>> +
>>   	ret = devm_snd_soc_register_component(&pdev->dev,
>>   					      &rockchip_i2s_component,
>>   					      soc_dai, 1);
> Hello,
>
> I recommend doing the same thing with clk-trcm that I'm going to do in v3 of
> my i2s-tdm driver, as per Robin Murphy's suggestion:
>
> Have tx-only and rx-only be two boolean properties. I named them
> rockchip,trcm-sync-tx-only and rockchip,trcm-sync-rx-only.
>
> I also recommend only shifting the value when writing it to registers, and
> storing it in its unshifted state for debug reasons.
okay, will do in v2.
> My probe function looks like this:
>
> 	i2s_tdm->clk_trcm = TRCM_TXRX;
> 	if (of_property_read_bool(node, "rockchip,trcm-sync-tx-only"))
> 		i2s_tdm->clk_trcm = TRCM_TX;
> 	if (of_property_read_bool(node, "rockchip,trcm-sync-rx-only")) {
> 		if (i2s_tdm->clk_trcm) {
> 			dev_err(i2s_tdm->dev, "invalid trcm-sync
> configuration\n");
> 			return -EINVAL;
> 		}
> 		i2s_tdm->clk_trcm = TRCM_RX;
> 	}
> 	if (i2s_tdm->clk_trcm != TRCM_TXRX)
> 		i2s_tdm_dai.symmetric_rate = 1;
>
> When writing clk_trcm to a register, I then just do:
>
> 	regmap_update_bits(i2s_tdm->regmap, I2S_CKR, I2S_CKR_TRCM_MASK,
> 			   i2s_tdm->clk_trcm << I2S_CKR_TRCM_SHIFT);
>
> This way if I need to add an error message or debug print somewhere, then
> clk_trcm is still either 0, 1 or 2.
>
> In general, we should look into supporting both i2s and i2s-tdm controllers in
> the same driver if possible. This way we don't need to duplicate work like
> this. Do you think this is feasible to do? When I looked at the register maps
> I saw that the bits I2S/TDM uses were reserved in the I2S version of the
> controller, so I think it should work.

It's possible, but will make the driver much more complicate(a lot of 
'if...else...').

Though the registers were compatible, but the design is totally 
different, such as clks, reset signal,  pins and core logic.

so, split into two drivers to support old controller and new one.

> Regards,
> Nicolas Frattaroli
>
>
>
>
>
diff mbox series

Patch

diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c
index 6ccb62e..b9d9c88 100644
--- a/sound/soc/rockchip/rockchip_i2s.c
+++ b/sound/soc/rockchip/rockchip_i2s.c
@@ -54,6 +54,7 @@  struct rk_i2s_dev {
 	bool is_master_mode;
 	const struct rk_i2s_pins *pins;
 	unsigned int bclk_ratio;
+	unsigned int clk_trcm;
 };
 
 /* tx/rx ctrl lock */
@@ -321,7 +322,6 @@  static int rockchip_i2s_hw_params(struct snd_pcm_substream *substream,
 				  struct snd_soc_dai *dai)
 {
 	struct rk_i2s_dev *i2s = to_info(dai);
-	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
 	unsigned int val = 0;
 	unsigned int mclk_rate, bclk_rate, div_bclk, div_lrck;
 
@@ -421,13 +421,6 @@  static int rockchip_i2s_hw_params(struct snd_pcm_substream *substream,
 	regmap_update_bits(i2s->regmap, I2S_DMACR, I2S_DMACR_RDL_MASK,
 			   I2S_DMACR_RDL(16));
 
-	val = I2S_CKR_TRCM_TXRX;
-	if (dai->driver->symmetric_rate && rtd->dai_link->symmetric_rate)
-		val = I2S_CKR_TRCM_TXONLY;
-
-	regmap_update_bits(i2s->regmap, I2S_CKR,
-			   I2S_CKR_TRCM_MASK,
-			   val);
 	return 0;
 }
 
@@ -531,7 +524,6 @@  static struct snd_soc_dai_driver rockchip_i2s_dai = {
 			    SNDRV_PCM_FMTBIT_S32_LE),
 	},
 	.ops = &rockchip_i2s_dai_ops,
-	.symmetric_rate = 1,
 };
 
 static const struct snd_soc_component_driver rockchip_i2s_component = {
@@ -750,6 +742,18 @@  static int rockchip_i2s_probe(struct platform_device *pdev)
 	else if (of_property_read_bool(node, "rockchip,capture-only"))
 		soc_dai->playback.channels_min = 0;
 
+	i2s->clk_trcm = I2S_CKR_TRCM_TXRX;
+	if (!of_property_read_u32(node, "rockchip,clk-trcm", &val)) {
+		if (val >= 0 && val <= 2) {
+			i2s->clk_trcm = val << I2S_CKR_TRCM_SHIFT;
+			if (i2s->clk_trcm)
+				soc_dai->symmetric_rate = 1;
+		}
+	}
+
+	regmap_update_bits(i2s->regmap, I2S_CKR,
+			   I2S_CKR_TRCM_MASK, i2s->clk_trcm);
+
 	ret = devm_snd_soc_register_component(&pdev->dev,
 					      &rockchip_i2s_component,
 					      soc_dai, 1);