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/tegra: dsi: add support for Tegra20/Tegra30 Date: Sat, 16 May 2026 15:48:59 +1000 Message-ID: In-Reply-To: <20260511074538.24563-2-clamor95@gmail.com> References: <20260511074538.24563-1-clamor95@gmail.com> <20260511074538.24563-2-clamor95@gmail.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 **Overall: Reasonable fix, but has a potential issue with ganged mode and a= style nit.** This patch re-adds the workaround from commit b22fd0b9639e that was reverte= d in 660b299bed2a. The key insight is that the original workaround was plac= ed *before* DSI hardware was prepared (causing a hang on devices where the = bootloader didn't touch DSI), and this patch moves it *after* `tegra_dsi_pr= epare()` completes (which enables clocks and deasserts reset), so the regis= ter read is always safe. **Issue =E2=80=94 ganged-mode interaction:** ```c + value =3D tegra_dsi_readl(dsi, DSI_POWER_CONTROL); + + if (value & DSI_POWER_CONTROL_ENABLE) + tegra_dsi_disable(dsi); ``` `tegra_dsi_disable()` checks `dsi->slave` and, if non-NULL, calls `tegra_ds= i_ganged_disable(dsi->slave)` and then recursively `tegra_dsi_disable(dsi->= slave)`. In `tegra_dsi_encoder_enable()`, this new code runs right after `t= egra_dsi_prepare(dsi)`, which at line 903-904 calls `tegra_dsi_prepare(dsi-= >slave)` =E2=80=94 so the slave hardware *is* prepared. However, `tegra_dsi= _disable()` also calls `tegra_dsi_ganged_disable()` which writes to `DSI_GA= NGED_MODE_*` registers on the slave. In the bootloader-init case for ganged= panels, this should be fine since both DSI controllers would have been set= up. But the logic feels fragile: it assumes that if the master's `DSI_POWE= R_CONTROL_ENABLE` is set by the bootloader, the slave's is too, and that bo= th are in a consistent ganged state. This probably works in practice (a boo= tloader wouldn't set up half a ganged link), but worth a comment or conside= ration of whether the check/disable should also be slave-aware. **Style nit =E2=80=94 comment format:** ```c + /* If the bootloader enabled DSI it needs to be disabled + * in order for the panel initialization commands to be + * properly sent. + */ ``` This uses the `/* ... */` network-style multi-line comment format. The pref= erred kernel DRM subsystem style is: ```c /* * If the bootloader enabled DSI it needs to be disabled * in order for the panel initialization commands to be * properly sent. */ ``` The opening `/*` should be on its own line for multi-line comments (as used= throughout the rest of this file =E2=80=94 see e.g., the existing comment = at line 929 "The D-PHY timing fields are expressed in byte-clock cycles..."= ). **Commit message:** The commit message is excellent =E2=80=94 it clearly explains the history (= original commit, revert, why the original placement was wrong, and how this= fix addresses both cases). The testing matrix across T20/T114/T124 is thor= ough and helpful. The dual `Fixes:` tags referencing both the original and = the revert are appropriate. --- Generated by Claude Code Patch Reviewer