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 at 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
