Hi Maxime,
Thank you for the patch.
On Mon, Oct 05, 2020 at 05:15:39PM +0200, Maxime Ripard wrote:
> The drm_of_lvds_get_dual_link_pixel_order() function took so far the
> device_node of the two ports used together to make up a dual-link LVDS
> output.
>
> This assumes that a binding would use an entire port for the LVDS output.
> However, some bindings have used endpoints instead and thus we need to
> operate at the endpoint level. Change slightly the arguments to allow that.
>
> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> ---
> drivers/gpu/drm/drm_of.c | 98 +++++++++++++++---------------
> drivers/gpu/drm/rcar-du/rcar_lvds.c | 8 +--
> include/drm/drm_of.h | 16 +++--
> 3 files changed, 63 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index b50b44e76279..2dcb49b0401b 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -291,50 +291,34 @@ static int drm_of_lvds_get_port_pixels_type(struct device_node *port_node)
> (odd_pixels ? DRM_OF_LVDS_ODD : 0);
> }
>
> -static int drm_of_lvds_get_remote_pixels_type(
> - const struct device_node *port_node)
> +static int drm_of_lvds_get_remote_pixels_type(const struct device_node *endpoint)
> {
> - struct device_node *endpoint = NULL;
> - int pixels_type = -EPIPE;
> + struct device_node *remote_port;
> + int pixels_type;
>
> - for_each_child_of_node(port_node, endpoint) {
> - struct device_node *remote_port;
> - int current_pt;
> -
> - if (!of_node_name_eq(endpoint, "endpoint"))
> - continue;
> -
> - remote_port = of_graph_get_remote_port(endpoint);
> - if (!remote_port) {
> - of_node_put(remote_port);
> - return -EPIPE;
> - }
> -
> - current_pt = drm_of_lvds_get_port_pixels_type(remote_port);
> + remote_port = of_graph_get_remote_port(endpoint);
> + if (!remote_port) {
> of_node_put(remote_port);
You can drop this line.
> - if (pixels_type < 0)
> - pixels_type = current_pt;
> -
> - /*
> - * Sanity check, ensure that all remote endpoints have the same
> - * pixel type. We may lift this restriction later if we need to
> - * support multiple sinks with different dual-link
> - * configurations by passing the endpoints explicitly to
> - * drm_of_lvds_get_dual_link_pixel_order().
> - */
Shouldn't we keep this check when endpoint_id is -1 in
drm_of_lvds_get_dual_link_pixel_order() ?
> - if (!current_pt || pixels_type != current_pt) {
> - of_node_put(remote_port);
> - return -EINVAL;
> - }
> + return -EPIPE;
> }
>
> + pixels_type = drm_of_lvds_get_port_pixels_type(remote_port);
> + of_node_put(remote_port);
> +
> + if (pixels_type < 0)
> + pixels_type = -EPIPE;
> +
> return pixels_type;
> }
>
> /**
> * drm_of_lvds_get_dual_link_pixel_order - Get LVDS dual-link pixel order
> - * @port1: First DT port node of the Dual-link LVDS source
> - * @port2: Second DT port node of the Dual-link LVDS source
> + * @dev1: First DT device node of the Dual-Link LVDS source
> + * @port1_id: ID of the first DT port node of the Dual-Link LVDS source
> + * @endpoint1_id: ID of the first DT port node of the Dual-Link LVDS source
The port1_id and endpoint1_id parameters have the exact same
documentation. Same for port2.
I would shorten port1_id to port1 and endpoint1_id to endpoint1, but
that's up to you.
> + * @dev2: First DT device node of the Dual-Link LVDS source
> + * @port2_id: ID of the first DT port node of the Dual-Link LVDS source
> + * @endpoint2_id: ID of the first DT port node of the Dual-Link LVDS source
> *
> * An LVDS dual-link connection is made of two links, with even pixels
> * transitting on one link, and odd pixels on the other link. This function
> @@ -348,32 +332,48 @@ static int drm_of_lvds_get_remote_pixels_type(
> *
> * If either port is not connected, this function returns -EPIPE.
> *
> - * @port1 and @port2 are typically DT sibling nodes, but may have different
> - * parents when, for instance, two separate LVDS encoders carry the even and odd
> - * pixels.
> + * @port1_id and @port2_id are typically DT sibling nodes, but may have
> + * different parents when, for instance, two separate LVDS encoders carry the
> + * even and odd pixels.
> + *
> + * If @port1_id, @port2_id, @endpoint1_id or @endpoint2_id are set to -1, their
> + * value is going to be ignored.
And what happens when they're ignored ? :-) You should document that
the first endpoint / port is then used.
> *
> * Return:
> - * * DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS - @port1 carries even pixels and @port2
> - * carries odd pixels
> - * * DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS - @port1 carries odd pixels and @port2
> - * carries even pixels
> - * * -EINVAL - @port1 and @port2 are not connected to a dual-link LVDS sink, or
> - * the sink configuration is invalid
> - * * -EPIPE - when @port1 or @port2 are not connected
> + * * DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS - @endpoint1_id carries even pixels and
> + * @endpoint2_id carries odd pixels
> + * * DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS - @endpoint1_id carries odd pixels and
> + * @endpoint2_id carries even pixels
> + * * -EINVAL - @endpoint1_id and @endpoint2_id are not connected to a dual-link
> + * LVDS sink, or the sink configuration is invalid
> + * * -EPIPE - when @endpoint1_id or @endpoint2_id are not connected
> */
> -int drm_of_lvds_get_dual_link_pixel_order(const struct device_node *port1,
> - const struct device_node *port2)
> +int drm_of_lvds_get_dual_link_pixel_order(const struct device_node *dev1,
> + int port1_id,
> + int endpoint1_id,
> + const struct device_node *dev2,
> + int port2_id,
> + int endpoint2_id)
> {
> + struct device_node *endpoint1, *endpoint2;
> int remote_p1_pt, remote_p2_pt;
>
> - if (!port1 || !port2)
> + if (!dev1 || !dev2)
> + return -EINVAL;
> +
> + endpoint1 = of_graph_get_endpoint_by_regs(dev1, port1_id, endpoint1_id);
> + if (!endpoint1)
> + return -EINVAL;
> +
> + endpoint2 = of_graph_get_endpoint_by_regs(dev2, port2_id, endpoint2_id);
> + if (!endpoint2)
> return -EINVAL;
YOu're leaking a reference to endpoint1 here, and to both endpoint1 and
endpoint2 in all the error paths (and the success path actually) below.
>
> - remote_p1_pt = drm_of_lvds_get_remote_pixels_type(port1);
> + remote_p1_pt = drm_of_lvds_get_remote_pixels_type(endpoint1);
> if (remote_p1_pt < 0)
> return remote_p1_pt;
>
> - remote_p2_pt = drm_of_lvds_get_remote_pixels_type(port2);
> + remote_p2_pt = drm_of_lvds_get_remote_pixels_type(endpoint2);
> if (remote_p2_pt < 0)
> return remote_p2_pt;
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index ab0d49618cf9..02d8c4ce820e 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -715,7 +715,6 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> {
> const struct of_device_id *match;
> struct device_node *companion;
> - struct device_node *port0, *port1;
> struct rcar_lvds *companion_lvds;
> struct device *dev = lvds->dev;
> int dual_link;
> @@ -743,11 +742,8 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> * connected to, if they are marked as expecting even pixels and
> * odd pixels than we need to enable vertical stripe output.
> */
> - port0 = of_graph_get_port_by_id(dev->of_node, 1);
> - port1 = of_graph_get_port_by_id(companion, 1);
> - dual_link = drm_of_lvds_get_dual_link_pixel_order(port0, port1);
> - of_node_put(port0);
> - of_node_put(port1);
> + dual_link = drm_of_lvds_get_dual_link_pixel_order(dev->of_node, 1, -1,
> + companion, 1, -1);
>
> switch (dual_link) {
> case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS:
> diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
> index b9b093add92e..7bb1f6603beb 100644
> --- a/include/drm/drm_of.h
> +++ b/include/drm/drm_of.h
> @@ -47,8 +47,12 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> int port, int endpoint,
> struct drm_panel **panel,
> struct drm_bridge **bridge);
> -int drm_of_lvds_get_dual_link_pixel_order(const struct device_node *port1,
> - const struct device_node *port2);
> +int drm_of_lvds_get_dual_link_pixel_order(const struct device_node *dev1,
> + int port1_id,
> + int endpoint1_id,
> + const struct device_node *dev2,
> + int port2_id,
> + int endpoint2_id);
> #else
> static inline uint32_t drm_of_crtc_port_mask(struct drm_device *dev,
> struct device_node *port)
> @@ -93,8 +97,12 @@ static inline int drm_of_find_panel_or_bridge(const struct device_node *np,
> }
>
> static inline int
> -drm_of_lvds_get_dual_link_pixel_order(const struct device_node *port1,
> - const struct device_node *port2)
> +drm_of_lvds_get_dual_link_pixel_order(const struct device_node *dev1,
> + int port1_id,
> + int endpoint1_id,
> + const struct device_node *dev2,
> + int port2_id,
> + int endpoint2_id)
> {
> return -EINVAL;
> }
--
Regards,
Laurent Pinchart
