Quoting Doug Anderson (2022-03-07 23:22:17)
> Hi,
>
> On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham
> <kieran.bingham+renesas@xxxxxxxxxxxxxxxx> 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
