[PATCH v5 WIP 1/5] parport: add device-model to parport subsystem

[PATCH v5 WIP 1/5] parport: add device-model to parport subsystem

[Sudip Mukherjee]

parport subsystem starts using the device-model. drivers using the
device-model has to define probe and should register the device with
parport with parport_register_dev_model()

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---

v5:
	a) addition/removal of ports are now handled.
	b) is_parport moved to the core level
	c) common parport_driver_register code
	d) parport_register_dev_model now accepts instance number.

v4:	use of is_parport() is introduced to check the type of device
	that has been passed to probe or match_port.

v3:	started use of parport_del_port(). previously it was creating
	some ghost parallel ports during port probing.
	parport_del_port() removes registered ports if probing has
	failed.

v2:	started using probe function. Without probe,whenever any driver
	is trying to register, it is getting bound to all the available
	parallel ports. To solve that probe was required.
	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 v1 Greg mentioned that we do not need to maintain our own
	list. That has been partially done. we are no longer 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.

 drivers/parport/parport_pc.c |   4 +-
 drivers/parport/procfs.c     |  15 +-
 drivers/parport/share.c      | 335 +++++++++++++++++++++++++++++++++++++++----
 include/linux/parport.h      |  43 +++++-
 4 files changed, 368 insertions(+), 29 deletions(-)

diff --git a/drivers/parport/parport_pc.c b/drivers/parport/parport_pc.c
index 53d15b3..78530d1 100644
--- a/drivers/parport/parport_pc.c
+++ b/drivers/parport/parport_pc.c
@@ -2255,7 +2255,7 @@ out5:
 		release_region(base+0x3, 5);
 	release_region(base, 3);
 out4:
-	parport_put_port(p);
+	parport_del_port(p);
 out3:
 	kfree(priv);
 out2:
@@ -2294,7 +2294,7 @@ void parport_pc_unregister_port(struct parport *p)
 				    priv->dma_handle);
 #endif
 	kfree(p->private_data);
-	parport_put_port(p);
+	parport_del_port(p);
 	kfree(ops); /* hope no-one cached it */
 }
 EXPORT_SYMBOL(parport_pc_unregister_port);
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..eb5d91d 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,13 +101,79 @@ static struct parport_operations dead_ops = {
 	.owner		= NULL,
 };
 
+static struct device_type parport_device_type = {
+	.name = "parport",
+};
+
+static int is_parport(struct device *dev)
+{
+	return dev->type == &parport_device_type;
+}
+
+static int parport_probe(struct device *dev)
+{
+	struct parport_driver *drv;
+
+	if (is_parport(dev))
+		return -ENODEV;
+
+	drv = to_parport_driver(dev->driver);
+	if (!drv->probe)
+		return -ENODEV;
+
+	return drv->probe(to_pardevice(dev));
+}
+
+static struct bus_type parport_bus_type = {
+	.name = "parport",
+	.probe = parport_probe,
+};
+
+int parport_bus_init(void)
+{
+	return bus_register(&parport_bus_type);
+}
+
+void parport_bus_exit(void)
+{
+	bus_unregister(&parport_bus_type);
+}
+
+static int driver_check(struct device_driver *_drv, void *_port)
+{
+	struct parport *port = _port;
+	struct parport_driver *drv = to_parport_driver(_drv);
+
+	if (drv->match_port)
+		drv->match_port(port);
+	return 0;
+}
+
 /* Call attach(port) for each registered driver. */
 static void attach_driver_chain(struct parport *port)
 {
 	/* caller has exclusive registration_lock */
 	struct parport_driver *drv;
+
 	list_for_each_entry(drv, &drivers, list)
 		drv->attach(port);
+
+	/*
+	 * call the port_check function of the drivers registered in
+	 * new device model
+	 */
+
+	bus_for_each_drv(&parport_bus_type, NULL, port, driver_check);
+}
+
+static int driver_detach(struct device_driver *_drv, void *_port)
+{
+	struct parport *port = _port;
+	struct parport_driver *drv = to_parport_driver(_drv);
+
+	if (drv->detach)
+		drv->detach(port);
+	return 0;
 }
 
 /* Call detach(port) for each registered driver. */
@@ -116,6 +183,13 @@ static void detach_driver_chain(struct parport *port)
 	/* caller has exclusive registration_lock */
 	list_for_each_entry(drv, &drivers, list)
 		drv->detach (port);
+
+	/*
+	 * call the detach function of the drivers registered in
+	 * new device model
+	 */
+
+	bus_for_each_drv(&parport_bus_type, NULL, port, driver_detach);
 }
 
 /* Ask kmod for some lowlevel drivers. */
@@ -126,17 +200,39 @@ static void get_lowlevel_driver (void)
 	request_module ("parport_lowlevel");
 }
 
+/*
+ * iterates through all the devices connected to the bus and sends the device
+ * details to the match_port 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 = _drv;
+
+	/* only send ports, do not send other devices connected to bus */
+	if (is_parport(dev))
+		drv->match_port(to_parport_dev(dev));
+	return 0;
+}
+
 /**
  *	parport_register_driver - register a parallel port device driver
  *	@drv: structure describing the driver
+ *	@owner: owner module of drv
+ *	@mod_name: module name string
  *
  *	This can be called by a parallel port device driver in order
  *	to receive notifications about ports being found in the
  *	system, as well as ports no longer available.
  *
+ *	If the probe function is defined then the new device model is used
+ *	for registration.
+ *
  *	The @drv structure is allocated by the caller and must not be
  *	deallocated until after calling parport_unregister_driver().
  *
+ *	If using the non device model:
  *	The driver's attach() function may block.  The port that
  *	attach() is given will be valid for the duration of the
  *	callback, but if the driver wants to take a copy of the
@@ -148,21 +244,58 @@ static void get_lowlevel_driver (void)
  *	callback, but if the driver wants to take a copy of the
  *	pointer it must call parport_get_port() to do so.
  *
- *	Returns 0 on success.  Currently it always succeeds.
+ *
+ *	Returns 0 on success. The non device model will always succeeds.
+ *	but the new device model can fail and will return the error code.
  **/
 
-int parport_register_driver (struct parport_driver *drv)
+int __parport_register_driver(struct parport_driver *drv, struct module *owner,
+			      const char *mod_name)
 {
-	struct parport *port;
-
 	if (list_empty(&portlist))
 		get_lowlevel_driver ();
 
-	mutex_lock(&registration_lock);
-	list_for_each_entry(port, &portlist, list)
-		drv->attach(port);
-	list_add(&drv->list, &drivers);
-	mutex_unlock(&registration_lock);
+	if (drv->probe) {
+		/* using device model */
+		int ret;
+
+		/* 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->devmodel = true;
+		ret = driver_register(&drv->driver);
+		if (ret)
+			return ret;
+
+		mutex_lock(&registration_lock);
+		if (drv->match_port)
+			bus_for_each_dev(&parport_bus_type, NULL, drv,
+					 port_check);
+		mutex_unlock(&registration_lock);
+	} else {
+		struct parport *port;
+
+		drv->devmodel = false;
+
+		mutex_lock(&registration_lock);
+		list_for_each_entry(port, &portlist, list)
+			drv->attach(port);
+		list_add(&drv->list, &drivers);
+		mutex_unlock(&registration_lock);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(__parport_register_driver);
+
+static int port_detach(struct device *dev, void *_drv)
+{
+	struct parport_driver *drv = _drv;
+
+	if (is_parport(dev) && drv->detach)
+		drv->detach(to_parport_dev(dev));
 
 	return 0;
 }
@@ -189,15 +322,22 @@ 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) {
+		bus_for_each_dev(&parport_bus_type, NULL, drv, port_detach);
+		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,9 +363,16 @@ 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->bus_dev);
+
+	return to_parport_dev(dev);
+}
+
+void parport_del_port(struct parport *port)
+{
+	device_unregister(&port->bus_dev);
 }
+EXPORT_SYMBOL(parport_del_port);
 
 /**
  *	parport_put_port - decrement a port's reference count
@@ -237,11 +384,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->bus_dev);
 }
 
 /**
@@ -281,6 +424,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 +477,10 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma,
 	 */
 	sprintf(name, "parport%d", tmp->portnum = tmp->number);
 	tmp->name = name;
+	tmp->bus_dev.bus = &parport_bus_type;
+	tmp->bus_dev.release = free_port;
+	dev_set_name(&tmp->bus_dev, name);
+	tmp->bus_dev.type = &parport_device_type;
 
 	for (device = 0; device < 5; device++)
 		/* assume the worst */
@@ -340,6 +488,12 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma,
 
 	tmp->waithead = tmp->waittail = NULL;
 
+	ret = device_register(&tmp->bus_dev);
+	if (ret) {
+		put_device(&tmp->bus_dev);
+		return NULL;
+	}
+
 	return tmp;
 }
 
@@ -575,6 +729,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 +785,136 @@ 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_model(struct parport *port, const char *name,
+			   struct pardev_cb *par_dev_cb, int cnt)
+{
+	struct pardevice *par_dev;
+	int ret;
+	char *devname;
+
+	if (port->physport->flags & PARPORT_FLAG_EXCL) {
+		/* An exclusive device is registered. */
+		pr_err("%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->bus_dev;
+	par_dev->dev.bus = &parport_bus_type;
+	ret = dev_set_name(&par_dev->dev, "%s.%d", devname, cnt);
+	if (ret)
+		goto err_free_devname;
+	par_dev->dev.release = free_pardevice;
+	par_dev->devmodel = true;
+	ret = device_register(&par_dev->dev);
+	if (ret)
+		goto 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_put_dev;
+		}
+		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_put_dev:
+	put_device(&par_dev->dev);
+err_free_devname:
+	kfree(devname);
+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_model);
+
 /**
  *	parport_unregister_device - deregister a device on a parallel port
  *	@dev: pointer to structure representing device
@@ -691,7 +976,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 +1214,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;
 }
 
@@ -1019,7 +1307,6 @@ EXPORT_SYMBOL(parport_release);
 EXPORT_SYMBOL(parport_register_port);
 EXPORT_SYMBOL(parport_announce_port);
 EXPORT_SYMBOL(parport_remove_port);
-EXPORT_SYMBOL(parport_register_driver);
 EXPORT_SYMBOL(parport_unregister_driver);
 EXPORT_SYMBOL(parport_register_device);
 EXPORT_SYMBOL(parport_unregister_device);
diff --git a/include/linux/parport.h b/include/linux/parport.h
index c22f125..d8d425e 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 bus_dev;	/* 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, bus_dev)
+
 #define DEFAULT_SPIN_TIME 500 /* us */
 
 struct parport_driver {
 	const char *name;
 	void (*attach) (struct parport *);
 	void (*detach) (struct parport *);
+	void (*match_port)(struct parport *);
+	int (*probe)(struct pardevice *);
+	struct device_driver driver;
+	bool devmodel;
 	struct list_head list;
 };
 
+#define to_parport_driver(n) container_of(n, struct parport_driver, driver)
+
+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
@@ -272,10 +288,20 @@ void parport_announce_port (struct parport *port);
 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_driver(struct parport_driver *,
+					   struct module *,
+					   const char *mod_name);
+/*
+ * parport_register_driver must be a macro so that KBUILD_MODNAME can
+ * be expanded
+ */
+#define parport_register_driver(driver)             \
+	__parport_register_driver(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. */
@@ -288,6 +314,15 @@ extern irqreturn_t parport_irq_handler(int irq, void *dev_id);
 /* Reference counting for ports. */
 extern struct parport *parport_get_port (struct parport *);
 extern void parport_put_port (struct parport *);
+void parport_del_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.
@@ -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_model(struct parport *port, const char *name,
+			   struct pardev_cb *par_dev_cb, int cnt);
+
 /* parport_unregister unlinks a device from the chain. */
 extern void parport_unregister_device(struct pardevice *dev);
 
-- 
1.8.1.2

[PATCH v5 WIP 2/5] staging: panel: use new parport device model

[Sudip Mukherjee]

converted to use the new device-model parallel port.

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

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 1d8ed8b..772a82a 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -2188,6 +2188,18 @@ static struct notifier_block panel_notifier = {
 	0
 };
 
+static int panel_probe(struct pardevice *par_dev)
+{
+	if (strcmp(par_dev->name, "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 +2211,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_model(port, "panel", &panel_cb, 0);
 	if (pprt == NULL) {
 		pr_err("%s: port->number=%d parport=%d, parport_register_device() failed\n",
 		       __func__, port->number, parport);
@@ -2256,8 +2265,9 @@ static void panel_detach(struct parport *port)
 
 static struct parport_driver panel_driver = {
 	.name = "panel",
-	.attach = panel_attach,
+	.match_port = panel_attach,
 	.detach = panel_detach,
+	.probe = panel_probe,
 };
 
 /* init function */
-- 
1.8.1.2

[PATCH v5 WIP 3/5] i2c-parport: define ports to connect

[Sudip Mukherjee]

as of now i2c-parport was connecting to all the available parallel
ports. Lets limit that to maximum of 4 instances and at the same time
define which instance connects to which parallel port

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/i2c/busses/i2c-parport.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c
index a1fac5a..a9b25c3 100644
--- a/drivers/i2c/busses/i2c-parport.c
+++ b/drivers/i2c/busses/i2c-parport.c
@@ -46,6 +46,9 @@ struct i2c_par {
 
 static LIST_HEAD(adapter_list);
 static DEFINE_MUTEX(adapter_list_lock);
+#define MAX_DEVICE 4
+static int parport[MAX_DEVICE] = {0, -1, -1, -1};
+
 
 /* ----- Low-level parallel port access ----------------------------------- */
 
@@ -163,6 +166,18 @@ static void i2c_parport_irq(void *data)
 static void i2c_parport_attach(struct parport *port)
 {
 	struct i2c_par *adapter;
+	int i;
+
+	for (i = 0; i < MAX_DEVICE; i++) {
+		if (parport[i] == -1)
+			continue;
+		if (port->number == parport[i])
+			break;
+	}
+	if (i == MAX_DEVICE) {
+		pr_err("port mentioned not found\n");
+		return;
+	}
 
 	adapter = kzalloc(sizeof(struct i2c_par), GFP_KERNEL);
 	if (adapter == NULL) {
@@ -298,5 +313,11 @@ MODULE_AUTHOR("Jean Delvare <jdelvare@suse.de>");
 MODULE_DESCRIPTION("I2C bus over parallel port");
 MODULE_LICENSE("GPL");
 
+module_param_array(parport, int, NULL, 0);
+MODULE_PARM_DESC(parport, "Atmost 4 instances are allowed.\n"
+			  "Mention portnumbers in array.\n"
+			  "If the port is not to be used mention -1.\n"
+			  "Default is one instance connected to parport0\n");
+
 module_init(i2c_parport_init);
 module_exit(i2c_parport_exit);
-- 
1.8.1.2

[PATCH v5 WIP 4/5] i2c-parport: use new parport device model

[Sudip Mukherjee]

modify i2c-parport driver to use the new parallel port device model.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/i2c/busses/i2c-parport.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c
index a9b25c3..6db5b45 100644
--- a/drivers/i2c/busses/i2c-parport.c
+++ b/drivers/i2c/busses/i2c-parport.c
@@ -163,6 +163,11 @@ static void i2c_parport_irq(void *data)
 			"SMBus alert received but no ARA client!\n");
 }
 
+static struct pardev_cb i2c_parport_cb = {
+	.flags = PARPORT_FLAG_EXCL,
+	.irq_func = i2c_parport_irq,
+};
+
 static void i2c_parport_attach(struct parport *port)
 {
 	struct i2c_par *adapter;
@@ -184,11 +189,12 @@ static void i2c_parport_attach(struct parport *port)
 		printk(KERN_ERR "i2c-parport: Failed to kzalloc\n");
 		return;
 	}
+	i2c_parport_cb.private = adapter;
 
 	pr_debug("i2c-parport: attaching to %s\n", port->name);
 	parport_disable_irq(port);
-	adapter->pdev = parport_register_device(port, "i2c-parport",
-		NULL, NULL, i2c_parport_irq, PARPORT_FLAG_EXCL, adapter);
+	adapter->pdev = parport_register_dev_model(port, "i2c-parport",
+						   &i2c_parport_cb, i);
 	if (!adapter->pdev) {
 		printk(KERN_ERR "i2c-parport: Unable to register with parport\n");
 		goto err_free;
@@ -281,10 +287,18 @@ static void i2c_parport_detach(struct parport *port)
 	mutex_unlock(&adapter_list_lock);
 }
 
+static int i2c_parport_probe(struct pardevice *par_dev)
+{
+	if (strcmp(par_dev->name, "i2c-parport"))
+		return -ENODEV;
+	return 0;
+}
+
 static struct parport_driver i2c_parport_driver = {
 	.name	= "i2c-parport",
-	.attach	= i2c_parport_attach,
+	.match_port = i2c_parport_attach,
 	.detach	= i2c_parport_detach,
+	.probe	= i2c_parport_probe,
 };
 
 /* ----- Module loading, unloading and information ------------------------ */
-- 
1.8.1.2

[PATCH v5 WIP 5/5] paride: use new parport device model

[Sudip Mukherjee]

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

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---

in v4 of i2c-parport patch Jean mentioned to use the full name while
in probe. That has been done in other drivers except this one.
The higher layer drivers (pcd , pd etc.) are appending the unit number
to the name before the name is being sent to the lower level (paride).

 drivers/block/paride/paride.c | 61 ++++++++++++++++++++++++++++++++++++++-----
 drivers/block/paride/paride.h |  2 ++
 drivers/block/paride/pcd.c    |  9 +++++++
 drivers/block/paride/pd.c     | 12 ++++++++-
 drivers/block/paride/pf.c     |  7 +++++
 drivers/block/paride/pg.c     |  8 ++++++
 drivers/block/paride/pt.c     |  8 ++++++
 7 files changed, 100 insertions(+), 7 deletions(-)

diff --git a/drivers/block/paride/paride.c b/drivers/block/paride/paride.c
index 48c50f1..75c217a 100644
--- a/drivers/block/paride/paride.c
+++ b/drivers/block/paride/paride.c
@@ -30,6 +30,7 @@
 #include <linux/wait.h>
 #include <linux/sched.h>	/* TASK_* */
 #include <linux/parport.h>
+#include <linux/slab.h>
 
 #include "paride.h"
 
@@ -244,18 +245,26 @@ void paride_unregister(PIP * pr)
 
 EXPORT_SYMBOL(paride_unregister);
 
-static int pi_register_parport(PIA * pi, int verbose)
+static int pi_register_parport(PIA *pi, int verbose, int unit)
 {
 	struct parport *port;
+	struct pardev_cb *par_cb;
 
 	port = parport_find_base(pi->port);
 	if (!port)
 		return 0;
-
-	pi->pardev = parport_register_device(port,
-					     pi->device, NULL,
-					     pi_wake_up, NULL, 0, (void *) pi);
+	par_cb = kzalloc(sizeof(*par_cb), GFP_KERNEL);
+	if (!par_cb) {
+		parport_put_port(port);
+		return 0;
+	}
+	par_cb->flags = 0;
+	par_cb->wakeup = pi_wake_up;
+	par_cb->private = (void *)pi;
+	pi->pardev = parport_register_dev_model(port, pi->device, par_cb,
+						unit);
 	parport_put_port(port);
+	kfree(par_cb);
 	if (!pi->pardev)
 		return 0;
 
@@ -311,7 +320,7 @@ static int pi_probe_unit(PIA * pi, int unit, char *scratch, int verbose)
 		e = pi->proto->max_units;
 	}
 
-	if (!pi_register_parport(pi, verbose))
+	if (!pi_register_parport(pi, verbose, s))
 		return 0;
 
 	if (pi->proto->test_port) {
@@ -432,3 +441,43 @@ int pi_init(PIA * pi, int autoprobe, int port, int mode,
 }
 
 EXPORT_SYMBOL(pi_init);
+
+static int pi_probe(struct pardevice *par_dev)
+{
+	struct device_driver *drv = par_dev->dev.driver;
+	int len = strlen(drv->name);
+
+	if (strncmp(par_dev->name, drv->name, len))
+		return -ENODEV;
+
+	return 0;
+}
+
+void *pi_register_driver(char *name)
+{
+	struct parport_driver *parp_drv;
+	int ret;
+
+	parp_drv = kzalloc(sizeof(*parp_drv), GFP_KERNEL);
+	if (!parp_drv)
+		return NULL;
+
+	parp_drv->name = name;
+	parp_drv->probe = pi_probe;
+
+	ret = parport_register_driver(parp_drv);
+
+	if (ret)
+		return NULL;
+	return (void *)parp_drv;
+}
+EXPORT_SYMBOL(pi_register_driver);
+
+void pi_unregister_driver(void *_drv)
+{
+	struct parport_driver *drv = _drv;
+
+	parport_unregister_driver(drv);
+	kfree(drv);
+}
+EXPORT_SYMBOL(pi_unregister_driver);
diff --git a/drivers/block/paride/paride.h b/drivers/block/paride/paride.h
index 2bddbf4..ddb9e58 100644
--- a/drivers/block/paride/paride.h
+++ b/drivers/block/paride/paride.h
@@ -165,6 +165,8 @@ typedef struct pi_protocol PIP;
 
 extern int paride_register( PIP * );
 extern void paride_unregister ( PIP * );
+void *pi_register_driver(char *);
+void pi_unregister_driver(void *);
 
 #endif /* __DRIVERS_PARIDE_H__ */
 /* end of paride.h */
diff --git a/drivers/block/paride/pcd.c b/drivers/block/paride/pcd.c
index 3b7c9f1..9336236 100644
--- a/drivers/block/paride/pcd.c
+++ b/drivers/block/paride/pcd.c
@@ -221,6 +221,7 @@ static int pcd_busy;		/* request being processed ? */
 static int pcd_sector;		/* address of next requested sector */
 static int pcd_count;		/* number of blocks still to do */
 static char *pcd_buf;		/* buffer for request in progress */
+static void *par_drv;		/* reference of parport driver */
 
 /* kernel glue structures */
 
@@ -690,6 +691,12 @@ static int pcd_detect(void)
 	printk("%s: %s version %s, major %d, nice %d\n",
 	       name, name, PCD_VERSION, major, nice);
 
+	par_drv = pi_register_driver(name);
+	if (!par_drv) {
+		pr_err("failed to register %s driver\n", name);
+		return -1;
+	}
+
 	k = 0;
 	if (pcd_drive_count == 0) { /* nothing spec'd - so autoprobe for 1 */
 		cd = pcd;
@@ -723,6 +730,7 @@ static int pcd_detect(void)
 	printk("%s: No CD-ROM drive found\n", name);
 	for (unit = 0, cd = pcd; unit < PCD_UNITS; unit++, cd++)
 		put_disk(cd->disk);
+	pi_unregister_driver(par_drv);
 	return -1;
 }
 
@@ -984,6 +992,7 @@ static void __exit pcd_exit(void)
 	}
 	blk_cleanup_queue(pcd_queue);
 	unregister_blkdev(major, name);
+	pi_unregister_driver(par_drv);
 }
 
 MODULE_LICENSE("GPL");
diff --git a/drivers/block/paride/pd.c b/drivers/block/paride/pd.c
index dbb4da1..b9242d7 100644
--- a/drivers/block/paride/pd.c
+++ b/drivers/block/paride/pd.c
@@ -247,6 +247,8 @@ static char *pd_errs[17] = { "ERR", "INDEX", "ECC", "DRQ", "SEEK", "WRERR",
 	"IDNF", "MC", "UNC", "???", "TMO"
 };
 
+static void *par_drv;		/* reference of parport driver */
+
 static inline int status_reg(struct pd_unit *disk)
 {
 	return pi_read_regr(disk->pi, 1, 6);
@@ -872,6 +874,12 @@ static int pd_detect(void)
 			pd_drive_count++;
 	}
 
+	par_drv = pi_register_driver(name);
+	if (!par_drv) {
+		pr_err("failed to register %s driver\n", name);
+		return -1;
+	}
+
 	if (pd_drive_count == 0) { /* nothing spec'd - so autoprobe for 1 */
 		disk = pd;
 		if (pi_init(disk->pi, 1, -1, -1, -1, -1, -1, pd_scratch,
@@ -902,8 +910,10 @@ static int pd_detect(void)
 			found = 1;
 		}
 	}
-	if (!found)
+	if (!found) {
 		printk("%s: no valid drive found\n", name);
+		pi_unregister_driver(par_drv);
+	}
 	return found;
 }
 
diff --git a/drivers/block/paride/pf.c b/drivers/block/paride/pf.c
index 9a15fd3..7a7d977 100644
--- a/drivers/block/paride/pf.c
+++ b/drivers/block/paride/pf.c
@@ -264,6 +264,7 @@ static int pf_cmd;		/* current command READ/WRITE */
 static struct pf_unit *pf_current;/* unit of current request */
 static int pf_mask;		/* stopper for pseudo-int */
 static char *pf_buf;		/* buffer for request in progress */
+static void *par_drv;		/* reference of parport driver */
 
 /* kernel glue structures */
 
@@ -703,6 +704,11 @@ static int pf_detect(void)
 	printk("%s: %s version %s, major %d, cluster %d, nice %d\n",
 	       name, name, PF_VERSION, major, cluster, nice);
 
+	par_drv = pi_register_driver(name);
+	if (!par_drv) {
+		pr_err("failed to register %s driver\n", name);
+		return -1;
+	}
 	k = 0;
 	if (pf_drive_count == 0) {
 		if (pi_init(pf->pi, 1, -1, -1, -1, -1, -1, pf_scratch, PI_PF,
@@ -735,6 +741,7 @@ static int pf_detect(void)
 	printk("%s: No ATAPI disk detected\n", name);
 	for (pf = units, unit = 0; unit < PF_UNITS; pf++, unit++)
 		put_disk(pf->disk);
+	pi_unregister_driver(par_drv);
 	return -1;
 }
 
diff --git a/drivers/block/paride/pg.c b/drivers/block/paride/pg.c
index 876d0c3..bfbd4c8 100644
--- a/drivers/block/paride/pg.c
+++ b/drivers/block/paride/pg.c
@@ -227,6 +227,7 @@ static int pg_identify(struct pg *dev, int log);
 static char pg_scratch[512];	/* scratch block buffer */
 
 static struct class *pg_class;
+static void *par_drv;		/* reference of parport driver */
 
 /* kernel glue structures */
 
@@ -481,6 +482,12 @@ static int pg_detect(void)
 
 	printk("%s: %s version %s, major %d\n", name, name, PG_VERSION, major);
 
+	par_drv = pi_register_driver(name);
+	if (!par_drv) {
+		pr_err("failed to register %s driver\n", name);
+		return -1;
+	}
+
 	k = 0;
 	if (pg_drive_count == 0) {
 		if (pi_init(dev->pi, 1, -1, -1, -1, -1, -1, pg_scratch,
@@ -511,6 +518,7 @@ static int pg_detect(void)
 	if (k)
 		return 0;
 
+	pi_unregister_driver(par_drv);
 	printk("%s: No ATAPI device detected\n", name);
 	return -1;
 }
diff --git a/drivers/block/paride/pt.c b/drivers/block/paride/pt.c
index 2596042..1740d75 100644
--- a/drivers/block/paride/pt.c
+++ b/drivers/block/paride/pt.c
@@ -232,6 +232,7 @@ static int pt_identify(struct pt_unit *tape);
 static struct pt_unit pt[PT_UNITS];
 
 static char pt_scratch[512];	/* scratch block buffer */
+static void *par_drv;		/* reference of parport driver */
 
 /* kernel glue structures */
 
@@ -605,6 +606,12 @@ static int pt_detect(void)
 
 	printk("%s: %s version %s, major %d\n", name, name, PT_VERSION, major);
 
+	par_drv = pi_register_driver(name);
+	if (!par_drv) {
+		pr_err("failed to register %s driver\n", name);
+		return -1;
+	}
+
 	specified = 0;
 	for (unit = 0; unit < PT_UNITS; unit++) {
 		struct pt_unit *tape = &pt[unit];
@@ -644,6 +651,7 @@ static int pt_detect(void)
 	if (found)
 		return 0;
 
+	pi_unregister_driver(par_drv);
 	printk("%s: No ATAPI tape drive detected\n", name);
 	return -1;
 }
-- 
1.8.1.2

Re: [PATCH v5 WIP 3/5] i2c-parport: define ports to connect

[Jean Delvare]

Hi Sudip,

Le Wednesday 06 May 2015 à 15:46 +0530, Sudip Mukherjee a écrit :
> as of now i2c-parport was connecting to all the available parallel
> ports. Lets limit that to maximum of 4 instances and at the same time
> define which instance connects to which parallel port

A leading capital and a trailing dot would look better.

> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
>  drivers/i2c/busses/i2c-parport.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c
> index a1fac5a..a9b25c3 100644
> --- a/drivers/i2c/busses/i2c-parport.c
> +++ b/drivers/i2c/busses/i2c-parport.c
> @@ -46,6 +46,9 @@ struct i2c_par {
>  
>  static LIST_HEAD(adapter_list);
>  static DEFINE_MUTEX(adapter_list_lock);
> +#define MAX_DEVICE 4
> +static int parport[MAX_DEVICE] = {0, -1, -1, -1};
> +
>  
>  /* ----- Low-level parallel port access ----------------------------------- */
>  
> @@ -163,6 +166,18 @@ static void i2c_parport_irq(void *data)
>  static void i2c_parport_attach(struct parport *port)
>  {
>  	struct i2c_par *adapter;
> +	int i;
> +
> +	for (i = 0; i < MAX_DEVICE; i++) {
> +		if (parport[i] == -1)
> +			continue;
> +		if (port->number == parport[i])
> +			break;
> +	}
> +	if (i == MAX_DEVICE) {
> +		pr_err("port mentioned not found\n");

This error message needs to be improved. Someone seeing this in his/her
logs would have no idea where it comes from and what it means exactly.
You want to add "i2c-parport: " at the beginning, and say which port
number was specified.

> +		return;
> +	}
>  
>  	adapter = kzalloc(sizeof(struct i2c_par), GFP_KERNEL);
>  	if (adapter == NULL) {
> @@ -298,5 +313,11 @@ MODULE_AUTHOR("Jean Delvare <jdelvare@suse.de>");
>  MODULE_DESCRIPTION("I2C bus over parallel port");
>  MODULE_LICENSE("GPL");
>  
> +module_param_array(parport, int, NULL, 0);
> +MODULE_PARM_DESC(parport, "Atmost 4 instances are allowed.\n"

You should first say what the parameter does, before going into the
details. Please use __stringify(MAX_DEVICE) instead of hard-coding 4, so
that it doesn't need to be updated if MAX_DEVICE ever changes.

> +			  "Mention portnumbers in array.\n"

Missing space between "port" and "numbers". Don't mention that this is
an array here, "modinfo" already prints the parameter type. For improved
readability, please also add a leading space to the second and following
lines (as done for parameter "type" in i2c-parport.h.)

> +			  "If the port is not to be used mention -1.\n"
> +			  "Default is one instance connected to parport0\n");

Missing trailing dot.

> +
>  module_init(i2c_parport_init);
>  module_exit(i2c_parport_exit);

I tested this patch and it works fine.

Tested-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v5 WIP 3/5] i2c-parport: define ports to connect

[Sudip Mukherjee]

On Tue, May 19, 2015 at 09:50:47AM +0200, Jean Delvare wrote:
> Hi Sudip,
> 
> Le Wednesday 06 May 2015 à 15:46 +0530, Sudip Mukherjee a écrit :
> > as of now i2c-parport was connecting to all the available parallel
> > ports. Lets limit that to maximum of 4 instances and at the same time
> > define which instance connects to which parallel port
> 
> A leading capital and a trailing dot would look better.
sure.
> 
> > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > ---
<snip>
> > +	for (i = 0; i < MAX_DEVICE; i++) {
> > +		if (parport[i] == -1)
> > +			continue;
> > +		if (port->number == parport[i])
> > +			break;
> > +	}
> > +	if (i == MAX_DEVICE) {
> > +		pr_err("port mentioned not found\n");
> 
> This error message needs to be improved. Someone seeing this in his/her
> logs would have no idea where it comes from and what it means exactly.
> You want to add "i2c-parport: " at the beginning, and say which port
> number was specified.
oops. sorry about it. I saw all the other messages are having
"i2c-parport: " mentioned. I will modify it. And maybe later I will
send you another patch to use pr_fmt. Or here it may be better if I
mention:
pr_err("i2c-parport: You have chosen not to use parport%d.\n",
	port->number);

> 
> > +		return;
> > +	}
> >  
> >  	adapter = kzalloc(sizeof(struct i2c_par), GFP_KERNEL);
> >  	if (adapter == NULL) {
> > @@ -298,5 +313,11 @@ MODULE_AUTHOR("Jean Delvare <jdelvare@suse.de>");
> >  MODULE_DESCRIPTION("I2C bus over parallel port");
> >  MODULE_LICENSE("GPL");
> >  
> > +module_param_array(parport, int, NULL, 0);
> > +MODULE_PARM_DESC(parport, "Atmost 4 instances are allowed.\n"
> 
> You should first say what the parameter does, before going into the
> details. Please use __stringify(MAX_DEVICE) instead of hard-coding 4, so
> that it doesn't need to be updated if MAX_DEVICE ever changes.
then what about:
MODULE_PARM_DESC(parport, "Mention number of i2c-parport instances you
		 want.\n Atmost " __stringify(MAX_DEVICE) " instances are
		 allowed.\n Mention port numbers you want to use.\n"
		 " If the port is not to be used mention -1.\n"
		 " Default is one instance connected to parport0.\n");

> 
<snip>
> 
> I tested this patch and it works fine.
> 
> Tested-by: Jean Delvare <jdelvare@suse.de>

do i add your Tested-by: to the main patch and this patch?

regards
sudip	
> 
> -- 
> Jean Delvare
> SUSE L3 Support
> 

Re: [PATCH v5 WIP 3/5] i2c-parport: define ports to connect

[Jean Delvare]

Le Tuesday 19 May 2015 à 14:14 +0530, Sudip Mukherjee a écrit :
> On Tue, May 19, 2015 at 09:50:47AM +0200, Jean Delvare wrote:
> > Hi Sudip,
> > 
> > Le Wednesday 06 May 2015 à 15:46 +0530, Sudip Mukherjee a écrit :
> > > as of now i2c-parport was connecting to all the available parallel
> > > ports. Lets limit that to maximum of 4 instances and at the same time
> > > define which instance connects to which parallel port
> > 
> > A leading capital and a trailing dot would look better.
> sure.
> > 
> > > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > > ---
> <snip>
> > > +	for (i = 0; i < MAX_DEVICE; i++) {
> > > +		if (parport[i] == -1)
> > > +			continue;
> > > +		if (port->number == parport[i])
> > > +			break;
> > > +	}
> > > +	if (i == MAX_DEVICE) {
> > > +		pr_err("port mentioned not found\n");
> > 
> > This error message needs to be improved. Someone seeing this in his/her
> > logs would have no idea where it comes from and what it means exactly.
> > You want to add "i2c-parport: " at the beginning, and say which port
> > number was specified.
> oops. sorry about it. I saw all the other messages are having
> "i2c-parport: " mentioned. I will modify it. And maybe later I will
> send you another patch to use pr_fmt.

Using pr_fmt would be great indeed, but later as a separate patch.

> Or here it may be better if I
> mention:
> pr_err("i2c-parport: You have chosen not to use parport%d.\n",
> 	port->number);

Ah, now I realize that the original error message was misleading. In
fact you should not print any error message here (maybe only a debug
message if you really want.) There's nothing wrong with not binding to a
specific port, actually the whole point of this patch is to allow the
user to do exactly that.

> > 
> > > +		return;
> > > +	}
> > >  
> > >  	adapter = kzalloc(sizeof(struct i2c_par), GFP_KERNEL);
> > >  	if (adapter == NULL) {
> > > @@ -298,5 +313,11 @@ MODULE_AUTHOR("Jean Delvare <jdelvare@suse.de>");
> > >  MODULE_DESCRIPTION("I2C bus over parallel port");
> > >  MODULE_LICENSE("GPL");
> > >  
> > > +module_param_array(parport, int, NULL, 0);
> > > +MODULE_PARM_DESC(parport, "Atmost 4 instances are allowed.\n"
> > 
> > You should first say what the parameter does, before going into the
> > details. Please use __stringify(MAX_DEVICE) instead of hard-coding 4, so
> > that it doesn't need to be updated if MAX_DEVICE ever changes.
> then what about:
> MODULE_PARM_DESC(parport, "Mention number of i2c-parport instances you
> 		 want.\n Atmost " __stringify(MAX_DEVICE) " instances are
> 		 allowed.\n Mention port numbers you want to use.\n"
> 		 " If the port is not to be used mention -1.\n"
> 		 " Default is one instance connected to parport0.\n");

I suggest formatting this in such a way that "\n"s are at the end of
lines (as your patch had.) This makes this much more readable and
unexpected embedded line breaks and spaces as you have here (the output
of modinfo would look ugly.)

Also the initial description is still misleading, and there are too many
occurrences of "mention" to my taste. I'd suggest the following:

MODULE_PARM_DESC(parport,
	"List of parallel ports to bind to, by index.\n"
	" Atmost " __stringify(MAX_DEVICE) " devices are supported.\n"
	" Default is one device connected to parport0.\n"
);

I have removed the mention of -1 as this is an internal implementation
detail and I can't think of any reason why the user would ever want to
pass it.


> > 
> <snip>
> > 
> > I tested this patch and it works fine.
> > 
> > Tested-by: Jean Delvare <jdelvare@suse.de>
> 
> do i add your Tested-by: to the main patch and this patch?

Yes.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v5 WIP 3/5] i2c-parport: define ports to connect

[Sudip Mukherjee]

On Tue, May 19, 2015 at 11:28:15AM +0200, Jean Delvare wrote:
> Le Tuesday 19 May 2015 à 14:14 +0530, Sudip Mukherjee a écrit :
> > On Tue, May 19, 2015 at 09:50:47AM +0200, Jean Delvare wrote:
> > > Hi Sudip,
> > > 
> > > Le Wednesday 06 May 2015 à 15:46 +0530, Sudip Mukherjee a écrit :
<snip>
> > then what about:
> > MODULE_PARM_DESC(parport, "Mention number of i2c-parport instances you
> > 		 want.\n Atmost " __stringify(MAX_DEVICE) " instances are
> > 		 allowed.\n Mention port numbers you want to use.\n"
> > 		 " If the port is not to be used mention -1.\n"
> > 		 " Default is one instance connected to parport0.\n");
> 
> I suggest formatting this in such a way that "\n"s are at the end of
> lines (as your patch had.) This makes this much more readable and
> unexpected embedded line breaks and spaces as you have here (the output
> of modinfo would look ugly.)
> 
> Also the initial description is still misleading, and there are too many
> occurrences of "mention" to my taste. I'd suggest the following:
> 
> MODULE_PARM_DESC(parport,
> 	"List of parallel ports to bind to, by index.\n"
> 	" Atmost " __stringify(MAX_DEVICE) " devices are supported.\n"
> 	" Default is one device connected to parport0.\n"
> );
> 
> I have removed the mention of -1 as this is an internal implementation
> detail and I can't think of any reason why the user would ever want to
> pass it.
will use this.
> 
> 
> > > 
> > <snip>
> > > 
> > > I tested this patch and it works fine.
> > > 
> > > Tested-by: Jean Delvare <jdelvare@suse.de>
> > 
> > do i add your Tested-by: to the main patch and this patch?
> 
> Yes.

Thanks Jean.

regards
sudip
> 
> -- 
> Jean Delvare
> SUSE L3 Support
> 

Re: [PATCH v5 WIP 1/5] parport: add device-model to parport subsystem

[Dan Carpenter]

On Wed, May 06, 2015 at 03:46:13PM +0530, Sudip Mukherjee wrote:
> parport subsystem starts using the device-model. drivers using the
> device-model has to define probe and should register the device with
> parport with parport_register_dev_model()
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
> 
> v5:
> 	a) addition/removal of ports are now handled.
> 	b) is_parport moved to the core level
> 	c) common parport_driver_register code
> 	d) parport_register_dev_model now accepts instance number.
> 
> v4:	use of is_parport() is introduced to check the type of device
> 	that has been passed to probe or match_port.
> 
> v3:	started use of parport_del_port(). previously it was creating
> 	some ghost parallel ports during port probing.
> 	parport_del_port() removes registered ports if probing has
> 	failed.
> 
> v2:	started using probe function. Without probe,whenever any driver
> 	is trying to register, it is getting bound to all the available
> 	parallel ports. To solve that probe was required.
> 	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 v1 Greg mentioned that we do not need to maintain our own
> 	list. That has been partially done. we are no longer 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.
> 
>  drivers/parport/parport_pc.c |   4 +-
>  drivers/parport/procfs.c     |  15 +-
>  drivers/parport/share.c      | 335 +++++++++++++++++++++++++++++++++++++++----
>  include/linux/parport.h      |  43 +++++-
>  4 files changed, 368 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/parport/parport_pc.c b/drivers/parport/parport_pc.c
> index 53d15b3..78530d1 100644
> --- a/drivers/parport/parport_pc.c
> +++ b/drivers/parport/parport_pc.c
> @@ -2255,7 +2255,7 @@ out5:
>  		release_region(base+0x3, 5);
>  	release_region(base, 3);
>  out4:
> -	parport_put_port(p);
> +	parport_del_port(p);
>  out3:
>  	kfree(priv);
>  out2:
> @@ -2294,7 +2294,7 @@ void parport_pc_unregister_port(struct parport *p)
>  				    priv->dma_handle);
>  #endif
>  	kfree(p->private_data);
> -	parport_put_port(p);
> +	parport_del_port(p);
>  	kfree(ops); /* hope no-one cached it */
>  }
>  EXPORT_SYMBOL(parport_pc_unregister_port);
> 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..eb5d91d 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,13 +101,79 @@ static struct parport_operations dead_ops = {
>  	.owner		= NULL,
>  };
>  
> +static struct device_type parport_device_type = {
> +	.name = "parport",
> +};
> +
> +static int is_parport(struct device *dev)
> +{
> +	return dev->type == &parport_device_type;
> +}
> +
> +static int parport_probe(struct device *dev)
> +{
> +	struct parport_driver *drv;
> +
> +	if (is_parport(dev))
> +		return -ENODEV;

Is this test reversed?  You guys have tested probing though so it
doesn't seem like it can be reversed.

> +
> +	drv = to_parport_driver(dev->driver);
> +	if (!drv->probe)
> +		return -ENODEV;
> +
> +	return drv->probe(to_pardevice(dev));
> +}
> +
> +static struct bus_type parport_bus_type = {
> +	.name = "parport",
> +	.probe = parport_probe,
> +};
> +
> +int parport_bus_init(void)
> +{
> +	return bus_register(&parport_bus_type);
> +}
> +
> +void parport_bus_exit(void)
> +{
> +	bus_unregister(&parport_bus_type);
> +}
> +
> +static int driver_check(struct device_driver *_drv, void *_port)

_drv is a poor name.  _port is a good name because it's actually port,
but _drv should be dev_drv or something.  The port_check() function has
a nice comment explaining why it exists can you add one here?

> +{
> +	struct parport *port = _port;
> +	struct parport_driver *drv = to_parport_driver(_drv);
> +
> +	if (drv->match_port)
> +		drv->match_port(port);
> +	return 0;
> +}
> +
>  /* Call attach(port) for each registered driver. */
>  static void attach_driver_chain(struct parport *port)
>  {
>  	/* caller has exclusive registration_lock */
>  	struct parport_driver *drv;
> +
>  	list_for_each_entry(drv, &drivers, list)
>  		drv->attach(port);
> +
> +	/*
> +	 * call the port_check function of the drivers registered in

s/port_check/driver_check/

> +	 * new device model
> +	 */
> +
> +	bus_for_each_drv(&parport_bus_type, NULL, port, driver_check);
> +}
> +
> +static int driver_detach(struct device_driver *_drv, void *_port)
> +{
> +	struct parport *port = _port;
> +	struct parport_driver *drv = to_parport_driver(_drv);
> +
> +	if (drv->detach)
> +		drv->detach(port);
> +	return 0;
>  }
>  
>  /* Call detach(port) for each registered driver. */
> @@ -116,6 +183,13 @@ static void detach_driver_chain(struct parport *port)
>  	/* caller has exclusive registration_lock */
>  	list_for_each_entry(drv, &drivers, list)
>  		drv->detach (port);
> +
> +	/*
> +	 * call the detach function of the drivers registered in
> +	 * new device model
> +	 */
> +
> +	bus_for_each_drv(&parport_bus_type, NULL, port, driver_detach);
>  }
>  
>  /* Ask kmod for some lowlevel drivers. */
> @@ -126,17 +200,39 @@ static void get_lowlevel_driver (void)
>  	request_module ("parport_lowlevel");
>  }
>  
> +/*
> + * iterates through all the devices connected to the bus and sends the device
> + * details to the match_port 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 = _drv;
> +
> +	/* only send ports, do not send other devices connected to bus */
> +	if (is_parport(dev))
> +		drv->match_port(to_parport_dev(dev));
> +	return 0;
> +}
> +
>  /**
>   *	parport_register_driver - register a parallel port device driver
>   *	@drv: structure describing the driver
> + *	@owner: owner module of drv
> + *	@mod_name: module name string
>   *
>   *	This can be called by a parallel port device driver in order
>   *	to receive notifications about ports being found in the
>   *	system, as well as ports no longer available.
>   *
> + *	If the probe function is defined then the new device model is used
> + *	for registration.
> + *
>   *	The @drv structure is allocated by the caller and must not be
>   *	deallocated until after calling parport_unregister_driver().
>   *
> + *	If using the non device model:
>   *	The driver's attach() function may block.  The port that
>   *	attach() is given will be valid for the duration of the
>   *	callback, but if the driver wants to take a copy of the
> @@ -148,21 +244,58 @@ static void get_lowlevel_driver (void)
>   *	callback, but if the driver wants to take a copy of the
>   *	pointer it must call parport_get_port() to do so.
>   *
> - *	Returns 0 on success.  Currently it always succeeds.
> + *
> + *	Returns 0 on success. The non device model will always succeeds.
> + *	but the new device model can fail and will return the error code.
>   **/
>  
> -int parport_register_driver (struct parport_driver *drv)
> +int __parport_register_driver(struct parport_driver *drv, struct module *owner,
> +			      const char *mod_name)
>  {
> -	struct parport *port;
> -
>  	if (list_empty(&portlist))
>  		get_lowlevel_driver ();
>  
> -	mutex_lock(&registration_lock);
> -	list_for_each_entry(port, &portlist, list)
> -		drv->attach(port);
> -	list_add(&drv->list, &drivers);
> -	mutex_unlock(&registration_lock);
> +	if (drv->probe) {

I feel like this is weird.  The driver should just set ->devmodel
itself and we test for it here.

> +		/* using device model */
> +		int ret;
> +
> +		/* 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->devmodel = true;
> +		ret = driver_register(&drv->driver);
> +		if (ret)
> +			return ret;
> +
> +		mutex_lock(&registration_lock);
> +		if (drv->match_port)
> +			bus_for_each_dev(&parport_bus_type, NULL, drv,
> +					 port_check);

> +		mutex_unlock(&registration_lock);
> +	} else {
> +		struct parport *port;
> +
> +		drv->devmodel = false;
> +
> +		mutex_lock(&registration_lock);
> +		list_for_each_entry(port, &portlist, list)
> +			drv->attach(port);
> +		list_add(&drv->list, &drivers);
> +		mutex_unlock(&registration_lock);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(__parport_register_driver);
> +
> +static int port_detach(struct device *dev, void *_drv)
> +{
> +	struct parport_driver *drv = _drv;
> +
> +	if (is_parport(dev) && drv->detach)
> +		drv->detach(to_parport_dev(dev));
>  
>  	return 0;
>  }
> @@ -189,15 +322,22 @@ 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) {
> +		bus_for_each_dev(&parport_bus_type, NULL, drv, port_detach);
> +		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,9 +363,16 @@ 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->bus_dev);
> +
> +	return to_parport_dev(dev);
> +}
> +
> +void parport_del_port(struct parport *port)
> +{
> +	device_unregister(&port->bus_dev);
>  }
> +EXPORT_SYMBOL(parport_del_port);
>  
>  /**
>   *	parport_put_port - decrement a port's reference count
> @@ -237,11 +384,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->bus_dev);

I don't understand this.  The comment is out of date now.  Part of the
issue I'm having is that we need to support both old and new models for
now and I just get confused.  This seems like new model stuff but then
what happened to the old model stuff.

Also I seem to remember that the old model stuff was buggy here?  Is
that it?  If this is a bug fix then send that separately so we can
merge it right away.

Anyway, it's not your fault I'm confused.  But please, if it's a bugfix
send that by itself and also update the comment.

>  }
>  
>  /**
> @@ -281,6 +424,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 +477,10 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma,
>  	 */
>  	sprintf(name, "parport%d", tmp->portnum = tmp->number);
>  	tmp->name = name;
> +	tmp->bus_dev.bus = &parport_bus_type;
> +	tmp->bus_dev.release = free_port;
> +	dev_set_name(&tmp->bus_dev, name);
> +	tmp->bus_dev.type = &parport_device_type;
>  
>  	for (device = 0; device < 5; device++)
>  		/* assume the worst */
> @@ -340,6 +488,12 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma,
>  
>  	tmp->waithead = tmp->waittail = NULL;
>  
> +	ret = device_register(&tmp->bus_dev);
> +	if (ret) {
> +		put_device(&tmp->bus_dev);
> +		return NULL;
> +	}
> +
>  	return tmp;
>  }
>  
> @@ -575,6 +729,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 +785,136 @@ 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_model(struct parport *port, const char *name,
> +			   struct pardev_cb *par_dev_cb, int cnt)
> +{
> +	struct pardevice *par_dev;
> +	int ret;
> +	char *devname;
> +
> +	if (port->physport->flags & PARPORT_FLAG_EXCL) {
> +		/* An exclusive device is registered. */
> +		pr_err("%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;

Could you rename this to err_put_port?  Sorry, for being nit-picky but
naming labels after the goto location is a pet peeve of mine.

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

We have a par_dev->flags, par_dev->port->flags, and
par_dev->port->physport->flags.  The are slightly different.  Do we need
all three?

> +	par_dev->irq_func = par_dev_cb->irq_func;
> +	par_dev->waiting = 0;
> +	par_dev->timeout = 5 * HZ;
> +
> +	par_dev->dev.parent = &port->bus_dev;
> +	par_dev->dev.bus = &parport_bus_type;
> +	ret = dev_set_name(&par_dev->dev, "%s.%d", devname, cnt);
> +	if (ret)
> +		goto err_free_devname;
> +	par_dev->dev.release = free_pardevice;
> +	par_dev->devmodel = true;
> +	ret = device_register(&par_dev->dev);
> +	if (ret)
> +		goto 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_put_dev;
> +		}
> +		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_put_dev:
> +	put_device(&par_dev->dev);
> +err_free_devname:
> +	kfree(devname);
> +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_model);
> +
>  /**
>   *	parport_unregister_device - deregister a device on a parallel port
>   *	@dev: pointer to structure representing device
> @@ -691,7 +976,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 +1214,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;

What is this change for?

>  	return r;
>  }

regards,
dan carpenter

Re: [PATCH v5 WIP 2/5] staging: panel: use new parport device model

[Dan Carpenter]

On Wed, May 06, 2015 at 03:46:14PM +0530, Sudip Mukherjee wrote:
> converted to use the new device-model parallel port.
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
>  drivers/staging/panel/panel.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> index 1d8ed8b..772a82a 100644
> --- a/drivers/staging/panel/panel.c
> +++ b/drivers/staging/panel/panel.c
> @@ -2188,6 +2188,18 @@ static struct notifier_block panel_notifier = {
>  	0
>  };
>  
> +static int panel_probe(struct pardevice *par_dev)
> +{
> +	if (strcmp(par_dev->name, "panel"))
> +		return -ENODEV;
> +	return 0;
> +}
> +
> +struct pardev_cb panel_cb = {
> +	.flags = 0,     /*PARPORT_DEV_EXCL */

I don't understand this comment.  PARPORT_DEV_EXCL is BIT(2).  Update:
Oh, I see this comment is just copied from the earlier code and probably
means that it maybe should be set to PARPORT_DEV_EXCL.  Just put
something like this:

	.flags = 0,     /* should be PARPORT_DEV_EXCL? */

regards,
dan carpenter

Re: [PATCH v5 WIP 3/5] i2c-parport: define ports to connect

[Dan Carpenter]

On Wed, May 06, 2015 at 03:46:15PM +0530, Sudip Mukherjee wrote:
> as of now i2c-parport was connecting to all the available parallel
> ports. Lets limit that to maximum of 4 instances and at the same time
> define which instance connects to which parallel port
> 

So if you loaded this driver first then it was the only paraport driver
which could run?  That's unfortunate.

regards,
dan carpenter

Re: [PATCH v5 WIP 5/5] paride: use new parport device model

[Dan Carpenter]

On Wed, May 06, 2015 at 03:46:17PM +0530, Sudip Mukherjee wrote:
> +void *pi_register_driver(char *name)
> +{
> +	struct parport_driver *parp_drv;
> +	int ret;
> +
> +	parp_drv = kzalloc(sizeof(*parp_drv), GFP_KERNEL);
> +	if (!parp_drv)
> +		return NULL;
> +
> +	parp_drv->name = name;
> +	parp_drv->probe = pi_probe;
> +
> +	ret = parport_register_driver(parp_drv);
> +
> +	if (ret)
> +		return NULL;

Remove the blank line between the call and the error handling.  We
should probably free parp_drv on error.

> +	return (void *)parp_drv;
> +}
> +EXPORT_SYMBOL(pi_register_driver);
> +
> +void pi_unregister_driver(void *_drv)
> +{
> +	struct parport_driver *drv = _drv;
> +
> +	parport_unregister_driver(drv);
> +	kfree(drv);
> +}
> +EXPORT_SYMBOL(pi_unregister_driver);
> diff --git a/drivers/block/paride/paride.h b/drivers/block/paride/paride.h
> index 2bddbf4..ddb9e58 100644
> --- a/drivers/block/paride/paride.h
> +++ b/drivers/block/paride/paride.h
> @@ -165,6 +165,8 @@ typedef struct pi_protocol PIP;
>  
>  extern int paride_register( PIP * );
>  extern void paride_unregister ( PIP * );
> +void *pi_register_driver(char *);
> +void pi_unregister_driver(void *);
>  
>  #endif /* __DRIVERS_PARIDE_H__ */
>  /* end of paride.h */
> diff --git a/drivers/block/paride/pcd.c b/drivers/block/paride/pcd.c
> index 3b7c9f1..9336236 100644
> --- a/drivers/block/paride/pcd.c
> +++ b/drivers/block/paride/pcd.c
> @@ -221,6 +221,7 @@ static int pcd_busy;		/* request being processed ? */
>  static int pcd_sector;		/* address of next requested sector */
>  static int pcd_count;		/* number of blocks still to do */
>  static char *pcd_buf;		/* buffer for request in progress */
> +static void *par_drv;		/* reference of parport driver */
>  
>  /* kernel glue structures */
>  
> @@ -690,6 +691,12 @@ static int pcd_detect(void)
>  	printk("%s: %s version %s, major %d, nice %d\n",
>  	       name, name, PCD_VERSION, major, nice);
>  
> +	par_drv = pi_register_driver(name);
> +	if (!par_drv) {
> +		pr_err("failed to register %s driver\n", name);
> +		return -1;
> +	}
> +
>  	k = 0;
>  	if (pcd_drive_count == 0) { /* nothing spec'd - so autoprobe for 1 */
>  		cd = pcd;
> @@ -723,6 +730,7 @@ static int pcd_detect(void)
>  	printk("%s: No CD-ROM drive found\n", name);
>  	for (unit = 0, cd = pcd; unit < PCD_UNITS; unit++, cd++)
>  		put_disk(cd->disk);
> +	pi_unregister_driver(par_drv);

You didn't introduce this, but I think the error handling here is broken
for the autoprobe case.  The autoprobe case doesn't have a for loop.

>  	return -1;
>  }
>  

regards,
dan carpenter

Re: [PATCH v5 WIP 3/5] i2c-parport: define ports to connect

[Jean Delvare]

Hi Dan,

Le Tuesday 19 May 2015 à 14:23 +0300, Dan Carpenter a écrit :
> On Wed, May 06, 2015 at 03:46:15PM +0530, Sudip Mukherjee wrote:
> > as of now i2c-parport was connecting to all the available parallel
> > ports. Lets limit that to maximum of 4 instances and at the same time
> > define which instance connects to which parallel port
> 
> So if you loaded this driver first then it was the only paraport driver
> which could run?  That's unfortunate.

This was indeed the case. The i2c-parport driver was for testing
evaluation boards and other home-brew electronics, so had a small count
of users, and the driver would always be loaded last anyway, so it was
not a problem in practice. Still I agree with the change as it makes
things cleaner and more flexible.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v5 WIP 5/5] paride: use new parport device model

[Sudip Mukherjee]

On Tue, May 19, 2015 at 02:32:55PM +0300, Dan Carpenter wrote:
> On Wed, May 06, 2015 at 03:46:17PM +0530, Sudip Mukherjee wrote:
> > +void *pi_register_driver(char *name)
> > +{
> > +	struct parport_driver *parp_drv;
> > +	int ret;
> > +
> > +	parp_drv = kzalloc(sizeof(*parp_drv), GFP_KERNEL);
> > +	if (!parp_drv)
> > +		return NULL;
> > +
> > +	parp_drv->name = name;
> > +	parp_drv->probe = pi_probe;
> > +
> > +	ret = parport_register_driver(parp_drv);
> > +
> > +	if (ret)
> > +		return NULL;
> 
> Remove the blank line between the call and the error handling.  We
> should probably free parp_drv on error.
yes, i missed that. will add it in the patch.
> 
> > +	return (void *)parp_drv;
> > +}
<snip>
> >  	k = 0;
> >  	if (pcd_drive_count == 0) { /* nothing spec'd - so autoprobe for 1 */
> >  		cd = pcd;
> > @@ -723,6 +730,7 @@ static int pcd_detect(void)
> >  	printk("%s: No CD-ROM drive found\n", name);
> >  	for (unit = 0, cd = pcd; unit < PCD_UNITS; unit++, cd++)
> >  		put_disk(cd->disk);
> > +	pi_unregister_driver(par_drv);
> 
> You didn't introduce this, but I think the error handling here is broken
> for the autoprobe case.  The autoprobe case doesn't have a for loop.
ok, will fix it afterwards in a later patch. Alan has the hardware,
and I hope he will check that later patch also, like he tested this one.

Thanks again Alan.

regards
sudip

Re: [PATCH v5 WIP 1/5] parport: add device-model to parport subsystem

[Sudip Mukherjee]

On Tue, May 19, 2015 at 02:08:13PM +0300, Dan Carpenter wrote:
> On Wed, May 06, 2015 at 03:46:13PM +0530, Sudip Mukherjee wrote:
> > parport subsystem starts using the device-model. drivers using the
> > device-model has to define probe and should register the device with
> > parport with parport_register_dev_model()
> > 
> > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > ---
<snip>
> > +static int is_parport(struct device *dev)
> > +{
> > +	return dev->type == &parport_device_type;
> > +}
> > +
> > +static int parport_probe(struct device *dev)
> > +{
> > +	struct parport_driver *drv;
> > +
> > +	if (is_parport(dev))
> > +		return -ENODEV;
> 
> Is this test reversed?  You guys have tested probing though so it
> doesn't seem like it can be reversed.
reversed means? this test is to check if the device is a parport or
pardevice. if it is a parport then we should not send that to the driver
probe function.
> 
> > +
> > +	drv = to_parport_driver(dev->driver);
<snip>
> > +
> > +void parport_bus_exit(void)
> > +{
> > +	bus_unregister(&parport_bus_type);
> > +}
> > +
> > +static int driver_check(struct device_driver *_drv, void *_port)
> 
> _drv is a poor name.  _port is a good name because it's actually port,
> but _drv should be dev_drv or something.  The port_check() function has
> a nice comment explaining why it exists can you add one here?
you have suggested _port in an earlier version, so I have taken the idea
about _drv from that :)
will change it and add the comment also. anyhow I will also submit a
patch to add these details in the documentation also.
> 
> > +{
<snip>	
> > +	/*
> > +	 * call the port_check function of the drivers registered in
> 
> s/port_check/driver_check/
my mistake :(
> 
> > +	 * new device model
> > +	 */
> > +
> > -	list_for_each_entry(port, &portlist, list)
> > -		drv->attach(port);
> > -	list_add(&drv->list, &drivers);
> > -	mutex_unlock(&registration_lock);
> > +	if (drv->probe) {
> 
> I feel like this is weird.  The driver should just set ->devmodel
> itself and we test for it here.
well, devmodel and this test all will be removed when we totally remove
the old model. For now we just need a way to check if the driver is using
the old model or the new model. So anything is ok. If you want I will
use the ->devmodel in the driver itself.

> 
> > +		/* using device model */
> > +		int ret;
<snip>
> >  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->bus_dev);
> 
> I don't understand this.  The comment is out of date now.  Part of the
> issue I'm having is that we need to support both old and new models for
> now and I just get confused.  This seems like new model stuff but then
> what happened to the old model stuff.
> 
> Also I seem to remember that the old model stuff was buggy here?  Is
> that it?  If this is a bug fix then send that separately so we can
> merge it right away.
> 
> Anyway, it's not your fault I'm confused.  But please, if it's a bugfix
> send that by itself and also update the comment.
It is not a bugfix. parport is totally in new model. we can not have few
parport in old model and few in new model. So parport is registered in
the device model, we are only maintaining few of the old linked lists to
make the code compatible. drivers using new model will show up in the sys
tree and who are using the old model will not show.

> 
> >  }
> >  
<snip>	
> > +	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;
> 
> We have a par_dev->flags, par_dev->port->flags, and
> par_dev->port->physport->flags.  The are slightly different.  Do we need
> all three?
I have not touched these parts as all the drivers are still not converted.
after all drivers are converted then we will have a cleanup of this code,
and then we can see how to reduce or combine these flags.      
> 
> > +	par_dev->irq_func = par_dev_cb->irq_func;
<snip>
> > @@ -926,8 +1214,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;
> 
> What is this change for?
not required. parport_claim is returning only -EAGAIN or 0.
I will remove that.

regards
sudip
> 
> >  	return r;
> >  }
> 
> regards,
> dan carpenter

Re: [PATCH v5 WIP 1/5] parport: add device-model to parport subsystem

[Jean Delvare]

On Wed,  6 May 2015 15:46:13 +0530, Sudip Mukherjee wrote:
> parport subsystem starts using the device-model. drivers using the
> device-model has to define probe and should register the device with
> parport with parport_register_dev_model()
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---

Just one think I noticed while reviewing the i2c-parport patches:

> +struct pardevice *
> +parport_register_dev_model(struct parport *port, const char *name,
> +			   struct pardev_cb *par_dev_cb, int cnt)

par_dev_cb isn't modified by this function so it should be const.

Also I don't much like the name "cnt", as the value is not a count,
it's an index or identifier. What about "id"?

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v5 WIP 4/5] i2c-parport: use new parport device model

[Jean Delvare]

Hi Sudip,

On Wed,  6 May 2015 15:46:16 +0530, Sudip Mukherjee wrote:
> modify i2c-parport driver to use the new parallel port device model.

Leading capital please.

> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
>  drivers/i2c/busses/i2c-parport.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)

I like it very much. The simplicity of this patch is IMHO a good sign
that you are going in the right direction.

> 
> diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c
> index a9b25c3..6db5b45 100644
> --- a/drivers/i2c/busses/i2c-parport.c
> +++ b/drivers/i2c/busses/i2c-parport.c
> @@ -163,6 +163,11 @@ static void i2c_parport_irq(void *data)
>  			"SMBus alert received but no ARA client!\n");
>  }
>  
> +static struct pardev_cb i2c_parport_cb = {
> +	.flags = PARPORT_FLAG_EXCL,
> +	.irq_func = i2c_parport_irq,
> +};

There's no reason for this variable to be global. It is only needed
temporarily at attach time if I understand correctly, so it should be
local to function i2c_parport_attach().

> +
>  static void i2c_parport_attach(struct parport *port)
>  {
>  	struct i2c_par *adapter;
> @@ -184,11 +189,12 @@ static void i2c_parport_attach(struct parport *port)
>  		printk(KERN_ERR "i2c-parport: Failed to kzalloc\n");
>  		return;
>  	}
> +	i2c_parport_cb.private = adapter;
>  
>  	pr_debug("i2c-parport: attaching to %s\n", port->name);
>  	parport_disable_irq(port);
> -	adapter->pdev = parport_register_device(port, "i2c-parport",
> -		NULL, NULL, i2c_parport_irq, PARPORT_FLAG_EXCL, adapter);
> +	adapter->pdev = parport_register_dev_model(port, "i2c-parport",
> +						   &i2c_parport_cb, i);
>  	if (!adapter->pdev) {
>  		printk(KERN_ERR "i2c-parport: Unable to register with parport\n");
>  		goto err_free;
> @@ -281,10 +287,18 @@ static void i2c_parport_detach(struct parport *port)
>  	mutex_unlock(&adapter_list_lock);
>  }
>  
> +static int i2c_parport_probe(struct pardevice *par_dev)
> +{
> +	if (strcmp(par_dev->name, "i2c-parport"))
> +		return -ENODEV;
> +	return 0;
> +}

I'm wondering, is there any reason why this can't be automated by the
driver core part of the code? Most drivers will simply compare
drv->name with par_dev->name, which could be done in function
parport_probe() when no custom probe function is provided.

Now I see that you use the existence of the probe callback to decide
that the driver implements the device driver model. I suppose you could
use match_port instead, except that for some reason the paride driver
doesn't implement one. Maybe it should, or maybe you can check of the
presence of either to decide that this is a device model driver.

> +
>  static struct parport_driver i2c_parport_driver = {
>  	.name	= "i2c-parport",
> -	.attach	= i2c_parport_attach,
> +	.match_port = i2c_parport_attach,
>  	.detach	= i2c_parport_detach,
> +	.probe	= i2c_parport_probe,
>  };
>  
>  /* ----- Module loading, unloading and information ------------------------ */

Tested OK on my ADM1032 evaluation board.

Tested-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v5 WIP 5/5] paride: use new parport device model

[Jean Delvare]

Hi Sudip,

On Wed,  6 May 2015 15:46:17 +0530, Sudip Mukherjee wrote:
> modify paride driver to use the new parallel port device model.

Leading capital please ;-)

> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
> 
> in v4 of i2c-parport patch Jean mentioned to use the full name while
> in probe. That has been done in other drivers except this one.
> The higher layer drivers (pcd , pd etc.) are appending the unit number
> to the name before the name is being sent to the lower level (paride).

Mmmpf. If not all drivers can stick to the convention, this kind of
voids my original point.

Anyway... Again not a complete review, just two things which caught my
eye:

> -static int pi_register_parport(PIA * pi, int verbose)
> +static int pi_register_parport(PIA *pi, int verbose, int unit)
>  {
>  	struct parport *port;
> +	struct pardev_cb *par_cb;
>  
>  	port = parport_find_base(pi->port);
>  	if (!port)
>  		return 0;
> -
> -	pi->pardev = parport_register_device(port,
> -					     pi->device, NULL,
> -					     pi_wake_up, NULL, 0, (void *) pi);
> +	par_cb = kzalloc(sizeof(*par_cb), GFP_KERNEL);
> +	if (!par_cb) {
> +		parport_put_port(port);
> +		return 0;
> +	}
> +	par_cb->flags = 0;
> +	par_cb->wakeup = pi_wake_up;
> +	par_cb->private = (void *)pi;
> +	pi->pardev = parport_register_dev_model(port, pi->device, par_cb,
> +						unit);

If pi->device already includes the device number, and you are passing
it again as unit, won't you end up with odd names like pcd0.0, pcd1.1,
pcd2.2 etc?

>  	parport_put_port(port);
> +	kfree(par_cb);

If you don't need par_cb anywhere else then you shouldn't be allocating
it dynamically. It's small enough to fit on the stack really.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v5 WIP 4/5] i2c-parport: use new parport device model

[Sudip Mukherjee]

On Wed, May 20, 2015 at 09:57:24AM +0200, Jean Delvare wrote:
> Hi Sudip,
> 
> On Wed,  6 May 2015 15:46:16 +0530, Sudip Mukherjee wrote:
<snip>
> > +static struct pardev_cb i2c_parport_cb = {
> > +	.flags = PARPORT_FLAG_EXCL,
> > +	.irq_func = i2c_parport_irq,
> > +};
> 
> There's no reason for this variable to be global. It is only needed
> temporarily at attach time if I understand correctly, so it should be
> local to function i2c_parport_attach().
yes, this is only like a placeholder of the function pointers.
will make it local in the patch. (I guess WIP is over)
> 
> > +
<snip>
> > +static int i2c_parport_probe(struct pardevice *par_dev)
> > +{
> > +	if (strcmp(par_dev->name, "i2c-parport"))
> > +		return -ENODEV;
> > +	return 0;
> > +}
> 
> I'm wondering, is there any reason why this can't be automated by the
> driver core part of the code? Most drivers will simply compare
> drv->name with par_dev->name, which could be done in function
> parport_probe() when no custom probe function is provided.
yes, it can be done. I will add that in the patch.
> 
> Now I see that you use the existence of the probe callback to decide
> that the driver implements the device driver model. I suppose you could
> use match_port instead, except that for some reason the paride driver
> doesn't implement one. Maybe it should, or maybe you can check of the
> presence of either to decide that this is a device model driver.
as Dan suggested I am checking on devmodel now, instead of the existence
of probe, so what you are suggestiong now can definitely be done with
very small change.
> 
> > +
> >  static struct parport_driver i2c_parport_driver = {
> >  	.name	= "i2c-parport",
> > -	.attach	= i2c_parport_attach,
> > +	.match_port = i2c_parport_attach,
> >  	.detach	= i2c_parport_detach,
> > +	.probe	= i2c_parport_probe,
> >  };
> >  
> >  /* ----- Module loading, unloading and information ------------------------ */
> 
> Tested OK on my ADM1032 evaluation board.
> 
> Tested-by: Jean Delvare <jdelvare@suse.de>
Thanks again.

regards
sudip
> 
> -- 
> Jean Delvare
> SUSE L3 Support

Re: [PATCH v5 WIP 2/5] staging: panel: use new parport device model

[Jean Delvare]

Hi Sudip,

On Wed,  6 May 2015 15:46:14 +0530, Sudip Mukherjee wrote:
> converted to use the new device-model parallel port.

Hmm, leading capital? :-D

> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
>  drivers/staging/panel/panel.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> index 1d8ed8b..772a82a 100644
> --- a/drivers/staging/panel/panel.c
> +++ b/drivers/staging/panel/panel.c
> @@ -2188,6 +2188,18 @@ static struct notifier_block panel_notifier = {
>  	0
>  };
>  
> +static int panel_probe(struct pardevice *par_dev)
> +{
> +	if (strcmp(par_dev->name, "panel"))
> +		return -ENODEV;
> +	return 0;
> +}
> +
> +struct pardev_cb panel_cb = {

If this stays there, it should be static. And it could be const as you
never modify it. But as with i2c-parport I believe this variable should
be local to function panel_attach().

> +	.flags = 0,     /*PARPORT_DEV_EXCL */

0/NULL fields don't need to be explicitly initialized.

> +	.private = &pprt,
> +};
> +
>  static void panel_attach(struct parport *port)
>  {
>  	if (port->number != parport)
> @@ -2199,10 +2211,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_model(port, "panel", &panel_cb, 0);
>  	if (pprt == NULL) {
>  		pr_err("%s: port->number=%d parport=%d, parport_register_device() failed\n",
>  		       __func__, port->number, parport);
> @@ -2256,8 +2265,9 @@ static void panel_detach(struct parport *port)
>  
>  static struct parport_driver panel_driver = {
>  	.name = "panel",
> -	.attach = panel_attach,
> +	.match_port = panel_attach,
>  	.detach = panel_detach,
> +	.probe = panel_probe,
>  };
>  
>  /* init function */


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v5 WIP 5/5] paride: use new parport device model

[Sudip Mukherjee]

On Wed, May 20, 2015 at 10:07:48AM +0200, Jean Delvare wrote:
> Hi Sudip,
> 
> On Wed,  6 May 2015 15:46:17 +0530, Sudip Mukherjee wrote:
> > modify paride driver to use the new parallel port device model.
> 
> Leading capital please ;-)
yes, its the linux habit of writing in small letters. :)
> 
> > 
> > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > ---
> > 
> > +	}
> > +	par_cb->flags = 0;
> > +	par_cb->wakeup = pi_wake_up;
> > +	par_cb->private = (void *)pi;
> > +	pi->pardev = parport_register_dev_model(port, pi->device, par_cb,
> > +						unit);
> 
> If pi->device already includes the device number, and you are passing
> it again as unit, won't you end up with odd names like pcd0.0, pcd1.1,
> pcd2.2 etc?
yes, it is. This is the only driver using parport where in the probe
i had to use strncmp again.

> 
> >  	parport_put_port(port);
> > +	kfree(par_cb);
> 
> If you don't need par_cb anywhere else then you shouldn't be allocating
> it dynamically. It's small enough to fit on the stack really.
like you suggested, I will make it local in all the drivers.

regards
sudip.
> 
> -- 
> Jean Delvare
> SUSE L3 Support