drivers/gpu/drm/bridge/ti-sn65dsi86.c | 93 +++++++++++++++++++++++++++ 1 file changed, 93 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
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 93 +++++++++++++++++++++++++++
1 file changed, 93 insertions(+)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index ae0d08e5e960..ad0393212279 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -106,10 +106,24 @@
#define SN_PWM_EN_INV_REG 0xA5
#define SN_PWM_INV_MASK BIT(0)
#define SN_PWM_EN_MASK BIT(1)
+
+#define SN_IRQ_EN_REG 0xE0
+#define IRQ_EN BIT(0)
+
+#define SN_IRQ_EVENTS_EN_REG 0xE6
+#define IRQ_HPD_EN BIT(0)
+#define HPD_INSERTION_EN BIT(1)
+#define HPD_REMOVAL_EN BIT(2)
+#define HPD_REPLUG_EN BIT(3)
+#define PLL_UNLOCK_EN BIT(5)
+
#define SN_AUX_CMD_STATUS_REG 0xF4
#define AUX_IRQ_STATUS_AUX_RPLY_TOUT BIT(3)
#define AUX_IRQ_STATUS_AUX_SHORT BIT(5)
#define AUX_IRQ_STATUS_NAT_I2C_FAIL BIT(6)
+#define SN_IRQ_STATUS_REG 0xF5
+#define HPD_REMOVAL_STATUS BIT(2)
+#define HPD_INSERTION_STATUS BIT(1)
#define MIN_DSI_CLK_FREQ_MHZ 40
@@ -221,6 +235,19 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = {
.max_register = 0xFF,
};
+static int ti_sn65dsi86_read(struct ti_sn65dsi86 *pdata, unsigned int reg,
+ unsigned int *val)
+{
+ int ret;
+
+ ret = regmap_read(pdata->regmap, reg, val);
+ if (ret)
+ dev_err(pdata->dev, "fail to read raw reg %x: %d\n",
+ reg, ret);
+
+ return ret;
+}
+
static int __maybe_unused ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
unsigned int reg, u16 *val)
{
@@ -1219,12 +1246,28 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
*/
pm_runtime_get_sync(pdata->dev);
+
+ /* Enable HPD and PLL events. */
+ regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
+ PLL_UNLOCK_EN |
+ HPD_REPLUG_EN |
+ HPD_REMOVAL_EN |
+ HPD_INSERTION_EN |
+ IRQ_HPD_EN);
+
+ regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN,
+ IRQ_EN);
}
static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
{
struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+ /* Disable HPD and PLL events. */
+ regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG, 0);
+
+ regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN, 0);
+
pm_runtime_put_autosuspend(pdata->dev);
}
@@ -1309,6 +1352,32 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
return 0;
}
+static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
+{
+ struct ti_sn65dsi86 *pdata = private;
+ struct drm_device *dev = pdata->bridge.dev;
+ u32 status = 0;
+ bool hpd_event = false;
+ int ret = 0;
+
+ ret = ti_sn65dsi86_read(pdata, SN_IRQ_STATUS_REG, &status);
+ if (ret)
+ return ret;
+ hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
+ if (status) {
+ drm_dbg(dev, "(SN_IRQ_STATUS_REG = %#x)\n", status);
+ ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
+ if (ret)
+ return ret;
+ }
+
+ /* Only send the HPD event if we are bound with a device. */
+ if (dev && hpd_event)
+ drm_kms_helper_hotplug_event(dev);
+
+ return IRQ_HANDLED;
+}
+
static int ti_sn_bridge_probe(struct auxiliary_device *adev,
const struct auxiliary_device_id *id)
{
@@ -1971,6 +2040,30 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
if (strncmp(id_buf, "68ISD ", ARRAY_SIZE(id_buf)))
return dev_err_probe(dev, -EOPNOTSUPP, "unsupported device id\n");
+ if (client->irq) {
+ enum drm_connector_status status;
+
+ ret = devm_request_threaded_irq(pdata->dev, client->irq, NULL,
+ ti_sn_bridge_interrupt,
+ IRQF_TRIGGER_RISING |
+ IRQF_TRIGGER_FALLING |
+ IRQF_ONESHOT,
+ "ti_sn65dsi86", pdata);
+ if (ret) {
+ return dev_err_probe(dev, ret,
+ "failed to request interrupt\n");
+ }
+
+ /*
+ * Cleaning status register at probe is needed because if the irq is
+ * already high, the rising/falling condition will never occurs
+ */
+ ret = ti_sn65dsi86_read(pdata, SN_IRQ_STATUS_REG, &status);
+ ret |= regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
+ if (ret)
+ pr_warn("Failed to clear IRQ initial state: %d\n", ret);
+ }
+
/*
* Break ourselves up into a collection of aux devices. The only real
* motiviation here is to solve the chicken-and-egg problem of probeHi,
On Mon, Sep 8, 2025 at 1:37 PM John Ripple <john.ripple@keysight.com> wrote:
>
> Add support for DisplayPort to the bridge, which entails the following:
> - Get and use an interrupt for HPD;
> - Properly clear all status bits in the interrupt handler;
>
> Signed-off-by: John Ripple <john.ripple@keysight.com>
> ---
> V1 -> V2: Cleaned up coding style and addressed review comments
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 93 +++++++++++++++++++++++++++
> 1 file changed, 93 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index ae0d08e5e960..ad0393212279 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -106,10 +106,24 @@
> #define SN_PWM_EN_INV_REG 0xA5
> #define SN_PWM_INV_MASK BIT(0)
> #define SN_PWM_EN_MASK BIT(1)
> +
> +#define SN_IRQ_EN_REG 0xE0
> +#define IRQ_EN BIT(0)
> +
> +#define SN_IRQ_EVENTS_EN_REG 0xE6
> +#define IRQ_HPD_EN BIT(0)
> +#define HPD_INSERTION_EN BIT(1)
> +#define HPD_REMOVAL_EN BIT(2)
> +#define HPD_REPLUG_EN BIT(3)
> +#define PLL_UNLOCK_EN BIT(5)
> +
> #define SN_AUX_CMD_STATUS_REG 0xF4
> #define AUX_IRQ_STATUS_AUX_RPLY_TOUT BIT(3)
> #define AUX_IRQ_STATUS_AUX_SHORT BIT(5)
> #define AUX_IRQ_STATUS_NAT_I2C_FAIL BIT(6)
> +#define SN_IRQ_STATUS_REG 0xF5
> +#define HPD_REMOVAL_STATUS BIT(2)
> +#define HPD_INSERTION_STATUS BIT(1)
>
> #define MIN_DSI_CLK_FREQ_MHZ 40
>
> @@ -221,6 +235,19 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = {
> .max_register = 0xFF,
> };
>
> +static int ti_sn65dsi86_read(struct ti_sn65dsi86 *pdata, unsigned int reg,
> + unsigned int *val)
This is reading a byte, right? So "val" should be an "u8 *". Yeah,
that means you need a local variable to adjust for the generic regmap
call, but it makes a cleaner and more obvious API to the users in this
file.
> +{
> + int ret;
> +
> + ret = regmap_read(pdata->regmap, reg, val);
> + if (ret)
> + dev_err(pdata->dev, "fail to read raw reg %x: %d\n",
> + reg, ret);
nit: use %#x so that the 0x is included.
> @@ -1219,12 +1246,28 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
> */
>
> pm_runtime_get_sync(pdata->dev);
> +
> + /* Enable HPD and PLL events. */
> + regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
> + PLL_UNLOCK_EN |
> + HPD_REPLUG_EN |
> + HPD_REMOVAL_EN |
> + HPD_INSERTION_EN |
> + IRQ_HPD_EN);
* Shouldn't this be `regmap_update_bits()` to just update the bits
related to HPD?
* why enable "PLL_UNLOCK_EN" when you don't handle it?
* I also don't think your IRQ handler handles "replug" and "irq_hpd",
right? So you shouldn't enable those either?
Also: should the above only be if the IRQ is enabled? AKA obtain a
pointer to the i2c_client and check `client->irq`?
> +
> + regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN,
> + IRQ_EN);
I guess this is OK to be here if you want, but I would maybe consider
putting it in `ti_sn65dsi86_resume()` if `client->irq` is set. Then if
we ever enable more interrupts it'll already be in the correct place.
> @@ -1309,6 +1352,32 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
> return 0;
> }
>
> +static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
> +{
> + struct ti_sn65dsi86 *pdata = private;
> + struct drm_device *dev = pdata->bridge.dev;
I'm unsure if accessing "dev" here without any sort of locking is
safe... It feels like, in theory, "detach" could be called and race
with the IRQ handler? Maybe you need a spinlock to be sure?
> + u32 status = 0;
> + bool hpd_event = false;
> + int ret = 0;
Don't initialize variables unless they're needed. In this case it's
obvious that both `hpd_event` and `ret` don't need to be initialized.
Maybe you could argue that `status` should be initialized since it's
passed by reference, but even that's iffy...
> +
> + ret = ti_sn65dsi86_read(pdata, SN_IRQ_STATUS_REG, &status);
> + if (ret)
> + return ret;
The return for this function is not an error code but an
`irqreturn_t`. You need to account for that.
> + hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
> + if (status) {
> + drm_dbg(dev, "(SN_IRQ_STATUS_REG = %#x)\n", status);
> + ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
> + if (ret)
> + return ret;
Same--you can't just return an error code.
> + }
> +
> + /* Only send the HPD event if we are bound with a device. */
> + if (dev && hpd_event)
> + drm_kms_helper_hotplug_event(dev);
> +
> + return IRQ_HANDLED;
If "status" gives back 0 (which would be weird), you probably want to
return IRQ_NONE, right?
> +}
> +
> static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> const struct auxiliary_device_id *id)
> {
> @@ -1971,6 +2040,30 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
> if (strncmp(id_buf, "68ISD ", ARRAY_SIZE(id_buf)))
> return dev_err_probe(dev, -EOPNOTSUPP, "unsupported device id\n");
>
> + if (client->irq) {
> + enum drm_connector_status status;
> +
> + ret = devm_request_threaded_irq(pdata->dev, client->irq, NULL,
> + ti_sn_bridge_interrupt,
> + IRQF_TRIGGER_RISING |
> + IRQF_TRIGGER_FALLING |
> + IRQF_ONESHOT,
> + "ti_sn65dsi86", pdata);
> + if (ret) {
> + return dev_err_probe(dev, ret,
> + "failed to request interrupt\n");
> + }
> +
> + /*
> + * Cleaning status register at probe is needed because if the irq is
> + * already high, the rising/falling condition will never occurs
nit: s/occurs/occur
> + */
> + ret = ti_sn65dsi86_read(pdata, SN_IRQ_STATUS_REG, &status);
> + ret |= regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
> + if (ret)
> + pr_warn("Failed to clear IRQ initial state: %d\n", ret);
Do you even need to read? Can't you just write all possible bits and
that should be safe?
Hi,
>> +static int ti_sn65dsi86_read(struct ti_sn65dsi86 *pdata, unsigned int reg,
>> + unsigned int *val)
>
>This is reading a byte, right? So "val" should be an "u8 *". Yeah,
>that means you need a local variable to adjust for the generic regmap
>call, but it makes a cleaner and more obvious API to the users in this
>file.
The regmap_read function takes in an "unsigned int *" as the "val"
parameter and I'm using it to return u32 values (which could probably
be u8 instead). Would it be better to leave this as the more generic
int type or change it to u8 so its more specific to this driver?
If this function gets used elsewhere in this file at some point, I'm
not sure everything that could be read would be single bytes.
>> @@ -1219,12 +1246,28 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
>> */
>>
>> pm_runtime_get_sync(pdata->dev);
>> +
>> + /* Enable HPD and PLL events. */
>> + regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
>> + PLL_UNLOCK_EN |
>> + HPD_REPLUG_EN |
>> + HPD_REMOVAL_EN |
>> + HPD_INSERTION_EN |
>> + IRQ_HPD_EN);
>
>* Shouldn't this be `regmap_update_bits()` to just update the bits
>related to HPD?
>
>* why enable "PLL_UNLOCK_EN" when you don't handle it?
>
>* I also don't think your IRQ handler handles "replug" and "irq_hpd",
>right? So you shouldn't enable those either?
The IRQ_HPD_EN documentation said:
"When IRQ_EN and IRQ_HPD_EN is enabled, the DSIx6 will assert the
IRQ whenever the eDP generates a IRQ_HPD event. An IRQ_HPD event
is defined as a change from INSERTION state to the IRQ_HPD state."
I thought that meant the IRQ_HPD_EN needed to be enabled to get any irqs,
but when I tried removing the IRQ_HPD_EN and it doesn't seem to change
anything, so I'm not sure what the documentation is trying to say.
>> @@ -1309,6 +1352,32 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
>> return 0;
>> }
>>
>> +static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
>> +{
>> + struct ti_sn65dsi86 *pdata = private;
>> + struct drm_device *dev = pdata->bridge.dev;
>
>I'm unsure if accessing "dev" here without any sort of locking is
>safe... It feels like, in theory, "detach" could be called and race
>with the IRQ handler? Maybe you need a spinlock to be sure?
I tested a spinlock added to the ti-sn65dsi86 structure that gets used
in the ti_sn_bridge_detach and ti_sn_bridge_interrupt functions and it
seems to work. Is there another spinlock created somewhere that I could
use instead? Is using the spin lock in the interrupt and detach functions
the correct way to do it?
Hi,
On Tue, Sep 9, 2025 at 12:36 PM John Ripple <john.ripple@keysight.com> wrote:
>
> Hi,
>
> >> +static int ti_sn65dsi86_read(struct ti_sn65dsi86 *pdata, unsigned int reg,
> >> + unsigned int *val)
> >
> >This is reading a byte, right? So "val" should be an "u8 *". Yeah,
> >that means you need a local variable to adjust for the generic regmap
> >call, but it makes a cleaner and more obvious API to the users in this
> >file.
>
> The regmap_read function takes in an "unsigned int *" as the "val"
> parameter and I'm using it to return u32 values (which could probably
> be u8 instead). Would it be better to leave this as the more generic
> int type or change it to u8 so its more specific to this driver?
> If this function gets used elsewhere in this file at some point, I'm
> not sure everything that could be read would be single bytes.
Sure, the "regmap_read" takes "unsigned int *" because it's a generic
API. ...but we initialize the regmap API with:
.reg_bits = 8,
.val_bits = 8,
In other words, each read/write is 8-byte AKA 1 byte. So you're not
returning 32-bit values, but 8-bit values.
There's already a 16-bit version of this function:
ti_sn65dsi86_read_u16(). Reading that function and yours next to each
other makes it seem (at first glance) like yours is returning 32-bits.
It's not. It would be much more documenting showing that it returns
8-bits. If we need a 32-bit version for some reason we'll have to
actually write that up.
> >> @@ -1219,12 +1246,28 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
> >> */
> >>
> >> pm_runtime_get_sync(pdata->dev);
> >> +
> >> + /* Enable HPD and PLL events. */
> >> + regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
> >> + PLL_UNLOCK_EN |
> >> + HPD_REPLUG_EN |
> >> + HPD_REMOVAL_EN |
> >> + HPD_INSERTION_EN |
> >> + IRQ_HPD_EN);
> >
> >* Shouldn't this be `regmap_update_bits()` to just update the bits
> >related to HPD?
> >
> >* why enable "PLL_UNLOCK_EN" when you don't handle it?
> >
> >* I also don't think your IRQ handler handles "replug" and "irq_hpd",
> >right? So you shouldn't enable those either?
>
> The IRQ_HPD_EN documentation said:
> "When IRQ_EN and IRQ_HPD_EN is enabled, the DSIx6 will assert the
> IRQ whenever the eDP generates a IRQ_HPD event. An IRQ_HPD event
> is defined as a change from INSERTION state to the IRQ_HPD state."
>
> I thought that meant the IRQ_HPD_EN needed to be enabled to get any irqs,
> but when I tried removing the IRQ_HPD_EN and it doesn't seem to change
> anything, so I'm not sure what the documentation is trying to say.
IRQ_HPD is defined in the spec. It's basically an "attention"
interrupt from the panel to ti-sn65dsi86. It (and replug) are a
temporary deassertion of HPD while a display is connected.
See "Figure 17. HPD State Diagram" for a description of all these
things. Note that the min/max values there are (I think) because
sn65dsi86's HPD timings are implemented by a very inaccurate ring
oscillator.
If you see that "replug" or "irq_hpd" are needed then your interrupt
handler should do something with them.
> >> @@ -1309,6 +1352,32 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
> >> return 0;
> >> }
> >>
> >> +static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
> >> +{
> >> + struct ti_sn65dsi86 *pdata = private;
> >> + struct drm_device *dev = pdata->bridge.dev;
> >
> >I'm unsure if accessing "dev" here without any sort of locking is
> >safe... It feels like, in theory, "detach" could be called and race
> >with the IRQ handler? Maybe you need a spinlock to be sure?
>
> I tested a spinlock added to the ti-sn65dsi86 structure that gets used
> in the ti_sn_bridge_detach and ti_sn_bridge_interrupt functions and it
> seems to work. Is there another spinlock created somewhere that I could
> use instead? Is using the spin lock in the interrupt and detach functions
> the correct way to do it?
In this case you could probably use a mutex since you're running a
threaded IRQ handler and sleeping is allowed. You could probably
create a new mutex for this case.
I assume you'd need some sort of boolean variable instead of just
checking if "bridge.dev" is non-NULL? "bridge.dev" is set by the DRM
core before your attach is called (and cleared after detach). Maybe
just have a boolean about whether HPD is enabled and only send the
event if HPD is enabled? Then use the mutex to protect access to that
boolean between the IRQ handler and the HPD enable/disable code?
-Doug
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.