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: drm/tidss: oldi: Convert OLDI to an aux driver
Date: Thu, 04 Jun 2026 16:54:13 +1000
Message-ID: <review-patch12-20260529-beagley-ai-display-v3-12-7fefdc5d1adf@ideasonboard.com>
In-Reply-To: <20260529-beagley-ai-display-v3-12-7fefdc5d1adf@ideasonboard.com>
References: <20260529-beagley-ai-display-v3-0-7fefdc5d1adf@ideasonboard.com> <20260529-beagley-ai-display-v3-12-7fefdc5d1adf@ideasonboard.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

This is the largest and most significant patch. It converts OLDI from direc=
t bridge creation to an auxiliary device/driver pattern to enable proper po=
wer-domain management.

**Good aspects:**
- The aux driver pattern is appropriate for the requirement
- Proper `of_node_put()` calls added to error paths (e.g., in `get_oldi_mod=
e()`)
- Device links between primary/secondary OLDI and between OLDI/DSS for PM
- The `tidss_oldi_aux_device_release()` properly frees IDA, puts of_node, f=
rees platform_data and the auxdev itself

**Concerns:**

1. **`pm_runtime_get_sync` without error handling in `atomic_pre_enable`:**
```c
+	WARN_ON(pm_runtime_get_sync(oldi->dev) < 0);
```
If `pm_runtime_get_sync()` fails, the code continues to configure OLDI hard=
ware, which could lead to register access on a powered-down domain. A `WARN=
_ON` will alert but not prevent the damage. In the pre-enable path it's har=
d to return an error, so this is pragmatically acceptable but worth noting.

2. **`clk_put` called unconditionally in remove but clock is only acquired =
conditionally:**
```c
+static void tidss_oldi_remove(struct auxiliary_device *auxdev)
+{
...
+	clk_put(oldi->serial);
```
The `oldi->serial` is only acquired when `link_type !=3D OLDI_MODE_SECONDAR=
Y_DUAL_LINK && link_type !=3D OLDI_MODE_SECONDARY_CLONE_SINGLE_LINK`. In th=
e secondary dual-link case, `oldi->serial` will be uninitialized (could be =
NULL or garbage). Similarly, in the error path of `tidss_oldi_probe()`:
```c
+err_bridge_remove:
...
+	clk_put(oldi->serial);
```
If the error occurs before `of_clk_get_by_name()`, `oldi->serial` will be u=
ninitialized. Since `devm_drm_bridge_alloc` zeroes the struct, `oldi->seria=
l` should be NULL, and `clk_put(NULL)` is a no-op, so this should be safe i=
n practice. But it's fragile =E2=80=94 if the allocation behavior changes, =
this could break.

3. **`device_link_del` never called on `link` between OLDI and tidss:**
In `tidss_oldi_probe()`, a device_link is created with `DL_FLAG_AUTOREMOVE_=
CONSUMER`, so it should be automatically removed. This is correct.

4. **Ordering of `auxiliary_device_destroy` in teardown:**
`tidss_oldi_destroy_devices()` iterates forward through `tidss->oldis[]`, w=
hich means if the primary was added first and the secondary was added as pa=
rt of primary creation, the secondary may be destroyed after its primary's =
device_link to it is already gone. The `DL_FLAG_AUTOREMOVE_SUPPLIER` flag o=
n the primary-secondary link should handle this, but the ordering deserves =
careful testing.

---
Generated by Claude Code Patch Reviewer