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: Add BeagleY-AI display support (and some more) Date: Thu, 23 Apr 2026 09:45:56 +1000 Message-ID: In-Reply-To: <20260420-beagley-ai-display-v1-0-f628543dfd14@ideasonboard.com> References: <20260420-beagley-ai-display-v1-0-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 Overall Series Review Subject: drm/tidss: Add BeagleY-AI display support (and some more) Author: Tomi Valkeinen Patches: 16 Reviewed: 2026-04-23T09:45:56.852621 --- This is a well-structured 15-patch series that adds display support for TI = AM62P/J722S/AM67A SoCs and the BeagleY-AI board. The series covers DT bindi= ngs, driver enhancements (DPI detection, signal edge config, DPIENABLE, OLD= I polarity fixes), a significant OLDI-to-auxiliary-device conversion, AM62P= device support, and board-level DTS for BeagleY-AI HDMI output. The overall architecture is sound. The OLDI auxiliary bus conversion (patch= 12) is the most complex change and is generally well done =E2=80=94 it cle= anly separates OLDI into a proper device model enabling per-OLDI power doma= in management. The DPI detection mechanism (patch 8) is pragmatic given the= hardware constraints. **Key concerns:** 1. **`WARN_ON(pm_runtime_get_sync())` in patch 12** =E2=80=94 `pm_runtime_g= et_sync()` returns a positive value (1) on success when the device was alre= ady active. `WARN_ON()` treats any non-zero as true, so this will fire a sp= urious warning on every pre_enable after the first. Should use `pm_runtime_= resume_and_get()` or check `< 0`. 2. **`syscon_regmap_lookup_by_compatible()` in patch 9** =E2=80=94 This is = a global lookup by compatible string rather than by phandle. On SoCs with t= wo DSS instances (AM62P), both DSS instances would find the same syscon nod= e. This happens to be correct if the register is shared, but it's fragile a= nd not the usual pattern. The DT binding (patch 5) doesn't add a `ti,dpi-io= -ctrl` phandle property to the DSS binding, though the DTS (patch 14) does = reference it. Consider using `syscon_regmap_lookup_by_phandle()` for robust= ness. 3. **`dpi_output` stored redundantly** =E2=80=94 The `dpi_output` flag is s= et per-call in `dispc_vp_setup()` via a parameter from `tcrtc->dpi_output`,= stored into `dispc->vp_data[hw_videoport].dpi_output`. This value never ch= anges after CRTC creation, so writing it on every `dispc_vp_setup()` call i= s unnecessary; it should be set once during init. 4. **Missing `of_node_put` for `oldi_tx` on success path in `for_each_avail= able_child_of_node` loop** in patch 12's `tidss_oldi_create_devices()` =E2= =80=94 the loop's `oldi_tx` reference is put on error, but `of_node_put(old= i_tx)` is not called explicitly on a normal break/return. The `for_each_ava= ilable_child_of_node` macro only puts the node automatically when the loop = runs to completion, which it does here (falling through), so this is actual= ly fine. --- --- Generated by Claude Code Patch Reviewer