On Montag, 23. August 2021 12:54:35 CEST Sugar Zhang wrote:
> From: Xing Zheng <zhengxing@xxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx>
> Signed-off-by: Sugar Zhang <sugar.zhang@xxxxxxxxxxxxxx>
> ---
> 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
