diff mbox series

[v3,1/2] drm/msm/dp: Add support for determining the eDP/DP mode from DT

Message ID 20240322-x1e80100-display-refactor-connector-v3-1-af14c29af665@linaro.org (mailing list archive)
State Superseded
Headers show
Series drm/msm/dp: Rework the eDP/DP modes and add support for X1E80100 | expand

Commit Message

Abel Vesa March 22, 2024, 1:22 p.m. UTC
Instead of relying on different compatibles for eDP and DP, lookup
the panel node in devicetree to figure out the connector type and
then pass on that information to the PHY. External DP is not described
in DT, therefore, assume it's eDP if panel node is present.

Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 43 +++++++++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 4 deletions(-)

Comments

Dmitry Baryshkov March 22, 2024, 1:30 p.m. UTC | #1
On Fri, 22 Mar 2024 at 15:22, Abel Vesa <abel.vesa@linaro.org> wrote:
>
> Instead of relying on different compatibles for eDP and DP, lookup
> the panel node in devicetree to figure out the connector type and
> then pass on that information to the PHY. External DP is not described

Nit: External DP doesn't have a panel described in DT...

> in DT, therefore, assume it's eDP if panel node is present.
>
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 43 +++++++++++++++++++++++++++++++++----
>  1 file changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index c4cb82af5c2f..c9763f77c832 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -726,6 +726,14 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
>         if (IS_ERR(phy))
>                 return PTR_ERR(phy);
>
> +       rc = phy_set_mode_ext(phy, PHY_MODE_DP,
> +                             dp->dp_display.is_edp ? PHY_SUBMODE_EDP : PHY_SUBMODE_DP);
> +       if (rc) {
> +               DRM_ERROR("failed to set phy submode, rc = %d\n", rc);
> +               dp->catalog = NULL;
> +               goto error;
> +       }
> +
>         dp->catalog = dp_catalog_get(dev);
>         if (IS_ERR(dp->catalog)) {
>                 rc = PTR_ERR(dp->catalog);
> @@ -734,9 +742,7 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
>                 goto error;
>         }
>
> -       dp->aux = dp_aux_get(dev, dp->catalog,
> -                            phy,
> -                            dp->dp_display.is_edp);
> +       dp->aux = dp_aux_get(dev, dp->catalog, phy, dp->dp_display.is_edp);

Unrelated

>         if (IS_ERR(dp->aux)) {
>                 rc = PTR_ERR(dp->aux);
>                 DRM_ERROR("failed to initialize aux, rc = %d\n", rc);
> @@ -1241,6 +1247,35 @@ static int dp_auxbus_done_probe(struct drm_dp_aux *aux)
>         return dp_display_probe_tail(aux->dev);
>  }
>
> +static int dp_display_get_connector_type(struct platform_device *pdev,
> +                                        const struct msm_dp_desc *desc)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct device_node *aux_bus;
> +       struct device_node *panel;
> +       int ret = DRM_MODE_CONNECTOR_DisplayPort;
> +
> +       /* legacy platforms specify connector type in match data */
> +       if (desc->connector_type == DRM_MODE_CONNECTOR_eDP ||
> +               desc->connector_type == DRM_MODE_CONNECTOR_DisplayPort)

misaligned

> +               return desc->connector_type;

Can we drop this part completely?

> +
> +       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
> +       if (!aux_bus)
> +               goto out;
> +
> +       panel = of_get_child_by_name(aux_bus, "panel");
> +       if (!panel)
> +               goto out;
> +
> +       ret = DRM_MODE_CONNECTOR_eDP;
> +
> +out:
> +       of_node_put(panel);
> +       of_node_put(aux_bus);
> +       return ret;
> +}
> +
>  static int dp_display_probe(struct platform_device *pdev)
>  {
>         int rc = 0;
> @@ -1263,7 +1298,7 @@ static int dp_display_probe(struct platform_device *pdev)
>         dp->dp_display.pdev = pdev;
>         dp->name = "drm_dp";
>         dp->id = desc->id;
> -       dp->dp_display.connector_type = desc->connector_type;
> +       dp->dp_display.connector_type = dp_display_get_connector_type(pdev, desc);
>         dp->wide_bus_supported = desc->wide_bus_supported;
>         dp->dp_display.is_edp =
>                 (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP);
>
> --
> 2.34.1
>
Abel Vesa March 22, 2024, 1:36 p.m. UTC | #2
On 24-03-22 15:30:54, Dmitry Baryshkov wrote:
> On Fri, 22 Mar 2024 at 15:22, Abel Vesa <abel.vesa@linaro.org> wrote:
> >
> > Instead of relying on different compatibles for eDP and DP, lookup
> > the panel node in devicetree to figure out the connector type and
> > then pass on that information to the PHY. External DP is not described
> 
> Nit: External DP doesn't have a panel described in DT...
> 

Will fix.

> > in DT, therefore, assume it's eDP if panel node is present.
> >
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > ---
> >  drivers/gpu/drm/msm/dp/dp_display.c | 43 +++++++++++++++++++++++++++++++++----
> >  1 file changed, 39 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > index c4cb82af5c2f..c9763f77c832 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -726,6 +726,14 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
> >         if (IS_ERR(phy))
> >                 return PTR_ERR(phy);
> >
> > +       rc = phy_set_mode_ext(phy, PHY_MODE_DP,
> > +                             dp->dp_display.is_edp ? PHY_SUBMODE_EDP : PHY_SUBMODE_DP);
> > +       if (rc) {
> > +               DRM_ERROR("failed to set phy submode, rc = %d\n", rc);
> > +               dp->catalog = NULL;
> > +               goto error;
> > +       }
> > +
> >         dp->catalog = dp_catalog_get(dev);
> >         if (IS_ERR(dp->catalog)) {
> >                 rc = PTR_ERR(dp->catalog);
> > @@ -734,9 +742,7 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
> >                 goto error;
> >         }
> >
> > -       dp->aux = dp_aux_get(dev, dp->catalog,
> > -                            phy,
> > -                            dp->dp_display.is_edp);
> > +       dp->aux = dp_aux_get(dev, dp->catalog, phy, dp->dp_display.is_edp);
> 
> Unrelated
> 

Yep, will drop the change.

> >         if (IS_ERR(dp->aux)) {
> >                 rc = PTR_ERR(dp->aux);
> >                 DRM_ERROR("failed to initialize aux, rc = %d\n", rc);
> > @@ -1241,6 +1247,35 @@ static int dp_auxbus_done_probe(struct drm_dp_aux *aux)
> >         return dp_display_probe_tail(aux->dev);
> >  }
> >
> > +static int dp_display_get_connector_type(struct platform_device *pdev,
> > +                                        const struct msm_dp_desc *desc)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       struct device_node *aux_bus;
> > +       struct device_node *panel;
> > +       int ret = DRM_MODE_CONNECTOR_DisplayPort;
> > +
> > +       /* legacy platforms specify connector type in match data */
> > +       if (desc->connector_type == DRM_MODE_CONNECTOR_eDP ||
> > +               desc->connector_type == DRM_MODE_CONNECTOR_DisplayPort)
> 
> misaligned
> 

Sure, will fix.

> > +               return desc->connector_type;
> 
> Can we drop this part completely?
> 

You mean the whole if clause? How should we handle the legacy approach
then?

> > +
> > +       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
> > +       if (!aux_bus)
> > +               goto out;
> > +
> > +       panel = of_get_child_by_name(aux_bus, "panel");
> > +       if (!panel)
> > +               goto out;
> > +
> > +       ret = DRM_MODE_CONNECTOR_eDP;
> > +
> > +out:
> > +       of_node_put(panel);
> > +       of_node_put(aux_bus);
> > +       return ret;
> > +}
> > +
> >  static int dp_display_probe(struct platform_device *pdev)
> >  {
> >         int rc = 0;
> > @@ -1263,7 +1298,7 @@ static int dp_display_probe(struct platform_device *pdev)
> >         dp->dp_display.pdev = pdev;
> >         dp->name = "drm_dp";
> >         dp->id = desc->id;
> > -       dp->dp_display.connector_type = desc->connector_type;
> > +       dp->dp_display.connector_type = dp_display_get_connector_type(pdev, desc);
> >         dp->wide_bus_supported = desc->wide_bus_supported;
> >         dp->dp_display.is_edp =
> >                 (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP);
> >
> > --
> > 2.34.1
> >
> 
> 
> -- 
> With best wishes
> Dmitry
Dmitry Baryshkov March 22, 2024, 1:38 p.m. UTC | #3
On Fri, 22 Mar 2024 at 15:36, Abel Vesa <abel.vesa@linaro.org> wrote:
>
> On 24-03-22 15:30:54, Dmitry Baryshkov wrote:
> > On Fri, 22 Mar 2024 at 15:22, Abel Vesa <abel.vesa@linaro.org> wrote:
> > >
> > > Instead of relying on different compatibles for eDP and DP, lookup
> > > the panel node in devicetree to figure out the connector type and
> > > then pass on that information to the PHY. External DP is not described
> >
> > Nit: External DP doesn't have a panel described in DT...
> >
>
> Will fix.
>
> > > in DT, therefore, assume it's eDP if panel node is present.
> > >
> > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > > ---
> > >  drivers/gpu/drm/msm/dp/dp_display.c | 43 +++++++++++++++++++++++++++++++++----
> > >  1 file changed, 39 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > index c4cb82af5c2f..c9763f77c832 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > @@ -726,6 +726,14 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
> > >         if (IS_ERR(phy))
> > >                 return PTR_ERR(phy);
> > >
> > > +       rc = phy_set_mode_ext(phy, PHY_MODE_DP,
> > > +                             dp->dp_display.is_edp ? PHY_SUBMODE_EDP : PHY_SUBMODE_DP);
> > > +       if (rc) {
> > > +               DRM_ERROR("failed to set phy submode, rc = %d\n", rc);
> > > +               dp->catalog = NULL;
> > > +               goto error;
> > > +       }
> > > +
> > >         dp->catalog = dp_catalog_get(dev);
> > >         if (IS_ERR(dp->catalog)) {
> > >                 rc = PTR_ERR(dp->catalog);
> > > @@ -734,9 +742,7 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
> > >                 goto error;
> > >         }
> > >
> > > -       dp->aux = dp_aux_get(dev, dp->catalog,
> > > -                            phy,
> > > -                            dp->dp_display.is_edp);
> > > +       dp->aux = dp_aux_get(dev, dp->catalog, phy, dp->dp_display.is_edp);
> >
> > Unrelated
> >
>
> Yep, will drop the change.
>
> > >         if (IS_ERR(dp->aux)) {
> > >                 rc = PTR_ERR(dp->aux);
> > >                 DRM_ERROR("failed to initialize aux, rc = %d\n", rc);
> > > @@ -1241,6 +1247,35 @@ static int dp_auxbus_done_probe(struct drm_dp_aux *aux)
> > >         return dp_display_probe_tail(aux->dev);
> > >  }
> > >
> > > +static int dp_display_get_connector_type(struct platform_device *pdev,
> > > +                                        const struct msm_dp_desc *desc)
> > > +{
> > > +       struct device *dev = &pdev->dev;
> > > +       struct device_node *aux_bus;
> > > +       struct device_node *panel;
> > > +       int ret = DRM_MODE_CONNECTOR_DisplayPort;
> > > +
> > > +       /* legacy platforms specify connector type in match data */
> > > +       if (desc->connector_type == DRM_MODE_CONNECTOR_eDP ||
> > > +               desc->connector_type == DRM_MODE_CONNECTOR_DisplayPort)
> >
> > misaligned
> >
>
> Sure, will fix.
>
> > > +               return desc->connector_type;
> >
> > Can we drop this part completely?
> >
>
> You mean the whole if clause? How should we handle the legacy approach
> then?

Legacy platforms still have the aux-bus/panel. so they should be
handled by the check below.

>
> > > +
> > > +       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
> > > +       if (!aux_bus)
> > > +               goto out;
> > > +
> > > +       panel = of_get_child_by_name(aux_bus, "panel");
> > > +       if (!panel)
> > > +               goto out;
> > > +
> > > +       ret = DRM_MODE_CONNECTOR_eDP;
> > > +
> > > +out:
> > > +       of_node_put(panel);
> > > +       of_node_put(aux_bus);
> > > +       return ret;
> > > +}
> > > +
> > >  static int dp_display_probe(struct platform_device *pdev)
> > >  {
> > >         int rc = 0;
> > > @@ -1263,7 +1298,7 @@ static int dp_display_probe(struct platform_device *pdev)
> > >         dp->dp_display.pdev = pdev;
> > >         dp->name = "drm_dp";
> > >         dp->id = desc->id;
> > > -       dp->dp_display.connector_type = desc->connector_type;
> > > +       dp->dp_display.connector_type = dp_display_get_connector_type(pdev, desc);
> > >         dp->wide_bus_supported = desc->wide_bus_supported;
> > >         dp->dp_display.is_edp =
> > >                 (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP);
> > >
> > > --
> > > 2.34.1
> > >
> >
> >
> > --
> > With best wishes
> > Dmitry
Abel Vesa March 22, 2024, 2:15 p.m. UTC | #4
On 24-03-22 15:38:03, Dmitry Baryshkov wrote:
> On Fri, 22 Mar 2024 at 15:36, Abel Vesa <abel.vesa@linaro.org> wrote:
> >
> > On 24-03-22 15:30:54, Dmitry Baryshkov wrote:
> > > On Fri, 22 Mar 2024 at 15:22, Abel Vesa <abel.vesa@linaro.org> wrote:
> > > >
> > > > Instead of relying on different compatibles for eDP and DP, lookup
> > > > the panel node in devicetree to figure out the connector type and
> > > > then pass on that information to the PHY. External DP is not described
> > >
> > > Nit: External DP doesn't have a panel described in DT...
> > >
> >
> > Will fix.
> >
> > > > in DT, therefore, assume it's eDP if panel node is present.
> > > >
> > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > > > ---
> > > >  drivers/gpu/drm/msm/dp/dp_display.c | 43 +++++++++++++++++++++++++++++++++----
> > > >  1 file changed, 39 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > index c4cb82af5c2f..c9763f77c832 100644
> > > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > @@ -726,6 +726,14 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
> > > >         if (IS_ERR(phy))
> > > >                 return PTR_ERR(phy);
> > > >
> > > > +       rc = phy_set_mode_ext(phy, PHY_MODE_DP,
> > > > +                             dp->dp_display.is_edp ? PHY_SUBMODE_EDP : PHY_SUBMODE_DP);
> > > > +       if (rc) {
> > > > +               DRM_ERROR("failed to set phy submode, rc = %d\n", rc);
> > > > +               dp->catalog = NULL;
> > > > +               goto error;
> > > > +       }
> > > > +
> > > >         dp->catalog = dp_catalog_get(dev);
> > > >         if (IS_ERR(dp->catalog)) {
> > > >                 rc = PTR_ERR(dp->catalog);
> > > > @@ -734,9 +742,7 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
> > > >                 goto error;
> > > >         }
> > > >
> > > > -       dp->aux = dp_aux_get(dev, dp->catalog,
> > > > -                            phy,
> > > > -                            dp->dp_display.is_edp);
> > > > +       dp->aux = dp_aux_get(dev, dp->catalog, phy, dp->dp_display.is_edp);
> > >
> > > Unrelated
> > >
> >
> > Yep, will drop the change.
> >
> > > >         if (IS_ERR(dp->aux)) {
> > > >                 rc = PTR_ERR(dp->aux);
> > > >                 DRM_ERROR("failed to initialize aux, rc = %d\n", rc);
> > > > @@ -1241,6 +1247,35 @@ static int dp_auxbus_done_probe(struct drm_dp_aux *aux)
> > > >         return dp_display_probe_tail(aux->dev);
> > > >  }
> > > >
> > > > +static int dp_display_get_connector_type(struct platform_device *pdev,
> > > > +                                        const struct msm_dp_desc *desc)
> > > > +{
> > > > +       struct device *dev = &pdev->dev;
> > > > +       struct device_node *aux_bus;
> > > > +       struct device_node *panel;
> > > > +       int ret = DRM_MODE_CONNECTOR_DisplayPort;
> > > > +
> > > > +       /* legacy platforms specify connector type in match data */
> > > > +       if (desc->connector_type == DRM_MODE_CONNECTOR_eDP ||
> > > > +               desc->connector_type == DRM_MODE_CONNECTOR_DisplayPort)
> > >
> > > misaligned
> > >
> >
> > Sure, will fix.
> >
> > > > +               return desc->connector_type;
> > >
> > > Can we drop this part completely?
> > >
> >
> > You mean the whole if clause? How should we handle the legacy approach
> > then?
> 
> Legacy platforms still have the aux-bus/panel. so they should be
> handled by the check below.
> 

Oh, in that case we can drop the connector_type from every desc for all
platforms.

> >
> > > > +
> > > > +       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
> > > > +       if (!aux_bus)
> > > > +               goto out;
> > > > +
> > > > +       panel = of_get_child_by_name(aux_bus, "panel");
> > > > +       if (!panel)
> > > > +               goto out;
> > > > +
> > > > +       ret = DRM_MODE_CONNECTOR_eDP;
> > > > +
> > > > +out:
> > > > +       of_node_put(panel);
> > > > +       of_node_put(aux_bus);
> > > > +       return ret;
> > > > +}
> > > > +
> > > >  static int dp_display_probe(struct platform_device *pdev)
> > > >  {
> > > >         int rc = 0;
> > > > @@ -1263,7 +1298,7 @@ static int dp_display_probe(struct platform_device *pdev)
> > > >         dp->dp_display.pdev = pdev;
> > > >         dp->name = "drm_dp";
> > > >         dp->id = desc->id;
> > > > -       dp->dp_display.connector_type = desc->connector_type;
> > > > +       dp->dp_display.connector_type = dp_display_get_connector_type(pdev, desc);
> > > >         dp->wide_bus_supported = desc->wide_bus_supported;
> > > >         dp->dp_display.is_edp =
> > > >                 (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP);
> > > >
> > > > --
> > > > 2.34.1
> > > >
> > >
> > >
> > > --
> > > With best wishes
> > > Dmitry
> 
> 
> 
> -- 
> With best wishes
> Dmitry
Johan Hovold March 22, 2024, 2:25 p.m. UTC | #5
On Fri, Mar 22, 2024 at 04:15:23PM +0200, Abel Vesa wrote:
> On 24-03-22 15:38:03, Dmitry Baryshkov wrote:
> > On Fri, 22 Mar 2024 at 15:36, Abel Vesa <abel.vesa@linaro.org> wrote:
> > > On 24-03-22 15:30:54, Dmitry Baryshkov wrote:
> > > > On Fri, 22 Mar 2024 at 15:22, Abel Vesa <abel.vesa@linaro.org> wrote:

> > > > > +static int dp_display_get_connector_type(struct platform_device *pdev,
> > > > > +                                        const struct msm_dp_desc *desc)
> > > > > +{
> > > > > +       struct device *dev = &pdev->dev;
> > > > > +       struct device_node *aux_bus;
> > > > > +       struct device_node *panel;
> > > > > +       int ret = DRM_MODE_CONNECTOR_DisplayPort;
> > > > > +
> > > > > +       /* legacy platforms specify connector type in match data */
> > > > > +       if (desc->connector_type == DRM_MODE_CONNECTOR_eDP ||
> > > > > +               desc->connector_type == DRM_MODE_CONNECTOR_DisplayPort)
> > > >
> > > > misaligned
> > > >
> > >
> > > Sure, will fix.
> > >
> > > > > +               return desc->connector_type;
> > > >
> > > > Can we drop this part completely?
> > > >
> > >
> > > You mean the whole if clause? How should we handle the legacy approach
> > > then?
> > 
> > Legacy platforms still have the aux-bus/panel. so they should be
> > handled by the check below.
> > 
> 
> Oh, in that case we can drop the connector_type from every desc for all
> platforms.

Guys, please trim your replies!

Johan
Bjorn Andersson March 22, 2024, 2:30 p.m. UTC | #6
On Fri, Mar 22, 2024 at 03:22:22PM +0200, Abel Vesa wrote:
> Instead of relying on different compatibles for eDP and DP, lookup
> the panel node in devicetree to figure out the connector type and
> then pass on that information to the PHY. External DP is not described
> in DT, therefore, assume it's eDP if panel node is present.
> 
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 43 +++++++++++++++++++++++++++++++++----
>  1 file changed, 39 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index c4cb82af5c2f..c9763f77c832 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -726,6 +726,14 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
>  	if (IS_ERR(phy))
>  		return PTR_ERR(phy);
>  
> +	rc = phy_set_mode_ext(phy, PHY_MODE_DP,
> +			      dp->dp_display.is_edp ? PHY_SUBMODE_EDP : PHY_SUBMODE_DP);
> +	if (rc) {
> +		DRM_ERROR("failed to set phy submode, rc = %d\n", rc);
> +		dp->catalog = NULL;
> +		goto error;
> +	}
> +
>  	dp->catalog = dp_catalog_get(dev);
>  	if (IS_ERR(dp->catalog)) {
>  		rc = PTR_ERR(dp->catalog);
> @@ -734,9 +742,7 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
>  		goto error;
>  	}
>  
> -	dp->aux = dp_aux_get(dev, dp->catalog,
> -			     phy,
> -			     dp->dp_display.is_edp);
> +	dp->aux = dp_aux_get(dev, dp->catalog, phy, dp->dp_display.is_edp);
>  	if (IS_ERR(dp->aux)) {
>  		rc = PTR_ERR(dp->aux);
>  		DRM_ERROR("failed to initialize aux, rc = %d\n", rc);
> @@ -1241,6 +1247,35 @@ static int dp_auxbus_done_probe(struct drm_dp_aux *aux)
>  	return dp_display_probe_tail(aux->dev);
>  }
>  
> +static int dp_display_get_connector_type(struct platform_device *pdev,
> +					 const struct msm_dp_desc *desc)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *aux_bus;
> +	struct device_node *panel;
> +	int ret = DRM_MODE_CONNECTOR_DisplayPort;
> +
> +	/* legacy platforms specify connector type in match data */
> +	if (desc->connector_type == DRM_MODE_CONNECTOR_eDP ||
> +		desc->connector_type == DRM_MODE_CONNECTOR_DisplayPort)
> +		return desc->connector_type;
> +
> +	aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
> +	if (!aux_bus)
> +		goto out;

My compiler warns that if we take this code path, then you will
of_node_put(<uninitialized panel>) below.

> +
> +	panel = of_get_child_by_name(aux_bus, "panel");
> +	if (!panel)
> +		goto out;
> +
> +	ret = DRM_MODE_CONNECTOR_eDP;

My brain read this function as:
check something
if (error)
  bailout!

check something
if (error)
  bailout!

ret should be edp

I then have to scan the code again to figure out what ret is otherwise,
and convince myself that the error path is never an error, but a totally
normal case.


If you instead rely on the fact that both of_get_child_by_name() and
of_node_put() can be passed NULL, you can write this as:

static int fn(..) {
  aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
  panel = of_get_child_by_name(aux_bus, "panel");

  if (panel)
    connector_type = DRM_MODE_CONNECTOR_eDP;
  else
    connector_type = DRM_MODE_CONNECTOR_DisplayPort;

  of_node_put(panel);
  of_node_put(aux_bus);

  return connector_type;
}

Much easier to read, and you don't even have to zero-initialize panel to
avoid that compiler warning.

Regards,
Bjorn

> +
> +out:
> +	of_node_put(panel);
> +	of_node_put(aux_bus);
> +	return ret;
> +}
> +
>  static int dp_display_probe(struct platform_device *pdev)
>  {
>  	int rc = 0;
> @@ -1263,7 +1298,7 @@ static int dp_display_probe(struct platform_device *pdev)
>  	dp->dp_display.pdev = pdev;
>  	dp->name = "drm_dp";
>  	dp->id = desc->id;
> -	dp->dp_display.connector_type = desc->connector_type;
> +	dp->dp_display.connector_type = dp_display_get_connector_type(pdev, desc);
>  	dp->wide_bus_supported = desc->wide_bus_supported;
>  	dp->dp_display.is_edp =
>  		(dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP);
> 
> -- 
> 2.34.1
>
Abel Vesa March 22, 2024, 2:50 p.m. UTC | #7
On 24-03-22 09:30:21, Bjorn Andersson wrote:
> On Fri, Mar 22, 2024 at 03:22:22PM +0200, Abel Vesa wrote:
> > Instead of relying on different compatibles for eDP and DP, lookup
> > the panel node in devicetree to figure out the connector type and
> > then pass on that information to the PHY. External DP is not described
> > in DT, therefore, assume it's eDP if panel node is present.
> > 
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > ---
> >  drivers/gpu/drm/msm/dp/dp_display.c | 43 +++++++++++++++++++++++++++++++++----
> >  1 file changed, 39 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > index c4cb82af5c2f..c9763f77c832 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -726,6 +726,14 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
> >  	if (IS_ERR(phy))
> >  		return PTR_ERR(phy);
> >  
> > +	rc = phy_set_mode_ext(phy, PHY_MODE_DP,
> > +			      dp->dp_display.is_edp ? PHY_SUBMODE_EDP : PHY_SUBMODE_DP);
> > +	if (rc) {
> > +		DRM_ERROR("failed to set phy submode, rc = %d\n", rc);
> > +		dp->catalog = NULL;
> > +		goto error;
> > +	}
> > +
> >  	dp->catalog = dp_catalog_get(dev);
> >  	if (IS_ERR(dp->catalog)) {
> >  		rc = PTR_ERR(dp->catalog);
> > @@ -734,9 +742,7 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
> >  		goto error;
> >  	}
> >  
> > -	dp->aux = dp_aux_get(dev, dp->catalog,
> > -			     phy,
> > -			     dp->dp_display.is_edp);
> > +	dp->aux = dp_aux_get(dev, dp->catalog, phy, dp->dp_display.is_edp);
> >  	if (IS_ERR(dp->aux)) {
> >  		rc = PTR_ERR(dp->aux);
> >  		DRM_ERROR("failed to initialize aux, rc = %d\n", rc);
> > @@ -1241,6 +1247,35 @@ static int dp_auxbus_done_probe(struct drm_dp_aux *aux)
> >  	return dp_display_probe_tail(aux->dev);
> >  }
> >  
> > +static int dp_display_get_connector_type(struct platform_device *pdev,
> > +					 const struct msm_dp_desc *desc)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *aux_bus;
> > +	struct device_node *panel;
> > +	int ret = DRM_MODE_CONNECTOR_DisplayPort;
> > +
> > +	/* legacy platforms specify connector type in match data */
> > +	if (desc->connector_type == DRM_MODE_CONNECTOR_eDP ||
> > +		desc->connector_type == DRM_MODE_CONNECTOR_DisplayPort)
> > +		return desc->connector_type;
> > +
> > +	aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
> > +	if (!aux_bus)
> > +		goto out;
> 
> My compiler warns that if we take this code path, then you will
> of_node_put(<uninitialized panel>) below.
> 
> > +
> > +	panel = of_get_child_by_name(aux_bus, "panel");
> > +	if (!panel)
> > +		goto out;
> > +
> > +	ret = DRM_MODE_CONNECTOR_eDP;
> 
> My brain read this function as:
> check something
> if (error)
>   bailout!
> 
> check something
> if (error)
>   bailout!
> 
> ret should be edp
> 
> I then have to scan the code again to figure out what ret is otherwise,
> and convince myself that the error path is never an error, but a totally
> normal case.
> 
> 
> If you instead rely on the fact that both of_get_child_by_name() and
> of_node_put() can be passed NULL, you can write this as:
> 
> static int fn(..) {
>   aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
>   panel = of_get_child_by_name(aux_bus, "panel");
> 
>   if (panel)
>     connector_type = DRM_MODE_CONNECTOR_eDP;
>   else
>     connector_type = DRM_MODE_CONNECTOR_DisplayPort;
> 
>   of_node_put(panel);
>   of_node_put(aux_bus);
> 
>   return connector_type;
> }
> 
> Much easier to read, and you don't even have to zero-initialize panel to
> avoid that compiler warning.
> 

Fair enough, will do that instead.

> Regards,
> Bjorn
> 
> > +
> > +out:
> > +	of_node_put(panel);
> > +	of_node_put(aux_bus);
> > +	return ret;
> > +}
> > +
> >  static int dp_display_probe(struct platform_device *pdev)
> >  {
> >  	int rc = 0;
> > @@ -1263,7 +1298,7 @@ static int dp_display_probe(struct platform_device *pdev)
> >  	dp->dp_display.pdev = pdev;
> >  	dp->name = "drm_dp";
> >  	dp->id = desc->id;
> > -	dp->dp_display.connector_type = desc->connector_type;
> > +	dp->dp_display.connector_type = dp_display_get_connector_type(pdev, desc);
> >  	dp->wide_bus_supported = desc->wide_bus_supported;
> >  	dp->dp_display.is_edp =
> >  		(dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP);
> > 
> > -- 
> > 2.34.1
> >
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index c4cb82af5c2f..c9763f77c832 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -726,6 +726,14 @@  static int dp_init_sub_modules(struct dp_display_private *dp)
 	if (IS_ERR(phy))
 		return PTR_ERR(phy);
 
+	rc = phy_set_mode_ext(phy, PHY_MODE_DP,
+			      dp->dp_display.is_edp ? PHY_SUBMODE_EDP : PHY_SUBMODE_DP);
+	if (rc) {
+		DRM_ERROR("failed to set phy submode, rc = %d\n", rc);
+		dp->catalog = NULL;
+		goto error;
+	}
+
 	dp->catalog = dp_catalog_get(dev);
 	if (IS_ERR(dp->catalog)) {
 		rc = PTR_ERR(dp->catalog);
@@ -734,9 +742,7 @@  static int dp_init_sub_modules(struct dp_display_private *dp)
 		goto error;
 	}
 
-	dp->aux = dp_aux_get(dev, dp->catalog,
-			     phy,
-			     dp->dp_display.is_edp);
+	dp->aux = dp_aux_get(dev, dp->catalog, phy, dp->dp_display.is_edp);
 	if (IS_ERR(dp->aux)) {
 		rc = PTR_ERR(dp->aux);
 		DRM_ERROR("failed to initialize aux, rc = %d\n", rc);
@@ -1241,6 +1247,35 @@  static int dp_auxbus_done_probe(struct drm_dp_aux *aux)
 	return dp_display_probe_tail(aux->dev);
 }
 
+static int dp_display_get_connector_type(struct platform_device *pdev,
+					 const struct msm_dp_desc *desc)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *aux_bus;
+	struct device_node *panel;
+	int ret = DRM_MODE_CONNECTOR_DisplayPort;
+
+	/* legacy platforms specify connector type in match data */
+	if (desc->connector_type == DRM_MODE_CONNECTOR_eDP ||
+		desc->connector_type == DRM_MODE_CONNECTOR_DisplayPort)
+		return desc->connector_type;
+
+	aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
+	if (!aux_bus)
+		goto out;
+
+	panel = of_get_child_by_name(aux_bus, "panel");
+	if (!panel)
+		goto out;
+
+	ret = DRM_MODE_CONNECTOR_eDP;
+
+out:
+	of_node_put(panel);
+	of_node_put(aux_bus);
+	return ret;
+}
+
 static int dp_display_probe(struct platform_device *pdev)
 {
 	int rc = 0;
@@ -1263,7 +1298,7 @@  static int dp_display_probe(struct platform_device *pdev)
 	dp->dp_display.pdev = pdev;
 	dp->name = "drm_dp";
 	dp->id = desc->id;
-	dp->dp_display.connector_type = desc->connector_type;
+	dp->dp_display.connector_type = dp_display_get_connector_type(pdev, desc);
 	dp->wide_bus_supported = desc->wide_bus_supported;
 	dp->dp_display.is_edp =
 		(dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP);