From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: gpu: host1x: convert MIPI to use operation function pointers
Date: Wed, 04 Mar 2026 07:48:06 +1000
Message-ID: <review-patch5-20260303084239.15007-6-clamor95@gmail.com>
In-Reply-To: <20260303084239.15007-6-clamor95@gmail.com>
References: <20260303084239.15007-1-clamor95@gmail.com> <20260303084239.15007-6-clamor95@gmail.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

This is the large refactoring patch that introduces the `tegra_mipi_ops` abstraction, creates `tegra114-mipi.c` with the Tegra114+ calibration logic, and establishes the provider model via `devm_tegra_mipi_add_provider()`.

**Bug: clock leak in `tegra114_mipi_power_down()`:**

```c
static int tegra114_mipi_power_down(struct tegra_mipi *mipi)
{
	u32 value;
	int err;

	err = clk_enable(mipi->clk);
	if (err < 0)
		return err;
	...
	value |= MIPI_CAL_BIAS_PAD_PDVCLAMP;
	tegra_mipi_writel(mipi, value, MIPI_CAL_BIAS_PAD_CFG0);

	return 0;  // <-- missing clk_disable(mipi->clk)
}
```

`clk_enable()` is called at the top but `clk_disable()` is never called before returning. Compare with `tegra114_mipi_power_up()` which does correctly call `clk_disable()`. This will leak a clock enable count on every power-down.

**Minor: Duplicate defines with same register offset.** `MIPI_CAL_CONFIG_DSIC_CLK` and `MIPI_CAL_CONFIG_CSICD_CLK` are both `0x1c`, and `MIPI_CAL_CONFIG_DSID_CLK` and `MIPI_CAL_CONFIG_CSIE_CLK` are both `0x1d`. This is intentional (shared hardware registers), but a comment would be helpful for clarity.

---
Generated by Claude Code Patch Reviewer