Skip to contentSkip to conversation

dm: video: fix abuse of enum

PATCH New Lothar Waßmann 3 participants 9 messages Jun 20, 2017 – Jul 14, 2017

Subsystems (3)
Framebuffer layer Tegra architecture support Arm/Microchip (AT91) SoC support
lore
Share & tools
lore

Lothar Waßmann

author
#0 lore raw

PATCH 1/1 dm: video: fix abuse of enum

LCD_MAX_WIDTH, LCD_MAX_HEIGHT and LCD_MAX_LSBPP are not alternative values for one specific variable, but unrelated entities with distinct purposes. There is no use defining them as values of an 'enum'.

The 'enum' construct would fail miserably for an LCD controller that has a square max. frame size (e.g. 4096x4096).

Signed-off-by Lothar Waßmann <[email protected]>
---
 drivers/video/atmel_hlcdfb.c     |  8 +++-----
 drivers/video/atmel_lcdfb.c      | 10 ++++------
 drivers/video/sandbox_sdl.c      |  8 +++-----
 drivers/video/tegra.c            | 12 +++++-------
 drivers/video/tegra124/display.c | 11 +++++------
 5 files changed, 20 insertions(+), 29 deletions(-)
drivers/video/atmel_hlcdfb.c +3 -5
index 59b9c45..7426abb 100644
@@ -244,11 +244,9 @@ void lcd_ctrl_init(void *lcdbase)
 
 #else
 
-enum {
-	LCD_MAX_WIDTH		= 1024,
-	LCD_MAX_HEIGHT		= 768,
-	LCD_MAX_LOG2_BPP	= VIDEO_BPP16,
-};
+#define LCD_MAX_WIDTH		1024
+#define LCD_MAX_HEIGHT		768
+#define LCD_MAX_LOG2_BPP	VIDEO_BPP16
 
 struct atmel_hlcdc_priv {
 	struct atmel_hlcd_regs *regs;
drivers/video/atmel_lcdfb.c +4 -6
index ed2bd30..c4f6251 100644
@@ -21,12 +21,10 @@
 DECLARE_GLOBAL_DATA_PTR;
 
 #ifdef CONFIG_DM_VIDEO
-enum {
-	/* Maximum LCD size we support */
-	LCD_MAX_WIDTH		= 1366,
-	LCD_MAX_HEIGHT		= 768,
-	LCD_MAX_LOG2_BPP	= VIDEO_BPP16,
-};
+/* Maximum LCD size we support */
+#define LCD_MAX_WIDTH		1366
+#define LCD_MAX_HEIGHT		768
+#define LCD_MAX_LOG2_BPP	VIDEO_BPP16
 #endif
 
 struct atmel_fb_priv {
drivers/video/sandbox_sdl.c +3 -5
index 18afe2f..e1b6a85 100644
@@ -14,11 +14,9 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-enum {
-	/* Default LCD size we support */
-	LCD_MAX_WIDTH		= 1366,
-	LCD_MAX_HEIGHT		= 768,
-};
+/* Default LCD size we support */
+#define LCD_MAX_WIDTH		1366
+#define LCD_MAX_HEIGHT		768
 
 static int sandbox_sdl_probe(struct udevice *dev)
 {
drivers/video/tegra.c +5 -7
index 0ba3f2c..87db07a 100644
@@ -1,4 +1,4 @@
-/*
+s/*
  * Copyright (c) 2011 The Chromium OS Authors.
  * SPDX-License-Identifier:	GPL-2.0+
  */

@@ -34,12 +34,10 @@ struct tegra_lcd_priv {
 	unsigned pixel_clock;		/* Pixel clock in Hz */
 };
 
-enum {
-	/* Maximum LCD size we support */
-	LCD_MAX_WIDTH		= 1366,
-	LCD_MAX_HEIGHT		= 768,
-	LCD_MAX_LOG2_BPP	= VIDEO_BPP16,
-};
+/* Maximum LCD size we support */
+#define LCD_MAX_WIDTH		1366
+#define LCD_MAX_HEIGHT		768
+#define LCD_MAX_LOG2_BPP	VIDEO_BPP16
 
 static void update_window(struct dc_ctlr *dc, struct disp_ctl_win *win)
 {
drivers/video/tegra124/display.c +5 -6
index bbbca13..1c641a4 100644
@@ -420,12 +420,11 @@ static int display_init(struct udevice *dev, void *lcdbase,
 	return 0;
 }
 
-enum {
-	/* Maximum LCD size we support */
-	LCD_MAX_WIDTH		= 1920,
-	LCD_MAX_HEIGHT		= 1200,
-	LCD_MAX_LOG2_BPP	= 4,		/* 2^4 = 16 bpp */
-};
+
+/* Maximum LCD size we support */
+#define LCD_MAX_WIDTH		1920
+#define LCD_MAX_HEIGHT		1200
+#define LCD_MAX_LOG2_BPP	VIDEO_BPP16
 
 static int tegra124_lcd_init(struct udevice *dev, void *lcdbase,
 			     enum video_log2_bpp l2bpp)
Signature · 1 line
2.1.4
#1 lore raw

PATCH 1/1 dm: video: fix abuse of enum

Hi Lothar,

Quoted 3 lines · Lothar Waßmann

LCD_MAX_WIDTH, LCD_MAX_HEIGHT and LCD_MAX_LSBPP are not alternative values for one specific variable, but unrelated entities with distinct purposes. There is no use defining them as values of an 'enum'.

Can you explain why #define is better? I prefer enum since they are a compiler construct instead of preprocessor (thus no need for brackets, no strange conversion things) and the debugger knows about them.

Quoted 3 lines

The 'enum' construct would fail miserably for an LCD controller that has a square max. frame size (e.g. 4096x4096).

What does this mean? I don't understand sorry.

Quoted 10 lines
Signed-off-by Lothar Waßmann <[email protected]>
---
 drivers/video/atmel_hlcdfb.c     |  8 +++-----
 drivers/video/atmel_lcdfb.c      | 10 ++++------
 drivers/video/sandbox_sdl.c      |  8 +++-----
 drivers/video/tegra.c            | 12 +++++-------
 drivers/video/tegra124/display.c | 11 +++++------
 5 files changed, 20 insertions(+), 29 deletions(-)

Regards,
Simon

author
#2 lore raw

PATCH 1/1 dm: video: fix abuse of enum

Hi,

Quoted 11 lines · On Tue, 20 Jun 2017 12:26:29 -0600 Simon Glass wrote:

Hi Lothar,

Quoted 3 lines · Lothar Waßmann

LCD_MAX_WIDTH, LCD_MAX_HEIGHT and LCD_MAX_LSBPP are not alternative values for one specific variable, but unrelated entities with distinct purposes. There is no use defining them as values of an 'enum'.

Can you explain why #define is better? I prefer enum since they are a compiler construct instead of preprocessor (thus no need for brackets, no strange conversion things) and the debugger knows about them.

An enum defines alternative values for one specific entity (e.g. clauses for a switch construct), but not a collection of arbitrary data items.

Quoted 5 lines
Quoted 2 lines

The 'enum' construct would fail miserably for an LCD controller that has a square max. frame size (e.g. 4096x4096).

What does this mean? I don't understand sorry.

Try your enum with MAX_LCD_WITDH == MAC_LCD_HEIGHT.

Lothar Waßmann

author
#3 lore raw

PATCH 1/1 dm: video: fix abuse of enum

Hi,

Quoted 18 lines · On Wed, 21 Jun 2017 09:59:05 +0200 Lothar Waßmann wrote:

Hi,

Quoted 11 lines · On Tue, 20 Jun 2017 12:26:29 -0600 Simon Glass wrote:

Hi Lothar,

Quoted 3 lines · Lothar Waßmann

LCD_MAX_WIDTH, LCD_MAX_HEIGHT and LCD_MAX_LSBPP are not alternative values for one specific variable, but unrelated entities with distinct purposes. There is no use defining them as values of an 'enum'.

Can you explain why #define is better? I prefer enum since they are a compiler construct instead of preprocessor (thus no need for brackets, no strange conversion things) and the debugger knows about them.

An enum defines alternative values for one specific entity (e.g. clauses for a switch construct), but not a collection of arbitrary data items.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

The 'enum' construct would fail miserably for an LCD controller that has a square max. frame size (e.g. 4096x4096).

What does this mean? I don't understand sorry.

Try your enum with MAX_LCD_WITDH == MAC_LCD_HEIGHT.

Are you still not convinced, that the use of 'enum' is inappropriate in this context?

Lothar Waßmann

#4 lore raw

PATCH 1/1 dm: video: fix abuse of enum

Hi Lothar,

Quoted 27 lines · Lothar Waßmann

Hi,

Quoted 24 lines · On Wed, 21 Jun 2017 09:59:05 +0200 Lothar Waßmann wrote:

Hi,

Quoted 11 lines · On Tue, 20 Jun 2017 12:26:29 -0600 Simon Glass wrote:

Hi Lothar,

Quoted 3 lines · Lothar Waßmann

LCD_MAX_WIDTH, LCD_MAX_HEIGHT and LCD_MAX_LSBPP are not alternative values for one specific variable, but unrelated entities with distinct purposes. There is no use defining them as values of an 'enum'.

Can you explain why #define is better? I prefer enum since they are a compiler construct instead of preprocessor (thus no need for brackets, no strange conversion things) and the debugger knows about them.

An enum defines alternative values for one specific entity (e.g. clauses for a switch construct), but not a collection of arbitrary data items.

Quoted 5 lines
Quoted 2 lines

The 'enum' construct would fail miserably for an LCD controller that has a square max. frame size (e.g. 4096x4096).

What does this mean? I don't understand sorry.

Try your enum with MAX_LCD_WITDH == MAC_LCD_HEIGHT.

Can you please be explicit as to what the problem is? Sorry but I don't understand what you are driving at. Do you have a test program which shows the problem?

Quoted 3 lines
Quoted 1 line

Are you still not convinced, that the use of 'enum' is inappropriate in this context?

Not yet. I don't know of any problem with it, and I prefer enum to #define in many situations for the reasons I list above.

Regards,
Simon

author
#5 lore raw

PATCH 1/1 dm: video: fix abuse of enum

Hi,

Quoted 27 lines · On Wed, 5 Jul 2017 22:49:28 -0600 Simon Glass wrote:

Hi Lothar,

Quoted 24 lines · Lothar Waßmann

Hi,

Quoted 21 lines · On Wed, 21 Jun 2017 09:59:05 +0200 Lothar Waßmann wrote:

Hi,

Quoted 11 lines · On Tue, 20 Jun 2017 12:26:29 -0600 Simon Glass wrote:

Hi Lothar,

Quoted 3 lines · Lothar Waßmann

LCD_MAX_WIDTH, LCD_MAX_HEIGHT and LCD_MAX_LSBPP are not alternative values for one specific variable, but unrelated entities with distinct purposes. There is no use defining them as values of an 'enum'.

Can you explain why #define is better? I prefer enum since they are a compiler construct instead of preprocessor (thus no need for brackets, no strange conversion things) and the debugger knows about them.

An enum defines alternative values for one specific entity (e.g. clauses for a switch construct), but not a collection of arbitrary data items.

Quoted 3 lines
Quoted 2 lines

The 'enum' construct would fail miserably for an LCD controller that has a square max. frame size (e.g. 4096x4096).

Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines

What does this mean? I don't understand sorry.

Try your enum with MAX_LCD_WITDH == MAC_LCD_HEIGHT.

Can you please be explicit as to what the problem is? Sorry but I don't understand what you are driving at. Do you have a test program which shows the problem?

You cannot have two different enum items with the same value! Thus:

enum {
	MAX_LCD_WIDTH = 4096,
	MAX_LCD_HEIGHT = 4096,
};

won't compile.

The purpose of an enum is to provide a collection of possible values that can be taken by a single variable. E.g. enumerate the states of a state machine, video modes, CPU types...
It's not meant to group together otherwise unsolicited values.

Lothar Waßmann

#6 lore raw

PATCH 1/1 dm: video: fix abuse of enum

Dear Lothar

Quoted 53 lines · Lothar Waßmann

Hi,

Quoted 42 lines · On Wed, 5 Jul 2017 22:49:28 -0600 Simon Glass wrote:

Hi Lothar,

Quoted 38 lines · Lothar Waßmann

Hi,

Quoted 35 lines · On Wed, 21 Jun 2017 09:59:05 +0200 Lothar Waßmann wrote:

Hi,

Quoted 11 lines · On Tue, 20 Jun 2017 12:26:29 -0600 Simon Glass wrote:

Hi Lothar,

Quoted 6 lines · Lothar Waßmann

LCD_MAX_WIDTH, LCD_MAX_HEIGHT and LCD_MAX_LSBPP are not alternative
values for one specific variable, but unrelated entities with distinct
purposes. There is no use defining them as values of an 'enum'.

Quoted 7 lines

Can you explain why #define is better? I prefer enum since they are a
compiler construct instead of preprocessor (thus no need for brackets,
no strange conversion things) and the debugger knows about them.

An enum defines alternative values for one specific entity (e.g.
clauses for a switch construct), but not a collection of arbitrary data
items.

Quoted 6 lines
Quoted 3 lines

The 'enum' construct would fail miserably for an LCD controller that
has a square max. frame size (e.g. 4096x4096).

What does this mean? I don't understand sorry.

Quoted 7 lines
Quoted 1 line
Quoted 1 line

Try your enum with MAX_LCD_WITDH == MAC_LCD_HEIGHT.

Can you please be explicit as to what the problem is? Sorry but I don't understand what you are driving at. Do you have a test program
which shows the problem?

Quoted 7 lines

You cannot have two different enum items with the same value! Thus:

enum {
	MAX_LCD_WIDTH = 4096,
	MAX_LCD_HEIGHT = 4096,
};

won't compile.

Says who?

At least my gcc compilers even compile the following just fine:

enum {
	MAX_LCD_WIDTH = 4096,
	MAX_LCD_HEIGHT = MAX_LCD_WIDTH,
};

I really don't think you have any valid point here. Enum items are really just numbers and there is no limitation as such whether two items may share the same number or not. It's really absolutely OK. Of course as such one can not really guess the exact enum item/name from just the number which may sound suboptimal but that is completely legal in C. Some other high level languages do not allow that as far as I remember (e.g. Java).

Quoted 8 lines

The purpose of an enum is to provide a collection of possible values that can be taken by a single variable. E.g. enumerate the states of a
state machine, video modes, CPU types...
It's not meant to group together otherwise unsolicited values.

Lothar Waßmann

Cheers

Marcel

author
#7 lore raw

PATCH 1/1 dm: video: fix abuse of enum

Hi,

Quoted 64 lines · On Thu, 6 Jul 2017 12:22:52 +0000 Marcel Ziswiler wrote:

Dear Lothar

Quoted 53 lines · Lothar Waßmann

Hi,

Quoted 42 lines · On Wed, 5 Jul 2017 22:49:28 -0600 Simon Glass wrote:

Hi Lothar,

Quoted 38 lines · Lothar Waßmann

Hi,

Quoted 35 lines · On Wed, 21 Jun 2017 09:59:05 +0200 Lothar Waßmann wrote:

Hi,

Quoted 11 lines · On Tue, 20 Jun 2017 12:26:29 -0600 Simon Glass wrote:

Hi Lothar,

Quoted 6 lines · Lothar Waßmann

LCD_MAX_WIDTH, LCD_MAX_HEIGHT and LCD_MAX_LSBPP are not alternative
values for one specific variable, but unrelated entities with distinct
purposes. There is no use defining them as values of an 'enum'.

Quoted 7 lines

Can you explain why #define is better? I prefer enum since they are a
compiler construct instead of preprocessor (thus no need for brackets,
no strange conversion things) and the debugger knows about them.

An enum defines alternative values for one specific entity (e.g.
clauses for a switch construct), but not a collection of arbitrary data
items.

Quoted 6 lines
Quoted 3 lines

The 'enum' construct would fail miserably for an LCD controller that
has a square max. frame size (e.g. 4096x4096).

What does this mean? I don't understand sorry.

Quoted 7 lines
Quoted 1 line
Quoted 1 line

Try your enum with MAX_LCD_WITDH == MAC_LCD_HEIGHT.

Can you please be explicit as to what the problem is? Sorry but I don't understand what you are driving at. Do you have a test program
which shows the problem?

Quoted 7 lines

You cannot have two different enum items with the same value! Thus:

enum {
	MAX_LCD_WIDTH = 4096,
	MAX_LCD_HEIGHT = 4096,
};

won't compile.

Quoted 9 lines

Says who?

At least my gcc compilers even compile the following just fine:

enum {
	MAX_LCD_WIDTH = 4096,
	MAX_LCD_HEIGHT = MAX_LCD_WIDTH,
};

Sorry, I was so locked in to the "normal" use of enum that I got confused.

Lothar Waßmann

#8 lore raw

PATCH 1/1 dm: video: fix abuse of enum

Hi Lothar,

Quoted 71 lines · Lothar Waßmann

Hi,

Quoted 68 lines · On Thu, 6 Jul 2017 12:22:52 +0000 Marcel Ziswiler wrote:

Dear Lothar

Quoted 53 lines · Lothar Waßmann

Hi,

Quoted 42 lines · On Wed, 5 Jul 2017 22:49:28 -0600 Simon Glass wrote:

Hi Lothar,

Quoted 38 lines · Lothar Waßmann

Hi,

Quoted 35 lines · On Wed, 21 Jun 2017 09:59:05 +0200 Lothar Waßmann wrote:

Hi,

Quoted 11 lines · On Tue, 20 Jun 2017 12:26:29 -0600 Simon Glass wrote:

Hi Lothar,

Quoted 6 lines · Lothar Waßmann

LCD_MAX_WIDTH, LCD_MAX_HEIGHT and LCD_MAX_LSBPP are not alternative
values for one specific variable, but unrelated entities with distinct
purposes. There is no use defining them as values of an 'enum'.

Quoted 7 lines

Can you explain why #define is better? I prefer enum since they are a
compiler construct instead of preprocessor (thus no need for brackets,
no strange conversion things) and the debugger knows about them.

An enum defines alternative values for one specific entity (e.g.
clauses for a switch construct), but not a collection of arbitrary data
items.

Quoted 6 lines
Quoted 3 lines

The 'enum' construct would fail miserably for an LCD controller that
has a square max. frame size (e.g. 4096x4096).

What does this mean? I don't understand sorry.

Quoted 7 lines
Quoted 1 line
Quoted 1 line

Try your enum with MAX_LCD_WITDH == MAC_LCD_HEIGHT.

Can you please be explicit as to what the problem is? Sorry but I don't understand what you are driving at. Do you have a test program
which shows the problem?

Quoted 7 lines

You cannot have two different enum items with the same value! Thus:

enum {
    MAX_LCD_WIDTH = 4096,
    MAX_LCD_HEIGHT = 4096,
};

won't compile.

Says who?

At least my gcc compilers even compile the following just fine:

Quoted 7 lines
Quoted 5 lines

enum {
MAX_LCD_WIDTH = 4096,
MAX_LCD_HEIGHT = MAX_LCD_WIDTH,
};

Sorry, I was so locked in to the "normal" use of enum that I got confused.

OK I see, no worries.

I agree this is a borderline case, but I still prefer it :-)

Regards,
Simon