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,
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
© 2016 - 2025 Red Hat, Inc.