Re: [PATCH v4 1/7] drm: of: Add drm_of_lvds_get_dual_link_pixel_order

[Laurent Pinchart]

Hi Fabrizio,

Thank you for the patch.

On Fri, Dec 06, 2019 at 04:32:48PM +0000, Fabrizio Castro wrote:
> An LVDS dual-link connection is made of two links, with even
> pixels transitting on one link, and odd pixels on the other
> link. The device tree can be used to fully describe dual-link
> LVDS connections between encoders and bridges/panels.
> The sink of an LVDS dual-link connection is made of two ports,
> the corresponding OF graph port nodes can be marked
> with either dual-lvds-even-pixels or dual-lvds-odd-pixels,
> and that fully describes an LVDS dual-link connection,
> including pixel order.
> 
> drm_of_lvds_get_dual_link_pixel_order is a new helper
> added by this patch, given the source port nodes it
> returns DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS if the source
> port nodes belong to an LVDS dual-link connection, with even
> pixels expected to be generated from the first port, and odd
> pixels expected to be generated from the second port.
> If the new helper returns DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS,
> odd pixels are expected to be generated from the first port,
> and even pixels from the other port.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@xxxxxxxxxxxxxx>
> 
> ---
> v3->v4:
> * The patch had title "drm: Add bus timings helper" in v3
> * The code has now been moved to drm_of, and has been fully
>   restructured, thanks to Laurent and Daniel for the comments
> 
> v2->v3:
> * new patch
> ---
>  drivers/gpu/drm/drm_of.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_of.h     |  20 +++++++++
>  2 files changed, 124 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 0ca5880..c2e9ab7 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -274,3 +274,107 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
> +
> +enum drm_of_lvds_pixels {
> +	DRM_OF_LVDS_EVEN = BIT(0),
> +	DRM_OF_LVDS_ODD = BIT(1),
> +};
> +
> +static int drm_of_lvds_get_port_pixels_type(struct device_node *port_node)
> +{
> +	bool even_pixels =
> +		of_property_read_bool(port_node, "dual-lvds-even-pixels");
> +	bool odd_pixels =
> +		of_property_read_bool(port_node, "dual-lvds-odd-pixels");
> +
> +	return (even_pixels ? DRM_OF_LVDS_EVEN : 0) |
> +	       (odd_pixels ? DRM_OF_LVDS_ODD : 0);
> +}
> +
> +static int drm_of_lvds_get_remote_pixels_type(
> +			const struct device_node *port_node)
> +{
> +	struct device_node *endpoint = NULL;
> +	int pixels_type = -EPIPE;
> +
> +	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)

You need an of_node_put(endpoint) in the code paths that exit from the
loop.

> +			return -EPIPE;
> +
> +		current_pt = drm_of_lvds_get_port_pixels_type(remote_port);
> +		of_node_put(remote_port);
> +		if (!pixels_type)
> +			pixels_type = current_pt;

This will never happen as pixels_type is initialized to -EPIPE.
Replacing the condition with if (pixels_type < 0) should fix it.

> +		if (!current_pt || pixels_type != current_pt)
> +			return -EINVAL;

I would add a comment to explain this. If I understand the code
correcty, something along the lines of

		/*
		 * 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().
		 /

> +	}
> +
> +	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
> + *
> + * 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
> + * returns, for two ports of an LVDS dual-link source, which port shall transmit
> + * the even and odd pixels, based on the requirements of the connected sink.
> + *
> + * The pixel order is determined from the dual-lvds-even-pixels and
> + * dual-lvds-odd-pixels properties in the sink's DT port nodes. If those
> + * properties are not present, or if their usage is not valid, this function
> + * returns -EINVAL.
> + *
> + * 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.
> + *
> + * Return:
> + * * DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS - @port1 carries even pixels and @port2
> + *   carries odd pixels
> + * * DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS - @port1 carries odd pixels and @port1

This should be DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS, and the second @port1
should be @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

s/port2/@port2/

With those small issues addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>

> + */
> +int drm_of_lvds_get_dual_link_pixel_order(const struct device_node *port1,
> +					  const struct device_node *port2)
> +{
> +	int remote_p1_pt, remote_p2_pt;
> +
> +	if (!port1 || !port2)
> +		return -EINVAL;
> +
> +	remote_p1_pt = drm_of_lvds_get_remote_pixels_type(port1);
> +	if (remote_p1_pt < 0)
> +		return remote_p1_pt;
> +
> +	remote_p2_pt = drm_of_lvds_get_remote_pixels_type(port2);
> +	if (remote_p2_pt < 0)
> +		return remote_p2_pt;
> +
> +	/*
> +	 * A valid dual-lVDS bus is found when one remote port is marked with
> +	 * "dual-lvds-even-pixels", and the other remote port is marked with
> +	 * "dual-lvds-odd-pixels", bail out if the markers are not right.
> +	 */
> +	if (remote_p1_pt + remote_p2_pt != DRM_OF_LVDS_EVEN + DRM_OF_LVDS_ODD)
> +		return -EINVAL;
> +
> +	return remote_p1_pt == DRM_OF_LVDS_EVEN ?
> +		DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS :
> +		DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS;
> +}
> +EXPORT_SYMBOL_GPL(drm_of_lvds_get_dual_link_pixel_order);
> diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
> index ead34ab..8ec7ca6 100644
> --- a/include/drm/drm_of.h
> +++ b/include/drm/drm_of.h
> @@ -16,6 +16,18 @@ struct drm_panel;
>  struct drm_bridge;
>  struct device_node;
>  
> +/**
> + * enum drm_lvds_dual_link_pixels - Pixel order of an LVDS dual-link connection
> + * @DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS: Even pixels are expected to be generated
> + *    from the first port, odd pixels from the second port
> + * @DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS: Odd pixels are expected to be generated
> + *    from the first port, even pixels from the second port
> + */
> +enum drm_lvds_dual_link_pixels {
> +	DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS = 0,
> +	DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS = 1,
> +};
> +
>  #ifdef CONFIG_OF
>  uint32_t drm_of_crtc_port_mask(struct drm_device *dev,
>  			    struct device_node *port);
> @@ -35,6 +47,8 @@ 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);
>  #else
>  static inline uint32_t drm_of_crtc_port_mask(struct drm_device *dev,
>  					  struct device_node *port)
> @@ -77,6 +91,12 @@ static inline int drm_of_find_panel_or_bridge(const struct device_node *np,
>  {
>  	return -EINVAL;
>  }
> +
> +int drm_of_lvds_get_dual_link_pixel_order(const struct device_node *port1,
> +					  const struct device_node *port2)
> +{
> +	return -EINVAL;
> +}
>  #endif
>  
>  /*

-- 
Regards,

Laurent Pinchart