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

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

[Shengjiu Wang]

Check if ipg clock is in clock-names property, then we can move the
ipg clock enable and disable operation to startup and shutdown, that
is only enable ipg clock when ssi is working and keep clock is disabled
when ssi is in idle.
But when the checking is failed, remain the clock control as before.

Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
---
V3 change log:
update patch according Nicolin and markus's comments


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

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 2fc3e66..6d1dfd5 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -169,6 +169,7 @@ struct fsl_ssi_private {
 	u8 i2s_mode;
 	bool use_dma;
 	bool use_dual_fifo;
+	bool has_ipg_clk_name;
 	unsigned int fifo_depth;
 	struct fsl_ssi_rxtx_reg_val rxtx_reg_val;
 
@@ -530,6 +531,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 +550,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
@@ -1043,6 +1064,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,
@@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
 	u32 dmas[4];
 	int ret;
 
-	ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
+	if (ssi_private->has_ipg_clk_name)
+		ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
+	else
+		ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(ssi_private->clk)) {
 		ret = PTR_ERR(ssi_private->clk);
 		dev_err(&pdev->dev, "could not get clock: %d\n", ret);
 		return ret;
 	}
 
-	ret = clk_prepare_enable(ssi_private->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret);
-		return ret;
+	if (!ssi_private->has_ipg_clk_name) {
+		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
@@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
 	return 0;
 
 error_pcm:
-	clk_disable_unprepare(ssi_private->clk);
 
+	if (!ssi_private->has_ipg_clk_name)
+		clk_disable_unprepare(ssi_private->clk);
 	return ret;
 }
 
@@ -1246,7 +1274,8 @@ 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);
+	if (!ssi_private->has_ipg_clk_name)
+		clk_disable_unprepare(ssi_private->clk);
 }
 
 static int fsl_ssi_probe(struct platform_device *pdev)
@@ -1321,8 +1350,16 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
+	ret = of_property_match_string(np, "clock-names", "ipg");
+	if (ret < 0) {
+		ssi_private->has_ipg_clk_name = false;
+		ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
 			&fsl_ssi_regconfig);
+	} else {
+		ssi_private->has_ipg_clk_name = true;
+		ssi_private->regs = devm_regmap_init_mmio_clk(&pdev->dev,
+			"ipg", iomem, &fsl_ssi_regconfig);
+	}
 	if (IS_ERR(ssi_private->regs)) {
 		dev_err(&pdev->dev, "Failed to init register map\n");
 		return PTR_ERR(ssi_private->regs);
-- 
1.7.9.5

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

[Shengjiu Wang]

Check if ipg clock is in clock-names property, then we can move the
ipg clock enable and disable operation to startup and shutdown, that
is only enable ipg clock when ssi is working and keep clock is disabled
when ssi is in idle.
But when the checking is failed, remain the clock control as before.

Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
---
V3 change log:
update patch according Nicolin and markus's comments


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

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 2fc3e66..6d1dfd5 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -169,6 +169,7 @@ struct fsl_ssi_private {
 	u8 i2s_mode;
 	bool use_dma;
 	bool use_dual_fifo;
+	bool has_ipg_clk_name;
 	unsigned int fifo_depth;
 	struct fsl_ssi_rxtx_reg_val rxtx_reg_val;
 
@@ -530,6 +531,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 +550,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
@@ -1043,6 +1064,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,
@@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
 	u32 dmas[4];
 	int ret;
 
-	ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
+	if (ssi_private->has_ipg_clk_name)
+		ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
+	else
+		ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(ssi_private->clk)) {
 		ret = PTR_ERR(ssi_private->clk);
 		dev_err(&pdev->dev, "could not get clock: %d\n", ret);
 		return ret;
 	}
 
-	ret = clk_prepare_enable(ssi_private->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret);
-		return ret;
+	if (!ssi_private->has_ipg_clk_name) {
+		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
@@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
 	return 0;
 
 error_pcm:
-	clk_disable_unprepare(ssi_private->clk);
 
+	if (!ssi_private->has_ipg_clk_name)
+		clk_disable_unprepare(ssi_private->clk);
 	return ret;
 }
 
@@ -1246,7 +1274,8 @@ 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);
+	if (!ssi_private->has_ipg_clk_name)
+		clk_disable_unprepare(ssi_private->clk);
 }
 
 static int fsl_ssi_probe(struct platform_device *pdev)
@@ -1321,8 +1350,16 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
+	ret = of_property_match_string(np, "clock-names", "ipg");
+	if (ret < 0) {
+		ssi_private->has_ipg_clk_name = false;
+		ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
 			&fsl_ssi_regconfig);
+	} else {
+		ssi_private->has_ipg_clk_name = true;
+		ssi_private->regs = devm_regmap_init_mmio_clk(&pdev->dev,
+			"ipg", iomem, &fsl_ssi_regconfig);
+	}
 	if (IS_ERR(ssi_private->regs)) {
 		dev_err(&pdev->dev, "Failed to init register map\n");
 		return PTR_ERR(ssi_private->regs);
-- 
1.7.9.5

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

[Shengjiu Wang]

Check if ipg clock is in clock-names property, then we can move the
ipg clock enable and disable operation to startup and shutdown, that
is only enable ipg clock when ssi is working and keep clock is disabled
when ssi is in idle.
But when the checking is failed, remain the clock control as before.

Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
---
V3 change log:
update patch according Nicolin and markus's comments


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

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 2fc3e66..6d1dfd5 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -169,6 +169,7 @@ struct fsl_ssi_private {
 	u8 i2s_mode;
 	bool use_dma;
 	bool use_dual_fifo;
+	bool has_ipg_clk_name;
 	unsigned int fifo_depth;
 	struct fsl_ssi_rxtx_reg_val rxtx_reg_val;
 
@@ -530,6 +531,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 +550,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
@@ -1043,6 +1064,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,
@@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
 	u32 dmas[4];
 	int ret;
 
-	ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
+	if (ssi_private->has_ipg_clk_name)
+		ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
+	else
+		ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(ssi_private->clk)) {
 		ret = PTR_ERR(ssi_private->clk);
 		dev_err(&pdev->dev, "could not get clock: %d\n", ret);
 		return ret;
 	}
 
-	ret = clk_prepare_enable(ssi_private->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret);
-		return ret;
+	if (!ssi_private->has_ipg_clk_name) {
+		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
@@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
 	return 0;
 
 error_pcm:
-	clk_disable_unprepare(ssi_private->clk);
 
+	if (!ssi_private->has_ipg_clk_name)
+		clk_disable_unprepare(ssi_private->clk);
 	return ret;
 }
 
@@ -1246,7 +1274,8 @@ 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);
+	if (!ssi_private->has_ipg_clk_name)
+		clk_disable_unprepare(ssi_private->clk);
 }
 
 static int fsl_ssi_probe(struct platform_device *pdev)
@@ -1321,8 +1350,16 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
+	ret = of_property_match_string(np, "clock-names", "ipg");
+	if (ret < 0) {
+		ssi_private->has_ipg_clk_name = false;
+		ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
 			&fsl_ssi_regconfig);
+	} else {
+		ssi_private->has_ipg_clk_name = true;
+		ssi_private->regs = devm_regmap_init_mmio_clk(&pdev->dev,
+			"ipg", iomem, &fsl_ssi_regconfig);
+	}
 	if (IS_ERR(ssi_private->regs)) {
 		dev_err(&pdev->dev, "Failed to init register map\n");
 		return PTR_ERR(ssi_private->regs);
-- 
1.7.9.5

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

[Nicolin Chen]

On Fri, Sep 12, 2014 at 06:35:15PM +0800, Shengjiu Wang wrote:
> Check if ipg clock is in clock-names property, then we can move the
> ipg clock enable and disable operation to startup and shutdown, that
> is only enable ipg clock when ssi is working and keep clock is disabled
> when ssi is in idle.
> But when the checking is failed, remain the clock control as before.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>

The patch looks fine to me. I tested the has_ipg_clk_name == true
case, and it works fine. As long as there's no further objection,
I can give an Acked-by. (I think Timur and Markus may like to have
a test as well.)

Thank you
Nicolin

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

[Nicolin Chen]

On Fri, Sep 12, 2014 at 06:35:15PM +0800, Shengjiu Wang wrote:
> Check if ipg clock is in clock-names property, then we can move the
> ipg clock enable and disable operation to startup and shutdown, that
> is only enable ipg clock when ssi is working and keep clock is disabled
> when ssi is in idle.
> But when the checking is failed, remain the clock control as before.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>

The patch looks fine to me. I tested the has_ipg_clk_name == true
case, and it works fine. As long as there's no further objection,
I can give an Acked-by. (I think Timur and Markus may like to have
a test as well.)

Thank you
Nicolin

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

[Michael Trimarchi]

Hi

Il 12/set/2014 14:18 "Shengjiu Wang" <shengjiu.wang@freescale.com> ha
scritto:
>
> Check if ipg clock is in clock-names property, then we can move the
> ipg clock enable and disable operation to startup and shutdown, that
> is only enable ipg clock when ssi is working and keep clock is disabled
> when ssi is in idle.
> But when the checking is failed, remain the clock control as before.
>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> ---
> V3 change log:
> update patch according Nicolin and markus's comments
>
>
>  sound/soc/fsl/fsl_ssi.c |   53
++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 45 insertions(+), 8 deletions(-)
>
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 2fc3e66..6d1dfd5 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -169,6 +169,7 @@ struct fsl_ssi_private {
>         u8 i2s_mode;
>         bool use_dma;
>         bool use_dual_fifo;
> +       bool has_ipg_clk_name;
>         unsigned int fifo_depth;
>         struct fsl_ssi_rxtx_reg_val rxtx_reg_val;
>
> @@ -530,6 +531,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 +550,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
> @@ -1043,6 +1064,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,
> @@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct
platform_device *pdev,
>         u32 dmas[4];
>         int ret;
>
> -       ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> +       if (ssi_private->has_ipg_clk_name)
> +               ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
> +       else
> +               ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
>         if (IS_ERR(ssi_private->clk)) {
>                 ret = PTR_ERR(ssi_private->clk);
>                 dev_err(&pdev->dev, "could not get clock: %d\n", ret);
>                 return ret;
>         }
>
> -       ret = clk_prepare_enable(ssi_private->clk);
> -       if (ret) {
> -               dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n",
ret);
> -               return ret;
> +       if (!ssi_private->has_ipg_clk_name) {
> +               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
> @@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct platform_device
*pdev,
>         return 0;
>
>  error_pcm:
> -       clk_disable_unprepare(ssi_private->clk);
>
> +       if (!ssi_private->has_ipg_clk_name)
> +               clk_disable_unprepare(ssi_private->clk);
>         return ret;
>  }
>
> @@ -1246,7 +1274,8 @@ 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);
> +       if (!ssi_private->has_ipg_clk_name)
> +               clk_disable_unprepare(ssi_private->clk);
>  }
>
>  static int fsl_ssi_probe(struct platform_device *pdev)
> @@ -1321,8 +1350,16 @@ static int fsl_ssi_probe(struct platform_device
*pdev)
>                 return -ENOMEM;
>         }
>
> -       ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> +       ret = of_property_match_string(np, "clock-names", "ipg");
> +       if (ret < 0) {
> +               ssi_private->has_ipg_clk_name = false;

It's false already.

Michael

> +               ssi_private->regs = devm_regmap_init_mmio(&pdev->dev,
iomem,
>                         &fsl_ssi_regconfig);
> +       } else {
> +               ssi_private->has_ipg_clk_name = true;
> +               ssi_private->regs = devm_regmap_init_mmio_clk(&pdev->dev,
> +                       "ipg", iomem, &fsl_ssi_regconfig);
> +       }
>         if (IS_ERR(ssi_private->regs)) {
>                 dev_err(&pdev->dev, "Failed to init register map\n");
>                 return PTR_ERR(ssi_private->regs);
> --
> 1.7.9.5
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH V3] ASoC: fsl_ssi: refine ipg clock usage in this module

[Michael Trimarchi]

[-- Attachment #1: Type: text/plain, Size: 5613 bytes --]

Hi

Il 12/set/2014 14:18 "Shengjiu Wang" <shengjiu.wang@freescale.com> ha
scritto:
>
> Check if ipg clock is in clock-names property, then we can move the
> ipg clock enable and disable operation to startup and shutdown, that
> is only enable ipg clock when ssi is working and keep clock is disabled
> when ssi is in idle.
> But when the checking is failed, remain the clock control as before.
>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> ---
> V3 change log:
> update patch according Nicolin and markus's comments
>
>
>  sound/soc/fsl/fsl_ssi.c |   53
++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 45 insertions(+), 8 deletions(-)
>
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 2fc3e66..6d1dfd5 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -169,6 +169,7 @@ struct fsl_ssi_private {
>         u8 i2s_mode;
>         bool use_dma;
>         bool use_dual_fifo;
> +       bool has_ipg_clk_name;
>         unsigned int fifo_depth;
>         struct fsl_ssi_rxtx_reg_val rxtx_reg_val;
>
> @@ -530,6 +531,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 +550,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
> @@ -1043,6 +1064,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,
> @@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct
platform_device *pdev,
>         u32 dmas[4];
>         int ret;
>
> -       ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> +       if (ssi_private->has_ipg_clk_name)
> +               ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
> +       else
> +               ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
>         if (IS_ERR(ssi_private->clk)) {
>                 ret = PTR_ERR(ssi_private->clk);
>                 dev_err(&pdev->dev, "could not get clock: %d\n", ret);
>                 return ret;
>         }
>
> -       ret = clk_prepare_enable(ssi_private->clk);
> -       if (ret) {
> -               dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n",
ret);
> -               return ret;
> +       if (!ssi_private->has_ipg_clk_name) {
> +               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
> @@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct platform_device
*pdev,
>         return 0;
>
>  error_pcm:
> -       clk_disable_unprepare(ssi_private->clk);
>
> +       if (!ssi_private->has_ipg_clk_name)
> +               clk_disable_unprepare(ssi_private->clk);
>         return ret;
>  }
>
> @@ -1246,7 +1274,8 @@ 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);
> +       if (!ssi_private->has_ipg_clk_name)
> +               clk_disable_unprepare(ssi_private->clk);
>  }
>
>  static int fsl_ssi_probe(struct platform_device *pdev)
> @@ -1321,8 +1350,16 @@ static int fsl_ssi_probe(struct platform_device
*pdev)
>                 return -ENOMEM;
>         }
>
> -       ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> +       ret = of_property_match_string(np, "clock-names", "ipg");
> +       if (ret < 0) {
> +               ssi_private->has_ipg_clk_name = false;

It's false already.

Michael

> +               ssi_private->regs = devm_regmap_init_mmio(&pdev->dev,
iomem,
>                         &fsl_ssi_regconfig);
> +       } else {
> +               ssi_private->has_ipg_clk_name = true;
> +               ssi_private->regs = devm_regmap_init_mmio_clk(&pdev->dev,
> +                       "ipg", iomem, &fsl_ssi_regconfig);
> +       }
>         if (IS_ERR(ssi_private->regs)) {
>                 dev_err(&pdev->dev, "Failed to init register map\n");
>                 return PTR_ERR(ssi_private->regs);
> --
> 1.7.9.5
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[-- Attachment #2: Type: text/html, Size: 7825 bytes --]

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

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 5842 bytes --]

On Fri, Sep 12, 2014 at 06:35:15PM +0800, Shengjiu Wang wrote:
> Check if ipg clock is in clock-names property, then we can move the
> ipg clock enable and disable operation to startup and shutdown, that
> is only enable ipg clock when ssi is working and keep clock is disabled
> when ssi is in idle.
> But when the checking is failed, remain the clock control as before.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> ---
> V3 change log:
> update patch according Nicolin and markus's comments
> 
> 
>  sound/soc/fsl/fsl_ssi.c |   53 ++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 45 insertions(+), 8 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 2fc3e66..6d1dfd5 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -169,6 +169,7 @@ struct fsl_ssi_private {
>  	u8 i2s_mode;
>  	bool use_dma;
>  	bool use_dual_fifo;
> +	bool has_ipg_clk_name;
>  	unsigned int fifo_depth;
>  	struct fsl_ssi_rxtx_reg_val rxtx_reg_val;
>  
> @@ -530,6 +531,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 +550,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
> @@ -1043,6 +1064,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,
> @@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
>  	u32 dmas[4];
>  	int ret;
>  
> -	ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (ssi_private->has_ipg_clk_name)
> +		ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
> +	else
> +		ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(ssi_private->clk)) {
>  		ret = PTR_ERR(ssi_private->clk);
>  		dev_err(&pdev->dev, "could not get clock: %d\n", ret);
>  		return ret;
>  	}
>  
> -	ret = clk_prepare_enable(ssi_private->clk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret);
> -		return ret;
> +	if (!ssi_private->has_ipg_clk_name) {
> +		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
> @@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
>  	return 0;
>  
>  error_pcm:
> -	clk_disable_unprepare(ssi_private->clk);
>  
> +	if (!ssi_private->has_ipg_clk_name)
> +		clk_disable_unprepare(ssi_private->clk);
>  	return ret;
>  }
>  
> @@ -1246,7 +1274,8 @@ 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);
> +	if (!ssi_private->has_ipg_clk_name)
> +		clk_disable_unprepare(ssi_private->clk);
>  }
>  
>  static int fsl_ssi_probe(struct platform_device *pdev)
> @@ -1321,8 +1350,16 @@ static int fsl_ssi_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	}
>  
> -	ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> +	ret = of_property_match_string(np, "clock-names", "ipg");
> +	if (ret < 0) {
> +		ssi_private->has_ipg_clk_name = false;
> +		ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
>  			&fsl_ssi_regconfig);

Sorry if I was unclear about that. My suggestion was to enable the clock
right here:
		clk_prepare_enable(ssi_private->clk);

Then you can remove ssi_private->has_ipg_clk_name and all
clk_prepare_enable() and clk_disable_unprepare() from above. Also you
can move the devm_clk_get() into this block.

It seems you really want to implement this for devicetrees where the
"ipg" clock-name is missing, but I don't understand why? I really can't
see any benefit of adding all these clk_prepare_enable() calls for all
cornercases that may occure. For example the clocks for AC97 are still
missing in this version.

Best regards,

Markus

> +	} else {
> +		ssi_private->has_ipg_clk_name = true;
> +		ssi_private->regs = devm_regmap_init_mmio_clk(&pdev->dev,
> +			"ipg", iomem, &fsl_ssi_regconfig);
> +	}
>  	if (IS_ERR(ssi_private->regs)) {
>  		dev_err(&pdev->dev, "Failed to init register map\n");
>  		return PTR_ERR(ssi_private->regs);
> -- 
> 1.7.9.5
> 
> 

-- 
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 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 5842 bytes --]

On Fri, Sep 12, 2014 at 06:35:15PM +0800, Shengjiu Wang wrote:
> Check if ipg clock is in clock-names property, then we can move the
> ipg clock enable and disable operation to startup and shutdown, that
> is only enable ipg clock when ssi is working and keep clock is disabled
> when ssi is in idle.
> But when the checking is failed, remain the clock control as before.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> ---
> V3 change log:
> update patch according Nicolin and markus's comments
> 
> 
>  sound/soc/fsl/fsl_ssi.c |   53 ++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 45 insertions(+), 8 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 2fc3e66..6d1dfd5 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -169,6 +169,7 @@ struct fsl_ssi_private {
>  	u8 i2s_mode;
>  	bool use_dma;
>  	bool use_dual_fifo;
> +	bool has_ipg_clk_name;
>  	unsigned int fifo_depth;
>  	struct fsl_ssi_rxtx_reg_val rxtx_reg_val;
>  
> @@ -530,6 +531,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 +550,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
> @@ -1043,6 +1064,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,
> @@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
>  	u32 dmas[4];
>  	int ret;
>  
> -	ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (ssi_private->has_ipg_clk_name)
> +		ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
> +	else
> +		ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(ssi_private->clk)) {
>  		ret = PTR_ERR(ssi_private->clk);
>  		dev_err(&pdev->dev, "could not get clock: %d\n", ret);
>  		return ret;
>  	}
>  
> -	ret = clk_prepare_enable(ssi_private->clk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret);
> -		return ret;
> +	if (!ssi_private->has_ipg_clk_name) {
> +		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
> @@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
>  	return 0;
>  
>  error_pcm:
> -	clk_disable_unprepare(ssi_private->clk);
>  
> +	if (!ssi_private->has_ipg_clk_name)
> +		clk_disable_unprepare(ssi_private->clk);
>  	return ret;
>  }
>  
> @@ -1246,7 +1274,8 @@ 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);
> +	if (!ssi_private->has_ipg_clk_name)
> +		clk_disable_unprepare(ssi_private->clk);
>  }
>  
>  static int fsl_ssi_probe(struct platform_device *pdev)
> @@ -1321,8 +1350,16 @@ static int fsl_ssi_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	}
>  
> -	ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> +	ret = of_property_match_string(np, "clock-names", "ipg");
> +	if (ret < 0) {
> +		ssi_private->has_ipg_clk_name = false;
> +		ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
>  			&fsl_ssi_regconfig);

Sorry if I was unclear about that. My suggestion was to enable the clock
right here:
		clk_prepare_enable(ssi_private->clk);

Then you can remove ssi_private->has_ipg_clk_name and all
clk_prepare_enable() and clk_disable_unprepare() from above. Also you
can move the devm_clk_get() into this block.

It seems you really want to implement this for devicetrees where the
"ipg" clock-name is missing, but I don't understand why? I really can't
see any benefit of adding all these clk_prepare_enable() calls for all
cornercases that may occure. For example the clocks for AC97 are still
missing in this version.

Best regards,

Markus

> +	} else {
> +		ssi_private->has_ipg_clk_name = true;
> +		ssi_private->regs = devm_regmap_init_mmio_clk(&pdev->dev,
> +			"ipg", iomem, &fsl_ssi_regconfig);
> +	}
>  	if (IS_ERR(ssi_private->regs)) {
>  		dev_err(&pdev->dev, "Failed to init register map\n");
>  		return PTR_ERR(ssi_private->regs);
> -- 
> 1.7.9.5
> 
> 

-- 
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 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Shengjiu Wang]

On Mon, Sep 15, 2014 at 12:05:41PM +0200, Markus Pargmann wrote:
> On Fri, Sep 12, 2014 at 06:35:15PM +0800, Shengjiu Wang wrote:
> > Check if ipg clock is in clock-names property, then we can move the
> > ipg clock enable and disable operation to startup and shutdown, that
> > is only enable ipg clock when ssi is working and keep clock is disabled
> > when ssi is in idle.
> > But when the checking is failed, remain the clock control as before.
> > 
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> > ---
> > V3 change log:
> > update patch according Nicolin and markus's comments
> > 
> > 
> >  sound/soc/fsl/fsl_ssi.c |   53 ++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 45 insertions(+), 8 deletions(-)
> > 
> > diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> > index 2fc3e66..6d1dfd5 100644
> > --- a/sound/soc/fsl/fsl_ssi.c
> > +++ b/sound/soc/fsl/fsl_ssi.c
> > @@ -169,6 +169,7 @@ struct fsl_ssi_private {
> >  	u8 i2s_mode;
> >  	bool use_dma;
> >  	bool use_dual_fifo;
> > +	bool has_ipg_clk_name;
> >  	unsigned int fifo_depth;
> >  	struct fsl_ssi_rxtx_reg_val rxtx_reg_val;
> >  
> > @@ -530,6 +531,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 +550,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
> > @@ -1043,6 +1064,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,
> > @@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
> >  	u32 dmas[4];
> >  	int ret;
> >  
> > -	ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (ssi_private->has_ipg_clk_name)
> > +		ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
> > +	else
> > +		ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> >  	if (IS_ERR(ssi_private->clk)) {
> >  		ret = PTR_ERR(ssi_private->clk);
> >  		dev_err(&pdev->dev, "could not get clock: %d\n", ret);
> >  		return ret;
> >  	}
> >  
> > -	ret = clk_prepare_enable(ssi_private->clk);
> > -	if (ret) {
> > -		dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret);
> > -		return ret;
> > +	if (!ssi_private->has_ipg_clk_name) {
> > +		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
> > @@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
> >  	return 0;
> >  
> >  error_pcm:
> > -	clk_disable_unprepare(ssi_private->clk);
> >  
> > +	if (!ssi_private->has_ipg_clk_name)
> > +		clk_disable_unprepare(ssi_private->clk);
> >  	return ret;
> >  }
> >  
> > @@ -1246,7 +1274,8 @@ 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);
> > +	if (!ssi_private->has_ipg_clk_name)
> > +		clk_disable_unprepare(ssi_private->clk);
> >  }
> >  
> >  static int fsl_ssi_probe(struct platform_device *pdev)
> > @@ -1321,8 +1350,16 @@ static int fsl_ssi_probe(struct platform_device *pdev)
> >  		return -ENOMEM;
> >  	}
> >  
> > -	ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> > +	ret = of_property_match_string(np, "clock-names", "ipg");
> > +	if (ret < 0) {
> > +		ssi_private->has_ipg_clk_name = false;
> > +		ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> >  			&fsl_ssi_regconfig);
> 
> Sorry if I was unclear about that. My suggestion was to enable the clock
> right here:
> 		clk_prepare_enable(ssi_private->clk);
> 
> Then you can remove ssi_private->has_ipg_clk_name and all
> clk_prepare_enable() and clk_disable_unprepare() from above. Also you
> can move the devm_clk_get() into this block.
>

ipg clock not only need to be enabled when accessing register, but also 
need to be enabled when ssi is working. So I add clk_enable in startup.
 
> It seems you really want to implement this for devicetrees where the
> "ipg" clock-name is missing, but I don't understand why? I really can't
> see any benefit of adding all these clk_prepare_enable() calls for all
> cornercases that may occure. For example the clocks for AC97 are still
> missing in this version.

When ipg clock-name is missing, I just remain the code logic as before.
So AC97 case is same as before too. 
When have ipg clock-name, register it to regmap for accessing register,
and enabling ipg clock in startup also is useful for AC97.

I don't understand why you think clock for AC97 is still missing?

wang shengjiu

> 
> Best regards,
> 
> Markus
> 
> > +	} else {
> > +		ssi_private->has_ipg_clk_name = true;
> > +		ssi_private->regs = devm_regmap_init_mmio_clk(&pdev->dev,
> > +			"ipg", iomem, &fsl_ssi_regconfig);
> > +	}
> >  	if (IS_ERR(ssi_private->regs)) {
> >  		dev_err(&pdev->dev, "Failed to init register map\n");
> >  		return PTR_ERR(ssi_private->regs);
> > -- 
> > 1.7.9.5
> > 
> > 
> 
> -- 
> 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 |

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

[Shengjiu Wang]

On Mon, Sep 15, 2014 at 12:05:41PM +0200, Markus Pargmann wrote:
> On Fri, Sep 12, 2014 at 06:35:15PM +0800, Shengjiu Wang wrote:
> > Check if ipg clock is in clock-names property, then we can move the
> > ipg clock enable and disable operation to startup and shutdown, that
> > is only enable ipg clock when ssi is working and keep clock is disabled
> > when ssi is in idle.
> > But when the checking is failed, remain the clock control as before.
> > 
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> > ---
> > V3 change log:
> > update patch according Nicolin and markus's comments
> > 
> > 
> >  sound/soc/fsl/fsl_ssi.c |   53 ++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 45 insertions(+), 8 deletions(-)
> > 
> > diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> > index 2fc3e66..6d1dfd5 100644
> > --- a/sound/soc/fsl/fsl_ssi.c
> > +++ b/sound/soc/fsl/fsl_ssi.c
> > @@ -169,6 +169,7 @@ struct fsl_ssi_private {
> >  	u8 i2s_mode;
> >  	bool use_dma;
> >  	bool use_dual_fifo;
> > +	bool has_ipg_clk_name;
> >  	unsigned int fifo_depth;
> >  	struct fsl_ssi_rxtx_reg_val rxtx_reg_val;
> >  
> > @@ -530,6 +531,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 +550,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
> > @@ -1043,6 +1064,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,
> > @@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
> >  	u32 dmas[4];
> >  	int ret;
> >  
> > -	ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (ssi_private->has_ipg_clk_name)
> > +		ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
> > +	else
> > +		ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> >  	if (IS_ERR(ssi_private->clk)) {
> >  		ret = PTR_ERR(ssi_private->clk);
> >  		dev_err(&pdev->dev, "could not get clock: %d\n", ret);
> >  		return ret;
> >  	}
> >  
> > -	ret = clk_prepare_enable(ssi_private->clk);
> > -	if (ret) {
> > -		dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret);
> > -		return ret;
> > +	if (!ssi_private->has_ipg_clk_name) {
> > +		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
> > @@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
> >  	return 0;
> >  
> >  error_pcm:
> > -	clk_disable_unprepare(ssi_private->clk);
> >  
> > +	if (!ssi_private->has_ipg_clk_name)
> > +		clk_disable_unprepare(ssi_private->clk);
> >  	return ret;
> >  }
> >  
> > @@ -1246,7 +1274,8 @@ 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);
> > +	if (!ssi_private->has_ipg_clk_name)
> > +		clk_disable_unprepare(ssi_private->clk);
> >  }
> >  
> >  static int fsl_ssi_probe(struct platform_device *pdev)
> > @@ -1321,8 +1350,16 @@ static int fsl_ssi_probe(struct platform_device *pdev)
> >  		return -ENOMEM;
> >  	}
> >  
> > -	ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> > +	ret = of_property_match_string(np, "clock-names", "ipg");
> > +	if (ret < 0) {
> > +		ssi_private->has_ipg_clk_name = false;
> > +		ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> >  			&fsl_ssi_regconfig);
> 
> Sorry if I was unclear about that. My suggestion was to enable the clock
> right here:
> 		clk_prepare_enable(ssi_private->clk);
> 
> Then you can remove ssi_private->has_ipg_clk_name and all
> clk_prepare_enable() and clk_disable_unprepare() from above. Also you
> can move the devm_clk_get() into this block.
>

ipg clock not only need to be enabled when accessing register, but also 
need to be enabled when ssi is working. So I add clk_enable in startup.
 
> It seems you really want to implement this for devicetrees where the
> "ipg" clock-name is missing, but I don't understand why? I really can't
> see any benefit of adding all these clk_prepare_enable() calls for all
> cornercases that may occure. For example the clocks for AC97 are still
> missing in this version.

When ipg clock-name is missing, I just remain the code logic as before.
So AC97 case is same as before too. 
When have ipg clock-name, register it to regmap for accessing register,
and enabling ipg clock in startup also is useful for AC97.

I don't understand why you think clock for AC97 is still missing?

wang shengjiu

> 
> Best regards,
> 
> Markus
> 
> > +	} else {
> > +		ssi_private->has_ipg_clk_name = true;
> > +		ssi_private->regs = devm_regmap_init_mmio_clk(&pdev->dev,
> > +			"ipg", iomem, &fsl_ssi_regconfig);
> > +	}
> >  	if (IS_ERR(ssi_private->regs)) {
> >  		dev_err(&pdev->dev, "Failed to init register map\n");
> >  		return PTR_ERR(ssi_private->regs);
> > -- 
> > 1.7.9.5
> > 
> > 
> 
> -- 
> 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 |

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

[Shengjiu Wang]

On Mon, Sep 15, 2014 at 12:05:41PM +0200, Markus Pargmann wrote:
> On Fri, Sep 12, 2014 at 06:35:15PM +0800, Shengjiu Wang wrote:
> > Check if ipg clock is in clock-names property, then we can move the
> > ipg clock enable and disable operation to startup and shutdown, that
> > is only enable ipg clock when ssi is working and keep clock is disabled
> > when ssi is in idle.
> > But when the checking is failed, remain the clock control as before.
> > 
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> > ---
> > V3 change log:
> > update patch according Nicolin and markus's comments
> > 
> > 
> >  sound/soc/fsl/fsl_ssi.c |   53 ++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 45 insertions(+), 8 deletions(-)
> > 
> > diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> > index 2fc3e66..6d1dfd5 100644
> > --- a/sound/soc/fsl/fsl_ssi.c
> > +++ b/sound/soc/fsl/fsl_ssi.c
> > @@ -169,6 +169,7 @@ struct fsl_ssi_private {
> >  	u8 i2s_mode;
> >  	bool use_dma;
> >  	bool use_dual_fifo;
> > +	bool has_ipg_clk_name;
> >  	unsigned int fifo_depth;
> >  	struct fsl_ssi_rxtx_reg_val rxtx_reg_val;
> >  
> > @@ -530,6 +531,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 +550,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
> > @@ -1043,6 +1064,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,
> > @@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
> >  	u32 dmas[4];
> >  	int ret;
> >  
> > -	ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (ssi_private->has_ipg_clk_name)
> > +		ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
> > +	else
> > +		ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> >  	if (IS_ERR(ssi_private->clk)) {
> >  		ret = PTR_ERR(ssi_private->clk);
> >  		dev_err(&pdev->dev, "could not get clock: %d\n", ret);
> >  		return ret;
> >  	}
> >  
> > -	ret = clk_prepare_enable(ssi_private->clk);
> > -	if (ret) {
> > -		dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret);
> > -		return ret;
> > +	if (!ssi_private->has_ipg_clk_name) {
> > +		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
> > @@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
> >  	return 0;
> >  
> >  error_pcm:
> > -	clk_disable_unprepare(ssi_private->clk);
> >  
> > +	if (!ssi_private->has_ipg_clk_name)
> > +		clk_disable_unprepare(ssi_private->clk);
> >  	return ret;
> >  }
> >  
> > @@ -1246,7 +1274,8 @@ 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);
> > +	if (!ssi_private->has_ipg_clk_name)
> > +		clk_disable_unprepare(ssi_private->clk);
> >  }
> >  
> >  static int fsl_ssi_probe(struct platform_device *pdev)
> > @@ -1321,8 +1350,16 @@ static int fsl_ssi_probe(struct platform_device *pdev)
> >  		return -ENOMEM;
> >  	}
> >  
> > -	ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> > +	ret = of_property_match_string(np, "clock-names", "ipg");
> > +	if (ret < 0) {
> > +		ssi_private->has_ipg_clk_name = false;
> > +		ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> >  			&fsl_ssi_regconfig);
> 
> Sorry if I was unclear about that. My suggestion was to enable the clock
> right here:
> 		clk_prepare_enable(ssi_private->clk);
> 
> Then you can remove ssi_private->has_ipg_clk_name and all
> clk_prepare_enable() and clk_disable_unprepare() from above. Also you
> can move the devm_clk_get() into this block.
>

ipg clock not only need to be enabled when accessing register, but also 
need to be enabled when ssi is working. So I add clk_enable in startup.
 
> It seems you really want to implement this for devicetrees where the
> "ipg" clock-name is missing, but I don't understand why? I really can't
> see any benefit of adding all these clk_prepare_enable() calls for all
> cornercases that may occure. For example the clocks for AC97 are still
> missing in this version.

When ipg clock-name is missing, I just remain the code logic as before.
So AC97 case is same as before too. 
When have ipg clock-name, register it to regmap for accessing register,
and enabling ipg clock in startup also is useful for AC97.

I don't understand why you think clock for AC97 is still missing?

wang shengjiu

> 
> Best regards,
> 
> Markus
> 
> > +	} else {
> > +		ssi_private->has_ipg_clk_name = true;
> > +		ssi_private->regs = devm_regmap_init_mmio_clk(&pdev->dev,
> > +			"ipg", iomem, &fsl_ssi_regconfig);
> > +	}
> >  	if (IS_ERR(ssi_private->regs)) {
> >  		dev_err(&pdev->dev, "Failed to init register map\n");
> >  		return PTR_ERR(ssi_private->regs);
> > -- 
> > 1.7.9.5
> > 
> > 
> 
> -- 
> 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 |

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

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 6863 bytes --]

On Mon, Sep 15, 2014 at 06:22:27PM +0800, Shengjiu Wang wrote:
> On Mon, Sep 15, 2014 at 12:05:41PM +0200, Markus Pargmann wrote:
> > On Fri, Sep 12, 2014 at 06:35:15PM +0800, Shengjiu Wang wrote:
> > > Check if ipg clock is in clock-names property, then we can move the
> > > ipg clock enable and disable operation to startup and shutdown, that
> > > is only enable ipg clock when ssi is working and keep clock is disabled
> > > when ssi is in idle.
> > > But when the checking is failed, remain the clock control as before.
> > > 
> > > Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> > > ---
> > > V3 change log:
> > > update patch according Nicolin and markus's comments
> > > 
> > > 
> > >  sound/soc/fsl/fsl_ssi.c |   53 ++++++++++++++++++++++++++++++++++++++++-------
> > >  1 file changed, 45 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> > > index 2fc3e66..6d1dfd5 100644
> > > --- a/sound/soc/fsl/fsl_ssi.c
> > > +++ b/sound/soc/fsl/fsl_ssi.c
> > > @@ -169,6 +169,7 @@ struct fsl_ssi_private {
> > >  	u8 i2s_mode;
> > >  	bool use_dma;
> > >  	bool use_dual_fifo;
> > > +	bool has_ipg_clk_name;
> > >  	unsigned int fifo_depth;
> > >  	struct fsl_ssi_rxtx_reg_val rxtx_reg_val;
> > >  
> > > @@ -530,6 +531,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 +550,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
> > > @@ -1043,6 +1064,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,
> > > @@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
> > >  	u32 dmas[4];
> > >  	int ret;
> > >  
> > > -	ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> > > +	if (ssi_private->has_ipg_clk_name)
> > > +		ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
> > > +	else
> > > +		ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> > >  	if (IS_ERR(ssi_private->clk)) {
> > >  		ret = PTR_ERR(ssi_private->clk);
> > >  		dev_err(&pdev->dev, "could not get clock: %d\n", ret);
> > >  		return ret;
> > >  	}
> > >  
> > > -	ret = clk_prepare_enable(ssi_private->clk);
> > > -	if (ret) {
> > > -		dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret);
> > > -		return ret;
> > > +	if (!ssi_private->has_ipg_clk_name) {
> > > +		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
> > > @@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
> > >  	return 0;
> > >  
> > >  error_pcm:
> > > -	clk_disable_unprepare(ssi_private->clk);
> > >  
> > > +	if (!ssi_private->has_ipg_clk_name)
> > > +		clk_disable_unprepare(ssi_private->clk);
> > >  	return ret;
> > >  }
> > >  
> > > @@ -1246,7 +1274,8 @@ 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);
> > > +	if (!ssi_private->has_ipg_clk_name)
> > > +		clk_disable_unprepare(ssi_private->clk);
> > >  }
> > >  
> > >  static int fsl_ssi_probe(struct platform_device *pdev)
> > > @@ -1321,8 +1350,16 @@ static int fsl_ssi_probe(struct platform_device *pdev)
> > >  		return -ENOMEM;
> > >  	}
> > >  
> > > -	ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> > > +	ret = of_property_match_string(np, "clock-names", "ipg");
> > > +	if (ret < 0) {
> > > +		ssi_private->has_ipg_clk_name = false;
> > > +		ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> > >  			&fsl_ssi_regconfig);
> > 
> > Sorry if I was unclear about that. My suggestion was to enable the clock
> > right here:
> > 		clk_prepare_enable(ssi_private->clk);
> > 
> > Then you can remove ssi_private->has_ipg_clk_name and all
> > clk_prepare_enable() and clk_disable_unprepare() from above. Also you
> > can move the devm_clk_get() into this block.
> >
> 
> ipg clock not only need to be enabled when accessing register, but also 
> need to be enabled when ssi is working. So I add clk_enable in startup.
>  
> > It seems you really want to implement this for devicetrees where the
> > "ipg" clock-name is missing, but I don't understand why? I really can't
> > see any benefit of adding all these clk_prepare_enable() calls for all
> > cornercases that may occure. For example the clocks for AC97 are still
> > missing in this version.
> 
> When ipg clock-name is missing, I just remain the code logic as before.
> So AC97 case is same as before too. 
> When have ipg clock-name, register it to regmap for accessing register,
> and enabling ipg clock in startup also is useful for AC97.
> 
> I don't understand why you think clock for AC97 is still missing?

Sorry got it all wrong, monday morning tiredness perhaps ;-).

The patch looks good for me at the second look. I will test it later
today and give you feedback.

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 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 6863 bytes --]

On Mon, Sep 15, 2014 at 06:22:27PM +0800, Shengjiu Wang wrote:
> On Mon, Sep 15, 2014 at 12:05:41PM +0200, Markus Pargmann wrote:
> > On Fri, Sep 12, 2014 at 06:35:15PM +0800, Shengjiu Wang wrote:
> > > Check if ipg clock is in clock-names property, then we can move the
> > > ipg clock enable and disable operation to startup and shutdown, that
> > > is only enable ipg clock when ssi is working and keep clock is disabled
> > > when ssi is in idle.
> > > But when the checking is failed, remain the clock control as before.
> > > 
> > > Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> > > ---
> > > V3 change log:
> > > update patch according Nicolin and markus's comments
> > > 
> > > 
> > >  sound/soc/fsl/fsl_ssi.c |   53 ++++++++++++++++++++++++++++++++++++++++-------
> > >  1 file changed, 45 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> > > index 2fc3e66..6d1dfd5 100644
> > > --- a/sound/soc/fsl/fsl_ssi.c
> > > +++ b/sound/soc/fsl/fsl_ssi.c
> > > @@ -169,6 +169,7 @@ struct fsl_ssi_private {
> > >  	u8 i2s_mode;
> > >  	bool use_dma;
> > >  	bool use_dual_fifo;
> > > +	bool has_ipg_clk_name;
> > >  	unsigned int fifo_depth;
> > >  	struct fsl_ssi_rxtx_reg_val rxtx_reg_val;
> > >  
> > > @@ -530,6 +531,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 +550,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
> > > @@ -1043,6 +1064,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,
> > > @@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
> > >  	u32 dmas[4];
> > >  	int ret;
> > >  
> > > -	ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> > > +	if (ssi_private->has_ipg_clk_name)
> > > +		ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
> > > +	else
> > > +		ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> > >  	if (IS_ERR(ssi_private->clk)) {
> > >  		ret = PTR_ERR(ssi_private->clk);
> > >  		dev_err(&pdev->dev, "could not get clock: %d\n", ret);
> > >  		return ret;
> > >  	}
> > >  
> > > -	ret = clk_prepare_enable(ssi_private->clk);
> > > -	if (ret) {
> > > -		dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret);
> > > -		return ret;
> > > +	if (!ssi_private->has_ipg_clk_name) {
> > > +		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
> > > @@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
> > >  	return 0;
> > >  
> > >  error_pcm:
> > > -	clk_disable_unprepare(ssi_private->clk);
> > >  
> > > +	if (!ssi_private->has_ipg_clk_name)
> > > +		clk_disable_unprepare(ssi_private->clk);
> > >  	return ret;
> > >  }
> > >  
> > > @@ -1246,7 +1274,8 @@ 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);
> > > +	if (!ssi_private->has_ipg_clk_name)
> > > +		clk_disable_unprepare(ssi_private->clk);
> > >  }
> > >  
> > >  static int fsl_ssi_probe(struct platform_device *pdev)
> > > @@ -1321,8 +1350,16 @@ static int fsl_ssi_probe(struct platform_device *pdev)
> > >  		return -ENOMEM;
> > >  	}
> > >  
> > > -	ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> > > +	ret = of_property_match_string(np, "clock-names", "ipg");
> > > +	if (ret < 0) {
> > > +		ssi_private->has_ipg_clk_name = false;
> > > +		ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> > >  			&fsl_ssi_regconfig);
> > 
> > Sorry if I was unclear about that. My suggestion was to enable the clock
> > right here:
> > 		clk_prepare_enable(ssi_private->clk);
> > 
> > Then you can remove ssi_private->has_ipg_clk_name and all
> > clk_prepare_enable() and clk_disable_unprepare() from above. Also you
> > can move the devm_clk_get() into this block.
> >
> 
> ipg clock not only need to be enabled when accessing register, but also 
> need to be enabled when ssi is working. So I add clk_enable in startup.
>  
> > It seems you really want to implement this for devicetrees where the
> > "ipg" clock-name is missing, but I don't understand why? I really can't
> > see any benefit of adding all these clk_prepare_enable() calls for all
> > cornercases that may occure. For example the clocks for AC97 are still
> > missing in this version.
> 
> When ipg clock-name is missing, I just remain the code logic as before.
> So AC97 case is same as before too. 
> When have ipg clock-name, register it to regmap for accessing register,
> and enabling ipg clock in startup also is useful for AC97.
> 
> I don't understand why you think clock for AC97 is still missing?

Sorry got it all wrong, monday morning tiredness perhaps ;-).

The patch looks good for me at the second look. I will test it later
today and give you feedback.

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 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Shengjiu Wang]

On Mon, Sep 15, 2014 at 12:32:13PM +0200, Markus Pargmann wrote:
> On Mon, Sep 15, 2014 at 06:22:27PM +0800, Shengjiu Wang wrote:
> > On Mon, Sep 15, 2014 at 12:05:41PM +0200, Markus Pargmann wrote:
> > > On Fri, Sep 12, 2014 at 06:35:15PM +0800, Shengjiu Wang wrote:
> > > > Check if ipg clock is in clock-names property, then we can move the
> > > > ipg clock enable and disable operation to startup and shutdown, that
> > > > is only enable ipg clock when ssi is working and keep clock is disabled
> > > > when ssi is in idle.
> > > > But when the checking is failed, remain the clock control as before.
> > > > 
> > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> > > > ---
> > > > V3 change log:
> > > > update patch according Nicolin and markus's comments
> > > > 
> > > > 
> > > >  sound/soc/fsl/fsl_ssi.c |   53 ++++++++++++++++++++++++++++++++++++++++-------
> > > >  1 file changed, 45 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> > > > index 2fc3e66..6d1dfd5 100644
> > > > --- a/sound/soc/fsl/fsl_ssi.c
> > > > +++ b/sound/soc/fsl/fsl_ssi.c
> > > > @@ -169,6 +169,7 @@ struct fsl_ssi_private {
> > > >  	u8 i2s_mode;
> > > >  	bool use_dma;
> > > >  	bool use_dual_fifo;
> > > > +	bool has_ipg_clk_name;
> > > >  	unsigned int fifo_depth;
> > > >  	struct fsl_ssi_rxtx_reg_val rxtx_reg_val;
> > > >  
> > > > @@ -530,6 +531,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 +550,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
> > > > @@ -1043,6 +1064,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,
> > > > @@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
> > > >  	u32 dmas[4];
> > > >  	int ret;
> > > >  
> > > > -	ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> > > > +	if (ssi_private->has_ipg_clk_name)
> > > > +		ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
> > > > +	else
> > > > +		ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> > > >  	if (IS_ERR(ssi_private->clk)) {
> > > >  		ret = PTR_ERR(ssi_private->clk);
> > > >  		dev_err(&pdev->dev, "could not get clock: %d\n", ret);
> > > >  		return ret;
> > > >  	}
> > > >  
> > > > -	ret = clk_prepare_enable(ssi_private->clk);
> > > > -	if (ret) {
> > > > -		dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret);
> > > > -		return ret;
> > > > +	if (!ssi_private->has_ipg_clk_name) {
> > > > +		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
> > > > @@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
> > > >  	return 0;
> > > >  
> > > >  error_pcm:
> > > > -	clk_disable_unprepare(ssi_private->clk);
> > > >  
> > > > +	if (!ssi_private->has_ipg_clk_name)
> > > > +		clk_disable_unprepare(ssi_private->clk);
> > > >  	return ret;
> > > >  }
> > > >  
> > > > @@ -1246,7 +1274,8 @@ 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);
> > > > +	if (!ssi_private->has_ipg_clk_name)
> > > > +		clk_disable_unprepare(ssi_private->clk);
> > > >  }
> > > >  
> > > >  static int fsl_ssi_probe(struct platform_device *pdev)
> > > > @@ -1321,8 +1350,16 @@ static int fsl_ssi_probe(struct platform_device *pdev)
> > > >  		return -ENOMEM;
> > > >  	}
> > > >  
> > > > -	ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> > > > +	ret = of_property_match_string(np, "clock-names", "ipg");
> > > > +	if (ret < 0) {
> > > > +		ssi_private->has_ipg_clk_name = false;
> > > > +		ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> > > >  			&fsl_ssi_regconfig);
> > > 
> > > Sorry if I was unclear about that. My suggestion was to enable the clock
> > > right here:
> > > 		clk_prepare_enable(ssi_private->clk);
> > > 
> > > Then you can remove ssi_private->has_ipg_clk_name and all
> > > clk_prepare_enable() and clk_disable_unprepare() from above. Also you
> > > can move the devm_clk_get() into this block.
> > >
> > 
> > ipg clock not only need to be enabled when accessing register, but also 
> > need to be enabled when ssi is working. So I add clk_enable in startup.
> >  
> > > It seems you really want to implement this for devicetrees where the
> > > "ipg" clock-name is missing, but I don't understand why? I really can't
> > > see any benefit of adding all these clk_prepare_enable() calls for all
> > > cornercases that may occure. For example the clocks for AC97 are still
> > > missing in this version.
> > 
> > When ipg clock-name is missing, I just remain the code logic as before.
> > So AC97 case is same as before too. 
> > When have ipg clock-name, register it to regmap for accessing register,
> > and enabling ipg clock in startup also is useful for AC97.
> > 
> > I don't understand why you think clock for AC97 is still missing?
> 
> Sorry got it all wrong, monday morning tiredness perhaps ;-).
> 
> The patch looks good for me at the second look. I will test it later
> today and give you feedback.
> 
> Best regards,
> 
> Markus

Thank you very much. Your comments help me a lot. and wait your feedback.

best regards
wang shengjiu
> 
> -- 
> 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 |

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

[Shengjiu Wang]

On Mon, Sep 15, 2014 at 12:32:13PM +0200, Markus Pargmann wrote:
> On Mon, Sep 15, 2014 at 06:22:27PM +0800, Shengjiu Wang wrote:
> > On Mon, Sep 15, 2014 at 12:05:41PM +0200, Markus Pargmann wrote:
> > > On Fri, Sep 12, 2014 at 06:35:15PM +0800, Shengjiu Wang wrote:
> > > > Check if ipg clock is in clock-names property, then we can move the
> > > > ipg clock enable and disable operation to startup and shutdown, that
> > > > is only enable ipg clock when ssi is working and keep clock is disabled
> > > > when ssi is in idle.
> > > > But when the checking is failed, remain the clock control as before.
> > > > 
> > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> > > > ---
> > > > V3 change log:
> > > > update patch according Nicolin and markus's comments
> > > > 
> > > > 
> > > >  sound/soc/fsl/fsl_ssi.c |   53 ++++++++++++++++++++++++++++++++++++++++-------
> > > >  1 file changed, 45 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> > > > index 2fc3e66..6d1dfd5 100644
> > > > --- a/sound/soc/fsl/fsl_ssi.c
> > > > +++ b/sound/soc/fsl/fsl_ssi.c
> > > > @@ -169,6 +169,7 @@ struct fsl_ssi_private {
> > > >  	u8 i2s_mode;
> > > >  	bool use_dma;
> > > >  	bool use_dual_fifo;
> > > > +	bool has_ipg_clk_name;
> > > >  	unsigned int fifo_depth;
> > > >  	struct fsl_ssi_rxtx_reg_val rxtx_reg_val;
> > > >  
> > > > @@ -530,6 +531,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 +550,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
> > > > @@ -1043,6 +1064,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,
> > > > @@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
> > > >  	u32 dmas[4];
> > > >  	int ret;
> > > >  
> > > > -	ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> > > > +	if (ssi_private->has_ipg_clk_name)
> > > > +		ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
> > > > +	else
> > > > +		ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> > > >  	if (IS_ERR(ssi_private->clk)) {
> > > >  		ret = PTR_ERR(ssi_private->clk);
> > > >  		dev_err(&pdev->dev, "could not get clock: %d\n", ret);
> > > >  		return ret;
> > > >  	}
> > > >  
> > > > -	ret = clk_prepare_enable(ssi_private->clk);
> > > > -	if (ret) {
> > > > -		dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret);
> > > > -		return ret;
> > > > +	if (!ssi_private->has_ipg_clk_name) {
> > > > +		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
> > > > @@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
> > > >  	return 0;
> > > >  
> > > >  error_pcm:
> > > > -	clk_disable_unprepare(ssi_private->clk);
> > > >  
> > > > +	if (!ssi_private->has_ipg_clk_name)
> > > > +		clk_disable_unprepare(ssi_private->clk);
> > > >  	return ret;
> > > >  }
> > > >  
> > > > @@ -1246,7 +1274,8 @@ 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);
> > > > +	if (!ssi_private->has_ipg_clk_name)
> > > > +		clk_disable_unprepare(ssi_private->clk);
> > > >  }
> > > >  
> > > >  static int fsl_ssi_probe(struct platform_device *pdev)
> > > > @@ -1321,8 +1350,16 @@ static int fsl_ssi_probe(struct platform_device *pdev)
> > > >  		return -ENOMEM;
> > > >  	}
> > > >  
> > > > -	ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> > > > +	ret = of_property_match_string(np, "clock-names", "ipg");
> > > > +	if (ret < 0) {
> > > > +		ssi_private->has_ipg_clk_name = false;
> > > > +		ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> > > >  			&fsl_ssi_regconfig);
> > > 
> > > Sorry if I was unclear about that. My suggestion was to enable the clock
> > > right here:
> > > 		clk_prepare_enable(ssi_private->clk);
> > > 
> > > Then you can remove ssi_private->has_ipg_clk_name and all
> > > clk_prepare_enable() and clk_disable_unprepare() from above. Also you
> > > can move the devm_clk_get() into this block.
> > >
> > 
> > ipg clock not only need to be enabled when accessing register, but also 
> > need to be enabled when ssi is working. So I add clk_enable in startup.
> >  
> > > It seems you really want to implement this for devicetrees where the
> > > "ipg" clock-name is missing, but I don't understand why? I really can't
> > > see any benefit of adding all these clk_prepare_enable() calls for all
> > > cornercases that may occure. For example the clocks for AC97 are still
> > > missing in this version.
> > 
> > When ipg clock-name is missing, I just remain the code logic as before.
> > So AC97 case is same as before too. 
> > When have ipg clock-name, register it to regmap for accessing register,
> > and enabling ipg clock in startup also is useful for AC97.
> > 
> > I don't understand why you think clock for AC97 is still missing?
> 
> Sorry got it all wrong, monday morning tiredness perhaps ;-).
> 
> The patch looks good for me at the second look. I will test it later
> today and give you feedback.
> 
> Best regards,
> 
> Markus

Thank you very much. Your comments help me a lot. and wait your feedback.

best regards
wang shengjiu
> 
> -- 
> 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 |

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

[Shengjiu Wang]

On Mon, Sep 15, 2014 at 12:32:13PM +0200, Markus Pargmann wrote:
> On Mon, Sep 15, 2014 at 06:22:27PM +0800, Shengjiu Wang wrote:
> > On Mon, Sep 15, 2014 at 12:05:41PM +0200, Markus Pargmann wrote:
> > > On Fri, Sep 12, 2014 at 06:35:15PM +0800, Shengjiu Wang wrote:
> > > > Check if ipg clock is in clock-names property, then we can move the
> > > > ipg clock enable and disable operation to startup and shutdown, that
> > > > is only enable ipg clock when ssi is working and keep clock is disabled
> > > > when ssi is in idle.
> > > > But when the checking is failed, remain the clock control as before.
> > > > 
> > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> > > > ---
> > > > V3 change log:
> > > > update patch according Nicolin and markus's comments
> > > > 
> > > > 
> > > >  sound/soc/fsl/fsl_ssi.c |   53 ++++++++++++++++++++++++++++++++++++++++-------
> > > >  1 file changed, 45 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> > > > index 2fc3e66..6d1dfd5 100644
> > > > --- a/sound/soc/fsl/fsl_ssi.c
> > > > +++ b/sound/soc/fsl/fsl_ssi.c
> > > > @@ -169,6 +169,7 @@ struct fsl_ssi_private {
> > > >  	u8 i2s_mode;
> > > >  	bool use_dma;
> > > >  	bool use_dual_fifo;
> > > > +	bool has_ipg_clk_name;
> > > >  	unsigned int fifo_depth;
> > > >  	struct fsl_ssi_rxtx_reg_val rxtx_reg_val;
> > > >  
> > > > @@ -530,6 +531,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 +550,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
> > > > @@ -1043,6 +1064,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,
> > > > @@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
> > > >  	u32 dmas[4];
> > > >  	int ret;
> > > >  
> > > > -	ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> > > > +	if (ssi_private->has_ipg_clk_name)
> > > > +		ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
> > > > +	else
> > > > +		ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> > > >  	if (IS_ERR(ssi_private->clk)) {
> > > >  		ret = PTR_ERR(ssi_private->clk);
> > > >  		dev_err(&pdev->dev, "could not get clock: %d\n", ret);
> > > >  		return ret;
> > > >  	}
> > > >  
> > > > -	ret = clk_prepare_enable(ssi_private->clk);
> > > > -	if (ret) {
> > > > -		dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret);
> > > > -		return ret;
> > > > +	if (!ssi_private->has_ipg_clk_name) {
> > > > +		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
> > > > @@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
> > > >  	return 0;
> > > >  
> > > >  error_pcm:
> > > > -	clk_disable_unprepare(ssi_private->clk);
> > > >  
> > > > +	if (!ssi_private->has_ipg_clk_name)
> > > > +		clk_disable_unprepare(ssi_private->clk);
> > > >  	return ret;
> > > >  }
> > > >  
> > > > @@ -1246,7 +1274,8 @@ 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);
> > > > +	if (!ssi_private->has_ipg_clk_name)
> > > > +		clk_disable_unprepare(ssi_private->clk);
> > > >  }
> > > >  
> > > >  static int fsl_ssi_probe(struct platform_device *pdev)
> > > > @@ -1321,8 +1350,16 @@ static int fsl_ssi_probe(struct platform_device *pdev)
> > > >  		return -ENOMEM;
> > > >  	}
> > > >  
> > > > -	ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> > > > +	ret = of_property_match_string(np, "clock-names", "ipg");
> > > > +	if (ret < 0) {
> > > > +		ssi_private->has_ipg_clk_name = false;
> > > > +		ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> > > >  			&fsl_ssi_regconfig);
> > > 
> > > Sorry if I was unclear about that. My suggestion was to enable the clock
> > > right here:
> > > 		clk_prepare_enable(ssi_private->clk);
> > > 
> > > Then you can remove ssi_private->has_ipg_clk_name and all
> > > clk_prepare_enable() and clk_disable_unprepare() from above. Also you
> > > can move the devm_clk_get() into this block.
> > >
> > 
> > ipg clock not only need to be enabled when accessing register, but also 
> > need to be enabled when ssi is working. So I add clk_enable in startup.
> >  
> > > It seems you really want to implement this for devicetrees where the
> > > "ipg" clock-name is missing, but I don't understand why? I really can't
> > > see any benefit of adding all these clk_prepare_enable() calls for all
> > > cornercases that may occure. For example the clocks for AC97 are still
> > > missing in this version.
> > 
> > When ipg clock-name is missing, I just remain the code logic as before.
> > So AC97 case is same as before too. 
> > When have ipg clock-name, register it to regmap for accessing register,
> > and enabling ipg clock in startup also is useful for AC97.
> > 
> > I don't understand why you think clock for AC97 is still missing?
> 
> Sorry got it all wrong, monday morning tiredness perhaps ;-).
> 
> The patch looks good for me at the second look. I will test it later
> today and give you feedback.
> 
> Best regards,
> 
> Markus

Thank you very much. Your comments help me a lot. and wait your feedback.

best regards
wang shengjiu
> 
> -- 
> 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 |

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

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 8013 bytes --]

On Mon, Sep 15, 2014 at 06:37:15PM +0800, Shengjiu Wang wrote:
> On Mon, Sep 15, 2014 at 12:32:13PM +0200, Markus Pargmann wrote:
> > On Mon, Sep 15, 2014 at 06:22:27PM +0800, Shengjiu Wang wrote:
> > > On Mon, Sep 15, 2014 at 12:05:41PM +0200, Markus Pargmann wrote:
> > > > On Fri, Sep 12, 2014 at 06:35:15PM +0800, Shengjiu Wang wrote:
> > > > > Check if ipg clock is in clock-names property, then we can move the
> > > > > ipg clock enable and disable operation to startup and shutdown, that
> > > > > is only enable ipg clock when ssi is working and keep clock is disabled
> > > > > when ssi is in idle.
> > > > > But when the checking is failed, remain the clock control as before.
> > > > > 
> > > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> > > > > ---
> > > > > V3 change log:
> > > > > update patch according Nicolin and markus's comments
> > > > > 
> > > > > 
> > > > >  sound/soc/fsl/fsl_ssi.c |   53 ++++++++++++++++++++++++++++++++++++++++-------
> > > > >  1 file changed, 45 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> > > > > index 2fc3e66..6d1dfd5 100644
> > > > > --- a/sound/soc/fsl/fsl_ssi.c
> > > > > +++ b/sound/soc/fsl/fsl_ssi.c
> > > > > @@ -169,6 +169,7 @@ struct fsl_ssi_private {
> > > > >  	u8 i2s_mode;
> > > > >  	bool use_dma;
> > > > >  	bool use_dual_fifo;
> > > > > +	bool has_ipg_clk_name;
> > > > >  	unsigned int fifo_depth;
> > > > >  	struct fsl_ssi_rxtx_reg_val rxtx_reg_val;
> > > > >  
> > > > > @@ -530,6 +531,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 +550,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
> > > > > @@ -1043,6 +1064,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,
> > > > > @@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
> > > > >  	u32 dmas[4];
> > > > >  	int ret;
> > > > >  
> > > > > -	ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> > > > > +	if (ssi_private->has_ipg_clk_name)
> > > > > +		ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
> > > > > +	else
> > > > > +		ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> > > > >  	if (IS_ERR(ssi_private->clk)) {
> > > > >  		ret = PTR_ERR(ssi_private->clk);
> > > > >  		dev_err(&pdev->dev, "could not get clock: %d\n", ret);
> > > > >  		return ret;
> > > > >  	}
> > > > >  
> > > > > -	ret = clk_prepare_enable(ssi_private->clk);
> > > > > -	if (ret) {
> > > > > -		dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret);
> > > > > -		return ret;
> > > > > +	if (!ssi_private->has_ipg_clk_name) {
> > > > > +		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
> > > > > @@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
> > > > >  	return 0;
> > > > >  
> > > > >  error_pcm:
> > > > > -	clk_disable_unprepare(ssi_private->clk);
> > > > >  
> > > > > +	if (!ssi_private->has_ipg_clk_name)
> > > > > +		clk_disable_unprepare(ssi_private->clk);
> > > > >  	return ret;
> > > > >  }
> > > > >  
> > > > > @@ -1246,7 +1274,8 @@ 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);
> > > > > +	if (!ssi_private->has_ipg_clk_name)
> > > > > +		clk_disable_unprepare(ssi_private->clk);
> > > > >  }
> > > > >  
> > > > >  static int fsl_ssi_probe(struct platform_device *pdev)
> > > > > @@ -1321,8 +1350,16 @@ static int fsl_ssi_probe(struct platform_device *pdev)
> > > > >  		return -ENOMEM;
> > > > >  	}
> > > > >  
> > > > > -	ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> > > > > +	ret = of_property_match_string(np, "clock-names", "ipg");
> > > > > +	if (ret < 0) {
> > > > > +		ssi_private->has_ipg_clk_name = false;
> > > > > +		ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> > > > >  			&fsl_ssi_regconfig);
> > > > 
> > > > Sorry if I was unclear about that. My suggestion was to enable the clock
> > > > right here:
> > > > 		clk_prepare_enable(ssi_private->clk);
> > > > 
> > > > Then you can remove ssi_private->has_ipg_clk_name and all
> > > > clk_prepare_enable() and clk_disable_unprepare() from above. Also you
> > > > can move the devm_clk_get() into this block.
> > > >
> > > 
> > > ipg clock not only need to be enabled when accessing register, but also 
> > > need to be enabled when ssi is working. So I add clk_enable in startup.
> > >  
> > > > It seems you really want to implement this for devicetrees where the
> > > > "ipg" clock-name is missing, but I don't understand why? I really can't
> > > > see any benefit of adding all these clk_prepare_enable() calls for all
> > > > cornercases that may occure. For example the clocks for AC97 are still
> > > > missing in this version.
> > > 
> > > When ipg clock-name is missing, I just remain the code logic as before.
> > > So AC97 case is same as before too. 
> > > When have ipg clock-name, register it to regmap for accessing register,
> > > and enabling ipg clock in startup also is useful for AC97.
> > > 
> > > I don't understand why you think clock for AC97 is still missing?
> > 
> > Sorry got it all wrong, monday morning tiredness perhaps ;-).
> > 
> > The patch looks good for me at the second look. I will test it later
> > today and give you feedback.
> > 
> > Best regards,
> > 
> > Markus
> 
> Thank you very much. Your comments help me a lot. and wait your feedback.

Okay, I tested this patch on imx53-qsb with and without the "ipg"
clock-name. Both worked, so:

Tested-by: Markus Pargmann <mpa@pengutronix.de>

I noticed that checkpatch shows two minor style issues. It would be good
if you could fix them.

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 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Markus Pargmann]

[-- Attachment #1.1: Type: text/plain, Size: 8013 bytes --]

On Mon, Sep 15, 2014 at 06:37:15PM +0800, Shengjiu Wang wrote:
> On Mon, Sep 15, 2014 at 12:32:13PM +0200, Markus Pargmann wrote:
> > On Mon, Sep 15, 2014 at 06:22:27PM +0800, Shengjiu Wang wrote:
> > > On Mon, Sep 15, 2014 at 12:05:41PM +0200, Markus Pargmann wrote:
> > > > On Fri, Sep 12, 2014 at 06:35:15PM +0800, Shengjiu Wang wrote:
> > > > > Check if ipg clock is in clock-names property, then we can move the
> > > > > ipg clock enable and disable operation to startup and shutdown, that
> > > > > is only enable ipg clock when ssi is working and keep clock is disabled
> > > > > when ssi is in idle.
> > > > > But when the checking is failed, remain the clock control as before.
> > > > > 
> > > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> > > > > ---
> > > > > V3 change log:
> > > > > update patch according Nicolin and markus's comments
> > > > > 
> > > > > 
> > > > >  sound/soc/fsl/fsl_ssi.c |   53 ++++++++++++++++++++++++++++++++++++++++-------
> > > > >  1 file changed, 45 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> > > > > index 2fc3e66..6d1dfd5 100644
> > > > > --- a/sound/soc/fsl/fsl_ssi.c
> > > > > +++ b/sound/soc/fsl/fsl_ssi.c
> > > > > @@ -169,6 +169,7 @@ struct fsl_ssi_private {
> > > > >  	u8 i2s_mode;
> > > > >  	bool use_dma;
> > > > >  	bool use_dual_fifo;
> > > > > +	bool has_ipg_clk_name;
> > > > >  	unsigned int fifo_depth;
> > > > >  	struct fsl_ssi_rxtx_reg_val rxtx_reg_val;
> > > > >  
> > > > > @@ -530,6 +531,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 +550,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
> > > > > @@ -1043,6 +1064,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,
> > > > > @@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
> > > > >  	u32 dmas[4];
> > > > >  	int ret;
> > > > >  
> > > > > -	ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> > > > > +	if (ssi_private->has_ipg_clk_name)
> > > > > +		ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
> > > > > +	else
> > > > > +		ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> > > > >  	if (IS_ERR(ssi_private->clk)) {
> > > > >  		ret = PTR_ERR(ssi_private->clk);
> > > > >  		dev_err(&pdev->dev, "could not get clock: %d\n", ret);
> > > > >  		return ret;
> > > > >  	}
> > > > >  
> > > > > -	ret = clk_prepare_enable(ssi_private->clk);
> > > > > -	if (ret) {
> > > > > -		dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret);
> > > > > -		return ret;
> > > > > +	if (!ssi_private->has_ipg_clk_name) {
> > > > > +		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
> > > > > @@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
> > > > >  	return 0;
> > > > >  
> > > > >  error_pcm:
> > > > > -	clk_disable_unprepare(ssi_private->clk);
> > > > >  
> > > > > +	if (!ssi_private->has_ipg_clk_name)
> > > > > +		clk_disable_unprepare(ssi_private->clk);
> > > > >  	return ret;
> > > > >  }
> > > > >  
> > > > > @@ -1246,7 +1274,8 @@ 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);
> > > > > +	if (!ssi_private->has_ipg_clk_name)
> > > > > +		clk_disable_unprepare(ssi_private->clk);
> > > > >  }
> > > > >  
> > > > >  static int fsl_ssi_probe(struct platform_device *pdev)
> > > > > @@ -1321,8 +1350,16 @@ static int fsl_ssi_probe(struct platform_device *pdev)
> > > > >  		return -ENOMEM;
> > > > >  	}
> > > > >  
> > > > > -	ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> > > > > +	ret = of_property_match_string(np, "clock-names", "ipg");
> > > > > +	if (ret < 0) {
> > > > > +		ssi_private->has_ipg_clk_name = false;
> > > > > +		ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> > > > >  			&fsl_ssi_regconfig);
> > > > 
> > > > Sorry if I was unclear about that. My suggestion was to enable the clock
> > > > right here:
> > > > 		clk_prepare_enable(ssi_private->clk);
> > > > 
> > > > Then you can remove ssi_private->has_ipg_clk_name and all
> > > > clk_prepare_enable() and clk_disable_unprepare() from above. Also you
> > > > can move the devm_clk_get() into this block.
> > > >
> > > 
> > > ipg clock not only need to be enabled when accessing register, but also 
> > > need to be enabled when ssi is working. So I add clk_enable in startup.
> > >  
> > > > It seems you really want to implement this for devicetrees where the
> > > > "ipg" clock-name is missing, but I don't understand why? I really can't
> > > > see any benefit of adding all these clk_prepare_enable() calls for all
> > > > cornercases that may occure. For example the clocks for AC97 are still
> > > > missing in this version.
> > > 
> > > When ipg clock-name is missing, I just remain the code logic as before.
> > > So AC97 case is same as before too. 
> > > When have ipg clock-name, register it to regmap for accessing register,
> > > and enabling ipg clock in startup also is useful for AC97.
> > > 
> > > I don't understand why you think clock for AC97 is still missing?
> > 
> > Sorry got it all wrong, monday morning tiredness perhaps ;-).
> > 
> > The patch looks good for me at the second look. I will test it later
> > today and give you feedback.
> > 
> > Best regards,
> > 
> > Markus
> 
> Thank you very much. Your comments help me a lot. and wait your feedback.

Okay, I tested this patch on imx53-qsb with and without the "ipg"
clock-name. Both worked, so:

Tested-by: Markus Pargmann <mpa@pengutronix.de>

I noticed that checkpatch shows two minor style issues. It would be good
if you could fix them.

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 |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 8013 bytes --]

On Mon, Sep 15, 2014 at 06:37:15PM +0800, Shengjiu Wang wrote:
> On Mon, Sep 15, 2014 at 12:32:13PM +0200, Markus Pargmann wrote:
> > On Mon, Sep 15, 2014 at 06:22:27PM +0800, Shengjiu Wang wrote:
> > > On Mon, Sep 15, 2014 at 12:05:41PM +0200, Markus Pargmann wrote:
> > > > On Fri, Sep 12, 2014 at 06:35:15PM +0800, Shengjiu Wang wrote:
> > > > > Check if ipg clock is in clock-names property, then we can move the
> > > > > ipg clock enable and disable operation to startup and shutdown, that
> > > > > is only enable ipg clock when ssi is working and keep clock is disabled
> > > > > when ssi is in idle.
> > > > > But when the checking is failed, remain the clock control as before.
> > > > > 
> > > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> > > > > ---
> > > > > V3 change log:
> > > > > update patch according Nicolin and markus's comments
> > > > > 
> > > > > 
> > > > >  sound/soc/fsl/fsl_ssi.c |   53 ++++++++++++++++++++++++++++++++++++++++-------
> > > > >  1 file changed, 45 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> > > > > index 2fc3e66..6d1dfd5 100644
> > > > > --- a/sound/soc/fsl/fsl_ssi.c
> > > > > +++ b/sound/soc/fsl/fsl_ssi.c
> > > > > @@ -169,6 +169,7 @@ struct fsl_ssi_private {
> > > > >  	u8 i2s_mode;
> > > > >  	bool use_dma;
> > > > >  	bool use_dual_fifo;
> > > > > +	bool has_ipg_clk_name;
> > > > >  	unsigned int fifo_depth;
> > > > >  	struct fsl_ssi_rxtx_reg_val rxtx_reg_val;
> > > > >  
> > > > > @@ -530,6 +531,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 +550,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
> > > > > @@ -1043,6 +1064,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,
> > > > > @@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
> > > > >  	u32 dmas[4];
> > > > >  	int ret;
> > > > >  
> > > > > -	ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> > > > > +	if (ssi_private->has_ipg_clk_name)
> > > > > +		ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
> > > > > +	else
> > > > > +		ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> > > > >  	if (IS_ERR(ssi_private->clk)) {
> > > > >  		ret = PTR_ERR(ssi_private->clk);
> > > > >  		dev_err(&pdev->dev, "could not get clock: %d\n", ret);
> > > > >  		return ret;
> > > > >  	}
> > > > >  
> > > > > -	ret = clk_prepare_enable(ssi_private->clk);
> > > > > -	if (ret) {
> > > > > -		dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret);
> > > > > -		return ret;
> > > > > +	if (!ssi_private->has_ipg_clk_name) {
> > > > > +		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
> > > > > @@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
> > > > >  	return 0;
> > > > >  
> > > > >  error_pcm:
> > > > > -	clk_disable_unprepare(ssi_private->clk);
> > > > >  
> > > > > +	if (!ssi_private->has_ipg_clk_name)
> > > > > +		clk_disable_unprepare(ssi_private->clk);
> > > > >  	return ret;
> > > > >  }
> > > > >  
> > > > > @@ -1246,7 +1274,8 @@ 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);
> > > > > +	if (!ssi_private->has_ipg_clk_name)
> > > > > +		clk_disable_unprepare(ssi_private->clk);
> > > > >  }
> > > > >  
> > > > >  static int fsl_ssi_probe(struct platform_device *pdev)
> > > > > @@ -1321,8 +1350,16 @@ static int fsl_ssi_probe(struct platform_device *pdev)
> > > > >  		return -ENOMEM;
> > > > >  	}
> > > > >  
> > > > > -	ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> > > > > +	ret = of_property_match_string(np, "clock-names", "ipg");
> > > > > +	if (ret < 0) {
> > > > > +		ssi_private->has_ipg_clk_name = false;
> > > > > +		ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
> > > > >  			&fsl_ssi_regconfig);
> > > > 
> > > > Sorry if I was unclear about that. My suggestion was to enable the clock
> > > > right here:
> > > > 		clk_prepare_enable(ssi_private->clk);
> > > > 
> > > > Then you can remove ssi_private->has_ipg_clk_name and all
> > > > clk_prepare_enable() and clk_disable_unprepare() from above. Also you
> > > > can move the devm_clk_get() into this block.
> > > >
> > > 
> > > ipg clock not only need to be enabled when accessing register, but also 
> > > need to be enabled when ssi is working. So I add clk_enable in startup.
> > >  
> > > > It seems you really want to implement this for devicetrees where the
> > > > "ipg" clock-name is missing, but I don't understand why? I really can't
> > > > see any benefit of adding all these clk_prepare_enable() calls for all
> > > > cornercases that may occure. For example the clocks for AC97 are still
> > > > missing in this version.
> > > 
> > > When ipg clock-name is missing, I just remain the code logic as before.
> > > So AC97 case is same as before too. 
> > > When have ipg clock-name, register it to regmap for accessing register,
> > > and enabling ipg clock in startup also is useful for AC97.
> > > 
> > > I don't understand why you think clock for AC97 is still missing?
> > 
> > Sorry got it all wrong, monday morning tiredness perhaps ;-).
> > 
> > The patch looks good for me at the second look. I will test it later
> > today and give you feedback.
> > 
> > Best regards,
> > 
> > Markus
> 
> Thank you very much. Your comments help me a lot. and wait your feedback.

Okay, I tested this patch on imx53-qsb with and without the "ipg"
clock-name. Both worked, so:

Tested-by: Markus Pargmann <mpa@pengutronix.de>

I noticed that checkpatch shows two minor style issues. It would be good
if you could fix them.

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 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]