[PATCH v3 1/2] drm/fsl-dcu: rework codes to support of_graph dt binding for panel

[Stefan Agner]

Hi Meng,

This currently does not apply on top of drm-next, can you please rebase?

Some more comments below:

On 2016-07-14 03:54, Meng Yi wrote:
> This patch rework the output code to add of_graph dt binding support
> for panel device and also keeps the backward compatibility
> 
> Signed-off-by: Meng Yi <meng.yi at nxp.com>
> ---
> Changes in V3:
> -simplify return value statements
> Changes in V2:
> -fix some coding style issue
> -add fsl_dev->connector.panel check
> -use fsl_dev->np and drop fsl_dev->dev->of_node
> -return 'ret' when fsl_dcu_attach_panel failed
> ---
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c    |  2 +-
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_output.h |  3 +-
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c    | 73 ++++++++++++++++++++--------
>  3 files changed, 55 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c
> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c
> index c564ec6..b48ffa7 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c
> @@ -43,7 +43,7 @@ int fsl_dcu_drm_modeset_init(struct
> fsl_dcu_drm_device *fsl_dev)
>  	if (ret)
>  		goto fail_encoder;
>  
> -	ret = fsl_dcu_drm_connector_create(fsl_dev, &fsl_dev->encoder);
> +	ret = fsl_dcu_create_outputs(fsl_dev);
>  	if (ret)
>  		goto fail_connector;
>  
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_output.h
> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_output.h
> index 7093109..5a7b88e 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_output.h
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_output.h
> @@ -25,9 +25,8 @@ to_fsl_dcu_connector(struct drm_connector *con)
>  		     : NULL;
>  }
>  
> -int fsl_dcu_drm_connector_create(struct fsl_dcu_drm_device *fsl_dev,
> -				 struct drm_encoder *encoder);
>  int fsl_dcu_drm_encoder_create(struct fsl_dcu_drm_device *fsl_dev,
>  			        struct drm_crtc *crtc);
> +int fsl_dcu_create_outputs(struct fsl_dcu_drm_device *fsl_dev);
>  
>  #endif /* __FSL_DCU_DRM_CONNECTOR_H__ */
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c
> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c
> index 98c998d..2e71f4b 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c
> @@ -10,6 +10,7 @@
>   */
>  
>  #include <linux/backlight.h>
> +#include <linux/of_graph.h>
>  
>  #include <drm/drmP.h>
>  #include <drm/drm_atomic_helper.h>
> @@ -141,12 +142,12 @@ static const struct drm_connector_helper_funcs
> connector_helper_funcs = {
>  	.mode_valid = fsl_dcu_drm_connector_mode_valid,
>  };
>  
> -int fsl_dcu_drm_connector_create(struct fsl_dcu_drm_device *fsl_dev,
> -				 struct drm_encoder *encoder)
> +static int fsl_dcu_attach_panel(struct fsl_dcu_drm_device *fsl_dev,
> +				 struct drm_panel *panel)
>  {
> +	struct drm_encoder *encoder = &fsl_dev->encoder;
>  	struct drm_connector *connector = &fsl_dev->connector.base;
>  	struct drm_mode_config *mode_config = &fsl_dev->drm->mode_config;
> -	struct device_node *panel_node;
>  	int ret;
>  
>  	fsl_dev->connector.encoder = encoder;
> @@ -170,21 +171,7 @@ int fsl_dcu_drm_connector_create(struct
> fsl_dcu_drm_device *fsl_dev,
>  				      mode_config->dpms_property,
>  				      DRM_MODE_DPMS_OFF);
>  
> -	panel_node = of_parse_phandle(fsl_dev->np, "fsl,panel", 0);
> -	if (!panel_node) {
> -		dev_err(fsl_dev->dev, "fsl,panel property not found\n");
> -		ret = -ENODEV;
> -		goto err_sysfs;
> -	}
> -
> -	fsl_dev->connector.panel = of_drm_find_panel(panel_node);
> -	if (!fsl_dev->connector.panel) {
> -		ret = -EPROBE_DEFER;
> -		goto err_panel;
> -	}
> -	of_node_put(panel_node);
> -
> -	ret = drm_panel_attach(fsl_dev->connector.panel, connector);
> +	ret = drm_panel_attach(panel, connector);
>  	if (ret) {
>  		dev_err(fsl_dev->dev, "failed to attach panel\n");
>  		goto err_sysfs;
> @@ -192,11 +179,57 @@ int fsl_dcu_drm_connector_create(struct
> fsl_dcu_drm_device *fsl_dev,
>  
>  	return 0;
>  
> -err_panel:
> -	of_node_put(panel_node);
>  err_sysfs:
>  	drm_connector_unregister(connector);
>  err_cleanup:
>  	drm_connector_cleanup(connector);
>  	return ret;
>  }
> +
> +static int fsl_dcu_attach_endpoint(struct fsl_dcu_drm_device *fsl_dev,
> +				    const struct of_endpoint *ep)
> +{
> +	struct device_node *np;
> +	int ret;
> +
> +	np = of_graph_get_remote_port_parent(ep->local_node);
> +
> +	fsl_dev->connector.panel = of_drm_find_panel(np);
> +	of_node_put(np);
> +	if (fsl_dev->connector.panel) {
> +		ret = fsl_dcu_attach_panel(fsl_dev, fsl_dev->connector.panel);
> +		return ret;

Why put it in a variable? That is confusing, just return like this:

return fsl_dcu_attach_panel(fsl_dev, fsl_dev->connector.panel);


> +	}
> +
> +	return -ENODEV;
> +}
> +
> +int fsl_dcu_create_outputs(struct fsl_dcu_drm_device *fsl_dev)
> +{
> +	struct of_endpoint ep;
> +	struct device_node *ep_node, *panel_node;
> +	int ret;
> +
> +	/* This is for backward compatibility */
> +	panel_node = of_parse_phandle(fsl_dev->np, "fsl,panel", 0);
> +	if (panel_node) {
> +		fsl_dev->connector.panel = of_drm_find_panel(panel_node);
> +		of_node_put(panel_node);
> +		if (!fsl_dev->connector.panel)
> +			return -EPROBE_DEFER;
> +		ret = fsl_dcu_attach_panel(fsl_dev, fsl_dev->connector.panel);
> +		return ret;

Same here

> +	}
> +
> +	ep_node = of_graph_get_next_endpoint(fsl_dev->np, NULL);
> +	if (!ep_node)
> +		return -ENODEV;
> +
> +	ret = of_graph_parse_endpoint(ep_node, &ep);
> +	of_node_put(ep_node);
> +	if (ret)
> +		return -ENODEV;
> +
> +	ret = fsl_dcu_attach_endpoint(fsl_dev, &ep);
> +	return ret;

And here.

Also did you see my other email about the Documentation changes? Please
fold that in this patch, so we have the code change and the
corresponding documentation change in one patch.

--
Stefan


> +}