UIO: fixes, cleanups and a new driver

Signed-off-by Uwe Kleine-König <[email protected]>

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

drivers/uio/Kconfig +7 -0

index 6bc2891..5ec353f 100644
@@ -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

drivers/uio/Makefile +1 -0

index 7fecfb4..a6dcb99 100644
@@ -1,2 +1,3 @@
 obj-$(CONFIG_UIO)	+= uio.o
 obj-$(CONFIG_UIO_CIF)	+= uio_cif.o
+obj-$(CONFIG_UIO_PDRV) += uio_pdrv.o

drivers/uio/uio_pdrv.c new file +165 -0

new file mode 100644
index 0000000..31d1aaf
@@ -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);

Signature · 1 line

1.5.4.5

Quoted 7 lines · Uwe Kleine-König

Signed-off-by Uwe Kleine-König <[email protected]>

---
 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?

Hello Hans,

Quoted 9 lines · Hans J. Koch

Quoted 7 lines · Uwe Kleine-König

Signed-off-by Uwe Kleine-König <[email protected]>

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

Quoted 5 lines

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

Signature · 124 lines

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

Quoted 9 lines · Uwe Kleine-König

Quoted 7 lines · Uwe Kleine-König

Signed-off-by Uwe Kleine-König <[email protected]>

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

Quoted 5 lines

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?

Signed-off-by Uwe Kleine-König <[email protected]>

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

drivers/uio/Kconfig +7 -0

index 6bc2891..5ec353f 100644
@@ -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

drivers/uio/Makefile +1 -0

index 7fecfb4..a6dcb99 100644
@@ -1,2 +1,3 @@
 obj-$(CONFIG_UIO)	+= uio.o
 obj-$(CONFIG_UIO_CIF)	+= uio_cif.o
+obj-$(CONFIG_UIO_PDRV) += uio_pdrv.o

drivers/uio/uio_pdrv.c new file +9 -0

new file mode 100644
index 0000000..31d1aaf
@@ -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.

drivers/uio/uio_pdrv.c new file +6 -0

+#ifndef STRINGIFY
+#define STRINGIFY(x) __STRINGIFY_HELPER(x)
+#define __STRINGIFY_HELPER(x) #x
+#endif
+
+struct uio_platdata {

drivers/uio/uio_pdrv.c new file +46 -0

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

drivers/uio/uio_pdrv.c new file +14 -0

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

drivers/uio/uio_pdrv.c new file +42 -0

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

Signature · 1 line

1.5.4.5

Quoted 3 lines

+/* 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.

drivers/uio/uio_pdrv.c line 35

Quoted 6 lines

drivers/uio/uio_pdrv.c new file +6 -0

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

drivers/uio/uio_pdrv.c line 91

Quoted 2 lines

drivers/uio/uio_pdrv.c new file +2 -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.

drivers/uio/uio_pdrv.c line 96

Quoted 5 lines

drivers/uio/uio_pdrv.c new file +5 -0

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

I'd prefer this:

		if (i >= MAX_UIO_MAPS) {
		...

drivers/uio/uio_pdrv.c line 99

Quoted 3 lines

drivers/uio/uio_pdrv.c new file +3 -0

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

Quoted 6 lines

drivers/uio/uio_pdrv.c new file +6 -0

+	ret = uio_register_device(&pdev->dev, pdata->uioinfo);
+
+	if (ret) {
+		clk_put(pdata->clk);
+err_clk_get:
+

drivers/uio/uio_pdrv.c line 123

Quoted 5 lines

drivers/uio/uio_pdrv.c new file +5 -0

+		kfree(pdata);
+err_alloc_pdata:
+err_uioinfo:
+		return ret;
+	}

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

Quoted 11 lines

Review

Quoted 7 lines

(quoted diff) +7 -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]) {

I'd prefer this:

		if (i >= MAX_UIO_MAPS) {
		...

OK.

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

drivers/uio/Kconfig +7 -0

index 6bc2891..5ec353f 100644
@@ -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

drivers/uio/Makefile +1 -0

index 7fecfb4..a6dcb99 100644
@@ -1,2 +1,3 @@
 obj-$(CONFIG_UIO)	+= uio.o
 obj-$(CONFIG_UIO_CIF)	+= uio_cif.o
+obj-$(CONFIG_UIO_PDRV) += uio_pdrv.o

drivers/uio/uio_pdrv.c new file +163 -0

new file mode 100644
index 0000000..47506aa
@@ -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);

Signature · 8 lines

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

Quoted 3 lines · Uwe Kleine-König

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?

Hello,

Quoted 9 lines · Hans J. Koch

Quoted 3 lines · Uwe Kleine-König

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.

Quoted 7 lines

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!

Quoted 1 line

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*\)/;'

Signature · 3 lines

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

Quoted 22 lines · Uwe Kleine-König

Hello,

Quoted 9 lines · Hans J. Koch

Quoted 3 lines · Uwe Kleine-König

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.

Quoted 7 lines

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!

Quoted 5 lines

Quoted 1 line

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).

Hello,

[Added Russell King to Cc:]

Quoted 29 lines · Hans J. Koch

Quoted 22 lines · Uwe Kleine-König

Hello,

Quoted 9 lines · Hans J. Koch

Quoted 3 lines · Uwe Kleine-König

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.

Quoted 7 lines

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!

Quoted 5 lines

Quoted 1 line

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].

Quoted 4 lines

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!?

Quoted 17 lines

Quoted 13 lines

Quoted 7 lines

Quoted 7 lines

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!

Quoted 5 lines

Quoted 1 line

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].

Quoted 4 lines

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).

Quoted 5 lines

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.

Hello,

Quoted 26 lines

Quoted 17 lines

Quoted 13 lines

Quoted 7 lines

Quoted 7 lines

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!

Quoted 5 lines

Quoted 1 line

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].

Quoted 4 lines

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).

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 33 lines · Uwe Kleine-König

Quoted 26 lines

Quoted 17 lines

Quoted 13 lines

Quoted 7 lines

Quoted 7 lines

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!

Quoted 5 lines

Quoted 1 line

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].

Quoted 4 lines

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).

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 6 lines

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?

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

Hi Russell,

Quoted 41 lines · Russell King - ARM Linux

Quoted 33 lines · Uwe Kleine-König

Quoted 26 lines

Quoted 17 lines

Quoted 13 lines

Quoted 7 lines

Quoted 7 lines

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!

Quoted 5 lines

Quoted 1 line

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].

Quoted 4 lines

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).

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

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

Quoted 6 lines

Quoted 2 lines

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?

(quoted diff) +4 -0

+               /* 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 ?

(quoted diff) +4 -0

+       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)?

(quoted diff) +7 -0

+/* 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> ?

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

Quoted 2 lines · Uwe Kleine-König

[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.

Hello,

drivers/uio/uio_pdrv.c line 91

Quoted 2 lines · Uwe Kleine-K?nig

drivers/uio/uio_pdrv.c new file +2 -0

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

Quoted 7 lines · Uwe Kleine-K?nig

Hello,

drivers/uio/uio_pdrv.c line 91

Quoted 2 lines · Uwe Kleine-K?nig

drivers/uio/uio_pdrv.c new file +2 -0

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

Quoted 11 lines · Greg KH

Quoted 7 lines · Uwe Kleine-K?nig

Hello,

drivers/uio/uio_pdrv.c line 91

Quoted 2 lines · Uwe Kleine-K?nig

drivers/uio/uio_pdrv.c new file +2 -0

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

Hello Hans,

Quoted 4 lines · Hans J. Koch

Quoted 3 lines

+/* XXX: I thought there already exists something like STRINGIFY, but I could not
+ * find it ...
+ */

Quoted 2 lines

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:

Quoted 9 lines

drivers/uio/uio_pdrv.c line 35

Quoted 7 lines

drivers/uio/uio_pdrv.c new file +6 -0

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

Quoted 5 lines

drivers/uio/uio_pdrv.c line 91

Quoted 3 lines

drivers/uio/uio_pdrv.c new file +2 -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.

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.

Quoted 10 lines

drivers/uio/uio_pdrv.c line 96

Quoted 6 lines

drivers/uio/uio_pdrv.c new file +5 -0

+
+		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.)

Quoted 11 lines

drivers/uio/uio_pdrv.c line 99

Quoted 4 lines

drivers/uio/uio_pdrv.c new file +3 -0

+			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 :-)

Quoted 15 lines

drivers/uio/uio_pdrv.c line 123

Quoted 12 lines

drivers/uio/uio_pdrv.c new file +11 -0

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

Quoted 6 lines · Uwe Kleine-König

Quoted 4 lines

drivers/uio/uio_pdrv.c line 91

Quoted 2 lines

drivers/uio/uio_pdrv.c new file +2 -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.

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.

Quoted 12 lines

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:

Quoted 4 lines

	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...

Quoted 1 line

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).

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

Signed-off-by Uwe Kleine-König <[email protected]>

---
 drivers/uio/Kconfig |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

drivers/uio/Kconfig +5 -1

index a899306..6bc2891 100644
@@ -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

Signature · 1 line

1.5.4.5

Quoted 2 lines · Uwe Kleine-K??nig

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

Agreed.

Signed-off-by Hans J Koch <[email protected]>

This allows configuring CONFIG_UIO without changing into the UIO submenu

Signed-off-by Uwe Kleine-König <[email protected]>

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

drivers/uio/Kconfig +2 -6

index b778ed7..a899306 100644
@@ -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

Signature · 1 line

1.5.4.5

Quoted 1 line · Uwe Kleine-K??nig

This allows configuring CONFIG_UIO without changing into the UIO submenu

I like it.

Signed-off-by Hans J Koch <[email protected]>

Quoted 5 lines · Hans J. Koch

Quoted 1 line · Uwe Kleine-K??nig

This allows configuring CONFIG_UIO without changing into the UIO submenu

I like it.

Quoted 1 line

Signed-off-by Hans J Koch <[email protected]>

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

:)

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

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

Signed-off-by Uwe Kleine-König <[email protected]>

---
 drivers/uio/uio.c |   40 +++++++++++++++++++++++-----------------
 1 files changed, 23 insertions(+), 17 deletions(-)

drivers/uio/uio.c +12 -8

index 1175908..005fc55 100644
@@ -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);

drivers/uio/uio.c +4 -8

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

Signature · 1 line

1.5.4.5

Quoted 2 lines · Uwe Kleine-K??nig

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()?

Quoted 4 lines · Hans J. Koch

Quoted 2 lines · Uwe Kleine-K??nig

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

Quoted 5 lines

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

Quoted 2 lines · Uwe Kleine-König

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()?

Quoted 12 lines

drivers/uio/uio.c +3 -6

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

drivers/uio/uio.c static int uio_open(struct inode *inode, struct file *filep) · line 333

Quoted 10 lines

drivers/uio/uio.c +7 -1

+			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".

Quoted 2 lines

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.

Quoted 35 lines

Quoted 13 lines

drivers/uio/uio.c +3 -6

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

drivers/uio/uio.c static int uio_open(struct inode *inode, struct file *filep) · line 333

Quoted 10 lines

drivers/uio/uio.c +7 -1

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

Quoted 4 lines

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.

Quoted 4 lines

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. :-)

Quoted 9 lines

Quoted 3 lines

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?

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

Signed-off-by Uwe Kleine-König <[email protected]>

---

Quoted 10 lines · Hans J. Koch

Quoted 9 lines

Quoted 3 lines

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. :-)

Quoted 7 lines

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 ...

Quoted 4 lines

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(-)

drivers/uio/uio.c +21 -15

index 1175908..55cc7b8 100644
@@ -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;
 	}
 

Signature · 7 lines

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

Signed-off-by Uwe Kleine-König <[email protected]>

---
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.

Quoted 3 lines

- 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.

Signed-off-by Uwe Kleine-König <[email protected]>

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

Hello,

Quoted 6 lines · Uwe Kleine-König

Signed-off-by Uwe Kleine-König <[email protected]>

---
Hello,

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

Quoted 3 lines

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.

Quoted 9 lines · Uwe Kleine-König

Hello,

Quoted 6 lines · Uwe Kleine-König

Signed-off-by Uwe Kleine-König <[email protected]>

---
Hello,

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

Quoted 8 lines

Quoted 3 lines

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?)

Signed-off-by Uwe Kleine-König <[email protected]>

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

drivers/uio/Kconfig +7 -0

index 78e139c..2e9079d 100644
@@ -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

drivers/uio/Makefile +1 -0

index 18c4566..e00ce0d 100644
@@ -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

drivers/uio/uio_pdrv.c new file +118 -0

new file mode 100644
index 0000000..5d0d2e8
@@ -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);

Signature · 1 line

1.5.5.1

Quoted 1 line · Uwe Kleine-König

Signed-off-by Uwe Kleine-König <[email protected]>

Signed-off-by Hans J. Koch <[email protected]>

Hello,

Quoted 4 lines · Hans J. Koch

Quoted 1 line · Uwe Kleine-K?nig

Signed-off-by Uwe Kleine-K?nig <[email protected]>

Signed-off-by Hans J. Koch <[email protected]>

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?

Quoted 4 lines · Uwe Kleine-K?nig

Hello,

Quoted 1 line · Hans J. Koch

On Tue, May 20, 2008 at 11:24:42AM +0200, Uwe Kleine-K?nig wrote:

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Signed-off-by Uwe Kleine-K?nig <[email protected]>

Signed-off-by Hans J. Koch <[email protected]>

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?

Hello Greg,

Quoted 11 lines · Greg KH

Quoted 9 lines · Uwe Kleine-K?nig

Hello,

Quoted 4 lines · Hans J. Koch

Quoted 1 line · Uwe Kleine-K?nig

Signed-off-by Uwe Kleine-K?nig <[email protected]>

Signed-off-by Hans J. Koch <[email protected]>

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?

Quoted 2 lines

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.

Quoted 8 lines

Quoted 2 lines

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.

Hello Greg,

Quoted 5 lines · Greg KH

Quoted 5 lines

Quoted 2 lines

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

Quoted 6 lines

Quoted 3 lines

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.

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 <[email protected]>

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

drivers/uio/Kconfig +1 -2

index a4aaab9..78e139c 100644
@@ -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

Signature · 1 line

1.5.5.1

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.