Fabio Estevam | 10 Jun 18:26 2013

[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 <at> 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
 <at>  <at>  -128,7 +128,7  <at>  <at>  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;

--

-- 
1.8.1.2

Mark Brown | 10 Jun 19:00 2013

Re: [PATCH] ASoC: imx-sgtl5000: Use devm_clk_get()

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.

Applied, thanks.
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.

Applied, thanks.
Shawn Guo | 11 Jun 02:44 2013

Re: [PATCH] ASoC: imx-sgtl5000: Use devm_clk_get()

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 <at> 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
>  <at>  <at>  -128,7 +128,7  <at>  <at>  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
(Continue reading)

Fabio Estevam | 11 Jun 04:04 2013
Picon

Re: [PATCH] ASoC: imx-sgtl5000: Use devm_clk_get()

On Mon, Jun 10, 2013 at 9:44 PM, Shawn Guo <shawn.guo <at> 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.
Shawn Guo | 11 Jun 04:50 2013

Re: [PATCH] ASoC: imx-sgtl5000: Use devm_clk_get()

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 <at> 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
Mark Brown | 11 Jun 10:41 2013

Re: [PATCH] ASoC: imx-sgtl5000: Use devm_clk_get()

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.
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.
(Continue reading)


Gmane