* [PATCH 3/5] drivers/video/mbx/mbxfb.c: use devm_ functions
2012-07-31 16:39 [PATCH 0/5] use devm_ functions Damien Cassou
@ 2012-07-31 16:39 ` Damien Cassou
2012-08-23 20:37 ` Florian Tobias Schandinat
2012-07-31 16:39 ` [PATCH 2/5] drivers/video/gbefb.c: " Damien Cassou
` (3 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Damien Cassou @ 2012-07-31 16:39 UTC (permalink / raw)
To: Florian Tobias Schandinat; +Cc: kernel-janitors, linux-fbdev, linux-kernel
From: Damien Cassou <damien.cassou@lifl.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.
Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
---
drivers/video/mbx/mbxfb.c | 22 ++++++++--------------
1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/drivers/video/mbx/mbxfb.c b/drivers/video/mbx/mbxfb.c
index 85e4f44..9229acf 100644
--- a/drivers/video/mbx/mbxfb.c
+++ b/drivers/video/mbx/mbxfb.c
@@ -939,8 +939,9 @@ static int __devinit mbxfb_probe(struct platform_device *dev)
}
mfbi->reg_phys_addr = mfbi->reg_res->start;
- mfbi->reg_virt_addr = ioremap_nocache(mfbi->reg_phys_addr,
- res_size(mfbi->reg_req));
+ mfbi->reg_virt_addr = devm_ioremap_nocache(&dev->dev,
+ mfbi->reg_phys_addr,
+ res_size(mfbi->reg_req));
if (!mfbi->reg_virt_addr) {
dev_err(&dev->dev, "failed to ioremap Marathon registers\n");
ret = -EINVAL;
@@ -948,12 +949,12 @@ static int __devinit mbxfb_probe(struct platform_device *dev)
}
virt_base_2700 = mfbi->reg_virt_addr;
- mfbi->fb_virt_addr = ioremap_nocache(mfbi->fb_phys_addr,
- res_size(mfbi->fb_req));
+ mfbi->fb_virt_addr = devm_ioremap_nocache(&dev->dev, mfbi->fb_phys_addr,
+ res_size(mfbi->fb_req));
if (!mfbi->fb_virt_addr) {
dev_err(&dev->dev, "failed to ioremap frame buffer\n");
ret = -EINVAL;
- goto err4;
+ goto err3;
}
fbi->screen_base = (char __iomem *)(mfbi->fb_virt_addr + 0x60000);
@@ -971,7 +972,7 @@ static int __devinit mbxfb_probe(struct platform_device *dev)
if (ret < 0) {
dev_err(&dev->dev, "fb_alloc_cmap failed\n");
ret = -EINVAL;
- goto err5;
+ goto err3;
}
platform_set_drvdata(dev, fbi);
@@ -996,10 +997,6 @@ static int __devinit mbxfb_probe(struct platform_device *dev)
err6:
fb_dealloc_cmap(&fbi->cmap);
-err5:
- iounmap(mfbi->fb_virt_addr);
-err4:
- iounmap(mfbi->reg_virt_addr);
err3:
release_mem_region(mfbi->reg_res->start, res_size(mfbi->reg_res));
err2:
@@ -1026,10 +1023,7 @@ static int __devexit mbxfb_remove(struct platform_device *dev)
if (mfbi->platform_remove)
mfbi->platform_remove(fbi);
- if (mfbi->fb_virt_addr)
- iounmap(mfbi->fb_virt_addr);
- if (mfbi->reg_virt_addr)
- iounmap(mfbi->reg_virt_addr);
+
if (mfbi->reg_req)
release_mem_region(mfbi->reg_req->start,
res_size(mfbi->reg_req));
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 3/5] drivers/video/mbx/mbxfb.c: use devm_ functions
2012-07-31 16:39 ` [PATCH 3/5] drivers/video/mbx/mbxfb.c: " Damien Cassou
@ 2012-08-23 20:37 ` Florian Tobias Schandinat
0 siblings, 0 replies; 20+ messages in thread
From: Florian Tobias Schandinat @ 2012-08-23 20:37 UTC (permalink / raw)
To: Damien Cassou; +Cc: kernel-janitors, linux-fbdev, linux-kernel
On 07/31/2012 04:39 PM, Damien Cassou wrote:
> From: Damien Cassou <damien.cassou@lifl.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.
>
> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
Applied.
Thanks,
Florian Tobias Schandinat
>
> ---
> drivers/video/mbx/mbxfb.c | 22 ++++++++--------------
> 1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/video/mbx/mbxfb.c b/drivers/video/mbx/mbxfb.c
> index 85e4f44..9229acf 100644
> --- a/drivers/video/mbx/mbxfb.c
> +++ b/drivers/video/mbx/mbxfb.c
> @@ -939,8 +939,9 @@ static int __devinit mbxfb_probe(struct platform_device *dev)
> }
> mfbi->reg_phys_addr = mfbi->reg_res->start;
>
> - mfbi->reg_virt_addr = ioremap_nocache(mfbi->reg_phys_addr,
> - res_size(mfbi->reg_req));
> + mfbi->reg_virt_addr = devm_ioremap_nocache(&dev->dev,
> + mfbi->reg_phys_addr,
> + res_size(mfbi->reg_req));
> if (!mfbi->reg_virt_addr) {
> dev_err(&dev->dev, "failed to ioremap Marathon registers\n");
> ret = -EINVAL;
> @@ -948,12 +949,12 @@ static int __devinit mbxfb_probe(struct platform_device *dev)
> }
> virt_base_2700 = mfbi->reg_virt_addr;
>
> - mfbi->fb_virt_addr = ioremap_nocache(mfbi->fb_phys_addr,
> - res_size(mfbi->fb_req));
> + mfbi->fb_virt_addr = devm_ioremap_nocache(&dev->dev, mfbi->fb_phys_addr,
> + res_size(mfbi->fb_req));
> if (!mfbi->fb_virt_addr) {
> dev_err(&dev->dev, "failed to ioremap frame buffer\n");
> ret = -EINVAL;
> - goto err4;
> + goto err3;
> }
>
> fbi->screen_base = (char __iomem *)(mfbi->fb_virt_addr + 0x60000);
> @@ -971,7 +972,7 @@ static int __devinit mbxfb_probe(struct platform_device *dev)
> if (ret < 0) {
> dev_err(&dev->dev, "fb_alloc_cmap failed\n");
> ret = -EINVAL;
> - goto err5;
> + goto err3;
> }
>
> platform_set_drvdata(dev, fbi);
> @@ -996,10 +997,6 @@ static int __devinit mbxfb_probe(struct platform_device *dev)
>
> err6:
> fb_dealloc_cmap(&fbi->cmap);
> -err5:
> - iounmap(mfbi->fb_virt_addr);
> -err4:
> - iounmap(mfbi->reg_virt_addr);
> err3:
> release_mem_region(mfbi->reg_res->start, res_size(mfbi->reg_res));
> err2:
> @@ -1026,10 +1023,7 @@ static int __devexit mbxfb_remove(struct platform_device *dev)
> if (mfbi->platform_remove)
> mfbi->platform_remove(fbi);
>
> - if (mfbi->fb_virt_addr)
> - iounmap(mfbi->fb_virt_addr);
> - if (mfbi->reg_virt_addr)
> - iounmap(mfbi->reg_virt_addr);
> +
> if (mfbi->reg_req)
> release_mem_region(mfbi->reg_req->start,
> res_size(mfbi->reg_req));
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/5] drivers/video/gbefb.c: use devm_ functions
2012-07-31 16:39 [PATCH 0/5] use devm_ functions Damien Cassou
2012-07-31 16:39 ` [PATCH 3/5] drivers/video/mbx/mbxfb.c: " Damien Cassou
@ 2012-07-31 16:39 ` Damien Cassou
2012-08-23 20:37 ` Florian Tobias Schandinat
2012-07-31 16:39 ` [PATCH 1/5] drivers/video/fsl-diu-fb.c: use devm_ functions Damien Cassou
` (2 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Damien Cassou @ 2012-07-31 16:39 UTC (permalink / raw)
To: Florian Tobias Schandinat; +Cc: kernel-janitors, linux-fbdev, linux-kernel
From: Damien Cassou <damien.cassou@lifl.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.
Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
---
drivers/video/gbefb.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/video/gbefb.c b/drivers/video/gbefb.c
index 7e7b7a9..9b79d38 100644
--- a/drivers/video/gbefb.c
+++ b/drivers/video/gbefb.c
@@ -1156,7 +1156,8 @@ static int __devinit gbefb_probe(struct platform_device *p_dev)
goto out_release_framebuffer;
}
- gbe = (struct sgi_gbe *) ioremap(GBE_BASE, sizeof(struct sgi_gbe));
+ gbe = (struct sgi_gbe *) devm_ioremap(&p_dev->dev, GBE_BASE,
+ sizeof(struct sgi_gbe));
if (!gbe) {
printk(KERN_ERR "gbefb: couldn't map mmio region\n");
ret = -ENXIO;
@@ -1170,12 +1171,13 @@ static int __devinit gbefb_probe(struct platform_device *p_dev)
if (!gbe_tiles.cpu) {
printk(KERN_ERR "gbefb: couldn't allocate tiles table\n");
ret = -ENOMEM;
- goto out_unmap;
+ goto out_release_mem_region;
}
if (gbe_mem_phys) {
/* memory was allocated at boot time */
- gbe_mem = ioremap_nocache(gbe_mem_phys, gbe_mem_size);
+ gbe_mem = devm_ioremap_nocache(&p_dev->dev, gbe_mem_phys,
+ gbe_mem_size);
if (!gbe_mem) {
printk(KERN_ERR "gbefb: couldn't map framebuffer\n");
ret = -ENOMEM;
@@ -1241,13 +1243,9 @@ static int __devinit gbefb_probe(struct platform_device *p_dev)
out_gbe_unmap:
if (gbe_dma_addr)
dma_free_coherent(NULL, gbe_mem_size, gbe_mem, gbe_mem_phys);
- else
- iounmap(gbe_mem);
out_tiles_free:
dma_free_coherent(NULL, GBE_TLB_SIZE * sizeof(uint16_t),
(void *)gbe_tiles.cpu, gbe_tiles.dma);
-out_unmap:
- iounmap(gbe);
out_release_mem_region:
release_mem_region(GBE_BASE, sizeof(struct sgi_gbe));
out_release_framebuffer:
@@ -1264,12 +1262,9 @@ static int __devexit gbefb_remove(struct platform_device* p_dev)
gbe_turn_off();
if (gbe_dma_addr)
dma_free_coherent(NULL, gbe_mem_size, gbe_mem, gbe_mem_phys);
- else
- iounmap(gbe_mem);
dma_free_coherent(NULL, GBE_TLB_SIZE * sizeof(uint16_t),
(void *)gbe_tiles.cpu, gbe_tiles.dma);
release_mem_region(GBE_BASE, sizeof(struct sgi_gbe));
- iounmap(gbe);
gbefb_remove_sysfs(&p_dev->dev);
framebuffer_release(info);
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 2/5] drivers/video/gbefb.c: use devm_ functions
2012-07-31 16:39 ` [PATCH 2/5] drivers/video/gbefb.c: " Damien Cassou
@ 2012-08-23 20:37 ` Florian Tobias Schandinat
2012-09-13 19:06 ` Geert Uytterhoeven
0 siblings, 1 reply; 20+ messages in thread
From: Florian Tobias Schandinat @ 2012-08-23 20:37 UTC (permalink / raw)
To: Damien Cassou; +Cc: kernel-janitors, linux-fbdev, linux-kernel
On 07/31/2012 04:39 PM, Damien Cassou wrote:
> From: Damien Cassou <damien.cassou@lifl.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.
>
> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
Applied.
Thanks,
Florian Tobias Schandinat
>
> ---
> drivers/video/gbefb.c | 15 +++++----------
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/video/gbefb.c b/drivers/video/gbefb.c
> index 7e7b7a9..9b79d38 100644
> --- a/drivers/video/gbefb.c
> +++ b/drivers/video/gbefb.c
> @@ -1156,7 +1156,8 @@ static int __devinit gbefb_probe(struct platform_device *p_dev)
> goto out_release_framebuffer;
> }
>
> - gbe = (struct sgi_gbe *) ioremap(GBE_BASE, sizeof(struct sgi_gbe));
> + gbe = (struct sgi_gbe *) devm_ioremap(&p_dev->dev, GBE_BASE,
> + sizeof(struct sgi_gbe));
> if (!gbe) {
> printk(KERN_ERR "gbefb: couldn't map mmio region\n");
> ret = -ENXIO;
> @@ -1170,12 +1171,13 @@ static int __devinit gbefb_probe(struct platform_device *p_dev)
> if (!gbe_tiles.cpu) {
> printk(KERN_ERR "gbefb: couldn't allocate tiles table\n");
> ret = -ENOMEM;
> - goto out_unmap;
> + goto out_release_mem_region;
> }
>
> if (gbe_mem_phys) {
> /* memory was allocated at boot time */
> - gbe_mem = ioremap_nocache(gbe_mem_phys, gbe_mem_size);
> + gbe_mem = devm_ioremap_nocache(&p_dev->dev, gbe_mem_phys,
> + gbe_mem_size);
> if (!gbe_mem) {
> printk(KERN_ERR "gbefb: couldn't map framebuffer\n");
> ret = -ENOMEM;
> @@ -1241,13 +1243,9 @@ static int __devinit gbefb_probe(struct platform_device *p_dev)
> out_gbe_unmap:
> if (gbe_dma_addr)
> dma_free_coherent(NULL, gbe_mem_size, gbe_mem, gbe_mem_phys);
> - else
> - iounmap(gbe_mem);
> out_tiles_free:
> dma_free_coherent(NULL, GBE_TLB_SIZE * sizeof(uint16_t),
> (void *)gbe_tiles.cpu, gbe_tiles.dma);
> -out_unmap:
> - iounmap(gbe);
> out_release_mem_region:
> release_mem_region(GBE_BASE, sizeof(struct sgi_gbe));
> out_release_framebuffer:
> @@ -1264,12 +1262,9 @@ static int __devexit gbefb_remove(struct platform_device* p_dev)
> gbe_turn_off();
> if (gbe_dma_addr)
> dma_free_coherent(NULL, gbe_mem_size, gbe_mem, gbe_mem_phys);
> - else
> - iounmap(gbe_mem);
> dma_free_coherent(NULL, GBE_TLB_SIZE * sizeof(uint16_t),
> (void *)gbe_tiles.cpu, gbe_tiles.dma);
> release_mem_region(GBE_BASE, sizeof(struct sgi_gbe));
> - iounmap(gbe);
> gbefb_remove_sysfs(&p_dev->dev);
> framebuffer_release(info);
>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/5] drivers/video/gbefb.c: use devm_ functions
2012-08-23 20:37 ` Florian Tobias Schandinat
@ 2012-09-13 19:06 ` Geert Uytterhoeven
2012-09-20 21:58 ` Florian Tobias Schandinat
0 siblings, 1 reply; 20+ messages in thread
From: Geert Uytterhoeven @ 2012-09-13 19:06 UTC (permalink / raw)
To: Florian Tobias Schandinat
Cc: Damien Cassou, kernel-janitors, linux-fbdev, linux-kernel
On Thu, Aug 23, 2012 at 10:37 PM, Florian Tobias Schandinat
<FlorianSchandinat@gmx.de> wrote:
> On 07/31/2012 04:39 PM, Damien Cassou wrote:
>> From: Damien Cassou <damien.cassou@lifl.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.
>>
>> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
>
> Applied.
>> diff --git a/drivers/video/gbefb.c b/drivers/video/gbefb.c
>> index 7e7b7a9..9b79d38 100644
>> --- a/drivers/video/gbefb.c
>> +++ b/drivers/video/gbefb.c
>> @@ -1156,7 +1156,8 @@ static int __devinit gbefb_probe(struct platform_device *p_dev)
>> goto out_release_framebuffer;
>> }
>>
>> - gbe = (struct sgi_gbe *) ioremap(GBE_BASE, sizeof(struct sgi_gbe));
>> + gbe = (struct sgi_gbe *) devm_ioremap(&p_dev->dev, GBE_BASE,
>> + sizeof(struct sgi_gbe));
drivers/video/gbefb.c:1159:16: error: implicit declaration of function
'devm_ioremap' [-Werror=implicit-function-declaration]
drivers/video/gbefb.c:1179:3: error: implicit declaration of function
'devm_ioremap_nocache' [-Werror=implicit-function-declaration]
http://kisskb.ellerman.id.au/kisskb/buildresult/7187731/
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/5] drivers/video/gbefb.c: use devm_ functions
2012-09-13 19:06 ` Geert Uytterhoeven
@ 2012-09-20 21:58 ` Florian Tobias Schandinat
2012-10-11 0:38 ` [PATCH] gbefb: fix compile error Florian Tobias Schandinat
0 siblings, 1 reply; 20+ messages in thread
From: Florian Tobias Schandinat @ 2012-09-20 21:58 UTC (permalink / raw)
To: Geert Uytterhoeven, Damien Cassou
Cc: kernel-janitors, linux-fbdev, linux-kernel
Hi Geert,
On 09/13/2012 07:06 PM, Geert Uytterhoeven wrote:
> On Thu, Aug 23, 2012 at 10:37 PM, Florian Tobias Schandinat
> <FlorianSchandinat@gmx.de> wrote:
>> On 07/31/2012 04:39 PM, Damien Cassou wrote:
>>> From: Damien Cassou <damien.cassou@lifl.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.
>>>
>>> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
>>
>> Applied.
>
>>> diff --git a/drivers/video/gbefb.c b/drivers/video/gbefb.c
>>> index 7e7b7a9..9b79d38 100644
>>> --- a/drivers/video/gbefb.c
>>> +++ b/drivers/video/gbefb.c
>>> @@ -1156,7 +1156,8 @@ static int __devinit gbefb_probe(struct platform_device *p_dev)
>>> goto out_release_framebuffer;
>>> }
>>>
>>> - gbe = (struct sgi_gbe *) ioremap(GBE_BASE, sizeof(struct sgi_gbe));
>>> + gbe = (struct sgi_gbe *) devm_ioremap(&p_dev->dev, GBE_BASE,
>>> + sizeof(struct sgi_gbe));
>
> drivers/video/gbefb.c:1159:16: error: implicit declaration of function
> 'devm_ioremap' [-Werror=implicit-function-declaration]
> drivers/video/gbefb.c:1179:3: error: implicit declaration of function
> 'devm_ioremap_nocache' [-Werror=implicit-function-declaration]
Thanks for pointing this out. I guess the solution is to replace the
include asm/io.h by linux/io.h as Axel Lin did for some other driver in
this patch series.
@Damien:
Can you please do a patch as I suggested and preferable also recheck the
other drivers you touched?
And it probably would have been a good idea to mention that you didn't
compile-test it. I do some compile tests before pushing my branch but
that is far from complete due to arch and platform specific drivers.
Best regards,
Florian Tobias Schandinat
>
> http://kisskb.ellerman.id.au/kisskb/buildresult/7187731/
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
>
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH] gbefb: fix compile error
2012-09-20 21:58 ` Florian Tobias Schandinat
@ 2012-10-11 0:38 ` Florian Tobias Schandinat
0 siblings, 0 replies; 20+ messages in thread
From: Florian Tobias Schandinat @ 2012-10-11 0:38 UTC (permalink / raw)
To: linux-fbdev
Cc: Geert Uytterhoeven, linux-kernel, Florian Tobias Schandinat,
Damien Cassou
The patch "drivers/video/gbefb.c: use devm_ functions" caused a
compile error.
drivers/video/gbefb.c:1159:16: error: implicit declaration of function
'devm_ioremap' [-Werror=implicit-function-declaration]
drivers/video/gbefb.c:1179:3: error: implicit declaration of function
'devm_ioremap_nocache' [-Werror=implicit-function-declaration]
Fix it by including linux/io.h which defines those functions.
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
Cc: Damien Cassou <damien.cassou@lifl.fr>
---
drivers/video/gbefb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/gbefb.c b/drivers/video/gbefb.c
index 9b79d38..8dc1f7a 100644
--- a/drivers/video/gbefb.c
+++ b/drivers/video/gbefb.c
@@ -20,6 +20,7 @@
#include <linux/kernel.h>
#include <linux/mm.h>
#include <linux/module.h>
+#include <linux/io.h>
#ifdef CONFIG_X86
#include <asm/mtrr.h>
@@ -28,7 +29,6 @@
#include <asm/addrspace.h>
#endif
#include <asm/byteorder.h>
-#include <asm/io.h>
#include <asm/tlbflush.h>
#include <video/gbe.h>
--
1.7.10.4
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 1/5] drivers/video/fsl-diu-fb.c: use devm_ functions
2012-07-31 16:39 [PATCH 0/5] use devm_ functions Damien Cassou
2012-07-31 16:39 ` [PATCH 3/5] drivers/video/mbx/mbxfb.c: " Damien Cassou
2012-07-31 16:39 ` [PATCH 2/5] drivers/video/gbefb.c: " Damien Cassou
@ 2012-07-31 16:39 ` Damien Cassou
2012-08-23 20:38 ` Florian Tobias Schandinat
2012-07-31 16:39 ` [PATCH 4/5] drivers/video/exynos/exynos_mipi_dsi.c: " Damien Cassou
2012-07-31 16:39 ` [PATCH 5/5] drivers/video/exynos/exynos_dp_core.c: " Damien Cassou
4 siblings, 1 reply; 20+ messages in thread
From: Damien Cassou @ 2012-07-31 16:39 UTC (permalink / raw)
To: Florian Tobias Schandinat; +Cc: kernel-janitors, linux-fbdev, linux-kernel
From: Damien Cassou <damien.cassou@lifl.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.
Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
---
drivers/video/fsl-diu-fb.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/drivers/video/fsl-diu-fb.c b/drivers/video/fsl-diu-fb.c
index 458c006..19194c5 100644
--- a/drivers/video/fsl-diu-fb.c
+++ b/drivers/video/fsl-diu-fb.c
@@ -1501,8 +1501,8 @@ static int __devinit fsl_diu_probe(struct platform_device *pdev)
unsigned int i;
int ret;
- data = dma_alloc_coherent(&pdev->dev, sizeof(struct fsl_diu_data),
- &dma_addr, GFP_DMA | __GFP_ZERO);
+ data = dmam_alloc_coherent(&pdev->dev, sizeof(struct fsl_diu_data),
+ &dma_addr, GFP_DMA | __GFP_ZERO);
if (!data)
return -ENOMEM;
data->dma_addr = dma_addr;
@@ -1628,9 +1628,6 @@ error:
iounmap(data->diu_reg);
- dma_free_coherent(&pdev->dev, sizeof(struct fsl_diu_data), data,
- data->dma_addr);
-
return ret;
}
@@ -1648,9 +1645,6 @@ static int fsl_diu_remove(struct platform_device *pdev)
iounmap(data->diu_reg);
- dma_free_coherent(&pdev->dev, sizeof(struct fsl_diu_data), data,
- data->dma_addr);
-
return 0;
}
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] drivers/video/fsl-diu-fb.c: use devm_ functions
2012-07-31 16:39 ` [PATCH 1/5] drivers/video/fsl-diu-fb.c: use devm_ functions Damien Cassou
@ 2012-08-23 20:38 ` Florian Tobias Schandinat
0 siblings, 0 replies; 20+ messages in thread
From: Florian Tobias Schandinat @ 2012-08-23 20:38 UTC (permalink / raw)
To: Damien Cassou; +Cc: kernel-janitors, linux-fbdev, linux-kernel
On 07/31/2012 04:39 PM, Damien Cassou wrote:
> From: Damien Cassou <damien.cassou@lifl.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.
>
> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
Applied.
Thanks,
Florian Tobias Schandinat
>
> ---
> drivers/video/fsl-diu-fb.c | 11 ++---------
> 1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/video/fsl-diu-fb.c b/drivers/video/fsl-diu-fb.c
> index 458c006..19194c5 100644
> --- a/drivers/video/fsl-diu-fb.c
> +++ b/drivers/video/fsl-diu-fb.c
> @@ -1501,8 +1501,8 @@ static int __devinit fsl_diu_probe(struct platform_device *pdev)
> unsigned int i;
> int ret;
>
> - data = dma_alloc_coherent(&pdev->dev, sizeof(struct fsl_diu_data),
> - &dma_addr, GFP_DMA | __GFP_ZERO);
> + data = dmam_alloc_coherent(&pdev->dev, sizeof(struct fsl_diu_data),
> + &dma_addr, GFP_DMA | __GFP_ZERO);
> if (!data)
> return -ENOMEM;
> data->dma_addr = dma_addr;
> @@ -1628,9 +1628,6 @@ error:
>
> iounmap(data->diu_reg);
>
> - dma_free_coherent(&pdev->dev, sizeof(struct fsl_diu_data), data,
> - data->dma_addr);
> -
> return ret;
> }
>
> @@ -1648,9 +1645,6 @@ static int fsl_diu_remove(struct platform_device *pdev)
>
> iounmap(data->diu_reg);
>
> - dma_free_coherent(&pdev->dev, sizeof(struct fsl_diu_data), data,
> - data->dma_addr);
> -
> return 0;
> }
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/5] drivers/video/exynos/exynos_mipi_dsi.c: use devm_ functions
2012-07-31 16:39 [PATCH 0/5] use devm_ functions Damien Cassou
` (2 preceding siblings ...)
2012-07-31 16:39 ` [PATCH 1/5] drivers/video/fsl-diu-fb.c: use devm_ functions Damien Cassou
@ 2012-07-31 16:39 ` Damien Cassou
2012-07-31 16:39 ` [PATCH 5/5] drivers/video/exynos/exynos_dp_core.c: " Damien Cassou
4 siblings, 0 replies; 20+ messages in thread
From: Damien Cassou @ 2012-07-31 16:39 UTC (permalink / raw)
To: Inki Dae
Cc: kernel-janitors, Donghwa Lee, Kyungmin Park,
Florian Tobias Schandinat, linux-fbdev, linux-kernel
From: Damien Cassou <damien.cassou@lifl.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.
Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
---
drivers/video/exynos/exynos_mipi_dsi.c | 41 ++++++++++++---------------------
1 file changed, 15 insertions(+), 26 deletions(-)
diff --git a/drivers/video/exynos/exynos_mipi_dsi.c b/drivers/video/exynos/exynos_mipi_dsi.c
index 4bc2b8a..c277a07 100644
--- a/drivers/video/exynos/exynos_mipi_dsi.c
+++ b/drivers/video/exynos/exynos_mipi_dsi.c
@@ -336,7 +336,8 @@ static int exynos_mipi_dsi_probe(struct platform_device *pdev)
struct mipi_dsim_ddi *dsim_ddi;
int ret = -EINVAL;
- dsim = kzalloc(sizeof(struct mipi_dsim_device), GFP_KERNEL);
+ dsim = devm_kzalloc(&pdev->dev, sizeof(struct mipi_dsim_device),
+ GFP_KERNEL);
if (!dsim) {
dev_err(&pdev->dev, "failed to allocate dsim object.\n");
return -ENOMEM;
@@ -350,13 +351,13 @@ static int exynos_mipi_dsi_probe(struct platform_device *pdev)
dsim_pd = (struct mipi_dsim_platform_data *)dsim->pd;
if (dsim_pd == NULL) {
dev_err(&pdev->dev, "failed to get platform data for dsim.\n");
- goto err_clock_get;
+ goto err_platform_get_irq;
}
/* get mipi_dsim_config. */
dsim_config = dsim_pd->dsim_config;
if (dsim_config == NULL) {
dev_err(&pdev->dev, "failed to get dsim config data.\n");
- goto err_clock_get;
+ goto err_platform_get_irq;
}
dsim->dsim_config = dsim_config;
@@ -367,13 +368,13 @@ static int exynos_mipi_dsi_probe(struct platform_device *pdev)
ret = regulator_bulk_get(&pdev->dev, ARRAY_SIZE(supplies), supplies);
if (ret) {
dev_err(&pdev->dev, "Failed to get regulators: %d\n", ret);
- goto err_clock_get;
+ goto err_platform_get_irq;
}
- dsim->clock = clk_get(&pdev->dev, "dsim0");
+ dsim->clock = devm_clk_get(&pdev->dev, "dsim0");
if (IS_ERR(dsim->clock)) {
dev_err(&pdev->dev, "failed to get dsim clock source\n");
- goto err_clock_get;
+ goto err_platform_get_irq;
}
clk_enable(dsim->clock);
@@ -384,15 +385,17 @@ static int exynos_mipi_dsi_probe(struct platform_device *pdev)
goto err_platform_get;
}
- dsim->res = request_mem_region(res->start, resource_size(res),
- dev_name(&pdev->dev));
+ dsim->res = devm_request_mem_region(&pdev->dev, res->start,
+ resource_size(res),
+ dev_name(&pdev->dev));
if (!dsim->res) {
dev_err(&pdev->dev, "failed to request io memory region\n");
ret = -ENOMEM;
- goto err_mem_region;
+ goto err_platform_get;
}
- dsim->reg_base = ioremap(res->start, resource_size(res));
+ dsim->reg_base = devm_ioremap(&pdev->dev, res->start,
+ resource_size(res));
if (!dsim->reg_base) {
dev_err(&pdev->dev, "failed to remap io region\n");
ret = -ENOMEM;
@@ -405,7 +408,7 @@ static int exynos_mipi_dsi_probe(struct platform_device *pdev)
dsim_ddi = exynos_mipi_dsi_bind_lcd_ddi(dsim, dsim_pd->lcd_panel_name);
if (!dsim_ddi) {
dev_err(&pdev->dev, "mipi_dsim_ddi object not found.\n");
- goto err_bind;
+ goto err_ioremap;
}
dsim->irq = platform_get_irq(pdev, 0);
@@ -424,7 +427,7 @@ static int exynos_mipi_dsi_probe(struct platform_device *pdev)
if (ret != 0) {
dev_err(&pdev->dev, "failed to request dsim irq\n");
ret = -EINVAL;
- goto err_bind;
+ goto err_ioremap;
}
/* enable interrupts */
@@ -466,20 +469,11 @@ done:
return 0;
-err_bind:
- iounmap(dsim->reg_base);
-
err_ioremap:
release_mem_region(dsim->res->start, resource_size(dsim->res));
-err_mem_region:
- release_resource(dsim->res);
-
err_platform_get:
clk_disable(dsim->clock);
- clk_put(dsim->clock);
-err_clock_get:
- kfree(dsim);
err_platform_get_irq:
return ret;
@@ -491,12 +485,8 @@ static int __devexit exynos_mipi_dsi_remove(struct platform_device *pdev)
struct mipi_dsim_ddi *dsim_ddi, *next;
struct mipi_dsim_lcd_driver *dsim_lcd_drv;
- iounmap(dsim->reg_base);
-
clk_disable(dsim->clock);
- clk_put(dsim->clock);
- release_resource(dsim->res);
release_mem_region(dsim->res->start, resource_size(dsim->res));
list_for_each_entry_safe(dsim_ddi, next, &dsim_ddi_list, list) {
@@ -514,7 +504,6 @@ static int __devexit exynos_mipi_dsi_remove(struct platform_device *pdev)
}
regulator_bulk_free(ARRAY_SIZE(supplies), supplies);
- kfree(dsim);
return 0;
}
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 5/5] drivers/video/exynos/exynos_dp_core.c: use devm_ functions
2012-07-31 16:39 [PATCH 0/5] use devm_ functions Damien Cassou
` (3 preceding siblings ...)
2012-07-31 16:39 ` [PATCH 4/5] drivers/video/exynos/exynos_mipi_dsi.c: " Damien Cassou
@ 2012-07-31 16:39 ` Damien Cassou
2012-07-31 23:21 ` Jingoo Han
4 siblings, 1 reply; 20+ messages in thread
From: Damien Cassou @ 2012-07-31 16:39 UTC (permalink / raw)
To: Jingoo Han
Cc: kernel-janitors, Florian Tobias Schandinat, linux-fbdev,
linux-kernel
From: Damien Cassou <damien.cassou@lifl.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.
Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
---
drivers/video/exynos/exynos_dp_core.c | 27 +++++++--------------------
1 file changed, 7 insertions(+), 20 deletions(-)
diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
index c6c016a..00fe4f0 100644
--- a/drivers/video/exynos/exynos_dp_core.c
+++ b/drivers/video/exynos/exynos_dp_core.c
@@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
dp->dev = &pdev->dev;
- dp->clock = clk_get(&pdev->dev, "dp");
+ dp->clock = devm_clk_get(&pdev->dev, "dp");
if (IS_ERR(dp->clock)) {
dev_err(&pdev->dev, "failed to get clock\n");
return PTR_ERR(dp->clock);
@@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
clk_enable(dp->clock);
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!res) {
- dev_err(&pdev->dev, "failed to get registers\n");
- ret = -EINVAL;
- goto err_clock;
- }
dp->reg_base = devm_request_and_ioremap(&pdev->dev, res);
if (!dp->reg_base) {
dev_err(&pdev->dev, "failed to ioremap\n");
- ret = -ENOMEM;
- goto err_clock;
+ return -ENOMEM;
}
dp->irq = platform_get_irq(pdev, 0);
if (!dp->irq) {
dev_err(&pdev->dev, "failed to get irq\n");
- ret = -ENODEV;
- goto err_clock;
+ return -ENODEV;
}
ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0,
"exynos-dp", dp);
if (ret) {
dev_err(&pdev->dev, "failed to request irq\n");
- goto err_clock;
+ return ret;
}
dp->video_info = pdata->video_info;
@@ -917,7 +910,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
ret = exynos_dp_detect_hpd(dp);
if (ret) {
dev_err(&pdev->dev, "unable to detect hpd\n");
- goto err_clock;
+ return ret;
}
exynos_dp_handle_edid(dp);
@@ -926,7 +919,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
dp->video_info->link_rate);
if (ret) {
dev_err(&pdev->dev, "unable to do link train\n");
- goto err_clock;
+ return ret;
}
exynos_dp_enable_scramble(dp, 1);
@@ -940,17 +933,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
ret = exynos_dp_config_video(dp, dp->video_info);
if (ret) {
dev_err(&pdev->dev, "unable to config video\n");
- goto err_clock;
+ return ret;
}
platform_set_drvdata(pdev, dp);
return 0;
-
-err_clock:
- clk_put(dp->clock);
-
- return ret;
}
static int __devexit exynos_dp_remove(struct platform_device *pdev)
@@ -962,7 +950,6 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
pdata->phy_exit();
clk_disable(dp->clock);
- clk_put(dp->clock);
return 0;
}
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 5/5] drivers/video/exynos/exynos_dp_core.c: use devm_ functions
2012-07-31 16:39 ` [PATCH 5/5] drivers/video/exynos/exynos_dp_core.c: " Damien Cassou
@ 2012-07-31 23:21 ` Jingoo Han
2012-08-01 3:59 ` Sachin Kamat
0 siblings, 1 reply; 20+ messages in thread
From: Jingoo Han @ 2012-07-31 23:21 UTC (permalink / raw)
To: 'Damien Cassou'
Cc: kernel-janitors, 'Florian Tobias Schandinat', linux-fbdev,
linux-kernel, 'Jingoo Han'
On Wednesday, August 01, 2012 1:39 AM Damien Cassou wrote:
>
> From: Damien Cassou <damien.cassou@lifl.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.
>
> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
>
> ---
> drivers/video/exynos/exynos_dp_core.c | 27 +++++++--------------------
> 1 file changed, 7 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> index c6c016a..00fe4f0 100644
> --- a/drivers/video/exynos/exynos_dp_core.c
> +++ b/drivers/video/exynos/exynos_dp_core.c
> @@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>
> dp->dev = &pdev->dev;
>
> - dp->clock = clk_get(&pdev->dev, "dp");
> + dp->clock = devm_clk_get(&pdev->dev, "dp");
> if (IS_ERR(dp->clock)) {
> dev_err(&pdev->dev, "failed to get clock\n");
> return PTR_ERR(dp->clock);
> @@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> clk_enable(dp->clock);
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - if (!res) {
> - dev_err(&pdev->dev, "failed to get registers\n");
> - ret = -EINVAL;
> - goto err_clock;
> - }
Why do you remove this return check?
If there is no reason, please, do it as follows:
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!res) {
dev_err(&pdev->dev, "failed to get registers\n");
- ret = -EINVAL;
- goto err_clock;
+ return -EINVAL;
}
Best regards,
Jingoo Han
>
> dp->reg_base = devm_request_and_ioremap(&pdev->dev, res);
> if (!dp->reg_base) {
> dev_err(&pdev->dev, "failed to ioremap\n");
> - ret = -ENOMEM;
> - goto err_clock;
> + return -ENOMEM;
> }
>
> dp->irq = platform_get_irq(pdev, 0);
> if (!dp->irq) {
> dev_err(&pdev->dev, "failed to get irq\n");
> - ret = -ENODEV;
> - goto err_clock;
> + return -ENODEV;
> }
>
> ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0,
> "exynos-dp", dp);
> if (ret) {
> dev_err(&pdev->dev, "failed to request irq\n");
> - goto err_clock;
> + return ret;
> }
>
> dp->video_info = pdata->video_info;
> @@ -917,7 +910,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> ret = exynos_dp_detect_hpd(dp);
> if (ret) {
> dev_err(&pdev->dev, "unable to detect hpd\n");
> - goto err_clock;
> + return ret;
> }
>
> exynos_dp_handle_edid(dp);
> @@ -926,7 +919,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> dp->video_info->link_rate);
> if (ret) {
> dev_err(&pdev->dev, "unable to do link train\n");
> - goto err_clock;
> + return ret;
> }
>
> exynos_dp_enable_scramble(dp, 1);
> @@ -940,17 +933,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> ret = exynos_dp_config_video(dp, dp->video_info);
> if (ret) {
> dev_err(&pdev->dev, "unable to config video\n");
> - goto err_clock;
> + return ret;
> }
>
> platform_set_drvdata(pdev, dp);
>
> return 0;
> -
> -err_clock:
> - clk_put(dp->clock);
> -
> - return ret;
> }
>
> static int __devexit exynos_dp_remove(struct platform_device *pdev)
> @@ -962,7 +950,6 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
> pdata->phy_exit();
>
> clk_disable(dp->clock);
> - clk_put(dp->clock);
>
> return 0;
> }
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 5/5] drivers/video/exynos/exynos_dp_core.c: use devm_ functions
2012-07-31 23:21 ` Jingoo Han
@ 2012-08-01 3:59 ` Sachin Kamat
2012-08-01 4:30 ` Jingoo Han
2012-08-01 5:08 ` Jingoo Han
0 siblings, 2 replies; 20+ messages in thread
From: Sachin Kamat @ 2012-08-01 3:59 UTC (permalink / raw)
To: Jingoo Han
Cc: Damien Cassou, kernel-janitors, Florian Tobias Schandinat,
linux-fbdev, linux-kernel
On 1 August 2012 04:51, Jingoo Han <jg1.han@samsung.com> wrote:
> On Wednesday, August 01, 2012 1:39 AM Damien Cassou wrote:
>>
>> From: Damien Cassou <damien.cassou@lifl.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.
>>
>> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
>>
>> ---
>> drivers/video/exynos/exynos_dp_core.c | 27 +++++++--------------------
>> 1 file changed, 7 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
>> index c6c016a..00fe4f0 100644
>> --- a/drivers/video/exynos/exynos_dp_core.c
>> +++ b/drivers/video/exynos/exynos_dp_core.c
>> @@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>>
>> dp->dev = &pdev->dev;
>>
>> - dp->clock = clk_get(&pdev->dev, "dp");
>> + dp->clock = devm_clk_get(&pdev->dev, "dp");
>> if (IS_ERR(dp->clock)) {
>> dev_err(&pdev->dev, "failed to get clock\n");
>> return PTR_ERR(dp->clock);
>> @@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>> clk_enable(dp->clock);
>>
>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> - if (!res) {
>> - dev_err(&pdev->dev, "failed to get registers\n");
>> - ret = -EINVAL;
>> - goto err_clock;
>> - }
>
> Why do you remove this return check?
> If there is no reason, please, do it as follows:
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> if (!res) {
> dev_err(&pdev->dev, "failed to get registers\n");
> - ret = -EINVAL;
> - goto err_clock;
> + return -EINVAL;
> }
>
>
devm_request_and_ioremap function checks the validity of res. Hence
this check above is redundant and can be removed.
Damien,
This patch only adds devm_clk_get() function. Hence you could make the
subject line more specific.
> Best regards,
> Jingoo Han
>
>
>>
>> dp->reg_base = devm_request_and_ioremap(&pdev->dev, res);
>> if (!dp->reg_base) {
>> dev_err(&pdev->dev, "failed to ioremap\n");
>> - ret = -ENOMEM;
>> - goto err_clock;
>> + return -ENOMEM;
>> }
>>
>> dp->irq = platform_get_irq(pdev, 0);
>> if (!dp->irq) {
>> dev_err(&pdev->dev, "failed to get irq\n");
>> - ret = -ENODEV;
>> - goto err_clock;
>> + return -ENODEV;
>> }
>>
>> ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0,
>> "exynos-dp", dp);
>> if (ret) {
>> dev_err(&pdev->dev, "failed to request irq\n");
>> - goto err_clock;
>> + return ret;
>> }
>>
>> dp->video_info = pdata->video_info;
>> @@ -917,7 +910,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>> ret = exynos_dp_detect_hpd(dp);
>> if (ret) {
>> dev_err(&pdev->dev, "unable to detect hpd\n");
>> - goto err_clock;
>> + return ret;
>> }
>>
>> exynos_dp_handle_edid(dp);
>> @@ -926,7 +919,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>> dp->video_info->link_rate);
>> if (ret) {
>> dev_err(&pdev->dev, "unable to do link train\n");
>> - goto err_clock;
>> + return ret;
>> }
>>
>> exynos_dp_enable_scramble(dp, 1);
>> @@ -940,17 +933,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>> ret = exynos_dp_config_video(dp, dp->video_info);
>> if (ret) {
>> dev_err(&pdev->dev, "unable to config video\n");
>> - goto err_clock;
>> + return ret;
>> }
>>
>> platform_set_drvdata(pdev, dp);
>>
>> return 0;
>> -
>> -err_clock:
>> - clk_put(dp->clock);
>> -
>> - return ret;
>> }
>>
>> static int __devexit exynos_dp_remove(struct platform_device *pdev)
>> @@ -962,7 +950,6 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
>> pdata->phy_exit();
>>
>> clk_disable(dp->clock);
>> - clk_put(dp->clock);
>>
>> return 0;
>> }
>
> --
> 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/
--
With warm regards,
Sachin
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 5/5] drivers/video/exynos/exynos_dp_core.c: use devm_ functions
2012-08-01 3:59 ` Sachin Kamat
@ 2012-08-01 4:30 ` Jingoo Han
2012-08-01 4:38 ` Sachin Kamat
2012-08-01 5:08 ` Jingoo Han
1 sibling, 1 reply; 20+ messages in thread
From: Jingoo Han @ 2012-08-01 4:30 UTC (permalink / raw)
To: 'Sachin Kamat'
Cc: 'Damien Cassou', kernel-janitors,
'Florian Tobias Schandinat', linux-fbdev, linux-kernel,
'Jingoo Han'
On Wednesday, August 01, 2012 1:00 PMSachin Kamat wrote:
>
> On 1 August 2012 04:51, Jingoo Han <jg1.han@samsung.com> wrote:
> > On Wednesday, August 01, 2012 1:39 AM Damien Cassou wrote:
> >>
> >> From: Damien Cassou <damien.cassou@lifl.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.
> >>
> >> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
> >>
> >> ---
> >> drivers/video/exynos/exynos_dp_core.c | 27 +++++++--------------------
> >> 1 file changed, 7 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> >> index c6c016a..00fe4f0 100644
> >> --- a/drivers/video/exynos/exynos_dp_core.c
> >> +++ b/drivers/video/exynos/exynos_dp_core.c
> >> @@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >>
> >> dp->dev = &pdev->dev;
> >>
> >> - dp->clock = clk_get(&pdev->dev, "dp");
> >> + dp->clock = devm_clk_get(&pdev->dev, "dp");
> >> if (IS_ERR(dp->clock)) {
> >> dev_err(&pdev->dev, "failed to get clock\n");
> >> return PTR_ERR(dp->clock);
> >> @@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> clk_enable(dp->clock);
> >>
> >> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> - if (!res) {
> >> - dev_err(&pdev->dev, "failed to get registers\n");
> >> - ret = -EINVAL;
> >> - goto err_clock;
> >> - }
> >
> > Why do you remove this return check?
> > If there is no reason, please, do it as follows:
> >
> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > if (!res) {
> > dev_err(&pdev->dev, "failed to get registers\n");
> > - ret = -EINVAL;
> > - goto err_clock;
> > + return -EINVAL;
> > }
> >
> >
>
> devm_request_and_ioremap function checks the validity of res. Hence
> this check above is redundant and can be removed.
I don't think so.
Even though function called next checks the NULL value,
for robustness, the return value of platform_get_resource() should be
checked.
It is possible that devm_request_and_ioremap() can be changed in the future,
as request_mem_region() & ioremap() were changed to devm_request_and_ioremap().
Best regards,
Jingoo Han
>
> Damien,
> This patch only adds devm_clk_get() function. Hence you could make the
> subject line more specific.
>
>
>
>
> > Best regards,
> > Jingoo Han
> >
> >
> >>
> >> dp->reg_base = devm_request_and_ioremap(&pdev->dev, res);
> >> if (!dp->reg_base) {
> >> dev_err(&pdev->dev, "failed to ioremap\n");
> >> - ret = -ENOMEM;
> >> - goto err_clock;
> >> + return -ENOMEM;
> >> }
> >>
> >> dp->irq = platform_get_irq(pdev, 0);
> >> if (!dp->irq) {
> >> dev_err(&pdev->dev, "failed to get irq\n");
> >> - ret = -ENODEV;
> >> - goto err_clock;
> >> + return -ENODEV;
> >> }
> >>
> >> ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0,
> >> "exynos-dp", dp);
> >> if (ret) {
> >> dev_err(&pdev->dev, "failed to request irq\n");
> >> - goto err_clock;
> >> + return ret;
> >> }
> >>
> >> dp->video_info = pdata->video_info;
> >> @@ -917,7 +910,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> ret = exynos_dp_detect_hpd(dp);
> >> if (ret) {
> >> dev_err(&pdev->dev, "unable to detect hpd\n");
> >> - goto err_clock;
> >> + return ret;
> >> }
> >>
> >> exynos_dp_handle_edid(dp);
> >> @@ -926,7 +919,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> dp->video_info->link_rate);
> >> if (ret) {
> >> dev_err(&pdev->dev, "unable to do link train\n");
> >> - goto err_clock;
> >> + return ret;
> >> }
> >>
> >> exynos_dp_enable_scramble(dp, 1);
> >> @@ -940,17 +933,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> ret = exynos_dp_config_video(dp, dp->video_info);
> >> if (ret) {
> >> dev_err(&pdev->dev, "unable to config video\n");
> >> - goto err_clock;
> >> + return ret;
> >> }
> >>
> >> platform_set_drvdata(pdev, dp);
> >>
> >> return 0;
> >> -
> >> -err_clock:
> >> - clk_put(dp->clock);
> >> -
> >> - return ret;
> >> }
> >>
> >> static int __devexit exynos_dp_remove(struct platform_device *pdev)
> >> @@ -962,7 +950,6 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
> >> pdata->phy_exit();
> >>
> >> clk_disable(dp->clock);
> >> - clk_put(dp->clock);
> >>
> >> return 0;
> >> }
> >
> > --
> > 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/
>
>
>
> --
> With warm regards,
> Sachin
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 5/5] drivers/video/exynos/exynos_dp_core.c: use devm_ functions
2012-08-01 4:30 ` Jingoo Han
@ 2012-08-01 4:38 ` Sachin Kamat
2012-08-01 4:57 ` Jingoo Han
0 siblings, 1 reply; 20+ messages in thread
From: Sachin Kamat @ 2012-08-01 4:38 UTC (permalink / raw)
To: Jingoo Han
Cc: Damien Cassou, kernel-janitors, Florian Tobias Schandinat,
linux-fbdev, linux-kernel
On 1 August 2012 10:00, Jingoo Han <jg1.han@samsung.com> wrote:
> On Wednesday, August 01, 2012 1:00 PMSachin Kamat wrote:
>>
>> On 1 August 2012 04:51, Jingoo Han <jg1.han@samsung.com> wrote:
>> > On Wednesday, August 01, 2012 1:39 AM Damien Cassou wrote:
>> >>
>> >> From: Damien Cassou <damien.cassou@lifl.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.
>> >>
>> >> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
>> >>
>> >> ---
>> >> drivers/video/exynos/exynos_dp_core.c | 27 +++++++--------------------
>> >> 1 file changed, 7 insertions(+), 20 deletions(-)
>> >>
>> >> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
>> >> index c6c016a..00fe4f0 100644
>> >> --- a/drivers/video/exynos/exynos_dp_core.c
>> >> +++ b/drivers/video/exynos/exynos_dp_core.c
>> >> @@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>> >>
>> >> dp->dev = &pdev->dev;
>> >>
>> >> - dp->clock = clk_get(&pdev->dev, "dp");
>> >> + dp->clock = devm_clk_get(&pdev->dev, "dp");
>> >> if (IS_ERR(dp->clock)) {
>> >> dev_err(&pdev->dev, "failed to get clock\n");
>> >> return PTR_ERR(dp->clock);
>> >> @@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>> >> clk_enable(dp->clock);
>> >>
>> >> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> >> - if (!res) {
>> >> - dev_err(&pdev->dev, "failed to get registers\n");
>> >> - ret = -EINVAL;
>> >> - goto err_clock;
>> >> - }
>> >
>> > Why do you remove this return check?
>> > If there is no reason, please, do it as follows:
>> >
>> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> > if (!res) {
>> > dev_err(&pdev->dev, "failed to get registers\n");
>> > - ret = -EINVAL;
>> > - goto err_clock;
>> > + return -EINVAL;
>> > }
>> >
>> >
>>
>> devm_request_and_ioremap function checks the validity of res. Hence
>> this check above is redundant and can be removed.
>
>
> I don't think so.
> Even though function called next checks the NULL value,
> for robustness, the return value of platform_get_resource() should be
> checked.
>
> It is possible that devm_request_and_ioremap() can be changed in the future,
> as request_mem_region() & ioremap() were changed to devm_request_and_ioremap().
They are not changed. They still exist. devm_request_and_ioremap() is
an additional function provided for device managed resources.
>
>
> Best regards,
> Jingoo Han
>
>
>>
>> Damien,
>> This patch only adds devm_clk_get() function. Hence you could make the
>> subject line more specific.
>>
>>
>>
>>
>> > Best regards,
>> > Jingoo Han
>> >
>> >
>> >>
>> >> dp->reg_base = devm_request_and_ioremap(&pdev->dev, res);
>> >> if (!dp->reg_base) {
>> >> dev_err(&pdev->dev, "failed to ioremap\n");
>> >> - ret = -ENOMEM;
>> >> - goto err_clock;
>> >> + return -ENOMEM;
>> >> }
>> >>
>> >> dp->irq = platform_get_irq(pdev, 0);
>> >> if (!dp->irq) {
>> >> dev_err(&pdev->dev, "failed to get irq\n");
>> >> - ret = -ENODEV;
>> >> - goto err_clock;
>> >> + return -ENODEV;
>> >> }
>> >>
>> >> ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0,
>> >> "exynos-dp", dp);
>> >> if (ret) {
>> >> dev_err(&pdev->dev, "failed to request irq\n");
>> >> - goto err_clock;
>> >> + return ret;
>> >> }
>> >>
>> >> dp->video_info = pdata->video_info;
>> >> @@ -917,7 +910,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>> >> ret = exynos_dp_detect_hpd(dp);
>> >> if (ret) {
>> >> dev_err(&pdev->dev, "unable to detect hpd\n");
>> >> - goto err_clock;
>> >> + return ret;
>> >> }
>> >>
>> >> exynos_dp_handle_edid(dp);
>> >> @@ -926,7 +919,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>> >> dp->video_info->link_rate);
>> >> if (ret) {
>> >> dev_err(&pdev->dev, "unable to do link train\n");
>> >> - goto err_clock;
>> >> + return ret;
>> >> }
>> >>
>> >> exynos_dp_enable_scramble(dp, 1);
>> >> @@ -940,17 +933,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>> >> ret = exynos_dp_config_video(dp, dp->video_info);
>> >> if (ret) {
>> >> dev_err(&pdev->dev, "unable to config video\n");
>> >> - goto err_clock;
>> >> + return ret;
>> >> }
>> >>
>> >> platform_set_drvdata(pdev, dp);
>> >>
>> >> return 0;
>> >> -
>> >> -err_clock:
>> >> - clk_put(dp->clock);
>> >> -
>> >> - return ret;
>> >> }
>> >>
>> >> static int __devexit exynos_dp_remove(struct platform_device *pdev)
>> >> @@ -962,7 +950,6 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
>> >> pdata->phy_exit();
>> >>
>> >> clk_disable(dp->clock);
>> >> - clk_put(dp->clock);
>> >>
>> >> return 0;
>> >> }
>> >
>> > --
>> > 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/
>>
>>
>>
>> --
>> With warm regards,
>> Sachin
>
--
With warm regards,
Sachin
^ permalink raw reply [flat|nested] 20+ messages in thread* RE: [PATCH 5/5] drivers/video/exynos/exynos_dp_core.c: use devm_ functions
2012-08-01 4:38 ` Sachin Kamat
@ 2012-08-01 4:57 ` Jingoo Han
2012-08-01 5:13 ` Julia Lawall
0 siblings, 1 reply; 20+ messages in thread
From: Jingoo Han @ 2012-08-01 4:57 UTC (permalink / raw)
To: 'Sachin Kamat'
Cc: 'Damien Cassou', kernel-janitors,
'Florian Tobias Schandinat', linux-fbdev, linux-kernel,
'Jingoo Han'
On Wednesday, August 01, 2012 1:38 PM Sachin Kamat wrote:
>
> On 1 August 2012 10:00, Jingoo Han <jg1.han@samsung.com> wrote:
> > On Wednesday, August 01, 2012 1:00 PMSachin Kamat wrote:
> >>
> >> On 1 August 2012 04:51, Jingoo Han <jg1.han@samsung.com> wrote:
> >> > On Wednesday, August 01, 2012 1:39 AM Damien Cassou wrote:
> >> >>
> >> >> From: Damien Cassou <damien.cassou@lifl.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.
> >> >>
> >> >> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
> >> >>
> >> >> ---
> >> >> drivers/video/exynos/exynos_dp_core.c | 27 +++++++--------------------
> >> >> 1 file changed, 7 insertions(+), 20 deletions(-)
> >> >>
> >> >> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> >> >> index c6c016a..00fe4f0 100644
> >> >> --- a/drivers/video/exynos/exynos_dp_core.c
> >> >> +++ b/drivers/video/exynos/exynos_dp_core.c
> >> >> @@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> >>
> >> >> dp->dev = &pdev->dev;
> >> >>
> >> >> - dp->clock = clk_get(&pdev->dev, "dp");
> >> >> + dp->clock = devm_clk_get(&pdev->dev, "dp");
> >> >> if (IS_ERR(dp->clock)) {
> >> >> dev_err(&pdev->dev, "failed to get clock\n");
> >> >> return PTR_ERR(dp->clock);
> >> >> @@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> >> clk_enable(dp->clock);
> >> >>
> >> >> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> >> - if (!res) {
> >> >> - dev_err(&pdev->dev, "failed to get registers\n");
> >> >> - ret = -EINVAL;
> >> >> - goto err_clock;
> >> >> - }
> >> >
> >> > Why do you remove this return check?
> >> > If there is no reason, please, do it as follows:
> >> >
> >> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> > if (!res) {
> >> > dev_err(&pdev->dev, "failed to get registers\n");
> >> > - ret = -EINVAL;
> >> > - goto err_clock;
> >> > + return -EINVAL;
> >> > }
> >> >
> >> >
> >>
> >> devm_request_and_ioremap function checks the validity of res. Hence
> >> this check above is redundant and can be removed.
> >
> >
> > I don't think so.
> > Even though function called next checks the NULL value,
> > for robustness, the return value of platform_get_resource() should be
> > checked.
> >
> > It is possible that devm_request_and_ioremap() can be changed in the future,
> > as request_mem_region() & ioremap() were changed to devm_request_and_ioremap().
>
> They are not changed. They still exist. devm_request_and_ioremap() is
> an additional function provided for device managed resources.
OK, I see. I accept it.
Anyway it is simpler.
>
>
> >
> >
> > Best regards,
> > Jingoo Han
> >
> >
> >>
> >> Damien,
> >> This patch only adds devm_clk_get() function. Hence you could make the
> >> subject line more specific.
> >>
> >>
> >>
> >>
> >> > Best regards,
> >> > Jingoo Han
> >> >
> >> >
> >> >>
> >> >> dp->reg_base = devm_request_and_ioremap(&pdev->dev, res);
> >> >> if (!dp->reg_base) {
> >> >> dev_err(&pdev->dev, "failed to ioremap\n");
> >> >> - ret = -ENOMEM;
> >> >> - goto err_clock;
> >> >> + return -ENOMEM;
> >> >> }
> >> >>
> >> >> dp->irq = platform_get_irq(pdev, 0);
> >> >> if (!dp->irq) {
> >> >> dev_err(&pdev->dev, "failed to get irq\n");
> >> >> - ret = -ENODEV;
> >> >> - goto err_clock;
> >> >> + return -ENODEV;
> >> >> }
> >> >>
> >> >> ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0,
> >> >> "exynos-dp", dp);
> >> >> if (ret) {
> >> >> dev_err(&pdev->dev, "failed to request irq\n");
> >> >> - goto err_clock;
> >> >> + return ret;
> >> >> }
> >> >>
> >> >> dp->video_info = pdata->video_info;
> >> >> @@ -917,7 +910,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> >> ret = exynos_dp_detect_hpd(dp);
> >> >> if (ret) {
> >> >> dev_err(&pdev->dev, "unable to detect hpd\n");
> >> >> - goto err_clock;
> >> >> + return ret;
> >> >> }
> >> >>
> >> >> exynos_dp_handle_edid(dp);
> >> >> @@ -926,7 +919,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> >> dp->video_info->link_rate);
> >> >> if (ret) {
> >> >> dev_err(&pdev->dev, "unable to do link train\n");
> >> >> - goto err_clock;
> >> >> + return ret;
> >> >> }
> >> >>
> >> >> exynos_dp_enable_scramble(dp, 1);
> >> >> @@ -940,17 +933,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> >> ret = exynos_dp_config_video(dp, dp->video_info);
> >> >> if (ret) {
> >> >> dev_err(&pdev->dev, "unable to config video\n");
> >> >> - goto err_clock;
> >> >> + return ret;
> >> >> }
> >> >>
> >> >> platform_set_drvdata(pdev, dp);
> >> >>
> >> >> return 0;
> >> >> -
> >> >> -err_clock:
> >> >> - clk_put(dp->clock);
> >> >> -
> >> >> - return ret;
> >> >> }
> >> >>
> >> >> static int __devexit exynos_dp_remove(struct platform_device *pdev)
> >> >> @@ -962,7 +950,6 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
> >> >> pdata->phy_exit();
> >> >>
> >> >> clk_disable(dp->clock);
> >> >> - clk_put(dp->clock);
> >> >>
> >> >> return 0;
> >> >> }
> >> >
> >> > --
> >> > 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/
> >>
> >>
> >>
> >> --
> >> With warm regards,
> >> Sachin
> >
>
>
>
> --
> With warm regards,
> Sachin
^ permalink raw reply [flat|nested] 20+ messages in thread* RE: [PATCH 5/5] drivers/video/exynos/exynos_dp_core.c: use devm_ functions
2012-08-01 4:57 ` Jingoo Han
@ 2012-08-01 5:13 ` Julia Lawall
0 siblings, 0 replies; 20+ messages in thread
From: Julia Lawall @ 2012-08-01 5:13 UTC (permalink / raw)
To: Jingoo Han
Cc: 'Sachin Kamat', 'Damien Cassou', kernel-janitors,
'Florian Tobias Schandinat', linux-fbdev, linux-kernel
On Wed, 1 Aug 2012, Jingoo Han wrote:
> On Wednesday, August 01, 2012 1:38 PM Sachin Kamat wrote:
>>
>> On 1 August 2012 10:00, Jingoo Han <jg1.han@samsung.com> wrote:
>>> On Wednesday, August 01, 2012 1:00 PMSachin Kamat wrote:
>>>>
>>>> On 1 August 2012 04:51, Jingoo Han <jg1.han@samsung.com> wrote:
>>>>> On Wednesday, August 01, 2012 1:39 AM Damien Cassou wrote:
>>>>>>
>>>>>> From: Damien Cassou <damien.cassou@lifl.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.
>>>>>>
>>>>>> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
>>>>>>
>>>>>> ---
>>>>>> drivers/video/exynos/exynos_dp_core.c | 27 +++++++--------------------
>>>>>> 1 file changed, 7 insertions(+), 20 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
>>>>>> index c6c016a..00fe4f0 100644
>>>>>> --- a/drivers/video/exynos/exynos_dp_core.c
>>>>>> +++ b/drivers/video/exynos/exynos_dp_core.c
>>>>>> @@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>>>>>>
>>>>>> dp->dev = &pdev->dev;
>>>>>>
>>>>>> - dp->clock = clk_get(&pdev->dev, "dp");
>>>>>> + dp->clock = devm_clk_get(&pdev->dev, "dp");
>>>>>> if (IS_ERR(dp->clock)) {
>>>>>> dev_err(&pdev->dev, "failed to get clock\n");
>>>>>> return PTR_ERR(dp->clock);
>>>>>> @@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>>>>>> clk_enable(dp->clock);
>>>>>>
>>>>>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>>> - if (!res) {
>>>>>> - dev_err(&pdev->dev, "failed to get registers\n");
>>>>>> - ret = -EINVAL;
>>>>>> - goto err_clock;
>>>>>> - }
>>>>>
>>>>> Why do you remove this return check?
>>>>> If there is no reason, please, do it as follows:
>>>>>
>>>>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>> if (!res) {
>>>>> dev_err(&pdev->dev, "failed to get registers\n");
>>>>> - ret = -EINVAL;
>>>>> - goto err_clock;
>>>>> + return -EINVAL;
>>>>> }
>>>>>
>>>>>
>>>>
>>>> devm_request_and_ioremap function checks the validity of res. Hence
>>>> this check above is redundant and can be removed.
>>>
>>>
>>> I don't think so.
>>> Even though function called next checks the NULL value,
>>> for robustness, the return value of platform_get_resource() should be
>>> checked.
>>>
>>> It is possible that devm_request_and_ioremap() can be changed in the future,
>>> as request_mem_region() & ioremap() were changed to devm_request_and_ioremap().
>>
>> They are not changed. They still exist. devm_request_and_ioremap() is
>> an additional function provided for device managed resources.
>
>
> OK, I see. I accept it.
> Anyway it is simpler.
This thread contains a discussion about the issue
http://lkml.org/lkml/2012/1/28/10
Look for the comments by Wolfram Sang, who
implemented devm_request_and_ioremap, and who suggests that the NULL test
be removed.
I rather agree with the desire to be safe and uniform, but these
initialization functions are really large, and with error handling code
(although not in this case) there is always the danger of jumping to the
wrong place, and thus making more of a mess. It would be nice if the
platform_get_resource could be merged with devm_request_and_ioremap, but I
think that I looked once and there were not enough calls that were similar
enough to make that compelling.
julia
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH 5/5] drivers/video/exynos/exynos_dp_core.c: use devm_ functions
2012-08-01 3:59 ` Sachin Kamat
2012-08-01 4:30 ` Jingoo Han
@ 2012-08-01 5:08 ` Jingoo Han
2012-08-01 16:36 ` Damien Cassou
1 sibling, 1 reply; 20+ messages in thread
From: Jingoo Han @ 2012-08-01 5:08 UTC (permalink / raw)
To: 'Damien Cassou', 'Sachin Kamat'
Cc: kernel-janitors, 'Florian Tobias Schandinat', linux-fbdev,
linux-kernel, 'Jingoo Han'
On Wednesday, August 01, 2012 1:00 PM Sachin Kamat wrote:
>
> On 1 August 2012 04:51, Jingoo Han <jg1.han@samsung.com> wrote:
> > On Wednesday, August 01, 2012 1:39 AM Damien Cassou wrote:
> >>
> >> From: Damien Cassou <damien.cassou@lifl.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.
> >>
> >> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
> >>
> >> ---
> >> drivers/video/exynos/exynos_dp_core.c | 27 +++++++--------------------
> >> 1 file changed, 7 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> >> index c6c016a..00fe4f0 100644
> >> --- a/drivers/video/exynos/exynos_dp_core.c
> >> +++ b/drivers/video/exynos/exynos_dp_core.c
> >> @@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >>
> >> dp->dev = &pdev->dev;
> >>
> >> - dp->clock = clk_get(&pdev->dev, "dp");
> >> + dp->clock = devm_clk_get(&pdev->dev, "dp");
> >> if (IS_ERR(dp->clock)) {
> >> dev_err(&pdev->dev, "failed to get clock\n");
> >> return PTR_ERR(dp->clock);
> >> @@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> clk_enable(dp->clock);
> >>
> >> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> - if (!res) {
> >> - dev_err(&pdev->dev, "failed to get registers\n");
> >> - ret = -EINVAL;
> >> - goto err_clock;
> >> - }
> >
> > Why do you remove this return check?
> > If there is no reason, please, do it as follows:
> >
> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > if (!res) {
> > dev_err(&pdev->dev, "failed to get registers\n");
> > - ret = -EINVAL;
> > - goto err_clock;
> > + return -EINVAL;
> > }
> >
> >
>
> devm_request_and_ioremap function checks the validity of res. Hence
> this check above is redundant and can be removed.
>
> Damien,
> This patch only adds devm_clk_get() function. Hence you could make the
> subject line more specific.
To Damien,
As Sachin Kamat mentioned, please change the subject more specific. For example,
video: exynos_dp: use devm_clk_get function
Best regards,
Jingoo Han
>
>
>
>
> > Best regards,
> > Jingoo Han
> >
> >
> >>
> >> dp->reg_base = devm_request_and_ioremap(&pdev->dev, res);
> >> if (!dp->reg_base) {
> >> dev_err(&pdev->dev, "failed to ioremap\n");
> >> - ret = -ENOMEM;
> >> - goto err_clock;
> >> + return -ENOMEM;
> >> }
> >>
> >> dp->irq = platform_get_irq(pdev, 0);
> >> if (!dp->irq) {
> >> dev_err(&pdev->dev, "failed to get irq\n");
> >> - ret = -ENODEV;
> >> - goto err_clock;
> >> + return -ENODEV;
> >> }
> >>
> >> ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0,
> >> "exynos-dp", dp);
> >> if (ret) {
> >> dev_err(&pdev->dev, "failed to request irq\n");
> >> - goto err_clock;
> >> + return ret;
> >> }
> >>
> >> dp->video_info = pdata->video_info;
> >> @@ -917,7 +910,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> ret = exynos_dp_detect_hpd(dp);
> >> if (ret) {
> >> dev_err(&pdev->dev, "unable to detect hpd\n");
> >> - goto err_clock;
> >> + return ret;
> >> }
> >>
> >> exynos_dp_handle_edid(dp);
> >> @@ -926,7 +919,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> dp->video_info->link_rate);
> >> if (ret) {
> >> dev_err(&pdev->dev, "unable to do link train\n");
> >> - goto err_clock;
> >> + return ret;
> >> }
> >>
> >> exynos_dp_enable_scramble(dp, 1);
> >> @@ -940,17 +933,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> ret = exynos_dp_config_video(dp, dp->video_info);
> >> if (ret) {
> >> dev_err(&pdev->dev, "unable to config video\n");
> >> - goto err_clock;
> >> + return ret;
> >> }
> >>
> >> platform_set_drvdata(pdev, dp);
> >>
> >> return 0;
> >> -
> >> -err_clock:
> >> - clk_put(dp->clock);
> >> -
> >> - return ret;
> >> }
> >>
> >> static int __devexit exynos_dp_remove(struct platform_device *pdev)
> >> @@ -962,7 +950,6 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
> >> pdata->phy_exit();
> >>
> >> clk_disable(dp->clock);
> >> - clk_put(dp->clock);
> >>
> >> return 0;
> >> }
> >
> > --
> > 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/
>
>
>
> --
> With warm regards,
> Sachin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 5/5] drivers/video/exynos/exynos_dp_core.c: use devm_ functions
2012-08-01 5:08 ` Jingoo Han
@ 2012-08-01 16:36 ` Damien Cassou
0 siblings, 0 replies; 20+ messages in thread
From: Damien Cassou @ 2012-08-01 16:36 UTC (permalink / raw)
To: Jingoo Han
Cc: Sachin Kamat, kernel-janitors, Florian Tobias Schandinat,
linux-fbdev, linux-kernel
On Wed, Aug 1, 2012 at 7:08 AM, Jingoo Han <jg1.han@samsung.com> wrote:
> To Damien,
> As Sachin Kamat mentioned, please change the subject more specific. For example,
>
> video: exynos_dp: use devm_clk_get function
ok, done in a new thread titled:
[PATCH v2] video: exynos_dp: use devm_clk_get function
Thank you for the feedback
--
Damien Cassou
http://damiencassou.seasidehosting.st
"Lambdas are relegated to relative obscurity until Java makes them
popular by not having them." James Iry
^ permalink raw reply [flat|nested] 20+ messages in thread