Dear Nicolas, Thanks for the review. On Fri, 2022-11-25 at 17:24 -0500, Nícolas F. R. A. Prado wrote: > On Mon, Nov 07, 2022 at 03:24:12PM +0800, Nancy.Lin wrote: > > Add drm ovl_adaptor sub driver. Bring up ovl_adaptor sub driver if > > the component exists in the path. > > > > Signed-off-by: Nancy.Lin <nancy.lin@xxxxxxxxxxxx> > > Reviewed-by: AngeloGioacchino Del Regno < > > angelogioacchino.delregno@xxxxxxxxxxxxx> > > Reviewed-by: CK Hu <ck.hu@xxxxxxxxxxxx> > > Tested-by: AngeloGioacchino Del Regno < > > angelogioacchino.delregno@xxxxxxxxxxxxx> > > Tested-by: Bo-Chen Chen <rex-bc.chen@xxxxxxxxxxxx> > > Tested-by: Nícolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxx> > > --- > > [..] > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > > index 30dcb65d8a5a..ce5617ad04cb 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > > [..] > > @@ -897,22 +906,18 @@ int mtk_drm_crtc_create(struct drm_device > > *drm_dev, > > crtc_i++; > > > > for (i = 0; i < path_len; i++) { > > - enum mtk_ddp_comp_id comp_id = path[i]; > > + unsigned int comp_id = path[i]; > > struct device_node *node; > > - struct mtk_ddp_comp *comp; > > > > node = priv->comp_node[comp_id]; > > - comp = &priv->ddp_comp[comp_id]; > > - > > - if (!node) { > > - dev_info(dev, > > - "Not creating crtc %d because > > component %d is disabled or missing\n", > > - crtc_i, comp_id); > > - return 0; > > - } > > > > - if (!comp->dev) { > > - dev_err(dev, "Component %pOF not > > initialized\n", node); > > + /* Not all drm components have a DTS device node, such > > as ovl_adaptor, > > + * which is the drm bring up sub driver > > + */ > > + if (!node && comp_id != DDP_COMPONENT_DRM_OVL_ADAPTOR) > > { > > + dev_err(dev, > > + "Not creating crtc %d because component > > %d is disabled, missing or not initialized\n", > > + crtc_i, comp_id); > > return -ENODEV; > > Why do you change the behavior here? If !node, the return should be > 0, because > there are two separate data streams, for internal and external > display, and they > are optional. It should be possible to for example have the nodes for > external > display disabled in DT and still have the driver probe and have a > working > internal display. But with this change you're breaking that. Also, > this message > should stay as dev_info and not mention "not initialized", so > basically it > should stay the same as before the change. > > Thanks, > Nícolas Yes, You are right. This is my mistake. I should not change this behavior. I will fix it and modify as following for the ovl_adaptor sub driver component, which don't have dts device node. @@ -905,7 +914,10 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev, node = priv->comp_node[comp_id]; comp = &priv->ddp_comp[comp_id]; - if (!node) { + /* Not all drm components have a DTS device node, such as ovl_adaptor, + * which is the drm bring up sub driver + */ + if (!node && comp_id != DDP_COMPONENT_DRM_OVL_ADAPTOR) { dev_info(dev, "Not creating crtc %d because component %d is disabled or missing\n", crtc_i, comp_id); @@ -938,7 +950,7 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev, } Regards, Nancy