[PATCH 1/2] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[PATCH 1/2] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[John Ripple]

Add support for DisplayPort to the bridge, which entails the following:
- Register the proper connector type;
- Get and use an interrupt for HPD;
- Properly clear all status bits in the interrupt handler;
- Implement bridge and connector detection;
- Report DSI channel errors;
- Report Display Port errors;
- Disable runtime pm entirely;

Signed-off-by: John Ripple <john.ripple@keysight.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 287 +++++++++++++++++++++++++-
 1 file changed, 281 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 464390372b34..75f9be347b41 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -37,6 +37,8 @@
 
 #define SN_DEVICE_ID_REGS			0x00	/* up to 0x07 */
 #define SN_DEVICE_REV_REG			0x08
+#define SN_RESET_REG				0x09
+#define  SOFT_RESET				BIT(0)
 #define SN_DPPLL_SRC_REG			0x0A
 #define  DPPLL_CLK_SRC_DSICLK			BIT(0)
 #define  REFCLK_FREQ_MASK			GENMASK(3, 1)
@@ -48,7 +50,9 @@
 #define  CHA_DSI_LANES(x)			((x) << 3)
 #define SN_DSIA_CLK_FREQ_REG			0x12
 #define SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG	0x20
+#define SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG	0x21
 #define SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG	0x24
+#define SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG	0x25
 #define SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG	0x2C
 #define SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG	0x2D
 #define  CHA_HSYNC_POLARITY			BIT(7)
@@ -59,9 +63,14 @@
 #define SN_CHA_VERTICAL_BACK_PORCH_REG		0x36
 #define SN_CHA_HORIZONTAL_FRONT_PORCH_REG	0x38
 #define SN_CHA_VERTICAL_FRONT_PORCH_REG		0x3A
+#define SN_COLOR_BAR_REG			0x3C
+#define  COLOR_BAR_EN				BIT(4)
 #define SN_LN_ASSIGN_REG			0x59
 #define  LN_ASSIGN_WIDTH			2
 #define SN_ENH_FRAME_REG			0x5A
+#define  SCRAMBLER_CONTROL_MASK			GENMASK(1, 0)
+#define  SCRAMBLER_CONTROL_STANDARD		0
+#define  SCRAMBLER_CONTROL_ASSR			1
 #define  VSTREAM_ENABLE				BIT(3)
 #define  LN_POLRS_OFFSET			4
 #define  LN_POLRS_MASK				0xf0
@@ -106,10 +115,116 @@
 #define SN_PWM_EN_INV_REG			0xA5
 #define  SN_PWM_INV_MASK			BIT(0)
 #define  SN_PWM_EN_MASK				BIT(1)
+
+#define SN_PSR_REG				0xC8
+#define  PSR_TRAIN				BIT(0)
+#define  PSR_EXIT_VIDEO				BIT(1)
+
+#define SN_IRQ_EN_REG				0xE0
+#define  IRQ_EN					BIT(0)
+#define SN_CHA_IRQ_EN0_REG			0xE1
+#define  CHA_CONTENTION_DET_EN			BIT(7)
+#define  CHA_FALSE_CTRL_EN			BIT(6)
+#define  CHA_TIMEOUT_EN				BIT(5)
+#define  CHA_LP_TX_SYNC_EN			BIT(4)
+#define  CHA_ESC_ENTRY_EN			BIT(3)
+#define  CHA_EOT_SYNC_EN			BIT(2)
+#define  CHA_SOT_SYNC_EN			BIT(1)
+#define  CHA_SOT_BIT_EN				BIT(0)
+
+#define SN_CHB_IRQ_EN0_REG			0xE3
+#define SN_CHB_IRQ_EN1_REG			0xE4
+#define SN_AUX_CMD_EN_REG			0xE5
+
+#define SN_CHA_IRQ_EN1_REG			0xE2
+#define  CHA_DSI_PROTOCOL_EN			BIT(7)
+#define  CHA_INVALID_LENGTH_EN			BIT(5)
+#define  CHA_DATATYPE_EN			BIT(3)
+#define  CHA_CHECKSUM_EN			BIT(2)
+#define  CHA_UNC_ECC_EN				BIT(1)
+#define  CHA_COR_ECC_EN				BIT(0)
+
+#define SN_IRQ_EVENTS_EN_REG			0xE6
+#define  IRQ_HPD_EN				BIT(0)
+#define  HPD_INSERTION_EN			BIT(1)
+#define  HPD_REMOVAL_EN				BIT(2)
+#define  HPD_REPLUG_EN				BIT(3)
+#define  PLL_UNLOCK_EN				BIT(5)
+
+#define SN_DPTL_IRQ_EN0_REG			0xE7
+#define SN_DPTL_IRQ_EN1_REG			0xE8
+#define SN_LT_IRQ_EN_REG			0xE9
+#define SN_CHA_IRQ_STATUS0_REG			0xF0
+#define  CHA_CONTENTION_DET_ERR			BIT(7)
+#define  CHA_FALSE_CTRL_ERR			BIT(6)
+#define  CHA_TIMEOUT_ERR			BIT(5)
+#define  CHA_LP_TX_SYNC_ERR			BIT(4)
+#define  CHA_ESC_ERRTRY_ERR			BIT(3)
+#define  CHA_EOT_SYNC_ERR			BIT(2)
+#define  CHA_SOT_SYNC_ERR			BIT(1)
+#define  CHA_SOT_BIT_ERR			BIT(0)
+#define SN_CHA_IRQ_STATUS1_REG			0xF1
+#define  CHA_DSI_PROTOCOL_ERR			BIT(7)
+#define  CHA_INVALID_LENGTH_ERR			BIT(5)
+#define  CHA_DATATYPE_ERR			BIT(3)
+#define  CHA_CHECKSUM_ERR			BIT(2)
+#define  CHA_UNC_ECC_ERR			BIT(1)
+#define  CHA_COR_ECC_ERR			BIT(0)
+#define SN_CHB_IRQ_STATUS0_REG			0xF2
+#define SN_CHB_IRQ_STATUS1_REG			0xF3
+#define  CHB_FALSE_CTRL_ERR			BIT(6)
+#define  CHB_LP_TX_SYNC_ERR			BIT(4)
+#define  CHB_EOT_SYNC_ERR			BIT(2)
+#define  CHB_SOT_SYNC_ERR			BIT(1)
+#define  CHB_SOT_BIT_ERR			BIT(0)
+
+#define  CHB_DSI_PROTOCOL_ERR			BIT(7)
+#define  CHB_INVALID_LENGTH_ERR			BIT(5)
+#define  CHB_DATATYPE_ERR			BIT(3)
+#define  CHB_CHECKSUM_ERR			BIT(2)
+#define  CHB_UNC_ECC_ERR			BIT(1)
+#define  CHB_COR_ECC_ERR			BIT(0)
 #define SN_AUX_CMD_STATUS_REG			0xF4
 #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
 #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
 #define  AUX_IRQ_STATUS_NAT_I2C_FAIL		BIT(6)
+#define  AUX_IRQ_STATUS_I2C_DEFR		BIT(7)
+#define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
+#define  AUX_IRQ_STATUS_AUX_DEFR		BIT(4)
+#define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
+#define  AUX_IRQ_STATUS_SEND_INT		BIT(0)
+#define SN_IRQ_STATUS_REG			0xF5
+#define  HPD_PLL_UNLOCK				BIT(5)
+#define  HPD_REPLUG_STATUS			BIT(3)
+#define  HPD_REMOVAL_STATUS			BIT(2)
+#define  HPD_INSERTION_STATUS			BIT(1)
+#define  IRQ_HPD_STATUS				BIT(0)
+#define SN_IRQ_EVENTS_DPTL_REG_1		0xF6
+#define  VIDEO_WIDTH_PROG_ERR			BIT(7)
+#define  LOSS_OF_DP_SYNC_LOCK_ERR		BIT(6)
+#define  DPTL_UNEXPECTED_DATA_ERR		BIT(5)
+#define  DPTL_UNEXPECTED_SECDATA_ERR		BIT(4)
+#define  DPTL_UNEXPECTED_DATA_END_ERR		BIT(3)
+#define  DPTL_UNEXPECTED_PIXEL_DATA_ERR		BIT(2)
+#define  DPTL_UNEXPECTED_HSYNC_ERR		BIT(1)
+#define  DPTL_UNEXPECTED_VSYNC_ERR		BIT(0)
+#define SN_IRQ_EVENTS_DPTL_REG_2		0xF7
+#define  DPTL_SECONDARY_DATA_PACKET_PROG_ERR	BIT(1)
+#define  DPTL_DATA_UNDERRUN_ERR			BIT(0)
+#define SN_IRQ_LT				0xF8
+#define  LT_EQ_CR_ERR				BIT(5)
+#define  LT_EQ_LPCNT_ERR			BIT(4)
+#define  LT_CR_MAXVOD_ERR			BIT(3)
+#define  LT_CR_LPCNT_ERR			BIT(2)
+#define  LT_FAIL				BIT(1)
+#define  LT_PASS				BIT(0)
+
+#define SN_PAGE_SELECT_REG			0xFF
+#define  SN_PAGE_SELECT_STANDARD		0x00
+#define  SN_PAGE_SELECT_TEST			0x07
+#define SN_ASSR_OVERRIDE_REG			0x16
+#define SN_ASSR_OVERRIDE_RO			0x00
+#define SN_ASSR_OVERRIDE_RW			0x01
 
 #define MIN_DSI_CLK_FREQ_MHZ	40
 
@@ -151,6 +266,7 @@
  * @dp_lanes:     Count of dp_lanes we're using.
  * @ln_assign:    Value to program to the LN_ASSIGN register.
  * @ln_polrs:     Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG.
+ * @no_hpd:       If true then the hot-plug functionality is disabled.
  * @comms_enabled: If true then communication over the aux channel is enabled.
  * @comms_mutex:   Protects modification of comms_enabled.
  *
@@ -189,6 +305,7 @@ struct ti_sn65dsi86 {
 	int				dp_lanes;
 	u8				ln_assign;
 	u8				ln_polrs;
+	bool			no_hpd;
 	bool				comms_enabled;
 	struct mutex			comms_mutex;
 
@@ -987,6 +1104,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
 	int ret;
 	int i;
 
+	/*
+	 * DP data rate and lanes number will be set by the bridge by writing
+	 * to DP_LINK_BW_SET and DP_LANE_COUNT_SET.
+	 */
+
 	/* set dp clk frequency value */
 	regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
 			   DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx));
@@ -1105,7 +1227,10 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
 
 	valid_rates = ti_sn_bridge_read_valid_rates(pdata);
 
-	/* Train until we run out of rates */
+	/*
+	 * Train until we run out of rates. Start with the lowest possible rate
+	 * and move up in order to select the lowest working functioning point.
+	 */
 	for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata, state, bpp);
 	     dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut);
 	     dp_rate_idx++) {
@@ -1116,9 +1241,13 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
 		if (!ret)
 			break;
 	}
-	if (ret) {
+	if (ret || dp_rate_idx == ARRAY_SIZE(ti_sn_bridge_dp_rate_lut)) {
 		DRM_DEV_ERROR(pdata->dev, "%s (%d)\n", last_err_str, ret);
 		return;
+	} else {
+		DRM_DEV_INFO(pdata->dev,
+			     "Link training selected rate: %u MHz\n",
+			     ti_sn_bridge_dp_rate_lut[dp_rate_idx]);
 	}
 
 	/* config video parameters */
@@ -1298,6 +1427,69 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
 	return 0;
 }
 
+static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
+{
+	struct ti_sn65dsi86 *pdata = private;
+	struct drm_device *dev = pdata->bridge.dev;
+	u32 status = 0;
+	bool hpd_event = false;
+
+	regmap_read(pdata->regmap, SN_IRQ_STATUS_REG, &status);
+	if (status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS))
+		hpd_event = true;
+
+	/*
+	 * Writing back the status register to acknowledge the IRQ apparently
+	 * needs to take place right after reading it or the bridge will get
+	 * confused and fail to report subsequent IRQs.
+	 */
+	if (status)
+		drm_err(dev, "(SN_IRQ_STATUS_REG = %#x)\n", status);
+	regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
+
+	regmap_read(pdata->regmap, SN_CHA_IRQ_STATUS0_REG, &status);
+	if (status)
+		drm_err(dev, "DSI CHA error reported (status0 = %#x)\n", status);
+	regmap_write(pdata->regmap, SN_CHA_IRQ_STATUS0_REG, status);
+	if (status)
+		regmap_write(pdata->regmap, SN_RESET_REG, SOFT_RESET);
+
+	regmap_read(pdata->regmap, SN_CHA_IRQ_STATUS1_REG, &status);
+	if (status)
+		drm_err(dev, "DSI CHA error reported (status1 = %#x)\n", status);
+	regmap_write(pdata->regmap, SN_CHA_IRQ_STATUS1_REG, status);
+	if (status)
+		regmap_write(pdata->regmap, SN_RESET_REG, SOFT_RESET);
+
+	/* Dirty hack to reset the soft if any error occurs on the DP side */
+	regmap_read(pdata->regmap, SN_IRQ_EVENTS_DPTL_REG_1, &status);
+	if (status)
+		drm_err(dev, "(SN_IRQ_EVENTS_DPTL_REG_1 = %#x)\n", status);
+	regmap_write(pdata->regmap, SN_IRQ_EVENTS_DPTL_REG_1, status);
+	if (status)
+		regmap_write(pdata->regmap, SN_RESET_REG, SOFT_RESET);
+
+	regmap_read(pdata->regmap, SN_IRQ_EVENTS_DPTL_REG_2, &status);
+	if (status)
+		drm_err(dev, "(SN_IRQ_EVENTS_DPTL_REG_2 = %#x)\n", status);
+	regmap_write(pdata->regmap, SN_IRQ_EVENTS_DPTL_REG_2, status);
+	if (status)
+		regmap_write(pdata->regmap, SN_RESET_REG, SOFT_RESET);
+
+	regmap_read(pdata->regmap, SN_IRQ_LT, &status);
+	if (status)
+		drm_err(dev, "(SN_IRQ_LT = %#x)\n", status);
+	regmap_write(pdata->regmap, SN_IRQ_LT, status);
+	if (status)
+		regmap_write(pdata->regmap, SN_RESET_REG, SOFT_RESET);
+
+	/* Only send the HPD event if we are bound with a device. */
+	if (dev && !pdata->no_hpd && hpd_event)
+		drm_kms_helper_hotplug_event(dev);
+
+	return IRQ_HANDLED;
+}
+
 static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 			      const struct auxiliary_device_id *id)
 {
@@ -1335,9 +1527,48 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 		 * for eDP.
 		 */
 		mutex_lock(&pdata->comms_mutex);
-		if (pdata->comms_enabled)
+		if (pdata->comms_enabled) {
+			/* Enable HPD and PLL events. */
+			regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
+					PLL_UNLOCK_EN |
+					HPD_REPLUG_EN |
+					HPD_REMOVAL_EN |
+					HPD_INSERTION_EN |
+					IRQ_HPD_EN);
+
+			/* Enable DSI CHA error reporting events. */
+			regmap_write(pdata->regmap, SN_CHA_IRQ_EN0_REG,
+					CHA_CONTENTION_DET_EN |
+					CHA_FALSE_CTRL_EN |
+					CHA_TIMEOUT_EN |
+					CHA_LP_TX_SYNC_EN |
+					CHA_ESC_ENTRY_EN |
+					CHA_EOT_SYNC_EN |
+					CHA_SOT_SYNC_EN |
+					CHA_SOT_BIT_EN);
+
+			regmap_write(pdata->regmap, SN_CHA_IRQ_EN1_REG,
+					CHA_DSI_PROTOCOL_EN |
+					CHA_INVALID_LENGTH_EN |
+					CHA_DATATYPE_EN |
+					CHA_CHECKSUM_EN |
+					CHA_UNC_ECC_EN |
+					CHA_COR_ECC_EN);
+
+			/* Disable DSI CHB error reporting events. */
+			regmap_write(pdata->regmap, SN_CHB_IRQ_EN0_REG, 0);
+			regmap_write(pdata->regmap, SN_CHB_IRQ_EN1_REG, 0);
+
 			regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
-					   HPD_DISABLE, 0);
+					HPD_DISABLE, 0);
+
+			/* Enable DisplayPort error reporting events. */
+			regmap_write(pdata->regmap, SN_DPTL_IRQ_EN0_REG, 0xFF);
+			regmap_write(pdata->regmap, SN_DPTL_IRQ_EN1_REG, 0xFF);
+
+			regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN,
+			IRQ_EN);
+		}
 		mutex_unlock(&pdata->comms_mutex);
 	}
 
@@ -1884,8 +2115,12 @@ static inline void ti_sn_gpio_unregister(void) {}
 
 static void ti_sn65dsi86_runtime_disable(void *data)
 {
-	pm_runtime_dont_use_autosuspend(data);
-	pm_runtime_disable(data);
+	if (pm_runtime_enabled(data)) {
+		pm_runtime_dont_use_autosuspend(data);
+		pm_runtime_disable(data);
+	} else {
+		ti_sn65dsi86_suspend(data);
+	}
 }
 
 static int ti_sn65dsi86_parse_regulators(struct ti_sn65dsi86 *pdata)
@@ -1943,6 +2178,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 		return dev_err_probe(dev, PTR_ERR(pdata->refclk),
 				     "failed to get reference clock\n");
 
+	pdata->no_hpd = of_property_read_bool(pdata->host_node, "no-hpd");
 	pm_runtime_enable(dev);
 	pm_runtime_set_autosuspend_delay(pdata->dev, 500);
 	pm_runtime_use_autosuspend(pdata->dev);
@@ -1950,6 +2186,45 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
+	if (client->irq && !pdata->no_hpd) {
+		enum drm_connector_status status;
+
+		pm_runtime_disable(pdata->dev);
+		ti_sn65dsi86_resume(pdata->dev);
+		ret = devm_request_threaded_irq(pdata->dev, client->irq, NULL,
+						ti_sn_bridge_interrupt,
+						IRQF_TRIGGER_RISING |
+						IRQF_TRIGGER_FALLING |
+						IRQF_ONESHOT,
+						"ti_sn65dsi86", pdata);
+
+		/*
+		 * Cleaning status register at probe is needed because if the irq is
+		 * already high, the rising/falling condition will never occurs
+		 */
+		regmap_read(pdata->regmap, SN_IRQ_STATUS_REG, &status);
+		regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
+		regmap_read(pdata->regmap, SN_CHA_IRQ_STATUS0_REG, &status);
+		regmap_write(pdata->regmap, SN_CHA_IRQ_STATUS0_REG, status);
+		regmap_read(pdata->regmap, SN_CHA_IRQ_STATUS1_REG, &status);
+		regmap_write(pdata->regmap, SN_CHA_IRQ_STATUS1_REG, status);
+		regmap_read(pdata->regmap, SN_IRQ_EVENTS_DPTL_REG_1, &status);
+		regmap_write(pdata->regmap, SN_IRQ_EVENTS_DPTL_REG_1, status);
+		regmap_read(pdata->regmap, SN_IRQ_EVENTS_DPTL_REG_2, &status);
+		regmap_write(pdata->regmap, SN_IRQ_EVENTS_DPTL_REG_2, status);
+		regmap_read(pdata->regmap, SN_IRQ_LT, &status);
+		regmap_write(pdata->regmap, SN_IRQ_LT, status);
+
+		if (ret) {
+			return dev_err_probe(dev, ret,
+					     "failed to request interrupt\n");
+		}
+	} else {
+		pm_runtime_enable(dev);
+		pm_runtime_set_autosuspend_delay(pdata->dev, 500);
+		pm_runtime_use_autosuspend(pdata->dev);
+	}
+
 	pm_runtime_get_sync(dev);
 	ret = regmap_bulk_read(pdata->regmap, SN_DEVICE_ID_REGS, id_buf, ARRAY_SIZE(id_buf));
 	pm_runtime_put_autosuspend(dev);

[PATCH 2/2] drm/bridge: ti-sn65dsi86: break probe dependency loop

[John Ripple]

The commit c3b75d4734cb ("drm/bridge: sn65dsi86: Register and attach our
DSI device at probe") was intended to prevent probe ordering issues and
created the ti_sn_attach_host function.

In practice, I found the following when using the nwl-dsi driver:
 - ti_sn_bridge_probe happens and it adds the i2c bridge. Then
   ti_sn_attach_host runs (in the ti_sn_bridge_probe function) and fails to
   find the dsi host which then returns to ti_sn_bridge_probe and removes
   the i2c bridge because of the failure.
 - The nwl_dsi_probe then runs and adds dsi host to the host list and then
   looks for the i2c bridge, which is now gone, so it fails. This loop
   continues for the entire boot sequence.

Looking at the other drivers (like simple-bridge.c) they end the probe
function after attaching the bridge with no option to remove the bridge in
the probe. Moving the ti_sn_attach_host to the ti_sn_bridge_attach function
follows the format of other drivers and ensures the i2c bridge won't be
removed while probing is still occurring fixing the dependency loop.

Signed-off-by: John Ripple <john.ripple@keysight.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 75f9be347b41..58dfb0f39cea 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -815,6 +815,7 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 	int ret;
+	struct auxiliary_device *adev = pdata->bridge_aux;
 
 	pdata->aux.drm_dev = bridge->dev;
 	ret = drm_dp_aux_register(&pdata->aux);
@@ -823,6 +824,12 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 		return ret;
 	}
 
+	ret = ti_sn_attach_host(adev, pdata);
+	if (ret) {
+		dev_err_probe(&adev->dev, ret, "failed to attach dsi host\n");
+		goto err_remove_bridge;
+	}
+
 	/*
 	 * Attach the next bridge.
 	 * We never want the next bridge to *also* create a connector.
@@ -849,6 +856,9 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 err_initted_aux:
 	drm_dp_aux_unregister(&pdata->aux);
 	return ret;
+err_remove_bridge:
+	drm_bridge_remove(&pdata->bridge);
+	return ret;
 }
 
 static void ti_sn_bridge_detach(struct drm_bridge *bridge)
@@ -1574,17 +1584,7 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 
 	drm_bridge_add(&pdata->bridge);
 
-	ret = ti_sn_attach_host(adev, pdata);
-	if (ret) {
-		dev_err_probe(&adev->dev, ret, "failed to attach dsi host\n");
-		goto err_remove_bridge;
-	}
-
 	return 0;
-
-err_remove_bridge:
-	drm_bridge_remove(&pdata->bridge);
-	return ret;
 }
 
 static void ti_sn_bridge_remove(struct auxiliary_device *adev)

Re: [PATCH 1/2] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[Doug Anderson]

Hi,

On Wed, Aug 20, 2025 at 8:24 AM John Ripple <john.ripple@keysight.com> wrote:
>
> Add support for DisplayPort to the bridge, which entails the following:
> - Register the proper connector type;
> - Get and use an interrupt for HPD;
> - Properly clear all status bits in the interrupt handler;
> - Implement bridge and connector detection;
> - Report DSI channel errors;
> - Report Display Port errors;
> - Disable runtime pm entirely;
>
> Signed-off-by: John Ripple <john.ripple@keysight.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 287 +++++++++++++++++++++++++-
>  1 file changed, 281 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 464390372b34..75f9be347b41 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -37,6 +37,8 @@
>
>  #define SN_DEVICE_ID_REGS                      0x00    /* up to 0x07 */
>  #define SN_DEVICE_REV_REG                      0x08
> +#define SN_RESET_REG                           0x09
> +#define  SOFT_RESET                            BIT(0)
>  #define SN_DPPLL_SRC_REG                       0x0A
>  #define  DPPLL_CLK_SRC_DSICLK                  BIT(0)
>  #define  REFCLK_FREQ_MASK                      GENMASK(3, 1)
> @@ -48,7 +50,9 @@
>  #define  CHA_DSI_LANES(x)                      ((x) << 3)
>  #define SN_DSIA_CLK_FREQ_REG                   0x12
>  #define SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG      0x20
> +#define SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG     0x21

This and many other #defines you added aren't actually used in your patch.

One can make an argument that adding #defines for all the registers
and bits in the datasheet (even if they're not used) is a good thing.
...but one can also make the argument that we should avoid cluttering
the driver with extra #defines until they're needed, especially since
the datasheet for this part is public. We can certainly have that
debate if you want, but let's please do it in a separate patch. Adding
all of these defines is not required for your HPD case so shouldn't be
in the HPD patch.


> @@ -189,6 +305,7 @@ struct ti_sn65dsi86 {
>         int                             dp_lanes;
>         u8                              ln_assign;
>         u8                              ln_polrs;
> +       bool                    no_hpd;

nit: in my editor the indentation seems wrong here.


> @@ -987,6 +1104,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
>         int ret;
>         int i;
>
> +       /*
> +        * DP data rate and lanes number will be set by the bridge by writing
> +        * to DP_LINK_BW_SET and DP_LANE_COUNT_SET.
> +        */
> +

This comment seems unrelated to your patch. If we want to add it,
please do so in a separate patch.

Also, please match the names used elsewhere in the file. Searching the
file for DP_LINK_BW_SET and DP_LANE_COUNT_SET shows no hits.


> @@ -1105,7 +1227,10 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
>
>         valid_rates = ti_sn_bridge_read_valid_rates(pdata);
>
> -       /* Train until we run out of rates */
> +       /*
> +        * Train until we run out of rates. Start with the lowest possible rate
> +        * and move up in order to select the lowest working functioning point.
> +        */

Similar to the last comment. If we want to improve comments it should
be done in a separate patch.


>         for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata, state, bpp);
>              dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut);
>              dp_rate_idx++) {
> @@ -1116,9 +1241,13 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
>                 if (!ret)
>                         break;
>         }
> -       if (ret) {
> +       if (ret || dp_rate_idx == ARRAY_SIZE(ti_sn_bridge_dp_rate_lut)) {

Why did you need to change? We'll always run through the above loop at
least once, right? That means we'll always set "ret"


>                 DRM_DEV_ERROR(pdata->dev, "%s (%d)\n", last_err_str, ret);
>                 return;
> +       } else {
> +               DRM_DEV_INFO(pdata->dev,
> +                            "Link training selected rate: %u MHz\n",
> +                            ti_sn_bridge_dp_rate_lut[dp_rate_idx]);

Again, this should be in a separate patch.

Also: this is logspam. If there's really a reason we need to logspam
every time we link train then that reason needs to be justified. IMO
it would be fine to make this a "debug" level log, but I'd be against
leaving it at INFO level.


> @@ -1298,6 +1427,69 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
>         return 0;
>  }
>
> +static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
> +{
> +       struct ti_sn65dsi86 *pdata = private;
> +       struct drm_device *dev = pdata->bridge.dev;
> +       u32 status = 0;
> +       bool hpd_event = false;
> +
> +       regmap_read(pdata->regmap, SN_IRQ_STATUS_REG, &status);

Please check the return code from regmap_read(), since i2c transfers
can fail. I know this driver isn't terribly good about it with
existing code, but that doesn't mean we should keep being bad about it
with new code.



> +       if (status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS))
> +               hpd_event = true;

hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);


> +       /*
> +        * Writing back the status register to acknowledge the IRQ apparently
> +        * needs to take place right after reading it or the bridge will get
> +        * confused and fail to report subsequent IRQs.
> +        */

Really? Is this documented?

In general for edge triggered interrupts you always want to
acknowledge before you actually act on them. Is it just that, or does
this specifically have to be before other stuff below?


> +       if (status)
> +               drm_err(dev, "(SN_IRQ_STATUS_REG = %#x)\n", status);

Getting interrupts is an "error" ? That doesn't seem right.


> +       regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);

Shouldn't it only write if non-zero?


> +       regmap_read(pdata->regmap, SN_CHA_IRQ_STATUS0_REG, &status);
> +       if (status)
> +               drm_err(dev, "DSI CHA error reported (status0 = %#x)\n", status);
> +       regmap_write(pdata->regmap, SN_CHA_IRQ_STATUS0_REG, status);
> +       if (status)
> +               regmap_write(pdata->regmap, SN_RESET_REG, SOFT_RESET);
> +
> +       regmap_read(pdata->regmap, SN_CHA_IRQ_STATUS1_REG, &status);
> +       if (status)
> +               drm_err(dev, "DSI CHA error reported (status1 = %#x)\n", status);
> +       regmap_write(pdata->regmap, SN_CHA_IRQ_STATUS1_REG, status);
> +       if (status)
> +               regmap_write(pdata->regmap, SN_RESET_REG, SOFT_RESET);
> +
> +       /* Dirty hack to reset the soft if any error occurs on the DP side */
> +       regmap_read(pdata->regmap, SN_IRQ_EVENTS_DPTL_REG_1, &status);
> +       if (status)
> +               drm_err(dev, "(SN_IRQ_EVENTS_DPTL_REG_1 = %#x)\n", status);
> +       regmap_write(pdata->regmap, SN_IRQ_EVENTS_DPTL_REG_1, status);
> +       if (status)
> +               regmap_write(pdata->regmap, SN_RESET_REG, SOFT_RESET);
> +
> +       regmap_read(pdata->regmap, SN_IRQ_EVENTS_DPTL_REG_2, &status);
> +       if (status)
> +               drm_err(dev, "(SN_IRQ_EVENTS_DPTL_REG_2 = %#x)\n", status);
> +       regmap_write(pdata->regmap, SN_IRQ_EVENTS_DPTL_REG_2, status);
> +       if (status)
> +               regmap_write(pdata->regmap, SN_RESET_REG, SOFT_RESET);
> +
> +       regmap_read(pdata->regmap, SN_IRQ_LT, &status);
> +       if (status)
> +               drm_err(dev, "(SN_IRQ_LT = %#x)\n", status);
> +       regmap_write(pdata->regmap, SN_IRQ_LT, status);
> +       if (status)
> +               regmap_write(pdata->regmap, SN_RESET_REG, SOFT_RESET);

If we want to start handling error interrupts, let's do it in a
separate patch please. Then we can talk about what kind of value this
brings and how you've tested it.


> @@ -1335,9 +1527,48 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
>                  * for eDP.
>                  */
>                 mutex_lock(&pdata->comms_mutex);
> -               if (pdata->comms_enabled)
> +               if (pdata->comms_enabled) {
> +                       /* Enable HPD and PLL events. */
> +                       regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
> +                                       PLL_UNLOCK_EN |
> +                                       HPD_REPLUG_EN |
> +                                       HPD_REMOVAL_EN |
> +                                       HPD_INSERTION_EN |
> +                                       IRQ_HPD_EN);
> +
> +                       /* Enable DSI CHA error reporting events. */
> +                       regmap_write(pdata->regmap, SN_CHA_IRQ_EN0_REG,
> +                                       CHA_CONTENTION_DET_EN |
> +                                       CHA_FALSE_CTRL_EN |
> +                                       CHA_TIMEOUT_EN |
> +                                       CHA_LP_TX_SYNC_EN |
> +                                       CHA_ESC_ENTRY_EN |
> +                                       CHA_EOT_SYNC_EN |
> +                                       CHA_SOT_SYNC_EN |
> +                                       CHA_SOT_BIT_EN);
> +
> +                       regmap_write(pdata->regmap, SN_CHA_IRQ_EN1_REG,
> +                                       CHA_DSI_PROTOCOL_EN |
> +                                       CHA_INVALID_LENGTH_EN |
> +                                       CHA_DATATYPE_EN |
> +                                       CHA_CHECKSUM_EN |
> +                                       CHA_UNC_ECC_EN |
> +                                       CHA_COR_ECC_EN);
> +
> +                       /* Disable DSI CHB error reporting events. */
> +                       regmap_write(pdata->regmap, SN_CHB_IRQ_EN0_REG, 0);
> +                       regmap_write(pdata->regmap, SN_CHB_IRQ_EN1_REG, 0);
> +
>                         regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
> -                                          HPD_DISABLE, 0);
> +                                       HPD_DISABLE, 0);
> +
> +                       /* Enable DisplayPort error reporting events. */
> +                       regmap_write(pdata->regmap, SN_DPTL_IRQ_EN0_REG, 0xFF);
> +                       regmap_write(pdata->regmap, SN_DPTL_IRQ_EN1_REG, 0xFF);
> +
> +                       regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN,

As per previous comment, enabling the error interrupts should be in a
separate patch.


> @@ -1884,8 +2115,12 @@ static inline void ti_sn_gpio_unregister(void) {}
>
>  static void ti_sn65dsi86_runtime_disable(void *data)
>  {
> -       pm_runtime_dont_use_autosuspend(data);
> -       pm_runtime_disable(data);
> +       if (pm_runtime_enabled(data)) {
> +               pm_runtime_dont_use_autosuspend(data);
> +               pm_runtime_disable(data);
> +       } else {
> +               ti_sn65dsi86_suspend(data);
> +       }

See below--we should leverage the existing code to keep PM Runtime on
when HPD is used.


> @@ -1943,6 +2178,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
>                 return dev_err_probe(dev, PTR_ERR(pdata->refclk),
>                                      "failed to get reference clock\n");
>
> +       pdata->no_hpd = of_property_read_bool(pdata->host_node, "no-hpd");
>         pm_runtime_enable(dev);
>         pm_runtime_set_autosuspend_delay(pdata->dev, 500);
>         pm_runtime_use_autosuspend(pdata->dev);
> @@ -1950,6 +2186,45 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
>         if (ret)
>                 return ret;
>
> +       if (client->irq && !pdata->no_hpd) {
> +               enum drm_connector_status status;
> +
> +               pm_runtime_disable(pdata->dev);

You can't permanently disable pm_runtime (AKA always keep things
powered on) based on just having an IRQ and lacking "no-hpd".

Instead, this decision needs to be made based on "DP" vs. "eDP". When
using the bridge in "eDP" mode you still want to be able to power the
bridge down even if interrupts and HPD are hooked up. This is because
in the "eDP" case you always just assume that the panel is there and
you don't need to detect it. This allows you to go into a lower power
state when the eDP panel is turned off because you can power the
bridge off.

We also already handle powering the bridge on in that case. See commit
55e8ff842051 ("drm/bridge: ti-sn65dsi86: Add HPD for DisplayPort
connector type"). You can see there that when
ti_sn_bridge_hpd_enable() is called we power the bridge on and it will
stay on.

What you should be doing is hooking into the ti_sn_bridge_hpd_enable()
function. When you see that then you should enable the interrupt. IMO
in probe you could still request the HPD interrupt. One way to do this
would be to set the `SN_IRQ_EVENTS_EN_REG` to turn on the HPD
interrupts in ti_sn_bridge_hpd_enable() after grabbing the runtime PM
reference and turn them off in ti_sn_bridge_hpd_disable() before
dropping it.



-Doug

Re: [PATCH 2/2] drm/bridge: ti-sn65dsi86: break probe dependency loop

[Doug Anderson]

Hi,

On Wed, Aug 20, 2025 at 8:24 AM John Ripple <john.ripple@keysight.com> wrote:
>
> The commit c3b75d4734cb ("drm/bridge: sn65dsi86: Register and attach our
> DSI device at probe") was intended to prevent probe ordering issues and
> created the ti_sn_attach_host function.
>
> In practice, I found the following when using the nwl-dsi driver:
>  - ti_sn_bridge_probe happens and it adds the i2c bridge. Then
>    ti_sn_attach_host runs (in the ti_sn_bridge_probe function) and fails to
>    find the dsi host which then returns to ti_sn_bridge_probe and removes
>    the i2c bridge because of the failure.
>  - The nwl_dsi_probe then runs and adds dsi host to the host list and then
>    looks for the i2c bridge, which is now gone, so it fails. This loop
>    continues for the entire boot sequence.

Which i2c bridge are you talking about? You mean the one created by
i2c_add_adapter() in drm_dp_aux_register()? I guess I'm confused about
why the DSI probe routine would even be looking for that adapter.

In any case, I don't _think_ your patch is valid. Specifically, if you
notice ti_sn_attach_host() can return "-EPROBE_DEFER". That's a valid
error code to return from a probe routine but I don't think it's a
valid error code to return from a bridge attach function, is it?

Re: [PATCH 2/2] drm/bridge: ti-sn65dsi86: break probe dependency loop

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1649 bytes --]

On Fri, Aug 29, 2025 at 09:40:30AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Wed, Aug 20, 2025 at 8:24 AM John Ripple <john.ripple@keysight.com> wrote:
> >
> > The commit c3b75d4734cb ("drm/bridge: sn65dsi86: Register and attach our
> > DSI device at probe") was intended to prevent probe ordering issues and
> > created the ti_sn_attach_host function.
> >
> > In practice, I found the following when using the nwl-dsi driver:
> >  - ti_sn_bridge_probe happens and it adds the i2c bridge. Then
> >    ti_sn_attach_host runs (in the ti_sn_bridge_probe function) and fails to
> >    find the dsi host which then returns to ti_sn_bridge_probe and removes
> >    the i2c bridge because of the failure.
> >  - The nwl_dsi_probe then runs and adds dsi host to the host list and then
> >    looks for the i2c bridge, which is now gone, so it fails. This loop
> >    continues for the entire boot sequence.
> 
> Which i2c bridge are you talking about? You mean the one created by
> i2c_add_adapter() in drm_dp_aux_register()? I guess I'm confused about
> why the DSI probe routine would even be looking for that adapter.
> 
> In any case, I don't _think_ your patch is valid. Specifically, if you
> notice ti_sn_attach_host() can return "-EPROBE_DEFER". That's a valid
> error code to return from a probe routine but I don't think it's a
> valid error code to return from a bridge attach function, is it?

It's not documented anywhere though, so we'd need to document (and
assess) if it's acceptable first.

We should also amend
https://docs.kernel.org/gpu/drm-kms-helpers.html#special-care-with-mipi-dsi-bridges

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 2/2] drm/bridge: ti-sn65dsi86: break probe dependency loop

[John Ripple]

Hi,

>Which i2c bridge are you talking about? You mean the one created by
>i2c_add_adapter() in drm_dp_aux_register()? I guess I'm confused about
>why the DSI probe routine would even be looking for that adapter.

The i2c bridge I was seeing was created by the drm_bridge_add() function
in the ti_sn_bridge_probe() function. Without moving the ti_sn_attach_host()
function out of the ti_sn_bridge_probe() function I kept getting stuck in a 
loop during boot where the bridge would never come up. It's possible this
could be a unique interaction with the hardware I'm using and the nwl-dsi
driver. 

>In any case, I don't _think_ your patch is valid. Specifically, if you
>notice ti_sn_attach_host() can return "-EPROBE_DEFER". That's a valid
>error code to return from a probe routine but I don't think it's a
>valid error code to return from a bridge attach function, is it?

What error code would you suggest?

Re: [PATCH 2/2] drm/bridge: ti-sn65dsi86: break probe dependency loop

[Doug Anderson]

Hi,

On Tue, Sep 2, 2025 at 9:23 AM John Ripple <john.ripple@keysight.com> wrote:
>
> Hi,
>
> >Which i2c bridge are you talking about? You mean the one created by
> >i2c_add_adapter() in drm_dp_aux_register()? I guess I'm confused about
> >why the DSI probe routine would even be looking for that adapter.
>
> The i2c bridge I was seeing was created by the drm_bridge_add() function
> in the ti_sn_bridge_probe() function. Without moving the ti_sn_attach_host()
> function out of the ti_sn_bridge_probe() function I kept getting stuck in a
> loop during boot where the bridge would never come up. It's possible this
> could be a unique interaction with the hardware I'm using and the nwl-dsi
> driver.

Sorry, I still don't really know what i2c bridge you're talking about
here. At this point there are a number of different MIPI hosts that
are using ti-sn65dsi86 and they don't seem to run into this, so
probably digging into your MIPI host to see exactly what it's doing
makes sense. Where exactly is the nwl-dsi driver trying to acquire
this bridge and failing?


> >In any case, I don't _think_ your patch is valid. Specifically, if you
> >notice ti_sn_attach_host() can return "-EPROBE_DEFER". That's a valid
> >error code to return from a probe routine but I don't think it's a
> >valid error code to return from a bridge attach function, is it?
>
> What error code would you suggest?

You can't just change the error code. The problem here is that, in
general, there is no guarantee of the order that devices are probed in
Linux. The general solution for this in Linux is for drivers to find
all the devices that they depend on during their probe routine. If any
are missing then they return -EPROBE_DEFER and the system will try
again once more things are loaded. In the case of ti-sn65dsi86 we need
the MIPI host device so we find it at probe time. If it's not there
then we want to try again later.

The whole "try again" logic for -EPROBE_DEFER is only guaranteed in
certain contexts. Generally it's reserved for probe. ...but that logic
_could_ be extended to other contexts. It's possible it could be
extended to bridge attach, but one would have to make sure it actually
is (I haven't checked) and, as Maxime says, it should be documented.

I suppose it's also possible that when ti_sn_bridge_attach() is
called, it's guaranteed that of_find_mipi_dsi_host_by_node() won't
return NULL. If you can prove this by looking through the DRM code
_then_ you could probably make your change and just change the error
code.


To sum it up

1. Ideally you can fix the nwl-dsi driver to work however everyone
else is working.

2. If you can't then your commit message needs to prove that it's safe
to move the code to the "attach" routine. You either need to prove
that it's guaranteed that of_find_mipi_dsi_host_by_node() won't return
NULL when called from ti_sn_bridge_attach() or you need to prove that
returning -EPROBE_DEFER in this case is safe.

3. In either case, updating the docs that Maxime pointed to would be useful.


-Doug

[PATCH V2] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[John Ripple]

Add support for DisplayPort to the bridge, which entails the following:
- Get and use an interrupt for HPD;
- Properly clear all status bits in the interrupt handler;

Signed-off-by: John Ripple <john.ripple@keysight.com>
---
V1 -> V2: Cleaned up coding style and addressed review comments

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 93 +++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index ae0d08e5e960..ad0393212279 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -106,10 +106,24 @@
 #define SN_PWM_EN_INV_REG			0xA5
 #define  SN_PWM_INV_MASK			BIT(0)
 #define  SN_PWM_EN_MASK				BIT(1)
+
+#define SN_IRQ_EN_REG				0xE0
+#define  IRQ_EN					BIT(0)
+
+#define SN_IRQ_EVENTS_EN_REG			0xE6
+#define  IRQ_HPD_EN				BIT(0)
+#define  HPD_INSERTION_EN			BIT(1)
+#define  HPD_REMOVAL_EN				BIT(2)
+#define  HPD_REPLUG_EN				BIT(3)
+#define  PLL_UNLOCK_EN				BIT(5)
+
 #define SN_AUX_CMD_STATUS_REG			0xF4
 #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
 #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
 #define  AUX_IRQ_STATUS_NAT_I2C_FAIL		BIT(6)
+#define SN_IRQ_STATUS_REG			0xF5
+#define  HPD_REMOVAL_STATUS			BIT(2)
+#define  HPD_INSERTION_STATUS			BIT(1)
 
 #define MIN_DSI_CLK_FREQ_MHZ	40
 
@@ -221,6 +235,19 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = {
 	.max_register = 0xFF,
 };
 
+static int ti_sn65dsi86_read(struct ti_sn65dsi86 *pdata, unsigned int reg,
+			     unsigned int *val)
+{
+	int ret;
+
+	ret = regmap_read(pdata->regmap, reg, val);
+	if (ret)
+		dev_err(pdata->dev, "fail to read raw reg %x: %d\n",
+			reg, ret);
+
+	return ret;
+}
+
 static int __maybe_unused ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
 						unsigned int reg, u16 *val)
 {
@@ -1219,12 +1246,28 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 	 */
 
 	pm_runtime_get_sync(pdata->dev);
+
+	/* Enable HPD and PLL events. */
+	regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
+		     PLL_UNLOCK_EN |
+		     HPD_REPLUG_EN |
+		     HPD_REMOVAL_EN |
+		     HPD_INSERTION_EN |
+		     IRQ_HPD_EN);
+
+	regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN,
+			   IRQ_EN);
 }
 
 static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 
+	/* Disable HPD and PLL events. */
+	regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG, 0);
+
+	regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN, 0);
+
 	pm_runtime_put_autosuspend(pdata->dev);
 }
 
@@ -1309,6 +1352,32 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
 	return 0;
 }
 
+static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
+{
+	struct ti_sn65dsi86 *pdata = private;
+	struct drm_device *dev = pdata->bridge.dev;
+	u32 status = 0;
+	bool hpd_event = false;
+	int ret = 0;
+
+	ret = ti_sn65dsi86_read(pdata, SN_IRQ_STATUS_REG, &status);
+	if (ret)
+		return ret;
+	hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
+	if (status) {
+		drm_dbg(dev, "(SN_IRQ_STATUS_REG = %#x)\n", status);
+		ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
+		if (ret)
+			return ret;
+	}
+
+	/* Only send the HPD event if we are bound with a device. */
+	if (dev && hpd_event)
+		drm_kms_helper_hotplug_event(dev);
+
+	return IRQ_HANDLED;
+}
+
 static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 			      const struct auxiliary_device_id *id)
 {
@@ -1971,6 +2040,30 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	if (strncmp(id_buf, "68ISD   ", ARRAY_SIZE(id_buf)))
 		return dev_err_probe(dev, -EOPNOTSUPP, "unsupported device id\n");
 
+	if (client->irq) {
+		enum drm_connector_status status;
+
+		ret = devm_request_threaded_irq(pdata->dev, client->irq, NULL,
+						ti_sn_bridge_interrupt,
+						IRQF_TRIGGER_RISING |
+						IRQF_TRIGGER_FALLING |
+						IRQF_ONESHOT,
+						"ti_sn65dsi86", pdata);
+		if (ret) {
+			return dev_err_probe(dev, ret,
+					     "failed to request interrupt\n");
+		}
+
+		/*
+		 * Cleaning status register at probe is needed because if the irq is
+		 * already high, the rising/falling condition will never occurs
+		 */
+		ret = ti_sn65dsi86_read(pdata, SN_IRQ_STATUS_REG, &status);
+		ret |= regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
+		if (ret)
+			pr_warn("Failed to clear IRQ initial state: %d\n", ret);
+	}
+
 	/*
 	 * Break ourselves up into a collection of aux devices. The only real
 	 * motiviation here is to solve the chicken-and-egg problem of probe

[PATCH V2] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[John Ripple]

Add support for DisplayPort to the bridge, which entails the following:
- Get and use an interrupt for HPD;
- Properly clear all status bits in the interrupt handler;

Signed-off-by: John Ripple <john.ripple@keysight.com>
---
V1 -> V2: Cleaned up coding style and addressed review comments

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 93 +++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index ae0d08e5e960..ad0393212279 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -106,10 +106,24 @@
 #define SN_PWM_EN_INV_REG			0xA5
 #define  SN_PWM_INV_MASK			BIT(0)
 #define  SN_PWM_EN_MASK				BIT(1)
+
+#define SN_IRQ_EN_REG				0xE0
+#define  IRQ_EN					BIT(0)
+
+#define SN_IRQ_EVENTS_EN_REG			0xE6
+#define  IRQ_HPD_EN				BIT(0)
+#define  HPD_INSERTION_EN			BIT(1)
+#define  HPD_REMOVAL_EN				BIT(2)
+#define  HPD_REPLUG_EN				BIT(3)
+#define  PLL_UNLOCK_EN				BIT(5)
+
 #define SN_AUX_CMD_STATUS_REG			0xF4
 #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
 #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
 #define  AUX_IRQ_STATUS_NAT_I2C_FAIL		BIT(6)
+#define SN_IRQ_STATUS_REG			0xF5
+#define  HPD_REMOVAL_STATUS			BIT(2)
+#define  HPD_INSERTION_STATUS			BIT(1)
 
 #define MIN_DSI_CLK_FREQ_MHZ	40
 
@@ -221,6 +235,19 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = {
 	.max_register = 0xFF,
 };
 
+static int ti_sn65dsi86_read(struct ti_sn65dsi86 *pdata, unsigned int reg,
+			     unsigned int *val)
+{
+	int ret;
+
+	ret = regmap_read(pdata->regmap, reg, val);
+	if (ret)
+		dev_err(pdata->dev, "fail to read raw reg %x: %d\n",
+			reg, ret);
+
+	return ret;
+}
+
 static int __maybe_unused ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
 						unsigned int reg, u16 *val)
 {
@@ -1219,12 +1246,28 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 	 */
 
 	pm_runtime_get_sync(pdata->dev);
+
+	/* Enable HPD and PLL events. */
+	regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
+		     PLL_UNLOCK_EN |
+		     HPD_REPLUG_EN |
+		     HPD_REMOVAL_EN |
+		     HPD_INSERTION_EN |
+		     IRQ_HPD_EN);
+
+	regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN,
+			   IRQ_EN);
 }
 
 static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 
+	/* Disable HPD and PLL events. */
+	regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG, 0);
+
+	regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN, 0);
+
 	pm_runtime_put_autosuspend(pdata->dev);
 }
 
@@ -1309,6 +1352,32 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
 	return 0;
 }
 
+static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
+{
+	struct ti_sn65dsi86 *pdata = private;
+	struct drm_device *dev = pdata->bridge.dev;
+	u32 status = 0;
+	bool hpd_event = false;
+	int ret = 0;
+
+	ret = ti_sn65dsi86_read(pdata, SN_IRQ_STATUS_REG, &status);
+	if (ret)
+		return ret;
+	hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
+	if (status) {
+		drm_dbg(dev, "(SN_IRQ_STATUS_REG = %#x)\n", status);
+		ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
+		if (ret)
+			return ret;
+	}
+
+	/* Only send the HPD event if we are bound with a device. */
+	if (dev && hpd_event)
+		drm_kms_helper_hotplug_event(dev);
+
+	return IRQ_HANDLED;
+}
+
 static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 			      const struct auxiliary_device_id *id)
 {
@@ -1971,6 +2040,30 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	if (strncmp(id_buf, "68ISD   ", ARRAY_SIZE(id_buf)))
 		return dev_err_probe(dev, -EOPNOTSUPP, "unsupported device id\n");
 
+	if (client->irq) {
+		enum drm_connector_status status;
+
+		ret = devm_request_threaded_irq(pdata->dev, client->irq, NULL,
+						ti_sn_bridge_interrupt,
+						IRQF_TRIGGER_RISING |
+						IRQF_TRIGGER_FALLING |
+						IRQF_ONESHOT,
+						"ti_sn65dsi86", pdata);
+		if (ret) {
+			return dev_err_probe(dev, ret,
+					     "failed to request interrupt\n");
+		}
+
+		/*
+		 * Cleaning status register at probe is needed because if the irq is
+		 * already high, the rising/falling condition will never occurs
+		 */
+		ret = ti_sn65dsi86_read(pdata, SN_IRQ_STATUS_REG, &status);
+		ret |= regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
+		if (ret)
+			pr_warn("Failed to clear IRQ initial state: %d\n", ret);
+	}
+
 	/*
 	 * Break ourselves up into a collection of aux devices. The only real
 	 * motiviation here is to solve the chicken-and-egg problem of probe

Re: [PATCH V2] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[Doug Anderson]

Hi,

On Mon, Sep 8, 2025 at 1:37 PM John Ripple <john.ripple@keysight.com> wrote:
>
> Add support for DisplayPort to the bridge, which entails the following:
> - Get and use an interrupt for HPD;
> - Properly clear all status bits in the interrupt handler;
>
> Signed-off-by: John Ripple <john.ripple@keysight.com>
> ---
> V1 -> V2: Cleaned up coding style and addressed review comments
>
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 93 +++++++++++++++++++++++++++
>  1 file changed, 93 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index ae0d08e5e960..ad0393212279 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -106,10 +106,24 @@
>  #define SN_PWM_EN_INV_REG                      0xA5
>  #define  SN_PWM_INV_MASK                       BIT(0)
>  #define  SN_PWM_EN_MASK                                BIT(1)
> +
> +#define SN_IRQ_EN_REG                          0xE0
> +#define  IRQ_EN                                        BIT(0)
> +
> +#define SN_IRQ_EVENTS_EN_REG                   0xE6
> +#define  IRQ_HPD_EN                            BIT(0)
> +#define  HPD_INSERTION_EN                      BIT(1)
> +#define  HPD_REMOVAL_EN                                BIT(2)
> +#define  HPD_REPLUG_EN                         BIT(3)
> +#define  PLL_UNLOCK_EN                         BIT(5)
> +
>  #define SN_AUX_CMD_STATUS_REG                  0xF4
>  #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT          BIT(3)
>  #define  AUX_IRQ_STATUS_AUX_SHORT              BIT(5)
>  #define  AUX_IRQ_STATUS_NAT_I2C_FAIL           BIT(6)
> +#define SN_IRQ_STATUS_REG                      0xF5
> +#define  HPD_REMOVAL_STATUS                    BIT(2)
> +#define  HPD_INSERTION_STATUS                  BIT(1)
>
>  #define MIN_DSI_CLK_FREQ_MHZ   40
>
> @@ -221,6 +235,19 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = {
>         .max_register = 0xFF,
>  };
>
> +static int ti_sn65dsi86_read(struct ti_sn65dsi86 *pdata, unsigned int reg,
> +                            unsigned int *val)

This is reading a byte, right? So "val" should be an "u8 *". Yeah,
that means you need a local variable to adjust for the generic regmap
call, but it makes a cleaner and more obvious API to the users in this
file.


> +{
> +       int ret;
> +
> +       ret = regmap_read(pdata->regmap, reg, val);
> +       if (ret)
> +               dev_err(pdata->dev, "fail to read raw reg %x: %d\n",
> +                       reg, ret);

nit: use %#x so that the 0x is included.


> @@ -1219,12 +1246,28 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
>          */
>
>         pm_runtime_get_sync(pdata->dev);
> +
> +       /* Enable HPD and PLL events. */
> +       regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
> +                    PLL_UNLOCK_EN |
> +                    HPD_REPLUG_EN |
> +                    HPD_REMOVAL_EN |
> +                    HPD_INSERTION_EN |
> +                    IRQ_HPD_EN);

* Shouldn't this be `regmap_update_bits()` to just update the bits
related to HPD?

* why enable "PLL_UNLOCK_EN" when you don't handle it?

* I also don't think your IRQ handler handles "replug" and "irq_hpd",
right? So you shouldn't enable those either?

Also: should the above only be if the IRQ is enabled? AKA obtain a
pointer to the i2c_client and check `client->irq`?


> +
> +       regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN,
> +                          IRQ_EN);

I guess this is OK to be here if you want, but I would maybe consider
putting it in `ti_sn65dsi86_resume()` if `client->irq` is set. Then if
we ever enable more interrupts it'll already be in the correct place.


> @@ -1309,6 +1352,32 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
>         return 0;
>  }
>
> +static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
> +{
> +       struct ti_sn65dsi86 *pdata = private;
> +       struct drm_device *dev = pdata->bridge.dev;

I'm unsure if accessing "dev" here without any sort of locking is
safe... It feels like, in theory, "detach" could be called and race
with the IRQ handler? Maybe you need a spinlock to be sure?


> +       u32 status = 0;
> +       bool hpd_event = false;
> +       int ret = 0;

Don't initialize variables unless they're needed. In this case it's
obvious that both `hpd_event` and `ret` don't need to be initialized.
Maybe you could argue that `status` should be initialized since it's
passed by reference, but even that's iffy...


> +
> +       ret = ti_sn65dsi86_read(pdata, SN_IRQ_STATUS_REG, &status);
> +       if (ret)
> +               return ret;

The return for this function is not an error code but an
`irqreturn_t`. You need to account for that.


> +       hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
> +       if (status) {
> +               drm_dbg(dev, "(SN_IRQ_STATUS_REG = %#x)\n", status);
> +               ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
> +               if (ret)
> +                       return ret;

Same--you can't just return an error code.


> +       }
> +
> +       /* Only send the HPD event if we are bound with a device. */
> +       if (dev && hpd_event)
> +               drm_kms_helper_hotplug_event(dev);
> +
> +       return IRQ_HANDLED;

If "status" gives back 0 (which would be weird), you probably want to
return IRQ_NONE, right?


> +}
> +
>  static int ti_sn_bridge_probe(struct auxiliary_device *adev,
>                               const struct auxiliary_device_id *id)
>  {
> @@ -1971,6 +2040,30 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
>         if (strncmp(id_buf, "68ISD   ", ARRAY_SIZE(id_buf)))
>                 return dev_err_probe(dev, -EOPNOTSUPP, "unsupported device id\n");
>
> +       if (client->irq) {
> +               enum drm_connector_status status;
> +
> +               ret = devm_request_threaded_irq(pdata->dev, client->irq, NULL,
> +                                               ti_sn_bridge_interrupt,
> +                                               IRQF_TRIGGER_RISING |
> +                                               IRQF_TRIGGER_FALLING |
> +                                               IRQF_ONESHOT,
> +                                               "ti_sn65dsi86", pdata);
> +               if (ret) {
> +                       return dev_err_probe(dev, ret,
> +                                            "failed to request interrupt\n");
> +               }
> +
> +               /*
> +                * Cleaning status register at probe is needed because if the irq is
> +                * already high, the rising/falling condition will never occurs

nit: s/occurs/occur


> +                */
> +               ret = ti_sn65dsi86_read(pdata, SN_IRQ_STATUS_REG, &status);
> +               ret |= regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
> +               if (ret)
> +                       pr_warn("Failed to clear IRQ initial state: %d\n", ret);

Do you even need to read? Can't you just write all possible bits and
that should be safe?

Re: [PATCH V2] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[John Ripple]

Hi,

>> +static int ti_sn65dsi86_read(struct ti_sn65dsi86 *pdata, unsigned int reg,
>> +                            unsigned int *val)
>
>This is reading a byte, right? So "val" should be an "u8 *". Yeah,
>that means you need a local variable to adjust for the generic regmap
>call, but it makes a cleaner and more obvious API to the users in this
>file.

The regmap_read function takes in an "unsigned int *" as the "val"
parameter and I'm using it to return u32 values (which could probably
be u8 instead). Would it be better to leave this as the more generic 
int type or change it to u8 so its more specific to this driver?
If this function gets used elsewhere in this file at some point, I'm 
not sure everything that could be read would be single bytes.

>> @@ -1219,12 +1246,28 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
>>          */
>>
>>         pm_runtime_get_sync(pdata->dev);
>> +
>> +       /* Enable HPD and PLL events. */
>> +       regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
>> +                    PLL_UNLOCK_EN |
>> +                    HPD_REPLUG_EN |
>> +                    HPD_REMOVAL_EN |
>> +                    HPD_INSERTION_EN |
>> +                    IRQ_HPD_EN);
>
>* Shouldn't this be `regmap_update_bits()` to just update the bits
>related to HPD?
>
>* why enable "PLL_UNLOCK_EN" when you don't handle it?
>
>* I also don't think your IRQ handler handles "replug" and "irq_hpd",
>right? So you shouldn't enable those either?

The IRQ_HPD_EN documentation said:
"When IRQ_EN and IRQ_HPD_EN is enabled, the DSIx6 will assert the 
IRQ whenever the eDP generates a IRQ_HPD event. An IRQ_HPD event 
is defined as a change from INSERTION state to the IRQ_HPD state."

I thought that meant the IRQ_HPD_EN needed to be enabled to get any irqs,
but when I tried removing the IRQ_HPD_EN and it doesn't seem to change
anything, so I'm not sure what the documentation is trying to say.

>> @@ -1309,6 +1352,32 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
>>         return 0;
>>  }
>>
>> +static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
>> +{
>> +       struct ti_sn65dsi86 *pdata = private;
>> +       struct drm_device *dev = pdata->bridge.dev;
>
>I'm unsure if accessing "dev" here without any sort of locking is
>safe... It feels like, in theory, "detach" could be called and race
>with the IRQ handler? Maybe you need a spinlock to be sure?

I tested a spinlock added to the ti-sn65dsi86 structure that gets used
in the ti_sn_bridge_detach and ti_sn_bridge_interrupt functions and it
seems to work. Is there another spinlock created somewhere that I could
use instead? Is using the spin lock in the interrupt and detach functions
the correct way to do it?

Re: [PATCH V2] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[Doug Anderson]

Hi,

On Tue, Sep 9, 2025 at 12:36 PM John Ripple <john.ripple@keysight.com> wrote:
>
> Hi,
>
> >> +static int ti_sn65dsi86_read(struct ti_sn65dsi86 *pdata, unsigned int reg,
> >> +                            unsigned int *val)
> >
> >This is reading a byte, right? So "val" should be an "u8 *". Yeah,
> >that means you need a local variable to adjust for the generic regmap
> >call, but it makes a cleaner and more obvious API to the users in this
> >file.
>
> The regmap_read function takes in an "unsigned int *" as the "val"
> parameter and I'm using it to return u32 values (which could probably
> be u8 instead). Would it be better to leave this as the more generic
> int type or change it to u8 so its more specific to this driver?
> If this function gets used elsewhere in this file at some point, I'm
> not sure everything that could be read would be single bytes.

Sure, the "regmap_read" takes "unsigned int *" because it's a generic
API. ...but we initialize the regmap API with:

  .reg_bits = 8,
  .val_bits = 8,

In other words, each read/write is 8-byte AKA 1 byte. So you're not
returning 32-bit values, but 8-bit values.

There's already a 16-bit version of this function:
ti_sn65dsi86_read_u16(). Reading that function and yours next to each
other makes it seem (at first glance) like yours is returning 32-bits.
It's not. It would be much more documenting showing that it returns
8-bits. If we need a 32-bit version for some reason we'll have to
actually write that up.


> >> @@ -1219,12 +1246,28 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
> >>          */
> >>
> >>         pm_runtime_get_sync(pdata->dev);
> >> +
> >> +       /* Enable HPD and PLL events. */
> >> +       regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
> >> +                    PLL_UNLOCK_EN |
> >> +                    HPD_REPLUG_EN |
> >> +                    HPD_REMOVAL_EN |
> >> +                    HPD_INSERTION_EN |
> >> +                    IRQ_HPD_EN);
> >
> >* Shouldn't this be `regmap_update_bits()` to just update the bits
> >related to HPD?
> >
> >* why enable "PLL_UNLOCK_EN" when you don't handle it?
> >
> >* I also don't think your IRQ handler handles "replug" and "irq_hpd",
> >right? So you shouldn't enable those either?
>
> The IRQ_HPD_EN documentation said:
> "When IRQ_EN and IRQ_HPD_EN is enabled, the DSIx6 will assert the
> IRQ whenever the eDP generates a IRQ_HPD event. An IRQ_HPD event
> is defined as a change from INSERTION state to the IRQ_HPD state."
>
> I thought that meant the IRQ_HPD_EN needed to be enabled to get any irqs,
> but when I tried removing the IRQ_HPD_EN and it doesn't seem to change
> anything, so I'm not sure what the documentation is trying to say.

IRQ_HPD is defined in the spec. It's basically an "attention"
interrupt from the panel to ti-sn65dsi86. It (and replug) are a
temporary deassertion of HPD while a display is connected.

See "Figure 17. HPD State Diagram" for a description of all these
things. Note that the min/max values there are (I think) because
sn65dsi86's HPD timings are implemented by a very inaccurate ring
oscillator.

If you see that "replug" or "irq_hpd" are needed then your interrupt
handler should do something with them.


> >> @@ -1309,6 +1352,32 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
> >>         return 0;
> >>  }
> >>
> >> +static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
> >> +{
> >> +       struct ti_sn65dsi86 *pdata = private;
> >> +       struct drm_device *dev = pdata->bridge.dev;
> >
> >I'm unsure if accessing "dev" here without any sort of locking is
> >safe... It feels like, in theory, "detach" could be called and race
> >with the IRQ handler? Maybe you need a spinlock to be sure?
>
> I tested a spinlock added to the ti-sn65dsi86 structure that gets used
> in the ti_sn_bridge_detach and ti_sn_bridge_interrupt functions and it
> seems to work. Is there another spinlock created somewhere that I could
> use instead? Is using the spin lock in the interrupt and detach functions
> the correct way to do it?

In this case you could probably use a mutex since you're running a
threaded IRQ handler and sleeping is allowed. You could probably
create a new mutex for this case.

I assume you'd need some sort of boolean variable instead of just
checking if "bridge.dev" is non-NULL? "bridge.dev" is set by the DRM
core before your attach is called (and cleared after detach). Maybe
just have a boolean about whether HPD is enabled and only send the
event if HPD is enabled? Then use the mutex to protect access to that
boolean between the IRQ handler and the HPD enable/disable code?

-Doug

[PATCH V3] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[John Ripple]

Add support for DisplayPort to the bridge, which entails the following:
- Get and use an interrupt for HPD;
- Properly clear all status bits in the interrupt handler;

Signed-off-by: John Ripple <john.ripple@keysight.com>
---
V1 -> V2: Cleaned up coding style and addressed review comments
V2 -> V3:
- Removed unused HPD IRQs
- Added mutex around HPD enable/disable and IRQ handler.
- Cleaned up error handling and variable declarations
- Only enable IRQs if the i2c client has an IRQ
- Moved IRQ_EN to ti_sn65dsi86_resume()
- Created ti_sn65dsi86_read_u8() helper function
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 126 ++++++++++++++++++++++++++
 1 file changed, 126 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index ae0d08e5e960..da1508c14145 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -106,10 +106,21 @@
 #define SN_PWM_EN_INV_REG			0xA5
 #define  SN_PWM_INV_MASK			BIT(0)
 #define  SN_PWM_EN_MASK				BIT(1)
+
+#define SN_IRQ_EN_REG				0xE0
+#define  IRQ_EN					BIT(0)
+
+#define SN_IRQ_EVENTS_EN_REG			0xE6
+#define  HPD_INSERTION_EN			BIT(1)
+#define  HPD_REMOVAL_EN				BIT(2)
+
 #define SN_AUX_CMD_STATUS_REG			0xF4
 #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
 #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
 #define  AUX_IRQ_STATUS_NAT_I2C_FAIL		BIT(6)
+#define SN_IRQ_STATUS_REG			0xF5
+#define  HPD_REMOVAL_STATUS			BIT(2)
+#define  HPD_INSERTION_STATUS			BIT(1)
 
 #define MIN_DSI_CLK_FREQ_MHZ	40
 
@@ -153,6 +164,8 @@
  * @ln_polrs:     Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG.
  * @comms_enabled: If true then communication over the aux channel is enabled.
  * @comms_mutex:   Protects modification of comms_enabled.
+ * @hpd_enabled:   If true then HPD events are enabled.
+ * @hpd_mutex:     Protects modification of hpd_enabled.
  *
  * @gchip:        If we expose our GPIOs, this is used.
  * @gchip_output: A cache of whether we've set GPIOs to output.  This
@@ -190,7 +203,9 @@ struct ti_sn65dsi86 {
 	u8				ln_assign;
 	u8				ln_polrs;
 	bool				comms_enabled;
+	bool				hpd_enabled;
 	struct mutex			comms_mutex;
+	struct mutex			hpd_mutex;
 
 #if defined(CONFIG_OF_GPIO)
 	struct gpio_chip		gchip;
@@ -221,6 +236,23 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = {
 	.max_register = 0xFF,
 };
 
+static int ti_sn65dsi86_read_u8(struct ti_sn65dsi86 *pdata, unsigned int reg,
+			     u8 *val)
+{
+	int ret;
+	unsigned int reg_val;
+
+	ret = regmap_read(pdata->regmap, reg, &reg_val);
+	if (ret) {
+		dev_err(pdata->dev, "fail to read raw reg %#x: %d\n",
+			reg, ret);
+		return ret;
+	}
+	*val = (u8)reg_val;
+
+	return 0;
+}
+
 static int __maybe_unused ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
 						unsigned int reg, u16 *val)
 {
@@ -379,6 +411,7 @@ static void ti_sn65dsi86_disable_comms(struct ti_sn65dsi86 *pdata)
 static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 {
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
 	int ret;
 
 	ret = regulator_bulk_enable(SN_REGULATOR_SUPPLY_NUM, pdata->supplies);
@@ -413,6 +446,13 @@ static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 	if (pdata->refclk)
 		ti_sn65dsi86_enable_comms(pdata, NULL);
 
+	if (client->irq) {
+		ret = regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN,
+			IRQ_EN);
+		if (ret)
+			pr_err("Failed to enable IRQ events: %d\n", ret);
+	}
+
 	return ret;
 }
 
@@ -1211,6 +1251,9 @@ static void ti_sn65dsi86_debugfs_init(struct drm_bridge *bridge, struct dentry *
 static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
+	int ret;
+	unsigned int val;
 
 	/*
 	 * Device needs to be powered on before reading the HPD state
@@ -1219,11 +1262,32 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 	 */
 
 	pm_runtime_get_sync(pdata->dev);
+
+	mutex_lock(&pdata->hpd_mutex);
+	if (client->irq) {
+		/* Enable HPD events. */
+		val = HPD_REMOVAL_EN | HPD_INSERTION_EN;
+		ret = regmap_update_bits(pdata->regmap, SN_IRQ_EVENTS_EN_REG, val, val);
+		if (ret)
+			pr_err("Failed to enable HPD events: %d\n", ret);
+	}
+	pdata->hpd_enabled = true;
+	mutex_unlock(&pdata->hpd_mutex);
 }
 
 static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
+
+	mutex_lock(&pdata->hpd_mutex);
+	pdata->hpd_enabled = false;
+	if (client->irq) {
+		/* Disable HPD events. */
+		regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG, 0);
+		regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN, 0);
+	}
+	mutex_unlock(&pdata->hpd_mutex);
 
 	pm_runtime_put_autosuspend(pdata->dev);
 }
@@ -1309,6 +1373,44 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
 	return 0;
 }
 
+static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
+{
+	struct ti_sn65dsi86 *pdata = private;
+	struct drm_device *dev = pdata->bridge.dev;
+	u8 status;
+	int ret;
+	bool hpd_event = false;
+
+	mutex_lock(&pdata->hpd_mutex);
+	if (!pdata->hpd_enabled) {
+		mutex_unlock(&pdata->hpd_mutex);
+		return IRQ_HANDLED;
+	}
+
+	ret = ti_sn65dsi86_read_u8(pdata, SN_IRQ_STATUS_REG, &status);
+	if (ret)
+		pr_err("Failed to read IRQ status: %d\n", ret);
+	else
+		hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
+
+	if (status) {
+		drm_dbg(dev, "(SN_IRQ_STATUS_REG = %#x)\n", status);
+		ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
+		if (ret)
+			pr_err("Failed to clear IRQ status: %d\n", ret);
+	} else {
+		mutex_unlock(&pdata->hpd_mutex);
+		return IRQ_NONE;
+	}
+
+	/* Only send the HPD event if we are bound with a device. */
+	if (dev && hpd_event)
+		drm_kms_helper_hotplug_event(dev);
+	mutex_unlock(&pdata->hpd_mutex);
+
+	return IRQ_HANDLED;
+}
+
 static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 			      const struct auxiliary_device_id *id)
 {
@@ -1931,6 +2033,8 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	dev_set_drvdata(dev, pdata);
 	pdata->dev = dev;
 
+	mutex_init(&pdata->hpd_mutex);
+
 	mutex_init(&pdata->comms_mutex);
 
 	pdata->regmap = devm_regmap_init_i2c(client,
@@ -1971,6 +2075,28 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	if (strncmp(id_buf, "68ISD   ", ARRAY_SIZE(id_buf)))
 		return dev_err_probe(dev, -EOPNOTSUPP, "unsupported device id\n");
 
+	if (client->irq) {
+		ret = devm_request_threaded_irq(pdata->dev, client->irq, NULL,
+						ti_sn_bridge_interrupt,
+						IRQF_TRIGGER_RISING |
+						IRQF_TRIGGER_FALLING |
+						IRQF_ONESHOT,
+						"ti_sn65dsi86", pdata);
+
+		if (ret) {
+			return dev_err_probe(dev, ret,
+					     "failed to request interrupt\n");
+		}
+
+		/*
+		 * Cleaning status register at probe is needed because if the irq is
+		 * already high, the rising/falling condition will never occur
+		 */
+		ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, 0xFF);
+		if (ret)
+			pr_warn("Failed to clear IRQ initial state: %d\n", ret);
+	}
+
 	/*
 	 * Break ourselves up into a collection of aux devices. The only real
 	 * motiviation here is to solve the chicken-and-egg problem of probe

[PATCH V3] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[John Ripple]

Add support for DisplayPort to the bridge, which entails the following:
- Get and use an interrupt for HPD;
- Properly clear all status bits in the interrupt handler;

Signed-off-by: John Ripple <john.ripple@keysight.com>
---
V1 -> V2: Cleaned up coding style and addressed review comments
V2 -> V3:
- Removed unused HPD IRQs
- Added mutex around HPD enable/disable and IRQ handler.
- Cleaned up error handling and variable declarations
- Only enable IRQs if the i2c client has an IRQ
- Moved IRQ_EN to ti_sn65dsi86_resume()
- Created ti_sn65dsi86_read_u8() helper function
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 126 ++++++++++++++++++++++++++
 1 file changed, 126 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index ae0d08e5e960..da1508c14145 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -106,10 +106,21 @@
 #define SN_PWM_EN_INV_REG			0xA5
 #define  SN_PWM_INV_MASK			BIT(0)
 #define  SN_PWM_EN_MASK				BIT(1)
+
+#define SN_IRQ_EN_REG				0xE0
+#define  IRQ_EN					BIT(0)
+
+#define SN_IRQ_EVENTS_EN_REG			0xE6
+#define  HPD_INSERTION_EN			BIT(1)
+#define  HPD_REMOVAL_EN				BIT(2)
+
 #define SN_AUX_CMD_STATUS_REG			0xF4
 #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
 #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
 #define  AUX_IRQ_STATUS_NAT_I2C_FAIL		BIT(6)
+#define SN_IRQ_STATUS_REG			0xF5
+#define  HPD_REMOVAL_STATUS			BIT(2)
+#define  HPD_INSERTION_STATUS			BIT(1)
 
 #define MIN_DSI_CLK_FREQ_MHZ	40
 
@@ -153,6 +164,8 @@
  * @ln_polrs:     Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG.
  * @comms_enabled: If true then communication over the aux channel is enabled.
  * @comms_mutex:   Protects modification of comms_enabled.
+ * @hpd_enabled:   If true then HPD events are enabled.
+ * @hpd_mutex:     Protects modification of hpd_enabled.
  *
  * @gchip:        If we expose our GPIOs, this is used.
  * @gchip_output: A cache of whether we've set GPIOs to output.  This
@@ -190,7 +203,9 @@ struct ti_sn65dsi86 {
 	u8				ln_assign;
 	u8				ln_polrs;
 	bool				comms_enabled;
+	bool				hpd_enabled;
 	struct mutex			comms_mutex;
+	struct mutex			hpd_mutex;
 
 #if defined(CONFIG_OF_GPIO)
 	struct gpio_chip		gchip;
@@ -221,6 +236,23 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = {
 	.max_register = 0xFF,
 };
 
+static int ti_sn65dsi86_read_u8(struct ti_sn65dsi86 *pdata, unsigned int reg,
+			     u8 *val)
+{
+	int ret;
+	unsigned int reg_val;
+
+	ret = regmap_read(pdata->regmap, reg, &reg_val);
+	if (ret) {
+		dev_err(pdata->dev, "fail to read raw reg %#x: %d\n",
+			reg, ret);
+		return ret;
+	}
+	*val = (u8)reg_val;
+
+	return 0;
+}
+
 static int __maybe_unused ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
 						unsigned int reg, u16 *val)
 {
@@ -379,6 +411,7 @@ static void ti_sn65dsi86_disable_comms(struct ti_sn65dsi86 *pdata)
 static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 {
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
 	int ret;
 
 	ret = regulator_bulk_enable(SN_REGULATOR_SUPPLY_NUM, pdata->supplies);
@@ -413,6 +446,13 @@ static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 	if (pdata->refclk)
 		ti_sn65dsi86_enable_comms(pdata, NULL);
 
+	if (client->irq) {
+		ret = regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN,
+			IRQ_EN);
+		if (ret)
+			pr_err("Failed to enable IRQ events: %d\n", ret);
+	}
+
 	return ret;
 }
 
@@ -1211,6 +1251,9 @@ static void ti_sn65dsi86_debugfs_init(struct drm_bridge *bridge, struct dentry *
 static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
+	int ret;
+	unsigned int val;
 
 	/*
 	 * Device needs to be powered on before reading the HPD state
@@ -1219,11 +1262,32 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 	 */
 
 	pm_runtime_get_sync(pdata->dev);
+
+	mutex_lock(&pdata->hpd_mutex);
+	if (client->irq) {
+		/* Enable HPD events. */
+		val = HPD_REMOVAL_EN | HPD_INSERTION_EN;
+		ret = regmap_update_bits(pdata->regmap, SN_IRQ_EVENTS_EN_REG, val, val);
+		if (ret)
+			pr_err("Failed to enable HPD events: %d\n", ret);
+	}
+	pdata->hpd_enabled = true;
+	mutex_unlock(&pdata->hpd_mutex);
 }
 
 static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
+
+	mutex_lock(&pdata->hpd_mutex);
+	pdata->hpd_enabled = false;
+	if (client->irq) {
+		/* Disable HPD events. */
+		regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG, 0);
+		regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN, 0);
+	}
+	mutex_unlock(&pdata->hpd_mutex);
 
 	pm_runtime_put_autosuspend(pdata->dev);
 }
@@ -1309,6 +1373,44 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
 	return 0;
 }
 
+static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
+{
+	struct ti_sn65dsi86 *pdata = private;
+	struct drm_device *dev = pdata->bridge.dev;
+	u8 status;
+	int ret;
+	bool hpd_event = false;
+
+	mutex_lock(&pdata->hpd_mutex);
+	if (!pdata->hpd_enabled) {
+		mutex_unlock(&pdata->hpd_mutex);
+		return IRQ_HANDLED;
+	}
+
+	ret = ti_sn65dsi86_read_u8(pdata, SN_IRQ_STATUS_REG, &status);
+	if (ret)
+		pr_err("Failed to read IRQ status: %d\n", ret);
+	else
+		hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
+
+	if (status) {
+		drm_dbg(dev, "(SN_IRQ_STATUS_REG = %#x)\n", status);
+		ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
+		if (ret)
+			pr_err("Failed to clear IRQ status: %d\n", ret);
+	} else {
+		mutex_unlock(&pdata->hpd_mutex);
+		return IRQ_NONE;
+	}
+
+	/* Only send the HPD event if we are bound with a device. */
+	if (dev && hpd_event)
+		drm_kms_helper_hotplug_event(dev);
+	mutex_unlock(&pdata->hpd_mutex);
+
+	return IRQ_HANDLED;
+}
+
 static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 			      const struct auxiliary_device_id *id)
 {
@@ -1931,6 +2033,8 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	dev_set_drvdata(dev, pdata);
 	pdata->dev = dev;
 
+	mutex_init(&pdata->hpd_mutex);
+
 	mutex_init(&pdata->comms_mutex);
 
 	pdata->regmap = devm_regmap_init_i2c(client,
@@ -1971,6 +2075,28 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	if (strncmp(id_buf, "68ISD   ", ARRAY_SIZE(id_buf)))
 		return dev_err_probe(dev, -EOPNOTSUPP, "unsupported device id\n");
 
+	if (client->irq) {
+		ret = devm_request_threaded_irq(pdata->dev, client->irq, NULL,
+						ti_sn_bridge_interrupt,
+						IRQF_TRIGGER_RISING |
+						IRQF_TRIGGER_FALLING |
+						IRQF_ONESHOT,
+						"ti_sn65dsi86", pdata);
+
+		if (ret) {
+			return dev_err_probe(dev, ret,
+					     "failed to request interrupt\n");
+		}
+
+		/*
+		 * Cleaning status register at probe is needed because if the irq is
+		 * already high, the rising/falling condition will never occur
+		 */
+		ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, 0xFF);
+		if (ret)
+			pr_warn("Failed to clear IRQ initial state: %d\n", ret);
+	}
+
 	/*
 	 * Break ourselves up into a collection of aux devices. The only real
 	 * motiviation here is to solve the chicken-and-egg problem of probe

Re: [PATCH V3] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[Doug Anderson]

Hi,

On Wed, Sep 10, 2025 at 11:34 AM John Ripple <john.ripple@keysight.com> wrote:
>
> @@ -221,6 +236,23 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = {
>         .max_register = 0xFF,
>  };
>
> +static int ti_sn65dsi86_read_u8(struct ti_sn65dsi86 *pdata, unsigned int reg,
> +                            u8 *val)

nit: indentation is slightly off. checkpatch --strict yells:

CHECK: Alignment should match open parenthesis
#79: FILE: drivers/gpu/drm/bridge/ti-sn65dsi86.c:240:
+static int ti_sn65dsi86_read_u8(struct ti_sn65dsi86 *pdata, unsigned int reg,
+                            u8 *val)


> @@ -413,6 +446,13 @@ static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
>         if (pdata->refclk)
>                 ti_sn65dsi86_enable_comms(pdata, NULL);
>
> +       if (client->irq) {
> +               ret = regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN,
> +                       IRQ_EN);

nit: checkpatch --strict yells:

CHECK: Alignment should match open parenthesis
#112: FILE: drivers/gpu/drm/bridge/ti-sn65dsi86.c:451:
+               ret = regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN,
+                       IRQ_EN);


> @@ -1219,11 +1262,32 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
>          */
>
>         pm_runtime_get_sync(pdata->dev);
> +
> +       mutex_lock(&pdata->hpd_mutex);
> +       if (client->irq) {
> +               /* Enable HPD events. */
> +               val = HPD_REMOVAL_EN | HPD_INSERTION_EN;
> +               ret = regmap_update_bits(pdata->regmap, SN_IRQ_EVENTS_EN_REG, val, val);

nit: regmap_set_bits() ?

...and then you don't need the "val" temporary variable.


> +               if (ret)
> +                       pr_err("Failed to enable HPD events: %d\n", ret);
> +       }
> +       pdata->hpd_enabled = true;
> +       mutex_unlock(&pdata->hpd_mutex);

So I _think_ you only need the mutex around the set of "hpd_enabled".
Really the only things you're trying to do are:

* Make sure that by the time ti_sn_bridge_hpd_disable() returns that
no more HPD callback will be made

* Make sure that after ti_sn_bridge_hpd_enable() is called that the
next interrupt will notice the update.

So I'd make the enable case look something like this:

  mutex_lock(&pdata->hpd_mutex);
  pdata->hpd_enabled = true;
  mutex_unlock(&pdata->hpd_mutex);

  if (client->irq) {
    /* Enable HPD events. */
    val = HPD_REMOVAL_EN | HPD_INSERTION_EN;
    ret = regmap_update_bits(pdata->regmap, SN_IRQ_EVENTS_EN_REG, val, val);
    if (ret)
      pr_err("Failed to enable HPD events: %d\n", ret);
  }

...and the disable case:

  if (client->irq) {
    /* Disable HPD events. */
    regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG, 0);
    regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN, 0);
  }

  mutex_lock(&pdata->hpd_mutex);
  pdata->hpd_enabled = false;
  mutex_unlock(&pdata->hpd_mutex);


Does that seem reasonable?


> @@ -1309,6 +1373,44 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
>         return 0;
>  }
>
> +static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
> +{
> +       struct ti_sn65dsi86 *pdata = private;
> +       struct drm_device *dev = pdata->bridge.dev;
> +       u8 status;
> +       int ret;
> +       bool hpd_event = false;
> +
> +       mutex_lock(&pdata->hpd_mutex);
> +       if (!pdata->hpd_enabled) {
> +               mutex_unlock(&pdata->hpd_mutex);
> +               return IRQ_HANDLED;
> +       }

I also think you _always_ want to Ack all status interrupts so there's
no way you could end up with an interrupt storm, so you shouldn't do
this early return.


> +       ret = ti_sn65dsi86_read_u8(pdata, SN_IRQ_STATUS_REG, &status);
> +       if (ret)
> +               pr_err("Failed to read IRQ status: %d\n", ret);
> +       else
> +               hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
> +
> +       if (status) {
> +               drm_dbg(dev, "(SN_IRQ_STATUS_REG = %#x)\n", status);
> +               ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
> +               if (ret)
> +                       pr_err("Failed to clear IRQ status: %d\n", ret);
> +       } else {
> +               mutex_unlock(&pdata->hpd_mutex);
> +               return IRQ_NONE;
> +       }
> +
> +       /* Only send the HPD event if we are bound with a device. */
> +       if (dev && hpd_event)
> +               drm_kms_helper_hotplug_event(dev);
> +       mutex_unlock(&pdata->hpd_mutex);

I think you only want the mutex to protect your checking of hpd_mutex
and sending the "drm_kms_helper_hotplug_event()". I don't think you
need it for the whole IRQ routine. AKA:

mutex_lock(&pdata->hpd_mutex);
if (hpd_event && pdata->hpd_enabled)
  drm_kms_helper_hotplug_event(dev);
mutex_unlock(&pdata->hpd_mutex);

...and you don't need to check for "dev" being NULL because there's no
way "hpd_enabled" could be true with "dev" being NULL. At least this
is my assumption that the core DRM framework won't detach a bridge
while HPD is enabled. If nothing else, I guess you could call
ti_sn_bridge_hpd_disable() from ti_sn_bridge_detach()


> @@ -1971,6 +2075,28 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
>         if (strncmp(id_buf, "68ISD   ", ARRAY_SIZE(id_buf)))
>                 return dev_err_probe(dev, -EOPNOTSUPP, "unsupported device id\n");
>
> +       if (client->irq) {
> +               ret = devm_request_threaded_irq(pdata->dev, client->irq, NULL,
> +                                               ti_sn_bridge_interrupt,
> +                                               IRQF_TRIGGER_RISING |
> +                                               IRQF_TRIGGER_FALLING |
> +                                               IRQF_ONESHOT,
> +                                               "ti_sn65dsi86", pdata);
> +
> +               if (ret) {
> +                       return dev_err_probe(dev, ret,
> +                                            "failed to request interrupt\n");
> +               }
> +
> +               /*
> +                * Cleaning status register at probe is needed because if the irq is
> +                * already high, the rising/falling condition will never occur
> +                */
> +               ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, 0xFF);
> +               if (ret)
> +                       pr_warn("Failed to clear IRQ initial state: %d\n", ret);

Actually, wait. Why do you want "rising" and "falling". Isn't this a
level-triggered interrupt? Then you also don't need this bogus clear
of interrupts here...

...and also, I seem to recall it's usually better to not specify a
type here and rely on the type in the device tree. I seem to remember
there being some weird corner cases (maybe around remove / reprobe or
maybe about deferred probes?) if an interrupt type is specified in
both code and device tree and those types don't match...

-Doug

Re: [PATCH V3] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[John Ripple]

Hi, 

>...and you don't need to check for "dev" being NULL because there's no
>way "hpd_enabled" could be true with "dev" being NULL. At least this
>is my assumption that the core DRM framework won't detach a bridge
>while HPD is enabled. If nothing else, I guess you could call
>ti_sn_bridge_hpd_disable() from ti_sn_bridge_detach()

I don't think ti_sn_bridge_hpd_disable() needs to be in 
ti_sn_brdige_detach(). The DRM framework should run the disable for hpd
before detaching the device. I haven't seen any issues with it so far.

>> @@ -1971,6 +2075,28 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
>>         if (strncmp(id_buf, "68ISD   ", ARRAY_SIZE(id_buf)))
>>                 return dev_err_probe(dev, -EOPNOTSUPP, "unsupported device >id\n");
>>
>> +       if (client->irq) {
>> +               ret = devm_request_threaded_irq(pdata->dev, client->irq, NULL,
>> +                                               ti_sn_bridge_interrupt,
>> +                                               IRQF_TRIGGER_RISING |
>> +                                               IRQF_TRIGGER_FALLING |
>> +                                               IRQF_ONESHOT,
>> +                                               "ti_sn65dsi86", pdata);
>> +
>> +               if (ret) {
>> +                       return dev_err_probe(dev, ret,
>> +                                            "failed to request interrupt\n");
>> +               }
>> +
>> +               /*
>> +                * Cleaning status register at probe is needed because if the >irq is
>> +                * already high, the rising/falling condition will never occur
>> +                */
>> +               ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, 0xFF);
>> +               if (ret)
>> +                       pr_warn("Failed to clear IRQ initial state: %d\n", >ret);
>
>Actually, wait. Why do you want "rising" and "falling". Isn't this a
>level-triggered interrupt? Then you also don't need this bogus clear
>of interrupts here...

I changed it out for a high level interrupt and it looks fine. The IRQ
line off the check also seems to only send one pulse for about 1.09 ms
when the IRQ is toggled, so I think its doing a level interrupt since 
1 KHz is way slower than the refclk. For 0xE0 the documentation also 
says "the IRQ output is driven high to communicate IRQ events" so I
think you're correct.

>...and also, I seem to recall it's usually better to not specify a
>type here and rely on the type in the device tree. I seem to remember
>there being some weird corner cases (maybe around remove / reprobe or
>maybe about deferred probes?) if an interrupt type is specified in
>both code and device tree and those types don't match...

I couldn't find anything about this and all the other drivers in 
drivers/gpu-drm/bridge that use the devm_request_threaded_irq just 
directly set the irq type. I couldn't find any that read in the device 
tree for it. The display-connector.c general driver also seems to just 
set the type directly. Do you have an example where this is used?

The tisn65dsi86 chip also shouldn't be changing how it does its 
interrupts, so having the hardcoded high interrupt in the driver seems
like it would be fine.

Re: [PATCH V3] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[Doug Anderson]

Hi,

On Thu, Sep 11, 2025 at 11:39 AM John Ripple <john.ripple@keysight.com> wrote:
>
> Hi,
>
> >...and you don't need to check for "dev" being NULL because there's no
> >way "hpd_enabled" could be true with "dev" being NULL. At least this
> >is my assumption that the core DRM framework won't detach a bridge
> >while HPD is enabled. If nothing else, I guess you could call
> >ti_sn_bridge_hpd_disable() from ti_sn_bridge_detach()
>
> I don't think ti_sn_bridge_hpd_disable() needs to be in
> ti_sn_brdige_detach(). The DRM framework should run the disable for hpd
> before detaching the device. I haven't seen any issues with it so far.

Sure, that's fine with me. So I guess the result is to just remove the
check for the drm_dev being NULL.


> >> @@ -1971,6 +2075,28 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
> >>         if (strncmp(id_buf, "68ISD   ", ARRAY_SIZE(id_buf)))
> >>                 return dev_err_probe(dev, -EOPNOTSUPP, "unsupported device >id\n");
> >>
> >> +       if (client->irq) {
> >> +               ret = devm_request_threaded_irq(pdata->dev, client->irq, NULL,
> >> +                                               ti_sn_bridge_interrupt,
> >> +                                               IRQF_TRIGGER_RISING |
> >> +                                               IRQF_TRIGGER_FALLING |
> >> +                                               IRQF_ONESHOT,
> >> +                                               "ti_sn65dsi86", pdata);
> >> +
> >> +               if (ret) {
> >> +                       return dev_err_probe(dev, ret,
> >> +                                            "failed to request interrupt\n");
> >> +               }
> >> +
> >> +               /*
> >> +                * Cleaning status register at probe is needed because if the >irq is
> >> +                * already high, the rising/falling condition will never occur
> >> +                */
> >> +               ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, 0xFF);
> >> +               if (ret)
> >> +                       pr_warn("Failed to clear IRQ initial state: %d\n", >ret);
> >
> >Actually, wait. Why do you want "rising" and "falling". Isn't this a
> >level-triggered interrupt? Then you also don't need this bogus clear
> >of interrupts here...
>
> I changed it out for a high level interrupt and it looks fine. The IRQ
> line off the check also seems to only send one pulse for about 1.09 ms
> when the IRQ is toggled, so I think its doing a level interrupt since
> 1 KHz is way slower than the refclk. For 0xE0 the documentation also
> says "the IRQ output is driven high to communicate IRQ events" so I
> think you're correct.
>
> >...and also, I seem to recall it's usually better to not specify a
> >type here and rely on the type in the device tree. I seem to remember
> >there being some weird corner cases (maybe around remove / reprobe or
> >maybe about deferred probes?) if an interrupt type is specified in
> >both code and device tree and those types don't match...
>
> I couldn't find anything about this and all the other drivers in
> drivers/gpu-drm/bridge that use the devm_request_threaded_irq just
> directly set the irq type. I couldn't find any that read in the device
> tree for it. The display-connector.c general driver also seems to just
> set the type directly. Do you have an example where this is used?
>
> The tisn65dsi86 chip also shouldn't be changing how it does its
> interrupts, so having the hardcoded high interrupt in the driver seems
> like it would be fine.

Yeah, it's probably fine and I won't yell too much if you want to just
set HIGH in the driver. I think the one argument for letting the
device tree specify this is that you could conceivably imagine a
hardware design where this signal needs to be inverted at the board
level. In that case the board could specify "level low". That's kind
of a stretch.

Let me see if I can find an example of the problems with specifying
the interrupt level, though. Hmmm, I found one old patch that removed
setting the type from source code here:

https://lore.kernel.org/all/20200310154358.39367-2-swboyd@chromium.org/

Interestingly enough that patch also suggested not hardcoding a name
and using dev_name(), which is also probably a reasonable thing to do.

...but that patch didn't actually talk about any problems being
solved. It's possible that whatever problems were there are no longer
there, but I at least found some stack overflow question that sounded
like the problems I remember when the code and DT mismatched [1],
where someone mentioned:

> do you know why if I use request_irq() or devm_request_irq() for an
> SPI device (for which spi_drv_probe does almost the same as
> i2c_device_probe does), then after unloading the module and of
> course doing free_irq or even explicitly calling devm_free_irq,
> and then trying to load the same module again, I get an
> "irq: type mismatch, failed to map hwirq..." message?

As I said, maybe the problem is fixed now, but at the same time there
is something nice about the interrupt type only being specified in one
place. ...and IIRC the device tree validator gets upset if you don't
specify the interrupt type there, so removing it from the source code
seems nice...

[1] https://stackoverflow.com/questions/40011799/mapping-device-tree-interrupt-flags-to-devm-request-irq


-Doug

Re: [PATCH V3] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[John Ripple]

Hi, 

>As I said, maybe the problem is fixed now, but at the same time there
>is something nice about the interrupt type only being specified in one
>place. ...and IIRC the device tree validator gets upset if you don't
>specify the interrupt type there, so removing it from the source code
>seems nice...

My device tree does have interrupts already specified for the node, 
so allowing the driver to use that does seem like a good idea.

> [1] https://stackoverflow.com/questions/40011799/mapping-device-tree-interrupt-flags-to-devm-request-irq

The link you sent suggests just using "0" for the flag value lets the 
device tree flags to be used. I couldn't do this because the 
IRQF_ONESHOT needed to be passed as a flag or I'd get stack traces
every time the irq was triggered. Several of the other drivers in 
drivers/gpu/drm/bridge only pass IRQ_ONESHOT so I think those might be
using the device tree flags with the oneshot flag. As far as I could
tell, only passing the oneshot flag and setting the interrupt level
high in the device tree works.

[PATCH V4] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[John Ripple]

Add support for DisplayPort to the bridge, which entails the following:
- Get and use an interrupt for HPD;
- Properly clear all status bits in the interrupt handler;

Signed-off-by: John Ripple <john.ripple@keysight.com>
---
V1 -> V2: Cleaned up coding style and addressed review comments
V2 -> V3:
- Removed unused HPD IRQs
- Added mutex around HPD enable/disable and IRQ handler.
- Cleaned up error handling and variable declarations
- Only enable IRQs if the i2c client has an IRQ
- Moved IRQ_EN to ti_sn65dsi86_resume()
- Created ti_sn65dsi86_read_u8() helper function
V3 -> V4:
- Formatting
- Allow device tree to set interrupt type.
- Use hpd_mutex over less code.
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 110 ++++++++++++++++++++++++++
 1 file changed, 110 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index ae0d08e5e960..166920b2fcc7 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -106,10 +106,21 @@
 #define SN_PWM_EN_INV_REG			0xA5
 #define  SN_PWM_INV_MASK			BIT(0)
 #define  SN_PWM_EN_MASK				BIT(1)
+
+#define SN_IRQ_EN_REG				0xE0
+#define  IRQ_EN					BIT(0)
+
+#define SN_IRQ_EVENTS_EN_REG			0xE6
+#define  HPD_INSERTION_EN			BIT(1)
+#define  HPD_REMOVAL_EN				BIT(2)
+
 #define SN_AUX_CMD_STATUS_REG			0xF4
 #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
 #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
 #define  AUX_IRQ_STATUS_NAT_I2C_FAIL		BIT(6)
+#define SN_IRQ_STATUS_REG			0xF5
+#define  HPD_REMOVAL_STATUS			BIT(2)
+#define  HPD_INSERTION_STATUS			BIT(1)
 
 #define MIN_DSI_CLK_FREQ_MHZ	40
 
@@ -153,6 +164,8 @@
  * @ln_polrs:     Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG.
  * @comms_enabled: If true then communication over the aux channel is enabled.
  * @comms_mutex:   Protects modification of comms_enabled.
+ * @hpd_enabled:   If true then HPD events are enabled.
+ * @hpd_mutex:     Protects modification of hpd_enabled.
  *
  * @gchip:        If we expose our GPIOs, this is used.
  * @gchip_output: A cache of whether we've set GPIOs to output.  This
@@ -190,7 +203,9 @@ struct ti_sn65dsi86 {
 	u8				ln_assign;
 	u8				ln_polrs;
 	bool				comms_enabled;
+	bool				hpd_enabled;
 	struct mutex			comms_mutex;
+	struct mutex			hpd_mutex;
 
 #if defined(CONFIG_OF_GPIO)
 	struct gpio_chip		gchip;
@@ -221,6 +236,23 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = {
 	.max_register = 0xFF,
 };
 
+static int ti_sn65dsi86_read_u8(struct ti_sn65dsi86 *pdata, unsigned int reg,
+				u8 *val)
+{
+	int ret;
+	unsigned int reg_val;
+
+	ret = regmap_read(pdata->regmap, reg, &reg_val);
+	if (ret) {
+		dev_err(pdata->dev, "fail to read raw reg %#x: %d\n",
+			reg, ret);
+		return ret;
+	}
+	*val = (u8)reg_val;
+
+	return 0;
+}
+
 static int __maybe_unused ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
 						unsigned int reg, u16 *val)
 {
@@ -379,6 +411,7 @@ static void ti_sn65dsi86_disable_comms(struct ti_sn65dsi86 *pdata)
 static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 {
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
 	int ret;
 
 	ret = regulator_bulk_enable(SN_REGULATOR_SUPPLY_NUM, pdata->supplies);
@@ -413,6 +446,13 @@ static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 	if (pdata->refclk)
 		ti_sn65dsi86_enable_comms(pdata, NULL);
 
+	if (client->irq) {
+		ret = regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN,
+					 IRQ_EN);
+		if (ret)
+			pr_err("Failed to enable IRQ events: %d\n", ret);
+	}
+
 	return ret;
 }
 
@@ -1211,6 +1251,8 @@ static void ti_sn65dsi86_debugfs_init(struct drm_bridge *bridge, struct dentry *
 static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
+	int ret;
 
 	/*
 	 * Device needs to be powered on before reading the HPD state
@@ -1219,11 +1261,33 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 	 */
 
 	pm_runtime_get_sync(pdata->dev);
+
+	if (client->irq) {
+		/* Enable HPD events. */
+		ret = regmap_set_bits(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
+				      HPD_REMOVAL_EN | HPD_INSERTION_EN);
+		if (ret)
+			pr_err("Failed to enable HPD events: %d\n", ret);
+	}
+	mutex_lock(&pdata->hpd_mutex);
+	pdata->hpd_enabled = true;
+	mutex_unlock(&pdata->hpd_mutex);
 }
 
 static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
+
+	if (client->irq) {
+		/* Disable HPD events. */
+		regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG, 0);
+		regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN, 0);
+	}
+
+	mutex_lock(&pdata->hpd_mutex);
+	pdata->hpd_enabled = false;
+	mutex_unlock(&pdata->hpd_mutex);
 
 	pm_runtime_put_autosuspend(pdata->dev);
 }
@@ -1309,6 +1373,38 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
 	return 0;
 }
 
+static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
+{
+	struct ti_sn65dsi86 *pdata = private;
+	struct drm_device *dev = pdata->bridge.dev;
+	u8 status;
+	int ret;
+	bool hpd_event = false;
+
+	ret = ti_sn65dsi86_read_u8(pdata, SN_IRQ_STATUS_REG, &status);
+	if (ret)
+		pr_err("Failed to read IRQ status: %d\n", ret);
+	else
+		hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
+
+	if (status) {
+		pr_debug("(SN_IRQ_STATUS_REG = %#x)\n", status);
+		ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
+		if (ret)
+			pr_err("Failed to clear IRQ status: %d\n", ret);
+	} else {
+		return IRQ_NONE;
+	}
+
+	/* Only send the HPD event if we are bound with a device. */
+	mutex_lock(&pdata->hpd_mutex);
+	if (pdata->hpd_enabled && hpd_event)
+		drm_kms_helper_hotplug_event(dev);
+	mutex_unlock(&pdata->hpd_mutex);
+
+	return IRQ_HANDLED;
+}
+
 static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 			      const struct auxiliary_device_id *id)
 {
@@ -1931,6 +2027,8 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	dev_set_drvdata(dev, pdata);
 	pdata->dev = dev;
 
+	mutex_init(&pdata->hpd_mutex);
+
 	mutex_init(&pdata->comms_mutex);
 
 	pdata->regmap = devm_regmap_init_i2c(client,
@@ -1971,6 +2069,18 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	if (strncmp(id_buf, "68ISD   ", ARRAY_SIZE(id_buf)))
 		return dev_err_probe(dev, -EOPNOTSUPP, "unsupported device id\n");
 
+	if (client->irq) {
+		ret = devm_request_threaded_irq(pdata->dev, client->irq, NULL,
+						ti_sn_bridge_interrupt,
+						IRQF_ONESHOT,
+						dev_name(pdata->dev), pdata);
+
+		if (ret) {
+			return dev_err_probe(dev, ret,
+					     "failed to request interrupt\n");
+		}
+	}
+
 	/*
 	 * Break ourselves up into a collection of aux devices. The only real
 	 * motiviation here is to solve the chicken-and-egg problem of probe

[PATCH V4] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[John Ripple]

Add support for DisplayPort to the bridge, which entails the following:
- Get and use an interrupt for HPD;
- Properly clear all status bits in the interrupt handler;

Signed-off-by: John Ripple <john.ripple@keysight.com>
---
V1 -> V2: Cleaned up coding style and addressed review comments
V2 -> V3:
- Removed unused HPD IRQs
- Added mutex around HPD enable/disable and IRQ handler.
- Cleaned up error handling and variable declarations
- Only enable IRQs if the i2c client has an IRQ
- Moved IRQ_EN to ti_sn65dsi86_resume()
- Created ti_sn65dsi86_read_u8() helper function
V3 -> V4:
- Formatting
- Allow device tree to set interrupt type.
- Use hpd_mutex over less code.
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 110 ++++++++++++++++++++++++++
 1 file changed, 110 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index ae0d08e5e960..166920b2fcc7 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -106,10 +106,21 @@
 #define SN_PWM_EN_INV_REG			0xA5
 #define  SN_PWM_INV_MASK			BIT(0)
 #define  SN_PWM_EN_MASK				BIT(1)
+
+#define SN_IRQ_EN_REG				0xE0
+#define  IRQ_EN					BIT(0)
+
+#define SN_IRQ_EVENTS_EN_REG			0xE6
+#define  HPD_INSERTION_EN			BIT(1)
+#define  HPD_REMOVAL_EN				BIT(2)
+
 #define SN_AUX_CMD_STATUS_REG			0xF4
 #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
 #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
 #define  AUX_IRQ_STATUS_NAT_I2C_FAIL		BIT(6)
+#define SN_IRQ_STATUS_REG			0xF5
+#define  HPD_REMOVAL_STATUS			BIT(2)
+#define  HPD_INSERTION_STATUS			BIT(1)
 
 #define MIN_DSI_CLK_FREQ_MHZ	40
 
@@ -153,6 +164,8 @@
  * @ln_polrs:     Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG.
  * @comms_enabled: If true then communication over the aux channel is enabled.
  * @comms_mutex:   Protects modification of comms_enabled.
+ * @hpd_enabled:   If true then HPD events are enabled.
+ * @hpd_mutex:     Protects modification of hpd_enabled.
  *
  * @gchip:        If we expose our GPIOs, this is used.
  * @gchip_output: A cache of whether we've set GPIOs to output.  This
@@ -190,7 +203,9 @@ struct ti_sn65dsi86 {
 	u8				ln_assign;
 	u8				ln_polrs;
 	bool				comms_enabled;
+	bool				hpd_enabled;
 	struct mutex			comms_mutex;
+	struct mutex			hpd_mutex;
 
 #if defined(CONFIG_OF_GPIO)
 	struct gpio_chip		gchip;
@@ -221,6 +236,23 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = {
 	.max_register = 0xFF,
 };
 
+static int ti_sn65dsi86_read_u8(struct ti_sn65dsi86 *pdata, unsigned int reg,
+				u8 *val)
+{
+	int ret;
+	unsigned int reg_val;
+
+	ret = regmap_read(pdata->regmap, reg, &reg_val);
+	if (ret) {
+		dev_err(pdata->dev, "fail to read raw reg %#x: %d\n",
+			reg, ret);
+		return ret;
+	}
+	*val = (u8)reg_val;
+
+	return 0;
+}
+
 static int __maybe_unused ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
 						unsigned int reg, u16 *val)
 {
@@ -379,6 +411,7 @@ static void ti_sn65dsi86_disable_comms(struct ti_sn65dsi86 *pdata)
 static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 {
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
 	int ret;
 
 	ret = regulator_bulk_enable(SN_REGULATOR_SUPPLY_NUM, pdata->supplies);
@@ -413,6 +446,13 @@ static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 	if (pdata->refclk)
 		ti_sn65dsi86_enable_comms(pdata, NULL);
 
+	if (client->irq) {
+		ret = regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN,
+					 IRQ_EN);
+		if (ret)
+			pr_err("Failed to enable IRQ events: %d\n", ret);
+	}
+
 	return ret;
 }
 
@@ -1211,6 +1251,8 @@ static void ti_sn65dsi86_debugfs_init(struct drm_bridge *bridge, struct dentry *
 static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
+	int ret;
 
 	/*
 	 * Device needs to be powered on before reading the HPD state
@@ -1219,11 +1261,33 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 	 */
 
 	pm_runtime_get_sync(pdata->dev);
+
+	if (client->irq) {
+		/* Enable HPD events. */
+		ret = regmap_set_bits(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
+				      HPD_REMOVAL_EN | HPD_INSERTION_EN);
+		if (ret)
+			pr_err("Failed to enable HPD events: %d\n", ret);
+	}
+	mutex_lock(&pdata->hpd_mutex);
+	pdata->hpd_enabled = true;
+	mutex_unlock(&pdata->hpd_mutex);
 }
 
 static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
+
+	if (client->irq) {
+		/* Disable HPD events. */
+		regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG, 0);
+		regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN, 0);
+	}
+
+	mutex_lock(&pdata->hpd_mutex);
+	pdata->hpd_enabled = false;
+	mutex_unlock(&pdata->hpd_mutex);
 
 	pm_runtime_put_autosuspend(pdata->dev);
 }
@@ -1309,6 +1373,38 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
 	return 0;
 }
 
+static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
+{
+	struct ti_sn65dsi86 *pdata = private;
+	struct drm_device *dev = pdata->bridge.dev;
+	u8 status;
+	int ret;
+	bool hpd_event = false;
+
+	ret = ti_sn65dsi86_read_u8(pdata, SN_IRQ_STATUS_REG, &status);
+	if (ret)
+		pr_err("Failed to read IRQ status: %d\n", ret);
+	else
+		hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
+
+	if (status) {
+		pr_debug("(SN_IRQ_STATUS_REG = %#x)\n", status);
+		ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
+		if (ret)
+			pr_err("Failed to clear IRQ status: %d\n", ret);
+	} else {
+		return IRQ_NONE;
+	}
+
+	/* Only send the HPD event if we are bound with a device. */
+	mutex_lock(&pdata->hpd_mutex);
+	if (pdata->hpd_enabled && hpd_event)
+		drm_kms_helper_hotplug_event(dev);
+	mutex_unlock(&pdata->hpd_mutex);
+
+	return IRQ_HANDLED;
+}
+
 static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 			      const struct auxiliary_device_id *id)
 {
@@ -1931,6 +2027,8 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	dev_set_drvdata(dev, pdata);
 	pdata->dev = dev;
 
+	mutex_init(&pdata->hpd_mutex);
+
 	mutex_init(&pdata->comms_mutex);
 
 	pdata->regmap = devm_regmap_init_i2c(client,
@@ -1971,6 +2069,18 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	if (strncmp(id_buf, "68ISD   ", ARRAY_SIZE(id_buf)))
 		return dev_err_probe(dev, -EOPNOTSUPP, "unsupported device id\n");
 
+	if (client->irq) {
+		ret = devm_request_threaded_irq(pdata->dev, client->irq, NULL,
+						ti_sn_bridge_interrupt,
+						IRQF_ONESHOT,
+						dev_name(pdata->dev), pdata);
+
+		if (ret) {
+			return dev_err_probe(dev, ret,
+					     "failed to request interrupt\n");
+		}
+	}
+
 	/*
 	 * Break ourselves up into a collection of aux devices. The only real
 	 * motiviation here is to solve the chicken-and-egg problem of probe

Re: [PATCH V4] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[Doug Anderson]

Hi,

On Fri, Sep 12, 2025 at 12:25 PM John Ripple <john.ripple@keysight.com> wrote:
>
> @@ -153,6 +164,8 @@
>   * @ln_polrs:     Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG.
>   * @comms_enabled: If true then communication over the aux channel is enabled.
>   * @comms_mutex:   Protects modification of comms_enabled.
> + * @hpd_enabled:   If true then HPD events are enabled.
> + * @hpd_mutex:     Protects modification of hpd_enabled.

nit: order should match the order of elements in the struct, so
hpd_enabled should be above comms_mutex.


> @@ -1219,11 +1261,33 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
>          */
>
>         pm_runtime_get_sync(pdata->dev);
> +
> +       if (client->irq) {
> +               /* Enable HPD events. */

nit: the comment probably doesn't buy us too much...


> +               ret = regmap_set_bits(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
> +                                     HPD_REMOVAL_EN | HPD_INSERTION_EN);
> +               if (ret)
> +                       pr_err("Failed to enable HPD events: %d\n", ret);
> +       }
> +       mutex_lock(&pdata->hpd_mutex);
> +       pdata->hpd_enabled = true;
> +       mutex_unlock(&pdata->hpd_mutex);

For enable, I think you want this _above_ the setting of the bits.
Then if an interrupt fires right off the bat you'll see it. That also
matches the intuition that usually enable and disable should be mirror
images of each other (they should do things in the opposite order).


>  }
>
>  static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
>  {
>         struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +       const struct i2c_client *client = to_i2c_client(pdata->dev);
> +
> +       if (client->irq) {
> +               /* Disable HPD events. */
> +               regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG, 0);

To make it symmetric to ti_sn_bridge_hpd_enable(), you should probably
use regmap_clear_bits(). Then if we later enable other interrupt
sources then they'll stay enabled.


> +               regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN, 0);

I don't think you need to clear this. It can just stay enabled but no
interrupts will fire since there are no interrupt sources. This will
make things work right if we later enable other interrupt sources.


> @@ -1309,6 +1373,38 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
>         return 0;
>  }
>
> +static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
> +{
> +       struct ti_sn65dsi86 *pdata = private;
> +       struct drm_device *dev = pdata->bridge.dev;
> +       u8 status;
> +       int ret;
> +       bool hpd_event = false;
> +
> +       ret = ti_sn65dsi86_read_u8(pdata, SN_IRQ_STATUS_REG, &status);
> +       if (ret)
> +               pr_err("Failed to read IRQ status: %d\n", ret);

You should not only print but return IRQ_NONE here IMO. If there are
constant errors (for whatever reason) the interrupt will eventually
get disabled by the system which is better than storming. It also
means that later code doesn't access a bogus "status".


> +       else
> +               hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
> +
> +       if (status) {
> +               pr_debug("(SN_IRQ_STATUS_REG = %#x)\n", status);
> +               ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
> +               if (ret)
> +                       pr_err("Failed to clear IRQ status: %d\n", ret);
> +       } else {
> +               return IRQ_NONE;
> +       }

FWIW: Linux conventions usually are to handle error cases in the "if"
case to avoid indentation (because you don't need an "else"). AKA:

if (!status)
  return IRQ_NONE;

pr_debug(...)
ret = ...
...

[PATCH V5] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[John Ripple]

Add support for DisplayPort to the bridge, which entails the following:
- Get and use an interrupt for HPD;
- Properly clear all status bits in the interrupt handler;

Signed-off-by: John Ripple <john.ripple@keysight.com>
---
V1 -> V2: Cleaned up coding style and addressed review comments
V2 -> V3:
- Removed unused HPD IRQs
- Added mutex around HPD enable/disable and IRQ handler.
- Cleaned up error handling and variable declarations
- Only enable IRQs if the i2c client has an IRQ
- Moved IRQ_EN to ti_sn65dsi86_resume()
- Created ti_sn65dsi86_read_u8() helper function
V3 -> V4:
- Formatting
- Allow device tree to set interrupt type.
- Use hpd_mutex over less code.
V4 -> V5:
- Formatting.
- Symmetric mutex use in hpd enable/disable functions.
- Only set and clear specific bits for IRQ enable and disable.
- Better error handling in interrupt.
---
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 112 ++++++++++++++++++++++++++
 1 file changed, 112 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index ae0d08e5e960..239b89838845 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -106,10 +106,21 @@
 #define SN_PWM_EN_INV_REG			0xA5
 #define  SN_PWM_INV_MASK			BIT(0)
 #define  SN_PWM_EN_MASK				BIT(1)
+
+#define SN_IRQ_EN_REG				0xE0
+#define  IRQ_EN					BIT(0)
+
+#define SN_IRQ_EVENTS_EN_REG			0xE6
+#define  HPD_INSERTION_EN			BIT(1)
+#define  HPD_REMOVAL_EN				BIT(2)
+
 #define SN_AUX_CMD_STATUS_REG			0xF4
 #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
 #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
 #define  AUX_IRQ_STATUS_NAT_I2C_FAIL		BIT(6)
+#define SN_IRQ_STATUS_REG			0xF5
+#define  HPD_REMOVAL_STATUS			BIT(2)
+#define  HPD_INSERTION_STATUS			BIT(1)
 
 #define MIN_DSI_CLK_FREQ_MHZ	40
 
@@ -152,7 +163,9 @@
  * @ln_assign:    Value to program to the LN_ASSIGN register.
  * @ln_polrs:     Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG.
  * @comms_enabled: If true then communication over the aux channel is enabled.
+ * @hpd_enabled:   If true then HPD events are enabled.
  * @comms_mutex:   Protects modification of comms_enabled.
+ * @hpd_mutex:     Protects modification of hpd_enabled.
  *
  * @gchip:        If we expose our GPIOs, this is used.
  * @gchip_output: A cache of whether we've set GPIOs to output.  This
@@ -190,7 +203,9 @@ struct ti_sn65dsi86 {
 	u8				ln_assign;
 	u8				ln_polrs;
 	bool				comms_enabled;
+	bool				hpd_enabled;
 	struct mutex			comms_mutex;
+	struct mutex			hpd_mutex;
 
 #if defined(CONFIG_OF_GPIO)
 	struct gpio_chip		gchip;
@@ -221,6 +236,23 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = {
 	.max_register = 0xFF,
 };
 
+static int ti_sn65dsi86_read_u8(struct ti_sn65dsi86 *pdata, unsigned int reg,
+				u8 *val)
+{
+	int ret;
+	unsigned int reg_val;
+
+	ret = regmap_read(pdata->regmap, reg, &reg_val);
+	if (ret) {
+		dev_err(pdata->dev, "fail to read raw reg %#x: %d\n",
+			reg, ret);
+		return ret;
+	}
+	*val = (u8)reg_val;
+
+	return 0;
+}
+
 static int __maybe_unused ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
 						unsigned int reg, u16 *val)
 {
@@ -379,6 +411,7 @@ static void ti_sn65dsi86_disable_comms(struct ti_sn65dsi86 *pdata)
 static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 {
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
 	int ret;
 
 	ret = regulator_bulk_enable(SN_REGULATOR_SUPPLY_NUM, pdata->supplies);
@@ -413,6 +446,13 @@ static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 	if (pdata->refclk)
 		ti_sn65dsi86_enable_comms(pdata, NULL);
 
+	if (client->irq) {
+		ret = regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN,
+					 IRQ_EN);
+		if (ret)
+			pr_err("Failed to enable IRQ events: %d\n", ret);
+	}
+
 	return ret;
 }
 
@@ -1211,6 +1251,8 @@ static void ti_sn65dsi86_debugfs_init(struct drm_bridge *bridge, struct dentry *
 static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
+	int ret;
 
 	/*
 	 * Device needs to be powered on before reading the HPD state
@@ -1219,11 +1261,32 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 	 */
 
 	pm_runtime_get_sync(pdata->dev);
+
+	mutex_lock(&pdata->hpd_mutex);
+	pdata->hpd_enabled = true;
+	mutex_unlock(&pdata->hpd_mutex);
+
+	if (client->irq) {
+		ret = regmap_set_bits(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
+				      HPD_REMOVAL_EN | HPD_INSERTION_EN);
+		if (ret)
+			pr_err("Failed to enable HPD events: %d\n", ret);
+	}
 }
 
 static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
+
+	if (client->irq) {
+		regmap_clear_bits(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
+				  HPD_REMOVAL_EN | HPD_INSERTION_EN);
+	}
+
+	mutex_lock(&pdata->hpd_mutex);
+	pdata->hpd_enabled = false;
+	mutex_unlock(&pdata->hpd_mutex);
 
 	pm_runtime_put_autosuspend(pdata->dev);
 }
@@ -1309,6 +1372,41 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
 	return 0;
 }
 
+static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
+{
+	struct ti_sn65dsi86 *pdata = private;
+	struct drm_device *dev = pdata->bridge.dev;
+	u8 status;
+	int ret;
+	bool hpd_event;
+
+	ret = ti_sn65dsi86_read_u8(pdata, SN_IRQ_STATUS_REG, &status);
+	if (ret) {
+		pr_err("Failed to read IRQ status: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
+
+	if (!status)
+		return IRQ_NONE;
+
+	pr_debug("(SN_IRQ_STATUS_REG = %#x)\n", status);
+	ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
+	if (ret) {
+		pr_err("Failed to clear IRQ status: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	/* Only send the HPD event if we are bound with a device. */
+	mutex_lock(&pdata->hpd_mutex);
+	if (pdata->hpd_enabled && hpd_event)
+		drm_kms_helper_hotplug_event(dev);
+	mutex_unlock(&pdata->hpd_mutex);
+
+	return IRQ_HANDLED;
+}
+
 static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 			      const struct auxiliary_device_id *id)
 {
@@ -1931,6 +2029,8 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	dev_set_drvdata(dev, pdata);
 	pdata->dev = dev;
 
+	mutex_init(&pdata->hpd_mutex);
+
 	mutex_init(&pdata->comms_mutex);
 
 	pdata->regmap = devm_regmap_init_i2c(client,
@@ -1971,6 +2071,18 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	if (strncmp(id_buf, "68ISD   ", ARRAY_SIZE(id_buf)))
 		return dev_err_probe(dev, -EOPNOTSUPP, "unsupported device id\n");
 
+	if (client->irq) {
+		ret = devm_request_threaded_irq(pdata->dev, client->irq, NULL,
+						ti_sn_bridge_interrupt,
+						IRQF_ONESHOT,
+						dev_name(pdata->dev), pdata);
+
+		if (ret) {
+			return dev_err_probe(dev, ret,
+					     "failed to request interrupt\n");
+		}
+	}
+
 	/*
 	 * Break ourselves up into a collection of aux devices. The only real
 	 * motiviation here is to solve the chicken-and-egg problem of probe

[PATCH V5] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[John Ripple]

Add support for DisplayPort to the bridge, which entails the following:
- Get and use an interrupt for HPD;
- Properly clear all status bits in the interrupt handler;

Signed-off-by: John Ripple <john.ripple@keysight.com>
---
V1 -> V2: Cleaned up coding style and addressed review comments
V2 -> V3:
- Removed unused HPD IRQs
- Added mutex around HPD enable/disable and IRQ handler.
- Cleaned up error handling and variable declarations
- Only enable IRQs if the i2c client has an IRQ
- Moved IRQ_EN to ti_sn65dsi86_resume()
- Created ti_sn65dsi86_read_u8() helper function
V3 -> V4:
- Formatting
- Allow device tree to set interrupt type.
- Use hpd_mutex over less code.
V4 -> V5:
- Formatting.
- Symmetric mutex use in hpd enable/disable functions.
- Only set and clear specific bits for IRQ enable and disable.
- Better error handling in interrupt.
---
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 112 ++++++++++++++++++++++++++
 1 file changed, 112 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index ae0d08e5e960..239b89838845 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -106,10 +106,21 @@
 #define SN_PWM_EN_INV_REG			0xA5
 #define  SN_PWM_INV_MASK			BIT(0)
 #define  SN_PWM_EN_MASK				BIT(1)
+
+#define SN_IRQ_EN_REG				0xE0
+#define  IRQ_EN					BIT(0)
+
+#define SN_IRQ_EVENTS_EN_REG			0xE6
+#define  HPD_INSERTION_EN			BIT(1)
+#define  HPD_REMOVAL_EN				BIT(2)
+
 #define SN_AUX_CMD_STATUS_REG			0xF4
 #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
 #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
 #define  AUX_IRQ_STATUS_NAT_I2C_FAIL		BIT(6)
+#define SN_IRQ_STATUS_REG			0xF5
+#define  HPD_REMOVAL_STATUS			BIT(2)
+#define  HPD_INSERTION_STATUS			BIT(1)
 
 #define MIN_DSI_CLK_FREQ_MHZ	40
 
@@ -152,7 +163,9 @@
  * @ln_assign:    Value to program to the LN_ASSIGN register.
  * @ln_polrs:     Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG.
  * @comms_enabled: If true then communication over the aux channel is enabled.
+ * @hpd_enabled:   If true then HPD events are enabled.
  * @comms_mutex:   Protects modification of comms_enabled.
+ * @hpd_mutex:     Protects modification of hpd_enabled.
  *
  * @gchip:        If we expose our GPIOs, this is used.
  * @gchip_output: A cache of whether we've set GPIOs to output.  This
@@ -190,7 +203,9 @@ struct ti_sn65dsi86 {
 	u8				ln_assign;
 	u8				ln_polrs;
 	bool				comms_enabled;
+	bool				hpd_enabled;
 	struct mutex			comms_mutex;
+	struct mutex			hpd_mutex;
 
 #if defined(CONFIG_OF_GPIO)
 	struct gpio_chip		gchip;
@@ -221,6 +236,23 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = {
 	.max_register = 0xFF,
 };
 
+static int ti_sn65dsi86_read_u8(struct ti_sn65dsi86 *pdata, unsigned int reg,
+				u8 *val)
+{
+	int ret;
+	unsigned int reg_val;
+
+	ret = regmap_read(pdata->regmap, reg, &reg_val);
+	if (ret) {
+		dev_err(pdata->dev, "fail to read raw reg %#x: %d\n",
+			reg, ret);
+		return ret;
+	}
+	*val = (u8)reg_val;
+
+	return 0;
+}
+
 static int __maybe_unused ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
 						unsigned int reg, u16 *val)
 {
@@ -379,6 +411,7 @@ static void ti_sn65dsi86_disable_comms(struct ti_sn65dsi86 *pdata)
 static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 {
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
 	int ret;
 
 	ret = regulator_bulk_enable(SN_REGULATOR_SUPPLY_NUM, pdata->supplies);
@@ -413,6 +446,13 @@ static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 	if (pdata->refclk)
 		ti_sn65dsi86_enable_comms(pdata, NULL);
 
+	if (client->irq) {
+		ret = regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN,
+					 IRQ_EN);
+		if (ret)
+			pr_err("Failed to enable IRQ events: %d\n", ret);
+	}
+
 	return ret;
 }
 
@@ -1211,6 +1251,8 @@ static void ti_sn65dsi86_debugfs_init(struct drm_bridge *bridge, struct dentry *
 static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
+	int ret;
 
 	/*
 	 * Device needs to be powered on before reading the HPD state
@@ -1219,11 +1261,32 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 	 */
 
 	pm_runtime_get_sync(pdata->dev);
+
+	mutex_lock(&pdata->hpd_mutex);
+	pdata->hpd_enabled = true;
+	mutex_unlock(&pdata->hpd_mutex);
+
+	if (client->irq) {
+		ret = regmap_set_bits(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
+				      HPD_REMOVAL_EN | HPD_INSERTION_EN);
+		if (ret)
+			pr_err("Failed to enable HPD events: %d\n", ret);
+	}
 }
 
 static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
+
+	if (client->irq) {
+		regmap_clear_bits(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
+				  HPD_REMOVAL_EN | HPD_INSERTION_EN);
+	}
+
+	mutex_lock(&pdata->hpd_mutex);
+	pdata->hpd_enabled = false;
+	mutex_unlock(&pdata->hpd_mutex);
 
 	pm_runtime_put_autosuspend(pdata->dev);
 }
@@ -1309,6 +1372,41 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
 	return 0;
 }
 
+static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
+{
+	struct ti_sn65dsi86 *pdata = private;
+	struct drm_device *dev = pdata->bridge.dev;
+	u8 status;
+	int ret;
+	bool hpd_event;
+
+	ret = ti_sn65dsi86_read_u8(pdata, SN_IRQ_STATUS_REG, &status);
+	if (ret) {
+		pr_err("Failed to read IRQ status: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
+
+	if (!status)
+		return IRQ_NONE;
+
+	pr_debug("(SN_IRQ_STATUS_REG = %#x)\n", status);
+	ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
+	if (ret) {
+		pr_err("Failed to clear IRQ status: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	/* Only send the HPD event if we are bound with a device. */
+	mutex_lock(&pdata->hpd_mutex);
+	if (pdata->hpd_enabled && hpd_event)
+		drm_kms_helper_hotplug_event(dev);
+	mutex_unlock(&pdata->hpd_mutex);
+
+	return IRQ_HANDLED;
+}
+
 static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 			      const struct auxiliary_device_id *id)
 {
@@ -1931,6 +2029,8 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	dev_set_drvdata(dev, pdata);
 	pdata->dev = dev;
 
+	mutex_init(&pdata->hpd_mutex);
+
 	mutex_init(&pdata->comms_mutex);
 
 	pdata->regmap = devm_regmap_init_i2c(client,
@@ -1971,6 +2071,18 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	if (strncmp(id_buf, "68ISD   ", ARRAY_SIZE(id_buf)))
 		return dev_err_probe(dev, -EOPNOTSUPP, "unsupported device id\n");
 
+	if (client->irq) {
+		ret = devm_request_threaded_irq(pdata->dev, client->irq, NULL,
+						ti_sn_bridge_interrupt,
+						IRQF_ONESHOT,
+						dev_name(pdata->dev), pdata);
+
+		if (ret) {
+			return dev_err_probe(dev, ret,
+					     "failed to request interrupt\n");
+		}
+	}
+
 	/*
 	 * Break ourselves up into a collection of aux devices. The only real
 	 * motiviation here is to solve the chicken-and-egg problem of probe

Re: [PATCH V5] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[Doug Anderson]

Hi,

On Fri, Sep 12, 2025 at 2:08 PM John Ripple <john.ripple@keysight.com> wrote:
>
> @@ -413,6 +446,13 @@ static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
>         if (pdata->refclk)
>                 ti_sn65dsi86_enable_comms(pdata, NULL);
>
> +       if (client->irq) {
> +               ret = regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN,
> +                                        IRQ_EN);
> +               if (ret)
> +                       pr_err("Failed to enable IRQ events: %d\n", ret);

Shoot, I should have noticed it before. Sorry! :(

Probably most of the "pr_" calls in your patch should be "dev_" calls.
"struct ti_sn65dsi86" should have a dev pointer in it. That's probably
worth spinning the patch. It's really close now, though!


> @@ -1309,6 +1372,41 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
>         return 0;
>  }
>
> +static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
> +{
> +       struct ti_sn65dsi86 *pdata = private;
> +       struct drm_device *dev = pdata->bridge.dev;
> +       u8 status;
> +       int ret;
> +       bool hpd_event;
> +
> +       ret = ti_sn65dsi86_read_u8(pdata, SN_IRQ_STATUS_REG, &status);
> +       if (ret) {
> +               pr_err("Failed to read IRQ status: %d\n", ret);
> +               return IRQ_NONE;
> +       }
> +
> +       hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
> +
> +       if (!status)
> +               return IRQ_NONE;

It wouldn't have been worth spinning just for this, but if we're
spinning anyway I'd probably put the "if (!status)" check down below
right before you grab the mutex, just to keep all the HPD processing
together.


> @@ -1931,6 +2029,8 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
>         dev_set_drvdata(dev, pdata);
>         pdata->dev = dev;
>
> +       mutex_init(&pdata->hpd_mutex);
> +
>         mutex_init(&pdata->comms_mutex);

Again, it wouldn't be worth spinning on its own, but if you happened
to want to get rid of the blank line between the two I wouldn't
object. ;-)


> @@ -1971,6 +2071,18 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
>         if (strncmp(id_buf, "68ISD   ", ARRAY_SIZE(id_buf)))
>                 return dev_err_probe(dev, -EOPNOTSUPP, "unsupported device id\n");
>
> +       if (client->irq) {
> +               ret = devm_request_threaded_irq(pdata->dev, client->irq, NULL,
> +                                               ti_sn_bridge_interrupt,
> +                                               IRQF_ONESHOT,
> +                                               dev_name(pdata->dev), pdata);
> +
> +               if (ret) {
> +                       return dev_err_probe(dev, ret,
> +                                            "failed to request interrupt\n");
> +               }

Another tiny nitpick if you happen to want to fix up when you're
spinning. Officially the above "if" statement probably shouldn't have
braces. I think checkpatch won't yell since it's kinda two lines, but
it's really just one statement... You could even make it one line
since the 80-column rule has been relaxed a bit in the last few
years...


Sorry, I should have noticed the "pr_" stuff on the last review. Sorry
for making you spin again. Hopefully the last one? I think the patch
looks a lot nicer now FWIW! :-)

-Doug

[PATCH V6] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[John Ripple]

Add support for DisplayPort to the bridge, which entails the following:
- Get and use an interrupt for HPD;
- Properly clear all status bits in the interrupt handler;

Signed-off-by: John Ripple <john.ripple@keysight.com>
---
V1 -> V2: Cleaned up coding style and addressed review comments
V2 -> V3:
- Removed unused HPD IRQs
- Added mutex around HPD enable/disable and IRQ handler.
- Cleaned up error handling and variable declarations
- Only enable IRQs if the i2c client has an IRQ
- Moved IRQ_EN to ti_sn65dsi86_resume()
- Created ti_sn65dsi86_read_u8() helper function
V3 -> V4:
- Formatting
- Allow device tree to set interrupt type.
- Use hpd_mutex over less code.
V4 -> V5:
- Formatting.
- Symmetric mutex use in hpd enable/disable functions.
- Only set and clear specific bits for IRQ enable and disable.
- Better error handling in interrupt.
V5 -> V6:
- Formatting.
- Convert pr_ to dev_ for printing.
- Add error check to regmap_clear_bits() call.
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 112 ++++++++++++++++++++++++++
 1 file changed, 112 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index ae0d08e5e960..3d161148801a 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -106,10 +106,21 @@
 #define SN_PWM_EN_INV_REG			0xA5
 #define  SN_PWM_INV_MASK			BIT(0)
 #define  SN_PWM_EN_MASK				BIT(1)
+
+#define SN_IRQ_EN_REG				0xE0
+#define  IRQ_EN					BIT(0)
+
+#define SN_IRQ_EVENTS_EN_REG			0xE6
+#define  HPD_INSERTION_EN			BIT(1)
+#define  HPD_REMOVAL_EN				BIT(2)
+
 #define SN_AUX_CMD_STATUS_REG			0xF4
 #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
 #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
 #define  AUX_IRQ_STATUS_NAT_I2C_FAIL		BIT(6)
+#define SN_IRQ_STATUS_REG			0xF5
+#define  HPD_REMOVAL_STATUS			BIT(2)
+#define  HPD_INSERTION_STATUS			BIT(1)
 
 #define MIN_DSI_CLK_FREQ_MHZ	40
 
@@ -152,7 +163,9 @@
  * @ln_assign:    Value to program to the LN_ASSIGN register.
  * @ln_polrs:     Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG.
  * @comms_enabled: If true then communication over the aux channel is enabled.
+ * @hpd_enabled:   If true then HPD events are enabled.
  * @comms_mutex:   Protects modification of comms_enabled.
+ * @hpd_mutex:     Protects modification of hpd_enabled.
  *
  * @gchip:        If we expose our GPIOs, this is used.
  * @gchip_output: A cache of whether we've set GPIOs to output.  This
@@ -190,7 +203,9 @@ struct ti_sn65dsi86 {
 	u8				ln_assign;
 	u8				ln_polrs;
 	bool				comms_enabled;
+	bool				hpd_enabled;
 	struct mutex			comms_mutex;
+	struct mutex			hpd_mutex;
 
 #if defined(CONFIG_OF_GPIO)
 	struct gpio_chip		gchip;
@@ -221,6 +236,23 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = {
 	.max_register = 0xFF,
 };
 
+static int ti_sn65dsi86_read_u8(struct ti_sn65dsi86 *pdata, unsigned int reg,
+				u8 *val)
+{
+	int ret;
+	unsigned int reg_val;
+
+	ret = regmap_read(pdata->regmap, reg, &reg_val);
+	if (ret) {
+		dev_err(pdata->dev, "fail to read raw reg %#x: %d\n",
+			reg, ret);
+		return ret;
+	}
+	*val = (u8)reg_val;
+
+	return 0;
+}
+
 static int __maybe_unused ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
 						unsigned int reg, u16 *val)
 {
@@ -379,6 +411,7 @@ static void ti_sn65dsi86_disable_comms(struct ti_sn65dsi86 *pdata)
 static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 {
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
 	int ret;
 
 	ret = regulator_bulk_enable(SN_REGULATOR_SUPPLY_NUM, pdata->supplies);
@@ -413,6 +446,13 @@ static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 	if (pdata->refclk)
 		ti_sn65dsi86_enable_comms(pdata, NULL);
 
+	if (client->irq) {
+		ret = regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN,
+					 IRQ_EN);
+		if (ret)
+			dev_err(pdata->dev, "Failed to enable IRQ events: %d\n", ret);
+	}
+
 	return ret;
 }
 
@@ -1211,6 +1251,8 @@ static void ti_sn65dsi86_debugfs_init(struct drm_bridge *bridge, struct dentry *
 static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
+	int ret;
 
 	/*
 	 * Device needs to be powered on before reading the HPD state
@@ -1219,11 +1261,35 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 	 */
 
 	pm_runtime_get_sync(pdata->dev);
+
+	mutex_lock(&pdata->hpd_mutex);
+	pdata->hpd_enabled = true;
+	mutex_unlock(&pdata->hpd_mutex);
+
+	if (client->irq) {
+		ret = regmap_set_bits(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
+				      HPD_REMOVAL_EN | HPD_INSERTION_EN);
+		if (ret)
+			dev_err(pdata->dev, "Failed to enable HPD events: %d\n", ret);
+	}
 }
 
 static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
+	int ret;
+
+	if (client->irq) {
+		ret = regmap_clear_bits(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
+					HPD_REMOVAL_EN | HPD_INSERTION_EN);
+		if (ret)
+			dev_err(pdata->dev, "Failed to disable HPD events: %d\n", ret);
+	}
+
+	mutex_lock(&pdata->hpd_mutex);
+	pdata->hpd_enabled = false;
+	mutex_unlock(&pdata->hpd_mutex);
 
 	pm_runtime_put_autosuspend(pdata->dev);
 }
@@ -1309,6 +1375,41 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
 	return 0;
 }
 
+static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
+{
+	struct ti_sn65dsi86 *pdata = private;
+	struct drm_device *dev = pdata->bridge.dev;
+	u8 status;
+	int ret;
+	bool hpd_event;
+
+	ret = ti_sn65dsi86_read_u8(pdata, SN_IRQ_STATUS_REG, &status);
+	if (ret) {
+		dev_err(pdata->dev, "Failed to read IRQ status: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
+
+	dev_dbg(pdata->dev, "(SN_IRQ_STATUS_REG = %#x)\n", status);
+	ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
+	if (ret) {
+		dev_err(pdata->dev, "Failed to clear IRQ status: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	if (!status)
+		return IRQ_NONE;
+
+	/* Only send the HPD event if we are bound with a device. */
+	mutex_lock(&pdata->hpd_mutex);
+	if (pdata->hpd_enabled && hpd_event)
+		drm_kms_helper_hotplug_event(dev);
+	mutex_unlock(&pdata->hpd_mutex);
+
+	return IRQ_HANDLED;
+}
+
 static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 			      const struct auxiliary_device_id *id)
 {
@@ -1931,6 +2032,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	dev_set_drvdata(dev, pdata);
 	pdata->dev = dev;
 
+	mutex_init(&pdata->hpd_mutex);
 	mutex_init(&pdata->comms_mutex);
 
 	pdata->regmap = devm_regmap_init_i2c(client,
@@ -1971,6 +2073,16 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	if (strncmp(id_buf, "68ISD   ", ARRAY_SIZE(id_buf)))
 		return dev_err_probe(dev, -EOPNOTSUPP, "unsupported device id\n");
 
+	if (client->irq) {
+		ret = devm_request_threaded_irq(pdata->dev, client->irq, NULL,
+						ti_sn_bridge_interrupt,
+						IRQF_ONESHOT,
+						dev_name(pdata->dev), pdata);
+
+		if (ret)
+			return dev_err_probe(dev, ret, "failed to request interrupt\n");
+	}
+
 	/*
 	 * Break ourselves up into a collection of aux devices. The only real
 	 * motiviation here is to solve the chicken-and-egg problem of probe

[PATCH V6] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[John Ripple]

Add support for DisplayPort to the bridge, which entails the following:
- Get and use an interrupt for HPD;
- Properly clear all status bits in the interrupt handler;

Signed-off-by: John Ripple <john.ripple@keysight.com>
---
V1 -> V2: Cleaned up coding style and addressed review comments
V2 -> V3:
- Removed unused HPD IRQs
- Added mutex around HPD enable/disable and IRQ handler.
- Cleaned up error handling and variable declarations
- Only enable IRQs if the i2c client has an IRQ
- Moved IRQ_EN to ti_sn65dsi86_resume()
- Created ti_sn65dsi86_read_u8() helper function
V3 -> V4:
- Formatting
- Allow device tree to set interrupt type.
- Use hpd_mutex over less code.
V4 -> V5:
- Formatting.
- Symmetric mutex use in hpd enable/disable functions.
- Only set and clear specific bits for IRQ enable and disable.
- Better error handling in interrupt.
V5 -> V6:
- Formatting.
- Convert pr_ to dev_ for printing.
- Add error check to regmap_clear_bits() call.
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 112 ++++++++++++++++++++++++++
 1 file changed, 112 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index ae0d08e5e960..3d161148801a 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -106,10 +106,21 @@
 #define SN_PWM_EN_INV_REG			0xA5
 #define  SN_PWM_INV_MASK			BIT(0)
 #define  SN_PWM_EN_MASK				BIT(1)
+
+#define SN_IRQ_EN_REG				0xE0
+#define  IRQ_EN					BIT(0)
+
+#define SN_IRQ_EVENTS_EN_REG			0xE6
+#define  HPD_INSERTION_EN			BIT(1)
+#define  HPD_REMOVAL_EN				BIT(2)
+
 #define SN_AUX_CMD_STATUS_REG			0xF4
 #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
 #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
 #define  AUX_IRQ_STATUS_NAT_I2C_FAIL		BIT(6)
+#define SN_IRQ_STATUS_REG			0xF5
+#define  HPD_REMOVAL_STATUS			BIT(2)
+#define  HPD_INSERTION_STATUS			BIT(1)
 
 #define MIN_DSI_CLK_FREQ_MHZ	40
 
@@ -152,7 +163,9 @@
  * @ln_assign:    Value to program to the LN_ASSIGN register.
  * @ln_polrs:     Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG.
  * @comms_enabled: If true then communication over the aux channel is enabled.
+ * @hpd_enabled:   If true then HPD events are enabled.
  * @comms_mutex:   Protects modification of comms_enabled.
+ * @hpd_mutex:     Protects modification of hpd_enabled.
  *
  * @gchip:        If we expose our GPIOs, this is used.
  * @gchip_output: A cache of whether we've set GPIOs to output.  This
@@ -190,7 +203,9 @@ struct ti_sn65dsi86 {
 	u8				ln_assign;
 	u8				ln_polrs;
 	bool				comms_enabled;
+	bool				hpd_enabled;
 	struct mutex			comms_mutex;
+	struct mutex			hpd_mutex;
 
 #if defined(CONFIG_OF_GPIO)
 	struct gpio_chip		gchip;
@@ -221,6 +236,23 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = {
 	.max_register = 0xFF,
 };
 
+static int ti_sn65dsi86_read_u8(struct ti_sn65dsi86 *pdata, unsigned int reg,
+				u8 *val)
+{
+	int ret;
+	unsigned int reg_val;
+
+	ret = regmap_read(pdata->regmap, reg, &reg_val);
+	if (ret) {
+		dev_err(pdata->dev, "fail to read raw reg %#x: %d\n",
+			reg, ret);
+		return ret;
+	}
+	*val = (u8)reg_val;
+
+	return 0;
+}
+
 static int __maybe_unused ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
 						unsigned int reg, u16 *val)
 {
@@ -379,6 +411,7 @@ static void ti_sn65dsi86_disable_comms(struct ti_sn65dsi86 *pdata)
 static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 {
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
 	int ret;
 
 	ret = regulator_bulk_enable(SN_REGULATOR_SUPPLY_NUM, pdata->supplies);
@@ -413,6 +446,13 @@ static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 	if (pdata->refclk)
 		ti_sn65dsi86_enable_comms(pdata, NULL);
 
+	if (client->irq) {
+		ret = regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN,
+					 IRQ_EN);
+		if (ret)
+			dev_err(pdata->dev, "Failed to enable IRQ events: %d\n", ret);
+	}
+
 	return ret;
 }
 
@@ -1211,6 +1251,8 @@ static void ti_sn65dsi86_debugfs_init(struct drm_bridge *bridge, struct dentry *
 static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
+	int ret;
 
 	/*
 	 * Device needs to be powered on before reading the HPD state
@@ -1219,11 +1261,35 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 	 */
 
 	pm_runtime_get_sync(pdata->dev);
+
+	mutex_lock(&pdata->hpd_mutex);
+	pdata->hpd_enabled = true;
+	mutex_unlock(&pdata->hpd_mutex);
+
+	if (client->irq) {
+		ret = regmap_set_bits(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
+				      HPD_REMOVAL_EN | HPD_INSERTION_EN);
+		if (ret)
+			dev_err(pdata->dev, "Failed to enable HPD events: %d\n", ret);
+	}
 }
 
 static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
+	int ret;
+
+	if (client->irq) {
+		ret = regmap_clear_bits(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
+					HPD_REMOVAL_EN | HPD_INSERTION_EN);
+		if (ret)
+			dev_err(pdata->dev, "Failed to disable HPD events: %d\n", ret);
+	}
+
+	mutex_lock(&pdata->hpd_mutex);
+	pdata->hpd_enabled = false;
+	mutex_unlock(&pdata->hpd_mutex);
 
 	pm_runtime_put_autosuspend(pdata->dev);
 }
@@ -1309,6 +1375,41 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
 	return 0;
 }
 
+static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
+{
+	struct ti_sn65dsi86 *pdata = private;
+	struct drm_device *dev = pdata->bridge.dev;
+	u8 status;
+	int ret;
+	bool hpd_event;
+
+	ret = ti_sn65dsi86_read_u8(pdata, SN_IRQ_STATUS_REG, &status);
+	if (ret) {
+		dev_err(pdata->dev, "Failed to read IRQ status: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
+
+	dev_dbg(pdata->dev, "(SN_IRQ_STATUS_REG = %#x)\n", status);
+	ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
+	if (ret) {
+		dev_err(pdata->dev, "Failed to clear IRQ status: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	if (!status)
+		return IRQ_NONE;
+
+	/* Only send the HPD event if we are bound with a device. */
+	mutex_lock(&pdata->hpd_mutex);
+	if (pdata->hpd_enabled && hpd_event)
+		drm_kms_helper_hotplug_event(dev);
+	mutex_unlock(&pdata->hpd_mutex);
+
+	return IRQ_HANDLED;
+}
+
 static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 			      const struct auxiliary_device_id *id)
 {
@@ -1931,6 +2032,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	dev_set_drvdata(dev, pdata);
 	pdata->dev = dev;
 
+	mutex_init(&pdata->hpd_mutex);
 	mutex_init(&pdata->comms_mutex);
 
 	pdata->regmap = devm_regmap_init_i2c(client,
@@ -1971,6 +2073,16 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	if (strncmp(id_buf, "68ISD   ", ARRAY_SIZE(id_buf)))
 		return dev_err_probe(dev, -EOPNOTSUPP, "unsupported device id\n");
 
+	if (client->irq) {
+		ret = devm_request_threaded_irq(pdata->dev, client->irq, NULL,
+						ti_sn_bridge_interrupt,
+						IRQF_ONESHOT,
+						dev_name(pdata->dev), pdata);
+
+		if (ret)
+			return dev_err_probe(dev, ret, "failed to request interrupt\n");
+	}
+
 	/*
 	 * Break ourselves up into a collection of aux devices. The only real
 	 * motiviation here is to solve the chicken-and-egg problem of probe

Re: [PATCH V6] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[Doug Anderson]

Hi,

On Mon, Sep 15, 2025 at 9:51 AM John Ripple <john.ripple@keysight.com> wrote:
>
> @@ -1309,6 +1375,41 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
>         return 0;
>  }
>
> +static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
> +{
> +       struct ti_sn65dsi86 *pdata = private;
> +       struct drm_device *dev = pdata->bridge.dev;
> +       u8 status;
> +       int ret;
> +       bool hpd_event;
> +
> +       ret = ti_sn65dsi86_read_u8(pdata, SN_IRQ_STATUS_REG, &status);
> +       if (ret) {
> +               dev_err(pdata->dev, "Failed to read IRQ status: %d\n", ret);
> +               return IRQ_NONE;
> +       }
> +
> +       hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
> +
> +       dev_dbg(pdata->dev, "(SN_IRQ_STATUS_REG = %#x)\n", status);
> +       ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
> +       if (ret) {
> +               dev_err(pdata->dev, "Failed to clear IRQ status: %d\n", ret);
> +               return IRQ_NONE;
> +       }
> +
> +       if (!status)
> +               return IRQ_NONE;
> +
> +       /* Only send the HPD event if we are bound with a device. */
> +       mutex_lock(&pdata->hpd_mutex);
> +       if (pdata->hpd_enabled && hpd_event)
> +               drm_kms_helper_hotplug_event(dev);
> +       mutex_unlock(&pdata->hpd_mutex);

The order above wasn't quite what I was suggesting. I was suggesting:

ret = ti_sn65dsi86_read_u8(...);
if (ret) {
 ...
}
dev_dbg(..., status);
if (!status)
  return IRQ_NONE;

ret = regmap_write(..., status);
if (ret) {
 ...
}

/* Only send ... */
hpd_event = status & ...;
mutex_lock(...);
...
mutex_unlock(...);

...but it doesn't really matter. I guess it's a little weird that your
current code still writes status even if it's 0, but it shouldn't
really hurt. There's no need to spin with that change unless you feel
like it.

At this point I'm happy with things. Thanks for putting up with the
review process!

Reviewed-by: Douglas Anderson <dianders@chromium.org>


I'll plan to give this a week or so in case anyone else wants to jump
in, then apply it. I'll also try to find some time this week to test
this on a device using ti-sn65dsi86 to make sure nothing breaks,
though I don't expect it to.

-Doug

[PATCH V7] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[John Ripple]

Add support for DisplayPort to the bridge, which entails the following:
- Get and use an interrupt for HPD;
- Properly clear all status bits in the interrupt handler;

Signed-off-by: John Ripple <john.ripple@keysight.com>
---
V1 -> V2: Cleaned up coding style and addressed review comments
V2 -> V3:
- Removed unused HPD IRQs
- Added mutex around HPD enable/disable and IRQ handler.
- Cleaned up error handling and variable declarations
- Only enable IRQs if the i2c client has an IRQ
- Moved IRQ_EN to ti_sn65dsi86_resume()
- Created ti_sn65dsi86_read_u8() helper function
V3 -> V4:
- Formatting
- Allow device tree to set interrupt type.
- Use hpd_mutex over less code.
V4 -> V5:
- Formatting.
- Symmetric mutex use in hpd enable/disable functions.
- Only set and clear specific bits for IRQ enable and disable.
- Better error handling in interrupt.
V5 -> V6:
- Formatting.
- Convert pr_ to dev_ for printing.
- Add error check to regmap_clear_bits() call.
V7:
- Move status check in interrupt.
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 112 ++++++++++++++++++++++++++
 1 file changed, 112 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index ae0d08e5e960..276d05d25ad8 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -106,10 +106,21 @@
 #define SN_PWM_EN_INV_REG			0xA5
 #define  SN_PWM_INV_MASK			BIT(0)
 #define  SN_PWM_EN_MASK				BIT(1)
+
+#define SN_IRQ_EN_REG				0xE0
+#define  IRQ_EN					BIT(0)
+
+#define SN_IRQ_EVENTS_EN_REG			0xE6
+#define  HPD_INSERTION_EN			BIT(1)
+#define  HPD_REMOVAL_EN				BIT(2)
+
 #define SN_AUX_CMD_STATUS_REG			0xF4
 #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
 #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
 #define  AUX_IRQ_STATUS_NAT_I2C_FAIL		BIT(6)
+#define SN_IRQ_STATUS_REG			0xF5
+#define  HPD_REMOVAL_STATUS			BIT(2)
+#define  HPD_INSERTION_STATUS			BIT(1)
 
 #define MIN_DSI_CLK_FREQ_MHZ	40
 
@@ -152,7 +163,9 @@
  * @ln_assign:    Value to program to the LN_ASSIGN register.
  * @ln_polrs:     Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG.
  * @comms_enabled: If true then communication over the aux channel is enabled.
+ * @hpd_enabled:   If true then HPD events are enabled.
  * @comms_mutex:   Protects modification of comms_enabled.
+ * @hpd_mutex:     Protects modification of hpd_enabled.
  *
  * @gchip:        If we expose our GPIOs, this is used.
  * @gchip_output: A cache of whether we've set GPIOs to output.  This
@@ -190,7 +203,9 @@ struct ti_sn65dsi86 {
 	u8				ln_assign;
 	u8				ln_polrs;
 	bool				comms_enabled;
+	bool				hpd_enabled;
 	struct mutex			comms_mutex;
+	struct mutex			hpd_mutex;
 
 #if defined(CONFIG_OF_GPIO)
 	struct gpio_chip		gchip;
@@ -221,6 +236,23 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = {
 	.max_register = 0xFF,
 };
 
+static int ti_sn65dsi86_read_u8(struct ti_sn65dsi86 *pdata, unsigned int reg,
+				u8 *val)
+{
+	int ret;
+	unsigned int reg_val;
+
+	ret = regmap_read(pdata->regmap, reg, &reg_val);
+	if (ret) {
+		dev_err(pdata->dev, "fail to read raw reg %#x: %d\n",
+			reg, ret);
+		return ret;
+	}
+	*val = (u8)reg_val;
+
+	return 0;
+}
+
 static int __maybe_unused ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
 						unsigned int reg, u16 *val)
 {
@@ -379,6 +411,7 @@ static void ti_sn65dsi86_disable_comms(struct ti_sn65dsi86 *pdata)
 static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 {
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
 	int ret;
 
 	ret = regulator_bulk_enable(SN_REGULATOR_SUPPLY_NUM, pdata->supplies);
@@ -413,6 +446,13 @@ static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 	if (pdata->refclk)
 		ti_sn65dsi86_enable_comms(pdata, NULL);
 
+	if (client->irq) {
+		ret = regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN,
+					 IRQ_EN);
+		if (ret)
+			dev_err(pdata->dev, "Failed to enable IRQ events: %d\n", ret);
+	}
+
 	return ret;
 }
 
@@ -1211,6 +1251,8 @@ static void ti_sn65dsi86_debugfs_init(struct drm_bridge *bridge, struct dentry *
 static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
+	int ret;
 
 	/*
 	 * Device needs to be powered on before reading the HPD state
@@ -1219,11 +1261,35 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 	 */
 
 	pm_runtime_get_sync(pdata->dev);
+
+	mutex_lock(&pdata->hpd_mutex);
+	pdata->hpd_enabled = true;
+	mutex_unlock(&pdata->hpd_mutex);
+
+	if (client->irq) {
+		ret = regmap_set_bits(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
+				      HPD_REMOVAL_EN | HPD_INSERTION_EN);
+		if (ret)
+			dev_err(pdata->dev, "Failed to enable HPD events: %d\n", ret);
+	}
 }
 
 static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
+	int ret;
+
+	if (client->irq) {
+		ret = regmap_clear_bits(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
+					HPD_REMOVAL_EN | HPD_INSERTION_EN);
+		if (ret)
+			dev_err(pdata->dev, "Failed to disable HPD events: %d\n", ret);
+	}
+
+	mutex_lock(&pdata->hpd_mutex);
+	pdata->hpd_enabled = false;
+	mutex_unlock(&pdata->hpd_mutex);
 
 	pm_runtime_put_autosuspend(pdata->dev);
 }
@@ -1309,6 +1375,41 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
 	return 0;
 }
 
+static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
+{
+	struct ti_sn65dsi86 *pdata = private;
+	struct drm_device *dev = pdata->bridge.dev;
+	u8 status;
+	int ret;
+	bool hpd_event;
+
+	ret = ti_sn65dsi86_read_u8(pdata, SN_IRQ_STATUS_REG, &status);
+	if (ret) {
+		dev_err(pdata->dev, "Failed to read IRQ status: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
+
+	dev_dbg(pdata->dev, "(SN_IRQ_STATUS_REG = %#x)\n", status);
+	if (!status)
+		return IRQ_NONE;
+
+	ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
+	if (ret) {
+		dev_err(pdata->dev, "Failed to clear IRQ status: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	/* Only send the HPD event if we are bound with a device. */
+	mutex_lock(&pdata->hpd_mutex);
+	if (pdata->hpd_enabled && hpd_event)
+		drm_kms_helper_hotplug_event(dev);
+	mutex_unlock(&pdata->hpd_mutex);
+
+	return IRQ_HANDLED;
+}
+
 static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 			      const struct auxiliary_device_id *id)
 {
@@ -1931,6 +2032,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	dev_set_drvdata(dev, pdata);
 	pdata->dev = dev;
 
+	mutex_init(&pdata->hpd_mutex);
 	mutex_init(&pdata->comms_mutex);
 
 	pdata->regmap = devm_regmap_init_i2c(client,
@@ -1971,6 +2073,16 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	if (strncmp(id_buf, "68ISD   ", ARRAY_SIZE(id_buf)))
 		return dev_err_probe(dev, -EOPNOTSUPP, "unsupported device id\n");
 
+	if (client->irq) {
+		ret = devm_request_threaded_irq(pdata->dev, client->irq, NULL,
+						ti_sn_bridge_interrupt,
+						IRQF_ONESHOT,
+						dev_name(pdata->dev), pdata);
+
+		if (ret)
+			return dev_err_probe(dev, ret, "failed to request interrupt\n");
+	}
+
 	/*
 	 * Break ourselves up into a collection of aux devices. The only real
 	 * motiviation here is to solve the chicken-and-egg problem of probe

[PATCH V7] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[John Ripple]

Add support for DisplayPort to the bridge, which entails the following:
- Get and use an interrupt for HPD;
- Properly clear all status bits in the interrupt handler;

Signed-off-by: John Ripple <john.ripple@keysight.com>
---
V1 -> V2: Cleaned up coding style and addressed review comments
V2 -> V3:
- Removed unused HPD IRQs
- Added mutex around HPD enable/disable and IRQ handler.
- Cleaned up error handling and variable declarations
- Only enable IRQs if the i2c client has an IRQ
- Moved IRQ_EN to ti_sn65dsi86_resume()
- Created ti_sn65dsi86_read_u8() helper function
V3 -> V4:
- Formatting
- Allow device tree to set interrupt type.
- Use hpd_mutex over less code.
V4 -> V5:
- Formatting.
- Symmetric mutex use in hpd enable/disable functions.
- Only set and clear specific bits for IRQ enable and disable.
- Better error handling in interrupt.
V5 -> V6:
- Formatting.
- Convert pr_ to dev_ for printing.
- Add error check to regmap_clear_bits() call.
V7:
- Move status check in interrupt.
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 112 ++++++++++++++++++++++++++
 1 file changed, 112 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index ae0d08e5e960..276d05d25ad8 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -106,10 +106,21 @@
 #define SN_PWM_EN_INV_REG			0xA5
 #define  SN_PWM_INV_MASK			BIT(0)
 #define  SN_PWM_EN_MASK				BIT(1)
+
+#define SN_IRQ_EN_REG				0xE0
+#define  IRQ_EN					BIT(0)
+
+#define SN_IRQ_EVENTS_EN_REG			0xE6
+#define  HPD_INSERTION_EN			BIT(1)
+#define  HPD_REMOVAL_EN				BIT(2)
+
 #define SN_AUX_CMD_STATUS_REG			0xF4
 #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
 #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
 #define  AUX_IRQ_STATUS_NAT_I2C_FAIL		BIT(6)
+#define SN_IRQ_STATUS_REG			0xF5
+#define  HPD_REMOVAL_STATUS			BIT(2)
+#define  HPD_INSERTION_STATUS			BIT(1)
 
 #define MIN_DSI_CLK_FREQ_MHZ	40
 
@@ -152,7 +163,9 @@
  * @ln_assign:    Value to program to the LN_ASSIGN register.
  * @ln_polrs:     Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG.
  * @comms_enabled: If true then communication over the aux channel is enabled.
+ * @hpd_enabled:   If true then HPD events are enabled.
  * @comms_mutex:   Protects modification of comms_enabled.
+ * @hpd_mutex:     Protects modification of hpd_enabled.
  *
  * @gchip:        If we expose our GPIOs, this is used.
  * @gchip_output: A cache of whether we've set GPIOs to output.  This
@@ -190,7 +203,9 @@ struct ti_sn65dsi86 {
 	u8				ln_assign;
 	u8				ln_polrs;
 	bool				comms_enabled;
+	bool				hpd_enabled;
 	struct mutex			comms_mutex;
+	struct mutex			hpd_mutex;
 
 #if defined(CONFIG_OF_GPIO)
 	struct gpio_chip		gchip;
@@ -221,6 +236,23 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = {
 	.max_register = 0xFF,
 };
 
+static int ti_sn65dsi86_read_u8(struct ti_sn65dsi86 *pdata, unsigned int reg,
+				u8 *val)
+{
+	int ret;
+	unsigned int reg_val;
+
+	ret = regmap_read(pdata->regmap, reg, &reg_val);
+	if (ret) {
+		dev_err(pdata->dev, "fail to read raw reg %#x: %d\n",
+			reg, ret);
+		return ret;
+	}
+	*val = (u8)reg_val;
+
+	return 0;
+}
+
 static int __maybe_unused ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
 						unsigned int reg, u16 *val)
 {
@@ -379,6 +411,7 @@ static void ti_sn65dsi86_disable_comms(struct ti_sn65dsi86 *pdata)
 static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 {
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
 	int ret;
 
 	ret = regulator_bulk_enable(SN_REGULATOR_SUPPLY_NUM, pdata->supplies);
@@ -413,6 +446,13 @@ static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 	if (pdata->refclk)
 		ti_sn65dsi86_enable_comms(pdata, NULL);
 
+	if (client->irq) {
+		ret = regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN,
+					 IRQ_EN);
+		if (ret)
+			dev_err(pdata->dev, "Failed to enable IRQ events: %d\n", ret);
+	}
+
 	return ret;
 }
 
@@ -1211,6 +1251,8 @@ static void ti_sn65dsi86_debugfs_init(struct drm_bridge *bridge, struct dentry *
 static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
+	int ret;
 
 	/*
 	 * Device needs to be powered on before reading the HPD state
@@ -1219,11 +1261,35 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 	 */
 
 	pm_runtime_get_sync(pdata->dev);
+
+	mutex_lock(&pdata->hpd_mutex);
+	pdata->hpd_enabled = true;
+	mutex_unlock(&pdata->hpd_mutex);
+
+	if (client->irq) {
+		ret = regmap_set_bits(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
+				      HPD_REMOVAL_EN | HPD_INSERTION_EN);
+		if (ret)
+			dev_err(pdata->dev, "Failed to enable HPD events: %d\n", ret);
+	}
 }
 
 static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	const struct i2c_client *client = to_i2c_client(pdata->dev);
+	int ret;
+
+	if (client->irq) {
+		ret = regmap_clear_bits(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
+					HPD_REMOVAL_EN | HPD_INSERTION_EN);
+		if (ret)
+			dev_err(pdata->dev, "Failed to disable HPD events: %d\n", ret);
+	}
+
+	mutex_lock(&pdata->hpd_mutex);
+	pdata->hpd_enabled = false;
+	mutex_unlock(&pdata->hpd_mutex);
 
 	pm_runtime_put_autosuspend(pdata->dev);
 }
@@ -1309,6 +1375,41 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
 	return 0;
 }
 
+static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
+{
+	struct ti_sn65dsi86 *pdata = private;
+	struct drm_device *dev = pdata->bridge.dev;
+	u8 status;
+	int ret;
+	bool hpd_event;
+
+	ret = ti_sn65dsi86_read_u8(pdata, SN_IRQ_STATUS_REG, &status);
+	if (ret) {
+		dev_err(pdata->dev, "Failed to read IRQ status: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
+
+	dev_dbg(pdata->dev, "(SN_IRQ_STATUS_REG = %#x)\n", status);
+	if (!status)
+		return IRQ_NONE;
+
+	ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
+	if (ret) {
+		dev_err(pdata->dev, "Failed to clear IRQ status: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	/* Only send the HPD event if we are bound with a device. */
+	mutex_lock(&pdata->hpd_mutex);
+	if (pdata->hpd_enabled && hpd_event)
+		drm_kms_helper_hotplug_event(dev);
+	mutex_unlock(&pdata->hpd_mutex);
+
+	return IRQ_HANDLED;
+}
+
 static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 			      const struct auxiliary_device_id *id)
 {
@@ -1931,6 +2032,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	dev_set_drvdata(dev, pdata);
 	pdata->dev = dev;
 
+	mutex_init(&pdata->hpd_mutex);
 	mutex_init(&pdata->comms_mutex);
 
 	pdata->regmap = devm_regmap_init_i2c(client,
@@ -1971,6 +2073,16 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	if (strncmp(id_buf, "68ISD   ", ARRAY_SIZE(id_buf)))
 		return dev_err_probe(dev, -EOPNOTSUPP, "unsupported device id\n");
 
+	if (client->irq) {
+		ret = devm_request_threaded_irq(pdata->dev, client->irq, NULL,
+						ti_sn_bridge_interrupt,
+						IRQF_ONESHOT,
+						dev_name(pdata->dev), pdata);
+
+		if (ret)
+			return dev_err_probe(dev, ret, "failed to request interrupt\n");
+	}
+
 	/*
 	 * Break ourselves up into a collection of aux devices. The only real
 	 * motiviation here is to solve the chicken-and-egg problem of probe

Re: [PATCH V3] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[John Ripple]

Hi,

>...but it doesn't really matter. I guess it's a little weird that your
>current code still writes status even if it's 0, but it shouldn't
>really hurt. There's no need to spin with that change unless you feel
>like it.

I'm already working on it so I might as well.

Thanks for all the feedback its been very helpful!

-John

Re: [PATCH V7] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[Doug Anderson]

Hi,

On Mon, Sep 15, 2025 at 10:46 AM John Ripple <john.ripple@keysight.com> wrote:
>
> Add support for DisplayPort to the bridge, which entails the following:
> - Get and use an interrupt for HPD;
> - Properly clear all status bits in the interrupt handler;
>
> Signed-off-by: John Ripple <john.ripple@keysight.com>
> ---
> V1 -> V2: Cleaned up coding style and addressed review comments
> V2 -> V3:
> - Removed unused HPD IRQs
> - Added mutex around HPD enable/disable and IRQ handler.
> - Cleaned up error handling and variable declarations
> - Only enable IRQs if the i2c client has an IRQ
> - Moved IRQ_EN to ti_sn65dsi86_resume()
> - Created ti_sn65dsi86_read_u8() helper function
> V3 -> V4:
> - Formatting
> - Allow device tree to set interrupt type.
> - Use hpd_mutex over less code.
> V4 -> V5:
> - Formatting.
> - Symmetric mutex use in hpd enable/disable functions.
> - Only set and clear specific bits for IRQ enable and disable.
> - Better error handling in interrupt.
> V5 -> V6:
> - Formatting.
> - Convert pr_ to dev_ for printing.
> - Add error check to regmap_clear_bits() call.
> V7:
> - Move status check in interrupt.
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 112 ++++++++++++++++++++++++++
>  1 file changed, 112 insertions(+)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

I'll plan to apply to drm-misc-next next week unless anything else comes up.

-Doug

Re: [PATCH V3] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20250910183353.2045339-1-john.ripple@keysight.com>
References: <20250910183353.2045339-1-john.ripple@keysight.com>
TO: John Ripple <john.ripple@keysight.com>
TO: john.ripple@keysight.com
CC: Laurent.pinchart@ideasonboard.com
CC: airlied@gmail.com
CC: andrzej.hajda@intel.com
CC: blake.vermeer@keysight.com
CC: dianders@chromium.org
CC: dri-devel@lists.freedesktop.org
CC: jernej.skrabec@gmail.com
CC: jonas@kwiboo.se
CC: linux-kernel@vger.kernel.org
CC: maarten.lankhorst@linux.intel.com
CC: matt_laubhan@keysight.com
CC: mripard@kernel.org
CC: neil.armstrong@linaro.org
CC: rfoss@kernel.org
CC: simona@ffwll.ch
CC: tzimmermann@suse.de

Hi John,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.17-rc6 next-20250915]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/UPDATE-20250911-023707/John-Ripple/drm-bridge-ti-sn65dsi86-break-probe-dependency-loop/20250820-235209
base:   linus/master
patch link:    https://lore.kernel.org/r/20250910183353.2045339-1-john.ripple%40keysight.com
patch subject: [PATCH V3] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD
:::::: branch date: 5 days ago
:::::: commit date: 5 days ago
config: x86_64-randconfig-161-20250916 (https://download.01.org/0day-ci/archive/20250916/202509161344.FPfsjq01-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202509161344.FPfsjq01-lkp@intel.com/

smatch warnings:
drivers/gpu/drm/bridge/ti-sn65dsi86.c:1385 ti_sn_bridge_interrupt() error: uninitialized symbol 'status'.

vim +/status +1385 drivers/gpu/drm/bridge/ti-sn65dsi86.c

982f589bde7a7f Stephen Boyd 2020-11-02  1364  
b8670cf7e6a41b John Ripple  2025-09-10  1365  static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
b8670cf7e6a41b John Ripple  2025-09-10  1366  {
b8670cf7e6a41b John Ripple  2025-09-10  1367  	struct ti_sn65dsi86 *pdata = private;
b8670cf7e6a41b John Ripple  2025-09-10  1368  	struct drm_device *dev = pdata->bridge.dev;
b8670cf7e6a41b John Ripple  2025-09-10  1369  	u8 status;
b8670cf7e6a41b John Ripple  2025-09-10  1370  	int ret;
b8670cf7e6a41b John Ripple  2025-09-10  1371  	bool hpd_event = false;
b8670cf7e6a41b John Ripple  2025-09-10  1372  
b8670cf7e6a41b John Ripple  2025-09-10  1373  	mutex_lock(&pdata->hpd_mutex);
b8670cf7e6a41b John Ripple  2025-09-10  1374  	if (!pdata->hpd_enabled) {
b8670cf7e6a41b John Ripple  2025-09-10  1375  		mutex_unlock(&pdata->hpd_mutex);
b8670cf7e6a41b John Ripple  2025-09-10  1376  		return IRQ_HANDLED;
b8670cf7e6a41b John Ripple  2025-09-10  1377  	}
b8670cf7e6a41b John Ripple  2025-09-10  1378  
b8670cf7e6a41b John Ripple  2025-09-10  1379  	ret = ti_sn65dsi86_read_u8(pdata, SN_IRQ_STATUS_REG, &status);
b8670cf7e6a41b John Ripple  2025-09-10  1380  	if (ret)
b8670cf7e6a41b John Ripple  2025-09-10  1381  		pr_err("Failed to read IRQ status: %d\n", ret);
b8670cf7e6a41b John Ripple  2025-09-10  1382  	else
b8670cf7e6a41b John Ripple  2025-09-10  1383  		hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
b8670cf7e6a41b John Ripple  2025-09-10  1384  
b8670cf7e6a41b John Ripple  2025-09-10 @1385  	if (status) {
b8670cf7e6a41b John Ripple  2025-09-10  1386  		drm_dbg(dev, "(SN_IRQ_STATUS_REG = %#x)\n", status);
b8670cf7e6a41b John Ripple  2025-09-10  1387  		ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
b8670cf7e6a41b John Ripple  2025-09-10  1388  		if (ret)
b8670cf7e6a41b John Ripple  2025-09-10  1389  			pr_err("Failed to clear IRQ status: %d\n", ret);
b8670cf7e6a41b John Ripple  2025-09-10  1390  	} else {
b8670cf7e6a41b John Ripple  2025-09-10  1391  		mutex_unlock(&pdata->hpd_mutex);
b8670cf7e6a41b John Ripple  2025-09-10  1392  		return IRQ_NONE;
b8670cf7e6a41b John Ripple  2025-09-10  1393  	}
b8670cf7e6a41b John Ripple  2025-09-10  1394  
b8670cf7e6a41b John Ripple  2025-09-10  1395  	/* Only send the HPD event if we are bound with a device. */
b8670cf7e6a41b John Ripple  2025-09-10  1396  	if (dev && hpd_event)
b8670cf7e6a41b John Ripple  2025-09-10  1397  		drm_kms_helper_hotplug_event(dev);
b8670cf7e6a41b John Ripple  2025-09-10  1398  	mutex_unlock(&pdata->hpd_mutex);
b8670cf7e6a41b John Ripple  2025-09-10  1399  
b8670cf7e6a41b John Ripple  2025-09-10  1400  	return IRQ_HANDLED;
b8670cf7e6a41b John Ripple  2025-09-10  1401  }
b8670cf7e6a41b John Ripple  2025-09-10  1402  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH V3] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[Dan Carpenter]

Hi John,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/UPDATE-20250911-023707/John-Ripple/drm-bridge-ti-sn65dsi86-break-probe-dependency-loop/20250820-235209
base:   linus/master
patch link:    https://lore.kernel.org/r/20250910183353.2045339-1-john.ripple%40keysight.com
patch subject: [PATCH V3] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD
config: x86_64-randconfig-161-20250916 (https://download.01.org/0day-ci/archive/20250916/202509161344.FPfsjq01-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202509161344.FPfsjq01-lkp@intel.com/

smatch warnings:
drivers/gpu/drm/bridge/ti-sn65dsi86.c:1385 ti_sn_bridge_interrupt() error: uninitialized symbol 'status'.

vim +/status +1385 drivers/gpu/drm/bridge/ti-sn65dsi86.c

b8670cf7e6a41b John Ripple  2025-09-10  1365  static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
b8670cf7e6a41b John Ripple  2025-09-10  1366  {
b8670cf7e6a41b John Ripple  2025-09-10  1367  	struct ti_sn65dsi86 *pdata = private;
b8670cf7e6a41b John Ripple  2025-09-10  1368  	struct drm_device *dev = pdata->bridge.dev;
b8670cf7e6a41b John Ripple  2025-09-10  1369  	u8 status;
b8670cf7e6a41b John Ripple  2025-09-10  1370  	int ret;
b8670cf7e6a41b John Ripple  2025-09-10  1371  	bool hpd_event = false;
b8670cf7e6a41b John Ripple  2025-09-10  1372  
b8670cf7e6a41b John Ripple  2025-09-10  1373  	mutex_lock(&pdata->hpd_mutex);
b8670cf7e6a41b John Ripple  2025-09-10  1374  	if (!pdata->hpd_enabled) {
b8670cf7e6a41b John Ripple  2025-09-10  1375  		mutex_unlock(&pdata->hpd_mutex);
b8670cf7e6a41b John Ripple  2025-09-10  1376  		return IRQ_HANDLED;
b8670cf7e6a41b John Ripple  2025-09-10  1377  	}
b8670cf7e6a41b John Ripple  2025-09-10  1378  
b8670cf7e6a41b John Ripple  2025-09-10  1379  	ret = ti_sn65dsi86_read_u8(pdata, SN_IRQ_STATUS_REG, &status);
b8670cf7e6a41b John Ripple  2025-09-10  1380  	if (ret)
b8670cf7e6a41b John Ripple  2025-09-10  1381  		pr_err("Failed to read IRQ status: %d\n", ret);

status isn't initialized on error.

b8670cf7e6a41b John Ripple  2025-09-10  1382  	else
b8670cf7e6a41b John Ripple  2025-09-10  1383  		hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
b8670cf7e6a41b John Ripple  2025-09-10  1384  
b8670cf7e6a41b John Ripple  2025-09-10 @1385  	if (status) {
                                                    ^^^^^^
warning

b8670cf7e6a41b John Ripple  2025-09-10  1386  		drm_dbg(dev, "(SN_IRQ_STATUS_REG = %#x)\n", status);
b8670cf7e6a41b John Ripple  2025-09-10  1387  		ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
b8670cf7e6a41b John Ripple  2025-09-10  1388  		if (ret)
b8670cf7e6a41b John Ripple  2025-09-10  1389  			pr_err("Failed to clear IRQ status: %d\n", ret);
b8670cf7e6a41b John Ripple  2025-09-10  1390  	} else {
b8670cf7e6a41b John Ripple  2025-09-10  1391  		mutex_unlock(&pdata->hpd_mutex);
b8670cf7e6a41b John Ripple  2025-09-10  1392  		return IRQ_NONE;
b8670cf7e6a41b John Ripple  2025-09-10  1393  	}
b8670cf7e6a41b John Ripple  2025-09-10  1394  
b8670cf7e6a41b John Ripple  2025-09-10  1395  	/* Only send the HPD event if we are bound with a device. */
b8670cf7e6a41b John Ripple  2025-09-10  1396  	if (dev && hpd_event)
b8670cf7e6a41b John Ripple  2025-09-10  1397  		drm_kms_helper_hotplug_event(dev);
b8670cf7e6a41b John Ripple  2025-09-10  1398  	mutex_unlock(&pdata->hpd_mutex);
b8670cf7e6a41b John Ripple  2025-09-10  1399  
b8670cf7e6a41b John Ripple  2025-09-10  1400  	return IRQ_HANDLED;
b8670cf7e6a41b John Ripple  2025-09-10  1401  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH V3] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[Doug Anderson]

Hi,

On Mon, Sep 15, 2025 at 10:46 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> Hi John,
>
> kernel test robot noticed the following build warnings:
>
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/UPDATE-20250911-023707/John-Ripple/drm-bridge-ti-sn65dsi86-break-probe-dependency-loop/20250820-235209
> base:   linus/master
> patch link:    https://lore.kernel.org/r/20250910183353.2045339-1-john.ripple%40keysight.com
> patch subject: [PATCH V3] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD
> config: x86_64-randconfig-161-20250916 (https://download.01.org/0day-ci/archive/20250916/202509161344.FPfsjq01-lkp@intel.com/config)
> compiler: gcc-14 (Debian 14.2.0-19) 14.2.0
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202509161344.FPfsjq01-lkp@intel.com/
>
> smatch warnings:
> drivers/gpu/drm/bridge/ti-sn65dsi86.c:1385 ti_sn_bridge_interrupt() error: uninitialized symbol 'status'.
>
> vim +/status +1385 drivers/gpu/drm/bridge/ti-sn65dsi86.c
>
> b8670cf7e6a41b John Ripple  2025-09-10  1365  static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
> b8670cf7e6a41b John Ripple  2025-09-10  1366  {
> b8670cf7e6a41b John Ripple  2025-09-10  1367    struct ti_sn65dsi86 *pdata = private;
> b8670cf7e6a41b John Ripple  2025-09-10  1368    struct drm_device *dev = pdata->bridge.dev;
> b8670cf7e6a41b John Ripple  2025-09-10  1369    u8 status;
> b8670cf7e6a41b John Ripple  2025-09-10  1370    int ret;
> b8670cf7e6a41b John Ripple  2025-09-10  1371    bool hpd_event = false;
> b8670cf7e6a41b John Ripple  2025-09-10  1372
> b8670cf7e6a41b John Ripple  2025-09-10  1373    mutex_lock(&pdata->hpd_mutex);
> b8670cf7e6a41b John Ripple  2025-09-10  1374    if (!pdata->hpd_enabled) {
> b8670cf7e6a41b John Ripple  2025-09-10  1375            mutex_unlock(&pdata->hpd_mutex);
> b8670cf7e6a41b John Ripple  2025-09-10  1376            return IRQ_HANDLED;
> b8670cf7e6a41b John Ripple  2025-09-10  1377    }
> b8670cf7e6a41b John Ripple  2025-09-10  1378
> b8670cf7e6a41b John Ripple  2025-09-10  1379    ret = ti_sn65dsi86_read_u8(pdata, SN_IRQ_STATUS_REG, &status);
> b8670cf7e6a41b John Ripple  2025-09-10  1380    if (ret)
> b8670cf7e6a41b John Ripple  2025-09-10  1381            pr_err("Failed to read IRQ status: %d\n", ret);
>
> status isn't initialized on error.
>
> b8670cf7e6a41b John Ripple  2025-09-10  1382    else
> b8670cf7e6a41b John Ripple  2025-09-10  1383            hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
> b8670cf7e6a41b John Ripple  2025-09-10  1384
> b8670cf7e6a41b John Ripple  2025-09-10 @1385    if (status) {
>                                                     ^^^^^^
> warning

It looks like the bot reported this on an old version. The newest is
v7 [1] and things should be OK there. Yell if I missed something. :-)

[1] https:/lore.kernel.org/r/20250915174543.2564994-1-john.ripple@keysight.com

Re: [PATCH V7] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[Doug Anderson]

Hi,

On Mon, Sep 15, 2025 at 10:57 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Sep 15, 2025 at 10:46 AM John Ripple <john.ripple@keysight.com> wrote:
> >
> > Add support for DisplayPort to the bridge, which entails the following:
> > - Get and use an interrupt for HPD;
> > - Properly clear all status bits in the interrupt handler;
> >
> > Signed-off-by: John Ripple <john.ripple@keysight.com>
> > ---
> > V1 -> V2: Cleaned up coding style and addressed review comments
> > V2 -> V3:
> > - Removed unused HPD IRQs
> > - Added mutex around HPD enable/disable and IRQ handler.
> > - Cleaned up error handling and variable declarations
> > - Only enable IRQs if the i2c client has an IRQ
> > - Moved IRQ_EN to ti_sn65dsi86_resume()
> > - Created ti_sn65dsi86_read_u8() helper function
> > V3 -> V4:
> > - Formatting
> > - Allow device tree to set interrupt type.
> > - Use hpd_mutex over less code.
> > V4 -> V5:
> > - Formatting.
> > - Symmetric mutex use in hpd enable/disable functions.
> > - Only set and clear specific bits for IRQ enable and disable.
> > - Better error handling in interrupt.
> > V5 -> V6:
> > - Formatting.
> > - Convert pr_ to dev_ for printing.
> > - Add error check to regmap_clear_bits() call.
> > V7:
> > - Move status check in interrupt.
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 112 ++++++++++++++++++++++++++
> >  1 file changed, 112 insertions(+)
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
> I'll plan to apply to drm-misc-next next week unless anything else comes up.

Pushed to drm-misc-next:

[1/1] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD
      commit: 9133bc3f0564890218cbba6cc7e81ebc0841a6f1