[PATCH 0/4] convert parport to device-model

[PATCH 0/4] convert parport to device-model

[Sudip Mukherjee]

As suggested by Greg, new functions were introduced in the parport
subsystem which will use the device-model and we convert the drivers
one by one. So accordingly we now have parport_register_drv(), 
parport_register_dev() and attach_ret() which are using the device-model.
And we have a flag to indicate if the driver has registered using this
new api, and based on that flag other functions are behaving.

The registration of ports is using the device-model and we now will have
/sys/bus/parport and as and when new ports are discovered they will be
added to the tree.

The drivers after converting to the device-model will register their
device as an subdevice under the relevant port it is using and when they
claim the port the actual binding between the device and the driver will
take place.

Greg also suggested to use a probe function in the drivers which will
return 0 for automatic binding, but since we do not have specific
deviceid for many parallel port devices so I was not sure what the
logic will be in the probe to identify and match the device. So instead
I thought of using device_attach() for the actual binding.

As of now only two drivers have been converted, i2c-parport which
Jean says he can test, and staging/panel which i can test as I have
the hardware for LCD part.

The zip-driver is stil not converted, but still cc-ing
gnomes@lxorguk.ukuu.org.uk if he can please check the zip drive to know
if the changes are not breaking the drivers which have not yet been
converted.

I am also in the process of procuring a parallel port printer so that
the code can properly be maintained and can be marked as Maintained.

Sudip Mukherjee (4):
  parport: modify parport subsystem to use devicemodel
  parport: update TODO and documentation
  i2c-parport: use device-model parport
  staging: panel: use parport in device-model

 Documentation/parport-lowlevel.txt |  49 ++++++++
 drivers/i2c/busses/i2c-parport.c   |  19 +--
 drivers/parport/TODO-parport       |   4 +
 drivers/parport/procfs.c           |  15 ++-
 drivers/parport/share.c            | 236 +++++++++++++++++++++++++++++++++++--
 drivers/staging/panel/panel.c      |  20 ++--
 include/linux/parport.h            |  29 ++++-
 7 files changed, 343 insertions(+), 29 deletions(-)

-- 
1.8.1.2

[PATCH 0/4] convert parport to device-model

[Sudip Mukherjee]

As suggested by Greg, new functions were introduced in the parport
subsystem which will use the device-model and we convert the drivers
one by one. So accordingly we now have parport_register_drv(), 
parport_register_dev() and attach_ret() which are using the device-model.
And we have a flag to indicate if the driver has registered using this
new api, and based on that flag other functions are behaving.

The registration of ports is using the device-model and we now will have
/sys/bus/parport and as and when new ports are discovered they will be
added to the tree.

The drivers after converting to the device-model will register their
device as an subdevice under the relevant port it is using and when they
claim the port the actual binding between the device and the driver will
take place.

Greg also suggested to use a probe function in the drivers which will
return 0 for automatic binding, but since we do not have specific
deviceid for many parallel port devices so I was not sure what the
logic will be in the probe to identify and match the device. So instead
I thought of using device_attach() for the actual binding.

As of now only two drivers have been converted, i2c-parport which
Jean says he can test, and staging/panel which i can test as I have
the hardware for LCD part.

The zip-driver is stil not converted, but still cc-ing
gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org if he can please check the zip drive to know
if the changes are not breaking the drivers which have not yet been
converted.

I am also in the process of procuring a parallel port printer so that
the code can properly be maintained and can be marked as Maintained.

Sudip Mukherjee (4):
  parport: modify parport subsystem to use devicemodel
  parport: update TODO and documentation
  i2c-parport: use device-model parport
  staging: panel: use parport in device-model

 Documentation/parport-lowlevel.txt |  49 ++++++++
 drivers/i2c/busses/i2c-parport.c   |  19 +--
 drivers/parport/TODO-parport       |   4 +
 drivers/parport/procfs.c           |  15 ++-
 drivers/parport/share.c            | 236 +++++++++++++++++++++++++++++++++++--
 drivers/staging/panel/panel.c      |  20 ++--
 include/linux/parport.h            |  29 ++++-
 7 files changed, 343 insertions(+), 29 deletions(-)

-- 
1.8.1.2

[PATCH 1/4] parport: modify parport subsystem to use devicemodel

[Sudip Mukherjee]

parport starts using device-model and we now have parport under
/sys/bus. As the ports are discovered they are added as device under
/sys/bus/parport. As and when other drivers register new device,
they will be registered as a subdevice under the relevant parport.

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

diff --git a/drivers/parport/procfs.c b/drivers/parport/procfs.c
index 3b47080..1ce363b 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 = bus_register(&parport_bus_type);
+	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;
 	}
+	bus_unregister(&parport_bus_type);
 }
 
 #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 bus_register(&parport_bus_type);
 }
 
 static void __exit parport_default_proc_unregister (void)
 {
+	bus_unregister(&parport_bus_type);
 }
 #endif
 
diff --git a/drivers/parport/share.c b/drivers/parport/share.c
index 3fa6624..452b2c0 100644
--- a/drivers/parport/share.c
+++ b/drivers/parport/share.c
@@ -10,6 +10,8 @@
  * based on work by Grant Guenther <grant@torque.net>
  *          and Philip Blundell
  *
+ * Added Device-Model - Sudip Mukherjee <sudip@vectorindia.org>
+ *
  * Any part of this program may be used in documents licensed under
  * the GNU Free Documentation License, Version 1.1 or any later version
  * published by the Free Software Foundation.
@@ -29,6 +31,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 +103,11 @@ static struct parport_operations dead_ops = {
 	.owner		= NULL,
 };
 
+struct bus_type parport_bus_type = {
+	.name           = "parport",
+};
+EXPORT_SYMBOL(parport_bus_type);
+
 /* Call attach(port) for each registered driver. */
 static void attach_driver_chain(struct parport *port)
 {
@@ -157,6 +165,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 +176,57 @@ int parport_register_driver (struct parport_driver *drv)
 	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)
+{
+	struct parport *port;
+	int ret, err = 0;
+	bool attached = false;
+
+	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->devmodel = true;
+	ret = driver_register(&drv->driver);
+	if (ret)
+		return ret;
+
+	mutex_lock(&registration_lock);
+	list_for_each_entry(port, &portlist, list) {
+		ret = drv->attach_ret(port, drv);
+		if (ret == 0)
+			attached = true;
+		else
+			err = ret;
+	}
+	if (attached)
+		list_add(&drv->list, &drivers);
+	mutex_unlock(&registration_lock);
+	if (!attached) {
+		driver_unregister(&drv->driver);
+		return err;
+	}
+
+	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
@@ -193,11 +253,15 @@ void parport_unregister_driver (struct parport_driver *drv)
 	list_for_each_entry(port, &portlist, list)
 		drv->detach(port);
 	mutex_unlock(&registration_lock);
+	if (drv->devmodel)
+		driver_unregister(&drv->driver);
 }
 
-static void free_port (struct parport *port)
+static void free_port(struct device *dev)
 {
 	int d;
+	struct parport *port = to_parport_dev(dev);
+
 	spin_lock(&full_list_lock);
 	list_del(&port->full_list);
 	spin_unlock(&full_list_lock);
@@ -223,8 +287,9 @@ static void free_port (struct parport *port)
 
 struct parport *parport_get_port (struct parport *port)
 {
-	atomic_inc (&port->ref_count);
-	return port;
+	struct device *dev = get_device(&port->ddev);
+
+	return to_parport_dev(dev);
 }
 
 /**
@@ -237,11 +302,7 @@ struct parport *parport_get_port (struct parport *port)
 
 void parport_put_port (struct parport *port)
 {
-	if (atomic_dec_and_test (&port->ref_count))
-		/* Can destroy it now. */
-		free_port (port);
-
-	return;
+	put_device(&port->ddev);
 }
 
 /**
@@ -281,6 +342,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 +395,9 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma,
 	 */
 	sprintf(name, "parport%d", tmp->portnum = tmp->number);
 	tmp->name = name;
+	tmp->ddev.bus = &parport_bus_type;
+	tmp->ddev.release = free_port;
+	dev_set_name(&tmp->ddev, name);
 
 	for (device = 0; device < 5; device++)
 		/* assume the worst */
@@ -340,6 +405,13 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma,
 
 	tmp->waithead = tmp->waittail = NULL;
 
+	ret = device_register(&tmp->ddev);
+	if (ret) {
+		list_del(&tmp->full_list);
+		kfree(tmp);
+		return NULL;
+	}
+
 	return tmp;
 }
 
@@ -575,6 +647,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 +703,133 @@ parport_register_device(struct parport *port, const char *name,
 	return NULL;
 }
 
+void free_pardevice(struct device *dev)
+{
+}
+
+struct pardevice *
+parport_register_dev(struct parport *port, const char *name,
+		     int (*pf)(void *), void (*kf)(void *),
+		     void (*irq_func)(void *), int flags,
+		     void *handle, struct parport_driver *drv)
+{
+	struct pardevice *tmp;
+	int ret;
+	char *devname;
+
+	if (port->physport->flags & PARPORT_FLAG_EXCL) {
+		/* An exclusive device is registered. */
+		pr_debug("%s: no more devices allowed\n",
+			 port->name);
+		return NULL;
+	}
+
+	if (flags & PARPORT_DEV_LURK) {
+		if (!pf || !kf) {
+			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);
+
+	tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
+	if (!tmp) {
+		pr_warn("%s: memory squeeze, couldn't register %s.\n",
+			port->name, name);
+		goto out;
+	}
+
+	tmp->state = kmalloc(sizeof(*tmp->state), GFP_KERNEL);
+	if (!tmp->state) {
+		pr_warn("%s: memory squeeze, couldn't register %s.\n",
+			port->name, name);
+		goto out_free_pardevice;
+	}
+
+	tmp->name = name;
+	tmp->port = port;
+	tmp->daisy = -1;
+	tmp->preempt = pf;
+	tmp->wakeup = kf;
+	tmp->private = handle;
+	tmp->flags = flags;
+	tmp->irq_func = irq_func;
+	tmp->waiting = 0;
+	tmp->timeout = 5 * HZ;
+
+	tmp->dev.parent = &port->ddev;
+	devname = kstrdup(name, GFP_KERNEL);
+	dev_set_name(&tmp->dev, "%s", name);
+	tmp->dev.driver = &drv->driver;
+	tmp->dev.release = free_pardevice;
+	tmp->devmodel = true;
+	ret = device_register(&tmp->dev);
+	if (ret)
+		goto out_free_all;
+
+	/* Chain this onto the list */
+	tmp->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 (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 out_free_dev;
+		}
+		port->flags |= PARPORT_FLAG_EXCL;
+	}
+
+	tmp->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 = tmp;
+	port->physport->devices = tmp;
+	spin_unlock(&port->physport->pardevice_lock);
+
+	init_waitqueue_head(&tmp->wait_q);
+	tmp->timeslice = parport_default_timeslice;
+	tmp->waitnext = NULL;
+	tmp->waitprev = NULL;
+
+	/*
+	 * This has to be run as last thing since init_state may need other
+	 * pardevice fields. -arca
+	 */
+	port->ops->init_state(tmp, tmp->state);
+	if (!test_and_set_bit(PARPORT_DEVPROC_REGISTERED, &port->devflags)) {
+		port->proc_device = tmp;
+		parport_device_proc_register(tmp);
+	}
+
+	return tmp;
+out_free_dev:
+	put_device(&tmp->dev);
+out_free_all:
+	kfree(tmp->state);
+out_free_pardevice:
+	kfree(tmp);
+out:
+	parport_put_port(port);
+	module_put(port->ops->owner);
+
+	return NULL;
+}
+
 /**
  *	parport_unregister_device - deregister a device on a parallel port
  *	@dev: pointer to structure representing device
@@ -691,7 +891,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);
@@ -774,6 +977,7 @@ int parport_claim(struct pardevice *dev)
 	struct pardevice *oldcad;
 	struct parport *port = dev->port->physport;
 	unsigned long flags;
+	int ret;
 
 	if (port->cad == dev) {
 		printk(KERN_INFO "%s: %s already owner\n",
@@ -802,6 +1006,13 @@ int parport_claim(struct pardevice *dev)
 		}
 	}
 
+	if (dev->devmodel) {
+		ret = device_attach(&dev->dev);
+		if (ret != 1) {
+			return -ENODEV;
+		}
+	}
+
 	/* Can't fail from now on, so mark ourselves as no longer waiting.  */
 	if (dev->waiting & 1) {
 		dev->waiting = 0;
@@ -926,8 +1137,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;
 }
 
@@ -954,6 +1165,8 @@ void parport_release(struct pardevice *dev)
 		       "when not owner\n", port->name, dev->name);
 		return;
 	}
+	if (dev->devmodel)
+		device_release_driver(&dev->dev);
 
 #ifdef CONFIG_PARPORT_1284
 	/* If this is on a mux port, deselect it. */
@@ -1022,6 +1235,7 @@ EXPORT_SYMBOL(parport_remove_port);
 EXPORT_SYMBOL(parport_register_driver);
 EXPORT_SYMBOL(parport_unregister_driver);
 EXPORT_SYMBOL(parport_register_device);
+EXPORT_SYMBOL(parport_register_dev);
 EXPORT_SYMBOL(parport_unregister_device);
 EXPORT_SYMBOL(parport_get_port);
 EXPORT_SYMBOL(parport_put_port);
diff --git a/include/linux/parport.h b/include/linux/parport.h
index c22f125..61b4e4e 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;
@@ -195,7 +198,7 @@ struct parport {
 				 * This may unfortulately be null if the
 				 * port has a legacy driver.
 				 */
-
+	struct device ddev;	/* to link with the bus */
 	struct parport *physport;
 				/* If this is a non-default mux
 				   parport, i.e. we're a clone of a real
@@ -245,15 +248,22 @@ struct parport {
 	struct parport *slaves[3];
 };
 
+#define to_parport_dev(n) container_of(n, struct parport, ddev)
+
 #define DEFAULT_SPIN_TIME 500 /* us */
 
 struct parport_driver {
 	const char *name;
 	void (*attach) (struct parport *);
 	void (*detach) (struct parport *);
+	int (*attach_ret)(struct parport *, struct parport_driver *);
+	struct device_driver driver;
+	bool devmodel;
 	struct list_head list;
 };
 
+extern struct bus_type parport_bus_type;
+
 /* 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 +284,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. */
@@ -301,6 +322,12 @@ struct pardevice *parport_register_device(struct parport *port,
 			  void (*irq_func)(void *), 
 			  int flags, void *handle);
 
+struct pardevice *
+parport_register_dev(struct parport *port, const char *name,
+		     int (*pf)(void *), void (*kf)(void *),
+		     void (*irq_func)(void *), int flags,
+		     void *handle, struct parport_driver *drv);
+
 /* parport_unregister unlinks a device from the chain. */
 extern void parport_unregister_device(struct pardevice *dev);
 
-- 
1.8.1.2

[PATCH 1/4] parport: modify parport subsystem to use devicemodel

[Sudip Mukherjee]

parport starts using device-model and we now have parport under
/sys/bus. As the ports are discovered they are added as device under
/sys/bus/parport. As and when other drivers register new device,
they will be registered as a subdevice under the relevant parport.

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

diff --git a/drivers/parport/procfs.c b/drivers/parport/procfs.c
index 3b47080..1ce363b 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 = bus_register(&parport_bus_type);
+	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;
 	}
+	bus_unregister(&parport_bus_type);
 }
 
 #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 bus_register(&parport_bus_type);
 }
 
 static void __exit parport_default_proc_unregister (void)
 {
+	bus_unregister(&parport_bus_type);
 }
 #endif
 
diff --git a/drivers/parport/share.c b/drivers/parport/share.c
index 3fa6624..452b2c0 100644
--- a/drivers/parport/share.c
+++ b/drivers/parport/share.c
@@ -10,6 +10,8 @@
  * based on work by Grant Guenther <grant@torque.net>
  *          and Philip Blundell
  *
+ * Added Device-Model - Sudip Mukherjee <sudip@vectorindia.org>
+ *
  * Any part of this program may be used in documents licensed under
  * the GNU Free Documentation License, Version 1.1 or any later version
  * published by the Free Software Foundation.
@@ -29,6 +31,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 +103,11 @@ static struct parport_operations dead_ops = {
 	.owner		= NULL,
 };
 
+struct bus_type parport_bus_type = {
+	.name           = "parport",
+};
+EXPORT_SYMBOL(parport_bus_type);
+
 /* Call attach(port) for each registered driver. */
 static void attach_driver_chain(struct parport *port)
 {
@@ -157,6 +165,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 +176,57 @@ int parport_register_driver (struct parport_driver *drv)
 	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)
+{
+	struct parport *port;
+	int ret, err = 0;
+	bool attached = false;
+
+	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->devmodel = true;
+	ret = driver_register(&drv->driver);
+	if (ret)
+		return ret;
+
+	mutex_lock(&registration_lock);
+	list_for_each_entry(port, &portlist, list) {
+		ret = drv->attach_ret(port, drv);
+		if (ret == 0)
+			attached = true;
+		else
+			err = ret;
+	}
+	if (attached)
+		list_add(&drv->list, &drivers);
+	mutex_unlock(&registration_lock);
+	if (!attached) {
+		driver_unregister(&drv->driver);
+		return err;
+	}
+
+	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
@@ -193,11 +253,15 @@ void parport_unregister_driver (struct parport_driver *drv)
 	list_for_each_entry(port, &portlist, list)
 		drv->detach(port);
 	mutex_unlock(&registration_lock);
+	if (drv->devmodel)
+		driver_unregister(&drv->driver);
 }
 
-static void free_port (struct parport *port)
+static void free_port(struct device *dev)
 {
 	int d;
+	struct parport *port = to_parport_dev(dev);
+
 	spin_lock(&full_list_lock);
 	list_del(&port->full_list);
 	spin_unlock(&full_list_lock);
@@ -223,8 +287,9 @@ static void free_port (struct parport *port)
 
 struct parport *parport_get_port (struct parport *port)
 {
-	atomic_inc (&port->ref_count);
-	return port;
+	struct device *dev = get_device(&port->ddev);
+
+	return to_parport_dev(dev);
 }
 
 /**
@@ -237,11 +302,7 @@ struct parport *parport_get_port (struct parport *port)
 
 void parport_put_port (struct parport *port)
 {
-	if (atomic_dec_and_test (&port->ref_count))
-		/* Can destroy it now. */
-		free_port (port);
-
-	return;
+	put_device(&port->ddev);
 }
 
 /**
@@ -281,6 +342,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 +395,9 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma,
 	 */
 	sprintf(name, "parport%d", tmp->portnum = tmp->number);
 	tmp->name = name;
+	tmp->ddev.bus = &parport_bus_type;
+	tmp->ddev.release = free_port;
+	dev_set_name(&tmp->ddev, name);
 
 	for (device = 0; device < 5; device++)
 		/* assume the worst */
@@ -340,6 +405,13 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma,
 
 	tmp->waithead = tmp->waittail = NULL;
 
+	ret = device_register(&tmp->ddev);
+	if (ret) {
+		list_del(&tmp->full_list);
+		kfree(tmp);
+		return NULL;
+	}
+
 	return tmp;
 }
 
@@ -575,6 +647,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 +703,133 @@ parport_register_device(struct parport *port, const char *name,
 	return NULL;
 }
 
+void free_pardevice(struct device *dev)
+{
+}
+
+struct pardevice *
+parport_register_dev(struct parport *port, const char *name,
+		     int (*pf)(void *), void (*kf)(void *),
+		     void (*irq_func)(void *), int flags,
+		     void *handle, struct parport_driver *drv)
+{
+	struct pardevice *tmp;
+	int ret;
+	char *devname;
+
+	if (port->physport->flags & PARPORT_FLAG_EXCL) {
+		/* An exclusive device is registered. */
+		pr_debug("%s: no more devices allowed\n",
+			 port->name);
+		return NULL;
+	}
+
+	if (flags & PARPORT_DEV_LURK) {
+		if (!pf || !kf) {
+			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);
+
+	tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
+	if (!tmp) {
+		pr_warn("%s: memory squeeze, couldn't register %s.\n",
+			port->name, name);
+		goto out;
+	}
+
+	tmp->state = kmalloc(sizeof(*tmp->state), GFP_KERNEL);
+	if (!tmp->state) {
+		pr_warn("%s: memory squeeze, couldn't register %s.\n",
+			port->name, name);
+		goto out_free_pardevice;
+	}
+
+	tmp->name = name;
+	tmp->port = port;
+	tmp->daisy = -1;
+	tmp->preempt = pf;
+	tmp->wakeup = kf;
+	tmp->private = handle;
+	tmp->flags = flags;
+	tmp->irq_func = irq_func;
+	tmp->waiting = 0;
+	tmp->timeout = 5 * HZ;
+
+	tmp->dev.parent = &port->ddev;
+	devname = kstrdup(name, GFP_KERNEL);
+	dev_set_name(&tmp->dev, "%s", name);
+	tmp->dev.driver = &drv->driver;
+	tmp->dev.release = free_pardevice;
+	tmp->devmodel = true;
+	ret = device_register(&tmp->dev);
+	if (ret)
+		goto out_free_all;
+
+	/* Chain this onto the list */
+	tmp->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 (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 out_free_dev;
+		}
+		port->flags |= PARPORT_FLAG_EXCL;
+	}
+
+	tmp->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 = tmp;
+	port->physport->devices = tmp;
+	spin_unlock(&port->physport->pardevice_lock);
+
+	init_waitqueue_head(&tmp->wait_q);
+	tmp->timeslice = parport_default_timeslice;
+	tmp->waitnext = NULL;
+	tmp->waitprev = NULL;
+
+	/*
+	 * This has to be run as last thing since init_state may need other
+	 * pardevice fields. -arca
+	 */
+	port->ops->init_state(tmp, tmp->state);
+	if (!test_and_set_bit(PARPORT_DEVPROC_REGISTERED, &port->devflags)) {
+		port->proc_device = tmp;
+		parport_device_proc_register(tmp);
+	}
+
+	return tmp;
+out_free_dev:
+	put_device(&tmp->dev);
+out_free_all:
+	kfree(tmp->state);
+out_free_pardevice:
+	kfree(tmp);
+out:
+	parport_put_port(port);
+	module_put(port->ops->owner);
+
+	return NULL;
+}
+
 /**
  *	parport_unregister_device - deregister a device on a parallel port
  *	@dev: pointer to structure representing device
@@ -691,7 +891,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);
@@ -774,6 +977,7 @@ int parport_claim(struct pardevice *dev)
 	struct pardevice *oldcad;
 	struct parport *port = dev->port->physport;
 	unsigned long flags;
+	int ret;
 
 	if (port->cad == dev) {
 		printk(KERN_INFO "%s: %s already owner\n",
@@ -802,6 +1006,13 @@ int parport_claim(struct pardevice *dev)
 		}
 	}
 
+	if (dev->devmodel) {
+		ret = device_attach(&dev->dev);
+		if (ret != 1) {
+			return -ENODEV;
+		}
+	}
+
 	/* Can't fail from now on, so mark ourselves as no longer waiting.  */
 	if (dev->waiting & 1) {
 		dev->waiting = 0;
@@ -926,8 +1137,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;
 }
 
@@ -954,6 +1165,8 @@ void parport_release(struct pardevice *dev)
 		       "when not owner\n", port->name, dev->name);
 		return;
 	}
+	if (dev->devmodel)
+		device_release_driver(&dev->dev);
 
 #ifdef CONFIG_PARPORT_1284
 	/* If this is on a mux port, deselect it. */
@@ -1022,6 +1235,7 @@ EXPORT_SYMBOL(parport_remove_port);
 EXPORT_SYMBOL(parport_register_driver);
 EXPORT_SYMBOL(parport_unregister_driver);
 EXPORT_SYMBOL(parport_register_device);
+EXPORT_SYMBOL(parport_register_dev);
 EXPORT_SYMBOL(parport_unregister_device);
 EXPORT_SYMBOL(parport_get_port);
 EXPORT_SYMBOL(parport_put_port);
diff --git a/include/linux/parport.h b/include/linux/parport.h
index c22f125..61b4e4e 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;
@@ -195,7 +198,7 @@ struct parport {
 				 * This may unfortulately be null if the
 				 * port has a legacy driver.
 				 */
-
+	struct device ddev;	/* to link with the bus */
 	struct parport *physport;
 				/* If this is a non-default mux
 				   parport, i.e. we're a clone of a real
@@ -245,15 +248,22 @@ struct parport {
 	struct parport *slaves[3];
 };
 
+#define to_parport_dev(n) container_of(n, struct parport, ddev)
+
 #define DEFAULT_SPIN_TIME 500 /* us */
 
 struct parport_driver {
 	const char *name;
 	void (*attach) (struct parport *);
 	void (*detach) (struct parport *);
+	int (*attach_ret)(struct parport *, struct parport_driver *);
+	struct device_driver driver;
+	bool devmodel;
 	struct list_head list;
 };
 
+extern struct bus_type parport_bus_type;
+
 /* 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 +284,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. */
@@ -301,6 +322,12 @@ struct pardevice *parport_register_device(struct parport *port,
 			  void (*irq_func)(void *), 
 			  int flags, void *handle);
 
+struct pardevice *
+parport_register_dev(struct parport *port, const char *name,
+		     int (*pf)(void *), void (*kf)(void *),
+		     void (*irq_func)(void *), int flags,
+		     void *handle, struct parport_driver *drv);
+
 /* parport_unregister unlinks a device from the chain. */
 extern void parport_unregister_device(struct pardevice *dev);
 
-- 
1.8.1.2

[PATCH 2/4] parport: update TODO and documentation

[Sudip Mukherjee]

as parport starts using the device-model, update the documentation
to show the newly added functions and update TODO with some other
planned modifications.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 Documentation/parport-lowlevel.txt | 49 ++++++++++++++++++++++++++++++++++++++
 drivers/parport/TODO-parport       |  4 ++++
 2 files changed, 53 insertions(+)

diff --git a/Documentation/parport-lowlevel.txt b/Documentation/parport-lowlevel.txt
index 120eb20..4c180af 100644
--- a/Documentation/parport-lowlevel.txt
+++ b/Documentation/parport-lowlevel.txt
@@ -7,9 +7,11 @@ Described here are the following functions:
 
 Global functions:
   parport_register_driver
+  parport_register_drv
   parport_unregister_driver
   parport_enumerate
   parport_register_device
+  parport_register_dev
   parport_unregister_device
   parport_claim
   parport_claim_or_block
@@ -222,6 +224,26 @@ SEE ALSO
 
 parport_unregister_driver, parport_register_device, parport_enumerate
 \f
+
+parport_register_drv - register a device driver with parport
+--------------------
+
+Everything is same as parport_register_driver, but it will use the device-model
+and will register the driver under /sys/bus/parport/driver. Just like
+parport_register_driver it will also call the attach callback function but the
+differences being attach wil now return its success or error code and the
+reference of the driver is also passed to the attach so that it can be used
+when calling parport_register_dev.
+
+static int lp_attach (struct parport *port, struct parport_driver *drv)
+{
+	...
+	private = kmalloc (...);
+	dev[count++] = parport_register_dev (...);
+	...
+}
+
+
 parport_unregister_driver - tell parport to forget about this driver
 -------------------------
 
@@ -442,6 +464,33 @@ SEE ALSO
 
 parport_unregister_device, parport_claim
 \f
+
+parport_register_dev - register to use a port
+-----------------------
+
+SYNOPSIS
+
+#include <linux/parport.h>
+
+typedef int (*preempt_func) (void *handle);
+typedef void (*wakeup_func) (void *handle);
+typedef int (*irq_func) (int irq, void *handle, struct pt_regs *);
+
+struct pardevice *parport_register_dev(struct parport *port,
+                                          const char *name,
+                                          preempt_func preempt,
+                                          wakeup_func wakeup,
+                                          irq_func irq,
+                                          int flags,
+                                          void *handle,
+					  struct parport_driver *drv);
+
+DESCRIPTION
+
+It is same as parport_register_device, and it should be only used if your
+driver is using the device-model, that is you have registered your driver
+using parport_register_drv.
+
 parport_unregister_device - finish using a port
 -------------------------
 
diff --git a/drivers/parport/TODO-parport b/drivers/parport/TODO-parport
index 089b14e..bdfbf50 100644
--- a/drivers/parport/TODO-parport
+++ b/drivers/parport/TODO-parport
@@ -17,4 +17,8 @@ Things to be done.
 
 4. A better PLIP (make use of bidirectional/ECP/EPP ports).
 
+5. Show base, irq information in the sys tree.
+
+6. Module autoloading based on the connected device.
+
 See <URL:http://people.redhat.com/twaugh/parport/>.
-- 
1.8.1.2

[PATCH 3/4] i2c-parport: use device-model parport

[Sudip Mukherjee]

modified the functions to use the new device-model of parport.

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

diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c
index a1fac5a..9464e74 100644
--- a/drivers/i2c/busses/i2c-parport.c
+++ b/drivers/i2c/busses/i2c-parport.c
@@ -160,20 +160,24 @@ static void i2c_parport_irq(void *data)
 			"SMBus alert received but no ARA client!\n");
 }
 
-static void i2c_parport_attach(struct parport *port)
+static int i2c_parport_attach(struct parport *port,
+			      struct parport_driver *drv)
 {
 	struct i2c_par *adapter;
 
 	adapter = kzalloc(sizeof(struct i2c_par), GFP_KERNEL);
 	if (adapter == NULL) {
 		printk(KERN_ERR "i2c-parport: Failed to kzalloc\n");
-		return;
+		return -ENOMEM;
 	}
 
 	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(port, "i2c-parport",
+					     NULL, NULL,
+					     i2c_parport_irq,
+					     PARPORT_FLAG_EXCL,
+					     adapter, drv);
 	if (!adapter->pdev) {
 		printk(KERN_ERR "i2c-parport: Unable to register with parport\n");
 		goto err_free;
@@ -230,13 +234,14 @@ static void i2c_parport_attach(struct parport *port)
 	mutex_lock(&adapter_list_lock);
 	list_add_tail(&adapter->node, &adapter_list);
 	mutex_unlock(&adapter_list_lock);
-	return;
+	return 0;
 
  err_unregister:
 	parport_release(adapter->pdev);
 	parport_unregister_device(adapter->pdev);
  err_free:
 	kfree(adapter);
+	return -ENODEV;
 }
 
 static void i2c_parport_detach(struct parport *port)
@@ -268,7 +273,7 @@ static void i2c_parport_detach(struct parport *port)
 
 static struct parport_driver i2c_parport_driver = {
 	.name	= "i2c-parport",
-	.attach	= i2c_parport_attach,
+	.attach_ret	= i2c_parport_attach,
 	.detach	= i2c_parport_detach,
 };
 
@@ -286,7 +291,7 @@ 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)
-- 
1.8.1.2

[PATCH 3/4] i2c-parport: use device-model parport

[Sudip Mukherjee]

modified the functions to use the new device-model of parport.

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

diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c
index a1fac5a..9464e74 100644
--- a/drivers/i2c/busses/i2c-parport.c
+++ b/drivers/i2c/busses/i2c-parport.c
@@ -160,20 +160,24 @@ static void i2c_parport_irq(void *data)
 			"SMBus alert received but no ARA client!\n");
 }
 
-static void i2c_parport_attach(struct parport *port)
+static int i2c_parport_attach(struct parport *port,
+			      struct parport_driver *drv)
 {
 	struct i2c_par *adapter;
 
 	adapter = kzalloc(sizeof(struct i2c_par), GFP_KERNEL);
 	if (adapter == NULL) {
 		printk(KERN_ERR "i2c-parport: Failed to kzalloc\n");
-		return;
+		return -ENOMEM;
 	}
 
 	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(port, "i2c-parport",
+					     NULL, NULL,
+					     i2c_parport_irq,
+					     PARPORT_FLAG_EXCL,
+					     adapter, drv);
 	if (!adapter->pdev) {
 		printk(KERN_ERR "i2c-parport: Unable to register with parport\n");
 		goto err_free;
@@ -230,13 +234,14 @@ static void i2c_parport_attach(struct parport *port)
 	mutex_lock(&adapter_list_lock);
 	list_add_tail(&adapter->node, &adapter_list);
 	mutex_unlock(&adapter_list_lock);
-	return;
+	return 0;
 
  err_unregister:
 	parport_release(adapter->pdev);
 	parport_unregister_device(adapter->pdev);
  err_free:
 	kfree(adapter);
+	return -ENODEV;
 }
 
 static void i2c_parport_detach(struct parport *port)
@@ -268,7 +273,7 @@ static void i2c_parport_detach(struct parport *port)
 
 static struct parport_driver i2c_parport_driver = {
 	.name	= "i2c-parport",
-	.attach	= i2c_parport_attach,
+	.attach_ret	= i2c_parport_attach,
 	.detach	= i2c_parport_detach,
 };
 
@@ -286,7 +291,7 @@ 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)
-- 
1.8.1.2

[PATCH 4/4] staging: panel: use parport in device-model

[Sudip Mukherjee]

modified the required functions to start using the new parport
device-model

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

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index ea54fb4..475c690 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -2188,25 +2188,26 @@ static struct notifier_block panel_notifier = {
 	0
 };
 
-static void panel_attach(struct parport *port)
+static int panel_attach(struct parport *port,
+			struct parport_driver *drv)
 {
 	if (port->number != parport)
-		return;
+		return -ENODEV;
 
 	if (pprt) {
 		pr_err("%s: port->number=%d parport=%d, already registered!\n",
 		       __func__, port->number, parport);
-		return;
+		return -EAGAIN;
 	}
 
-	pprt = parport_register_device(port, "panel", NULL, NULL,  /* pf, kf */
+	pprt = parport_register_dev(port, "panel", NULL, NULL,  /* pf, kf */
 				       NULL,
 				       /*PARPORT_DEV_EXCL */
-				       0, (void *)&pprt);
+				       0, (void *)&pprt, drv);
 	if (pprt == NULL) {
 		pr_err("%s: port->number=%d parport=%d, parport_register_device() failed\n",
 		       __func__, port->number, parport);
-		return;
+		return -ENODEV;
 	}
 
 	if (parport_claim(pprt)) {
@@ -2230,7 +2231,7 @@ static void panel_attach(struct parport *port)
 			goto err_lcd_unreg;
 	}
 	register_reboot_notifier(&panel_notifier);
-	return;
+	return 0;
 
 err_lcd_unreg:
 	if (lcd.enabled)
@@ -2238,6 +2239,7 @@ err_lcd_unreg:
 err_unreg_device:
 	parport_unregister_device(pprt);
 	pprt = NULL;
+	return -ENODEV;
 }
 
 static void panel_detach(struct parport *port)
@@ -2270,7 +2272,7 @@ static void panel_detach(struct parport *port)
 
 static struct parport_driver panel_driver = {
 	.name = "panel",
-	.attach = panel_attach,
+	.attach_ret = panel_attach,
 	.detach = panel_detach,
 };
 
@@ -2380,7 +2382,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;
-- 
1.8.1.2

[PATCH 4/4] staging: panel: use parport in device-model

[Sudip Mukherjee]

modified the required functions to start using the new parport
device-model

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

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index ea54fb4..475c690 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -2188,25 +2188,26 @@ static struct notifier_block panel_notifier = {
 	0
 };
 
-static void panel_attach(struct parport *port)
+static int panel_attach(struct parport *port,
+			struct parport_driver *drv)
 {
 	if (port->number != parport)
-		return;
+		return -ENODEV;
 
 	if (pprt) {
 		pr_err("%s: port->number=%d parport=%d, already registered!\n",
 		       __func__, port->number, parport);
-		return;
+		return -EAGAIN;
 	}
 
-	pprt = parport_register_device(port, "panel", NULL, NULL,  /* pf, kf */
+	pprt = parport_register_dev(port, "panel", NULL, NULL,  /* pf, kf */
 				       NULL,
 				       /*PARPORT_DEV_EXCL */
-				       0, (void *)&pprt);
+				       0, (void *)&pprt, drv);
 	if (pprt == NULL) {
 		pr_err("%s: port->number=%d parport=%d, parport_register_device() failed\n",
 		       __func__, port->number, parport);
-		return;
+		return -ENODEV;
 	}
 
 	if (parport_claim(pprt)) {
@@ -2230,7 +2231,7 @@ static void panel_attach(struct parport *port)
 			goto err_lcd_unreg;
 	}
 	register_reboot_notifier(&panel_notifier);
-	return;
+	return 0;
 
 err_lcd_unreg:
 	if (lcd.enabled)
@@ -2238,6 +2239,7 @@ err_lcd_unreg:
 err_unreg_device:
 	parport_unregister_device(pprt);
 	pprt = NULL;
+	return -ENODEV;
 }
 
 static void panel_detach(struct parport *port)
@@ -2270,7 +2272,7 @@ static void panel_detach(struct parport *port)
 
 static struct parport_driver panel_driver = {
 	.name = "panel",
-	.attach = panel_attach,
+	.attach_ret = panel_attach,
 	.detach = panel_detach,
 };
 
@@ -2380,7 +2382,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;
-- 
1.8.1.2

Re: [PATCH 1/4] parport: modify parport subsystem to use devicemodel

[Dan Carpenter]

Sorry, I still haven't done a proper review.

On Wed, Apr 15, 2015 at 01:18:41PM +0530, Sudip Mukherjee wrote:
> +struct pardevice *
> +parport_register_dev(struct parport *port, const char *name,
> +		     int (*pf)(void *), void (*kf)(void *),
> +		     void (*irq_func)(void *), int flags,
> +		     void *handle, struct parport_driver *drv)
> +{
> +	struct pardevice *tmp;

"tmp" is inaccurate.  It's not a tmp variable.  Use par_dev or
something.


> +	int ret;
> +	char *devname;
> +
> +	if (port->physport->flags & PARPORT_FLAG_EXCL) {
> +		/* An exclusive device is registered. */
> +		pr_debug("%s: no more devices allowed\n",
> +			 port->name);
> +		return NULL;
> +	}
> +
> +	if (flags & PARPORT_DEV_LURK) {
> +		if (!pf || !kf) {
> +			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);
> +
> +	tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
> +	if (!tmp) {
> +		pr_warn("%s: memory squeeze, couldn't register %s.\n",
> +			port->name, name);

Don't print warnings on kmalloc() failure.

> +		goto out;

out is a bad label name.  Use err_put_port or something descriptive.

> +	}
> +
> +	tmp->state = kmalloc(sizeof(*tmp->state), GFP_KERNEL);

I think kzalloc() is better here.  That way if the ->init_state()
functions don't set it, then we know it's zeroed out.

> +	if (!tmp->state) {
> +		pr_warn("%s: memory squeeze, couldn't register %s.\n",
> +			port->name, name);
> +		goto out_free_pardevice;
> +	}
> +
> +	tmp->name = name;

I wonder who frees this name variable.  My concern is that it gets
freed before we are done using it or something.  (I have not looked at
the details).

> +	tmp->port = port;
> +	tmp->daisy = -1;
> +	tmp->preempt = pf;
> +	tmp->wakeup = kf;
> +	tmp->private = handle;

handle sounds like a function pointer.  It should be private_data.

> +	tmp->flags = flags;
> +	tmp->irq_func = irq_func;
> +	tmp->waiting = 0;
> +	tmp->timeout = 5 * HZ;
> +
> +	tmp->dev.parent = &port->ddev;
> +	devname = kstrdup(name, GFP_KERNEL);

kstrdup() can fail.

> +	dev_set_name(&tmp->dev, "%s", name);
> +	tmp->dev.driver = &drv->driver;
> +	tmp->dev.release = free_pardevice;
> +	tmp->devmodel = true;
> +	ret = device_register(&tmp->dev);
> +	if (ret)
> +		goto out_free_all;

out_free_all is a bad label name.  It should be free_state.  Except the
most recently allocated resource is devname.  It should be
goto free_devname.  The beauty of labeling things this way is that you
only have to read back a few lines to see what is being freed.

> +
> +	/* Chain this onto the list */
> +	tmp->prev = NULL;

Not really needed because this was allocated with kzalloc(), of course.
But sometimes people like to say things twice for documentation so
that's also fine.

> +	/*
> +	 * 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 (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 out_free_dev;

			goto err_put_dev;

> +		}
> +		port->flags |= PARPORT_FLAG_EXCL;
> +	}
> +
> +	tmp->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 = tmp;
> +	port->physport->devices = tmp;
> +	spin_unlock(&port->physport->pardevice_lock);
> +
> +	init_waitqueue_head(&tmp->wait_q);
> +	tmp->timeslice = parport_default_timeslice;
> +	tmp->waitnext = NULL;
> +	tmp->waitprev = NULL;
> +
> +	/*
> +	 * This has to be run as last thing since init_state may need other
> +	 * pardevice fields. -arca
> +	 */
> +	port->ops->init_state(tmp, tmp->state);
> +	if (!test_and_set_bit(PARPORT_DEVPROC_REGISTERED, &port->devflags)) {
> +		port->proc_device = tmp;
> +		parport_device_proc_register(tmp);
> +	}

I don't understand this test_and_set_bit() condition.  It's weird to me
that parport_register_dev() succeeds even though we haven't called
parport_device_proc_register(tmp).

> +
> +	return tmp;

Put a blank line here.

> +out_free_dev:
> +	put_device(&tmp->dev);
> +out_free_all:
> +	kfree(tmp->state);
> +out_free_pardevice:
> +	kfree(tmp);
> +out:
> +	parport_put_port(port);
> +	module_put(port->ops->owner);
> +
> +	return NULL;
> +}
> +

regards,
dan carpenter

Re: [PATCH 1/4] parport: modify parport subsystem to use devicemodel

[Dan Carpenter]

Sorry, I still haven't done a proper review.

On Wed, Apr 15, 2015 at 01:18:41PM +0530, Sudip Mukherjee wrote:
> +struct pardevice *
> +parport_register_dev(struct parport *port, const char *name,
> +		     int (*pf)(void *), void (*kf)(void *),
> +		     void (*irq_func)(void *), int flags,
> +		     void *handle, struct parport_driver *drv)
> +{
> +	struct pardevice *tmp;

"tmp" is inaccurate.  It's not a tmp variable.  Use par_dev or
something.


> +	int ret;
> +	char *devname;
> +
> +	if (port->physport->flags & PARPORT_FLAG_EXCL) {
> +		/* An exclusive device is registered. */
> +		pr_debug("%s: no more devices allowed\n",
> +			 port->name);
> +		return NULL;
> +	}
> +
> +	if (flags & PARPORT_DEV_LURK) {
> +		if (!pf || !kf) {
> +			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);
> +
> +	tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
> +	if (!tmp) {
> +		pr_warn("%s: memory squeeze, couldn't register %s.\n",
> +			port->name, name);

Don't print warnings on kmalloc() failure.

> +		goto out;

out is a bad label name.  Use err_put_port or something descriptive.

> +	}
> +
> +	tmp->state = kmalloc(sizeof(*tmp->state), GFP_KERNEL);

I think kzalloc() is better here.  That way if the ->init_state()
functions don't set it, then we know it's zeroed out.

> +	if (!tmp->state) {
> +		pr_warn("%s: memory squeeze, couldn't register %s.\n",
> +			port->name, name);
> +		goto out_free_pardevice;
> +	}
> +
> +	tmp->name = name;

I wonder who frees this name variable.  My concern is that it gets
freed before we are done using it or something.  (I have not looked at
the details).

> +	tmp->port = port;
> +	tmp->daisy = -1;
> +	tmp->preempt = pf;
> +	tmp->wakeup = kf;
> +	tmp->private = handle;

handle sounds like a function pointer.  It should be private_data.

> +	tmp->flags = flags;
> +	tmp->irq_func = irq_func;
> +	tmp->waiting = 0;
> +	tmp->timeout = 5 * HZ;
> +
> +	tmp->dev.parent = &port->ddev;
> +	devname = kstrdup(name, GFP_KERNEL);

kstrdup() can fail.

> +	dev_set_name(&tmp->dev, "%s", name);
> +	tmp->dev.driver = &drv->driver;
> +	tmp->dev.release = free_pardevice;
> +	tmp->devmodel = true;
> +	ret = device_register(&tmp->dev);
> +	if (ret)
> +		goto out_free_all;

out_free_all is a bad label name.  It should be free_state.  Except the
most recently allocated resource is devname.  It should be
goto free_devname.  The beauty of labeling things this way is that you
only have to read back a few lines to see what is being freed.

> +
> +	/* Chain this onto the list */
> +	tmp->prev = NULL;

Not really needed because this was allocated with kzalloc(), of course.
But sometimes people like to say things twice for documentation so
that's also fine.

> +	/*
> +	 * 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 (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 out_free_dev;

			goto err_put_dev;

> +		}
> +		port->flags |= PARPORT_FLAG_EXCL;
> +	}
> +
> +	tmp->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 = tmp;
> +	port->physport->devices = tmp;
> +	spin_unlock(&port->physport->pardevice_lock);
> +
> +	init_waitqueue_head(&tmp->wait_q);
> +	tmp->timeslice = parport_default_timeslice;
> +	tmp->waitnext = NULL;
> +	tmp->waitprev = NULL;
> +
> +	/*
> +	 * This has to be run as last thing since init_state may need other
> +	 * pardevice fields. -arca
> +	 */
> +	port->ops->init_state(tmp, tmp->state);
> +	if (!test_and_set_bit(PARPORT_DEVPROC_REGISTERED, &port->devflags)) {
> +		port->proc_device = tmp;
> +		parport_device_proc_register(tmp);
> +	}

I don't understand this test_and_set_bit() condition.  It's weird to me
that parport_register_dev() succeeds even though we haven't called
parport_device_proc_register(tmp).

> +
> +	return tmp;

Put a blank line here.

> +out_free_dev:
> +	put_device(&tmp->dev);
> +out_free_all:
> +	kfree(tmp->state);
> +out_free_pardevice:
> +	kfree(tmp);
> +out:
> +	parport_put_port(port);
> +	module_put(port->ops->owner);
> +
> +	return NULL;
> +}
> +

regards,
dan carpenter

Re: [PATCH 1/4] parport: modify parport subsystem to use devicemodel

[Dan Carpenter]

On Wed, Apr 15, 2015 at 01:18:41PM +0530, Sudip Mukherjee wrote:
> @@ -575,6 +647,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 +703,133 @@ parport_register_device(struct parport *port, const char *name,
>  	return NULL;
>  }
>  
> +void free_pardevice(struct device *dev)
> +{
> +}
> +
> +struct pardevice *
> +parport_register_dev(struct parport *port, const char *name,
> +		     int (*pf)(void *), void (*kf)(void *),
> +		     void (*irq_func)(void *), int flags,
> +		     void *handle, struct parport_driver *drv)


The difference between parport_register_device() and
parport_register_dev() isn't clear from the name.

regards,
dan carpenter

Re: [PATCH 1/4] parport: modify parport subsystem to use devicemodel

[Dan Carpenter]

On Wed, Apr 15, 2015 at 01:18:41PM +0530, Sudip Mukherjee wrote:
> @@ -575,6 +647,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 +703,133 @@ parport_register_device(struct parport *port, const char *name,
>  	return NULL;
>  }
>  
> +void free_pardevice(struct device *dev)
> +{
> +}
> +
> +struct pardevice *
> +parport_register_dev(struct parport *port, const char *name,
> +		     int (*pf)(void *), void (*kf)(void *),
> +		     void (*irq_func)(void *), int flags,
> +		     void *handle, struct parport_driver *drv)


The difference between parport_register_device() and
parport_register_dev() isn't clear from the name.

regards,
dan carpenter

Re: [PATCH 1/4] parport: modify parport subsystem to use devicemodel

[Sudip Mukherjee]

On Wed, Apr 15, 2015 at 11:27:46AM +0300, Dan Carpenter wrote:
> Sorry, I still haven't done a proper review.

for almost all your points: it came as i copied the parport_register_dev
from parport_register_device and just added some part leaving everything
else same. I will fix these points in v2 of this patch series.
> 
<snip>
> On Wed, Apr 15, 2015 at 01:18:41PM +0530, Sudip Mukherjee wrote:
> Don't print warnings on kmalloc() failure.
> 
> I think kzalloc() is better here.  That way if the ->init_state()
> functions don't set it, then we know it's zeroed out.
yes, i will.
Infact parport_register_device() is using kmalloc for allocating
pardevice and I copied the same code to parport_register_dev()
and had a very tough time to find why I am getting
"tried to init an initialized object" and stackdump, finally after a
coffee break, found it being caused because of not using kzalloc .. :)
> 
<snip>
> > +	tmp->name = name;
> 
> I wonder who frees this name variable.  My concern is that it gets
> freed before we are done using it or something.  (I have not looked at
> the details).
it will be done in free_port() the release callback of parport device.
> 
<snip>
> > +	tmp->dev.parent = &port->ddev;
> > +	devname = kstrdup(name, GFP_KERNEL);
> 
> kstrdup() can fail.
it is actually my mistake. I was looking for various ways I can use in
dev_set_name. This devname and kstrdup is not needed and will be removed
in v2.
> 
<snip>
> > +	}
> 
> I don't understand this test_and_set_bit() condition.  It's weird to me
> that parport_register_dev() succeeds even though we haven't called
> parport_device_proc_register(tmp).

this PARPORT_DEVPROC_REGISTERED flag is cleared in parport_unregister_device()
and is set in parport_register_dev[ice], so when we call
parport_register_device() or parport_register_dev() it will be not set
and the condition will always be true.

regards
sudip
> 
> > +
> 
> regards,
> dan carpenter

Re: [PATCH 1/4] parport: modify parport subsystem to use devicemodel

[Sudip Mukherjee]

On Wed, Apr 15, 2015 at 11:27:46AM +0300, Dan Carpenter wrote:
> Sorry, I still haven't done a proper review.

for almost all your points: it came as i copied the parport_register_dev
from parport_register_device and just added some part leaving everything
else same. I will fix these points in v2 of this patch series.
> 
<snip>
> On Wed, Apr 15, 2015 at 01:18:41PM +0530, Sudip Mukherjee wrote:
> Don't print warnings on kmalloc() failure.
> 
> I think kzalloc() is better here.  That way if the ->init_state()
> functions don't set it, then we know it's zeroed out.
yes, i will.
Infact parport_register_device() is using kmalloc for allocating
pardevice and I copied the same code to parport_register_dev()
and had a very tough time to find why I am getting
"tried to init an initialized object" and stackdump, finally after a
coffee break, found it being caused because of not using kzalloc .. :)
> 
<snip>
> > +	tmp->name = name;
> 
> I wonder who frees this name variable.  My concern is that it gets
> freed before we are done using it or something.  (I have not looked at
> the details).
it will be done in free_port() the release callback of parport device.
> 
<snip>
> > +	tmp->dev.parent = &port->ddev;
> > +	devname = kstrdup(name, GFP_KERNEL);
> 
> kstrdup() can fail.
it is actually my mistake. I was looking for various ways I can use in
dev_set_name. This devname and kstrdup is not needed and will be removed
in v2.
> 
<snip>
> > +	}
> 
> I don't understand this test_and_set_bit() condition.  It's weird to me
> that parport_register_dev() succeeds even though we haven't called
> parport_device_proc_register(tmp).

this PARPORT_DEVPROC_REGISTERED flag is cleared in parport_unregister_device()
and is set in parport_register_dev[ice], so when we call
parport_register_device() or parport_register_dev() it will be not set
and the condition will always be true.

regards
sudip
> 
> > +
> 
> regards,
> dan carpenter

Re: [PATCH 1/4] parport: modify parport subsystem to use devicemodel

[Sudip Mukherjee]

On Wed, Apr 15, 2015 at 11:33:59AM +0300, Dan Carpenter wrote:
> On Wed, Apr 15, 2015 at 01:18:41PM +0530, Sudip Mukherjee wrote:
> 
> 
> The difference between parport_register_device() and
> parport_register_dev() isn't clear from the name.
i kept the name similar deliberately as I thought that after all the
drivers are modified to use these new api the old functions will be
removed so then this name will have some meaning.

so, should i send a v2 now with the changes you suggested or should we
wait for some review about the functionality and testing?


regards
sudip
> 
> regards,
> dan carpenter
> 

Re: [PATCH 1/4] parport: modify parport subsystem to use devicemodel

[Dan Carpenter]

On Wed, Apr 15, 2015 at 02:50:55PM +0530, Sudip Mukherjee wrote:
> this PARPORT_DEVPROC_REGISTERED flag is cleared in parport_unregister_device()
> and is set in parport_register_dev[ice], so when we call
> parport_register_device() or parport_register_dev() it will be not set
> and the condition will always be true.

The question of how to handle impossible conditions is always tricky.
:P  In this case just remove the condition.

regards,
dan carpenter

Re: [PATCH 1/4] parport: modify parport subsystem to use devicemodel

[Dan Carpenter]

On Wed, Apr 15, 2015 at 02:50:55PM +0530, Sudip Mukherjee wrote:
> this PARPORT_DEVPROC_REGISTERED flag is cleared in parport_unregister_device()
> and is set in parport_register_dev[ice], so when we call
> parport_register_device() or parport_register_dev() it will be not set
> and the condition will always be true.

The question of how to handle impossible conditions is always tricky.
:P  In this case just remove the condition.

regards,
dan carpenter

Re: [PATCH 1/4] parport: modify parport subsystem to use devicemodel

[Dan Carpenter]

On Wed, Apr 15, 2015 at 02:50:55PM +0530, Sudip Mukherjee wrote:
> <snip>
> > > +	tmp->name = name;
> > 
> > I wonder who frees this name variable.  My concern is that it gets
> > freed before we are done using it or something.  (I have not looked at
> > the details).
> it will be done in free_port() the release callback of parport device.

That doesn't work.  Some of the callers pass a string literal.

regards,
dan carpenter

Re: [PATCH 1/4] parport: modify parport subsystem to use devicemodel

[Dan Carpenter]

On Wed, Apr 15, 2015 at 02:50:55PM +0530, Sudip Mukherjee wrote:
> <snip>
> > > +	tmp->name = name;
> > 
> > I wonder who frees this name variable.  My concern is that it gets
> > freed before we are done using it or something.  (I have not looked at
> > the details).
> it will be done in free_port() the release callback of parport device.

That doesn't work.  Some of the callers pass a string literal.

regards,
dan carpenter

Re: [PATCH 1/4] parport: modify parport subsystem to use devicemodel

[Sudip Mukherjee]

On Wed, Apr 15, 2015 at 12:45:00PM +0300, Dan Carpenter wrote:
> On Wed, Apr 15, 2015 at 02:50:55PM +0530, Sudip Mukherjee wrote:
> > <snip>
> > > > +	tmp->name = name;
> > > 
> > > I wonder who frees this name variable.  My concern is that it gets
> > > freed before we are done using it or something.  (I have not looked at
> > > the details).
> > it will be done in free_port() the release callback of parport device.
> 
> That doesn't work.  Some of the callers pass a string literal.
oops, when i replied to your first mail about this I looked at
parport_register_port(), but the one that you have mentioned is the one
I gave in parport_register_dev() and it seems that the name is not freed
anywhere except the driver that has called parport_register_device() or
parport_register_dev(). I think I should better copy it and then give it
to tmp->name and free it also in the unregister function. And this should
be fixed in the original parport_register_device() also.
I will wait for some more reviews send the v2 series tomorrow.

regards
sudip

> 
> regards,
> dan carpenter
> 

Re: [PATCH 1/4] parport: modify parport subsystem to use devicemodel

[Greg Kroah-Hartman]

On Wed, Apr 15, 2015 at 01:18:41PM +0530, Sudip Mukherjee wrote:
> parport starts using device-model and we now have parport under
> /sys/bus. As the ports are discovered they are added as device under
> /sys/bus/parport. As and when other drivers register new device,
> they will be registered as a subdevice under the relevant parport.
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
>  drivers/parport/procfs.c |  15 ++-
>  drivers/parport/share.c  | 236 ++++++++++++++++++++++++++++++++++++++++++++---
>  include/linux/parport.h  |  29 +++++-
>  3 files changed, 267 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/parport/procfs.c b/drivers/parport/procfs.c
> index 3b47080..1ce363b 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 = bus_register(&parport_bus_type);
> +	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;
>  	}
> +	bus_unregister(&parport_bus_type);
>  }
>  
>  #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 bus_register(&parport_bus_type);
>  }
>  
>  static void __exit parport_default_proc_unregister (void)
>  {
> +	bus_unregister(&parport_bus_type);
>  }
>  #endif

Don't bury bus register/unregister down in proc file creation, it's hard
to review and justify.

greg k-h

Re: [PATCH 1/4] parport: modify parport subsystem to use devicemodel

[Greg Kroah-Hartman]

On Wed, Apr 15, 2015 at 01:18:41PM +0530, Sudip Mukherjee wrote:
> --- a/drivers/parport/share.c
> +++ b/drivers/parport/share.c
> @@ -10,6 +10,8 @@
>   * based on work by Grant Guenther <grant@torque.net>
>   *          and Philip Blundell
>   *
> + * Added Device-Model - Sudip Mukherjee <sudip@vectorindia.org>

Changelog handles this, no need to add it to the file.

Re: [PATCH 1/4] parport: modify parport subsystem to use devicemodel

[Greg Kroah-Hartman]

On Wed, Apr 15, 2015 at 01:18:41PM +0530, Sudip Mukherjee wrote:
> @@ -29,6 +31,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 +103,11 @@ static struct parport_operations dead_ops = {
>  	.owner		= NULL,
>  };
>  
> +struct bus_type parport_bus_type = {
> +	.name           = "parport",
> +};
> +EXPORT_SYMBOL(parport_bus_type);

They bus type shouldn't need to be exported, unless something is really
wrong.  Why did you do this?

> +
>  /* Call attach(port) for each registered driver. */
>  static void attach_driver_chain(struct parport *port)
>  {
> @@ -157,6 +165,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 +176,57 @@ int parport_register_driver (struct parport_driver *drv)
>  	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)
> +{
> +	struct parport *port;
> +	int ret, err = 0;
> +	bool attached = false;

You shouldn't need to track attached/not attached with the driver model,
that's what it is there for to handle for you.

> +
> +	if (list_empty(&portlist))
> +		get_lowlevel_driver();

what does this call do?

> +
> +	/* 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);
> +	list_for_each_entry(port, &portlist, list) {
> +		ret = drv->attach_ret(port, drv);
> +		if (ret == 0)
> +			attached = true;
> +		else
> +			err = ret;
> +	}
> +	if (attached)
> +		list_add(&drv->list, &drivers);

Why all of this?  You shouldn't need your own matching anymore, the
driver core does it for you when you register the driver, against the
devices you have already registered, if any.


> +	mutex_unlock(&registration_lock);
> +	if (!attached) {
> +		driver_unregister(&drv->driver);
> +		return err;
> +	}
> +
> +	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
> @@ -193,11 +253,15 @@ void parport_unregister_driver (struct parport_driver *drv)
>  	list_for_each_entry(port, &portlist, list)
>  		drv->detach(port);
>  	mutex_unlock(&registration_lock);
> +	if (drv->devmodel)
> +		driver_unregister(&drv->driver);
>  }
>  
> -static void free_port (struct parport *port)
> +static void free_port(struct device *dev)
>  {
>  	int d;
> +	struct parport *port = to_parport_dev(dev);
> +
>  	spin_lock(&full_list_lock);
>  	list_del(&port->full_list);
>  	spin_unlock(&full_list_lock);
> @@ -223,8 +287,9 @@ static void free_port (struct parport *port)
>  
>  struct parport *parport_get_port (struct parport *port)
>  {
> -	atomic_inc (&port->ref_count);
> -	return port;
> +	struct device *dev = get_device(&port->ddev);
> +
> +	return to_parport_dev(dev);
>  }
>  
>  /**
> @@ -237,11 +302,7 @@ struct parport *parport_get_port (struct parport *port)
>  
>  void parport_put_port (struct parport *port)
>  {
> -	if (atomic_dec_and_test (&port->ref_count))
> -		/* Can destroy it now. */
> -		free_port (port);
> -
> -	return;
> +	put_device(&port->ddev);
>  }
>  
>  /**
> @@ -281,6 +342,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 +395,9 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma,
>  	 */
>  	sprintf(name, "parport%d", tmp->portnum = tmp->number);
>  	tmp->name = name;
> +	tmp->ddev.bus = &parport_bus_type;
> +	tmp->ddev.release = free_port;
> +	dev_set_name(&tmp->ddev, name);
>  
>  	for (device = 0; device < 5; device++)
>  		/* assume the worst */
> @@ -340,6 +405,13 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma,
>  
>  	tmp->waithead = tmp->waittail = NULL;
>  
> +	ret = device_register(&tmp->ddev);
> +	if (ret) {
> +		list_del(&tmp->full_list);
> +		kfree(tmp);
> +		return NULL;

Please read the kerneldoc comments for device_register(), you can't
clean things up this way if device_register() fails :(

> +	}
> +
>  	return tmp;
>  }
>  
> @@ -575,6 +647,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 +703,133 @@ parport_register_device(struct parport *port, const char *name,
>  	return NULL;
>  }
>  
> +void free_pardevice(struct device *dev)
> +{
> +}

empty free functions are a huge red flag.  So much so the kobject
documentation in the kernel says I get to make fun of anyone who tries
to do this.  So please don't do this :)


> +struct pardevice *
> +parport_register_dev(struct parport *port, const char *name,
> +		     int (*pf)(void *), void (*kf)(void *),
> +		     void (*irq_func)(void *), int flags,
> +		     void *handle, struct parport_driver *drv)

I think you need a structure, what's with all of the function pointers?




> +{
> +	struct pardevice *tmp;
> +	int ret;
> +	char *devname;
> +
> +	if (port->physport->flags & PARPORT_FLAG_EXCL) {
> +		/* An exclusive device is registered. */
> +		pr_debug("%s: no more devices allowed\n",
> +			 port->name);
> +		return NULL;
> +	}
> +
> +	if (flags & PARPORT_DEV_LURK) {
> +		if (!pf || !kf) {
> +			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);
> +
> +	tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
> +	if (!tmp) {
> +		pr_warn("%s: memory squeeze, couldn't register %s.\n",
> +			port->name, name);
> +		goto out;
> +	}
> +
> +	tmp->state = kmalloc(sizeof(*tmp->state), GFP_KERNEL);
> +	if (!tmp->state) {
> +		pr_warn("%s: memory squeeze, couldn't register %s.\n",
> +			port->name, name);
> +		goto out_free_pardevice;
> +	}
> +
> +	tmp->name = name;
> +	tmp->port = port;
> +	tmp->daisy = -1;
> +	tmp->preempt = pf;
> +	tmp->wakeup = kf;
> +	tmp->private = handle;
> +	tmp->flags = flags;
> +	tmp->irq_func = irq_func;
> +	tmp->waiting = 0;
> +	tmp->timeout = 5 * HZ;
> +
> +	tmp->dev.parent = &port->ddev;
> +	devname = kstrdup(name, GFP_KERNEL);
> +	dev_set_name(&tmp->dev, "%s", name);

Why create devname and name here in different ways?

> +	tmp->dev.driver = &drv->driver;
> +	tmp->dev.release = free_pardevice;
> +	tmp->devmodel = true;
> +	ret = device_register(&tmp->dev);
> +	if (ret)
> +		goto out_free_all;

Again, wrong error handling.


> +	/* Chain this onto the list */
> +	tmp->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 (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 out_free_dev;
> +		}
> +		port->flags |= PARPORT_FLAG_EXCL;
> +	}
> +
> +	tmp->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 = tmp;
> +	port->physport->devices = tmp;
> +	spin_unlock(&port->physport->pardevice_lock);

This whole list of device management seems odd, is it really still
needed with the conversion to the new model?


> +
> +	init_waitqueue_head(&tmp->wait_q);
> +	tmp->timeslice = parport_default_timeslice;
> +	tmp->waitnext = NULL;
> +	tmp->waitprev = NULL;
> +
> +	/*
> +	 * This has to be run as last thing since init_state may need other
> +	 * pardevice fields. -arca
> +	 */
> +	port->ops->init_state(tmp, tmp->state);
> +	if (!test_and_set_bit(PARPORT_DEVPROC_REGISTERED, &port->devflags)) {
> +		port->proc_device = tmp;
> +		parport_device_proc_register(tmp);
> +	}
> +
> +	return tmp;
> +out_free_dev:
> +	put_device(&tmp->dev);
> +out_free_all:
> +	kfree(tmp->state);
> +out_free_pardevice:
> +	kfree(tmp);
> +out:
> +	parport_put_port(port);
> +	module_put(port->ops->owner);
> +
> +	return NULL;
> +}
> +
>  /**
>   *	parport_unregister_device - deregister a device on a parallel port
>   *	@dev: pointer to structure representing device
> @@ -691,7 +891,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);
> @@ -774,6 +977,7 @@ int parport_claim(struct pardevice *dev)
>  	struct pardevice *oldcad;
>  	struct parport *port = dev->port->physport;
>  	unsigned long flags;
> +	int ret;
>  
>  	if (port->cad == dev) {
>  		printk(KERN_INFO "%s: %s already owner\n",
> @@ -802,6 +1006,13 @@ int parport_claim(struct pardevice *dev)
>  		}
>  	}
>  
> +	if (dev->devmodel) {
> +		ret = device_attach(&dev->dev);
> +		if (ret != 1) {
> +			return -ENODEV;
> +		}
> +	}
> +
>  	/* Can't fail from now on, so mark ourselves as no longer waiting.  */
>  	if (dev->waiting & 1) {
>  		dev->waiting = 0;
> @@ -926,8 +1137,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;
>  }
>  
> @@ -954,6 +1165,8 @@ void parport_release(struct pardevice *dev)
>  		       "when not owner\n", port->name, dev->name);
>  		return;
>  	}
> +	if (dev->devmodel)
> +		device_release_driver(&dev->dev);
>  
>  #ifdef CONFIG_PARPORT_1284
>  	/* If this is on a mux port, deselect it. */
> @@ -1022,6 +1235,7 @@ EXPORT_SYMBOL(parport_remove_port);
>  EXPORT_SYMBOL(parport_register_driver);
>  EXPORT_SYMBOL(parport_unregister_driver);
>  EXPORT_SYMBOL(parport_register_device);
> +EXPORT_SYMBOL(parport_register_dev);

checkpatch doesn't like you putting this here :)

thanks,

greg k-h

Re: [PATCH 1/4] parport: modify parport subsystem to use devicemodel

[Sudip Mukherjee]

On Wed, Apr 15, 2015 at 03:31:15PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 15, 2015 at 01:18:41PM +0530, Sudip Mukherjee wrote:
> > @@ -29,6 +31,7 @@
<snip>
> > +struct bus_type parport_bus_type = {
> > +	.name           = "parport",
> > +};
> > +EXPORT_SYMBOL(parport_bus_type);
> 
> They bus type shouldn't need to be exported, unless something is really
> wrong.  Why did you do this?
I was thinking why it was needed but i saw the same for pci_bus_type,
i2c_bus_type and spi_bus_type and i blindly followed. :(
> 
> > +
<snip>
> > +int __parport_register_drv(struct parport_driver *drv,
> > +			   struct module *owner, const char *mod_name)
> > +{
> > +	struct parport *port;
> > +	int ret, err = 0;
> > +	bool attached = false;
> 
> You shouldn't need to track attached/not attached with the driver model,
> that's what it is there for to handle for you.
this attach is different from the driver model. The driver when it calls
prport_register_drv(), it will again call back a function which was called
attach, so i named the variable as attached. but i guess the name is
misleading, i will rename it in v2.
> 
> > +
> > +	if (list_empty(&portlist))
> > +		get_lowlevel_driver();
> 
> what does this call do?
if parport_pc is not loaded it does a request_module, and the
documentation says to add "alias parport_lowlevel parport_pc"
in /etc/modprobe.d directory. parport_pc will actually register the
ports, so if that module is not loaded then the system is not yet
having any parallel port registered.
> 
<snip>
> > +			err = ret;
> > +	}
> > +	if (attached)
> > +		list_add(&drv->list, &drivers);
> 
> Why all of this?  You shouldn't need your own matching anymore, the
> driver core does it for you when you register the driver, against the
> devices you have already registered, if any.
ok. I will remove. actully, I have copied the old function and just
added the device-model specific parts to it, and I thought after we have
a working model then we can remove these parts which will not be needed.
> 
> 
<snip>
> > +		list_del(&tmp->full_list);
> > +		kfree(tmp);
> > +		return NULL;
> 
> Please read the kerneldoc comments for device_register(), you can't
> clean things up this way if device_register() fails :(
oops.. sorry, i read that and did that while unregistering the device,
but here the old habit of kfree came ... :(
> 
<snip>
> >  	tmp->prev = NULL;
> > @@ -630,6 +703,133 @@ parport_register_device(struct parport *port, const char *name,
> >  	return NULL;
> >  }
> >  
> > +void free_pardevice(struct device *dev)
> > +{
> > +}
> 
> empty free functions are a huge red flag.  So much so the kobject
> documentation in the kernel says I get to make fun of anyone who tries
> to do this.  So please don't do this :)
yeahh, i remember reading it, but when i got the stackdump, that went
out of my head. working with the device-mode for the first time, so i
guess it can be excused. :)
> 
> 
> > +struct pardevice *
> > +parport_register_dev(struct parport *port, const char *name,
> > +		     int (*pf)(void *), void (*kf)(void *),
> > +		     void (*irq_func)(void *), int flags,
> > +		     void *handle, struct parport_driver *drv)
> 
> I think you need a structure, what's with all of the function pointers?
ok. that will keep the code tidy.
> 
> > +	tmp->waiting = 0;
> > +	tmp->timeout = 5 * HZ;
> > +
> > +	tmp->dev.parent = &port->ddev;
> > +	devname = kstrdup(name, GFP_KERNEL);
> > +	dev_set_name(&tmp->dev, "%s", name);
> 
> Why create devname and name here in different ways?
I was experimenting and finally forgot to remove devname. sorry.
> 
> > +	tmp->dev.driver = &drv->driver;
> > +	tmp->dev.release = free_pardevice;
> > +	tmp->devmodel = true;
> > +	ret = device_register(&tmp->dev);
> > +	if (ret)
> > +		goto out_free_all;
> 
> Again, wrong error handling.
will fix it.
> 
> 
> > +	/* Chain this onto the list */
<snip>
> > +	if (port->physport->devices)
> > +		port->physport->devices->prev = tmp;
> > +	port->physport->devices = tmp;
> > +	spin_unlock(&port->physport->pardevice_lock);
> 
> This whole list of device management seems odd, is it really still
> needed with the conversion to the new model?
like i said before, I have not removed anything from the function.
> 
> 
> >  		return;
> >  	}
> > +	if (dev->devmodel)
> > +		device_release_driver(&dev->dev);
> >  
> >  #ifdef CONFIG_PARPORT_1284
> >  	/* If this is on a mux port, deselect it. */
> > @@ -1022,6 +1235,7 @@ EXPORT_SYMBOL(parport_remove_port);
> >  EXPORT_SYMBOL(parport_register_driver);
> >  EXPORT_SYMBOL(parport_unregister_driver);
> >  EXPORT_SYMBOL(parport_register_device);
> > +EXPORT_SYMBOL(parport_register_dev);
> 
> checkpatch doesn't like you putting this here :)
oops... missed that.
I will send in a v2, better I will send to u and Dan and ofcourse lkml
as RFC. After the final version of RFC is approved then I will send the
patch for applying.

regards
sudip

> thanks,
> 
> greg k-h

Re: [PATCH 1/4] parport: modify parport subsystem to use devicemodel

[Sudip Mukherjee]

On Wed, Apr 15, 2015 at 03:31:15PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 15, 2015 at 01:18:41PM +0530, Sudip Mukherjee wrote:
> > @@ -29,6 +31,7 @@
<snip>
> > +struct bus_type parport_bus_type = {
> > +	.name           = "parport",
> > +};
> > +EXPORT_SYMBOL(parport_bus_type);
> 
> They bus type shouldn't need to be exported, unless something is really
> wrong.  Why did you do this?
I was thinking why it was needed but i saw the same for pci_bus_type,
i2c_bus_type and spi_bus_type and i blindly followed. :(
> 
> > +
<snip>
> > +int __parport_register_drv(struct parport_driver *drv,
> > +			   struct module *owner, const char *mod_name)
> > +{
> > +	struct parport *port;
> > +	int ret, err = 0;
> > +	bool attached = false;
> 
> You shouldn't need to track attached/not attached with the driver model,
> that's what it is there for to handle for you.
this attach is different from the driver model. The driver when it calls
prport_register_drv(), it will again call back a function which was called
attach, so i named the variable as attached. but i guess the name is
misleading, i will rename it in v2.
> 
> > +
> > +	if (list_empty(&portlist))
> > +		get_lowlevel_driver();
> 
> what does this call do?
if parport_pc is not loaded it does a request_module, and the
documentation says to add "alias parport_lowlevel parport_pc"
in /etc/modprobe.d directory. parport_pc will actually register the
ports, so if that module is not loaded then the system is not yet
having any parallel port registered.
> 
<snip>
> > +			err = ret;
> > +	}
> > +	if (attached)
> > +		list_add(&drv->list, &drivers);
> 
> Why all of this?  You shouldn't need your own matching anymore, the
> driver core does it for you when you register the driver, against the
> devices you have already registered, if any.
ok. I will remove. actully, I have copied the old function and just
added the device-model specific parts to it, and I thought after we have
a working model then we can remove these parts which will not be needed.
> 
> 
<snip>
> > +		list_del(&tmp->full_list);
> > +		kfree(tmp);
> > +		return NULL;
> 
> Please read the kerneldoc comments for device_register(), you can't
> clean things up this way if device_register() fails :(
oops.. sorry, i read that and did that while unregistering the device,
but here the old habit of kfree came ... :(
> 
<snip>
> >  	tmp->prev = NULL;
> > @@ -630,6 +703,133 @@ parport_register_device(struct parport *port, const char *name,
> >  	return NULL;
> >  }
> >  
> > +void free_pardevice(struct device *dev)
> > +{
> > +}
> 
> empty free functions are a huge red flag.  So much so the kobject
> documentation in the kernel says I get to make fun of anyone who tries
> to do this.  So please don't do this :)
yeahh, i remember reading it, but when i got the stackdump, that went
out of my head. working with the device-mode for the first time, so i
guess it can be excused. :)
> 
> 
> > +struct pardevice *
> > +parport_register_dev(struct parport *port, const char *name,
> > +		     int (*pf)(void *), void (*kf)(void *),
> > +		     void (*irq_func)(void *), int flags,
> > +		     void *handle, struct parport_driver *drv)
> 
> I think you need a structure, what's with all of the function pointers?
ok. that will keep the code tidy.
> 
> > +	tmp->waiting = 0;
> > +	tmp->timeout = 5 * HZ;
> > +
> > +	tmp->dev.parent = &port->ddev;
> > +	devname = kstrdup(name, GFP_KERNEL);
> > +	dev_set_name(&tmp->dev, "%s", name);
> 
> Why create devname and name here in different ways?
I was experimenting and finally forgot to remove devname. sorry.
> 
> > +	tmp->dev.driver = &drv->driver;
> > +	tmp->dev.release = free_pardevice;
> > +	tmp->devmodel = true;
> > +	ret = device_register(&tmp->dev);
> > +	if (ret)
> > +		goto out_free_all;
> 
> Again, wrong error handling.
will fix it.
> 
> 
> > +	/* Chain this onto the list */
<snip>
> > +	if (port->physport->devices)
> > +		port->physport->devices->prev = tmp;
> > +	port->physport->devices = tmp;
> > +	spin_unlock(&port->physport->pardevice_lock);
> 
> This whole list of device management seems odd, is it really still
> needed with the conversion to the new model?
like i said before, I have not removed anything from the function.
> 
> 
> >  		return;
> >  	}
> > +	if (dev->devmodel)
> > +		device_release_driver(&dev->dev);
> >  
> >  #ifdef CONFIG_PARPORT_1284
> >  	/* If this is on a mux port, deselect it. */
> > @@ -1022,6 +1235,7 @@ EXPORT_SYMBOL(parport_remove_port);
> >  EXPORT_SYMBOL(parport_register_driver);
> >  EXPORT_SYMBOL(parport_unregister_driver);
> >  EXPORT_SYMBOL(parport_register_device);
> > +EXPORT_SYMBOL(parport_register_dev);
> 
> checkpatch doesn't like you putting this here :)
oops... missed that.
I will send in a v2, better I will send to u and Dan and ofcourse lkml
as RFC. After the final version of RFC is approved then I will send the
patch for applying.

regards
sudip

> thanks,
> 
> greg k-h

Re: [PATCH 1/4] parport: modify parport subsystem to use devicemodel

[Sudip Mukherjee]

On Wed, Apr 15, 2015 at 03:31:15PM +0200, Greg Kroah-Hartman wrote:
> > +
> > +}
> 
> empty free functions are a huge red flag.  So much so the kobject
> documentation in the kernel says I get to make fun of anyone who tries
> to do this.  So please don't do this :)
> 
i was just working on the v2 and I saw :
"@release: pointer to the function that will clean up the object when the
last reference to the object is released. This pointer is required, and
it is not acceptable to pass kfree in as this function.  If the caller
does pass kfree to this function, you will be publicly mocked mercilessly
by the kref maintainer".

so i thought of checking and it looks like there are a few instances
where kref has been passed as release.  :)

regards
sudip
>