Re: [RFC PATCH v2 3/5] drm/msm/dp: support finding next bridge even for DP interfaces

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

On 2/24/2022 12:49 PM, Dmitry Baryshkov wrote:
> On Thu, 24 Feb 2022 at 23:13, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 2/11/2022 2:40 PM, Dmitry Baryshkov wrote:
>>> It is possible to supply display-connector (bridge) to the DP interface,
>>> add support for parsing it too.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/dp/dp_parser.c | 19 ++++++++++++-------
>>>    1 file changed, 12 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
>>> index 901d7967370f..1056b8d5755b 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
>>> @@ -301,17 +301,22 @@ static int dp_parser_parse(struct dp_parser *parser, int connector_type)
>>>                return rc;
>>>
>>>        /*
>>> -      * Currently we support external bridges only for eDP connectors.
>>> +      * External bridges are mandatory for eDP interfaces: one has to
>>> +      * provide at least an eDP panel (which gets wrapped into panel-bridge).
>>>         *
>>> -      * No external bridges are expected for the DisplayPort connector,
>>> -      * it is physically present in a form of a DP or USB-C connector.
>>> +      * For DisplayPort interfaces external bridges are optional, so
>>> +      * silently ignore an error if one is not present (-ENODEV).
>>>         */
>>> -     if (connector_type == DRM_MODE_CONNECTOR_eDP) {
>>> -             rc = dp_parser_find_next_bridge(parser);
>>> -             if (rc) {
>>> -                     DRM_ERROR("DP: failed to find next bridge\n");
>>> +     rc = dp_parser_find_next_bridge(parser);
>>> +     if (rc == -ENODEV) {
>>> +             if (connector_type == DRM_MODE_CONNECTOR_eDP) {
>>> +                     DRM_ERROR("eDP: next bridge is not present\n");
>>>                        return rc;
>>>                }
>>> +     } else if (rc) {
>>> +             if (rc != -EPROBE_DEFER)
>>> +                     DRM_ERROR("DP: error parsing next bridge: %d\n", rc);
>>> +             return rc;
>>>        }
>>
>> How is this silently ignoring?
>>
>> static int dp_display_bind(struct device *dev, struct device *master,
>>                  void *data)
>> {
>>       int rc = 0;
>>       struct dp_display_private *dp = dev_get_dp_display_private(dev);
>>       struct msm_drm_private *priv = dev_get_drvdata(master);
>>       struct drm_device *drm = priv->dev;
>>
>>       dp->dp_display.drm_dev = drm;
>>       priv->dp[dp->id] = &dp->dp_display;
>>
>>       rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
>>       if (rc) {
>>           DRM_ERROR("device tree parsing failed\n");
>>           goto end;
>>       }
>>
>> dp_display_bind will still fail if a bridge is not found.
>>
>> If supplying a bridge is optional even this should succeed right?
> 
> It will succeed as dp_parser_parse() will not return -ENODEV if the
> connector is not eDP.
> To rephrase the comment:
> For the dp_parser_find_next_bridge() result:
> - for eDP the driver passes all errors to the calling function.
> - for DP the driver ignores -ENODEV (no external bridge is supplied),
> but passes all other errors (which can mean e.g. that the bridge is
> not properly declared or that it did hasn't been probed yet).
> 

Ah okay, I just noticed that dp_parser_parse() returns 0 by default and 
not rc.

So in this case it will still return 0.

Hence this change LGTM,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>