[PATCH v3 0/2] imx-wm8962: Let codec driver enable/disable its MCLK

This is a small cleanup patch series which removes MLCK enable/disable
from imx-wm8962 machine driver and lets the codec handle that.

While reviewing this, Nicolin noticed that codec_clk cleanup is not correct
so the second patch fixes this.

Changes since v2:
	* added 2/2 patch to fix codec_clk cleanup
	* fixed prefix subject

Daniel Baluta (2):
  ASoC: imx-wm8962: Let codec driver enable/disable its MCLK
  ASoC: imx-wm8962: Fix codec_clk cleanup

 sound/soc/fsl/imx-wm8962.c | 41 +++++++++--------------------------------
 1 file changed, 9 insertions(+), 32 deletions(-)

-- 
2.7.4

WM8962 needs its MCLK when powerup in wm8962_resume(). Thus it's better
to control the MCLK in codec driver. Thus remove the clock enable in
machine driver accordingly.

While at it, get rid of imx_wm8962_remove function since it is now
empty.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 sound/soc/fsl/imx-wm8962.c | 40 ++++++++--------------------------------
 1 file changed, 8 insertions(+), 32 deletions(-)

diff --git a/sound/soc/fsl/imx-wm8962.c b/sound/soc/fsl/imx-wm8962.c
index 1b60958..3d894d9 100644
--- a/sound/soc/fsl/imx-wm8962.c
+++ b/sound/soc/fsl/imx-wm8962.c

@@ -33,7 +33,6 @@ struct imx_wm8962_data {
 	struct snd_soc_card card;
 	char codec_dai_name[DAI_NAME_SIZE];
 	char platform_name[DAI_NAME_SIZE];
-	struct clk *codec_clk;
 	unsigned int clk_frequency;
 };
 

@@ -163,6 +162,7 @@ static int imx_wm8962_probe(struct platform_device *pdev)
 	struct imx_priv *priv = &card_priv;
 	struct i2c_client *codec_dev;
 	struct imx_wm8962_data *data;
+	struct clk *codec_clk;
 	int int_port, ext_port;
 	int ret;
 

@@ -231,19 +231,14 @@ static int imx_wm8962_probe(struct platform_device *pdev)
 		goto fail;
 	}
 
-	data->codec_clk = devm_clk_get(&codec_dev->dev, NULL);
-	if (IS_ERR(data->codec_clk)) {
-		ret = PTR_ERR(data->codec_clk);
+	codec_clk = devm_clk_get(&codec_dev->dev, NULL);
+	if (IS_ERR(codec_clk)) {
+		ret = PTR_ERR(codec_clk);
 		dev_err(&codec_dev->dev, "failed to get codec clk: %d\n", ret);
 		goto fail;
 	}
 
-	data->clk_frequency = clk_get_rate(data->codec_clk);
-	ret = clk_prepare_enable(data->codec_clk);
-	if (ret) {
-		dev_err(&codec_dev->dev, "failed to enable codec clk: %d\n", ret);
-		goto fail;
-	}
+	data->clk_frequency = clk_get_rate(codec_clk);
 
 	data->dai.name = "HiFi";
 	data->dai.stream_name = "HiFi";

@@ -258,10 +253,10 @@ static int imx_wm8962_probe(struct platform_device *pdev)
 	data->card.dev = &pdev->dev;
 	ret = snd_soc_of_parse_card_name(&data->card, "model");
 	if (ret)
-		goto clk_fail;
+		goto fail;
 	ret = snd_soc_of_parse_audio_routing(&data->card, "audio-routing");
 	if (ret)
-		goto clk_fail;
+		goto fail;
 	data->card.num_links = 1;
 	data->card.owner = THIS_MODULE;
 	data->card.dai_link = &data->dai;

@@ -277,16 +272,9 @@ static int imx_wm8962_probe(struct platform_device *pdev)
 	ret = devm_snd_soc_register_card(&pdev->dev, &data->card);
 	if (ret) {
 		dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n", ret);
-		goto clk_fail;
+		goto fail;
 	}
 
-	of_node_put(ssi_np);
-	of_node_put(codec_np);
-
-	return 0;
-
-clk_fail:
-	clk_disable_unprepare(data->codec_clk);
 fail:
 	of_node_put(ssi_np);
 	of_node_put(codec_np);

@@ -294,17 +282,6 @@ static int imx_wm8962_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int imx_wm8962_remove(struct platform_device *pdev)
-{
-	struct snd_soc_card *card = platform_get_drvdata(pdev);
-	struct imx_wm8962_data *data = snd_soc_card_get_drvdata(card);
-
-	if (!IS_ERR(data->codec_clk))
-		clk_disable_unprepare(data->codec_clk);
-
-	return 0;
-}
-
 static const struct of_device_id imx_wm8962_dt_ids[] = {
 	{ .compatible = "fsl,imx-audio-wm8962", },
 	{ /* sentinel */ }

@@ -318,7 +295,6 @@ static struct platform_driver imx_wm8962_driver = {
 		.of_match_table = imx_wm8962_dt_ids,
 	},
 	.probe = imx_wm8962_probe,
-	.remove = imx_wm8962_remove,
 };
 module_platform_driver(imx_wm8962_driver);
 

-- 
2.7.4

Resource managed devm_clk_get only works with platform's device dev.

Reported-by: Nicolin Chen <nicoleotsuka@gmail.com>
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 sound/soc/fsl/imx-wm8962.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/fsl/imx-wm8962.c b/sound/soc/fsl/imx-wm8962.c
index 3d894d9..52659fa 100644
--- a/sound/soc/fsl/imx-wm8962.c
+++ b/sound/soc/fsl/imx-wm8962.c

@@ -231,7 +231,7 @@ static int imx_wm8962_probe(struct platform_device *pdev)
 		goto fail;
 	}
 
-	codec_clk = devm_clk_get(&codec_dev->dev, NULL);
+	codec_clk = clk_get(&codec_dev->dev, NULL);
 	if (IS_ERR(codec_clk)) {
 		ret = PTR_ERR(codec_clk);
 		dev_err(&codec_dev->dev, "failed to get codec clk: %d\n", ret);

@@ -239,6 +239,7 @@ static int imx_wm8962_probe(struct platform_device *pdev)
 	}
 
 	data->clk_frequency = clk_get_rate(codec_clk);
+	clk_put(codec_clk);
 
 	data->dai.name = "HiFi";
 	data->dai.stream_name = "HiFi";

-- 
2.7.4

On Tue, Mar 28, 2017 at 11:58:52AM +0300, Daniel Baluta wrote:

Resource managed devm_clk_get only works with platform's device dev.

I feel like this could use an explaination of why not using devm
is the correct fix, rather than just using the platform device
for the call. Its not obvious to me, that using the platform
device would be an issue.

Thanks,
Charles

On Tue, Mar 28, 2017 at 12:21 PM, Charles Keepax
[off-list ref] wrote:

On Tue, Mar 28, 2017 at 11:58:52AM +0300, Daniel Baluta wrote:

quoted

Resource managed devm_clk_get only works with platform's device dev.

I feel like this could use an explaination of why not using devm
is the correct fix, rather than just using the platform device
for the call. Its not obvious to me, that using the platform
device would be an issue.

Hi Charles,

I see where the confusion comes from :) and I thought the explanation
is obvious from the code, see inline comments.

Would an explanation like the one below, work better?

" We cannot use devm_clk_get with &codec_dev->dev device because
the kernel uses pdev->dev to free the managed resources, so we will end
up with a leaking codec_clk reference"

quoted

@@ -231,7 +231,7 @@ static int imx_wm8962_probe(struct platform_device *pdev)
              goto fail;
      }

pdev->dev from here:

quoted

-     codec_clk = devm_clk_get(&codec_dev->dev, NULL);
+     codec_clk = clk_get(&codec_dev->dev, NULL);

is different from &codec_dev->dev.

quoted

      if (IS_ERR(codec_clk)) {
              ret = PTR_ERR(codec_clk);
              dev_err(&codec_dev->dev, "failed to get codec clk: %d\n", ret);

@@ -239,6 +239,7 @@ static int imx_wm8962_probe(struct platform_device *pdev)
      }

      data->clk_frequency = clk_get_rate(codec_clk);
+     clk_put(codec_clk);

      data->dai.name = "HiFi";
      data->dai.stream_name = "HiFi";

thanks,
Daniel.

On Tue, Mar 28, 2017 at 01:47:04PM +0300, Daniel Baluta wrote:

On Tue, Mar 28, 2017 at 12:21 PM, Charles Keepax
[off-list ref] wrote:

quoted

On Tue, Mar 28, 2017 at 11:58:52AM +0300, Daniel Baluta wrote:

quoted

Resource managed devm_clk_get only works with platform's device dev.

I feel like this could use an explaination of why not using devm
is the correct fix, rather than just using the platform device
for the call. Its not obvious to me, that using the platform
device would be an issue.

Hi Charles,

I see where the confusion comes from :) and I thought the explanation
is obvious from the code, see inline comments.

Would an explanation like the one below, work better?

" We cannot use devm_clk_get with &codec_dev->dev device because
the kernel uses pdev->dev to free the managed resources, so we will end
up with a leaking codec_clk reference"

quoted

quoted

@@ -231,7 +231,7 @@ static int imx_wm8962_probe(struct platform_device *pdev)
              goto fail;
      }

pdev->dev from here:

quoted

quoted

-     codec_clk = devm_clk_get(&codec_dev->dev, NULL);
+     codec_clk = clk_get(&codec_dev->dev, NULL);

is different from &codec_dev->dev.

I get that they are different, I just don't get why changing
from a devm_clk_get to a clk_get is a better fix than changing
&codec->dev to &pdev->dev.

Thanks,
Charles

On Tue, Mar 28, 2017 at 12:53:06PM +0100, Charles Keepax wrote:

On Tue, Mar 28, 2017 at 01:47:04PM +0300, Daniel Baluta wrote:

quoted

quoted

quoted

-     codec_clk = devm_clk_get(&codec_dev->dev, NULL);
+     codec_clk = clk_get(&codec_dev->dev, NULL);

quoted

is different from &codec_dev->dev.

I get that they are different, I just don't get why changing
from a devm_clk_get to a clk_get is a better fix than changing
&codec->dev to &pdev->dev.

This should be clear from the semantics of clk_get(): you're looking up
the clock in the context of the supplied device and the clock is
attached to the CODEC so you need to look up in the CODEC context.  What
would be even better would be to move the allocation of the clock into
the CODEC driver...

On Tue, Mar 28, 2017 at 04:24:57PM +0100, Mark Brown wrote:

On Tue, Mar 28, 2017 at 12:53:06PM +0100, Charles Keepax wrote:

quoted

On Tue, Mar 28, 2017 at 01:47:04PM +0300, Daniel Baluta wrote:

quoted

quoted

quoted

quoted

-     codec_clk = devm_clk_get(&codec_dev->dev, NULL);
+     codec_clk = clk_get(&codec_dev->dev, NULL);

quoted

quoted

is different from &codec_dev->dev.

quoted

I get that they are different, I just don't get why changing
from a devm_clk_get to a clk_get is a better fix than changing
&codec->dev to &pdev->dev.

This should be clear from the semantics of clk_get(): you're looking up
the clock in the context of the supplied device and the clock is
attached to the CODEC so you need to look up in the CODEC context.  What
would be even better would be to move the allocation of the clock into
the CODEC driver...

Ah yes thats what I was missing.

Thanks,
Charles

On Tue, Mar 28, 2017 at 6:24 PM, Mark Brown [off-list ref] wrote:

On Tue, Mar 28, 2017 at 12:53:06PM +0100, Charles Keepax wrote:

quoted

On Tue, Mar 28, 2017 at 01:47:04PM +0300, Daniel Baluta wrote:

quoted

quoted

quoted

quoted

-     codec_clk = devm_clk_get(&codec_dev->dev, NULL);
+     codec_clk = clk_get(&codec_dev->dev, NULL);

quoted

quoted

is different from &codec_dev->dev.

quoted

I get that they are different, I just don't get why changing
from a devm_clk_get to a clk_get is a better fix than changing
&codec->dev to &pdev->dev.

This should be clear from the semantics of clk_get(): you're looking up
the clock in the context of the supplied device and the clock is
attached to the CODEC so you need to look up in the CODEC context.  What
would be even better would be to move the allocation of the clock into
the CODEC driver...

If I read the code correctly, both machine and codec driver are doing:

* codec_clk = clk_get(...)

I guess that the codec driver is the first to be loaded and it will call:

* __clk_create_clk

which will do the allocation. Then when machine driver is loaded, it
will only take a reference to the allocated clock.

I might be very wrong on this. Can you clarify what does moving allocation
of the clock into codec driver implies? Machine driver only needs a reference
to codec_clk to just get the clock's rate.

Daniel.

On Wed, Mar 29, 2017 at 02:38:23PM +0300, Daniel Baluta wrote:

If I read the code correctly, both machine and codec driver are doing:

* codec_clk = clk_get(...)

I guess that the codec driver is the first to be loaded and it will call:

* __clk_create_clk

which will do the allocation. Then when machine driver is loaded, it
will only take a reference to the allocated clock.

I might be very wrong on this. Can you clarify what does moving allocation
of the clock into codec driver implies? Machine driver only needs a reference
to codec_clk to just get the clock's rate.

Oh, so the CODEC is already requesting the clock.  Then what I'm saying
is work out a way to use the handle to the clock that the CODEC driver
created.  In effect the machine driver is telling the CODEC driver to do
things with the input clock.

The patch

   ASoC: imx-wm8962: Fix codec_clk cleanup

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From db22d189453cee666f8da2e67419f14f4b2fd9d1 Mon Sep 17 00:00:00 2001

From: Daniel Baluta <daniel.baluta@nxp.com>
Date: Tue, 28 Mar 2017 11:58:52 +0300
Subject: [PATCH] ASoC: imx-wm8962: Fix codec_clk cleanup

Resource managed devm_clk_get only works with platform's device dev.

Reported-by: Nicolin Chen <nicoleotsuka@gmail.com>
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/fsl/imx-wm8962.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/fsl/imx-wm8962.c b/sound/soc/fsl/imx-wm8962.c
index 3d894d9123e0..52659faa2eb9 100644
--- a/sound/soc/fsl/imx-wm8962.c
+++ b/sound/soc/fsl/imx-wm8962.c

@@ -231,7 +231,7 @@ static int imx_wm8962_probe(struct platform_device *pdev)
 		goto fail;
 	}
 
-	codec_clk = devm_clk_get(&codec_dev->dev, NULL);
+	codec_clk = clk_get(&codec_dev->dev, NULL);
 	if (IS_ERR(codec_clk)) {
 		ret = PTR_ERR(codec_clk);
 		dev_err(&codec_dev->dev, "failed to get codec clk: %d\n", ret);

@@ -239,6 +239,7 @@ static int imx_wm8962_probe(struct platform_device *pdev)
 	}
 
 	data->clk_frequency = clk_get_rate(codec_clk);
+	clk_put(codec_clk);
 
 	data->dai.name = "HiFi";
 	data->dai.stream_name = "HiFi";

-- 
2.11.0