Date:	Fri, 31 Jul 2015 09:54:27 -0700
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	Sudip Mukherjee <sudipm.mukherjee@...il.com>
Cc:	linux-kernel@...r.kernel.org, linux-input@...r.kernel.org
Subject: Re: [PATCH] Input: drivers/joystick: use parallel port device model

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@...torindia.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?

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists