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: drm/imagination: Define packed BVNCs in the uapi
Date: Sun, 22 Mar 2026 03:38:20 +1000
Message-ID: <review-patch1-20260320-bvnc-cleanup-v2-1-49c3c1453289@imgtec.com>
In-Reply-To: <20260320-bvnc-cleanup-v2-1-49c3c1453289@imgtec.com>
References: <20260320-bvnc-cleanup-v2-0-49c3c1453289@imgtec.com> <20260320-bvnc-cleanup-v2-1-49c3c1453289@imgtec.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

**Overall:** Good cleanup. The approach of defining the bit layout once in =
the UAPI and referencing it everywhere is the right pattern.

**UAPI additions look correct:**
```c
#define DRM_PVR_BVNC_B __GENMASK_ULL(63, 48)
#define DRM_PVR_BVNC_V __GENMASK_ULL(47, 32)
#define DRM_PVR_BVNC_N __GENMASK_ULL(31, 16)
#define DRM_PVR_BVNC_C __GENMASK_ULL(15, 0)
```
These correctly match the previously documented layout. Using `__GENMASK_UL=
L` (the double-underscore UAPI-safe variant) is correct for a UAPI header, =
and the new `#include <linux/bits.h>` provides it.

**The `__PVR_PACKED_BVNC` helper pattern is clever:**
```c
#define __PVR_PACKED_BVNC(prep, b, v, n, c) \
	(prep(DRM_PVR_BVNC_B, b) | \
	 prep(DRM_PVR_BVNC_V, v) | \
	 prep(DRM_PVR_BVNC_N, n) | \
	 prep(DRM_PVR_BVNC_C, c))
```
Parameterizing on the prep function to support both `FIELD_PREP` (runtime) =
and `FIELD_PREP_CONST` (compile-time) avoids code duplication while address=
ing the v1 build issue where `FIELD_PREP` couldn't be used in `case` labels.

**Include change is correct:** Replacing `<linux/bits.h>` with `<linux/bitf=
ield.h>` in `pvr_device.h` is appropriate since `bitfield.h` pulls in `bits=
.h` transitively and `FIELD_PREP`/`FIELD_GET` are now used.

**`packed_bvnc_to_pvr_gpu_id()` conversion looks correct:**
```c
gpu_id->b =3D FIELD_GET(DRM_PVR_BVNC_B, bvnc);
```
This is equivalent to the previous `(bvnc & GENMASK_ULL(63, 48)) >> 48` and=
 is more readable.

**Minor nit:** The `__PVR_PACKED_BVNC` macro uses a leading double-undersco=
re name. While this is common for internal helper macros in the kernel, it'=
s technically in the compiler-reserved namespace. This is a style nit, not =
a blocking issue =E2=80=94 the kernel uses this convention extensively.

**No issues found.** Reviewed-by worthy.

---
Generated by Claude Code Patch Reviewer