drivers/gpu/drm/bridge/ti-sn65dsi86.c | 126 ++++++++++++++++++++++++++ 1 file changed, 126 insertions(+)
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, ®_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 probeHi 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
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
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
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.
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
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.
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, ®_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 probeHi,
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 = ...
...
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, ®_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 probeHi,
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
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, ®_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 probeHi,
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
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
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, ®_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 probeHi, 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
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
© 2016 - 2026 Red Hat, Inc.