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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel