All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 1/1] pps: clients: parport: Switch to use module_parport_driver()
@ 2021-05-10 14:13 Andy Shevchenko
  2021-05-11  7:05 ` Rodolfo Giometti
  0 siblings, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2021-05-10 14:13 UTC (permalink / raw)
  To: Andy Shevchenko, linux-kernel; +Cc: Rodolfo Giometti, gregkh

Switch to use module_parport_driver() to reduce boilerplate code.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pps/clients/pps_parport.c | 42 ++++++-------------------------
 1 file changed, 8 insertions(+), 34 deletions(-)

diff --git a/drivers/pps/clients/pps_parport.c b/drivers/pps/clients/pps_parport.c
index 7a41fb7b0dec..42f93d4c6ee3 100644
--- a/drivers/pps/clients/pps_parport.c
+++ b/drivers/pps/clients/pps_parport.c
@@ -22,8 +22,6 @@
 #include <linux/parport.h>
 #include <linux/pps_kernel.h>
 
-#define DRVDESC "parallel port PPS client"
-
 /* module parameters */
 
 #define CLEAR_WAIT_MAX		100
@@ -138,6 +136,12 @@ static void parport_attach(struct parport *port)
 		.dev		= NULL
 	};
 
+	if (clear_wait > CLEAR_WAIT_MAX) {
+		pr_err("clear_wait value should be not greater then %d\n",
+		       CLEAR_WAIT_MAX);
+		return;
+	}
+
 	device = kzalloc(sizeof(struct pps_client_pp), GFP_KERNEL);
 	if (!device) {
 		pr_err("memory allocation failed, not attaching\n");
@@ -214,38 +218,8 @@ static struct parport_driver pps_parport_driver = {
 	.detach = parport_detach,
 	.devmodel = true,
 };
-
-/* module staff */
-
-static int __init pps_parport_init(void)
-{
-	int ret;
-
-	pr_info(DRVDESC "\n");
-
-	if (clear_wait > CLEAR_WAIT_MAX) {
-		pr_err("clear_wait value should be not greater"
-				" then %d\n", CLEAR_WAIT_MAX);
-		return -EINVAL;
-	}
-
-	ret = parport_register_driver(&pps_parport_driver);
-	if (ret) {
-		pr_err("unable to register with parport\n");
-		return ret;
-	}
-
-	return  0;
-}
-
-static void __exit pps_parport_exit(void)
-{
-	parport_unregister_driver(&pps_parport_driver);
-}
-
-module_init(pps_parport_init);
-module_exit(pps_parport_exit);
+module_parport_driver(pps_parport_driver);
 
 MODULE_AUTHOR("Alexander Gordeev <lasaine@lvk.cs.msu.su>");
-MODULE_DESCRIPTION(DRVDESC);
+MODULE_DESCRIPTION("parallel port PPS client");
 MODULE_LICENSE("GPL");
-- 
2.30.2


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1 1/1] pps: clients: parport: Switch to use module_parport_driver()
  2021-05-10 14:13 [PATCH v1 1/1] pps: clients: parport: Switch to use module_parport_driver() Andy Shevchenko
@ 2021-05-11  7:05 ` Rodolfo Giometti
  2021-05-11  7:18   ` Andy Shevchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Rodolfo Giometti @ 2021-05-11  7:05 UTC (permalink / raw)
  To: Andy Shevchenko, linux-kernel; +Cc: gregkh

On 10/05/21 16:13, 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/clients/pps_parport.c | 42 ++++++-------------------------
>  1 file changed, 8 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/pps/clients/pps_parport.c b/drivers/pps/clients/pps_parport.c
> index 7a41fb7b0dec..42f93d4c6ee3 100644
> --- a/drivers/pps/clients/pps_parport.c
> +++ b/drivers/pps/clients/pps_parport.c
> @@ -22,8 +22,6 @@
>  #include <linux/parport.h>
>  #include <linux/pps_kernel.h>
>  
> -#define DRVDESC "parallel port PPS client"
> -
>  /* module parameters */
>  
>  #define CLEAR_WAIT_MAX		100
> @@ -138,6 +136,12 @@ static void parport_attach(struct parport *port)
>  		.dev		= NULL
>  	};
>  
> +	if (clear_wait > CLEAR_WAIT_MAX) {
> +		pr_err("clear_wait value should be not greater then %d\n",
> +		       CLEAR_WAIT_MAX);
> +		return;
> +	}
> +

Why do you need to do so? Maybe a comment would be welcomed.

>  	device = kzalloc(sizeof(struct pps_client_pp), GFP_KERNEL);
>  	if (!device) {
>  		pr_err("memory allocation failed, not attaching\n");
> @@ -214,38 +218,8 @@ static struct parport_driver pps_parport_driver = {
>  	.detach = parport_detach,
>  	.devmodel = true,
>  };
> -
> -/* module staff */
> -
> -static int __init pps_parport_init(void)
> -{
> -	int ret;
> -
> -	pr_info(DRVDESC "\n");
> -
> -	if (clear_wait > CLEAR_WAIT_MAX) {
> -		pr_err("clear_wait value should be not greater"
> -				" then %d\n", CLEAR_WAIT_MAX);
> -		return -EINVAL;
> -	}
> -
> -	ret = parport_register_driver(&pps_parport_driver);
> -	if (ret) {
> -		pr_err("unable to register with parport\n");
> -		return ret;
> -	}
> -
> -	return  0;
> -}
> -
> -static void __exit pps_parport_exit(void)
> -{
> -	parport_unregister_driver(&pps_parport_driver);
> -}
> -
> -module_init(pps_parport_init);
> -module_exit(pps_parport_exit);
> +module_parport_driver(pps_parport_driver);
>  
>  MODULE_AUTHOR("Alexander Gordeev <lasaine@lvk.cs.msu.su>");
> -MODULE_DESCRIPTION(DRVDESC);
> +MODULE_DESCRIPTION("parallel port PPS client");
>  MODULE_LICENSE("GPL");
> 

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1 1/1] pps: clients: parport: Switch to use module_parport_driver()
  2021-05-11  7:05 ` Rodolfo Giometti
@ 2021-05-11  7:18   ` Andy Shevchenko
  2021-05-11  7:26     ` Rodolfo Giometti
  0 siblings, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2021-05-11  7:18 UTC (permalink / raw)
  To: Rodolfo Giometti; +Cc: linux-kernel, gregkh

On Tue, May 11, 2021 at 09:05:00AM +0200, Rodolfo Giometti wrote:
> On 10/05/21 16:13, 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/clients/pps_parport.c | 42 ++++++-------------------------
> >  1 file changed, 8 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/pps/clients/pps_parport.c b/drivers/pps/clients/pps_parport.c
> > index 7a41fb7b0dec..42f93d4c6ee3 100644
> > --- a/drivers/pps/clients/pps_parport.c
> > +++ b/drivers/pps/clients/pps_parport.c
> > @@ -22,8 +22,6 @@
> >  #include <linux/parport.h>
> >  #include <linux/pps_kernel.h>
> >  
> > -#define DRVDESC "parallel port PPS client"
> > -
> >  /* module parameters */
> >  
> >  #define CLEAR_WAIT_MAX		100
> > @@ -138,6 +136,12 @@ static void parport_attach(struct parport *port)
> >  		.dev		= NULL
> >  	};
> >  
> > +	if (clear_wait > CLEAR_WAIT_MAX) {
> > +		pr_err("clear_wait value should be not greater then %d\n",
> > +		       CLEAR_WAIT_MAX);
> > +		return;
> > +	}
> > +
> 
> Why do you need to do so? Maybe a comment would be welcomed.

It's in original code, I just moved it to ->probe().

What comment do you want to have here, because original code has no comment (I
think in any case it's out of scope of this change, but may be prepended or
appended to the series)?

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1 1/1] pps: clients: parport: Switch to use module_parport_driver()
  2021-05-11  7:18   ` Andy Shevchenko
@ 2021-05-11  7:26     ` Rodolfo Giometti
  2021-05-11  7:50       ` Andy Shevchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Rodolfo Giometti @ 2021-05-11  7:26 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-kernel, gregkh, Alexander Gordeev

On 11/05/21 09:18, Andy Shevchenko wrote:
> On Tue, May 11, 2021 at 09:05:00AM +0200, Rodolfo Giometti wrote:
>> On 10/05/21 16:13, 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/clients/pps_parport.c | 42 ++++++-------------------------
>>>  1 file changed, 8 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/drivers/pps/clients/pps_parport.c b/drivers/pps/clients/pps_parport.c
>>> index 7a41fb7b0dec..42f93d4c6ee3 100644
>>> --- a/drivers/pps/clients/pps_parport.c
>>> +++ b/drivers/pps/clients/pps_parport.c
>>> @@ -22,8 +22,6 @@
>>>  #include <linux/parport.h>
>>>  #include <linux/pps_kernel.h>
>>>  
>>> -#define DRVDESC "parallel port PPS client"
>>> -
>>>  /* module parameters */
>>>  
>>>  #define CLEAR_WAIT_MAX		100
>>> @@ -138,6 +136,12 @@ static void parport_attach(struct parport *port)
>>>  		.dev		= NULL
>>>  	};
>>>  
>>> +	if (clear_wait > CLEAR_WAIT_MAX) {
>>> +		pr_err("clear_wait value should be not greater then %d\n",
>>> +		       CLEAR_WAIT_MAX);
>>> +		return;
>>> +	}
>>> +
>>
>> Why do you need to do so? Maybe a comment would be welcomed.
> 
> It's in original code, I just moved it to ->probe().
> 
> What comment do you want to have here, because original code has no comment (I
> think in any case it's out of scope of this change, but may be prepended or
> appended to the series)?

Mmm... these functions can be called at different times, so I don't know if we
can just move the code safely.

Maybe Alexander (in CC) can help us? :)

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1 1/1] pps: clients: parport: Switch to use module_parport_driver()
  2021-05-11  7:26     ` Rodolfo Giometti
@ 2021-05-11  7:50       ` Andy Shevchenko
  2021-05-11  8:46         ` Rodolfo Giometti
  0 siblings, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2021-05-11  7:50 UTC (permalink / raw)
  To: Rodolfo Giometti; +Cc: linux-kernel, gregkh, Alexander Gordeev

On Tue, May 11, 2021 at 09:26:36AM +0200, Rodolfo Giometti wrote:
> On 11/05/21 09:18, Andy Shevchenko wrote:
> > On Tue, May 11, 2021 at 09:05:00AM +0200, Rodolfo Giometti wrote:
> >> On 10/05/21 16:13, 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/clients/pps_parport.c | 42 ++++++-------------------------
> >>>  1 file changed, 8 insertions(+), 34 deletions(-)
> >>>
> >>> diff --git a/drivers/pps/clients/pps_parport.c b/drivers/pps/clients/pps_parport.c
> >>> index 7a41fb7b0dec..42f93d4c6ee3 100644
> >>> --- a/drivers/pps/clients/pps_parport.c
> >>> +++ b/drivers/pps/clients/pps_parport.c
> >>> @@ -22,8 +22,6 @@
> >>>  #include <linux/parport.h>
> >>>  #include <linux/pps_kernel.h>
> >>>  
> >>> -#define DRVDESC "parallel port PPS client"
> >>> -
> >>>  /* module parameters */
> >>>  
> >>>  #define CLEAR_WAIT_MAX		100
> >>> @@ -138,6 +136,12 @@ static void parport_attach(struct parport *port)
> >>>  		.dev		= NULL
> >>>  	};
> >>>  
> >>> +	if (clear_wait > CLEAR_WAIT_MAX) {
> >>> +		pr_err("clear_wait value should be not greater then %d\n",
> >>> +		       CLEAR_WAIT_MAX);
> >>> +		return;
> >>> +	}
> >>> +
> >>
> >> Why do you need to do so? Maybe a comment would be welcomed.
> > 
> > It's in original code, I just moved it to ->probe().
> > 
> > What comment do you want to have here, because original code has no comment (I
> > think in any case it's out of scope of this change, but may be prepended or
> > appended to the series)?
> 
> Mmm... these functions can be called at different times, so I don't know if we
> can just move the code safely.

I do not see any issue here. TL;DR: it won't be worse, but might even give an
improvement.

Before it prevented to module to be initialized,
now one may amend this at run time. the downside is that now it will require
module removal and inserting versus just two attempts of inserting in a row.

For the built-in case it shouldn't change much (but if
/sys/module/.../parameters/... is writable for this, then it will allow to do
the similar trick as above, so extending functionality with the flexibility,
means direct improvement).

Okay, permissions are 0 there, I don't remember what it means, maybe the
parameter won't be available under /sysfs at all, but again, it won't change
the functional behaviour, the downside is the memory consumed by the 'built-in'
code at run time.

> Maybe Alexander (in CC) can help us? :)

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1 1/1] pps: clients: parport: Switch to use module_parport_driver()
  2021-05-11  7:50       ` Andy Shevchenko
@ 2021-05-11  8:46         ` Rodolfo Giometti
  2021-05-11 14:11           ` Andy Shevchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Rodolfo Giometti @ 2021-05-11  8:46 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-kernel, gregkh, Alexander Gordeev

On 11/05/21 09:50, Andy Shevchenko wrote:
> On Tue, May 11, 2021 at 09:26:36AM +0200, Rodolfo Giometti wrote:
>> On 11/05/21 09:18, Andy Shevchenko wrote:
>>> On Tue, May 11, 2021 at 09:05:00AM +0200, Rodolfo Giometti wrote:
>>>> On 10/05/21 16:13, 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/clients/pps_parport.c | 42 ++++++-------------------------
>>>>>  1 file changed, 8 insertions(+), 34 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pps/clients/pps_parport.c b/drivers/pps/clients/pps_parport.c
>>>>> index 7a41fb7b0dec..42f93d4c6ee3 100644
>>>>> --- a/drivers/pps/clients/pps_parport.c
>>>>> +++ b/drivers/pps/clients/pps_parport.c
>>>>> @@ -22,8 +22,6 @@
>>>>>  #include <linux/parport.h>
>>>>>  #include <linux/pps_kernel.h>
>>>>>  
>>>>> -#define DRVDESC "parallel port PPS client"
>>>>> -
>>>>>  /* module parameters */
>>>>>  
>>>>>  #define CLEAR_WAIT_MAX		100
>>>>> @@ -138,6 +136,12 @@ static void parport_attach(struct parport *port)
>>>>>  		.dev		= NULL
>>>>>  	};
>>>>>  
>>>>> +	if (clear_wait > CLEAR_WAIT_MAX) {
>>>>> +		pr_err("clear_wait value should be not greater then %d\n",
>>>>> +		       CLEAR_WAIT_MAX);
>>>>> +		return;
>>>>> +	}
>>>>> +
>>>>
>>>> Why do you need to do so? Maybe a comment would be welcomed.
>>>
>>> It's in original code, I just moved it to ->probe().
>>>
>>> What comment do you want to have here, because original code has no comment (I
>>> think in any case it's out of scope of this change, but may be prepended or
>>> appended to the series)?
>>
>> Mmm... these functions can be called at different times, so I don't know if we
>> can just move the code safely.
> 
> I do not see any issue here. TL;DR: it won't be worse, but might even give an
> improvement.
> 
> Before it prevented to module to be initialized,
> now one may amend this at run time. the downside is that now it will require
> module removal and inserting versus just two attempts of inserting in a row.
> 
> For the built-in case it shouldn't change much (but if
> /sys/module/.../parameters/... is writable for this, then it will allow to do
> the similar trick as above, so extending functionality with the flexibility,
> means direct improvement).
> 
> Okay, permissions are 0 there, I don't remember what it means, maybe the
> parameter won't be available under /sysfs at all, but again, it won't change
> the functional behaviour, the downside is the memory consumed by the 'built-in'
> code at run time.

OK, I see. If so it's OK for me:

Acked-by: Rodolfo Giometti <giometti@enneenne.com>

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1 1/1] pps: clients: parport: Switch to use module_parport_driver()
  2021-05-11  8:46         ` Rodolfo Giometti
@ 2021-05-11 14:11           ` Andy Shevchenko
  0 siblings, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2021-05-11 14:11 UTC (permalink / raw)
  To: Rodolfo Giometti; +Cc: linux-kernel, gregkh, Alexander Gordeev

On Tue, May 11, 2021 at 10:46:01AM +0200, Rodolfo Giometti wrote:
> On 11/05/21 09:50, Andy Shevchenko wrote:
> > On Tue, May 11, 2021 at 09:26:36AM +0200, Rodolfo Giometti wrote:
> >> On 11/05/21 09:18, Andy Shevchenko wrote:
> >>> On Tue, May 11, 2021 at 09:05:00AM +0200, Rodolfo Giometti wrote:
> >>>> On 10/05/21 16:13, Andy Shevchenko wrote:

...

> >>>>> +	if (clear_wait > CLEAR_WAIT_MAX) {
> >>>>> +		pr_err("clear_wait value should be not greater then %d\n",
> >>>>> +		       CLEAR_WAIT_MAX);
> >>>>> +		return;
> >>>>> +	}
> >>>>> +
> >>>>
> >>>> Why do you need to do so? Maybe a comment would be welcomed.
> >>>
> >>> It's in original code, I just moved it to ->probe().
> >>>
> >>> What comment do you want to have here, because original code has no comment (I
> >>> think in any case it's out of scope of this change, but may be prepended or
> >>> appended to the series)?
> >>
> >> Mmm... these functions can be called at different times, so I don't know if we
> >> can just move the code safely.
> > 
> > I do not see any issue here. TL;DR: it won't be worse, but might even give an
> > improvement.
> > 
> > Before it prevented to module to be initialized,
> > now one may amend this at run time. the downside is that now it will require
> > module removal and inserting versus just two attempts of inserting in a row.
> > 
> > For the built-in case it shouldn't change much (but if
> > /sys/module/.../parameters/... is writable for this, then it will allow to do
> > the similar trick as above, so extending functionality with the flexibility,
> > means direct improvement).
> > 
> > Okay, permissions are 0 there, I don't remember what it means, maybe the
> > parameter won't be available under /sysfs at all, but again, it won't change
> > the functional behaviour, the downside is the memory consumed by the 'built-in'
> > code at run time.
> 
> OK, I see. If so

At least this is my understanding how it works before and after the change.
If anybody has something to clarify here, I would be glad to learn!

> it's OK for me:
> 
> Acked-by: Rodolfo Giometti <giometti@enneenne.com>

Thanks!

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-05-11 14:11 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-10 14:13 [PATCH v1 1/1] pps: clients: parport: Switch to use module_parport_driver() Andy Shevchenko
2021-05-11  7:05 ` Rodolfo Giometti
2021-05-11  7:18   ` Andy Shevchenko
2021-05-11  7:26     ` Rodolfo Giometti
2021-05-11  7:50       ` Andy Shevchenko
2021-05-11  8:46         ` Rodolfo Giometti
2021-05-11 14:11           ` Andy Shevchenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.