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:

quoted

Hi Sudip,

Hi Dmitry,

quoted

On Thu, Jul 30, 2015 at 07:36:34PM +0530, Sudip Mukherjee wrote:

quoted

Modify db9 driver to use the new Parallel Port device model.

Signed-off-by: Sudip Mukherjee <redacted>
---

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.

quoted

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.

quoted

<snip>

quoted

quoted

+
+	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.

quoted

quoted

+

<snip>

quoted

quoted

 
-	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