RE: [PATCH v9 1/4] drm/msm/dp: Add eDP support via aux_bus

[Sankeerth Billakanti (QUIC)]

Hi Stephen,

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

The host_init, config_hpd, phy_init and enable_irq are happening in modeset_init already for eDP.
So, I am not scheduling the EV_HPD_INIT_SETUP event for eDP. I am not modifying the delay for DP.

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

Okay

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

Okay

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

Okay

>> + *
>> + * 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
>

Okay

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