| Message ID | 1567017402-5895-4-git-send-email-fabrizio.castro@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | Add dual-LVDS panel support to EK874 | expand |
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). > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > --- > 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); } > +} > + > +/** > + * 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 ? > + * > + * 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 */ We could also add -EPIPE as a return code for the case where port1 or port2 are not connected. > + * > + * 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. > + 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. > + > + if (!p1 || !p2) > + return ret; You can return -EINVAL directly. > + if (of_property_read_u32(p1, "reg", ®_p1) || > + of_property_read_u32(p2, "reg", ®_p2)) > + return ret; Same here. > + 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. 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 ? 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. > +} > +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 ? > + > +#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__ */
On Thu, Nov 07, 2019 at 09:26:21PM +0200, Laurent Pinchart wrote: > 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). Also the kerneldoc needs to be pulled into the templates under Documentation/gpu. And since it's just one function, why not put this into drm_of.c? Gets rid of a pile of overhead. > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > > > --- > > 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 DRM core is supposed to be MIT. -Daniel > > +#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); > } > > > +} > > + > > +/** > > + * 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 ? > > > + * > > + * 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 > */ > > We could also add -EPIPE as a return code for the case where port1 or > port2 are not connected. > > > + * > > + * 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. > > > + 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. > > > + > > + if (!p1 || !p2) > > + return ret; > > You can return -EINVAL directly. > > > > + if (of_property_read_u32(p1, "reg", ®_p1) || > > + of_property_read_u32(p2, "reg", ®_p2)) > > + return ret; > > Same here. > > > + 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. > > 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 ? > > 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. > > > +} > > +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 ? > > > + > > +#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
Hi Laurent, Thank you for your feedback! > From: linux-kernel-owner@vger.kernel.org <linux-kernel-owner@vger.kernel.org> 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@bp.renesas.com> > > > > --- > > 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
Hi Daniel, Thank you for your feedback! > From: Daniel Vetter <daniel.vetter@ffwll.ch> On Behalf Of Daniel Vetter > Sent: 07 November 2019 19:30 > Subject: Re: [PATCH v3 3/8] drm: Add bus timings helper > > On Thu, Nov 07, 2019 at 09:26:21PM +0200, Laurent Pinchart wrote: > > 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). > > Also the kerneldoc needs to be pulled into the templates under > Documentation/gpu. And since it's just one function, why not put this into > drm_of.c? Gets rid of a pile of overhead. Yeah, you are right, will try and pull this into drm_of.c in v4. Thanks! Fab > > > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > > > > > --- > > > 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 > > DRM core is supposed to be MIT. > -Daniel > > > > +#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); > > } > > > > > +} > > > + > > > +/** > > > + * 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 ? > > > > > + * > > > + * 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 > > */ > > > > We could also add -EPIPE as a return code for the case where port1 or > > port2 are not connected. > > > > > + * > > > + * 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. > > > > > + 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. > > > > > + > > > + if (!p1 || !p2) > > > + return ret; > > > > You can return -EINVAL directly. > > > > > > > + if (of_property_read_u32(p1, "reg", ®_p1) || > > > + of_property_read_u32(p2, "reg", ®_p2)) > > > + return ret; > > > > Same here. > > > > > + 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. > > > > 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 ? > > > > 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. > > > > > +} > > > +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 ? > > > > > + > > > +#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 > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
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; +} + +/** + * drm_of_lvds_get_dual_link_configuration - get the dual-LVDS configuration + * @p1: device tree node corresponding to the first port of the source + * @p2: device tree node corresponding to the second port of the source + * + * An LVDS dual-link bus is made of two connections, even pixels transit on one + * connection, and odd pixels transit on the other connection. + * 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). + * + * 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; + struct device_node *ep1 = NULL, *ep2 = NULL; + u32 reg_p1, reg_p2; + int ret = -EINVAL, remote_p1_pt, remote_p2_pt; + + if (!p1 || !p2) + return ret; + if (of_property_read_u32(p1, "reg", ®_p1) || + of_property_read_u32(p2, "reg", ®_p2)) + return ret; + 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; + 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; +} +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 + +#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__ */
Helper to provide bus timing information. Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> --- 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