[PATCH v3 0/6] Add HDMI CEC support to Rockchip RK3588/RK3576 SoCs

[PATCH v3 0/6] Add HDMI CEC support to Rockchip RK3588/RK3576 SoCs

[Cristian Ciocaltea]

The first patch in the series implements the CEC capability of the
Synopsys DesignWare HDMI QP TX controller found in RK3588 & RK3576 Socs.
This is based on the downstream code, but rewritten on top of the CEC
helpers added recently to the DRM HDMI connector framework.

The second patch is needed for RK3576 in order to fixup the timer base
setup according to the actual reference clock rate, which differs
slightly from RK3588.

The following three patches setup platform data with the new information
expected by the HDMI QP transmitter library, while improving the error
handling in the probe path.

Please note the CEC helpers were affected by a resource deallocation
issue which could crash the kernel and freeze the system under certain
test conditions.  This has been already fixed in v6.17-rc1 via commit
19920ab98e17 ("drm/display: hdmi-cec-helper: Fix adapter
unregistration").

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
Changes in v3:
- Fixup PATCH 1 according to the recent upstream commit 02bb63d1a593
  ("drm/bridge: Make dp/hdmi_audio_* callback keep the same paramter
  order with get_modes") which changed the signature of ->hdmi_cec_init()
  callback of struct drm_bridge_funcs; while at it, also update the
  copyright section
- Updated cover letter to indicate that the CEC helpers fix is already
  available since v6.17-rc1
- Rebased series onto next-20250825
- Link to v2: https://lore.kernel.org/r/20250710-rk3588-hdmi-cec-v2-0-f5884be34bc1@collabora.com

Changes in v2:
- Collected R-b tag from Dmitry
- Restructured the generic bridge patches to not depend on the
  platform-specific changes and updated cover letter accordingly (Heiko)
- Replaced the loop searching for "ref" clock with clk_get() (Maxime)
- Added new patch "drm/rockchip: dw_hdmi_qp: Improve error handling with
  dev_err_probe()"
- Link to v1: https://lore.kernel.org/r/20250704-rk3588-hdmi-cec-v1-0-2bd8de8700cd@collabora.com

---
Cristian Ciocaltea (6):
      drm/bridge: dw-hdmi-qp: Add CEC support
      drm/bridge: dw-hdmi-qp: Fixup timer base setup
      drm/rockchip: dw_hdmi_qp: Improve error handling with dev_err_probe()
      drm/rockchip: dw_hdmi_qp: Provide CEC IRQ in dw_hdmi_qp_plat_data
      drm/rockchip: dw_hdmi_qp: Provide ref clock rate in dw_hdmi_qp_plat_data
      arm64: defconfig: Enable DW HDMI QP CEC support

 arch/arm64/configs/defconfig                   |   1 +
 drivers/gpu/drm/bridge/synopsys/Kconfig        |   8 +
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c   | 232 ++++++++++++++++++++++++-
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.h   |  14 ++
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c |  77 ++++----
 include/drm/bridge/dw_hdmi_qp.h                |   2 +
 6 files changed, 292 insertions(+), 42 deletions(-)
---
base-commit: 6c68f4c0a147c025ae0b25fab688c7c47964a02f
change-id: 20250703-rk3588-hdmi-cec-cea8f523df48

[PATCH v3 1/6] drm/bridge: dw-hdmi-qp: Add CEC support

[Cristian Ciocaltea]

Add support for the CEC interface of the Synopsys DesignWare HDMI QP TX
controller.

This is based on the downstream implementation, but rewritten on top of
the CEC helpers added recently to the DRM HDMI connector framework.

Also note struct dw_hdmi_qp_plat_data has been extended to include the
CEC IRQ number to be provided by the platform driver.

Co-developed-by: Algea Cao <algea.cao@rock-chips.com>
Signed-off-by: Algea Cao <algea.cao@rock-chips.com>
Co-developed-by: Derek Foreman <derek.foreman@collabora.com>
Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/bridge/synopsys/Kconfig      |   8 +
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 221 +++++++++++++++++++++++++++
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.h |  14 ++
 include/drm/bridge/dw_hdmi_qp.h              |   1 +
 4 files changed, 244 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/Kconfig b/drivers/gpu/drm/bridge/synopsys/Kconfig
index f3ab2f985f8ca9dc1eeac3bda6b4a31d355cd51c..99878f051067e65fa3b97d8132be8cfa15980966 100644
--- a/drivers/gpu/drm/bridge/synopsys/Kconfig
+++ b/drivers/gpu/drm/bridge/synopsys/Kconfig
@@ -54,6 +54,14 @@ config DRM_DW_HDMI_QP
 	select DRM_KMS_HELPER
 	select REGMAP_MMIO
 
+config DRM_DW_HDMI_QP_CEC
+	bool "Synopsis Designware QP CEC interface"
+	depends on DRM_DW_HDMI_QP
+	select DRM_DISPLAY_HDMI_CEC_HELPER
+	help
+	  Support the CEC interface which is part of the Synopsys
+	  Designware HDMI QP block.
+
 config DRM_DW_MIPI_DSI
 	tristate
 	select DRM_KMS_HELPER
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
index 39332c57f2c54296f39e27612544f4fbf923863f..96455b3bb7b6a3f6ad488d10bc9ba90a1b56e4c8 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
@@ -2,6 +2,7 @@
 /*
  * Copyright (c) 2021-2022 Rockchip Electronics Co., Ltd.
  * Copyright (c) 2024 Collabora Ltd.
+ * Copyright (c) 2025 Amazon.com, Inc. or its affiliates.
  *
  * Author: Algea Cao <algea.cao@rock-chips.com>
  * Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
@@ -18,6 +19,7 @@
 
 #include <drm/bridge/dw_hdmi_qp.h>
 #include <drm/display/drm_hdmi_helper.h>
+#include <drm/display/drm_hdmi_cec_helper.h>
 #include <drm/display/drm_hdmi_state_helper.h>
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
@@ -26,6 +28,8 @@
 #include <drm/drm_edid.h>
 #include <drm/drm_modes.h>
 
+#include <media/cec.h>
+
 #include <sound/hdmi-codec.h>
 
 #include "dw-hdmi-qp.h"
@@ -131,12 +135,28 @@ struct dw_hdmi_qp_i2c {
 	bool			is_segment;
 };
 
+#ifdef CONFIG_DRM_DW_HDMI_QP_CEC
+struct dw_hdmi_qp_cec {
+	struct drm_connector *connector;
+	int irq;
+	u32 addresses;
+	struct cec_msg rx_msg;
+	u8 tx_status;
+	bool tx_done;
+	bool rx_done;
+};
+#endif
+
 struct dw_hdmi_qp {
 	struct drm_bridge bridge;
 
 	struct device *dev;
 	struct dw_hdmi_qp_i2c *i2c;
 
+#ifdef CONFIG_DRM_DW_HDMI_QP_CEC
+	struct dw_hdmi_qp_cec *cec;
+#endif
+
 	struct {
 		const struct dw_hdmi_qp_phy_ops *ops;
 		void *data;
@@ -965,6 +985,191 @@ static int dw_hdmi_qp_bridge_write_infoframe(struct drm_bridge *bridge,
 	}
 }
 
+#ifdef CONFIG_DRM_DW_HDMI_QP_CEC
+static irqreturn_t dw_hdmi_qp_cec_hardirq(int irq, void *dev_id)
+{
+	struct dw_hdmi_qp *hdmi = dev_id;
+	struct dw_hdmi_qp_cec *cec = hdmi->cec;
+	irqreturn_t ret = IRQ_HANDLED;
+	u32 stat;
+
+	stat = dw_hdmi_qp_read(hdmi, CEC_INT_STATUS);
+	if (stat == 0)
+		return IRQ_NONE;
+
+	dw_hdmi_qp_write(hdmi, stat, CEC_INT_CLEAR);
+
+	if (stat & CEC_STAT_LINE_ERR) {
+		cec->tx_status = CEC_TX_STATUS_ERROR;
+		cec->tx_done = true;
+		ret = IRQ_WAKE_THREAD;
+	} else if (stat & CEC_STAT_DONE) {
+		cec->tx_status = CEC_TX_STATUS_OK;
+		cec->tx_done = true;
+		ret = IRQ_WAKE_THREAD;
+	} else if (stat & CEC_STAT_NACK) {
+		cec->tx_status = CEC_TX_STATUS_NACK;
+		cec->tx_done = true;
+		ret = IRQ_WAKE_THREAD;
+	}
+
+	if (stat & CEC_STAT_EOM) {
+		unsigned int len, i, val;
+
+		val = dw_hdmi_qp_read(hdmi, CEC_RX_COUNT_STATUS);
+		len = (val & 0xf) + 1;
+
+		if (len > sizeof(cec->rx_msg.msg))
+			len = sizeof(cec->rx_msg.msg);
+
+		for (i = 0; i < 4; i++) {
+			val = dw_hdmi_qp_read(hdmi, CEC_RX_DATA3_0 + i * 4);
+			cec->rx_msg.msg[i * 4] = val & 0xff;
+			cec->rx_msg.msg[i * 4 + 1] = (val >> 8) & 0xff;
+			cec->rx_msg.msg[i * 4 + 2] = (val >> 16) & 0xff;
+			cec->rx_msg.msg[i * 4 + 3] = (val >> 24) & 0xff;
+		}
+
+		dw_hdmi_qp_write(hdmi, 1, CEC_LOCK_CONTROL);
+
+		cec->rx_msg.len = len;
+		cec->rx_done = true;
+
+		ret = IRQ_WAKE_THREAD;
+	}
+
+	return ret;
+}
+
+static irqreturn_t dw_hdmi_qp_cec_thread(int irq, void *dev_id)
+{
+	struct dw_hdmi_qp *hdmi = dev_id;
+	struct dw_hdmi_qp_cec *cec = hdmi->cec;
+
+	if (cec->tx_done) {
+		cec->tx_done = false;
+		drm_connector_hdmi_cec_transmit_attempt_done(cec->connector,
+							     cec->tx_status);
+	}
+
+	if (cec->rx_done) {
+		cec->rx_done = false;
+		drm_connector_hdmi_cec_received_msg(cec->connector, &cec->rx_msg);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int dw_hdmi_qp_cec_init(struct drm_bridge *bridge,
+			       struct drm_connector *connector)
+{
+	struct dw_hdmi_qp *hdmi = dw_hdmi_qp_from_bridge(bridge);
+	struct dw_hdmi_qp_cec *cec = hdmi->cec;
+	int ret;
+
+	if (cec->irq < 0) {
+		dev_err(hdmi->dev, "Invalid cec irq: %d\n", cec->irq);
+		return -EINVAL;
+	}
+
+	cec->connector = connector;
+
+	dw_hdmi_qp_write(hdmi, 0, CEC_TX_COUNT);
+	dw_hdmi_qp_write(hdmi, ~0, CEC_INT_CLEAR);
+	dw_hdmi_qp_write(hdmi, 0, CEC_INT_MASK_N);
+
+	ret = devm_request_threaded_irq(hdmi->dev, cec->irq,
+					dw_hdmi_qp_cec_hardirq,
+					dw_hdmi_qp_cec_thread, IRQF_SHARED,
+					dev_name(hdmi->dev), hdmi);
+	if (ret < 0) {
+		dev_err(hdmi->dev, "Request cec irq thread failed: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int dw_hdmi_qp_cec_log_addr(struct drm_bridge *bridge, u8 logical_addr)
+{
+	struct dw_hdmi_qp *hdmi = dw_hdmi_qp_from_bridge(bridge);
+	struct dw_hdmi_qp_cec *cec = hdmi->cec;
+
+	if (logical_addr == CEC_LOG_ADDR_INVALID)
+		cec->addresses = 0;
+	else
+		cec->addresses |= BIT(logical_addr) | CEC_ADDR_BROADCAST;
+
+	dw_hdmi_qp_write(hdmi, cec->addresses, CEC_ADDR);
+
+	return 0;
+}
+
+static int dw_hdmi_qp_cec_enable(struct drm_bridge *bridge, bool enable)
+{
+	struct dw_hdmi_qp *hdmi = dw_hdmi_qp_from_bridge(bridge);
+	unsigned int irqs;
+	u32 swdisable;
+
+	if (!enable) {
+		dw_hdmi_qp_write(hdmi, 0, CEC_INT_MASK_N);
+		dw_hdmi_qp_write(hdmi, ~0, CEC_INT_CLEAR);
+
+		swdisable = dw_hdmi_qp_read(hdmi, GLOBAL_SWDISABLE);
+		swdisable = swdisable | CEC_SWDISABLE;
+		dw_hdmi_qp_write(hdmi, swdisable, GLOBAL_SWDISABLE);
+	} else {
+		swdisable = dw_hdmi_qp_read(hdmi, GLOBAL_SWDISABLE);
+		swdisable = swdisable & ~CEC_SWDISABLE;
+		dw_hdmi_qp_write(hdmi, swdisable, GLOBAL_SWDISABLE);
+
+		dw_hdmi_qp_write(hdmi, ~0, CEC_INT_CLEAR);
+		dw_hdmi_qp_write(hdmi, 1, CEC_LOCK_CONTROL);
+
+		dw_hdmi_qp_cec_log_addr(bridge, CEC_LOG_ADDR_INVALID);
+
+		irqs = CEC_STAT_LINE_ERR | CEC_STAT_NACK | CEC_STAT_EOM |
+		       CEC_STAT_DONE;
+		dw_hdmi_qp_write(hdmi, ~0, CEC_INT_CLEAR);
+		dw_hdmi_qp_write(hdmi, irqs, CEC_INT_MASK_N);
+	}
+
+	return 0;
+}
+
+static int dw_hdmi_qp_cec_transmit(struct drm_bridge *bridge, u8 attempts,
+				   u32 signal_free_time, struct cec_msg *msg)
+{
+	struct dw_hdmi_qp *hdmi = dw_hdmi_qp_from_bridge(bridge);
+	unsigned int i;
+	u32 val;
+
+	for (i = 0; i < msg->len; i++) {
+		if (!(i % 4))
+			val = msg->msg[i];
+		if ((i % 4) == 1)
+			val |= msg->msg[i] << 8;
+		if ((i % 4) == 2)
+			val |= msg->msg[i] << 16;
+		if ((i % 4) == 3)
+			val |= msg->msg[i] << 24;
+
+		if (i == (msg->len - 1) || (i % 4) == 3)
+			dw_hdmi_qp_write(hdmi, val, CEC_TX_DATA3_0 + (i / 4) * 4);
+	}
+
+	dw_hdmi_qp_write(hdmi, msg->len - 1, CEC_TX_COUNT);
+	dw_hdmi_qp_write(hdmi, CEC_CTRL_START, CEC_TX_CONTROL);
+
+	return 0;
+}
+#else
+#define dw_hdmi_qp_cec_init NULL
+#define dw_hdmi_qp_cec_enable NULL
+#define dw_hdmi_qp_cec_log_addr NULL
+#define dw_hdmi_qp_cec_transmit NULL
+#endif /* CONFIG_DRM_DW_HDMI_QP_CEC */
+
 static const struct drm_bridge_funcs dw_hdmi_qp_bridge_funcs = {
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
@@ -979,6 +1184,10 @@ static const struct drm_bridge_funcs dw_hdmi_qp_bridge_funcs = {
 	.hdmi_audio_startup = dw_hdmi_qp_audio_enable,
 	.hdmi_audio_shutdown = dw_hdmi_qp_audio_disable,
 	.hdmi_audio_prepare = dw_hdmi_qp_audio_prepare,
+	.hdmi_cec_init = dw_hdmi_qp_cec_init,
+	.hdmi_cec_enable = dw_hdmi_qp_cec_enable,
+	.hdmi_cec_log_addr = dw_hdmi_qp_cec_log_addr,
+	.hdmi_cec_transmit = dw_hdmi_qp_cec_transmit,
 };
 
 static irqreturn_t dw_hdmi_qp_main_hardirq(int irq, void *dev_id)
@@ -1093,6 +1302,18 @@ struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
 	hdmi->bridge.hdmi_audio_dev = dev;
 	hdmi->bridge.hdmi_audio_dai_port = 1;
 
+#ifdef CONFIG_DRM_DW_HDMI_QP_CEC
+	hdmi->bridge.ops |= DRM_BRIDGE_OP_HDMI_CEC_ADAPTER;
+	hdmi->bridge.hdmi_cec_dev = dev;
+	hdmi->bridge.hdmi_cec_adapter_name = dev_name(dev);
+
+	hdmi->cec = devm_kzalloc(hdmi->dev, sizeof(*hdmi->cec), GFP_KERNEL);
+	if (!hdmi->cec)
+		return ERR_PTR(-ENOMEM);
+
+	hdmi->cec->irq = plat_data->cec_irq;
+#endif
+
 	ret = devm_drm_bridge_add(dev, &hdmi->bridge);
 	if (ret)
 		return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.h
index 72987e6c468928f2b998099697a6f32726411557..91a15f82e32acc32eef58f11ec5ca958337ebb9a 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.h
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.h
@@ -488,9 +488,23 @@
 #define AUDPKT_VBIT_OVR0				0xf24
 /* CEC Registers */
 #define CEC_TX_CONTROL					0x1000
+#define CEC_CTRL_CLEAR					BIT(0)
+#define CEC_CTRL_START					BIT(0)
 #define CEC_STATUS					0x1004
+#define CEC_STAT_DONE					BIT(0)
+#define CEC_STAT_NACK					BIT(1)
+#define CEC_STAT_ARBLOST				BIT(2)
+#define CEC_STAT_LINE_ERR				BIT(3)
+#define CEC_STAT_RETRANS_FAIL				BIT(4)
+#define CEC_STAT_DISCARD				BIT(5)
+#define CEC_STAT_TX_BUSY				BIT(8)
+#define CEC_STAT_RX_BUSY				BIT(9)
+#define CEC_STAT_DRIVE_ERR				BIT(10)
+#define CEC_STAT_EOM					BIT(11)
+#define CEC_STAT_NOTIFY_ERR				BIT(12)
 #define CEC_CONFIG					0x1008
 #define CEC_ADDR					0x100c
+#define CEC_ADDR_BROADCAST				BIT(15)
 #define CEC_TX_COUNT					0x1020
 #define CEC_TX_DATA3_0					0x1024
 #define CEC_TX_DATA7_4					0x1028
diff --git a/include/drm/bridge/dw_hdmi_qp.h b/include/drm/bridge/dw_hdmi_qp.h
index e9be6d507ad9cdc55f5c7d6d3ef37eba41f1ce74..b4a9b739734ec7b67013b683fe6017551aa19172 100644
--- a/include/drm/bridge/dw_hdmi_qp.h
+++ b/include/drm/bridge/dw_hdmi_qp.h
@@ -23,6 +23,7 @@ struct dw_hdmi_qp_plat_data {
 	const struct dw_hdmi_qp_phy_ops *phy_ops;
 	void *phy_data;
 	int main_irq;
+	int cec_irq;
 };
 
 struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,

-- 
2.50.1

[PATCH v3 2/6] drm/bridge: dw-hdmi-qp: Fixup timer base setup

[Cristian Ciocaltea]

Currently the TIMER_BASE_CONFIG0 register gets initialized to a fixed
value as initially found in vendor driver code supporting the RK3588
SoC.  As a matter of fact the value matches the rate of the HDMI TX
reference clock, which is roughly 428.57 MHz.

However, on RK3576 SoC that rate is slightly lower, i.e. 396.00 MHz, and
the incorrect register configuration breaks CEC functionality.

Set the timer base according to the actual reference clock rate that
shall be provided by the platform driver.

While at it, also drop the unnecessary empty lines in
dw_hdmi_qp_init_hw().

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 11 ++++++++---
 include/drm/bridge/dw_hdmi_qp.h              |  1 +
 2 files changed, 9 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 96455b3bb7b6a3f6ad488d10bc9ba90a1b56e4c8..42a90e0383061dad6c8416af21b27db7a3ba6d7d 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
@@ -162,6 +162,7 @@ struct dw_hdmi_qp {
 		void *data;
 	} phy;
 
+	unsigned long ref_clk_rate;
 	struct regmap *regm;
 
 	unsigned long tmds_char_rate;
@@ -1223,13 +1224,11 @@ static void dw_hdmi_qp_init_hw(struct dw_hdmi_qp *hdmi)
 {
 	dw_hdmi_qp_write(hdmi, 0, MAINUNIT_0_INT_MASK_N);
 	dw_hdmi_qp_write(hdmi, 0, MAINUNIT_1_INT_MASK_N);
-	dw_hdmi_qp_write(hdmi, 428571429, TIMER_BASE_CONFIG0);
+	dw_hdmi_qp_write(hdmi, hdmi->ref_clk_rate, TIMER_BASE_CONFIG0);
 
 	/* Software reset */
 	dw_hdmi_qp_write(hdmi, 0x01, I2CM_CONTROL0);
-
 	dw_hdmi_qp_write(hdmi, 0x085c085c, I2CM_FM_SCL_CONFIG0);
-
 	dw_hdmi_qp_mod(hdmi, 0, I2CM_FM_EN, I2CM_INTERFACE_CONTROL0);
 
 	/* Clear DONE and ERROR interrupts */
@@ -1255,6 +1254,11 @@ struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
 		return ERR_PTR(-ENODEV);
 	}
 
+	if (!plat_data->ref_clk_rate) {
+		dev_err(dev, "Missing ref_clk rate\n");
+		return ERR_PTR(-ENODEV);
+	}
+
 	hdmi = devm_drm_bridge_alloc(dev, struct dw_hdmi_qp, bridge,
 				     &dw_hdmi_qp_bridge_funcs);
 	if (IS_ERR(hdmi))
@@ -1274,6 +1278,7 @@ struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
 
 	hdmi->phy.ops = plat_data->phy_ops;
 	hdmi->phy.data = plat_data->phy_data;
+	hdmi->ref_clk_rate = plat_data->ref_clk_rate;
 
 	dw_hdmi_qp_init_hw(hdmi);
 
diff --git a/include/drm/bridge/dw_hdmi_qp.h b/include/drm/bridge/dw_hdmi_qp.h
index b4a9b739734ec7b67013b683fe6017551aa19172..76ecf31301997718604a05f70ce9eab8695e26b5 100644
--- a/include/drm/bridge/dw_hdmi_qp.h
+++ b/include/drm/bridge/dw_hdmi_qp.h
@@ -24,6 +24,7 @@ struct dw_hdmi_qp_plat_data {
 	void *phy_data;
 	int main_irq;
 	int cec_irq;
+	unsigned long ref_clk_rate;
 };
 
 struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,

-- 
2.50.1

[PATCH v3 3/6] drm/rockchip: dw_hdmi_qp: Improve error handling with dev_err_probe()

[Cristian Ciocaltea]

The error handling in dw_hdmi_qp_rockchip_bind() is quite inconsistent,
i.e. in some cases the error code is not included in the message, while
in some other cases there is no check for -EPROBE_DEFER.

Since this is part of the probe path, address the aforementioned issues
by switching to dev_err_probe(), which also reduces the code a bit.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 62 ++++++++++----------------
 1 file changed, 24 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
index 7d531b6f4c098c6c548788dad487ce4613a2f32b..4e7794aa2dded4c124963eaa7f5158bde9bbbdb6 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
@@ -457,10 +457,8 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 		return -ENODEV;
 
 	if (!cfg->ctrl_ops || !cfg->ctrl_ops->io_init ||
-	    !cfg->ctrl_ops->irq_callback || !cfg->ctrl_ops->hardirq_callback) {
-		dev_err(dev, "Missing platform ctrl ops\n");
-		return -ENODEV;
-	}
+	    !cfg->ctrl_ops->irq_callback || !cfg->ctrl_ops->hardirq_callback)
+		return dev_err_probe(dev, -ENODEV, "Missing platform ctrl ops\n");
 
 	hdmi->ctrl_ops = cfg->ctrl_ops;
 	hdmi->dev = &pdev->dev;
@@ -473,10 +471,9 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 			break;
 		}
 	}
-	if (hdmi->port_id < 0) {
-		dev_err(hdmi->dev, "Failed to match HDMI port ID\n");
-		return hdmi->port_id;
-	}
+	if (hdmi->port_id < 0)
+		return dev_err_probe(hdmi->dev, hdmi->port_id,
+				     "Failed to match HDMI port ID\n");
 
 	plat_data.phy_ops = cfg->phy_ops;
 	plat_data.phy_data = hdmi;
@@ -497,39 +494,30 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 
 	hdmi->regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
 						       "rockchip,grf");
-	if (IS_ERR(hdmi->regmap)) {
-		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
-		return PTR_ERR(hdmi->regmap);
-	}
+	if (IS_ERR(hdmi->regmap))
+		return dev_err_probe(hdmi->dev, PTR_ERR(hdmi->regmap),
+				     "Unable to get rockchip,grf\n");
 
 	hdmi->vo_regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
 							  "rockchip,vo-grf");
-	if (IS_ERR(hdmi->vo_regmap)) {
-		dev_err(hdmi->dev, "Unable to get rockchip,vo-grf\n");
-		return PTR_ERR(hdmi->vo_regmap);
-	}
+	if (IS_ERR(hdmi->vo_regmap))
+		return dev_err_probe(hdmi->dev, PTR_ERR(hdmi->vo_regmap),
+				     "Unable to get rockchip,vo-grf\n");
 
 	ret = devm_clk_bulk_get_all_enabled(hdmi->dev, &clks);
-	if (ret < 0) {
-		dev_err(hdmi->dev, "Failed to get clocks: %d\n", ret);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(hdmi->dev, ret, "Failed to get clocks\n");
 
 	hdmi->enable_gpio = devm_gpiod_get_optional(hdmi->dev, "enable",
 						    GPIOD_OUT_HIGH);
-	if (IS_ERR(hdmi->enable_gpio)) {
-		ret = PTR_ERR(hdmi->enable_gpio);
-		dev_err(hdmi->dev, "Failed to request enable GPIO: %d\n", ret);
-		return ret;
-	}
+	if (IS_ERR(hdmi->enable_gpio))
+		return dev_err_probe(hdmi->dev, PTR_ERR(hdmi->enable_gpio),
+				     "Failed to request enable GPIO\n");
 
 	hdmi->phy = devm_of_phy_get_by_index(dev, dev->of_node, 0);
-	if (IS_ERR(hdmi->phy)) {
-		ret = PTR_ERR(hdmi->phy);
-		if (ret != -EPROBE_DEFER)
-			dev_err(hdmi->dev, "failed to get phy: %d\n", ret);
-		return ret;
-	}
+	if (IS_ERR(hdmi->phy))
+		return dev_err_probe(hdmi->dev, PTR_ERR(hdmi->phy),
+				     "Failed to get phy\n");
 
 	cfg->ctrl_ops->io_init(hdmi);
 
@@ -558,17 +546,15 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 
 	hdmi->hdmi = dw_hdmi_qp_bind(pdev, encoder, &plat_data);
 	if (IS_ERR(hdmi->hdmi)) {
-		ret = PTR_ERR(hdmi->hdmi);
 		drm_encoder_cleanup(encoder);
-		return ret;
+		return dev_err_probe(hdmi->dev, PTR_ERR(hdmi->hdmi),
+				     "Failed to bind dw-hdmi-qp");
 	}
 
 	connector = drm_bridge_connector_init(drm, encoder);
-	if (IS_ERR(connector)) {
-		ret = PTR_ERR(connector);
-		dev_err(hdmi->dev, "failed to init bridge connector: %d\n", ret);
-		return ret;
-	}
+	if (IS_ERR(connector))
+		return dev_err_probe(hdmi->dev, PTR_ERR(connector),
+				     "Failed to init bridge connector\n");
 
 	return drm_connector_attach_encoder(connector, encoder);
 }

-- 
2.50.1

[PATCH v3 4/6] drm/rockchip: dw_hdmi_qp: Provide CEC IRQ in dw_hdmi_qp_plat_data

[Cristian Ciocaltea]

In order to support the CEC interface of the DesignWare HDMI QP IP
block, setup platform data to include the required IRQ number.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
index 4e7794aa2dded4c124963eaa7f5158bde9bbbdb6..39b46327afd8e4753d96962fad66792d22b33402 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
@@ -527,6 +527,10 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	if (plat_data.main_irq < 0)
 		return plat_data.main_irq;
 
+	plat_data.cec_irq = platform_get_irq_byname(pdev, "cec");
+	if (plat_data.cec_irq < 0)
+		return plat_data.cec_irq;
+
 	irq = platform_get_irq_byname(pdev, "hpd");
 	if (irq < 0)
 		return irq;

-- 
2.50.1

[PATCH v3 5/6] drm/rockchip: dw_hdmi_qp: Provide ref clock rate in dw_hdmi_qp_plat_data

[Cristian Ciocaltea]

In order to support correct initialization of the timer base in the HDMI
QP IP block, setup platform data to include the required reference clock
rate.

While at it, ensure plat_data is zero-initialized in
dw_hdmi_qp_rockchip_bind().

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
index 39b46327afd8e4753d96962fad66792d22b33402..5280383febe25cf579c306ec1642557600595e58 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
@@ -431,14 +431,15 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 				    void *data)
 {
 	struct platform_device *pdev = to_platform_device(dev);
+	struct dw_hdmi_qp_plat_data plat_data = {};
 	const struct rockchip_hdmi_qp_cfg *cfg;
-	struct dw_hdmi_qp_plat_data plat_data;
 	struct drm_device *drm = data;
 	struct drm_connector *connector;
 	struct drm_encoder *encoder;
 	struct rockchip_hdmi_qp *hdmi;
 	struct resource *res;
 	struct clk_bulk_data *clks;
+	struct clk *ref_clk;
 	int ret, irq, i;
 
 	if (!pdev->dev.of_node)
@@ -508,6 +509,14 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	if (ret < 0)
 		return dev_err_probe(hdmi->dev, ret, "Failed to get clocks\n");
 
+	ref_clk = clk_get(hdmi->dev, "ref");
+	if (IS_ERR(ref_clk))
+		return dev_err_probe(hdmi->dev, PTR_ERR(ref_clk),
+				     "Failed to get ref clock\n");
+
+	plat_data.ref_clk_rate = clk_get_rate(ref_clk);
+	clk_put(ref_clk);
+
 	hdmi->enable_gpio = devm_gpiod_get_optional(hdmi->dev, "enable",
 						    GPIOD_OUT_HIGH);
 	if (IS_ERR(hdmi->enable_gpio))

-- 
2.50.1

[PATCH v3 6/6] arm64: defconfig: Enable DW HDMI QP CEC support

[Cristian Ciocaltea]

Enable support for the CEC interface of the Synopsys DesignWare HDMI QP
IP block.

This is used by all boards based on RK3588 & RK3576 SoCs.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index acb6807d3461384929e84f4c939fcd00c4b509ae..346ef79c1ddd0a317f0b9a8056c680c29a4e0baf 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -966,6 +966,7 @@ CONFIG_DRM_CDNS_MHDP8546=m
 CONFIG_DRM_IMX8MP_DW_HDMI_BRIDGE=m
 CONFIG_DRM_DW_HDMI_AHB_AUDIO=m
 CONFIG_DRM_DW_HDMI_CEC=m
+CONFIG_DRM_DW_HDMI_QP_CEC=y
 CONFIG_DRM_IMX_DCSS=m
 CONFIG_DRM_V3D=m
 CONFIG_DRM_VC4=m

-- 
2.50.1

Re: [PATCH v3 3/6] drm/rockchip: dw_hdmi_qp: Improve error handling with dev_err_probe()

[Daniel Stone]

On Mon, 25 Aug 2025 at 10:57, Cristian Ciocaltea
<cristian.ciocaltea@collabora.com> wrote:
> The error handling in dw_hdmi_qp_rockchip_bind() is quite inconsistent,
> i.e. in some cases the error code is not included in the message, while
> in some other cases there is no check for -EPROBE_DEFER.
>
> Since this is part of the probe path, address the aforementioned issues
> by switching to dev_err_probe(), which also reduces the code a bit.

Reviewed-by: Daniel Stone <daniels@collabora.com>

Re: [PATCH v3 4/6] drm/rockchip: dw_hdmi_qp: Provide CEC IRQ in dw_hdmi_qp_plat_data

[Daniel Stone]

On Mon, 25 Aug 2025 at 10:57, Cristian Ciocaltea
<cristian.ciocaltea@collabora.com> wrote:
> In order to support the CEC interface of the DesignWare HDMI QP IP
> block, setup platform data to include the required IRQ number.

Reviewed-by: Daniel Stone <daniels@collabora.com>

Re: [PATCH v3 1/6] drm/bridge: dw-hdmi-qp: Add CEC support

[Daniel Stone]

Hi Cristian,

On Mon, 25 Aug 2025 at 10:57, Cristian Ciocaltea
<cristian.ciocaltea@collabora.com> wrote:

> +static int dw_hdmi_qp_cec_init(struct drm_bridge *bridge,
> +                              struct drm_connector *connector)
> +{
> +       struct dw_hdmi_qp *hdmi = dw_hdmi_qp_from_bridge(bridge);
> +       struct dw_hdmi_qp_cec *cec = hdmi->cec;
> +       int ret;
> +
> +       if (cec->irq < 0) {
> +               dev_err(hdmi->dev, "Invalid cec irq: %d\n", cec->irq);
> +               return -EINVAL;
> +       }

There is a bisect break here until patch 4/6 as nothing provides the
CEC IRQ, so the whole connector init will fail.

You should either plumb the IRQ through first, or just make it
optional to retain compatibility.

Cheers,
Daniel

Re: [PATCH v3 5/6] drm/rockchip: dw_hdmi_qp: Provide ref clock rate in dw_hdmi_qp_plat_data

[Daniel Stone]

On Mon, 25 Aug 2025 at 10:57, Cristian Ciocaltea
<cristian.ciocaltea@collabora.com> wrote:
> In order to support correct initialization of the timer base in the HDMI
> QP IP block, setup platform data to include the required reference clock
> rate.
>
> While at it, ensure plat_data is zero-initialized in
> dw_hdmi_qp_rockchip_bind().

Reviewed-by: Daniel Stone <daniels@collabora.com>

Re: [PATCH v3 2/6] drm/bridge: dw-hdmi-qp: Fixup timer base setup

[Daniel Stone]

Hi Cristian,

On Mon, 25 Aug 2025 at 10:57, Cristian Ciocaltea
<cristian.ciocaltea@collabora.com> wrote:
> @@ -1255,6 +1254,11 @@ struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
>                 return ERR_PTR(-ENODEV);
>         }
>
> +       if (!plat_data->ref_clk_rate) {
> +               dev_err(dev, "Missing ref_clk rate\n");
> +               return ERR_PTR(-ENODEV);
> +       }

This introduces another bisect cliff, as the Rockchip integration
isn't added until patch 5/6, meaning together with the previous patch
the driver isn't usable between patches 1-5. It would be most sensible
I think to keep a default until the users have been fixed up. But
maybe a better sequence for this series would be:
* dev_err_probe() cleanup (easy, no dependencies)
* add refclk to plat_data (populated but unused)
* use refclk instead of hardcoded frequency in bridge driver, make it mandatory
* add CEC IRQ to plat_data (populated but unused)
* add CEC support to driver, probably make it not mandatory to provide
CEC IRQ in DT since it doesn't seem required for correct operation?
* enable CEC in defconfig

Cheers,
Daniel

Re: [PATCH v3 2/6] drm/bridge: dw-hdmi-qp: Fixup timer base setup

[Cristian Ciocaltea]

Hi Daniel,

On 8/29/25 6:21 PM, Daniel Stone wrote:
> Hi Cristian,
> 
> On Mon, 25 Aug 2025 at 10:57, Cristian Ciocaltea
> <cristian.ciocaltea@collabora.com> wrote:
>> @@ -1255,6 +1254,11 @@ struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
>>                 return ERR_PTR(-ENODEV);
>>         }
>>
>> +       if (!plat_data->ref_clk_rate) {
>> +               dev_err(dev, "Missing ref_clk rate\n");
>> +               return ERR_PTR(-ENODEV);
>> +       }
> 
> This introduces another bisect cliff, as the Rockchip integration
> isn't added until patch 5/6, meaning together with the previous patch
> the driver isn't usable between patches 1-5. It would be most sensible
> I think to keep a default until the users have been fixed up. But
> maybe a better sequence for this series would be:
> * dev_err_probe() cleanup (easy, no dependencies)
> * add refclk to plat_data (populated but unused)
> * use refclk instead of hardcoded frequency in bridge driver, make it mandatory
> * add CEC IRQ to plat_data (populated but unused)
> * add CEC support to driver, probably make it not mandatory to provide
> CEC IRQ in DT since it doesn't seem required for correct operation?
> * enable CEC in defconfig

Yeah, this is pretty similar to how the initial series looked like.  The
current sequence follows Heiko's suggestion, which I (still) think it's the
correct approach.

Both bisect issues are now fixed in v4:

https://lore.kernel.org/all/20250903-rk3588-hdmi-cec-v4-0-fa25163c4b08@collabora.com/

Thanks for the review!

Regards,
Cristian