* Claude review: drm/tegra: dsi: add support for Tegra20/Tegra30
2026-03-09 7:52 ` [PATCH v4 1/2] drm/tegra: dsi: add " Svyatoslav Ryhel
@ 2026-03-10 2:38 ` Claude Code Review Bot
0 siblings, 0 replies; 7+ messages in thread
From: Claude Code Review Bot @ 2026-03-10 2:38 UTC (permalink / raw)
To: dri-devel-reviews
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 fine, but worth noting the convention. More importantly, the `XXX` comment that was moved into the `has_multiple_pad_controls` branch at `dsi.c:676-678` — while this is just code motion and preserves the existing comment, it might be worth updating or removing it since this is a good opportunity.
2. **`devm_clk_get_optional` change for `clk_lp`:** The change from `devm_clk_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 = devm_clk_get_optional(&pdev->dev, "lp");
if (IS_ERR(dsi->clk_lp)) {
```
This is still correct — `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-1195`:
```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->config->has_mux_parent_clk)` guard, so on Tegra124+ this function *is* still called (since `has_mux_parent_clk = true`). The comment seems to be explaining why `has_mux_parent_clk` is set to `true` on Tegra124+, but its placement inside `tegra_dsi_setup_clocks()` rather than next to the config definition 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 Tegra30 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 after `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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v4 0/2 RESEND] gpu/drm: tegra: add DSI support for Tegra20/Tegra30
@ 2026-05-11 7:45 Svyatoslav Ryhel
2026-05-11 7:45 ` [PATCH v4 1/2 RESEND] drm/tegra: dsi: add " Svyatoslav Ryhel
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Svyatoslav Ryhel @ 2026-05-11 7:45 UTC (permalink / raw)
To: Thierry Reding, Mikko Perttunen, David Airlie, Simona Vetter,
Jonathan Hunter, Diogo Ivo, Svyatoslav Ryhel
Cc: dri-devel, linux-tegra, linux-kernel
Tegra20/Tegra30 DSI is quite similar to Tegra114+ apart MIPI calibration
logic and clocks. With a few minor tweaks, existing tegra DSI driver
should work on Tegra20/Tegra30 devices just fine. Tested on
Motorola Atrix 4G (T20) and ASUS VivoTab RT TF600T (T30).
This patchset depends on Tegra20/Tegra30 CSI bringup since both share
MIPI calibration logic. Ideally these patches should be picked after
CSI bringup but they will not break anything even if picked before
CSI patches.
---
Changes in v2:
- removed all MIPI calibration, it is handled within CSI bringup
- added per-soc structures into of_match
- added fix for hang caused by register access with uninited hw
Changes in v3:
- switched from version DSI to boolean flags
- added comment regarding Tegra30 clock configuration
- commits were picked:
"clk: tegra20: reparent dsi clock to pll_d_out0"
"ARM: tegra: adjust DSI nodes for Tegra20/Tegra30"
- commit adapted to recent DSI changes:
"gpu/drm: tegra: dsi: move prepare function at the top of encoder
enable" > "gpu/drm: tegra: dsi: re-add clear enable register if
DSI was powered by bootloader"
Changes in v4:
- switched to device_get_match_data and added check
- added comment regarding Tegra124+ quirk
- adjusted commit titles and message formatting
---
Svyatoslav Ryhel (2):
drm/tegra: dsi: add support for Tegra20/Tegra30
drm/tegra: dsi: re-add clear enable register if DSI was powered by
bootloader
drivers/gpu/drm/tegra/drm.c | 2 +
drivers/gpu/drm/tegra/dsi.c | 126 +++++++++++++++++++++++++++---------
drivers/gpu/drm/tegra/dsi.h | 10 +++
3 files changed, 107 insertions(+), 31 deletions(-)
--
2.51.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v4 1/2 RESEND] drm/tegra: dsi: add support for Tegra20/Tegra30
2026-05-11 7:45 [PATCH v4 0/2 RESEND] gpu/drm: tegra: add DSI support for Tegra20/Tegra30 Svyatoslav Ryhel
@ 2026-05-11 7:45 ` Svyatoslav Ryhel
2026-05-16 5:48 ` Claude review: " Claude Code Review Bot
2026-05-11 7:45 ` [PATCH v4 2/2 RESEND] drm/tegra: dsi: re-add clear enable register if DSI was powered by bootloader Svyatoslav Ryhel
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Svyatoslav Ryhel @ 2026-05-11 7:45 UTC (permalink / raw)
To: Thierry Reding, Mikko Perttunen, David Airlie, Simona Vetter,
Jonathan Hunter, Diogo Ivo, Svyatoslav Ryhel
Cc: dri-devel, linux-tegra, linux-kernel
Tegra20 and Tegra30 are fully compatible with existing Tegra DSI driver
apart from clock configuration and pad calibration which are addressed by
this patch.
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
Reviewed-by: Mikko Perttunen <mperttunen@nvidia.com>
---
drivers/gpu/drm/tegra/drm.c | 2 +
drivers/gpu/drm/tegra/dsi.c | 117 ++++++++++++++++++++++++++----------
drivers/gpu/drm/tegra/dsi.h | 10 +++
3 files changed, 98 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 1dcef4e7d104..b73d65cd0ac6 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -1358,10 +1358,12 @@ static SIMPLE_DEV_PM_OPS(host1x_drm_pm_ops, host1x_drm_suspend,
static const struct of_device_id host1x_drm_subdevs[] = {
{ .compatible = "nvidia,tegra20-dc", },
+ { .compatible = "nvidia,tegra20-dsi", },
{ .compatible = "nvidia,tegra20-hdmi", },
{ .compatible = "nvidia,tegra20-gr2d", },
{ .compatible = "nvidia,tegra20-gr3d", },
{ .compatible = "nvidia,tegra30-dc", },
+ { .compatible = "nvidia,tegra30-dsi", },
{ .compatible = "nvidia,tegra30-hdmi", },
{ .compatible = "nvidia,tegra30-gr2d", },
{ .compatible = "nvidia,tegra30-gr3d", },
diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index 7f25c50621c9..fbab10bc5c41 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -54,6 +54,11 @@ to_dsi_state(struct drm_connector_state *state)
return container_of(state, struct tegra_dsi_state, base);
}
+struct tegra_dsi_config {
+ bool has_multiple_pad_controls;
+ bool has_mux_parent_clk;
+};
+
struct tegra_dsi {
struct host1x_client client;
struct tegra_output output;
@@ -83,6 +88,8 @@ struct tegra_dsi {
/* for ganged-mode support */
struct tegra_dsi *master;
struct tegra_dsi *slave;
+
+ const struct tegra_dsi_config *config;
};
static inline struct tegra_dsi *
@@ -665,39 +672,46 @@ static int tegra_dsi_pad_enable(struct tegra_dsi *dsi)
{
u32 value;
- value = DSI_PAD_CONTROL_VS1_PULLDN(0) | DSI_PAD_CONTROL_VS1_PDIO(0);
- tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_0);
+ if (dsi->config->has_multiple_pad_controls) {
+ /*
+ * XXX Is this still needed? The module reset is deasserted right
+ * before this function is called.
+ */
+ tegra_dsi_writel(dsi, 0, DSI_PAD_CONTROL_0);
+ tegra_dsi_writel(dsi, 0, DSI_PAD_CONTROL_1);
+ tegra_dsi_writel(dsi, 0, DSI_PAD_CONTROL_2);
+ tegra_dsi_writel(dsi, 0, DSI_PAD_CONTROL_3);
+ tegra_dsi_writel(dsi, 0, DSI_PAD_CONTROL_4);
+
+ value = DSI_PAD_CONTROL_VS1_PULLDN(0) | DSI_PAD_CONTROL_VS1_PDIO(0);
+ tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_0);
+
+ value = DSI_PAD_SLEW_UP(0x7) | DSI_PAD_SLEW_DN(0x7) |
+ DSI_PAD_LP_UP(0x1) | DSI_PAD_LP_DN(0x1) |
+ DSI_PAD_OUT_CLK(0x0);
+ tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_2);
+
+ value = DSI_PAD_PREEMP_PD_CLK(0x3) | DSI_PAD_PREEMP_PU_CLK(0x3) |
+ DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
+ tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
+ } else {
+ value = 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);
+ }
return 0;
}
static int tegra_dsi_pad_calibrate(struct tegra_dsi *dsi)
{
- u32 value;
int err;
- /*
- * XXX Is this still needed? The module reset is deasserted right
- * before this function is called.
- */
- tegra_dsi_writel(dsi, 0, DSI_PAD_CONTROL_0);
- tegra_dsi_writel(dsi, 0, DSI_PAD_CONTROL_1);
- tegra_dsi_writel(dsi, 0, DSI_PAD_CONTROL_2);
- tegra_dsi_writel(dsi, 0, DSI_PAD_CONTROL_3);
- tegra_dsi_writel(dsi, 0, DSI_PAD_CONTROL_4);
-
/* start calibration */
tegra_dsi_pad_enable(dsi);
- value = DSI_PAD_SLEW_UP(0x7) | DSI_PAD_SLEW_DN(0x7) |
- DSI_PAD_LP_UP(0x1) | DSI_PAD_LP_DN(0x1) |
- DSI_PAD_OUT_CLK(0x0);
- tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_2);
-
- value = DSI_PAD_PREEMP_PD_CLK(0x3) | DSI_PAD_PREEMP_PU_CLK(0x3) |
- DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
- tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
-
err = tegra_mipi_start_calibration(dsi->mipi);
if (err < 0)
return err;
@@ -1174,6 +1188,12 @@ static int tegra_dsi_setup_clocks(struct tegra_dsi *dsi)
struct clk *parent;
int err;
+ /*
+ * Tegra124+ uses a clock gate, not a mux, so this step
+ * should be redundant for configuration; yet, DSI refuses
+ * to work without it.
+ */
+
parent = clk_get_parent(dsi->clk);
if (!parent)
return -EINVAL;
@@ -1562,6 +1582,10 @@ static int tegra_dsi_probe(struct platform_device *pdev)
if (!dsi)
return -ENOMEM;
+ dsi->config = device_get_match_data(&pdev->dev);
+ if (!dsi->config)
+ return -ENODEV;
+
dsi->output.dev = dsi->dev = &pdev->dev;
dsi->video_fifo_depth = 1920;
dsi->host_fifo_depth = 64;
@@ -1600,7 +1624,7 @@ static int tegra_dsi_probe(struct platform_device *pdev)
goto remove;
}
- dsi->clk_lp = devm_clk_get(&pdev->dev, "lp");
+ dsi->clk_lp = devm_clk_get_optional(&pdev->dev, "lp");
if (IS_ERR(dsi->clk_lp)) {
err = dev_err_probe(&pdev->dev, PTR_ERR(dsi->clk_lp),
"cannot get low-power clock\n");
@@ -1621,10 +1645,12 @@ static int tegra_dsi_probe(struct platform_device *pdev)
goto remove;
}
- err = tegra_dsi_setup_clocks(dsi);
- if (err < 0) {
- dev_err(&pdev->dev, "cannot setup clocks\n");
- goto remove;
+ if (dsi->config->has_mux_parent_clk) {
+ err = tegra_dsi_setup_clocks(dsi);
+ if (err < 0) {
+ dev_err(&pdev->dev, "cannot setup clocks\n");
+ goto remove;
+ }
}
dsi->regs = devm_platform_ioremap_resource(pdev, 0);
@@ -1688,11 +1714,40 @@ static void tegra_dsi_remove(struct platform_device *pdev)
tegra_mipi_free(dsi->mipi);
}
+static const struct tegra_dsi_config tegra20_dsi_config = {
+ .has_multiple_pad_controls = false,
+ .has_mux_parent_clk = false,
+};
+
+/*
+ * Tegra30 allows DSIA/DSIB to be muxed to either PLL_D or PLL_D2; this is
+ * simply not modeled in the clock driver yet. If this functionality is
+ * required, the has_mux_parent_clk flag can be set to true once the clock
+ * driver is patched.
+ */
+static const struct tegra_dsi_config tegra30_dsi_config = {
+ .has_multiple_pad_controls = false,
+ .has_mux_parent_clk = false,
+};
+
+static const struct tegra_dsi_config tegra114_dsi_config = {
+ .has_multiple_pad_controls = true,
+ .has_mux_parent_clk = true,
+};
+
+/* TODO: figure out why has_mux_parent_clk = true is necessary on Tegra124+ */
+static const struct tegra_dsi_config tegra124_dsi_config = {
+ .has_multiple_pad_controls = true,
+ .has_mux_parent_clk = true,
+};
+
static const struct of_device_id tegra_dsi_of_match[] = {
- { .compatible = "nvidia,tegra210-dsi", },
- { .compatible = "nvidia,tegra132-dsi", },
- { .compatible = "nvidia,tegra124-dsi", },
- { .compatible = "nvidia,tegra114-dsi", },
+ { .compatible = "nvidia,tegra210-dsi", .data = &tegra124_dsi_config },
+ { .compatible = "nvidia,tegra132-dsi", .data = &tegra124_dsi_config },
+ { .compatible = "nvidia,tegra124-dsi", .data = &tegra124_dsi_config },
+ { .compatible = "nvidia,tegra114-dsi", .data = &tegra114_dsi_config },
+ { .compatible = "nvidia,tegra30-dsi", .data = &tegra30_dsi_config },
+ { .compatible = "nvidia,tegra20-dsi", .data = &tegra20_dsi_config },
{ },
};
MODULE_DEVICE_TABLE(of, tegra_dsi_of_match);
diff --git a/drivers/gpu/drm/tegra/dsi.h b/drivers/gpu/drm/tegra/dsi.h
index f39594e65e97..d834ac0c47ab 100644
--- a/drivers/gpu/drm/tegra/dsi.h
+++ b/drivers/gpu/drm/tegra/dsi.h
@@ -95,6 +95,16 @@
#define DSI_TALLY_LRX(x) (((x) & 0xff) << 8)
#define DSI_TALLY_HTX(x) (((x) & 0xff) << 0)
#define DSI_PAD_CONTROL_0 0x4b
+/* Tegra20/Tegra30 */
+#define DSI_PAD_CONTROL_PULLDN_ENAB(x) (((x) & 0x1) << 28)
+#define DSI_PAD_CONTROL_SLEWUPADJ(x) (((x) & 0x7) << 24)
+#define DSI_PAD_CONTROL_SLEWDNADJ(x) (((x) & 0x7) << 20)
+#define DSI_PAD_CONTROL_PREEMP_EN(x) (((x) & 0x1) << 19)
+#define DSI_PAD_CONTROL_PDIO_CLK(x) (((x) & 0x1) << 18)
+#define DSI_PAD_CONTROL_PDIO(x) (((x) & 0x3) << 16)
+#define DSI_PAD_CONTROL_LPUPADJ(x) (((x) & 0x3) << 14)
+#define DSI_PAD_CONTROL_LPDNADJ(x) (((x) & 0x3) << 12)
+/* Tegra114+ */
#define DSI_PAD_CONTROL_VS1_PDIO(x) (((x) & 0xf) << 0)
#define DSI_PAD_CONTROL_VS1_PDIO_CLK (1 << 8)
#define DSI_PAD_CONTROL_VS1_PULLDN(x) (((x) & 0xf) << 16)
--
2.51.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v4 2/2 RESEND] drm/tegra: dsi: re-add clear enable register if DSI was powered by bootloader
2026-05-11 7:45 [PATCH v4 0/2 RESEND] gpu/drm: tegra: add DSI support for Tegra20/Tegra30 Svyatoslav Ryhel
2026-05-11 7:45 ` [PATCH v4 1/2 RESEND] drm/tegra: dsi: add " Svyatoslav Ryhel
@ 2026-05-11 7:45 ` Svyatoslav Ryhel
2026-05-16 5:48 ` Claude review: gpu/drm: tegra: add DSI support for Tegra20/Tegra30 Claude Code Review Bot
2026-05-16 5:48 ` Claude Code Review Bot
3 siblings, 0 replies; 7+ messages in thread
From: Svyatoslav Ryhel @ 2026-05-11 7:45 UTC (permalink / raw)
To: Thierry Reding, Mikko Perttunen, David Airlie, Simona Vetter,
Jonathan Hunter, Diogo Ivo, Svyatoslav Ryhel
Cc: dri-devel, linux-tegra, linux-kernel
Original commit b22fd0b9639e ("drm/tegra: dsi: Clear enable register if
powered by bootloader") was added to address the issue of DSI being in an
unknown state after the bootloader, ensuring correct panel configuration.
This worked fairly well under the assumption that the bootloader had set
up DSI; however, in cases where it did not, the device would hang because
a DSI read was called before the DSI hardware was ready.
Removing this workaround results in the issue described in the original
fix: the panel initialization sequence fails and the panel gets stuck in
an undefined state. This is especially noticeable with command mode panels
In order to properly address this issue, the original workaround is
restored and placed after the DSI hardware is prepared for R/W operations.
This fixes behavior for both cases: where DSI is set by the bootloader and
where DSI is untouched.
I have tested this change on Tegra20 (Motorola Atrix 4G),
Tegra114 (NVIDIA Tegra Note 7 and ASUS Transformer Pad TF701T), and
Tegra124 (Xiaomi Mi Pad) with U-Boot, using both bootloader-initialized
DSI and untouched DSI.
Fixes: b22fd0b9639e ("drm/tegra: dsi: Clear enable register if powered by bootloader")
Fixes: 660b299bed2a ("Revert "drm/tegra: dsi: Clear enable register if powered by bootloader"")
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
Reviewed-by: Mikko Perttunen <mperttunen@nvidia.com>
---
drivers/gpu/drm/tegra/dsi.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index fbab10bc5c41..e7fdd8c7ac12 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -936,6 +936,15 @@ static void tegra_dsi_encoder_enable(struct drm_encoder *encoder)
return;
}
+ /* If the bootloader enabled DSI it needs to be disabled
+ * in order for the panel initialization commands to be
+ * properly sent.
+ */
+ value = tegra_dsi_readl(dsi, DSI_POWER_CONTROL);
+
+ if (value & DSI_POWER_CONTROL_ENABLE)
+ tegra_dsi_disable(dsi);
+
state = tegra_dsi_get_state(dsi);
tegra_dsi_set_timeout(dsi, state->bclk, state->vrefresh);
--
2.51.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Claude review: gpu/drm: tegra: add DSI support for Tegra20/Tegra30
2026-05-11 7:45 [PATCH v4 0/2 RESEND] gpu/drm: tegra: add DSI support for Tegra20/Tegra30 Svyatoslav Ryhel
` (2 preceding siblings ...)
2026-05-16 5:48 ` Claude review: gpu/drm: tegra: add DSI support for Tegra20/Tegra30 Claude Code Review Bot
@ 2026-05-16 5:48 ` Claude Code Review Bot
3 siblings, 0 replies; 7+ messages in thread
From: Claude Code Review Bot @ 2026-05-16 5:48 UTC (permalink / raw)
To: dri-devel-reviews
Overall Series Review
Subject: gpu/drm: tegra: add DSI support for Tegra20/Tegra30
Author: Svyatoslav Ryhel <clamor95@gmail.com>
Patches: 3
Reviewed: 2026-05-16T15:48:58.584162
---
This is a well-structured 2-patch series adding DSI support for Tegra20/Tegra30 to the existing Tegra DSI driver. The approach is sound: introduce a per-SoC config struct with boolean flags to gate the hardware differences (pad control registers and clock muxing), then make `clk_lp` optional since Tegra20/Tegra30 don't have it. The second patch re-introduces a bootloader-DSI workaround with better placement. Both patches have a Reviewed-by from Mikko Perttunen and the author has tested on real hardware across multiple SoC generations.
**Overall assessment: Series looks good with minor issues.**
The primary concern is in patch 2 where `tegra_dsi_disable()` is called unconditionally — including on ganged-mode setups where `dsi->slave` may not be initialized yet — and the comment style doesn't match kernel conventions. Patch 1 is clean and well-factored.
---
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 7+ messages in thread
* Claude review: gpu/drm: tegra: add DSI support for Tegra20/Tegra30
2026-05-11 7:45 [PATCH v4 0/2 RESEND] gpu/drm: tegra: add DSI support for Tegra20/Tegra30 Svyatoslav Ryhel
2026-05-11 7:45 ` [PATCH v4 1/2 RESEND] drm/tegra: dsi: add " Svyatoslav Ryhel
2026-05-11 7:45 ` [PATCH v4 2/2 RESEND] drm/tegra: dsi: re-add clear enable register if DSI was powered by bootloader Svyatoslav Ryhel
@ 2026-05-16 5:48 ` Claude Code Review Bot
2026-05-16 5:48 ` Claude Code Review Bot
3 siblings, 0 replies; 7+ messages in thread
From: Claude Code Review Bot @ 2026-05-16 5:48 UTC (permalink / raw)
To: dri-devel-reviews
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 `tegra_dsi_pad_calibrate()` into `tegra_dsi_pad_enable()` is clean and logical — the Tegra114+ pad init sequence (zeroing PAD_CONTROL_0..4, then writing slew/preemp settings) was always done before `tegra_mipi_start_calibration()`, and for Tegra20/30 that MIPI calibration path won't be used anyway (handled by CSI per the cover letter).
**Positive notes:**
- Using `device_get_match_data()` with an `ENODEV` check is the preferred modern pattern.
- `devm_clk_get_optional()` for `clk_lp` is correct — `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 1136 are safe without any additional guards.
- The separate `tegra30_dsi_config` (identical to `tegra20_dsi_config` today but with a forward-looking comment about PLL_D/PLL_D2 muxing) is reasonable.
- New compatibles in `host1x_drm_subdevs[]` are inserted in the correct sorted 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 = 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 == true` (i.e., Tegra114+). The comment is useful context and matches the `TODO` on `tegra124_dsi_config`, so this is fine — 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 = 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 — 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 omit the zero fields. This is a style nit, no functional impact.
4. New register macros in `dsi.h` — 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 = 0x4b` as the Tegra114+ macros, which is correct since it's the same register address with different bit layouts per SoC generation.
---
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 7+ messages in thread
* Claude review: drm/tegra: dsi: add support for Tegra20/Tegra30
2026-05-11 7:45 ` [PATCH v4 1/2 RESEND] drm/tegra: dsi: add " Svyatoslav Ryhel
@ 2026-05-16 5:48 ` Claude Code Review Bot
0 siblings, 0 replies; 7+ messages in thread
From: Claude Code Review Bot @ 2026-05-16 5:48 UTC (permalink / raw)
To: dri-devel-reviews
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 reverted in 660b299bed2a. The key insight is that the original workaround was placed *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_prepare()` completes (which enables clocks and deasserts reset), so the register read is always safe.
**Issue — ganged-mode interaction:**
```c
+ value = 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_dsi_ganged_disable(dsi->slave)` and then recursively `tegra_dsi_disable(dsi->slave)`. In `tegra_dsi_encoder_enable()`, this new code runs right after `tegra_dsi_prepare(dsi)`, which at line 903-904 calls `tegra_dsi_prepare(dsi->slave)` — so the slave hardware *is* prepared. However, `tegra_dsi_disable()` also calls `tegra_dsi_ganged_disable()` which writes to `DSI_GANGED_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_POWER_CONTROL_ENABLE` is set by the bootloader, the slave's is too, and that both are in a consistent ganged state. This probably works in practice (a bootloader wouldn't set up half a ganged link), but worth a comment or consideration of whether the check/disable should also be slave-aware.
**Style nit — 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 preferred 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 — 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 — 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 thorough and helpful. The dual `Fixes:` tags referencing both the original and the revert are appropriate.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-05-16 5:48 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-11 7:45 [PATCH v4 0/2 RESEND] gpu/drm: tegra: add DSI support for Tegra20/Tegra30 Svyatoslav Ryhel
2026-05-11 7:45 ` [PATCH v4 1/2 RESEND] drm/tegra: dsi: add " Svyatoslav Ryhel
2026-05-16 5:48 ` Claude review: " Claude Code Review Bot
2026-05-11 7:45 ` [PATCH v4 2/2 RESEND] drm/tegra: dsi: re-add clear enable register if DSI was powered by bootloader Svyatoslav Ryhel
2026-05-16 5:48 ` Claude review: gpu/drm: tegra: add DSI support for Tegra20/Tegra30 Claude Code Review Bot
2026-05-16 5:48 ` Claude Code Review Bot
-- strict thread matches above, loose matches on Subject: below --
2026-03-09 7:52 [PATCH v4 0/2] " Svyatoslav Ryhel
2026-03-09 7:52 ` [PATCH v4 1/2] drm/tegra: dsi: add " Svyatoslav Ryhel
2026-03-10 2:38 ` Claude review: " Claude Code Review Bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox