<pre>
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@mediatek.com>
> > Reviewed-by: AngeloGioacchino Del Regno <
> > angelogioacchino.delregno@collabora.com>
> > Reviewed-by: CK Hu <ck.hu@mediatek.com>
> > Tested-by: AngeloGioacchino Del Regno <
> > angelogioacchino.delregno@collabora.com>
> > Tested-by: Bo-Chen Chen <rex-bc.chen@mediatek.com>
> > Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > ---
>
> [..]
> > 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


</pre><!--type:text--><!--{--><pre>************* MEDIATEK Confidentiality Notice ********************
The information contained in this e-mail message (including any 
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be 
conveyed only to the designated recipient(s). Any use, dissemination, 
distribution, printing, retaining or copying of this e-mail (including its 
attachments) by unintended recipient(s) is strictly prohibited and may 
be unlawful. If you are not an intended recipient of this e-mail, or believe 
that you have received this e-mail in error, please notify the sender 
immediately (by replying to this e-mail), delete any and all copies of 
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!
</pre><!--}-->