Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 01/04/2022 02:22, Doug Anderson wrote:
> Hi,
> 
> On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti
> <quic_sbillaka@quicinc.com> wrote:
>>
>> @@ -1547,6 +1593,10 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>>
>>          dp_display->encoder = encoder;
>>
>> +       ret = dp_display_get_next_bridge(dp_display);
>> +       if (ret)
>> +               return ret;
> 
> It feels weird to me that this is in a function called "modeset_init",
> though I certainly don't know the structure of the MSM display code
> well enough to fully comment.

It's called modeset_init() as it initializes KMS objects used by DP 
driver. We have similar functions for dsi and hdmi

> My expectation would have been that
> devm_of_dp_aux_populate_ep_devices() would have been called from your
> probe routine and then you would have returned -EPROBE_DEFER from your
> probe if you were unable to find the panel afterwards.

I don't think it's possible to call it from probe() since 
drm_dp_aux_register() is called only from dp_display_bind().
The PHY also isn't initialized at that moment, so we can not probe AUX 
devices.

The overall semantics of the AUX bus is not clear to me.
Typically the bus is populated (and probed) when devices are accessible. 
But for the display related buses this might not be the case.
For example for the DSI bus we clearly define that DSI transfer are not 
possible before the corresponding bridge's (or panel's) enable call.

Maybe the same approach should be adopted for the AUX bus. This would 
allow us to populate the AUX bus before hardware access is actually 
possible, thus creating all the DRM bridges before the hardware is 
actually up and running.

> Huh, but I guess you _are_ getting called (indirectly) from
> dpu_kms_hw_init() and I can't imagine AUX transfers working before
> that function is called, so maybe I should just accept that it's
> complicated and let those who understand this driver better confirm
> that it's OK. ;-)
> 
> 
>> @@ -140,5 +140,6 @@ struct dp_parser {
>>    * can be parsed using this module.
>>    */
>>   struct dp_parser *dp_parser_get(struct platform_device *pdev);
>> +int dp_parser_find_next_bridge(struct dp_parser *parser);
> 
> Everything else in this file is described w/ kerneldoc. Shouldn't your
> function also have a kerneldoc comment?

-- 
With best wishes
Dmitry