Re: [PATCH v28 6/7] drm/mediatek: add drm ovl_adaptor sub driver for MT8195

[Nícolas F. R. A. Prado]

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