Message-ID: <CAD=FV=WkbQeCaqHL3A3RzW9GhiLvkrZEbfLUFwfnx4LJQbUcWg@mail.gmail.com>
Date: Thu, 11 Sep 2025 16:25:07 -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 Thu, Sep 11, 2025 at 11:39 AM John Ripple <john.ripple@...sight.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

Powered by blists - more mailing lists