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: Tue, 10 Mar 2026 12:38:58 +1000 Message-ID: In-Reply-To: <20260309075248.47730-2-clamor95@gmail.com> References: <20260309075248.47730-1-clamor95@gmail.com> <20260309075248.47730-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 **Generally well done.** The `tegra_dsi_config` approach with boolean flags= is appropriate and keeps the code readable. **Minor issues:** 1. **Comment style (kernel convention violation):** The multi-line comment = block for `tegra124_dsi_config` uses `/* TODO:` on a single line. This is f= ine, but worth noting the convention. More importantly, the `XXX` comment t= hat was moved into the `has_multiple_pad_controls` branch at `dsi.c:676-678= ` =E2=80=94 while this is just code motion and preserves the existing comme= nt, it might be worth updating or removing it since this is a good opportun= ity. 2. **`devm_clk_get_optional` change for `clk_lp`:** The change from `devm_c= lk_get()` to `devm_clk_get_optional()` at line 1635 is correct for Tegra20/= 30 which lack this clock, but the subsequent error check: ```c dsi->clk_lp =3D devm_clk_get_optional(&pdev->dev, "lp"); if (IS_ERR(dsi->clk_lp)) { ``` This is still correct =E2=80=94 `devm_clk_get_optional` returns NULL if = the clock doesn't exist, and returns an error pointer only for real errors,= so the `IS_ERR` check is valid. 3. **`tegra_dsi_setup_clocks` comment:** The added comment at `dsi.c:1191-1= 195`: ```c /* * Tegra124+ uses a clock gate, not a mux, so this step * should be redundant for configuration; yet, DSI refuses * to work without it. */ ``` This is informative but the comment is placed *before* the `if (dsi->con= fig->has_mux_parent_clk)` guard, so on Tegra124+ this function *is* still c= alled (since `has_mux_parent_clk =3D true`). The comment seems to be explai= ning why `has_mux_parent_clk` is set to `true` on Tegra124+, but its placem= ent inside `tegra_dsi_setup_clocks()` rather than next to the config defini= tion is slightly confusing. The `TODO` comment on `tegra124_dsi_config` at = line 1741 is clearer. 4. **Tegra30 config identical to Tegra20:** Both `tegra20_dsi_config` and `= tegra30_dsi_config` have the same values. The explanatory comment about Teg= ra30 PLL_D/PLL_D2 muxing justifies having a separate struct for future use,= so this is acceptable. 5. **No functional concern with `device_get_match_data`:** The NULL check a= fter `device_get_match_data()` returning `-ENODEV` is the right pattern for= a required match data. **No blocking issues.** Already has Reviewed-by from Mikko Perttunen. --- Generated by Claude Code Patch Reviewer