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

login
register
mail settings
Submitter Wolfgang Denk
Date Jan. 5, 2013, 7:45 p.m.
Message ID <1357415148-9243-1-git-send-email-wd@denx.de>
Download mbox | patch
Permalink /patch/209692/
State Accepted
Delegated to: Anatolij Gustschin
Headers show

Comments

Wolfgang Denk - Jan. 5, 2013, 7:45 p.m.
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(-)
Wolfgang Denk - Jan. 5, 2013, 7:50 p.m.
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
Jeroen Hofstee - Jan. 6, 2013, 10:43 p.m.
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
Anatolij Gustschin - Jan. 6, 2013, 11:33 p.m.
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
Simon Glass - Jan. 12, 2013, 5:06 p.m.
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(-)
Simon Glass - Jan. 12, 2013, 5:07 p.m.
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
Jeroen Hofstee - Jan. 25, 2013, 9:26 p.m.
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>
Anatolij Gustschin - March 29, 2013, 10:56 a.m.
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

Patch

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