Re: [PATCH] drm: Turn bus flags macros into an enum

[Daniel Vetter <daniel@ffwll.ch>]

On Sat, Jan 12, 2019 at 03:10:51AM +0200, Laurent Pinchart wrote:
> This allows nicer kerneldoc with an easy way to reference the enum and
> the values.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/drm/drm_connector.h | 108 +++++++++++++++++++++---------------
>  1 file changed, 64 insertions(+), 44 deletions(-)
> 
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 00bb7a74962b..f5ce255a2cb4 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -253,6 +253,68 @@ enum drm_panel_orientation {
>  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
>  };
>  
> +/**
> + * enum drm_bus_flags - bus_flags info for &drm_display_info
> + *
> + * This enum defines signal polarities and clock edge information for signals on
> + * a bus as bitmask flags.
> + *
> + * The clock edge information is conveyed by two sets of symbols,
> + * DRM_BUS_FLAGS_*_DRIVE_\* and DRM_BUS_FLAGS_*_SAMPLE_\*. When this enum is
> + * used to describe a bus from the point of view of the transmitter, the
> + * \*_DRIVE_\* flags should be used. When used from the point of view of the
> + * receiver, the \*_SAMPLE_\* flags should be used. The \*_DRIVE_\* and
> + * \*_SAMPLE_\* flags alias each other, with the \*_SAMPLE_POSEDGE and
> + * \*_SAMPLE_NEGEDGE flags being equal to \*_DRIVE_NEGEDGE and \*_DRIVE_POSEDGE
> + * respectively. This simplifies code as signals are usually sampled on the
> + * opposite edge of the driving edge. Transmitters and receivers may however
> + * need to take other signal timings into account to convert between driving
> + * and sample edges.
> + *
> + * @DRM_BUS_FLAG_DE_LOW:		The Data Enable signal is active low
> + * @DRM_BUS_FLAG_DE_HIGH:		The Data Enable signal is active high
> + * @DRM_BUS_FLAG_PIXDATA_POSEDGE:	Legacy value, do not use
> + * @DRM_BUS_FLAG_PIXDATA_NEGEDGE:	Legacy value, do not use
> + * @DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE:	Data is driven on the rising edge of
> + *					the pixel clock
> + * @DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE:	Data is driven on the falling edge of
> + *					the pixel clock
> + * @DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE: Data is sampled on the rising edge of
> + *					the pixel clock
> + * @DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE: Data is sampled on the falling edge of
> + *					the pixel clock
> + * @DRM_BUS_FLAG_DATA_MSB_TO_LSB:	Data is transmitted MSB to LSB on the bus
> + * @DRM_BUS_FLAG_DATA_LSB_TO_MSB:	Data is transmitted LSB to MSB on the bus
> + * @DRM_BUS_FLAG_SYNC_POSEDGE:		Legacy value, do not use
> + * @DRM_BUS_FLAG_SYNC_NEGEDGE:		Legacy value, do not use
> + * @DRM_BUS_FLAG_SYNC_DRIVE_POSEDGE:	Sync signals are driven on the rising
> + *					edge of the pixel clock
> + * @DRM_BUS_FLAG_SYNC_DRIVE_NEGEDGE:	Sync signals are driven on the falling
> + *					edge of the pixel clock
> + * @DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE:	Sync signals are sampled on the rising
> + *					edge of the pixel clock
> + * @DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE:	Sync signals are sampled on the falling
> + *					edge of the pixel clock
> + */

For bigger stuff like enum/struct I tend to lean towards the inline
comment style, gives you a lot more space for documenting things. But also
uses a lot more whitespace, so a bit a judgement call.

Anyway, I'm always happy for more structured comments:

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +enum drm_bus_flags {
> +	DRM_BUS_FLAG_DE_LOW = BIT(0),
> +	DRM_BUS_FLAG_DE_HIGH = BIT(1),
> +	DRM_BUS_FLAG_PIXDATA_POSEDGE = BIT(2),
> +	DRM_BUS_FLAG_PIXDATA_NEGEDGE = BIT(3),
> +	DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> +	DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE = DRM_BUS_FLAG_PIXDATA_NEGEDGE,
> +	DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE = DRM_BUS_FLAG_PIXDATA_NEGEDGE,
> +	DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> +	DRM_BUS_FLAG_DATA_MSB_TO_LSB = BIT(4),
> +	DRM_BUS_FLAG_DATA_LSB_TO_MSB = BIT(5),
> +	DRM_BUS_FLAG_SYNC_POSEDGE = BIT(6),
> +	DRM_BUS_FLAG_SYNC_NEGEDGE = BIT(7),
> +	DRM_BUS_FLAG_SYNC_DRIVE_POSEDGE = DRM_BUS_FLAG_SYNC_POSEDGE,
> +	DRM_BUS_FLAG_SYNC_DRIVE_NEGEDGE = DRM_BUS_FLAG_SYNC_NEGEDGE,
> +	DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE = DRM_BUS_FLAG_SYNC_NEGEDGE,
> +	DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE = DRM_BUS_FLAG_SYNC_POSEDGE,
> +};
> +
>  /**
>   * struct drm_display_info - runtime data about the connected sink
>   *
> @@ -328,52 +390,10 @@ struct drm_display_info {
>  	 */
>  	unsigned int num_bus_formats;
>  
> -#define DRM_BUS_FLAG_DE_LOW		(1<<0)
> -#define DRM_BUS_FLAG_DE_HIGH		(1<<1)
> -
> -/*
> - * 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)
> -#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)
> -
> -/*
> - * 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)
> -#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.
> +	 * the pixel data on the bus, using &enum drm_bus_flags values
> +	 * DRM_BUS_FLAGS\_.
>  	 */
>  	u32 bus_flags;
>  
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel