[PATCH 0/4] UIO: fixes, cleanups and a new driver

[PATCH 0/4] UIO: fixes, cleanups and a new driver

[Uwe Kleine-König]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 747 bytes --]

Hello,

in this series you can find a generic platform_device driver for UIO and
a few cleanups and fixes to the existing UIO code.  The first patch fixes a
possible Oops.

Below you can find shortlog and diffstat.

I appreciate any (constructive) feedback.

Best regards
Uwe

Uwe Kleine-König (4):
      UIO: hold a reference to the device's owner while the device is open
      UIO: use menuconfig
      UIO: wrap all uio drivers in "if UIO" and "endif"
      [RFC] UIO: generic platform driver

 drivers/uio/Kconfig    |   19 ++++--
 drivers/uio/Makefile   |    1 +
 drivers/uio/uio.c      |   40 +++++++-----
 drivers/uio/uio_pdrv.c |  165 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 202 insertions(+), 23 deletions(-)

[PATCH 1/4] UIO: hold a reference to the device's owner while the device is open

[Uwe Kleine-König]

Otherwise the device might just disappear while /dev/uioX is being used
which results in an Oops.

Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
 drivers/uio/uio.c |   40 +++++++++++++++++++++++-----------------
 1 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 1175908..005fc55 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -301,25 +301,35 @@ static int uio_open(struct inode *inode, struct file *filep)
 	if (!idev)
 		return -ENODEV;
 
+	if (!try_module_get(idev->owner)) {
+		ret = -ENODEV;
+		goto err_module_get;
+	}
+
 	listener = kmalloc(sizeof(*listener), GFP_KERNEL);
-	if (!listener)
-		return -ENOMEM;
+	if (!listener) {
+		ret = -ENOMEM;
+		goto err_alloc_listener;
+	}
 
 	listener->dev = idev;
 	listener->event_count = atomic_read(&idev->event);
 	filep->private_data = listener;
 
 	if (idev->info->open) {
-		if (!try_module_get(idev->owner))
-			return -ENODEV;
 		ret = idev->info->open(idev->info, inode);
-		module_put(idev->owner);
-	}
+		if (ret) {
+			kfree(listener);
+err_alloc_listener:
 
-	if (ret)
-		kfree(listener);
+			module_put(idev->owner);
+err_module_get:
 
-	return ret;
+			return ret;
+		}
+	}
+
+	return 0;
 }
 
 static int uio_fasync(int fd, struct file *filep, int on)
@@ -336,12 +346,11 @@ static int uio_release(struct inode *inode, struct file *filep)
 	struct uio_listener *listener = filep->private_data;
 	struct uio_device *idev = listener->dev;
 
-	if (idev->info->release) {
-		if (!try_module_get(idev->owner))
-			return -ENODEV;
+	if (idev->info->release)
 		ret = idev->info->release(idev->info, inode);
-		module_put(idev->owner);
-	}
+
+	module_put(idev->owner);
+
 	if (filep->f_flags & FASYNC)
 		ret = uio_fasync(-1, filep, 0);
 	kfree(listener);
@@ -510,10 +519,7 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
 		return -EINVAL;
 
 	if (idev->info->mmap) {
-		if (!try_module_get(idev->owner))
-			return -ENODEV;
 		ret = idev->info->mmap(idev->info, vma);
-		module_put(idev->owner);
 		return ret;
 	}
 
-- 
1.5.4.5

[PATCH 2/4] UIO: use menuconfig

[Uwe Kleine-König]

This allows configuring CONFIG_UIO without changing into the UIO submenu

Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
 drivers/uio/Kconfig |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index b778ed7..a899306 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -1,9 +1,7 @@
-menu "Userspace I/O"
-	depends on !S390
-
-config UIO
+menuconfig UIO
 	tristate "Userspace I/O drivers"
 	default n
+	depends on !S390
 	help
 	  Enable this to allow the userspace driver core code to be
 	  built.  This code allows userspace programs easy access to
@@ -25,5 +23,3 @@ config UIO_CIF
 
 	  To compile this driver as a module, choose M here: the module
 	  will be called uio_cif.
-
-endmenu
-- 
1.5.4.5

[PATCH 3/4] UIO: wrap all uio drivers in "if UIO" and "endif"

[Uwe Kleine-König]

currently there is only one driver, but new entries don't need to depend
explicitly on UIO.

Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
 drivers/uio/Kconfig |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index a899306..6bc2891 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -11,9 +11,11 @@ menuconfig UIO
 
 	  If you don't know what to do here, say N.
 
+if UIO
+
 config UIO_CIF
 	tristate "generic Hilscher CIF Card driver"
-	depends on UIO && PCI
+	depends on PCI
 	default n
 	help
 	  Driver for Hilscher CIF DeviceNet and Profibus cards.  This
@@ -23,3 +25,5 @@ config UIO_CIF
 
 	  To compile this driver as a module, choose M here: the module
 	  will be called uio_cif.
+
+endif
-- 
1.5.4.5

[PATCH 4/4] [RFC] UIO: generic platform driver

[Uwe Kleine-König]

Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
 drivers/uio/Kconfig    |    7 ++
 drivers/uio/Makefile   |    1 +
 drivers/uio/uio_pdrv.c |  165 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 173 insertions(+), 0 deletions(-)
 create mode 100644 drivers/uio/uio_pdrv.c

diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index 6bc2891..5ec353f 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -26,4 +26,11 @@ config UIO_CIF
 	  To compile this driver as a module, choose M here: the module
 	  will be called uio_cif.
 
+config UIO_PDRV
+	tristate "Userspace I/O platform driver"
+	help
+	  Generic platform driver for Userspace I/O devices.
+
+	  If you don't know what to do here, say N.
+
 endif
diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
index 7fecfb4..a6dcb99 100644
--- a/drivers/uio/Makefile
+++ b/drivers/uio/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_UIO)	+= uio.o
 obj-$(CONFIG_UIO_CIF)	+= uio_cif.o
+obj-$(CONFIG_UIO_PDRV) += uio_pdrv.o
diff --git a/drivers/uio/uio_pdrv.c b/drivers/uio/uio_pdrv.c
new file mode 100644
index 0000000..31d1aaf
--- /dev/null
+++ b/drivers/uio/uio_pdrv.c
@@ -0,0 +1,165 @@
+/*
+ * drivers/uio/uio_pdrv.c
+ *
+ * Copyright (C) 2008 by Digi International Inc.
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+#include <linux/clk.h>
+#include <linux/platform_device.h>
+#include <linux/uio_driver.h>
+
+#define DRIVER_NAME "uio"
+
+/* XXX: I thought there already exists something like STRINGIFY, but I could not
+ * find it ...
+ */
+#ifndef STRINGIFY
+#define STRINGIFY(x) __STRINGIFY_HELPER(x)
+#define __STRINGIFY_HELPER(x) #x
+#endif
+
+struct uio_platdata {
+	struct uio_info *uioinfo;
+	struct clk *clk;
+};
+
+static int uio_pdrv_open(struct uio_info *info, struct inode *inode)
+{
+	struct uio_platdata *pdata = info->priv;
+	int ret;
+
+	BUG_ON(pdata->uioinfo != info);
+
+	ret = clk_enable(pdata->clk);
+	if (ret)
+		/* XXX: better use dev_dbg, but which device should I use?
+		 * info->uio_dev->dev isn't accessible here as struct uio_device
+		 * is opaque.
+		 */
+		pr_debug("%s: err_clk_enable -> %d\n", __func__, ret);
+
+	return ret;
+}
+
+static int uio_pdrv_release(struct uio_info *info, struct inode *inode)
+{
+	struct uio_platdata *pdata = info->priv;
+
+	BUG_ON(pdata->uioinfo != info);
+
+	clk_disable(pdata->clk);
+
+	return 0;
+}
+
+static int uio_pdrv_probe(struct platform_device *pdev)
+{
+	struct uio_info *uioinfo = pdev->dev.platform_data;
+	struct uio_platdata *pdata;
+	struct uio_mem *uiomem;
+	int ret = -ENODEV;
+	int i;
+
+	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
+		dev_dbg(&pdev->dev, "%s: err_uioinfo\n", __func__);
+		goto err_uioinfo;
+	}
+
+	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+	if (!pdata) {
+		ret = -ENOMEM;
+		dev_dbg(&pdev->dev, "%s: err_alloc_pdata\n", __func__);
+		goto err_alloc_pdata;
+	}
+
+	pdata->uioinfo = uioinfo;
+
+	pdata->clk = clk_get(&pdev->dev, DRIVER_NAME);
+	if (IS_ERR(pdata->clk)) {
+		ret = PTR_ERR(pdata->clk);
+		dev_dbg(&pdev->dev, "%s: err_clk_get -> %d\n", __func__, ret);
+		goto err_clk_get;
+	}
+
+	uiomem = &uioinfo->mem[0];
+
+	for (i = 0; i < pdev->num_resources; ++i) {
+		struct resource *r = &pdev->resource[i];
+
+		if (r->flags != IORESOURCE_MEM)
+			continue;
+
+		if (uiomem >= &uioinfo->mem[MAX_UIO_MAPS]) {
+			dev_warn(&pdev->dev, "device has more than "
+					STRINGIFY(MAX_UIO_MAPS)
+					" I/O memory resources.\n");
+			break;
+		}
+
+		uiomem->memtype = UIO_MEM_PHYS;
+		uiomem->addr = r->start;
+		uiomem->size = r->end - r->start + 1;
+		++uiomem;
+	}
+
+	pdata->uioinfo->open = uio_pdrv_open;
+	pdata->uioinfo->release = uio_pdrv_release;
+	pdata->uioinfo->priv = pdata;
+
+	ret = uio_register_device(&pdev->dev, pdata->uioinfo);
+
+	if (ret) {
+		clk_put(pdata->clk);
+err_clk_get:
+
+		kfree(pdata);
+err_alloc_pdata:
+err_uioinfo:
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, pdata);
+
+	return 0;
+}
+
+static int uio_pdrv_remove(struct platform_device *pdev)
+{
+	struct uio_platdata *pdata = platform_get_drvdata(pdev);
+
+	uio_unregister_device(pdata->uioinfo);
+
+	clk_put(pdata->clk);
+
+	return 0;
+}
+
+static struct platform_driver uio_pdrv = {
+	.probe = uio_pdrv_probe,
+	.remove = uio_pdrv_remove,
+	.driver = {
+		.name = DRIVER_NAME,
+		.owner = THIS_MODULE,
+	},
+};
+
+static int __init uio_pdrv_init(void)
+{
+	return platform_driver_register(&uio_pdrv);
+}
+
+static void __exit uio_pdrv_exit(void)
+{
+	platform_driver_unregister(&uio_pdrv);
+}
+module_init(uio_pdrv_init);
+module_exit(uio_pdrv_exit);
+
+MODULE_AUTHOR("Uwe Kleine-Koenig");
+MODULE_DESCRIPTION("Userspace I/O platform driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" DRIVER_NAME);
-- 
1.5.4.5

Re: [PATCH 2/4] UIO: use menuconfig

[Hans J. Koch]

On Thu, Apr 10, 2008 at 02:37:01PM +0200, Uwe Kleine-König wrote:
> This allows configuring CONFIG_UIO without changing into the UIO submenu

I like it.

Signed-off-by: Hans J Koch <hjk@linutronix.de>

> 
> Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> ---
>  drivers/uio/Kconfig |    8 ++------
>  1 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> index b778ed7..a899306 100644
> --- a/drivers/uio/Kconfig
> +++ b/drivers/uio/Kconfig
> @@ -1,9 +1,7 @@
> -menu "Userspace I/O"
> -	depends on !S390
> -
> -config UIO
> +menuconfig UIO
>  	tristate "Userspace I/O drivers"
>  	default n
> +	depends on !S390
>  	help
>  	  Enable this to allow the userspace driver core code to be
>  	  built.  This code allows userspace programs easy access to
> @@ -25,5 +23,3 @@ config UIO_CIF
>  
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called uio_cif.
> -
> -endmenu
> -- 
> 1.5.4.5

Re: [PATCH 3/4] UIO: wrap all uio drivers in "if UIO" and "endif"

[Hans J. Koch]

On Thu, Apr 10, 2008 at 02:37:02PM +0200, Uwe Kleine-König wrote:
> currently there is only one driver, but new entries don't need to depend
> explicitly on UIO.

Agreed.

Signed-off-by: Hans J Koch <hjk@linutronix.de>

> 
> Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> ---
>  drivers/uio/Kconfig |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> index a899306..6bc2891 100644
> --- a/drivers/uio/Kconfig
> +++ b/drivers/uio/Kconfig
> @@ -11,9 +11,11 @@ menuconfig UIO
>  
>  	  If you don't know what to do here, say N.
>  
> +if UIO
> +
>  config UIO_CIF
>  	tristate "generic Hilscher CIF Card driver"
> -	depends on UIO && PCI
> +	depends on PCI
>  	default n
>  	help
>  	  Driver for Hilscher CIF DeviceNet and Profibus cards.  This
> @@ -23,3 +25,5 @@ config UIO_CIF
>  
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called uio_cif.
> +
> +endif
> -- 
> 1.5.4.5

Re: [PATCH 4/4] [RFC] UIO: generic platform driver

[Hans J. Koch]

On Thu, Apr 10, 2008 at 02:37:03PM +0200, Uwe Kleine-König wrote:
> Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> ---
>  drivers/uio/Kconfig    |    7 ++
>  drivers/uio/Makefile   |    1 +
>  drivers/uio/uio_pdrv.c |  165 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 173 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/uio/uio_pdrv.c

Hi Uwe,
I'm a bit slow today, I don't really understand what this is good for.
It's to complicated to serve as a template, and it doesn't support
interrupts, so it's not good for any real device, too. So the only
usecase would be an irq-less platform_device that just needs its memory
mapped. Is this what you intended? What do _you_ use it for?

Thanks,
Hans

> 
> diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> index 6bc2891..5ec353f 100644
> --- a/drivers/uio/Kconfig
> +++ b/drivers/uio/Kconfig
> @@ -26,4 +26,11 @@ config UIO_CIF
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called uio_cif.
>  
> +config UIO_PDRV
> +	tristate "Userspace I/O platform driver"
> +	help
> +	  Generic platform driver for Userspace I/O devices.
> +
> +	  If you don't know what to do here, say N.
> +
>  endif
> diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
> index 7fecfb4..a6dcb99 100644
> --- a/drivers/uio/Makefile
> +++ b/drivers/uio/Makefile
> @@ -1,2 +1,3 @@
>  obj-$(CONFIG_UIO)	+= uio.o
>  obj-$(CONFIG_UIO_CIF)	+= uio_cif.o
> +obj-$(CONFIG_UIO_PDRV) += uio_pdrv.o
> diff --git a/drivers/uio/uio_pdrv.c b/drivers/uio/uio_pdrv.c
> new file mode 100644
> index 0000000..31d1aaf
> --- /dev/null
> +++ b/drivers/uio/uio_pdrv.c
> @@ -0,0 +1,165 @@
> +/*
> + * drivers/uio/uio_pdrv.c
> + *
> + * Copyright (C) 2008 by Digi International Inc.
> + * All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +#include <linux/clk.h>
> +#include <linux/platform_device.h>
> +#include <linux/uio_driver.h>
> +
> +#define DRIVER_NAME "uio"
> +
> +/* XXX: I thought there already exists something like STRINGIFY, but I could not
> + * find it ...
> + */
> +#ifndef STRINGIFY
> +#define STRINGIFY(x) __STRINGIFY_HELPER(x)
> +#define __STRINGIFY_HELPER(x) #x
> +#endif
> +
> +struct uio_platdata {
> +	struct uio_info *uioinfo;
> +	struct clk *clk;
> +};
> +
> +static int uio_pdrv_open(struct uio_info *info, struct inode *inode)
> +{
> +	struct uio_platdata *pdata = info->priv;
> +	int ret;
> +
> +	BUG_ON(pdata->uioinfo != info);
> +
> +	ret = clk_enable(pdata->clk);
> +	if (ret)
> +		/* XXX: better use dev_dbg, but which device should I use?
> +		 * info->uio_dev->dev isn't accessible here as struct uio_device
> +		 * is opaque.
> +		 */
> +		pr_debug("%s: err_clk_enable -> %d\n", __func__, ret);
> +
> +	return ret;
> +}
> +
> +static int uio_pdrv_release(struct uio_info *info, struct inode *inode)
> +{
> +	struct uio_platdata *pdata = info->priv;
> +
> +	BUG_ON(pdata->uioinfo != info);
> +
> +	clk_disable(pdata->clk);
> +
> +	return 0;
> +}
> +
> +static int uio_pdrv_probe(struct platform_device *pdev)
> +{
> +	struct uio_info *uioinfo = pdev->dev.platform_data;
> +	struct uio_platdata *pdata;
> +	struct uio_mem *uiomem;
> +	int ret = -ENODEV;
> +	int i;
> +
> +	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
> +		dev_dbg(&pdev->dev, "%s: err_uioinfo\n", __func__);
> +		goto err_uioinfo;
> +	}
> +
> +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata) {
> +		ret = -ENOMEM;
> +		dev_dbg(&pdev->dev, "%s: err_alloc_pdata\n", __func__);
> +		goto err_alloc_pdata;
> +	}
> +
> +	pdata->uioinfo = uioinfo;
> +
> +	pdata->clk = clk_get(&pdev->dev, DRIVER_NAME);
> +	if (IS_ERR(pdata->clk)) {
> +		ret = PTR_ERR(pdata->clk);
> +		dev_dbg(&pdev->dev, "%s: err_clk_get -> %d\n", __func__, ret);
> +		goto err_clk_get;
> +	}
> +
> +	uiomem = &uioinfo->mem[0];
> +
> +	for (i = 0; i < pdev->num_resources; ++i) {
> +		struct resource *r = &pdev->resource[i];
> +
> +		if (r->flags != IORESOURCE_MEM)
> +			continue;
> +
> +		if (uiomem >= &uioinfo->mem[MAX_UIO_MAPS]) {
> +			dev_warn(&pdev->dev, "device has more than "
> +					STRINGIFY(MAX_UIO_MAPS)
> +					" I/O memory resources.\n");
> +			break;
> +		}
> +
> +		uiomem->memtype = UIO_MEM_PHYS;
> +		uiomem->addr = r->start;
> +		uiomem->size = r->end - r->start + 1;
> +		++uiomem;
> +	}
> +
> +	pdata->uioinfo->open = uio_pdrv_open;
> +	pdata->uioinfo->release = uio_pdrv_release;
> +	pdata->uioinfo->priv = pdata;
> +
> +	ret = uio_register_device(&pdev->dev, pdata->uioinfo);
> +
> +	if (ret) {
> +		clk_put(pdata->clk);
> +err_clk_get:
> +
> +		kfree(pdata);
> +err_alloc_pdata:
> +err_uioinfo:
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, pdata);
> +
> +	return 0;
> +}
> +
> +static int uio_pdrv_remove(struct platform_device *pdev)
> +{
> +	struct uio_platdata *pdata = platform_get_drvdata(pdev);
> +
> +	uio_unregister_device(pdata->uioinfo);
> +
> +	clk_put(pdata->clk);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver uio_pdrv = {
> +	.probe = uio_pdrv_probe,
> +	.remove = uio_pdrv_remove,
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.owner = THIS_MODULE,
> +	},
> +};
> +
> +static int __init uio_pdrv_init(void)
> +{
> +	return platform_driver_register(&uio_pdrv);
> +}
> +
> +static void __exit uio_pdrv_exit(void)
> +{
> +	platform_driver_unregister(&uio_pdrv);
> +}
> +module_init(uio_pdrv_init);
> +module_exit(uio_pdrv_exit);
> +
> +MODULE_AUTHOR("Uwe Kleine-Koenig");
> +MODULE_DESCRIPTION("Userspace I/O platform driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRIVER_NAME);
> -- 
> 1.5.4.5

Re: [PATCH 4/4] [RFC] UIO: generic platform driver

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1299 bytes --]

Hello Hans,

Hans J. Koch wrote:
> On Thu, Apr 10, 2008 at 02:37:03PM +0200, Uwe Kleine-König wrote:
> > Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> > ---
> >  drivers/uio/Kconfig    |    7 ++
> >  drivers/uio/Makefile   |    1 +
> >  drivers/uio/uio_pdrv.c |  165 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 173 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/uio/uio_pdrv.c
> 
> I'm a bit slow today, I don't really understand what this is good for.
> It's to complicated to serve as a template, and it doesn't support
> interrupts, so it's not good for any real device, too. So the only
> usecase would be an irq-less platform_device that just needs its memory
> mapped. Is this what you intended? What do _you_ use it for?
In my use case I don't use an irq, but you're free to provide a handler
when you create the platform device.  Attached is the file that provides
an uio platform device.  Current vanilla's support for ns9xxx isn't
(yet) complete enough to compile that, but you should be able to see how
it works.

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

[-- Attachment #2: plat-uio.c --]
[-- Type: text/x-csrc, Size: 2387 bytes --]

/*
 * arch/arm/mach-ns9xxx/plat-uio.c
 *
 * Copyright (C) 2008 by Digi International Inc.
 * All rights reserved.
 *
 * This program is free software; you can redistribute it and/or modify it
 * under the terms of the GNU General Public License version 2 as published by
 * the Free Software Foundation.
 */
#include <linux/err.h>
#include <linux/platform_device.h>
#include <linux/uio_driver.h>

#include <asm/mach-types.h>

#include "clock.h"

#define DRIVER_NAME "uio"

struct ns9xxx_uio_platdata {
	struct uio_info uioinfo;
	struct clk clk;
};

static struct platform_device *__init ns9xxx_plat_uio_alloc(
		struct ns9xxx_uio_platdata **pdata)
{
	struct platform_device *pdev;
	static int id;

	BUG_ON(!pdata);

	pdev = platform_device_alloc(DRIVER_NAME, id);
	if (!pdev)
		goto err;

	*pdata = kzalloc(sizeof(**pdata), GFP_KERNEL);
	if (!*pdata) {
err:
		platform_device_put(pdev);
		return ERR_PTR(-ENOMEM);
	}

	(*pdata)->clk.owner = THIS_MODULE;
	(*pdata)->clk.name = DRIVER_NAME;
	(*pdata)->clk.id = id;

	pdev->dev.platform_data = *pdata;

	++id;

	return pdev;
}

static int __init ns9xxx_plat_uio_inc20otter_init(void)
{
	struct ns9xxx_uio_platdata *uninitialized_var(pdata);
	struct platform_device *pdev = ns9xxx_plat_uio_alloc(&pdata);
	struct resource res[] = {
		{
			.start = 0x70000000,
			.end = 0x70001fff,
			.flags = IORESOURCE_MEM,
		}, {
			.start = 0xa0700000,
			.end = 0xa070027b,
			.flags = IORESOURCE_MEM,
		}
	};
	int ret;

	if (IS_ERR(pdev)) {
		ret = PTR_ERR(pdev);
		pr_debug("%s: err_alloc_pdev -> %d\n", __func__, ret);
		goto err_alloc_pdev;
	}

	ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res));
	if (ret) {
		pr_debug("%s: err_add_resources -> %d\n", __func__, ret);
		goto err_add_resources;
	}

	ret = clk_register(&pdata->clk);
	if (ret) {
		pr_debug("%s: err_clk_register -> %d", __func__, ret);
		goto err_clk_register;
	}

	pdata->uioinfo.name = "inc20otter CS3";
	pdata->uioinfo.version = "0.0a";
	pdata->uioinfo.irq = UIO_IRQ_NONE;

	ret = platform_device_add(pdev);
	if (ret) {

		clk_unregister(&pdata->clk);
err_clk_register:
err_add_resources:

		/* this kfrees pdata, too */
		platform_device_put(pdev);
err_alloc_pdev:
		return ret;
	}

	return 0;
}


static int __init ns9xxx_plat_uio_init(void)
{
	if (machine_is_inc20otter())
		ns9xxx_plat_uio_inc20otter_init();

	return 0;
}

arch_initcall(ns9xxx_plat_uio_init);

Re: [PATCH 1/4] UIO: hold a reference to the device's owner while the device is open

[Uwe Kleine-König]

Uwe Kleine-König wrote:
> Otherwise the device might just disappear while /dev/uioX is being used
> which results in an Oops.
To help your understanding, without this patch I can trigger the Oops
by starting an app that does

	opening and mmapping /dev/uio0
	while(1);

and then in another shell do 

	rmmod uio_pdrv

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH 1/4] UIO: hold a reference to the device's owner while the device is open

[Hans J. Koch]

On Thu, Apr 10, 2008 at 02:37:00PM +0200, Uwe Kleine-König wrote:
> Otherwise the device might just disappear while /dev/uioX is being used
> which results in an Oops.

Hi Uwe,
thanks for this one, good catch! Looks fine to me. There are some minor issues, see
below.
And I'd like to hear Greg's opinion: Do you agree we can omit
try_module_get() in uio_mmap()?

Thanks,
Hans

> 
> Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> ---
>  drivers/uio/uio.c |   40 +++++++++++++++++++++++-----------------
>  1 files changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index 1175908..005fc55 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -301,25 +301,35 @@ static int uio_open(struct inode *inode, struct file *filep)
>  	if (!idev)
>  		return -ENODEV;
>  
> +	if (!try_module_get(idev->owner)) {
> +		ret = -ENODEV;
> +		goto err_module_get;
> +	}
> +
>  	listener = kmalloc(sizeof(*listener), GFP_KERNEL);
> -	if (!listener)
> -		return -ENOMEM;
> +	if (!listener) {
> +		ret = -ENOMEM;
> +		goto err_alloc_listener;
> +	}
>  
>  	listener->dev = idev;
>  	listener->event_count = atomic_read(&idev->event);
>  	filep->private_data = listener;
>  
>  	if (idev->info->open) {
> -		if (!try_module_get(idev->owner))
> -			return -ENODEV;
>  		ret = idev->info->open(idev->info, inode);
> -		module_put(idev->owner);
> -	}
> +		if (ret) {
> +			kfree(listener);
> +err_alloc_listener:
>  
> -	if (ret)
> -		kfree(listener);
> +			module_put(idev->owner);
> +err_module_get:
>  
> -	return ret;
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
>  }

I really don't like these labels inside the if-block. I find it hard to
read. What about this:


if (idev->info->open) {
	ret = idev->info->open(idev->info, inode);
	if (ret)
		kfree(listener);
	return ret;
}

err_alloc_listener:
	module_put(idev->owner);
err_module_get:
	return ret;



The label err_module_get should probably be omitted because it's used only
once and has just one line of code. You could simply write "return ret"
instead of "goto err_module_get".

}


>  
>  static int uio_fasync(int fd, struct file *filep, int on)
> @@ -336,12 +346,11 @@ static int uio_release(struct inode *inode, struct file *filep)
>  	struct uio_listener *listener = filep->private_data;
>  	struct uio_device *idev = listener->dev;
>  
> -	if (idev->info->release) {
> -		if (!try_module_get(idev->owner))
> -			return -ENODEV;
> +	if (idev->info->release)
>  		ret = idev->info->release(idev->info, inode);
> -		module_put(idev->owner);
> -	}
> +
> +	module_put(idev->owner);
> +
>  	if (filep->f_flags & FASYNC)
>  		ret = uio_fasync(-1, filep, 0);
>  	kfree(listener);
> @@ -510,10 +519,7 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
>  		return -EINVAL;
>  
>  	if (idev->info->mmap) {
> -		if (!try_module_get(idev->owner))
> -			return -ENODEV;
>  		ret = idev->info->mmap(idev->info, vma);
> -		module_put(idev->owner);
>  		return ret;
>  	}
>  
> -- 
> 1.5.4.5

Re: [PATCH 1/4] UIO: hold a reference to the device's owner while the device is open

[Greg KH]

On Thu, Apr 10, 2008 at 11:02:29PM +0200, Hans J. Koch wrote:
> On Thu, Apr 10, 2008 at 02:37:00PM +0200, Uwe Kleine-K??nig wrote:
> > Otherwise the device might just disappear while /dev/uioX is being used
> > which results in an Oops.
> 
> Hi Uwe,
> thanks for this one, good catch! Looks fine to me. There are some minor issues, see
> below.
> And I'd like to hear Greg's opinion: Do you agree we can omit
> try_module_get() in uio_mmap()?

If it's already been grabbed in the open() call, everything should be
safe, right?

thanks,

greg k-h

Re: [PATCH 4/4] [RFC] UIO: generic platform driver

[Hans J. Koch]

On Thu, Apr 10, 2008 at 10:08:19PM +0200, Uwe Kleine-König wrote:
> Hello Hans,
> 
> Hans J. Koch wrote:
> > On Thu, Apr 10, 2008 at 02:37:03PM +0200, Uwe Kleine-König wrote:
> > > Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> > > ---
> > >  drivers/uio/Kconfig    |    7 ++
> > >  drivers/uio/Makefile   |    1 +
> > >  drivers/uio/uio_pdrv.c |  165 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 173 insertions(+), 0 deletions(-)
> > >  create mode 100644 drivers/uio/uio_pdrv.c
> > 
> > I'm a bit slow today, I don't really understand what this is good for.
> > It's to complicated to serve as a template, and it doesn't support
> > interrupts, so it's not good for any real device, too. So the only
> > usecase would be an irq-less platform_device that just needs its memory
> > mapped. Is this what you intended? What do _you_ use it for?
> In my use case I don't use an irq, but you're free to provide a handler
> when you create the platform device.

Hmm, I get the idea. You could have a UIO driver for a platform device
just by setting up a struct in the board support file. Nice thought.

Hmm. It's late, let me sleep over it... Tomorrow I'll look at it again.

Thanks,
Hans


> Attached is the file that provides
> an uio platform device.  Current vanilla's support for ns9xxx isn't
> (yet) complete enough to compile that, but you should be able to see how
> it works.
> 
> Best regards
> Uwe
> 
> -- 
> Uwe Kleine-König, Software Engineer
> Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
> Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

> /*
>  * arch/arm/mach-ns9xxx/plat-uio.c
>  *
>  * Copyright (C) 2008 by Digi International Inc.
>  * All rights reserved.
>  *
>  * This program is free software; you can redistribute it and/or modify it
>  * under the terms of the GNU General Public License version 2 as published by
>  * the Free Software Foundation.
>  */
> #include <linux/err.h>
> #include <linux/platform_device.h>
> #include <linux/uio_driver.h>
> 
> #include <asm/mach-types.h>
> 
> #include "clock.h"
> 
> #define DRIVER_NAME "uio"
> 
> struct ns9xxx_uio_platdata {
> 	struct uio_info uioinfo;
> 	struct clk clk;
> };
> 
> static struct platform_device *__init ns9xxx_plat_uio_alloc(
> 		struct ns9xxx_uio_platdata **pdata)
> {
> 	struct platform_device *pdev;
> 	static int id;
> 
> 	BUG_ON(!pdata);
> 
> 	pdev = platform_device_alloc(DRIVER_NAME, id);
> 	if (!pdev)
> 		goto err;
> 
> 	*pdata = kzalloc(sizeof(**pdata), GFP_KERNEL);
> 	if (!*pdata) {
> err:
> 		platform_device_put(pdev);
> 		return ERR_PTR(-ENOMEM);
> 	}
> 
> 	(*pdata)->clk.owner = THIS_MODULE;
> 	(*pdata)->clk.name = DRIVER_NAME;
> 	(*pdata)->clk.id = id;
> 
> 	pdev->dev.platform_data = *pdata;
> 
> 	++id;
> 
> 	return pdev;
> }
> 
> static int __init ns9xxx_plat_uio_inc20otter_init(void)
> {
> 	struct ns9xxx_uio_platdata *uninitialized_var(pdata);
> 	struct platform_device *pdev = ns9xxx_plat_uio_alloc(&pdata);
> 	struct resource res[] = {
> 		{
> 			.start = 0x70000000,
> 			.end = 0x70001fff,
> 			.flags = IORESOURCE_MEM,
> 		}, {
> 			.start = 0xa0700000,
> 			.end = 0xa070027b,
> 			.flags = IORESOURCE_MEM,
> 		}
> 	};
> 	int ret;
> 
> 	if (IS_ERR(pdev)) {
> 		ret = PTR_ERR(pdev);
> 		pr_debug("%s: err_alloc_pdev -> %d\n", __func__, ret);
> 		goto err_alloc_pdev;
> 	}
> 
> 	ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res));
> 	if (ret) {
> 		pr_debug("%s: err_add_resources -> %d\n", __func__, ret);
> 		goto err_add_resources;
> 	}
> 
> 	ret = clk_register(&pdata->clk);
> 	if (ret) {
> 		pr_debug("%s: err_clk_register -> %d", __func__, ret);
> 		goto err_clk_register;
> 	}
> 
> 	pdata->uioinfo.name = "inc20otter CS3";
> 	pdata->uioinfo.version = "0.0a";
> 	pdata->uioinfo.irq = UIO_IRQ_NONE;
> 
> 	ret = platform_device_add(pdev);
> 	if (ret) {
> 
> 		clk_unregister(&pdata->clk);
> err_clk_register:
> err_add_resources:
> 
> 		/* this kfrees pdata, too */
> 		platform_device_put(pdev);
> err_alloc_pdev:
> 		return ret;
> 	}
> 
> 	return 0;
> }
> 
> 
> static int __init ns9xxx_plat_uio_init(void)
> {
> 	if (machine_is_inc20otter())
> 		ns9xxx_plat_uio_inc20otter_init();
> 
> 	return 0;
> }
> 
> arch_initcall(ns9xxx_plat_uio_init);

Re: [PATCH 1/4] UIO: hold a reference to the device's owner while the device is open

[Hans J. Koch]

On Thu, Apr 10, 2008 at 02:12:36PM -0700, Greg KH wrote:
> On Thu, Apr 10, 2008 at 11:02:29PM +0200, Hans J. Koch wrote:
> > On Thu, Apr 10, 2008 at 02:37:00PM +0200, Uwe Kleine-K??nig wrote:
> > > Otherwise the device might just disappear while /dev/uioX is being used
> > > which results in an Oops.
> > 
> > Hi Uwe,
> > thanks for this one, good catch! Looks fine to me. There are some minor issues, see
> > below.
> > And I'd like to hear Greg's opinion: Do you agree we can omit
> > try_module_get() in uio_mmap()?
> 
> If it's already been grabbed in the open() call, everything should be
> safe, right?

Yes, it looks quite clean to me. module_get in open(), module_put in
release() and nothing of that sort anywhere else. Maybe it just looked
toooo simple to me ;-)

Thanks for your confirmation.

Hans

> 
> thanks,
> 
> greg k-h

Re: [PATCH 4/4] [RFC] UIO: generic platform driver

[Hans J. Koch]

On Thu, Apr 10, 2008 at 02:37:03PM +0200, Uwe Kleine-König wrote:
> Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> ---
>  drivers/uio/Kconfig    |    7 ++
>  drivers/uio/Makefile   |    1 +
>  drivers/uio/uio_pdrv.c |  165 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 173 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/uio/uio_pdrv.c

I looked it over again. Some comments below.
I'm beginning to like the idea...

Hans

> 
> diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> index 6bc2891..5ec353f 100644
> --- a/drivers/uio/Kconfig
> +++ b/drivers/uio/Kconfig
> @@ -26,4 +26,11 @@ config UIO_CIF
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called uio_cif.
>  
> +config UIO_PDRV
> +	tristate "Userspace I/O platform driver"
> +	help
> +	  Generic platform driver for Userspace I/O devices.
> +
> +	  If you don't know what to do here, say N.
> +
>  endif
> diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
> index 7fecfb4..a6dcb99 100644
> --- a/drivers/uio/Makefile
> +++ b/drivers/uio/Makefile
> @@ -1,2 +1,3 @@
>  obj-$(CONFIG_UIO)	+= uio.o
>  obj-$(CONFIG_UIO_CIF)	+= uio_cif.o
> +obj-$(CONFIG_UIO_PDRV) += uio_pdrv.o
> diff --git a/drivers/uio/uio_pdrv.c b/drivers/uio/uio_pdrv.c
> new file mode 100644
> index 0000000..31d1aaf
> --- /dev/null
> +++ b/drivers/uio/uio_pdrv.c
> @@ -0,0 +1,165 @@
> +/*
> + * drivers/uio/uio_pdrv.c
> + *
> + * Copyright (C) 2008 by Digi International Inc.
> + * All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +#include <linux/clk.h>
> +#include <linux/platform_device.h>
> +#include <linux/uio_driver.h>
> +
> +#define DRIVER_NAME "uio"
> +
> +/* XXX: I thought there already exists something like STRINGIFY, but I could not
> + * find it ...
> + */

Why do you want that macro? You only use it once. The macro definition and the
comment above are more than you can ever expect to save with it.

> +#ifndef STRINGIFY
> +#define STRINGIFY(x) __STRINGIFY_HELPER(x)
> +#define __STRINGIFY_HELPER(x) #x
> +#endif
> +
> +struct uio_platdata {
> +	struct uio_info *uioinfo;
> +	struct clk *clk;
> +};
> +
> +static int uio_pdrv_open(struct uio_info *info, struct inode *inode)
> +{
> +	struct uio_platdata *pdata = info->priv;
> +	int ret;
> +
> +	BUG_ON(pdata->uioinfo != info);

How can this BUG ever be triggered?

> +
> +	ret = clk_enable(pdata->clk);
> +	if (ret)
> +		/* XXX: better use dev_dbg, but which device should I use?
> +		 * info->uio_dev->dev isn't accessible here as struct uio_device
> +		 * is opaque.
> +		 */
> +		pr_debug("%s: err_clk_enable -> %d\n", __func__, ret);
> +
> +	return ret;
> +}
> +
> +static int uio_pdrv_release(struct uio_info *info, struct inode *inode)
> +{
> +	struct uio_platdata *pdata = info->priv;
> +
> +	BUG_ON(pdata->uioinfo != info);
> +
> +	clk_disable(pdata->clk);
> +
> +	return 0;
> +}
> +
> +static int uio_pdrv_probe(struct platform_device *pdev)
> +{
> +	struct uio_info *uioinfo = pdev->dev.platform_data;
> +	struct uio_platdata *pdata;
> +	struct uio_mem *uiomem;
> +	int ret = -ENODEV;
> +	int i;
> +
> +	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
> +		dev_dbg(&pdev->dev, "%s: err_uioinfo\n", __func__);
> +		goto err_uioinfo;
> +	}
> +
> +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata) {
> +		ret = -ENOMEM;
> +		dev_dbg(&pdev->dev, "%s: err_alloc_pdata\n", __func__);
> +		goto err_alloc_pdata;
> +	}
> +
> +	pdata->uioinfo = uioinfo;
> +
> +	pdata->clk = clk_get(&pdev->dev, DRIVER_NAME);
> +	if (IS_ERR(pdata->clk)) {
> +		ret = PTR_ERR(pdata->clk);
> +		dev_dbg(&pdev->dev, "%s: err_clk_get -> %d\n", __func__, ret);
> +		goto err_clk_get;
> +	}
> +
> +	uiomem = &uioinfo->mem[0];
> +
> +	for (i = 0; i < pdev->num_resources; ++i) {
> +		struct resource *r = &pdev->resource[i];

Please don't define new variables in the middle of a function.

> +
> +		if (r->flags != IORESOURCE_MEM)
> +			continue;
> +
> +		if (uiomem >= &uioinfo->mem[MAX_UIO_MAPS]) {

I'd prefer this:
		if (i >= MAX_UIO_MAPS) {
		...


> +			dev_warn(&pdev->dev, "device has more than "
> +					STRINGIFY(MAX_UIO_MAPS)
> +					" I/O memory resources.\n");

What about this:

			dev_warn(&pdev->dev, "device has more than %d"
				" I/O memory resources.\n", MAX_UIO_MAPS);

would save the macro.

> +			break;
> +		}
> +
> +		uiomem->memtype = UIO_MEM_PHYS;
> +		uiomem->addr = r->start;
> +		uiomem->size = r->end - r->start + 1;
> +		++uiomem;
> +	}
> +
> +	pdata->uioinfo->open = uio_pdrv_open;
> +	pdata->uioinfo->release = uio_pdrv_release;
> +	pdata->uioinfo->priv = pdata;
> +
> +	ret = uio_register_device(&pdev->dev, pdata->uioinfo);
> +
> +	if (ret) {
> +		clk_put(pdata->clk);
> +err_clk_get:
> +
> +		kfree(pdata);
> +err_alloc_pdata:
> +err_uioinfo:
> +		return ret;
> +	}

How I hate labels within blocks... OK, I admit, it looks good here...
Well...

> +
> +	platform_set_drvdata(pdev, pdata);
> +
> +	return 0;
> +}
> +
> +static int uio_pdrv_remove(struct platform_device *pdev)
> +{
> +	struct uio_platdata *pdata = platform_get_drvdata(pdev);
> +
> +	uio_unregister_device(pdata->uioinfo);
> +
> +	clk_put(pdata->clk);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver uio_pdrv = {
> +	.probe = uio_pdrv_probe,
> +	.remove = uio_pdrv_remove,
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.owner = THIS_MODULE,
> +	},
> +};
> +
> +static int __init uio_pdrv_init(void)
> +{
> +	return platform_driver_register(&uio_pdrv);
> +}
> +
> +static void __exit uio_pdrv_exit(void)
> +{
> +	platform_driver_unregister(&uio_pdrv);
> +}
> +module_init(uio_pdrv_init);
> +module_exit(uio_pdrv_exit);
> +
> +MODULE_AUTHOR("Uwe Kleine-Koenig");
> +MODULE_DESCRIPTION("Userspace I/O platform driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRIVER_NAME);
> -- 
> 1.5.4.5

Re: [PATCH 4/4] [RFC] UIO: generic platform driver

[Ben Nizette]

On Thu, 2008-04-10 at 23:17 +0200, Hans J. Koch wrote:
> On Thu, Apr 10, 2008 at 10:08:19PM +0200, Uwe Kleine-König wrote:
> > On Thu, Apr 10, 2008 at 02:37:03PM +0200, Uwe Kleine-König wrote:
> > > Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> > > ---
> > >  drivers/uio/Kconfig    |    7 ++
> > >  drivers/uio/Makefile   |    1 +
> > >  drivers/uio/uio_pdrv.c |  165 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 173 insertions(+), 0 deletions(-)
> > >  create mode 100644 drivers/uio/uio_pdrv.c
> > 
> > I'm a bit slow today, I don't really understand what this is good for.
> > It's to complicated to serve as a template, and it doesn't support
> > interrupts, so it's not good for any real device, too. So the only
> > usecase would be an irq-less platform_device that just needs its memory
> > mapped. Is this what you intended? What do _you_ use it for?

I've seen this kind of thing hacked up by a few people already, mainly
as a replacement for /dev/mem.  Many people are being scared off
using /dev/mem (and rightly so) because
- They've seen discussions regarding future plans whereby /dev/mem
wouldn't be allowed access to physical memory
- They don't have anything like X forcing them to have /dev/mem and
therefore want to disable it completely for (perceived?) security
reasons.

I like it, it'll sure be used.

	--Ben.

Re: [PATCH 4/4] [RFC] UIO: generic platform driver

[Uwe Kleine-König]

Hello Hans,

Hans J. Koch wrote:
> > +/* XXX: I thought there already exists something like STRINGIFY, but I could not
> > + * find it ...
> > + */
> 
> Why do you want that macro? You only use it once. The macro definition and the
> comment above are more than you can ever expect to save with it.
See below.
BTW, I found it, it's in linux/stringify.h.  And there are several
possible users:

	ukleinek@zentaur:~/gsrc/linux-2.6$ git ls-files -z | xargs -0 perl -n -e 'print "$ARGV\n" if (/define\s+\w+\((\w+)\)\s*#\s*$1/)'
	arch/arm/vfp/vfpinstr.h
	arch/cris/arch-v10/boot/tools/build.c
	arch/m68k/lib/checksum.c
	arch/mips/kernel/unaligned.c
	arch/powerpc/boot/reg.h
	arch/um/drivers/mconsole_user.c
	arch/um/include/sysdep-i386/kernel-offsets.h
	arch/um/include/sysdep-x86_64/kernel-offsets.h
	arch/x86/kernel/machine_kexec_32.c
	drivers/atm/ambassador.c
	drivers/media/video/cpia2/cpia2_v4l.c
	drivers/mtd/maps/amd76xrom.c
	drivers/mtd/maps/ichxrom.c
	drivers/s390/cio/qdio.h
	drivers/scsi/aacraid/aacraid.h
	drivers/scsi/aacraid/linit.c
	drivers/scsi/arm/acornscsi.c
	drivers/scsi/g_NCR5380.h
	drivers/uio/uio_pdrv.c
	drivers/usb/serial/garmin_gps.c
	include/asm-cris/arch-v10/irq.h
	include/asm-cris/arch-v32/hwregs/supp_reg.h
	include/asm-cris/arch-v32/irq.h
	include/asm-m32r/assembler.h
	include/asm-m68k/entry.h
	include/asm-mips/mipsregs.h
	include/asm-mips/sim.h
	include/asm-sh/cpu-sh5/registers.h
	include/asm-v850/macrology.h
	include/linux/stringify.h
	include/sound/core.h
	sound/core/memalloc.c
	usr/gen_init_cpio.c

> > +static int uio_pdrv_open(struct uio_info *info, struct inode *inode)
> > +{
> > +	struct uio_platdata *pdata = info->priv;
> > +	int ret;
> > +
> > +	BUG_ON(pdata->uioinfo != info);
> 
> How can this BUG ever be triggered?
I hope it cannot, that's why it is a bug if it happens. :-)  And one
should expect that no BUG_ON should ever be triggered.  I usually use
BUG_ON for "declaring" preconditions.
 
> > +	for (i = 0; i < pdev->num_resources; ++i) {
> > +		struct resource *r = &pdev->resource[i];
> 
> Please don't define new variables in the middle of a function.
This is a matter of taste.  In my eyes it's better to declare it here
because then it's easier to see where it's used.
 
> > +
> > +		if (r->flags != IORESOURCE_MEM)
> > +			continue;
> > +
> > +		if (uiomem >= &uioinfo->mem[MAX_UIO_MAPS]) {
> 
> I'd prefer this:
> 		if (i >= MAX_UIO_MAPS) {
> 		...
OK.

BTW would you be open to redefine uio_info as:

	struct uio_info {
		struct uio_device       *uio_dev;
		...
		size_t			num_memmaps;
		struct uio_mem		mem[];
	}

This allows to allocate exactly the number of members in the mem array
that are needed (for the cost of a size_t).  (You just do:

	uio_info uioinfo = kzalloc(sizeof(*uioinfo) + num_memmaps * sizeof(uioinfo->mem[0]), GFP_KERNEL);

it's still one chunk of memory and usage is similar---just with
MAX_UIO_MAPS substituted by uioinfo->num_memmaps.)
	
> > +			dev_warn(&pdev->dev, "device has more than "
> > +					STRINGIFY(MAX_UIO_MAPS)
> > +					" I/O memory resources.\n");
> 
> What about this:
> 
> 			dev_warn(&pdev->dev, "device has more than %d"
> 				" I/O memory resources.\n", MAX_UIO_MAPS);
> 
> would save the macro.
The macro is for free, using "%d" is not:

	ukleinek@zentaur:~/kbuild-ns921x$ size drivers/uio/uio_pdrv_with*
	   text    data     bss     dec     hex filename
	    808      72       0     880     370 drivers/uio/uio_pdrv_withoutstringify.o
	    800      72       0     872     368 drivers/uio/uio_pdrv_withstringify.o

You might consider that a bit over-engineered :-)

> > +	ret = uio_register_device(&pdev->dev, pdata->uioinfo);
> > +
> > +	if (ret) {
> > +		clk_put(pdata->clk);
> > +err_clk_get:
> > +
> > +		kfree(pdata);
> > +err_alloc_pdata:
> > +err_uioinfo:
> > +		return ret;
> > +	}
> 
> How I hate labels within blocks... OK, I admit, it looks good here...
> Well...
That's a matter of taste, too, I like it that way.  Probably it doesn't
matter for the compiler (I havn't tried), but this way it's one goto
less.  And if it doesn't matter for the compiler it's at least nice for
the reader.  Though obviously YMMV.

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH 1/4] UIO: hold a reference to the device's owner while the device is open

[Uwe Kleine-König]

Hello,

Hans J. Koch wrote:
> On Thu, Apr 10, 2008 at 02:37:00PM +0200, Uwe Kleine-König wrote:
> > Otherwise the device might just disappear while /dev/uioX is being used
> > which results in an Oops.
> 
> And I'd like to hear Greg's opinion: Do you agree we can omit
> try_module_get() in uio_mmap()?
As Greg already pointed out, mmap only works for open files and so the
reference is already hold there.
 
> >  	if (idev->info->open) {
> > -		if (!try_module_get(idev->owner))
> > -			return -ENODEV;
> >  		ret = idev->info->open(idev->info, inode);
> > -		module_put(idev->owner);
> > -	}
> > +		if (ret) {
> > +			kfree(listener);
> > +err_alloc_listener:
> >  
> > -	if (ret)
> > -		kfree(listener);
> > +			module_put(idev->owner);
> > +err_module_get:
> >  
> > -	return ret;
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	return 0;
> >  }
> 
> I really don't like these labels inside the if-block. I find it hard to
> read. What about this:
> 
> 
> if (idev->info->open) {
> 	ret = idev->info->open(idev->info, inode);
> 	if (ret)
> 		kfree(listener);
> 	return ret;
> }
> 
> err_alloc_listener:
> 	module_put(idev->owner);
> err_module_get:
> 	return ret;
With that you leak a reference to idev->owner if idev->info->open() fails.
Things like that don't happen that easy if all error handing is in one
place.
 
> The label err_module_get should probably be omitted because it's used only
> once and has just one line of code. You could simply write "return ret"
> instead of "goto err_module_get".
This makes code shuffling easier.  For example if someone decides that
try_module_get should be done after allocating listener then you only
have to exchange the two corresponding code blocks and the two groups
(label + cleanup) in the error handling block.
If the error handling is spread over the whole functions you can easily
miss something---as happend above. :-)

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH 1/4] UIO: hold a reference to the device's owner while the device is open

[Hans J. Koch]

On Fri, Apr 11, 2008 at 08:50:27AM +0200, Uwe Kleine-König wrote:
> > And I'd like to hear Greg's opinion: Do you agree we can omit
> > try_module_get() in uio_mmap()?
> As Greg already pointed out, mmap only works for open files and so the
> reference is already hold there.

Yes, that's OK.

>  
> > >  	if (idev->info->open) {
> > > -		if (!try_module_get(idev->owner))
> > > -			return -ENODEV;
> > >  		ret = idev->info->open(idev->info, inode);
> > > -		module_put(idev->owner);
> > > -	}
> > > +		if (ret) {
> > > +			kfree(listener);
> > > +err_alloc_listener:
> > >  
> > > -	if (ret)
> > > -		kfree(listener);
> > > +			module_put(idev->owner);
> > > +err_module_get:
> > >  
> > > -	return ret;
> > > +			return ret;
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > >  }
> > 
> > I really don't like these labels inside the if-block. I find it hard to
> > read. What about this:
> > 
> > 
> > if (idev->info->open) {
> > 	ret = idev->info->open(idev->info, inode);
> > 	if (ret)
> > 		kfree(listener);
> > 	return ret;
> > }
> > 
> > err_alloc_listener:
> > 	module_put(idev->owner);
> > err_module_get:
> > 	return ret;
> With that you leak a reference to idev->owner if idev->info->open() fails.
> Things like that don't happen that easy if all error handing is in one
> place.

Maybe. It's merely an example to explain what I mean.
Documentation/CodingStyle says nothing about how to place labels, but I
find it best to have all error path exits at the end of a function. All
the UIO code does it that way.

>  
> > The label err_module_get should probably be omitted because it's used only
> > once and has just one line of code. You could simply write "return ret"
> > instead of "goto err_module_get".
> This makes code shuffling easier.  For example if someone decides that
> try_module_get should be done after allocating listener then you only
> have to exchange the two corresponding code blocks and the two groups
> (label + cleanup) in the error handling block.
> If the error handling is spread over the whole functions you can easily
> miss something---as happend above. :-)

Well, it depends. It's all about readability. Any function should be
written in a way that makes it as clear as possible what it does. Your
code is certainly not critical regarding that aspect, but I think it can
still be improved. And a label that is used only once and contains only
one line of code is definetly unnecessary. I don't follow the
maybe-one-day-in-the-future-it-might-be-useful philosophy. I like code
that is as clean and readable as possible _now_. And as this patch is
not just a driver but affects the UIO core, this is even more important.

Could you please send an updated patch?

Thanks,
Hans

[PATCH 1/4 v2] UIO: hold a reference to the device's owner while the device is open

[Uwe Kleine-König]

Otherwise the device might just disappear while /dev/uioX is being used
which results in an Oops.

Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---

Hans J. Koch wrote:
> > > The label err_module_get should probably be omitted because it's used only
> > > once and has just one line of code. You could simply write "return ret"
> > > instead of "goto err_module_get".
> > This makes code shuffling easier.  For example if someone decides that
> > try_module_get should be done after allocating listener then you only
> > have to exchange the two corresponding code blocks and the two groups
> > (label + cleanup) in the error handling block.
> > If the error handling is spread over the whole functions you can easily
> > miss something---as happend above. :-)
> 
> Well, it depends. It's all about readability. Any function should be
> written in a way that makes it as clear as possible what it does. Your
> code is certainly not critical regarding that aspect, but I think it can
> still be improved. And a label that is used only once and contains only
> one line of code is definetly unnecessary. I don't follow the
> maybe-one-day-in-the-future-it-might-be-useful philosophy. I like code
> that is as clean and readable as possible _now_.
That thing about code reordering is minor compared to having all error
handling in one place, but ...

>                                                  And as this patch is
> not just a driver but affects the UIO core, this is even more important.
> 
> Could you please send an updated patch?
... , it's your code, so you can find a new version below.

Best regards
Uwe

 drivers/uio/uio.c |   36 +++++++++++++++++++++---------------
 1 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 1175908..55cc7b8 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -301,23 +301,33 @@ static int uio_open(struct inode *inode, struct file *filep)
 	if (!idev)
 		return -ENODEV;
 
+	if (!try_module_get(idev->owner))
+		return -ENODEV;
+
 	listener = kmalloc(sizeof(*listener), GFP_KERNEL);
-	if (!listener)
-		return -ENOMEM;
+	if (!listener) {
+		ret = -ENOMEM;
+		goto err_alloc_listener;
+	}
 
 	listener->dev = idev;
 	listener->event_count = atomic_read(&idev->event);
 	filep->private_data = listener;
 
 	if (idev->info->open) {
-		if (!try_module_get(idev->owner))
-			return -ENODEV;
 		ret = idev->info->open(idev->info, inode);
-		module_put(idev->owner);
+		if (ret)
+			goto err_infoopen;
 	}
 
-	if (ret)
-		kfree(listener);
+	return 0;
+
+err_infoopen:
+
+	kfree(listener);
+err_alloc_listener:
+
+	module_put(idev->owner);
 
 	return ret;
 }
@@ -336,12 +346,11 @@ static int uio_release(struct inode *inode, struct file *filep)
 	struct uio_listener *listener = filep->private_data;
 	struct uio_device *idev = listener->dev;
 
-	if (idev->info->release) {
-		if (!try_module_get(idev->owner))
-			return -ENODEV;
+	if (idev->info->release)
 		ret = idev->info->release(idev->info, inode);
-		module_put(idev->owner);
-	}
+
+	module_put(idev->owner);
+
 	if (filep->f_flags & FASYNC)
 		ret = uio_fasync(-1, filep, 0);
 	kfree(listener);
@@ -510,10 +519,7 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
 		return -EINVAL;
 
 	if (idev->info->mmap) {
-		if (!try_module_get(idev->owner))
-			return -ENODEV;
 		ret = idev->info->mmap(idev->info, vma);
-		module_put(idev->owner);
 		return ret;
 	}
 
-- 
1.5.4.5


-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

[PATCH 4/4 v2] [RFC] UIO: generic platform driver

[Uwe Kleine-König]

Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
Hello,

> > > +	for (i = 0; i < pdev->num_resources; ++i) {
> > > +		struct resource *r = &pdev->resource[i];
> > > +
> > > +		if (r->flags != IORESOURCE_MEM)
> > > +			continue;
> > > +
> > > +		if (uiomem >= &uioinfo->mem[MAX_UIO_MAPS]) {
> > 
> > I'd prefer this:
> > 		if (i >= MAX_UIO_MAPS) {
> > 		...
> OK.
While reworking the code I saw that your variant is not correct because
if pdev has resources with flags != IORESOURCE_MEM the constraint

	uiomem == &uioinfo->mem[i]

doesn't hold.

Below is a new version that uses linux/stringify and zeros size for
unused mappings (line 102ff).

Best regards
Uwe

 drivers/uio/Kconfig    |    7 ++
 drivers/uio/Makefile   |    1 +
 drivers/uio/uio_pdrv.c |  163 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 171 insertions(+), 0 deletions(-)
 create mode 100644 drivers/uio/uio_pdrv.c

diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index 6bc2891..5ec353f 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -26,4 +26,11 @@ config UIO_CIF
 	  To compile this driver as a module, choose M here: the module
 	  will be called uio_cif.
 
+config UIO_PDRV
+	tristate "Userspace I/O platform driver"
+	help
+	  Generic platform driver for Userspace I/O devices.
+
+	  If you don't know what to do here, say N.
+
 endif
diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
index 7fecfb4..a6dcb99 100644
--- a/drivers/uio/Makefile
+++ b/drivers/uio/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_UIO)	+= uio.o
 obj-$(CONFIG_UIO_CIF)	+= uio_cif.o
+obj-$(CONFIG_UIO_PDRV) += uio_pdrv.o
diff --git a/drivers/uio/uio_pdrv.c b/drivers/uio/uio_pdrv.c
new file mode 100644
index 0000000..47506aa
--- /dev/null
+++ b/drivers/uio/uio_pdrv.c
@@ -0,0 +1,163 @@
+/*
+ * drivers/uio/uio_pdrv.c
+ *
+ * Copyright (C) 2008 by Digi International Inc.
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+#include <linux/clk.h>
+#include <linux/platform_device.h>
+#include <linux/uio_driver.h>
+#include <linux/stringify.h>
+
+#define DRIVER_NAME "uio"
+
+struct uio_platdata {
+	struct uio_info *uioinfo;
+	struct clk *clk;
+};
+
+static int uio_pdrv_open(struct uio_info *info, struct inode *inode)
+{
+	struct uio_platdata *pdata = info->priv;
+	int ret;
+
+	BUG_ON(pdata->uioinfo != info);
+
+	ret = clk_enable(pdata->clk);
+	if (ret)
+		/* XXX: better use dev_dbg, but which device should I use?
+		 * info->uio_dev->dev isn't accessible here as struct uio_device
+		 * is opaque.
+		 */
+		pr_debug("%s: err_clk_enable -> %d\n", __func__, ret);
+
+	return ret;
+}
+
+static int uio_pdrv_release(struct uio_info *info, struct inode *inode)
+{
+	struct uio_platdata *pdata = info->priv;
+
+	BUG_ON(pdata->uioinfo != info);
+
+	clk_disable(pdata->clk);
+
+	return 0;
+}
+
+static int uio_pdrv_probe(struct platform_device *pdev)
+{
+	struct uio_info *uioinfo = pdev->dev.platform_data;
+	struct uio_platdata *pdata;
+	struct uio_mem *uiomem;
+	int ret = -ENODEV;
+	int i;
+
+	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
+		dev_dbg(&pdev->dev, "%s: err_uioinfo\n", __func__);
+		goto err_uioinfo;
+	}
+
+	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+	if (!pdata) {
+		ret = -ENOMEM;
+		dev_dbg(&pdev->dev, "%s: err_alloc_pdata\n", __func__);
+		goto err_alloc_pdata;
+	}
+
+	pdata->uioinfo = uioinfo;
+
+	pdata->clk = clk_get(&pdev->dev, DRIVER_NAME);
+	if (IS_ERR(pdata->clk)) {
+		ret = PTR_ERR(pdata->clk);
+		dev_dbg(&pdev->dev, "%s: err_clk_get -> %d\n", __func__, ret);
+		goto err_clk_get;
+	}
+
+	uiomem = &uioinfo->mem[0];
+
+	for (i = 0; i < pdev->num_resources; ++i) {
+		struct resource *r = &pdev->resource[i];
+
+		if (r->flags != IORESOURCE_MEM)
+			continue;
+
+		if (uiomem >= &uioinfo->mem[MAX_UIO_MAPS]) {
+			dev_warn(&pdev->dev, "device has more than "
+					stringify(MAX_UIO_MAPS)
+					" I/O memory resources.\n");
+			break;
+		}
+
+		uiomem->memtype = UIO_MEM_PHYS;
+		uiomem->addr = r->start;
+		uiomem->size = r->end - r->start + 1;
+		++uiomem;
+	}
+
+	while (uiomem < &uioinfo->mem[MAX_UIO_MAPS]) {
+		uiomem->size = 0;
+		++uiomem;
+	}
+
+	pdata->uioinfo->open = uio_pdrv_open;
+	pdata->uioinfo->release = uio_pdrv_release;
+	pdata->uioinfo->priv = pdata;
+
+	ret = uio_register_device(&pdev->dev, pdata->uioinfo);
+
+	if (ret) {
+		clk_put(pdata->clk);
+err_clk_get:
+
+		kfree(pdata);
+err_alloc_pdata:
+err_uioinfo:
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, pdata);
+
+	return 0;
+}
+
+static int uio_pdrv_remove(struct platform_device *pdev)
+{
+	struct uio_platdata *pdata = platform_get_drvdata(pdev);
+
+	uio_unregister_device(pdata->uioinfo);
+
+	clk_put(pdata->clk);
+
+	return 0;
+}
+
+static struct platform_driver uio_pdrv = {
+	.probe = uio_pdrv_probe,
+	.remove = uio_pdrv_remove,
+	.driver = {
+		.name = DRIVER_NAME,
+		.owner = THIS_MODULE,
+	},
+};
+
+static int __init uio_pdrv_init(void)
+{
+	return platform_driver_register(&uio_pdrv);
+}
+
+static void __exit uio_pdrv_exit(void)
+{
+	platform_driver_unregister(&uio_pdrv);
+}
+module_init(uio_pdrv_init);
+module_exit(uio_pdrv_exit);
+
+MODULE_AUTHOR("Uwe Kleine-Koenig");
+MODULE_DESCRIPTION("Userspace I/O platform driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" DRIVER_NAME);
-- 
1.5.4.5



-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH 4/4] [RFC] UIO: generic platform driver

[Hans J. Koch]

On Fri, Apr 11, 2008 at 08:21:06AM +0200, Uwe Kleine-König wrote:
> Hello Hans,
> 
> Hans J. Koch wrote:
> > > +/* XXX: I thought there already exists something like STRINGIFY, but I could not
> > > + * find it ...
> > > + */
> > 
> > Why do you want that macro? You only use it once. The macro definition and the
> > comment above are more than you can ever expect to save with it.
> See below.
> BTW, I found it, it's in linux/stringify.h.  And there are several
> possible users:

All right, I won't argue about this one. If you like it, use it.

> 
> > > +static int uio_pdrv_open(struct uio_info *info, struct inode *inode)
> > > +{
> > > +	struct uio_platdata *pdata = info->priv;
> > > +	int ret;
> > > +
> > > +	BUG_ON(pdata->uioinfo != info);
> > 
> > How can this BUG ever be triggered?
> I hope it cannot, that's why it is a bug if it happens. :-)  And one
> should expect that no BUG_ON should ever be triggered.  I usually use
> BUG_ON for "declaring" preconditions.

OK, as this is a generic driver where we don't know what crap people
will pass in, it might be justified. OK.

>  
> > > +	for (i = 0; i < pdev->num_resources; ++i) {
> > > +		struct resource *r = &pdev->resource[i];
> > 
> > Please don't define new variables in the middle of a function.
> This is a matter of taste.  In my eyes it's better to declare it here
> because then it's easier to see where it's used.

No. It's more important to see which variables are declared in the
function and which are declared elsewhere. If you have to search the
whole body of a function for possible declarations, this is BAD. And if
it's not clear where a variable is used, the function is too long or has
other style problems. Your function is short and clean, so where's the
problem? Please move the declaration to the top of the function.

>  
> > > +
> > > +		if (r->flags != IORESOURCE_MEM)
> > > +			continue;
> > > +
> > > +		if (uiomem >= &uioinfo->mem[MAX_UIO_MAPS]) {
> > 
> > I'd prefer this:
> > 		if (i >= MAX_UIO_MAPS) {
> > 		...
> OK.
> 
> BTW would you be open to redefine uio_info as:
> 
> 	struct uio_info {
> 		struct uio_device       *uio_dev;
> 		...
> 		size_t			num_memmaps;
> 		struct uio_mem		mem[];
> 	}
> 
> This allows to allocate exactly the number of members in the mem array
> that are needed (for the cost of a size_t).  (You just do:
> 
> 	uio_info uioinfo = kzalloc(sizeof(*uioinfo) + num_memmaps * sizeof(uioinfo->mem[0]), GFP_KERNEL);
> 
> it's still one chunk of memory and usage is similar---just with
> MAX_UIO_MAPS substituted by uioinfo->num_memmaps.)

I don't like it. It makes things more complicated without any obvious
gain. sizeof(struct uio_info) would return wrong values, you need to
free the extra memory, userspace applications need to be able to deal
with 10000 mappings...

If there's an actual usecase where 5 mappings are not enough, we can
talk about increasing MAX_UIO_MAPS to some other value.

> 	
> > > +			dev_warn(&pdev->dev, "device has more than "
> > > +					STRINGIFY(MAX_UIO_MAPS)
> > > +					" I/O memory resources.\n");
> > 
> > What about this:
> > 
> > 			dev_warn(&pdev->dev, "device has more than %d"
> > 				" I/O memory resources.\n", MAX_UIO_MAPS);
> > 
> > would save the macro.
> The macro is for free, using "%d" is not:
> 
> 	ukleinek@zentaur:~/kbuild-ns921x$ size drivers/uio/uio_pdrv_with*
> 	   text    data     bss     dec     hex filename
> 	    808      72       0     880     370 drivers/uio/uio_pdrv_withoutstringify.o
> 	    800      72       0     872     368 drivers/uio/uio_pdrv_withstringify.o
> 
> You might consider that a bit over-engineered :-)

As I said above, feel free to use it.

> 
> > > +	ret = uio_register_device(&pdev->dev, pdata->uioinfo);
> > > +
> > > +	if (ret) {
> > > +		clk_put(pdata->clk);
> > > +err_clk_get:
> > > +
> > > +		kfree(pdata);
> > > +err_alloc_pdata:
> > > +err_uioinfo:
> > > +		return ret;
> > > +	}
> > 
> > How I hate labels within blocks... OK, I admit, it looks good here...
> > Well...
> That's a matter of taste, too, I like it that way.  Probably it doesn't
> matter for the compiler (I havn't tried), but this way it's one goto
> less.  And if it doesn't matter for the compiler it's at least nice for
> the reader.  Though obviously YMMV.

As I said, it looks OK here. You can keep it if you like it.

I'd like to thank you for your work. After giving it some thought, I
really like the idea of having a generic UIO driver for platform
devices. I think many people (including /me) will use it. So, please send
an updated patch, I think we should push it to mainline.

Thanks,
Hans

Re: [PATCH 4/4 v2] [RFC] UIO: generic platform driver

[Hans J. Koch]

On Fri, Apr 11, 2008 at 11:21:58AM +0200, Uwe Kleine-König wrote:
> 
> Below is a new version that uses linux/stringify and zeros size for
> unused mappings (line 102ff).

Thanks, but it doesn't compile, neither with -rc8 nor with Linus' git.

One problem can easily be fixed, the macro is called __stringify, not
stringify.

But what about this:

ERROR: "clk_get" [drivers/uio/uio_pdrv.ko] undefined!
ERROR: "clk_enable" [drivers/uio/uio_pdrv.ko] undefined!
ERROR: "clk_disable" [drivers/uio/uio_pdrv.ko] undefined!
ERROR: "clk_put" [drivers/uio/uio_pdrv.ko] undefined!

Do you have any extra patches applied?
Against which kernel do you test?

Thanks,
Hans


> 
> Best regards
> Uwe
> 
>  drivers/uio/Kconfig    |    7 ++
>  drivers/uio/Makefile   |    1 +
>  drivers/uio/uio_pdrv.c |  163 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 171 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/uio/uio_pdrv.c
> 
> diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> index 6bc2891..5ec353f 100644
> --- a/drivers/uio/Kconfig
> +++ b/drivers/uio/Kconfig
> @@ -26,4 +26,11 @@ config UIO_CIF
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called uio_cif.
>  
> +config UIO_PDRV
> +	tristate "Userspace I/O platform driver"
> +	help
> +	  Generic platform driver for Userspace I/O devices.
> +
> +	  If you don't know what to do here, say N.
> +
>  endif
> diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
> index 7fecfb4..a6dcb99 100644
> --- a/drivers/uio/Makefile
> +++ b/drivers/uio/Makefile
> @@ -1,2 +1,3 @@
>  obj-$(CONFIG_UIO)	+= uio.o
>  obj-$(CONFIG_UIO_CIF)	+= uio_cif.o
> +obj-$(CONFIG_UIO_PDRV) += uio_pdrv.o
> diff --git a/drivers/uio/uio_pdrv.c b/drivers/uio/uio_pdrv.c
> new file mode 100644
> index 0000000..47506aa
> --- /dev/null
> +++ b/drivers/uio/uio_pdrv.c
> @@ -0,0 +1,163 @@
> +/*
> + * drivers/uio/uio_pdrv.c
> + *
> + * Copyright (C) 2008 by Digi International Inc.
> + * All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +#include <linux/clk.h>
> +#include <linux/platform_device.h>
> +#include <linux/uio_driver.h>
> +#include <linux/stringify.h>
> +
> +#define DRIVER_NAME "uio"
> +
> +struct uio_platdata {
> +	struct uio_info *uioinfo;
> +	struct clk *clk;
> +};
> +
> +static int uio_pdrv_open(struct uio_info *info, struct inode *inode)
> +{
> +	struct uio_platdata *pdata = info->priv;
> +	int ret;
> +
> +	BUG_ON(pdata->uioinfo != info);
> +
> +	ret = clk_enable(pdata->clk);
> +	if (ret)
> +		/* XXX: better use dev_dbg, but which device should I use?
> +		 * info->uio_dev->dev isn't accessible here as struct uio_device
> +		 * is opaque.
> +		 */
> +		pr_debug("%s: err_clk_enable -> %d\n", __func__, ret);
> +
> +	return ret;
> +}
> +
> +static int uio_pdrv_release(struct uio_info *info, struct inode *inode)
> +{
> +	struct uio_platdata *pdata = info->priv;
> +
> +	BUG_ON(pdata->uioinfo != info);
> +
> +	clk_disable(pdata->clk);
> +
> +	return 0;
> +}
> +
> +static int uio_pdrv_probe(struct platform_device *pdev)
> +{
> +	struct uio_info *uioinfo = pdev->dev.platform_data;
> +	struct uio_platdata *pdata;
> +	struct uio_mem *uiomem;
> +	int ret = -ENODEV;
> +	int i;
> +
> +	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
> +		dev_dbg(&pdev->dev, "%s: err_uioinfo\n", __func__);
> +		goto err_uioinfo;
> +	}
> +
> +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata) {
> +		ret = -ENOMEM;
> +		dev_dbg(&pdev->dev, "%s: err_alloc_pdata\n", __func__);
> +		goto err_alloc_pdata;
> +	}
> +
> +	pdata->uioinfo = uioinfo;
> +
> +	pdata->clk = clk_get(&pdev->dev, DRIVER_NAME);
> +	if (IS_ERR(pdata->clk)) {
> +		ret = PTR_ERR(pdata->clk);
> +		dev_dbg(&pdev->dev, "%s: err_clk_get -> %d\n", __func__, ret);
> +		goto err_clk_get;
> +	}
> +
> +	uiomem = &uioinfo->mem[0];
> +
> +	for (i = 0; i < pdev->num_resources; ++i) {
> +		struct resource *r = &pdev->resource[i];
> +
> +		if (r->flags != IORESOURCE_MEM)
> +			continue;
> +
> +		if (uiomem >= &uioinfo->mem[MAX_UIO_MAPS]) {
> +			dev_warn(&pdev->dev, "device has more than "
> +					stringify(MAX_UIO_MAPS)
> +					" I/O memory resources.\n");
> +			break;
> +		}
> +
> +		uiomem->memtype = UIO_MEM_PHYS;
> +		uiomem->addr = r->start;
> +		uiomem->size = r->end - r->start + 1;
> +		++uiomem;
> +	}
> +
> +	while (uiomem < &uioinfo->mem[MAX_UIO_MAPS]) {
> +		uiomem->size = 0;
> +		++uiomem;
> +	}
> +
> +	pdata->uioinfo->open = uio_pdrv_open;
> +	pdata->uioinfo->release = uio_pdrv_release;
> +	pdata->uioinfo->priv = pdata;
> +
> +	ret = uio_register_device(&pdev->dev, pdata->uioinfo);
> +
> +	if (ret) {
> +		clk_put(pdata->clk);
> +err_clk_get:
> +
> +		kfree(pdata);
> +err_alloc_pdata:
> +err_uioinfo:
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, pdata);
> +
> +	return 0;
> +}
> +
> +static int uio_pdrv_remove(struct platform_device *pdev)
> +{
> +	struct uio_platdata *pdata = platform_get_drvdata(pdev);
> +
> +	uio_unregister_device(pdata->uioinfo);
> +
> +	clk_put(pdata->clk);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver uio_pdrv = {
> +	.probe = uio_pdrv_probe,
> +	.remove = uio_pdrv_remove,
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.owner = THIS_MODULE,
> +	},
> +};
> +
> +static int __init uio_pdrv_init(void)
> +{
> +	return platform_driver_register(&uio_pdrv);
> +}
> +
> +static void __exit uio_pdrv_exit(void)
> +{
> +	platform_driver_unregister(&uio_pdrv);
> +}
> +module_init(uio_pdrv_init);
> +module_exit(uio_pdrv_exit);
> +
> +MODULE_AUTHOR("Uwe Kleine-Koenig");
> +MODULE_DESCRIPTION("Userspace I/O platform driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRIVER_NAME);
> -- 
> 1.5.4.5
> 
> 
> 
> -- 
> Uwe Kleine-König, Software Engineer
> Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
> Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH 4/4] [RFC] UIO: generic platform driver

[Uwe Kleine-König]

Hello Hans,

Hans J. Koch wrote:
> On Fri, Apr 11, 2008 at 08:21:06AM +0200, Uwe Kleine-König wrote:
> > > > +	for (i = 0; i < pdev->num_resources; ++i) {
> > > > +		struct resource *r = &pdev->resource[i];
> > > 
> > > Please don't define new variables in the middle of a function.
> > This is a matter of taste.  In my eyes it's better to declare it here
> > because then it's easier to see where it's used.
> 
> No. It's more important to see which variables are declared in the
> function and which are declared elsewhere. If you have to search the
> whole body of a function for possible declarations, this is BAD. And if
> it's not clear where a variable is used, the function is too long or has
> other style problems. Your function is short and clean, so where's the
> problem? Please move the declaration to the top of the function.
I'm not conviced and still prefer it that way.  I gave way for your
requests in uio.c because it's your code.  I want to leave it as it is
and hope you will accept that (as this is my code).

> > BTW would you be open to redefine uio_info as:
> > 
> > 	struct uio_info {
> > 		struct uio_device       *uio_dev;
> > 		...
> > 		size_t			num_memmaps;
> > 		struct uio_mem		mem[];
> > 	}
> > 
> > This allows to allocate exactly the number of members in the mem array
> > that are needed (for the cost of a size_t).  (You just do:
> > 
> > 	uio_info uioinfo = kzalloc(sizeof(*uioinfo) + num_memmaps * sizeof(uioinfo->mem[0]), GFP_KERNEL);
> > 
> > it's still one chunk of memory and usage is similar---just with
> > MAX_UIO_MAPS substituted by uioinfo->num_memmaps.)
> 
> I don't like it. It makes things more complicated without any obvious
> gain.
Most use cases I imagine only use a single mapping, so the gain would be
to save 4 (or later more) 'struct uio_mem's per device.

>       sizeof(struct uio_info) would return wrong values,
For which definition of wrong?  sizeof(struct uio_info) don't include
space for mem then, but in my eyes that's correct.  Having to care about
the size of mem is the burden when it's not constant.

>                                                          you need to
> free the extra memory,
There is no extra memory because uioinfo and it's mem member are
allocated together with a single kzalloc (and must be).  (Thats the
difference to

	struct uio_info {
		...
		size_t			num_memmaps;
		struct uio_mem		*mem;
	};

.)
>                        userspace applications need to be able to deal
> with 10000 mappings...
For the userspace it's exactly the same, isn't it?

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH 4/4 v2] [RFC] UIO: generic platform driver

[Uwe Kleine-König]

Hello,

Uwe Kleine-König wrote:
> +			dev_warn(&pdev->dev, "device has more than "
> +					stringify(MAX_UIO_MAPS)
This must read __stringify(MAX_UIO_MAPS).  Sorry, I didn't compile test
that.

Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH 4/4 v2] [RFC] UIO: generic platform driver

[Uwe Kleine-König]

Hello,

Hans J. Koch wrote:
> On Fri, Apr 11, 2008 at 11:21:58AM +0200, Uwe Kleine-König wrote:
> > 
> > Below is a new version that uses linux/stringify and zeros size for
> > unused mappings (line 102ff).
> 
> Thanks, but it doesn't compile, neither with -rc8 nor with Linus' git.
> 
> One problem can easily be fixed, the macro is called __stringify, not
> stringify.
I just notice that, too.  My mail address that and your's just crossed
each other.
 
> But what about this:
> 
> ERROR: "clk_get" [drivers/uio/uio_pdrv.ko] undefined!
> ERROR: "clk_enable" [drivers/uio/uio_pdrv.ko] undefined!
> ERROR: "clk_disable" [drivers/uio/uio_pdrv.ko] undefined!
> ERROR: "clk_put" [drivers/uio/uio_pdrv.ko] undefined!
> 
> Do you have any extra patches applied?
Yes I have, but nothing special.  This is part of a generic API defined
in include/linux/clk.h.  One of it's use it to abstract away some
platform dependencies.  There are several architectures that define
it[1].  I used it to allow enabling the device only when the device is
opened.  Typical things in the enable routine are enabling a clock or
reserve and configure gpios etc.

A minimal dummy implementation that should work here is:

	#define clk_get(dev, id)	NULL
	#define clk_put(clk)		((void)0)
	#define clk_enable(clk)		(1)
	#define clk_disable(clk)	((void)0)

Best regards
Uwe

[1] Try:

	git ls-files -z | xargs -0 perl -n -e 'print "$ARGV\n" if /EXPORT_SYMBOL(?:_GPL)?\s*\(\s*clk_get\s*\)/;'

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH 4/4 v2] [RFC] UIO: generic platform driver

[Hans J. Koch]

On Fri, Apr 11, 2008 at 01:03:58PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> Hans J. Koch wrote:
> > On Fri, Apr 11, 2008 at 11:21:58AM +0200, Uwe Kleine-König wrote:
> > > 
> > > Below is a new version that uses linux/stringify and zeros size for
> > > unused mappings (line 102ff).
> > 
> > Thanks, but it doesn't compile, neither with -rc8 nor with Linus' git.
> > 
> > One problem can easily be fixed, the macro is called __stringify, not
> > stringify.
> I just notice that, too.  My mail address that and your's just crossed
> each other.
>  
> > But what about this:
> > 
> > ERROR: "clk_get" [drivers/uio/uio_pdrv.ko] undefined!
> > ERROR: "clk_enable" [drivers/uio/uio_pdrv.ko] undefined!
> > ERROR: "clk_disable" [drivers/uio/uio_pdrv.ko] undefined!
> > ERROR: "clk_put" [drivers/uio/uio_pdrv.ko] undefined!
> > 
> > Do you have any extra patches applied?
> Yes I have, but nothing special.  This is part of a generic API defined
> in include/linux/clk.h.  One of it's use it to abstract away some
> platform dependencies.  There are several architectures that define
> it[1].

I know. Unfortunately, I tested on x86_64, and it doesn't compile.
If it's depending on something, then this dependency should be added in
Kconfig. If it can be selected in the configuration, I expect it to
compile (and work).

Thanks,
Hans


> I used it to allow enabling the device only when the device is
> opened.  Typical things in the enable routine are enabling a clock or
> reserve and configure gpios etc.
> 
> A minimal dummy implementation that should work here is:
> 
> 	#define clk_get(dev, id)	NULL
> 	#define clk_put(clk)		((void)0)
> 	#define clk_enable(clk)		(1)
> 	#define clk_disable(clk)	((void)0)
> 
> Best regards
> Uwe
> 
> [1] Try:
> 
> 	git ls-files -z | xargs -0 perl -n -e 'print "$ARGV\n" if /EXPORT_SYMBOL(?:_GPL)?\s*\(\s*clk_get\s*\)/;'
> 
> -- 
> Uwe Kleine-König, Software Engineer
> Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
> Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH 4/4 v2] [RFC] UIO: generic platform driver

[Uwe Kleine-König]

Hello,

[Added Russell King to Cc:]

Hans J. Koch wrote:
> On Fri, Apr 11, 2008 at 01:03:58PM +0200, Uwe Kleine-König wrote:
> > Hello,
> > 
> > Hans J. Koch wrote:
> > > On Fri, Apr 11, 2008 at 11:21:58AM +0200, Uwe Kleine-König wrote:
> > > > 
> > > > Below is a new version that uses linux/stringify and zeros size for
> > > > unused mappings (line 102ff).
> > > 
> > > Thanks, but it doesn't compile, neither with -rc8 nor with Linus' git.
> > > 
> > > One problem can easily be fixed, the macro is called __stringify, not
> > > stringify.
> > I just notice that, too.  My mail address that and your's just crossed
> > each other.
> >  
> > > But what about this:
> > > 
> > > ERROR: "clk_get" [drivers/uio/uio_pdrv.ko] undefined!
> > > ERROR: "clk_enable" [drivers/uio/uio_pdrv.ko] undefined!
> > > ERROR: "clk_disable" [drivers/uio/uio_pdrv.ko] undefined!
> > > ERROR: "clk_put" [drivers/uio/uio_pdrv.ko] undefined!
> > > 
> > > Do you have any extra patches applied?
> > Yes I have, but nothing special.  This is part of a generic API defined
> > in include/linux/clk.h.  One of it's use it to abstract away some
> > platform dependencies.  There are several architectures that define
> > it[1].
> 
> I know. Unfortunately, I tested on x86_64, and it doesn't compile.
> If it's depending on something, then this dependency should be added in
> Kconfig. If it can be selected in the configuration, I expect it to
> compile (and work).
Maybe adding a dummy implementation that is compiled for machines that
don't provide a native one.  Currently there is no cpp symbol that tells
if an machine supports the API.

@Russell: Do you have an opinion regarding this!?

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH 1/4 v2] UIO: hold a reference to the device's owner while the device is open

[Hans J. Koch]

On Fri, Apr 11, 2008 at 11:07:39AM +0200, Uwe Kleine-König wrote:
> Otherwise the device might just disappear while /dev/uioX is being used
> which results in an Oops.
> 
> Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>

Looks alright, thanks!

Signed-off-by: Hans J Koch <hjk@linutronix.de>

> ---
> 
> Hans J. Koch wrote:
> > > > The label err_module_get should probably be omitted because it's used only
> > > > once and has just one line of code. You could simply write "return ret"
> > > > instead of "goto err_module_get".
> > > This makes code shuffling easier.  For example if someone decides that
> > > try_module_get should be done after allocating listener then you only
> > > have to exchange the two corresponding code blocks and the two groups
> > > (label + cleanup) in the error handling block.
> > > If the error handling is spread over the whole functions you can easily
> > > miss something---as happend above. :-)
> > 
> > Well, it depends. It's all about readability. Any function should be
> > written in a way that makes it as clear as possible what it does. Your
> > code is certainly not critical regarding that aspect, but I think it can
> > still be improved. And a label that is used only once and contains only
> > one line of code is definetly unnecessary. I don't follow the
> > maybe-one-day-in-the-future-it-might-be-useful philosophy. I like code
> > that is as clean and readable as possible _now_.
> That thing about code reordering is minor compared to having all error
> handling in one place, but ...
> 
> >                                                  And as this patch is
> > not just a driver but affects the UIO core, this is even more important.
> > 
> > Could you please send an updated patch?
> ... , it's your code, so you can find a new version below.

It's not _my_ code, it's _our_ code, partly written by me. At home, I use any
coding style I like. But this is in mainline, so we use the coding style the
kernel community has agreed upon.

> 
> Best regards
> Uwe
> 
>  drivers/uio/uio.c |   36 +++++++++++++++++++++---------------
>  1 files changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index 1175908..55cc7b8 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -301,23 +301,33 @@ static int uio_open(struct inode *inode, struct file *filep)
>  	if (!idev)
>  		return -ENODEV;
>  
> +	if (!try_module_get(idev->owner))
> +		return -ENODEV;
> +
>  	listener = kmalloc(sizeof(*listener), GFP_KERNEL);
> -	if (!listener)
> -		return -ENOMEM;
> +	if (!listener) {
> +		ret = -ENOMEM;
> +		goto err_alloc_listener;
> +	}
>  
>  	listener->dev = idev;
>  	listener->event_count = atomic_read(&idev->event);
>  	filep->private_data = listener;
>  
>  	if (idev->info->open) {
> -		if (!try_module_get(idev->owner))
> -			return -ENODEV;
>  		ret = idev->info->open(idev->info, inode);
> -		module_put(idev->owner);
> +		if (ret)
> +			goto err_infoopen;
>  	}
>  
> -	if (ret)
> -		kfree(listener);
> +	return 0;
> +
> +err_infoopen:
> +
> +	kfree(listener);
> +err_alloc_listener:
> +
> +	module_put(idev->owner);
>  
>  	return ret;
>  }
> @@ -336,12 +346,11 @@ static int uio_release(struct inode *inode, struct file *filep)
>  	struct uio_listener *listener = filep->private_data;
>  	struct uio_device *idev = listener->dev;
>  
> -	if (idev->info->release) {
> -		if (!try_module_get(idev->owner))
> -			return -ENODEV;
> +	if (idev->info->release)
>  		ret = idev->info->release(idev->info, inode);
> -		module_put(idev->owner);
> -	}
> +
> +	module_put(idev->owner);
> +
>  	if (filep->f_flags & FASYNC)
>  		ret = uio_fasync(-1, filep, 0);
>  	kfree(listener);
> @@ -510,10 +519,7 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
>  		return -EINVAL;
>  
>  	if (idev->info->mmap) {
> -		if (!try_module_get(idev->owner))
> -			return -ENODEV;
>  		ret = idev->info->mmap(idev->info, vma);
> -		module_put(idev->owner);
>  		return ret;
>  	}
>  
> -- 
> 1.5.4.5
> 
> 
> -- 
> Uwe Kleine-König, Software Engineer
> Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
> Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH 4/4] [RFC] UIO: generic platform driver

[Hans J. Koch]

On Fri, Apr 11, 2008 at 12:41:54PM +0200, Uwe Kleine-König wrote:
> > problem? Please move the declaration to the top of the function.

> I'm not conviced and still prefer it that way.  I gave way for your
> requests in uio.c because it's your code.  I want to leave it as it is
> and hope you will accept that (as this is my code).

I looked around a bit and talked to some people. It seems that a local
variable declaration within a for{} block is OK. I still don't like it,
but I won't object any longer.

So, AFAICT you've only got that arch dependency problem left to solve.

Thanks,
Hans

Re: [PATCH 2/4] UIO: use menuconfig

[Greg KH]

On Thu, Apr 10, 2008 at 09:39:51PM +0200, Hans J. Koch wrote:
> On Thu, Apr 10, 2008 at 02:37:01PM +0200, Uwe Kleine-K??nig wrote:
> > This allows configuring CONFIG_UIO without changing into the UIO submenu
> 
> I like it.
> 
> Signed-off-by: Hans J Koch <hjk@linutronix.de>

You like it so much you already acked this same change from someone
else, which is in my tree at:
	http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-01-driver-core/uio-kconfig-improvements.patch

:)

thanks,

greg k-h

Re: [PATCH 3/4] UIO: wrap all uio drivers in "if UIO" and "endif"

[Greg KH]

On Thu, Apr 10, 2008 at 09:45:14PM +0200, Hans J. Koch wrote:
> On Thu, Apr 10, 2008 at 02:37:02PM +0200, Uwe Kleine-K??nig wrote:
> > currently there is only one driver, but new entries don't need to depend
> > explicitly on UIO.
> 
> Agreed.
> 
> Signed-off-by: Hans J Koch <hjk@linutronix.de>

Already in the -mm tree...

thanks,

greg k-h

Re: [PATCH 4/4 v2] [RFC] UIO: generic platform driver

[Greg KH]

On Fri, Apr 11, 2008 at 12:48:35PM +0200, Uwe Kleine-K?nig wrote:
> Hello,
> 
> Uwe Kleine-K?nig wrote:
> > +			dev_warn(&pdev->dev, "device has more than "
> > +					stringify(MAX_UIO_MAPS)
> This must read __stringify(MAX_UIO_MAPS).  Sorry, I didn't compile test
> that.

Care to send the latest version of this, I'm a bit lost as to what
people want me to apply...

thanks,

greg k-h

Re: [PATCH 4/4 v2] [RFC] UIO: generic platform driver

[Hans J. Koch]

On Fri, Apr 11, 2008 at 02:41:34PM -0700, Greg KH wrote:
> On Fri, Apr 11, 2008 at 12:48:35PM +0200, Uwe Kleine-K?nig wrote:
> > Hello,
> > 
> > Uwe Kleine-K?nig wrote:
> > > +			dev_warn(&pdev->dev, "device has more than "
> > > +					stringify(MAX_UIO_MAPS)
> > This must read __stringify(MAX_UIO_MAPS).  Sorry, I didn't compile test
> > that.
> 
> Care to send the latest version of this, I'm a bit lost as to what
> people want me to apply...

Hi Greg,
PATCH 4/4 still has a problem. It uses some clock framework functions
not available on every architecture. E.g. on x86_64 you can select this
driver in menuconfig, but it won't compile.

The first three patches are OK in my opinion. Uwe provided a second
version of PATCH 1/4, PATCH 2/4 and PATCH 3/4 were alright in their
original version. I added my Signed-off-by to 1-3, but not to 4.

Thanks,
Hans

Re: [PATCH 2/4] UIO: use menuconfig

[Hans J. Koch]

On Fri, Apr 11, 2008 at 02:36:08PM -0700, Greg KH wrote:
> On Thu, Apr 10, 2008 at 09:39:51PM +0200, Hans J. Koch wrote:
> > On Thu, Apr 10, 2008 at 02:37:01PM +0200, Uwe Kleine-K??nig wrote:
> > > This allows configuring CONFIG_UIO without changing into the UIO submenu
> > 
> > I like it.
> > 
> > Signed-off-by: Hans J Koch <hjk@linutronix.de>
> 
> You like it so much you already acked this same change from someone
> else, which is in my tree at:
> 	http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-01-driver-core/uio-kconfig-improvements.patch
> 
> :)

Hey, I just wanted to improve my Signed-off-by statistics :-) Damn it,
one less...
No, seriously, it looked somehow familiar but I didn't remember. Sorry.

Thanks,
Hans

Re: [PATCH 4/4 v2] [RFC] UIO: generic platform driver

[Greg KH]

On Sat, Apr 12, 2008 at 12:54:02AM +0200, Hans J. Koch wrote:
> On Fri, Apr 11, 2008 at 02:41:34PM -0700, Greg KH wrote:
> > On Fri, Apr 11, 2008 at 12:48:35PM +0200, Uwe Kleine-K?nig wrote:
> > > Hello,
> > > 
> > > Uwe Kleine-K?nig wrote:
> > > > +			dev_warn(&pdev->dev, "device has more than "
> > > > +					stringify(MAX_UIO_MAPS)
> > > This must read __stringify(MAX_UIO_MAPS).  Sorry, I didn't compile test
> > > that.
> > 
> > Care to send the latest version of this, I'm a bit lost as to what
> > people want me to apply...
> 
> Hi Greg,
> PATCH 4/4 still has a problem. It uses some clock framework functions
> not available on every architecture. E.g. on x86_64 you can select this
> driver in menuconfig, but it won't compile.
> 
> The first three patches are OK in my opinion. Uwe provided a second
> version of PATCH 1/4, PATCH 2/4 and PATCH 3/4 were alright in their
> original version. I added my Signed-off-by to 1-3, but not to 4.

Ok, I grabbed patch 1, 2 and 3 are already in my tree and -mm :)

Let me know if you all come to an agreement on patch 4.

thanks,

greg k-h

Re: [PATCH 4/4 v2] [RFC] UIO: generic platform driver

[Russell King - ARM Linux]

On Fri, Apr 11, 2008 at 01:25:43PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> [Added Russell King to Cc:]
> 
> Hans J. Koch wrote:
> > On Fri, Apr 11, 2008 at 01:03:58PM +0200, Uwe Kleine-König wrote:
> > > Hello,
> > > 
> > > Hans J. Koch wrote:
> > > > On Fri, Apr 11, 2008 at 11:21:58AM +0200, Uwe Kleine-König wrote:
> > > > > 
> > > > > Below is a new version that uses linux/stringify and zeros size for
> > > > > unused mappings (line 102ff).
> > > > 
> > > > Thanks, but it doesn't compile, neither with -rc8 nor with Linus' git.
> > > > 
> > > > One problem can easily be fixed, the macro is called __stringify, not
> > > > stringify.
> > > I just notice that, too.  My mail address that and your's just crossed
> > > each other.
> > >  
> > > > But what about this:
> > > > 
> > > > ERROR: "clk_get" [drivers/uio/uio_pdrv.ko] undefined!
> > > > ERROR: "clk_enable" [drivers/uio/uio_pdrv.ko] undefined!
> > > > ERROR: "clk_disable" [drivers/uio/uio_pdrv.ko] undefined!
> > > > ERROR: "clk_put" [drivers/uio/uio_pdrv.ko] undefined!
> > > > 
> > > > Do you have any extra patches applied?
> > > Yes I have, but nothing special.  This is part of a generic API defined
> > > in include/linux/clk.h.  One of it's use it to abstract away some
> > > platform dependencies.  There are several architectures that define
> > > it[1].
> > 
> > I know. Unfortunately, I tested on x86_64, and it doesn't compile.
> > If it's depending on something, then this dependency should be added in
> > Kconfig. If it can be selected in the configuration, I expect it to
> > compile (and work).
> Maybe adding a dummy implementation that is compiled for machines that
> don't provide a native one.  Currently there is no cpp symbol that tells
> if an machine supports the API.
> 
> @Russell: Do you have an opinion regarding this!?

Only that the kernels Kconfig is turning into a real complicated mess
of dependencies IMHO.

We could add a HAVE_CLK and add that to the dependency of all the drivers
which use linux/clk.h.  The problem will be finding all those drivers and
their corresponding Kconfig entries.

My feeling is that we're just going to end up creating another Kconfig
symbol which folk half-heartedly use.

[PATCH] Re: [PATCH 4/4 v2] [RFC] UIO: generic platform driver

[Uwe Kleine-König]

Hello,

> > > > > But what about this:
> > > > > 
> > > > > ERROR: "clk_get" [drivers/uio/uio_pdrv.ko] undefined!
> > > > > ERROR: "clk_enable" [drivers/uio/uio_pdrv.ko] undefined!
> > > > > ERROR: "clk_disable" [drivers/uio/uio_pdrv.ko] undefined!
> > > > > ERROR: "clk_put" [drivers/uio/uio_pdrv.ko] undefined!
> > > > > 
> > > > > Do you have any extra patches applied?
> > > > Yes I have, but nothing special.  This is part of a generic API defined
> > > > in include/linux/clk.h.  One of it's use it to abstract away some
> > > > platform dependencies.  There are several architectures that define
> > > > it[1].
> > > 
> > > I know. Unfortunately, I tested on x86_64, and it doesn't compile.
> > > If it's depending on something, then this dependency should be added in
> > > Kconfig. If it can be selected in the configuration, I expect it to
> > > compile (and work).
> > Maybe adding a dummy implementation that is compiled for machines that
> > don't provide a native one.  Currently there is no cpp symbol that tells
> > if an machine supports the API.
> > 
> > @Russell: Do you have an opinion regarding this!?
> 
> Only that the kernels Kconfig is turning into a real complicated mess
> of dependencies IMHO.
> 
> We could add a HAVE_CLK and add that to the dependency of all the drivers
> which use linux/clk.h.  The problem will be finding all those drivers and
> their corresponding Kconfig entries.
> 
> My feeling is that we're just going to end up creating another Kconfig
> symbol which folk half-heartedly use.
I don't like that either.  What do you think about the patch below?
It doesn't introduce a new symbol that needs much care and attention.
This way the clk API is available on all configurations provided that 
CONFIG_DUMMY_CLK is set correctly.  If CONFIG_DUMMY_CLK is set wrong it
should result in a compile error.  Either because there are two
implementations of clk_get or none.

The condition on when to define DUMMY_CLK isn't yet perfect, but not
defining it for a platform isn't a regression as there was no
implementation before this patch either.

This could supersede the implementation in
drivers/usb/gadget/pxa2xx_udc.c for IXP.  (That driver obviously doesn't
check if clk_enable() succeeded, because it's defined as:

	#define clk_enable(clk)		do { } while (0)

.)

Maybe it would be fine to make these functions inline and define them
directly in linux/clk.h?

Best regards
Uwe

---->8----
From: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
Date: Mon, 14 Apr 2008 09:02:30 +0200
Subject: [PATCH] provide a dummy implementation of the clk API

With this implementation clk_get and clk_enable always succeed.  The
counterparts clk_put and clk_disable only do some minor error checking.

Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
 lib/Kconfig    |    6 ++++++
 lib/Makefile   |    2 ++
 lib/dummyclk.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 0 deletions(-)
 create mode 100644 lib/dummyclk.c

diff --git a/lib/Kconfig b/lib/Kconfig
index ba3d104..53fee1c 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -141,4 +141,10 @@ config HAS_DMA
 config CHECK_SIGNATURE
 	bool
 
+config DUMMY_CLK
+	def_bool y if X86
+	help
+	  This provides a dummy implementation for the API defined in
+	  linux/clk.h for platforms that don't implement it theirselves.
+
 endmenu
diff --git a/lib/Makefile b/lib/Makefile
index 23de261..2ca3e82 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -70,6 +70,8 @@ obj-$(CONFIG_FAULT_INJECTION) += fault-inject.o
 
 lib-$(CONFIG_GENERIC_BUG) += bug.o
 
+obj-$(CONFIG_DUMMY_CLK) += dummyclk.o
+
 hostprogs-y	:= gen_crc32table
 clean-files	:= crc32table.h
 
diff --git a/lib/dummyclk.c b/lib/dummyclk.c
new file mode 100644
index 0000000..bf364df
--- /dev/null
+++ b/lib/dummyclk.c
@@ -0,0 +1,54 @@
+/*
+ * lib/dummyclk.c
+ *
+ * Copyright (C) 2008 by Digi International Inc.
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+#include <linux/device.h>
+#include <linux/err.h>
+
+struct clk {
+	unsigned int enablecnt;
+};
+
+struct clk *clk_get(struct device *dev, const char *id)
+{
+	struct clk *ret = kzalloc(sizeof(*ret), GFP_KERNEL);
+
+	if (ret)
+		return ret;
+	else
+		return ERR_PTR(-ENOMEM);
+}
+EXPORT_SYMBOL(clk_get);
+
+void clk_put(struct clk *clk)
+{
+	WARN_ON(clk->enablecnt);
+}
+EXPORT_SYMBOL(clk_put);
+
+int clk_enable(struct clk *clk)
+{
+	++clk->enablecnt;
+	return 0;
+}
+EXPORT_SYMBOL(clk_enable);
+
+void clk_disable(struct clk *clk)
+{
+	BUG_ON(!clk->enablecnt);
+	--clk->enablecnt;
+}
+EXPORT_SYMBOL(clk_disable);
+
+unsigned long clk_get_rate(struct clk *clk)
+{
+	BUG_ON(!clk->enablecnt);
+	return 0;
+}
+EXPORT_SYMBOL(clk_get_rate);
-- 
1.5.4.5


-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH] Re: [PATCH 4/4 v2] [RFC] UIO: generic platform driver

[Russell King - ARM Linux]

On Mon, Apr 14, 2008 at 09:48:58AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> > > > > > But what about this:
> > > > > > 
> > > > > > ERROR: "clk_get" [drivers/uio/uio_pdrv.ko] undefined!
> > > > > > ERROR: "clk_enable" [drivers/uio/uio_pdrv.ko] undefined!
> > > > > > ERROR: "clk_disable" [drivers/uio/uio_pdrv.ko] undefined!
> > > > > > ERROR: "clk_put" [drivers/uio/uio_pdrv.ko] undefined!
> > > > > > 
> > > > > > Do you have any extra patches applied?
> > > > > Yes I have, but nothing special.  This is part of a generic API defined
> > > > > in include/linux/clk.h.  One of it's use it to abstract away some
> > > > > platform dependencies.  There are several architectures that define
> > > > > it[1].
> > > > 
> > > > I know. Unfortunately, I tested on x86_64, and it doesn't compile.
> > > > If it's depending on something, then this dependency should be added in
> > > > Kconfig. If it can be selected in the configuration, I expect it to
> > > > compile (and work).
> > > Maybe adding a dummy implementation that is compiled for machines that
> > > don't provide a native one.  Currently there is no cpp symbol that tells
> > > if an machine supports the API.
> > > 
> > > @Russell: Do you have an opinion regarding this!?
> > 
> > Only that the kernels Kconfig is turning into a real complicated mess
> > of dependencies IMHO.
> > 
> > We could add a HAVE_CLK and add that to the dependency of all the drivers
> > which use linux/clk.h.  The problem will be finding all those drivers and
> > their corresponding Kconfig entries.
> > 
> > My feeling is that we're just going to end up creating another Kconfig
> > symbol which folk half-heartedly use.
>
> I don't like that either.  What do you think about the patch below?
> It doesn't introduce a new symbol that needs much care and attention.
> This way the clk API is available on all configurations provided that 
> CONFIG_DUMMY_CLK is set correctly.  If CONFIG_DUMMY_CLK is set wrong it
> should result in a compile error.  Either because there are two
> implementations of clk_get or none.

Hang on.  I'm lost.  What are we talking about here?  I thought the
thread was about the one liner patch for UIO to arch/arm/Kconfi
(which still hasn't hit the patch system so is still on target for
being missed...)

What's this drivers/uio/uio_pdrv.ko module, and why doesn't it appear
in the LKML archive of this thread?

Re: [PATCH] Re: [PATCH 4/4 v2] [RFC] UIO: generic platform driver

[Uwe Kleine-König]

Hi Russell,

Russell King - ARM Linux wrote:
> On Mon, Apr 14, 2008 at 09:48:58AM +0200, Uwe Kleine-König wrote:
> > > > > > > But what about this:
> > > > > > > 
> > > > > > > ERROR: "clk_get" [drivers/uio/uio_pdrv.ko] undefined!
> > > > > > > ERROR: "clk_enable" [drivers/uio/uio_pdrv.ko] undefined!
> > > > > > > ERROR: "clk_disable" [drivers/uio/uio_pdrv.ko] undefined!
> > > > > > > ERROR: "clk_put" [drivers/uio/uio_pdrv.ko] undefined!
> > > > > > > 
> > > > > > > Do you have any extra patches applied?
> > > > > > Yes I have, but nothing special.  This is part of a generic API defined
> > > > > > in include/linux/clk.h.  One of it's use it to abstract away some
> > > > > > platform dependencies.  There are several architectures that define
> > > > > > it[1].
> > > > > 
> > > > > I know. Unfortunately, I tested on x86_64, and it doesn't compile.
> > > > > If it's depending on something, then this dependency should be added in
> > > > > Kconfig. If it can be selected in the configuration, I expect it to
> > > > > compile (and work).
> > > > Maybe adding a dummy implementation that is compiled for machines that
> > > > don't provide a native one.  Currently there is no cpp symbol that tells
> > > > if an machine supports the API.
> > > > 
> > > > @Russell: Do you have an opinion regarding this!?
> > > 
> > > Only that the kernels Kconfig is turning into a real complicated mess
> > > of dependencies IMHO.
> > > 
> > > We could add a HAVE_CLK and add that to the dependency of all the drivers
> > > which use linux/clk.h.  The problem will be finding all those drivers and
> > > their corresponding Kconfig entries.
> > > 
> > > My feeling is that we're just going to end up creating another Kconfig
> > > symbol which folk half-heartedly use.
> >
> > I don't like that either.  What do you think about the patch below?
> > It doesn't introduce a new symbol that needs much care and attention.
> > This way the clk API is available on all configurations provided that 
> > CONFIG_DUMMY_CLK is set correctly.  If CONFIG_DUMMY_CLK is set wrong it
> > should result in a compile error.  Either because there are two
> > implementations of clk_get or none.
> 
> Hang on.  I'm lost.  What are we talking about here?  I thought the
> thread was about the one liner patch for UIO to arch/arm/Kconfi
> (which still hasn't hit the patch system so is still on target for
> being missed...)
No, the topic here is a generic uio platform driver.  It uses the clk
API and Hans criticised that is doesn't compile on x86 (because there is
no implementation of the clk API).  So I suggested to implement a dummy
for that.

This is completly independant of the inclusion of drivers/uio/Kconfig in
arch/arm/Kconfig.  I will send a patch for that.

> What's this drivers/uio/uio_pdrv.ko module, and why doesn't it appear
> in the LKML archive of this thread?
Don't know why lkml.org didn't link these.  The start of the thread can
be found at

	http://lkml.org/lkml/2008/4/10/110

gmane managed to link these mails:

	http://thread.gmane.org/gmane.linux.kernel/663884

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH] Re: [PATCH 4/4 v2] [RFC] UIO: generic platform driver

[Uwe Kleine-König]

Hello,

> This is completly independant of the inclusion of drivers/uio/Kconfig in
> arch/arm/Kconfig.  I will send a patch for that.
BTW: I just found

	http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/driver-core/uio-arch-arm-kconfig-make-uio-available-on-arm-architecture.patch

So obviously gkh looks for that issue.  In my eyes it should better go
via rmk, but it doesn't really matter for me.  Should I still send a
patch to the patch system?

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH] Re: [PATCH 4/4 v2] [RFC] UIO: generic platform driver

[Russell King - ARM Linux]

On Mon, Apr 14, 2008 at 11:54:45AM +0200, Uwe Kleine-König wrote:
> Hi Russell,
> 
> Russell King - ARM Linux wrote:
> > On Mon, Apr 14, 2008 at 09:48:58AM +0200, Uwe Kleine-König wrote:
> > > > > > > > But what about this:
> > > > > > > > 
> > > > > > > > ERROR: "clk_get" [drivers/uio/uio_pdrv.ko] undefined!
> > > > > > > > ERROR: "clk_enable" [drivers/uio/uio_pdrv.ko] undefined!
> > > > > > > > ERROR: "clk_disable" [drivers/uio/uio_pdrv.ko] undefined!
> > > > > > > > ERROR: "clk_put" [drivers/uio/uio_pdrv.ko] undefined!
> > > > > > > > 
> > > > > > > > Do you have any extra patches applied?
> > > > > > > Yes I have, but nothing special.  This is part of a generic API defined
> > > > > > > in include/linux/clk.h.  One of it's use it to abstract away some
> > > > > > > platform dependencies.  There are several architectures that define
> > > > > > > it[1].
> > > > > > 
> > > > > > I know. Unfortunately, I tested on x86_64, and it doesn't compile.
> > > > > > If it's depending on something, then this dependency should be added in
> > > > > > Kconfig. If it can be selected in the configuration, I expect it to
> > > > > > compile (and work).
> > > > > Maybe adding a dummy implementation that is compiled for machines that
> > > > > don't provide a native one.  Currently there is no cpp symbol that tells
> > > > > if an machine supports the API.
> > > > > 
> > > > > @Russell: Do you have an opinion regarding this!?
> > > > 
> > > > Only that the kernels Kconfig is turning into a real complicated mess
> > > > of dependencies IMHO.
> > > > 
> > > > We could add a HAVE_CLK and add that to the dependency of all the drivers
> > > > which use linux/clk.h.  The problem will be finding all those drivers and
> > > > their corresponding Kconfig entries.
> > > > 
> > > > My feeling is that we're just going to end up creating another Kconfig
> > > > symbol which folk half-heartedly use.
> > >
> > > I don't like that either.  What do you think about the patch below?
> > > It doesn't introduce a new symbol that needs much care and attention.
> > > This way the clk API is available on all configurations provided that 
> > > CONFIG_DUMMY_CLK is set correctly.  If CONFIG_DUMMY_CLK is set wrong it
> > > should result in a compile error.  Either because there are two
> > > implementations of clk_get or none.
> > 
> > Hang on.  I'm lost.  What are we talking about here?  I thought the
> > thread was about the one liner patch for UIO to arch/arm/Kconfi
> > (which still hasn't hit the patch system so is still on target for
> > being missed...)
> No, the topic here is a generic uio platform driver.  It uses the clk
> API and Hans criticised that is doesn't compile on x86 (because there is
> no implementation of the clk API).  So I suggested to implement a dummy
> for that.
> 
> This is completly independant of the inclusion of drivers/uio/Kconfig in
> arch/arm/Kconfig.  I will send a patch for that.
> 
> > What's this drivers/uio/uio_pdrv.ko module, and why doesn't it appear
> > in the LKML archive of this thread?
> Don't know why lkml.org didn't link these.  The start of the thread can
> be found at
> 
> 	http://lkml.org/lkml/2008/4/10/110

Thanks.  Well, tbh, I don't know which way to go on this.  Each of the
suggested ways have their downsides.

However:

+       pdata->clk = clk_get(&pdev->dev, DRIVER_NAME);

seems wrong - "uio" as a clock name?

+               /* XXX: better use dev_dbg, but which device should I use?
+                * info->uio_dev->dev isn't accessible here as struct uio_device+                * is opaque.
+                */

why not store a copy of 'dev' in struct uio_platdata ?

+       uiomem = &uioinfo->mem[0];
+       for (i = 0; i < pdev->num_resources; ++i) {
...
+               ++uiomem;
+       }

Who's to say there's pdev->num_resources entries in the 'mem' array?
Shouldn't this loop also be limited to MAX_UIO_MAPS iterations (or
maybe complain if there's more than MAX_UIO_MAPS)?

+/* XXX: I thought there already exists something like STRINGIFY, but I could not
+ * find it ...
+ */
+#ifndef STRINGIFY
+#define STRINGIFY(x) __STRINGIFY_HELPER(x)
+#define __STRINGIFY_HELPER(x) #x
+#endif

#include <linux/stringify.h> ?

Re: [PATCH] Re: [PATCH 4/4 v2] [RFC] UIO: generic platform driver

[Uwe Kleine-König]

Hello Russell,

> > > What's this drivers/uio/uio_pdrv.ko module, and why doesn't it appear
> > > in the LKML archive of this thread?
> > Don't know why lkml.org didn't link these.  The start of the thread can
> > be found at
> > 
> > 	http://lkml.org/lkml/2008/4/10/110
> 
> Thanks.  Well, tbh, I don't know which way to go on this.  Each of the
> suggested ways have their downsides.
> 
> However:
> 
> +       pdata->clk = clk_get(&pdev->dev, DRIVER_NAME);
> 
> seems wrong - "uio" as a clock name?
What do you suggest?  "uioclk"?
 
> +               /* XXX: better use dev_dbg, but which device should I use?
> +                * info->uio_dev->dev isn't accessible here as struct uio_device
> +                * is opaque.
> +                */
> 
> why not store a copy of 'dev' in struct uio_platdata ?
I don't like increasing the size of struct uio_platdata only for a debug
helper.  In most cases pr_debug (or dev_dbg) compiles to nothing.
Maybe the easiest way is to remove that debug statement.

> +       uiomem = &uioinfo->mem[0];
> +       for (i = 0; i < pdev->num_resources; ++i) {
> ...
> +               ++uiomem;
> +       }
> 
> Who's to say there's pdev->num_resources entries in the 'mem' array?
The code you skipped with ... from that loop includes

	if (uiomem >= &uioinfo->mem[MAX_UIO_MAPS]) {
		dev_warn(&pdev->dev, "...");
		break;
	}

that should take care of that.  In v2 of this patch the remaining
entries are disabled by .size = 0.

> Shouldn't this loop also be limited to MAX_UIO_MAPS iterations (or
> maybe complain if there's more than MAX_UIO_MAPS)?
> 
> +/* XXX: I thought there already exists something like STRINGIFY, but I could not
> + * find it ...
> + */
> +#ifndef STRINGIFY
> +#define STRINGIFY(x) __STRINGIFY_HELPER(x)
> +#define __STRINGIFY_HELPER(x) #x
> +#endif
> 
> #include <linux/stringify.h> ?
If you look at the v2 patch[1] I found that in the mean time.  Thanks
all the same.

Best regards and thanks for your review,
Uwe

[1] http://lkml.org/lkml/2008/4/11/81 or
http://thread.gmane.org/gmane.linux.kernel/665257

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH] Re: [PATCH 4/4 v2] [RFC] UIO: generic platform driver

[Russell King - ARM Linux]

On Mon, Apr 14, 2008 at 01:20:21PM +0200, Uwe Kleine-König wrote:
> [1] http://lkml.org/lkml/2008/4/11/81 or
> http://thread.gmane.org/gmane.linux.kernel/665257

In one of the mails, it was said:
  No. It's more important to see which variables are declared in the
  function and which are declared elsewhere. If you have to search the
  whole body of a function for possible declarations, this is BAD. And if
  it's not clear where a variable is used, the function is too long or has
  other style problems. Your function is short and clean, so where's the
  problem? Please move the declaration to the top of the function.

I disagree with this statement.  It's far better to limit the scope of
variables so that you know they only have local use, and eg, not used
inside a loop and then outside with possible unintended effects.

If a variable is only used inside a loop, it should be declared _inside_
that loop.

The statement goes on to talk about the function being short and clean -
that's not an argument to apply any particular point of view on this
subject, since you can argue that because it's short and clean you can
see that the variable is only used within the loop.

So, please, keep the variable declaration inside the loop, and don't
pollute the outer levels with unnecessary variable declarations.

Re: [PATCH] Re: [PATCH 4/4 v2] [RFC] UIO: generic platform driver

[Hans J. Koch]

On Mon, Apr 14, 2008 at 12:37:59PM +0100, Russell King - ARM Linux wrote:
> On Mon, Apr 14, 2008 at 01:20:21PM +0200, Uwe Kleine-König wrote:
> > [1] http://lkml.org/lkml/2008/4/11/81 or
> > http://thread.gmane.org/gmane.linux.kernel/665257
> 
> In one of the mails, it was said:
>   No. It's more important to see which variables are declared in the
>   function and which are declared elsewhere. If you have to search the
>   whole body of a function for possible declarations, this is BAD. And if
>   it's not clear where a variable is used, the function is too long or has
>   other style problems. Your function is short and clean, so where's the
>   problem? Please move the declaration to the top of the function.
> 
> I disagree with this statement.  It's far better to limit the scope of
> variables so that you know they only have local use, and eg, not used
> inside a loop and then outside with possible unintended effects.
> 
> If a variable is only used inside a loop, it should be declared _inside_
> that loop.
> 
> The statement goes on to talk about the function being short and clean -
> that's not an argument to apply any particular point of view on this
> subject, since you can argue that because it's short and clean you can
> see that the variable is only used within the loop.
> 
> So, please, keep the variable declaration inside the loop, and don't
> pollute the outer levels with unnecessary variable declarations.

OK, I'm finally convinced :-)
I knew this style from C++ where it makes sense, because a (class-)
variable declaration can implicitly call the constructor of that class
which you normally want to avoid if not needed. As this doesn't happen
in C, I found it unnecessary.
I agree now that there's also a readability argument. The limitation of
the scope is probably not that important as compilers will optimize that
anyway in a lot of cases.

Thanks,
Hans

[PATCH 0/3] UIO: cleanup and platform driver

[Uwe Kleine-König]

Hello,

there are three patches left in my uio queue that didn't made it into
mainline up to now.

You can find the patches in my uio branch at 

	git://www.modarm9.com/gitsrc/pub/people/ukleinek/linux-2.6.git uio

and I will resend the patches as a reply to this mail.

Shortlog and diffstat can be found below.

Best regards
Uwe

Uwe Kleine-König (3):
      UIO: don't let UIO_CIF and UIO_SMX depend twice on UIO
      provide a dummy implementation of the clk API
      UIO: generic platform driver

 drivers/uio/Kconfig    |   10 +++-
 drivers/uio/Makefile   |    1 +
 drivers/uio/uio_pdrv.c |  155 ++++++++++++++++++++++++++++++++++++++++++++++++
 lib/Kconfig            |    6 ++
 lib/Makefile           |    2 +
 lib/dummyclk.c         |   54 +++++++++++++++++
 6 files changed, 226 insertions(+), 2 deletions(-)

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

[PATCH 1/3] UIO: don't let UIO_CIF and UIO_SMX depend twice on UIO

[Uwe Kleine-König]

ae210f188614bb3d1ee3f19c64e28e3cdd44877c introduced a big "if UIO"/"endif"
where all uio drivers are defined.  So know there is no need for them to
depend explicitly on UIO.

Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
Hello,

these hunks were part of a patch in my first series but that patch was dropped
because another patch in Greg's queue did a subset of mine.

Best regards
Uwe

 drivers/uio/Kconfig |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index a4aaab9..78e139c 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -15,7 +15,7 @@ if UIO
 
 config UIO_CIF
 	tristate "generic Hilscher CIF Card driver"
-	depends on UIO && PCI
+	depends on PCI
 	default n
 	help
 	  Driver for Hilscher CIF DeviceNet and Profibus cards.  This
@@ -28,7 +28,6 @@ config UIO_CIF
 
 config UIO_SMX
 	tristate "SMX cryptengine UIO interface"
-	depends on UIO
 	default n
 	help
 	  Userspace IO interface to the Cryptography engine found on the
-- 
1.5.5

[PATCH 2/3] provide a dummy implementation of the clk API

[Uwe Kleine-König]

With this implementation clk_get and clk_enable always succeed.  The
counterparts clk_put and clk_disable only do some minor error checking.

Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
 lib/Kconfig    |    6 ++++++
 lib/Makefile   |    2 ++
 lib/dummyclk.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 0 deletions(-)
 create mode 100644 lib/dummyclk.c

Hello,

this patch isn't really UIO related, but as my platform driver uses the
clk API and that's not implemented on X86 here comes a dummy
implementation.

This patch was already sent but up to now I got no feedback for it.

Best regards
Uwe

diff --git a/lib/Kconfig b/lib/Kconfig
index 2d53dc0..098830d 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -144,4 +144,10 @@ config CHECK_SIGNATURE
 config HAVE_LMB
 	boolean
 
+config DUMMY_CLK
+	def_bool y if X86
+	help
+	  This provides a dummy implementation for the API defined in
+	  linux/clk.h for platforms that don't implement it theirselves.
+
 endmenu
diff --git a/lib/Makefile b/lib/Makefile
index bf8000f..e355c69 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -70,6 +70,8 @@ lib-$(CONFIG_GENERIC_BUG) += bug.o
 
 obj-$(CONFIG_HAVE_LMB) += lmb.o
 
+obj-$(CONFIG_DUMMY_CLK) += dummyclk.o
+
 hostprogs-y	:= gen_crc32table
 clean-files	:= crc32table.h
 
diff --git a/lib/dummyclk.c b/lib/dummyclk.c
new file mode 100644
index 0000000..bf364df
--- /dev/null
+++ b/lib/dummyclk.c
@@ -0,0 +1,54 @@
+/*
+ * lib/dummyclk.c
+ *
+ * Copyright (C) 2008 by Digi International Inc.
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+#include <linux/device.h>
+#include <linux/err.h>
+
+struct clk {
+	unsigned int enablecnt;
+};
+
+struct clk *clk_get(struct device *dev, const char *id)
+{
+	struct clk *ret = kzalloc(sizeof(*ret), GFP_KERNEL);
+
+	if (ret)
+		return ret;
+	else
+		return ERR_PTR(-ENOMEM);
+}
+EXPORT_SYMBOL(clk_get);
+
+void clk_put(struct clk *clk)
+{
+	WARN_ON(clk->enablecnt);
+}
+EXPORT_SYMBOL(clk_put);
+
+int clk_enable(struct clk *clk)
+{
+	++clk->enablecnt;
+	return 0;
+}
+EXPORT_SYMBOL(clk_enable);
+
+void clk_disable(struct clk *clk)
+{
+	BUG_ON(!clk->enablecnt);
+	--clk->enablecnt;
+}
+EXPORT_SYMBOL(clk_disable);
+
+unsigned long clk_get_rate(struct clk *clk)
+{
+	BUG_ON(!clk->enablecnt);
+	return 0;
+}
+EXPORT_SYMBOL(clk_get_rate);
-- 
1.5.5

[PATCH 3/3] UIO: generic platform driver

[Uwe Kleine-König]

Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
Hello,

This is the former patch 4/4 after some discussion.

Open issues:
  - clock name "uio" isn't considered good by Russell King
    I don't have a better suggestion
  - some code could be shared with uio_smx.c
    I would address that in a seperate patch after this one hits mainline.

I appreciate any feedback.

Best regards,
Uwe

 drivers/uio/Kconfig    |    7 ++
 drivers/uio/Makefile   |    1 +
 drivers/uio/uio_pdrv.c |  154 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 162 insertions(+), 0 deletions(-)
 create mode 100644 drivers/uio/uio_pdrv.c

diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index 78e139c..2e9079d 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -26,6 +26,13 @@ config UIO_CIF
 	  To compile this driver as a module, choose M here: the module
 	  will be called uio_cif.
 
+config UIO_PDRV
+	tristate "Userspace I/O platform driver"
+	help
+	  Generic platform driver for Userspace I/O devices.
+
+	  If you don't know what to do here, say N.
+
 config UIO_SMX
 	tristate "SMX cryptengine UIO interface"
 	default n
diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
index 18c4566..e00ce0d 100644
--- a/drivers/uio/Makefile
+++ b/drivers/uio/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_UIO)	+= uio.o
 obj-$(CONFIG_UIO_CIF)	+= uio_cif.o
+obj-$(CONFIG_UIO_PDRV)	+= uio_pdrv.o
 obj-$(CONFIG_UIO_SMX)	+= uio_smx.o
diff --git a/drivers/uio/uio_pdrv.c b/drivers/uio/uio_pdrv.c
new file mode 100644
index 0000000..f421a6e
--- /dev/null
+++ b/drivers/uio/uio_pdrv.c
@@ -0,0 +1,154 @@
+/*
+ * drivers/uio/uio_pdrv.c
+ *
+ * Copyright (C) 2008 by Digi International Inc.
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+#include <linux/clk.h>
+#include <linux/platform_device.h>
+#include <linux/uio_driver.h>
+#include <linux/stringify.h>
+
+#define DRIVER_NAME "uio"
+
+struct uio_platdata {
+	struct uio_info *uioinfo;
+	struct clk *clk;
+};
+
+static int uio_pdrv_open(struct uio_info *info, struct inode *inode)
+{
+	struct uio_platdata *pdata = info->priv;
+
+	BUG_ON(pdata->uioinfo != info);
+
+	return clk_enable(pdata->clk);
+}
+
+static int uio_pdrv_release(struct uio_info *info, struct inode *inode)
+{
+	struct uio_platdata *pdata = info->priv;
+
+	BUG_ON(pdata->uioinfo != info);
+
+	clk_disable(pdata->clk);
+
+	return 0;
+}
+
+static int uio_pdrv_probe(struct platform_device *pdev)
+{
+	struct uio_info *uioinfo = pdev->dev.platform_data;
+	struct uio_platdata *pdata;
+	struct uio_mem *uiomem;
+	int ret = -ENODEV;
+	int i;
+
+	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
+		dev_dbg(&pdev->dev, "%s: err_uioinfo\n", __func__);
+		goto err_uioinfo;
+	}
+
+	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+	if (!pdata) {
+		ret = -ENOMEM;
+		dev_dbg(&pdev->dev, "%s: err_alloc_pdata\n", __func__);
+		goto err_alloc_pdata;
+	}
+
+	pdata->uioinfo = uioinfo;
+
+	pdata->clk = clk_get(&pdev->dev, DRIVER_NAME);
+	if (IS_ERR(pdata->clk)) {
+		ret = PTR_ERR(pdata->clk);
+		dev_dbg(&pdev->dev, "%s: err_clk_get -> %d\n", __func__, ret);
+		goto err_clk_get;
+	}
+
+	uiomem = &uioinfo->mem[0];
+
+	for (i = 0; i < pdev->num_resources; ++i) {
+		struct resource *r = &pdev->resource[i];
+
+		if (r->flags != IORESOURCE_MEM)
+			continue;
+
+		if (uiomem >= &uioinfo->mem[MAX_UIO_MAPS]) {
+			dev_warn(&pdev->dev, "device has more than "
+					__stringify(MAX_UIO_MAPS)
+					" I/O memory resources.\n");
+			break;
+		}
+
+		uiomem->memtype = UIO_MEM_PHYS;
+		uiomem->addr = r->start;
+		uiomem->size = r->end - r->start + 1;
+		++uiomem;
+	}
+
+	while (uiomem < &uioinfo->mem[MAX_UIO_MAPS]) {
+		uiomem->size = 0;
+		++uiomem;
+	}
+
+	pdata->uioinfo->open = uio_pdrv_open;
+	pdata->uioinfo->release = uio_pdrv_release;
+	pdata->uioinfo->priv = pdata;
+
+	ret = uio_register_device(&pdev->dev, pdata->uioinfo);
+
+	if (ret) {
+		clk_put(pdata->clk);
+err_clk_get:
+
+		kfree(pdata);
+err_alloc_pdata:
+err_uioinfo:
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, pdata);
+
+	return 0;
+}
+
+static int uio_pdrv_remove(struct platform_device *pdev)
+{
+	struct uio_platdata *pdata = platform_get_drvdata(pdev);
+
+	uio_unregister_device(pdata->uioinfo);
+
+	clk_put(pdata->clk);
+
+	return 0;
+}
+
+static struct platform_driver uio_pdrv = {
+	.probe = uio_pdrv_probe,
+	.remove = uio_pdrv_remove,
+	.driver = {
+		.name = DRIVER_NAME,
+		.owner = THIS_MODULE,
+	},
+};
+
+static int __init uio_pdrv_init(void)
+{
+	return platform_driver_register(&uio_pdrv);
+}
+
+static void __exit uio_pdrv_exit(void)
+{
+	platform_driver_unregister(&uio_pdrv);
+}
+module_init(uio_pdrv_init);
+module_exit(uio_pdrv_exit);
+
+MODULE_AUTHOR("Uwe Kleine-Koenig");
+MODULE_DESCRIPTION("Userspace I/O platform driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" DRIVER_NAME);
-- 
1.5.5

Re: [PATCH 3/3] UIO: generic platform driver

[Ben Nizette]

On Tue, 2008-04-22 at 11:52 +0200, Uwe Kleine-König wrote:
> Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> ---
> Hello,
> 
> This is the former patch 4/4 after some discussion.
> 
> Open issues:
>   - clock name "uio" isn't considered good by Russell King
>     I don't have a better suggestion

I'd suggest it should be passed through from the platform code: only it
knows what it's going to be (and if indeed there's going to be one at
all).  In fact, the lack-of-clock at the moment is fatal; it shouldn't
be IMO.  For example, I don't need one in SMX.

>   - some code could be shared with uio_smx.c
>     I would address that in a seperate patch after this one hits mainline.

As I said before I'd prefer to keep my driver separate and standalone,
but you and HJK have expressed a wish for unification.  If I get this
formally I'll consider myself outvoted and submit a patch to unify it
all when I submit my platform code (soon).

	--Ben.

Re: [PATCH 3/3] UIO: generic platform driver

[Hans J. Koch]

On Tue, Apr 22, 2008 at 08:26:57PM +1000, Ben Nizette wrote:
> 
> >   - some code could be shared with uio_smx.c
> >     I would address that in a seperate patch after this one hits mainline.
> 
> As I said before I'd prefer to keep my driver separate and standalone,
> but you and HJK have expressed a wish for unification.

This seems to be a misunderstanding. Actually, I don't care too much. If
you like to keep your driver standalone, I don't have objections, even
if Uwe's generic driver is merged. I think there are still valid reasons
not to reduce a driver to a platform data struct.

Greg, what do you say to that?

Thanks,
Hans

Re: [PATCH 0/3] UIO: cleanup and platform driver

[Hans J. Koch]

On Tue, Apr 22, 2008 at 11:47:08AM +0200, Uwe Kleine-K?nig wrote:
> Hello,
> 
> there are three patches left in my uio queue that didn't made it into
> mainline up to now.
> 
> You can find the patches in my uio branch at 
> 
> 	git://www.modarm9.com/gitsrc/pub/people/ukleinek/linux-2.6.git uio
> 
> and I will resend the patches as a reply to this mail.

Hi Uwe,
thanks for your work. Greg and I are at the Hannover trade fair ATM.
I'll be back home on Thursday and will have a look at your patches then.

Thanks,
Hans

Re: [PATCH 3/3] UIO: generic platform driver

[Uwe Kleine-König]

Hello,

Uwe Kleine-König wrote:
> Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> ---
> Hello,
> 
> This is the former patch 4/4 after some discussion.
> 
> Open issues:
>   - clock name "uio" isn't considered good by Russell King
>     I don't have a better suggestion
I added another branch[1] on my repo that doesn't have the dummy clk
patch and variant of this one that doesn't use the clk API.

This way the clk API isn't needed anymore for my patch and the issue
about the clock name disappeard, too.

Best regards
Uwe

[1] git://www.modarm9.com/gitsrc/pub/people/ukleinek/linux-2.6.git uio2
-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH 3/3] UIO: generic platform driver

[Hans J. Koch]

On Wed, Apr 23, 2008 at 10:56:29AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> Uwe Kleine-König wrote:
> > Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> > ---
> > Hello,
> > 
> > This is the former patch 4/4 after some discussion.
> > 
> > Open issues:
> >   - clock name "uio" isn't considered good by Russell King
> >     I don't have a better suggestion
> I added another branch[1] on my repo that doesn't have the dummy clk
> patch and variant of this one that doesn't use the clk API.
> 
> This way the clk API isn't needed anymore for my patch and the issue
> about the clock name disappeard, too.

Hi Uwe,
sorry for the delay, I was away for a few days and had an awful lot of
work when I came back.

About your generic platform driver: I think we've got two choices, both
of them are acceptable as far as I'm concerned:

1.) Use the clk API and make your driver depend on it. AFAICS, only ARM
and PPC implement it right now. On some platforms, it will probably
never be implemented. E.g. x86 doesn't have any clocks that could be
controlled that way. It's probably only useful for SoCs.
Advantages: People who need it get clk support for free, without having
to write much code.
Disadvantages: The generic platform driver is not available for all
platforms. It might not be easy to implement the dependency in Kconfig
in a way acceptable to all maintainers ;-)

2.) Don't use the clk API. I don't think we would lose much. Drivers
could implement clk stuff in their board support. You could add some
generic function pointers in struct uio_platdata that are called in
open/release/probe/remove. That way, any platform specific stuff,
including clk, could be handled.
Advantages: The generic platform driver is available for all
platforms, no need for dependencies in Kconfig.
Disadvantages: People who need clk_* must write a lot of code within
their board support file. Not nice and clean...

I'm ready to accept 1.) or 2.), or even both of them (why can't we have
two generic platform drivers?)

As you are the author (and probably user) of this driver, please decide,
and send a new patch for review.

Thanks,
Hans

Re: [PATCH 3/3] UIO: generic platform driver

[Uwe Kleine-König]

Hello Hans,

Hans J. Koch wrote:
> On Wed, Apr 23, 2008 at 10:56:29AM +0200, Uwe Kleine-König wrote:
> > Hello,
> > 
> > Uwe Kleine-König wrote:
> > > Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> > > ---
> > > Hello,
> > > 
> > > This is the former patch 4/4 after some discussion.
> > > 
> > > Open issues:
> > >   - clock name "uio" isn't considered good by Russell King
> > >     I don't have a better suggestion
> > I added another branch[1] on my repo that doesn't have the dummy clk
> > patch and variant of this one that doesn't use the clk API.
> > 
> > This way the clk API isn't needed anymore for my patch and the issue
> > about the clock name disappeard, too.
> 
> Hi Uwe,
> sorry for the delay, I was away for a few days and had an awful lot of
> work when I came back.
> 
> About your generic platform driver: I think we've got two choices, both
> of them are acceptable as far as I'm concerned:
> 
> 1.) Use the clk API and make your driver depend on it. AFAICS, only ARM
> and PPC implement it right now. On some platforms, it will probably
> never be implemented. E.g. x86 doesn't have any clocks that could be
> controlled that way. It's probably only useful for SoCs.
> Advantages: People who need it get clk support for free, without having
> to write much code.
> Disadvantages: The generic platform driver is not available for all
> platforms. It might not be easy to implement the dependency in Kconfig
> in a way acceptable to all maintainers ;-)
> 
> 2.) Don't use the clk API. I don't think we would lose much. Drivers
> could implement clk stuff in their board support. You could add some
> generic function pointers in struct uio_platdata that are called in
> open/release/probe/remove. That way, any platform specific stuff,
> including clk, could be handled.
> Advantages: The generic platform driver is available for all
> platforms, no need for dependencies in Kconfig.
> Disadvantages: People who need clk_* must write a lot of code within
> their board support file. Not nice and clean...
> 
> I'm ready to accept 1.) or 2.), or even both of them (why can't we have
> two generic platform drivers?)
For now I suggest 2).  Using the clk API might be implemented by a
generic open/release routine.  Maybe I will look into that at a later
time.  For now I'm happy without clk support, too.

For now you can find two patches in my uio branch at
git://www.modarm9.com/gitsrc/pub/people/ukleinek/linux-2.6.git uio

I rebased them to current Linus' master; otherwise they are unmodified
since the last posts.  For completeness I'll resend them as a reply to
this mail.

For shortlog and diffstat see below.

Best regards
Uwe

Uwe Kleine-König (2):
      UIO: don't let UIO_CIF and UIO_SMX depend twice on UIO
      UIO: generic platform driver

 drivers/uio/Kconfig    |   10 +++-
 drivers/uio/Makefile   |    1 +
 drivers/uio/uio_pdrv.c |  118 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 127 insertions(+), 2 deletions(-)

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

[PATCH] UIO: don't let UIO_CIF and UIO_SMX depend twice on UIO

[Uwe Kleine-König]

ae210f188614bb3d1ee3f19c64e28e3cdd44877c introduced a big "if UIO"/"endif"
where all uio drivers are defined.  So know there is no need for them to
depend explicitly on UIO.

Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
 drivers/uio/Kconfig |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index a4aaab9..78e139c 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -15,7 +15,7 @@ if UIO
 
 config UIO_CIF
 	tristate "generic Hilscher CIF Card driver"
-	depends on UIO && PCI
+	depends on PCI
 	default n
 	help
 	  Driver for Hilscher CIF DeviceNet and Profibus cards.  This
@@ -28,7 +28,6 @@ config UIO_CIF
 
 config UIO_SMX
 	tristate "SMX cryptengine UIO interface"
-	depends on UIO
 	default n
 	help
 	  Userspace IO interface to the Cryptography engine found on the
-- 
1.5.5.1

[PATCH] UIO: generic platform driver

[Uwe Kleine-König]

Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
 drivers/uio/Kconfig    |    7 +++
 drivers/uio/Makefile   |    1 +
 drivers/uio/uio_pdrv.c |  118 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 126 insertions(+), 0 deletions(-)
 create mode 100644 drivers/uio/uio_pdrv.c

diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index 78e139c..2e9079d 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -26,6 +26,13 @@ config UIO_CIF
 	  To compile this driver as a module, choose M here: the module
 	  will be called uio_cif.
 
+config UIO_PDRV
+	tristate "Userspace I/O platform driver"
+	help
+	  Generic platform driver for Userspace I/O devices.
+
+	  If you don't know what to do here, say N.
+
 config UIO_SMX
 	tristate "SMX cryptengine UIO interface"
 	default n
diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
index 18c4566..e00ce0d 100644
--- a/drivers/uio/Makefile
+++ b/drivers/uio/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_UIO)	+= uio.o
 obj-$(CONFIG_UIO_CIF)	+= uio_cif.o
+obj-$(CONFIG_UIO_PDRV)	+= uio_pdrv.o
 obj-$(CONFIG_UIO_SMX)	+= uio_smx.o
diff --git a/drivers/uio/uio_pdrv.c b/drivers/uio/uio_pdrv.c
new file mode 100644
index 0000000..5d0d2e8
--- /dev/null
+++ b/drivers/uio/uio_pdrv.c
@@ -0,0 +1,118 @@
+/*
+ * drivers/uio/uio_pdrv.c
+ *
+ * Copyright (C) 2008 by Digi International Inc.
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+#include <linux/platform_device.h>
+#include <linux/uio_driver.h>
+#include <linux/stringify.h>
+
+#define DRIVER_NAME "uio"
+
+struct uio_platdata {
+	struct uio_info *uioinfo;
+};
+
+static int uio_pdrv_probe(struct platform_device *pdev)
+{
+	struct uio_info *uioinfo = pdev->dev.platform_data;
+	struct uio_platdata *pdata;
+	struct uio_mem *uiomem;
+	int ret = -ENODEV;
+	int i;
+
+	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
+		dev_dbg(&pdev->dev, "%s: err_uioinfo\n", __func__);
+		goto err_uioinfo;
+	}
+
+	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+	if (!pdata) {
+		ret = -ENOMEM;
+		dev_dbg(&pdev->dev, "%s: err_alloc_pdata\n", __func__);
+		goto err_alloc_pdata;
+	}
+
+	pdata->uioinfo = uioinfo;
+
+	uiomem = &uioinfo->mem[0];
+
+	for (i = 0; i < pdev->num_resources; ++i) {
+		struct resource *r = &pdev->resource[i];
+
+		if (r->flags != IORESOURCE_MEM)
+			continue;
+
+		if (uiomem >= &uioinfo->mem[MAX_UIO_MAPS]) {
+			dev_warn(&pdev->dev, "device has more than "
+					__stringify(MAX_UIO_MAPS)
+					" I/O memory resources.\n");
+			break;
+		}
+
+		uiomem->memtype = UIO_MEM_PHYS;
+		uiomem->addr = r->start;
+		uiomem->size = r->end - r->start + 1;
+		++uiomem;
+	}
+
+	while (uiomem < &uioinfo->mem[MAX_UIO_MAPS]) {
+		uiomem->size = 0;
+		++uiomem;
+	}
+
+	pdata->uioinfo->priv = pdata;
+
+	ret = uio_register_device(&pdev->dev, pdata->uioinfo);
+
+	if (ret) {
+		kfree(pdata);
+err_alloc_pdata:
+err_uioinfo:
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, pdata);
+
+	return 0;
+}
+
+static int uio_pdrv_remove(struct platform_device *pdev)
+{
+	struct uio_platdata *pdata = platform_get_drvdata(pdev);
+
+	uio_unregister_device(pdata->uioinfo);
+
+	return 0;
+}
+
+static struct platform_driver uio_pdrv = {
+	.probe = uio_pdrv_probe,
+	.remove = uio_pdrv_remove,
+	.driver = {
+		.name = DRIVER_NAME,
+		.owner = THIS_MODULE,
+	},
+};
+
+static int __init uio_pdrv_init(void)
+{
+	return platform_driver_register(&uio_pdrv);
+}
+
+static void __exit uio_pdrv_exit(void)
+{
+	platform_driver_unregister(&uio_pdrv);
+}
+module_init(uio_pdrv_init);
+module_exit(uio_pdrv_exit);
+
+MODULE_AUTHOR("Uwe Kleine-Koenig");
+MODULE_DESCRIPTION("Userspace I/O platform driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" DRIVER_NAME);
-- 
1.5.5.1

Re: [PATCH] UIO: generic platform driver

[Hans J. Koch]

On Tue, May 20, 2008 at 11:24:42AM +0200, Uwe Kleine-König wrote:
> Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>

Signed-off-by: Hans J. Koch <hjk@linutronix.de>

> ---
>  drivers/uio/Kconfig    |    7 +++
>  drivers/uio/Makefile   |    1 +
>  drivers/uio/uio_pdrv.c |  118 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 126 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/uio/uio_pdrv.c
> 
> diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> index 78e139c..2e9079d 100644
> --- a/drivers/uio/Kconfig
> +++ b/drivers/uio/Kconfig
> @@ -26,6 +26,13 @@ config UIO_CIF
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called uio_cif.
>  
> +config UIO_PDRV
> +	tristate "Userspace I/O platform driver"
> +	help
> +	  Generic platform driver for Userspace I/O devices.
> +
> +	  If you don't know what to do here, say N.
> +
>  config UIO_SMX
>  	tristate "SMX cryptengine UIO interface"
>  	default n
> diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
> index 18c4566..e00ce0d 100644
> --- a/drivers/uio/Makefile
> +++ b/drivers/uio/Makefile
> @@ -1,3 +1,4 @@
>  obj-$(CONFIG_UIO)	+= uio.o
>  obj-$(CONFIG_UIO_CIF)	+= uio_cif.o
> +obj-$(CONFIG_UIO_PDRV)	+= uio_pdrv.o
>  obj-$(CONFIG_UIO_SMX)	+= uio_smx.o
> diff --git a/drivers/uio/uio_pdrv.c b/drivers/uio/uio_pdrv.c
> new file mode 100644
> index 0000000..5d0d2e8
> --- /dev/null
> +++ b/drivers/uio/uio_pdrv.c
> @@ -0,0 +1,118 @@
> +/*
> + * drivers/uio/uio_pdrv.c
> + *
> + * Copyright (C) 2008 by Digi International Inc.
> + * All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +#include <linux/platform_device.h>
> +#include <linux/uio_driver.h>
> +#include <linux/stringify.h>
> +
> +#define DRIVER_NAME "uio"
> +
> +struct uio_platdata {
> +	struct uio_info *uioinfo;
> +};
> +
> +static int uio_pdrv_probe(struct platform_device *pdev)
> +{
> +	struct uio_info *uioinfo = pdev->dev.platform_data;
> +	struct uio_platdata *pdata;
> +	struct uio_mem *uiomem;
> +	int ret = -ENODEV;
> +	int i;
> +
> +	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
> +		dev_dbg(&pdev->dev, "%s: err_uioinfo\n", __func__);
> +		goto err_uioinfo;
> +	}
> +
> +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata) {
> +		ret = -ENOMEM;
> +		dev_dbg(&pdev->dev, "%s: err_alloc_pdata\n", __func__);
> +		goto err_alloc_pdata;
> +	}
> +
> +	pdata->uioinfo = uioinfo;
> +
> +	uiomem = &uioinfo->mem[0];
> +
> +	for (i = 0; i < pdev->num_resources; ++i) {
> +		struct resource *r = &pdev->resource[i];
> +
> +		if (r->flags != IORESOURCE_MEM)
> +			continue;
> +
> +		if (uiomem >= &uioinfo->mem[MAX_UIO_MAPS]) {
> +			dev_warn(&pdev->dev, "device has more than "
> +					__stringify(MAX_UIO_MAPS)
> +					" I/O memory resources.\n");
> +			break;
> +		}
> +
> +		uiomem->memtype = UIO_MEM_PHYS;
> +		uiomem->addr = r->start;
> +		uiomem->size = r->end - r->start + 1;
> +		++uiomem;
> +	}
> +
> +	while (uiomem < &uioinfo->mem[MAX_UIO_MAPS]) {
> +		uiomem->size = 0;
> +		++uiomem;
> +	}
> +
> +	pdata->uioinfo->priv = pdata;
> +
> +	ret = uio_register_device(&pdev->dev, pdata->uioinfo);
> +
> +	if (ret) {
> +		kfree(pdata);
> +err_alloc_pdata:
> +err_uioinfo:
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, pdata);
> +
> +	return 0;
> +}
> +
> +static int uio_pdrv_remove(struct platform_device *pdev)
> +{
> +	struct uio_platdata *pdata = platform_get_drvdata(pdev);
> +
> +	uio_unregister_device(pdata->uioinfo);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver uio_pdrv = {
> +	.probe = uio_pdrv_probe,
> +	.remove = uio_pdrv_remove,
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.owner = THIS_MODULE,
> +	},
> +};
> +
> +static int __init uio_pdrv_init(void)
> +{
> +	return platform_driver_register(&uio_pdrv);
> +}
> +
> +static void __exit uio_pdrv_exit(void)
> +{
> +	platform_driver_unregister(&uio_pdrv);
> +}
> +module_init(uio_pdrv_init);
> +module_exit(uio_pdrv_exit);
> +
> +MODULE_AUTHOR("Uwe Kleine-Koenig");
> +MODULE_DESCRIPTION("Userspace I/O platform driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRIVER_NAME);
> -- 
> 1.5.5.1

Re: [PATCH] UIO: don't let UIO_CIF and UIO_SMX depend twice on UIO

[Hans J. Koch]

On Tue, May 20, 2008 at 11:24:41AM +0200, Uwe Kleine-König wrote:
> ae210f188614bb3d1ee3f19c64e28e3cdd44877c introduced a big "if UIO"/"endif"
> where all uio drivers are defined.  So know there is no need for them to
> depend explicitly on UIO.

Ahem, of course. Thanks!

> 
> Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>

Signed-off-by: Hans J. Koch <hjk@linutronix.de>

> ---
>  drivers/uio/Kconfig |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> index a4aaab9..78e139c 100644
> --- a/drivers/uio/Kconfig
> +++ b/drivers/uio/Kconfig
> @@ -15,7 +15,7 @@ if UIO
>  
>  config UIO_CIF
>  	tristate "generic Hilscher CIF Card driver"
> -	depends on UIO && PCI
> +	depends on PCI
>  	default n
>  	help
>  	  Driver for Hilscher CIF DeviceNet and Profibus cards.  This
> @@ -28,7 +28,6 @@ config UIO_CIF
>  
>  config UIO_SMX
>  	tristate "SMX cryptengine UIO interface"
> -	depends on UIO
>  	default n
>  	help
>  	  Userspace IO interface to the Cryptography engine found on the
> -- 
> 1.5.5.1

Re: [PATCH] UIO: generic platform driver

[Uwe Kleine-König]

Hello,

Hans J. Koch wrote:
> On Tue, May 20, 2008 at 11:24:42AM +0200, Uwe Kleine-König wrote:
> > Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> 
> Signed-off-by: Hans J. Koch <hjk@linutronix.de>
I assume now these two patches are ready go into mainline?!  What's the
next step?  Greg, do you take them into your driver tree?

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH] UIO: generic platform driver

[Greg KH]

On Mon, May 26, 2008 at 07:58:18AM +0200, Uwe Kleine-K?nig wrote:
> Hello,
> 
> Hans J. Koch wrote:
> > On Tue, May 20, 2008 at 11:24:42AM +0200, Uwe Kleine-K?nig wrote:
> > > Signed-off-by: Uwe Kleine-K?nig <Uwe.Kleine-Koenig@digi.com>
> > 
> > Signed-off-by: Hans J. Koch <hjk@linutronix.de>
> I assume now these two patches are ready go into mainline?!  What's the
> next step?  Greg, do you take them into your driver tree?

Yes, if everyone has finally agreed, Hans, can you resend them to me so
I know what to apply, with you signed-off-by?

thanks,

greg k-h

Re: [PATCH] UIO: generic platform driver

[Uwe Kleine-König]

Hello Greg,

Greg KH wrote:
> On Mon, May 26, 2008 at 07:58:18AM +0200, Uwe Kleine-K?nig wrote:
> > Hello,
> > 
> > Hans J. Koch wrote:
> > > On Tue, May 20, 2008 at 11:24:42AM +0200, Uwe Kleine-K?nig wrote:
> > > > Signed-off-by: Uwe Kleine-K?nig <Uwe.Kleine-Koenig@digi.com>
> > > 
> > > Signed-off-by: Hans J. Koch <hjk@linutronix.de>
> > I assume now these two patches are ready go into mainline?!  What's the
> > next step?  Greg, do you take them into your driver tree?
> 
> Yes, if everyone has finally agreed, Hans, can you resend them to me so
> I know what to apply, with you signed-off-by?
If Hans did that, I didn't notice it.  If you want to take the patches
from me you can pull them from my uio branch at

	git://www.modarm9.com/gitsrc/pub/people/ukleinek/linux-2.6.git uio

These have the Signed-off-by: by Hans, too.

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH] UIO: generic platform driver

[Greg KH]

On Fri, May 30, 2008 at 11:16:31AM +0200, Uwe Kleine-König wrote:
> Hello Greg,
> 
> Greg KH wrote:
> > On Mon, May 26, 2008 at 07:58:18AM +0200, Uwe Kleine-K?nig wrote:
> > > Hello,
> > > 
> > > Hans J. Koch wrote:
> > > > On Tue, May 20, 2008 at 11:24:42AM +0200, Uwe Kleine-K?nig wrote:
> > > > > Signed-off-by: Uwe Kleine-K?nig <Uwe.Kleine-Koenig@digi.com>
> > > > 
> > > > Signed-off-by: Hans J. Koch <hjk@linutronix.de>
> > > I assume now these two patches are ready go into mainline?!  What's the
> > > next step?  Greg, do you take them into your driver tree?
> > 
> > Yes, if everyone has finally agreed, Hans, can you resend them to me so
> > I know what to apply, with you signed-off-by?
> If Hans did that, I didn't notice it.  If you want to take the patches
> from me you can pull them from my uio branch at
> 
> 	git://www.modarm9.com/gitsrc/pub/people/ukleinek/linux-2.6.git uio
> 
> These have the Signed-off-by: by Hans, too.

Hans is at LinuxTag this week and said he would send them to me when he
got back.  So I'll wait for his copies, I'd prefer not to pull from git,
as that doesn't really work well with my patchflow process.

thanks,

greg k-h

Re: [PATCH] UIO: generic platform driver

[Uwe Kleine-König]

Hello Greg,

Greg KH wrote:
> > > Yes, if everyone has finally agreed, Hans, can you resend them to me so
> > > I know what to apply, with you signed-off-by?
> > If Hans did that, I didn't notice it.  If you want to take the patches
> > from me you can pull them from my uio branch at
> > 
> > 	git://www.modarm9.com/gitsrc/pub/people/ukleinek/linux-2.6.git uio
> > 
> > These have the Signed-off-by: by Hans, too.
> 
> Hans is at LinuxTag this week and said he would send them to me when he
> got back.  So I'll wait for his copies,
OK.

>                                         I'd prefer not to pull from git,
> as that doesn't really work well with my patchflow process.
I know you work with quilt, but I thought it to be no problem for you to
extract the patches from my tree into your quilt queue.  I'll remember
that for the future. :-)

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH] UIO: generic platform driver

[Hans J. Koch]

On Tue, Jun 03, 2008 at 09:21:55AM +0200, Uwe Kleine-K?nig wrote:
> Hello Greg,
> 
> Greg KH wrote:
> > > > Yes, if everyone has finally agreed, Hans, can you resend them to me so
> > > > I know what to apply, with you signed-off-by?
> > > If Hans did that, I didn't notice it.  If you want to take the patches
> > > from me you can pull them from my uio branch at
> > > 
> > > 	git://www.modarm9.com/gitsrc/pub/people/ukleinek/linux-2.6.git uio
> > > 
> > > These have the Signed-off-by: by Hans, too.
> > 
> > Hans is at LinuxTag this week and said he would send them to me when he
> > got back.  So I'll wait for his copies,
> OK.

I sent the latest version to Greg, should get merged soon.

Thanks,
Hans