From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: fbdev: goldfishfb: use devm_platform_ioremap_resource()
Date: Wed, 04 Mar 2026 07:21:04 +1000
Message-ID: <review-patch1-20260303-master-v1-1-85eb5ff584b0@gmail.com>
In-Reply-To: <20260303-master-v1-1-85eb5ff584b0@gmail.com>
References: <20260303-master-v1-1-85eb5ff584b0@gmail.com> <20260303-master-v1-1-85eb5ff584b0@gmail.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Correctness of the core transformation: OK**

The replacement of `platform_get_resource()` + `ioremap()` with `devm_platf=
orm_ioremap_resource()`, and the corresponding `IS_ERR()` / `PTR_ERR()` err=
or check, is textbook correct:

```c
-	r =3D platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (r =3D=3D NULL) {
-		ret =3D -ENODEV;
-		goto err_no_io_base;
-	}
-	fb->reg_base =3D ioremap(r->start, PAGE_SIZE);
-	if (fb->reg_base =3D=3D NULL) {
-		ret =3D -ENOMEM;
+	fb->reg_base =3D devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(fb->reg_base)) {
+		ret =3D 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 =E2=80=94 on both pro=
be 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 =E2=80=94 mapping size**: The original code m=
aps exactly `PAGE_SIZE`:
```c
fb->reg_base =3D ioremap(r->start, PAGE_SIZE);
```
`devm_platform_ioremap_resource()` maps `resource_size(r)`, i.e. the full r=
esource region as described in the device tree / platform data. Since the h=
ighest register offset used is `FB_GET_PHYS_HEIGHT =3D 0x20`, both PAGE_SIZ=
E and any reasonably-defined resource region will cover this =E2=80=94 so t=
his is fine in practice, but it's a behavioral change worth being aware of.

**Subtle behavioral change #2 =E2=80=94 `request_mem_region()` is now calle=
d**: The original code never calls `request_mem_region()`. `devm_platform_i=
oremap_resource()` internally calls `devm_request_mem_region()` before mapp=
ing. This is actually an improvement (the original was technically sloppy),=
 but it could theoretically cause probe to fail if another driver has alrea=
dy 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-manage=
d. This mixed pattern works correctly here =E2=80=94 the devm unmap happens=
 after `goldfish_fb_remove()` returns, but the devm infrastructure stores t=
he 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. Howe=
ver, 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