[PATCH 1/2] drivers/w1/masters/mxc_w1.c: use devm_ functions

[PATCH 1/2] drivers/w1/masters/mxc_w1.c: use devm_ functions

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

The various devm_ functions allocate memory that is released when a driver
detaches.  This patch uses these functions for data that is allocated in
the probe function of a platform device and is only freed in the remove
function.

At the same time, this fixes two faults.  First, mdev, the result of
kzalloc, was never freed.  Second, on failure of ioremap, 0 was returned.
This has been replaced by -EBUSY, which was the failure value for the call
to request_mem_region, with which the call to ioremap has been combined.

The warning message on failure of ioremap is dropped, because
devm_request_and_ioremap already gives such messages on failure.

Finally, the initial call to platform_get_resource is moved closer to the
call to devm_request_and_ioremap, which takes care of checking whether its
result is NULL, implying that a test on the result of this call to
platform_get_resource is not needed.

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/w1/masters/mxc_w1.c |   49 ++++++++------------------------------------
 1 file changed, 10 insertions(+), 39 deletions(-)

diff --git a/drivers/w1/masters/mxc_w1.c b/drivers/w1/masters/mxc_w1.c
index d338b56..999a30c 100644
--- a/drivers/w1/masters/mxc_w1.c
+++ b/drivers/w1/masters/mxc_w1.c
@@ -109,34 +109,21 @@ static int mxc_w1_probe(struct platform_device *pdev)
 	struct resource *res;
 	int err = 0;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res)
-		return -ENODEV;
-
-	mdev = kzalloc(sizeof(struct mxc_w1_device), GFP_KERNEL);
+	mdev = devm_kzalloc(&pdev->dev, sizeof(struct mxc_w1_device),
+			    GFP_KERNEL);
 	if (!mdev)
 		return -ENOMEM;
 
-	mdev->clk = clk_get(&pdev->dev, NULL);
-	if (IS_ERR(mdev->clk)) {
-		err = PTR_ERR(mdev->clk);
-		goto failed_clk;
-	}
+	mdev->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(mdev->clk))
+		return PTR_ERR(mdev->clk);
 
 	mdev->clkdiv = (clk_get_rate(mdev->clk) / 1000000) - 1;
 
-	res = request_mem_region(res->start, resource_size(res),
-				"mxc_w1");
-	if (!res) {
-		err = -EBUSY;
-		goto failed_req;
-	}
-
-	mdev->regs = ioremap(res->start, resource_size(res));
-	if (!mdev->regs) {
-		dev_err(&pdev->dev, "Cannot map mxc_w1 registers\n");
-		goto failed_ioremap;
-	}
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mdev->regs = devm_request_and_ioremap(&pdev->dev, res);
+	if (!mdev->regs)
+		return -EBUSY;
 
 	clk_prepare_enable(mdev->clk);
 	__raw_writeb(mdev->clkdiv, mdev->regs + MXC_W1_TIME_DIVIDER);
@@ -148,20 +135,10 @@ static int mxc_w1_probe(struct platform_device *pdev)
 	err = w1_add_master_device(&mdev->bus_master);
 
 	if (err)
-		goto failed_add;
+		return err;
 
 	platform_set_drvdata(pdev, mdev);
 	return 0;
-
-failed_add:
-	iounmap(mdev->regs);
-failed_ioremap:
-	release_mem_region(res->start, resource_size(res));
-failed_req:
-	clk_put(mdev->clk);
-failed_clk:
-	kfree(mdev);
-	return err;
 }
 
 /*
@@ -170,16 +147,10 @@ failed_clk:
 static int mxc_w1_remove(struct platform_device *pdev)
 {
 	struct mxc_w1_device *mdev = platform_get_drvdata(pdev);
-	struct resource *res;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 
 	w1_remove_master_device(&mdev->bus_master);
 
-	iounmap(mdev->regs);
-	release_mem_region(res->start, resource_size(res));
 	clk_disable_unprepare(mdev->clk);
-	clk_put(mdev->clk);
 
 	platform_set_drvdata(pdev, NULL);
 

Re: [PATCH 1/2] drivers/w1/masters/mxc_w1.c: use devm_ functions

[Evgeniy Polyakov]

Hi

On Fri, Dec 07, 2012 at 12:15:24AM +0100, Julia Lawall (Julia.Lawall@lip6.fr) wrote:
> +	mdev = devm_kzalloc(&pdev->dev, sizeof(struct mxc_w1_device),
> +			    GFP_KERNEL);
>  	if (!mdev)
>  		return -ENOMEM;
>  
> -	mdev->clk = clk_get(&pdev->dev, NULL);
> -	if (IS_ERR(mdev->clk)) {
> -		err = PTR_ERR(mdev->clk);
> -		goto failed_clk;
> -	}
> +	mdev->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(mdev->clk))
> +		return PTR_ERR(mdev->clk);
>  
>  	mdev->clkdiv = (clk_get_rate(mdev->clk) / 1000000) - 1;
>  
> -	res = request_mem_region(res->start, resource_size(res),
> -				"mxc_w1");
> -	if (!res) {
> -		err = -EBUSY;
> -		goto failed_req;
> -	}
> -
> -	mdev->regs = ioremap(res->start, resource_size(res));
> -	if (!mdev->regs) {
> -		dev_err(&pdev->dev, "Cannot map mxc_w1 registers\n");
> -		goto failed_ioremap;
> -	}
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	mdev->regs = devm_request_and_ioremap(&pdev->dev, res);
> +	if (!mdev->regs)
> +		return -EBUSY;

I suppose mdev will be automatically freed, but who will release
mdev->clk and other private members of mdev structure?

-- 
	Evgeniy Polyakov

Re: [PATCH 1/2] drivers/w1/masters/mxc_w1.c: use devm_ functions

[Dan Carpenter]

On Wed, Dec 12, 2012 at 01:25:56AM +0400, Evgeniy Polyakov wrote:
> I suppose mdev will be automatically freed, but who will release
> mdev->clk and other private members of mdev structure?

+   mdev->clk = devm_clk_get(&pdev->dev, NULL);

->clk is now a devm pointer as well.

regards,
dan carpenter

Re: [PATCH 1/2] drivers/w1/masters/mxc_w1.c: use devm_ functions

[Julia Lawall]

On Thu, 13 Dec 2012, Dan Carpenter wrote:

> On Wed, Dec 12, 2012 at 01:25:56AM +0400, Evgeniy Polyakov wrote:
> > I suppose mdev will be automatically freed, but who will release
> > mdev->clk and other private members of mdev structure?
>
> +   mdev->clk = devm_clk_get(&pdev->dev, NULL);
>
> ->clk is now a devm pointer as well.

Thanks for the suggestion.  I will submit a patch shortly.

julia

Re: [PATCH 1/2] drivers/w1/masters/mxc_w1.c: use devm_ functions

[Dan Carpenter]

On Thu, Dec 13, 2012 at 11:18:53AM +0100, Julia Lawall wrote:
> On Thu, 13 Dec 2012, Dan Carpenter wrote:
> 
> > On Wed, Dec 12, 2012 at 01:25:56AM +0400, Evgeniy Polyakov wrote:
> > > I suppose mdev will be automatically freed, but who will release
> > > mdev->clk and other private members of mdev structure?
> >
> > +   mdev->clk = devm_clk_get(&pdev->dev, NULL);
> >
> > ->clk is now a devm pointer as well.
> 
> Thanks for the suggestion.  I will submit a patch shortly.
> 

Huh?  I think there is a miscommunication here.  I was trying to
explain that your original patch handles releasing ->clk.

I've reviewed the patch again and I still don't see a problem with
it.  What will you change when you resubmit?

regards,
dan carpenter

Re: [PATCH 1/2] drivers/w1/masters/mxc_w1.c: use devm_ functions

[Julia Lawall]

On Thu, 13 Dec 2012, Dan Carpenter wrote:

> On Thu, Dec 13, 2012 at 11:18:53AM +0100, Julia Lawall wrote:
> > On Thu, 13 Dec 2012, Dan Carpenter wrote:
> >
> > > On Wed, Dec 12, 2012 at 01:25:56AM +0400, Evgeniy Polyakov wrote:
> > > > I suppose mdev will be automatically freed, but who will release
> > > > mdev->clk and other private members of mdev structure?
> > >
> > > +   mdev->clk = devm_clk_get(&pdev->dev, NULL);
> > >
> > > ->clk is now a devm pointer as well.
> >
> > Thanks for the suggestion.  I will submit a patch shortly.
> >
>
> Huh?  I think there is a miscommunication here.  I was trying to
> explain that your original patch handles releasing ->clk.
>
> I've reviewed the patch again and I still don't see a problem with
> it.  What will you change when you resubmit?

Oops, sorry.  I thought your +   mdev->clk = devm_clk_get(&pdev->dev,
NULL); was a suggestion of something I had overlooked.

Thanks,
julia

Re: [PATCH 1/2] drivers/w1/masters/mxc_w1.c: use devm_ functions

[Evgeniy Polyakov]

On Thu, Dec 13, 2012 at 12:39:38PM +0300, Dan Carpenter (dan.carpenter@oracle.com) wrote:
> On Wed, Dec 12, 2012 at 01:25:56AM +0400, Evgeniy Polyakov wrote:
> > I suppose mdev will be automatically freed, but who will release
> > mdev->clk and other private members of mdev structure?
> 
> +   mdev->clk = devm_clk_get(&pdev->dev, NULL);
> 
> ->clk is now a devm pointer as well.

Argh, I see it. Thanks for clarification.

-- 
	Evgeniy Polyakov

Re: [PATCH 1/2] drivers/w1/masters/mxc_w1.c: use devm_ functions

[Evgeniy Polyakov]

On Fri, Dec 07, 2012 at 12:15:24AM +0100, Julia Lawall (Julia.Lawall@lip6.fr) wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> The various devm_ functions allocate memory that is released when a driver
> detaches.  This patch uses these functions for data that is allocated in
> the probe function of a platform device and is only freed in the remove
> function.
> 
> At the same time, this fixes two faults.  First, mdev, the result of
> kzalloc, was never freed.  Second, on failure of ioremap, 0 was returned.
> This has been replaced by -EBUSY, which was the failure value for the call
> to request_mem_region, with which the call to ioremap has been combined.
> 
> The warning message on failure of ioremap is dropped, because
> devm_request_and_ioremap already gives such messages on failure.
> 
> Finally, the initial call to platform_get_resource is moved closer to the
> call to devm_request_and_ioremap, which takes care of checking whether its
> result is NULL, implying that a test on the result of this call to
> platform_get_resource is not needed.
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

Looks good. Greg, please pull both patches into your tree.
Thank you.

Acked-by: Evgeniy Polyakov <zbr@ioremap.net>

-- 
	Evgeniy Polyakov