public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/bridge: dw-hdmi-qp: Return -EOPNOTSUPP in HDMI audio functions
@ 2026-05-19 18:00 Detlev Casanova
  2026-05-25 12:33 ` Claude review: " Claude Code Review Bot
  2026-05-25 12:33 ` Claude Code Review Bot
  0 siblings, 2 replies; 3+ messages in thread
From: Detlev Casanova @ 2026-05-19 18:00 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel, kernel, Detlev Casanova

-EOPNOTSUPP is not logged as an error by the ASoC subsystem, but -ENODEV
is.
It also better represents the situation: The operation is currently not
supported (because clocks are not enabled and tmds_char_rate is
unavailable), but the hardware is present.

Using -EOPNOTSUPP in the audio_prepare callback removes possible repeated
warning log lines when HDMI is not connected.

Returning -EOPNOTSUPP in the audio_enable callback is also needed as it
avoids logging 0-valued ELD errors.

When tmds_char_rate is available, the clocks are enabled and the
functions will keep returning 0 as before.

Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
index d649a1cf07f5..270c8c47d853 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
@@ -469,8 +469,10 @@ static int dw_hdmi_qp_audio_enable(struct drm_bridge *bridge,
 {
 	struct dw_hdmi_qp *hdmi = dw_hdmi_qp_from_bridge(bridge);
 
-	if (hdmi->tmds_char_rate)
-		dw_hdmi_qp_mod(hdmi, 0, AVP_DATAPATH_PACKET_AUDIO_SWDISABLE, GLOBAL_SWDISABLE);
+	if (!hdmi->tmds_char_rate)
+		return -EOPNOTSUPP;
+
+	dw_hdmi_qp_mod(hdmi, 0, AVP_DATAPATH_PACKET_AUDIO_SWDISABLE, GLOBAL_SWDISABLE);
 
 	return 0;
 }
@@ -484,7 +486,7 @@ static int dw_hdmi_qp_audio_prepare(struct drm_bridge *bridge,
 	bool ref2stream = false;
 
 	if (!hdmi->tmds_char_rate)
-		return -ENODEV;
+		return -EOPNOTSUPP;
 
 	if (fmt->bit_clk_provider | fmt->frame_clk_provider) {
 		dev_err(hdmi->dev, "unsupported clock settings\n");

---
base-commit: 1a2ab0feaa23147e347b4d4cb79cb3fc392118eb
change-id: 20260519-fix-hdmi-audio-warnings-139062ec00b9

Best regards,
--  
Detlev Casanova <detlev.casanova@collabora.com>


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Claude review: drm/bridge: dw-hdmi-qp: Return -EOPNOTSUPP in HDMI audio functions
  2026-05-19 18:00 [PATCH] drm/bridge: dw-hdmi-qp: Return -EOPNOTSUPP in HDMI audio functions Detlev Casanova
@ 2026-05-25 12:33 ` Claude Code Review Bot
  2026-05-25 12:33 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-05-25 12:33 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/bridge: dw-hdmi-qp: Return -EOPNOTSUPP in HDMI audio functions
Author: Detlev Casanova <detlev.casanova@collabora.com>
Patches: 1
Reviewed: 2026-05-25T22:33:16.962963

---

This is a single-patch series that changes the error code returned by the `dw_hdmi_qp_audio_enable` and `dw_hdmi_qp_audio_prepare` callbacks from `-ENODEV` (and implicit success-when-not-ready) to `-EOPNOTSUPP` when `tmds_char_rate == 0` (i.e., HDMI is not connected).

The change is well-motivated and correct. The ASoC framework's `snd_soc_ret()` function (`sound/soc/soc-utils.c:18-42`) explicitly suppresses logging for `-EOPNOTSUPP` and `-ENOTSUPP`, while all other negative error codes (including the previous `-ENODEV`) produce `dev_err` messages. This means the old code was generating spurious error logs whenever audio operations were attempted while HDMI was disconnected — a normal and frequent runtime condition.

The semantic choice is also correct: `-EOPNOTSUPP` ("operation not supported") accurately describes a transient state where the hardware exists but clocks aren't enabled yet, whereas `-ENODEV` ("no such device") incorrectly suggests the hardware is absent.

**Verdict: Looks good. Suitable for merging.**

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Claude review: drm/bridge: dw-hdmi-qp: Return -EOPNOTSUPP in HDMI audio functions
  2026-05-19 18:00 [PATCH] drm/bridge: dw-hdmi-qp: Return -EOPNOTSUPP in HDMI audio functions Detlev Casanova
  2026-05-25 12:33 ` Claude review: " Claude Code Review Bot
@ 2026-05-25 12:33 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-05-25 12:33 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**audio_enable refactoring (lines 472-475 after patch):**

```c
	if (!hdmi->tmds_char_rate)
		return -EOPNOTSUPP;

	dw_hdmi_qp_mod(hdmi, 0, AVP_DATAPATH_PACKET_AUDIO_SWDISABLE, GLOBAL_SWDISABLE);
```

Previously, the logic was:

```c
	if (hdmi->tmds_char_rate)
		dw_hdmi_qp_mod(hdmi, 0, AVP_DATAPATH_PACKET_AUDIO_SWDISABLE, GLOBAL_SWDISABLE);

	return 0;
```

The old code returned success (0) even when `tmds_char_rate` was 0 — meaning audio was "enabled" successfully without actually touching the hardware. This is a subtle behavioral change: callers (the `hdmi_codec_startup` path) now see a failure when HDMI is disconnected, rather than a silent no-op success. This is the correct fix — a silent success would allow the ASoC stack to proceed to `get_eld` and `prepare`, which would operate on stale/zero ELD data. By returning `-EOPNOTSUPP` here, the startup aborts cleanly before the ELD path (`hdmi-codec.c:468-482`) is reached, which prevents the "0-valued ELD errors" mentioned in the commit message.

**audio_prepare change (line 489 after patch):**

```c
	if (!hdmi->tmds_char_rate)
		return -EOPNOTSUPP;
```

This is a straightforward error code substitution from `-ENODEV` to `-EOPNOTSUPP`. The guard already existed; only the error code changes.

**Consistency with audio_disable:**

The `dw_hdmi_qp_audio_disable` function (line 526) returns `void` and already has a `tmds_char_rate` guard that simply skips the register writes. No change needed there, and the patch correctly doesn't touch it.

**No issues found.** The patch is minimal, well-scoped, and the commit message accurately explains both the problem and the mechanism. The `-EOPNOTSUPP` choice is validated by `snd_soc_ret()` in `sound/soc/soc-utils.c:30-31` which explicitly skips `dev_err` logging for this error code.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-05-25 12:33 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-19 18:00 [PATCH] drm/bridge: dw-hdmi-qp: Return -EOPNOTSUPP in HDMI audio functions Detlev Casanova
2026-05-25 12:33 ` Claude review: " Claude Code Review Bot
2026-05-25 12:33 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox