Hi Laurent,
Thank you for your feedback!
> From: linux-renesas-soc-owner@xxxxxxxxxxxxxxx <linux-renesas-soc-owner@xxxxxxxxxxxxxxx> On Behalf Of Laurent Pinchart
> Sent: 13 December 2019 21:06
> Subject: Re: [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@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.
Right, thank you for spotting this!
>
> > + 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.
I agree
>
> > + 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().
> /
I think this will work. Thank you for the suggestion
>
> > + }
> > +
> > + 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.
And I thought I double checked those... :)
>
> > + * 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/
Cheers
Will fix the highlighted issues in v5.
Thanks,
Fab
>
> 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
