[PATCH v5 4/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Doug Anderson]

Hi,

On Wed, Aug 24, 2022 at 6:00 AM Tomi Valkeinen
<tomi.valkeinen at ideasonboard.com> wrote:
>
> From: Laurent Pinchart <laurent.pinchart+renesas at ideasonboard.com>
>
> Implement the bridge connector-related .get_edid() and .detect()
> operations, and report the related bridge capabilities and type.
>
> These ops are only added for DP mode. They should also be used for eDP
> mode, but the driver seems to be mostly used for eDP and, according to
> the comments, they've had issues with eDP panels and HPD. So better be
> safe and only enable them for DP for now.

Just to be clear: the "They should also be used for eDP" is not correct.

* The detect() function should be returning whether the display is
physically there. For eDP it is _always_ physically there. Thus for
eDP the _correct_ implementation for detect is to always return true.
Yes, there is a line called HPD for eDP and yes that line is used for
full DisplayPort for detecting a display. For eDP, though, HPD does
not detect the presence of a display. A display is always there.

* For eDP implementing get_edid() is done in the panel so that power
sequencing can be done properly. While it could have been designed
other ways, that's how we ended up in the end. Thus eDP controllers
don't implement get_edid().


> @@ -1163,10 +1165,33 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
>         pm_runtime_put_sync(pdata->dev);
>  }
>
> +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
> +{
> +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +       int val = 0;
> +
> +       pm_runtime_get_sync(pdata->dev);
> +       regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
> +       pm_runtime_put_autosuspend(pdata->dev);
> +
> +       return val & HPD_DEBOUNCED_STATE ? connector_status_connected
> +                                        : connector_status_disconnected;
> +}

I thought in the end we decided that you _could_ get a hot plug detect
interrupt if you just did a pm_runtime_get_sync() sometime earlier in
the case of DP. Basically you're just saying that if you're DP that
you always powered up. Doing some searches makes me find some
discussion at:

https://lore.kernel.org/r/20220310152227.2122960-4-kieran.bingham+renesas@ideasonboard.com

Specifically, the right answer is: "In general the pm_runtime_get
reference need to go with the IRQ enabling"

In any case, if we want to start with just implementing "detect"
that's OK with me...

Thus with the commit message clarified, feel free to add my Reviewed-by.