[PATCH] ASoC: sgtl5000: Use devm_ functions

[PATCH] ASoC: sgtl5000: Use devm_ functions

[Himangi Saraogi]

This patch introduces the use of managed interfaces like devm_kzalloc,
devm_kstrdup and devm_regulator_register and does avay with the calls to
the functions to free the allocated memory in ldo_regulator_register and
ldo_regulator_remove. The ldo_regulator_remove function is completely
removed as it is no longer required. ldo_regulator_register is called
from a probe function and on failure its value is returned as the
result.

Signed-off-by: Himangi Saraogi <himangi774@gmail.com>
---
To send to: Liam Girdwood <lgirdwood@gmail.com>,Mark Brown <broonie@kernel.org>,Jaroslav Kysela <perex@perex.cz>,Takashi Iwai <tiwai@suse.de>,alsa-devel@alsa-project.org,linux-kernel@vger.kernel.org
 sound/soc/codecs/sgtl5000.c | 46 +++++++--------------------------------------
 1 file changed, 7 insertions(+), 39 deletions(-)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index 249fadb..0efd6d6 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -841,14 +841,15 @@ static int ldo_regulator_register(struct snd_soc_codec *codec,
 	struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
 	struct regulator_config config = { };
 
-	ldo = kzalloc(sizeof(struct ldo_regulator), GFP_KERNEL);
+	ldo = devm_kzalloc(codec->dev, sizeof(struct ldo_regulator),
+			   GFP_KERNEL);
 
 	if (!ldo)
 		return -ENOMEM;
 
-	ldo->desc.name = kstrdup(dev_name(codec->dev), GFP_KERNEL);
+	ldo->desc.name = devm_kstrdup(codec->dev, dev_name(codec->dev),
+				      GFP_KERNEL);
 	if (!ldo->desc.name) {
-		kfree(ldo);
 		dev_err(codec->dev, "failed to allocate decs name memory\n");
 		return -ENOMEM;
 	}
@@ -865,35 +866,17 @@ static int ldo_regulator_register(struct snd_soc_codec *codec,
 	config.driver_data = ldo;
 	config.init_data = init_data;
 
-	ldo->dev = regulator_register(&ldo->desc, &config);
+	ldo->dev = devm_regulator_register(codec->dev, &ldo->desc, &config);
 	if (IS_ERR(ldo->dev)) {
 		int ret = PTR_ERR(ldo->dev);
 
 		dev_err(codec->dev, "failed to register regulator\n");
-		kfree(ldo->desc.name);
-		kfree(ldo);
-
 		return ret;
 	}
 	sgtl5000->ldo = ldo;
 
 	return 0;
 }
-
-static int ldo_regulator_remove(struct snd_soc_codec *codec)
-{
-	struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
-	struct ldo_regulator *ldo = sgtl5000->ldo;
-
-	if (!ldo)
-		return 0;
-
-	regulator_unregister(ldo->dev);
-	kfree(ldo->desc.name);
-	kfree(ldo);
-
-	return 0;
-}
 #else
 static int ldo_regulator_register(struct snd_soc_codec *codec,
 				struct regulator_init_data *init_data,
@@ -902,11 +885,6 @@ static int ldo_regulator_register(struct snd_soc_codec *codec,
 	dev_err(codec->dev, "this setup needs regulator support in the kernel\n");
 	return -EINVAL;
 }
-
-static int ldo_regulator_remove(struct snd_soc_codec *codec)
-{
-	return 0;
-}
 #endif
 
 /*
@@ -1278,23 +1256,17 @@ static int sgtl5000_enable_regulators(struct snd_soc_codec *codec)
 	ret = devm_regulator_bulk_get(codec->dev, ARRAY_SIZE(sgtl5000->supplies),
 				 sgtl5000->supplies);
 	if (ret)
-		goto err_ldo_remove;
+		return ret;
 
 	ret = regulator_bulk_enable(ARRAY_SIZE(sgtl5000->supplies),
 					sgtl5000->supplies);
 	if (ret)
-		goto err_ldo_remove;
+		return ret;
 
 	/* wait for all power rails bring up */
 	udelay(10);
 
 	return 0;
-
-err_ldo_remove:
-	if (!external_vddd)
-		ldo_regulator_remove(codec);
-	return ret;
-
 }
 
 static int sgtl5000_probe(struct snd_soc_codec *codec)
@@ -1359,8 +1331,6 @@ static int sgtl5000_probe(struct snd_soc_codec *codec)
 err:
 	regulator_bulk_disable(ARRAY_SIZE(sgtl5000->supplies),
 						sgtl5000->supplies);
-	ldo_regulator_remove(codec);
-
 	return ret;
 }
 
@@ -1372,8 +1342,6 @@ static int sgtl5000_remove(struct snd_soc_codec *codec)
 
 	regulator_bulk_disable(ARRAY_SIZE(sgtl5000->supplies),
 						sgtl5000->supplies);
-	ldo_regulator_remove(codec);
-
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH] ASoC: sgtl5000: Use devm_ functions

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 922 bytes --]

On Sun, Jul 06, 2014 at 12:38:00PM +0530, Himangi Saraogi wrote:

> diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
> index 249fadb..0efd6d6 100644
> --- a/sound/soc/codecs/sgtl5000.c
> +++ b/sound/soc/codecs/sgtl5000.c
> @@ -841,14 +841,15 @@ static int ldo_regulator_register(struct snd_soc_codec *codec,
>  	struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
>  	struct regulator_config config = { };
>  
> -	ldo = kzalloc(sizeof(struct ldo_regulator), GFP_KERNEL);
> +	ldo = devm_kzalloc(codec->dev, sizeof(struct ldo_regulator),
> +			   GFP_KERNEL);

You're using the managed functions within the ASoC level probe functions
which doesn't work - devm_ is only usable as part of the driver model
binding and unbinding.  All this resource allocation needs to be moved
into the device level probe (which is a better thing anyway) before it
is converted to devm.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] ASoC: sgtl5000: Use devm_ functions

[Julia Lawall]

On Mon, 7 Jul 2014, Mark Brown wrote:

> On Sun, Jul 06, 2014 at 12:38:00PM +0530, Himangi Saraogi wrote:
>
> > diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
> > index 249fadb..0efd6d6 100644
> > --- a/sound/soc/codecs/sgtl5000.c
> > +++ b/sound/soc/codecs/sgtl5000.c
> > @@ -841,14 +841,15 @@ static int ldo_regulator_register(struct snd_soc_codec *codec,
> >  	struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
> >  	struct regulator_config config = { };
> >
> > -	ldo = kzalloc(sizeof(struct ldo_regulator), GFP_KERNEL);
> > +	ldo = devm_kzalloc(codec->dev, sizeof(struct ldo_regulator),
> > +			   GFP_KERNEL);
>
> You're using the managed functions within the ASoC level probe functions
> which doesn't work - devm_ is only usable as part of the driver model
> binding and unbinding.  All this resource allocation needs to be moved
> into the device level probe (which is a better thing anyway) before it
> is converted to devm.

Nevertheless, there is already a call to devm_regulator_bulk_get in
sgtl5000_enable_regulators which calls sgtl5000_replace_vddd_with_ldo
which calls ldo_regulator_register.  That call was introduced by

commit 63e54cd9caa3ce03635810608519e2b37d8bc706
Author: Fabio Estevam <fabio.estevam@freescale.com>
Date:   Thu Apr 24 14:13:08 2014 -0300

It seems that that patch should be reverted?

julia

Re: [alsa-devel] [PATCH] ASoC: sgtl5000: Use devm_ functions

[Fabio Estevam]

On Mon, Jul 7, 2014 at 11:58 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:

> Nevertheless, there is already a call to devm_regulator_bulk_get in
> sgtl5000_enable_regulators which calls sgtl5000_replace_vddd_with_ldo
> which calls ldo_regulator_register.  That call was introduced by
>
> commit 63e54cd9caa3ce03635810608519e2b37d8bc706
> Author: Fabio Estevam <fabio.estevam@freescale.com>
> Date:   Thu Apr 24 14:13:08 2014 -0300
>
> It seems that that patch should be reverted?

I think so. Russell also reported a kernel oops when unbinding this
module, so I will prepare a patch reverting it.

Thanks

Re: [alsa-devel] [PATCH] ASoC: sgtl5000: Use devm_ functions

[Julia Lawall]

On Mon, 7 Jul 2014, Fabio Estevam wrote:

> On Mon, Jul 7, 2014 at 11:58 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
> > Nevertheless, there is already a call to devm_regulator_bulk_get in
> > sgtl5000_enable_regulators which calls sgtl5000_replace_vddd_with_ldo
> > which calls ldo_regulator_register.  That call was introduced by
> >
> > commit 63e54cd9caa3ce03635810608519e2b37d8bc706
> > Author: Fabio Estevam <fabio.estevam@freescale.com>
> > Date:   Thu Apr 24 14:13:08 2014 -0300
> >
> > It seems that that patch should be reverted?
>
> I think so. Russell also reported a kernel oops when unbinding this
> module, so I will prepare a patch reverting it.

There is documentation about what kinds of devm functions exist, but it is
too bad that there is no documentation about where they can be used.
Often there are several levels of function pointers involved, so it can be
hard to figure out whether they can be used just by looking at the code.
I have only taken the strategy of using them in kinds of functions where
someone else has alreadyy figured out that they can be used.

julia

Re: [alsa-devel] [PATCH] ASoC: sgtl5000: Use devm_ functions

[Lars-Peter Clausen]

On 07/07/2014 05:23 PM, Julia Lawall wrote:
>
>
> On Mon, 7 Jul 2014, Fabio Estevam wrote:
>
>> On Mon, Jul 7, 2014 at 11:58 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>>
>>> Nevertheless, there is already a call to devm_regulator_bulk_get in
>>> sgtl5000_enable_regulators which calls sgtl5000_replace_vddd_with_ldo
>>> which calls ldo_regulator_register.  That call was introduced by
>>>
>>> commit 63e54cd9caa3ce03635810608519e2b37d8bc706
>>> Author: Fabio Estevam <fabio.estevam@freescale.com>
>>> Date:   Thu Apr 24 14:13:08 2014 -0300
>>>
>>> It seems that that patch should be reverted?
>>
>> I think so. Russell also reported a kernel oops when unbinding this
>> module, so I will prepare a patch reverting it.
>
> There is documentation about what kinds of devm functions exist, but it is
> too bad that there is no documentation about where they can be used.
> Often there are several levels of function pointers involved, so it can be
> hard to figure out whether they can be used just by looking at the code.
> I have only taken the strategy of using them in kinds of functions where
> someone else has alreadyy figured out that they can be used.

Yes, it is probably a bit underdocumented. The rule of thumb is don't use it 
anywhere else except for device probe callbacks (and functions that are only 
called from a device probe function) and only for the device that is being 
probed. There are probably a couple of instances in the kernel where the 
manged resource allocators are used in places where they shouldn't be used.

- Lars

Re: [alsa-devel] [PATCH] ASoC: sgtl5000: Use devm_ functions

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 826 bytes --]

On Mon, Jul 07, 2014 at 05:23:39PM +0200, Julia Lawall wrote:

> There is documentation about what kinds of devm functions exist, but it is
> too bad that there is no documentation about where they can be used.
> Often there are several levels of function pointers involved, so it can be
> hard to figure out whether they can be used just by looking at the code.
> I have only taken the strategy of using them in kinds of functions where
> someone else has alreadyy figured out that they can be used.

It should be fairly clear given what they do I'd have thought - the
devm_ functions tie the deallocation of a resource to the unbinding of
a driver from a device so they can only be used to replace things that
get cleaned up in a device model unbind path.  There's not usually a
great deal of indirection going on in those.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: sgtl5000: Use devm_ functions

[Julia Lawall]

On Tue, 8 Jul 2014, Mark Brown wrote:

> On Mon, Jul 07, 2014 at 05:23:39PM +0200, Julia Lawall wrote:
>
> > There is documentation about what kinds of devm functions exist, but it is
> > too bad that there is no documentation about where they can be used.
> > Often there are several levels of function pointers involved, so it can be
> > hard to figure out whether they can be used just by looking at the code.
> > I have only taken the strategy of using them in kinds of functions where
> > someone else has alreadyy figured out that they can be used.
>
> It should be fairly clear given what they do I'd have thought - the
> devm_ functions tie the deallocation of a resource to the unbinding of
> a driver from a device so they can only be used to replace things that
> get cleaned up in a device model unbind path.  There's not usually a
> great deal of indirection going on in those.

It is completely clear what they do.  What is not clear is what device
libraries are set up to call the freeing functions at what point.  For
example, I know that that platform drivers are set up for this, but once I
tried to find the lines of code that would justify that, but I could not.
Perhaps I was not patient enough or missed something.

julia

Re: [alsa-devel] [PATCH] ASoC: sgtl5000: Use devm_ functions

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 931 bytes --]

On Tue, Jul 08, 2014 at 10:15:20AM +0200, Julia Lawall wrote:
> On Tue, 8 Jul 2014, Mark Brown wrote:

> > It should be fairly clear given what they do I'd have thought - the
> > devm_ functions tie the deallocation of a resource to the unbinding of
> > a driver from a device so they can only be used to replace things that
> > get cleaned up in a device model unbind path.  There's not usually a
> > great deal of indirection going on in those.

> It is completely clear what they do.  What is not clear is what device
> libraries are set up to call the freeing functions at what point.  For
> example, I know that that platform drivers are set up for this, but once I
> tried to find the lines of code that would justify that, but I could not.
> Perhaps I was not patient enough or missed something.

All devices do this - it's done as part of the driver model core code so
there is no need for individual buses to do anything.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: sgtl5000: Use devm_ functions

[Julia Lawall]

On Tue, 8 Jul 2014, Mark Brown wrote:

> On Tue, Jul 08, 2014 at 10:15:20AM +0200, Julia Lawall wrote:
> > On Tue, 8 Jul 2014, Mark Brown wrote:
> 
> > > It should be fairly clear given what they do I'd have thought - the
> > > devm_ functions tie the deallocation of a resource to the unbinding of
> > > a driver from a device so they can only be used to replace things that
> > > get cleaned up in a device model unbind path.  There's not usually a
> > > great deal of indirection going on in those.
> 
> > It is completely clear what they do.  What is not clear is what device
> > libraries are set up to call the freeing functions at what point.  For
> > example, I know that that platform drivers are set up for this, but once I
> > tried to find the lines of code that would justify that, but I could not.
> > Perhaps I was not patient enough or missed something.
> 
> All devices do this - it's done as part of the driver model core code so
> there is no need for individual buses to do anything.

How should one realize that this does not apply to the original file 
under discussion, sound/soc/codecs/sgtl5000.c?  The associated structure 
is snd_soc_codec_driver.  What code could one look for at the call sites 
of the probe and remove functions to know that managed memory can be used?

thanks,
julia

Re: [alsa-devel] [PATCH] ASoC: sgtl5000: Use devm_ functions

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

On Wed, Jul 09, 2014 at 07:30:40AM +0200, Julia Lawall wrote:
> On Tue, 8 Jul 2014, Mark Brown wrote:

> > All devices do this - it's done as part of the driver model core code so
> > there is no need for individual buses to do anything.

> How should one realize that this does not apply to the original file 
> under discussion, sound/soc/codecs/sgtl5000.c?  The associated structure 
> is snd_soc_codec_driver.  What code could one look for at the call sites 
> of the probe and remove functions to know that managed memory can be used?

That's not a device model driver; the way you should realise this is
that there's no device being registered on a bus which is matched by the
driver core.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: sgtl5000: Use devm_ functions

[Julia Lawall]

On Wed, 9 Jul 2014, Mark Brown wrote:

> On Wed, Jul 09, 2014 at 07:30:40AM +0200, Julia Lawall wrote:
> > On Tue, 8 Jul 2014, Mark Brown wrote:
>
> > > All devices do this - it's done as part of the driver model core code so
> > > there is no need for individual buses to do anything.
>
> > How should one realize that this does not apply to the original file
> > under discussion, sound/soc/codecs/sgtl5000.c?  The associated structure
> > is snd_soc_codec_driver.  What code could one look for at the call sites
> > of the probe and remove functions to know that managed memory can be used?
>
> That's not a device model driver; the way you should realise this is
> that there's no device being registered on a bus which is matched by the
> driver core.

OK, thanks for the clarification.

julia