diff mbox series

ASoC: qcom: lpass-cpu: Remove bit clock state check

Message ID 20210127063038.1399-1-srivasam@codeaurora.org (mailing list archive)
State Superseded
Headers show
Series ASoC: qcom: lpass-cpu: Remove bit clock state check | expand

Commit Message

Srinivasa Rao Mandadapu Jan. 27, 2021, 6:30 a.m. UTC
No need of BCLK state maintenance from driver side as
clock_enable and clk_disable API's maintaing state counter.

One of the major issue was spotted when Headset jack inserted
while playback continues, due to same PCM device node opens twice
for playaback/capture and closes once for capture and playback continues.

It can resolve the errors in such scenarios.

Signed-off-by: Srinivasa Rao Mandadapu <srivasam@codeaurora.org>
---
 sound/soc/qcom/lpass-cpu.c       | 22 ++++++++--------------
 sound/soc/qcom/lpass-lpaif-reg.h |  3 ---
 sound/soc/qcom/lpass.h           |  1 -
 3 files changed, 8 insertions(+), 18 deletions(-)

Comments

Srinivas Kandagatla Jan. 27, 2021, 9:51 a.m. UTC | #1
On 27/01/2021 06:30, Srinivasa Rao Mandadapu wrote:
> No need of BCLK state maintenance from driver side as
> clock_enable and clk_disable API's maintaing state counter.
> 
> One of the major issue was spotted when Headset jack inserted
> while playback continues, due to same PCM device node opens twice
> for playaback/capture and closes once for capture and playback continues.
> 
> It can resolve the errors in such scenarios.
> 
> Signed-off-by: Srinivasa Rao Mandadapu <srivasam@codeaurora.org>

Thanks for the cleanup, yes clk core will take care of this by 
enable_count check!

You should add

Fixes: b1824968221c ("ASoC: qcom: Fix enabling BCLK and LRCLK in LPAIF 
invalid state")


> ---
>   sound/soc/qcom/lpass-cpu.c       | 22 ++++++++--------------
>   sound/soc/qcom/lpass-lpaif-reg.h |  3 ---
>   sound/soc/qcom/lpass.h           |  1 -
>   3 files changed, 8 insertions(+), 18 deletions(-)
> 
> diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
> index ae8efbc89af2..a669202e0001 100644
> --- a/sound/soc/qcom/lpass-cpu.c
> +++ b/sound/soc/qcom/lpass-cpu.c
> @@ -286,16 +286,12 @@ static int lpass_cpu_daiops_trigger(struct snd_pcm_substream *substream,
>   			dev_err(dai->dev, "error writing to i2sctl reg: %d\n",
>   				ret);
>   
> -		if (drvdata->bit_clk_state[id] == LPAIF_BIT_CLK_DISABLE) {
> -			ret = clk_enable(drvdata->mi2s_bit_clk[id]);
> -			if (ret) {
> -				dev_err(dai->dev, "error in enabling mi2s bit clk: %d\n", ret);
> -				clk_disable(drvdata->mi2s_osr_clk[id]);
> -				return ret;
> -			}
> -			drvdata->bit_clk_state[id] = LPAIF_BIT_CLK_ENABLE;
> +		ret = clk_enable(drvdata->mi2s_bit_clk[id]);
> +		if (ret) {
> +			dev_err(dai->dev, "error in enabling mi2s bit clk: %d\n", ret);
> +			clk_disable(drvdata->mi2s_osr_clk[id]);

Can you also remove this unnecessary disable here!


-srini

> +			return ret;
>   		}
> -
>   		break;
>   	case SNDRV_PCM_TRIGGER_STOP:
>   	case SNDRV_PCM_TRIGGER_SUSPEND:
> @@ -310,10 +306,9 @@ static int lpass_cpu_daiops_trigger(struct snd_pcm_substream *substream,
>   		if (ret)
>   			dev_err(dai->dev, "error writing to i2sctl reg: %d\n",
>   				ret);
> -		if (drvdata->bit_clk_state[id] == LPAIF_BIT_CLK_ENABLE) {
> -			clk_disable(drvdata->mi2s_bit_clk[dai->driver->id]);
> -			drvdata->bit_clk_state[id] = LPAIF_BIT_CLK_DISABLE;
> -		}
> +
> +		clk_disable(drvdata->mi2s_bit_clk[dai->driver->id]);
> +
>   		break;
>   	}
>   
> @@ -861,7 +856,6 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev)
>   				PTR_ERR(drvdata->mi2s_bit_clk[dai_id]));
>   			return PTR_ERR(drvdata->mi2s_bit_clk[dai_id]);
>   		}
> -		drvdata->bit_clk_state[dai_id] = LPAIF_BIT_CLK_DISABLE;
>   	}
>   
>   	/* Allocation for i2sctl regmap fields */
> diff --git a/sound/soc/qcom/lpass-lpaif-reg.h b/sound/soc/qcom/lpass-lpaif-reg.h
> index 405542832e99..c8e1d75340b2 100644
> --- a/sound/soc/qcom/lpass-lpaif-reg.h
> +++ b/sound/soc/qcom/lpass-lpaif-reg.h
> @@ -60,9 +60,6 @@
>   #define LPAIF_I2SCTL_BITWIDTH_24	1
>   #define LPAIF_I2SCTL_BITWIDTH_32	2
>   
> -#define LPAIF_BIT_CLK_DISABLE		0
> -#define LPAIF_BIT_CLK_ENABLE		1
> -
>   #define LPAIF_I2SCTL_RESET_STATE	0x003C0004
>   #define LPAIF_DMACTL_RESET_STATE	0x00200000
>   
> diff --git a/sound/soc/qcom/lpass.h b/sound/soc/qcom/lpass.h
> index 2d68af0da34d..83b2e08ade06 100644
> --- a/sound/soc/qcom/lpass.h
> +++ b/sound/soc/qcom/lpass.h
> @@ -68,7 +68,6 @@ struct lpass_data {
>   	unsigned int mi2s_playback_sd_mode[LPASS_MAX_MI2S_PORTS];
>   	unsigned int mi2s_capture_sd_mode[LPASS_MAX_MI2S_PORTS];
>   	int hdmi_port_enable;
> -	int bit_clk_state[LPASS_MAX_MI2S_PORTS];
>   
>   	/* low-power audio interface (LPAIF) registers */
>   	void __iomem *lpaif;
>
Srinivasa Rao Mandadapu Jan. 27, 2021, 10:54 a.m. UTC | #2
Thanks Srinivas For Your time!!!

On 1/27/2021 3:21 PM, Srinivas Kandagatla wrote:
>
>
> On 27/01/2021 06:30, Srinivasa Rao Mandadapu wrote:
>> No need of BCLK state maintenance from driver side as
>> clock_enable and clk_disable API's maintaing state counter.
>>
>> One of the major issue was spotted when Headset jack inserted
>> while playback continues, due to same PCM device node opens twice
>> for playaback/capture and closes once for capture and playback 
>> continues.
>>
>> It can resolve the errors in such scenarios.
>>
>> Signed-off-by: Srinivasa Rao Mandadapu <srivasam@codeaurora.org>
>
> Thanks for the cleanup, yes clk core will take care of this by 
> enable_count check!
>
> You should add
>
> Fixes: b1824968221c ("ASoC: qcom: Fix enabling BCLK and LRCLK in LPAIF 
> invalid state")
>
Okay. I will do in next patch.
>
>> ---
>>   sound/soc/qcom/lpass-cpu.c       | 22 ++++++++--------------
>>   sound/soc/qcom/lpass-lpaif-reg.h |  3 ---
>>   sound/soc/qcom/lpass.h           |  1 -
>>   3 files changed, 8 insertions(+), 18 deletions(-)
>>
>> diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
>> index ae8efbc89af2..a669202e0001 100644
>> --- a/sound/soc/qcom/lpass-cpu.c
>> +++ b/sound/soc/qcom/lpass-cpu.c
>> @@ -286,16 +286,12 @@ static int lpass_cpu_daiops_trigger(struct 
>> snd_pcm_substream *substream,
>>               dev_err(dai->dev, "error writing to i2sctl reg: %d\n",
>>                   ret);
>>   -        if (drvdata->bit_clk_state[id] == LPAIF_BIT_CLK_DISABLE) {
>> -            ret = clk_enable(drvdata->mi2s_bit_clk[id]);
>> -            if (ret) {
>> -                dev_err(dai->dev, "error in enabling mi2s bit clk: 
>> %d\n", ret);
>> -                clk_disable(drvdata->mi2s_osr_clk[id]);
>> -                return ret;
>> -            }
>> -            drvdata->bit_clk_state[id] = LPAIF_BIT_CLK_ENABLE;
>> +        ret = clk_enable(drvdata->mi2s_bit_clk[id]);
>> +        if (ret) {
>> +            dev_err(dai->dev, "error in enabling mi2s bit clk: 
>> %d\n", ret);
>> +            clk_disable(drvdata->mi2s_osr_clk[id]);
>
> Can you also remove this unnecessary disable here!
>
>
Actually this is MI2S OSR clock disable on failure of bit clock enable.

Do You think it's redundant?

> -srini
>
>> +            return ret;
>>           }
>> -
>>           break;
>>       case SNDRV_PCM_TRIGGER_STOP:
>>       case SNDRV_PCM_TRIGGER_SUSPEND:
>> @@ -310,10 +306,9 @@ static int lpass_cpu_daiops_trigger(struct 
>> snd_pcm_substream *substream,
>>           if (ret)
>>               dev_err(dai->dev, "error writing to i2sctl reg: %d\n",
>>                   ret);
>> -        if (drvdata->bit_clk_state[id] == LPAIF_BIT_CLK_ENABLE) {
>> - clk_disable(drvdata->mi2s_bit_clk[dai->driver->id]);
>> -            drvdata->bit_clk_state[id] = LPAIF_BIT_CLK_DISABLE;
>> -        }
>> +
>> + clk_disable(drvdata->mi2s_bit_clk[dai->driver->id]);
>> +
>>           break;
>>       }
>>   @@ -861,7 +856,6 @@ int asoc_qcom_lpass_cpu_platform_probe(struct 
>> platform_device *pdev)
>>                   PTR_ERR(drvdata->mi2s_bit_clk[dai_id]));
>>               return PTR_ERR(drvdata->mi2s_bit_clk[dai_id]);
>>           }
>> -        drvdata->bit_clk_state[dai_id] = LPAIF_BIT_CLK_DISABLE;
>>       }
>>         /* Allocation for i2sctl regmap fields */
>> diff --git a/sound/soc/qcom/lpass-lpaif-reg.h 
>> b/sound/soc/qcom/lpass-lpaif-reg.h
>> index 405542832e99..c8e1d75340b2 100644
>> --- a/sound/soc/qcom/lpass-lpaif-reg.h
>> +++ b/sound/soc/qcom/lpass-lpaif-reg.h
>> @@ -60,9 +60,6 @@
>>   #define LPAIF_I2SCTL_BITWIDTH_24    1
>>   #define LPAIF_I2SCTL_BITWIDTH_32    2
>>   -#define LPAIF_BIT_CLK_DISABLE        0
>> -#define LPAIF_BIT_CLK_ENABLE        1
>> -
>>   #define LPAIF_I2SCTL_RESET_STATE    0x003C0004
>>   #define LPAIF_DMACTL_RESET_STATE    0x00200000
>>   diff --git a/sound/soc/qcom/lpass.h b/sound/soc/qcom/lpass.h
>> index 2d68af0da34d..83b2e08ade06 100644
>> --- a/sound/soc/qcom/lpass.h
>> +++ b/sound/soc/qcom/lpass.h
>> @@ -68,7 +68,6 @@ struct lpass_data {
>>       unsigned int mi2s_playback_sd_mode[LPASS_MAX_MI2S_PORTS];
>>       unsigned int mi2s_capture_sd_mode[LPASS_MAX_MI2S_PORTS];
>>       int hdmi_port_enable;
>> -    int bit_clk_state[LPASS_MAX_MI2S_PORTS];
>>         /* low-power audio interface (LPAIF) registers */
>>       void __iomem *lpaif;
>>
Srinivas Kandagatla Jan. 27, 2021, 10:56 a.m. UTC | #3
On 27/01/2021 10:54, Srinivasa Rao Mandadapu wrote:
>>
> Actually this is MI2S OSR clock disable on failure of bit clock enable.
> 
> Do You think it's redundant?
My Bad!! Just ignore my comment on removing this!

--srini
diff mbox series

Patch

diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
index ae8efbc89af2..a669202e0001 100644
--- a/sound/soc/qcom/lpass-cpu.c
+++ b/sound/soc/qcom/lpass-cpu.c
@@ -286,16 +286,12 @@  static int lpass_cpu_daiops_trigger(struct snd_pcm_substream *substream,
 			dev_err(dai->dev, "error writing to i2sctl reg: %d\n",
 				ret);
 
-		if (drvdata->bit_clk_state[id] == LPAIF_BIT_CLK_DISABLE) {
-			ret = clk_enable(drvdata->mi2s_bit_clk[id]);
-			if (ret) {
-				dev_err(dai->dev, "error in enabling mi2s bit clk: %d\n", ret);
-				clk_disable(drvdata->mi2s_osr_clk[id]);
-				return ret;
-			}
-			drvdata->bit_clk_state[id] = LPAIF_BIT_CLK_ENABLE;
+		ret = clk_enable(drvdata->mi2s_bit_clk[id]);
+		if (ret) {
+			dev_err(dai->dev, "error in enabling mi2s bit clk: %d\n", ret);
+			clk_disable(drvdata->mi2s_osr_clk[id]);
+			return ret;
 		}
-
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
@@ -310,10 +306,9 @@  static int lpass_cpu_daiops_trigger(struct snd_pcm_substream *substream,
 		if (ret)
 			dev_err(dai->dev, "error writing to i2sctl reg: %d\n",
 				ret);
-		if (drvdata->bit_clk_state[id] == LPAIF_BIT_CLK_ENABLE) {
-			clk_disable(drvdata->mi2s_bit_clk[dai->driver->id]);
-			drvdata->bit_clk_state[id] = LPAIF_BIT_CLK_DISABLE;
-		}
+
+		clk_disable(drvdata->mi2s_bit_clk[dai->driver->id]);
+
 		break;
 	}
 
@@ -861,7 +856,6 @@  int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev)
 				PTR_ERR(drvdata->mi2s_bit_clk[dai_id]));
 			return PTR_ERR(drvdata->mi2s_bit_clk[dai_id]);
 		}
-		drvdata->bit_clk_state[dai_id] = LPAIF_BIT_CLK_DISABLE;
 	}
 
 	/* Allocation for i2sctl regmap fields */
diff --git a/sound/soc/qcom/lpass-lpaif-reg.h b/sound/soc/qcom/lpass-lpaif-reg.h
index 405542832e99..c8e1d75340b2 100644
--- a/sound/soc/qcom/lpass-lpaif-reg.h
+++ b/sound/soc/qcom/lpass-lpaif-reg.h
@@ -60,9 +60,6 @@ 
 #define LPAIF_I2SCTL_BITWIDTH_24	1
 #define LPAIF_I2SCTL_BITWIDTH_32	2
 
-#define LPAIF_BIT_CLK_DISABLE		0
-#define LPAIF_BIT_CLK_ENABLE		1
-
 #define LPAIF_I2SCTL_RESET_STATE	0x003C0004
 #define LPAIF_DMACTL_RESET_STATE	0x00200000
 
diff --git a/sound/soc/qcom/lpass.h b/sound/soc/qcom/lpass.h
index 2d68af0da34d..83b2e08ade06 100644
--- a/sound/soc/qcom/lpass.h
+++ b/sound/soc/qcom/lpass.h
@@ -68,7 +68,6 @@  struct lpass_data {
 	unsigned int mi2s_playback_sd_mode[LPASS_MAX_MI2S_PORTS];
 	unsigned int mi2s_capture_sd_mode[LPASS_MAX_MI2S_PORTS];
 	int hdmi_port_enable;
-	int bit_clk_state[LPASS_MAX_MI2S_PORTS];
 
 	/* low-power audio interface (LPAIF) registers */
 	void __iomem *lpaif;