[alsa-devel] [PATCH] ASoC: imx-sgtl5000: Use devm_clk_get()
Commit 9e13f345 (ASoC: sgtl5000: Let the codec acquire its clock) removed the clk_put calls.
Let's use devm_clk_get() instead, so that we do not need to call them anymore.
Signed-off-by: Fabio Estevam fabio.estevam@freescale.com --- sound/soc/fsl/imx-sgtl5000.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/fsl/imx-sgtl5000.c b/sound/soc/fsl/imx-sgtl5000.c index 823151b..7a8bc12 100644 --- a/sound/soc/fsl/imx-sgtl5000.c +++ b/sound/soc/fsl/imx-sgtl5000.c @@ -128,7 +128,7 @@ static int imx_sgtl5000_probe(struct platform_device *pdev) goto fail; }
- data->codec_clk = clk_get(&codec_dev->dev, NULL); + data->codec_clk = devm_clk_get(&codec_dev->dev, NULL); if (IS_ERR(data->codec_clk)) goto fail;
On Mon, Jun 10, 2013 at 01:26:05PM -0300, Fabio Estevam wrote:
Commit 9e13f345 (ASoC: sgtl5000: Let the codec acquire its clock) removed the clk_put calls.
Let's use devm_clk_get() instead, so that we do not need to call them anymore.
Signed-off-by: Fabio Estevam fabio.estevam@freescale.com
sound/soc/fsl/imx-sgtl5000.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/fsl/imx-sgtl5000.c b/sound/soc/fsl/imx-sgtl5000.c index 823151b..7a8bc12 100644 --- a/sound/soc/fsl/imx-sgtl5000.c +++ b/sound/soc/fsl/imx-sgtl5000.c @@ -128,7 +128,7 @@ static int imx_sgtl5000_probe(struct platform_device *pdev) goto fail; }
- data->codec_clk = clk_get(&codec_dev->dev, NULL);
- data->codec_clk = devm_clk_get(&codec_dev->dev, NULL);
I do not think the change is correct. It only works if the struct *dev is what imx_sgtl5000 platform_device points to, that is pdev->dev here. However what you pass here is codec's. So when imx_sgtl5000_probe() fails or imx_sgtl5000 module is removed, clk_put() will not be called. And if I remember correctly, that's the reason why devm_clk_get() wasn't used here in the first place.
Shawn
if (IS_ERR(data->codec_clk)) goto fail;
-- 1.8.1.2
On Mon, Jun 10, 2013 at 9:44 PM, Shawn Guo shawn.guo@linaro.org wrote:
I do not think the change is correct. It only works if the struct *dev is what imx_sgtl5000 platform_device points to, that is pdev->dev here. However what you pass here is codec's. So when imx_sgtl5000_probe() fails or imx_sgtl5000 module is removed, clk_put() will not be called. And if I remember correctly, that's the reason why devm_clk_get() wasn't used here in the first place.
Ok, as you suggested I think we should handle the clocks at the codec driver only and pass the codec frequency to the machine driver.
Will work on it tomorrow.
On Mon, Jun 10, 2013 at 11:04:29PM -0300, Fabio Estevam wrote:
On Mon, Jun 10, 2013 at 9:44 PM, Shawn Guo shawn.guo@linaro.org wrote:
I do not think the change is correct. It only works if the struct *dev is what imx_sgtl5000 platform_device points to, that is pdev->dev here. However what you pass here is codec's. So when imx_sgtl5000_probe() fails or imx_sgtl5000 module is removed, clk_put() will not be called. And if I remember correctly, that's the reason why devm_clk_get() wasn't used here in the first place.
Ok, as you suggested I think we should handle the clocks at the codec driver only and pass the codec frequency to the machine driver.
Why do you need to pass it at all? It seems to me that the machine driver only needs the frequency to set up sgtl5000 sysclk. So it ends up with something below.
- sgtl5000 driver calls clk_get_rate() to get the frequency - sgtl5000 driver passes the frequency to imx-sgtl500 driver - imx-sgtl500 driver sets the frequency back to sgtl5000 via .set_sysclk callback
I need some help to understand why sgtl5000 driver can not just set up its sysclk but having to do the above.
Shawn
On Tue, Jun 11, 2013 at 10:50:40AM +0800, Shawn Guo wrote:
Why do you need to pass it at all? It seems to me that the machine driver only needs the frequency to set up sgtl5000 sysclk. So it ends up with something below.
- sgtl5000 driver calls clk_get_rate() to get the frequency
- sgtl5000 driver passes the frequency to imx-sgtl500 driver
- imx-sgtl500 driver sets the frequency back to sgtl5000 via .set_sysclk callback
I need some help to understand why sgtl5000 driver can not just set up its sysclk but having to do the above.
If it's got the clock itself it can do. The main reason we have a custom clock API in ASoC is that there's no generic clock API we can rely on being available but since the driver has been modified to rely on having a clock directly we may as well use it.
We do still want the ability to set the rate in order to allow machine drivers to reprogram if they want to (so they can switch between 8kHz and 44.1kHz based rates for example) but it's OK for the driver to figure this out automatically if it has the clock provided.
participants (4)
-
Fabio Estevam -
Fabio Estevam -
Mark Brown -
Shawn Guo