lkml.org 
[lkml]   [2019]   [Dec]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v4 1/7] drm: of: Add drm_of_lvds_get_dual_link_pixel_order
    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@bp.renesas.com>
    >
    > ---
    > 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@ideasonboard.com>

    > + */
    > +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

    \
     
     \ /
      Last update: 2019-12-13 22:06    [W:7.016 / U:0.056 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site