[PATCH 1/3] staging: panel: register reboot

[PATCH 1/3] staging: panel: register reboot

[Sudip Mukherjee]

we donot need the reboot notifier in module init section, as the
notifier is used after lcd is initialized. so lets register for the
reboot notifier only after we have successfully attached to the
parallel port. and similarly unregister at detach.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/staging/panel/panel.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index f329e3f..3ef3dcf 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -2206,6 +2206,7 @@ static void panel_attach(struct parport *port)
 		if (misc_register(&keypad_dev))
 			goto err_lcd_unreg;
 	}
+	register_reboot_notifier(&panel_notifier);
 	return;
 
 err_lcd_unreg:
@@ -2227,6 +2228,8 @@ static void panel_detach(struct parport *port)
 		return;
 	}
 
+	unregister_reboot_notifier(&panel_notifier);
+
 	if (keypad.enabled && keypad_initialized) {
 		misc_deregister(&keypad_dev);
 		keypad_initialized = 0;
@@ -2293,7 +2296,6 @@ static int __init panel_init_module(void)
 		break;
 	}
 
-
 	/*
 	 * Overwrite selection with module param values (both keypad and lcd),
 	 * where the deprecated params have lower prio.
@@ -2363,8 +2365,6 @@ static int __init panel_init_module(void)
 		return -EIO;
 	}
 
-	register_reboot_notifier(&panel_notifier);
-
 	if (pprt)
 		pr_info("driver version " PANEL_VERSION
 			" registered on parport%d (io=0x%lx).\n", parport,
@@ -2380,7 +2380,6 @@ static int __init panel_init_module(void)
 
 static void __exit panel_cleanup_module(void)
 {
-	unregister_reboot_notifier(&panel_notifier);
 
 	if (scan_timer.function != NULL)
 		del_timer_sync(&scan_timer);
-- 
1.8.1.2

[PATCH 2/3] staging: panel: return register value

[Sudip Mukherjee]

we were returning success even if the module failed to register.
now we are returning the actual return value, success or error.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 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 3ef3dcf..b4c143a 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -2360,11 +2360,6 @@ static int __init panel_init_module(void)
 		return -ENODEV;
 	}
 
-	if (parport_register_driver(&panel_driver)) {
-		pr_err("could not register with parport. Aborting.\n");
-		return -EIO;
-	}
-
 	if (pprt)
 		pr_info("driver version " PANEL_VERSION
 			" registered on parport%d (io=0x%lx).\n", parport,
@@ -2375,7 +2370,7 @@ static int __init panel_init_module(void)
 	/* tells various subsystems about the fact that initialization
 	   is finished */
 	init_in_progress = 0;
-	return 0;
+	return parport_register_driver(&panel_driver);
 }
 
 static void __exit panel_cleanup_module(void)
-- 
1.8.1.2

Re: [PATCH 2/3] staging: panel: return register value

[Willy Tarreau]

On Sun, Mar 08, 2015 at 11:07:25PM +0530, Sudip Mukherjee wrote:
> we were returning success even if the module failed to register.
> now we are returning the actual return value, success or error.
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
>  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 3ef3dcf..b4c143a 100644
> --- a/drivers/staging/panel/panel.c
> +++ b/drivers/staging/panel/panel.c
> @@ -2360,11 +2360,6 @@ static int __init panel_init_module(void)
>  		return -ENODEV;
>  	}
>  
> -	if (parport_register_driver(&panel_driver)) {
> -		pr_err("could not register with parport. Aborting.\n");
> -		return -EIO;
> -	}
> -
>  	if (pprt)
>  		pr_info("driver version " PANEL_VERSION
>  			" registered on parport%d (io=0x%lx).\n", parport,
> @@ -2375,7 +2370,7 @@ static int __init panel_init_module(void)
>  	/* tells various subsystems about the fact that initialization
>  	   is finished */
>  	init_in_progress = 0;
> -	return 0;
> +	return parport_register_driver(&panel_driver);

Here I'd rather keep the error message, as it's quite annoying when a
driver does not load and you don't know why, especially with something
like parport where there are many possible causes. Something like this
would be better in my opinion :

-	return 0;
+	err = parport_register_driver(&panel_driver);
+	if (err)
+		pr_err("could not register with parport. Aborting.\n");
+	return err;

Well, I just found that currently parport_register_driver() always
succeeds, but I still think it's better to verbosely handle errors.

Thanks,
Willy

Re: [PATCH 2/3] staging: panel: return register value

[Sudip Mukherjee]

On Mon, Mar 09, 2015 at 07:30:07AM +0100, Willy Tarreau wrote:
> On Sun, Mar 08, 2015 at 11:07:25PM +0530, Sudip Mukherjee wrote:
> >  
> > -	if (parport_register_driver(&panel_driver)) {
> > -		pr_err("could not register with parport. Aborting.\n");
> > -		return -EIO;
> > -	}
> > -
> >  	if (pprt)
> >  		pr_info("driver version " PANEL_VERSION
> >  			" registered on parport%d (io=0x%lx).\n", parport,
> > @@ -2375,7 +2370,7 @@ static int __init panel_init_module(void)
> >  	/* tells various subsystems about the fact that initialization
> >  	   is finished */
> >  	init_in_progress = 0;
> > -	return 0;
> > +	return parport_register_driver(&panel_driver);
> 
> Here I'd rather keep the error message, as it's quite annoying when a
> driver does not load and you don't know why, especially with something
> like parport where there are many possible causes. Something like this
> would be better in my opinion :
> 
> -	return 0;
> +	err = parport_register_driver(&panel_driver);
> +	if (err)
> +		pr_err("could not register with parport. Aborting.\n");
> +	return err;
> 
> Well, I just found that currently parport_register_driver() always
> succeeds, but I still think it's better to verbosely handle errors.

ok, i will send a v2, but is the error message really required? considering the fact that parport_register_driver() always succeeds ..

regards
sudip

> 
> Thanks,
> Willy
> 

Re: [PATCH 2/3] staging: panel: return register value

[Willy Tarreau]

On Mon, Mar 09, 2015 at 07:05:59PM +0530, Sudip Mukherjee wrote:
> On Mon, Mar 09, 2015 at 07:30:07AM +0100, Willy Tarreau wrote:
> > On Sun, Mar 08, 2015 at 11:07:25PM +0530, Sudip Mukherjee wrote:
> > >  
> > > -	if (parport_register_driver(&panel_driver)) {
> > > -		pr_err("could not register with parport. Aborting.\n");
> > > -		return -EIO;
> > > -	}
> > > -
> > >  	if (pprt)
> > >  		pr_info("driver version " PANEL_VERSION
> > >  			" registered on parport%d (io=0x%lx).\n", parport,
> > > @@ -2375,7 +2370,7 @@ static int __init panel_init_module(void)
> > >  	/* tells various subsystems about the fact that initialization
> > >  	   is finished */
> > >  	init_in_progress = 0;
> > > -	return 0;
> > > +	return parport_register_driver(&panel_driver);
> > 
> > Here I'd rather keep the error message, as it's quite annoying when a
> > driver does not load and you don't know why, especially with something
> > like parport where there are many possible causes. Something like this
> > would be better in my opinion :
> > 
> > -	return 0;
> > +	err = parport_register_driver(&panel_driver);
> > +	if (err)
> > +		pr_err("could not register with parport. Aborting.\n");
> > +	return err;
> > 
> > Well, I just found that currently parport_register_driver() always
> > succeeds, but I still think it's better to verbosely handle errors.
> 
> ok, i will send a v2, but is the error message really required? considering
> the fact that parport_register_driver() always succeeds ..

It's a matter of choice :
   - either we consider that it can fail and we emit an appropriate
     error message ;

   - or we consider it cannot fail and instead of returning its own
     code we always call it without checking it then return zero. That
     way if it were ever to change, it would be easy to spot the change.

I still find that the first choice is appropriate, comes with a very low
cost and is in line with what the parrallel port driver does as well.

Regards,
Willy

[PATCH 3/3] staging: panel: remove initialization check

[Sudip Mukherjee]

no need to monitor init_in_progress now as keypad_send_key() can only
be called after the timer is initialized. and timer is initialized
from keypad_init() which is in the attach section and can only execute
after the module has initialized.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/staging/panel/panel.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index b4c143a..16fee49 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -452,8 +452,6 @@ static struct pardevice *pprt;
 
 static int keypad_initialized;
 
-static char init_in_progress;
-
 static void (*lcd_write_cmd)(int);
 static void (*lcd_write_data)(int);
 static void (*lcd_clear_fast)(void);
@@ -1688,9 +1686,6 @@ static struct miscdevice keypad_dev = {
 
 static void keypad_send_key(const char *string, int max_len)
 {
-	if (init_in_progress)
-		return;
-
 	/* send the key to the device only if a process is attached to it. */
 	if (!atomic_read(&keypad_available)) {
 		while (max_len-- && keypad_buflen < KEYPAD_BUFFER && *string) {
@@ -2351,9 +2346,6 @@ static int __init panel_init_module(void)
 		break;
 	}
 
-	/* tells various subsystems about the fact that we are initializing */
-	init_in_progress = 1;
-
 	if (!lcd.enabled && !keypad.enabled) {
 		/* no device enabled, let's exit */
 		pr_err("driver version " PANEL_VERSION " disabled.\n");
@@ -2367,9 +2359,6 @@ static int __init panel_init_module(void)
 	else
 		pr_info("driver version " PANEL_VERSION
 			" not yet registered\n");
-	/* tells various subsystems about the fact that initialization
-	   is finished */
-	init_in_progress = 0;
 	return parport_register_driver(&panel_driver);
 }
 
-- 
1.8.1.2

Re: [PATCH 3/3] staging: panel: remove initialization check

[Willy Tarreau]

On Sun, Mar 08, 2015 at 11:07:26PM +0530, Sudip Mukherjee wrote:
> no need to monitor init_in_progress now as keypad_send_key() can only
> be called after the timer is initialized. and timer is initialized
> from keypad_init() which is in the attach section and can only execute
> after the module has initialized.
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>

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

willy

Re: [PATCH 1/3] staging: panel: register reboot

[Willy Tarreau]

On Sun, Mar 08, 2015 at 11:07:24PM +0530, Sudip Mukherjee wrote:
> we donot need the reboot notifier in module init section, as the
> notifier is used after lcd is initialized. so lets register for the
> reboot notifier only after we have successfully attached to the
> parallel port. and similarly unregister at detach.
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>

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

Willy