Thread (21 messages) 21 messages, 3 authors, 2021-08-24

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

From: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
Date: 2021-08-23 11:47:43
Also in: alsa-devel, linux-devicetree

On Montag, 23. August 2021 12:54:35 CEST Sugar Zhang wrote:
quoted hunk ↗ jump to hunk
From: Xing Zheng <redacted>

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 <redacted>
Signed-off-by: Sugar Zhang <redacted>
---
 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



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help