[PATCH v2 WIP 1/2] parport: add device-model to parport subsystem

[PATCH v2 WIP 1/2] parport: add device-model to parport subsystem

[Sudip Mukherjee]

This is again another WIP for your review.
almost all the points raised by Greg and Dan has been covered in this
patch.

apart from those points, I found another problem with my previous code,
that whenever any driver is trying to register, it is getting bound to
all the available parallel ports. To solve that probe was required and
it has been used in this version. Now the driver is binding only to the
device it has registered. And that device will appear as a subdevice
of the particular parallel port it wants to use.

In the previous version Greg mentioned that we do not need to maintain
our own list now. That has been partially done in this version, we are
no longet maintaining the list of drivers. But we still need to maintain
the list of ports and devices as that will be used by drivers which are
not yet converted to device model. When all drivers are converted to
use the device-model parallel port all these lists can be removed.

Just a little off-topic: I am using the staging/panel to test this code,
and last few days i had a very tough time finding a stackdump. I was
always thinking that it is happening because of the changes I have done.
But it turned out to be a bug in the panel code. And incidentally, with
the changes done in this version that bug is also resolved.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/parport/procfs.c |  15 ++-
 drivers/parport/share.c  | 243 ++++++++++++++++++++++++++++++++++++++++++++---
 include/linux/parport.h  |  41 +++++++-
 3 files changed, 283 insertions(+), 16 deletions(-)

diff --git a/drivers/parport/procfs.c b/drivers/parport/procfs.c
index 3b47080..c776333 100644
--- a/drivers/parport/procfs.c
+++ b/drivers/parport/procfs.c
@@ -21,6 +21,7 @@
 #include <linux/parport.h>
 #include <linux/ctype.h>
 #include <linux/sysctl.h>
+#include <linux/device.h>
 
 #include <asm/uaccess.h>
 
@@ -558,8 +559,18 @@ int parport_device_proc_unregister(struct pardevice *device)
 
 static int __init parport_default_proc_register(void)
 {
+	int ret;
+
 	parport_default_sysctl_table.sysctl_header =
 		register_sysctl_table(parport_default_sysctl_table.dev_dir);
+	if (!parport_default_sysctl_table.sysctl_header)
+		return -ENOMEM;
+	ret = parport_bus_init();
+	if (ret) {
+		unregister_sysctl_table(parport_default_sysctl_table.
+					sysctl_header);
+		return ret;
+	}
 	return 0;
 }
 
@@ -570,6 +581,7 @@ static void __exit parport_default_proc_unregister(void)
 					sysctl_header);
 		parport_default_sysctl_table.sysctl_header = NULL;
 	}
+	parport_bus_exit();
 }
 
 #else /* no sysctl or no procfs*/
@@ -596,11 +608,12 @@ int parport_device_proc_unregister(struct pardevice *device)
 
 static int __init parport_default_proc_register (void)
 {
-	return 0;
+	return parport_bus_init();
 }
 
 static void __exit parport_default_proc_unregister (void)
 {
+	parport_bus_exit();
 }
 #endif
 
diff --git a/drivers/parport/share.c b/drivers/parport/share.c
index 3fa6624..8a28c54 100644
--- a/drivers/parport/share.c
+++ b/drivers/parport/share.c
@@ -29,6 +29,7 @@
 #include <linux/slab.h>
 #include <linux/sched.h>
 #include <linux/kmod.h>
+#include <linux/device.h>
 
 #include <linux/spinlock.h>
 #include <linux/mutex.h>
@@ -100,6 +101,20 @@ static struct parport_operations dead_ops = {
 	.owner		= NULL,
 };
 
+struct bus_type parport_bus_type = {
+	.name           = "parport",
+};
+
+int parport_bus_init(void)
+{
+	return bus_register(&parport_bus_type);
+}
+
+void parport_bus_exit(void)
+{
+	bus_unregister(&parport_bus_type);
+}
+
 /* Call attach(port) for each registered driver. */
 static void attach_driver_chain(struct parport *port)
 {
@@ -157,6 +172,7 @@ int parport_register_driver (struct parport_driver *drv)
 
 	if (list_empty(&portlist))
 		get_lowlevel_driver ();
+	drv->devmodel = false;
 
 	mutex_lock(&registration_lock);
 	list_for_each_entry(port, &portlist, list)
@@ -167,6 +183,61 @@ int parport_register_driver (struct parport_driver *drv)
 	return 0;
 }
 
+/*
+ * iterates through all the devices connected to the bus and sends the device
+ * details to the check callback of the driver, so that the driver can know
+ * what are all the ports that are connected to the bus and choose the port to
+ * which it wants to register its device.
+ */
+static int port_check(struct device *dev, void *drv)
+{
+	((struct parport_driver *)drv)->check(to_parport_dev(dev));
+	return 0;
+}
+
+/*
+ * __parport_register_drv - register a new parport driver
+ * @drv: the driver structure to register
+ * @owner: owner module of drv
+ * @mod_name: module name string
+ *
+ * Adds the driver structure to the list of registered drivers.
+ * Returns a negative value on error, otherwise 0.
+ * If no error occurred, the driver remains registered even if
+ * no device was claimed during registration.
+ */
+int __parport_register_drv(struct parport_driver *drv,
+			   struct module *owner, const char *mod_name)
+{
+	int ret;
+
+	if (!drv->probe) {
+		pr_err("please use probe in %s\n", drv->name);
+		return -ENODEV;
+	}
+
+	if (list_empty(&portlist))
+		get_lowlevel_driver();
+
+	/* initialize common driver fields */
+	drv->driver.name = drv->name;
+	drv->driver.bus = &parport_bus_type;
+	drv->driver.owner = owner;
+	drv->driver.mod_name = mod_name;
+	drv->driver.probe = drv->probe;
+	drv->devmodel = true;
+	ret = driver_register(&drv->driver);
+	if (ret)
+		return ret;
+
+	mutex_lock(&registration_lock);
+	bus_find_device(&parport_bus_type, NULL, drv, port_check);
+	mutex_unlock(&registration_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(__parport_register_drv);
+
 /**
  *	parport_unregister_driver - deregister a parallel port device driver
  *	@drv: structure describing the driver that was given to
@@ -189,15 +260,21 @@ void parport_unregister_driver (struct parport_driver *drv)
 	struct parport *port;
 
 	mutex_lock(&registration_lock);
-	list_del_init(&drv->list);
-	list_for_each_entry(port, &portlist, list)
-		drv->detach(port);
+	if (drv->devmodel) {
+		driver_unregister(&drv->driver);
+	} else {
+		list_del_init(&drv->list);
+		list_for_each_entry(port, &portlist, list)
+			drv->detach(port);
+	}
 	mutex_unlock(&registration_lock);
 }
 
-static void free_port (struct parport *port)
+static void free_port(struct device *dev)
 {
 	int d;
+	struct parport *port = to_parport_dev(dev);
+
 	spin_lock(&full_list_lock);
 	list_del(&port->full_list);
 	spin_unlock(&full_list_lock);
@@ -223,8 +300,9 @@ static void free_port (struct parport *port)
 
 struct parport *parport_get_port (struct parport *port)
 {
-	atomic_inc (&port->ref_count);
-	return port;
+	struct device *dev = get_device(&port->ddev);
+
+	return to_parport_dev(dev);
 }
 
 /**
@@ -237,11 +315,7 @@ struct parport *parport_get_port (struct parport *port)
 
 void parport_put_port (struct parport *port)
 {
-	if (atomic_dec_and_test (&port->ref_count))
-		/* Can destroy it now. */
-		free_port (port);
-
-	return;
+	put_device(&port->ddev);
 }
 
 /**
@@ -281,6 +355,7 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma,
 	int num;
 	int device;
 	char *name;
+	int ret;
 
 	tmp = kzalloc(sizeof(struct parport), GFP_KERNEL);
 	if (!tmp) {
@@ -333,6 +408,9 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma,
 	 */
 	sprintf(name, "parport%d", tmp->portnum = tmp->number);
 	tmp->name = name;
+	tmp->ddev.bus = &parport_bus_type;
+	tmp->ddev.release = free_port;
+	dev_set_name(&tmp->ddev, name);
 
 	for (device = 0; device < 5; device++)
 		/* assume the worst */
@@ -340,6 +418,12 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma,
 
 	tmp->waithead = tmp->waittail = NULL;
 
+	ret = device_register(&tmp->ddev);
+	if (ret) {
+		put_device(&tmp->ddev);
+		return NULL;
+	}
+
 	return tmp;
 }
 
@@ -575,6 +659,7 @@ parport_register_device(struct parport *port, const char *name,
 	tmp->irq_func = irq_func;
 	tmp->waiting = 0;
 	tmp->timeout = 5 * HZ;
+	tmp->devmodel = false;
 
 	/* Chain this onto the list */
 	tmp->prev = NULL;
@@ -630,6 +715,133 @@ parport_register_device(struct parport *port, const char *name,
 	return NULL;
 }
 
+static void free_pardevice(struct device *dev)
+{
+	struct pardevice *par_dev = to_pardevice(dev);
+
+	kfree(par_dev->name);
+	kfree(par_dev);
+}
+
+struct pardevice *
+parport_register_dev(struct parport *port, const char *name,
+		     struct pardev_cb *par_dev_cb)
+{
+	struct pardevice *par_dev;
+	int ret;
+	char *devname;
+
+	if (port->physport->flags & PARPORT_FLAG_EXCL) {
+		/* An exclusive device is registered. */
+		pr_debug("%s: no more devices allowed\n",
+			 port->name);
+		return NULL;
+	}
+
+	if (par_dev_cb->flags & PARPORT_DEV_LURK) {
+		if (!par_dev_cb->preempt || !par_dev_cb->wakeup) {
+			pr_info("%s: refused to register lurking device (%s) without callbacks\n",
+				port->name, name);
+			return NULL;
+		}
+	}
+
+	if (!try_module_get(port->ops->owner))
+		return NULL;
+
+	parport_get_port(port);
+
+	par_dev = kzalloc(sizeof(*par_dev), GFP_KERNEL);
+	if (!par_dev)
+		goto err_par_dev;
+
+	par_dev->state = kzalloc(sizeof(*par_dev->state), GFP_KERNEL);
+	if (!par_dev->state)
+		goto err_put_par_dev;
+
+	devname = kstrdup(name, GFP_KERNEL);
+	if (!devname)
+		goto err_free_par_dev;
+
+	par_dev->name = devname;
+	par_dev->port = port;
+	par_dev->daisy = -1;
+	par_dev->preempt = par_dev_cb->preempt;
+	par_dev->wakeup = par_dev_cb->wakeup;
+	par_dev->private = par_dev_cb->private;
+	par_dev->flags = par_dev_cb->flags;
+	par_dev->irq_func = par_dev_cb->irq_func;
+	par_dev->waiting = 0;
+	par_dev->timeout = 5 * HZ;
+
+	par_dev->dev.parent = &port->ddev;
+	par_dev->dev.bus = &parport_bus_type;
+	dev_set_name(&par_dev->dev, "%s", devname);
+	par_dev->dev.release = free_pardevice;
+	par_dev->devmodel = true;
+	ret = device_register(&par_dev->dev);
+	if (ret)
+		goto err_free_all;
+
+	/* Chain this onto the list */
+	par_dev->prev = NULL;
+	/*
+	 * This function must not run from an irq handler so we don' t need
+	 * to clear irq on the local CPU. -arca
+	 */
+	spin_lock(&port->physport->pardevice_lock);
+
+	if (par_dev_cb->flags & PARPORT_DEV_EXCL) {
+		if (port->physport->devices) {
+			spin_unlock(&port->physport->pardevice_lock);
+			pr_debug("%s: cannot grant exclusive access for device %s\n",
+				 port->name, name);
+			goto err_free_all;
+		}
+		port->flags |= PARPORT_FLAG_EXCL;
+	}
+
+	par_dev->next = port->physport->devices;
+	wmb();	/*
+		 * Make sure that tmp->next is written before it's
+		 * added to the list; see comments marked 'no locking
+		 * required'
+		 */
+	if (port->physport->devices)
+		port->physport->devices->prev = par_dev;
+	port->physport->devices = par_dev;
+	spin_unlock(&port->physport->pardevice_lock);
+
+	init_waitqueue_head(&par_dev->wait_q);
+	par_dev->timeslice = parport_default_timeslice;
+	par_dev->waitnext = NULL;
+	par_dev->waitprev = NULL;
+
+	/*
+	 * This has to be run as last thing since init_state may need other
+	 * pardevice fields. -arca
+	 */
+	port->ops->init_state(par_dev, par_dev->state);
+	port->proc_device = par_dev;
+	parport_device_proc_register(par_dev);
+
+	return par_dev;
+
+err_free_all:
+	put_device(&par_dev->dev);
+err_free_par_dev:
+	kfree(par_dev->state);
+err_put_par_dev:
+	if (!par_dev->devmodel)
+		kfree(par_dev);
+err_par_dev:
+	parport_put_port(port);
+	module_put(port->ops->owner);
+
+	return NULL;
+}
+EXPORT_SYMBOL(parport_register_dev);
+
 /**
  *	parport_unregister_device - deregister a device on a parallel port
  *	@dev: pointer to structure representing device
@@ -691,7 +903,10 @@ void parport_unregister_device(struct pardevice *dev)
 	spin_unlock_irq(&port->waitlist_lock);
 
 	kfree(dev->state);
-	kfree(dev);
+	if (dev->devmodel)
+		device_unregister(&dev->dev);
+	else
+		kfree(dev);
 
 	module_put(port->ops->owner);
 	parport_put_port (port);
@@ -926,8 +1141,8 @@ int parport_claim_or_block(struct pardevice *dev)
 			       dev->port->physport->cad ?
 			       dev->port->physport->cad->name:"nobody");
 #endif
-	}
-	dev->waiting = 0;
+	} else if (r == 0)
+		dev->waiting = 0;
 	return r;
 }
 
diff --git a/include/linux/parport.h b/include/linux/parport.h
index c22f125..83e1a6e 100644
--- a/include/linux/parport.h
+++ b/include/linux/parport.h
@@ -13,6 +13,7 @@
 #include <linux/wait.h>
 #include <linux/irqreturn.h>
 #include <linux/semaphore.h>
+#include <linux/device.h>
 #include <asm/ptrace.h>
 #include <uapi/linux/parport.h>
 
@@ -145,6 +146,8 @@ struct pardevice {
 	unsigned int flags;
 	struct pardevice *next;
 	struct pardevice *prev;
+	struct device dev;
+	bool devmodel;
 	struct parport_state *state;     /* saved status over preemption */
 	wait_queue_head_t wait_q;
 	unsigned long int time;
@@ -156,6 +159,8 @@ struct pardevice {
 	void * sysctl_table;
 };
 
+#define to_pardevice(n) container_of(n, struct pardevice, dev)
+
 /* IEEE1284 information */
 
 /* IEEE1284 phases. These are exposed to userland through ppdev IOCTL
@@ -195,7 +200,7 @@ struct parport {
 				 * This may unfortulately be null if the
 				 * port has a legacy driver.
 				 */
-
+	struct device ddev;	/* to link with the bus */
 	struct parport *physport;
 				/* If this is a non-default mux
 				   parport, i.e. we're a clone of a real
@@ -245,15 +250,26 @@ struct parport {
 	struct parport *slaves[3];
 };
 
+#define to_parport_dev(n) container_of(n, struct parport, ddev)
+
 #define DEFAULT_SPIN_TIME 500 /* us */
 
 struct parport_driver {
 	const char *name;
 	void (*attach) (struct parport *);
 	void (*detach) (struct parport *);
+	void (*check)(struct parport *);
+	int (*probe)(struct device *);
+	struct device_driver driver;
+	bool devmodel;
 	struct list_head list;
 };
 
+extern struct bus_type parport_bus_type;
+
+int parport_bus_init(void);
+void parport_bus_exit(void);
+
 /* parport_register_port registers a new parallel port at the given
    address (if one does not already exist) and returns a pointer to it.
    This entails claiming the I/O region, IRQ and DMA.  NULL is returned
@@ -274,8 +290,19 @@ extern void parport_remove_port(struct parport *port);
 /* Register a new high-level driver. */
 extern int parport_register_driver (struct parport_driver *);
 
+int __must_check __parport_register_drv(struct parport_driver *,
+					struct module *,
+					const char *mod_name);
+/*
+ * parport_register_drv must be a macro so that KBUILD_MODNAME can
+ * be expanded
+ */
+#define parport_register_drv(driver)             \
+	__parport_register_drv(driver, THIS_MODULE, KBUILD_MODNAME)
+
 /* Unregister a high-level driver. */
 extern void parport_unregister_driver (struct parport_driver *);
+void parport_unregister_driver(struct parport_driver *);
 
 /* If parport_register_driver doesn't fit your needs, perhaps
  * parport_find_xxx does. */
@@ -289,6 +316,14 @@ extern irqreturn_t parport_irq_handler(int irq, void *dev_id);
 extern struct parport *parport_get_port (struct parport *);
 extern void parport_put_port (struct parport *);
 
+struct pardev_cb {
+	int (*preempt)(void *);
+	void (*wakeup)(void *);
+	void *private;
+	void (*irq_func)(void *);
+	unsigned int flags;
+};
+
 /* parport_register_device declares that a device is connected to a
    port, and tells the kernel all it needs to know.
    - pf is the preemption function (may be NULL for no callback)
@@ -301,6 +336,10 @@ struct pardevice *parport_register_device(struct parport *port,
 			  void (*irq_func)(void *), 
 			  int flags, void *handle);
 
+struct pardevice *
+parport_register_dev(struct parport *port, const char *name,
+		     struct pardev_cb *par_dev_cb);
+
 /* parport_unregister unlinks a device from the chain. */
 extern void parport_unregister_device(struct pardevice *dev);
 
-- 
1.8.1.2

[PATCH v2 WIP 2/2] staging: panel: modify driver to use new parport device model

[Sudip Mukherjee]

modify panel driver to use the new parallel port device model.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/staging/panel/panel.c | 53 +++++++++++++++----------------------------
 1 file changed, 18 insertions(+), 35 deletions(-)

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index ea54fb4..e9511ee 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -2188,6 +2188,19 @@ static struct notifier_block panel_notifier = {
 	0
 };
 
+static int panel_probe(struct device *dev)
+{
+	if (strcmp(dev_name(dev), "panel"))
+		return -ENODEV;
+
+	return 0;
+}
+
+struct pardev_cb panel_cb = {
+	.flags = 0,	/*PARPORT_DEV_EXCL */
+	.private = &pprt,
+};
+
 static void panel_attach(struct parport *port)
 {
 	if (port->number != parport)
@@ -2199,10 +2212,7 @@ static void panel_attach(struct parport *port)
 		return;
 	}
 
-	pprt = parport_register_device(port, "panel", NULL, NULL,  /* pf, kf */
-				       NULL,
-				       /*PARPORT_DEV_EXCL */
-				       0, (void *)&pprt);
+	pprt = parport_register_dev(port, "panel", &panel_cb);
 	if (pprt == NULL) {
 		pr_err("%s: port->number=%d parport=%d, parport_register_device() failed\n",
 		       __func__, port->number, parport);
@@ -2240,38 +2250,10 @@ err_unreg_device:
 	pprt = NULL;
 }
 
-static void panel_detach(struct parport *port)
-{
-	if (port->number != parport)
-		return;
-
-	if (!pprt) {
-		pr_err("%s: port->number=%d parport=%d, nothing to unregister.\n",
-		       __func__, port->number, parport);
-		return;
-	}
-
-	unregister_reboot_notifier(&panel_notifier);
-
-	if (keypad.enabled && keypad_initialized) {
-		misc_deregister(&keypad_dev);
-		keypad_initialized = 0;
-	}
-
-	if (lcd.enabled && lcd.initialized) {
-		misc_deregister(&lcd_dev);
-		lcd.initialized = false;
-	}
-
-	parport_release(pprt);
-	parport_unregister_device(pprt);
-	pprt = NULL;
-}
-
 static struct parport_driver panel_driver = {
 	.name = "panel",
-	.attach = panel_attach,
-	.detach = panel_detach,
+	.check = panel_attach,
+	.probe = panel_probe,
 };
 
 /* init function */
@@ -2380,7 +2362,7 @@ static int __init panel_init_module(void)
 		return -ENODEV;
 	}
 
-	err = parport_register_driver(&panel_driver);
+	err = parport_register_drv(&panel_driver);
 	if (err) {
 		pr_err("could not register with parport. Aborting.\n");
 		return err;
@@ -2398,6 +2380,7 @@ static int __init panel_init_module(void)
 
 static void __exit panel_cleanup_module(void)
 {
+	unregister_reboot_notifier(&panel_notifier);
 
 	if (scan_timer.function != NULL)
 		del_timer_sync(&scan_timer);
-- 
1.8.1.2

Re: [PATCH v2 WIP 1/2] parport: add device-model to parport subsystem

[Dan Carpenter]

On Tue, Apr 21, 2015 at 07:22:34PM +0530, Sudip Mukherjee wrote:
> This is again another WIP for your review.
> almost all the points raised by Greg and Dan has been covered in this
> patch.
> 
> apart from those points, I found another problem with my previous code,
> that whenever any driver is trying to register, it is getting bound to
> all the available parallel ports. To solve that probe was required and
> it has been used in this version. Now the driver is binding only to the
> device it has registered. And that device will appear as a subdevice
> of the particular parallel port it wants to use.
> 
> In the previous version Greg mentioned that we do not need to maintain
> our own list now. That has been partially done in this version, we are
> no longet maintaining the list of drivers. But we still need to maintain
> the list of ports and devices as that will be used by drivers which are
> not yet converted to device model. When all drivers are converted to
> use the device-model parallel port all these lists can be removed.
> 
> Just a little off-topic: I am using the staging/panel to test this code,
> and last few days i had a very tough time finding a stackdump. I was
> always thinking that it is happening because of the changes I have done.
> But it turned out to be a bug in the panel code. And incidentally, with
> the changes done in this version that bug is also resolved.
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
>  drivers/parport/procfs.c |  15 ++-
>  drivers/parport/share.c  | 243 ++++++++++++++++++++++++++++++++++++++++++++---
>  include/linux/parport.h  |  41 +++++++-
>  3 files changed, 283 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/parport/procfs.c b/drivers/parport/procfs.c
> index 3b47080..c776333 100644
> --- a/drivers/parport/procfs.c
> +++ b/drivers/parport/procfs.c
> @@ -21,6 +21,7 @@
>  #include <linux/parport.h>
>  #include <linux/ctype.h>
>  #include <linux/sysctl.h>
> +#include <linux/device.h>
>  
>  #include <asm/uaccess.h>
>  
> @@ -558,8 +559,18 @@ int parport_device_proc_unregister(struct pardevice *device)
>  
>  static int __init parport_default_proc_register(void)
>  {
> +	int ret;
> +
>  	parport_default_sysctl_table.sysctl_header =
>  		register_sysctl_table(parport_default_sysctl_table.dev_dir);
> +	if (!parport_default_sysctl_table.sysctl_header)
> +		return -ENOMEM;
> +	ret = parport_bus_init();
> +	if (ret) {
> +		unregister_sysctl_table(parport_default_sysctl_table.
> +					sysctl_header);
> +		return ret;
> +	}
>  	return 0;
>  }
>  
> @@ -570,6 +581,7 @@ static void __exit parport_default_proc_unregister(void)
>  					sysctl_header);
>  		parport_default_sysctl_table.sysctl_header = NULL;
>  	}
> +	parport_bus_exit();
>  }
>  
>  #else /* no sysctl or no procfs*/
> @@ -596,11 +608,12 @@ int parport_device_proc_unregister(struct pardevice *device)
>  
>  static int __init parport_default_proc_register (void)
>  {
> -	return 0;
> +	return parport_bus_init();
>  }
>  
>  static void __exit parport_default_proc_unregister (void)
>  {
> +	parport_bus_exit();
>  }
>  #endif
>  
> diff --git a/drivers/parport/share.c b/drivers/parport/share.c
> index 3fa6624..8a28c54 100644
> --- a/drivers/parport/share.c
> +++ b/drivers/parport/share.c
> @@ -29,6 +29,7 @@
>  #include <linux/slab.h>
>  #include <linux/sched.h>
>  #include <linux/kmod.h>
> +#include <linux/device.h>
>  
>  #include <linux/spinlock.h>
>  #include <linux/mutex.h>
> @@ -100,6 +101,20 @@ static struct parport_operations dead_ops = {
>  	.owner		= NULL,
>  };
>  
> +struct bus_type parport_bus_type = {
> +	.name           = "parport",
> +};

Can you make this static?  The indenting is a bit off.

> +
> +int parport_bus_init(void)
> +{
> +	return bus_register(&parport_bus_type);
> +}
> +
> +void parport_bus_exit(void)
> +{
> +	bus_unregister(&parport_bus_type);
> +}
> +
>  /* Call attach(port) for each registered driver. */
>  static void attach_driver_chain(struct parport *port)
>  {
> @@ -157,6 +172,7 @@ int parport_register_driver (struct parport_driver *drv)
>  
>  	if (list_empty(&portlist))
>  		get_lowlevel_driver ();
> +	drv->devmodel = false;
>  
>  	mutex_lock(&registration_lock);
>  	list_for_each_entry(port, &portlist, list)
> @@ -167,6 +183,61 @@ int parport_register_driver (struct parport_driver *drv)
>  	return 0;
>  }
>  
> +/*
> + * iterates through all the devices connected to the bus and sends the device
> + * details to the check callback of the driver, so that the driver can know
> + * what are all the ports that are connected to the bus and choose the port to
> + * which it wants to register its device.
> + */
> +static int port_check(struct device *dev, void *drv)
> +{
> +	((struct parport_driver *)drv)->check(to_parport_dev(dev));
> +	return 0;
> +}

The cast isn't beautiful.  Do this:

static int port_check(struct device *dev, void *_drv)
{
	struct parport_driver *drv = _drv;

	drv->check(to_parport_dev(dev));
	return 0;
}

What is the point of the check function really?  The name isn't clear.

Since it always returns zero that means we loop through all the devices
and then returns NULL.  It feels like a function called
bus_find_device() should find something.  We have bus_for_each_dev() if
we just want to iterate.

> +
> +/*
> + * __parport_register_drv - register a new parport driver
> + * @drv: the driver structure to register
> + * @owner: owner module of drv
> + * @mod_name: module name string
> + *
> + * Adds the driver structure to the list of registered drivers.
> + * Returns a negative value on error, otherwise 0.
> + * If no error occurred, the driver remains registered even if
> + * no device was claimed during registration.
> + */
> +int __parport_register_drv(struct parport_driver *drv,
> +			   struct module *owner, const char *mod_name)
> +{
> +	int ret;
> +
> +	if (!drv->probe) {
> +		pr_err("please use probe in %s\n", drv->name);
> +		return -ENODEV;
> +	}
> +
> +	if (list_empty(&portlist))
> +		get_lowlevel_driver();
> +
> +	/* initialize common driver fields */
> +	drv->driver.name = drv->name;
> +	drv->driver.bus = &parport_bus_type;
> +	drv->driver.owner = owner;
> +	drv->driver.mod_name = mod_name;
> +	drv->driver.probe = drv->probe;
> +	drv->devmodel = true;
> +	ret = driver_register(&drv->driver);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&registration_lock);
> +	bus_find_device(&parport_bus_type, NULL, drv, port_check);
> +	mutex_unlock(&registration_lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(__parport_register_drv);
> +
>  /**
>   *	parport_unregister_driver - deregister a parallel port device driver
>   *	@drv: structure describing the driver that was given to
> @@ -189,15 +260,21 @@ void parport_unregister_driver (struct parport_driver *drv)
>  	struct parport *port;
>  
>  	mutex_lock(&registration_lock);
> -	list_del_init(&drv->list);
> -	list_for_each_entry(port, &portlist, list)
> -		drv->detach(port);
> +	if (drv->devmodel) {
> +		driver_unregister(&drv->driver);
> +	} else {
> +		list_del_init(&drv->list);
> +		list_for_each_entry(port, &portlist, list)
> +			drv->detach(port);
> +	}
>  	mutex_unlock(&registration_lock);
>  }
>  
> -static void free_port (struct parport *port)
> +static void free_port(struct device *dev)
>  {
>  	int d;
> +	struct parport *port = to_parport_dev(dev);
> +
>  	spin_lock(&full_list_lock);
>  	list_del(&port->full_list);
>  	spin_unlock(&full_list_lock);
> @@ -223,8 +300,9 @@ static void free_port (struct parport *port)
>  
>  struct parport *parport_get_port (struct parport *port)
>  {
> -	atomic_inc (&port->ref_count);
> -	return port;
> +	struct device *dev = get_device(&port->ddev);
> +
> +	return to_parport_dev(dev);
>  }
>  
>  /**
> @@ -237,11 +315,7 @@ struct parport *parport_get_port (struct parport *port)
>  
>  void parport_put_port (struct parport *port)
>  {
> -	if (atomic_dec_and_test (&port->ref_count))
> -		/* Can destroy it now. */
> -		free_port (port);
> -
> -	return;
> +	put_device(&port->ddev);
>  }
>  
>  /**
> @@ -281,6 +355,7 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma,
>  	int num;
>  	int device;
>  	char *name;
> +	int ret;
>  
>  	tmp = kzalloc(sizeof(struct parport), GFP_KERNEL);
>  	if (!tmp) {
> @@ -333,6 +408,9 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma,
>  	 */
>  	sprintf(name, "parport%d", tmp->portnum = tmp->number);
>  	tmp->name = name;
> +	tmp->ddev.bus = &parport_bus_type;
> +	tmp->ddev.release = free_port;
> +	dev_set_name(&tmp->ddev, name);
>  
>  	for (device = 0; device < 5; device++)
>  		/* assume the worst */
> @@ -340,6 +418,12 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma,
>  
>  	tmp->waithead = tmp->waittail = NULL;
>  
> +	ret = device_register(&tmp->ddev);
> +	if (ret) {
> +		put_device(&tmp->ddev);
> +		return NULL;
> +	}
> +
>  	return tmp;
>  }
>  
> @@ -575,6 +659,7 @@ parport_register_device(struct parport *port, const char *name,
>  	tmp->irq_func = irq_func;
>  	tmp->waiting = 0;
>  	tmp->timeout = 5 * HZ;
> +	tmp->devmodel = false;
>  
>  	/* Chain this onto the list */
>  	tmp->prev = NULL;
> @@ -630,6 +715,133 @@ parport_register_device(struct parport *port, const char *name,
>  	return NULL;
>  }
>  
> +static void free_pardevice(struct device *dev)
> +{
> +	struct pardevice *par_dev = to_pardevice(dev);
> +
> +	kfree(par_dev->name);
> +	kfree(par_dev);
> +}
> +
> +struct pardevice *
> +parport_register_dev(struct parport *port, const char *name,

Please choose a better name like parport_register_dev_model() or
something.

> +		     struct pardev_cb *par_dev_cb)
> +{
> +	struct pardevice *par_dev;
> +	int ret;
> +	char *devname;
> +
> +	if (port->physport->flags & PARPORT_FLAG_EXCL) {
> +		/* An exclusive device is registered. */
> +		pr_debug("%s: no more devices allowed\n",
> +			 port->name);

This should probably be a pr_err().

> +		return NULL;
> +	}
> +
> +	if (par_dev_cb->flags & PARPORT_DEV_LURK) {
> +		if (!par_dev_cb->preempt || !par_dev_cb->wakeup) {
> +			pr_info("%s: refused to register lurking device (%s) without callbacks\n",
> +				port->name, name);
> +			return NULL;
> +		}
> +	}
> +
> +	if (!try_module_get(port->ops->owner))
> +		return NULL;
> +
> +	parport_get_port(port);
> +
> +	par_dev = kzalloc(sizeof(*par_dev), GFP_KERNEL);
> +	if (!par_dev)
> +		goto err_par_dev;
> +
> +	par_dev->state = kzalloc(sizeof(*par_dev->state), GFP_KERNEL);
> +	if (!par_dev->state)
> +		goto err_put_par_dev;
> +
> +	devname = kstrdup(name, GFP_KERNEL);
> +	if (!devname)
> +		goto err_free_par_dev;
> +
> +	par_dev->name = devname;

The existing code is buggy here as we discussed previously.  Could you
just fix that before we do anything else?  It's freaking me out.

> +	par_dev->port = port;
> +	par_dev->daisy = -1;
> +	par_dev->preempt = par_dev_cb->preempt;
> +	par_dev->wakeup = par_dev_cb->wakeup;
> +	par_dev->private = par_dev_cb->private;
> +	par_dev->flags = par_dev_cb->flags;
> +	par_dev->irq_func = par_dev_cb->irq_func;
> +	par_dev->waiting = 0;
> +	par_dev->timeout = 5 * HZ;
> +
> +	par_dev->dev.parent = &port->ddev;
> +	par_dev->dev.bus = &parport_bus_type;
> +	dev_set_name(&par_dev->dev, "%s", devname);

dev_set_name() allocates a copy of "devname".  It can fail with -ENOMEM.
It's not likely but we check all the other allocations so we may as
well check here.

> +	par_dev->dev.release = free_pardevice;
> +	par_dev->devmodel = true;
> +	ret = device_register(&par_dev->dev);
> +	if (ret)
> +		goto err_free_all;

This is a bad name because as soon as you add a new allocation you will
have to change the name.  Use something like err_put_dev.

> +
> +	/* Chain this onto the list */
> +	par_dev->prev = NULL;
> +	/*
> +	 * This function must not run from an irq handler so we don' t need
> +	 * to clear irq on the local CPU. -arca
> +	 */
> +	spin_lock(&port->physport->pardevice_lock);
> +
> +	if (par_dev_cb->flags & PARPORT_DEV_EXCL) {
> +		if (port->physport->devices) {
> +			spin_unlock(&port->physport->pardevice_lock);
> +			pr_debug("%s: cannot grant exclusive access for device %s\n",
> +				 port->name, name);
> +			goto err_free_all;
> +		}
> +		port->flags |= PARPORT_FLAG_EXCL;
> +	}
> +
> +	par_dev->next = port->physport->devices;
> +	wmb();	/*
> +		 * Make sure that tmp->next is written before it's
> +		 * added to the list; see comments marked 'no locking
> +		 * required'
> +		 */
> +	if (port->physport->devices)
> +		port->physport->devices->prev = par_dev;
> +	port->physport->devices = par_dev;
> +	spin_unlock(&port->physport->pardevice_lock);
> +
> +	init_waitqueue_head(&par_dev->wait_q);
> +	par_dev->timeslice = parport_default_timeslice;
> +	par_dev->waitnext = NULL;
> +	par_dev->waitprev = NULL;
> +
> +	/*
> +	 * This has to be run as last thing since init_state may need other
> +	 * pardevice fields. -arca
> +	 */
> +	port->ops->init_state(par_dev, par_dev->state);
> +	port->proc_device = par_dev;
> +	parport_device_proc_register(par_dev);
> +
> +	return par_dev;
> +
> +err_free_all:
> +	put_device(&par_dev->dev);
> +err_free_par_dev:
> +	kfree(par_dev->state);
> +err_put_par_dev:
> +	if (!par_dev->devmodel)
> +		kfree(par_dev);
> +err_par_dev:
> +	parport_put_port(port);
> +	module_put(port->ops->owner);
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL(parport_register_dev);
> +
>  /**
>   *	parport_unregister_device - deregister a device on a parallel port
>   *	@dev: pointer to structure representing device
> @@ -691,7 +903,10 @@ void parport_unregister_device(struct pardevice *dev)
>  	spin_unlock_irq(&port->waitlist_lock);
>  
>  	kfree(dev->state);
> -	kfree(dev);
> +	if (dev->devmodel)
> +		device_unregister(&dev->dev);
> +	else
> +		kfree(dev);
>  
>  	module_put(port->ops->owner);
>  	parport_put_port (port);
> @@ -926,8 +1141,8 @@ int parport_claim_or_block(struct pardevice *dev)
>  			       dev->port->physport->cad ?
>  			       dev->port->physport->cad->name:"nobody");
>  #endif
> -	}
> -	dev->waiting = 0;
> +	} else if (r == 0)
> +		dev->waiting = 0;
>  	return r;
>  }
>  
> diff --git a/include/linux/parport.h b/include/linux/parport.h
> index c22f125..83e1a6e 100644
> --- a/include/linux/parport.h
> +++ b/include/linux/parport.h
> @@ -13,6 +13,7 @@
>  #include <linux/wait.h>
>  #include <linux/irqreturn.h>
>  #include <linux/semaphore.h>
> +#include <linux/device.h>
>  #include <asm/ptrace.h>
>  #include <uapi/linux/parport.h>
>  
> @@ -145,6 +146,8 @@ struct pardevice {
>  	unsigned int flags;
>  	struct pardevice *next;
>  	struct pardevice *prev;
> +	struct device dev;
> +	bool devmodel;
>  	struct parport_state *state;     /* saved status over preemption */
>  	wait_queue_head_t wait_q;
>  	unsigned long int time;
> @@ -156,6 +159,8 @@ struct pardevice {
>  	void * sysctl_table;
>  };
>  
> +#define to_pardevice(n) container_of(n, struct pardevice, dev)
> +
>  /* IEEE1284 information */
>  
>  /* IEEE1284 phases. These are exposed to userland through ppdev IOCTL
> @@ -195,7 +200,7 @@ struct parport {
>  				 * This may unfortulately be null if the
>  				 * port has a legacy driver.
>  				 */
> -
> +	struct device ddev;	/* to link with the bus */

What does ddev stand for?  Anyway, it's probably better to call it
bus_dev?


regards,
dan carpenter

Re: [PATCH v2 WIP 1/2] parport: add device-model to parport subsystem

[Sudip Mukherjee]

On Thu, Apr 23, 2015 at 06:18:37PM +0300, Dan Carpenter wrote:
> On Tue, Apr 21, 2015 at 07:22:34PM +0530, Sudip Mukherjee wrote:
<snip>
> >  	.owner		= NULL,
> >  };
> >  
> > +struct bus_type parport_bus_type = {
> > +	.name           = "parport",
> > +};
> 
> Can you make this static?  The indenting is a bit off.

done in v3.
> 
<snip>
> > + * which it wants to register its device.
> > + */
> > +static int port_check(struct device *dev, void *drv)
> > +{
> > +	((struct parport_driver *)drv)->check(to_parport_dev(dev));
> > +	return 0;
> > +}
> 
> The cast isn't beautiful.  Do this:
> 
> static int port_check(struct device *dev, void *_drv)
> {
> 	struct parport_driver *drv = _drv;
> 
> 	drv->check(to_parport_dev(dev));
> 	return 0;
> }
> 
> What is the point of the check function really?  The name isn't clear.
yes, i am a bit blunt in thinking of new names, i hope you have noticed
that in my naming of the labels .. :)

as the name was not sufficient i mentioned it in the comments. This check
function will receive the device details and will decide if it wants to
connect to that device. If it wants to connect then it registers its device
and mark the port as claimed.
Infact, on second thought, i will return the success or error from check,
then if the driver has found the device to connect then we can stop the
iteration there.

maybe a better name can be check_port() ? 
> 
> Since it always returns zero that means we loop through all the devices
> and then returns NULL.  It feels like a function called
> bus_find_device() should find something.  We have bus_for_each_dev() if
> we just want to iterate.
> 
yes, bus_for_each_dev() will be better here. thanks.

> > +
> > +/*
<snip>	
> > +
> > +	par_dev->name = devname;
> 
> The existing code is buggy here as we discussed previously.  Could you
> just fix that before we do anything else?  It's freaking me out.

quoting from your previous mail:
>My concern is that it gets freed before we are done using it or something

here, i have modified that and we are no longer using the string passed
as an argument. we have duplicated it using kstrdup and using that and
it gets freed in free_pardevice().
or am i missing something here?

> 
> > +	par_dev->port = port;
> > +	par_dev->daisy = -1;
> > +	par_dev->preempt = par_dev_cb->preempt;
> > +	par_dev->wakeup = par_dev_cb->wakeup;
> > +	par_dev->private = par_dev_cb->private;
> > +	par_dev->flags = par_dev_cb->flags;
> > +	par_dev->irq_func = par_dev_cb->irq_func;
> > +	par_dev->waiting = 0;
> > +	par_dev->timeout = 5 * HZ;
> > +
> > +	par_dev->dev.parent = &port->ddev;
> > +	par_dev->dev.bus = &parport_bus_type;
> > +	dev_set_name(&par_dev->dev, "%s", devname);
> 
> dev_set_name() allocates a copy of "devname".  It can fail with -ENOMEM.
> It's not likely but we check all the other allocations so we may as
> well check here.
ok.
> 
> > +	par_dev->dev.release = free_pardevice;
> > +	par_dev->devmodel = true;
> > +	ret = device_register(&par_dev->dev);
> > +	if (ret)
> > +		goto err_free_all;
> 
> This is a bad name because as soon as you add a new allocation you will
> have to change the name.  Use something like err_put_dev.
you have said this in your last mail also. i missed it .. sorry .. :(
		
> 
> > +
> > -
> > +	struct device ddev;	/* to link with the bus */
> 
> What does ddev stand for?  Anyway, it's probably better to call it
> bus_dev?
ok.
I think I will wait today for some comments from Greg and then send in
the v3 tomorrow.

thanks for your time and the review.

regards
sudip

> 
> 
> regards,
> dan carpenter

Re: [PATCH v2 WIP 1/2] parport: add device-model to parport subsystem

[Dan Carpenter]

On Fri, Apr 24, 2015 at 12:20:26PM +0530, Sudip Mukherjee wrote:
> > What is the point of the check function really?  The name isn't clear.
> yes, i am a bit blunt in thinking of new names, i hope you have noticed
> that in my naming of the labels .. :)
> 
> as the name was not sufficient i mentioned it in the comments. This check
> function will receive the device details and will decide if it wants to
> connect to that device. If it wants to connect then it registers its device
> and mark the port as claimed.
> Infact, on second thought, i will return the success or error from check,
> then if the driver has found the device to connect then we can stop the
> iteration there.
> 
> maybe a better name can be check_port() ? 

match() or match_port() something.

> > 
> > Since it always returns zero that means we loop through all the devices
> > and then returns NULL.  It feels like a function called
> > bus_find_device() should find something.  We have bus_for_each_dev() if
> > we just want to iterate.
> > 
> yes, bus_for_each_dev() will be better here. thanks.

If we're match then bus_find_device() is correct.  It's just that's not
what v2 did.

> 
> > > +
> > > +/*
> <snip>	
> > > +
> > > +	par_dev->name = devname;
> > 
> > The existing code is buggy here as we discussed previously.  Could you
> > just fix that before we do anything else?  It's freaking me out.
> 
> quoting from your previous mail:
> >My concern is that it gets freed before we are done using it or something
> 
> here, i have modified that and we are no longer using the string passed
> as an argument. we have duplicated it using kstrdup and using that and
> it gets freed in free_pardevice().
> or am i missing something here?

Ah.  Ok.  Thanks.  I missed that and I don't think the patch has hit
linux-next yet.

regards,
dan carpenter

Re: [PATCH v2 WIP 1/2] parport: add device-model to parport subsystem

[Greg KH]

On Fri, Apr 24, 2015 at 12:20:26PM +0530, Sudip Mukherjee wrote:
> > What does ddev stand for?  Anyway, it's probably better to call it
> > bus_dev?
> ok.
> I think I will wait today for some comments from Greg and then send in
> the v3 tomorrow.

Don't wait for me, it's going to be a week or so at the earliest before
I have a chance to look at this, sorry.

greg "burried in email" k-h

Re: [PATCH v2 WIP 1/2] parport: add device-model to parport subsystem

[Sudip Mukherjee]

On Fri, Apr 24, 2015 at 09:26:08AM +0200, Greg KH wrote:
> On Fri, Apr 24, 2015 at 12:20:26PM +0530, Sudip Mukherjee wrote:
> > > What does ddev stand for?  Anyway, it's probably better to call it
> > > bus_dev?
> > ok.
> > I think I will wait today for some comments from Greg and then send in
> > the v3 tomorrow.
> 
> Don't wait for me, it's going to be a week or so at the earliest before
> I have a chance to look at this, sorry.

No problem. I will change what Dan has suggested and will
request Alan and Dean to check on their hardware as well (ofcourse if
they have time).

but finally it has to be you as you have to add it to your tree and I know
you can not modify your tree during the merge window.

regards
sudip

> 
> greg "burried in email" k-h

Re: [PATCH v2 WIP 1/2] parport: add device-model to parport subsystem

[Sudip Mukherjee]

On Fri, Apr 24, 2015 at 10:04:54AM +0300, Dan Carpenter wrote:
> On Fri, Apr 24, 2015 at 12:20:26PM +0530, Sudip Mukherjee wrote:
> > > What is the point of the check function really?  The name isn't clear.
> > yes, i am a bit blunt in thinking of new names, i hope you have noticed
> > that in my naming of the labels .. :)
> > 
> > as the name was not sufficient i mentioned it in the comments. This check
> > function will receive the device details and will decide if it wants to
> > connect to that device. If it wants to connect then it registers its device
> > and mark the port as claimed.
> > Infact, on second thought, i will return the success or error from check,
> > then if the driver has found the device to connect then we can stop the
> > iteration there.
> > 
> > maybe a better name can be check_port() ? 
> 
> match() or match_port() something.
> 
> > > 
> > > Since it always returns zero that means we loop through all the devices
> > > and then returns NULL.  It feels like a function called
> > > bus_find_device() should find something.  We have bus_for_each_dev() if
> > > we just want to iterate.
> > > 
> > yes, bus_for_each_dev() will be better here. thanks.
> 
> If we're match then bus_find_device() is correct.  It's just that's not
> what v2 did.
that was the main intention but but bus_find_device() will also do
a get_device() once a match is found, then in that case I will have to do
a put_device() immediately after bus_find_device() completes.
> 
> > 
> > > > +
> > > > +/*
> > <snip>	
> > > > +
> > > > +	par_dev->name = devname;
> > > 
> > > The existing code is buggy here as we discussed previously.  Could you
> > > just fix that before we do anything else?  It's freaking me out.
> > 
> > quoting from your previous mail:
> > >My concern is that it gets freed before we are done using it or something
> > 
> > here, i have modified that and we are no longer using the string passed
> > as an argument. we have duplicated it using kstrdup and using that and
> > it gets freed in free_pardevice().
> > or am i missing something here?
> 
> Ah.  Ok.  Thanks.  I missed that and I don't think the patch has hit
> linux-next yet.
hehe , no. Greg has to apply the patch, and in the last patch he found
some points in his review. I will send in a v3 as soon as the confusion
about the bus_find_device() or bus_for_each_dev() clears.

regards
sudip

> 
> regards,
> dan carpenter
> 

Re: [PATCH v2 WIP 1/2] parport: add device-model to parport subsystem

[One Thousand Gnomes]

On Fri, 24 Apr 2015 13:08:25 +0530
Sudip Mukherjee <sudipm.mukherjee@gmail.com> wrote:

> On Fri, Apr 24, 2015 at 09:26:08AM +0200, Greg KH wrote:
> > On Fri, Apr 24, 2015 at 12:20:26PM +0530, Sudip Mukherjee wrote:
> > > > What does ddev stand for?  Anyway, it's probably better to call it
> > > > bus_dev?
> > > ok.
> > > I think I will wait today for some comments from Greg and then send in
> > > the v3 tomorrow.
> > 
> > Don't wait for me, it's going to be a week or so at the earliest before
> > I have a chance to look at this, sorry.
> 
> No problem. I will change what Dan has suggested and will
> request Alan and Dean to check on their hardware as well (ofcourse if
> they have time).

Let me know when you have a set of patches you feel are good for testing
and I'll give them a go. I just picked up a backpack CD-ROM drive on ebay
for 99p as well 8)

Alan

Re: [PATCH v2 WIP 1/2] parport: add device-model to parport subsystem

[Sudip Mukherjee]

On Fri, Apr 24, 2015 at 07:37:02PM +0100, One Thousand Gnomes wrote:

Hi Alan,
> On Fri, 24 Apr 2015 13:08:25 +0530
> Sudip Mukherjee <sudipm.mukherjee@gmail.com> wrote:
> 
> > On Fri, Apr 24, 2015 at 09:26:08AM +0200, Greg KH wrote:
> > > On Fri, Apr 24, 2015 at 12:20:26PM +0530, Sudip Mukherjee wrote:
> > > > > What does ddev stand for?  Anyway, it's probably better to call it
> > > > > bus_dev?
> > > > ok.
> > > > I think I will wait today for some comments from Greg and then send in
> > > > the v3 tomorrow.
> > > 
> > > Don't wait for me, it's going to be a week or so at the earliest before
> > > I have a chance to look at this, sorry.
> > 
> > No problem. I will change what Dan has suggested and will
> > request Alan and Dean to check on their hardware as well (ofcourse if
> > they have time).
> 
> Let me know when you have a set of patches you feel are good for testing
> and I'll give them a go.
This series of WIP is also good for testing. But for next version of WIP
I need to know something from Jean about his i2c-parport.
For all other drivers using the parallel port, they are choosing which
port to use. But i2c-parport is not specifying that. So as a result, in
my test system which has multiple parallel ports, when I load the module,
i2c-parport is claiming all the ports with exclusive access. So if i
want to use i2c-parport on one parallel port and any other device on
another port then that is not possible. If that is the intended behaviour
then I need to change some parts in the way I was making the parallel
port device-model. Anyways, I have mailed Jean yesterday, and waiting
for his comments now. Adding cc to Jean also to this mail.

>I just picked up a backpack CD-ROM drive on ebay
> for 99p as well 8)

wow, share me the link please. I will also get one.
I am able to manage only one parallel printer and staging/panel for
testing. I am looking for a parallel port zip drive or cdrom or scanner
to manage and test the code better. but :(

regards
sudip

> 
> Alan

Re: [PATCH v2 WIP 1/2] parport: add device-model to parport subsystem

[Sudip Mukherjee]

On Sat, Apr 25, 2015 at 11:39:29AM +0530, Sudip Mukherjee wrote:
> On Fri, Apr 24, 2015 at 07:37:02PM +0100, One Thousand Gnomes wrote:
> 
<snip>
> > Let me know when you have a set of patches you feel are good for testing
> > and I'll give them a go.
> This series of WIP is also good for testing.
well, this series will create some ghost parport in /sys/bus/parport,
which were being created while probing.
It has been fixed in my local tree and next WIP patch should be a fully
working one. But if you are testig this patchset then please use only
parport0.

regards
sudip