[PATCH v2 4/4] drm/bridge: ti-sn65dsi86: Support hotplug detection

[Kieran Bingham]

Quoting Doug Anderson (2022-03-07 23:22:17)
> Hi,
> 
> On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham
> <kieran.bingham+renesas at ideasonboard.com> wrote:
> >
> > @@ -1264,6 +1321,14 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> >                 return PTR_ERR(pdata->next_bridge);
> >         }
> >
> > +       pdata->no_hpd = of_property_read_bool(np, "no-hpd");
> > +       if (pdata->next_bridge->type != DRM_MODE_CONNECTOR_DisplayPort &&
> > +           !pdata->no_hpd) {
> > +               dev_warn(pdata->dev,
> > +                        "HPD support requires a DisplayPort connector\n");
> 
> Maybe "HPD support only implemented for full DP connectors".
> 
> Plausibly someone could come up with a reason to hook HPD up for eDP
> one day, but so far we haven't seen it.
> 

Ok, updated.


> 
> > @@ -1272,7 +1337,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> >
> >         pdata->bridge.funcs = &ti_sn_bridge_funcs;
> >         pdata->bridge.of_node = np;
> > -       pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> > +       pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_HPD)
> > +                         | DRM_BRIDGE_OP_EDID;
> 
> Seems like "OP_HPD" ought to be dependent on whether the IRQ was
> provided, right? AKA you could have "detect" because HPD is plumbed
> through to the bridge but not "HPD" if the IRQ from the bridge isn't
> hooked up to the main processor...

Yes, I think that's right. If there's no IRQ, then OP_HPD shouldn't be
set, and it will fall back to polling.

I'll fix that up.

> > @@ -1840,6 +1906,34 @@ static int ti_sn65dsi86_parse_regulators(struct ti_sn65dsi86 *pdata)
> >                                        pdata->supplies);
> >  }
> >
> > +static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg)
> > +{
> > +       struct ti_sn65dsi86 *pdata = arg;
> > +       int ret;
> > +       int hpd;
> 
> `hpd` should be an `unsigned int` to match the prototype of regmap-read.

Agreed, and updated.

> > @@ -1881,6 +1975,16 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
> >                 return dev_err_probe(dev, PTR_ERR(pdata->refclk),
> >                                      "failed to get reference clock\n");
> >
> > +       if (client->irq > 0) {
> > +               ret = devm_request_threaded_irq(dev, client->irq, NULL,
> > +                                               ti_sn65dsi86_irq_handler,
> > +                                               IRQF_ONESHOT, "sn65dsi86-irq",
> > +                                               pdata);
> > +               if (ret)
> > +                       return dev_err_probe(dev, ret,
> > +                                            "Failed to register DP interrupt\n");
> > +       }
> 
> Is this the right place to request the IRQ, or should it be in
> ti_sn_bridge_probe(). As soon as you request it the interrupt can go
> off, but you're relying on a bunch of bridge stuff to have been
> initted, right?

Indeed, it will be relying upon the bridge to have been set up.

You're right I believe, ti_sn_bridge_probe() sounds reasonable. And only
after that should we enable the interrupts.

Fixing ... (But getting stuck/blocked by the connector changes, so ..
I'll keep plowing through).

> > @@ -1888,6 +1992,9 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
> >         pm_runtime_set_autosuspend_delay(pdata->dev, 500);
> >         pm_runtime_use_autosuspend(pdata->dev);
> >
> > +       /* Enable interrupt handling */
> > +       regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);
> 
> Shouldn't we only enable interrupt handling if client->irq > 0? AKA
> combine this with the "if" statement?

Agreed.

> -Doug