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: Add MHDP8501 DP/HDMI driver Date: Mon, 25 May 2026 22:48:49 +1000 Message-ID: In-Reply-To: <20260519-dcss-hdmi-upstreaming-v23-6-5615524a9c63@oss.nxp.com> References: <20260519-dcss-hdmi-upstreaming-v23-0-5615524a9c63@oss.nxp.com> <20260519-dcss-hdmi-upstreaming-v23-6-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 This is the main new driver (3 source files, ~2000 lines). Overall well-str= uctured with proper use of DRM atomic helpers. **Issues:** 1. **IRQ vs. removal race**: The `removing` flag uses `WRITE_ONCE`/`READ_ON= CE` but this is not sufficient to prevent the IRQ handler from scheduling `= hotplug_work` between checking the flag and calling `disable_irq()` in remo= ve. The sequence should be: ```c // In remove(): WRITE_ONCE(mhdp->removing, true); disable_irq(mhdp->irq[IRQ_IN]); disable_irq(mhdp->irq[IRQ_OUT]); cancel_delayed_work_sync(&mhdp->hotplug_work); ``` rather than relying on `READ_ONCE` in the IRQ handler to prevent scheduling. 2. **HDMI I2C adapter limitation**: The adapter only supports SCDC slave (0= x54). While documented, the `i2c_algorithm` should return an error for unsu= pported addresses rather than silently failing. The code does check: ```c if (msgs[0].addr !=3D DDC_SEGMENT_ADDR && msgs[0].addr !=3D DDC_ADDR) ``` but returns `-EINVAL` without explanation. A `dev_dbg` message would help. 3. **`dev_set_drvdata(dev, &mhdp->base)`**: The bridge driver stores `&mhdp= ->base` (not `mhdp` itself) via `dev_set_drvdata` so the PHY child can find= it. This is clever but fragile =E2=80=94 any caller using `dev_get_drvdata= ` must know to expect a `cdns_mhdp_base*`, not the full device struct. A co= mment at the `dev_set_drvdata` call site would prevent confusion. 4. **Debugfs firmware version**: The firmware version is read at debugfs op= en time via mailbox. If the firmware is not loaded/ready, this will fail si= lently or block. Consider caching the version after firmware ready is confi= rmed. 5. **HDMI infoframe handling**: `cdns_hdmi_config_infoframe()` silently ret= urns if `len > 31` without reporting an error. The caller should know the c= onfiguration failed. --- Generated by Claude Code Patch Reviewer