[v2,35/49] drm: Clarify definition of the DRM_BUS_FLAG_(PIXDATA|SYNC)_* macros

Submitted by Laurent Pinchart on Jan. 11, 2019, 3:51 a.m.

Details

Message ID 20190111035120.20668-36-laurent.pinchart@ideasonboard.com
State New
Series "omapdrm: drm_bridge and drm_panel support"
Headers show

Commit Message

Laurent Pinchart Jan. 11, 2019, 3:51 a.m.
From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

The DRM_BUS_FLAG_PIXDATA_POSEDGE and DRM_BUS_FLAG_PIXDATA_NEGEDGE macros
and their DRM_BUS_FLAG_SYNC_* counterparts define on which pixel clock
edge data and sync signals are driven. They are however used in some
drivers to define on which pixel clock edge data and sync signals are
sampled, which should usually (but not always) be the opposite edge of
the driving edge. This creates confusion.

Create four new macros for both PIXDATA and SYNC that explicitly state
the driving and sampling edge in their name to remove the confusion. The
driving macros are defined as the opposite of the sampling macros to
made code simpler based on the assumption that the driving and sampling
edges are opposite.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
Changes since v1:

- Address the DRM_BUS_FLAG_SYNC_* flags
- Rebase on top of drm-next
---
 include/drm/drm_connector.h | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 9be2181b3ed7..00bb7a74962b 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -330,19 +330,47 @@  struct drm_display_info {
 
 #define DRM_BUS_FLAG_DE_LOW		(1<<0)
 #define DRM_BUS_FLAG_DE_HIGH		(1<<1)
-/* drive data on pos. edge */
+
+/*
+ * Don't use those two flags directly, use the DRM_BUS_FLAG_PIXDATA_DRIVE_*
+ * and DRM_BUS_FLAG_PIXDATA_SAMPLE_* variants to qualify the flags explicitly.
+ * The DRM_BUS_FLAG_PIXDATA_SAMPLE_* flags are defined as the opposite of the
+ * DRM_BUS_FLAG_PIXDATA_DRIVE_* flags to make code simpler, as signals are
+ * usually to be sampled on the opposite edge of the driving edge.
+ */
 #define DRM_BUS_FLAG_PIXDATA_POSEDGE	(1<<2)
-/* drive data on neg. edge */
 #define DRM_BUS_FLAG_PIXDATA_NEGEDGE	(1<<3)
+
+/* Drive data on rising edge */
+#define DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE	DRM_BUS_FLAG_PIXDATA_POSEDGE
+/* Drive data on falling edge */
+#define DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE	DRM_BUS_FLAG_PIXDATA_NEGEDGE
+/* Sample data on rising edge */
+#define DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE	DRM_BUS_FLAG_PIXDATA_NEGEDGE
+/* Sample data on falling edge */
+#define DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE	DRM_BUS_FLAG_PIXDATA_POSEDGE
+
 /* data is transmitted MSB to LSB on the bus */
 #define DRM_BUS_FLAG_DATA_MSB_TO_LSB	(1<<4)
 /* data is transmitted LSB to MSB on the bus */
 #define DRM_BUS_FLAG_DATA_LSB_TO_MSB	(1<<5)
-/* drive sync on pos. edge */
+
+/*
+ * Similarly to the DRM_BUS_FLAG_PIXDATA_* flags, don't use these two flags
+ * directly, use one of the DRM_BUS_FLAG_SYNC_(DRIVE|SAMPLE)_* instead.
+ */
 #define DRM_BUS_FLAG_SYNC_POSEDGE	(1<<6)
-/* drive sync on neg. edge */
 #define DRM_BUS_FLAG_SYNC_NEGEDGE	(1<<7)
 
+/* Drive sync on rising edge */
+#define DRM_BUS_FLAG_SYNC_DRIVE_POSEDGE		DRM_BUS_FLAG_SYNC_POSEDGE
+/* Drive sync on falling edge */
+#define DRM_BUS_FLAG_SYNC_DRIVE_NEGEDGE		DRM_BUS_FLAG_SYNC_NEGEDGE
+/* Sample sync on rising edge */
+#define DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE	DRM_BUS_FLAG_SYNC_NEGEDGE
+/* Sample sync on falling edge */
+#define DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE	DRM_BUS_FLAG_SYNC_POSEDGE
+
 	/**
 	 * @bus_flags: Additional information (like pixel signal polarity) for
 	 * the pixel data on the bus, using DRM_BUS_FLAGS\_ defines.

Comments

Stefan Agner Jan. 11, 2019, 8:47 a.m.
On 11.01.2019 04:51, Laurent Pinchart wrote:
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> The DRM_BUS_FLAG_PIXDATA_POSEDGE and DRM_BUS_FLAG_PIXDATA_NEGEDGE macros
> and their DRM_BUS_FLAG_SYNC_* counterparts define on which pixel clock
> edge data and sync signals are driven. They are however used in some
> drivers to define on which pixel clock edge data and sync signals are
> sampled, which should usually (but not always) be the opposite edge of
> the driving edge. This creates confusion.
> 
> Create four new macros for both PIXDATA and SYNC that explicitly state
> the driving and sampling edge in their name to remove the confusion. The
> driving macros are defined as the opposite of the sampling macros to
> made code simpler based on the assumption that the driving and sampling
> edges are opposite.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>

Reviewed-by: Stefan Agner <stefan@agner.ch>

--
Stefan

> ---
> Changes since v1:
> 
> - Address the DRM_BUS_FLAG_SYNC_* flags
> - Rebase on top of drm-next
> ---
>  include/drm/drm_connector.h | 36 ++++++++++++++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 9be2181b3ed7..00bb7a74962b 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -330,19 +330,47 @@ struct drm_display_info {
>  
>  #define DRM_BUS_FLAG_DE_LOW		(1<<0)
>  #define DRM_BUS_FLAG_DE_HIGH		(1<<1)
> -/* drive data on pos. edge */
> +
> +/*
> + * Don't use those two flags directly, use the DRM_BUS_FLAG_PIXDATA_DRIVE_*
> + * and DRM_BUS_FLAG_PIXDATA_SAMPLE_* variants to qualify the flags explicitly.
> + * The DRM_BUS_FLAG_PIXDATA_SAMPLE_* flags are defined as the opposite of the
> + * DRM_BUS_FLAG_PIXDATA_DRIVE_* flags to make code simpler, as signals are
> + * usually to be sampled on the opposite edge of the driving edge.
> + */
>  #define DRM_BUS_FLAG_PIXDATA_POSEDGE	(1<<2)
> -/* drive data on neg. edge */
>  #define DRM_BUS_FLAG_PIXDATA_NEGEDGE	(1<<3)
> +
> +/* Drive data on rising edge */
> +#define DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE	DRM_BUS_FLAG_PIXDATA_POSEDGE
> +/* Drive data on falling edge */
> +#define DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE	DRM_BUS_FLAG_PIXDATA_NEGEDGE
> +/* Sample data on rising edge */
> +#define DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE	DRM_BUS_FLAG_PIXDATA_NEGEDGE
> +/* Sample data on falling edge */
> +#define DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE	DRM_BUS_FLAG_PIXDATA_POSEDGE
> +
>  /* data is transmitted MSB to LSB on the bus */
>  #define DRM_BUS_FLAG_DATA_MSB_TO_LSB	(1<<4)
>  /* data is transmitted LSB to MSB on the bus */
>  #define DRM_BUS_FLAG_DATA_LSB_TO_MSB	(1<<5)
> -/* drive sync on pos. edge */
> +
> +/*
> + * Similarly to the DRM_BUS_FLAG_PIXDATA_* flags, don't use these two flags
> + * directly, use one of the DRM_BUS_FLAG_SYNC_(DRIVE|SAMPLE)_* instead.
> + */
>  #define DRM_BUS_FLAG_SYNC_POSEDGE	(1<<6)
> -/* drive sync on neg. edge */
>  #define DRM_BUS_FLAG_SYNC_NEGEDGE	(1<<7)
>  
> +/* Drive sync on rising edge */
> +#define DRM_BUS_FLAG_SYNC_DRIVE_POSEDGE		DRM_BUS_FLAG_SYNC_POSEDGE
> +/* Drive sync on falling edge */
> +#define DRM_BUS_FLAG_SYNC_DRIVE_NEGEDGE		DRM_BUS_FLAG_SYNC_NEGEDGE
> +/* Sample sync on rising edge */
> +#define DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE	DRM_BUS_FLAG_SYNC_NEGEDGE
> +/* Sample sync on falling edge */
> +#define DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE	DRM_BUS_FLAG_SYNC_POSEDGE
> +
>  	/**
>  	 * @bus_flags: Additional information (like pixel signal polarity) for
>  	 * the pixel data on the bus, using DRM_BUS_FLAGS\_ defines.
Daniel Vetter Jan. 11, 2019, 9:23 a.m.
On Fri, Jan 11, 2019 at 05:51:06AM +0200, Laurent Pinchart wrote:
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> The DRM_BUS_FLAG_PIXDATA_POSEDGE and DRM_BUS_FLAG_PIXDATA_NEGEDGE macros
> and their DRM_BUS_FLAG_SYNC_* counterparts define on which pixel clock
> edge data and sync signals are driven. They are however used in some
> drivers to define on which pixel clock edge data and sync signals are
> sampled, which should usually (but not always) be the opposite edge of
> the driving edge. This creates confusion.
> 
> Create four new macros for both PIXDATA and SYNC that explicitly state
> the driving and sampling edge in their name to remove the confusion. The
> driving macros are defined as the opposite of the sampling macros to
> made code simpler based on the assumption that the driving and sampling
> edges are opposite.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Changes since v1:
> 
> - Address the DRM_BUS_FLAG_SYNC_* flags
> - Rebase on top of drm-next
> ---
>  include/drm/drm_connector.h | 36 ++++++++++++++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 9be2181b3ed7..00bb7a74962b 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -330,19 +330,47 @@ struct drm_display_info {
>  
>  #define DRM_BUS_FLAG_DE_LOW		(1<<0)
>  #define DRM_BUS_FLAG_DE_HIGH		(1<<1)
> -/* drive data on pos. edge */
> +
> +/*
> + * Don't use those two flags directly, use the DRM_BUS_FLAG_PIXDATA_DRIVE_*
> + * and DRM_BUS_FLAG_PIXDATA_SAMPLE_* variants to qualify the flags explicitly.
> + * The DRM_BUS_FLAG_PIXDATA_SAMPLE_* flags are defined as the opposite of the
> + * DRM_BUS_FLAG_PIXDATA_DRIVE_* flags to make code simpler, as signals are
> + * usually to be sampled on the opposite edge of the driving edge.
> + */
>  #define DRM_BUS_FLAG_PIXDATA_POSEDGE	(1<<2)
> -/* drive data on neg. edge */
>  #define DRM_BUS_FLAG_PIXDATA_NEGEDGE	(1<<3)
> +
> +/* Drive data on rising edge */
> +#define DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE	DRM_BUS_FLAG_PIXDATA_POSEDGE
> +/* Drive data on falling edge */
> +#define DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE	DRM_BUS_FLAG_PIXDATA_NEGEDGE
> +/* Sample data on rising edge */
> +#define DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE	DRM_BUS_FLAG_PIXDATA_NEGEDGE
> +/* Sample data on falling edge */
> +#define DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE	DRM_BUS_FLAG_PIXDATA_POSEDGE
> +
>  /* data is transmitted MSB to LSB on the bus */
>  #define DRM_BUS_FLAG_DATA_MSB_TO_LSB	(1<<4)
>  /* data is transmitted LSB to MSB on the bus */
>  #define DRM_BUS_FLAG_DATA_LSB_TO_MSB	(1<<5)
> -/* drive sync on pos. edge */
> +
> +/*
> + * Similarly to the DRM_BUS_FLAG_PIXDATA_* flags, don't use these two flags
> + * directly, use one of the DRM_BUS_FLAG_SYNC_(DRIVE|SAMPLE)_* instead.
> + */
>  #define DRM_BUS_FLAG_SYNC_POSEDGE	(1<<6)
> -/* drive sync on neg. edge */
>  #define DRM_BUS_FLAG_SYNC_NEGEDGE	(1<<7)
>  
> +/* Drive sync on rising edge */
> +#define DRM_BUS_FLAG_SYNC_DRIVE_POSEDGE		DRM_BUS_FLAG_SYNC_POSEDGE
> +/* Drive sync on falling edge */
> +#define DRM_BUS_FLAG_SYNC_DRIVE_NEGEDGE		DRM_BUS_FLAG_SYNC_NEGEDGE
> +/* Sample sync on rising edge */
> +#define DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE	DRM_BUS_FLAG_SYNC_NEGEDGE
> +/* Sample sync on falling edge */
> +#define DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE	DRM_BUS_FLAG_SYNC_POSEDGE

Since these are internal I recommend you convert them over to an enum and
up this to full kerneldoc comments. That way you can have lots more pretty
formatting and linking, and an easy way to give others a public link to
full docs.
-Daniel

> +
>  	/**
>  	 * @bus_flags: Additional information (like pixel signal polarity) for
>  	 * the pixel data on the bus, using DRM_BUS_FLAGS\_ defines.
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Laurent Pinchart Jan. 12, 2019, 1:13 a.m.
Hi Daniel,

On Friday, 11 January 2019 11:23:10 EET Daniel Vetter wrote:
> On Fri, Jan 11, 2019 at 05:51:06AM +0200, Laurent Pinchart wrote:
> > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > 
> > The DRM_BUS_FLAG_PIXDATA_POSEDGE and DRM_BUS_FLAG_PIXDATA_NEGEDGE macros
> > and their DRM_BUS_FLAG_SYNC_* counterparts define on which pixel clock
> > edge data and sync signals are driven. They are however used in some
> > drivers to define on which pixel clock edge data and sync signals are
> > sampled, which should usually (but not always) be the opposite edge of
> > the driving edge. This creates confusion.
> > 
> > Create four new macros for both PIXDATA and SYNC that explicitly state
> > the driving and sampling edge in their name to remove the confusion. The
> > driving macros are defined as the opposite of the sampling macros to
> > made code simpler based on the assumption that the driving and sampling
> > edges are opposite.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > Acked-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> > Changes since v1:
> > 
> > - Address the DRM_BUS_FLAG_SYNC_* flags
> > - Rebase on top of drm-next
> > ---
> > 
> >  include/drm/drm_connector.h | 36 ++++++++++++++++++++++++++++++++----
> >  1 file changed, 32 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 9be2181b3ed7..00bb7a74962b 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -330,19 +330,47 @@ struct drm_display_info {
> > 
> >  #define DRM_BUS_FLAG_DE_LOW		(1<<0)
> >  #define DRM_BUS_FLAG_DE_HIGH		(1<<1)
> > 
> > -/* drive data on pos. edge */
> > +
> > +/*
> > + * Don't use those two flags directly, use the
> > DRM_BUS_FLAG_PIXDATA_DRIVE_* + * and DRM_BUS_FLAG_PIXDATA_SAMPLE_*
> > variants to qualify the flags explicitly. + * The
> > DRM_BUS_FLAG_PIXDATA_SAMPLE_* flags are defined as the opposite of the +
> > * DRM_BUS_FLAG_PIXDATA_DRIVE_* flags to make code simpler, as signals are
> > + * usually to be sampled on the opposite edge of the driving edge. + */
> > 
> >  #define DRM_BUS_FLAG_PIXDATA_POSEDGE	(1<<2)
> > 
> > -/* drive data on neg. edge */
> > 
> >  #define DRM_BUS_FLAG_PIXDATA_NEGEDGE	(1<<3)
> > 
> > +
> > +/* Drive data on rising edge */
> > +#define DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE	DRM_BUS_FLAG_PIXDATA_POSEDGE
> > +/* Drive data on falling edge */
> > +#define DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE	DRM_BUS_FLAG_PIXDATA_NEGEDGE
> > +/* Sample data on rising edge */
> > +#define DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE	
DRM_BUS_FLAG_PIXDATA_NEGEDGE
> > +/* Sample data on falling edge */
> > +#define DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE	
DRM_BUS_FLAG_PIXDATA_POSEDGE
> > +
> > 
> >  /* data is transmitted MSB to LSB on the bus */
> >  #define DRM_BUS_FLAG_DATA_MSB_TO_LSB	(1<<4)
> >  /* data is transmitted LSB to MSB on the bus */
> >  #define DRM_BUS_FLAG_DATA_LSB_TO_MSB	(1<<5)
> > 
> > -/* drive sync on pos. edge */
> > +
> > +/*
> > + * Similarly to the DRM_BUS_FLAG_PIXDATA_* flags, don't use these two
> > flags + * directly, use one of the DRM_BUS_FLAG_SYNC_(DRIVE|SAMPLE)_*
> > instead. + */
> > 
> >  #define DRM_BUS_FLAG_SYNC_POSEDGE	(1<<6)
> > 
> > -/* drive sync on neg. edge */
> > 
> >  #define DRM_BUS_FLAG_SYNC_NEGEDGE	(1<<7)
> > 
> > +/* Drive sync on rising edge */
> > +#define DRM_BUS_FLAG_SYNC_DRIVE_POSEDGE		DRM_BUS_FLAG_SYNC_POSEDGE
> > +/* Drive sync on falling edge */
> > +#define DRM_BUS_FLAG_SYNC_DRIVE_NEGEDGE		DRM_BUS_FLAG_SYNC_NEGEDGE
> > +/* Sample sync on rising edge */
> > +#define DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE	DRM_BUS_FLAG_SYNC_NEGEDGE
> > +/* Sample sync on falling edge */
> > +#define DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE	DRM_BUS_FLAG_SYNC_POSEDGE
> 
> Since these are internal I recommend you convert them over to an enum and
> up this to full kerneldoc comments. That way you can have lots more pretty
> formatting and linking, and an easy way to give others a public link to
> full docs.

The patch is in your mailbox.

> > +
> >  	/**
> >  	 * @bus_flags: Additional information (like pixel signal polarity) for
> >  	 * the pixel data on the bus, using DRM_BUS_FLAGS\_ defines.