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
