[PATCH 0/9] staging: panel: Refactor panel initialization

[PATCH 0/9] staging: panel: Refactor panel initialization

[Mariusz Gorski]

This set of patches focuses on making current initialization
process easier to understand - especially it tries to emphasize
what are the priorities of the params coming from different
sources (Kconfig values, device profiles and module param values
set on loading). I paid attention not to change to behaviour of
the code itself (at least for now), so all hacky places are kept.

Mariusz Gorski (9):
  staging: panel: Set default parport module param value
  staging: panel: Call init function directly
  staging: panel: Remove magic numbers
  staging: panel: Use a macro for checking module params state
  staging: panel: Start making module params read-only
  staging: panel: Make two more module params read-only
  staging: panel: Refactor LCD init code
  staging: panel: Remove more magic number comparison
  staging: panel: Move LCD-related state into struct lcd

 drivers/staging/panel/panel.c | 672 ++++++++++++++++++++++--------------------
 1 file changed, 357 insertions(+), 315 deletions(-)

-- 
2.1.3

[PATCH 1/9] staging: panel: Set default parport module param value

[Mariusz Gorski]

Set default parport module param value to DEFAULT_PARPORT so that
a if-block can be avoided.

Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
---
 drivers/staging/panel/panel.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index c6eeddf..3dd318a 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -429,7 +429,7 @@ static struct timer_list scan_timer;
 
 MODULE_DESCRIPTION("Generic parallel port LCD/Keypad driver");
 
-static int parport = -1;
+static int parport = DEFAULT_PARPORT;
 module_param(parport, int, 0000);
 MODULE_PARM_DESC(parport, "Parallel port index (0=lpt1, 1=lpt2, ...)");
 
@@ -2231,9 +2231,6 @@ static int panel_init(void)
 	if (lcd_type < 0)
 		lcd_type = lcd_enabled;
 
-	if (parport < 0)
-		parport = DEFAULT_PARPORT;
-
 	/* take care of an eventual profile */
 	switch (profile) {
 	case PANEL_PROFILE_CUSTOM:
-- 
2.1.3

Re: [PATCH 1/9] staging: panel: Set default parport module param value

[Willy Tarreau]

On Tue, Nov 18, 2014 at 09:56:06PM +0100, Mariusz Gorski wrote:
> Set default parport module param value to DEFAULT_PARPORT so that
> a if-block can be avoided.
> 
> Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>

Acked-by: Willy Tarreau <w@1wt.eu>

[PATCH 2/9] staging: panel: Call init function directly

[Mariusz Gorski]

Remove useless function and let the kernel call the actual
init function directly.

Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
---
 drivers/staging/panel/panel.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 3dd318a..0d3f09e 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -2222,7 +2222,7 @@ static struct parport_driver panel_driver = {
 };
 
 /* init function */
-static int panel_init(void)
+static int __init panel_init_module(void)
 {
 	/* for backwards compatibility */
 	if (keypad_type < 0)
@@ -2334,11 +2334,6 @@ static int panel_init(void)
 	return 0;
 }
 
-static int __init panel_init_module(void)
-{
-	return panel_init();
-}
-
 static void __exit panel_cleanup_module(void)
 {
 	unregister_reboot_notifier(&panel_notifier);
-- 
2.1.3

Re: [PATCH 2/9] staging: panel: Call init function directly

[Willy Tarreau]

On Tue, Nov 18, 2014 at 09:56:07PM +0100, Mariusz Gorski wrote:
> Remove useless function and let the kernel call the actual
> init function directly.
> 
> Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>

Acked-by: Willy Tarreau <w@1wt.eu>

[PATCH 3/9] staging: panel: Remove magic numbers

[Mariusz Gorski]

Get rid of magic numbers indicating that the value of a module param
is not set. Use a defined value instead.

Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
---
 drivers/staging/panel/panel.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 0d3f09e..1b4a211 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -133,6 +133,8 @@
 #define LCD_ESCAPE_LEN		24	/* max chars for LCD escape command */
 #define LCD_ESCAPE_CHAR	27	/* use char 27 for escape command */
 
+#define NOT_SET			-1
+
 /* macros to simplify use of the parallel port */
 #define r_ctr(x)        (parport_read_control((x)->port))
 #define r_dtr(x)        (parport_read_data((x)->port))
@@ -439,37 +441,37 @@ MODULE_PARM_DESC(profile,
 		 "1=16x2 old kp; 2=serial 16x2, new kp; 3=16x2 hantronix; "
 		 "4=16x2 nexcom; default=40x2, old kp");
 
-static int keypad_type = -1;
+static int keypad_type = NOT_SET;
 module_param(keypad_type, int, 0000);
 MODULE_PARM_DESC(keypad_type,
 		 "Keypad type: 0=none, 1=old 6 keys, 2=new 6+1 keys, 3=nexcom 4 keys");
 
-static int lcd_type = -1;
+static int lcd_type = NOT_SET;
 module_param(lcd_type, int, 0000);
 MODULE_PARM_DESC(lcd_type,
 		 "LCD type: 0=none, 1=old //, 2=serial ks0074, 3=hantronix //, 4=nexcom //, 5=compiled-in");
 
-static int lcd_height = -1;
+static int lcd_height = NOT_SET;
 module_param(lcd_height, int, 0000);
 MODULE_PARM_DESC(lcd_height, "Number of lines on the LCD");
 
-static int lcd_width = -1;
+static int lcd_width = NOT_SET;
 module_param(lcd_width, int, 0000);
 MODULE_PARM_DESC(lcd_width, "Number of columns on the LCD");
 
-static int lcd_bwidth = -1;	/* internal buffer width (usually 40) */
+static int lcd_bwidth = NOT_SET;	/* internal buffer width (usually 40) */
 module_param(lcd_bwidth, int, 0000);
 MODULE_PARM_DESC(lcd_bwidth, "Internal LCD line width (40)");
 
-static int lcd_hwidth = -1;	/* hardware buffer width (usually 64) */
+static int lcd_hwidth = NOT_SET;	/* hardware buffer width (usually 64) */
 module_param(lcd_hwidth, int, 0000);
 MODULE_PARM_DESC(lcd_hwidth, "LCD line hardware address (64)");
 
-static int lcd_charset = -1;
+static int lcd_charset = NOT_SET;
 module_param(lcd_charset, int, 0000);
 MODULE_PARM_DESC(lcd_charset, "LCD character set: 0=standard, 1=KS0074");
 
-static int lcd_proto = -1;
+static int lcd_proto = NOT_SET;
 module_param(lcd_proto, int, 0000);
 MODULE_PARM_DESC(lcd_proto,
 		 "LCD communication: 0=parallel (//), 1=serial, 2=TI LCD Interface");
@@ -515,11 +517,11 @@ MODULE_PARM_DESC(lcd_bl_pin,
 
 /* Deprecated module parameters - consider not using them anymore */
 
-static int lcd_enabled = -1;
+static int lcd_enabled = NOT_SET;
 module_param(lcd_enabled, int, 0000);
 MODULE_PARM_DESC(lcd_enabled, "Deprecated option, use lcd_type instead");
 
-static int keypad_enabled = -1;
+static int keypad_enabled = NOT_SET;
 module_param(keypad_enabled, int, 0000);
 MODULE_PARM_DESC(keypad_enabled, "Deprecated option, use keypad_type instead");
 
-- 
2.1.3

Re: [PATCH 3/9] staging: panel: Remove magic numbers

[Willy Tarreau]

On Tue, Nov 18, 2014 at 09:56:08PM +0100, Mariusz Gorski wrote:
> Get rid of magic numbers indicating that the value of a module param
> is not set. Use a defined value instead.
> 
> Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>

Acked-by: Willy Tarreau <w@1wt.eu>

[PATCH 4/9] staging: panel: Use a macro for checking module params state

[Mariusz Gorski]

Avoid values comparison and use a macro instead that checks
whether module param has been set by the user to some value
at loading time.

Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
---
 drivers/staging/panel/panel.c | 88 ++++++++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 43 deletions(-)

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 1b4a211..97fc4ca 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -135,6 +135,8 @@
 
 #define NOT_SET			-1
 
+#define IS_NOT_SET(x)	(x == NOT_SET)
+
 /* macros to simplify use of the parallel port */
 #define r_ctr(x)        (parport_read_control((x)->port))
 #define r_dtr(x)        (parport_read_data((x)->port))
@@ -1411,29 +1413,29 @@ static void lcd_init(void)
 	switch (lcd_type) {
 	case LCD_TYPE_OLD:
 		/* parallel mode, 8 bits */
-		if (lcd_proto < 0)
+		if (IS_NOT_SET(lcd_proto))
 			lcd_proto = LCD_PROTO_PARALLEL;
-		if (lcd_charset < 0)
+		if (IS_NOT_SET(lcd_charset))
 			lcd_charset = LCD_CHARSET_NORMAL;
 		if (lcd_e_pin == PIN_NOT_SET)
 			lcd_e_pin = PIN_STROBE;
 		if (lcd_rs_pin == PIN_NOT_SET)
 			lcd_rs_pin = PIN_AUTOLF;
 
-		if (lcd_width < 0)
+		if (IS_NOT_SET(lcd_width))
 			lcd_width = 40;
-		if (lcd_bwidth < 0)
+		if (IS_NOT_SET(lcd_bwidth))
 			lcd_bwidth = 40;
-		if (lcd_hwidth < 0)
+		if (IS_NOT_SET(lcd_hwidth))
 			lcd_hwidth = 64;
-		if (lcd_height < 0)
+		if (IS_NOT_SET(lcd_height))
 			lcd_height = 2;
 		break;
 	case LCD_TYPE_KS0074:
 		/* serial mode, ks0074 */
-		if (lcd_proto < 0)
+		if (IS_NOT_SET(lcd_proto))
 			lcd_proto = LCD_PROTO_SERIAL;
-		if (lcd_charset < 0)
+		if (IS_NOT_SET(lcd_charset))
 			lcd_charset = LCD_CHARSET_KS0074;
 		if (lcd_bl_pin == PIN_NOT_SET)
 			lcd_bl_pin = PIN_AUTOLF;
@@ -1442,20 +1444,20 @@ static void lcd_init(void)
 		if (lcd_da_pin == PIN_NOT_SET)
 			lcd_da_pin = PIN_D0;
 
-		if (lcd_width < 0)
+		if (IS_NOT_SET(lcd_width))
 			lcd_width = 16;
-		if (lcd_bwidth < 0)
+		if (IS_NOT_SET(lcd_bwidth))
 			lcd_bwidth = 40;
-		if (lcd_hwidth < 0)
+		if (IS_NOT_SET(lcd_hwidth))
 			lcd_hwidth = 16;
-		if (lcd_height < 0)
+		if (IS_NOT_SET(lcd_height))
 			lcd_height = 2;
 		break;
 	case LCD_TYPE_NEXCOM:
 		/* parallel mode, 8 bits, generic */
-		if (lcd_proto < 0)
+		if (IS_NOT_SET(lcd_proto))
 			lcd_proto = LCD_PROTO_PARALLEL;
-		if (lcd_charset < 0)
+		if (IS_NOT_SET(lcd_charset))
 			lcd_charset = LCD_CHARSET_NORMAL;
 		if (lcd_e_pin == PIN_NOT_SET)
 			lcd_e_pin = PIN_AUTOLF;
@@ -1464,42 +1466,42 @@ static void lcd_init(void)
 		if (lcd_rw_pin == PIN_NOT_SET)
 			lcd_rw_pin = PIN_INITP;
 
-		if (lcd_width < 0)
+		if (IS_NOT_SET(lcd_width))
 			lcd_width = 16;
-		if (lcd_bwidth < 0)
+		if (IS_NOT_SET(lcd_bwidth))
 			lcd_bwidth = 40;
-		if (lcd_hwidth < 0)
+		if (IS_NOT_SET(lcd_hwidth))
 			lcd_hwidth = 64;
-		if (lcd_height < 0)
+		if (IS_NOT_SET(lcd_height))
 			lcd_height = 2;
 		break;
 	case LCD_TYPE_CUSTOM:
 		/* customer-defined */
-		if (lcd_proto < 0)
+		if (IS_NOT_SET(lcd_proto))
 			lcd_proto = DEFAULT_LCD_PROTO;
-		if (lcd_charset < 0)
+		if (IS_NOT_SET(lcd_charset))
 			lcd_charset = DEFAULT_LCD_CHARSET;
 		/* default geometry will be set later */
 		break;
 	case LCD_TYPE_HANTRONIX:
 		/* parallel mode, 8 bits, hantronix-like */
 	default:
-		if (lcd_proto < 0)
+		if (IS_NOT_SET(lcd_proto))
 			lcd_proto = LCD_PROTO_PARALLEL;
-		if (lcd_charset < 0)
+		if (IS_NOT_SET(lcd_charset))
 			lcd_charset = LCD_CHARSET_NORMAL;
 		if (lcd_e_pin == PIN_NOT_SET)
 			lcd_e_pin = PIN_STROBE;
 		if (lcd_rs_pin == PIN_NOT_SET)
 			lcd_rs_pin = PIN_SELECP;
 
-		if (lcd_width < 0)
+		if (IS_NOT_SET(lcd_width))
 			lcd_width = 16;
-		if (lcd_bwidth < 0)
+		if (IS_NOT_SET(lcd_bwidth))
 			lcd_bwidth = 40;
-		if (lcd_hwidth < 0)
+		if (IS_NOT_SET(lcd_hwidth))
 			lcd_hwidth = 64;
-		if (lcd_height < 0)
+		if (IS_NOT_SET(lcd_height))
 			lcd_height = 2;
 		break;
 	}
@@ -1557,7 +1559,7 @@ static void lcd_init(void)
 	if (lcd_da_pin == PIN_NOT_SET)
 		lcd_da_pin = PIN_NONE;
 
-	if (lcd_charset < 0)
+	if (IS_NOT_SET(lcd_charset))
 		lcd_charset = DEFAULT_LCD_CHARSET;
 
 	if (lcd_charset == LCD_CHARSET_KS0074)
@@ -2227,58 +2229,58 @@ static struct parport_driver panel_driver = {
 static int __init panel_init_module(void)
 {
 	/* for backwards compatibility */
-	if (keypad_type < 0)
+	if (IS_NOT_SET(keypad_type))
 		keypad_type = keypad_enabled;
 
-	if (lcd_type < 0)
+	if (IS_NOT_SET(lcd_type))
 		lcd_type = lcd_enabled;
 
 	/* take care of an eventual profile */
 	switch (profile) {
 	case PANEL_PROFILE_CUSTOM:
 		/* custom profile */
-		if (keypad_type < 0)
+		if (IS_NOT_SET(keypad_type))
 			keypad_type = DEFAULT_KEYPAD_TYPE;
-		if (lcd_type < 0)
+		if (IS_NOT_SET(lcd_type))
 			lcd_type = DEFAULT_LCD_TYPE;
 		break;
 	case PANEL_PROFILE_OLD:
 		/* 8 bits, 2*16, old keypad */
-		if (keypad_type < 0)
+		if (IS_NOT_SET(keypad_type))
 			keypad_type = KEYPAD_TYPE_OLD;
-		if (lcd_type < 0)
+		if (IS_NOT_SET(lcd_type))
 			lcd_type = LCD_TYPE_OLD;
-		if (lcd_width < 0)
+		if (IS_NOT_SET(lcd_width))
 			lcd_width = 16;
-		if (lcd_hwidth < 0)
+		if (IS_NOT_SET(lcd_hwidth))
 			lcd_hwidth = 16;
 		break;
 	case PANEL_PROFILE_NEW:
 		/* serial, 2*16, new keypad */
-		if (keypad_type < 0)
+		if (IS_NOT_SET(keypad_type))
 			keypad_type = KEYPAD_TYPE_NEW;
-		if (lcd_type < 0)
+		if (IS_NOT_SET(lcd_type))
 			lcd_type = LCD_TYPE_KS0074;
 		break;
 	case PANEL_PROFILE_HANTRONIX:
 		/* 8 bits, 2*16 hantronix-like, no keypad */
-		if (keypad_type < 0)
+		if (IS_NOT_SET(keypad_type))
 			keypad_type = KEYPAD_TYPE_NONE;
-		if (lcd_type < 0)
+		if (IS_NOT_SET(lcd_type))
 			lcd_type = LCD_TYPE_HANTRONIX;
 		break;
 	case PANEL_PROFILE_NEXCOM:
 		/* generic 8 bits, 2*16, nexcom keypad, eg. Nexcom. */
-		if (keypad_type < 0)
+		if (IS_NOT_SET(keypad_type))
 			keypad_type = KEYPAD_TYPE_NEXCOM;
-		if (lcd_type < 0)
+		if (IS_NOT_SET(lcd_type))
 			lcd_type = LCD_TYPE_NEXCOM;
 		break;
 	case PANEL_PROFILE_LARGE:
 		/* 8 bits, 2*40, old keypad */
-		if (keypad_type < 0)
+		if (IS_NOT_SET(keypad_type))
 			keypad_type = KEYPAD_TYPE_OLD;
-		if (lcd_type < 0)
+		if (IS_NOT_SET(lcd_type))
 			lcd_type = LCD_TYPE_OLD;
 		break;
 	}
-- 
2.1.3

Re: [PATCH 4/9] staging: panel: Use a macro for checking module params state

[Willy Tarreau]

On Tue, Nov 18, 2014 at 09:56:09PM +0100, Mariusz Gorski wrote:
> Avoid values comparison and use a macro instead that checks
> whether module param has been set by the user to some value
> at loading time.
> 
> Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
> ---
>  drivers/staging/panel/panel.c | 88 ++++++++++++++++++++++---------------------
>  1 file changed, 45 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> index 1b4a211..97fc4ca 100644
> --- a/drivers/staging/panel/panel.c
> +++ b/drivers/staging/panel/panel.c
> @@ -135,6 +135,8 @@
>  
>  #define NOT_SET			-1
>  
> +#define IS_NOT_SET(x)	(x == NOT_SET)
> +
>  /* macros to simplify use of the parallel port */
>  #define r_ctr(x)        (parport_read_control((x)->port))
>  #define r_dtr(x)        (parport_read_data((x)->port))
> @@ -1411,29 +1413,29 @@ static void lcd_init(void)
>  	switch (lcd_type) {
>  	case LCD_TYPE_OLD:
>  		/* parallel mode, 8 bits */
> -		if (lcd_proto < 0)
> +		if (IS_NOT_SET(lcd_proto))
>  			lcd_proto = LCD_PROTO_PARALLEL;
> -		if (lcd_charset < 0)
> +		if (IS_NOT_SET(lcd_charset))
>  			lcd_charset = LCD_CHARSET_NORMAL;
>  		if (lcd_e_pin == PIN_NOT_SET)
>  			lcd_e_pin = PIN_STROBE;
>  		if (lcd_rs_pin == PIN_NOT_SET)
>  			lcd_rs_pin = PIN_AUTOLF;
>  
> -		if (lcd_width < 0)
> +		if (IS_NOT_SET(lcd_width))
>  			lcd_width = 40;
(...)

I'm not convinced at all by the increased readability of this one.
To me it adds obfuscation since I have to look for the macro definition
to see how it checks whether the type is set or not.

I think you'd rather simply open-code the macro here and keep the natural
comparison. The fields were already initialized to the NOT_SET value, better
check agaist the same value here especially since it matches other tests as
well. That would give :

-		if (lcd_proto < 0)
+		if (lcd_proto == NOT_SET)
  			lcd_proto = LCD_PROTO_PARALLEL;

etc...  To me it's more readable.

Willy

Re: [PATCH 4/9] staging: panel: Use a macro for checking module params state

[Mariusz Gorski]

On Tue, Nov 18, 2014 at 10:18:44PM +0100, Willy Tarreau wrote:
> On Tue, Nov 18, 2014 at 09:56:09PM +0100, Mariusz Gorski wrote:
> > Avoid values comparison and use a macro instead that checks
> > whether module param has been set by the user to some value
> > at loading time.
> > 
> > Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
> > ---
> >  drivers/staging/panel/panel.c | 88 ++++++++++++++++++++++---------------------
> >  1 file changed, 45 insertions(+), 43 deletions(-)
> > 
> > diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> > index 1b4a211..97fc4ca 100644
> > --- a/drivers/staging/panel/panel.c
> > +++ b/drivers/staging/panel/panel.c
> > @@ -135,6 +135,8 @@
> >  
> >  #define NOT_SET			-1
> >  
> > +#define IS_NOT_SET(x)	(x == NOT_SET)
> > +
> >  /* macros to simplify use of the parallel port */
> >  #define r_ctr(x)        (parport_read_control((x)->port))
> >  #define r_dtr(x)        (parport_read_data((x)->port))
> > @@ -1411,29 +1413,29 @@ static void lcd_init(void)
> >  	switch (lcd_type) {
> >  	case LCD_TYPE_OLD:
> >  		/* parallel mode, 8 bits */
> > -		if (lcd_proto < 0)
> > +		if (IS_NOT_SET(lcd_proto))
> >  			lcd_proto = LCD_PROTO_PARALLEL;
> > -		if (lcd_charset < 0)
> > +		if (IS_NOT_SET(lcd_charset))
> >  			lcd_charset = LCD_CHARSET_NORMAL;
> >  		if (lcd_e_pin == PIN_NOT_SET)
> >  			lcd_e_pin = PIN_STROBE;
> >  		if (lcd_rs_pin == PIN_NOT_SET)
> >  			lcd_rs_pin = PIN_AUTOLF;
> >  
> > -		if (lcd_width < 0)
> > +		if (IS_NOT_SET(lcd_width))
> >  			lcd_width = 40;
> (...)
> 
> I'm not convinced at all by the increased readability of this one.
> To me it adds obfuscation since I have to look for the macro definition
> to see how it checks whether the type is set or not.
> 
> I think you'd rather simply open-code the macro here and keep the natural
> comparison. The fields were already initialized to the NOT_SET value, better
> check agaist the same value here especially since it matches other tests as
> well. That would give :
> 
> -		if (lcd_proto < 0)
> +		if (lcd_proto == NOT_SET)
>   			lcd_proto = LCD_PROTO_PARALLEL;
> 
> etc...  To me it's more readable.
> 
> Willy
>
Hmm ok maybe you're right, maybe I've tried to hide too much here.
"x == NOT_SET" hides already enough ;) I'll fix that.

Thanks,
Mariusz

[PATCH 5/9] staging: panel: Start making module params read-only

[Mariusz Gorski]

Start decoupling module params from the actual device state,
both for lcd and keypad, by keeping the params read-only
and moving the device state to related structs.

Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
---
 drivers/staging/panel/panel.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 97fc4ca..7f2f5f8 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -214,6 +214,10 @@ static pmask_t phys_prev;
 static char inputs_stable;
 
 /* these variables are specific to the keypad */
+static struct {
+	bool enabled;
+} keypad;
+
 static char keypad_buffer[KEYPAD_BUFFER];
 static int keypad_buflen;
 static int keypad_start;
@@ -221,6 +225,9 @@ static char keypressed;
 static wait_queue_head_t keypad_read_wait;
 
 /* lcd-specific variables */
+static struct {
+	bool enabled;
+} lcd;
 
 /* contains the LCD config state */
 static unsigned long int lcd_flags;
@@ -1395,7 +1402,7 @@ static void panel_lcd_print(const char *s)
 	const char *tmp = s;
 	int count = strlen(s);
 
-	if (lcd_enabled && lcd_initialized) {
+	if (lcd.enabled && lcd_initialized) {
 		for (; count-- > 0; tmp++) {
 			if (!in_interrupt() && (((count + 1) & 0x1f) == 0))
 				/* let's be a little nice with other processes
@@ -1925,7 +1932,7 @@ static void panel_process_inputs(void)
 
 static void panel_scan_timer(void)
 {
-	if (keypad_enabled && keypad_initialized) {
+	if (keypad.enabled && keypad_initialized) {
 		if (spin_trylock_irq(&pprt_lock)) {
 			phys_scan_contacts();
 
@@ -1937,7 +1944,7 @@ static void panel_scan_timer(void)
 			panel_process_inputs();
 	}
 
-	if (lcd_enabled && lcd_initialized) {
+	if (lcd.enabled && lcd_initialized) {
 		if (keypressed) {
 			if (light_tempo == 0 && ((lcd_flags & LCD_FLAG_L) == 0))
 				lcd_backlight(1);
@@ -2116,7 +2123,7 @@ static void keypad_init(void)
 static int panel_notify_sys(struct notifier_block *this, unsigned long code,
 			    void *unused)
 {
-	if (lcd_enabled && lcd_initialized) {
+	if (lcd.enabled && lcd_initialized) {
 		switch (code) {
 		case SYS_DOWN:
 			panel_lcd_print
@@ -2172,13 +2179,13 @@ static void panel_attach(struct parport *port)
 	/* must init LCD first, just in case an IRQ from the keypad is
 	 * generated at keypad init
 	 */
-	if (lcd_enabled) {
+	if (lcd.enabled) {
 		lcd_init();
 		if (misc_register(&lcd_dev))
 			goto err_unreg_device;
 	}
 
-	if (keypad_enabled) {
+	if (keypad.enabled) {
 		keypad_init();
 		if (misc_register(&keypad_dev))
 			goto err_lcd_unreg;
@@ -2186,7 +2193,7 @@ static void panel_attach(struct parport *port)
 	return;
 
 err_lcd_unreg:
-	if (lcd_enabled)
+	if (lcd.enabled)
 		misc_deregister(&lcd_dev);
 err_unreg_device:
 	parport_unregister_device(pprt);
@@ -2204,12 +2211,12 @@ static void panel_detach(struct parport *port)
 		return;
 	}
 
-	if (keypad_enabled && keypad_initialized) {
+	if (keypad.enabled && keypad_initialized) {
 		misc_deregister(&keypad_dev);
 		keypad_initialized = 0;
 	}
 
-	if (lcd_enabled && lcd_initialized) {
+	if (lcd.enabled && lcd_initialized) {
 		misc_deregister(&lcd_dev);
 		lcd_initialized = 0;
 	}
@@ -2285,8 +2292,8 @@ static int __init panel_init_module(void)
 		break;
 	}
 
-	lcd_enabled = (lcd_type > 0);
-	keypad_enabled = (keypad_type > 0);
+	lcd.enabled = (lcd_type > 0);
+	keypad.enabled = (keypad_type > 0);
 
 	switch (keypad_type) {
 	case KEYPAD_TYPE_OLD:
@@ -2311,7 +2318,7 @@ static int __init panel_init_module(void)
 		return -EIO;
 	}
 
-	if (!lcd_enabled && !keypad_enabled) {
+	if (!lcd.enabled && !keypad.enabled) {
 		/* no device enabled, let's release the parport */
 		if (pprt) {
 			parport_release(pprt);
@@ -2346,12 +2353,12 @@ static void __exit panel_cleanup_module(void)
 		del_timer_sync(&scan_timer);
 
 	if (pprt != NULL) {
-		if (keypad_enabled) {
+		if (keypad.enabled) {
 			misc_deregister(&keypad_dev);
 			keypad_initialized = 0;
 		}
 
-		if (lcd_enabled) {
+		if (lcd.enabled) {
 			panel_lcd_print("\x0cLCD driver " PANEL_VERSION
 					"\nunloaded.\x1b[Lc\x1b[Lb\x1b[L-");
 			misc_deregister(&lcd_dev);
-- 
2.1.3

Re: [PATCH 5/9] staging: panel: Start making module params read-only

[Willy Tarreau]

On Tue, Nov 18, 2014 at 09:56:10PM +0100, Mariusz Gorski wrote:
> Start decoupling module params from the actual device state,
> both for lcd and keypad, by keeping the params read-only
> and moving the device state to related structs.
> 
> Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>

Acked-by: Willy Tarreau <w@1wt.eu>

[PATCH 6/9] staging: panel: Make two more module params read-only

[Mariusz Gorski]

Make keypad_type and lcd_type module params read-only.
This step also starts making it more clear what is
the precedence of device params coming from different
sources (device profile, runtime module param values etc).

Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
---
 drivers/staging/panel/panel.c | 71 ++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 34 deletions(-)

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 7f2f5f8..5b4f0a4 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -229,6 +229,9 @@ static struct {
 	bool enabled;
 } lcd;
 
+/* Needed only for init */
+static int selected_lcd_type = NOT_SET;
+
 /* contains the LCD config state */
 static unsigned long int lcd_flags;
 /* contains the LCD X offset */
@@ -1417,7 +1420,7 @@ static void panel_lcd_print(const char *s)
 /* initialize the LCD driver */
 static void lcd_init(void)
 {
-	switch (lcd_type) {
+	switch (selected_lcd_type) {
 	case LCD_TYPE_OLD:
 		/* parallel mode, 8 bits */
 		if (IS_NOT_SET(lcd_proto))
@@ -2235,28 +2238,21 @@ static struct parport_driver panel_driver = {
 /* init function */
 static int __init panel_init_module(void)
 {
-	/* for backwards compatibility */
-	if (IS_NOT_SET(keypad_type))
-		keypad_type = keypad_enabled;
-
-	if (IS_NOT_SET(lcd_type))
-		lcd_type = lcd_enabled;
+	int selected_keypad_type = NOT_SET;
 
 	/* take care of an eventual profile */
 	switch (profile) {
 	case PANEL_PROFILE_CUSTOM:
 		/* custom profile */
-		if (IS_NOT_SET(keypad_type))
-			keypad_type = DEFAULT_KEYPAD_TYPE;
-		if (IS_NOT_SET(lcd_type))
-			lcd_type = DEFAULT_LCD_TYPE;
+		selected_keypad_type = DEFAULT_KEYPAD_TYPE;
+		selected_lcd_type = DEFAULT_LCD_TYPE;
 		break;
 	case PANEL_PROFILE_OLD:
 		/* 8 bits, 2*16, old keypad */
-		if (IS_NOT_SET(keypad_type))
-			keypad_type = KEYPAD_TYPE_OLD;
-		if (IS_NOT_SET(lcd_type))
-			lcd_type = LCD_TYPE_OLD;
+		selected_keypad_type = KEYPAD_TYPE_OLD;
+		selected_lcd_type = LCD_TYPE_OLD;
+
+		/* TODO: This two are a little hacky, sort it out later */
 		if (IS_NOT_SET(lcd_width))
 			lcd_width = 16;
 		if (IS_NOT_SET(lcd_hwidth))
@@ -2264,38 +2260,45 @@ static int __init panel_init_module(void)
 		break;
 	case PANEL_PROFILE_NEW:
 		/* serial, 2*16, new keypad */
-		if (IS_NOT_SET(keypad_type))
-			keypad_type = KEYPAD_TYPE_NEW;
-		if (IS_NOT_SET(lcd_type))
-			lcd_type = LCD_TYPE_KS0074;
+		selected_keypad_type = KEYPAD_TYPE_NEW;
+		selected_lcd_type = LCD_TYPE_KS0074;
 		break;
 	case PANEL_PROFILE_HANTRONIX:
 		/* 8 bits, 2*16 hantronix-like, no keypad */
-		if (IS_NOT_SET(keypad_type))
-			keypad_type = KEYPAD_TYPE_NONE;
-		if (IS_NOT_SET(lcd_type))
-			lcd_type = LCD_TYPE_HANTRONIX;
+		selected_keypad_type = KEYPAD_TYPE_NONE;
+		selected_lcd_type = LCD_TYPE_HANTRONIX;
 		break;
 	case PANEL_PROFILE_NEXCOM:
 		/* generic 8 bits, 2*16, nexcom keypad, eg. Nexcom. */
-		if (IS_NOT_SET(keypad_type))
-			keypad_type = KEYPAD_TYPE_NEXCOM;
-		if (IS_NOT_SET(lcd_type))
-			lcd_type = LCD_TYPE_NEXCOM;
+		selected_keypad_type = KEYPAD_TYPE_NEXCOM;
+		selected_lcd_type = LCD_TYPE_NEXCOM;
 		break;
 	case PANEL_PROFILE_LARGE:
 		/* 8 bits, 2*40, old keypad */
-		if (IS_NOT_SET(keypad_type))
-			keypad_type = KEYPAD_TYPE_OLD;
-		if (IS_NOT_SET(lcd_type))
-			lcd_type = LCD_TYPE_OLD;
+		selected_keypad_type = KEYPAD_TYPE_OLD;
+		selected_lcd_type = LCD_TYPE_OLD;
 		break;
 	}
 
-	lcd.enabled = (lcd_type > 0);
-	keypad.enabled = (keypad_type > 0);
+	/*
+	 * Overwrite selection with module param values (both keypad and lcd),
+	 * where the deprecated params have lower prio.
+	 */
+	if (keypad_enabled > -1)
+		selected_keypad_type = keypad_enabled;
+	if (keypad_type > -1)
+		selected_keypad_type = keypad_type;
+
+	keypad.enabled = (selected_keypad_type > 0);
+
+	if (lcd_enabled > -1)
+		selected_lcd_type = lcd_enabled;
+	if (lcd_type > -1)
+		selected_lcd_type = lcd_type;
+
+	lcd.enabled = (selected_lcd_type > 0);
 
-	switch (keypad_type) {
+	switch (selected_keypad_type) {
 	case KEYPAD_TYPE_OLD:
 		keypad_profile = old_keypad_profile;
 		break;
-- 
2.1.3

Re: [PATCH 6/9] staging: panel: Make two more module params read-only

[Willy Tarreau]

On Tue, Nov 18, 2014 at 09:56:11PM +0100, Mariusz Gorski wrote:
> Make keypad_type and lcd_type module params read-only.
> This step also starts making it more clear what is
> the precedence of device params coming from different
> sources (device profile, runtime module param values etc).
> 
> Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
> ---
>  drivers/staging/panel/panel.c | 71 ++++++++++++++++++++++---------------------
>  1 file changed, 37 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> index 7f2f5f8..5b4f0a4 100644
> --- a/drivers/staging/panel/panel.c
> +++ b/drivers/staging/panel/panel.c
> @@ -229,6 +229,9 @@ static struct {
>  	bool enabled;
>  } lcd;
>  
> +/* Needed only for init */
> +static int selected_lcd_type = NOT_SET;
> +
>  /* contains the LCD config state */
>  static unsigned long int lcd_flags;
>  /* contains the LCD X offset */
> @@ -1417,7 +1420,7 @@ static void panel_lcd_print(const char *s)
>  /* initialize the LCD driver */
>  static void lcd_init(void)
>  {
> -	switch (lcd_type) {
> +	switch (selected_lcd_type) {

(...)

stupid question : why not move that to the lcd struct you just
created instead of creating a new variable ? It would make sense
to me to have lcd.type here just like you did with enabled.
Same for keypad.

Willy

Re: [PATCH 6/9] staging: panel: Make two more module params read-only

[Mariusz Gorski]

On Tue, Nov 18, 2014 at 10:20:34PM +0100, Willy Tarreau wrote:
> On Tue, Nov 18, 2014 at 09:56:11PM +0100, Mariusz Gorski wrote:
> > Make keypad_type and lcd_type module params read-only.
> > This step also starts making it more clear what is
> > the precedence of device params coming from different
> > sources (device profile, runtime module param values etc).
> > 
> > Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
> > ---
> >  drivers/staging/panel/panel.c | 71 ++++++++++++++++++++++---------------------
> >  1 file changed, 37 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> > index 7f2f5f8..5b4f0a4 100644
> > --- a/drivers/staging/panel/panel.c
> > +++ b/drivers/staging/panel/panel.c
> > @@ -229,6 +229,9 @@ static struct {
> >  	bool enabled;
> >  } lcd;
> >  
> > +/* Needed only for init */
> > +static int selected_lcd_type = NOT_SET;
> > +
> >  /* contains the LCD config state */
> >  static unsigned long int lcd_flags;
> >  /* contains the LCD X offset */
> > @@ -1417,7 +1420,7 @@ static void panel_lcd_print(const char *s)
> >  /* initialize the LCD driver */
> >  static void lcd_init(void)
> >  {
> > -	switch (lcd_type) {
> > +	switch (selected_lcd_type) {
> 
> (...)
> 
> stupid question : why not move that to the lcd struct you just
> created instead of creating a new variable ? It would make sense
> to me to have lcd.type here just like you did with enabled.
> Same for keypad.
> 
> Willy
>

I get your point here. This var is here only because it's set
in init_panel_module() and then used in lcd_init(). So it's needed
really only for the init code. It doesn't directly_ describe the 
lcd's state, so I decided to keep it out.

Thanks,
Mariusz

Re: [PATCH 6/9] staging: panel: Make two more module params read-only

[Willy Tarreau]

On Tue, Nov 18, 2014 at 10:50:35PM +0100, Mariusz Gorski wrote:
> On Tue, Nov 18, 2014 at 10:20:34PM +0100, Willy Tarreau wrote:
> > On Tue, Nov 18, 2014 at 09:56:11PM +0100, Mariusz Gorski wrote:
> > > Make keypad_type and lcd_type module params read-only.
> > > This step also starts making it more clear what is
> > > the precedence of device params coming from different
> > > sources (device profile, runtime module param values etc).
> > > 
> > > Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
> > > ---
> > >  drivers/staging/panel/panel.c | 71 ++++++++++++++++++++++---------------------
> > >  1 file changed, 37 insertions(+), 34 deletions(-)
> > > 
> > > diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> > > index 7f2f5f8..5b4f0a4 100644
> > > --- a/drivers/staging/panel/panel.c
> > > +++ b/drivers/staging/panel/panel.c
> > > @@ -229,6 +229,9 @@ static struct {
> > >  	bool enabled;
> > >  } lcd;
> > >  
> > > +/* Needed only for init */
> > > +static int selected_lcd_type = NOT_SET;
> > > +
> > >  /* contains the LCD config state */
> > >  static unsigned long int lcd_flags;
> > >  /* contains the LCD X offset */
> > > @@ -1417,7 +1420,7 @@ static void panel_lcd_print(const char *s)
> > >  /* initialize the LCD driver */
> > >  static void lcd_init(void)
> > >  {
> > > -	switch (lcd_type) {
> > > +	switch (selected_lcd_type) {
> > 
> > (...)
> > 
> > stupid question : why not move that to the lcd struct you just
> > created instead of creating a new variable ? It would make sense
> > to me to have lcd.type here just like you did with enabled.
> > Same for keypad.
> > 
> > Willy
> >
> 
> I get your point here. This var is here only because it's set
> in init_panel_module() and then used in lcd_init(). So it's needed
> really only for the init code. It doesn't directly_ describe the 
> lcd's state, so I decided to keep it out.

OK but isn't it the same for some of the other variables you moved there ?
Anyway that's not important, I have nothing against this, it was just a
question.

Willy

[PATCH 7/9] staging: panel: Refactor LCD init code

[Mariusz Gorski]

Rework lcd_init method to make it a little bit more clear about
the precedence of the params, move LCD geometry and pins layout
to the LCD struct and thus make the LCD-related module params
effectively read-only.

Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
---
 drivers/staging/panel/panel.c | 304 ++++++++++++++++++++++--------------------
 1 file changed, 163 insertions(+), 141 deletions(-)

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 5b4f0a4..ee48bca 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -227,6 +227,21 @@ static wait_queue_head_t keypad_read_wait;
 /* lcd-specific variables */
 static struct {
 	bool enabled;
+	int height;
+	int width;
+	int bwidth;
+	int hwidth;
+	int charset;
+	int proto;
+	/* TODO: use union here? */
+	struct {
+		int e;
+		int rs;
+		int rw;
+		int cl;
+		int da;
+		int bl;
+	} pins;
 } lcd;
 
 /* Needed only for init */
@@ -768,7 +783,7 @@ static void lcd_send_serial(int byte)
 /* turn the backlight on or off */
 static void lcd_backlight(int on)
 {
-	if (lcd_bl_pin == PIN_NONE)
+	if (lcd.pins.bl == PIN_NONE)
 		return;
 
 	/* The backlight is activated by setting the AUTOFEED line to +5V  */
@@ -867,23 +882,23 @@ static void lcd_write_data_tilcd(int data)
 static void lcd_gotoxy(void)
 {
 	lcd_write_cmd(0x80	/* set DDRAM address */
-		      | (lcd_addr_y ? lcd_hwidth : 0)
+		      | (lcd_addr_y ? lcd.hwidth : 0)
 		      /* we force the cursor to stay at the end of the
 			 line if it wants to go farther */
-		      | ((lcd_addr_x < lcd_bwidth) ? lcd_addr_x &
-			 (lcd_hwidth - 1) : lcd_bwidth - 1));
+		      | ((lcd_addr_x < lcd.bwidth) ? lcd_addr_x &
+			 (lcd.hwidth - 1) : lcd.bwidth - 1));
 }
 
 static void lcd_print(char c)
 {
-	if (lcd_addr_x < lcd_bwidth) {
+	if (lcd_addr_x < lcd.bwidth) {
 		if (lcd_char_conv != NULL)
 			c = lcd_char_conv[(unsigned char)c];
 		lcd_write_data(c);
 		lcd_addr_x++;
 	}
 	/* prevents the cursor from wrapping onto the next line */
-	if (lcd_addr_x == lcd_bwidth)
+	if (lcd_addr_x == lcd.bwidth)
 		lcd_gotoxy();
 }
 
@@ -897,7 +912,7 @@ static void lcd_clear_fast_s(void)
 	lcd_gotoxy();
 
 	spin_lock_irq(&pprt_lock);
-	for (pos = 0; pos < lcd_height * lcd_hwidth; pos++) {
+	for (pos = 0; pos < lcd.height * lcd.hwidth; pos++) {
 		lcd_send_serial(0x5F);	/* R/W=W, RS=1 */
 		lcd_send_serial(' ' & 0x0F);
 		lcd_send_serial((' ' >> 4) & 0x0F);
@@ -920,7 +935,7 @@ static void lcd_clear_fast_p8(void)
 	lcd_gotoxy();
 
 	spin_lock_irq(&pprt_lock);
-	for (pos = 0; pos < lcd_height * lcd_hwidth; pos++) {
+	for (pos = 0; pos < lcd.height * lcd.hwidth; pos++) {
 		/* present the data to the data port */
 		w_dtr(pprt, ' ');
 
@@ -958,7 +973,7 @@ static void lcd_clear_fast_tilcd(void)
 	lcd_gotoxy();
 
 	spin_lock_irq(&pprt_lock);
-	for (pos = 0; pos < lcd_height * lcd_hwidth; pos++) {
+	for (pos = 0; pos < lcd.height * lcd.hwidth; pos++) {
 		/* present the data to the data port */
 		w_dtr(pprt, ' ');
 		udelay(60);
@@ -983,7 +998,7 @@ static void lcd_clear_display(void)
 
 static void lcd_init_display(void)
 {
-	lcd_flags = ((lcd_height > 1) ? LCD_FLAG_N : 0)
+	lcd_flags = ((lcd.height > 1) ? LCD_FLAG_N : 0)
 	    | LCD_FLAG_D | LCD_FLAG_C | LCD_FLAG_B;
 
 	long_sleep(20);		/* wait 20 ms after power-up for the paranoid */
@@ -1097,17 +1112,17 @@ static inline int handle_lcd_special_code(void)
 	case 'l':	/* Shift Cursor Left */
 		if (lcd_addr_x > 0) {
 			/* back one char if not at end of line */
-			if (lcd_addr_x < lcd_bwidth)
+			if (lcd_addr_x < lcd.bwidth)
 				lcd_write_cmd(0x10);
 			lcd_addr_x--;
 		}
 		processed = 1;
 		break;
 	case 'r':	/* shift cursor right */
-		if (lcd_addr_x < lcd_width) {
+		if (lcd_addr_x < lcd.width) {
 			/* allow the cursor to pass the end of the line */
 			if (lcd_addr_x <
-			    (lcd_bwidth - 1))
+			    (lcd.bwidth - 1))
 				lcd_write_cmd(0x14);
 			lcd_addr_x++;
 		}
@@ -1126,7 +1141,7 @@ static inline int handle_lcd_special_code(void)
 	case 'k': {	/* kill end of line */
 		int x;
 
-		for (x = lcd_addr_x; x < lcd_bwidth; x++)
+		for (x = lcd_addr_x; x < lcd.bwidth; x++)
 			lcd_write_data(' ');
 
 		/* restore cursor position */
@@ -1274,7 +1289,7 @@ static void lcd_write_char(char c)
 			if (lcd_addr_x > 0) {
 				/* check if we're not at the
 				   end of the line */
-				if (lcd_addr_x < lcd_bwidth)
+				if (lcd_addr_x < lcd.bwidth)
 					/* back one char */
 					lcd_write_cmd(0x10);
 				lcd_addr_x--;
@@ -1291,10 +1306,10 @@ static void lcd_write_char(char c)
 		case '\n':
 			/* flush the remainder of the current line and
 			   go to the beginning of the next line */
-			for (; lcd_addr_x < lcd_bwidth; lcd_addr_x++)
+			for (; lcd_addr_x < lcd.bwidth; lcd_addr_x++)
 				lcd_write_data(' ');
 			lcd_addr_x = 0;
-			lcd_addr_y = (lcd_addr_y + 1) % lcd_height;
+			lcd_addr_y = (lcd_addr_y + 1) % lcd.height;
 			lcd_gotoxy();
 			break;
 		case '\r':
@@ -1423,174 +1438,164 @@ static void lcd_init(void)
 	switch (selected_lcd_type) {
 	case LCD_TYPE_OLD:
 		/* parallel mode, 8 bits */
-		if (IS_NOT_SET(lcd_proto))
-			lcd_proto = LCD_PROTO_PARALLEL;
-		if (IS_NOT_SET(lcd_charset))
-			lcd_charset = LCD_CHARSET_NORMAL;
-		if (lcd_e_pin == PIN_NOT_SET)
-			lcd_e_pin = PIN_STROBE;
-		if (lcd_rs_pin == PIN_NOT_SET)
-			lcd_rs_pin = PIN_AUTOLF;
-
-		if (IS_NOT_SET(lcd_width))
-			lcd_width = 40;
-		if (IS_NOT_SET(lcd_bwidth))
-			lcd_bwidth = 40;
-		if (IS_NOT_SET(lcd_hwidth))
-			lcd_hwidth = 64;
-		if (IS_NOT_SET(lcd_height))
-			lcd_height = 2;
+		lcd.proto = LCD_PROTO_PARALLEL;
+		lcd.charset = LCD_CHARSET_NORMAL;
+		lcd.pins.e = PIN_STROBE;
+		lcd.pins.rs = PIN_AUTOLF;
+
+		lcd.width = 40;
+		lcd.bwidth = 40;
+		lcd.hwidth = 64;
+		lcd.height = 2;
 		break;
 	case LCD_TYPE_KS0074:
 		/* serial mode, ks0074 */
-		if (IS_NOT_SET(lcd_proto))
-			lcd_proto = LCD_PROTO_SERIAL;
-		if (IS_NOT_SET(lcd_charset))
-			lcd_charset = LCD_CHARSET_KS0074;
-		if (lcd_bl_pin == PIN_NOT_SET)
-			lcd_bl_pin = PIN_AUTOLF;
-		if (lcd_cl_pin == PIN_NOT_SET)
-			lcd_cl_pin = PIN_STROBE;
-		if (lcd_da_pin == PIN_NOT_SET)
-			lcd_da_pin = PIN_D0;
-
-		if (IS_NOT_SET(lcd_width))
-			lcd_width = 16;
-		if (IS_NOT_SET(lcd_bwidth))
-			lcd_bwidth = 40;
-		if (IS_NOT_SET(lcd_hwidth))
-			lcd_hwidth = 16;
-		if (IS_NOT_SET(lcd_height))
-			lcd_height = 2;
+		lcd.proto = LCD_PROTO_SERIAL;
+		lcd.charset = LCD_CHARSET_KS0074;
+		lcd.pins.bl = PIN_AUTOLF;
+		lcd.pins.cl = PIN_STROBE;
+		lcd.pins.da = PIN_D0;
+
+		lcd.width = 16;
+		lcd.bwidth = 40;
+		lcd.hwidth = 16;
+		lcd.height = 2;
 		break;
 	case LCD_TYPE_NEXCOM:
 		/* parallel mode, 8 bits, generic */
-		if (IS_NOT_SET(lcd_proto))
-			lcd_proto = LCD_PROTO_PARALLEL;
-		if (IS_NOT_SET(lcd_charset))
-			lcd_charset = LCD_CHARSET_NORMAL;
-		if (lcd_e_pin == PIN_NOT_SET)
-			lcd_e_pin = PIN_AUTOLF;
-		if (lcd_rs_pin == PIN_NOT_SET)
-			lcd_rs_pin = PIN_SELECP;
-		if (lcd_rw_pin == PIN_NOT_SET)
-			lcd_rw_pin = PIN_INITP;
-
-		if (IS_NOT_SET(lcd_width))
-			lcd_width = 16;
-		if (IS_NOT_SET(lcd_bwidth))
-			lcd_bwidth = 40;
-		if (IS_NOT_SET(lcd_hwidth))
-			lcd_hwidth = 64;
-		if (IS_NOT_SET(lcd_height))
-			lcd_height = 2;
+		lcd.proto = LCD_PROTO_PARALLEL;
+		lcd.charset = LCD_CHARSET_NORMAL;
+		lcd.pins.e = PIN_AUTOLF;
+		lcd.pins.rs = PIN_SELECP;
+		lcd.pins.rw = PIN_INITP;
+
+		lcd.width = 16;
+		lcd.bwidth = 40;
+		lcd.hwidth = 64;
+		lcd.height = 2;
 		break;
 	case LCD_TYPE_CUSTOM:
 		/* customer-defined */
-		if (IS_NOT_SET(lcd_proto))
-			lcd_proto = DEFAULT_LCD_PROTO;
-		if (IS_NOT_SET(lcd_charset))
-			lcd_charset = DEFAULT_LCD_CHARSET;
+		lcd.proto = DEFAULT_LCD_PROTO;
+		lcd.charset = DEFAULT_LCD_CHARSET;
 		/* default geometry will be set later */
 		break;
 	case LCD_TYPE_HANTRONIX:
 		/* parallel mode, 8 bits, hantronix-like */
 	default:
-		if (IS_NOT_SET(lcd_proto))
-			lcd_proto = LCD_PROTO_PARALLEL;
-		if (IS_NOT_SET(lcd_charset))
-			lcd_charset = LCD_CHARSET_NORMAL;
-		if (lcd_e_pin == PIN_NOT_SET)
-			lcd_e_pin = PIN_STROBE;
-		if (lcd_rs_pin == PIN_NOT_SET)
-			lcd_rs_pin = PIN_SELECP;
-
-		if (IS_NOT_SET(lcd_width))
-			lcd_width = 16;
-		if (IS_NOT_SET(lcd_bwidth))
-			lcd_bwidth = 40;
-		if (IS_NOT_SET(lcd_hwidth))
-			lcd_hwidth = 64;
-		if (IS_NOT_SET(lcd_height))
-			lcd_height = 2;
+		lcd.proto = LCD_PROTO_PARALLEL;
+		lcd.charset = LCD_CHARSET_NORMAL;
+		lcd.pins.e = PIN_STROBE;
+		lcd.pins.rs = PIN_SELECP;
+
+		lcd.width = 16;
+		lcd.bwidth = 40;
+		lcd.hwidth = 64;
+		lcd.height = 2;
 		break;
 	}
 
+	/* Overwrite with module params set on loading */
+	if (lcd_height > -1)
+		lcd.height = lcd_height;
+	if (lcd_width > -1)
+		lcd.width = lcd_width;
+	if (lcd_bwidth > -1)
+		lcd.bwidth = lcd_bwidth;
+	if (lcd_hwidth > -1)
+		lcd.hwidth = lcd_hwidth;
+	if (lcd_charset > -1)
+		lcd.charset = lcd_charset;
+	if (lcd_proto > -1)
+		lcd.proto = lcd_proto;
+	if (lcd_e_pin != PIN_NOT_SET)
+		lcd.pins.e = lcd_e_pin;
+	if (lcd_rs_pin != PIN_NOT_SET)
+		lcd.pins.rs = lcd_rs_pin;
+	if (lcd_rw_pin != PIN_NOT_SET)
+		lcd.pins.rw = lcd_rw_pin;
+	if (lcd_cl_pin != PIN_NOT_SET)
+		lcd.pins.cl = lcd_cl_pin;
+	if (lcd_da_pin != PIN_NOT_SET)
+		lcd.pins.da = lcd_da_pin;
+	if (lcd_bl_pin != PIN_NOT_SET)
+		lcd.pins.bl = lcd_bl_pin;
+
 	/* this is used to catch wrong and default values */
-	if (lcd_width <= 0)
-		lcd_width = DEFAULT_LCD_WIDTH;
-	if (lcd_bwidth <= 0)
-		lcd_bwidth = DEFAULT_LCD_BWIDTH;
-	if (lcd_hwidth <= 0)
-		lcd_hwidth = DEFAULT_LCD_HWIDTH;
-	if (lcd_height <= 0)
-		lcd_height = DEFAULT_LCD_HEIGHT;
-
-	if (lcd_proto == LCD_PROTO_SERIAL) {	/* SERIAL */
+	if (lcd.width <= 0)
+		lcd.width = DEFAULT_LCD_WIDTH;
+	if (lcd.bwidth <= 0)
+		lcd.bwidth = DEFAULT_LCD_BWIDTH;
+	if (lcd.hwidth <= 0)
+		lcd.hwidth = DEFAULT_LCD_HWIDTH;
+	if (lcd.height <= 0)
+		lcd.height = DEFAULT_LCD_HEIGHT;
+
+	if (lcd.proto == LCD_PROTO_SERIAL) {	/* SERIAL */
 		lcd_write_cmd = lcd_write_cmd_s;
 		lcd_write_data = lcd_write_data_s;
 		lcd_clear_fast = lcd_clear_fast_s;
 
-		if (lcd_cl_pin == PIN_NOT_SET)
-			lcd_cl_pin = DEFAULT_LCD_PIN_SCL;
-		if (lcd_da_pin == PIN_NOT_SET)
-			lcd_da_pin = DEFAULT_LCD_PIN_SDA;
+		if (lcd.pins.cl == PIN_NOT_SET)
+			lcd.pins.cl = DEFAULT_LCD_PIN_SCL;
+		if (lcd.pins.da == PIN_NOT_SET)
+			lcd.pins.da = DEFAULT_LCD_PIN_SDA;
 
-	} else if (lcd_proto == LCD_PROTO_PARALLEL) {	/* PARALLEL */
+	} else if (lcd.proto == LCD_PROTO_PARALLEL) {	/* PARALLEL */
 		lcd_write_cmd = lcd_write_cmd_p8;
 		lcd_write_data = lcd_write_data_p8;
 		lcd_clear_fast = lcd_clear_fast_p8;
 
-		if (lcd_e_pin == PIN_NOT_SET)
-			lcd_e_pin = DEFAULT_LCD_PIN_E;
-		if (lcd_rs_pin == PIN_NOT_SET)
-			lcd_rs_pin = DEFAULT_LCD_PIN_RS;
-		if (lcd_rw_pin == PIN_NOT_SET)
-			lcd_rw_pin = DEFAULT_LCD_PIN_RW;
+		if (lcd.pins.e == PIN_NOT_SET)
+			lcd.pins.e = DEFAULT_LCD_PIN_E;
+		if (lcd.pins.rs == PIN_NOT_SET)
+			lcd.pins.rs = DEFAULT_LCD_PIN_RS;
+		if (lcd.pins.rw == PIN_NOT_SET)
+			lcd.pins.rw = DEFAULT_LCD_PIN_RW;
 	} else {
 		lcd_write_cmd = lcd_write_cmd_tilcd;
 		lcd_write_data = lcd_write_data_tilcd;
 		lcd_clear_fast = lcd_clear_fast_tilcd;
 	}
 
-	if (lcd_bl_pin == PIN_NOT_SET)
-		lcd_bl_pin = DEFAULT_LCD_PIN_BL;
-
-	if (lcd_e_pin == PIN_NOT_SET)
-		lcd_e_pin = PIN_NONE;
-	if (lcd_rs_pin == PIN_NOT_SET)
-		lcd_rs_pin = PIN_NONE;
-	if (lcd_rw_pin == PIN_NOT_SET)
-		lcd_rw_pin = PIN_NONE;
-	if (lcd_bl_pin == PIN_NOT_SET)
-		lcd_bl_pin = PIN_NONE;
-	if (lcd_cl_pin == PIN_NOT_SET)
-		lcd_cl_pin = PIN_NONE;
-	if (lcd_da_pin == PIN_NOT_SET)
-		lcd_da_pin = PIN_NONE;
-
-	if (IS_NOT_SET(lcd_charset))
-		lcd_charset = DEFAULT_LCD_CHARSET;
-
-	if (lcd_charset == LCD_CHARSET_KS0074)
+	if (lcd.pins.bl == PIN_NOT_SET)
+		lcd.pins.bl = DEFAULT_LCD_PIN_BL;
+
+	if (lcd.pins.e == PIN_NOT_SET)
+		lcd.pins.e = PIN_NONE;
+	if (lcd.pins.rs == PIN_NOT_SET)
+		lcd.pins.rs = PIN_NONE;
+	if (lcd.pins.rw == PIN_NOT_SET)
+		lcd.pins.rw = PIN_NONE;
+	if (lcd.pins.bl == PIN_NOT_SET)
+		lcd.pins.bl = PIN_NONE;
+	if (lcd.pins.cl == PIN_NOT_SET)
+		lcd.pins.cl = PIN_NONE;
+	if (lcd.pins.da == PIN_NOT_SET)
+		lcd.pins.da = PIN_NONE;
+
+	if (IS_NOT_SET(lcd.charset))
+		lcd.charset = DEFAULT_LCD_CHARSET;
+
+	if (lcd.charset == LCD_CHARSET_KS0074)
 		lcd_char_conv = lcd_char_conv_ks0074;
 	else
 		lcd_char_conv = NULL;
 
-	if (lcd_bl_pin != PIN_NONE)
+	if (lcd.pins.bl != PIN_NONE)
 		init_scan_timer();
 
-	pin_to_bits(lcd_e_pin, lcd_bits[LCD_PORT_D][LCD_BIT_E],
+	pin_to_bits(lcd.pins.e, lcd_bits[LCD_PORT_D][LCD_BIT_E],
 		    lcd_bits[LCD_PORT_C][LCD_BIT_E]);
-	pin_to_bits(lcd_rs_pin, lcd_bits[LCD_PORT_D][LCD_BIT_RS],
+	pin_to_bits(lcd.pins.rs, lcd_bits[LCD_PORT_D][LCD_BIT_RS],
 		    lcd_bits[LCD_PORT_C][LCD_BIT_RS]);
-	pin_to_bits(lcd_rw_pin, lcd_bits[LCD_PORT_D][LCD_BIT_RW],
+	pin_to_bits(lcd.pins.rw, lcd_bits[LCD_PORT_D][LCD_BIT_RW],
 		    lcd_bits[LCD_PORT_C][LCD_BIT_RW]);
-	pin_to_bits(lcd_bl_pin, lcd_bits[LCD_PORT_D][LCD_BIT_BL],
+	pin_to_bits(lcd.pins.bl, lcd_bits[LCD_PORT_D][LCD_BIT_BL],
 		    lcd_bits[LCD_PORT_C][LCD_BIT_BL]);
-	pin_to_bits(lcd_cl_pin, lcd_bits[LCD_PORT_D][LCD_BIT_CL],
+	pin_to_bits(lcd.pins.cl, lcd_bits[LCD_PORT_D][LCD_BIT_CL],
 		    lcd_bits[LCD_PORT_C][LCD_BIT_CL]);
-	pin_to_bits(lcd_da_pin, lcd_bits[LCD_PORT_D][LCD_BIT_DA],
+	pin_to_bits(lcd.pins.da, lcd_bits[LCD_PORT_D][LCD_BIT_DA],
 		    lcd_bits[LCD_PORT_C][LCD_BIT_DA]);
 
 	/* before this line, we must NOT send anything to the display.
@@ -2281,6 +2286,23 @@ static int __init panel_init_module(void)
 	}
 
 	/*
+	 * Init lcd struct with load-time values to preserve exact current
+	 * functionality (at least for now).
+	 */
+	lcd.height = lcd_height;
+	lcd.width = lcd_width;
+	lcd.bwidth = lcd_bwidth;
+	lcd.hwidth = lcd_hwidth;
+	lcd.charset = lcd_charset;
+	lcd.proto = lcd_proto;
+	lcd.pins.e = lcd_e_pin;
+	lcd.pins.rs = lcd_rs_pin;
+	lcd.pins.rw = lcd_rw_pin;
+	lcd.pins.cl = lcd_cl_pin;
+	lcd.pins.da = lcd_da_pin;
+	lcd.pins.bl = lcd_bl_pin;
+
+	/*
 	 * Overwrite selection with module param values (both keypad and lcd),
 	 * where the deprecated params have lower prio.
 	 */
-- 
2.1.3

Re: [PATCH 7/9] staging: panel: Refactor LCD init code

[Willy Tarreau]

On Tue, Nov 18, 2014 at 09:56:12PM +0100, Mariusz Gorski wrote:
> Rework lcd_init method to make it a little bit more clear about
> the precedence of the params, move LCD geometry and pins layout
> to the LCD struct and thus make the LCD-related module params
> effectively read-only.
> 
> Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>

Acked-by: Willy Tarreau <w@1wt.eu>

I like this refactoring. However I don't know if you got your LCD module
to work or not, but for such important changes you should definitely test
the code, since it's very easy to introduce minor bugs or even to fix a
bug that used to make the whole driver work...

Willy

Re: [PATCH 7/9] staging: panel: Refactor LCD init code

[Mariusz Gorski]

On Tue, Nov 18, 2014 at 10:23:26PM +0100, Willy Tarreau wrote:
> On Tue, Nov 18, 2014 at 09:56:12PM +0100, Mariusz Gorski wrote:
> > Rework lcd_init method to make it a little bit more clear about
> > the precedence of the params, move LCD geometry and pins layout
> > to the LCD struct and thus make the LCD-related module params
> > effectively read-only.
> > 
> > Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
> 
> Acked-by: Willy Tarreau <w@1wt.eu>
> 
> I like this refactoring. However I don't know if you got your LCD module
> to work or not, but for such important changes you should definitely test
> the code, since it's very easy to introduce minor bugs or even to fix a
> bug that used to make the whole driver work...
> 
> Willy
>
Yes, I have tested this code with my LCD module and it works
as before. This is what I've emphasized in the cover letter -
- I made sure (and tested), that all lcd and keypad params are
set to the same state as before this patch series, and that
the current behaviour of the module doesn't change. I've tested
all profile/lcd_type combinations in an automated way to catch
any edge cases :)

Thanks,
Mariusz

Re: [PATCH 7/9] staging: panel: Refactor LCD init code

[Willy Tarreau]

On Tue, Nov 18, 2014 at 10:51:08PM +0100, Mariusz Gorski wrote:
> On Tue, Nov 18, 2014 at 10:23:26PM +0100, Willy Tarreau wrote:
> > On Tue, Nov 18, 2014 at 09:56:12PM +0100, Mariusz Gorski wrote:
> > > Rework lcd_init method to make it a little bit more clear about
> > > the precedence of the params, move LCD geometry and pins layout
> > > to the LCD struct and thus make the LCD-related module params
> > > effectively read-only.
> > > 
> > > Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
> > 
> > Acked-by: Willy Tarreau <w@1wt.eu>
> > 
> > I like this refactoring. However I don't know if you got your LCD module
> > to work or not, but for such important changes you should definitely test
> > the code, since it's very easy to introduce minor bugs or even to fix a
> > bug that used to make the whole driver work...
> > 
> > Willy
> >
> Yes, I have tested this code with my LCD module and it works
> as before.

great!

> This is what I've emphasized in the cover letter -
> - I made sure (and tested), that all lcd and keypad params are
> set to the same state as before this patch series, and that
> the current behaviour of the module doesn't change. I've tested
> all profile/lcd_type combinations in an automated way to catch
> any edge cases :)

"tested" was not mentionned in the cover letter, hence my asking :-)

Willy

[PATCH 8/9] staging: panel: Remove more magic number comparison

[Mariusz Gorski]

Use a macro instead of magic number comparison for checking
whether a module param value has been set.

Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
---
 drivers/staging/panel/panel.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index ee48bca..268ad2e 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -136,6 +136,7 @@
 #define NOT_SET			-1
 
 #define IS_NOT_SET(x)	(x == NOT_SET)
+#define IS_SET(x)		(x > NOT_SET)
 
 /* macros to simplify use of the parallel port */
 #define r_ctr(x)        (parport_read_control((x)->port))
@@ -1496,17 +1497,17 @@ static void lcd_init(void)
 	}
 
 	/* Overwrite with module params set on loading */
-	if (lcd_height > -1)
+	if (IS_SET(lcd_height))
 		lcd.height = lcd_height;
-	if (lcd_width > -1)
+	if (IS_SET(lcd_width))
 		lcd.width = lcd_width;
-	if (lcd_bwidth > -1)
+	if (IS_SET(lcd_bwidth))
 		lcd.bwidth = lcd_bwidth;
-	if (lcd_hwidth > -1)
+	if (IS_SET(lcd_hwidth))
 		lcd.hwidth = lcd_hwidth;
-	if (lcd_charset > -1)
+	if (IS_SET(lcd_charset))
 		lcd.charset = lcd_charset;
-	if (lcd_proto > -1)
+	if (IS_SET(lcd_proto))
 		lcd.proto = lcd_proto;
 	if (lcd_e_pin != PIN_NOT_SET)
 		lcd.pins.e = lcd_e_pin;
@@ -2306,16 +2307,16 @@ static int __init panel_init_module(void)
 	 * Overwrite selection with module param values (both keypad and lcd),
 	 * where the deprecated params have lower prio.
 	 */
-	if (keypad_enabled > -1)
+	if (IS_SET(keypad_enabled))
 		selected_keypad_type = keypad_enabled;
-	if (keypad_type > -1)
+	if (IS_SET(keypad_type))
 		selected_keypad_type = keypad_type;
 
 	keypad.enabled = (selected_keypad_type > 0);
 
-	if (lcd_enabled > -1)
+	if (IS_SET(lcd_enabled))
 		selected_lcd_type = lcd_enabled;
-	if (lcd_type > -1)
+	if (IS_SET(lcd_type))
 		selected_lcd_type = lcd_type;
 
 	lcd.enabled = (selected_lcd_type > 0);
-- 
2.1.3

Re: [PATCH 8/9] staging: panel: Remove more magic number comparison

[Willy Tarreau]

On Tue, Nov 18, 2014 at 09:56:13PM +0100, Mariusz Gorski wrote:
> Use a macro instead of magic number comparison for checking
> whether a module param value has been set.
> 
> Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
> ---
>  drivers/staging/panel/panel.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> index ee48bca..268ad2e 100644
> --- a/drivers/staging/panel/panel.c
> +++ b/drivers/staging/panel/panel.c
> @@ -136,6 +136,7 @@
>  #define NOT_SET			-1
>  
>  #define IS_NOT_SET(x)	(x == NOT_SET)
> +#define IS_SET(x)		(x > NOT_SET)
>  
>  /* macros to simplify use of the parallel port */
>  #define r_ctr(x)        (parport_read_control((x)->port))
> @@ -1496,17 +1497,17 @@ static void lcd_init(void)
>  	}
>  
>  	/* Overwrite with module params set on loading */
> -	if (lcd_height > -1)
> +	if (IS_SET(lcd_height))
>  		lcd.height = lcd_height;
> -	if (lcd_width > -1)
> +	if (IS_SET(lcd_width))
>  		lcd.width = lcd_width;

(...)

Same as for the other patch, better open-code the test for readability :

-	if (lcd_height > -1)
+	if (lcd_height != NOT_SET)
 		lcd.height = lcd_height;

Note that we take the freedom to change the operator since we only want
to check equality and not sign in practice.

Willy

Re: [PATCH 8/9] staging: panel: Remove more magic number comparison

[Mariusz Gorski]

On Tue, Nov 18, 2014 at 10:25:23PM +0100, Willy Tarreau wrote:
> On Tue, Nov 18, 2014 at 09:56:13PM +0100, Mariusz Gorski wrote:
> > Use a macro instead of magic number comparison for checking
> > whether a module param value has been set.
> > 
> > Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
> > ---
> >  drivers/staging/panel/panel.c | 21 +++++++++++----------
> >  1 file changed, 11 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> > index ee48bca..268ad2e 100644
> > --- a/drivers/staging/panel/panel.c
> > +++ b/drivers/staging/panel/panel.c
> > @@ -136,6 +136,7 @@
> >  #define NOT_SET			-1
> >  
> >  #define IS_NOT_SET(x)	(x == NOT_SET)
> > +#define IS_SET(x)		(x > NOT_SET)
> >  
> >  /* macros to simplify use of the parallel port */
> >  #define r_ctr(x)        (parport_read_control((x)->port))
> > @@ -1496,17 +1497,17 @@ static void lcd_init(void)
> >  	}
> >  
> >  	/* Overwrite with module params set on loading */
> > -	if (lcd_height > -1)
> > +	if (IS_SET(lcd_height))
> >  		lcd.height = lcd_height;
> > -	if (lcd_width > -1)
> > +	if (IS_SET(lcd_width))
> >  		lcd.width = lcd_width;
> 
> (...)
> 
> Same as for the other patch, better open-code the test for readability :
> 
> -	if (lcd_height > -1)
> +	if (lcd_height != NOT_SET)
>  		lcd.height = lcd_height;
> 
> Note that we take the freedom to change the operator since we only want
> to check equality and not sign in practice.
> 
> Willy
>

Good point, will apply :)

Thanks,
Mariusz

[PATCH 9/9] staging: panel: Move LCD-related state into struct lcd

[Mariusz Gorski]

Move more or less all LCD-related state into struct lcd
in order to get better cohesion; use bool instead of int
where it makes sense.

Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
---
 drivers/staging/panel/panel.c | 255 ++++++++++++++++++++++--------------------
 1 file changed, 134 insertions(+), 121 deletions(-)

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 268ad2e..7a11871 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -228,12 +228,20 @@ static wait_queue_head_t keypad_read_wait;
 /* lcd-specific variables */
 static struct {
 	bool enabled;
+	bool initialized;
+	bool must_clear;
+
+	/* TODO: use bool here? */
+	char left_shift;
+
 	int height;
 	int width;
 	int bwidth;
 	int hwidth;
 	int charset;
 	int proto;
+	int light_tempo;
+
 	/* TODO: use union here? */
 	struct {
 		int e;
@@ -243,22 +251,26 @@ static struct {
 		int da;
 		int bl;
 	} pins;
+
+	/* contains the LCD config state */
+	unsigned long int flags;
+
+	/* Contains the LCD X and Y offset */
+	struct {
+		unsigned long int x;
+		unsigned long int y;
+	} addr;
+
+	/* Current escape sequence and it's length or -1 if outside */
+	struct {
+		char buf[LCD_ESCAPE_LEN + 1];
+		int len;
+	} esc_seq;
 } lcd;
 
 /* Needed only for init */
 static int selected_lcd_type = NOT_SET;
 
-/* contains the LCD config state */
-static unsigned long int lcd_flags;
-/* contains the LCD X offset */
-static unsigned long int lcd_addr_x;
-/* contains the LCD Y offset */
-static unsigned long int lcd_addr_y;
-/* current escape sequence, 0 terminated */
-static char lcd_escape[LCD_ESCAPE_LEN + 1];
-/* not in escape state. >=0 = escape cmd len */
-static int lcd_escape_len = -1;
-
 /*
  * Bit masks to convert LCD signals to parallel port outputs.
  * _d_ are values for data port, _c_ are for control port.
@@ -441,13 +453,8 @@ static atomic_t keypad_available = ATOMIC_INIT(1);
 
 static struct pardevice *pprt;
 
-static int lcd_initialized;
 static int keypad_initialized;
 
-static int light_tempo;
-
-static char lcd_must_clear;
-static char lcd_left_shift;
 static char init_in_progress;
 
 static void (*lcd_write_cmd)(int);
@@ -883,23 +890,23 @@ static void lcd_write_data_tilcd(int data)
 static void lcd_gotoxy(void)
 {
 	lcd_write_cmd(0x80	/* set DDRAM address */
-		      | (lcd_addr_y ? lcd.hwidth : 0)
+		      | (lcd.addr.y ? lcd.hwidth : 0)
 		      /* we force the cursor to stay at the end of the
 			 line if it wants to go farther */
-		      | ((lcd_addr_x < lcd.bwidth) ? lcd_addr_x &
+		      | ((lcd.addr.x < lcd.bwidth) ? lcd.addr.x &
 			 (lcd.hwidth - 1) : lcd.bwidth - 1));
 }
 
 static void lcd_print(char c)
 {
-	if (lcd_addr_x < lcd.bwidth) {
+	if (lcd.addr.x < lcd.bwidth) {
 		if (lcd_char_conv != NULL)
 			c = lcd_char_conv[(unsigned char)c];
 		lcd_write_data(c);
-		lcd_addr_x++;
+		lcd.addr.x++;
 	}
 	/* prevents the cursor from wrapping onto the next line */
-	if (lcd_addr_x == lcd.bwidth)
+	if (lcd.addr.x == lcd.bwidth)
 		lcd_gotoxy();
 }
 
@@ -908,8 +915,8 @@ static void lcd_clear_fast_s(void)
 {
 	int pos;
 
-	lcd_addr_x = 0;
-	lcd_addr_y = 0;
+	lcd.addr.x = 0;
+	lcd.addr.y = 0;
 	lcd_gotoxy();
 
 	spin_lock_irq(&pprt_lock);
@@ -921,8 +928,8 @@ static void lcd_clear_fast_s(void)
 	}
 	spin_unlock_irq(&pprt_lock);
 
-	lcd_addr_x = 0;
-	lcd_addr_y = 0;
+	lcd.addr.x = 0;
+	lcd.addr.y = 0;
 	lcd_gotoxy();
 }
 
@@ -931,8 +938,8 @@ static void lcd_clear_fast_p8(void)
 {
 	int pos;
 
-	lcd_addr_x = 0;
-	lcd_addr_y = 0;
+	lcd.addr.x = 0;
+	lcd.addr.y = 0;
 	lcd_gotoxy();
 
 	spin_lock_irq(&pprt_lock);
@@ -959,8 +966,8 @@ static void lcd_clear_fast_p8(void)
 	}
 	spin_unlock_irq(&pprt_lock);
 
-	lcd_addr_x = 0;
-	lcd_addr_y = 0;
+	lcd.addr.x = 0;
+	lcd.addr.y = 0;
 	lcd_gotoxy();
 }
 
@@ -969,8 +976,8 @@ static void lcd_clear_fast_tilcd(void)
 {
 	int pos;
 
-	lcd_addr_x = 0;
-	lcd_addr_y = 0;
+	lcd.addr.x = 0;
+	lcd.addr.y = 0;
 	lcd_gotoxy();
 
 	spin_lock_irq(&pprt_lock);
@@ -982,8 +989,8 @@ static void lcd_clear_fast_tilcd(void)
 
 	spin_unlock_irq(&pprt_lock);
 
-	lcd_addr_x = 0;
-	lcd_addr_y = 0;
+	lcd.addr.x = 0;
+	lcd.addr.y = 0;
 	lcd_gotoxy();
 }
 
@@ -991,15 +998,15 @@ static void lcd_clear_fast_tilcd(void)
 static void lcd_clear_display(void)
 {
 	lcd_write_cmd(0x01);	/* clear display */
-	lcd_addr_x = 0;
-	lcd_addr_y = 0;
+	lcd.addr.x = 0;
+	lcd.addr.y = 0;
 	/* we must wait a few milliseconds (15) */
 	long_sleep(15);
 }
 
 static void lcd_init_display(void)
 {
-	lcd_flags = ((lcd.height > 1) ? LCD_FLAG_N : 0)
+	lcd.flags = ((lcd.height > 1) ? LCD_FLAG_N : 0)
 	    | LCD_FLAG_D | LCD_FLAG_C | LCD_FLAG_B;
 
 	long_sleep(20);		/* wait 20 ms after power-up for the paranoid */
@@ -1012,8 +1019,8 @@ static void lcd_init_display(void)
 	long_sleep(10);
 
 	lcd_write_cmd(0x30	/* set font height and lines number */
-		      | ((lcd_flags & LCD_FLAG_F) ? 4 : 0)
-		      | ((lcd_flags & LCD_FLAG_N) ? 8 : 0)
+		      | ((lcd.flags & LCD_FLAG_F) ? 4 : 0)
+		      | ((lcd.flags & LCD_FLAG_N) ? 8 : 0)
 	    );
 	long_sleep(10);
 
@@ -1021,12 +1028,12 @@ static void lcd_init_display(void)
 	long_sleep(10);
 
 	lcd_write_cmd(0x08	/* set display mode */
-		      | ((lcd_flags & LCD_FLAG_D) ? 4 : 0)
-		      | ((lcd_flags & LCD_FLAG_C) ? 2 : 0)
-		      | ((lcd_flags & LCD_FLAG_B) ? 1 : 0)
+		      | ((lcd.flags & LCD_FLAG_D) ? 4 : 0)
+		      | ((lcd.flags & LCD_FLAG_C) ? 2 : 0)
+		      | ((lcd.flags & LCD_FLAG_B) ? 1 : 0)
 	    );
 
-	lcd_backlight((lcd_flags & LCD_FLAG_L) ? 1 : 0);
+	lcd_backlight((lcd.flags & LCD_FLAG_L) ? 1 : 0);
 
 	long_sleep(10);
 
@@ -1049,100 +1056,101 @@ static inline int handle_lcd_special_code(void)
 
 	int processed = 0;
 
-	char *esc = lcd_escape + 2;
-	int oldflags = lcd_flags;
+	char *esc = lcd.esc_seq.buf + 2;
+	int oldflags = lcd.flags;
 
 	/* check for display mode flags */
 	switch (*esc) {
 	case 'D':	/* Display ON */
-		lcd_flags |= LCD_FLAG_D;
+		lcd.flags |= LCD_FLAG_D;
 		processed = 1;
 		break;
 	case 'd':	/* Display OFF */
-		lcd_flags &= ~LCD_FLAG_D;
+		lcd.flags &= ~LCD_FLAG_D;
 		processed = 1;
 		break;
 	case 'C':	/* Cursor ON */
-		lcd_flags |= LCD_FLAG_C;
+		lcd.flags |= LCD_FLAG_C;
 		processed = 1;
 		break;
 	case 'c':	/* Cursor OFF */
-		lcd_flags &= ~LCD_FLAG_C;
+		lcd.flags &= ~LCD_FLAG_C;
 		processed = 1;
 		break;
 	case 'B':	/* Blink ON */
-		lcd_flags |= LCD_FLAG_B;
+		lcd.flags |= LCD_FLAG_B;
 		processed = 1;
 		break;
 	case 'b':	/* Blink OFF */
-		lcd_flags &= ~LCD_FLAG_B;
+		lcd.flags &= ~LCD_FLAG_B;
 		processed = 1;
 		break;
 	case '+':	/* Back light ON */
-		lcd_flags |= LCD_FLAG_L;
+		lcd.flags |= LCD_FLAG_L;
 		processed = 1;
 		break;
 	case '-':	/* Back light OFF */
-		lcd_flags &= ~LCD_FLAG_L;
+		lcd.flags &= ~LCD_FLAG_L;
 		processed = 1;
 		break;
 	case '*':
 		/* flash back light using the keypad timer */
 		if (scan_timer.function != NULL) {
-			if (light_tempo == 0 && ((lcd_flags & LCD_FLAG_L) == 0))
+			if (lcd.light_tempo == 0
+					&& ((lcd.flags & LCD_FLAG_L) == 0))
 				lcd_backlight(1);
-			light_tempo = FLASH_LIGHT_TEMPO;
+			lcd.light_tempo = FLASH_LIGHT_TEMPO;
 		}
 		processed = 1;
 		break;
 	case 'f':	/* Small Font */
-		lcd_flags &= ~LCD_FLAG_F;
+		lcd.flags &= ~LCD_FLAG_F;
 		processed = 1;
 		break;
 	case 'F':	/* Large Font */
-		lcd_flags |= LCD_FLAG_F;
+		lcd.flags |= LCD_FLAG_F;
 		processed = 1;
 		break;
 	case 'n':	/* One Line */
-		lcd_flags &= ~LCD_FLAG_N;
+		lcd.flags &= ~LCD_FLAG_N;
 		processed = 1;
 		break;
 	case 'N':	/* Two Lines */
-		lcd_flags |= LCD_FLAG_N;
+		lcd.flags |= LCD_FLAG_N;
 		break;
 	case 'l':	/* Shift Cursor Left */
-		if (lcd_addr_x > 0) {
+		if (lcd.addr.x > 0) {
 			/* back one char if not at end of line */
-			if (lcd_addr_x < lcd.bwidth)
+			if (lcd.addr.x < lcd.bwidth)
 				lcd_write_cmd(0x10);
-			lcd_addr_x--;
+			lcd.addr.x--;
 		}
 		processed = 1;
 		break;
 	case 'r':	/* shift cursor right */
-		if (lcd_addr_x < lcd.width) {
+		if (lcd.addr.x < lcd.width) {
 			/* allow the cursor to pass the end of the line */
-			if (lcd_addr_x <
+			if (lcd.addr.x <
 			    (lcd.bwidth - 1))
 				lcd_write_cmd(0x14);
-			lcd_addr_x++;
+			lcd.addr.x++;
 		}
 		processed = 1;
 		break;
 	case 'L':	/* shift display left */
-		lcd_left_shift++;
+		lcd.left_shift++;
 		lcd_write_cmd(0x18);
 		processed = 1;
 		break;
 	case 'R':	/* shift display right */
-		lcd_left_shift--;
+		lcd.left_shift--;
 		lcd_write_cmd(0x1C);
 		processed = 1;
 		break;
 	case 'k': {	/* kill end of line */
 		int x;
 
-		for (x = lcd_addr_x; x < lcd.bwidth; x++)
+		for (x = lcd.addr.x; x < lcd.bwidth; x++)
 			lcd_write_data(' ');
 
 		/* restore cursor position */
@@ -1152,7 +1160,7 @@ static inline int handle_lcd_special_code(void)
 	}
 	case 'I':	/* reinitialize display */
 		lcd_init_display();
-		lcd_left_shift = 0;
+		lcd.left_shift = 0;
 		processed = 1;
 		break;
 	case 'G': {
@@ -1223,11 +1231,11 @@ static inline int handle_lcd_special_code(void)
 		while (*esc) {
 			if (*esc == 'x') {
 				esc++;
-				if (kstrtoul(esc, 10, &lcd_addr_x) < 0)
+				if (kstrtoul(esc, 10, &lcd.addr.x) < 0)
 					break;
 			} else if (*esc == 'y') {
 				esc++;
-				if (kstrtoul(esc, 10, &lcd_addr_y) < 0)
+				if (kstrtoul(esc, 10, &lcd.addr.y) < 0)
 					break;
 			} else {
 				break;
@@ -1240,25 +1248,25 @@ static inline int handle_lcd_special_code(void)
 	}
 
 	/* Check whether one flag was changed */
-	if (oldflags != lcd_flags) {
+	if (oldflags != lcd.flags) {
 		/* check whether one of B,C,D flags were changed */
-		if ((oldflags ^ lcd_flags) &
+		if ((oldflags ^ lcd.flags) &
 		    (LCD_FLAG_B | LCD_FLAG_C | LCD_FLAG_D))
 			/* set display mode */
 			lcd_write_cmd(0x08
-				      | ((lcd_flags & LCD_FLAG_D) ? 4 : 0)
-				      | ((lcd_flags & LCD_FLAG_C) ? 2 : 0)
-				      | ((lcd_flags & LCD_FLAG_B) ? 1 : 0));
+				      | ((lcd.flags & LCD_FLAG_D) ? 4 : 0)
+				      | ((lcd.flags & LCD_FLAG_C) ? 2 : 0)
+				      | ((lcd.flags & LCD_FLAG_B) ? 1 : 0));
 		/* check whether one of F,N flags was changed */
-		else if ((oldflags ^ lcd_flags) & (LCD_FLAG_F | LCD_FLAG_N))
+		else if ((oldflags ^ lcd.flags) & (LCD_FLAG_F | LCD_FLAG_N))
 			lcd_write_cmd(0x30
-				      | ((lcd_flags & LCD_FLAG_F) ? 4 : 0)
-				      | ((lcd_flags & LCD_FLAG_N) ? 8 : 0));
+				      | ((lcd.flags & LCD_FLAG_F) ? 4 : 0)
+				      | ((lcd.flags & LCD_FLAG_N) ? 8 : 0));
 		/* check whether L flag was changed */
-		else if ((oldflags ^ lcd_flags) & (LCD_FLAG_L)) {
-			if (lcd_flags & (LCD_FLAG_L))
+		else if ((oldflags ^ lcd.flags) & (LCD_FLAG_L)) {
+			if (lcd.flags & (LCD_FLAG_L))
 				lcd_backlight(1);
-			else if (light_tempo == 0)
+			else if (lcd.light_tempo == 0)
 				/* switch off the light only when the tempo
 				   lighting is gone */
 				lcd_backlight(0);
@@ -1271,29 +1279,29 @@ static inline int handle_lcd_special_code(void)
 static void lcd_write_char(char c)
 {
 	/* first, we'll test if we're in escape mode */
-	if ((c != '\n') && lcd_escape_len >= 0) {
+	if ((c != '\n') && lcd.esc_seq.len >= 0) {
 		/* yes, let's add this char to the buffer */
-		lcd_escape[lcd_escape_len++] = c;
-		lcd_escape[lcd_escape_len] = 0;
+		lcd.esc_seq.buf[lcd.esc_seq.len++] = c;
+		lcd.esc_seq.buf[lcd.esc_seq.len] = 0;
 	} else {
 		/* aborts any previous escape sequence */
-		lcd_escape_len = -1;
+		lcd.esc_seq.len = -1;
 
 		switch (c) {
 		case LCD_ESCAPE_CHAR:
 			/* start of an escape sequence */
-			lcd_escape_len = 0;
-			lcd_escape[lcd_escape_len] = 0;
+			lcd.esc_seq.len = 0;
+			lcd.esc_seq.buf[lcd.esc_seq.len] = 0;
 			break;
 		case '\b':
 			/* go back one char and clear it */
-			if (lcd_addr_x > 0) {
+			if (lcd.addr.x > 0) {
 				/* check if we're not at the
 				   end of the line */
-				if (lcd_addr_x < lcd.bwidth)
+				if (lcd.addr.x < lcd.bwidth)
 					/* back one char */
 					lcd_write_cmd(0x10);
-				lcd_addr_x--;
+				lcd.addr.x--;
 			}
 			/* replace with a space */
 			lcd_write_data(' ');
@@ -1307,15 +1315,15 @@ static void lcd_write_char(char c)
 		case '\n':
 			/* flush the remainder of the current line and
 			   go to the beginning of the next line */
-			for (; lcd_addr_x < lcd.bwidth; lcd_addr_x++)
+			for (; lcd.addr.x < lcd.bwidth; lcd.addr.x++)
 				lcd_write_data(' ');
-			lcd_addr_x = 0;
-			lcd_addr_y = (lcd_addr_y + 1) % lcd.height;
+			lcd.addr.x = 0;
+			lcd.addr.y = (lcd.addr.y + 1) % lcd.height;
 			lcd_gotoxy();
 			break;
 		case '\r':
 			/* go to the beginning of the same line */
-			lcd_addr_x = 0;
+			lcd.addr.x = 0;
 			lcd_gotoxy();
 			break;
 		case '\t':
@@ -1331,32 +1339,32 @@ static void lcd_write_char(char c)
 
 	/* now we'll see if we're in an escape mode and if the current
 	   escape sequence can be understood. */
-	if (lcd_escape_len >= 2) {
+	if (lcd.esc_seq.len >= 2) {
 		int processed = 0;
 
-		if (!strcmp(lcd_escape, "[2J")) {
+		if (!strcmp(lcd.esc_seq.buf, "[2J")) {
 			/* clear the display */
 			lcd_clear_fast();
 			processed = 1;
-		} else if (!strcmp(lcd_escape, "[H")) {
+		} else if (!strcmp(lcd.esc_seq.buf, "[H")) {
 			/* cursor to home */
-			lcd_addr_x = 0;
-			lcd_addr_y = 0;
+			lcd.addr.x = 0;
+			lcd.addr.y = 0;
 			lcd_gotoxy();
 			processed = 1;
 		}
 		/* codes starting with ^[[L */
-		else if ((lcd_escape_len >= 3) &&
-			 (lcd_escape[0] == '[') &&
-			 (lcd_escape[1] == 'L')) {
+		else if ((lcd.esc_seq.len >= 3) &&
+			 (lcd.esc_seq.buf[0] == '[') &&
+			 (lcd.esc_seq.buf[1] == 'L')) {
 			processed = handle_lcd_special_code();
 		}
 
 		/* LCD special escape codes */
 		/* flush the escape sequence if it's been processed
 		   or if it is getting too long. */
-		if (processed || (lcd_escape_len >= LCD_ESCAPE_LEN))
-			lcd_escape_len = -1;
+		if (processed || (lcd.esc_seq.len >= LCD_ESCAPE_LEN))
+			lcd.esc_seq.len = -1;
 	} /* escape codes */
 }
 
@@ -1389,9 +1397,9 @@ static int lcd_open(struct inode *inode, struct file *file)
 	if (file->f_mode & FMODE_READ)	/* device is write-only */
 		return -EPERM;
 
-	if (lcd_must_clear) {
+	if (lcd.must_clear) {
 		lcd_clear_display();
-		lcd_must_clear = 0;
+		lcd.must_clear = false;
 	}
 	return nonseekable_open(inode, file);
 }
@@ -1421,7 +1429,7 @@ static void panel_lcd_print(const char *s)
 	const char *tmp = s;
 	int count = strlen(s);
 
-	if (lcd.enabled && lcd_initialized) {
+	if (lcd.enabled && lcd.initialized) {
 		for (; count-- > 0; tmp++) {
 			if (!in_interrupt() && (((count + 1) & 0x1f) == 0))
 				/* let's be a little nice with other processes
@@ -1602,7 +1610,7 @@ static void lcd_init(void)
 	/* before this line, we must NOT send anything to the display.
 	 * Since lcd_init_display() needs to write data, we have to
 	 * enable mark the LCD initialized just before. */
-	lcd_initialized = 1;
+	lcd.initialized = true;
 	lcd_init_display();
 
 	/* display a short message */
@@ -1614,10 +1622,10 @@ static void lcd_init(void)
 	panel_lcd_print("\x1b[Lc\x1b[Lb\x1b[L*Linux-" UTS_RELEASE "\nPanel-"
 			PANEL_VERSION);
 #endif
-	lcd_addr_x = 0;
-	lcd_addr_y = 0;
+	lcd.addr.x = 0;
+	lcd.addr.y = 0;
 	/* clear the display on the next device opening */
-	lcd_must_clear = 1;
+	lcd.must_clear = true;
 	lcd_gotoxy();
 }
 
@@ -1953,14 +1961,16 @@ static void panel_scan_timer(void)
 			panel_process_inputs();
 	}
 
-	if (lcd.enabled && lcd_initialized) {
+	if (lcd.enabled && lcd.initialized) {
 		if (keypressed) {
-			if (light_tempo == 0 && ((lcd_flags & LCD_FLAG_L) == 0))
+			if (lcd.light_tempo == 0
+					&& ((lcd.flags & LCD_FLAG_L) == 0))
 				lcd_backlight(1);
-			light_tempo = FLASH_LIGHT_TEMPO;
-		} else if (light_tempo > 0) {
-			light_tempo--;
-			if (light_tempo == 0 && ((lcd_flags & LCD_FLAG_L) == 0))
+			lcd.light_tempo = FLASH_LIGHT_TEMPO;
+		} else if (lcd.light_tempo > 0) {
+			lcd.light_tempo--;
+			if (lcd.light_tempo == 0
+					&& ((lcd.flags & LCD_FLAG_L) == 0))
 				lcd_backlight(0);
 		}
 	}
@@ -2132,7 +2142,7 @@ static void keypad_init(void)
 static int panel_notify_sys(struct notifier_block *this, unsigned long code,
 			    void *unused)
 {
-	if (lcd.enabled && lcd_initialized) {
+	if (lcd.enabled && lcd.initialized) {
 		switch (code) {
 		case SYS_DOWN:
 			panel_lcd_print
@@ -2225,9 +2235,9 @@ static void panel_detach(struct parport *port)
 		keypad_initialized = 0;
 	}
 
-	if (lcd.enabled && lcd_initialized) {
+	if (lcd.enabled && lcd.initialized) {
 		misc_deregister(&lcd_dev);
-		lcd_initialized = 0;
+		lcd.initialized = false;
 	}
 
 	parport_release(pprt);
@@ -2303,6 +2313,9 @@ static int __init panel_init_module(void)
 	lcd.pins.da = lcd_da_pin;
 	lcd.pins.bl = lcd_bl_pin;
 
+	/* Leave it for now, just in case */
+	lcd.esc_seq.len = -1;
+
 	/*
 	 * Overwrite selection with module param values (both keypad and lcd),
 	 * where the deprecated params have lower prio.
@@ -2388,7 +2401,7 @@ static void __exit panel_cleanup_module(void)
 			panel_lcd_print("\x0cLCD driver " PANEL_VERSION
 					"\nunloaded.\x1b[Lc\x1b[Lb\x1b[L-");
 			misc_deregister(&lcd_dev);
-			lcd_initialized = 0;
+			lcd.initialized = false;
 		}
 
 		/* TODO: free all input signals */
-- 
2.1.3

Re: [PATCH 9/9] staging: panel: Move LCD-related state into struct lcd

[Willy Tarreau]

On Tue, Nov 18, 2014 at 09:56:14PM +0100, Mariusz Gorski wrote:
> Move more or less all LCD-related state into struct lcd
> in order to get better cohesion; use bool instead of int
> where it makes sense.
> 
> Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>

Acked-by: Willy Tarreau <w@1wt.eu>