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/omap: dsi: avoid sending bta sync all the time in writes Date: Thu, 04 Jun 2026 16:00:55 +1000 Message-ID: In-Reply-To: <20260529-vm-upstr-v2-1-24c30671719f@kernel.org> References: <20260529-vm-upstr-v2-1-24c30671719f@kernel.org> <20260529-vm-upstr-v2-1-24c30671719f@kernel.org> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Positive aspects:** - The rationale is clear and the problem is well-explained: TC358762 needs = configuration before it can transmit, so BTA sync during early writes fails. - Waiting for `DSI_VC_IRQ_PACKET_SENT` on long packets is the right approac= h to prevent FIFO overflows without requiring a round-trip BTA. - The `_dsi_send_nop` removal is correct =E2=80=94 it was only called from = `dsi_update_channel` and its real purpose was the BTA sync side-effect. Rep= lacing it with a direct `dsi_vc_send_bta_sync()` is cleaner and more explic= it. - The conditional BTA based on `MIPI_DSI_MSG_REQ_ACK` is the right abstract= ion. **Issues:** 1. **Typo in TODO comment** (line 2225 of patched file): ```c /* TODO: find out if more needs to be done for MIPI_DIS_MSG_REQ_ACK */ ``` Should be `MIPI_DSI_MSG_REQ_ACK` (not `MIPI_DIS_MSG_REQ_ACK`). This is a= transposition of "DSI" =E2=86=92 "DIS". 2. **Race between PACKET_SENT IRQ and ISR registration** (lines 2204=E2=80= =932213): `dsi_vc_send_long()` is called *before* `dsi_register_isr_vc()`. = If the long packet completes extremely quickly (or the hardware is fast), t= he PACKET_SENT IRQ could fire before the ISR is registered, and the complet= ion would never be signalled =E2=80=94 resulting in a 500ms timeout and spu= rious `-EIO`. The existing pattern in `dsi_if_enable` (and the BTA sync cod= e in `dsi_vc_send_bta_sync`) registers the ISR *before* triggering the hard= ware action. The correct order should be: ```c r =3D dsi_register_isr_vc(dsi, vc, dsi_completion_handler, &completion, DSI_VC_IRQ_PACKET_SENT); if (r) return r; r =3D dsi_vc_send_long(dsi, vc, msg); if (r < 0) { dsi_unregister_isr_vc(dsi, vc, dsi_completion_handler, &completion, DSI_VC_IRQ_PACKET_SENT); return r; } ``` This is the same register-before-trigger pattern used in `dsi_vc_send_bt= a_sync()` at lines 2026=E2=80=932036. 3. **Short packet early return skips RX_FIFO_NOT_EMPTY check** (line 2202):= The early `return` for short packets bypasses the `RX_FIFO_NOT_EMPTY` chec= k at lines 2236=E2=80=932240. The old code ran that check for all writes. T= his is likely intentional (short packets without BTA shouldn't produce RX d= ata), but it's worth confirming this is safe =E2=80=94 if some panel/bridge= does produce unexpected data in the RX FIFO after a short write, it would = go undetected and could confuse subsequent reads. 4. **Minor: commit message typos** =E2=80=94 "wos" should be "was", and "ac= ually" should be "actually" in line 99 of the mbox: ``` the most relevant part wos the bta sync to acually force low power mode. ``` --- Generated by Claude Code Patch Reviewer