Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1610468 > unrolled thread

Re: [alsa-devel] [PATCH v3 2/2] ASoC: imx-wm8962: Fix codec_clk cleanup

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [alsa-devel] [PATCH v3 2/2] ASoC: imx-wm8962: Fix codec_clk  cleanup Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-03-28 11:30 +0200
    Re: [alsa-devel] [PATCH v3 2/2] ASoC: imx-wm8962: Fix codec_clk cleanup Daniel Baluta <daniel.baluta@gmail.com> - 2017-03-28 12:50 +0200
      Re: [alsa-devel] [PATCH v3 2/2] ASoC: imx-wm8962: Fix codec_clk  cleanup Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-03-28 14:00 +0200
        Re: [alsa-devel] [PATCH v3 2/2] ASoC: imx-wm8962: Fix codec_clk  cleanup Mark Brown <broonie@kernel.org> - 2017-03-28 17:30 +0200
          Re: [alsa-devel] [PATCH v3 2/2] ASoC: imx-wm8962: Fix codec_clk  cleanup Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-03-28 17:50 +0200
          Re: [alsa-devel] [PATCH v3 2/2] ASoC: imx-wm8962: Fix codec_clk cleanup Daniel Baluta <daniel.baluta@gmail.com> - 2017-03-29 13:40 +0200
            Re: [alsa-devel] [PATCH v3 2/2] ASoC: imx-wm8962: Fix codec_clk  cleanup Mark Brown <broonie@kernel.org> - 2017-03-29 14:00 +0200

#1610468 — Re: [alsa-devel] [PATCH v3 2/2] ASoC: imx-wm8962: Fix codec_clk cleanup

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

[toc] | [next] | [standalone]

#1610554 — Re: [alsa-devel] [PATCH v3 2/2] ASoC: imx-wm8962: Fix codec_clk cleanup

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.

[toc] | [prev] | [next] | [standalone]

#1610623

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

[toc] | [prev] | [next] | [standalone]

#1611103

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1611115

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

[toc] | [prev] | [next] | [standalone]

#1611857 — Re: [alsa-devel] [PATCH v3 2/2] ASoC: imx-wm8962: Fix codec_clk cleanup

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.

[toc] | [prev] | [next] | [standalone]

#1611863

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web