| Message ID | 20210712141119.22426-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Accepted |
| Commit | 9c5b1daa3b24f13b7c517ba616c7a49abd7ab843 |
| Headers | show |
| Series | [v2,1/1] i2c: parport: Switch to use module_parport_driver() | expand |
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
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
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
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!
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);
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(-)