Message-ID: <CAD=FV=Um5NKHFZJJkC6eC0rnea0xSPeWVpK91PwGcrRjri28NA@mail.gmail.com>
Date: Wed, 10 Sep 2025 13:48:25 -0700
From: Doug Anderson <dianders@...omium.org>
To: John Ripple <john.ripple@...sight.com>
Cc: Laurent.pinchart@...asonboard.com, airlied@...il.com, 
	andrzej.hajda@...el.com, blake.vermeer@...sight.com, 
	dri-devel@...ts.freedesktop.org, jernej.skrabec@...il.com, jonas@...boo.se, 
	linux-kernel@...r.kernel.org, maarten.lankhorst@...ux.intel.com, 
	matt_laubhan@...sight.com, mripard@...nel.org, neil.armstrong@...aro.org, 
	rfoss@...nel.org, simona@...ll.ch, tzimmermann@...e.de
Subject: Re: [PATCH V3] drm/bridge: ti-sn65dsi86: Add support for DisplayPort
 mode with HPD

Hi,

On Wed, Sep 10, 2025 at 11:34 AM John Ripple <john.ripple@...sight.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

Powered by blists - more mailing lists