linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/1] i2c: parport: Switch to use module_parport_driver()
@ 2021-07-12 14:11 Andy Shevchenko
  2021-07-21  8:21 ` Jean Delvare
  2021-08-10 21:08 ` Wolfram Sang
  0 siblings, 2 replies; 5+ messages in thread
From: Andy Shevchenko @ 2021-07-12 14:11 UTC (permalink / raw)
  To: Andy Shevchenko, linux-i2c, linux-kernel; +Cc: Jean Delvare

Switch to use module_parport_driver() to reduce boilerplate code.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: fixed compilation error (Jean, LKP)
 drivers/i2c/busses/i2c-parport.c | 36 ++++++++++----------------------
 1 file changed, 11 insertions(+), 25 deletions(-)

diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c
index a535889acca6..231145c48728 100644
--- a/drivers/i2c/busses/i2c-parport.c
+++ b/drivers/i2c/busses/i2c-parport.c
@@ -267,6 +267,16 @@ static void i2c_parport_attach(struct parport *port)
 	int i;
 	struct pardev_cb i2c_parport_cb;
 
+	if (type < 0) {
+		pr_warn("adapter type unspecified\n");
+		return;
+	}
+
+	if (type >= ARRAY_SIZE(adapter_parm)) {
+		pr_warn("invalid type (%d)\n", type);
+		return;
+	}
+
 	for (i = 0; i < MAX_DEVICE; i++) {
 		if (parport[i] == -1)
 			continue;
@@ -392,32 +402,8 @@ static struct parport_driver i2c_parport_driver = {
 	.detach = i2c_parport_detach,
 	.devmodel = true,
 };
-
-/* ----- Module loading, unloading and information ------------------------ */
-
-static int __init i2c_parport_init(void)
-{
-	if (type < 0) {
-		pr_warn("adapter type unspecified\n");
-		return -ENODEV;
-	}
-
-	if (type >= ARRAY_SIZE(adapter_parm)) {
-		pr_warn("invalid type (%d)\n", type);
-		return -ENODEV;
-	}
-
-	return parport_register_driver(&i2c_parport_driver);
-}
-
-static void __exit i2c_parport_exit(void)
-{
-	parport_unregister_driver(&i2c_parport_driver);
-}
+module_parport_driver(i2c_parport_driver);
 
 MODULE_AUTHOR("Jean Delvare <jdelvare@suse.de>");
 MODULE_DESCRIPTION("I2C bus over parallel port");
 MODULE_LICENSE("GPL");
-
-module_init(i2c_parport_init);
-module_exit(i2c_parport_exit);
-- 
2.30.2


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

* Re: [PATCH v2 1/1] i2c: parport: Switch to use module_parport_driver()
  2021-07-12 14:11 [PATCH v2 1/1] i2c: parport: Switch to use module_parport_driver() Andy Shevchenko
@ 2021-07-21  8:21 ` Jean Delvare
  2021-07-21  8:37   ` Andy Shevchenko
  2021-08-10 21:08 ` Wolfram Sang
  1 sibling, 1 reply; 5+ messages in thread
From: Jean Delvare @ 2021-07-21  8:21 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-i2c, linux-kernel

Hi Andy,

On Mon, 12 Jul 2021 17:11:19 +0300, Andy Shevchenko wrote:
> Switch to use module_parport_driver() to reduce boilerplate code.

This has the downside of moving the sanity check of the type parameter
to run time, instead of driver load time. In particular this means that
loading the i2c-parport driver without specifying the type will no
longer fail. The driver will load successfully, but won't do anything.

While I prefer user errors to be reported as soon as possible, I don't
really mind here, as parallel port drivers are not something worth
debating over at this point in time. As a matter of fact, I can't
possibly test this change as I no longer have a parallel port on any of
my systems.

So if that's the direction we want to take then so be it.

Reviewed-by: Jean Delvare <jdelvare@suse.de>

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: fixed compilation error (Jean, LKP)
>  drivers/i2c/busses/i2c-parport.c | 36 ++++++++++----------------------
>  1 file changed, 11 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c
> index a535889acca6..231145c48728 100644
> --- a/drivers/i2c/busses/i2c-parport.c
> +++ b/drivers/i2c/busses/i2c-parport.c
> @@ -267,6 +267,16 @@ static void i2c_parport_attach(struct parport *port)
>  	int i;
>  	struct pardev_cb i2c_parport_cb;
>  
> +	if (type < 0) {
> +		pr_warn("adapter type unspecified\n");
> +		return;
> +	}
> +
> +	if (type >= ARRAY_SIZE(adapter_parm)) {
> +		pr_warn("invalid type (%d)\n", type);
> +		return;
> +	}
> +
>  	for (i = 0; i < MAX_DEVICE; i++) {
>  		if (parport[i] == -1)
>  			continue;
> @@ -392,32 +402,8 @@ static struct parport_driver i2c_parport_driver = {
>  	.detach = i2c_parport_detach,
>  	.devmodel = true,
>  };
> -
> -/* ----- Module loading, unloading and information ------------------------ */
> -
> -static int __init i2c_parport_init(void)
> -{
> -	if (type < 0) {
> -		pr_warn("adapter type unspecified\n");
> -		return -ENODEV;
> -	}
> -
> -	if (type >= ARRAY_SIZE(adapter_parm)) {
> -		pr_warn("invalid type (%d)\n", type);
> -		return -ENODEV;
> -	}
> -
> -	return parport_register_driver(&i2c_parport_driver);
> -}
> -
> -static void __exit i2c_parport_exit(void)
> -{
> -	parport_unregister_driver(&i2c_parport_driver);
> -}
> +module_parport_driver(i2c_parport_driver);
>  
>  MODULE_AUTHOR("Jean Delvare <jdelvare@suse.de>");
>  MODULE_DESCRIPTION("I2C bus over parallel port");
>  MODULE_LICENSE("GPL");
> -
> -module_init(i2c_parport_init);
> -module_exit(i2c_parport_exit);


-- 
Jean Delvare
SUSE L3 Support

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

* Re: [PATCH v2 1/1] i2c: parport: Switch to use module_parport_driver()
  2021-07-21  8:21 ` Jean Delvare
@ 2021-07-21  8:37   ` Andy Shevchenko
  2021-07-21  8:42     ` Andy Shevchenko
  0 siblings, 1 reply; 5+ messages in thread
From: Andy Shevchenko @ 2021-07-21  8:37 UTC (permalink / raw)
  To: Jean Delvare; +Cc: linux-i2c, linux-kernel

On Wed, Jul 21, 2021 at 10:21:46AM +0200, Jean Delvare wrote:
> Hi Andy,
> 
> On Mon, 12 Jul 2021 17:11:19 +0300, Andy Shevchenko wrote:
> > Switch to use module_parport_driver() to reduce boilerplate code.
> 
> This has the downside of moving the sanity check of the type parameter
> to run time, instead of driver load time. In particular this means that
> loading the i2c-parport driver without specifying the type will no
> longer fail.

And this is actually an advantage of the change if you think about it.
Now we can have a module that won't fail at boot time and give the user
a chance to amend parameter at run time if it was wrong at boot time.

> The driver will load successfully, but won't do anything.
> 
> While I prefer user errors to be reported as soon as possible, I don't
> really mind here, as parallel port drivers are not something worth
> debating over at this point in time. As a matter of fact, I can't
> possibly test this change as I no longer have a parallel port on any of
> my systems.
> 
> So if that's the direction we want to take then so be it.
> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>

Thanks!

> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> > v2: fixed compilation error (Jean, LKP)
> >  drivers/i2c/busses/i2c-parport.c | 36 ++++++++++----------------------
> >  1 file changed, 11 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c
> > index a535889acca6..231145c48728 100644
> > --- a/drivers/i2c/busses/i2c-parport.c
> > +++ b/drivers/i2c/busses/i2c-parport.c
> > @@ -267,6 +267,16 @@ static void i2c_parport_attach(struct parport *port)
> >  	int i;
> >  	struct pardev_cb i2c_parport_cb;
> >  
> > +	if (type < 0) {
> > +		pr_warn("adapter type unspecified\n");
> > +		return;
> > +	}
> > +
> > +	if (type >= ARRAY_SIZE(adapter_parm)) {
> > +		pr_warn("invalid type (%d)\n", type);
> > +		return;
> > +	}
> > +
> >  	for (i = 0; i < MAX_DEVICE; i++) {
> >  		if (parport[i] == -1)
> >  			continue;
> > @@ -392,32 +402,8 @@ static struct parport_driver i2c_parport_driver = {
> >  	.detach = i2c_parport_detach,
> >  	.devmodel = true,
> >  };
> > -
> > -/* ----- Module loading, unloading and information ------------------------ */
> > -
> > -static int __init i2c_parport_init(void)
> > -{
> > -	if (type < 0) {
> > -		pr_warn("adapter type unspecified\n");
> > -		return -ENODEV;
> > -	}
> > -
> > -	if (type >= ARRAY_SIZE(adapter_parm)) {
> > -		pr_warn("invalid type (%d)\n", type);
> > -		return -ENODEV;
> > -	}
> > -
> > -	return parport_register_driver(&i2c_parport_driver);
> > -}
> > -
> > -static void __exit i2c_parport_exit(void)
> > -{
> > -	parport_unregister_driver(&i2c_parport_driver);
> > -}
> > +module_parport_driver(i2c_parport_driver);
> >  
> >  MODULE_AUTHOR("Jean Delvare <jdelvare@suse.de>");
> >  MODULE_DESCRIPTION("I2C bus over parallel port");
> >  MODULE_LICENSE("GPL");
> > -
> > -module_init(i2c_parport_init);
> > -module_exit(i2c_parport_exit);
> 
> 
> -- 
> Jean Delvare
> SUSE L3 Support

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH v2 1/1] i2c: parport: Switch to use module_parport_driver()
  2021-07-21  8:37   ` Andy Shevchenko
@ 2021-07-21  8:42     ` Andy Shevchenko
  0 siblings, 0 replies; 5+ messages in thread
From: Andy Shevchenko @ 2021-07-21  8:42 UTC (permalink / raw)
  To: Jean Delvare; +Cc: linux-i2c, linux-kernel

On Wed, Jul 21, 2021 at 11:37:45AM +0300, Andy Shevchenko wrote:
> On Wed, Jul 21, 2021 at 10:21:46AM +0200, Jean Delvare wrote:
> > On Mon, 12 Jul 2021 17:11:19 +0300, Andy Shevchenko wrote:
> > > Switch to use module_parport_driver() to reduce boilerplate code.
> > 
> > This has the downside of moving the sanity check of the type parameter
> > to run time, instead of driver load time. In particular this means that
> > loading the i2c-parport driver without specifying the type will no
> > longer fail.
> 
> And this is actually an advantage of the change if you think about it.
> Now we can have a module that won't fail at boot time and give the user
> a chance to amend parameter at run time if it was wrong at boot time.

Okay, for built-in driver it might require to check and fix permissions
of the parameter sysfs node (currently 0).

> > The driver will load successfully, but won't do anything.
> > 
> > While I prefer user errors to be reported as soon as possible, I don't
> > really mind here, as parallel port drivers are not something worth
> > debating over at this point in time. As a matter of fact, I can't
> > possibly test this change as I no longer have a parallel port on any of
> > my systems.

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH v2 1/1] i2c: parport: Switch to use module_parport_driver()
  2021-07-12 14:11 [PATCH v2 1/1] i2c: parport: Switch to use module_parport_driver() Andy Shevchenko
  2021-07-21  8:21 ` Jean Delvare
@ 2021-08-10 21:08 ` Wolfram Sang
  1 sibling, 0 replies; 5+ messages in thread
From: Wolfram Sang @ 2021-08-10 21:08 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-i2c, linux-kernel, Jean Delvare

[-- Attachment #1: Type: text/plain, Size: 243 bytes --]

On Mon, Jul 12, 2021 at 05:11:19PM +0300, Andy Shevchenko wrote:
> Switch to use module_parport_driver() to reduce boilerplate code.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

end of thread, other threads:[~2021-08-10 21:08 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-12 14:11 [PATCH v2 1/1] i2c: parport: Switch to use module_parport_driver() Andy Shevchenko
2021-07-21  8:21 ` Jean Delvare
2021-07-21  8:37   ` Andy Shevchenko
2021-07-21  8:42     ` Andy Shevchenko
2021-08-10 21:08 ` Wolfram Sang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).