Claude review: drm/scdc-helper: Add scdc_status debugfs entry

Claude review: drm/scdc-helper: Add scdc_status debugfs entry

[Claude Code Review Bot]

Patch Review

This is the bulk of the series — adds `drm_scdc_read_state`, status flag structs, error counter reading, and the debugfs show function. Generally well done, but a few issues:

**1. Inappropriate comment (medium)**

```c
/* why in the fucking fuck does this return a ssize_t */
ret = drm_scdc_read(connector->ddc, SCDC_ERR_DET_0_L, buf, ARRAY_SIZE(buf));
```

This comment is not appropriate for the kernel tree. If you want to note the type mismatch, something like `/* drm_scdc_read returns ssize_t but 0 on success */` would be acceptable.

**2. Bitfield union portability on big-endian (medium)**

```c
struct drm_scdc_status_flags {
	union {
		struct {
			bool clock_detected : 1;
			bool ch0_locked : 1;
			bool ch1_locked : 1;
			bool ch2_locked : 1;
			...
		} flags __packed;
		u8 data;
	} status0;
```

GCC orders bitfields from LSB on little-endian targets but from MSB on big-endian targets. On a big-endian system, `clock_detected` would map to bit 7 instead of bit 0 (where `SCDC_CLOCK_DETECT` is defined). This means the bitfield-to-register mapping is wrong on big-endian. Consider using explicit bit masks (the header already defines `SCDC_CLOCK_DETECT`, `SCDC_CH0_LOCK`, etc.) and accessor functions instead, or adding an endianness guard/comment explaining why this is acceptable.

**3. Doc typo**

```c
/** @scramling_enabled: true if TMDS scrambling is on */
```

Should be `@scrambling_enabled`.

**4. Status reading clears update flag before reading the register**

In `drm_scdc_read_status0_flags`:
```c
ret = drm_scdc_writeb(connector->ddc, SCDC_UPDATE_0, SCDC_STATUS_UPDATE);
if (ret)
    return ret;

return drm_scdc_readb(connector->ddc, SCDC_STATUS_FLAGS_0, &flags->status0.data);
```

The update flag is cleared (by writing 1 to it) *before* reading the status register. There's a small race window where the sink could update the status between the clear and the read, meaning the source wouldn't know to re-read later. For debugfs this is fine, but since `drm_scdc_read_status0_flags` is exported as a general-purpose API, consider documenting this behavior or swapping the order (read first, then clear). The same pattern appears in `drm_scdc_read_error_counters` with `SCDC_CED_UPDATE`, though for CED clearing before reading is the spec-mandated behavior, so that one is correct.

**5. Minor: `status1` bitfields are all reserved and unused**

The `status1` member in `drm_scdc_status_flags` has 8 reserved bool bitfields but is never read or displayed in this patch. This is fine since patch 3 replaces them, but in isolation patch 1 adds dead code.

---
Generated by Claude Code Patch Reviewer

[PATCH v3 0/4] Add SCDC information to connector debugfs

[Nicolas Frattaroli]

HDMI uses the DDC I2C bus for communicating various bits of link status
out of band with the actual HDMI video signal. This information can be
useful for debugging issues like questionable cables sabotaged by feline
teeth, Enthusiast Grade cables made of cow fencing wire, and other such
problems that ruin one's media viewing plans.

Consequently, this series exposes various bits of pertinent information
from the SCDC protocol in an HDMI connector's debugfs. To continually
poll the link status, userspace can poll the debugfs file.

---
Changes in v3:
- Add patch to change return type of drm_scdc_read/write.
- Rework error counter reading to duplicate less code.
- Also check lane 3 counter valid flag when reading its error counter.
- Use memset to clear buf for error counters, rather than doing it in
  the loop.
- Make read_error_counters not accept 0 as num_lanes; fix it up in the
  caller instead.
- Link to v2: https://patch.msgid.link/20260520-scdc-link-health-v2-0-511af18cd64b@collabora.com

Changes in v2:
- Add HDMI 2.1 SCDC status reporting
- Link to v1: https://patch.msgid.link/20260415-scdc-link-health-v1-0-8e731e88eaf0@collabora.com

To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Maxime Ripard <mripard@kernel.org>
To: Thomas Zimmermann <tzimmermann@suse.de>
To: David Airlie <airlied@gmail.com>
To: Simona Vetter <simona@ffwll.ch>
To: Andrzej Hajda <andrzej.hajda@intel.com>
To: Neil Armstrong <neil.armstrong@linaro.org>
To: Robert Foss <rfoss@kernel.org>
To: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
To: Jonas Karlman <jonas@kwiboo.se>
To: Jernej Skrabec <jernej.skrabec@gmail.com>
To: Luca Ceresoli <luca.ceresoli@bootlin.com>
To: Daniel Stone <daniel@fooishbar.org>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
Cc: kernel@collabora.com
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

---
Nicolas Frattaroli (4):
      drm/scdc-helper: Don't use ssize_t return type for scdc_read/write
      drm/scdc-helper: Add scdc_status debugfs entry
      drm/display: bridge_connector: init scdc debugfs for HDMI
      drm/scdc-helper: Implement parsing and printing HDMI 2.1 fields

 drivers/gpu/drm/display/drm_bridge_connector.c |   4 +
 drivers/gpu/drm/display/drm_scdc_helper.c      | 358 ++++++++++++++++++++++++-
 include/drm/display/drm_scdc.h                 |   9 +
 include/drm/display/drm_scdc_helper.h          | 121 ++++++++-
 4 files changed, 484 insertions(+), 8 deletions(-)
---
base-commit: 587c82eb2067eff6f13385e1b7cf49fc6c3febbc
change-id: 20260413-scdc-link-health-89326013d96c

Best regards,
--  
Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

[PATCH v3 1/4] drm/scdc-helper: Don't use ssize_t return type for scdc_read/write

[Nicolas Frattaroli]

drm_scdc_read and drm_scdc_write, both of which are only used within
drm_scdc_helper (although exported), use a ssize_t as their return type.

This would make sense if they returned the number of bytes read/written
on success, and negative errno otherwise. However, they return 0 on
success.

Demote them to "int" as their return type, in order to avoid needlessly
using 64 bits when less suffices.

No functional change.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/display/drm_scdc_helper.c | 8 ++++----
 include/drm/display/drm_scdc_helper.h     | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
index df878aad4a36..8403f2390ab6 100644
--- a/drivers/gpu/drm/display/drm_scdc_helper.c
+++ b/drivers/gpu/drm/display/drm_scdc_helper.c
@@ -67,8 +67,8 @@
  * Returns:
  * 0 on success, negative error code on failure.
  */
-ssize_t drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
-		      size_t size)
+int drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
+		  size_t size)
 {
 	int ret;
 	struct i2c_msg msgs[2] = {
@@ -107,8 +107,8 @@ EXPORT_SYMBOL(drm_scdc_read);
  * Returns:
  * 0 on success, negative error code on failure.
  */
-ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
-		       const void *buffer, size_t size)
+int drm_scdc_write(struct i2c_adapter *adapter, u8 offset, const void *buffer,
+		   size_t size)
 {
 	struct i2c_msg msg = {
 		.addr = SCDC_I2C_SLAVE_ADDRESS,
diff --git a/include/drm/display/drm_scdc_helper.h b/include/drm/display/drm_scdc_helper.h
index 34600476a1b9..e9ccaeba56dd 100644
--- a/include/drm/display/drm_scdc_helper.h
+++ b/include/drm/display/drm_scdc_helper.h
@@ -31,10 +31,10 @@
 struct drm_connector;
 struct i2c_adapter;
 
-ssize_t drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
-		      size_t size);
-ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
-		       const void *buffer, size_t size);
+int drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
+		  size_t size);
+int drm_scdc_write(struct i2c_adapter *adapter, u8 offset, const void *buffer,
+		   size_t size);
 
 /**
  * drm_scdc_readb - read a single byte from SCDC

-- 
2.54.0

[PATCH v3 2/4] drm/scdc-helper: Add scdc_status debugfs entry

[Nicolas Frattaroli]

SCDC provides status information on the current display link. At the
very least, it may be useful to expose this info through debugfs.

Add a debugfs entry for it under the connector, which displays a few
more details parsed out of the SCDC registers. A new
drm_scdc_debugfs_init function can be called by the connector
implementation to initialise the debugfs file.

Reviewed-by: Daniel Stone <daniels@collabora.com>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/display/drm_scdc_helper.c | 228 ++++++++++++++++++++++++++++++
 include/drm/display/drm_scdc_helper.h     |  56 ++++++++
 2 files changed, 284 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
index 8403f2390ab6..75a59c6fc7a5 100644
--- a/drivers/gpu/drm/display/drm_scdc_helper.c
+++ b/drivers/gpu/drm/display/drm_scdc_helper.c
@@ -24,11 +24,14 @@
 #include <linux/export.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
+#include <linux/debugfs.h>
 #include <linux/delay.h>
+#include <linux/overflow.h>
 
 #include <drm/display/drm_scdc_helper.h>
 #include <drm/drm_connector.h>
 #include <drm/drm_device.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_print.h>
 
 /**
@@ -55,6 +58,11 @@
 
 #define SCDC_I2C_SLAVE_ADDRESS 0x54
 
+struct scdc_debugfs_priv {
+	struct drm_connector *connector;
+	struct drm_scdc_state state;
+};
+
 /**
  * drm_scdc_read - read a block of data from SCDC
  * @adapter: I2C controller
@@ -276,3 +284,223 @@ bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector,
 	return true;
 }
 EXPORT_SYMBOL(drm_scdc_set_high_tmds_clock_ratio);
+
+/**
+ * drm_scdc_read_status0_flags - Read SCDC "Status Flags" Register
+ * @connector: pointer to &struct drm_connector to issue the scdc request on
+ * @flags: pointer to the caller's &struct drm_scdc_status_flags to output to
+ *
+ * Reads the SCDC Status Flags 0 register, and outputs its contents to the
+ * destination @flags. Contents of @flags.status0 is only valid if function
+ * returns 0.
+ *
+ * Returns: %0 on success, negative errno on error.
+ */
+int drm_scdc_read_status0_flags(struct drm_connector *connector,
+				struct drm_scdc_status_flags *flags)
+{
+	int ret;
+
+	ret = drm_scdc_writeb(connector->ddc, SCDC_UPDATE_0, SCDC_STATUS_UPDATE);
+	if (ret)
+		return ret;
+
+	return drm_scdc_readb(connector->ddc, SCDC_STATUS_FLAGS_0, &flags->status0.data);
+}
+EXPORT_SYMBOL(drm_scdc_read_status0_flags);
+
+/**
+ * drm_scdc_read_error_counters - Read and clear SCDC error counters
+ * @connector: pointer to &struct drm_connector to issue the scdc request on
+ * @counter: Caller's u16 array with 3 elements to write the counter values into
+ *
+ * Read the SCDC channel error counters. If the count of channel *n* is valid,
+ * write it into counter[n]. Otherwise, set counter[n] to 0. Reads all counters
+ * in one read chunk, then clears every counter, as is mandated.
+ *
+ * Returns: %0 on success, negative errno on error.
+ */
+int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3])
+{
+	u8 buf[7] = { 0 };
+	int ret;
+	u8 sum = 0;
+	int i;
+
+	ret = drm_scdc_writeb(connector->ddc, SCDC_UPDATE_0, SCDC_CED_UPDATE);
+	if (ret)
+		return ret;
+
+	ret = drm_scdc_read(connector->ddc, SCDC_ERR_DET_0_L, buf, ARRAY_SIZE(buf));
+	if (ret)
+		return ret;
+
+	/*
+	 * Verify the "checksum", i.e. sum up everything including the checksum
+	 * register as a wrapping unsigned 8-bit addition and verify it's 0.
+	 */
+	for (i = 0; i < ARRAY_SIZE(buf); i++)
+		sum = wrapping_add(u8, sum, buf[i]);
+
+	if (sum)
+		return -EPROTO;
+
+	for (i = 0; i < ARRAY_SIZE(buf) - 1; i += 2) {
+		if (buf[i + 1] & SCDC_CHANNEL_VALID)
+			counter[i / 2] = buf[i] | (buf[i + 1] & ~SCDC_CHANNEL_VALID) << 8;
+		else
+			counter[i / 2] = 0;
+
+		buf[i] = 0;
+		buf[i + 1] = 0;
+	}
+	buf[ARRAY_SIZE(buf) - 1] = 0;
+
+	return drm_scdc_write(connector->ddc, SCDC_ERR_DET_0_L, buf, ARRAY_SIZE(buf));
+}
+EXPORT_SYMBOL(drm_scdc_read_error_counters);
+
+/**
+ * drm_scdc_read_state - Update state from SCDC
+ * @connector: pointer to a &struct drm_connector on which to operate on
+ * @state: pointer to a &struct drm_scdc_state to fill
+ *
+ * Reads update flags from SCDC, and updates the parts of @state that SCDC
+ * claims have changed, as well as populating those where such a distinction
+ * can't be made.
+ *
+ * Returns: %0 on success, negative errno on failure.
+ */
+int drm_scdc_read_state(struct drm_connector *connector, struct drm_scdc_state *state)
+{
+	u8 upd_flags[2] = { 0 };
+	struct i2c_adapter *ddc;
+	struct drm_scdc *scdc;
+	int ret;
+	u8 val;
+
+	if (!state || !connector)
+		return -ENODEV;
+
+	scdc = &connector->display_info.hdmi.scdc;
+	ddc = connector->ddc;
+
+	if (!scdc->supported)
+		return -EOPNOTSUPP;
+
+	ret = drm_scdc_readb(ddc, SCDC_TMDS_CONFIG, &val);
+	if (ret)
+		return ret;
+
+	state->scrambling_enabled = !!(val & SCDC_SCRAMBLING_ENABLE);
+	state->tmds_bclk_x40 = !!(val & SCDC_TMDS_BIT_CLOCK_RATIO_BY_40);
+
+	state->scrambling_detected = drm_scdc_get_scrambling_status(connector);
+
+	ret = drm_scdc_read(ddc, SCDC_UPDATE_0, &upd_flags, sizeof(upd_flags));
+	if (ret)
+		return ret;
+
+	if (upd_flags[0] & SCDC_STATUS_UPDATE) {
+		ret = drm_scdc_read_status0_flags(connector, &state->stf);
+		if (ret)
+			return ret;
+	}
+
+	if (upd_flags[0] & SCDC_CED_UPDATE) {
+		ret = drm_scdc_read_error_counters(connector, state->error_count);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_scdc_read_state);
+
+#define scdc_print_str(_f, key, s) \
+	(seq_printf((_f), "%-30s: %s\n", (key), (s)))
+#define scdc_print_flag(_f, key, val) \
+	(scdc_print_str((_f), (key), str_yes_no((val))))
+#define scdc_print_dec(_f, key, val) \
+	(seq_printf((_f), "%-30s: %d\n", (key), (val)))
+
+static int scdc_status_show(struct seq_file *m, void *data)
+{
+	struct scdc_debugfs_priv *priv = m->private;
+	struct drm_scdc_state *st = &priv->state;
+	struct drm_connector *connector = priv->connector;
+	struct drm_scdc *scdc = &connector->display_info.hdmi.scdc;
+	int ret;
+
+	drm_connector_get(connector);
+
+	if (connector->status != connector_status_connected) {
+		ret = -ENODEV;
+		goto err_conn_put;
+	}
+
+	scdc_print_flag(m, "SCDC Supported", scdc->supported);
+	if (!scdc->supported) {
+		ret = 0;
+		goto err_conn_put;
+	}
+
+	scdc_print_flag(m, "Sink Read Request Capable", scdc->read_request);
+	scdc_print_flag(m, "Scrambling Supported", scdc->scrambling.supported);
+	scdc_print_flag(m, "Low Rate Scrambling Supported", scdc->scrambling.low_rates);
+
+	ret = drm_scdc_read_state(connector, st);
+	drm_connector_put(connector);
+	if (ret)
+		return ret;
+
+	scdc_print_flag(m, "Scrambling Enabled", st->scrambling_enabled);
+	scdc_print_flag(m, "Scrambling Detected", st->scrambling_detected);
+
+	if (st->tmds_bclk_x40)
+		scdc_print_str(m, "TMDS Bit Clock Ratio", "1/40");
+	else
+		scdc_print_str(m, "TMDS Bit Clock Ratio", "1/10");
+
+	scdc_print_flag(m, "Clock Detected", st->stf.status0.flags.clock_detected);
+	scdc_print_flag(m, "Channel 0 Locked", st->stf.status0.flags.ch0_locked);
+	scdc_print_flag(m, "Channel 1 Locked", st->stf.status0.flags.ch1_locked);
+	scdc_print_flag(m, "Channel 2 Locked", st->stf.status0.flags.ch2_locked);
+
+	scdc_print_dec(m, "Channel 0 Errors", st->error_count[0]);
+	scdc_print_dec(m, "Channel 1 Errors", st->error_count[1]);
+	scdc_print_dec(m, "Channel 2 Errors", st->error_count[2]);
+
+	return 0;
+
+err_conn_put:
+	drm_connector_put(connector);
+
+	return ret;
+}
+DEFINE_SHOW_ATTRIBUTE(scdc_status);
+
+/**
+ * drm_scdc_debugfs_init - Initialize scdc files in connector debugfs
+ * @connector: pointer to &struct drm_connector to operate on
+ * @root: debugfs &struct dentry for the debugfs root of @connector
+ *
+ * Creates SCDC-related debugfs files for @connector. Must be called after
+ * @root is already created.
+ */
+void drm_scdc_debugfs_init(struct drm_connector *connector, struct dentry *root)
+{
+	struct scdc_debugfs_priv *priv;
+
+	if (!root || !connector)
+		return;
+
+	priv = drmm_kzalloc(connector->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return;
+
+	priv->connector = connector;
+
+	debugfs_create_file("scdc_status", 0444, root, priv, &scdc_status_fops);
+}
+EXPORT_SYMBOL(drm_scdc_debugfs_init);
diff --git a/include/drm/display/drm_scdc_helper.h b/include/drm/display/drm_scdc_helper.h
index e9ccaeba56dd..c066c1158275 100644
--- a/include/drm/display/drm_scdc_helper.h
+++ b/include/drm/display/drm_scdc_helper.h
@@ -30,6 +30,55 @@
 
 struct drm_connector;
 struct i2c_adapter;
+struct dentry;
+
+struct drm_scdc_status_flags {
+	/* Status Register 0 */
+	union {
+		struct {
+			bool clock_detected : 1;
+			bool ch0_locked : 1;
+			bool ch1_locked : 1;
+			bool ch2_locked : 1;
+			bool rsvd_4 : 1;
+			bool rsvd_5 : 1;
+			bool rsvd_6 : 1;
+			bool rsvd_7 : 1;
+		} flags __packed;
+		u8 data;
+	} status0;
+
+	/* Status Register 1 */
+	union {
+		struct {
+			bool rsvd_0 : 1;
+			bool rsvd_1 : 1;
+			bool rsvd_2 : 1;
+			bool rsvd_3 : 1;
+			bool rsvd_4 : 1;
+			bool rsvd_5 : 1;
+			bool rsvd_6 : 1;
+			bool rsvd_7 : 1;
+		} flags __packed;
+		u8 data;
+	} status1;
+};
+
+struct drm_scdc_state {
+	/** @stf: contents of the status flag registers */
+	struct drm_scdc_status_flags stf;
+	/** @scramling_enabled: true if TMDS scrambling is on */
+	bool scrambling_enabled;
+	/** @scrambling_detected: true if the sink actually detected scrambling */
+	bool scrambling_detected;
+	/**
+	 * @tmds_bclk_x40: true if TMDS bit period is 1/40th of the TMDS
+	 * clock period, false if it's 1/10th of the clock period.
+	 */
+	bool tmds_bclk_x40;
+	/** @error_count: character error counts for each channel */
+	u16 error_count[3];
+};
 
 int drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
 		  size_t size);
@@ -77,4 +126,11 @@ bool drm_scdc_get_scrambling_status(struct drm_connector *connector);
 bool drm_scdc_set_scrambling(struct drm_connector *connector, bool enable);
 bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector, bool set);
 
+int drm_scdc_read_status0_flags(struct drm_connector *connector,
+				struct drm_scdc_status_flags *flags);
+int drm_scdc_read_state(struct drm_connector *connector,
+			struct drm_scdc_state *state);
+int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3]);
+void drm_scdc_debugfs_init(struct drm_connector *connector, struct dentry *root);
+
 #endif

-- 
2.54.0

[PATCH v3 3/4] drm/display: bridge_connector: init scdc debugfs for HDMI

[Nicolas Frattaroli]

On drm_bridge_connectors that contain an HDMI bridge, initialise the
SCDC debugfs entry under the connector's debugfs root.

Reviewed-by: Daniel Stone <daniels@collabora.com>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/display/drm_bridge_connector.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 649969fca141..8c2c890bd95d 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -25,6 +25,7 @@
 #include <drm/display/drm_hdmi_cec_helper.h>
 #include <drm/display/drm_hdmi_helper.h>
 #include <drm/display/drm_hdmi_state_helper.h>
+#include <drm/display/drm_scdc_helper.h>
 
 /**
  * DOC: overview
@@ -263,6 +264,9 @@ static void drm_bridge_connector_debugfs_init(struct drm_connector *connector,
 		if (bridge->funcs->debugfs_init)
 			bridge->funcs->debugfs_init(bridge, root);
 	}
+
+	if (bridge_connector->bridge_hdmi)
+		drm_scdc_debugfs_init(connector, root);
 }
 
 static void drm_bridge_connector_reset(struct drm_connector *connector)

-- 
2.54.0

[PATCH v3 4/4] drm/scdc-helper: Implement parsing and printing HDMI 2.1 fields

[Nicolas Frattaroli]

HDMI 2.1 redefines previously reserved fields in SCDC for various new
uses. No version check needs to be performed, as an HDMI 2.0 sink's
reserved SCDC fields are well-defined to be 0, and any zero-ness of
these fields for an HDMI 2.0 sink is not a surprise for SCDC parsers for
HDMI 2.1.

Implement reading and outputting these fields over debugfs.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/display/drm_scdc_helper.c | 154 ++++++++++++++++++++++++++----
 include/drm/display/drm_scdc.h            |   9 ++
 include/drm/display/drm_scdc_helper.h     |  85 ++++++++++++++---
 3 files changed, 218 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
index 75a59c6fc7a5..b5a866372b0f 100644
--- a/drivers/gpu/drm/display/drm_scdc_helper.c
+++ b/drivers/gpu/drm/display/drm_scdc_helper.c
@@ -21,6 +21,7 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
+#include <linux/bitfield.h>
 #include <linux/export.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
@@ -63,6 +64,38 @@ struct scdc_debugfs_priv {
 	struct drm_scdc_state state;
 };
 
+static __pure const char *drm_scdc_frl_rate_str(enum drm_scdc_frl_rate rate)
+{
+	switch (rate) {
+	case SCDC_FRL_RATE_OFF:
+		return "Off";
+	case SCDC_FRL_RATE_3X3:
+		return "3 Gbit/s x 3 lanes";
+	case SCDC_FRL_RATE_6X3:
+		return "6 Gbit/s x 3 lanes";
+	case SCDC_FRL_RATE_6X4:
+		return "6 Gbit/s x 4 lanes";
+	case SCDC_FRL_RATE_8X4:
+		return "8 Gbit/s x 4 lanes";
+	case SCDC_FRL_RATE_10X4:
+		return "10 Gbit/s x 4 lanes";
+	case SCDC_FRL_RATE_12X4:
+		return "12 Gbit/s x 4 lanes";
+	case SCDC_FRL_RATE_RESV_7:
+	case SCDC_FRL_RATE_RESV_8:
+	case SCDC_FRL_RATE_RESV_9:
+	case SCDC_FRL_RATE_RESV_10:
+	case SCDC_FRL_RATE_RESV_11:
+	case SCDC_FRL_RATE_RESV_12:
+	case SCDC_FRL_RATE_RESV_13:
+	case SCDC_FRL_RATE_RESV_14:
+	case SCDC_FRL_RATE_RESV_15:
+		return "(Reserved)";
+	default:
+		return NULL;
+	}
+}
+
 /**
  * drm_scdc_read - read a block of data from SCDC
  * @adapter: I2C controller
@@ -309,10 +342,57 @@ int drm_scdc_read_status0_flags(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_scdc_read_status0_flags);
 
+/**
+ * drm_scdc_read_status1_2_flags - Read SCDC "Status Flags" 1 and 2 Registers
+ * @connector: pointer to &struct drm_connector to issue the scdc request on
+ * @flags: pointer to the caller's &struct drm_scdc_status_flags to output to
+ *
+ * Reads the SCDC Status Flags 1 and 2 registers, and outputs their contents to
+ * the destination @flags. Contents of @flags.status1 and @flags.status2 are
+ * only valid if function returns 0.
+ *
+ * Returns 0 on success, negative errno on error.
+ */
+int drm_scdc_read_status1_2_flags(struct drm_connector *connector,
+				  struct drm_scdc_status_flags *flags)
+{
+	int ret;
+
+	ret = drm_scdc_writeb(connector->ddc, SCDC_UPDATE_0, SCDC_FLT_UPDATE);
+	if (ret)
+		return ret;
+
+	ret = drm_scdc_readb(connector->ddc, SCDC_STATUS_FLAGS_1, &flags->status1.data);
+	if (ret)
+		return ret;
+
+	return drm_scdc_readb(connector->ddc, SCDC_STATUS_FLAGS_2, &flags->status2.data);
+}
+EXPORT_SYMBOL(drm_scdc_read_status1_2_flags);
+
+#define ERR_DET_OFF(x) ((x) - SCDC_ERR_DET_0_L)
+
+static int __pure scdc_err_cnt_buf_idx(unsigned int lane)
+{
+	switch (lane) {
+	case 0:
+		return ERR_DET_OFF(SCDC_ERR_DET_0_L);
+	case 1:
+		return ERR_DET_OFF(SCDC_ERR_DET_1_L);
+	case 2:
+		return ERR_DET_OFF(SCDC_ERR_DET_2_L);
+	case 3:
+		return ERR_DET_OFF(SCDC_ERR_DET_3_L);
+	default:
+		return -EINVAL;
+	}
+}
+
 /**
  * drm_scdc_read_error_counters - Read and clear SCDC error counters
  * @connector: pointer to &struct drm_connector to issue the scdc request on
- * @counter: Caller's u16 array with 3 elements to write the counter values into
+ * @counter: Caller's u16 array with 4 elements to write the counter values into
+ * @num_lanes: number of active lanes, either 3 or 4
  *
  * Read the SCDC channel error counters. If the count of channel *n* is valid,
  * write it into counter[n]. Otherwise, set counter[n] to 0. Reads all counters
@@ -320,18 +400,31 @@ EXPORT_SYMBOL(drm_scdc_read_status0_flags);
  *
  * Returns: %0 on success, negative errno on error.
  */
-int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3])
+int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[4],
+				 unsigned int num_lanes)
 {
-	u8 buf[7] = { 0 };
+	unsigned int buf_sz;
+	u8 buf[9] = { 0 };
 	int ret;
 	u8 sum = 0;
-	int i;
+	int i, idx;
+
+	switch (num_lanes) {
+	case 3:
+		buf_sz = 7;
+		break;
+	case 4:
+		buf_sz = 9;
+		break;
+	default:
+		return -EINVAL;
+	}
 
 	ret = drm_scdc_writeb(connector->ddc, SCDC_UPDATE_0, SCDC_CED_UPDATE);
 	if (ret)
 		return ret;
 
-	ret = drm_scdc_read(connector->ddc, SCDC_ERR_DET_0_L, buf, ARRAY_SIZE(buf));
+	ret = drm_scdc_read(connector->ddc, SCDC_ERR_DET_0_L, buf, buf_sz);
 	if (ret)
 		return ret;
 
@@ -339,24 +432,23 @@ int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3]
 	 * Verify the "checksum", i.e. sum up everything including the checksum
 	 * register as a wrapping unsigned 8-bit addition and verify it's 0.
 	 */
-	for (i = 0; i < ARRAY_SIZE(buf); i++)
+	for (i = 0; i < buf_sz; i++)
 		sum = wrapping_add(u8, sum, buf[i]);
 
 	if (sum)
 		return -EPROTO;
 
-	for (i = 0; i < ARRAY_SIZE(buf) - 1; i += 2) {
-		if (buf[i + 1] & SCDC_CHANNEL_VALID)
-			counter[i / 2] = buf[i] | (buf[i + 1] & ~SCDC_CHANNEL_VALID) << 8;
+	for (i = 0; i < num_lanes; i++) {
+		idx = scdc_err_cnt_buf_idx(i);
+		if (buf[idx + 1] & SCDC_CHANNEL_VALID)
+			counter[i] = buf[idx] | (buf[idx + 1] & ~SCDC_CHANNEL_VALID) << 8;
 		else
-			counter[i / 2] = 0;
-
-		buf[i] = 0;
-		buf[i + 1] = 0;
+			counter[i] = 0;
 	}
-	buf[ARRAY_SIZE(buf) - 1] = 0;
 
-	return drm_scdc_write(connector->ddc, SCDC_ERR_DET_0_L, buf, ARRAY_SIZE(buf));
+	memset(buf, 0, buf_sz);
+
+	return drm_scdc_write(connector->ddc, SCDC_ERR_DET_0_L, buf, buf_sz);
 }
 EXPORT_SYMBOL(drm_scdc_read_error_counters);
 
@@ -376,6 +468,7 @@ int drm_scdc_read_state(struct drm_connector *connector, struct drm_scdc_state *
 	u8 upd_flags[2] = { 0 };
 	struct i2c_adapter *ddc;
 	struct drm_scdc *scdc;
+	int num_lanes;
 	int ret;
 	u8 val;
 
@@ -397,6 +490,19 @@ int drm_scdc_read_state(struct drm_connector *connector, struct drm_scdc_state *
 
 	state->scrambling_detected = drm_scdc_get_scrambling_status(connector);
 
+	ret = drm_scdc_readb(ddc, SCDC_CONFIG_1, &val);
+	if (ret)
+		return ret;
+
+	state->rate = FIELD_GET(SCDC_FRL_RATE, val);
+	num_lanes = drm_scdc_num_frl_lanes(state->rate);
+	if (num_lanes < 0)
+		return num_lanes;
+	if (!num_lanes)
+		num_lanes = 3;
+
+	state->ffe_levels = FIELD_GET(SCDC_FFE_LEVELS, val);
+
 	ret = drm_scdc_read(ddc, SCDC_UPDATE_0, &upd_flags, sizeof(upd_flags));
 	if (ret)
 		return ret;
@@ -407,8 +513,15 @@ int drm_scdc_read_state(struct drm_connector *connector, struct drm_scdc_state *
 			return ret;
 	}
 
+	if (upd_flags[0] & SCDC_FLT_UPDATE) {
+		ret = drm_scdc_read_status1_2_flags(connector, &state->stf);
+		if (ret)
+			return ret;
+	}
+
 	if (upd_flags[0] & SCDC_CED_UPDATE) {
-		ret = drm_scdc_read_error_counters(connector, state->error_count);
+		ret = drm_scdc_read_error_counters(connector, state->error_count,
+						   num_lanes);
 		if (ret)
 			return ret;
 	}
@@ -456,6 +569,8 @@ static int scdc_status_show(struct seq_file *m, void *data)
 
 	scdc_print_flag(m, "Scrambling Enabled", st->scrambling_enabled);
 	scdc_print_flag(m, "Scrambling Detected", st->scrambling_detected);
+	scdc_print_str(m, "FRL Rate", drm_scdc_frl_rate_str(st->rate));
+	scdc_print_dec(m, "FFE Levels", st->ffe_levels);
 
 	if (st->tmds_bclk_x40)
 		scdc_print_str(m, "TMDS Bit Clock Ratio", "1/40");
@@ -466,10 +581,17 @@ static int scdc_status_show(struct seq_file *m, void *data)
 	scdc_print_flag(m, "Channel 0 Locked", st->stf.status0.flags.ch0_locked);
 	scdc_print_flag(m, "Channel 1 Locked", st->stf.status0.flags.ch1_locked);
 	scdc_print_flag(m, "Channel 2 Locked", st->stf.status0.flags.ch2_locked);
+	if (drm_scdc_num_frl_lanes(st->rate) == 4)
+		scdc_print_flag(m, "Lane 3 Locked", st->stf.status0.flags.ln3_locked);
+
+	scdc_print_flag(m, "Sink Ready For Link Training", st->stf.status0.flags.flt_ready);
+	scdc_print_flag(m, "Sink Failed To Decode DSC", st->stf.status0.flags.dsc_fail);
 
 	scdc_print_dec(m, "Channel 0 Errors", st->error_count[0]);
 	scdc_print_dec(m, "Channel 1 Errors", st->error_count[1]);
 	scdc_print_dec(m, "Channel 2 Errors", st->error_count[2]);
+	if (drm_scdc_num_frl_lanes(st->rate) == 4)
+		scdc_print_dec(m, "Lane 3 Errors", st->error_count[3]);
 
 	return 0;
 
diff --git a/include/drm/display/drm_scdc.h b/include/drm/display/drm_scdc.h
index 3d58f37e8ed8..56cd909edac9 100644
--- a/include/drm/display/drm_scdc.h
+++ b/include/drm/display/drm_scdc.h
@@ -29,6 +29,7 @@
 #define SCDC_SOURCE_VERSION 0x02
 
 #define SCDC_UPDATE_0 0x10
+#define  SCDC_FLT_UPDATE (1 << 5)
 #define  SCDC_READ_REQUEST_TEST (1 << 2)
 #define  SCDC_CED_UPDATE (1 << 1)
 #define  SCDC_STATUS_UPDATE (1 << 0)
@@ -46,6 +47,10 @@
 #define SCDC_CONFIG_0 0x30
 #define  SCDC_READ_REQUEST_ENABLE (1 << 0)
 
+#define SCDC_CONFIG_1 0x31
+#define  SCDC_FRL_RATE   0x0f
+#define  SCDC_FFE_LEVELS 0xf0
+
 #define SCDC_STATUS_FLAGS_0 0x40
 #define  SCDC_CH2_LOCK (1 << 3)
 #define  SCDC_CH1_LOCK (1 << 2)
@@ -54,6 +59,7 @@
 #define  SCDC_CLOCK_DETECT (1 << 0)
 
 #define SCDC_STATUS_FLAGS_1 0x41
+#define SCDC_STATUS_FLAGS_2 0x42
 
 #define SCDC_ERR_DET_0_L 0x50
 #define SCDC_ERR_DET_0_H 0x51
@@ -65,6 +71,9 @@
 
 #define SCDC_ERR_DET_CHECKSUM 0x56
 
+#define SCDC_ERR_DET_3_L 0x57
+#define SCDC_ERR_DET_3_H 0x58
+
 #define SCDC_TEST_CONFIG_0 0xc0
 #define  SCDC_TEST_READ_REQUEST (1 << 7)
 #define  SCDC_TEST_READ_REQUEST_DELAY(x) ((x) & 0x7f)
diff --git a/include/drm/display/drm_scdc_helper.h b/include/drm/display/drm_scdc_helper.h
index c066c1158275..a1b73f0be73f 100644
--- a/include/drm/display/drm_scdc_helper.h
+++ b/include/drm/display/drm_scdc_helper.h
@@ -24,6 +24,7 @@
 #ifndef DRM_SCDC_HELPER_H
 #define DRM_SCDC_HELPER_H
 
+#include <linux/errno.h>
 #include <linux/types.h>
 
 #include <drm/display/drm_scdc.h>
@@ -40,10 +41,10 @@ struct drm_scdc_status_flags {
 			bool ch0_locked : 1;
 			bool ch1_locked : 1;
 			bool ch2_locked : 1;
-			bool rsvd_4 : 1;
+			bool ln3_locked : 1;
 			bool rsvd_5 : 1;
-			bool rsvd_6 : 1;
-			bool rsvd_7 : 1;
+			bool flt_ready : 1;
+			bool dsc_fail : 1;
 		} flags __packed;
 		u8 data;
 	} status0;
@@ -51,19 +52,68 @@ struct drm_scdc_status_flags {
 	/* Status Register 1 */
 	union {
 		struct {
-			bool rsvd_0 : 1;
-			bool rsvd_1 : 1;
-			bool rsvd_2 : 1;
-			bool rsvd_3 : 1;
-			bool rsvd_4 : 1;
-			bool rsvd_5 : 1;
-			bool rsvd_6 : 1;
-			bool rsvd_7 : 1;
+			u8 ln0_training_pattern : 4;
+			u8 ln1_training_pattern : 4;
 		} flags __packed;
 		u8 data;
 	} status1;
+
+	/* Status Register 2 */
+	union {
+		struct {
+			u8 ln2_training_pattern : 4;
+			u8 ln3_training_pattern : 4;
+		} flags __packed;
+		u8 data;
+	} status2;
+};
+
+enum drm_scdc_frl_rate {
+	SCDC_FRL_RATE_OFF = 0,
+	SCDC_FRL_RATE_3X3 = 1,
+	SCDC_FRL_RATE_6X3 = 2,
+	SCDC_FRL_RATE_6X4 = 3,
+	SCDC_FRL_RATE_8X4 = 4,
+	SCDC_FRL_RATE_10X4 = 5,
+	SCDC_FRL_RATE_12X4 = 6,
+	SCDC_FRL_RATE_RESV_7 = 7,
+	SCDC_FRL_RATE_RESV_8 = 8,
+	SCDC_FRL_RATE_RESV_9 = 9,
+	SCDC_FRL_RATE_RESV_10 = 10,
+	SCDC_FRL_RATE_RESV_11 = 11,
+	SCDC_FRL_RATE_RESV_12 = 12,
+	SCDC_FRL_RATE_RESV_13 = 13,
+	SCDC_FRL_RATE_RESV_14 = 14,
+	SCDC_FRL_RATE_RESV_15 = 15
 };
 
+/**
+ * drm_scdc_num_frl_lanes - get number of lanes for a given FRL rate
+ * @rate: one of &enum drm_scdc_frl_rate
+ *
+ * For a given @rate, return the number of lanes it uses.
+ *
+ * Returns: %-EINVAL if @rate is not a valid FRL rate, or the number of lanes
+ * for a given &enum drm_scdc_frl_rate on success (including %0 for "off")
+ */
+static inline __pure int drm_scdc_num_frl_lanes(enum drm_scdc_frl_rate rate)
+{
+	switch (rate) {
+	case SCDC_FRL_RATE_OFF:
+		return 0;
+	case SCDC_FRL_RATE_3X3:
+	case SCDC_FRL_RATE_6X3:
+		return 3;
+	case SCDC_FRL_RATE_6X4:
+	case SCDC_FRL_RATE_8X4:
+	case SCDC_FRL_RATE_10X4:
+	case SCDC_FRL_RATE_12X4:
+		return 4;
+	default:
+		return -EINVAL;
+	}
+}
+
 struct drm_scdc_state {
 	/** @stf: contents of the status flag registers */
 	struct drm_scdc_status_flags stf;
@@ -76,8 +126,12 @@ struct drm_scdc_state {
 	 * clock period, false if it's 1/10th of the clock period.
 	 */
 	bool tmds_bclk_x40;
-	/** @error_count: character error counts for each channel */
-	u16 error_count[3];
+	/** @rate: FRL rate set by the source */
+	enum drm_scdc_frl_rate rate : 4;
+	/** @ffe_levels: The FFE levels for @rate set by the source */
+	u8 ffe_levels : 4;
+	/** @error_count: character error counts for each channel/link */
+	u16 error_count[4];
 };
 
 int drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
@@ -128,9 +182,12 @@ bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector, bool se
 
 int drm_scdc_read_status0_flags(struct drm_connector *connector,
 				struct drm_scdc_status_flags *flags);
+int drm_scdc_read_status1_2_flags(struct drm_connector *connector,
+				  struct drm_scdc_status_flags *flags);
 int drm_scdc_read_state(struct drm_connector *connector,
 			struct drm_scdc_state *state);
-int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3]);
+int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[4],
+				 unsigned int num_lanes);
 void drm_scdc_debugfs_init(struct drm_connector *connector, struct dentry *root);
 
 #endif

-- 
2.54.0

Re: [PATCH v3 2/4] drm/scdc-helper: Add scdc_status debugfs entry

[Jani Nikula]

On Tue, 26 May 2026, Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:
> SCDC provides status information on the current display link. At the
> very least, it may be useful to expose this info through debugfs.
>
> Add a debugfs entry for it under the connector, which displays a few
> more details parsed out of the SCDC registers. A new
> drm_scdc_debugfs_init function can be called by the connector
> implementation to initialise the debugfs file.
>
> Reviewed-by: Daniel Stone <daniels@collabora.com>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_scdc_helper.c | 228 ++++++++++++++++++++++++++++++
>  include/drm/display/drm_scdc_helper.h     |  56 ++++++++
>  2 files changed, 284 insertions(+)
>
> diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
> index 8403f2390ab6..75a59c6fc7a5 100644
> --- a/drivers/gpu/drm/display/drm_scdc_helper.c
> +++ b/drivers/gpu/drm/display/drm_scdc_helper.c
> @@ -24,11 +24,14 @@
>  #include <linux/export.h>
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
> +#include <linux/debugfs.h>
>  #include <linux/delay.h>
> +#include <linux/overflow.h>
>  
>  #include <drm/display/drm_scdc_helper.h>
>  #include <drm/drm_connector.h>
>  #include <drm/drm_device.h>
> +#include <drm/drm_managed.h>
>  #include <drm/drm_print.h>
>  
>  /**
> @@ -55,6 +58,11 @@
>  
>  #define SCDC_I2C_SLAVE_ADDRESS 0x54
>  
> +struct scdc_debugfs_priv {
> +	struct drm_connector *connector;
> +	struct drm_scdc_state state;
> +};
> +
>  /**
>   * drm_scdc_read - read a block of data from SCDC
>   * @adapter: I2C controller
> @@ -276,3 +284,223 @@ bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector,
>  	return true;
>  }
>  EXPORT_SYMBOL(drm_scdc_set_high_tmds_clock_ratio);
> +
> +/**
> + * drm_scdc_read_status0_flags - Read SCDC "Status Flags" Register
> + * @connector: pointer to &struct drm_connector to issue the scdc request on
> + * @flags: pointer to the caller's &struct drm_scdc_status_flags to output to
> + *
> + * Reads the SCDC Status Flags 0 register, and outputs its contents to the
> + * destination @flags. Contents of @flags.status0 is only valid if function
> + * returns 0.
> + *
> + * Returns: %0 on success, negative errno on error.
> + */
> +int drm_scdc_read_status0_flags(struct drm_connector *connector,
> +				struct drm_scdc_status_flags *flags)
> +{
> +	int ret;
> +
> +	ret = drm_scdc_writeb(connector->ddc, SCDC_UPDATE_0, SCDC_STATUS_UPDATE);
> +	if (ret)
> +		return ret;
> +
> +	return drm_scdc_readb(connector->ddc, SCDC_STATUS_FLAGS_0, &flags->status0.data);
> +}
> +EXPORT_SYMBOL(drm_scdc_read_status0_flags);
> +
> +/**
> + * drm_scdc_read_error_counters - Read and clear SCDC error counters
> + * @connector: pointer to &struct drm_connector to issue the scdc request on
> + * @counter: Caller's u16 array with 3 elements to write the counter values into
> + *
> + * Read the SCDC channel error counters. If the count of channel *n* is valid,
> + * write it into counter[n]. Otherwise, set counter[n] to 0. Reads all counters
> + * in one read chunk, then clears every counter, as is mandated.
> + *
> + * Returns: %0 on success, negative errno on error.
> + */
> +int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3])
> +{
> +	u8 buf[7] = { 0 };
> +	int ret;
> +	u8 sum = 0;
> +	int i;
> +
> +	ret = drm_scdc_writeb(connector->ddc, SCDC_UPDATE_0, SCDC_CED_UPDATE);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_scdc_read(connector->ddc, SCDC_ERR_DET_0_L, buf, ARRAY_SIZE(buf));
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Verify the "checksum", i.e. sum up everything including the checksum
> +	 * register as a wrapping unsigned 8-bit addition and verify it's 0.
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(buf); i++)
> +		sum = wrapping_add(u8, sum, buf[i]);
> +
> +	if (sum)
> +		return -EPROTO;
> +
> +	for (i = 0; i < ARRAY_SIZE(buf) - 1; i += 2) {
> +		if (buf[i + 1] & SCDC_CHANNEL_VALID)
> +			counter[i / 2] = buf[i] | (buf[i + 1] & ~SCDC_CHANNEL_VALID) << 8;
> +		else
> +			counter[i / 2] = 0;
> +
> +		buf[i] = 0;
> +		buf[i + 1] = 0;
> +	}
> +	buf[ARRAY_SIZE(buf) - 1] = 0;
> +
> +	return drm_scdc_write(connector->ddc, SCDC_ERR_DET_0_L, buf, ARRAY_SIZE(buf));
> +}
> +EXPORT_SYMBOL(drm_scdc_read_error_counters);
> +
> +/**
> + * drm_scdc_read_state - Update state from SCDC
> + * @connector: pointer to a &struct drm_connector on which to operate on
> + * @state: pointer to a &struct drm_scdc_state to fill
> + *
> + * Reads update flags from SCDC, and updates the parts of @state that SCDC
> + * claims have changed, as well as populating those where such a distinction
> + * can't be made.
> + *
> + * Returns: %0 on success, negative errno on failure.
> + */
> +int drm_scdc_read_state(struct drm_connector *connector, struct drm_scdc_state *state)
> +{
> +	u8 upd_flags[2] = { 0 };

IMO just {} is better for the zero initializer when you're not doing
full initialization anyway.

> +	struct i2c_adapter *ddc;
> +	struct drm_scdc *scdc;
> +	int ret;
> +	u8 val;
> +
> +	if (!state || !connector)
> +		return -ENODEV;
> +
> +	scdc = &connector->display_info.hdmi.scdc;
> +	ddc = connector->ddc;
> +
> +	if (!scdc->supported)
> +		return -EOPNOTSUPP;
> +
> +	ret = drm_scdc_readb(ddc, SCDC_TMDS_CONFIG, &val);
> +	if (ret)
> +		return ret;
> +
> +	state->scrambling_enabled = !!(val & SCDC_SCRAMBLING_ENABLE);
> +	state->tmds_bclk_x40 = !!(val & SCDC_TMDS_BIT_CLOCK_RATIO_BY_40);

The !! and parenthesis are superfluous.

> +
> +	state->scrambling_detected = drm_scdc_get_scrambling_status(connector);
> +
> +	ret = drm_scdc_read(ddc, SCDC_UPDATE_0, &upd_flags, sizeof(upd_flags));
> +	if (ret)
> +		return ret;
> +
> +	if (upd_flags[0] & SCDC_STATUS_UPDATE) {
> +		ret = drm_scdc_read_status0_flags(connector, &state->stf);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (upd_flags[0] & SCDC_CED_UPDATE) {
> +		ret = drm_scdc_read_error_counters(connector, state->error_count);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_scdc_read_state);
> +
> +#define scdc_print_str(_f, key, s) \
> +	(seq_printf((_f), "%-30s: %s\n", (key), (s)))
> +#define scdc_print_flag(_f, key, val) \
> +	(scdc_print_str((_f), (key), str_yes_no((val))))
> +#define scdc_print_dec(_f, key, val) \
> +	(seq_printf((_f), "%-30s: %d\n", (key), (val)))
> +
> +static int scdc_status_show(struct seq_file *m, void *data)
> +{
> +	struct scdc_debugfs_priv *priv = m->private;
> +	struct drm_scdc_state *st = &priv->state;
> +	struct drm_connector *connector = priv->connector;
> +	struct drm_scdc *scdc = &connector->display_info.hdmi.scdc;
> +	int ret;
> +
> +	drm_connector_get(connector);
> +
> +	if (connector->status != connector_status_connected) {
> +		ret = -ENODEV;
> +		goto err_conn_put;
> +	}
> +
> +	scdc_print_flag(m, "SCDC Supported", scdc->supported);
> +	if (!scdc->supported) {
> +		ret = 0;
> +		goto err_conn_put;
> +	}
> +
> +	scdc_print_flag(m, "Sink Read Request Capable", scdc->read_request);
> +	scdc_print_flag(m, "Scrambling Supported", scdc->scrambling.supported);
> +	scdc_print_flag(m, "Low Rate Scrambling Supported", scdc->scrambling.low_rates);

I honestly think it would be more clear to *not* define these
scdc_print_* macros to hide what's actually being printed.

I'm also not sure the %-30s format width is all that great.

> +
> +	ret = drm_scdc_read_state(connector, st);
> +	drm_connector_put(connector);
> +	if (ret)
> +		return ret;
> +
> +	scdc_print_flag(m, "Scrambling Enabled", st->scrambling_enabled);
> +	scdc_print_flag(m, "Scrambling Detected", st->scrambling_detected);
> +
> +	if (st->tmds_bclk_x40)
> +		scdc_print_str(m, "TMDS Bit Clock Ratio", "1/40");
> +	else
> +		scdc_print_str(m, "TMDS Bit Clock Ratio", "1/10");
> +
> +	scdc_print_flag(m, "Clock Detected", st->stf.status0.flags.clock_detected);
> +	scdc_print_flag(m, "Channel 0 Locked", st->stf.status0.flags.ch0_locked);
> +	scdc_print_flag(m, "Channel 1 Locked", st->stf.status0.flags.ch1_locked);
> +	scdc_print_flag(m, "Channel 2 Locked", st->stf.status0.flags.ch2_locked);
> +
> +	scdc_print_dec(m, "Channel 0 Errors", st->error_count[0]);
> +	scdc_print_dec(m, "Channel 1 Errors", st->error_count[1]);
> +	scdc_print_dec(m, "Channel 2 Errors", st->error_count[2]);
> +
> +	return 0;
> +
> +err_conn_put:
> +	drm_connector_put(connector);
> +
> +	return ret;
> +}
> +DEFINE_SHOW_ATTRIBUTE(scdc_status);
> +
> +/**
> + * drm_scdc_debugfs_init - Initialize scdc files in connector debugfs
> + * @connector: pointer to &struct drm_connector to operate on
> + * @root: debugfs &struct dentry for the debugfs root of @connector
> + *
> + * Creates SCDC-related debugfs files for @connector. Must be called after
> + * @root is already created.
> + */
> +void drm_scdc_debugfs_init(struct drm_connector *connector, struct dentry *root)
> +{
> +	struct scdc_debugfs_priv *priv;
> +
> +	if (!root || !connector)
> +		return;
> +
> +	priv = drmm_kzalloc(connector->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return;
> +
> +	priv->connector = connector;
> +
> +	debugfs_create_file("scdc_status", 0444, root, priv, &scdc_status_fops);
> +}
> +EXPORT_SYMBOL(drm_scdc_debugfs_init);
> diff --git a/include/drm/display/drm_scdc_helper.h b/include/drm/display/drm_scdc_helper.h
> index e9ccaeba56dd..c066c1158275 100644
> --- a/include/drm/display/drm_scdc_helper.h
> +++ b/include/drm/display/drm_scdc_helper.h
> @@ -30,6 +30,55 @@
>  
>  struct drm_connector;
>  struct i2c_adapter;
> +struct dentry;
> +
> +struct drm_scdc_status_flags {
> +	/* Status Register 0 */
> +	union {
> +		struct {
> +			bool clock_detected : 1;
> +			bool ch0_locked : 1;
> +			bool ch1_locked : 1;
> +			bool ch2_locked : 1;
> +			bool rsvd_4 : 1;
> +			bool rsvd_5 : 1;
> +			bool rsvd_6 : 1;
> +			bool rsvd_7 : 1;
> +		} flags __packed;
> +		u8 data;
> +	} status0;
> +
> +	/* Status Register 1 */
> +	union {
> +		struct {
> +			bool rsvd_0 : 1;
> +			bool rsvd_1 : 1;
> +			bool rsvd_2 : 1;
> +			bool rsvd_3 : 1;
> +			bool rsvd_4 : 1;
> +			bool rsvd_5 : 1;
> +			bool rsvd_6 : 1;
> +			bool rsvd_7 : 1;
> +		} flags __packed;
> +		u8 data;
> +	} status1;
> +};

The bit order of bitfields is implementation defined. You can't portably
use them for parsing registers.

Some drivers (like i915) do this, but they rely on the little-endian
GCC/Clang behaviour, which can't be universally relied on in generic DRM
code.

I think you're better off defining macros for this; see the DPCD code.

> +
> +struct drm_scdc_state {
> +	/** @stf: contents of the status flag registers */
> +	struct drm_scdc_status_flags stf;
> +	/** @scramling_enabled: true if TMDS scrambling is on */
> +	bool scrambling_enabled;
> +	/** @scrambling_detected: true if the sink actually detected scrambling */
> +	bool scrambling_detected;
> +	/**
> +	 * @tmds_bclk_x40: true if TMDS bit period is 1/40th of the TMDS
> +	 * clock period, false if it's 1/10th of the clock period.
> +	 */
> +	bool tmds_bclk_x40;
> +	/** @error_count: character error counts for each channel */
> +	u16 error_count[3];
> +};
>  
>  int drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
>  		  size_t size);
> @@ -77,4 +126,11 @@ bool drm_scdc_get_scrambling_status(struct drm_connector *connector);
>  bool drm_scdc_set_scrambling(struct drm_connector *connector, bool enable);
>  bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector, bool set);
>  
> +int drm_scdc_read_status0_flags(struct drm_connector *connector,
> +				struct drm_scdc_status_flags *flags);
> +int drm_scdc_read_state(struct drm_connector *connector,
> +			struct drm_scdc_state *state);
> +int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3]);
> +void drm_scdc_debugfs_init(struct drm_connector *connector, struct dentry *root);
> +
>  #endif

-- 
Jani Nikula, Intel

Re: [PATCH v3 4/4] drm/scdc-helper: Implement parsing and printing HDMI 2.1 fields

[Jani Nikula]

On Tue, 26 May 2026, Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:
> HDMI 2.1 redefines previously reserved fields in SCDC for various new
> uses. No version check needs to be performed, as an HDMI 2.0 sink's
> reserved SCDC fields are well-defined to be 0, and any zero-ness of
> these fields for an HDMI 2.0 sink is not a surprise for SCDC parsers for
> HDMI 2.1.
>
> Implement reading and outputting these fields over debugfs.
>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_scdc_helper.c | 154 ++++++++++++++++++++++++++----
>  include/drm/display/drm_scdc.h            |   9 ++
>  include/drm/display/drm_scdc_helper.h     |  85 ++++++++++++++---
>  3 files changed, 218 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
> index 75a59c6fc7a5..b5a866372b0f 100644
> --- a/drivers/gpu/drm/display/drm_scdc_helper.c
> +++ b/drivers/gpu/drm/display/drm_scdc_helper.c
> @@ -21,6 +21,7 @@
>   * DEALINGS IN THE SOFTWARE.
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/export.h>
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
> @@ -63,6 +64,38 @@ struct scdc_debugfs_priv {
>  	struct drm_scdc_state state;
>  };
>  
> +static __pure const char *drm_scdc_frl_rate_str(enum drm_scdc_frl_rate rate)

There are a total of only 71 __pure usages in the kernel. Is this
helpful for a static function where the compiler can figure it out by
itself?

> +{
> +	switch (rate) {
> +	case SCDC_FRL_RATE_OFF:
> +		return "Off";
> +	case SCDC_FRL_RATE_3X3:
> +		return "3 Gbit/s x 3 lanes";
> +	case SCDC_FRL_RATE_6X3:
> +		return "6 Gbit/s x 3 lanes";
> +	case SCDC_FRL_RATE_6X4:
> +		return "6 Gbit/s x 4 lanes";
> +	case SCDC_FRL_RATE_8X4:
> +		return "8 Gbit/s x 4 lanes";
> +	case SCDC_FRL_RATE_10X4:
> +		return "10 Gbit/s x 4 lanes";
> +	case SCDC_FRL_RATE_12X4:
> +		return "12 Gbit/s x 4 lanes";
> +	case SCDC_FRL_RATE_RESV_7:
> +	case SCDC_FRL_RATE_RESV_8:
> +	case SCDC_FRL_RATE_RESV_9:
> +	case SCDC_FRL_RATE_RESV_10:
> +	case SCDC_FRL_RATE_RESV_11:
> +	case SCDC_FRL_RATE_RESV_12:
> +	case SCDC_FRL_RATE_RESV_13:
> +	case SCDC_FRL_RATE_RESV_14:
> +	case SCDC_FRL_RATE_RESV_15:
> +		return "(Reserved)";
> +	default:
> +		return NULL;
> +	}
> +}
> +
>  /**
>   * drm_scdc_read - read a block of data from SCDC
>   * @adapter: I2C controller
> @@ -309,10 +342,57 @@ int drm_scdc_read_status0_flags(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_scdc_read_status0_flags);
>  
> +/**
> + * drm_scdc_read_status1_2_flags - Read SCDC "Status Flags" 1 and 2 Registers
> + * @connector: pointer to &struct drm_connector to issue the scdc request on
> + * @flags: pointer to the caller's &struct drm_scdc_status_flags to output to
> + *
> + * Reads the SCDC Status Flags 1 and 2 registers, and outputs their contents to
> + * the destination @flags. Contents of @flags.status1 and @flags.status2 are
> + * only valid if function returns 0.
> + *
> + * Returns 0 on success, negative errno on error.
> + */
> +int drm_scdc_read_status1_2_flags(struct drm_connector *connector,
> +				  struct drm_scdc_status_flags *flags)
> +{
> +	int ret;
> +
> +	ret = drm_scdc_writeb(connector->ddc, SCDC_UPDATE_0, SCDC_FLT_UPDATE);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_scdc_readb(connector->ddc, SCDC_STATUS_FLAGS_1, &flags->status1.data);
> +	if (ret)
> +		return ret;
> +
> +	return drm_scdc_readb(connector->ddc, SCDC_STATUS_FLAGS_2, &flags->status2.data);
> +}
> +EXPORT_SYMBOL(drm_scdc_read_status1_2_flags);
> +
> +#define ERR_DET_OFF(x) ((x) - SCDC_ERR_DET_0_L)
> +
> +static int __pure scdc_err_cnt_buf_idx(unsigned int lane)
> +{
> +	switch (lane) {
> +	case 0:
> +		return ERR_DET_OFF(SCDC_ERR_DET_0_L);
> +	case 1:
> +		return ERR_DET_OFF(SCDC_ERR_DET_1_L);
> +	case 2:
> +		return ERR_DET_OFF(SCDC_ERR_DET_2_L);
> +	case 3:
> +		return ERR_DET_OFF(SCDC_ERR_DET_3_L);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  /**
>   * drm_scdc_read_error_counters - Read and clear SCDC error counters
>   * @connector: pointer to &struct drm_connector to issue the scdc request on
> - * @counter: Caller's u16 array with 3 elements to write the counter values into
> + * @counter: Caller's u16 array with 4 elements to write the counter values into
> + * @num_lanes: number of active lanes, either 3 or 4
>   *
>   * Read the SCDC channel error counters. If the count of channel *n* is valid,
>   * write it into counter[n]. Otherwise, set counter[n] to 0. Reads all counters
> @@ -320,18 +400,31 @@ EXPORT_SYMBOL(drm_scdc_read_status0_flags);
>   *
>   * Returns: %0 on success, negative errno on error.
>   */
> -int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3])
> +int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[4],
> +				 unsigned int num_lanes)
>  {
> -	u8 buf[7] = { 0 };
> +	unsigned int buf_sz;
> +	u8 buf[9] = { 0 };

I'd use {} for initialization.

>  	int ret;
>  	u8 sum = 0;
> -	int i;
> +	int i, idx;
> +
> +	switch (num_lanes) {
> +	case 3:
> +		buf_sz = 7;
> +		break;
> +	case 4:
> +		buf_sz = 9;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
>  
>  	ret = drm_scdc_writeb(connector->ddc, SCDC_UPDATE_0, SCDC_CED_UPDATE);
>  	if (ret)
>  		return ret;
>  
> -	ret = drm_scdc_read(connector->ddc, SCDC_ERR_DET_0_L, buf, ARRAY_SIZE(buf));
> +	ret = drm_scdc_read(connector->ddc, SCDC_ERR_DET_0_L, buf, buf_sz);
>  	if (ret)
>  		return ret;
>  
> @@ -339,24 +432,23 @@ int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3]
>  	 * Verify the "checksum", i.e. sum up everything including the checksum
>  	 * register as a wrapping unsigned 8-bit addition and verify it's 0.
>  	 */
> -	for (i = 0; i < ARRAY_SIZE(buf); i++)
> +	for (i = 0; i < buf_sz; i++)
>  		sum = wrapping_add(u8, sum, buf[i]);
>  
>  	if (sum)
>  		return -EPROTO;
>  
> -	for (i = 0; i < ARRAY_SIZE(buf) - 1; i += 2) {
> -		if (buf[i + 1] & SCDC_CHANNEL_VALID)
> -			counter[i / 2] = buf[i] | (buf[i + 1] & ~SCDC_CHANNEL_VALID) << 8;
> +	for (i = 0; i < num_lanes; i++) {
> +		idx = scdc_err_cnt_buf_idx(i);
> +		if (buf[idx + 1] & SCDC_CHANNEL_VALID)
> +			counter[i] = buf[idx] | (buf[idx + 1] & ~SCDC_CHANNEL_VALID) << 8;
>  		else
> -			counter[i / 2] = 0;
> -
> -		buf[i] = 0;
> -		buf[i + 1] = 0;
> +			counter[i] = 0;
>  	}
> -	buf[ARRAY_SIZE(buf) - 1] = 0;
>  
> -	return drm_scdc_write(connector->ddc, SCDC_ERR_DET_0_L, buf, ARRAY_SIZE(buf));
> +	memset(buf, 0, buf_sz);
> +
> +	return drm_scdc_write(connector->ddc, SCDC_ERR_DET_0_L, buf, buf_sz);
>  }
>  EXPORT_SYMBOL(drm_scdc_read_error_counters);
>  
> @@ -376,6 +468,7 @@ int drm_scdc_read_state(struct drm_connector *connector, struct drm_scdc_state *
>  	u8 upd_flags[2] = { 0 };
>  	struct i2c_adapter *ddc;
>  	struct drm_scdc *scdc;
> +	int num_lanes;
>  	int ret;
>  	u8 val;
>  
> @@ -397,6 +490,19 @@ int drm_scdc_read_state(struct drm_connector *connector, struct drm_scdc_state *
>  
>  	state->scrambling_detected = drm_scdc_get_scrambling_status(connector);
>  
> +	ret = drm_scdc_readb(ddc, SCDC_CONFIG_1, &val);
> +	if (ret)
> +		return ret;
> +
> +	state->rate = FIELD_GET(SCDC_FRL_RATE, val);
> +	num_lanes = drm_scdc_num_frl_lanes(state->rate);
> +	if (num_lanes < 0)
> +		return num_lanes;
> +	if (!num_lanes)
> +		num_lanes = 3;
> +
> +	state->ffe_levels = FIELD_GET(SCDC_FFE_LEVELS, val);
> +
>  	ret = drm_scdc_read(ddc, SCDC_UPDATE_0, &upd_flags, sizeof(upd_flags));
>  	if (ret)
>  		return ret;
> @@ -407,8 +513,15 @@ int drm_scdc_read_state(struct drm_connector *connector, struct drm_scdc_state *
>  			return ret;
>  	}
>  
> +	if (upd_flags[0] & SCDC_FLT_UPDATE) {
> +		ret = drm_scdc_read_status1_2_flags(connector, &state->stf);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if (upd_flags[0] & SCDC_CED_UPDATE) {
> -		ret = drm_scdc_read_error_counters(connector, state->error_count);
> +		ret = drm_scdc_read_error_counters(connector, state->error_count,
> +						   num_lanes);
>  		if (ret)
>  			return ret;
>  	}
> @@ -456,6 +569,8 @@ static int scdc_status_show(struct seq_file *m, void *data)
>  
>  	scdc_print_flag(m, "Scrambling Enabled", st->scrambling_enabled);
>  	scdc_print_flag(m, "Scrambling Detected", st->scrambling_detected);
> +	scdc_print_str(m, "FRL Rate", drm_scdc_frl_rate_str(st->rate));
> +	scdc_print_dec(m, "FFE Levels", st->ffe_levels);
>  
>  	if (st->tmds_bclk_x40)
>  		scdc_print_str(m, "TMDS Bit Clock Ratio", "1/40");
> @@ -466,10 +581,17 @@ static int scdc_status_show(struct seq_file *m, void *data)
>  	scdc_print_flag(m, "Channel 0 Locked", st->stf.status0.flags.ch0_locked);
>  	scdc_print_flag(m, "Channel 1 Locked", st->stf.status0.flags.ch1_locked);
>  	scdc_print_flag(m, "Channel 2 Locked", st->stf.status0.flags.ch2_locked);
> +	if (drm_scdc_num_frl_lanes(st->rate) == 4)
> +		scdc_print_flag(m, "Lane 3 Locked", st->stf.status0.flags.ln3_locked);
> +
> +	scdc_print_flag(m, "Sink Ready For Link Training", st->stf.status0.flags.flt_ready);
> +	scdc_print_flag(m, "Sink Failed To Decode DSC", st->stf.status0.flags.dsc_fail);
>  
>  	scdc_print_dec(m, "Channel 0 Errors", st->error_count[0]);
>  	scdc_print_dec(m, "Channel 1 Errors", st->error_count[1]);
>  	scdc_print_dec(m, "Channel 2 Errors", st->error_count[2]);
> +	if (drm_scdc_num_frl_lanes(st->rate) == 4)
> +		scdc_print_dec(m, "Lane 3 Errors", st->error_count[3]);
>  
>  	return 0;
>  
> diff --git a/include/drm/display/drm_scdc.h b/include/drm/display/drm_scdc.h
> index 3d58f37e8ed8..56cd909edac9 100644
> --- a/include/drm/display/drm_scdc.h
> +++ b/include/drm/display/drm_scdc.h
> @@ -29,6 +29,7 @@
>  #define SCDC_SOURCE_VERSION 0x02
>  
>  #define SCDC_UPDATE_0 0x10
> +#define  SCDC_FLT_UPDATE (1 << 5)
>  #define  SCDC_READ_REQUEST_TEST (1 << 2)
>  #define  SCDC_CED_UPDATE (1 << 1)
>  #define  SCDC_STATUS_UPDATE (1 << 0)
> @@ -46,6 +47,10 @@
>  #define SCDC_CONFIG_0 0x30
>  #define  SCDC_READ_REQUEST_ENABLE (1 << 0)
>  
> +#define SCDC_CONFIG_1 0x31
> +#define  SCDC_FRL_RATE   0x0f
> +#define  SCDC_FFE_LEVELS 0xf0
> +
>  #define SCDC_STATUS_FLAGS_0 0x40
>  #define  SCDC_CH2_LOCK (1 << 3)
>  #define  SCDC_CH1_LOCK (1 << 2)
> @@ -54,6 +59,7 @@
>  #define  SCDC_CLOCK_DETECT (1 << 0)
>  
>  #define SCDC_STATUS_FLAGS_1 0x41
> +#define SCDC_STATUS_FLAGS_2 0x42
>  
>  #define SCDC_ERR_DET_0_L 0x50
>  #define SCDC_ERR_DET_0_H 0x51
> @@ -65,6 +71,9 @@
>  
>  #define SCDC_ERR_DET_CHECKSUM 0x56
>  
> +#define SCDC_ERR_DET_3_L 0x57
> +#define SCDC_ERR_DET_3_H 0x58
> +
>  #define SCDC_TEST_CONFIG_0 0xc0
>  #define  SCDC_TEST_READ_REQUEST (1 << 7)
>  #define  SCDC_TEST_READ_REQUEST_DELAY(x) ((x) & 0x7f)
> diff --git a/include/drm/display/drm_scdc_helper.h b/include/drm/display/drm_scdc_helper.h
> index c066c1158275..a1b73f0be73f 100644
> --- a/include/drm/display/drm_scdc_helper.h
> +++ b/include/drm/display/drm_scdc_helper.h
> @@ -24,6 +24,7 @@
>  #ifndef DRM_SCDC_HELPER_H
>  #define DRM_SCDC_HELPER_H
>  
> +#include <linux/errno.h>
>  #include <linux/types.h>
>  
>  #include <drm/display/drm_scdc.h>
> @@ -40,10 +41,10 @@ struct drm_scdc_status_flags {
>  			bool ch0_locked : 1;
>  			bool ch1_locked : 1;
>  			bool ch2_locked : 1;
> -			bool rsvd_4 : 1;
> +			bool ln3_locked : 1;
>  			bool rsvd_5 : 1;
> -			bool rsvd_6 : 1;
> -			bool rsvd_7 : 1;
> +			bool flt_ready : 1;
> +			bool dsc_fail : 1;
>  		} flags __packed;
>  		u8 data;
>  	} status0;
> @@ -51,19 +52,68 @@ struct drm_scdc_status_flags {
>  	/* Status Register 1 */
>  	union {
>  		struct {
> -			bool rsvd_0 : 1;
> -			bool rsvd_1 : 1;
> -			bool rsvd_2 : 1;
> -			bool rsvd_3 : 1;
> -			bool rsvd_4 : 1;
> -			bool rsvd_5 : 1;
> -			bool rsvd_6 : 1;
> -			bool rsvd_7 : 1;
> +			u8 ln0_training_pattern : 4;
> +			u8 ln1_training_pattern : 4;
>  		} flags __packed;
>  		u8 data;
>  	} status1;
> +
> +	/* Status Register 2 */
> +	union {
> +		struct {
> +			u8 ln2_training_pattern : 4;
> +			u8 ln3_training_pattern : 4;
> +		} flags __packed;
> +		u8 data;
> +	} status2;

Here too, bitfield bit order is implementation defined. See my other
reply in the thread.

BR,
Jani.

> +};
> +
> +enum drm_scdc_frl_rate {
> +	SCDC_FRL_RATE_OFF = 0,
> +	SCDC_FRL_RATE_3X3 = 1,
> +	SCDC_FRL_RATE_6X3 = 2,
> +	SCDC_FRL_RATE_6X4 = 3,
> +	SCDC_FRL_RATE_8X4 = 4,
> +	SCDC_FRL_RATE_10X4 = 5,
> +	SCDC_FRL_RATE_12X4 = 6,
> +	SCDC_FRL_RATE_RESV_7 = 7,
> +	SCDC_FRL_RATE_RESV_8 = 8,
> +	SCDC_FRL_RATE_RESV_9 = 9,
> +	SCDC_FRL_RATE_RESV_10 = 10,
> +	SCDC_FRL_RATE_RESV_11 = 11,
> +	SCDC_FRL_RATE_RESV_12 = 12,
> +	SCDC_FRL_RATE_RESV_13 = 13,
> +	SCDC_FRL_RATE_RESV_14 = 14,
> +	SCDC_FRL_RATE_RESV_15 = 15
>  };
>  
> +/**
> + * drm_scdc_num_frl_lanes - get number of lanes for a given FRL rate
> + * @rate: one of &enum drm_scdc_frl_rate
> + *
> + * For a given @rate, return the number of lanes it uses.
> + *
> + * Returns: %-EINVAL if @rate is not a valid FRL rate, or the number of lanes
> + * for a given &enum drm_scdc_frl_rate on success (including %0 for "off")
> + */
> +static inline __pure int drm_scdc_num_frl_lanes(enum drm_scdc_frl_rate rate)
> +{
> +	switch (rate) {
> +	case SCDC_FRL_RATE_OFF:
> +		return 0;
> +	case SCDC_FRL_RATE_3X3:
> +	case SCDC_FRL_RATE_6X3:
> +		return 3;
> +	case SCDC_FRL_RATE_6X4:
> +	case SCDC_FRL_RATE_8X4:
> +	case SCDC_FRL_RATE_10X4:
> +	case SCDC_FRL_RATE_12X4:
> +		return 4;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  struct drm_scdc_state {
>  	/** @stf: contents of the status flag registers */
>  	struct drm_scdc_status_flags stf;
> @@ -76,8 +126,12 @@ struct drm_scdc_state {
>  	 * clock period, false if it's 1/10th of the clock period.
>  	 */
>  	bool tmds_bclk_x40;
> -	/** @error_count: character error counts for each channel */
> -	u16 error_count[3];
> +	/** @rate: FRL rate set by the source */
> +	enum drm_scdc_frl_rate rate : 4;
> +	/** @ffe_levels: The FFE levels for @rate set by the source */
> +	u8 ffe_levels : 4;
> +	/** @error_count: character error counts for each channel/link */
> +	u16 error_count[4];
>  };
>  
>  int drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
> @@ -128,9 +182,12 @@ bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector, bool se
>  
>  int drm_scdc_read_status0_flags(struct drm_connector *connector,
>  				struct drm_scdc_status_flags *flags);
> +int drm_scdc_read_status1_2_flags(struct drm_connector *connector,
> +				  struct drm_scdc_status_flags *flags);
>  int drm_scdc_read_state(struct drm_connector *connector,
>  			struct drm_scdc_state *state);
> -int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3]);
> +int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[4],
> +				 unsigned int num_lanes);
>  void drm_scdc_debugfs_init(struct drm_connector *connector, struct dentry *root);
>  
>  #endif

-- 
Jani Nikula, Intel

Re: [PATCH v3 1/4] drm/scdc-helper: Don't use ssize_t return type for scdc_read/write

[Luca Ceresoli]

On Tue, 26 May 2026 12:19:57 +0200, Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:
> drm_scdc_read and drm_scdc_write, both of which are only used within
> drm_scdc_helper (although exported), use a ssize_t as their return type.
> 
> This would make sense if they returned the number of bytes read/written
> on success, and negative errno otherwise. However, they return 0 on
> success.
> 
> [...]

Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 4/4] drm/scdc-helper: Implement parsing and printing HDMI 2.1 fields

[Nicolas Frattaroli]

On Tuesday, 26 May 2026 12:52:31 Central European Summer Time Jani Nikula wrote:
> On Tue, 26 May 2026, Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:
> > HDMI 2.1 redefines previously reserved fields in SCDC for various new
> > uses. No version check needs to be performed, as an HDMI 2.0 sink's
> > reserved SCDC fields are well-defined to be 0, and any zero-ness of
> > these fields for an HDMI 2.0 sink is not a surprise for SCDC parsers for
> > HDMI 2.1.
> >
> > Implement reading and outputting these fields over debugfs.
> >
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >  drivers/gpu/drm/display/drm_scdc_helper.c | 154 ++++++++++++++++++++++++++----
> >  include/drm/display/drm_scdc.h            |   9 ++
> >  include/drm/display/drm_scdc_helper.h     |  85 ++++++++++++++---
> >  3 files changed, 218 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
> > index 75a59c6fc7a5..b5a866372b0f 100644
> > --- a/drivers/gpu/drm/display/drm_scdc_helper.c
> > +++ b/drivers/gpu/drm/display/drm_scdc_helper.c
> > @@ -21,6 +21,7 @@
> >   * DEALINGS IN THE SOFTWARE.
> >   */
> >  
> > +#include <linux/bitfield.h>
> >  #include <linux/export.h>
> >  #include <linux/i2c.h>
> >  #include <linux/slab.h>
> > @@ -63,6 +64,38 @@ struct scdc_debugfs_priv {
> >  	struct drm_scdc_state state;
> >  };
> >  
> > +static __pure const char *drm_scdc_frl_rate_str(enum drm_scdc_frl_rate rate)
> 
> There are a total of only 71 __pure usages in the kernel. Is this
> helpful for a static function where the compiler can figure it out by
> itself?

For the compiler, no. For the programmer, maybe. It's basically
documentation that foo(x) remains the same for a specific x, so
that having 3 calls with the same parameter in some scope is not
actually 3 separate function calls, but just retrieving a value
without manually saving it into a stack variable. A compiler can
infer that in this case, but a programmer that has their LSP show
them the function signature on hover will immediately know what's
going on without having to look at the implementation.

I have a habit of slapping it onto any function that is pure just
because, but if you'd rather have me drop it then I can do that.
I'm aware that slapping attributes onto function signatures can
make them a bit noisy to read.

> Here too, bitfield bit order is implementation defined. See my other
> reply in the thread.

Ugh, I was hoping that with just 1-byte bitfields, this would not be
a problem. If that's not the case, then I guess Bug Endian ruins
another handy shortcut. :^( Oh well.

Thanks for the reviews, will send a new revision with the comments
addressed within the week probably.

Kind regards,
Nicolas Frattaroli

Re: [PATCH v3 4/4] drm/scdc-helper: Implement parsing and printing HDMI 2.1 fields

[Jani Nikula]

On Tue, 26 May 2026, Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:
> On Tuesday, 26 May 2026 12:52:31 Central European Summer Time Jani Nikula wrote:
>> On Tue, 26 May 2026, Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:
>> > +static __pure const char *drm_scdc_frl_rate_str(enum drm_scdc_frl_rate rate)
>> 
>> There are a total of only 71 __pure usages in the kernel. Is this
>> helpful for a static function where the compiler can figure it out by
>> itself?
>
> For the compiler, no. For the programmer, maybe. It's basically
> documentation that foo(x) remains the same for a specific x, so
> that having 3 calls with the same parameter in some scope is not
> actually 3 separate function calls, but just retrieving a value
> without manually saving it into a stack variable. A compiler can
> infer that in this case, but a programmer that has their LSP show
> them the function signature on hover will immediately know what's
> going on without having to look at the implementation.
>
> I have a habit of slapping it onto any function that is pure just
> because, but if you'd rather have me drop it then I can do that.
> I'm aware that slapping attributes onto function signatures can
> make them a bit noisy to read.

I'm not dead set on this.

But my observation, in general, is that setting an example is something
to be done with care. People try to do the right thing, and cargo
cult. And then you find out it's all over the place.

So IMO the question is not so much, "is it a good thing here", but
rather, "is this what we want for all pure functions".


BR,
Jani.


-- 
Jani Nikula, Intel

Claude review: Add SCDC information to connector debugfs

[Claude Code Review Bot]

Overall Series Review

Subject: Add SCDC information to connector debugfs
Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
Patches: 10
Reviewed: 2026-05-27T15:01:44.470669

---

This is a well-structured 4-patch series adding SCDC (Status and Control Data Channel) link health information to the DRM connector debugfs. The series progresses logically: fix the return types, add HDMI 2.0 SCDC debugfs, wire it into bridge connectors, and extend for HDMI 2.1 fields. The implementation is clean and useful for debugging cable/link issues.

There is one significant correctness concern around **bitfield portability** — the `drm_scdc_status_flags` union overlays `bool` bitfields with a `u8` register value, but the bit ordering of bitfields is architecture-dependent and will be wrong on big-endian systems. There's also a design concern about conditionally reading status data only when update flags are set, which can produce confusing debugfs output showing stale or zero data.

Overall the series is in good shape and close to mergeable, with the bitfield portability being the main issue to address.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/scdc-helper: Don't use ssize_t return type for scdc_read/write

[Claude Code Review Bot]

Patch Review

Clean, straightforward cleanup. Both functions return 0 on success and negative errno on failure, so `ssize_t` is semantically wrong and `int` is correct.

No issues found. The change is consistent between the `.c` and `.h` files, and the inline wrappers `drm_scdc_readb`/`drm_scdc_writeb` already return `int` so the implicit conversion that was happening before is now eliminated.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/scdc-helper: Add scdc_status debugfs entry

[Claude Code Review Bot]

Patch Review

This is the core patch. Several observations:

**Bitfield portability (issue):** The `drm_scdc_status_flags` struct uses a union of `bool` bitfields with `u8 data`:

```c
union {
    struct {
        bool clock_detected : 1;
        bool ch0_locked : 1;
        bool ch1_locked : 1;
        bool ch2_locked : 1;
        ...
    } flags __packed;
    u8 data;
} status0;
```

Bitfield ordering within a storage unit is implementation-defined. GCC allocates from LSB on little-endian targets and from MSB on big-endian targets. On a big-endian system, `clock_detected` would map to bit 7, not bit 0 as the SCDC register defines (`SCDC_CLOCK_DETECT` = `(1 << 0)`). The kernel handles this elsewhere (e.g., `struct iphdr`) using `#if defined(__LITTLE_ENDIAN_BITFIELD)` / `#elif defined(__BIG_ENDIAN_BITFIELD)` guards. The DRM subsystem doesn't currently use this pattern (no hits in `include/drm/`), but these bitfields directly overlay a hardware register byte, which is where it matters. Consider using explicit bit masks and shifts instead, or adding endianness guards.

**Conditional reads from update flags (design concern):** In `drm_scdc_read_state`, status flags and error counters are only read when the corresponding update flags are set:

```c
if (upd_flags[0] & SCDC_STATUS_UPDATE) {
    ret = drm_scdc_read_status0_flags(connector, &state->stf);
    ...
}
if (upd_flags[0] & SCDC_CED_UPDATE) {
    ret = drm_scdc_read_error_counters(connector, state->error_count);
    ...
}
```

Since the state persists in `scdc_debugfs_priv`, on the first debugfs read (or if no updates have occurred), the status and error fields will be zero. Subsequent reads will show stale data from the last update. For a polling-oriented debugfs interface, always reading status flags (which have no side effects besides clearing the notification bit) would produce more useful output. Error counters have clear-on-read semantics so the conditional approach is more defensible there.

**Update flag clearing order:** `drm_scdc_read_status0_flags` clears the `SCDC_STATUS_UPDATE` flag *before* reading the status register. This is fine because status registers always reflect current state (they aren't latched on the update flag), but it differs from the typical spec-described flow of read-then-clear. Not a bug, just worth noting.

**Minor: doc typo in `drm_scdc_state`:**

```c
/** @scramling_enabled: true if TMDS scrambling is on */
```

Should be `@scrambling_enabled`.

**Connector refcounting:** The `scdc_status_show` function correctly releases the connector ref after `drm_scdc_read_state` and before printing the state (which only accesses the local state copy). This is fine.

**Exported symbols:** `drm_scdc_read_status0_flags`, `drm_scdc_read_error_counters`, and `drm_scdc_read_state` are all `EXPORT_SYMBOL`. If these are only used by the debugfs show function, they could remain static to avoid expanding the kernel symbol surface. If they're intended for future driver use, the exports are reasonable.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/display: bridge_connector: init scdc debugfs for HDMI

[Claude Code Review Bot]

Patch Review

Simple and correct. Adds one call to `drm_scdc_debugfs_init` when `bridge_connector->bridge_hdmi` is set:

```c
if (bridge_connector->bridge_hdmi)
    drm_scdc_debugfs_init(connector, root);
```

The placement after the per-bridge `debugfs_init` loop is logical. No issues.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/scdc-helper: Implement parsing and printing HDMI 2.1 fields

[Claude Code Review Bot]

Patch Review

This patch extends the HDMI 2.0 code for HDMI 2.1 FRL (Fixed Rate Link) support. Good approach of not requiring a version check since HDMI 2.0 sinks have reserved fields defined as 0.

**Error counter checksum with 4 lanes:** For 4-lane mode, the code reads 9 bytes (0x50-0x58) and checksums all of them:

```c
case 4:
    buf_sz = 9;
    break;
...
for (i = 0; i < buf_sz; i++)
    sum = wrapping_add(u8, sum, buf[i]);
```

The checksum register is at 0x56 (buf[6]). This assumes the HDMI 2.1 checksum at 0x56 covers registers 0x50-0x55 *and* 0x57-0x58 (all error detection registers). Please confirm this matches the CTA-861-H spec. If the checksum only covers 0x50-0x55 (the HDMI 2.0 range), the validation would fail when lane 3 has non-zero data, as bytes 0x57-0x58 would be summed but not included in the checksum calculation by the sink.

**Enum bitfield:**

```c
enum drm_scdc_frl_rate rate : 4;
u8 ffe_levels : 4;
```

Enum bitfields are a GCC/Clang extension, not standard C. This works in the kernel build environment but is worth noting. The 4-bit width is sufficient for the 0-15 value range since GCC treats this enum's underlying type as unsigned (all values are non-negative).

**`scdc_err_cnt_buf_idx` function:** This works correctly but is more complex than needed. Since the registers are at known offsets, a simpler computation would be `lane * 2` for lanes 0-2 and 7 for lane 3. The switch statement is clearer about the non-contiguous register layout though (checksum sits between lane 2 and lane 3), so it's a reasonable choice.

**`drm_scdc_frl_rate_str` has a `default: return NULL` case:**

```c
default:
    return NULL;
```

All 16 possible 4-bit values are covered by the switch cases (0-6 named, 7-15 reserved), so the `default` is unreachable. Returning `NULL` from a function whose result is passed directly to `seq_printf` via `scdc_print_str` would cause a NULL dereference if it were ever reached. Consider returning `"(Unknown)"` instead, or removing the default case entirely.

**FRL rate 0 → 3 lanes fallback:** The handling in `drm_scdc_read_state` is correct:

```c
num_lanes = drm_scdc_num_frl_lanes(state->rate);
if (num_lanes < 0)
    return num_lanes;
if (!num_lanes)
    num_lanes = 3;
```

When FRL is off (TMDS mode), defaulting to 3 lanes for the HDMI 2.0 error counters makes sense.

**Status register 1 bitfield name:** The field is called `flags` but now contains 4-bit training pattern values rather than individual boolean flags:

```c
struct {
    u8 ln0_training_pattern : 4;
    u8 ln1_training_pattern : 4;
} flags __packed;
```

Consider renaming it to something like `fields` to better reflect the content, since these are multi-bit values, not flags.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/scdc-helper: Add scdc_status debugfs entry

[Claude Code Review Bot]

Patch Review

**Verdict: Minor issues.**

This is the bulk of the series. The overall architecture is clean: `drm_scdc_read_state` as the core state reader, separate `drm_scdc_read_status0_flags` and `drm_scdc_read_error_counters` for individual fields, and `scdc_status_show` as the debugfs presentation layer.

**1. Doc comment typo** in `drm_scdc_state`:

```c
/** @scramling_enabled: true if TMDS scrambling is on */
```
Should be `@scrambling_enabled`.

**2. Exported functions with no external callers yet**: `drm_scdc_read_status0_flags`, `drm_scdc_read_error_counters`, and `drm_scdc_read_state` are all `EXPORT_SYMBOL` and declared in the public header, but currently only used internally by `scdc_status_show`. If no driver is expected to call these directly soon, they could be `static` to reduce API surface. However, if the intent is to provide a public API for drivers that want to query SCDC state programmatically, then exporting proactively is fine -- just worth stating the intent.

**3. Status flag clear-before-read ordering**: `drm_scdc_read_status0_flags` writes `SCDC_STATUS_UPDATE` to clear the flag *before* reading the status register. The HDMI spec recommends reading the data first, then clearing the flag. For a debugfs polling interface this is fine in practice, but worth noting the deviation.

**4. Connector get/put pattern is correct**: The success path drops the ref after `drm_scdc_read_state` returns (before printing cached state), and the error path uses `err_conn_put`. Clean.

---
Generated by Claude Code Patch Reviewer