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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Fri, Jul 31, 2015 at 01:36:18PM -0700, Greg KH wrote:
> On Fri, Jul 31, 2015 at 11:04:26PM +0530, Sudip Mukherjee wrote:
> > On Fri, Jul 31, 2015 at 09:54:27AM -0700, Dmitry Torokhov wrote:
> > > On Fri, Jul 31, 2015 at 03:45:25PM +0530, Sudip Mukherjee wrote:
> > > > 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?
> > > 
> > > No, unfortunately I do not.
> > > 
> > > Since neither of us can test the change what is the benefit of doing the
> > > conversion? What will be gained by doing it? Are there plans for parport
> > > subsystem to remove the old style initialization?
> > Yes, that is the plan. Well, if you are not comfortable with introducing
> > attach and detach functions then this can be done in another way where
> > there will be very minimum change in the code. But I will prefer to have
> > attach and detach then it can take advantage of the hotplug feature.
> > Adding Greg in To: list for his comments.
> 
> Converting to the "new" api is the end goal here, no need to keep the
> old one around anymore.

OK, then I guess we can do the conversion right (dropping db9_base
module-global) and see if anyone screams at us.

Thanks.

-- 
Dmitry