global variables from comon/lcd.c
Subsystems (6)
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,
Quoted 6 lines · In message <[email protected]> you wrote:
While looking at the
common/lcd.cI fail to understand why it declares and uses global variables likelcd_color_fg,lcd_color_color_bg,lcd_baseand relies on the drivers / boards to provide them, leading to code duplication. It also provides getters / setters for some of these variables.
Quoted 6 lines
To me it seems more logical to have these variables in
lcd.citself. 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.citself?
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
Signature · 5 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Veni, Vidi, VISA:
I came, I saw, I did a little shopping.PATCH 1/1 common/lcd.c: cleanup use of global variables
In reply to Jeroen Hofsteelcd_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).
--- 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(-)
arch/arm/cpu/pxa/pxafb.c +0 -2
@@ -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 */
arch/powerpc/cpu/mpc8xx/lcd.c +0 -3
@@ -258,9 +258,6 @@ vidinfo_t panel_info = {int lcd_line_length; -int lcd_color_fg; -int lcd_color_bg; -/* * Frame buffer memory information */
board/mcc200/lcd.c +0 -3
@@ -70,9 +70,6 @@ vidinfo_t panel_info = {int lcd_line_length; -int lcd_color_fg; -int lcd_color_bg; -/* * Frame buffer memory information */
common/lcd.c +4 -3
@@ -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 */ /*----------------------------------------------------------------------*/
drivers/video/amba.c +0 -2
@@ -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;
drivers/video/atmel_hlcdfb.c +0 -2
@@ -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 */
drivers/video/atmel_lcdfb.c +0 -2
@@ -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 */
drivers/video/exynos_fb.c +0 -2
@@ -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;
drivers/video/tegra.c +1 -3
@@ -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; }
include/lcd.h +3 -3
@@ -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);
Signature · 1 line
1.7.11.7
PATCH 1/1 common/lcd.c: cleanup use of global variables
Dear Anatolij,
Quoted 6 lines · In message <[email protected]> you wrote:
lcd_color_fgandlcd_color_bghad 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
Signature · 5 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de Motto of the Electrical Engineer: Working computer hardware is a lot like an erect penis: it stays up as long as you don't fuck with it.
PATCH 1/1 common/lcd.c: cleanup use of global variables
Quoted 7 lines · Wolfgang Denk
Dear Anatolij,
Quoted 4 lines · In message <[email protected]> you wrote:
lcd_color_fgandlcd_color_bghad 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.
Quoted 8 lines
Quoted 2 lines
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
PATCH 1/1 common/lcd.c: cleanup use of global variables
In reply to Wolfgang DenkHello Wolfgang,
Quoted 7 lines · On Sat, 05 Jan 2013 20:50:01 +0100 Wolfgang Denk <[email protected]> wrote:
Dear Anatolij,
Quoted 4 lines · In message <[email protected]> you wrote:
lcd_color_fgandlcd_color_bghad 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.
Quoted 8 lines
Quoted 2 lines
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
PATCH 1/1 common/lcd.c: cleanup use of global variables
In reply to Wolfgang DenkQuoted 7 lines · Wolfgang Denk
lcd_color_fgandlcd_color_bghad 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).
Quoted 12 lines
Signed-off-by Wolfgang Denk <[email protected]>Cc Alessandro Rubini <[email protected]>Cc Anatolij Gustschin <[email protected]>Cc Bo Shen <[email protected]>Cc Haavard Skinnemoen <[email protected]>Cc Kyungmin Park <[email protected]>Cc Marek Vasut <[email protected]>Cc Minkyu Kang <[email protected]>Cc Nikita Kiryanov <[email protected]>Cc Simon Glass <[email protected]>Cc Stelian Pop <[email protected]>Cc Tom Warren <[email protected]>
Quoted 12 lines
--- 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 1/1 common/lcd.c: cleanup use of global variables
In reply to Jeroen HofsteeHi Jeroen,
Quoted 23 lines · Jeroen Hofstee
Quoted 12 lines · Wolfgang Denk
Dear Anatolij,
Quoted 7 lines · In message <[email protected]> you wrote:
lcd_color_fgandlcd_color_bghad 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).
Quoted 6 lines
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.
Quoted 6 lines
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.
Quoted 6 lines
The amba driver is scheduled for removal as well since it is not used.
Regards,
Jeroen
Regards,
Simon
PATCH 1/1 common/lcd.c: cleanup use of global variables
In reply to Wolfgang DenkQuoted 19 lines · Wolfgang Denk
lcd_color_fgandlcd_color_bghad 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 <[email protected]>Cc Alessandro Rubini <[email protected]>Cc Anatolij Gustschin <[email protected]>Cc Bo Shen <[email protected]>Cc Haavard Skinnemoen <[email protected]>Cc Kyungmin Park <[email protected]>Cc Marek Vasut <[email protected]>Cc Minkyu Kang <[email protected]>Cc Nikita Kiryanov <[email protected]>Cc Simon Glass <[email protected]>Cc Stelian Pop <[email protected]>Cc Tom Warren <[email protected]>
Quoted 2 lines
---
PATCH 1/1 common/lcd.c: cleanup use of global variables
In reply to Wolfgang DenkHello Wolfgang,
Quoted 19 lines · On Sat, 5 Jan 2013 20:45:48 +0100 Wolfgang Denk <[email protected]> wrote:
lcd_color_fgandlcd_color_bghad 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 <[email protected]>Cc Alessandro Rubini <[email protected]>Cc Anatolij Gustschin <[email protected]>Cc Bo Shen <[email protected]>Cc Haavard Skinnemoen <[email protected]>Cc Kyungmin Park <[email protected]>Cc Marek Vasut <[email protected]>Cc Minkyu Kang <[email protected]>Cc Nikita Kiryanov <[email protected]>Cc Simon Glass <[email protected]>Cc Stelian Pop <[email protected]>Cc Tom Warren <[email protected]>
Quoted 12 lines
--- 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