[PATCH v1 1/1] pps: generators: pps_gen_parport: Switch to use module_parport_driver()

[PATCH v1 1/1] pps: generators: pps_gen_parport: Switch to use module_parport_driver()

[Andy Shevchenko]

Switch to use module_parport_driver() to reduce boilerplate code.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pps/generators/pps_gen_parport.c | 44 +++++-------------------
 1 file changed, 9 insertions(+), 35 deletions(-)

diff --git a/drivers/pps/generators/pps_gen_parport.c b/drivers/pps/generators/pps_gen_parport.c
index 6a1af7664f3b..565f99782402 100644
--- a/drivers/pps/generators/pps_gen_parport.c
+++ b/drivers/pps/generators/pps_gen_parport.c
@@ -20,8 +20,6 @@
 #include <linux/hrtimer.h>
 #include <linux/parport.h>
 
-#define DRVDESC "parallel port PPS signal generator"
-
 #define SIGNAL		0
 #define NO_SIGNAL	PARPORT_CONTROL_STROBE
 
@@ -180,6 +178,11 @@ static void parport_attach(struct parport *port)
 {
 	struct pardev_cb pps_cb;
 
+	if (send_delay > SEND_DELAY_MAX) {
+		pr_err("delay value should be not greater then %d\n", SEND_DELAY_MAX);
+		return -EINVAL;
+	}
+
 	if (attached) {
 		/* we already have a port */
 		return;
@@ -223,6 +226,8 @@ static void parport_detach(struct parport *port)
 	hrtimer_cancel(&device.timer);
 	parport_release(device.pardev);
 	parport_unregister_device(device.pardev);
+
+	pr_info("hrtimer avg error is %ldns\n", hrtimer_error);
 }
 
 static struct parport_driver pps_gen_parport_driver = {
@@ -231,39 +236,8 @@ static struct parport_driver pps_gen_parport_driver = {
 	.detach = parport_detach,
 	.devmodel = true,
 };
-
-/* module staff */
-
-static int __init pps_gen_parport_init(void)
-{
-	int ret;
-
-	pr_info(DRVDESC "\n");
-
-	if (send_delay > SEND_DELAY_MAX) {
-		pr_err("delay value should be not greater"
-				" then %d\n", SEND_DELAY_MAX);
-		return -EINVAL;
-	}
-
-	ret = parport_register_driver(&pps_gen_parport_driver);
-	if (ret) {
-		pr_err("unable to register with parport\n");
-		return ret;
-	}
-
-	return  0;
-}
-
-static void __exit pps_gen_parport_exit(void)
-{
-	parport_unregister_driver(&pps_gen_parport_driver);
-	pr_info("hrtimer avg error is %ldns\n", hrtimer_error);
-}
-
-module_init(pps_gen_parport_init);
-module_exit(pps_gen_parport_exit);
+module_parport_driver(pps_gen_parport_driver);
 
 MODULE_AUTHOR("Alexander Gordeev <lasaine@lvk.cs.msu.su>");
-MODULE_DESCRIPTION(DRVDESC);
+MODULE_DESCRIPTION("parallel port PPS signal generator");
 MODULE_LICENSE("GPL");
-- 
2.30.2

Re: [PATCH v1 1/1] pps: generators: pps_gen_parport: Switch to use module_parport_driver()

[Greg Kroah-Hartman]

On Wed, Jun 16, 2021 at 05:31:21PM +0300, Andy Shevchenko wrote:
> Switch to use module_parport_driver() to reduce boilerplate code.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pps/generators/pps_gen_parport.c | 44 +++++-------------------
>  1 file changed, 9 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/pps/generators/pps_gen_parport.c b/drivers/pps/generators/pps_gen_parport.c
> index 6a1af7664f3b..565f99782402 100644
> --- a/drivers/pps/generators/pps_gen_parport.c
> +++ b/drivers/pps/generators/pps_gen_parport.c
> @@ -20,8 +20,6 @@
>  #include <linux/hrtimer.h>
>  #include <linux/parport.h>
>  
> -#define DRVDESC "parallel port PPS signal generator"
> -
>  #define SIGNAL		0
>  #define NO_SIGNAL	PARPORT_CONTROL_STROBE
>  
> @@ -180,6 +178,11 @@ static void parport_attach(struct parport *port)
>  {
>  	struct pardev_cb pps_cb;
>  
> +	if (send_delay > SEND_DELAY_MAX) {
> +		pr_err("delay value should be not greater then %d\n", SEND_DELAY_MAX);
> +		return -EINVAL;

Note this is at a later point in time than before, are you sure this is
ok?

> +	}
> +
>  	if (attached) {
>  		/* we already have a port */
>  		return;
> @@ -223,6 +226,8 @@ static void parport_detach(struct parport *port)
>  	hrtimer_cancel(&device.timer);
>  	parport_release(device.pardev);
>  	parport_unregister_device(device.pardev);
> +
> +	pr_info("hrtimer avg error is %ldns\n", hrtimer_error);

Why is this line needed?  When drivers work properly, they are quiet.

I know the existing code has it, no reason it needs to stay here, that's
why we created these macros, to remove the ability for drivers to be
printing junk like this that they do not need to be printing.

thanks,

greg k-h

Re: [PATCH v1 1/1] pps: generators: pps_gen_parport: Switch to use module_parport_driver()

[Andy Shevchenko]

+Cc: Alexander (I believe he is the author of the original code, sorry if not)

On Wed, Jun 16, 2021 at 5:47 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jun 16, 2021 at 05:31:21PM +0300, Andy Shevchenko wrote:
> > Switch to use module_parport_driver() to reduce boilerplate code.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/pps/generators/pps_gen_parport.c | 44 +++++-------------------
> >  1 file changed, 9 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/pps/generators/pps_gen_parport.c b/drivers/pps/generators/pps_gen_parport.c
> > index 6a1af7664f3b..565f99782402 100644
> > --- a/drivers/pps/generators/pps_gen_parport.c
> > +++ b/drivers/pps/generators/pps_gen_parport.c
> > @@ -20,8 +20,6 @@
> >  #include <linux/hrtimer.h>
> >  #include <linux/parport.h>
> >
> > -#define DRVDESC "parallel port PPS signal generator"
> > -
> >  #define SIGNAL               0
> >  #define NO_SIGNAL    PARPORT_CONTROL_STROBE
> >
> > @@ -180,6 +178,11 @@ static void parport_attach(struct parport *port)
> >  {
> >       struct pardev_cb pps_cb;
> >
> > +     if (send_delay > SEND_DELAY_MAX) {
> > +             pr_err("delay value should be not greater then %d\n", SEND_DELAY_MAX);
> > +             return -EINVAL;
>
> Note this is at a later point in time than before, are you sure this is
> ok?

Yes, it doesn't matter when we check the module parameter.

If it's writable we even have more flexibility of changing it at
runtime (when built-in the kernel) after this patch.

> > +     }
> > +
> >       if (attached) {
> >               /* we already have a port */
> >               return;
> > @@ -223,6 +226,8 @@ static void parport_detach(struct parport *port)
> >       hrtimer_cancel(&device.timer);
> >       parport_release(device.pardev);
> >       parport_unregister_device(device.pardev);
> > +
> > +     pr_info("hrtimer avg error is %ldns\n", hrtimer_error);
>
> Why is this line needed?  When drivers work properly, they are quiet.
>
> I know the existing code has it, no reason it needs to stay here, that's
> why we created these macros, to remove the ability for drivers to be
> printing junk like this that they do not need to be printing.

I believe it may be useful for some debugging / statistics. But let's
ask Alexander about this.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] pps: generators: pps_gen_parport: Switch to use module_parport_driver()

[Alexander Gordeev]

On Wed, Jun 16, 2021 at 05:57:47PM +0300, Andy Shevchenko wrote:
> +Cc: Alexander (I believe he is the author of the original code, sorry if not)

Hi Andy,

No problem, but I am not the author, unfortunately. 
CC-ed another Alexander
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v1 1/1] pps: generators: pps_gen_parport: Switch to use module_parport_driver()

[Andy Shevchenko]

On Wed, Jun 16, 2021 at 04:44:38PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jun 16, 2021 at 05:31:21PM +0300, Andy Shevchenko wrote:
> > Switch to use module_parport_driver() to reduce boilerplate code.

> > +	pr_info("hrtimer avg error is %ldns\n", hrtimer_error);
> 
> Why is this line needed?  When drivers work properly, they are quiet.
> 
> I know the existing code has it, no reason it needs to stay here, that's
> why we created these macros, to remove the ability for drivers to be
> printing junk like this that they do not need to be printing.

Seems the original authors keeps silent. What do you suggest in this case?
Remove the line and resubmit?

-- 
With Best Regards,
Andy Shevchenko