diff mbox series

[v2,1/3] ASoC: rockchip: i2s: switch BCLK to GPIO

Message ID 20220616155836.3401420-2-judyhsiao@chromium.org (mailing list archive)
State New, archived
Headers show
Series ASoC: rockchip: i2s: switch BCLK to GPIO | expand

Commit Message

Judy Hsiao June 16, 2022, 3:58 p.m. UTC
We discoverd that the state of BCLK on, LRCLK off and SD_MODE on
may cause the speaker melting issue. Removing LRCLK while BCLK
is present can cause unexpected output behavior including a large
DC output voltage as described in the Max98357a datasheet.

In order to:
  1. prevent BCLK from turning on by other component.
  2. keep BCLK and LRCLK being present at the same time

This patch switches BCLK to GPIO func before LRCLK output, and
configures BCLK func back during LRCLK is output.

Without this fix, BCLK is turned on 11 ms earlier than LRCK by the
da7219.
With this fix, BCLK is turned on only 0.4 ms earlier than LRCK by
the rockchip codec.

Signed-off-by: Judy Hsiao <judyhsiao@chromium.org>
---
 sound/soc/rockchip/rockchip_i2s.c | 134 ++++++++++++++++++++++++------
 1 file changed, 108 insertions(+), 26 deletions(-)

Comments

kernel test robot June 16, 2022, 7:03 p.m. UTC | #1
Hi Judy,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on rockchip/for-next]
[also build test ERROR on broonie-sound/for-next robh/for-next linus/master v5.19-rc2 next-20220616]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Judy-Hsiao/ASoC-rockchip-i2s-switch-BCLK-to-GPIO/20220617-000041
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git for-next
config: arc-randconfig-r043-20220616 (https://download.01.org/0day-ci/archive/20220617/202206170216.M52eiJwN-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/322064314e50b113217aa4c7412e2431defea08f
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Judy-Hsiao/ASoC-rockchip-i2s-switch-BCLK-to-GPIO/20220617-000041
        git checkout 322064314e50b113217aa4c7412e2431defea08f
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash sound/soc/rockchip/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   sound/soc/rockchip/rockchip_i2s.c: In function 'rockchip_snd_rxctrl':
   sound/soc/rockchip/rockchip_i2s.c:247:16: error: 'return' with a value, in function returning void [-Werror=return-type]
     247 |         return ret;
         |                ^~~
   sound/soc/rockchip/rockchip_i2s.c:188:13: note: declared here
     188 | static void rockchip_snd_rxctrl(struct rk_i2s_dev *i2s, int on)
         |             ^~~~~~~~~~~~~~~~~~~
   sound/soc/rockchip/rockchip_i2s.c: In function 'rockchip_i2s_trigger':
>> sound/soc/rockchip/rockchip_i2s.c:485:29: error: void value not ignored as it ought to be
     485 |                         ret = rockchip_snd_rxctrl(i2s, 1);
         |                             ^
   sound/soc/rockchip/rockchip_i2s.c:498:29: error: void value not ignored as it ought to be
     498 |                         ret = rockchip_snd_rxctrl(i2s, 0);
         |                             ^
   cc1: some warnings being treated as errors


vim +485 sound/soc/rockchip/rockchip_i2s.c

   473	
   474	static int rockchip_i2s_trigger(struct snd_pcm_substream *substream,
   475					int cmd, struct snd_soc_dai *dai)
   476	{
   477		struct rk_i2s_dev *i2s = to_info(dai);
   478		int ret = 0;
   479	
   480		switch (cmd) {
   481		case SNDRV_PCM_TRIGGER_START:
   482		case SNDRV_PCM_TRIGGER_RESUME:
   483		case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
   484			if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
 > 485				ret = rockchip_snd_rxctrl(i2s, 1);
   486			else
   487				ret = rockchip_snd_txctrl(i2s, 1);
   488			if (ret < 0)
   489				return ret;
   490			i2s_pinctrl_select_bclk_on(i2s);
   491			break;
   492		case SNDRV_PCM_TRIGGER_SUSPEND:
   493		case SNDRV_PCM_TRIGGER_STOP:
   494		case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
   495			if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
   496				if (!i2s->tx_start)
   497					i2s_pinctrl_select_bclk_off(i2s);
   498				ret = rockchip_snd_rxctrl(i2s, 0);
   499			} else {
   500				if (!i2s->rx_start)
   501					i2s_pinctrl_select_bclk_off(i2s);
   502				ret = rockchip_snd_txctrl(i2s, 0);
   503			}
   504			break;
   505		default:
   506			ret = -EINVAL;
   507			break;
   508		}
   509	
   510		return ret;
   511	}
   512
Brian Norris June 16, 2022, 8:48 p.m. UTC | #2
Hi,

On Thu, Jun 16, 2022 at 03:58:34PM +0000, Judy Hsiao wrote:
> We discoverd that the state of BCLK on, LRCLK off and SD_MODE on
> may cause the speaker melting issue. Removing LRCLK while BCLK
> is present can cause unexpected output behavior including a large
> DC output voltage as described in the Max98357a datasheet.
> 
> In order to:
>   1. prevent BCLK from turning on by other component.
>   2. keep BCLK and LRCLK being present at the same time
> 
> This patch switches BCLK to GPIO func before LRCLK output, and
> configures BCLK func back during LRCLK is output.
> 
> Without this fix, BCLK is turned on 11 ms earlier than LRCK by the
> da7219.
> With this fix, BCLK is turned on only 0.4 ms earlier than LRCK by
> the rockchip codec.
> 
> Signed-off-by: Judy Hsiao <judyhsiao@chromium.org>

It feels like a lot of the noise in this patch is due to adding
additional error handling, unrelated to the real change you're making.
Maybe that deserves a separate patch?

> ---
>  sound/soc/rockchip/rockchip_i2s.c | 134 ++++++++++++++++++++++++------
>  1 file changed, 108 insertions(+), 26 deletions(-)
> 
> diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c
> index 47a3971a9ce1..574d3d0900c4 100644
> --- a/sound/soc/rockchip/rockchip_i2s.c
> +++ b/sound/soc/rockchip/rockchip_i2s.c
> @@ -54,8 +54,40 @@ struct rk_i2s_dev {
>  	const struct rk_i2s_pins *pins;
>  	unsigned int bclk_ratio;
>  	spinlock_t lock; /* tx/rx lock */
> +	struct pinctrl *pinctrl;
> +	struct pinctrl_state *bclk_on;
> +	struct pinctrl_state *bclk_off;
>  };
>  
> +static int i2s_pinctrl_select_bclk_on(struct rk_i2s_dev *i2s)
> +{
> +	int ret = 0;
> +
> +	if (!IS_ERR(i2s->pinctrl) && !IS_ERR_OR_NULL(i2s->bclk_on))
> +		ret = pinctrl_select_state(i2s->pinctrl,
> +				     i2s->bclk_on);
> +
> +	if (ret)
> +		dev_err(i2s->dev, "bclk enable failed %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int i2s_pinctrl_select_bclk_off(struct rk_i2s_dev *i2s)
> +{
> +
> +	int ret = 0;
> +
> +	if (!IS_ERR(i2s->pinctrl) && !IS_ERR_OR_NULL(i2s->bclk_off))
> +		ret = pinctrl_select_state(i2s->pinctrl,
> +				     i2s->bclk_off);
> +
> +	if (ret)
> +		dev_err(i2s->dev, "bclk disable failed %d\n", ret);
> +
> +	return ret;
> +}
> +
>  static int i2s_runtime_suspend(struct device *dev)
>  {
>  	struct rk_i2s_dev *i2s = dev_get_drvdata(dev);
> @@ -92,39 +124,46 @@ static inline struct rk_i2s_dev *to_info(struct snd_soc_dai *dai)
>  	return snd_soc_dai_get_drvdata(dai);
>  }
>  
> -static void rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
> +static int rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
>  {
>  	unsigned int val = 0;
>  	int retry = 10;
> -
> +	int ret = 0;
> +
>  	spin_lock(&i2s->lock);
>  	if (on) {
> -		regmap_update_bits(i2s->regmap, I2S_DMACR,
> +		ret = regmap_update_bits(i2s->regmap, I2S_DMACR,
>  				   I2S_DMACR_TDE_ENABLE, I2S_DMACR_TDE_ENABLE);

I mentioned this off-list, but the 2nd-line indentation alignment *used*
to be in a very particular style, and you've moved that around a lot. To
match the previous style, it should be:

		ret = regmap_update_bits(i2s->regmap, I2S_DMACR,
					 I2S_DMACR_TDE_ENABLE,
					 I2S_DMACR_TDE_ENABLE);

(BTW, if you're using Gmail to view this, you're going to have no idea
what I'm talking about, since it doesn't do whitespace or monospace font
correctly...)

The same applies throughout; most of the 2nd-line, 3rd-line, ...
indentation is a little weird.

> -
> -		regmap_update_bits(i2s->regmap, I2S_XFER,
> +		if (ret < 0)
> +			goto end;
> +		ret = regmap_update_bits(i2s->regmap, I2S_XFER,
>  				   I2S_XFER_TXS_START | I2S_XFER_RXS_START,
>  				   I2S_XFER_TXS_START | I2S_XFER_RXS_START);
> -
> +		if (ret < 0)
> +			goto end;
>  		i2s->tx_start = true;
>  	} else {
>  		i2s->tx_start = false;
>  
> -		regmap_update_bits(i2s->regmap, I2S_DMACR,
> +		ret = regmap_update_bits(i2s->regmap, I2S_DMACR,
>  				   I2S_DMACR_TDE_ENABLE, I2S_DMACR_TDE_DISABLE);
> +		if (ret < 0)
> +			goto end;
>  
>  		if (!i2s->rx_start) {
> -			regmap_update_bits(i2s->regmap, I2S_XFER,
> +			ret = regmap_update_bits(i2s->regmap, I2S_XFER,
>  					   I2S_XFER_TXS_START |
>  					   I2S_XFER_RXS_START,
>  					   I2S_XFER_TXS_STOP |
>  					   I2S_XFER_RXS_STOP);
> -
> +			if (ret < 0)
> +				goto end;
>  			udelay(150);
> -			regmap_update_bits(i2s->regmap, I2S_CLR,
> +			ret = regmap_update_bits(i2s->regmap, I2S_CLR,
>  					   I2S_CLR_TXC | I2S_CLR_RXC,
>  					   I2S_CLR_TXC | I2S_CLR_RXC);
> -
> +			if (ret < 0)
> +				goto end;
>  			regmap_read(i2s->regmap, I2S_CLR, &val);
>  
>  			/* Should wait for clear operation to finish */
> @@ -138,42 +177,55 @@ static void rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
>  			}
>  		}
>  	}
> +end:
>  	spin_unlock(&i2s->lock);
> +	if (ret < 0)
> +		dev_err(i2s->dev, "lrclk update failed\n");
> +
> +	return ret;
>  }
>  
>  static void rockchip_snd_rxctrl(struct rk_i2s_dev *i2s, int on)
>  {
>  	unsigned int val = 0;
>  	int retry = 10;
> +	int ret = 0;
>  
>  	spin_lock(&i2s->lock);
>  	if (on) {
> -		regmap_update_bits(i2s->regmap, I2S_DMACR,
> +		ret = regmap_update_bits(i2s->regmap, I2S_DMACR,
>  				   I2S_DMACR_RDE_ENABLE, I2S_DMACR_RDE_ENABLE);
> +		if (ret < 0)
> +			goto end;
>  
> -		regmap_update_bits(i2s->regmap, I2S_XFER,
> +		ret = regmap_update_bits(i2s->regmap, I2S_XFER,
>  				   I2S_XFER_TXS_START | I2S_XFER_RXS_START,
>  				   I2S_XFER_TXS_START | I2S_XFER_RXS_START);
> -
> +		if (ret < 0)
> +			goto end;
>  		i2s->rx_start = true;
>  	} else {
>  		i2s->rx_start = false;
>  
> -		regmap_update_bits(i2s->regmap, I2S_DMACR,
> +		ret = regmap_update_bits(i2s->regmap, I2S_DMACR,
>  				   I2S_DMACR_RDE_ENABLE, I2S_DMACR_RDE_DISABLE);
> +		if (ret < 0)
> +			goto end;
>  
>  		if (!i2s->tx_start) {
> -			regmap_update_bits(i2s->regmap, I2S_XFER,
> +			ret = regmap_update_bits(i2s->regmap, I2S_XFER,
>  					   I2S_XFER_TXS_START |
> -					   I2S_XFER_RXS_START,
> +						   I2S_XFER_RXS_START,
>  					   I2S_XFER_TXS_STOP |
>  					   I2S_XFER_RXS_STOP);
> -
> +			if (ret < 0)
> +				goto end;
>  			udelay(150);
> -			regmap_update_bits(i2s->regmap, I2S_CLR,
> +			ret = regmap_update_bits(i2s->regmap, I2S_CLR,
>  					   I2S_CLR_TXC | I2S_CLR_RXC,
>  					   I2S_CLR_TXC | I2S_CLR_RXC);
> -
> +			if (ret < 0)
> +				goto end;
>  			regmap_read(i2s->regmap, I2S_CLR, &val);
>  
>  			/* Should wait for clear operation to finish */
> @@ -187,7 +239,12 @@ static void rockchip_snd_rxctrl(struct rk_i2s_dev *i2s, int on)
>  			}
>  		}
>  	}
> +end:
>  	spin_unlock(&i2s->lock);
> +	if (ret < 0)
> +		dev_err(i2s->dev, "lrclk update failed\n");
> +
> +	return ret;
>  }
>  
>  static int rockchip_i2s_set_fmt(struct snd_soc_dai *cpu_dai,
> @@ -425,17 +482,25 @@ static int rockchip_i2s_trigger(struct snd_pcm_substream *substream,
>  	case SNDRV_PCM_TRIGGER_RESUME:
>  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>  		if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
> -			rockchip_snd_rxctrl(i2s, 1);
> +			ret = rockchip_snd_rxctrl(i2s, 1);
>  		else
> -			rockchip_snd_txctrl(i2s, 1);
> +			ret = rockchip_snd_txctrl(i2s, 1);
> +		if (ret < 0)
> +			return ret;
> +		i2s_pinctrl_select_bclk_on(i2s);
>  		break;
>  	case SNDRV_PCM_TRIGGER_SUSPEND:
>  	case SNDRV_PCM_TRIGGER_STOP:
>  	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> -		if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
> -			rockchip_snd_rxctrl(i2s, 0);
> -		else
> -			rockchip_snd_txctrl(i2s, 0);
> +		if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
> +			if (!i2s->tx_start)
> +				i2s_pinctrl_select_bclk_off(i2s);
> +			ret = rockchip_snd_rxctrl(i2s, 0);
> +		} else {
> +			if (!i2s->rx_start)
> +				i2s_pinctrl_select_bclk_off(i2s);
> +			ret = rockchip_snd_txctrl(i2s, 0);
> +		}
>  		break;
>  	default:
>  		ret = -EINVAL;
> @@ -736,6 +801,23 @@ static int rockchip_i2s_probe(struct platform_device *pdev)
>  	}
>  
>  	i2s->bclk_ratio = 64;
> +	i2s->pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (IS_ERR(i2s->pinctrl))
> +		dev_err(&pdev->dev, "failed to find i2s pinctrl\n");
> +
> +	i2s->bclk_on = pinctrl_lookup_state(i2s->pinctrl,
> +				   "bclk_on");
> +	if (!IS_ERR_OR_NULL(i2s->bclk_on)) {
> +		dev_info(&pdev->dev, "switch bclk to GPIO func\n");

I don't think we need this printed at the KERN_INFO level. Either drop
it, or maybe dev_dbg().

Brian

> +		i2s->bclk_off = pinctrl_lookup_state(i2s->pinctrl,
> +				  "bclk_off");
> +		if (IS_ERR_OR_NULL(i2s->bclk_off)) {
> +			dev_err(&pdev->dev, "failed to find i2s bclk_off\n");
> +			goto err_clk;
> +		}
> +	}
> +
> +	i2s_pinctrl_select_bclk_off(i2s);
>  
>  	dev_set_drvdata(&pdev->dev, i2s);
>  
> -- 
> 2.36.1.476.g0c4daa206d-goog
>
Judy Hsiao June 17, 2022, 4:21 a.m. UTC | #3
> It feels like a lot of the noise in this patch is due to adding
> additional error handling, unrelated to the real change you're making.
> Maybe that deserves a separate patch?
The error handling is to detect the failure of switching LRCLK.
If LRCLK is OFF, we don’t want the BCLK to be ON. So the error
handling in the patch is relevant.

> I mentioned this off-list, but the 2nd-line indentation alignment *used*
> to be in a very particular style, and you've moved that around a lot. To
> match the previous style, it should be:
>
>                 ret = regmap_update_bits(i2s->regmap, I2S_DMACR,
>                                          I2S_DMACR_TDE_ENABLE,
>                                          I2S_DMACR_TDE_ENABLE);
>
> (BTW, if you're using Gmail to view this, you're going to have no idea
> what I'm talking about, since it doesn't do whitespace or monospace font
> correctly...)
>
> The same applies throughout; most of the 2nd-line, 3rd-line, ...
> indentation is a little weird.
Yes, let me fix the indentation in V3, thanks!
diff mbox series

Patch

diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c
index 47a3971a9ce1..574d3d0900c4 100644
--- a/sound/soc/rockchip/rockchip_i2s.c
+++ b/sound/soc/rockchip/rockchip_i2s.c
@@ -54,8 +54,40 @@  struct rk_i2s_dev {
 	const struct rk_i2s_pins *pins;
 	unsigned int bclk_ratio;
 	spinlock_t lock; /* tx/rx lock */
+	struct pinctrl *pinctrl;
+	struct pinctrl_state *bclk_on;
+	struct pinctrl_state *bclk_off;
 };
 
+static int i2s_pinctrl_select_bclk_on(struct rk_i2s_dev *i2s)
+{
+	int ret = 0;
+
+	if (!IS_ERR(i2s->pinctrl) && !IS_ERR_OR_NULL(i2s->bclk_on))
+		ret = pinctrl_select_state(i2s->pinctrl,
+				     i2s->bclk_on);
+
+	if (ret)
+		dev_err(i2s->dev, "bclk enable failed %d\n", ret);
+
+	return ret;
+}
+
+static int i2s_pinctrl_select_bclk_off(struct rk_i2s_dev *i2s)
+{
+
+	int ret = 0;
+
+	if (!IS_ERR(i2s->pinctrl) && !IS_ERR_OR_NULL(i2s->bclk_off))
+		ret = pinctrl_select_state(i2s->pinctrl,
+				     i2s->bclk_off);
+
+	if (ret)
+		dev_err(i2s->dev, "bclk disable failed %d\n", ret);
+
+	return ret;
+}
+
 static int i2s_runtime_suspend(struct device *dev)
 {
 	struct rk_i2s_dev *i2s = dev_get_drvdata(dev);
@@ -92,39 +124,46 @@  static inline struct rk_i2s_dev *to_info(struct snd_soc_dai *dai)
 	return snd_soc_dai_get_drvdata(dai);
 }
 
-static void rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
+static int rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
 {
 	unsigned int val = 0;
 	int retry = 10;
-
+	int ret = 0;
+
 	spin_lock(&i2s->lock);
 	if (on) {
-		regmap_update_bits(i2s->regmap, I2S_DMACR,
+		ret = regmap_update_bits(i2s->regmap, I2S_DMACR,
 				   I2S_DMACR_TDE_ENABLE, I2S_DMACR_TDE_ENABLE);
-
-		regmap_update_bits(i2s->regmap, I2S_XFER,
+		if (ret < 0)
+			goto end;
+		ret = regmap_update_bits(i2s->regmap, I2S_XFER,
 				   I2S_XFER_TXS_START | I2S_XFER_RXS_START,
 				   I2S_XFER_TXS_START | I2S_XFER_RXS_START);
-
+		if (ret < 0)
+			goto end;
 		i2s->tx_start = true;
 	} else {
 		i2s->tx_start = false;
 
-		regmap_update_bits(i2s->regmap, I2S_DMACR,
+		ret = regmap_update_bits(i2s->regmap, I2S_DMACR,
 				   I2S_DMACR_TDE_ENABLE, I2S_DMACR_TDE_DISABLE);
+		if (ret < 0)
+			goto end;
 
 		if (!i2s->rx_start) {
-			regmap_update_bits(i2s->regmap, I2S_XFER,
+			ret = regmap_update_bits(i2s->regmap, I2S_XFER,
 					   I2S_XFER_TXS_START |
 					   I2S_XFER_RXS_START,
 					   I2S_XFER_TXS_STOP |
 					   I2S_XFER_RXS_STOP);
-
+			if (ret < 0)
+				goto end;
 			udelay(150);
-			regmap_update_bits(i2s->regmap, I2S_CLR,
+			ret = regmap_update_bits(i2s->regmap, I2S_CLR,
 					   I2S_CLR_TXC | I2S_CLR_RXC,
 					   I2S_CLR_TXC | I2S_CLR_RXC);
-
+			if (ret < 0)
+				goto end;
 			regmap_read(i2s->regmap, I2S_CLR, &val);
 
 			/* Should wait for clear operation to finish */
@@ -138,42 +177,55 @@  static void rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
 			}
 		}
 	}
+end:
 	spin_unlock(&i2s->lock);
+	if (ret < 0)
+		dev_err(i2s->dev, "lrclk update failed\n");
+
+	return ret;
 }
 
 static void rockchip_snd_rxctrl(struct rk_i2s_dev *i2s, int on)
 {
 	unsigned int val = 0;
 	int retry = 10;
+	int ret = 0;
 
 	spin_lock(&i2s->lock);
 	if (on) {
-		regmap_update_bits(i2s->regmap, I2S_DMACR,
+		ret = regmap_update_bits(i2s->regmap, I2S_DMACR,
 				   I2S_DMACR_RDE_ENABLE, I2S_DMACR_RDE_ENABLE);
+		if (ret < 0)
+			goto end;
 
-		regmap_update_bits(i2s->regmap, I2S_XFER,
+		ret = regmap_update_bits(i2s->regmap, I2S_XFER,
 				   I2S_XFER_TXS_START | I2S_XFER_RXS_START,
 				   I2S_XFER_TXS_START | I2S_XFER_RXS_START);
-
+		if (ret < 0)
+			goto end;
 		i2s->rx_start = true;
 	} else {
 		i2s->rx_start = false;
 
-		regmap_update_bits(i2s->regmap, I2S_DMACR,
+		ret = regmap_update_bits(i2s->regmap, I2S_DMACR,
 				   I2S_DMACR_RDE_ENABLE, I2S_DMACR_RDE_DISABLE);
+		if (ret < 0)
+			goto end;
 
 		if (!i2s->tx_start) {
-			regmap_update_bits(i2s->regmap, I2S_XFER,
+			ret = regmap_update_bits(i2s->regmap, I2S_XFER,
 					   I2S_XFER_TXS_START |
-					   I2S_XFER_RXS_START,
+						   I2S_XFER_RXS_START,
 					   I2S_XFER_TXS_STOP |
 					   I2S_XFER_RXS_STOP);
-
+			if (ret < 0)
+				goto end;
 			udelay(150);
-			regmap_update_bits(i2s->regmap, I2S_CLR,
+			ret = regmap_update_bits(i2s->regmap, I2S_CLR,
 					   I2S_CLR_TXC | I2S_CLR_RXC,
 					   I2S_CLR_TXC | I2S_CLR_RXC);
-
+			if (ret < 0)
+				goto end;
 			regmap_read(i2s->regmap, I2S_CLR, &val);
 
 			/* Should wait for clear operation to finish */
@@ -187,7 +239,12 @@  static void rockchip_snd_rxctrl(struct rk_i2s_dev *i2s, int on)
 			}
 		}
 	}
+end:
 	spin_unlock(&i2s->lock);
+	if (ret < 0)
+		dev_err(i2s->dev, "lrclk update failed\n");
+
+	return ret;
 }
 
 static int rockchip_i2s_set_fmt(struct snd_soc_dai *cpu_dai,
@@ -425,17 +482,25 @@  static int rockchip_i2s_trigger(struct snd_pcm_substream *substream,
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
-			rockchip_snd_rxctrl(i2s, 1);
+			ret = rockchip_snd_rxctrl(i2s, 1);
 		else
-			rockchip_snd_txctrl(i2s, 1);
+			ret = rockchip_snd_txctrl(i2s, 1);
+		if (ret < 0)
+			return ret;
+		i2s_pinctrl_select_bclk_on(i2s);
 		break;
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
-			rockchip_snd_rxctrl(i2s, 0);
-		else
-			rockchip_snd_txctrl(i2s, 0);
+		if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
+			if (!i2s->tx_start)
+				i2s_pinctrl_select_bclk_off(i2s);
+			ret = rockchip_snd_rxctrl(i2s, 0);
+		} else {
+			if (!i2s->rx_start)
+				i2s_pinctrl_select_bclk_off(i2s);
+			ret = rockchip_snd_txctrl(i2s, 0);
+		}
 		break;
 	default:
 		ret = -EINVAL;
@@ -736,6 +801,23 @@  static int rockchip_i2s_probe(struct platform_device *pdev)
 	}
 
 	i2s->bclk_ratio = 64;
+	i2s->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (IS_ERR(i2s->pinctrl))
+		dev_err(&pdev->dev, "failed to find i2s pinctrl\n");
+
+	i2s->bclk_on = pinctrl_lookup_state(i2s->pinctrl,
+				   "bclk_on");
+	if (!IS_ERR_OR_NULL(i2s->bclk_on)) {
+		dev_info(&pdev->dev, "switch bclk to GPIO func\n");
+		i2s->bclk_off = pinctrl_lookup_state(i2s->pinctrl,
+				  "bclk_off");
+		if (IS_ERR_OR_NULL(i2s->bclk_off)) {
+			dev_err(&pdev->dev, "failed to find i2s bclk_off\n");
+			goto err_clk;
+		}
+	}
+
+	i2s_pinctrl_select_bclk_off(i2s);
 
 	dev_set_drvdata(&pdev->dev, i2s);