[alsa-devel] [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(-)
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);
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";
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
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
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Tue, Mar 28, 2017 at 12:21 PM, Charles Keepax ckeepax@opensource.wolfsonmicro.com wrote:
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.
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"
@@ -231,7 +231,7 @@ static int imx_wm8962_probe(struct platform_device *pdev) goto fail; }
pdev->dev from here:
codec_clk = devm_clk_get(&codec_dev->dev, NULL);
codec_clk = clk_get(&codec_dev->dev, NULL);
is different from &codec_dev->dev.
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 ckeepax@opensource.wolfsonmicro.com wrote:
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.
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"
@@ -231,7 +231,7 @@ static int imx_wm8962_probe(struct platform_device *pdev) goto fail; }
pdev->dev from here:
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:
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.
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:
On Tue, Mar 28, 2017 at 01:47:04PM +0300, Daniel Baluta wrote:
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.
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 broonie@kernel.org wrote:
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:
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.
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";
participants (4)
-
Charles Keepax -
Daniel Baluta -
Daniel Baluta -
Mark Brown