From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/imagination: Implement handling of context reset notification Date: Wed, 25 Mar 2026 07:34:16 +1000 Message-ID: In-Reply-To: <20260323-b4-firmware-context-reset-notification-handling-v3-3-1a66049a9a65@imgtec.com> References: <20260323-b4-firmware-context-reset-notification-handling-v3-0-1a66049a9a65@imgtec.com> <20260323-b4-firmware-context-reset-notification-handling-v3-3-1a66049a9a65@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 **Verdict: Acceptable with minor nits.** The implementation is clean and does what the commit message describes. A f= ew observations: 1. **Commit message typo**: "deecoding" should be "decoding": ``` part of handling hardware recovery (HWR) events deecoding the message ``` 2. **`get_reset_reason_desc` parameter type**: The function signature takes= `enum rogue_context_reset_reason reason`, but the caller casts a `u32` to = this enum: ```c get_reset_reason_desc((enum rogue_context_reset_reason)data->reset_reaso= n) ``` This is fine functionally and the `default` case handles unknown values,= but since patch 2 explicitly moved away from using the enum type in struct= ures (because "enum is not a fixed size"), it would be more consistent to h= ave `get_reset_reason_desc` take a `u32` parameter instead. This is a style= nit =E2=80=94 the generated code will be the same. 3. **`get_dm_name` comment about TDM**: The comment and return value are sl= ightly confusing: ```c /* PVR_FWIF_DM_TDM has the same index, but is discriminated by a device = feature */ case PVR_FWIF_DM_2D: return "2D or TDM"; ``` Since `PVR_FWIF_DM_TDM` and `PVR_FWIF_DM_2D` are both defined as `(1)` i= n `pvr_rogue_fwif_common.h`, you can't have separate `case` labels. Returni= ng `"2D or TDM"` is a reasonable compromise, but it could be improved by ch= ecking the device feature flag at runtime to return the correct name (since= `pvr_dev` is available to the caller). This is optional =E2=80=94 the curr= ent approach is acceptable for a diagnostic message. 4. **`pvr_dump_context_reset_notification` data parameter should be `const`= **: The function only reads from `data`, so the signature should be: ```c void pvr_dump_context_reset_notification(struct pvr_device *pvr_dev, const struct rogue_fwif_fwccb_cmd_context_reset_data *data); ``` (Both in the `.c` and `.h` files.) 5. **New file naming**: Creating `pvr_dump.c`/`.h` for a single function is= fine if more dump/diagnostic functions are expected to be added later (whi= ch seems likely given the generic naming). If this will remain the only fun= ction, it could arguably live in `pvr_ccb.c` directly. Not a blocking issue. 6. **Alignment in `pvr_ccb.c`**: The indentation of the continuation line u= ses spaces that don't align perfectly with the opening parenthesis =E2=80= =94 minor formatting nit: ```c pvr_dump_context_reset_notification(pvr_dev, &cmd->cmd_data.cmd_context_reset_notification); ``` The `&cmd` should align under `pvr_dev`. This is cosmetic. --- Generated by Claude Code Patch Reviewer