From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: gpu/drm: tegra: add DSI support for Tegra20/Tegra30 Date: Sat, 16 May 2026 15:48:58 +1000 Message-ID: In-Reply-To: <20260511074538.24563-1-clamor95@gmail.com> References: <20260511074538.24563-1-clamor95@gmail.com> <20260511074538.24563-1-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: Good patch, well-structured refactoring.** This patch introduces `struct tegra_dsi_config` with two boolean flags and = associates per-SoC configs via `of_match_data`. The code movement from `teg= ra_dsi_pad_calibrate()` into `tegra_dsi_pad_enable()` is clean and logical = =E2=80=94 the Tegra114+ pad init sequence (zeroing PAD_CONTROL_0..4, then w= riting slew/preemp settings) was always done before `tegra_mipi_start_calib= ration()`, and for Tegra20/30 that MIPI calibration path won't be used anyw= ay (handled by CSI per the cover letter). **Positive notes:** - Using `device_get_match_data()` with an `ENODEV` check is the preferred m= odern pattern. - `devm_clk_get_optional()` for `clk_lp` is correct =E2=80=94 `clk_prepare_= enable(NULL)` / `clk_disable_unprepare(NULL)` are documented no-ops in the = kernel clock API, so the runtime suspend/resume paths at lines 1103 and 113= 6 are safe without any additional guards. - The separate `tegra30_dsi_config` (identical to `tegra20_dsi_config` toda= y but with a forward-looking comment about PLL_D/PLL_D2 muxing) is reasonab= le. - New compatibles in `host1x_drm_subdevs[]` are inserted in the correct sor= ted position (between `-dc` and `-hdmi` for each SoC generation). **Minor comments:** 1. The comment added at the top of `tegra_dsi_setup_clocks()`: ```c + /* + * Tegra124+ uses a clock gate, not a mux, so this step + * should be redundant for configuration; yet, DSI refuses + * to work without it. + */ + parent =3D clk_get_parent(dsi->clk); ``` This comment describes a Tegra124+ quirk but sits inside a function that is= now only called when `has_mux_parent_clk =3D=3D true` (i.e., Tegra114+). T= he comment is useful context and matches the `TODO` on `tegra124_dsi_config= `, so this is fine =E2=80=94 but the blank line after the closing `*/` is a= bit unusual for kernel style. 2. The `tegra20_dsi_config` and `tegra30_dsi_config` structs have identical= content. This is a deliberate choice (documented by the comment on tegra30= 's struct) to allow independent future evolution. No issue, just noting it. 3. The Tegra20/30 pad control else-branch in `tegra_dsi_pad_enable()`: ```c + } else { + value =3D DSI_PAD_CONTROL_LPUPADJ(0x1) | DSI_PAD_CONTROL_LPDNADJ(0x1) | + DSI_PAD_CONTROL_PREEMP_EN(0x1) | DSI_PAD_CONTROL_SLEWDNADJ(0x6) | + DSI_PAD_CONTROL_SLEWUPADJ(0x6) | DSI_PAD_CONTROL_PDIO(0) | + DSI_PAD_CONTROL_PDIO_CLK(0) | DSI_PAD_CONTROL_PULLDN_ENAB(0); + tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_0); + } ``` The `PDIO(0)`, `PDIO_CLK(0)`, and `PULLDN_ENAB(0)` fields are all zero and = don't contribute to the final register value =E2=80=94 they're OR'd in as 0= . Including them for documentation/completeness is acceptable, but if there= was an intent to make these configurable later, it might be cleaner to omi= t the zero fields. This is a style nit, no functional impact. 4. New register macros in `dsi.h` =E2=80=94 the bit positions look correct = based on the Tegra TRM (e.g., `PULLDN_ENAB` at bit 28, `SLEWUPADJ` at bits = [26:24], etc.). The macros share the same register offset `DSI_PAD_CONTROL_= 0 =3D 0x4b` as the Tegra114+ macros, which is correct since it's the same r= egister address with different bit layouts per SoC generation. --- --- Generated by Claude Code Patch Reviewer