[PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[Julia Lawall]

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

Add missing iounmap in error handling code, in a case where the function
already preforms iounmap on some other execution path.

A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression e;
statement S,S1;
int ret;
@@
e = \(ioremap\|ioremap_nocache\)(...)
... when != iounmap(e)
if (<+...e...+>) S
... when any
    when != iounmap(e)
*if (...)
   { ... when != iounmap(e)
     return ...; }
... when any
iounmap(e);
// </smpl>

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

---
 sound/soc/mxs/mxs-saif.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c
index 049e543..5ee0adb 100644
--- a/sound/soc/mxs/mxs-saif.c
+++ b/sound/soc/mxs/mxs-saif.c
@@ -680,7 +680,7 @@ static int mxs_saif_probe(struct platform_device *pdev)
 		ret = -ENODEV;
 		dev_err(&pdev->dev, "failed to get dma resource: %d\n",
 			ret);
-		goto failed_ioremap;
+		goto failed_get_resource;
 	}
 	saif->dma_param.chan_num = dmares->start;
 
@@ -739,6 +739,7 @@ failed_register:
 failed_get_irq2:
 	free_irq(saif->irq, saif);
 failed_get_irq1:
+failed_get_resource:
 	iounmap(saif->base);
 failed_ioremap:
 	release_mem_region(iores->start, resource_size(iores));

Re: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[Felipe Balbi]

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

Hi,

On Thu, Jan 12, 2012 at 10:55:03AM +0100, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Add missing iounmap in error handling code, in a case where the function
> already preforms iounmap on some other execution path.
> 
> A simplified version of the semantic match that finds this problem is as
> follows: (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression e;
> statement S,S1;
> int ret;
> @@
> e = \(ioremap\|ioremap_nocache\)(...)
> ... when != iounmap(e)
> if (<+...e...+>) S
> ... when any
>     when != iounmap(e)
> *if (...)
>    { ... when != iounmap(e)
>      return ...; }
> ... when any
> iounmap(e);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

This is not related to this patch actually, but when you send a series
of patches could you make sure to send them all as a thread ? It makes
it easier (at least for me) to see that those are all related to each
other (in your case, sematich patches).

In order to do that with git you could:

$ git format-patch -o patches --cover-letter linus/master

[ edit cover letter]

$ git send-email --to lkml patches/

that will do the trick. Thanks

-- 
balbi

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

RE: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[Dong Aisheng-B29396]

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Julia Lawall
> Sent: Thursday, January 12, 2012 5:55 PM
> To: Liam Girdwood
> Cc: kernel-janitors@vger.kernel.org; Mark Brown; Jaroslav Kysela; Takashi Iwai;
> alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap
> 
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Add missing iounmap in error handling code, in a case where the function already
> preforms iounmap on some other execution path.
> 
> A simplified version of the semantic match that finds this problem is as
> follows: (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression e;
> statement S,S1;
> int ret;
> @@
> e = \(ioremap\|ioremap_nocache\)(...)
> ... when != iounmap(e)
> if (<+...e...+>) S
> ... when any
>     when != iounmap(e)
> *if (...)
>    { ... when != iounmap(e)
>      return ...; }
> ... when any
> iounmap(e);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---
>  sound/soc/mxs/mxs-saif.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c index
> 049e543..5ee0adb 100644
> --- a/sound/soc/mxs/mxs-saif.c
> +++ b/sound/soc/mxs/mxs-saif.c
> @@ -680,7 +680,7 @@ static int mxs_saif_probe(struct platform_device *pdev)
>  		ret = -ENODEV;
>  		dev_err(&pdev->dev, "failed to get dma resource: %d\n",
>  			ret);
> -		goto failed_ioremap;
> +		goto failed_get_resource;
>  	}
>  	saif->dma_param.chan_num = dmares->start;
> 
> @@ -739,6 +739,7 @@ failed_register:
>  failed_get_irq2:
>  	free_irq(saif->irq, saif);
>  failed_get_irq1:
> +failed_get_resource:
There's already a 'failed_get_resource' there, wouldn't your change break
The original code?

BTW, I guess a better way is that you can submit a patch to change the driver
To use devm_alloc_* and it's friend routines, then we do not need to fix
Such things any more.

Regards
Dong Aisheng
>  	iounmap(saif->base);
>  failed_ioremap:
>  	release_mem_region(iores->start, resource_size(iores));
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the
> body of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[Julia Lawall]

On Fri, 13 Jan 2012, Dong Aisheng-B29396 wrote:

>> -----Original Message-----
>> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
>> owner@vger.kernel.org] On Behalf Of Julia Lawall
>> Sent: Thursday, January 12, 2012 5:55 PM
>> To: Liam Girdwood
>> Cc: kernel-janitors@vger.kernel.org; Mark Brown; Jaroslav Kysela; Takashi Iwai;
>> alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org
>> Subject: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap
>>
>> From: Julia Lawall <Julia.Lawall@lip6.fr>
>>
>> Add missing iounmap in error handling code, in a case where the function already
>> preforms iounmap on some other execution path.
>>
>> A simplified version of the semantic match that finds this problem is as
>> follows: (http://coccinelle.lip6.fr/)
>>
>> // <smpl>
>> @@
>> expression e;
>> statement S,S1;
>> int ret;
>> @@
>> e = \(ioremap\|ioremap_nocache\)(...)
>> ... when != iounmap(e)
>> if (<+...e...+>) S
>> ... when any
>>     when != iounmap(e)
>> *if (...)
>>    { ... when != iounmap(e)
>>      return ...; }
>> ... when any
>> iounmap(e);
>> // </smpl>
>>
>> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>>
>> ---
>>  sound/soc/mxs/mxs-saif.c |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c index
>> 049e543..5ee0adb 100644
>> --- a/sound/soc/mxs/mxs-saif.c
>> +++ b/sound/soc/mxs/mxs-saif.c
>> @@ -680,7 +680,7 @@ static int mxs_saif_probe(struct platform_device *pdev)
>>  		ret = -ENODEV;
>>  		dev_err(&pdev->dev, "failed to get dma resource: %d\n",
>>  			ret);
>> -		goto failed_ioremap;
>> +		goto failed_get_resource;
>>  	}
>>  	saif->dma_param.chan_num = dmares->start;
>>
>> @@ -739,6 +739,7 @@ failed_register:
>>  failed_get_irq2:
>>  	free_irq(saif->irq, saif);
>>  failed_get_irq1:
>> +failed_get_resource:
> There's already a 'failed_get_resource' there, wouldn't your change break
> The original code?
>
> BTW, I guess a better way is that you can submit a patch to change the driver
> To use devm_alloc_* and it's friend routines, then we do not need to fix
> Such things any more.

Thanks for the suggestions.  I will fix it up with the devm_ solution.

julia

RE: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

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

By removing the need for iounmap, this also eliminates a missing iounmap
problem, in the the original error handling code following a successful
call to ioremap.

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

---
  sound/soc/mxs/mxs-saif.c |   44 ++++++++++++++------------------------------
  1 file changed, 14 insertions(+), 30 deletions(-)

diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c
index f204dba..6a72ad9 100644
--- a/sound/soc/mxs/mxs-saif.c
+++ b/sound/soc/mxs/mxs-saif.c
@@ -630,7 +630,7 @@ static int mxs_saif_probe(struct platform_device *pdev)
  	if (pdev->id >= ARRAY_SIZE(mxs_saif))
  		return -EINVAL;

-	saif = kzalloc(sizeof(*saif), GFP_KERNEL);
+	saif = devm_kzalloc(&pdev->dev, sizeof(*saif), GFP_KERNEL);
  	if (!saif)
  		return -ENOMEM;

@@ -652,10 +652,9 @@ static int mxs_saif_probe(struct platform_device *pdev)

  	saif->clk = clk_get(&pdev->dev, NULL);
  	if (IS_ERR(saif->clk)) {
-		ret = PTR_ERR(saif->clk);
  		dev_err(&pdev->dev, "Cannot get the clock: %d\n",
  			ret);
-		goto failed_clk;
+		return PTR_ERR(saif->clk);
  	}

  	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -666,18 +665,19 @@ static int mxs_saif_probe(struct platform_device *pdev)
  		goto failed_get_resource;
  	}

-	if (!request_mem_region(iores->start, resource_size(iores),
-				"mxs-saif")) {
+	if (!devm_request_mem_region(&pdev->dev, iores->start,
+				     resource_size(iores), "mxs-saif")) {
  		dev_err(&pdev->dev, "request_mem_region failed\n");
  		ret = -EBUSY;
  		goto failed_get_resource;
  	}

-	saif->base = ioremap(iores->start, resource_size(iores));
+	saif->base = devm_ioremap(&pdev->dev, iores->start,
+				  resource_size(iores));
  	if (!saif->base) {
  		dev_err(&pdev->dev, "ioremap failed\n");
  		ret = -ENODEV;
-		goto failed_ioremap;
+		goto failed_get_resource;
  	}

  	dmares = platform_get_resource(pdev, IORESOURCE_DMA, 0);
@@ -685,7 +685,7 @@ static int mxs_saif_probe(struct platform_device *pdev)
  		ret = -ENODEV;
  		dev_err(&pdev->dev, "failed to get dma resource: %d\n",
  			ret);
-		goto failed_ioremap;
+		goto failed_get_resource;
  	}
  	saif->dma_param.chan_num = dmares->start;

@@ -694,14 +694,15 @@ static int mxs_saif_probe(struct platform_device *pdev)
  		ret = saif->irq;
  		dev_err(&pdev->dev, "failed to get irq resource: %d\n",
  			ret);
-		goto failed_get_irq1;
+		goto failed_get_resource;
  	}

  	saif->dev = &pdev->dev;
-	ret = request_irq(saif->irq, mxs_saif_irq, 0, "mxs-saif", saif);
+	ret = devm_request_irq(&pdev->dev, saif->irq, mxs_saif_irq, 0,
+			       "mxs-saif", saif);
  	if (ret) {
  		dev_err(&pdev->dev, "failed to request irq\n");
-		goto failed_get_irq1;
+		goto failed_get_resource;
  	}

  	saif->dma_param.chan_irq = platform_get_irq(pdev, 1);
@@ -709,7 +710,7 @@ static int mxs_saif_probe(struct platform_device *pdev)
  		ret = saif->dma_param.chan_irq;
  		dev_err(&pdev->dev, "failed to get dma irq resource: %d\n",
  			ret);
-		goto failed_get_irq2;
+		goto failed_get_resource;
  	}

  	platform_set_drvdata(pdev, saif);
@@ -717,7 +718,7 @@ static int mxs_saif_probe(struct platform_device *pdev)
  	ret = snd_soc_register_dai(&pdev->dev, &mxs_saif_dai);
  	if (ret) {
  		dev_err(&pdev->dev, "register DAI failed\n");
-		goto failed_register;
+		goto failed_get_resource;
  	}

  	saif->soc_platform_pdev = platform_device_alloc(
@@ -740,36 +741,19 @@ failed_pdev_add:
  	platform_device_put(saif->soc_platform_pdev);
  failed_pdev_alloc:
  	snd_soc_unregister_dai(&pdev->dev);
-failed_register:
-failed_get_irq2:
-	free_irq(saif->irq, saif);
-failed_get_irq1:
-	iounmap(saif->base);
-failed_ioremap:
-	release_mem_region(iores->start, resource_size(iores));
  failed_get_resource:
  	clk_put(saif->clk);
-failed_clk:
-	kfree(saif);

  	return ret;
  }

  static int __devexit mxs_saif_remove(struct platform_device *pdev)
  {
-	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  	struct mxs_saif *saif = platform_get_drvdata(pdev);

  	platform_device_unregister(saif->soc_platform_pdev);
-
  	snd_soc_unregister_dai(&pdev->dev);
-
-	iounmap(saif->base);
-	release_mem_region(res->start, resource_size(res));
-	free_irq(saif->irq, saif);
-
  	clk_put(saif->clk);
-	kfree(saif);

  	return 0;
  }

Re: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[Mark Brown]

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

On Tue, Jan 24, 2012 at 06:29:13PM +0100, Julia Lawall wrote:

>  	if (IS_ERR(saif->clk)) {
> -		ret = PTR_ERR(saif->clk);
>  		dev_err(&pdev->dev, "Cannot get the clock: %d\n",
>  			ret);
> -		goto failed_clk;
> +		return PTR_ERR(saif->clk);

Looks like the transformation is overly enthusiastic here - the
assignment to ret is removed but ret is used in the log message.

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

Re: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[Julia Lawall]

On Tue, 24 Jan 2012, Mark Brown wrote:

> On Tue, Jan 24, 2012 at 06:29:13PM +0100, Julia Lawall wrote:
>
>>  	if (IS_ERR(saif->clk)) {
>> -		ret = PTR_ERR(saif->clk);
>>  		dev_err(&pdev->dev, "Cannot get the clock: %d\n",
>>  			ret);
>> -		goto failed_clk;
>> +		return PTR_ERR(saif->clk);
>
> Looks like the transformation is overly enthusiastic here - the
> assignment to ret is removed but ret is used in the log message.

Uh, no, I did that stupid thing by hand.  I'll fix it.  Thanks.

julia

Re: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[Julia Lawall]

>From nobody Tue Jan 24 18:26:10 CET 2012
From: Julia Lawall <Julia.Lawall@lip6.fr>
To: Liam Girdwood <lrg@ti.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>,Jaroslav Kysela <perex@perex.cz>,Takashi Iwai <tiwai@suse.de>,alsa-devel@alsa-project.org,linux-kernel@vger.kernel.org
Subject: [PATCH] sound/soc/mxs/mxs-saif.c: use devm_ functions

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.

By removing the need for iounmap, this also eliminates a missing iounmap
problem, in the the original error handling code following a successful
call to ioremap.

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

---
  sound/soc/mxs/mxs-saif.c |   43 ++++++++++++++-----------------------------
  1 file changed, 14 insertions(+), 29 deletions(-)

diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c
index f204dba..7d755e0 100644
--- a/sound/soc/mxs/mxs-saif.c
+++ b/sound/soc/mxs/mxs-saif.c
@@ -630,7 +630,7 @@ static int mxs_saif_probe(struct platform_device *pdev)
  	if (pdev->id >= ARRAY_SIZE(mxs_saif))
  		return -EINVAL;

-	saif = kzalloc(sizeof(*saif), GFP_KERNEL);
+	saif = devm_kzalloc(&pdev->dev, sizeof(*saif), GFP_KERNEL);
  	if (!saif)
  		return -ENOMEM;

@@ -655,7 +655,7 @@ static int mxs_saif_probe(struct platform_device *pdev)
  		ret = PTR_ERR(saif->clk);
  		dev_err(&pdev->dev, "Cannot get the clock: %d\n",
  			ret);
-		goto failed_clk;
+		return ret;
  	}

  	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -666,18 +666,19 @@ static int mxs_saif_probe(struct platform_device *pdev)
  		goto failed_get_resource;
  	}

-	if (!request_mem_region(iores->start, resource_size(iores),
-				"mxs-saif")) {
+	if (!devm_request_mem_region(&pdev->dev, iores->start,
+				     resource_size(iores), "mxs-saif")) {
  		dev_err(&pdev->dev, "request_mem_region failed\n");
  		ret = -EBUSY;
  		goto failed_get_resource;
  	}

-	saif->base = ioremap(iores->start, resource_size(iores));
+	saif->base = devm_ioremap(&pdev->dev, iores->start,
+				  resource_size(iores));
  	if (!saif->base) {
  		dev_err(&pdev->dev, "ioremap failed\n");
  		ret = -ENODEV;
-		goto failed_ioremap;
+		goto failed_get_resource;
  	}

  	dmares = platform_get_resource(pdev, IORESOURCE_DMA, 0);
@@ -685,7 +686,7 @@ static int mxs_saif_probe(struct platform_device *pdev)
  		ret = -ENODEV;
  		dev_err(&pdev->dev, "failed to get dma resource: %d\n",
  			ret);
-		goto failed_ioremap;
+		goto failed_get_resource;
  	}
  	saif->dma_param.chan_num = dmares->start;

@@ -694,14 +695,15 @@ static int mxs_saif_probe(struct platform_device *pdev)
  		ret = saif->irq;
  		dev_err(&pdev->dev, "failed to get irq resource: %d\n",
  			ret);
-		goto failed_get_irq1;
+		goto failed_get_resource;
  	}

  	saif->dev = &pdev->dev;
-	ret = request_irq(saif->irq, mxs_saif_irq, 0, "mxs-saif", saif);
+	ret = devm_request_irq(&pdev->dev, saif->irq, mxs_saif_irq, 0,
+			       "mxs-saif", saif);
  	if (ret) {
  		dev_err(&pdev->dev, "failed to request irq\n");
-		goto failed_get_irq1;
+		goto failed_get_resource;
  	}

  	saif->dma_param.chan_irq = platform_get_irq(pdev, 1);
@@ -709,7 +711,7 @@ static int mxs_saif_probe(struct platform_device *pdev)
  		ret = saif->dma_param.chan_irq;
  		dev_err(&pdev->dev, "failed to get dma irq resource: %d\n",
  			ret);
-		goto failed_get_irq2;
+		goto failed_get_resource;
  	}

  	platform_set_drvdata(pdev, saif);
@@ -717,7 +719,7 @@ static int mxs_saif_probe(struct platform_device *pdev)
  	ret = snd_soc_register_dai(&pdev->dev, &mxs_saif_dai);
  	if (ret) {
  		dev_err(&pdev->dev, "register DAI failed\n");
-		goto failed_register;
+		goto failed_get_resource;
  	}

  	saif->soc_platform_pdev = platform_device_alloc(
@@ -740,36 +742,19 @@ failed_pdev_add:
  	platform_device_put(saif->soc_platform_pdev);
  failed_pdev_alloc:
  	snd_soc_unregister_dai(&pdev->dev);
-failed_register:
-failed_get_irq2:
-	free_irq(saif->irq, saif);
-failed_get_irq1:
-	iounmap(saif->base);
-failed_ioremap:
-	release_mem_region(iores->start, resource_size(iores));
  failed_get_resource:
  	clk_put(saif->clk);
-failed_clk:
-	kfree(saif);

  	return ret;
  }

  static int __devexit mxs_saif_remove(struct platform_device *pdev)
  {
-	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  	struct mxs_saif *saif = platform_get_drvdata(pdev);

  	platform_device_unregister(saif->soc_platform_pdev);
-
  	snd_soc_unregister_dai(&pdev->dev);
-
-	iounmap(saif->base);
-	release_mem_region(res->start, resource_size(res));
-	free_irq(saif->irq, saif);
-
  	clk_put(saif->clk);
-	kfree(saif);

  	return 0;
  }

Re: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[Mark Brown]

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

On Tue, Jan 24, 2012 at 09:45:26PM +0100, Julia Lawall wrote:

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

This is a really nice cleanup - I tried to apply it but unfortunately it
needs a rebase due to the change to use clk_prepare()/clk_unprepare()
which went in recently.  Could you rebase against current -next please?

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

Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[Wolfram Sang]

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

> @@ -666,18 +666,19 @@ static int mxs_saif_probe(struct platform_device *pdev)
>   		goto failed_get_resource;
>   	}
> 
> -	if (!request_mem_region(iores->start, resource_size(iores),
> -				"mxs-saif")) {
> +	if (!devm_request_mem_region(&pdev->dev, iores->start,
> +				     resource_size(iores), "mxs-saif")) {
>   		dev_err(&pdev->dev, "request_mem_region failed\n");
>   		ret = -EBUSY;
>   		goto failed_get_resource;
>   	}
> 
> -	saif->base = ioremap(iores->start, resource_size(iores));
> +	saif->base = devm_ioremap(&pdev->dev, iores->start,
> +				  resource_size(iores));

devm_request_and_ioremap() to save even more code?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[Julia Lawall]

On Thu, 26 Jan 2012, Wolfram Sang wrote:

>> @@ -666,18 +666,19 @@ static int mxs_saif_probe(struct platform_device *pdev)
>>   		goto failed_get_resource;
>>   	}
>>
>> -	if (!request_mem_region(iores->start, resource_size(iores),
>> -				"mxs-saif")) {
>> +	if (!devm_request_mem_region(&pdev->dev, iores->start,
>> +				     resource_size(iores), "mxs-saif")) {
>>   		dev_err(&pdev->dev, "request_mem_region failed\n");
>>   		ret = -EBUSY;
>>   		goto failed_get_resource;
>>   	}
>>
>> -	saif->base = ioremap(iores->start, resource_size(iores));
>> +	saif->base = devm_ioremap(&pdev->dev, iores->start,
>> +				  resource_size(iores));
>
> devm_request_and_ioremap() to save even more code?

I didn't do that because apparently it is not yet in a release?  I have 
another for introducing those...

julia

Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[Mark Brown]

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

On Thu, Jan 26, 2012 at 12:21:35PM +0100, Julia Lawall wrote:
> On Thu, 26 Jan 2012, Wolfram Sang wrote:

> >devm_request_and_ioremap() to save even more code?

> I didn't do that because apparently it is not yet in a release?  I
> have another for introducing those...

It went in during the merge window so patches using it should be good to
go now.

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

Re: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[Julia Lawall]

On Thu, 26 Jan 2012, Mark Brown wrote:

> On Tue, Jan 24, 2012 at 09:45:26PM +0100, Julia Lawall wrote:
>
>> 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.
>
> This is a really nice cleanup - I tried to apply it but unfortunately it
> needs a rebase due to the change to use clk_prepare()/clk_unprepare()
> which went in recently.  Could you rebase against current -next please?

Could you tell me in more detail what the problem is?  I pulled linux-next 
and regenerated the patch, but got exactly the same thing as before.  I do 
see some calls to clk_prepare_enable in the code.

thanks,
julia

Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[Wolfram Sang]

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

On Thu, Jan 26, 2012 at 12:21:35PM +0100, Julia Lawall wrote:
> On Thu, 26 Jan 2012, Wolfram Sang wrote:
> 
> >>@@ -666,18 +666,19 @@ static int mxs_saif_probe(struct platform_device *pdev)
> >>  		goto failed_get_resource;
> >>  	}
> >>
> >>-	if (!request_mem_region(iores->start, resource_size(iores),
> >>-				"mxs-saif")) {
> >>+	if (!devm_request_mem_region(&pdev->dev, iores->start,
> >>+				     resource_size(iores), "mxs-saif")) {
> >>  		dev_err(&pdev->dev, "request_mem_region failed\n");
> >>  		ret = -EBUSY;
> >>  		goto failed_get_resource;
> >>  	}
> >>
> >>-	saif->base = ioremap(iores->start, resource_size(iores));
> >>+	saif->base = devm_ioremap(&pdev->dev, iores->start,
> >>+				  resource_size(iores));
> >
> >devm_request_and_ioremap() to save even more code?
> 
> I didn't do that because apparently it is not yet in a release?  I
> have another for introducing those...

It got included in the last merge window:

commit 72f8c0bfa0de64c68ee59f40eb9b2683bffffbb0
Author: Wolfram Sang <w.sang@pengutronix.de>
Date:   Tue Oct 25 15:16:47 2011 +0200

    lib: devres: add convenience function to remap a resource
...


-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[Mark Brown]

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

On Thu, Jan 26, 2012 at 12:24:21PM +0100, Julia Lawall wrote:
> On Thu, 26 Jan 2012, Mark Brown wrote:

> >This is a really nice cleanup - I tried to apply it but unfortunately it
> >needs a rebase due to the change to use clk_prepare()/clk_unprepare()
> >which went in recently.  Could you rebase against current -next please?

> Could you tell me in more detail what the problem is?  I pulled
> linux-next and regenerated the patch, but got exactly the same thing
> as before.  I do see some calls to clk_prepare_enable in the code.

I tried to apply it with git am but even with -3 it wouldn't apply (-3
said it didn't know about the original blobs which was worrying),
looking at the logs the clk_prepare stuff seemed most likely to have
overlapped.  Perhaps your regeneration will be enough.

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

Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[Julia Lawall]

On Thu, 26 Jan 2012, Wolfram Sang wrote:

> On Thu, Jan 26, 2012 at 12:21:35PM +0100, Julia Lawall wrote:
>> On Thu, 26 Jan 2012, Wolfram Sang wrote:
>>
>>>> @@ -666,18 +666,19 @@ static int mxs_saif_probe(struct platform_device *pdev)
>>>>  		goto failed_get_resource;
>>>>  	}
>>>>
>>>> -	if (!request_mem_region(iores->start, resource_size(iores),
>>>> -				"mxs-saif")) {
>>>> +	if (!devm_request_mem_region(&pdev->dev, iores->start,
>>>> +				     resource_size(iores), "mxs-saif")) {
>>>>  		dev_err(&pdev->dev, "request_mem_region failed\n");
>>>>  		ret = -EBUSY;
>>>>  		goto failed_get_resource;
>>>>  	}
>>>>
>>>> -	saif->base = ioremap(iores->start, resource_size(iores));
>>>> +	saif->base = devm_ioremap(&pdev->dev, iores->start,
>>>> +				  resource_size(iores));
>>>
>>> devm_request_and_ioremap() to save even more code?
>>
>> I didn't do that because apparently it is not yet in a release?  I
>> have another for introducing those...
>
> It got included in the last merge window:
>
> commit 72f8c0bfa0de64c68ee59f40eb9b2683bffffbb0
> Author: Wolfram Sang <w.sang@pengutronix.de>
> Date:   Tue Oct 25 15:16:47 2011 +0200
>
>    lib: devres: add convenience function to remap a resource
> ...

OK, thanks.  I'll take that into account.

julia

Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[walter harms]

Am 26.01.2012 12:05, schrieb Wolfram Sang:
>> @@ -666,18 +666,19 @@ static int mxs_saif_probe(struct platform_device *pdev)
>>   		goto failed_get_resource;
>>   	}
>>
>> -	if (!request_mem_region(iores->start, resource_size(iores),
>> -				"mxs-saif")) {
>> +	if (!devm_request_mem_region(&pdev->dev, iores->start,
>> +				     resource_size(iores), "mxs-saif")) {
>>   		dev_err(&pdev->dev, "request_mem_region failed\n");
>>   		ret = -EBUSY;
>>   		goto failed_get_resource;
>>   	}
>>
>> -	saif->base = ioremap(iores->start, resource_size(iores));
>> +	saif->base = devm_ioremap(&pdev->dev, iores->start,
>> +				  resource_size(iores));
> 
> devm_request_and_ioremap() to save even more code?
> 

hi all,
at first I am NOT the maintainer ...

personally i would stay with Julia's patch and add devm_request_and_ioremap() in a second round.
The devm stuff is brand new, and two steps here would allow to spot/locate problems more easy
since structural changes are less intrusive in each step.

re,
 wh

Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

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

By removing the need for kfree and iounmap, this also eliminates missing
resource-release problems.

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

---
This is a patch against the version in
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git

  sound/soc/mxs/mxs-saif.c |   47 ++++++++++++-----------------------------------
  1 file changed, 12 insertions(+), 35 deletions(-)

diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c
--- a/sound/soc/mxs/mxs-saif.c
+++ b/sound/soc/mxs/mxs-saif.c
@@ -632,7 +632,7 @@ static int mxs_saif_probe(struct platform_device *pdev)
  			return ret;
  	}

-	saif = kzalloc(sizeof(*saif), GFP_KERNEL);
+	saif = devm_kzalloc(&pdev->dev, sizeof(*saif), GFP_KERNEL);
  	if (!saif)
  		return -ENOMEM;

@@ -652,7 +652,7 @@ static int mxs_saif_probe(struct platform_device *pdev)
  		ret = PTR_ERR(saif->clk);
  		dev_err(&pdev->dev, "Cannot get the clock: %d\n",
  			ret);
-		goto failed_clk;
+		return ret;
  	}

  	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -663,18 +663,11 @@ static int mxs_saif_probe(struct platform_device *pdev)
  		goto failed_get_resource;
  	}

-	if (!request_mem_region(iores->start, resource_size(iores),
-				"mxs-saif")) {
-		dev_err(&pdev->dev, "request_mem_region failed\n");
-		ret = -EBUSY;
-		goto failed_get_resource;
-	}
-
-	saif->base = ioremap(iores->start, resource_size(iores));
+	saif->base = devm_request_and_ioremap(&pdev->dev, iores);
  	if (!saif->base) {
-		dev_err(&pdev->dev, "ioremap failed\n");
+		dev_err(&pdev->dev, "devm_request_and_ioremap failed\n");
  		ret = -ENODEV;
-		goto failed_ioremap;
+		goto failed_get_resource;
  	}

  	dmares = platform_get_resource(pdev, IORESOURCE_DMA, 0);
@@ -682,7 +675,7 @@ static int mxs_saif_probe(struct platform_device *pdev)
  		ret = -ENODEV;
  		dev_err(&pdev->dev, "failed to get dma resource: %d\n",
  			ret);
-		goto failed_ioremap;
+		goto failed_get_resource;
  	}
  	saif->dma_param.chan_num = dmares->start;

@@ -691,14 +684,15 @@ static int mxs_saif_probe(struct platform_device *pdev)
  		ret = saif->irq;
  		dev_err(&pdev->dev, "failed to get irq resource: %d\n",
  			ret);
-		goto failed_get_irq1;
+		goto failed_get_resource;
  	}

  	saif->dev = &pdev->dev;
-	ret = request_irq(saif->irq, mxs_saif_irq, 0, "mxs-saif", saif);
+	ret = devm_request_irq(&pdev->dev, saif->irq, mxs_saif_irq, 0,
+			       "mxs-saif", saif);
  	if (ret) {
  		dev_err(&pdev->dev, "failed to request irq\n");
-		goto failed_get_irq1;
+		goto failed_get_resource;
  	}

  	saif->dma_param.chan_irq = platform_get_irq(pdev, 1);
@@ -706,7 +700,7 @@ static int mxs_saif_probe(struct platform_device *pdev)
  		ret = saif->dma_param.chan_irq;
  		dev_err(&pdev->dev, "failed to get dma irq resource: %d\n",
  			ret);
-		goto failed_get_irq2;
+		goto failed_get_resource;
  	}

  	platform_set_drvdata(pdev, saif);
@@ -714,7 +708,7 @@ static int mxs_saif_probe(struct platform_device *pdev)
  	ret = snd_soc_register_dai(&pdev->dev, &mxs_saif_dai);
  	if (ret) {
  		dev_err(&pdev->dev, "register DAI failed\n");
-		goto failed_register;
+		goto failed_get_resource;
  	}

  	saif->soc_platform_pdev = platform_device_alloc(
@@ -737,36 +731,19 @@ failed_pdev_add:
  	platform_device_put(saif->soc_platform_pdev);
  failed_pdev_alloc:
  	snd_soc_unregister_dai(&pdev->dev);
-failed_register:
-failed_get_irq2:
-	free_irq(saif->irq, saif);
-failed_get_irq1:
-	iounmap(saif->base);
-failed_ioremap:
-	release_mem_region(iores->start, resource_size(iores));
  failed_get_resource:
  	clk_put(saif->clk);
-failed_clk:
-	kfree(saif);

  	return ret;
  }

  static int __devexit mxs_saif_remove(struct platform_device *pdev)
  {
-	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  	struct mxs_saif *saif = platform_get_drvdata(pdev);

  	platform_device_unregister(saif->soc_platform_pdev);
-
  	snd_soc_unregister_dai(&pdev->dev);
-
-	iounmap(saif->base);
-	release_mem_region(res->start, resource_size(res));
-	free_irq(saif->irq, saif);
-
  	clk_put(saif->clk);
-	kfree(saif);

  	return 0;
  }

Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[Wolfram Sang]

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

Julia,

>  	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -663,18 +663,11 @@ static int mxs_saif_probe(struct platform_device *pdev)
>  		goto failed_get_resource;
>  	}
> 
> -	if (!request_mem_region(iores->start, resource_size(iores),
> -				"mxs-saif")) {
> -		dev_err(&pdev->dev, "request_mem_region failed\n");
> -		ret = -EBUSY;
> -		goto failed_get_resource;
> -	}
> -
> -	saif->base = ioremap(iores->start, resource_size(iores));
> +	saif->base = devm_request_and_ioremap(&pdev->dev, iores);

You can skip checking 'iores', too. I also did that in the example, but
a lot of people seem to miss it. Where did you get the information how
to use devm_request_and_ioremap? I probably need to spread the word even
more...

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[Julia Lawall]

On Thu, 26 Jan 2012, Wolfram Sang wrote:

> Julia,
>
>>  	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> @@ -663,18 +663,11 @@ static int mxs_saif_probe(struct platform_device *pdev)
>>  		goto failed_get_resource;
>>  	}
>>
>> -	if (!request_mem_region(iores->start, resource_size(iores),
>> -				"mxs-saif")) {
>> -		dev_err(&pdev->dev, "request_mem_region failed\n");
>> -		ret = -EBUSY;
>> -		goto failed_get_resource;
>> -	}
>> -
>> -	saif->base = ioremap(iores->start, resource_size(iores));
>> +	saif->base = devm_request_and_ioremap(&pdev->dev, iores);
>
> You can skip checking 'iores', too. I also did that in the example, but
> a lot of people seem to miss it.

I can try to do that, but it seems a little bit unintuitive.  Perhaps it 
would be easier for people to remember to put in error handling code when 
they need it if they always have to do it?  If I remove it, there will be 
one call that has no test and then another call a few lines later that 
does.

> Where did you get the information how
> to use devm_request_and_ioremap? I probably need to spread the word even
> more...

I saw it in Documentation/driver-model/devres.txt and then looked around 
for some examples.

julia

Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

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

By removing the need for kfree and iounmap, this also eliminates missing
resource-release problems.

This also removes a now unneeded test on iores.

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

---

patch against 
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git

  sound/soc/mxs/mxs-saif.c |   53 ++++++++++-------------------------------------
  1 file changed, 12 insertions(+), 41 deletions(-)

diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c
--- a/sound/soc/mxs/mxs-saif.c
+++ b/sound/soc/mxs/mxs-saif.c
@@ -632,7 +632,7 @@ static int mxs_saif_probe(struct platform_device *pdev)
  			return ret;
  	}

-	saif = kzalloc(sizeof(*saif), GFP_KERNEL);
+	saif = devm_kzalloc(&pdev->dev, sizeof(*saif), GFP_KERNEL);
  	if (!saif)
  		return -ENOMEM;

@@ -652,29 +652,16 @@ static int mxs_saif_probe(struct platform_device *pdev)
  		ret = PTR_ERR(saif->clk);
  		dev_err(&pdev->dev, "Cannot get the clock: %d\n",
  			ret);
-		goto failed_clk;
+		return ret;
  	}

  	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!iores) {
-		ret = -ENODEV;
-		dev_err(&pdev->dev, "failed to get io resource: %d\n",
-			ret);
-		goto failed_get_resource;
-	}

-	if (!request_mem_region(iores->start, resource_size(iores),
-				"mxs-saif")) {
-		dev_err(&pdev->dev, "request_mem_region failed\n");
-		ret = -EBUSY;
-		goto failed_get_resource;
-	}
-
-	saif->base = ioremap(iores->start, resource_size(iores));
+	saif->base = devm_request_and_ioremap(&pdev->dev, iores);
  	if (!saif->base) {
-		dev_err(&pdev->dev, "ioremap failed\n");
+		dev_err(&pdev->dev, "devm_request_and_ioremap failed\n");
  		ret = -ENODEV;
-		goto failed_ioremap;
+		goto failed_get_resource;
  	}

  	dmares = platform_get_resource(pdev, IORESOURCE_DMA, 0);
@@ -682,7 +669,7 @@ static int mxs_saif_probe(struct platform_device *pdev)
  		ret = -ENODEV;
  		dev_err(&pdev->dev, "failed to get dma resource: %d\n",
  			ret);
-		goto failed_ioremap;
+		goto failed_get_resource;
  	}
  	saif->dma_param.chan_num = dmares->start;

@@ -691,14 +678,15 @@ static int mxs_saif_probe(struct platform_device *pdev)
  		ret = saif->irq;
  		dev_err(&pdev->dev, "failed to get irq resource: %d\n",
  			ret);
-		goto failed_get_irq1;
+		goto failed_get_resource;
  	}

  	saif->dev = &pdev->dev;
-	ret = request_irq(saif->irq, mxs_saif_irq, 0, "mxs-saif", saif);
+	ret = devm_request_irq(&pdev->dev, saif->irq, mxs_saif_irq, 0,
+			       "mxs-saif", saif);
  	if (ret) {
  		dev_err(&pdev->dev, "failed to request irq\n");
-		goto failed_get_irq1;
+		goto failed_get_resource;
  	}

  	saif->dma_param.chan_irq = platform_get_irq(pdev, 1);
@@ -706,7 +694,7 @@ static int mxs_saif_probe(struct platform_device *pdev)
  		ret = saif->dma_param.chan_irq;
  		dev_err(&pdev->dev, "failed to get dma irq resource: %d\n",
  			ret);
-		goto failed_get_irq2;
+		goto failed_get_resource;
  	}

  	platform_set_drvdata(pdev, saif);
@@ -714,7 +702,7 @@ static int mxs_saif_probe(struct platform_device *pdev)
  	ret = snd_soc_register_dai(&pdev->dev, &mxs_saif_dai);
  	if (ret) {
  		dev_err(&pdev->dev, "register DAI failed\n");
-		goto failed_register;
+		goto failed_get_resource;
  	}

  	saif->soc_platform_pdev = platform_device_alloc(
@@ -737,36 +725,19 @@ failed_pdev_add:
  	platform_device_put(saif->soc_platform_pdev);
  failed_pdev_alloc:
  	snd_soc_unregister_dai(&pdev->dev);
-failed_register:
-failed_get_irq2:
-	free_irq(saif->irq, saif);
-failed_get_irq1:
-	iounmap(saif->base);
-failed_ioremap:
-	release_mem_region(iores->start, resource_size(iores));
  failed_get_resource:
  	clk_put(saif->clk);
-failed_clk:
-	kfree(saif);

  	return ret;
  }

  static int __devexit mxs_saif_remove(struct platform_device *pdev)
  {
-	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  	struct mxs_saif *saif = platform_get_drvdata(pdev);

  	platform_device_unregister(saif->soc_platform_pdev);
-
  	snd_soc_unregister_dai(&pdev->dev);
-
-	iounmap(saif->base);
-	release_mem_region(res->start, resource_size(res));
-	free_irq(saif->irq, saif);
-
  	clk_put(saif->clk);
-	kfree(saif);

  	return 0;
  }

Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[Wolfram Sang]

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

> >You can skip checking 'iores', too. I also did that in the example, but
> >a lot of people seem to miss it.
> 
> I can try to do that, but it seems a little bit unintuitive.
> Perhaps it would be easier for people to remember to put in error
> handling code when they need it if they always have to do it?  If I
> remove it, there will be one call that has no test and then another
> call a few lines later that does.

I see your point. I would still like to get rid of the duplicated code
(then it can't be forgotten as well). Maybe I should have named the
function something alike devm_check_and_request_and_ioremap()? Then I
could have also introduced a similar function for requesting irq. Will
think about this a bit more. Thanks for updating your patch!

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[Wolfram Sang]

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

On Sat, Jan 28, 2012 at 08:03:34AM +0100, Julia Lawall 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.
> 
> By removing the need for kfree and iounmap, this also eliminates missing
> resource-release problems.
> 
> This also removes a now unneeded test on iores.
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

Looks good, will test it later today.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[Julia Lawall]

On Sat, 28 Jan 2012, Wolfram Sang wrote:

>>> You can skip checking 'iores', too. I also did that in the example, but
>>> a lot of people seem to miss it.
>>
>> I can try to do that, but it seems a little bit unintuitive.
>> Perhaps it would be easier for people to remember to put in error
>> handling code when they need it if they always have to do it?  If I
>> remove it, there will be one call that has no test and then another
>> call a few lines later that does.
>
> I see your point. I would still like to get rid of the duplicated code
> (then it can't be forgotten as well). Maybe I should have named the
> function something alike devm_check_and_request_and_ioremap()? Then I
> could have also introduced a similar function for requesting irq. Will
> think about this a bit more. Thanks for updating your patch!

Despite believing in my point, error handling code is where people make a 
lot of mistakes.  Getting rid of it seems always to be a good idea.  Maybe 
such checks could be moved into more uses of the result of 
platform_get_resource.

julia

devm_request_and_ioremap

[Julia Lawall]

How important is the name argument to request_mem_region?

devm_request_and_ioremap uses:

name = res->name ?: dev_name(dev);

In trying to introduce uses of devm_request_and_ioremap I thus look for 
res->name and dev_name(dev) as well as the name in the driver structure in
the platform_driver structure.  But sometimes the name is pdev->name, ie 
the name field of the argument to the probe function.  And sometimes it is 
a string that is slightly different than what is in the driver structure.

thanks,
julia

Re: devm_request_and_ioremap

[Mark Brown]

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

On Sat, Feb 04, 2012 at 07:25:05PM +0100, Julia Lawall wrote:
> How important is the name argument to request_mem_region?

It's just used for diagnostics.

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

Re: devm_request_and_ioremap

[Julia Lawall]

And how about a function devm_clk_get?  It looks like there would be 
around 150 opportunities for its use.

julia

Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[Mark Brown]

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

On Sat, Jan 28, 2012 at 08:03:34AM +0100, Julia Lawall wrote:

> patch against
> git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git

Hrm, I know there had been some debate about what to generate against
but this now once again fails to apply against for-3.4...  No idea
what's going on there and I didn't investigate the conflicts, sorry.

I had been waiting in the vauge hope that some of the mxs maintainers
would review.

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

Re: devm_request_and_ioremap

[Mark Brown]

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

On Sat, Feb 04, 2012 at 11:08:24PM +0100, Julia Lawall wrote:
> And how about a function devm_clk_get?  It looks like there would be
> around 150 opportunities for its use.

Note that currently there's no standard clk API so any work there is a
bit more painful than it perhaps should be.

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

Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[Dong Aisheng]

On Tue, Feb 7, 2012 at 12:20 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Sat, Jan 28, 2012 at 08:03:34AM +0100, Julia Lawall wrote:
>
>> patch against
>> git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
>
> Hrm, I know there had been some debate about what to generate against
> but this now once again fails to apply against for-3.4...  No idea
> what's going on there and I didn't investigate the conflicts, sorry.
>
> I had been waiting in the vauge hope that some of the mxs maintainers
> would review.
>
I also failed to apply it against for-3.4.
It seemed the conflicts are a lot.

Hi Julia,

Can you regenerate the patch based on Mark's for-3.4 branch?

BTW, i wonder if this patch's title should be changed a bit
since what this patch does is not adding missing iounmap anymore.
Maybe like: ASoC: mxs-saif: convert to use devm_kmalloc and its friends.

Regards
Dong Aisheng

Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1056 bytes --]

On Wed, 8 Feb 2012, Dong Aisheng wrote:

> On Tue, Feb 7, 2012 at 12:20 AM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
>> On Sat, Jan 28, 2012 at 08:03:34AM +0100, Julia Lawall wrote:
>>
>>> patch against
>>> git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
>>
>> Hrm, I know there had been some debate about what to generate against
>> but this now once again fails to apply against for-3.4...  No idea
>> what's going on there and I didn't investigate the conflicts, sorry.
>>
>> I had been waiting in the vauge hope that some of the mxs maintainers
>> would review.
>>
> I also failed to apply it against for-3.4.
> It seemed the conflicts are a lot.
>
> Hi Julia,
>
> Can you regenerate the patch based on Mark's for-3.4 branch?
>
> BTW, i wonder if this patch's title should be changed a bit
> since what this patch does is not adding missing iounmap anymore.
> Maybe like: ASoC: mxs-saif: convert to use devm_kmalloc and its friends.

Yes, thanks for the suggestions.  I will do both shortly (today or 
tomorrow).

julia