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