[U-Boot] [PATCH] dm: video: fix abuse of enum

[U-Boot] [PATCH] dm: video: fix abuse of enum

[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'.

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 <LW@KARO-electronics.de>
---
 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(-)

diff --git a/drivers/video/atmel_hlcdfb.c b/drivers/video/atmel_hlcdfb.c
index 59b9c45..7426abb 100644
--- a/drivers/video/atmel_hlcdfb.c
+++ b/drivers/video/atmel_hlcdfb.c
@@ -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;
diff --git a/drivers/video/atmel_lcdfb.c b/drivers/video/atmel_lcdfb.c
index ed2bd30..c4f6251 100644
--- a/drivers/video/atmel_lcdfb.c
+++ b/drivers/video/atmel_lcdfb.c
@@ -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 {
diff --git a/drivers/video/sandbox_sdl.c b/drivers/video/sandbox_sdl.c
index 18afe2f..e1b6a85 100644
--- a/drivers/video/sandbox_sdl.c
+++ b/drivers/video/sandbox_sdl.c
@@ -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)
 {
diff --git a/drivers/video/tegra.c b/drivers/video/tegra.c
index 0ba3f2c..87db07a 100644
--- a/drivers/video/tegra.c
+++ b/drivers/video/tegra.c
@@ -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)
 {
diff --git a/drivers/video/tegra124/display.c b/drivers/video/tegra124/display.c
index bbbca13..1c641a4 100644
--- a/drivers/video/tegra124/display.c
+++ b/drivers/video/tegra124/display.c
@@ -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)
-- 
2.1.4

[U-Boot] [PATCH] dm: video: fix abuse of enum

[Simon Glass]

Hi Lothar,

On 20 June 2017 at 04:25, Lothar Waßmann <LW@karo-electronics.de> wrote:
> 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.

>
> 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.

>
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  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

[U-Boot] [PATCH] dm: video: fix abuse of enum

[Lothar Waßmann]

Hi,

On Tue, 20 Jun 2017 12:26:29 -0600 Simon Glass wrote:
> Hi Lothar,
> 
> On 20 June 2017 at 04:25, Lothar Waßmann <LW@karo-electronics.de> wrote:
> > 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.

> > 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

[U-Boot] [PATCH] dm: video: fix abuse of enum

[Lothar Waßmann]

Hi,

On Wed, 21 Jun 2017 09:59:05 +0200 Lothar Waßmann wrote:
> Hi,
> 
> On Tue, 20 Jun 2017 12:26:29 -0600 Simon Glass wrote:
> > Hi Lothar,
> > 
> > On 20 June 2017 at 04:25, Lothar Waßmann <LW@karo-electronics.de> wrote:
> > > 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.
> 
> > > 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

[U-Boot] [PATCH] dm: video: fix abuse of enum

[Simon Glass]

Hi Lothar,

On 23 June 2017 at 00:30, Lothar Waßmann <LW@karo-electronics.de> wrote:
> Hi,
>
> On Wed, 21 Jun 2017 09:59:05 +0200 Lothar Waßmann wrote:
>> Hi,
>>
>> On Tue, 20 Jun 2017 12:26:29 -0600 Simon Glass wrote:
>> > Hi Lothar,
>> >
>> > On 20 June 2017 at 04:25, Lothar Waßmann <LW@karo-electronics.de> wrote:
>> > > 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.
>>
>> > > 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?

>>
> 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

[U-Boot] [PATCH] dm: video: fix abuse of enum

[Lothar Waßmann]

Hi,

On Wed, 5 Jul 2017 22:49:28 -0600 Simon Glass wrote:
> Hi Lothar,
> 
> On 23 June 2017 at 00:30, Lothar Waßmann <LW@karo-electronics.de> wrote:
> > Hi,
> >
> > On Wed, 21 Jun 2017 09:59:05 +0200 Lothar Waßmann wrote:
> >> Hi,
> >>
> >> On Tue, 20 Jun 2017 12:26:29 -0600 Simon Glass wrote:
> >> > Hi Lothar,
> >> >
> >> > On 20 June 2017 at 04:25, Lothar Waßmann <LW@karo-electronics.de> wrote:
> >> > > 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.
> >>
> >> > > 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?
> 
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

[U-Boot] [PATCH] dm: video: fix abuse of enum

[Marcel Ziswiler]

Dear Lothar

On Thu, 2017-07-06 at 09:50 +0200, Lothar Waßmann wrote:
> Hi,
> 
> On Wed, 5 Jul 2017 22:49:28 -0600 Simon Glass wrote:
> > Hi Lothar,
> > 
> > On 23 June 2017 at 00:30, Lothar Waßmann <LW@karo-electronics.de>
> > wrote:
> > > Hi,
> > > 
> > > On Wed, 21 Jun 2017 09:59:05 +0200 Lothar Waßmann wrote:
> > > > Hi,
> > > > 
> > > > On Tue, 20 Jun 2017 12:26:29 -0600 Simon Glass wrote:
> > > > > Hi Lothar,
> > > > > 
> > > > > On 20 June 2017 at 04:25, Lothar Waßmann <LW@karo-electronics
> > > > > .de> wrote:
> > > > > > 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.
> > > > 
> > > > > > 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?
> > 
> 
> 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).

> 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

[U-Boot] [PATCH] dm: video: fix abuse of enum

[Lothar Waßmann]

Hi,

On Thu, 6 Jul 2017 12:22:52 +0000 Marcel Ziswiler wrote:
> Dear Lothar
> 
> On Thu, 2017-07-06 at 09:50 +0200, Lothar Waßmann wrote:
> > Hi,
> > 
> > On Wed, 5 Jul 2017 22:49:28 -0600 Simon Glass wrote:
> > > Hi Lothar,
> > > 
> > > On 23 June 2017 at 00:30, Lothar Waßmann <LW@karo-electronics.de>
> > > wrote:
> > > > Hi,
> > > > 
> > > > On Wed, 21 Jun 2017 09:59:05 +0200 Lothar Waßmann wrote:
> > > > > Hi,
> > > > > 
> > > > > On Tue, 20 Jun 2017 12:26:29 -0600 Simon Glass wrote:
> > > > > > Hi Lothar,
> > > > > > 
> > > > > > On 20 June 2017 at 04:25, Lothar Waßmann <LW@karo-electronics
> > > > > > .de> wrote:
> > > > > > > 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.
> > > > > 
> > > > > > > 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?
> > > 
> > 
> > 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,
> };
>
Sorry, I was so locked in to the "normal" use of enum that I got
confused.


Lothar Waßmann

[U-Boot] [PATCH] dm: video: fix abuse of enum

[Simon Glass]

Hi Lothar,

On 7 July 2017 at 00:41, Lothar Waßmann <LW@karo-electronics.de> wrote:
> Hi,
>
> On Thu, 6 Jul 2017 12:22:52 +0000 Marcel Ziswiler wrote:
>> Dear Lothar
>>
>> On Thu, 2017-07-06 at 09:50 +0200, Lothar Waßmann wrote:
>> > Hi,
>> >
>> > On Wed, 5 Jul 2017 22:49:28 -0600 Simon Glass wrote:
>> > > Hi Lothar,
>> > >
>> > > On 23 June 2017 at 00:30, Lothar Waßmann <LW@karo-electronics.de>
>> > > wrote:
>> > > > Hi,
>> > > >
>> > > > On Wed, 21 Jun 2017 09:59:05 +0200 Lothar Waßmann wrote:
>> > > > > Hi,
>> > > > >
>> > > > > On Tue, 20 Jun 2017 12:26:29 -0600 Simon Glass wrote:
>> > > > > > Hi Lothar,
>> > > > > >
>> > > > > > On 20 June 2017 at 04:25, Lothar Waßmann <LW@karo-electronics
>> > > > > > .de> wrote:
>> > > > > > > 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.
>> > > > >
>> > > > > > > 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?
>> > >
>> >
>> > 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,
>> };
>>
> 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