Hi Jagan
Responding due to Marek's comment on the "Add Samsung MIPI DSIM
bridge" series, although I know very little about the Exynos
specifics, and may well be missing context of what you're trying to
achieve.
On Mon, 12 Dec 2022 at 18:29, Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> Restore the proper bridge chain by finding the previous bridge
> in the chain instead of passing NULL.
>
> This establishes a proper bridge chain while attaching downstream
> bridges.
>
> Reviewed-by: Marek Vasut <marex@xxxxxxx>
> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> Signed-off-by: Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx>
> ---
> Changes for v11:
> - add bridge.pre_enable_prev_first
> Changes for v10:
> - collect Marek review tag
>
> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index ec673223d6b7..9d10a89d28f1 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -1428,7 +1428,8 @@ static int exynos_dsi_attach(struct drm_bridge *bridge,
> {
> struct exynos_dsi *dsi = bridge_to_dsi(bridge);
>
> - return drm_bridge_attach(bridge->encoder, dsi->out_bridge, NULL, flags);
> + return drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge,
> + flags);
Agreed on this change.
> }
>
> static const struct drm_bridge_funcs exynos_dsi_bridge_funcs = {
> @@ -1474,7 +1475,10 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>
> drm_bridge_add(&dsi->bridge);
>
> - drm_bridge_attach(encoder, &dsi->bridge, NULL, 0);
> + drm_bridge_attach(encoder, &dsi->bridge,
> + list_first_entry_or_null(&encoder->bridge_chain,
> + struct drm_bridge,
> + chain_node), 0);
What bridge are you expecting between the encoder and this bridge?
The encoder is the drm_simple_encoder_init encoder that you've created
in exynos_dsi_bind, so separating that from the bridge you're also
creating here seems weird.
>
> /*
> * This is a temporary solution and should be made by more generic way.
> @@ -1709,6 +1713,7 @@ static int exynos_dsi_probe(struct platform_device *pdev)
> dsi->bridge.funcs = &exynos_dsi_bridge_funcs;
> dsi->bridge.of_node = dev->of_node;
> dsi->bridge.type = DRM_MODE_CONNECTOR_DSI;
> + dsi->bridge.pre_enable_prev_first = true;
Setting dsi->bridge.pre_enable_prev_first on what is presumably the
DSI host controller seems a little odd.
Same question again - what bridge are you expecting to be upstream of
the DSI host that needs to be preenabled before it? Whilst it's
possible that there's another bridge, I'd have expected that to be the
first link from your encoder as they appear to both belong to the same
bit of driver.
Dave
> ret = component_add(dev, &exynos_dsi_component_ops);
> if (ret)
> --
> 2.25.1
>
