Re: [PATCH/RFC 01/15] drm: Clarify definition of the DRM_BUS_FLAG_(PIXDATA|SYNC)_* macros

[Kieran Bingham <kieran.bingham@ideasonboard.com>]

Hi Laurent,

Thank you for the patch,

On 06/03/2019 23:23, Laurent Pinchart wrote:
> 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

s/made/make/

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

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>



> ---
>  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 8fe22abb1e10..411c0eb4c00e 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.
> 

-- 
Regards
--
Kieran