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: staging: media: tegra-video: add CSI support for Tegra20 and Tegra30
Date: Wed, 04 Mar 2026 07:48:09 +1000
Message-ID: <review-patch15-20260303084239.15007-16-clamor95@gmail.com>
In-Reply-To: <20260303084239.15007-16-clamor95@gmail.com>
References: <20260303084239.15007-1-clamor95@gmail.com> <20260303084239.15007-16-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 largest patch (~620 lines added to tegra20.c). It adds CSI streaming, MIPI calibration ops, the frame capture refactoring, and Tegra20/30 CSI soc data.

**Bug: `release_buffer` always called with `VB2_BUF_STATE_DONE` even on error:**

```c
exit:
	release_buffer(chan, buf, VB2_BUF_STATE_DONE);
	return err;
```

When `err` is non-zero (syncpoint timeout), the buffer should be released with `VB2_BUF_STATE_ERROR`, not `VB2_BUF_STATE_DONE`. The old code in the pre-patch version correctly used `VB2_BUF_STATE_ERROR`. This will cause corrupted/incomplete frames to be delivered to userspace as if they were valid. The fix should be:

```c
exit:
	release_buffer(chan, buf, err ? VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);
```

**Bug: wrong register written in `tegra20_finish_pad_calibration` exit path:**

```c
pp = tegra20_mipi_read(csi, TEGRA_CSI_CSI_PIXEL_PARSER_STATUS);
cil = tegra20_mipi_read(csi, TEGRA_CSI_CSI_CIL_STATUS);
if (pp | cil) {
    ...
    ret = -EINVAL;
    goto exit;
}

exit:
	tegra20_mipi_write(csi, TEGRA_CSI_CSI_CIL_STATUS, pp);
```

On the error path, `pp` (pixel parser status) is written to `TEGRA_CSI_CSI_CIL_STATUS`. These are W1C (write-1-to-clear) status registers. Writing PP status bits to the CIL status register is incorrect - it should be `cil`, or both registers should be cleared independently:

```c
exit:
	tegra20_mipi_write(csi, TEGRA_CSI_CSI_PIXEL_PARSER_STATUS, pp);
	tegra20_mipi_write(csi, TEGRA_CSI_CSI_CIL_STATUS, cil);
```

**Minor: `tegra20_vi_read`, `tegra20_csi_read`, `tegra20_mipi_read` return `int` instead of `u32`:**

```c
static int __maybe_unused tegra20_vi_read(...)
{
	return readl(chan->vi->iomem + addr);
}
```

`readl()` returns `u32`. Returning `int` can cause sign-extension issues for values with bit 31 set. These should return `u32`.

**Minor: `tegra20_csi_clks` with NULL entry:**

```c
static const char * const tegra20_csi_clks[] = {
	NULL,
};
```

`ARRAY_SIZE` gives 1, so the driver will try to get one clock with name NULL via `devm_clk_bulk_get`. This will try to find the default/unnamed clock for the device. If the Tegra20 CSI DT node doesn't have a `clocks` property, probe will fail. If the intent is "no clocks needed", this should be an empty array or `num_clks` should be 0.

**ERESTARTSYS retry loop:** The `do { } while (err == -ERESTARTSYS)` pattern around `host1x_syncpt_wait` is acknowledged with a TODO comment as a workaround. This is acceptable for staging code, but the dead code check `if (err != -ERESTARTSYS)` after the loop can never be true since the loop only exits when `err != -ERESTARTSYS`.

---
Generated by Claude Code Patch Reviewer