Re: [PATCH v2 1/1] i2c: parport: Switch to use module_parport_driver()

[Andy Shevchenko]

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@xxxxxxx>

Thanks!

> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxx>");
> >  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