drivers/gpu/drm/bridge/ti-sn65dsi86.c | 112 ++++++++++++++++++++++++++ 1 file changed, 112 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
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 - 2025 Red Hat, Inc.