Re: [PATCH 4/4] drm/rockchip: use for_each_endpoint_of_node macro, drop endpoint reference on break

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Daniel,

thank you for the review!

Am Dienstag, den 24.02.2015, 11:13 +0800 schrieb Daniel Kurtz:
> > @@ -375,18 +375,15 @@ int rockchip_drm_encoder_get_mux_id(struct device_node *node,
> >         if (!node || !crtc)
> >                 return -EINVAL;
> >
> > -       do {
> > -               ep = of_graph_get_next_endpoint(node, ep);
> > -               if (!ep)
> > -                       break;
> > -
> > +       for_each_endpoint_of_node(node, ep) {
> >                 port = of_graph_get_remote_port(ep);
> >                 of_node_put(port);
> 
> Shouldn't we put port after comparing it to crtc->port?

We don't dereference the pointer. It is only compared to crtc->port in
the line below, so this should be fine.

> This looks like an existing issue, though so this patch is (assuming
> the series [0] is merged):
> 
> Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>
>
> [0] https://lkml.org/lkml/2015/2/23/115
> 
> >
> >                 if (port == crtc->port) {
> >                         ret = of_graph_parse_endpoint(ep, &endpoint);
> > +                       of_node_put(ep);
> >                         return ret ?: endpoint.id;
> 
> This function looks really similar to imx_drm_encoder_get_mux_id().

Good point.

> The only difference is that it looks up endpoint.id rather than endpoint.port.
>
> Is there any way we can resolve this slight difference so that both
> can use a single common helper?

The way I see it, the ports describe the physical inputs to the mux, so
the port number should be used to determine the mux setting. In theory
there could be multiple endpoints (multiple sources on the same bus
connected to a single mux input), so I wouldn't like to change imx-drm
to determine the mux setting from endpoint ids.
Could we change the rockchip driver to use the port id instead?

> Or, at least move both of them to somewhere more generic?

There's drm_of_find_possible_crtcs already, we could add
drm_of_encoder_get_port_id and possibly drm_of_encoder_get_endpoint_id
next to it.

regards
Philipp

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel