Re: [PATCH 3/5] drm: convert drivers to use of_graph_get_remote_node

[Jyri Sarha]

Thanks Rob, for nice cleanup, but ...

On 02/04/17 05:36, Rob Herring wrote:
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 6dfdb145f3bb..e74cc236a79b 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -1013,16 +1013,7 @@ int tilcdc_crtc_create(struct drm_device *dev)
>  	drm_crtc_helper_add(crtc, &tilcdc_crtc_helper_funcs);
>  
>  	if (priv->is_componentized) {
> -		struct device_node *ports =
> -			of_get_child_by_name(dev->dev->of_node, "ports");
> -
> -		if (ports) {
> -			crtc->port = of_get_child_by_name(ports, "port");
> -			of_node_put(ports);
> -		} else {
> -			crtc->port =
> -				of_get_child_by_name(dev->dev->of_node, "port");
> -		}
> +		crtc->port = of_graph_get_port_by_id(dev->dev->of_node, 0, 0);
>  		if (!crtc->port) { /* This should never happen */
>  			dev_err(dev->dev, "Port node not found in %s\n",
>  				dev->dev->of_node->full_name);
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.c b/drivers/gpu/drm/tilcdc/tilcdc_external.c
> index c67d7cd7d57e..b7523dce4e8a 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_external.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_external.c
> @@ -187,39 +187,6 @@ int tilcdc_attach_bridge(struct drm_device *ddev, struct drm_bridge *bridge)
>  	return ret;
>  }
>  
> -static int tilcdc_node_has_port(struct device_node *dev_node)
> -{
> -	struct device_node *node;
> -
> -	node = of_get_child_by_name(dev_node, "ports");
> -	if (!node)
> -		node = of_get_child_by_name(dev_node, "port");
> -	if (!node)
> -		return 0;
> -	of_node_put(node);
> -
> -	return 1;
> -}
> -
> -static
> -struct device_node *tilcdc_get_remote_node(struct device_node *node)
> -{
> -	struct device_node *ep;
> -	struct device_node *parent;
> -
> -	if (!tilcdc_node_has_port(node))
> -		return NULL;
> -
> -	ep = of_graph_get_next_endpoint(node, NULL);
> -	if (!ep)
> -		return NULL;
> -
> -	parent = of_graph_get_remote_port_parent(ep);
> -	of_node_put(ep);
> -
> -	return parent;
> -}
> -
>  int tilcdc_attach_external_device(struct drm_device *ddev)
>  {
>  	struct tilcdc_drm_private *priv = ddev->dev_private;
> @@ -227,7 +194,7 @@ int tilcdc_attach_external_device(struct drm_device *ddev)
>  	struct drm_bridge *bridge;
>  	int ret;
>  
> -	remote_node = tilcdc_get_remote_node(ddev->dev->of_node);
> +	remote_node = of_graph_get_remote_node(ddev->dev->of_node, 0, 0);
>  	if (!remote_node)
>  		return 0;
>  
> @@ -266,35 +233,18 @@ int tilcdc_get_external_components(struct device *dev,
>  				   struct component_match **match)
>  {
>  	struct device_node *node;
> -	struct device_node *ep = NULL;
> -	int count = 0;
> -	int ret = 0;
>  
> -	if (!tilcdc_node_has_port(dev->of_node))


> +	if (!match)
>  		return 0;

This will break tilcdc on setups that use the old tilcdc internal panel-
or tfp410- support. The driver uses tilcdc_get_external_components()
with match == NULL to check if the setup uses graph binding or legacy
internal panel/tfp410 support.

My intention is to get rid off the legacy internal encoder and connector
support (I even did that once already, but DRM had lot of infrastructure
changes in that area and my changes became obsolete), but we still need it.

>  
> -	while ((ep = of_graph_get_next_endpoint(dev->of_node, ep))) {
> -		node = of_graph_get_remote_port_parent(ep);
> -		if (!node || !of_device_is_available(node)) {
> -			of_node_put(node);
> -			continue;
> -		}
> -
> -		dev_dbg(dev, "Subdevice node '%s' found\n", node->name);
> -
> -		if (of_device_is_compatible(node, "nxp,tda998x")) {
> -			if (match)
> -				drm_of_component_match_add(dev, match,
> -							   dev_match_of, node);
> -			ret = 1;
> -		}
> +	node = of_graph_get_remote_node(dev->of_node, 0, 0);
>  
> +	if (!of_device_is_compatible(node, "nxp,tda998x")) {
>  		of_node_put(node);
> -		if (count++ > 1) {
> -			dev_err(dev, "Only one port is supported\n");
> -			return -EINVAL;
> -		}
> +		return 0;
>  	}
>  
> -	return ret;

Simply remove the above mentioned if statement add and this here:
if (match)
> +	drm_of_component_match_add(dev, match, dev_match_of, node);
> +	of_node_put(node);
> +	return 1;
>  }

Best regards,
Jyri