Re: [PATCH 1/2] drm/bridge: ti-sn65dsi86: improve HPD interrupt handling

Hi,

On Fri, Aug 14, 2026 at 5:03 AM Yashas D <[email protected]> wrote:
>
> Fix the interrupt handler to clear all three IRQ status registers
> to fully de-assert the IRQ pin, enable replug event detection, and
> use per-connector hotplug notification instead of polling all
> connectors on every DP HPD event.
>
> Signed-off-by: Yashas D <[email protected]>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 34 ++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 48b83df9aed6..d9bd4ef8f0e2 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -113,14 +113,19 @@
>  #define SN_IRQ_EVENTS_EN_REG                   0xE6
>  #define  HPD_INSERTION_EN                      BIT(1)
>  #define  HPD_REMOVAL_EN                                BIT(2)
> +#define  HPD_REPLUG_EN                         BIT(3)
>
>  #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_REPLUG_STATUS                     BIT(3)
>  #define  HPD_REMOVAL_STATUS                    BIT(2)
>  #define  HPD_INSERTION_STATUS                  BIT(1)
> +/* General IRQ status registers, write-1-to-clear */
> +#define SN_IRQ_STATUS2_REG                     0xF0
> +#define SN_IRQ_STATUS3_REG                     0xF2

nit: blank line before this section?


> @@ -1387,23 +1391,35 @@ static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
>                 return IRQ_NONE;
>         }
>
> -       hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
> +       hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS |
> +                             HPD_REPLUG_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);
> +       /*
> +        * Clear all three IRQ status registers to fully de-assert
> +        * the IRQ pin
> +        */
> +       ret  = regmap_write(pdata->regmap, SN_IRQ_STATUS2_REG, 0xFF);
> +       ret |= regmap_write(pdata->regmap, SN_IRQ_STATUS3_REG, 0xFF);
> +       ret |= regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);

Can you explain clearly why you need to clear STATUS2 and STATUS3? Is
this actually a thing you saw causing problems?

As far as I understand, all these extra interrupts should be masked
anyway. That means they shouldn't be affecting the IRQ line and they
shouldn't need to be cleared, right? Taking one example bit
(CHA_SOT_BIT_ERR), we can see that in register 0xE1 they default state
is to mask it. That means there should be no need to clear it in
register 0xF2, right?

If you want to be defensive, you could explictly zero out some of the
0xE1 - 0xE5 at probe time just in some bootloader made things enabled.
...but I don't see any reason to be clearing the status registers at
IRQ time unless there's some interrupt that we can't mask.



>         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. */
> +       /* Notify only the DP connector, not all connectors on the device. */
>         mutex_lock(&pdata->hpd_mutex);
> -       if (pdata->hpd_enabled && hpd_event)
> -               drm_kms_helper_hotplug_event(dev);
> -       mutex_unlock(&pdata->hpd_mutex);
> +       if (pdata->hpd_enabled && hpd_event && pdata->bridge.hpd_data) {
> +               struct drm_connector *connector =
> +                       (struct drm_connector *)pdata->bridge.hpd_data;
> +               mutex_unlock(&pdata->hpd_mutex);
> +               drm_connector_helper_hpd_irq_event(connector);
> +       } else {
> +               mutex_unlock(&pdata->hpd_mutex);
> +       }

Do you really need the above change? I haven't studied the DRM
framework all that much, but...

1. It feels like an abstraction violation to reach into
"bridge.hpd_data" and assume it's the connector. I guess Sashiko
agress with me...

2. The documentation for the old function were were calling
(drm_kms_helper_hotplug_event) says that, if you know the connector,
you should call drm_kms_helper_connector_hotplug_event(). You're not
doing that and are instead calling
drm_connector_helper_hpd_irq_event().

Unless this is actually fixing problems, I'm inclined not to take this
patch. If you _really_ feel like it's an improvement, maybe someone
else can comment on it or I can schedule some time in the future to
dig more.


Maybe split the "replug" part of this patch into its own patch, if
that's actually something that you've confirmed is useful / does
something?


-Doug