[U-Boot] global variables from comon/lcd.c
Hi,
While looking at the common/lcd.c I fail to understand why it declares and uses global variables like lcd_color_fg, lcd_color_color_bg, lcd_base and relies on the drivers / boards to provide them, leading to code duplication. It also provides getters / setters for some of these variables.
To me it seems more logical to have these variables in lcd.c itself. This construction has been around since at least 2004 though, so I tend to think I am missing something.
Does someone know the reason why these variables are not part of lcd.c itself?
Regards, Jeroen
Dear Jeroen,
In message 50E86DFC.20304@myspectrum.nl you wrote:
While looking at the common/lcd.c I fail to understand why it declares and uses global variables like lcd_color_fg, lcd_color_color_bg, lcd_base and relies on the drivers / boards to provide them, leading to code duplication. It also provides getters / setters for some of these variables.
To me it seems more logical to have these variables in lcd.c itself. This construction has been around since at least 2004 though, so I tend to think I am missing something.
Does someone know the reason why these variables are not part of lcd.c itself?
The code has gone through a number of more or heavy restructuring, so it's difficult to tell what was the original design, and which might just be incomplete cleanup or similar.
Fact is, that lcd_color_fg and lcd_color_color_bg are nowhere used outside common/lcd.c - with a single exception: drivers/video/tegra.c does a
color ^= lcd_color_fg;
once - but this should be trivial to fix by changing it into
color ^= lcd_getfgcolor();
Hm... guess I should submit a cleanup patch :-)
Best regards,
Wolfgang Denk
lcd_color_fg and lcd_color_bg had to be declared in board specific code, but were not actually used there; in addition, we have getter / setter functions for these, which were not used either.
Get rid of the global variables, and use the getter function where needed (so far no setter calls are needed).
Signed-off-by: Wolfgang Denk wd@denx.de Cc: Alessandro Rubini rubini@unipv.it Cc: Anatolij Gustschin agust@denx.de Cc: Bo Shen voice.shen@atmel.com Cc: Haavard Skinnemoen haavard.skinnemoen@atmel.com Cc: Kyungmin Park kyungmin.park@samsung.com Cc: Marek Vasut marek.vasut@gmail.com Cc: Minkyu Kang mk7.kang@samsung.com Cc: Nikita Kiryanov nikita@compulab.co.il Cc: Simon Glass sjg@chromium.org Cc: Stelian Pop stelian@popies.net Cc: Tom Warren twarren@nvidia.com --- arch/arm/cpu/pxa/pxafb.c | 2 -- arch/powerpc/cpu/mpc8xx/lcd.c | 3 --- board/mcc200/lcd.c | 3 --- common/lcd.c | 7 ++++--- drivers/video/amba.c | 2 -- drivers/video/atmel_hlcdfb.c | 2 -- drivers/video/atmel_lcdfb.c | 2 -- drivers/video/exynos_fb.c | 2 -- drivers/video/tegra.c | 4 +--- include/lcd.h | 6 +++--- 10 files changed, 8 insertions(+), 25 deletions(-)
diff --git a/arch/arm/cpu/pxa/pxafb.c b/arch/arm/cpu/pxa/pxafb.c index 987fa06..25747b1 100644 --- a/arch/arm/cpu/pxa/pxafb.c +++ b/arch/arm/cpu/pxa/pxafb.c @@ -333,8 +333,6 @@ void lcd_ctrl_init (void *lcdbase); void lcd_enable (void);
int lcd_line_length; -int lcd_color_fg; -int lcd_color_bg;
void *lcd_base; /* Start of framebuffer memory */ void *lcd_console_address; /* Start of console buffer */ diff --git a/arch/powerpc/cpu/mpc8xx/lcd.c b/arch/powerpc/cpu/mpc8xx/lcd.c index 4b88b21..4fd44ac 100644 --- a/arch/powerpc/cpu/mpc8xx/lcd.c +++ b/arch/powerpc/cpu/mpc8xx/lcd.c @@ -258,9 +258,6 @@ vidinfo_t panel_info = {
int lcd_line_length;
-int lcd_color_fg; -int lcd_color_bg; - /* * Frame buffer memory information */ diff --git a/board/mcc200/lcd.c b/board/mcc200/lcd.c index 893f4b7..0f3f585 100644 --- a/board/mcc200/lcd.c +++ b/board/mcc200/lcd.c @@ -70,9 +70,6 @@ vidinfo_t panel_info = {
int lcd_line_length;
-int lcd_color_fg; -int lcd_color_bg; - /* * Frame buffer memory information */ diff --git a/common/lcd.c b/common/lcd.c index 4778655..b67724e 100644 --- a/common/lcd.c +++ b/common/lcd.c @@ -97,6 +97,9 @@ static int lcd_getbgcolor(void); static void lcd_setfgcolor(int color); static void lcd_setbgcolor(int color);
+static int lcd_color_fg; +static int lcd_color_bg; + char lcd_is_enabled = 0;
static char lcd_flush_dcache; /* 1 to flush dcache after each lcd update */ @@ -532,12 +535,10 @@ static void lcd_setbgcolor(int color)
/*----------------------------------------------------------------------*/
-#ifdef NOT_USED_SO_FAR -static int lcd_getfgcolor(void) +int lcd_getfgcolor(void) { return lcd_color_fg; } -#endif /* NOT_USED_SO_FAR */
/*----------------------------------------------------------------------*/
diff --git a/drivers/video/amba.c b/drivers/video/amba.c index ffa1c39..b4fb47d 100644 --- a/drivers/video/amba.c +++ b/drivers/video/amba.c @@ -29,8 +29,6 @@
/* These variables are required by lcd.c -- although it sets them by itself */ int lcd_line_length; -int lcd_color_fg; -int lcd_color_bg; void *lcd_base; void *lcd_console_address; short console_col; diff --git a/drivers/video/atmel_hlcdfb.c b/drivers/video/atmel_hlcdfb.c index b10ca4b..e74eb65 100644 --- a/drivers/video/atmel_hlcdfb.c +++ b/drivers/video/atmel_hlcdfb.c @@ -30,8 +30,6 @@ #include <atmel_hlcdc.h>
int lcd_line_length; -int lcd_color_fg; -int lcd_color_bg;
void *lcd_base; /* Start of framebuffer memory */ void *lcd_console_address; /* Start of console buffer */ diff --git a/drivers/video/atmel_lcdfb.c b/drivers/video/atmel_lcdfb.c index c02ffd8..d96f175 100644 --- a/drivers/video/atmel_lcdfb.c +++ b/drivers/video/atmel_lcdfb.c @@ -30,8 +30,6 @@ #include <atmel_lcdc.h>
int lcd_line_length; -int lcd_color_fg; -int lcd_color_bg;
void *lcd_base; /* Start of framebuffer memory */ void *lcd_console_address; /* Start of console buffer */ diff --git a/drivers/video/exynos_fb.c b/drivers/video/exynos_fb.c index d9a3f9a..3dd6100 100644 --- a/drivers/video/exynos_fb.c +++ b/drivers/video/exynos_fb.c @@ -34,8 +34,6 @@ #include "exynos_fb.h"
int lcd_line_length; -int lcd_color_fg; -int lcd_color_bg;
void *lcd_base; void *lcd_console_address; diff --git a/drivers/video/tegra.c b/drivers/video/tegra.c index 750a283..3709d0b 100644 --- a/drivers/video/tegra.c +++ b/drivers/video/tegra.c @@ -61,8 +61,6 @@ enum { };
int lcd_line_length; -int lcd_color_fg; -int lcd_color_bg;
void *lcd_base; /* Start of framebuffer memory */ void *lcd_console_address; /* Start of console buffer */ @@ -108,7 +106,7 @@ void lcd_toggle_cursor(void)
for (i = 0; i < lcd_cursor_width; ++i) { color = *d; - color ^= lcd_color_fg; + color ^= lcd_getfgcolor(); *d = color; ++d; } diff --git a/include/lcd.h b/include/lcd.h index c24164a..7d8c41f 100644 --- a/include/lcd.h +++ b/include/lcd.h @@ -32,13 +32,11 @@ extern char lcd_is_enabled;
extern int lcd_line_length; -extern int lcd_color_fg; -extern int lcd_color_bg;
/* * Frame buffer memory information */ -extern void *lcd_base; /* Start of framebuffer memory */ +extern void *lcd_base; /* Start of framebuffer memory */ extern void *lcd_console_address; /* Start of console buffer */
extern short console_col; @@ -53,6 +51,8 @@ extern void lcd_setcolreg (ushort regno, ushort red, ushort green, ushort blue); extern void lcd_initcolregs (void);
+extern int lcd_getfgcolor(void); + /* gunzip_bmp used if CONFIG_VIDEO_BMP_GZIP */ extern struct bmp_image *gunzip_bmp(unsigned long addr, unsigned long *lenp); extern int bmp_display(ulong addr, int x, int y);
Dear Anatolij,
In message 1357415148-9243-1-git-send-email-wd@denx.de you wrote:
lcd_color_fg and lcd_color_bg had to be declared in board specific code, but were not actually used there; in addition, we have getter / setter functions for these, which were not used either.
Get rid of the global variables, and use the getter function where needed (so far no setter calls are needed).
A similar change could (and probably should?) be implemented for lcd_base, too - but thjis requires a little more code changes, plus the introduction of both getter and setter functions.
Do you think this should be done? If so, I would prepare a patch... [Otherwise I'd save the effort...]
Best regards,
Wolfgang Denk
On 01/05/2013 08:50 PM, Wolfgang Denk wrote:
Dear Anatolij,
In message 1357415148-9243-1-git-send-email-wd@denx.de you wrote:
lcd_color_fg and lcd_color_bg had to be declared in board specific code, but were not actually used there; in addition, we have getter / setter functions for these, which were not used either.
Get rid of the global variables, and use the getter function where needed (so far no setter calls are needed).
A similar change could (and probably should?) be implemented for lcd_base, too - but thjis requires a little more code changes, plus the introduction of both getter and setter functions.
Do you think this should be done? If so, I would prepare a patch... [Otherwise I'd save the effort...]
yes I think it should be done, unless Anatolij has a really good reason these variables are defined all over the place.
I have patches ready to remove allot more of these globals. Yet I have to test them. (and as this is cross arch / several patches this gona take some time)
Simon: I intend to remove you cursors stuff from tegra since it is not used anywhere. What is the intention of the cursor stuff?
The amba driver is scheduled for removal as well since it is not used.
Regards, Jeroen
Hi Jeroen,
On Sun, Jan 6, 2013 at 2:43 PM, Jeroen Hofstee jeroen@myspectrum.nl wrote:
On 01/05/2013 08:50 PM, Wolfgang Denk wrote:
Dear Anatolij,
In message 1357415148-9243-1-git-send-email-wd@denx.de you wrote:
lcd_color_fg and lcd_color_bg had to be declared in board specific code, but were not actually used there; in addition, we have getter / setter functions for these, which were not used either.
Get rid of the global variables, and use the getter function where needed (so far no setter calls are needed).
A similar change could (and probably should?) be implemented for lcd_base, too - but thjis requires a little more code changes, plus the introduction of both getter and setter functions.
Do you think this should be done? If so, I would prepare a patch... [Otherwise I'd save the effort...]
yes I think it should be done, unless Anatolij has a really good reason these variables are defined all over the place.
I have patches ready to remove allot more of these globals. Yet I have to test them. (and as this is cross arch / several patches this gona take some time)
Simon: I intend to remove you cursors stuff from tegra since it is not used anywhere. What is the intention of the cursor stuff?
It can't be important - let's remove it.
The amba driver is scheduled for removal as well since it is not used.
Regards, Jeroen
Regards,
Simon
Hello Wolfgang,
On Sat, 05 Jan 2013 20:50:01 +0100 Wolfgang Denk wd@denx.de wrote:
Dear Anatolij,
In message 1357415148-9243-1-git-send-email-wd@denx.de you wrote:
lcd_color_fg and lcd_color_bg had to be declared in board specific code, but were not actually used there; in addition, we have getter / setter functions for these, which were not used either.
Get rid of the global variables, and use the getter function where needed (so far no setter calls are needed).
A similar change could (and probably should?) be implemented for lcd_base, too - but thjis requires a little more code changes, plus the introduction of both getter and setter functions.
Do you think this should be done? If so, I would prepare a patch...
it should be done. And we probably do not need getter and setter functions, we can use gd->fb_addr directly. lcd_base is set to the value of gd->fb_base.
Thanks, Anatolij
On Sat, Jan 5, 2013 at 11:45 AM, Wolfgang Denk wd@denx.de wrote:
lcd_color_fg and lcd_color_bg had to be declared in board specific code, but were not actually used there; in addition, we have getter / setter functions for these, which were not used either.
Get rid of the global variables, and use the getter function where needed (so far no setter calls are needed).
Signed-off-by: Wolfgang Denk wd@denx.de Cc: Alessandro Rubini rubini@unipv.it Cc: Anatolij Gustschin agust@denx.de Cc: Bo Shen voice.shen@atmel.com Cc: Haavard Skinnemoen haavard.skinnemoen@atmel.com Cc: Kyungmin Park kyungmin.park@samsung.com Cc: Marek Vasut marek.vasut@gmail.com Cc: Minkyu Kang mk7.kang@samsung.com Cc: Nikita Kiryanov nikita@compulab.co.il Cc: Simon Glass sjg@chromium.org Cc: Stelian Pop stelian@popies.net Cc: Tom Warren twarren@nvidia.com
Acked-by: Simon Glass sjg@chromium.org
arch/arm/cpu/pxa/pxafb.c | 2 -- arch/powerpc/cpu/mpc8xx/lcd.c | 3 --- board/mcc200/lcd.c | 3 --- common/lcd.c | 7 ++++--- drivers/video/amba.c | 2 -- drivers/video/atmel_hlcdfb.c | 2 -- drivers/video/atmel_lcdfb.c | 2 -- drivers/video/exynos_fb.c | 2 -- drivers/video/tegra.c | 4 +--- include/lcd.h | 6 +++--- 10 files changed, 8 insertions(+), 25 deletions(-)
On 01/05/2013 08:45 PM, Wolfgang Denk wrote:
lcd_color_fg and lcd_color_bg had to be declared in board specific code, but were not actually used there; in addition, we have getter / setter functions for these, which were not used either.
Get rid of the global variables, and use the getter function where needed (so far no setter calls are needed).
Signed-off-by: Wolfgang Denk wd@denx.de Cc: Alessandro Rubini rubini@unipv.it Cc: Anatolij Gustschin agust@denx.de Cc: Bo Shen voice.shen@atmel.com Cc: Haavard Skinnemoen haavard.skinnemoen@atmel.com Cc: Kyungmin Park kyungmin.park@samsung.com Cc: Marek Vasut marek.vasut@gmail.com Cc: Minkyu Kang mk7.kang@samsung.com Cc: Nikita Kiryanov nikita@compulab.co.il Cc: Simon Glass sjg@chromium.org Cc: Stelian Pop stelian@popies.net Cc: Tom Warren twarren@nvidia.com
Acked-by: Jeroen Hofstee jeroen@myspectrum.nl
Hello Wolfgang,
On Sat, 5 Jan 2013 20:45:48 +0100 Wolfgang Denk wd@denx.de wrote:
lcd_color_fg and lcd_color_bg had to be declared in board specific code, but were not actually used there; in addition, we have getter / setter functions for these, which were not used either.
Get rid of the global variables, and use the getter function where needed (so far no setter calls are needed).
Signed-off-by: Wolfgang Denk wd@denx.de Cc: Alessandro Rubini rubini@unipv.it Cc: Anatolij Gustschin agust@denx.de Cc: Bo Shen voice.shen@atmel.com Cc: Haavard Skinnemoen haavard.skinnemoen@atmel.com Cc: Kyungmin Park kyungmin.park@samsung.com Cc: Marek Vasut marek.vasut@gmail.com Cc: Minkyu Kang mk7.kang@samsung.com Cc: Nikita Kiryanov nikita@compulab.co.il Cc: Simon Glass sjg@chromium.org Cc: Stelian Pop stelian@popies.net Cc: Tom Warren twarren@nvidia.com
arch/arm/cpu/pxa/pxafb.c | 2 -- arch/powerpc/cpu/mpc8xx/lcd.c | 3 --- board/mcc200/lcd.c | 3 --- common/lcd.c | 7 ++++--- drivers/video/amba.c | 2 -- drivers/video/atmel_hlcdfb.c | 2 -- drivers/video/atmel_lcdfb.c | 2 -- drivers/video/exynos_fb.c | 2 -- drivers/video/tegra.c | 4 +--- include/lcd.h | 6 +++--- 10 files changed, 8 insertions(+), 25 deletions(-)
Patch rebased and merged, thanks!
Anatolij
participants (4)
-
Anatolij Gustschin -
Jeroen Hofstee -
Simon Glass -
Wolfgang Denk