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

[PATCH v2 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.

v2: Don't introduce new macros for param value check

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 defined value or 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 | 669 ++++++++++++++++++++++--------------------
 1 file changed, 354 insertions(+), 315 deletions(-)

-- 
2.1.3

[PATCH v2 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>
Acked-by: Willy Tarreau <w@1wt.eu>
---
 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

[PATCH v2 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>
Acked-by: Willy Tarreau <w@1wt.eu>
---
 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

[PATCH v2 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>
Acked-by: Willy Tarreau <w@1wt.eu>
---
 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

[PATCH v2 4/9] staging: panel: Use defined value or checking module params state

[Mariusz Gorski]

Avoid magic number and use a comparison with a defined value 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>
---
v2: Don't introduce new macros for param value check

 drivers/staging/panel/panel.c | 86 +++++++++++++++++++++----------------------
 1 file changed, 43 insertions(+), 43 deletions(-)

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 1b4a211..5288990 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -1411,29 +1411,29 @@ static void lcd_init(void)
 	switch (lcd_type) {
 	case LCD_TYPE_OLD:
 		/* parallel mode, 8 bits */
-		if (lcd_proto < 0)
+		if (lcd_proto == NOT_SET)
 			lcd_proto = LCD_PROTO_PARALLEL;
-		if (lcd_charset < 0)
+		if (lcd_charset == NOT_SET)
 			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 (lcd_width == NOT_SET)
 			lcd_width = 40;
-		if (lcd_bwidth < 0)
+		if (lcd_bwidth == NOT_SET)
 			lcd_bwidth = 40;
-		if (lcd_hwidth < 0)
+		if (lcd_hwidth == NOT_SET)
 			lcd_hwidth = 64;
-		if (lcd_height < 0)
+		if (lcd_height == NOT_SET)
 			lcd_height = 2;
 		break;
 	case LCD_TYPE_KS0074:
 		/* serial mode, ks0074 */
-		if (lcd_proto < 0)
+		if (lcd_proto == NOT_SET)
 			lcd_proto = LCD_PROTO_SERIAL;
-		if (lcd_charset < 0)
+		if (lcd_charset == NOT_SET)
 			lcd_charset = LCD_CHARSET_KS0074;
 		if (lcd_bl_pin == PIN_NOT_SET)
 			lcd_bl_pin = PIN_AUTOLF;
@@ -1442,20 +1442,20 @@ static void lcd_init(void)
 		if (lcd_da_pin == PIN_NOT_SET)
 			lcd_da_pin = PIN_D0;
 
-		if (lcd_width < 0)
+		if (lcd_width == NOT_SET)
 			lcd_width = 16;
-		if (lcd_bwidth < 0)
+		if (lcd_bwidth == NOT_SET)
 			lcd_bwidth = 40;
-		if (lcd_hwidth < 0)
+		if (lcd_hwidth == NOT_SET)
 			lcd_hwidth = 16;
-		if (lcd_height < 0)
+		if (lcd_height == NOT_SET)
 			lcd_height = 2;
 		break;
 	case LCD_TYPE_NEXCOM:
 		/* parallel mode, 8 bits, generic */
-		if (lcd_proto < 0)
+		if (lcd_proto == NOT_SET)
 			lcd_proto = LCD_PROTO_PARALLEL;
-		if (lcd_charset < 0)
+		if (lcd_charset == NOT_SET)
 			lcd_charset = LCD_CHARSET_NORMAL;
 		if (lcd_e_pin == PIN_NOT_SET)
 			lcd_e_pin = PIN_AUTOLF;
@@ -1464,42 +1464,42 @@ static void lcd_init(void)
 		if (lcd_rw_pin == PIN_NOT_SET)
 			lcd_rw_pin = PIN_INITP;
 
-		if (lcd_width < 0)
+		if (lcd_width == NOT_SET)
 			lcd_width = 16;
-		if (lcd_bwidth < 0)
+		if (lcd_bwidth == NOT_SET)
 			lcd_bwidth = 40;
-		if (lcd_hwidth < 0)
+		if (lcd_hwidth == NOT_SET)
 			lcd_hwidth = 64;
-		if (lcd_height < 0)
+		if (lcd_height == NOT_SET)
 			lcd_height = 2;
 		break;
 	case LCD_TYPE_CUSTOM:
 		/* customer-defined */
-		if (lcd_proto < 0)
+		if (lcd_proto == NOT_SET)
 			lcd_proto = DEFAULT_LCD_PROTO;
-		if (lcd_charset < 0)
+		if (lcd_charset == NOT_SET)
 			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 (lcd_proto == NOT_SET)
 			lcd_proto = LCD_PROTO_PARALLEL;
-		if (lcd_charset < 0)
+		if (lcd_charset == NOT_SET)
 			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 (lcd_width == NOT_SET)
 			lcd_width = 16;
-		if (lcd_bwidth < 0)
+		if (lcd_bwidth == NOT_SET)
 			lcd_bwidth = 40;
-		if (lcd_hwidth < 0)
+		if (lcd_hwidth == NOT_SET)
 			lcd_hwidth = 64;
-		if (lcd_height < 0)
+		if (lcd_height == NOT_SET)
 			lcd_height = 2;
 		break;
 	}
@@ -1557,7 +1557,7 @@ static void lcd_init(void)
 	if (lcd_da_pin == PIN_NOT_SET)
 		lcd_da_pin = PIN_NONE;
 
-	if (lcd_charset < 0)
+	if (lcd_charset == NOT_SET)
 		lcd_charset = DEFAULT_LCD_CHARSET;
 
 	if (lcd_charset == LCD_CHARSET_KS0074)
@@ -2227,58 +2227,58 @@ static struct parport_driver panel_driver = {
 static int __init panel_init_module(void)
 {
 	/* for backwards compatibility */
-	if (keypad_type < 0)
+	if (keypad_type == NOT_SET)
 		keypad_type = keypad_enabled;
 
-	if (lcd_type < 0)
+	if (lcd_type == NOT_SET)
 		lcd_type = lcd_enabled;
 
 	/* take care of an eventual profile */
 	switch (profile) {
 	case PANEL_PROFILE_CUSTOM:
 		/* custom profile */
-		if (keypad_type < 0)
+		if (keypad_type == NOT_SET)
 			keypad_type = DEFAULT_KEYPAD_TYPE;
-		if (lcd_type < 0)
+		if (lcd_type == NOT_SET)
 			lcd_type = DEFAULT_LCD_TYPE;
 		break;
 	case PANEL_PROFILE_OLD:
 		/* 8 bits, 2*16, old keypad */
-		if (keypad_type < 0)
+		if (keypad_type == NOT_SET)
 			keypad_type = KEYPAD_TYPE_OLD;
-		if (lcd_type < 0)
+		if (lcd_type == NOT_SET)
 			lcd_type = LCD_TYPE_OLD;
-		if (lcd_width < 0)
+		if (lcd_width == NOT_SET)
 			lcd_width = 16;
-		if (lcd_hwidth < 0)
+		if (lcd_hwidth == NOT_SET)
 			lcd_hwidth = 16;
 		break;
 	case PANEL_PROFILE_NEW:
 		/* serial, 2*16, new keypad */
-		if (keypad_type < 0)
+		if (keypad_type == NOT_SET)
 			keypad_type = KEYPAD_TYPE_NEW;
-		if (lcd_type < 0)
+		if (lcd_type == NOT_SET)
 			lcd_type = LCD_TYPE_KS0074;
 		break;
 	case PANEL_PROFILE_HANTRONIX:
 		/* 8 bits, 2*16 hantronix-like, no keypad */
-		if (keypad_type < 0)
+		if (keypad_type == NOT_SET)
 			keypad_type = KEYPAD_TYPE_NONE;
-		if (lcd_type < 0)
+		if (lcd_type == NOT_SET)
 			lcd_type = LCD_TYPE_HANTRONIX;
 		break;
 	case PANEL_PROFILE_NEXCOM:
 		/* generic 8 bits, 2*16, nexcom keypad, eg. Nexcom. */
-		if (keypad_type < 0)
+		if (keypad_type == NOT_SET)
 			keypad_type = KEYPAD_TYPE_NEXCOM;
-		if (lcd_type < 0)
+		if (lcd_type == NOT_SET)
 			lcd_type = LCD_TYPE_NEXCOM;
 		break;
 	case PANEL_PROFILE_LARGE:
 		/* 8 bits, 2*40, old keypad */
-		if (keypad_type < 0)
+		if (keypad_type == NOT_SET)
 			keypad_type = KEYPAD_TYPE_OLD;
-		if (lcd_type < 0)
+		if (lcd_type == NOT_SET)
 			lcd_type = LCD_TYPE_OLD;
 		break;
 	}
-- 
2.1.3

Re: [PATCH v2 4/9] staging: panel: Use defined value or checking module params state

[Willy Tarreau]

On Wed, Nov 19, 2014 at 09:38:46PM +0100, Mariusz Gorski wrote:
> Avoid magic number and use a comparison with a defined value 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>

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

Re: [PATCH v2 4/9] staging: panel: Use defined value or checking module params state

[Greg Kroah-Hartman]

On Wed, Nov 19, 2014 at 09:38:46PM +0100, Mariusz Gorski wrote:
> Avoid magic number and use a comparison with a defined value 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>
> Acked-by: Willy Tarreau <w@1wt.eu>
> ---
> v2: Don't introduce new macros for param value check
> 
>  drivers/staging/panel/panel.c | 86 +++++++++++++++++++++----------------------
>  1 file changed, 43 insertions(+), 43 deletions(-)

Ugh, I messed up here, and applied the first series, which was acked.

Mariusz, can you resend the patches that I didn't apply, I can't seem to
get the rest of these to work properly.

thanks,

greg k-h

Re: [PATCH v2 4/9] staging: panel: Use defined value or checking module params state

[Mariusz Gorski]

On Wed, Nov 26, 2014 at 01:58:01PM -0800, Greg Kroah-Hartman wrote:
> On Wed, Nov 19, 2014 at 09:38:46PM +0100, Mariusz Gorski wrote:
> > Avoid magic number and use a comparison with a defined value 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>
> > Acked-by: Willy Tarreau <w@1wt.eu>
> > ---
> > v2: Don't introduce new macros for param value check
> > 
> >  drivers/staging/panel/panel.c | 86 +++++++++++++++++++++----------------------
> >  1 file changed, 43 insertions(+), 43 deletions(-)
> 
> Ugh, I messed up here, and applied the first series, which was acked.
> 
> Mariusz, can you resend the patches that I didn't apply, I can't seem to
> get the rest of these to work properly.

Greg, if I get you here correctly, you've applied all 9 patches from v1
and none from v2, so what you need now is a v1->v2 patch, right?

> thanks,
> 
> greg k-h

Cheers,
Mariusz

Re: [PATCH v2 4/9] staging: panel: Use defined value or checking module params state

[Greg Kroah-Hartman]

On Thu, Nov 27, 2014 at 02:26:59PM +0100, Mariusz Gorski wrote:
> On Wed, Nov 26, 2014 at 01:58:01PM -0800, Greg Kroah-Hartman wrote:
> > On Wed, Nov 19, 2014 at 09:38:46PM +0100, Mariusz Gorski wrote:
> > > Avoid magic number and use a comparison with a defined value 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>
> > > Acked-by: Willy Tarreau <w@1wt.eu>
> > > ---
> > > v2: Don't introduce new macros for param value check
> > > 
> > >  drivers/staging/panel/panel.c | 86 +++++++++++++++++++++----------------------
> > >  1 file changed, 43 insertions(+), 43 deletions(-)
> > 
> > Ugh, I messed up here, and applied the first series, which was acked.
> > 
> > Mariusz, can you resend the patches that I didn't apply, I can't seem to
> > get the rest of these to work properly.
> 
> Greg, if I get you here correctly, you've applied all 9 patches from v1
> and none from v2, so what you need now is a v1->v2 patch, right?

No, I think I applied the patches sent _before_ the 9 series, it was 4
or 5 or so, you should have gotten an email about them.  Pull my
staging-testing branch and redo your remaining patches please.

thanks,

greg k-h

Re: [PATCH v2 4/9] staging: panel: Use defined value or checking module params state

[Greg Kroah-Hartman]

On Thu, Nov 27, 2014 at 07:24:17AM -0800, Greg Kroah-Hartman wrote:
> On Thu, Nov 27, 2014 at 02:26:59PM +0100, Mariusz Gorski wrote:
> > On Wed, Nov 26, 2014 at 01:58:01PM -0800, Greg Kroah-Hartman wrote:
> > > On Wed, Nov 19, 2014 at 09:38:46PM +0100, Mariusz Gorski wrote:
> > > > Avoid magic number and use a comparison with a defined value 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>
> > > > Acked-by: Willy Tarreau <w@1wt.eu>
> > > > ---
> > > > v2: Don't introduce new macros for param value check
> > > > 
> > > >  drivers/staging/panel/panel.c | 86 +++++++++++++++++++++----------------------
> > > >  1 file changed, 43 insertions(+), 43 deletions(-)
> > > 
> > > Ugh, I messed up here, and applied the first series, which was acked.
> > > 
> > > Mariusz, can you resend the patches that I didn't apply, I can't seem to
> > > get the rest of these to work properly.
> > 
> > Greg, if I get you here correctly, you've applied all 9 patches from v1
> > and none from v2, so what you need now is a v1->v2 patch, right?
> 
> No, I think I applied the patches sent _before_ the 9 series, it was 4
> or 5 or so, you should have gotten an email about them.  Pull my
> staging-testing branch and redo your remaining patches please.

And the reason I got confused was because you didn't label your second
set of patches "v2", which it was, I saw two separate series, one with a
few patches, and then 2 sets of 9, the second set labeled "v2" so I
thought they were independant.  Please think of the poor maintainer who
has to decipher things like this when you send them out...

thanks,

greg k-h

Re: [PATCH v2 4/9] staging: panel: Use defined value or checking module params state

[Willy Tarreau]

On Thu, Nov 27, 2014 at 07:57:06AM -0800, Greg Kroah-Hartman wrote:
> And the reason I got confused was because you didn't label your second
> set of patches "v2", which it was, I saw two separate series, one with a
> few patches, and then 2 sets of 9, the second set labeled "v2" so I
> thought they were independant.  Please think of the poor maintainer who
> has to decipher things like this when you send them out...

For Mariusz's defense, this was his first batch. He didn't feel comfortable
and asked me how to proceed when sending a series and I forgot to warn him
about the "v2" as initially I didn't think there would be a v2, and after I
obviously forgot I didn't speak about that. So I share some responsibility
for this one.

Mariusz, if you need some help, tell me so.

Regards,
Willy

Re: [PATCH v2 4/9] staging: panel: Use defined value or checking module params state

[Greg Kroah-Hartman]

On Thu, Nov 27, 2014 at 05:14:06PM +0100, Willy Tarreau wrote:
> On Thu, Nov 27, 2014 at 07:57:06AM -0800, Greg Kroah-Hartman wrote:
> > And the reason I got confused was because you didn't label your second
> > set of patches "v2", which it was, I saw two separate series, one with a
> > few patches, and then 2 sets of 9, the second set labeled "v2" so I
> > thought they were independant.  Please think of the poor maintainer who
> > has to decipher things like this when you send them out...
> 
> For Mariusz's defense, this was his first batch. He didn't feel comfortable
> and asked me how to proceed when sending a series and I forgot to warn him
> about the "v2" as initially I didn't think there would be a v2, and after I
> obviously forgot I didn't speak about that. So I share some responsibility
> for this one.

I wasn't trying to make anyone feel bad here, just trying to explain why
I got this all messed up on my end.  Mariusz, consider this a chance to
learn how to rebase your patches, a very common task for kernel
developers to have to do :)

thanks,

greg k-h

Re: [PATCH v2 4/9] staging: panel: Use defined value or checking module params state

[Mariusz Gorski]

On Thu, Nov 27, 2014 at 07:57:06AM -0800, Greg Kroah-Hartman wrote:
> On Thu, Nov 27, 2014 at 07:24:17AM -0800, Greg Kroah-Hartman wrote:
> > On Thu, Nov 27, 2014 at 02:26:59PM +0100, Mariusz Gorski wrote:
> > > On Wed, Nov 26, 2014 at 01:58:01PM -0800, Greg Kroah-Hartman wrote:
> > > > On Wed, Nov 19, 2014 at 09:38:46PM +0100, Mariusz Gorski wrote:
> > > > > Avoid magic number and use a comparison with a defined value 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>
> > > > > Acked-by: Willy Tarreau <w@1wt.eu>
> > > > > ---
> > > > > v2: Don't introduce new macros for param value check
> > > > > 
> > > > >  drivers/staging/panel/panel.c | 86 +++++++++++++++++++++----------------------
> > > > >  1 file changed, 43 insertions(+), 43 deletions(-)
> > > > 
> > > > Ugh, I messed up here, and applied the first series, which was acked.
> > > > 
> > > > Mariusz, can you resend the patches that I didn't apply, I can't seem to
> > > > get the rest of these to work properly.
> > > 
> > > Greg, if I get you here correctly, you've applied all 9 patches from v1
> > > and none from v2, so what you need now is a v1->v2 patch, right?
> > 
> > No, I think I applied the patches sent _before_ the 9 series, it was 4
> > or 5 or so, you should have gotten an email about them.  Pull my
> > staging-testing branch and redo your remaining patches please.
> 
> And the reason I got confused was because you didn't label your second
> set of patches "v2", which it was, I saw two separate series, one with a
> few patches, and then 2 sets of 9, the second set labeled "v2" so I
> thought they were independant.  Please think of the poor maintainer who
> has to decipher things like this when you send them out...

I'm confused right now. As you say, first I've sent a patchset of 4:
https://lkml.org/lkml/2014/11/11/963

Then, a couple of days later, I've sent the initial patchset of 9:
https://lkml.org/lkml/2014/11/18/922

And a day I've sent a fixed version of the above patchset, labeled with v2:
https://lkml.org/lkml/2014/11/19/653

Isn't this the right way to do? I still don't get my mistake. Because
what I was just about to do is to resend the v2 patchset, but now I'm
not sure anymore if this is what I'm supposed to do.

BTW: Out of these 3 patchsets, 1st and 3rd should be applied.

> thanks,
> 
> greg k-h

Cheers,
Mariusz

Re: [PATCH v2 4/9] staging: panel: Use defined value or checking module params state

[Willy Tarreau]

On Thu, Nov 27, 2014 at 08:50:55PM +0100, Mariusz Gorski wrote:
> > And the reason I got confused was because you didn't label your second
> > set of patches "v2", which it was, I saw two separate series, one with a
> > few patches, and then 2 sets of 9, the second set labeled "v2" so I
> > thought they were independant.  Please think of the poor maintainer who
> > has to decipher things like this when you send them out...
> 
> I'm confused right now. As you say, first I've sent a patchset of 4:
> https://lkml.org/lkml/2014/11/11/963
> 
> Then, a couple of days later, I've sent the initial patchset of 9:
> https://lkml.org/lkml/2014/11/18/922
> 
> And a day I've sent a fixed version of the above patchset, labeled with v2:
> https://lkml.org/lkml/2014/11/19/653
> 
> Isn't this the right way to do? I still don't get my mistake. Because
> what I was just about to do is to resend the v2 patchset, but now I'm
> not sure anymore if this is what I'm supposed to do.
> 
> BTW: Out of these 3 patchsets, 1st and 3rd should be applied.

Mariusz, for people who have to parse hundreds to thousands of e-mails
a day, dealing with non-trivial operation modes like this is never easy.

I think (I'll let Greg suggest what he prefers) that the most reliable
thing to do *right now* is to rebase your tree on top of Greg's staging
tree, and you send the resulting series (what you apply *after* staging)
at once, maybe even tagged as v3 to avoid any confusion.

Sometimes for the recipient, things apparently as simple as sorting
e-mails by subjects to find something can cause some confusion when
it's not obvious what replaces what, and tagging with the version or
ensuring that each series is different enough helps avoiding this.

If you need any help, contact me off-list and I'll gladly help you.

Don't worry, issues like this commonly happen and will happen again,
whatever you do against them will only reduce the likeliness that
they happen again (and that's important to care about this) :-)

Thanks,
Willy

Re: [PATCH v2 4/9] staging: panel: Use defined value or checking module params state

[Fabio Estevam]

On Thu, Nov 27, 2014 at 7:05 PM, Willy Tarreau <w@1wt.eu> wrote:

> Mariusz, for people who have to parse hundreds to thousands of e-mails
> a day, dealing with non-trivial operation modes like this is never easy.
>
> I think (I'll let Greg suggest what he prefers) that the most reliable
> thing to do *right now* is to rebase your tree on top of Greg's staging
> tree, and you send the resulting series (what you apply *after* staging)

Actually 'staging-testing' branch would be better:
https://git.kernel.org/cgit/linux/kernel/git/gregkh/staging.git/log/?h=staging-testing&ofs=100

There are 4 patches from Mariusz already applied there.

Re: [PATCH v2 4/9] staging: panel: Use defined value or checking module params state

[Greg Kroah-Hartman]

On Thu, Nov 27, 2014 at 08:50:55PM +0100, Mariusz Gorski wrote:
> On Thu, Nov 27, 2014 at 07:57:06AM -0800, Greg Kroah-Hartman wrote:
> > On Thu, Nov 27, 2014 at 07:24:17AM -0800, Greg Kroah-Hartman wrote:
> > > On Thu, Nov 27, 2014 at 02:26:59PM +0100, Mariusz Gorski wrote:
> > > > On Wed, Nov 26, 2014 at 01:58:01PM -0800, Greg Kroah-Hartman wrote:
> > > > > On Wed, Nov 19, 2014 at 09:38:46PM +0100, Mariusz Gorski wrote:
> > > > > > Avoid magic number and use a comparison with a defined value 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>
> > > > > > Acked-by: Willy Tarreau <w@1wt.eu>
> > > > > > ---
> > > > > > v2: Don't introduce new macros for param value check
> > > > > > 
> > > > > >  drivers/staging/panel/panel.c | 86 +++++++++++++++++++++----------------------
> > > > > >  1 file changed, 43 insertions(+), 43 deletions(-)
> > > > > 
> > > > > Ugh, I messed up here, and applied the first series, which was acked.
> > > > > 
> > > > > Mariusz, can you resend the patches that I didn't apply, I can't seem to
> > > > > get the rest of these to work properly.
> > > > 
> > > > Greg, if I get you here correctly, you've applied all 9 patches from v1
> > > > and none from v2, so what you need now is a v1->v2 patch, right?
> > > 
> > > No, I think I applied the patches sent _before_ the 9 series, it was 4
> > > or 5 or so, you should have gotten an email about them.  Pull my
> > > staging-testing branch and redo your remaining patches please.
> > 
> > And the reason I got confused was because you didn't label your second
> > set of patches "v2", which it was, I saw two separate series, one with a
> > few patches, and then 2 sets of 9, the second set labeled "v2" so I
> > thought they were independant.  Please think of the poor maintainer who
> > has to decipher things like this when you send them out...
> 
> I'm confused right now. As you say, first I've sent a patchset of 4:
> https://lkml.org/lkml/2014/11/11/963

Which I applied.

> Then, a couple of days later, I've sent the initial patchset of 9:
> https://lkml.org/lkml/2014/11/18/922
> 
> And a day I've sent a fixed version of the above patchset, labeled with v2:
> https://lkml.org/lkml/2014/11/19/653

So I thought your series of 9 was separate from the series of 4, you can
see my confusion (remember, I receive on _average_ about 1000 emails a day).

> Isn't this the right way to do? I still don't get my mistake. Because
> what I was just about to do is to resend the v2 patchset, but now I'm
> not sure anymore if this is what I'm supposed to do.
> 
> BTW: Out of these 3 patchsets, 1st and 3rd should be applied.

I tried to apply the 3rd, but it didn't apply due to patches I applied
in your first set of 4 patches.

Does that help?

greg k-h

Re: [PATCH v2 4/9] staging: panel: Use defined value or checking module params state

[Mariusz Gorski]

On Fri, Nov 28, 2014 at 12:32:48PM -0800, Greg Kroah-Hartman wrote:
> On Thu, Nov 27, 2014 at 08:50:55PM +0100, Mariusz Gorski wrote:
> > On Thu, Nov 27, 2014 at 07:57:06AM -0800, Greg Kroah-Hartman wrote:
> > > On Thu, Nov 27, 2014 at 07:24:17AM -0800, Greg Kroah-Hartman wrote:
> > > > On Thu, Nov 27, 2014 at 02:26:59PM +0100, Mariusz Gorski wrote:
> > > > > On Wed, Nov 26, 2014 at 01:58:01PM -0800, Greg Kroah-Hartman wrote:
> > > > > > On Wed, Nov 19, 2014 at 09:38:46PM +0100, Mariusz Gorski wrote:
> > > > > > > Avoid magic number and use a comparison with a defined value 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>
> > > > > > > Acked-by: Willy Tarreau <w@1wt.eu>
> > > > > > > ---
> > > > > > > v2: Don't introduce new macros for param value check
> > > > > > > 
> > > > > > >  drivers/staging/panel/panel.c | 86 +++++++++++++++++++++----------------------
> > > > > > >  1 file changed, 43 insertions(+), 43 deletions(-)
> > > > > > 
> > > > > > Ugh, I messed up here, and applied the first series, which was acked.
> > > > > > 
> > > > > > Mariusz, can you resend the patches that I didn't apply, I can't seem to
> > > > > > get the rest of these to work properly.
> > > > > 
> > > > > Greg, if I get you here correctly, you've applied all 9 patches from v1
> > > > > and none from v2, so what you need now is a v1->v2 patch, right?
> > > > 
> > > > No, I think I applied the patches sent _before_ the 9 series, it was 4
> > > > or 5 or so, you should have gotten an email about them.  Pull my
> > > > staging-testing branch and redo your remaining patches please.
> > > 
> > > And the reason I got confused was because you didn't label your second
> > > set of patches "v2", which it was, I saw two separate series, one with a
> > > few patches, and then 2 sets of 9, the second set labeled "v2" so I
> > > thought they were independant.  Please think of the poor maintainer who
> > > has to decipher things like this when you send them out...
> > 
> > I'm confused right now. As you say, first I've sent a patchset of 4:
> > https://lkml.org/lkml/2014/11/11/963
> 
> Which I applied.
> 
> > Then, a couple of days later, I've sent the initial patchset of 9:
> > https://lkml.org/lkml/2014/11/18/922
> > 
> > And a day I've sent a fixed version of the above patchset, labeled with v2:
> > https://lkml.org/lkml/2014/11/19/653
> 
> So I thought your series of 9 was separate from the series of 4, you can
> see my confusion (remember, I receive on _average_ about 1000 emails a day).
> 
> > Isn't this the right way to do? I still don't get my mistake. Because
> > what I was just about to do is to resend the v2 patchset, but now I'm
> > not sure anymore if this is what I'm supposed to do.
> > 
> > BTW: Out of these 3 patchsets, 1st and 3rd should be applied.
> 
> I tried to apply the 3rd, but it didn't apply due to patches I applied
> in your first set of 4 patches.
> 
> Does that help?

Yes, thanks. I've resent the patchset yesterday as v3:
https://lkml.org/lkml/2014/11/27/805

It applies cleanly to current staging-testing (aa9d9be),
I've just retested it. Please give it a try.

Thanks,
Mariusz

Re: [PATCH v2 4/9] staging: panel: Use defined value or checking module params state

[Greg Kroah-Hartman]

On Fri, Nov 28, 2014 at 09:57:06PM +0100, Mariusz Gorski wrote:
> > I tried to apply the 3rd, but it didn't apply due to patches I applied
> > in your first set of 4 patches.
> > 
> > Does that help?
> 
> Yes, thanks. I've resent the patchset yesterday as v3:
> https://lkml.org/lkml/2014/11/27/805
> 
> It applies cleanly to current staging-testing (aa9d9be),
> I've just retested it. Please give it a try.

Thanks, I'll do that when I get around to applying staging patches
again, sometime next week, I'm supposed to be on vacation right now...

greg k-h

[PATCH v2 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>
Acked-by: Willy Tarreau <w@1wt.eu>
---
 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 5288990..7e5bb53 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -212,6 +212,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;
@@ -219,6 +223,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;
@@ -1393,7 +1400,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
@@ -1923,7 +1930,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();
 
@@ -1935,7 +1942,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);
@@ -2114,7 +2121,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
@@ -2170,13 +2177,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;
@@ -2184,7 +2191,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);
@@ -2202,12 +2209,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;
 	}
@@ -2283,8 +2290,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:
@@ -2309,7 +2316,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);
@@ -2344,12 +2351,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

[PATCH v2 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 7e5bb53..5868a28 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -227,6 +227,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 */
@@ -1415,7 +1418,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 (lcd_proto == NOT_SET)
@@ -2233,28 +2236,21 @@ static struct parport_driver panel_driver = {
 /* init function */
 static int __init panel_init_module(void)
 {
-	/* for backwards compatibility */
-	if (keypad_type == NOT_SET)
-		keypad_type = keypad_enabled;
-
-	if (lcd_type == NOT_SET)
-		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 (keypad_type == NOT_SET)
-			keypad_type = DEFAULT_KEYPAD_TYPE;
-		if (lcd_type == NOT_SET)
-			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 (keypad_type == NOT_SET)
-			keypad_type = KEYPAD_TYPE_OLD;
-		if (lcd_type == NOT_SET)
-			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 (lcd_width == NOT_SET)
 			lcd_width = 16;
 		if (lcd_hwidth == NOT_SET)
@@ -2262,38 +2258,45 @@ static int __init panel_init_module(void)
 		break;
 	case PANEL_PROFILE_NEW:
 		/* serial, 2*16, new keypad */
-		if (keypad_type == NOT_SET)
-			keypad_type = KEYPAD_TYPE_NEW;
-		if (lcd_type == NOT_SET)
-			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 (keypad_type == NOT_SET)
-			keypad_type = KEYPAD_TYPE_NONE;
-		if (lcd_type == NOT_SET)
-			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 (keypad_type == NOT_SET)
-			keypad_type = KEYPAD_TYPE_NEXCOM;
-		if (lcd_type == NOT_SET)
-			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 (keypad_type == NOT_SET)
-			keypad_type = KEYPAD_TYPE_OLD;
-		if (lcd_type == NOT_SET)
-			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 v2 6/9] staging: panel: Make two more module params read-only

[Willy Tarreau]

On Wed, Nov 19, 2014 at 09:38:48PM +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>

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

[PATCH v2 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 5868a28..0bdb447 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -225,6 +225,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 */
@@ -766,7 +781,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  */
@@ -865,23 +880,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();
 }
 
@@ -895,7 +910,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);
@@ -918,7 +933,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, ' ');
 
@@ -956,7 +971,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);
@@ -981,7 +996,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 */
@@ -1095,17 +1110,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++;
 		}
@@ -1124,7 +1139,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 */
@@ -1272,7 +1287,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--;
@@ -1289,10 +1304,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':
@@ -1421,174 +1436,164 @@ static void lcd_init(void)
 	switch (selected_lcd_type) {
 	case LCD_TYPE_OLD:
 		/* parallel mode, 8 bits */
-		if (lcd_proto == NOT_SET)
-			lcd_proto = LCD_PROTO_PARALLEL;
-		if (lcd_charset == NOT_SET)
-			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 == NOT_SET)
-			lcd_width = 40;
-		if (lcd_bwidth == NOT_SET)
-			lcd_bwidth = 40;
-		if (lcd_hwidth == NOT_SET)
-			lcd_hwidth = 64;
-		if (lcd_height == NOT_SET)
-			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 (lcd_proto == NOT_SET)
-			lcd_proto = LCD_PROTO_SERIAL;
-		if (lcd_charset == NOT_SET)
-			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 (lcd_width == NOT_SET)
-			lcd_width = 16;
-		if (lcd_bwidth == NOT_SET)
-			lcd_bwidth = 40;
-		if (lcd_hwidth == NOT_SET)
-			lcd_hwidth = 16;
-		if (lcd_height == NOT_SET)
-			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 (lcd_proto == NOT_SET)
-			lcd_proto = LCD_PROTO_PARALLEL;
-		if (lcd_charset == NOT_SET)
-			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 (lcd_width == NOT_SET)
-			lcd_width = 16;
-		if (lcd_bwidth == NOT_SET)
-			lcd_bwidth = 40;
-		if (lcd_hwidth == NOT_SET)
-			lcd_hwidth = 64;
-		if (lcd_height == NOT_SET)
-			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 (lcd_proto == NOT_SET)
-			lcd_proto = DEFAULT_LCD_PROTO;
-		if (lcd_charset == NOT_SET)
-			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 (lcd_proto == NOT_SET)
-			lcd_proto = LCD_PROTO_PARALLEL;
-		if (lcd_charset == NOT_SET)
-			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 == NOT_SET)
-			lcd_width = 16;
-		if (lcd_bwidth == NOT_SET)
-			lcd_bwidth = 40;
-		if (lcd_hwidth == NOT_SET)
-			lcd_hwidth = 64;
-		if (lcd_height == NOT_SET)
-			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 (lcd_charset == NOT_SET)
-		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 (lcd.charset == NOT_SET)
+		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.
@@ -2279,6 +2284,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 v2 7/9] staging: panel: Refactor LCD init code

[Willy Tarreau]

On Wed, Nov 19, 2014 at 09:38:49PM +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>

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

[Mariusz Gorski]

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

Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
---
v2: Don't introduce new macros for param value check

 drivers/staging/panel/panel.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 0bdb447..19f6767 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -1494,17 +1494,17 @@ static void lcd_init(void)
 	}
 
 	/* Overwrite with module params set on loading */
-	if (lcd_height > -1)
+	if (lcd_height != NOT_SET)
 		lcd.height = lcd_height;
-	if (lcd_width > -1)
+	if (lcd_width != NOT_SET)
 		lcd.width = lcd_width;
-	if (lcd_bwidth > -1)
+	if (lcd_bwidth != NOT_SET)
 		lcd.bwidth = lcd_bwidth;
-	if (lcd_hwidth > -1)
+	if (lcd_hwidth != NOT_SET)
 		lcd.hwidth = lcd_hwidth;
-	if (lcd_charset > -1)
+	if (lcd_charset != NOT_SET)
 		lcd.charset = lcd_charset;
-	if (lcd_proto > -1)
+	if (lcd_proto != NOT_SET)
 		lcd.proto = lcd_proto;
 	if (lcd_e_pin != PIN_NOT_SET)
 		lcd.pins.e = lcd_e_pin;
@@ -2304,16 +2304,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 (keypad_enabled != NOT_SET)
 		selected_keypad_type = keypad_enabled;
-	if (keypad_type > -1)
+	if (keypad_type != NOT_SET)
 		selected_keypad_type = keypad_type;
 
 	keypad.enabled = (selected_keypad_type > 0);
 
-	if (lcd_enabled > -1)
+	if (lcd_enabled != NOT_SET)
 		selected_lcd_type = lcd_enabled;
-	if (lcd_type > -1)
+	if (lcd_type != NOT_SET)
 		selected_lcd_type = lcd_type;
 
 	lcd.enabled = (selected_lcd_type > 0);
-- 
2.1.3

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

[Willy Tarreau]

On Wed, Nov 19, 2014 at 09:38:50PM +0100, Mariusz Gorski wrote:
> Use a defined value instead of magic number comparison
> for checking whether a module param value has been set.
> 
> Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>

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

[PATCH v2 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>
Acked-by: Willy Tarreau <w@1wt.eu>
---
 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 19f6767..98325b7 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -225,12 +225,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;
@@ -240,22 +248,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.
@@ -438,13 +450,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);
@@ -880,23 +887,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();
 }
 
@@ -905,8 +912,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);
@@ -918,8 +925,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();
 }
 
@@ -928,8 +935,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);
@@ -956,8 +963,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();
 }
 
@@ -966,8 +973,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);
@@ -979,8 +986,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();
 }
 
@@ -988,15 +995,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 */
@@ -1009,8 +1016,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);
 
@@ -1018,12 +1025,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);
 
@@ -1046,100 +1053,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 */
@@ -1149,7 +1157,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': {
@@ -1220,11 +1228,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;
@@ -1237,25 +1245,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);
@@ -1268,29 +1276,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(' ');
@@ -1304,15 +1312,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':
@@ -1328,32 +1336,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 */
 }
 
@@ -1386,9 +1394,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);
 }
@@ -1418,7 +1426,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
@@ -1599,7 +1607,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 */
@@ -1611,10 +1619,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();
 }
 
@@ -1950,14 +1958,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);
 		}
 	}
@@ -2129,7 +2139,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
@@ -2222,9 +2232,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);
@@ -2300,6 +2310,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.
@@ -2385,7 +2398,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 v2 0/9] staging: panel: Refactor panel initialization

[Willy Tarreau]

Hi Mariusz,

On Wed, Nov 19, 2014 at 09:38:42PM +0100, Mariusz Gorski wrote:
> 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.
> 
> v2: Don't introduce new macros for param value check

(...)

Great work, the code is already significantly cleaner now, thanks
for doing this work! I've acked all the new patches.

Best regards,
Willy