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/bridge: dw-hdmi: Replace calls to deprecated HDMI/audio detection functions Date: Fri, 27 Feb 2026 13:44:34 +1000 Message-ID: In-Reply-To: <20260225093051.12031-1-karanja99erick@gmail.com> References: <20260225093051.12031-1-karanja99erick@gmail.com> <20260225093051.12031-1-karanja99erick@gmail.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 **Concept: Correct.** The existing code has a FIXME comment (lines 2485-248= 9 in the current tree) that explicitly asks for this exact change. Removing= the deprecated `drm_detect_hdmi_monitor()` and `drm_detect_monitor_audio()= ` in favor of `connector->display_info.is_hdmi` / `.has_audio` is the right= direction. **Problem 1: Duplicate `drm_edid_connector_update()` in the connector path.= ** The patch adds `drm_edid_connector_update(connector, drm_edid)` inside `dw_= hdmi_edid_read()`. But `dw_hdmi_edid_read()` has **two callers**: 1. `dw_hdmi_connector_get_modes()` (line 2520) =E2=80=94 which **already ca= lls** `drm_edid_connector_update()` at line 2522: ```c drm_edid =3D dw_hdmi_edid_read(hdmi, connector); drm_edid_connector_update(connector, drm_edid); /* existing call */ ``` 2. `dw_hdmi_bridge_edid_read()` (line 3009) =E2=80=94 which returns the EDI= D to the **bridge connector framework**, which then calls `drm_edid_connect= or_update()` itself in `drm_bridge_connector_get_modes_edid()` at `drm_brid= ge_connector.c:311`. So with this patch, `drm_edid_connector_update()` will be called **twice** = in both code paths =E2=80=94 once inside `dw_hdmi_edid_read()` and once by = the caller/framework. While calling it twice is not a crash-inducing bug, i= t's wasteful and indicates the wrong approach. **The proper fix** should keep the `drm_edid_connector_update()` call where= it already is (in the callers/framework) and just change the `dw_hdmi_edid= _read()` function to read from `connector->display_info` **after** `dw_hdmi= _edid_read()` returns and `drm_edid_connector_update()` has been called. Th= is likely means moving the `sink_is_hdmi`/`sink_has_audio` assignment out o= f `dw_hdmi_edid_read()` and into each caller, after their respective `drm_e= did_connector_update()` calls. **Problem 2: Lost debug logging.** The patch removes: ```c dev_dbg(hdmi->dev, "got edid: width[%d] x height[%d]\n", edid->width_cm, edid->height_cm); ``` This debug message is useful for driver developers and should be preserved.= It can be rewritten to use `connector->display_info` fields (e.g., `connec= tor->display_info.width_mm`) rather than raw EDID fields, or simply kept as= -is using `drm_edid_raw()`. **Problem 3: Ordering concern.** Even ignoring the duplicate call, the patch places `drm_edid_connector_upda= te()` *before* returning the `drm_edid` to the caller. In `dw_hdmi_connecto= r_get_modes()`, the caller then calls it again. But more importantly, the b= ridge path (`dw_hdmi_bridge_edid_read`) just returns `drm_edid` and the fra= mework calls `drm_edid_connector_update()` again. The intent of the FIXME w= as to read `display_info` **after** the normal update path has run, not to = add an early extra call. **Suggested approach:** Instead of adding `drm_edid_connector_update()` ins= ide `dw_hdmi_edid_read()`, refactor so that `sink_is_hdmi` and `sink_has_au= dio` are set in each caller *after* `drm_edid_connector_update()` has been = called: ```c /* In dw_hdmi_connector_get_modes(): */ drm_edid =3D dw_hdmi_edid_read(hdmi, connector); drm_edid_connector_update(connector, drm_edid); /* already exists */ hdmi->sink_is_hdmi =3D connector->display_info.is_hdmi; hdmi->sink_has_audio =3D connector->display_info.has_audio; ``` And similarly handle the bridge path (which is trickier since the framework= makes the call, so you may need to set these fields in the bridge `atomic_= enable` or `mode_set` callback where `display_info` is already populated). **Minor:** The commit message and subject are clear and well-written. The `= RFT` (Request For Testing) tag is appropriate given this is untested. --- Generated by Claude Code Patch Reviewer