From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/tidss: oldi: Convert OLDI to an aux driver Date: Thu, 23 Apr 2026 09:45:59 +1000 Message-ID: In-Reply-To: <20260420-beagley-ai-display-v1-12-f628543dfd14@ideasonboard.com> References: <20260420-beagley-ai-display-v1-0-f628543dfd14@ideasonboard.com> <20260420-beagley-ai-display-v1-12-f628543dfd14@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. The conversion to auxiliary= bus is well-motivated (per-OLDI power domains) and the implementation is g= enerally clean. **Bug =E2=80=94 `WARN_ON(pm_runtime_get_sync(oldi->dev))`:** ```c WARN_ON(pm_runtime_get_sync(oldi->dev)); ``` `pm_runtime_get_sync()` returns 1 when the device was already active, which= is a success case. `WARN_ON(1)` will fire a kernel warning. This should be: ```c ret =3D pm_runtime_resume_and_get(oldi->dev); WARN_ON(ret < 0); ``` or at minimum `WARN_ON(pm_runtime_get_sync(oldi->dev) < 0)`. **Missing `pm_runtime_put` on error paths in `tidss_oldi_atomic_pre_enable(= )`:** After the `pm_runtime_get_sync()` call at line 262, if any of the sub= sequent `WARN_ON(!connector)`, `WARN_ON(!conn_state)`, `WARN_ON(!crtc_state= )`, or `WARN_ON(!bridge_state)` checks fire and return early, the PM runtim= e reference is leaked. Each early return after the get should call `pm_runt= ime_put`. **`device_link_add` between primary and companion OLDI:** The flags include= both `DL_FLAG_AUTOREMOVE_CONSUMER` and `DL_FLAG_AUTOREMOVE_SUPPLIER`. Usin= g both simultaneously is documented as being correct (the link is removed w= hen either is unregistered), which fits the dual-link OLDI use case. **`tidss_oldi_destroy_devices` doesn't clear `num_oldis`:** After calling `= auxiliary_device_destroy()` for all devices, `tidss->num_oldis` is not rese= t to 0. If this function is called from the error path in `tidss_oldi_creat= e_devices()` and then the probe is retried (deferred probe), the stale `num= _oldis` could be problematic. Though in practice the whole probe would fail= and the device struct is freed. **IDA cleanup:** The `ida_free()` in `tidss_oldi_aux_device_release()` is c= orrect. The IDA is module-scoped (`static DEFINE_IDA`), and `ida_destroy()`= is not called at module exit =E2=80=94 should be called in `tidss_oldi_unr= egister_driver()` for clean shutdown. --- Generated by Claude Code Patch Reviewer