[U-Boot] [PATCH] 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 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)
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
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
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
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
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
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
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
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
participants (3)
-
Lothar Waßmann -
Marcel Ziswiler -
Simon Glass