[PATCH] fbdev: goldfishfb: use devm_platform_ioremap_resource()

[PATCH] fbdev: goldfishfb: use devm_platform_ioremap_resource()

[Amin GATTOUT]

Replace the open-coded platform_get_resource() + ioremap() pair with
devm_platform_ioremap_resource(), which requests the memory region and
maps it in a single call, with automatic cleanup on device removal.

Signed-off-by: Amin GATTOUT <amin.gattout@gmail.com>
---
 drivers/video/fbdev/goldfishfb.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/video/fbdev/goldfishfb.c b/drivers/video/fbdev/goldfishfb.c
index ffe33a36b944..c9871281bc1d 100644
--- a/drivers/video/fbdev/goldfishfb.c
+++ b/drivers/video/fbdev/goldfishfb.c
@@ -174,7 +174,6 @@ static const struct fb_ops goldfish_fb_ops = {
 static int goldfish_fb_probe(struct platform_device *pdev)
 {
 	int ret;
-	struct resource *r;
 	struct goldfish_fb *fb;
 	size_t framesize;
 	u32 width, height;
@@ -189,14 +188,9 @@ static int goldfish_fb_probe(struct platform_device *pdev)
 	init_waitqueue_head(&fb->wait);
 	platform_set_drvdata(pdev, fb);
 
-	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (r == NULL) {
-		ret = -ENODEV;
-		goto err_no_io_base;
-	}
-	fb->reg_base = ioremap(r->start, PAGE_SIZE);
-	if (fb->reg_base == NULL) {
-		ret = -ENOMEM;
+	fb->reg_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(fb->reg_base)) {
+		ret = PTR_ERR(fb->reg_base);
 		goto err_no_io_base;
 	}
 
@@ -273,7 +267,6 @@ static int goldfish_fb_probe(struct platform_device *pdev)
 				fb->fb.fix.smem_start);
 err_alloc_screen_base_failed:
 err_no_irq:
-	iounmap(fb->reg_base);
 err_no_io_base:
 	kfree(fb);
 err_fb_alloc_failed:
@@ -291,7 +284,6 @@ static void goldfish_fb_remove(struct platform_device *pdev)
 
 	dma_free_coherent(&pdev->dev, framesize, (void *)fb->fb.screen_base,
 						fb->fb.fix.smem_start);
-	iounmap(fb->reg_base);
 	kfree(fb);
 }
 

---
base-commit: 11439c4635edd669ae435eec308f4ab8a0804808
change-id: 20260303-master-341e700a2699

Best regards,
-- 
Amin GATTOUT <amin.gattout@gmail.com>

Re: [PATCH] fbdev: goldfishfb: use devm_platform_ioremap_resource()

[Thomas Zimmermann]

Am 03.03.26 um 13:22 schrieb Amin GATTOUT:
> Replace the open-coded platform_get_resource() + ioremap() pair with
> devm_platform_ioremap_resource(), which requests the memory region and
> maps it in a single call, with automatic cleanup on device removal.
>
> Signed-off-by: Amin GATTOUT <amin.gattout@gmail.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/video/fbdev/goldfishfb.c | 14 +++-----------
>   1 file changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/video/fbdev/goldfishfb.c b/drivers/video/fbdev/goldfishfb.c
> index ffe33a36b944..c9871281bc1d 100644
> --- a/drivers/video/fbdev/goldfishfb.c
> +++ b/drivers/video/fbdev/goldfishfb.c
> @@ -174,7 +174,6 @@ static const struct fb_ops goldfish_fb_ops = {
>   static int goldfish_fb_probe(struct platform_device *pdev)
>   {
>   	int ret;
> -	struct resource *r;
>   	struct goldfish_fb *fb;
>   	size_t framesize;
>   	u32 width, height;
> @@ -189,14 +188,9 @@ static int goldfish_fb_probe(struct platform_device *pdev)
>   	init_waitqueue_head(&fb->wait);
>   	platform_set_drvdata(pdev, fb);
>   
> -	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (r == NULL) {
> -		ret = -ENODEV;
> -		goto err_no_io_base;
> -	}
> -	fb->reg_base = ioremap(r->start, PAGE_SIZE);
> -	if (fb->reg_base == NULL) {
> -		ret = -ENOMEM;
> +	fb->reg_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(fb->reg_base)) {
> +		ret = PTR_ERR(fb->reg_base);
>   		goto err_no_io_base;
>   	}
>   
> @@ -273,7 +267,6 @@ static int goldfish_fb_probe(struct platform_device *pdev)
>   				fb->fb.fix.smem_start);
>   err_alloc_screen_base_failed:
>   err_no_irq:
> -	iounmap(fb->reg_base);
>   err_no_io_base:
>   	kfree(fb);
>   err_fb_alloc_failed:
> @@ -291,7 +284,6 @@ static void goldfish_fb_remove(struct platform_device *pdev)
>   
>   	dma_free_coherent(&pdev->dev, framesize, (void *)fb->fb.screen_base,
>   						fb->fb.fix.smem_start);
> -	iounmap(fb->reg_base);
>   	kfree(fb);
>   }
>   
>
> ---
> base-commit: 11439c4635edd669ae435eec308f4ab8a0804808
> change-id: 20260303-master-341e700a2699
>
> Best regards,

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

Re: [PATCH] fbdev: goldfishfb: use devm_platform_ioremap_resource()

[Helge Deller]

On 3/3/26 13:51, Thomas Zimmermann wrote:
> Am 03.03.26 um 13:22 schrieb Amin GATTOUT:
>> Replace the open-coded platform_get_resource() + ioremap() pair with
>> devm_platform_ioremap_resource(), which requests the memory region and
>> maps it in a single call, with automatic cleanup on device removal.
>>
>> Signed-off-by: Amin GATTOUT <amin.gattout@gmail.com>
> 
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
>> ---
>>   drivers/video/fbdev/goldfishfb.c | 14 +++-----------
>>   1 file changed, 3 insertions(+), 11 deletions(-)

applied to fbdev git tree.
Thanks!
Helge

Claude review: fbdev: goldfishfb: use devm_platform_ioremap_resource()

[Claude Code Review Bot]

Overall Series Review

Subject: fbdev: goldfishfb: use devm_platform_ioremap_resource()
Author: Amin GATTOUT <amin.gattout@gmail.com>
Patches: 3
Reviewed: 2026-03-04T07:21:04.586574

---

This is a single-patch series performing a standard kernel cleanup: replacing an open-coded `platform_get_resource()` + `ioremap()` pair with `devm_platform_ioremap_resource()` in the goldfish framebuffer driver, and removing the corresponding manual `iounmap()` calls. The patch is straightforward and largely correct, but has a couple of subtle behavioral changes worth noting.

**Verdict: Mostly clean, minor notes below.**

---
Generated by Claude Code Patch Reviewer

Claude review: fbdev: goldfishfb: use devm_platform_ioremap_resource()

[Claude Code Review Bot]

Patch Review

**Correctness of the core transformation: OK**

The replacement of `platform_get_resource()` + `ioremap()` with `devm_platform_ioremap_resource()`, and the corresponding `IS_ERR()` / `PTR_ERR()` error check, is textbook correct:

```c
-	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (r == NULL) {
-		ret = -ENODEV;
-		goto err_no_io_base;
-	}
-	fb->reg_base = ioremap(r->start, PAGE_SIZE);
-	if (fb->reg_base == NULL) {
-		ret = -ENOMEM;
+	fb->reg_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(fb->reg_base)) {
+		ret = PTR_ERR(fb->reg_base);
 		goto err_no_io_base;
 	}
```

**Removal of `iounmap()` calls: OK**

Both the error-path and remove-path `iounmap()` removals are correct since the devm infrastructure handles cleanup automatically — on both probe failure and device removal:

```c
 err_no_irq:
-	iounmap(fb->reg_base);
 err_no_io_base:
```

```c
-	iounmap(fb->reg_base);
 	kfree(fb);
```

**Subtle behavioral change #1 — mapping size**: The original code maps exactly `PAGE_SIZE`:
```c
fb->reg_base = ioremap(r->start, PAGE_SIZE);
```
`devm_platform_ioremap_resource()` maps `resource_size(r)`, i.e. the full resource region as described in the device tree / platform data. Since the highest register offset used is `FB_GET_PHYS_HEIGHT = 0x20`, both PAGE_SIZE and any reasonably-defined resource region will cover this — so this is fine in practice, but it's a behavioral change worth being aware of.

**Subtle behavioral change #2 — `request_mem_region()` is now called**: The original code never calls `request_mem_region()`. `devm_platform_ioremap_resource()` internally calls `devm_request_mem_region()` before mapping. This is actually an improvement (the original was technically sloppy), but it could theoretically cause probe to fail if another driver has already claimed the region. For goldfish, this is extremely unlikely.

**Mixed devm / manual allocation**: The `fb` struct is still allocated with `kzalloc()` and freed with `kfree()`, while the ioremap is now devm-managed. This mixed pattern works correctly here — the devm unmap happens after `goldfish_fb_remove()` returns, but the devm infrastructure stores the mapped address in its own devres entry rather than reading it back from `fb->reg_base`, so the `kfree(fb)` preceding the devm cleanup is safe. However, a follow-up patch converting `kzalloc`/`kfree` to `devm_kzalloc` would make the lifecycle management more consistent.

**Overall**: The patch is correct and a net improvement. No blocking issues.

---
Generated by Claude Code Patch Reviewer