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 + */ +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;
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\_.-- Regards,
Laurent Pinchart
Hi Laurent
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
Could you extend the comment to expalin what there replacements are? Otherwise we leave it to someone with less understanding of the subject to find out. There are 30+ cases we need to fix and we better do it right.
It would also be nice to have the legacy values grouped so they are easier to ignore.
Sam
Hi Sam,
On Sat, Jan 12, 2019 at 01:41:11PM +0100, Sam Ravnborg wrote:
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
Could you extend the comment to expalin what there replacements are? Otherwise we leave it to someone with less understanding of the subject to find out. There are 30+ cases we need to fix and we better do it right.
This patch will be merged as part of a series that replaces all the existing users of the legacy values. I'm leaving them here just in case another patch using them would be merged at the same time, and will then submit a patch to remove them.
It would also be nice to have the legacy values grouped so they are easier to ignore.
Can I keep this patch as-is given that I will remove the legacy values soon ?
Hi Laurent.
- @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
Could you extend the comment to expalin what there replacements are? Otherwise we leave it to someone with less understanding of the subject to find out. There are 30+ cases we need to fix and we better do it right.
This patch will be merged as part of a series that replaces all the existing users of the legacy values. I'm leaving them here just in case another patch using them would be merged at the same time, and will then submit a patch to remove them.
It would also be nice to have the legacy values grouped so they are easier to ignore.
Can I keep this patch as-is given that I will remove the legacy values soon ?
Thats much better so we gid of some legacy. With this you can add my: Reviewed-by: Sam Ravnborg sam@ravnborg.org
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Laurent Pinchart -
Sam Ravnborg