Re: [PATCH/RFC v3 06/19] video: display: OF support

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Philipp,

On Tuesday 13 August 2013 16:37:07 Philipp Zabel wrote:
> Hi Laurent,
> 
> thanks for this update. I'm very happy about the move to the display entity
> model, given that the i.MX6 IPU has both drm/display and v4l2/capture ports
> in a single device - this will allow to use a unified device tree binding
> scheme.

Thanks for the support :-)

> I'm still trying to see how this all fits together, so far I have only one
> comment, below.
> 
> Am Freitag, den 09.08.2013, 19:14 +0200 schrieb Laurent Pinchart:
> [...]
> 
> > +static int display_of_parse_dt(struct display_entity_notifier *notifier,
> > +			       struct list_head *entities,
> > +			       struct device_node *node)
> > +{
> > +	struct display_entity_of *entity;
> > +	struct device_node *remote;
> > +	struct device_node *ep = NULL;
> > +	struct device_node *next;
> > +	unsigned int num_entities = 0;
> > +	int ret = 0;
> > +
> > +	/* Walk the device tree and build a list of nodes. */
> > +	dev_dbg(notifier->dev, "parsing node %s\n", node->full_name);
> > +
> > +	while (1) {
> > +		next = display_of_get_next_endpoint(node, ep);
> > +		if (next == NULL)
> > +			break;
> > +
> > +		of_node_put(ep);
> > +		ep = next;
> > +
> > +		dev_dbg(notifier->dev, "handling endpoint %s\n", ep->full_name);
> > +
> > +		remote = display_of_get_remote_port_parent(ep);
> > +		if (remote == NULL)
> > +			continue;
> > +
> > +		/* Skip entities that we have already processed. */
> > +		if (display_of_find_entity(entities, remote) || remote == node) {
> > +			dev_dbg(notifier->dev,
> > +				"entity %s already in list, skipping\n",
> > +				remote->full_name);
> > +			continue;
> > +		}
> 
> device tree nodes with status = "disabled" should be skipped here:
> 
>                 if (!of_device_is_available(remote)) {
>                         dev_dbg(notifier->dev,
>                                 "entity %s is disabled, skipping\n",
>                                 remote->full_name);
>                         continue;
>                 }
> 
> Otherwise the completion notification will never be delivered if there
> are any disabled entities in the graph.

That's a good point, but if a device is disabled, why would it be in the DT 
graph in the first place ? Do you have a use case for this ?

> > +		entity = kzalloc(sizeof(*entity), GFP_KERNEL);
> > +		if (entity == NULL) {
> > +			of_node_put(remote);
> > +			ret = -ENOMEM;
> > +			break;
> > +		}
> > +
> > +		dev_dbg(notifier->dev, "adding remote entity %s to list\n",
> > +			remote->full_name);
> > +
> > +		entity->node = remote;
> > +		list_add_tail(&entity->list, entities);
> > +		num_entities++;
> > +	}
> > +
> > +	of_node_put(ep);
> > +
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return num_entities;
> > +}
> 
> [...]

-- 
Regards,

Laurent Pinchart