[PATCH V2] ASoC: fsl_ssi: refine ipg clock usage in this module

Move the ipg clock enable and disable operation to startup and shutdown,
that is only enable ipg clock when ssi is working. Keep clock is disabled
when ssi is in idle.
otherwise, _fsl_ssi_set_dai_fmt function need to be called in probe,
so add ipg clock control for it.

Signed-off-by: Shengjiu Wang <redacted>
---

change log
v2: update patch according to maintainer's review comments.

 sound/soc/fsl/fsl_ssi.c |   39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 2fc3e66..4447f95 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c

@@ -530,6 +530,11 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream,
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
 	struct fsl_ssi_private *ssi_private =
 		snd_soc_dai_get_drvdata(rtd->cpu_dai);
+	int ret;
+
+	ret = clk_prepare_enable(ssi_private->clk);
+	if (ret)
+		return ret;
 
 	/* When using dual fifo mode, it is safer to ensure an even period
 	 * size. If appearing to an odd number while DMA always starts its

@@ -544,6 +549,21 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream,
 }
 
 /**
+ * fsl_ssi_shutdown: shutdown the SSI
+ *
+ */
+static void fsl_ssi_shutdown(struct snd_pcm_substream *substream,
+                             struct snd_soc_dai *dai)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct fsl_ssi_private *ssi_private =
+		snd_soc_dai_get_drvdata(rtd->cpu_dai);
+
+	clk_disable_unprepare(ssi_private->clk);
+
+}
+
+/**
  * fsl_ssi_set_bclk - configure Digital Audio Interface bit clock
  *
  * Note: This function can be only called when using SSI as DAI master

@@ -771,6 +791,7 @@ static int _fsl_ssi_set_dai_fmt(struct fsl_ssi_private *ssi_private,
 	struct regmap *regs = ssi_private->regs;
 	u32 strcr = 0, stcr, srcr, scr, mask;
 	u8 wm;
+	int ret;
 
 	ssi_private->dai_fmt = fmt;
 

@@ -779,6 +800,10 @@ static int _fsl_ssi_set_dai_fmt(struct fsl_ssi_private *ssi_private,
 		return -EINVAL;
 	}
 
+	ret = clk_prepare_enable(ssi_private->clk);
+	if (ret)
+		return ret;
+
 	fsl_ssi_setup_reg_vals(ssi_private);
 
 	regmap_read(regs, CCSR_SSI_SCR, &scr);

@@ -811,6 +836,7 @@ static int _fsl_ssi_set_dai_fmt(struct fsl_ssi_private *ssi_private,
 			ssi_private->i2s_mode |= CCSR_SSI_SCR_I2S_MODE_SLAVE;
 			break;
 		default:
+			clk_disable_unprepare(ssi_private->clk);
 			return -EINVAL;
 		}
 

@@ -836,6 +862,7 @@ static int _fsl_ssi_set_dai_fmt(struct fsl_ssi_private *ssi_private,
 		ssi_private->i2s_mode |= CCSR_SSI_SCR_I2S_MODE_NORMAL;
 		break;
 	default:
+		clk_disable_unprepare(ssi_private->clk);
 		return -EINVAL;
 	}
 	scr |= ssi_private->i2s_mode;

@@ -859,6 +886,7 @@ static int _fsl_ssi_set_dai_fmt(struct fsl_ssi_private *ssi_private,
 		strcr ^= CCSR_SSI_STCR_TFSI;
 		break;
 	default:
+		clk_disable_unprepare(ssi_private->clk);
 		return -EINVAL;
 	}
 

@@ -877,6 +905,7 @@ static int _fsl_ssi_set_dai_fmt(struct fsl_ssi_private *ssi_private,
 		scr &= ~CCSR_SSI_SCR_SYS_CLK_EN;
 		break;
 	default:
+		clk_disable_unprepare(ssi_private->clk);
 		return -EINVAL;
 	}
 

@@ -925,6 +954,7 @@ static int _fsl_ssi_set_dai_fmt(struct fsl_ssi_private *ssi_private,
 	if (fmt & SND_SOC_DAIFMT_AC97)
 		fsl_ssi_setup_ac97(ssi_private);
 
+	clk_disable_unprepare(ssi_private->clk);
 	return 0;
 
 }

@@ -1043,6 +1073,7 @@ static int fsl_ssi_dai_probe(struct snd_soc_dai *dai)
 
 static const struct snd_soc_dai_ops fsl_ssi_dai_ops = {
 	.startup	= fsl_ssi_startup,
+	.shutdown       = fsl_ssi_shutdown,
 	.hw_params	= fsl_ssi_hw_params,
 	.hw_free	= fsl_ssi_hw_free,
 	.set_fmt	= fsl_ssi_set_dai_fmt,

@@ -1175,12 +1206,6 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
 		return ret;
 	}
 
-	ret = clk_prepare_enable(ssi_private->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret);
-		return ret;
-	}
-
 	/* For those SLAVE implementations, we ingore non-baudclk cases
 	 * and, instead, abandon MASTER mode that needs baud clock.
 	 */

@@ -1236,7 +1261,6 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
 	return 0;
 
 error_pcm:
-	clk_disable_unprepare(ssi_private->clk);
 
 	return ret;
 }

@@ -1246,7 +1270,6 @@ static void fsl_ssi_imx_clean(struct platform_device *pdev,
 {
 	if (!ssi_private->use_dma)
 		imx_pcm_fiq_exit(pdev);
-	clk_disable_unprepare(ssi_private->clk);
 }
 
 static int fsl_ssi_probe(struct platform_device *pdev)

-- 
1.7.9.5

On Thu, Sep 11, 2014 at 01:38:29PM +0800, Shengjiu Wang wrote:

Move the ipg clock enable and disable operation to startup and shutdown,
that is only enable ipg clock when ssi is working. Keep clock is disabled
when ssi is in idle.
otherwise, _fsl_ssi_set_dai_fmt function need to be called in probe,
so add ipg clock control for it.

It seems to be no objection so far against my last suggestion to
use regmap's mmio_clk() for named ipg clk only. So you may still
consider about that.

Anyway, I'd like to do thing in parallel. So I just simply tested
it on my side and its works fine, it may still need to be tested
by others though.

Nicolin

On Thu, Sep 11, 2014 at 03:57:37PM -0700, Nicolin Chen wrote:

On Thu, Sep 11, 2014 at 01:38:29PM +0800, Shengjiu Wang wrote:

quoted

Move the ipg clock enable and disable operation to startup and shutdown,
that is only enable ipg clock when ssi is working. Keep clock is disabled
when ssi is in idle.
otherwise, _fsl_ssi_set_dai_fmt function need to be called in probe,
so add ipg clock control for it.

It seems to be no objection so far against my last suggestion to
use regmap's mmio_clk() for named ipg clk only. So you may still
consider about that.

I think mmio_clk() can be put to another patch. and this patch only for clk_enable()
and clk_disable() operation.
 

Anyway, I'd like to do thing in parallel. So I just simply tested
it on my side and its works fine, it may still need to be tested
by others though.

Nicolina

Hi Markus

could you please review it, and share your comments?

wang shengjiu

Shengjiu Wang wrote:

+	ret = clk_prepare_enable(ssi_private->clk);
+	if (ret)
+		return ret;

Will this work on PowerPC, where ssi_private->clk is always NULL?

On Thu, Sep 11, 2014 at 09:43:59PM -0500, Timur Tabi wrote:

Shengjiu Wang wrote:

quoted

+	ret = clk_prepare_enable(ssi_private->clk);
+	if (ret)
+		return ret;

Will this work on PowerPC, where ssi_private->clk is always NULL?

When ssi_private->clk is NULL, then ret = 0, so here will not return,
the code in below will be continuously executed.
I think it work on PowerPC.

wang shengjiu

Hi,

On Fri, Sep 12, 2014 at 10:01:12AM +0800, Shengjiu Wang wrote:

On Thu, Sep 11, 2014 at 03:57:37PM -0700, Nicolin Chen wrote:

quoted

On Thu, Sep 11, 2014 at 01:38:29PM +0800, Shengjiu Wang wrote:

quoted

Move the ipg clock enable and disable operation to startup and shutdown,
that is only enable ipg clock when ssi is working. Keep clock is disabled
when ssi is in idle.
otherwise, _fsl_ssi_set_dai_fmt function need to be called in probe,
so add ipg clock control for it.

It seems to be no objection so far against my last suggestion to
use regmap's mmio_clk() for named ipg clk only. So you may still
consider about that.

I think mmio_clk() can be put to another patch. and this patch only for clk_enable()
and clk_disable() operation.

I would also prefer Nicolin's suggestion using regmap's mmio clk. I
think it may be better to not add this particular patch at all and just
go with the mmio_clk patch. It should be easy enough to just add the
clock names to the devicetrees. That way we can avoid all those clock
enable/disable function calls.

quoted

Anyway, I'd like to do thing in parallel. So I just simply tested
it on my side and its works fine, it may still need to be tested
by others though.

Nicolina

Hi Markus

could you please review it, and share your comments?

I think the clock enabling for AC97 is missing in your patch.

Best regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

On Fri, Sep 12, 2014 at 08:17:06AM +0200, Markus Pargmann wrote:

Hi,

On Fri, Sep 12, 2014 at 10:01:12AM +0800, Shengjiu Wang wrote:

quoted

On Thu, Sep 11, 2014 at 03:57:37PM -0700, Nicolin Chen wrote:

quoted

On Thu, Sep 11, 2014 at 01:38:29PM +0800, Shengjiu Wang wrote:

quoted

Move the ipg clock enable and disable operation to startup and shutdown,
that is only enable ipg clock when ssi is working. Keep clock is disabled
when ssi is in idle.
otherwise, _fsl_ssi_set_dai_fmt function need to be called in probe,
so add ipg clock control for it.

It seems to be no objection so far against my last suggestion to
use regmap's mmio_clk() for named ipg clk only. So you may still
consider about that.

I think mmio_clk() can be put to another patch. and this patch only for clk_enable()
and clk_disable() operation.

I would also prefer Nicolin's suggestion using regmap's mmio clk. I
think it may be better to not add this particular patch at all and just
go with the mmio_clk patch. It should be easy enough to just add the
clock names to the devicetrees. That way we can avoid all those clock
enable/disable function calls.

I considered if use Nicolin's suggestion, I still need ot add those
enable/disable function calls, because I want to remove the enable/disable
function call in fsl_ssi_imx_probe, then I need to add enable/disable 
function call in _fsl_ssi_set_dai_fmt(), which is called in fsl_ssi_probe().
_fsl_ssi_set_dai_fmt() need to access the registers.

quoted

quoted

Anyway, I'd like to do thing in parallel. So I just simply tested
it on my side and its works fine, it may still need to be tested
by others though.

Nicolina

Hi Markus

could you please review it, and share your comments?

I think the clock enabling for AC97 is missing in your patch.

I add clock enable in fsl_ssi_startup(), I think it is enough for AC97, does it?

wang shengjiu

Best regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

On Fri, Sep 12, 2014 at 03:14:28PM +0800, Shengjiu Wang wrote:

On Fri, Sep 12, 2014 at 08:17:06AM +0200, Markus Pargmann wrote:

quoted

Hi,

On Fri, Sep 12, 2014 at 10:01:12AM +0800, Shengjiu Wang wrote:

quoted

On Thu, Sep 11, 2014 at 03:57:37PM -0700, Nicolin Chen wrote:

quoted

On Thu, Sep 11, 2014 at 01:38:29PM +0800, Shengjiu Wang wrote:

quoted

Move the ipg clock enable and disable operation to startup and shutdown,
that is only enable ipg clock when ssi is working. Keep clock is disabled
when ssi is in idle.
otherwise, _fsl_ssi_set_dai_fmt function need to be called in probe,
so add ipg clock control for it.

It seems to be no objection so far against my last suggestion to
use regmap's mmio_clk() for named ipg clk only. So you may still
consider about that.

I think mmio_clk() can be put to another patch. and this patch only for clk_enable()
and clk_disable() operation.

I would also prefer Nicolin's suggestion using regmap's mmio clk. I
think it may be better to not add this particular patch at all and just
go with the mmio_clk patch. It should be easy enough to just add the
clock names to the devicetrees. That way we can avoid all those clock
enable/disable function calls.

I considered if use Nicolin's suggestion, I still need ot add those
enable/disable function calls, because I want to remove the enable/disable
function call in fsl_ssi_imx_probe, then I need to add enable/disable 
function call in _fsl_ssi_set_dai_fmt(), which is called in fsl_ssi_probe().
_fsl_ssi_set_dai_fmt() need to access the registers.

I think Nicolin's suggestion was to check for a clock named "ipg". If it
exists, you can simply use devm_regmap_init_mmio_clk(). Otherwise you
could fallback to the old behaviour and get the first clock and enable
it without disabling it after the probe function.

After that, you could add the clock-names property to the devicetrees
and have the same result.

quoted

quoted

quoted

Anyway, I'd like to do thing in parallel. So I just simply tested
it on my side and its works fine, it may still need to be tested
by others though.

Nicolina

Hi Markus

could you please review it, and share your comments?

I think the clock enabling for AC97 is missing in your patch.

I add clock enable in fsl_ssi_startup(), I think it is enough for AC97, does it?

No. We export ac97 read/write ops for the whole AC97 stuff. These may be
used outside of the usual startup/shutdown which is done for substreams.
It may work to have the ipg clock enabled in
fsl_ssi_ac97_read/fsl_ssi_ac97_write.

Best regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

On Fri, Sep 12, 2014 at 10:54:46AM +0200, Markus Pargmann wrote:

On Fri, Sep 12, 2014 at 03:14:28PM +0800, Shengjiu Wang wrote:

quoted

On Fri, Sep 12, 2014 at 08:17:06AM +0200, Markus Pargmann wrote:

quoted

Hi,

On Fri, Sep 12, 2014 at 10:01:12AM +0800, Shengjiu Wang wrote:

quoted

On Thu, Sep 11, 2014 at 03:57:37PM -0700, Nicolin Chen wrote:

quoted

On Thu, Sep 11, 2014 at 01:38:29PM +0800, Shengjiu Wang wrote:

quoted

Move the ipg clock enable and disable operation to startup and shutdown,
that is only enable ipg clock when ssi is working. Keep clock is disabled
when ssi is in idle.
otherwise, _fsl_ssi_set_dai_fmt function need to be called in probe,
so add ipg clock control for it.

It seems to be no objection so far against my last suggestion to
use regmap's mmio_clk() for named ipg clk only. So you may still
consider about that.

I think mmio_clk() can be put to another patch. and this patch only for clk_enable()
and clk_disable() operation.

I would also prefer Nicolin's suggestion using regmap's mmio clk. I
think it may be better to not add this particular patch at all and just
go with the mmio_clk patch. It should be easy enough to just add the
clock names to the devicetrees. That way we can avoid all those clock
enable/disable function calls.

I considered if use Nicolin's suggestion, I still need ot add those
enable/disable function calls, because I want to remove the enable/disable
function call in fsl_ssi_imx_probe, then I need to add enable/disable 
function call in _fsl_ssi_set_dai_fmt(), which is called in fsl_ssi_probe().
_fsl_ssi_set_dai_fmt() need to access the registers.

I think Nicolin's suggestion was to check for a clock named "ipg". If it
exists, you can simply use devm_regmap_init_mmio_clk(). Otherwise you
could fallback to the old behaviour and get the first clock and enable
it without disabling it after the probe function.

After that, you could add the clock-names property to the devicetrees
and have the same result.

Ok, I have sent V3 for review. please check.

wang shengjiu 

quoted

quoted

quoted

quoted

Anyway, I'd like to do thing in parallel. So I just simply tested
it on my side and its works fine, it may still need to be tested
by others though.

Nicolina

Hi Markus

could you please review it, and share your comments?

I think the clock enabling for AC97 is missing in your patch.

I add clock enable in fsl_ssi_startup(), I think it is enough for AC97, does it?

No. We export ac97 read/write ops for the whole AC97 stuff. These may be
used outside of the usual startup/shutdown which is done for substreams.
It may work to have the ipg clock enabled in
fsl_ssi_ac97_read/fsl_ssi_ac97_write.

Best regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |