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

[Doug Anderson]

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.


> @@ -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...


> @@ -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.


> @@ -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?


> @@ -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?



-Doug