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

[Fabrizio Castro]

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