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: bridge: cadence: Update mhdp8546 mailbox access functions Date: Mon, 25 May 2026 22:48:48 +1000 Message-ID: In-Reply-To: <20260519-dcss-hdmi-upstreaming-v23-2-5615524a9c63@oss.nxp.com> References: <20260519-dcss-hdmi-upstreaming-v23-0-5615524a9c63@oss.nxp.com> <20260519-dcss-hdmi-upstreaming-v23-2-5615524a9c63@oss.nxp.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 Mechanical conversion of the existing mhdp8546 driver to use the new shared= helpers. Well done =E2=80=94 removes ~550 lines of duplicated code. **Issues:** 1. **Data duplication in `cdns_mhdp_device`**: The patch adds `struct cdns_= mhdp_base base` which contains `regs`, `sapb_regs`, and `dev` pointers, but= the existing struct already has these same fields. In probe: ```c mhdp->base.dev =3D mhdp->dev; mhdp->base.regs =3D mhdp->regs; mhdp->base.sapb_regs =3D mhdp->sapb_regs; ``` This creates two copies of the same pointers. Consider embedding `base` at = the start of the struct and removing the duplicate fields, or at least maki= ng the existing fields point into `base`. 2. **`set_firmware_active` response handling changed**: The old code sent a= raw 5-byte message and read back a response. The new code uses `cdns_mhdp_= mailbox_send_recv()` with `sizeof(status)` (1 byte) for both message and re= sponse sizes. The old message was 5 bytes (including header). Make sure the= firmware protocol is actually `send(1 byte) =E2=86=92 recv(1 byte)` and no= t `send(5 bytes including header)`. 3. **`disable_irq`/cancel work ordering in remove()**: The patch adds: ```c disable_irq(mhdp->irq); cancel_work_sync(&mhdp->modeset_retry_work); flush_work(&mhdp->hpd_work); ``` before `wait_event_timeout`. The original code did `cancel_work_sync` + `fl= ush_work` after `pm_runtime_put_sync`. Moving `disable_irq` to the top of r= emove is correct for preventing new work from being scheduled, but the `drm= _bridge_remove()` call above it may trigger disconnect callbacks that refer= ence the device =E2=80=94 ensure there's no ordering issue with the bridge = still being live when the IRQ is disabled. 4. **`mutex_destroy` without corresponding `mutex_init` movement**: `mutex_= init(&mhdp->base.mailbox_mutex)` is in probe, and `mutex_destroy(&mhdp->bas= e.mailbox_mutex)` is in remove. But the old `mutex_init(&mhdp->mbox_mutex)`= is removed without adding the destroy. This is correct since the new mutex= is the replacement, but the asymmetric placement (init in probe, destroy a= t end of remove after pm_runtime_disable) is fine. --- Generated by Claude Code Patch Reviewer