RE: [PATCH v3 3/8] drm: Add bus timings helper
From: Fabrizio Castro
Date: Fri Dec 06 2019 - 10:24:29 EST
Hi Laurent,
Thank you for your feedback!
> From: linux-kernel-owner@xxxxxxxxxxxxxxx <linux-kernel-owner@xxxxxxxxxxxxxxx> On Behalf Of Laurent Pinchart
> Sent: 07 November 2019 19:26
> Subject: Re: [PATCH v3 3/8] drm: Add bus timings helper
>
> Hi Fabrizio,
>
> Thank you for the patch.
>
> On Wed, Aug 28, 2019 at 07:36:37PM +0100, Fabrizio Castro wrote:
> > Helper to provide bus timing information.
>
> You may want to expand this a bit. And actually fix it too, as the
> helper you introduce isn't related to timings (same for the subject
> line).
I'll rework this completely
>
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@xxxxxxxxxxxxxx>
> >
> > ---
> > v2->v3:
> > * new patch
> > ---
> > drivers/gpu/drm/Makefile | 3 +-
> > drivers/gpu/drm/drm_bus_timings.c | 97 +++++++++++++++++++++++++++++++++++++++
> > include/drm/drm_bus_timings.h | 21 +++++++++
> > 3 files changed, 120 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/gpu/drm/drm_bus_timings.c
> > create mode 100644 include/drm/drm_bus_timings.h
> >
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index 9f0d2ee..a270063 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -17,7 +17,8 @@ drm-y := drm_auth.o drm_cache.o \
> > drm_plane.o drm_color_mgmt.o drm_print.o \
> > drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
> > drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
> > - drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o
> > + drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o \
> > + drm_bus_timings.o
> >
> > drm-$(CONFIG_DRM_LEGACY) += drm_legacy_misc.o drm_bufs.o drm_context.o drm_dma.o drm_scatter.o drm_lock.o
> > drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
> > diff --git a/drivers/gpu/drm/drm_bus_timings.c b/drivers/gpu/drm/drm_bus_timings.c
> > new file mode 100644
> > index 0000000..e2ecd22
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_bus_timings.c
> > @@ -0,0 +1,97 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <drm/drm_bus_timings.h>
> > +#include <linux/errno.h>
> > +#include <linux/of_graph.h>
> > +#include <linux/of.h>
> > +#include <linux/types.h>
> > +
> > +#define DRM_OF_LVDS_ODD 1
> > +#define DRM_OF_LVDS_EVEN 2
> > +
> > +static int drm_of_lvds_get_port_pixels_type(struct device_node *port_node)
> > +{
> > + bool even_pixels, odd_pixels;
> > +
> > + even_pixels = of_property_read_bool(port_node, "dual-lvds-even-pixels");
> > + odd_pixels = of_property_read_bool(port_node, "dual-lvds-odd-pixels");
> > + return even_pixels * DRM_OF_LVDS_EVEN + odd_pixels * DRM_OF_LVDS_ODD;
>
> s/ / /
>
> But I would make these bitflags.
>
> 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)
> {
> bool even_pixels = of_property_read_bool(port, "dual-lvds-even-pixels");
> bool odd_pixels = of_property_read_bool(port, "dual-lvds-odd-pixels");
>
> return (even_pixels ? DRM_OF_LVDS_EVEN : 0) |
> (odd_pixels ? DRM_OF_LVDS_ODD : 0);
> }
Will do
>
> > +}
> > +
> > +/**
> > + * drm_of_lvds_get_dual_link_configuration - get the dual-LVDS configuration
>
> Should we name this drm_of_lvds_get_dual_link_pixel_order to better
> reflect its purpose ?
>
> > + * @p1: device tree node corresponding to the first port of the source
> > + * @p2: device tree node corresponding to the second port of the source
>
> Maybe port1 and port2 to make this more explicit ?
yeah
>
> > + *
> > + * An LVDS dual-link bus is made of two connections, even pixels transit on one
> > + * connection, and odd pixels transit on the other connection.
>
> To match the DT bindings documentation, I would recommand
>
> "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 walks the DT (from the source ports to the sink ports) looking
> > + * for a dual-LVDS bus. A dual-LVDS bus is identfied by markers found on the DT
> > + * ports of the sink device(s). If such a bus is found, this function returns
> > + * its configuration (either p1 connected to the even pixels port and p2
> > + * connected to the odd pixels port, or p1 connected to the odd pixels port and
> > + * p2 connected to the even pixels port).
>
> "walking the DT" sounds like the function goes through the whole graph.
> How about the following ?
>
> /**
> * 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 off 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.
> *
> * @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
> * carries even pixels
> * * -EINVAL - @port1 and @port2 are not connected to a dual-link LVDS sink, or
> * the sink configuration is invalid
> */
Can do
>
> We could also add -EPIPE as a return code for the case where port1 or
> port2 are not connected.
Good idea, will add
>
> > + *
> > + * Return: A code describing the bus configuration when a valid dual-LVDS bus is
> > + * found, or an error code when no valid dual-LVDS bus is found
> > + *
> > + * Possible codes for the bus configuration are:
> > + *
> > + * - DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS: when p1 is connected to the even pixels
> > + * port and p2 is connected to the odd pixels port
> > + * - DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS: when p1 is connected to the odd pixels
> > + * port and p2 is connected to the even pixels port
> > + *
> > + */
> > +int drm_of_lvds_get_dual_link_configuration(const struct device_node *p1,
> > + const struct device_node *p2)
> > +{
> > + struct device_node *remote_p1 = NULL, *remote_p2 = NULL;
> > + struct device_node *parent_p1 = NULL, *parent_p2 = NULL;
>
> There's no need to initialize those two variables.
Will change
>
> > + struct device_node *ep1 = NULL, *ep2 = NULL;
> > + u32 reg_p1, reg_p2;
> > + int ret = -EINVAL, remote_p1_pt, remote_p2_pt;
>
> Please split this last line, as it otherwise hides the initialization of
> ret in the middle.
Will change
>
> > +
> > + if (!p1 || !p2)
> > + return ret;
>
> You can return -EINVAL directly.
ok
>
>
> > + if (of_property_read_u32(p1, "reg", ®_p1) ||
> > + of_property_read_u32(p2, "reg", ®_p2))
> > + return ret;
>
> Same here.
ok
>
> > + parent_p1 = of_get_parent(p1);
> > + parent_p2 = of_get_parent(p2);
> > + if (!parent_p1 || !parent_p2)
> > + goto done;
> > + ep1 = of_graph_get_endpoint_by_regs(parent_p1, reg_p1, 0);
> > + ep2 = of_graph_get_endpoint_by_regs(parent_p2, reg_p2, 0);
> > + if (!ep1 || !ep2)
> > + goto done;
>
> If you only support the first endpoint, this should be mentioned in the
> documentation. Alternatively you could pass the endpoint nodes instead
> of the port nodes, or you could pass the endpoint number.
Or alternatively I could inspect all of the remote endpoints
>
> It's also a bit inefficient to use of_graph_get_endpoint_by_regs() when
> you already have the port nodes. How about adding the following helper
> function ?
I'll implement something to walk through all of the endpoints we are connected to.
Will send something shortly for you to have a look at.
>
> struct device_node *of_graph_get_port_endpoint(struct device_node *port, int reg)
> {
> struct device_node *endpoint = NULL;
>
> for_each_child_of_node(port, endpoint) {
> u32 id;
>
> if (!of_node_name_eq(endpoint, "endpoint") ||
> continue;
>
> if (reg == -1)
> return endpoint;
>
> if (of_property_read_u32(node, "reg", &id) < 0)
> continue;
>
> if (reg == id)
> return endpoint;
> }
>
> return NULL;
> }
>
> If you're concerned that adding a core helper would delay this patch
> series, you could add it as a local helper, and move it to of_graph.h in
> a second step.
>
> > + remote_p1 = of_graph_get_remote_port(ep1);
> > + remote_p2 = of_graph_get_remote_port(ep2);
> > + if (!remote_p1 || !remote_p2)
> > + goto done;
> > + remote_p1_pt = drm_of_lvds_get_port_pixels_type(remote_p1);
> > + remote_p2_pt = drm_of_lvds_get_port_pixels_type(remote_p2);
> > + /*
> > + * 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 ||
> > + remote_p1_pt + remote_p2_pt != DRM_OF_LVDS_EVEN + DRM_OF_LVDS_ODD)
> > + goto done;
> > + if (remote_p1_pt == DRM_OF_LVDS_EVEN)
> > + /* The sink expects even pixels through the first port */
> > + ret = DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS;
> > + else
> > + /* The sink expects odd pixels through the first port */
> > + ret = DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS;
> > +
> > +done:
> > + of_node_put(ep1);
> > + of_node_put(ep2);
> > + of_node_put(parent_p1);
> > + of_node_put(parent_p2);
> > + of_node_put(remote_p1);
> > + of_node_put(remote_p2);
> > + return ret;
>
> This is heavy, I would add blank lines to make the code easier to read.
Most of this will disappear in v4
>
> > +}
> > +EXPORT_SYMBOL_GPL(drm_of_lvds_get_dual_link_configuration);
> > diff --git a/include/drm/drm_bus_timings.h b/include/drm/drm_bus_timings.h
> > new file mode 100644
> > index 0000000..db8a385
> > --- /dev/null
> > +++ b/include/drm/drm_bus_timings.h
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __DRM_BUS_TIMINGS__
> > +#define __DRM_BUS_TIMINGS__
> > +
> > +struct device_node;
> > +
> > +#define DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS 0
> > +#define DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS 1
>
> These should be documented with kerneldoc. How about also turning them
> into an enum ?
Will change
Thanks,
Fab
>
> > +
> > +#ifdef CONFIG_OF
> > +int drm_of_lvds_get_dual_link_configuration(const struct device_node *p1,
> > + const struct device_node *p2);
> > +#else
> > +int drm_of_lvds_get_dual_link_configuration(const struct device_node *p1,
> > + const struct device_node *p2)
> > +{
> > + return -EINVAL;
> > +}
> > +#endif
> > +
> > +#endif /* __DRM_BUS_TIMINGS__ */
>
> --
> Regards,
>
> Laurent Pinchart