[U-Boot] global variables from comon/lcd.c

[U-Boot] global variables from comon/lcd.c

[Jeroen Hofstee]

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

[U-Boot] global variables from comon/lcd.c

[Wolfgang Denk]

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

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

[U-Boot] [PATCH] common/lcd.c: cleanup use of global variables

[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);
-- 
1.7.11.7

[U-Boot] [PATCH] common/lcd.c: cleanup use of global variables

[Wolfgang Denk]

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

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

[U-Boot] [PATCH] common/lcd.c: cleanup use of global variables

[Jeroen Hofstee]

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

[U-Boot] [PATCH] common/lcd.c: cleanup use of global variables

[Anatolij Gustschin]

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

[U-Boot] [PATCH] common/lcd.c: cleanup use of global variables

[Simon Glass]

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

[U-Boot] [PATCH] common/lcd.c: cleanup use of global variables

[Simon Glass]

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

[U-Boot] [PATCH] common/lcd.c: cleanup use of global variables

[Jeroen Hofstee]

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>

[U-Boot] [PATCH] common/lcd.c: cleanup use of global variables

[Anatolij Gustschin]

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