On 01/04/2022 02:22, Doug Anderson wrote:
Hi,
On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti
<quic_sbillaka@xxxxxxxxxxx> 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
