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

[Nicolas Frattaroli <frattaroli.nicolas@gmail.com>]

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