[PATCH v4 WIP 1/4] parport: add device-model to parport subsystem

[PATCH v4 WIP 1/4] parport: add device-model to parport subsystem

[Sudip Mukherjee]

another WIP for your review. since this is not a formal patch for
applying so writing the comments here.

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.

v2 had one more problem: it was creating some ghost parallel ports
during port probing. from v3 we have the use of parport_del_port
to remove registerd ports if probing has failed.

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.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/parport/parport_pc.c |   4 +-
 drivers/parport/procfs.c     |  15 ++-
 drivers/parport/share.c      | 266 ++++++++++++++++++++++++++++++++++++++++---
 include/linux/parport.h      |  41 ++++++-
 4 files changed, 308 insertions(+), 18 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..8e2a418 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,
 };
 
+static 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,64 @@ 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 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;
+
+	drv->match_port(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);
+	if (drv->match_port)
+		bus_for_each_dev(&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 +263,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,10 +303,17 @@ 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
  *	@port: the port
@@ -237,13 +324,13 @@ 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);
 }
 
+static struct device_type parport_device_type = {
+	.name = "parport",
+};
+
 /**
  *	parport_register_port - register a parallel port
  *	@base: base I/O address
@@ -281,6 +368,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 +421,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,9 +432,21 @@ 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;
 }
 
+int is_parport(struct device *dev)
+{
+	return dev->type == &parport_device_type;
+}
+EXPORT_SYMBOL(is_parport);
+
 /**
  *	parport_announce_port - tell device drivers about a parallel port
  *	@port: parallel port to announce
@@ -575,6 +679,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 +735,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)
+{
+	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", devname);
+	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 +926,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 +1164,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..1e318ef 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,24 @@ 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 device *);
+	struct device_driver driver;
+	bool devmodel;
 	struct list_head list;
 };
 
+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 +288,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. */
@@ -288,6 +313,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 +335,11 @@ 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 is_parport(struct device *dev);
+
 /* parport_unregister unlinks a device from the chain. */
 extern void parport_unregister_device(struct pardevice *dev);
 
-- 
1.8.1.2

[PATCH v4 WIP 2/4] i2c-parport: modify driver to use new parport device model

[Sudip Mukherjee]

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

v4: according to the suggestion of Alan, array is being used in the
module parameter. Hopefully no one will use more than 4 instances.

Hi Jean,
This patchset is good for testing. Can you please check what is
happening to your hardware...

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/i2c/busses/i2c-parport.c | 95 ++++++++++++++++++++++++++++------------
 drivers/i2c/busses/i2c-parport.h |  7 +++
 2 files changed, 74 insertions(+), 28 deletions(-)

diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c
index a1fac5a..b07db1c 100644
--- a/drivers/i2c/busses/i2c-parport.c
+++ b/drivers/i2c/busses/i2c-parport.c
@@ -160,20 +160,49 @@ 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;
+	int i;
+	char *name;
+
+	if (!is_parport(&port->bus_dev))
+		return;
+
+	name = kzalloc(13, GFP_KERNEL);
+	if (!name)
+		return;
+
+	for (i = 0; i < 4; i++) {
+		if (parport[i] == -1)
+			continue;
+		if (port->number == parport[i])
+			break;
+	}
+	if (i == 4) {	/* port mentioned not found */
+		kfree(name);
+		return;
+	}
+	sprintf(name, "i2c-parport%d", i);
 
 	adapter = kzalloc(sizeof(struct i2c_par), GFP_KERNEL);
 	if (adapter == NULL) {
 		printk(KERN_ERR "i2c-parport: Failed to kzalloc\n");
+		kfree(name);
 		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, name,
+						   &i2c_parport_cb);
+	kfree(name);
 	if (!adapter->pdev) {
 		printk(KERN_ERR "i2c-parport: Unable to register with parport\n");
 		goto err_free;
@@ -237,39 +266,26 @@ static void i2c_parport_attach(struct parport *port)
 	parport_unregister_device(adapter->pdev);
  err_free:
 	kfree(adapter);
+	return;
 }
 
-static void i2c_parport_detach(struct parport *port)
+static int i2c_parport_probe(struct device *dev)
 {
-	struct i2c_par *adapter, *_n;
+	char *name = dev_name(dev);
 
-	/* Walk the list */
-	mutex_lock(&adapter_list_lock);
-	list_for_each_entry_safe(adapter, _n, &adapter_list, node) {
-		if (adapter->pdev->port == port) {
-			if (adapter->ara) {
-				parport_disable_irq(port);
-				i2c_unregister_device(adapter->ara);
-			}
-			i2c_del_adapter(&adapter->adapter);
-
-			/* Un-init if needed (power off...) */
-			if (adapter_parm[type].init.val)
-				line_set(port, 0, &adapter_parm[type].init);
-
-			parport_release(adapter->pdev);
-			parport_unregister_device(adapter->pdev);
-			list_del(&adapter->node);
-			kfree(adapter);
-		}
-	}
-	mutex_unlock(&adapter_list_lock);
+	if (is_parport(dev))
+		return -ENODEV;
+
+	if (strlen(name) != 12 || strncmp(dev_name(dev), "i2c-parport", 11))
+		return -ENODEV;
+
+	return 0;
 }
 
 static struct parport_driver i2c_parport_driver = {
 	.name	= "i2c-parport",
-	.attach	= i2c_parport_attach,
-	.detach	= i2c_parport_detach,
+	.match_port	= i2c_parport_attach,
+	.probe	= i2c_parport_probe,
 };
 
 /* ----- Module loading, unloading and information ------------------------ */
@@ -286,11 +302,34 @@ static int __init i2c_parport_init(void)
 		return -ENODEV;
 	}
 
-	return parport_register_driver(&i2c_parport_driver);
+	return parport_register_drv(&i2c_parport_driver);
 }
 
 static void __exit i2c_parport_exit(void)
 {
+	struct i2c_par *adapter, *_n;
+
+	/* Walk the list */
+	mutex_lock(&adapter_list_lock);
+	list_for_each_entry_safe(adapter, _n, &adapter_list, node) {
+		if (adapter->ara) {
+			parport_disable_irq(adapter->pdev->port);
+			i2c_unregister_device(adapter->ara);
+		}
+		i2c_del_adapter(&adapter->adapter);
+
+		/* Un-init if needed (power off...) */
+		if (adapter_parm[type].init.val)
+			line_set(adapter->pdev->port, 0,
+				 &adapter_parm[type].init);
+
+		parport_release(adapter->pdev);
+		parport_unregister_device(adapter->pdev);
+		list_del(&adapter->node);
+		kfree(adapter);
+	}
+	mutex_unlock(&adapter_list_lock);
+
 	parport_unregister_driver(&i2c_parport_driver);
 }
 
diff --git a/drivers/i2c/busses/i2c-parport.h b/drivers/i2c/busses/i2c-parport.h
index 4e12945..00655f4 100644
--- a/drivers/i2c/busses/i2c-parport.h
+++ b/drivers/i2c/busses/i2c-parport.h
@@ -104,3 +104,10 @@ MODULE_PARM_DESC(type,
 	" 6 = Barco LPT->DVI (K5800236) adapter\n"
 	" 7 = One For All JP1 parallel port adapter\n"
 );
+
+static int parport[4] = {0, -1, -1, -1};
+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");
-- 
1.8.1.2

Re: [PATCH v4 WIP 2/4] i2c-parport: modify driver to use new parport device model

[Jean Delvare]

Hi Sudip,

On Tue, 28 Apr 2015 17:00:21 +0530, Sudip Mukherjee wrote:
> modify i2c-parport driver to use the new parallel port device model.
> 
> v4: according to the suggestion of Alan, array is being used in the
> module parameter. Hopefully no one will use more than 4 instances.
> 
> Hi Jean,
> This patchset is good for testing. Can you please check what is
> happening to your hardware...

I don't have the time to review the first patch with the core changes,
but I can comment on this second patch as a driver author.

Testing report will come a bit later.

> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
>  drivers/i2c/busses/i2c-parport.c | 95 ++++++++++++++++++++++++++++------------
>  drivers/i2c/busses/i2c-parport.h |  7 +++
>  2 files changed, 74 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c
> index a1fac5a..b07db1c 100644
> --- a/drivers/i2c/busses/i2c-parport.c
> +++ b/drivers/i2c/busses/i2c-parport.c
> @@ -160,20 +160,49 @@ 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;
> +	int i;
> +	char *name;
> +
> +	if (!is_parport(&port->bus_dev))
> +		return;

Can this actually happen?

> +
> +	name = kzalloc(13, GFP_KERNEL);
> +	if (!name)
> +		return;
> +
> +	for (i = 0; i < 4; i++) {

Please introduce a define for the maximum number of devices supported,
and use it everywhere this is relevant.

> +		if (parport[i] == -1)
> +			continue;
> +		if (port->number == parport[i])
> +			break;
> +	}
> +	if (i == 4) {	/* port mentioned not found */

Shouldn't this be reported as an error to the user?

> +		kfree(name);
> +		return;

Please move the kfree to the error path at the end of the function, and
use goto to jump there, as is already done in the rest of the function.
Or maybe this is not needed at all, see below.

> +	}
> +	sprintf(name, "i2c-parport%d", i);

Seems weird, see below.

>  
>  	adapter = kzalloc(sizeof(struct i2c_par), GFP_KERNEL);
>  	if (adapter == NULL) {
>  		printk(KERN_ERR "i2c-parport: Failed to kzalloc\n");
> +		kfree(name);
>  		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, name,
> +						   &i2c_parport_cb);
> +	kfree(name);

If you can free "name" at this point, this suggests that
parport_register_dev_model made a copy somehow. In that case, please
don't use dynamic memory allocation in the first place. Either use a
static buffer and sprintf to it, or (probably better) pass the instance
number to parport_register_dev_model() as a separate parameter.

>  	if (!adapter->pdev) {
>  		printk(KERN_ERR "i2c-parport: Unable to register with parport\n");
>  		goto err_free;
> @@ -237,39 +266,26 @@ static void i2c_parport_attach(struct parport *port)
>  	parport_unregister_device(adapter->pdev);
>   err_free:
>  	kfree(adapter);
> +	return;

This return statement serves no purpose.

>  }
>  
> -static void i2c_parport_detach(struct parport *port)
> +static int i2c_parport_probe(struct device *dev)
>  {
> -	struct i2c_par *adapter, *_n;
> +	char *name = dev_name(dev);

This adds the following warning at build time:

  CC [M]  drivers/i2c/busses/i2c-parport.o
drivers/i2c/busses/i2c-parport.c: In function ‘i2c_parport_probe’:
drivers/i2c/busses/i2c-parport.c:274:15: warning: initialization discards ‘const’ qualifier from pointer target type [enabled by default]
  char *name = dev_name(dev);

Very easy to fix, just declare "name" as const char *.

>  
> -	/* Walk the list */
> -	mutex_lock(&adapter_list_lock);
> -	list_for_each_entry_safe(adapter, _n, &adapter_list, node) {
> -		if (adapter->pdev->port == port) {
> -			if (adapter->ara) {
> -				parport_disable_irq(port);
> -				i2c_unregister_device(adapter->ara);
> -			}
> -			i2c_del_adapter(&adapter->adapter);
> -
> -			/* Un-init if needed (power off...) */
> -			if (adapter_parm[type].init.val)
> -				line_set(port, 0, &adapter_parm[type].init);
> -
> -			parport_release(adapter->pdev);
> -			parport_unregister_device(adapter->pdev);
> -			list_del(&adapter->node);
> -			kfree(adapter);
> -		}
> -	}
> -	mutex_unlock(&adapter_list_lock);
> +	if (is_parport(dev))
> +		return -ENODEV;
> +
> +	if (strlen(name) != 12 || strncmp(dev_name(dev), "i2c-parport", 11))

You already have the result of dev_name(dev) as "name" so don't call it
again.

This piece of code looks awkward, I am not aware of any subsystem
working that way. Look at the platform device driver subsystem for
example, or even i2c, the match is done on the full name, and several
devices can have the same name. The way to distinguish between them is
with their address or a device instance number, NOT by appending a
number at the end of the name string.

> +		return -ENODEV;
> +
> +	return 0;
>  }
>  
>  static struct parport_driver i2c_parport_driver = {
>  	.name	= "i2c-parport",
> -	.attach	= i2c_parport_attach,
> -	.detach	= i2c_parport_detach,
> +	.match_port	= i2c_parport_attach,
> +	.probe	= i2c_parport_probe,
>  };

The lack of detach function in the new model is suspicious, more on
that below.

>  
>  /* ----- Module loading, unloading and information ------------------------ */
> @@ -286,11 +302,34 @@ static int __init i2c_parport_init(void)
>  		return -ENODEV;
>  	}
>  
> -	return parport_register_driver(&i2c_parport_driver);
> +	return parport_register_drv(&i2c_parport_driver);

Can't you call parport_register_driver() for both models and decide
what to do based on which members of struct parport_driver are set?
This would be less confusing IMHO.

>  }
>  
>  static void __exit i2c_parport_exit(void)
>  {
> +	struct i2c_par *adapter, *_n;
> +
> +	/* Walk the list */
> +	mutex_lock(&adapter_list_lock);
> +	list_for_each_entry_safe(adapter, _n, &adapter_list, node) {
> +		if (adapter->ara) {
> +			parport_disable_irq(adapter->pdev->port);
> +			i2c_unregister_device(adapter->ara);
> +		}
> +		i2c_del_adapter(&adapter->adapter);
> +
> +		/* Un-init if needed (power off...) */
> +		if (adapter_parm[type].init.val)
> +			line_set(adapter->pdev->port, 0,
> +				 &adapter_parm[type].init);
> +
> +		parport_release(adapter->pdev);
> +		parport_unregister_device(adapter->pdev);
> +		list_del(&adapter->node);
> +		kfree(adapter);
> +	}
> +	mutex_unlock(&adapter_list_lock);

Moving all this code here seems inappropriate. What if a parallel port
is removed from the system while the i2c-parport driver is loaded? I
think this can happen with laptop docking stations or parallel ports on
PCI cards for example. Your new model should be able to deal with that,
so each driver still needs a detach or remove function which the core
can call on parallel port removal.

> +
>  	parport_unregister_driver(&i2c_parport_driver);
>  }
>  
> diff --git a/drivers/i2c/busses/i2c-parport.h b/drivers/i2c/busses/i2c-parport.h
> index 4e12945..00655f4 100644
> --- a/drivers/i2c/busses/i2c-parport.h
> +++ b/drivers/i2c/busses/i2c-parport.h
> @@ -104,3 +104,10 @@ MODULE_PARM_DESC(type,
>  	" 6 = Barco LPT->DVI (K5800236) adapter\n"
>  	" 7 = One For All JP1 parallel port adapter\n"
>  );
> +
> +static int parport[4] = {0, -1, -1, -1};
> +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");

This should go in i2c-parport.c, not i2c-parport.h. i2c-parport.h is
shared between the i2c-parport and i2c-parport-light drivers, and this
parameter is irrelevant for the latter.

Ideally the support for multiple instances and the conversion to the
new parport device model should also be added in separate patches.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v4 WIP 2/4] i2c-parport: modify driver to use new parport device model

[Sudip Mukherjee]

On Sun, May 03, 2015 at 03:33:40PM +0200, Jean Delvare wrote:
> Hi Sudip,
Thanks Jean for your time.	
> 
> On Tue, 28 Apr 2015 17:00:21 +0530, Sudip Mukherjee wrote:
<snip>
> > +	if (!is_parport(&port->bus_dev))
> > +		return;
> 
> Can this actually happen?
i got this idea from i2c_verify_client(), as I was getting problem with
different devices registered with the parallel port. But, anyways, i
guess I will not need it in the next iteration of the WIP and can be
handled in the core level.
> 
<snip>
> > +	adapter->pdev = parport_register_dev_model(port, name,
> > +						   &i2c_parport_cb);
> > +	kfree(name);
> 
> If you can free "name" at this point, this suggests that
> parport_register_dev_model made a copy somehow. In that case, please
> don't use dynamic memory allocation in the first place. Either use a
> static buffer and sprintf to it, or (probably better) pass the instance
> number to parport_register_dev_model() as a separate parameter.
well, first thought - there will be some drivers who will not have multiple
instances. but second thought - if we have separately device name and
instance number, we can just combine them when registering with the device
model, but it will become easier in the probe for the name comparison which
you have pointed out later in your reply.
I will try it out in the next iteration.
> 
> >  	if (!adapter->pdev) {
> >  		printk(KERN_ERR "i2c-parport: Unable to register with parport\n");
> >  		goto err_free;
> > @@ -237,39 +266,26 @@ static void i2c_parport_attach(struct parport *port)
> >  	parport_unregister_device(adapter->pdev);
> >   err_free:
> >  	kfree(adapter);
> > +	return;
> 
> This return statement serves no purpose.
sorry, it is a leftover from an idea I was trying,
> 
> >  }
> >  
> > -static void i2c_parport_detach(struct parport *port)
> > +static int i2c_parport_probe(struct device *dev)
> >  {
> > -	struct i2c_par *adapter, *_n;
> > +	char *name = dev_name(dev);
> 
> This adds the following warning at build time:
> 
>   CC [M]  drivers/i2c/busses/i2c-parport.o
> drivers/i2c/busses/i2c-parport.c: In function ‘i2c_parport_probe’:
> drivers/i2c/busses/i2c-parport.c:274:15: warning: initialization discards ‘const’ qualifier from pointer target type [enabled by default]
>   char *name = dev_name(dev);
> 
> Very easy to fix, just declare "name" as const char *.
I didnot get this warning, maybe I need to upgrade my gcc or will
W=1 show it?
> 
> >  
> > -	/* Walk the list */
<snip>
> >  
> > -	return parport_register_driver(&i2c_parport_driver);
> > +	return parport_register_drv(&i2c_parport_driver);
> 
> Can't you call parport_register_driver() for both models and decide
> what to do based on which members of struct parport_driver are set?
> This would be less confusing IMHO.
I guess it can be done. let me try it out.
> 
> >  }
> >  
<snip>
> > +	}
> > +	mutex_unlock(&adapter_list_lock);
> 
> Moving all this code here seems inappropriate. What if a parallel port
> is removed from the system while the i2c-parport driver is loaded? I
> think this can happen with laptop docking stations or parallel ports on
> PCI cards for example. Your new model should be able to deal with that,
> so each driver still needs a detach or remove function which the core
> can call on parallel port removal.
ok, will be done.
To be frank, I am actually confused with this part, not only for parport
subsystem but for all the other codes I have seen. We have a remove
function for all subsystems, lets assume PCI, so a pci driver is having
a remove callback. So if the particular pci device is removed then the
remove is called which does all the clearing part. But the driver still
remains registered, then what happens to the driver?
> 
my next WIP will have some changes in the core level also, so I shouldnot
add your Tested-by: to it. And I will again request you to check that.
And since Alan has not yet tested it on his backpack cd driver, so
please do not test this series. I will send in the next version in a day
or two. Please test that.

regards
sudip

> 
> -- 
> Jean Delvare
> SUSE L3 Support

Re: [PATCH v4 WIP 2/4] i2c-parport: modify driver to use new parport device model

[Jean Delvare]

On Mon, 4 May 2015 11:10:12 +0530, Sudip Mukherjee wrote:
> On Sun, May 03, 2015 at 03:33:40PM +0200, Jean Delvare wrote:
> > On Tue, 28 Apr 2015 17:00:21 +0530, Sudip Mukherjee wrote:
> > > +	adapter->pdev = parport_register_dev_model(port, name,
> > > +						   &i2c_parport_cb);
> > > +	kfree(name);
> > 
> > If you can free "name" at this point, this suggests that
> > parport_register_dev_model made a copy somehow. In that case, please
> > don't use dynamic memory allocation in the first place. Either use a
> > static buffer and sprintf to it, or (probably better) pass the instance
> > number to parport_register_dev_model() as a separate parameter.
> well, first thought - there will be some drivers who will not have multiple
> instances.

Please check how the platform bus handles this. You can see the device
names under /sys/bus/platform/devices, some end with .%d and some
don't. The code handling this is in
drivers/base/platform.c:platform_device_add(). That being said, I can't
think of a reason why any specific parallel port device could not have
several instances (contrary to some platform devices which can only
have a single instance by nature.) So even if a driver does not support
multiple instances today, it would make sense to stick to %s.%d as the
bus id pattern for all devices.

> but second thought - if we have separately device name and
> instance number, we can just combine them when registering with the device
> model, but it will become easier in the probe for the name comparison which
> you have pointed out later in your reply.

This was indeed one of the reasons for my suggestion. The other is that
identifiers need to be unique for a given bus type, and leaving the
responsibility of this to individual device drivers is risky. They
could get it wrong, or accidentally step on each other's toes. And even
if they do it right, they are likely to do it inconsistently, which is
never good.

> > > (...)
> > > -static void i2c_parport_detach(struct parport *port)
> > > +static int i2c_parport_probe(struct device *dev)
> > >  {
> > > -	struct i2c_par *adapter, *_n;
> > > +	char *name = dev_name(dev);
> > 
> > This adds the following warning at build time:
> > 
> >   CC [M]  drivers/i2c/busses/i2c-parport.o
> > drivers/i2c/busses/i2c-parport.c: In function ‘i2c_parport_probe’:
> > drivers/i2c/busses/i2c-parport.c:274:15: warning: initialization discards ‘const’ qualifier from pointer target type [enabled by default]
> >   char *name = dev_name(dev);
> > 
> > Very easy to fix, just declare "name" as const char *.
> I didnot get this warning, maybe I need to upgrade my gcc or will
> W=1 show it?

I see it without W=1, as the message says this type of warning is
enabled by default (gcc 4.8.1.) The check on const vs. non-const
pointers is very old so I am very surprised that you don't see it, even
if your gcc isn't recent.

> > 
> > >  
> > > -	/* Walk the list */
> <snip>
> > >  
> > > -	return parport_register_driver(&i2c_parport_driver);
> > > +	return parport_register_drv(&i2c_parport_driver);
> > 
> > Can't you call parport_register_driver() for both models and decide
> > what to do based on which members of struct parport_driver are set?
> > This would be less confusing IMHO.
> I guess it can be done. let me try it out.
> > 
> > >  }
> > >  
> <snip>
> > > +	}
> > > +	mutex_unlock(&adapter_list_lock);
> > 
> > Moving all this code here seems inappropriate. What if a parallel port
> > is removed from the system while the i2c-parport driver is loaded? I
> > think this can happen with laptop docking stations or parallel ports on
> > PCI cards for example. Your new model should be able to deal with that,
> > so each driver still needs a detach or remove function which the core
> > can call on parallel port removal.
> ok, will be done.
> To be frank, I am actually confused with this part, not only for parport
> subsystem but for all the other codes I have seen. We have a remove
> function for all subsystems, lets assume PCI, so a pci driver is having
> a remove callback. So if the particular pci device is removed then the
> remove is called which does all the clearing part. But the driver still
> remains registered, then what happens to the driver?

This was a key design decision of the (then) new device driver model in
kernel v2.5 that the lifetime of drivers should be independent from
the lifetime of device instances. Ideally, devices are even created
and deleted outside their driver. That works well for enumerated
devices such as PCI or USB devices. That won't work in your case
because parallel port devices have no unique ID so they can't be
enumerated.

Still, the lifetime of devices should be independent from the lifetime
of the driver. The driver should be registered as long as the module is
loaded. The devices, however, must be created and deleted dynamically
whenever the relevant parallel ports appear or disappear from the
system.

So basically the module's __init function should only register the
driver, and let the core call back its probe function for every parallel
port available on the system at that time. And likewise, the __exit
function should only unregister the driver, and let the core call back
its remove function for every device that still exists at that point in
time. This is what the platform bus subsystem does (see for example
drivers/i2c/busses/i2c-viperboard.c but there are many many others.)

> my next WIP will have some changes in the core level also, so I shouldnot
> add your Tested-by: to it. And I will again request you to check that.

I will do, no problem.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v4 WIP 2/4] i2c-parport: modify driver to use new parport device model

[Sudip Mukherjee]

On Mon, May 04, 2015 at 08:58:58AM +0200, Jean Delvare wrote:
> On Mon, 4 May 2015 11:10:12 +0530, Sudip Mukherjee wrote:
> > On Sun, May 03, 2015 at 03:33:40PM +0200, Jean Delvare wrote:
> > > On Tue, 28 Apr 2015 17:00:21 +0530, Sudip Mukherjee wrote:
<snip>
> > I didnot get this warning, maybe I need to upgrade my gcc or will
> > W=1 show it?
> 
> I see it without W=1, as the message says this type of warning is
> enabled by default (gcc 4.8.1.) The check on const vs. non-const
> pointers is very old so I am very surprised that you don't see it, even
> if your gcc isn't recent.
then maybe I have not looked closely. I will pay attention and look for
it this time. My version is 4.7.3.
> 
<snip>
> > remains registered, then what happens to the driver?
> 
> This was a key design decision of the (then) new device driver model in
> kernel v2.5 that the lifetime of drivers should be independent from
> the lifetime of device instances. Ideally, devices are even created
> and deleted outside their driver. That works well for enumerated
> devices such as PCI or USB devices. That won't work in your case
> because parallel port devices have no unique ID so they can't be
> enumerated.
> 
> Still, the lifetime of devices should be independent from the lifetime
> of the driver. The driver should be registered as long as the module is
> loaded. The devices, however, must be created and deleted dynamically
> whenever the relevant parallel ports appear or disappear from the
> system.
> 
> So basically the module's __init function should only register the
> driver, and let the core call back its probe function for every parallel
> port available on the system at that time. And likewise, the __exit
> function should only unregister the driver, and let the core call back
> its remove function for every device that still exists at that point in
> time. This is what the platform bus subsystem does (see for example
> drivers/i2c/busses/i2c-viperboard.c but there are many many others.)
Thanks. This explained many of the doubts I was having. And I have one
more doubt and I need some suggestion for it too.
This current version of the code will register devices like :
If i register i2c-parport0 with parport0 then the sys tree will be:
		sys
	 ________|____________
		| 
	     parport
	 _____|_______    
         |           |
     parport0   i2c-parport0
	|
   i2c-parport0

so basically it registers as a subdevice of parport0 and also a device
in the bus. And this is the reason why i needed the device_type.
But i think it is wrong. I think it should have been just:
		sys
	 ________|____________
		| 
	     parport
	 _____|_______    
	      |          
	 parport0   
            |
      i2c-parport0

so, which one is actually correct?

Thanks again for that explanation, that really helped a lot.

regards
sudip


> 
> > my next WIP will have some changes in the core level also, so I shouldnot
> > add your Tested-by: to it. And I will again request you to check that.
> 
> I will do, no problem.
> 
> -- 
> Jean Delvare
> SUSE L3 Support

Re: [PATCH v4 WIP 2/4] i2c-parport: modify driver to use new parport device model

[Jean Delvare]

Le Monday 04 May 2015 à 12:54 +0530, Sudip Mukherjee a écrit :
> Thanks. This explained many of the doubts I was having. And I have one
> more doubt and I need some suggestion for it too.
> This current version of the code will register devices like :
> If i register i2c-parport0 with parport0 then the sys tree will be:
> 		sys
> 	 ________|____________
> 		| 
> 	     parport
> 	 _____|_______    
>          |           |
>      parport0   i2c-parport0
> 	|
>    i2c-parport0
> 
> so basically it registers as a subdevice of parport0 and also a device
> in the bus. And this is the reason why i needed the device_type.
> But i think it is wrong. I think it should have been just:
> 		sys
> 	 ________|____________
> 		| 
> 	     parport
> 	 _____|_______    
> 	      |          
> 	 parport0   
>             |
>       i2c-parport0
> 
> so, which one is actually correct?

This was a surprise for me at first too, but the former is correct.
i2c-parport0 appears below parport0 as a directory because it is a child
of it. And both parport0 and i2c-parport0 appear as _links_
under /sys/bus/parport/devices because they are respectively a provider
device and a consumer device of the parport bus type.

As a note, i2c-parport.0 might be a better name (bus ID, actually), for
several reasons. It makes things more readable if a device name ends
with digits (not the case here, but could happen, this is rather
frequent.) It looks more like platform device names, and consistency is
always nice to have. And it avoids the confusion between "parport0"
where 0 is the parallel port number and i2c-"parport0" where 0 is the
instance number of the i2c-parport device - it may be connected to
parport0 but it's only by chance and it may well be connected to another
parallel port.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v4 WIP 2/4] i2c-parport: modify driver to use new parport device model

[Jean Delvare]

On Tue, 28 Apr 2015 17:00:21 +0530, Sudip Mukherjee wrote:
> modify i2c-parport driver to use the new parallel port device model.
> 
> v4: according to the suggestion of Alan, array is being used in the
> module parameter. Hopefully no one will use more than 4 instances.
> 
> Hi Jean,
> This patchset is good for testing. Can you please check what is
> happening to your hardware...
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
>  drivers/i2c/busses/i2c-parport.c | 95 ++++++++++++++++++++++++++++------------
>  drivers/i2c/busses/i2c-parport.h |  7 +++
>  2 files changed, 74 insertions(+), 28 deletions(-)
> (...)

Functional testing with a single device (ADM1032 eval board) went fine.
I didn't test interrupts but as you didn't touch that part of the code
I don't expect any specific problem.

-- 
Jean Delvare
SUSE L3 Support

[PATCH v4 WIP 3/4] paride: modify driver to use new parport device model

[Sudip Mukherjee]

v4: converted paride to use the device model.

Hi Alan,
This patchset is good for testing. I am not finding any issues, but
pcd and paride was just build and tested via debug messages.

While testing i noticed that the protocol module is not automatically
loading. so for your testing i guess you will need to load the bpck6.ko
first and then pcd.ko

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 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, 101 insertions(+), 6 deletions(-)

diff --git a/drivers/block/paride/paride.c b/drivers/block/paride/paride.c
index 48c50f1..56af32f 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"
 
@@ -247,15 +248,22 @@ EXPORT_SYMBOL(paride_unregister);
 static int pi_register_parport(PIA * pi, int verbose)
 {
 	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);
 	parport_put_port(port);
+	kfree(par_cb);
 	if (!pi->pardev)
 		return 0;
 
@@ -366,7 +374,6 @@ int pi_init(PIA * pi, int autoprobe, int port, int mode,
 		printk("%s: Invalid parameters\n", device);
 		return 0;
 	}
-
 	for (p = s; p < e; p++) {
 		struct pi_protocol *proto = protocols[p];
 		if (!proto)
@@ -432,3 +439,47 @@ int pi_init(PIA * pi, int autoprobe, int port, int mode,
 }
 
 EXPORT_SYMBOL(pi_init);
+
+static int pi_probe(struct device *dev)
+{
+	struct device_driver *drv = dev->driver;
+	int len;
+
+	if (is_parport(dev))
+		return -ENODEV;
+
+	len = strlen(drv->name);
+	if (strncmp(dev_name(dev), 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_drv(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 d48715b..d79c490 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

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

[Sudip Mukherjee]

converted to use the new device-model parallel port.
I have the lcd part of the hardware and has been tested.

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

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index ea54fb4..2e24cd5 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_model(port, "panel", &panel_cb);
 	if (pprt == NULL) {
 		pr_err("%s: port->number=%d parport=%d, parport_register_device() failed\n",
 		       __func__, port->number, parport);
@@ -2238,40 +2248,14 @@ err_lcd_unreg:
 err_unreg_device:
 	parport_unregister_device(pprt);
 	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;
+	return;
 }
 
 static struct parport_driver panel_driver = {
 	.name = "panel",
-	.attach = panel_attach,
-	.detach = panel_detach,
+	.match_port = panel_attach,
+	.probe = panel_probe,
 };
 
 /* init function */
@@ -2380,7 +2364,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 +2382,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 v4 WIP 1/4] parport: add device-model to parport subsystem

[Jean Delvare]

Hi Sudip,

On Tue, 28 Apr 2015 17:00:20 +0530, Sudip Mukherjee wrote:
> another WIP for your review. since this is not a formal patch for
> applying so writing the comments here.

You should still provide a proper description as if the patch was ready
to be committed. Ultimately the descriptions are going to be part of
the commits, so they need to be reviewed too.

The history is good to have too for now, but it should go after the
"---" separator, as it won't be part of the commit.

> 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.
> 
> v2 had one more problem: it was creating some ghost parallel ports
> during port probing. from v3 we have the use of parport_del_port
> to remove registerd ports if probing has failed.

Spelling: "registered".

(As pointed out by ./scripts/checkpatch.pl - did you run it on each
patch?)

> 
> 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.
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
>  drivers/parport/parport_pc.c |   4 +-
>  drivers/parport/procfs.c     |  15 ++-
>  drivers/parport/share.c      | 266 ++++++++++++++++++++++++++++++++++++++++---
>  include/linux/parport.h      |  41 ++++++-
>  4 files changed, 308 insertions(+), 18 deletions(-)
> (...)

Patch tested, no functional regression found.

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

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v4 WIP 1/4] parport: add device-model to parport subsystem

[Sudip Mukherjee]

On Sat, May 02, 2015 at 04:20:53PM +0200, Jean Delvare wrote:
> Hi Sudip,
> 
> On Tue, 28 Apr 2015 17:00:20 +0530, Sudip Mukherjee wrote:
> > another WIP for your review. since this is not a formal patch for
> > applying so writing the comments here.
> 
> You should still provide a proper description as if the patch was ready
> to be committed. Ultimately the descriptions are going to be part of
> the commits, so they need to be reviewed too.
> 
> The history is good to have too for now, but it should go after the
> "---" separator, as it won't be part of the commit.
should i then send a v5 of WIP with proper commit message?
I will mention the WIP history as comments in my formal patch also.
And I guess, formal patch will take some time. After Alan has tested
I need to work on the documentation also.
> 
> > v4: use of is_parport() is introduced to check the type of device that
> > has been passed to probe or match_port.
> > 
<snip>
> > 
> > v2 had one more problem: it was creating some ghost parallel ports
> > during port probing. from v3 we have the use of parport_del_port
> > to remove registerd ports if probing has failed.
> 
> Spelling: "registered".
> 
> (As pointed out by ./scripts/checkpatch.pl - did you run it on each
> patch?)
while working on the code I will be checking with:
git diff | scripts/checkpatch.pl --strict -
so the change in the code is properly checkpatch tested.
and for formal submission of patches I will check again after writing
the commit message. But since this was just a WIP and not a formal
patch submission so I have not checked after writing the comments.

> 
> > 
<snip>
> > 
> > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > ---
> >  drivers/parport/parport_pc.c |   4 +-
> >  drivers/parport/procfs.c     |  15 ++-
> >  drivers/parport/share.c      | 266 ++++++++++++++++++++++++++++++++++++++++---
> >  include/linux/parport.h      |  41 ++++++-
> >  4 files changed, 308 insertions(+), 18 deletions(-)
> > (...)
> 
> Patch tested, no functional regression found.
> 
> Tested-by: Jean Delvare <jdelvare@suse.de>
Thanks Jean.
Should i add your Tested-by: to the main patch and the patch
concerning the changes to i2c-parport?

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

Re: [PATCH v4 WIP 1/4] parport: add device-model to parport subsystem

[Jean Delvare]

Hi Sudip,

On Sun, 3 May 2015 12:07:33 +0530, Sudip Mukherjee wrote:
> On Sat, May 02, 2015 at 04:20:53PM +0200, Jean Delvare wrote:
> > On Tue, 28 Apr 2015 17:00:20 +0530, Sudip Mukherjee wrote:
> > > another WIP for your review. since this is not a formal patch for
> > > applying so writing the comments here.
> > 
> > You should still provide a proper description as if the patch was ready
> > to be committed. Ultimately the descriptions are going to be part of
> > the commits, so they need to be reviewed too.
> > 
> > The history is good to have too for now, but it should go after the
> > "---" separator, as it won't be part of the commit.
> should i then send a v5 of WIP with proper commit message?

Do not resend just for this. But please do it for the next iteration.

> > > (...)
> > > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > > ---
> > >  drivers/parport/parport_pc.c |   4 +-
> > >  drivers/parport/procfs.c     |  15 ++-
> > >  drivers/parport/share.c      | 266 ++++++++++++++++++++++++++++++++++++++++---
> > >  include/linux/parport.h      |  41 ++++++-
> > >  4 files changed, 308 insertions(+), 18 deletions(-)
> > > (...)
> > 
> > Patch tested, no functional regression found.
> > 
> > Tested-by: Jean Delvare <jdelvare@suse.de>
> Thanks Jean.
> Should i add your Tested-by: to the main patch and the patch
> concerning the changes to i2c-parport?

I have only tested the main patch at this point, I'll test the
i2c-parport changes later today and report then.

-- 
Jean Delvare
SUSE L3 Support