[v2,6/8] ASoC: cirrus: i2s: Prepare clock before using it

Commit Message

Nikita Shubin July 26, 2021, 1:59 p.m. UTC

From: Alexander Sverdlin <[e-mail]>

Use clk_prepare_enable()/clk_disable_unprepare() in preparation for switch
to Common Clock Framework, otherwise the following is visible:

WARNING: CPU: 0 PID: 97 at drivers/clk/clk.c:1011 clk_core_enable+0x9c/0xbc
Enabling unprepared mclk
...
Hardware name: Cirrus Logic EDB9302 Evaluation Board
...
clk_core_enable
clk_core_enable_lock
ep93xx_i2s_hw_params
snd_soc_dai_hw_params
soc_pcm_hw_params
snd_pcm_hw_params
snd_pcm_ioctl
...

Signed-off-by: Alexander Sverdlin <[e-mail]>
Signed-off-by: Nikita Shubin <[e-mail]>
---
 sound/soc/cirrus/ep93xx-i2s.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Alexander Sverdlin Sept. 13, 2021, 9:43 p.m. UTC | #1

Hello Mark,

On Mon, 2021-07-26 at 17:51 +0100, Mark Brown wrote:
> On Mon, Jul 26, 2021 at 04:59:54PM +0300, Nikita Shubin wrote:
> > From: Alexander Sverdlin <[e-mail]>
> > 
> > Use clk_prepare_enable()/clk_disable_unprepare() in preparation for switch
> > to Common Clock Framework, otherwise the following is visible:
> 
> Acked-by: Mark Brown <[e-mail]>

would you take the patch to a tree of yours, please?

Alexander Sverdlin Oct. 12, 2021, 7:25 a.m. UTC | #2

Hello Mark,

On Mon, 2021-09-13 at 23:43 +0200, Alexander Sverdlin wrote:
> On Mon, 2021-07-26 at 17:51 +0100, Mark Brown wrote:
> > On Mon, Jul 26, 2021 at 04:59:54PM +0300, Nikita Shubin wrote:
> > > From: Alexander Sverdlin <[e-mail]>
> > > 
> > > Use clk_prepare_enable()/clk_disable_unprepare() in preparation for switch
> > > to Common Clock Framework, otherwise the following is visible:
> > 
> > Acked-by: Mark Brown <[e-mail]>
> 
> would you take the patch to a tree of yours, please?

I still cannot find this patch in any of your trees, but I've found this one:

commit 726e6f31b1026f62206f1d32b5cbb7e9582c4d03
Merge: b09bff2676be 7c72dc56a631
Author: Mark Brown <[e-mail]>
Date:   Tue Aug 3 23:27:27 2021 +0100

    Merge series "arm: ep93xx: CCF conversion" from Nikita Shubin <[e-mail]>:
    
    This series series of patches converts ep93xx to Common Clock Framework.
    
    It consists of preparation patches to use clk_prepare_enable where it is
    needed, instead of clk_enable used in ep93xx drivers prior to CCF and
    a patch converting mach-ep93xx/clock.c to CCF.
    
    Link: https://lore.kernel.org/patchwork/cover/1445563/
    Link: https://lore.kernel.org/patchwork/patch/1435884/
    
    v1->v2:
    - added SoB
    
    Alexander Sverdlin (7):
      iio: ep93xx: Prepare clock before using it
      spi: spi-ep93xx: Prepare clock before using it
      Input: ep93xx_keypad: Prepare clock before using it
      video: ep93xx: Prepare clock before using it
      dmaengine: ep93xx: Prepare clock before using it
      ASoC: cirrus: i2s: Prepare clock before using it
      pwm: ep93xx: Prepare clock before using it
    
    Nikita Shubin (1):
      ep93xx: clock: convert in-place to COMMON_CLK


... which claims to merge both "ASoC: cirrus: i2s: Prepare clock before using it"
and "ep93xx: clock: convert in-place to COMMON_CLK", but they are actually not
merged.

Could you please consider ASoC patch, while I will resubmit the final clock conversion?

Mark Brown Oct. 12, 2021, 10:40 a.m. UTC | #3

On Tue, Oct 12, 2021 at 09:25:15AM +0200, Alexander Sverdlin wrote:
> On Mon, 2021-09-13 at 23:43 +0200, Alexander Sverdlin wrote:

> > would you take the patch to a tree of yours, please?

> I still cannot find this patch in any of your trees, but I've found this one:

You ignored my question about dependencies:

    https://lore.kernel.org/all/[e-mail]/

so I've no idea if it's safe to apply or if other people might need this
one patch from the middle of the series.

>       video: ep93xx: Prepare clock before using it
>       dmaengine: ep93xx: Prepare clock before using it
>       ASoC: cirrus: i2s: Prepare clock before using it
>       pwm: ep93xx: Prepare clock before using it
>     
>     Nikita Shubin (1):
>       ep93xx: clock: convert in-place to COMMON_CLK
> 
> 
> ... which claims to merge both "ASoC: cirrus: i2s: Prepare clock before using it"
> and "ep93xx: clock: convert in-place to COMMON_CLK", but they are actually not
> merged.

No, it doesn't - that's the cover letter from your series.

> Could you please consider ASoC patch, while I will resubmit the final clock conversion?

So please answer my question then: what's the story with dependencies?

Patch

diff --git a/sound/soc/cirrus/ep93xx-i2s.c b/sound/soc/cirrus/ep93xx-i2s.c
index 0d26550d0df8..4d3179f03202 100644
--- a/sound/soc/cirrus/ep93xx-i2s.c
+++ b/sound/soc/cirrus/ep93xx-i2s.c
@@ -111,9 +111,9 @@  static void ep93xx_i2s_enable(struct ep93xx_i2s_info *info, int stream)
 	if ((ep93xx_i2s_read_reg(info, EP93XX_I2S_TX0EN) & 0x1) == 0 &&
 	    (ep93xx_i2s_read_reg(info, EP93XX_I2S_RX0EN) & 0x1) == 0) {
 		/* Enable clocks */
-		clk_enable(info->mclk);
-		clk_enable(info->sclk);
-		clk_enable(info->lrclk);
+		clk_prepare_enable(info->mclk);
+		clk_prepare_enable(info->sclk);
+		clk_prepare_enable(info->lrclk);
 
 		/* Enable i2s */
 		ep93xx_i2s_write_reg(info, EP93XX_I2S_GLCTRL, 1);
@@ -156,9 +156,9 @@  static void ep93xx_i2s_disable(struct ep93xx_i2s_info *info, int stream)
 		ep93xx_i2s_write_reg(info, EP93XX_I2S_GLCTRL, 0);
 
 		/* Disable clocks */
-		clk_disable(info->lrclk);
-		clk_disable(info->sclk);
-		clk_disable(info->mclk);
+		clk_disable_unprepare(info->lrclk);
+		clk_disable_unprepare(info->sclk);
+		clk_disable_unprepare(info->mclk);
 	}
 }