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: writeback: Refactor drm_writeback_connector structure
Date: Mon, 25 May 2026 19:19:14 +1000
Message-ID: <review-patch1-20260522050313.1800378-2-suraj.kandpal@intel.com>
In-Reply-To: <20260522050313.1800378-2-suraj.kandpal@intel.com>
References: <20260522050313.1800378-1-suraj.kandpal@intel.com> <20260522050313.1800378-2-suraj.kandpal@intel.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

This is the main structural change. It moves `drm_writeback_connector` into `drm_connector` as a union with `hdmi`, removes the `base` (drm_connector) member from `drm_writeback_connector`, and updates all drivers.

**Issues:**

1. **Typo in kerneldoc** at the new union member:
```c
+		/**
+		 * @writeback: Writeback related valriables.
+		 */
```
`valriables` should be `variables`.

2. **`encoder` field dropped without comment.** The old `drm_writeback_connector` had:
```c
struct drm_encoder encoder;
```
This is silently removed. The cover letter says this sits on top of Dmitry's series which likely removes the old-style `drm_writeback_connector_init()` that uses this encoder, but the diffstat shows `drm_writeback_connector_init` still exists and is modified. If the encoder field is expected to be gone, the series should either explicitly state this dependency or include the encoder removal.

3. **`drm_writeback_to_connector()` replaces `drm_connector_to_writeback()`** -- the direction is reversed, which is the whole point. The helper is well-implemented using `container_of` on the union member:
```c
+static inline struct drm_connector *
+drm_writeback_to_connector(struct drm_writeback_connector *wb_connector)
+{
+	return container_of(wb_connector, struct drm_connector, writeback);
+}
```
This is correct.

4. **amdgpu `dm_set_writeback()`** changes `drm_connector_to_writeback(connector)` to `&connector->writeback`:
```c
-	struct drm_writeback_connector *wb_conn = drm_connector_to_writeback(connector);
+	struct drm_writeback_connector *wb_conn = &connector->writeback;
```
This is fine and consistent. The `wb_conn` is still needed later in the function body.

5. **malidp_hw.c changes** -- these touch `drm_writeback_signal_completion` calls, but patch 4 later changes the signature of `drm_writeback_signal_completion` from taking `drm_writeback_connector*` to `drm_connector*`. Patch 1 changes the callers to pass `&malidp->mw_connector.writeback`, then patch 4 changes them back to `&malidp->mw_connector`. However, looking at the patch 4 diff, the old_string in patch 4 is `&malidp->mw_connector.writeback` matching what patch 1 produces. The diff in patch 4 shows the malidp_hw.c file reverting to the *original* source hash (`9b845d3f34e1`), which is suspicious -- likely a rebase artifact. The final result after all patches should be correct.

6. **vkms_writeback.c `vkms_wb_cleanup_job()`** -- the intermediate form uses `container_of` manually:
```c
+	struct drm_connector *connector = container_of(wb_conn,
+						       struct drm_connector,
+						       writeback);
```
This is cleaned up in patch 7 when the callback signature changes.

7. The driver struct changes (e.g., `struct drm_writeback_connector base` -> `struct drm_connector base`) are all mechanically correct for AMD, komeda, malidp, vc4, vkms, rcar-du, and MSM.

---
Generated by Claude Code Patch Reviewer