Message-ID: <CAE-0n51VBDmOOworjpuB1nqVD-7055yqvn2Er5H13qgFC5R2AQ@mail.gmail.com>
Date:   Fri, 22 Apr 2022 17:13:45 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Sankeerth Billakanti <quic_sbillaka@...cinc.com>,
        devicetree@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        freedreno@...ts.freedesktop.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Cc:     robdclark@...il.com, seanpaul@...omium.org,
        quic_kalyant@...cinc.com, quic_abhinavk@...cinc.com,
        dianders@...omium.org, quic_khsieh@...cinc.com,
        bjorn.andersson@...aro.org, sean@...rly.run, airlied@...ux.ie,
        daniel@...ll.ch, dmitry.baryshkov@...aro.org,
        quic_vproddut@...cinc.com, quic_aravindh@...cinc.com,
        steev@...i.org
Subject: Re: [PATCH v9 1/4] drm/msm/dp: Add eDP support via aux_bus

Quoting Sankeerth Billakanti (2022-04-22 02:11:03)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index d7a19d6..055681a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c

Some nitpicks

Reviewed-by: Stephen Boyd <swboyd@...omium.org>

> @@ -1508,7 +1509,8 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
>
>         dp_hpd_event_setup(dp);
>
> -       dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
> +       if (!dp_display->is_edp)
> +               dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);

Did it turn out that in fact DP isn't ready still to setup even after
delaying the irq?

>  }
>
>  void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
> @@ -1530,6 +1532,65 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
>         }
>  }
>
> +static int dp_display_get_next_bridge(struct msm_dp *dp)
> +{
> +       int rc;
> +       struct dp_display_private *dp_priv;
> +       struct device_node *aux_bus;
> +       struct device *dev;
> +
> +       dp_priv = container_of(dp, struct dp_display_private, dp_display);
> +       dev = &dp_priv->pdev->dev;
> +       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
> +
> +       if (aux_bus && dp->is_edp) {
> +               dp_display_host_init(dp_priv);
> +               dp_catalog_ctrl_hpd_config(dp_priv->catalog);
> +               dp_display_host_phy_init(dp_priv);
> +               enable_irq(dp_priv->irq);
> +
> +               /*
> +                * The code below assumes that the panel will finish probing
> +                * by the time devm_of_dp_aux_populate_ep_devices() returns.
> +                * This isn't a great assumption since it will fail if the
> +                * panel driver is probed asynchronously but is the best we
> +                * can do without a bigger driver reorganization.
> +                */
> +               rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
> +               of_node_put(aux_bus);
> +               if (rc)
> +                       goto error;
> +       } else if (dp->is_edp) {
> +               DRM_ERROR("eDP aux_bus not found\n");
> +               return -ENODEV;
> +       }
> +
> +       /*
> +        * External bridges are mandatory for eDP interfaces: one has to
> +        * provide at least an eDP panel (which gets wrapped into panel-bridge).
> +        *
> +        * For DisplayPort interfaces external bridges are optional, so
> +        * silently ignore an error if one is not present (-ENODEV).
> +        */
> +       rc = dp_parser_find_next_bridge(dp_priv->parser);
> +       if (!dp->is_edp && rc == -ENODEV)
> +               return 0;
> +       else if (rc)

Just an if because there's a return above.

> +               goto error;
> +
> +       dp->next_bridge = dp_priv->parser->next_bridge;

In which case it almost feels like it could be written

	if (!dp->is_edp && rc == -ENODEV)
		return 0;
	if (!rc) {
		dp->next_bridge = dp_priv->parser->next_bridge;
		return 0;
	}
error:
	if (dp->is_edp) {

but I'm not worried either way, besides removing the else from the else-if.

> +
> +       return 0;
> +
> +error:
> +       if (dp->is_edp) {
> +               disable_irq(dp_priv->irq);
> +               dp_display_host_phy_exit(dp_priv);
> +               dp_display_host_deinit(dp_priv);
> +       }
> +       return rc;
> +}
> +
>  int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>                         struct drm_encoder *encoder)
>  {
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
> index d371bae..950416c 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> @@ -125,7 +125,7 @@ struct dp_parser {
>         u32 max_dp_lanes;
>         struct drm_bridge *next_bridge;
>
> -       int (*parse)(struct dp_parser *parser, int connector_type);
> +       int (*parse)(struct dp_parser *parser);
>  };
>
>  /**
> @@ -141,4 +141,15 @@ struct dp_parser {
>   */
>  struct dp_parser *dp_parser_get(struct platform_device *pdev);
>
> +/**
> + * dp_parser_find_next_bridge() - find an additional bridge to DP
> + *
> + * @parser: dp_parser data from client
> + * return: 0 if able to get the bridge else return an error code

return comes after the description below. Also should be capitalized.
You can check this by compiling with W=1 I believe, or run the
kernel doc format script on the file..

> + *
> + * This function is used to find any additional bridge attached to
> + * the DP controller. The eDP interface requires a panel bridge.

Return: 0 if able to get the bridge, otherwise negative errno for failure

> + */
> +int dp_parser_find_next_bridge(struct dp_parser *parser);
> +

Powered by blists - more mailing lists