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@xxxxxxxxxxxxxxx>
> > > > ---
> > > >
> > > > 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.
regards
sudip
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
