Re: [PATCH] Input: drivers/joystick: use parallel port device model

[Sudip Mukherjee <sudipm.mukherjee@gmail.com>]

On Thu, Jul 30, 2015 at 09:53:23AM -0700, Dmitry Torokhov wrote:
> Hi Sudip,
Hi Dmitry,
> 
> On Thu, Jul 30, 2015 at 07:36:34PM +0530, Sudip Mukherjee wrote:
> > Modify db9 driver to use the new Parallel Port device model.
> > 
> > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > ---
> > 
> > It will generate a checkpatch warning about long line but I have not
> > changed it as it was only 2 char more and for 2 char it is more readable
> > now.
> 
> You can also write it as
> 
> 	if (!have_dev)
> 		return -ENODEV;
> 
> 	return parport_register_driver(...);
sure.
> 
> 
> Could you please tell me how you tested this change? With these devices
> becoming exceedingly rare just compile-tested changes may introduce
> regressions that are not easily noticed.
I dont have the device. It was just tested with module loading,
unloading, bind and unbind and checking that it is getting attached
properly. Also checked that with lp loaded, this driver fails to load.
Since the modification is only in the init, exit and probe
section so that should not affect the working of the driver. After the
driver loads and gets access to the parport and binds to it then these
sections have done their part. After that the db9_open and other old
functions will be responsible and since I have not touched those
functions so theoretically there should not be any regressions.
> 
<snip>
> > 
> > +
> > +	for (i = 0; i < DB9_MAX_PORTS; i++) {
> > +		if (db9_cfg[i].nargs == 0 ||
> > +		    db9_cfg[i].args[DB9_ARG_PARPORT] < 0)
> > +			continue;
> > +
> > +		if (db9_cfg[i].nargs < 2) {
> > +			pr_warn("db9.c: Device type must be specified.\n");
> > +			return;
> > +		}
> > +
> > +		if (db9_cfg[i].args[DB9_ARG_PARPORT] == pp->number)
> > +			break;
> > +	}
> > +
> > +	if (i == DB9_MAX_PORTS) {
> > +		pr_debug("Not using parport%d.\n", pp->number);
> > +		return;
> > +	}
> 
> Why do we validate module options in attach() instead of how we did this
> in module init function? By doing this here we losing the ability to
> abort module loading when parameters are invalid.
It is there in the module_init. The same check was added here also.
we can remove the check for db9_cfg[i].nargs < 2 from here.
But the other one will be required to check for the port to which we
need to register.
> 
> > +
<snip>
> >  
> > -	parport_put_port(pp);
> > -	return db9;
> > +	db9_base[i] = db9;
> 
> Instead of using static array maybe store db9 in pardevice's private
> pointer? Given that we are using parport exclusively on detach we can
> just get the first pardevice and get db9 from it.
Well, yes... Actually I wanted to do this with the minimum possible code
change so that any chance of regression can be avoided. This should not
have any problem, but I am a bit hesitant as this can not be tested on
real hardware. If you confirm then I will make it this way in v2.
By any chance, do you have the hardware?

regards
sudip