[PATCH 0/2] add OF wrapper for uio-pdrv-genirq

[PATCH 0/2] add OF wrapper for uio-pdrv-genirq

[Wolfram Sang]

This series adds an OF wrapper for the uio-pdrv-genirq driver.

Patch 1 refactors the platform driver to expose a generic probe-routine.
Patch 2 then adds the OF wrapper. Documentation for the binding is also added.
        There may be an issue with stack-usage, as noted there.

Tested on MPC5200B-based custom boards with 2.6.30.

(Note: this is not meant for the current merge window.)

Thanks,

   Wolfram

[PATCH 1/2] uio/pdrv_genirq: Refactor probe routine to expose a generic part

[Wolfram Sang]

This patch refactors the probe routine, so that an of-version of a similiar
driver can pick it up later.

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Magnus Damm <magnus.damm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Hans J. Koch <hjk-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Cc: Greg KH <gregkh-l3A5Bk7waGM@public.gmane.org>
---
 drivers/uio/uio_pdrv_genirq.c   |   60 ++++++++++++++++++++------------------
 include/linux/uio_pdrv_genirq.h |   13 ++++++++
 2 files changed, 45 insertions(+), 28 deletions(-)
 create mode 100644 include/linux/uio_pdrv_genirq.h

diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index 3f06818..8f8a0f9 100644
--- a/drivers/uio/uio_pdrv_genirq.c
+++ b/drivers/uio/uio_pdrv_genirq.c
@@ -20,15 +20,10 @@
 #include <linux/bitops.h>
 #include <linux/interrupt.h>
 #include <linux/stringify.h>
+#include <linux/uio_pdrv_genirq.h>
 
 #define DRIVER_NAME "uio_pdrv_genirq"
 
-struct uio_pdrv_genirq_platdata {
-	struct uio_info *uioinfo;
-	spinlock_t lock;
-	unsigned long flags;
-};
-
 static irqreturn_t uio_pdrv_genirq_handler(int irq, struct uio_info *dev_info)
 {
 	struct uio_pdrv_genirq_platdata *priv = dev_info->priv;
@@ -68,29 +63,18 @@ static int uio_pdrv_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on)
 	return 0;
 }
 
-static int uio_pdrv_genirq_probe(struct platform_device *pdev)
+int __uio_pdrv_genirq_probe(struct device *dev, struct uio_info *uioinfo,
+		struct resource *resources, unsigned int num_resources)
 {
-	struct uio_info *uioinfo = pdev->dev.platform_data;
 	struct uio_pdrv_genirq_platdata *priv;
 	struct uio_mem *uiomem;
-	int ret = -EINVAL;
-	int i;
-
-	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
-		dev_err(&pdev->dev, "missing platform_data\n");
-		goto bad0;
-	}
-
-	if (uioinfo->handler || uioinfo->irqcontrol ||
-	    uioinfo->irq_flags & IRQF_SHARED) {
-		dev_err(&pdev->dev, "interrupt configuration error\n");
-		goto bad0;
-	}
+	unsigned int i;
+	int ret;
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv) {
 		ret = -ENOMEM;
-		dev_err(&pdev->dev, "unable to kmalloc\n");
+		dev_err(dev, "unable to kmalloc\n");
 		goto bad0;
 	}
 
@@ -100,14 +84,14 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
 
 	uiomem = &uioinfo->mem[0];
 
-	for (i = 0; i < pdev->num_resources; ++i) {
-		struct resource *r = &pdev->resource[i];
+	for (i = 0; i < num_resources; ++i) {
+		struct resource *r = resources + i;
 
 		if (r->flags != IORESOURCE_MEM)
 			continue;
 
 		if (uiomem >= &uioinfo->mem[MAX_UIO_MAPS]) {
-			dev_warn(&pdev->dev, "device has more than "
+			dev_warn(dev, "device has more than "
 					__stringify(MAX_UIO_MAPS)
 					" I/O memory resources.\n");
 			break;
@@ -138,19 +122,39 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
 	uioinfo->irqcontrol = uio_pdrv_genirq_irqcontrol;
 	uioinfo->priv = priv;
 
-	ret = uio_register_device(&pdev->dev, priv->uioinfo);
+	ret = uio_register_device(dev, priv->uioinfo);
 	if (ret) {
-		dev_err(&pdev->dev, "unable to register uio device\n");
+		dev_err(dev, "unable to register uio device\n");
 		goto bad1;
 	}
 
-	platform_set_drvdata(pdev, priv);
+	dev_set_drvdata(dev, priv);
 	return 0;
  bad1:
 	kfree(priv);
  bad0:
 	return ret;
 }
+EXPORT_SYMBOL_GPL(__uio_pdrv_genirq_probe);
+
+static int uio_pdrv_genirq_probe(struct platform_device *pdev)
+{
+	struct uio_info *uioinfo = pdev->dev.platform_data;
+
+	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
+		dev_err(&pdev->dev, "missing platform_data\n");
+		return -EINVAL;
+	}
+
+	if (uioinfo->handler || uioinfo->irqcontrol ||
+	    uioinfo->irq_flags & IRQF_SHARED) {
+		dev_err(&pdev->dev, "interrupt configuration error\n");
+		return -EINVAL;
+	}
+
+	return __uio_pdrv_genirq_probe(&pdev->dev, uioinfo, pdev->resource,
+			pdev->num_resources);
+}
 
 static int uio_pdrv_genirq_remove(struct platform_device *pdev)
 {
diff --git a/include/linux/uio_pdrv_genirq.h b/include/linux/uio_pdrv_genirq.h
new file mode 100644
index 0000000..a410390
--- /dev/null
+++ b/include/linux/uio_pdrv_genirq.h
@@ -0,0 +1,13 @@
+#ifndef _LINUX_UIO_PDRV_GENIRQ_H
+#define _LINUX_UIO_PDRV_GENIRQ_H
+
+struct uio_pdrv_genirq_platdata {
+	struct uio_info *uioinfo;
+	spinlock_t lock;
+	unsigned long flags;
+};
+
+extern int __uio_pdrv_genirq_probe(struct device *dev, struct uio_info *uioinfo,
+	struct resource *resources, unsigned int num_resources);
+
+#endif
-- 
1.6.3.1

Re: [PATCH 1/2] uio/pdrv_genirq: Refactor probe routine to expose a generic part

[Hans J. Koch]

On Fri, Jun 12, 2009 at 02:04:21AM +0200, Wolfram Sang wrote:
> This patch refactors the probe routine, so that an of-version of a similiar
> driver can pick it up later.

Looks good to me.

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

> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Cc: Hans J. Koch <hjk@linutronix.de>
> Cc: Greg KH <gregkh@suse.de>
> ---
>  drivers/uio/uio_pdrv_genirq.c   |   60 ++++++++++++++++++++------------------
>  include/linux/uio_pdrv_genirq.h |   13 ++++++++
>  2 files changed, 45 insertions(+), 28 deletions(-)
>  create mode 100644 include/linux/uio_pdrv_genirq.h
> 
> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
> index 3f06818..8f8a0f9 100644
> --- a/drivers/uio/uio_pdrv_genirq.c
> +++ b/drivers/uio/uio_pdrv_genirq.c
> @@ -20,15 +20,10 @@
>  #include <linux/bitops.h>
>  #include <linux/interrupt.h>
>  #include <linux/stringify.h>
> +#include <linux/uio_pdrv_genirq.h>
>  
>  #define DRIVER_NAME "uio_pdrv_genirq"
>  
> -struct uio_pdrv_genirq_platdata {
> -	struct uio_info *uioinfo;
> -	spinlock_t lock;
> -	unsigned long flags;
> -};
> -
>  static irqreturn_t uio_pdrv_genirq_handler(int irq, struct uio_info *dev_info)
>  {
>  	struct uio_pdrv_genirq_platdata *priv = dev_info->priv;
> @@ -68,29 +63,18 @@ static int uio_pdrv_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on)
>  	return 0;
>  }
>  
> -static int uio_pdrv_genirq_probe(struct platform_device *pdev)
> +int __uio_pdrv_genirq_probe(struct device *dev, struct uio_info *uioinfo,
> +		struct resource *resources, unsigned int num_resources)
>  {
> -	struct uio_info *uioinfo = pdev->dev.platform_data;
>  	struct uio_pdrv_genirq_platdata *priv;
>  	struct uio_mem *uiomem;
> -	int ret = -EINVAL;
> -	int i;
> -
> -	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
> -		dev_err(&pdev->dev, "missing platform_data\n");
> -		goto bad0;
> -	}
> -
> -	if (uioinfo->handler || uioinfo->irqcontrol ||
> -	    uioinfo->irq_flags & IRQF_SHARED) {
> -		dev_err(&pdev->dev, "interrupt configuration error\n");
> -		goto bad0;
> -	}
> +	unsigned int i;
> +	int ret;
>  
>  	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>  	if (!priv) {
>  		ret = -ENOMEM;
> -		dev_err(&pdev->dev, "unable to kmalloc\n");
> +		dev_err(dev, "unable to kmalloc\n");
>  		goto bad0;
>  	}
>  
> @@ -100,14 +84,14 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
>  
>  	uiomem = &uioinfo->mem[0];
>  
> -	for (i = 0; i < pdev->num_resources; ++i) {
> -		struct resource *r = &pdev->resource[i];
> +	for (i = 0; i < num_resources; ++i) {
> +		struct resource *r = resources + i;
>  
>  		if (r->flags != IORESOURCE_MEM)
>  			continue;
>  
>  		if (uiomem >= &uioinfo->mem[MAX_UIO_MAPS]) {
> -			dev_warn(&pdev->dev, "device has more than "
> +			dev_warn(dev, "device has more than "
>  					__stringify(MAX_UIO_MAPS)
>  					" I/O memory resources.\n");
>  			break;
> @@ -138,19 +122,39 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
>  	uioinfo->irqcontrol = uio_pdrv_genirq_irqcontrol;
>  	uioinfo->priv = priv;
>  
> -	ret = uio_register_device(&pdev->dev, priv->uioinfo);
> +	ret = uio_register_device(dev, priv->uioinfo);
>  	if (ret) {
> -		dev_err(&pdev->dev, "unable to register uio device\n");
> +		dev_err(dev, "unable to register uio device\n");
>  		goto bad1;
>  	}
>  
> -	platform_set_drvdata(pdev, priv);
> +	dev_set_drvdata(dev, priv);
>  	return 0;
>   bad1:
>  	kfree(priv);
>   bad0:
>  	return ret;
>  }
> +EXPORT_SYMBOL_GPL(__uio_pdrv_genirq_probe);
> +
> +static int uio_pdrv_genirq_probe(struct platform_device *pdev)
> +{
> +	struct uio_info *uioinfo = pdev->dev.platform_data;
> +
> +	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
> +		dev_err(&pdev->dev, "missing platform_data\n");
> +		return -EINVAL;
> +	}
> +
> +	if (uioinfo->handler || uioinfo->irqcontrol ||
> +	    uioinfo->irq_flags & IRQF_SHARED) {
> +		dev_err(&pdev->dev, "interrupt configuration error\n");
> +		return -EINVAL;
> +	}
> +
> +	return __uio_pdrv_genirq_probe(&pdev->dev, uioinfo, pdev->resource,
> +			pdev->num_resources);
> +}
>  
>  static int uio_pdrv_genirq_remove(struct platform_device *pdev)
>  {
> diff --git a/include/linux/uio_pdrv_genirq.h b/include/linux/uio_pdrv_genirq.h
> new file mode 100644
> index 0000000..a410390
> --- /dev/null
> +++ b/include/linux/uio_pdrv_genirq.h
> @@ -0,0 +1,13 @@
> +#ifndef _LINUX_UIO_PDRV_GENIRQ_H
> +#define _LINUX_UIO_PDRV_GENIRQ_H
> +
> +struct uio_pdrv_genirq_platdata {
> +	struct uio_info *uioinfo;
> +	spinlock_t lock;
> +	unsigned long flags;
> +};
> +
> +extern int __uio_pdrv_genirq_probe(struct device *dev, struct uio_info *uioinfo,
> +	struct resource *resources, unsigned int num_resources);
> +
> +#endif
> -- 
> 1.6.3.1

[PATCH 2/2] uio: add an of_genirq driver

[Wolfram Sang]

Picking up the now exported generic probe function from the
platform-variant of this driver, this patch adds an of-version. Also add
the binding documentation.

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Magnus Damm <magnus.damm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Hans J. Koch <hjk-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Cc: Greg KH <gregkh-l3A5Bk7waGM@public.gmane.org>
---

In probe, I put the resources-array on the stack to simplify the code. If this
is considered too huge for the stack (140 byte for a 32-bit system at the
moment), I can also post a version using kzalloc.

 Documentation/powerpc/dts-bindings/uio-generic.txt |   16 +++
 drivers/uio/Kconfig                                |    6 +
 drivers/uio/Makefile                               |    1 +
 drivers/uio/uio_of_genirq.c                        |   98 ++++++++++++++++++++
 4 files changed, 121 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/powerpc/dts-bindings/uio-generic.txt
 create mode 100644 drivers/uio/uio_of_genirq.c

diff --git a/Documentation/powerpc/dts-bindings/uio-generic.txt b/Documentation/powerpc/dts-bindings/uio-generic.txt
new file mode 100644
index 0000000..8ad9861
--- /dev/null
+++ b/Documentation/powerpc/dts-bindings/uio-generic.txt
@@ -0,0 +1,16 @@
+UIO for custom devices
+
+A device which will be mapped using the UIO subsystem.
+
+Properties:
+ - compatible : should contain the specific model used, followed by
+                "generic-uio".
+ - reg : address range(s) of the device (up to MAX_UIO_MAPS)
+ - interrupts : interrupt of the device
+
+Example:
+        c64fpga@0 {
+                compatible = "ptx,c64fpga001", "generic-uio";
+                reg = <0x0 0x10000>;
+                interrupts = <0 0 3>;
+        };
diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index 7f86534..18efe38 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -46,6 +46,12 @@ config UIO_PDRV_GENIRQ
 
 	  If you don't know what to do here, say N.
 
+config UIO_OF_GENIRQ
+	tristate "Userspace I/O OF driver with generic IRQ handling"
+	depends on UIO_PDRV_GENIRQ && OF
+	help
+	  OF wrapper for the above platform driver.
+
 config UIO_SMX
 	tristate "SMX cryptengine UIO interface"
 	default n
diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
index 5c2586d..089fd56 100644
--- a/drivers/uio/Makefile
+++ b/drivers/uio/Makefile
@@ -2,6 +2,7 @@ obj-$(CONFIG_UIO)	+= uio.o
 obj-$(CONFIG_UIO_CIF)	+= uio_cif.o
 obj-$(CONFIG_UIO_PDRV)	+= uio_pdrv.o
 obj-$(CONFIG_UIO_PDRV_GENIRQ)	+= uio_pdrv_genirq.o
+obj-$(CONFIG_UIO_OF_GENIRQ)	+= uio_of_genirq.o
 obj-$(CONFIG_UIO_SMX)	+= uio_smx.o
 obj-$(CONFIG_UIO_AEC)	+= uio_aec.o
 obj-$(CONFIG_UIO_SERCOS3)	+= uio_sercos3.o
diff --git a/drivers/uio/uio_of_genirq.c b/drivers/uio/uio_of_genirq.c
new file mode 100644
index 0000000..254ec5b
--- /dev/null
+++ b/drivers/uio/uio_of_genirq.c
@@ -0,0 +1,98 @@
+/*
+ * OF wrapper to make use of the uio_pdrv_genirq-driver.
+ *
+ * Copyright (C) 2009 Wolfram Sang, Pengutronix
+ *
+ * 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/kernel.h>
+#include <linux/module.h>
+#include <linux/uio_driver.h>
+#include <linux/of_device.h>
+#include <linux/of_platform.h>
+#include <linux/uio_pdrv_genirq.h>
+
+#define OF_DRIVER_VERSION "1"
+
+static __devinit int uio_of_genirq_probe(struct of_device *op,
+		const struct of_device_id *match)
+{
+	struct uio_info *uioinfo;
+	struct resource resources[MAX_UIO_MAPS];
+	int i, ret;
+
+	uioinfo = kzalloc(sizeof(*uioinfo), GFP_KERNEL);
+	if (!uioinfo)
+		return -ENOMEM;
+
+	uioinfo->name = op->node->name;
+	uioinfo->version = OF_DRIVER_VERSION;
+	uioinfo->irq = irq_of_parse_and_map(op->node, 0);
+	if (!uioinfo->irq)
+		uioinfo->irq = UIO_IRQ_NONE;
+
+	for (i = 0; i < MAX_UIO_MAPS; ++i)
+		if (of_address_to_resource(op->node, i, &resources[i]))
+			break;
+
+	ret = __uio_pdrv_genirq_probe(&op->dev, uioinfo, &resources, i);
+	if (ret)
+		goto err_cleanup;
+
+	return 0;
+
+err_cleanup:
+	if (uioinfo->irq != UIO_IRQ_NONE)
+		irq_dispose_mapping(uioinfo->irq);
+
+	kfree(uioinfo);
+	return ret;
+}
+
+static __devexit int uio_of_genirq_remove(struct of_device *op)
+{
+	struct uio_pdrv_genirq_platdata *priv = dev_get_drvdata(&op->dev);
+
+	uio_unregister_device(priv->uioinfo);
+
+	if (priv->uioinfo->irq != UIO_IRQ_NONE)
+		irq_dispose_mapping(priv->uioinfo->irq);
+
+	kfree(priv->uioinfo);
+	kfree(priv);
+	return 0;
+}
+
+/* Match table for of_platform binding */
+static const struct of_device_id __devinitconst uio_of_genirq_match[] = {
+	{ .compatible = "generic-uio", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, uio_of_genirq_match);
+
+static struct of_platform_driver uio_of_genirq_driver = {
+	.owner = THIS_MODULE,
+	.name = "uio-of-genirq",
+	.match_table = uio_of_genirq_match,
+	.probe = uio_of_genirq_probe,
+	.remove = __devexit_p(uio_of_genirq_remove),
+};
+
+static inline int __init uio_of_genirq_init(void)
+{
+	return of_register_platform_driver(&uio_of_genirq_driver);
+}
+module_init(uio_of_genirq_init);
+
+static inline void __exit uio_of_genirq_exit(void)
+{
+	of_unregister_platform_driver(&uio_of_genirq_driver);
+}
+module_exit(uio_of_genirq_exit);
+
+MODULE_AUTHOR("Wolfram Sang");
+MODULE_DESCRIPTION("Userspace I/O OF driver with generic IRQ handling");
+MODULE_LICENSE("GPL v2");
-- 
1.6.3.1

Re: [PATCH 2/2] uio: add an of_genirq driver

[Hans J. Koch]

On Fri, Jun 12, 2009 at 02:04:22AM +0200, Wolfram Sang wrote:
> Picking up the now exported generic probe function from the
> platform-variant of this driver, this patch adds an of-version. Also add
> the binding documentation.

Some minor problems, see below.

> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Cc: Hans J. Koch <hjk@linutronix.de>
> Cc: Greg KH <gregkh@suse.de>
> ---
> 
> In probe, I put the resources-array on the stack to simplify the code. If this
> is considered too huge for the stack (140 byte for a 32-bit system at the
> moment), I can also post a version using kzalloc.
> 
>  Documentation/powerpc/dts-bindings/uio-generic.txt |   16 +++
>  drivers/uio/Kconfig                                |    6 +
>  drivers/uio/Makefile                               |    1 +
>  drivers/uio/uio_of_genirq.c                        |   98 ++++++++++++++++++++
>  4 files changed, 121 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/powerpc/dts-bindings/uio-generic.txt
>  create mode 100644 drivers/uio/uio_of_genirq.c
> 
> diff --git a/Documentation/powerpc/dts-bindings/uio-generic.txt b/Documentation/powerpc/dts-bindings/uio-generic.txt
> new file mode 100644
> index 0000000..8ad9861
> --- /dev/null
> +++ b/Documentation/powerpc/dts-bindings/uio-generic.txt
> @@ -0,0 +1,16 @@
> +UIO for custom devices
> +
> +A device which will be mapped using the UIO subsystem.
> +
> +Properties:
> + - compatible : should contain the specific model used, followed by
> +                "generic-uio".
> + - reg : address range(s) of the device (up to MAX_UIO_MAPS)
> + - interrupts : interrupt of the device
> +
> +Example:
> +        c64fpga@0 {
> +                compatible = "ptx,c64fpga001", "generic-uio";
> +                reg = <0x0 0x10000>;
> +                interrupts = <0 0 3>;
> +        };
> diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> index 7f86534..18efe38 100644
> --- a/drivers/uio/Kconfig
> +++ b/drivers/uio/Kconfig
> @@ -46,6 +46,12 @@ config UIO_PDRV_GENIRQ
>  
>  	  If you don't know what to do here, say N.
>  
> +config UIO_OF_GENIRQ
> +	tristate "Userspace I/O OF driver with generic IRQ handling"
> +	depends on UIO_PDRV_GENIRQ && OF
> +	help
> +	  OF wrapper for the above platform driver.
> +
>  config UIO_SMX
>  	tristate "SMX cryptengine UIO interface"
>  	default n
> diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
> index 5c2586d..089fd56 100644
> --- a/drivers/uio/Makefile
> +++ b/drivers/uio/Makefile
> @@ -2,6 +2,7 @@ obj-$(CONFIG_UIO)	+= uio.o
>  obj-$(CONFIG_UIO_CIF)	+= uio_cif.o
>  obj-$(CONFIG_UIO_PDRV)	+= uio_pdrv.o
>  obj-$(CONFIG_UIO_PDRV_GENIRQ)	+= uio_pdrv_genirq.o
> +obj-$(CONFIG_UIO_OF_GENIRQ)	+= uio_of_genirq.o
>  obj-$(CONFIG_UIO_SMX)	+= uio_smx.o
>  obj-$(CONFIG_UIO_AEC)	+= uio_aec.o
>  obj-$(CONFIG_UIO_SERCOS3)	+= uio_sercos3.o
> diff --git a/drivers/uio/uio_of_genirq.c b/drivers/uio/uio_of_genirq.c
> new file mode 100644
> index 0000000..254ec5b
> --- /dev/null
> +++ b/drivers/uio/uio_of_genirq.c
> @@ -0,0 +1,98 @@
> +/*
> + * OF wrapper to make use of the uio_pdrv_genirq-driver.
> + *
> + * Copyright (C) 2009 Wolfram Sang, Pengutronix
> + *
> + * 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/kernel.h>
> +#include <linux/module.h>
> +#include <linux/uio_driver.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/uio_pdrv_genirq.h>
> +
> +#define OF_DRIVER_VERSION "1"
> +
> +static __devinit int uio_of_genirq_probe(struct of_device *op,
> +		const struct of_device_id *match)
> +{
> +	struct uio_info *uioinfo;
> +	struct resource resources[MAX_UIO_MAPS];
> +	int i, ret;
> +
> +	uioinfo = kzalloc(sizeof(*uioinfo), GFP_KERNEL);
> +	if (!uioinfo)
> +		return -ENOMEM;
> +
> +	uioinfo->name = op->node->name;
> +	uioinfo->version = OF_DRIVER_VERSION;
> +	uioinfo->irq = irq_of_parse_and_map(op->node, 0);
> +	if (!uioinfo->irq)
> +		uioinfo->irq = UIO_IRQ_NONE;

Please don't do this. It's inconsistent if all other UIO drivers require
people to use UIO_IRQ_NONE and you also allow zero. UIO_IRQ_NONE was
introduced because 0 may be a legal interrupt number on some platforms.

> +
> +	for (i = 0; i < MAX_UIO_MAPS; ++i)
> +		if (of_address_to_resource(op->node, i, &resources[i]))
> +			break;
> +
> +	ret = __uio_pdrv_genirq_probe(&op->dev, uioinfo, &resources, i);
> +	if (ret)
> +		goto err_cleanup;
> +
> +	return 0;
> +
> +err_cleanup:
> +	if (uioinfo->irq != UIO_IRQ_NONE)
> +		irq_dispose_mapping(uioinfo->irq);
> +
> +	kfree(uioinfo);
> +	return ret;
> +}
> +
> +static __devexit int uio_of_genirq_remove(struct of_device *op)
> +{
> +	struct uio_pdrv_genirq_platdata *priv = dev_get_drvdata(&op->dev);
> +
> +	uio_unregister_device(priv->uioinfo);
> +
> +	if (priv->uioinfo->irq != UIO_IRQ_NONE)
> +		irq_dispose_mapping(priv->uioinfo->irq);
> +
> +	kfree(priv->uioinfo);
> +	kfree(priv);
> +	return 0;
> +}
> +
> +/* Match table for of_platform binding */
> +static const struct of_device_id __devinitconst uio_of_genirq_match[] = {

checkpatch.pl complains about that. Please check.

> +	{ .compatible = "generic-uio", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, uio_of_genirq_match);
> +
> +static struct of_platform_driver uio_of_genirq_driver = {
> +	.owner = THIS_MODULE,
> +	.name = "uio-of-genirq",
> +	.match_table = uio_of_genirq_match,
> +	.probe = uio_of_genirq_probe,
> +	.remove = __devexit_p(uio_of_genirq_remove),
> +};
> +
> +static inline int __init uio_of_genirq_init(void)
> +{
> +	return of_register_platform_driver(&uio_of_genirq_driver);
> +}
> +module_init(uio_of_genirq_init);
> +
> +static inline void __exit uio_of_genirq_exit(void)
> +{
> +	of_unregister_platform_driver(&uio_of_genirq_driver);
> +}
> +module_exit(uio_of_genirq_exit);
> +
> +MODULE_AUTHOR("Wolfram Sang");
> +MODULE_DESCRIPTION("Userspace I/O OF driver with generic IRQ handling");
> +MODULE_LICENSE("GPL v2");
> -- 
> 1.6.3.1

Re: [PATCH 2/2] uio: add an of_genirq driver

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1123 bytes --]

Hello Hans,

> > +	uioinfo->irq = irq_of_parse_and_map(op->node, 0);
> > +	if (!uioinfo->irq)
> > +		uioinfo->irq = UIO_IRQ_NONE;
> 
> Please don't do this. It's inconsistent if all other UIO drivers require
> people to use UIO_IRQ_NONE and you also allow zero. UIO_IRQ_NONE was
> introduced because 0 may be a legal interrupt number on some platforms.

Yes, well, the '0' vs. 'NO_IRQ' thing is still not fully sorted out AFAIK. But
you are possibly right here, as long as irq_of_parse_and_map does return
NO_IRQ, I should explicitly check for it, like this:

	if (uioinfo->irq == NO_IRQ)
		uioinfo->irq = UIO_IRQ_NONE;

> > +/* Match table for of_platform binding */
> > +static const struct of_device_id __devinitconst uio_of_genirq_match[] = {
> 
> checkpatch.pl complains about that. Please check.

Did that, it is a false positive. See here:

http://lkml.indiana.edu/hypermail/linux/kernel/0906.1/02780.html

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 194 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-mnsaURCQ41sdnm+yROfE0A@public.gmane.org
https://ozlabs.org/mailman/listinfo/devicetree-discuss

Re: [PATCH 2/2] uio: add an of_genirq driver

[Hans J. Koch]

On Sun, Jun 14, 2009 at 07:14:06PM +0200, Wolfram Sang wrote:
> Hello Hans,
> 
> > > +	uioinfo->irq = irq_of_parse_and_map(op->node, 0);
> > > +	if (!uioinfo->irq)
> > > +		uioinfo->irq = UIO_IRQ_NONE;
> > 
> > Please don't do this. It's inconsistent if all other UIO drivers require
> > people to use UIO_IRQ_NONE and you also allow zero. UIO_IRQ_NONE was
> > introduced because 0 may be a legal interrupt number on some platforms.
> 
> Yes, well, the '0' vs. 'NO_IRQ' thing is still not fully sorted out AFAIK.

A "cat /proc/interrupts" on any common x86 PC shows you that IRQ 0 is used
there. OK, it's unlikely someone wants to write a UIO driver for that one,
but that might be different on other platforms.
Anyway, 0 is a valid IRQ number, so it cannot be used as "no irq".

> But you are possibly right here, as long as irq_of_parse_and_map does return
> NO_IRQ, I should explicitly check for it, like this:
> 
> 	if (uioinfo->irq == NO_IRQ)
> 		uioinfo->irq = UIO_IRQ_NONE;

Sorry for my ignorance, but what is NO_IRQ? If I do a

grep -r NO_IRQ include/

I get nothing.

> 
> > > +/* Match table for of_platform binding */
> > > +static const struct of_device_id __devinitconst uio_of_genirq_match[] = {
> > 
> > checkpatch.pl complains about that. Please check.
> 
> Did that, it is a false positive. See here:
> 
> http://lkml.indiana.edu/hypermail/linux/kernel/0906.1/02780.html

Well, you claim it's a false positive. So far, you did not get any responses,
AFAICS. I tend to agree with you, but I'd like to avoid patches that don't
pass checkpatch.pl, whatever the reason. Either the false positive gets
confirmed and fixed, or you should fix your patch.

Thanks,
Hans

> 
> Regards,
> 
>    Wolfram
> 
> -- 
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/2] uio: add an of_genirq driver

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1437 bytes --]


> Anyway, 0 is a valid IRQ number, so it cannot be used as "no irq".

May I point you to this thread?

http://lkml.org/lkml/2005/11/21/221

(The issue comes up once in a while as some archs still use NO_IRQ, some with
0 some with -1)

> > 	if (uioinfo->irq == NO_IRQ)
> > 		uioinfo->irq = UIO_IRQ_NONE;
> 
> Sorry for my ignorance, but what is NO_IRQ? If I do a
> 
> grep -r NO_IRQ include/
> 
> I get nothing.

Try a 'cd arch' before that :)

> Well, you claim it's a false positive. So far, you did not get any responses,
> AFAICS. I tend to agree with you, but I'd like to avoid patches that don't
> pass checkpatch.pl, whatever the reason. Either the false positive gets
> confirmed and fixed, or you should fix your patch.

Well, I assume that issues regarding checkpatch do not have the highest
priority (especially while the merge-window is open), which is understandable.
Fixing this bug (I take any bets that this is one ;)) might not be so trivial,
as modifying $Attributes can easily have lots of side-effects.

Now, all this does not matter much, as the objections Grant raised are valid
and there might be a totally different outcome to bind devices to UIO. But at
least, we have some code to discuss...

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 194 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-mnsaURCQ41sdnm+yROfE0A@public.gmane.org
https://ozlabs.org/mailman/listinfo/devicetree-discuss

Re: [PATCH 2/2] uio: add an of_genirq driver

[Hans J. Koch]

On Sun, Jun 14, 2009 at 09:05:33PM +0200, Wolfram Sang wrote:
> 
> > Anyway, 0 is a valid IRQ number, so it cannot be used as "no irq".
> 
> May I point you to this thread?
> 
> http://lkml.org/lkml/2005/11/21/221

Linus is just plain wrong in this 4 year old mail.

> 
> (The issue comes up once in a while as some archs still use NO_IRQ, some with
> 0 some with -1)
> 
> > > 	if (uioinfo->irq == NO_IRQ)
> > > 		uioinfo->irq = UIO_IRQ_NONE;
> > 
> > Sorry for my ignorance, but what is NO_IRQ? If I do a
> > 
> > grep -r NO_IRQ include/
> > 
> > I get nothing.
> 
> Try a 'cd arch' before that :)

no such luck in arch/x86/ ...

> 
> > Well, you claim it's a false positive. So far, you did not get any responses,
> > AFAICS. I tend to agree with you, but I'd like to avoid patches that don't
> > pass checkpatch.pl, whatever the reason. Either the false positive gets
> > confirmed and fixed, or you should fix your patch.
> 
> Well, I assume that issues regarding checkpatch do not have the highest
> priority (especially while the merge-window is open), which is understandable.
> Fixing this bug (I take any bets that this is one ;)) might not be so trivial,
> as modifying $Attributes can easily have lots of side-effects.
> 
> Now, all this does not matter much, as the objections Grant raised are valid
> and there might be a totally different outcome to bind devices to UIO. But at
> least, we have some code to discuss...

OK, I'm looking forward to your next version.

Thanks,
Hans


> 
> Regards,
> 
>    Wolfram
> 
> -- 
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/2] uio: add an of_genirq driver

[Wolfgang Grandegger]

Hans J. Koch wrote:
> On Sun, Jun 14, 2009 at 09:05:33PM +0200, Wolfram Sang wrote:
>>> Anyway, 0 is a valid IRQ number, so it cannot be used as "no irq".
>> May I point you to this thread?
>>
>> http://lkml.org/lkml/2005/11/21/221
> 
> Linus is just plain wrong in this 4 year old mail.

See also this related thread.

http://groups.google.com/group/rtc-linux/browse_thread/thread/9816648d5a8a1c9e/9968968188b5ab5a?lnk=gst&q=rx8025#9968968188b5ab5a

> 
>> (The issue comes up once in a while as some archs still use NO_IRQ, some with
>> 0 some with -1)
>>
>>>> 	if (uioinfo->irq == NO_IRQ)
>>>> 		uioinfo->irq = UIO_IRQ_NONE;
>>> Sorry for my ignorance, but what is NO_IRQ? If I do

It's 0 on PowerPC but ARM seems still to use -1.

http://lxr.linux.no/linux+v2.6.30/arch/powerpc/include/asm/irq.h#L29

For x86 it's not defined at all. But as this code is for the PowerPC,
where using NO_IRQ seems still to be OK.

Wolfgang.

Re: [PATCH 2/2] uio: add an of_genirq driver

[Hans J. Koch]

On Sun, Jun 14, 2009 at 09:36:53PM +0200, Wolfgang Grandegger wrote:
> >>>> 	if (uioinfo->irq == NO_IRQ)
> >>>> 		uioinfo->irq = UIO_IRQ_NONE;
> >>> Sorry for my ignorance, but what is NO_IRQ? If I do
> 
> It's 0 on PowerPC but ARM seems still to use -1.

Using 0 is simply wrong, especially if people do something like

if (!irq)
	return -ERROR;

IRQ number 0 _is_ a valid irq. Maybe not on all platforms, but in generic
code (like UIO) you have to assume it is.

> 
> http://lxr.linux.no/linux+v2.6.30/arch/powerpc/include/asm/irq.h#L29
> 
> For x86 it's not defined at all. But as this code is for the PowerPC,

No, it isn't. What makes you say that? The Kconfig entry doesn't depend
on PowerPC. I compiled it on x86...

> where using NO_IRQ seems still to be OK.

No. uio_pdrv_genirq can be used on all platforms, and not all platforms have
NO_IRQ. NO_IRQ can be used in platform specific code only.

Anyway, if someone fills in an invalid irq in his platform data, he deserves
to crash. No need for that test. UIO docs state that irq is a _required_
element. If you forget to set it, it will probably be 0. On most platforms,
register_irq will fail with that, and you'll notice. If you silently
replace it with UIO_IRQ_NONE, you simply cover up wrong code.

Thanks,
Hans

> 
> Wolfgang.

Re: [PATCH 2/2] uio: add an of_genirq driver

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1084 bytes --]

> > For x86 it's not defined at all. But as this code is for the PowerPC,
> 
> No, it isn't. What makes you say that? The Kconfig entry doesn't depend
> on PowerPC. I compiled it on x86...

It depends on OF. You could compile on x86? Have to check that...

> > where using NO_IRQ seems still to be OK.
> 
> No. uio_pdrv_genirq can be used on all platforms, and not all platforms have
> NO_IRQ. NO_IRQ can be used in platform specific code only.

Well, the wrapper uses irq_of_parse_and_map(). So far, it returns NO_IRQ if an
IRQ was not specified (or not found). I could check if there was an
interrupt-property at all, so I can distinguish between 'not specified' and
'not found'. Then, UIO_IRQ_NONE would only be used, if there was none
specified. Otherwise it will always be the result from irq_of_parse_and_map(),
whatever that is (even NO_IRQ). Is this what you have in mind?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 2/2] uio: add an of_genirq driver

[Hans J. Koch]

On Mon, Jun 15, 2009 at 12:00:22AM +0200, Wolfram Sang wrote:
> > > For x86 it's not defined at all. But as this code is for the PowerPC,
> > 
> > No, it isn't. What makes you say that? The Kconfig entry doesn't depend
> > on PowerPC. I compiled it on x86...
> 
> It depends on OF. You could compile on x86? Have to check that...

Ooops, forget it. It cannot be selected on x86. I was a bit distracted when
I wrote that, sorry.

> 
> > > where using NO_IRQ seems still to be OK.
> > 
> > No. uio_pdrv_genirq can be used on all platforms, and not all platforms have
> > NO_IRQ. NO_IRQ can be used in platform specific code only.
> 
> Well, the wrapper uses irq_of_parse_and_map(). So far, it returns NO_IRQ if an
> IRQ was not specified (or not found). I could check if there was an
> interrupt-property at all, so I can distinguish between 'not specified' and
> 'not found'. Then, UIO_IRQ_NONE would only be used, if there was none
> specified. Otherwise it will always be the result from irq_of_parse_and_map(),
> whatever that is (even NO_IRQ). Is this what you have in mind?

I would find it better if probe() failed if no irq is specified, printing a
message that tells the user to setup his data correctly before loading the
driver. A user _has_ to setup irq, if there is none, he still has to set
irq=UIO_IRQ_NONE. For that matter, 'not specified' and 'not found' is both
the same bad thing.

Thanks,
Hans

> 
> Regards,
> 
>    Wolfram
> 
> -- 
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/2] uio: add an of_genirq driver

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1306 bytes --]


> driver. A user _has_ to setup irq, if there is none, he still has to set
> irq=UIO_IRQ_NONE. For that matter, 'not specified' and 'not found' is both
> the same bad thing.

Hmm, what should I do?

A typical interrupts-property in a device-tree is specified as:

	interrupts = <&irq_controller_node irq_number irq_sense>;

Something like UIO_IRQ_NONE does not fit into this scheme, even more as it is
Linux-specific and device trees need to be OS independant.

I'm pretty sure the correct way to state that you don't need an interrupt in
the device-tree is to simply not specify the above interrupt property.

Well, yes, that means you can't distinguish between 'forgotten' and
'intentionally left out'. I wonder if it is really that bad? If something does
not work (= one is missing interrupts), the first place to look at is the
device tree. If one does not see an interrupt-property, voila, problem solved.

(Note that with my latest suggestion, a _wrong_ interrupt is handled the same
way as with platform_data. request_irq() should equally fail if the
return-value from irq_of_parse_and_map() is simply forwarded.)

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 194 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-mnsaURCQ41sdnm+yROfE0A@public.gmane.org
https://ozlabs.org/mailman/listinfo/devicetree-discuss

Re: [PATCH 2/2] uio: add an of_genirq driver

[Hans J. Koch]

On Mon, Jun 15, 2009 at 01:46:43AM +0200, Wolfram Sang wrote:
> 
> > driver. A user _has_ to setup irq, if there is none, he still has to set
> > irq=UIO_IRQ_NONE. For that matter, 'not specified' and 'not found' is both
> > the same bad thing.
> 
> Hmm, what should I do?
> 
> A typical interrupts-property in a device-tree is specified as:
> 
> 	interrupts = <&irq_controller_node irq_number irq_sense>;
> 
> Something like UIO_IRQ_NONE does not fit into this scheme, even more as it is
> Linux-specific and device trees need to be OS independant.
> 
> I'm pretty sure the correct way to state that you don't need an interrupt in
> the device-tree is to simply not specify the above interrupt property.
> 
> Well, yes, that means you can't distinguish between 'forgotten' and
> 'intentionally left out'. I wonder if it is really that bad? If something does
> not work (= one is missing interrupts), the first place to look at is the
> device tree. If one does not see an interrupt-property, voila, problem solved.
> 
> (Note that with my latest suggestion, a _wrong_ interrupt is handled the same
> way as with platform_data. request_irq() should equally fail if the
> return-value from irq_of_parse_and_map() is simply forwarded.)

I agree. And assuming Alan is right, forget what I said about IRQ 0 being a
valid interrupt number.

Thanks,
Hans

> 
> -- 
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/2] uio: add an of_genirq driver

[Greg KH]

On Sun, Jun 14, 2009 at 09:05:33PM +0200, Wolfram Sang wrote:
> Well, I assume that issues regarding checkpatch do not have the highest
> priority (especially while the merge-window is open), which is understandable.

Hm, the "merge-window" for new stuff like these patches is pretty much
already closed, as you didn't send them _before_ the merge window opened
up.  You need to get them to us sooner, so we can test stuff out in the
-next tree for a while before we can merge them.

thanks,

greg k-h

Re: [PATCH 2/2] uio: add an of_genirq driver

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1107 bytes --]

On Sun, Jun 14, 2009 at 12:27:07PM -0700, Greg KH wrote:
> On Sun, Jun 14, 2009 at 09:05:33PM +0200, Wolfram Sang wrote:
> > Well, I assume that issues regarding checkpatch do not have the highest
> > priority (especially while the merge-window is open), which is understandable.
> 
> Hm, the "merge-window" for new stuff like these patches is pretty much
> already closed, as you didn't send them _before_ the merge window opened
> up.  You need to get them to us sooner, so we can test stuff out in the
> -next tree for a while before we can merge them.

Seems you got me wrong here :)

As I stated in the introduction ("PATCH [0/2]"), this patch series is _not_
meant for the current merge-window. I just happened to be done with it now.

With the above sentence I just wanted to give a hint, why there was not a reply
to my checkpatch-mail (as Hans seemed to be concerned about that there was
none).

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 194 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-mnsaURCQ41sdnm+yROfE0A@public.gmane.org
https://ozlabs.org/mailman/listinfo/devicetree-discuss

Re: [PATCH 2/2] uio: add an of_genirq driver

[Alan Cox]

> > +	if (!uioinfo->irq)
> > +		uioinfo->irq = UIO_IRQ_NONE;
> 
> Please don't do this. It's inconsistent if all other UIO drivers require
> people to use UIO_IRQ_NONE and you also allow zero. UIO_IRQ_NONE was
> introduced because 0 may be a legal interrupt number on some platforms.

Zero is not a valid IRQ number in the kernel (except in arch specific
depths). IRQ numbers are also *unsigned* so -1 isn't a safe definition.

Zero means no IRQ. If any old UIO code is assuming otherwise it wants
fixing.

It is the job of the platform to map a physical IRQ 0 to some other
representation if it exists outside of arch specific code. This was
decided some years ago and a large part of the kernel simply doesn't
support any notion of a real IRQ 0.

Alan

Re: [PATCH 2/2] uio: add an of_genirq driver

[Hans J. Koch]

On Mon, Jun 15, 2009 at 12:12:07AM +0100, Alan Cox wrote:
> > > +	if (!uioinfo->irq)
> > > +		uioinfo->irq = UIO_IRQ_NONE;
> > 
> > Please don't do this. It's inconsistent if all other UIO drivers require
> > people to use UIO_IRQ_NONE and you also allow zero. UIO_IRQ_NONE was
> > introduced because 0 may be a legal interrupt number on some platforms.
> 
> Zero is not a valid IRQ number in the kernel (except in arch specific
> depths). IRQ numbers are also *unsigned* so -1 isn't a safe definition.

The above uioinfo->irq is a signed long.

> 
> Zero means no IRQ. If any old UIO code is assuming otherwise it wants
> fixing.

It doesn't. It won't complain about IRQ 0 and will pass it on to
request_irq, which will probably fail if it is as you say.

I did it that way because I saw IRQ 0 in /proc/interrupts on every PC...

> 
> It is the job of the platform to map a physical IRQ 0 to some other
> representation if it exists outside of arch specific code.

Funny.

> This was
> decided some years ago and a large part of the kernel simply doesn't
> support any notion of a real IRQ 0.

Can you tell me the reason for that decision or point me to some ml archive?

Thanks,
Hans

> 
> Alan

Re: [PATCH 2/2] uio: add an of_genirq driver

[Alan Cox]

> I did it that way because I saw IRQ 0 in /proc/interrupts on every PC...
> 
> > 
> > It is the job of the platform to map a physical IRQ 0 to some other
> > representation if it exists outside of arch specific code.
> 
> Funny.
> 
> > This was
> > decided some years ago and a large part of the kernel simply doesn't
> > support any notion of a real IRQ 0.
> 
> Can you tell me the reason for that decision or point me to some ml archive?

The natural C way to write "No xxx" is if (!xxx) hence

	if (!dev->irq) {
		polling_start();
		return 0;
	}

The PC "IRQ 0" is the timer - which only appears in the arch code.

Alan

Re: [PATCH 2/2] uio: add an of_genirq driver

[Benjamin Herrenschmidt]

On Mon, 2009-06-15 at 00:12 +0100, Alan Cox wrote:
> > > +	if (!uioinfo->irq)
> > > +		uioinfo->irq = UIO_IRQ_NONE;
> > 
> > Please don't do this. It's inconsistent if all other UIO drivers require
> > people to use UIO_IRQ_NONE and you also allow zero. UIO_IRQ_NONE was
> > introduced because 0 may be a legal interrupt number on some platforms.
> 
> Zero is not a valid IRQ number in the kernel (except in arch specific
> depths). IRQ numbers are also *unsigned* so -1 isn't a safe definition.
> 
> Zero means no IRQ. If any old UIO code is assuming otherwise it wants
> fixing.
> 
> It is the job of the platform to map a physical IRQ 0 to some other
> representation if it exists outside of arch specific code. This was
> decided some years ago and a large part of the kernel simply doesn't
> support any notion of a real IRQ 0.

Right, and powerpc complies with that rule, so 0 is fine for us.

Cheers,
Ben.

Re: [PATCH 2/2] uio: add an of_genirq driver

[Grant Likely]

On Thu, Jun 11, 2009 at 6:04 PM, Wolfram Sang<w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> Picking up the now exported generic probe function from the
> platform-variant of this driver, this patch adds an of-version. Also add
> the binding documentation.
>
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Magnus Damm <magnus.damm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Hans J. Koch <hjk-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> Cc: Greg KH <gregkh-l3A5Bk7waGM@public.gmane.org>
> ---
>
> In probe, I put the resources-array on the stack to simplify the code. If this
> is considered too huge for the stack (140 byte for a 32-bit system at the
> moment), I can also post a version using kzalloc.
>
>  Documentation/powerpc/dts-bindings/uio-generic.txt |   16 +++
>  drivers/uio/Kconfig                                |    6 +
>  drivers/uio/Makefile                               |    1 +
>  drivers/uio/uio_of_genirq.c                        |   98 ++++++++++++++++++++
>  4 files changed, 121 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/powerpc/dts-bindings/uio-generic.txt
>  create mode 100644 drivers/uio/uio_of_genirq.c
>
> diff --git a/Documentation/powerpc/dts-bindings/uio-generic.txt b/Documentation/powerpc/dts-bindings/uio-generic.txt
> new file mode 100644
> index 0000000..8ad9861
> --- /dev/null
> +++ b/Documentation/powerpc/dts-bindings/uio-generic.txt
> @@ -0,0 +1,16 @@
> +UIO for custom devices
> +
> +A device which will be mapped using the UIO subsystem.
> +
> +Properties:
> + - compatible : should contain the specific model used, followed by
> +                "generic-uio".
> + - reg : address range(s) of the device (up to MAX_UIO_MAPS)
> + - interrupts : interrupt of the device
> +
> +Example:
> +        c64fpga@0 {
> +                compatible = "ptx,c64fpga001", "generic-uio";
> +                reg = <0x0 0x10000>;
> +                interrupts = <0 0 3>;
> +        };

Hmmm, I'm not happy about this.  The device tree describes the
hardware, not the way Linux uses the hardware.  UIO definitely falls
into the category of Linux implementation detail.

This should be approached from the other way around.  Either the
generic-uio of_platform driver should contain an explicit list of
devices to be handled by UIO, or the OF infrastructure should be
modified to allow things like force binding of_devices to of_drivers
at runtime.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 2/2] uio: add an of_genirq driver

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 2107 bytes --]


> > diff --git a/Documentation/powerpc/dts-bindings/uio-generic.txt b/Documentation/powerpc/dts-bindings/uio-generic.txt
> > new file mode 100644
> > index 0000000..8ad9861
> > --- /dev/null
> > +++ b/Documentation/powerpc/dts-bindings/uio-generic.txt
> > @@ -0,0 +1,16 @@
> > +UIO for custom devices
> > +
> > +A device which will be mapped using the UIO subsystem.
> > +
> > +Properties:
> > + - compatible : should contain the specific model used, followed by
> > +                "generic-uio".
> > + - reg : address range(s) of the device (up to MAX_UIO_MAPS)
> > + - interrupts : interrupt of the device
> > +
> > +Example:
> > +        c64fpga@0 {
> > +                compatible = "ptx,c64fpga001", "generic-uio";
> > +                reg = <0x0 0x10000>;
> > +                interrupts = <0 0 3>;
> > +        };
> 
> Hmmm, I'm not happy about this.  The device tree describes the
> hardware, not the way Linux uses the hardware.  UIO definitely falls
> into the category of Linux implementation detail.

Yes, I am aware of that. I just started with the mechanisms which are available
today and hoped we could find some compatible-value which will suit all needs.

> This should be approached from the other way around.  Either the
> generic-uio of_platform driver should contain an explicit list of
> devices to be handled by UIO,

Well, that could lead to a quite huge match_table over time.

> or the OF infrastructure should be modified to allow things like force
> binding of_devices to of_drivers at runtime.

That is an interesting idea. I could imagine something like a 'new_compatible"
entry in the sysfs-section of the driver similar to 'new_id' for PCI. After
writing a new compatible-string into it, matching will triggered again with the
new entry added. That could (should?) also be placed at the of-core-level. Or
did you have something else in mind?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 194 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-mnsaURCQ41sdnm+yROfE0A@public.gmane.org
https://ozlabs.org/mailman/listinfo/devicetree-discuss

Re: [PATCH 2/2] uio: add an of_genirq driver

[Grant Likely]

On Tue, Jun 16, 2009 at 3:04 AM, Wolfram Sang<w.sang@pengutronix.de> wrote:
>
>> > diff --git a/Documentation/powerpc/dts-bindings/uio-generic.txt b/Documentation/powerpc/dts-bindings/uio-generic.txt
>> > new file mode 100644
>> > index 0000000..8ad9861
>> > --- /dev/null
>> > +++ b/Documentation/powerpc/dts-bindings/uio-generic.txt
>> > @@ -0,0 +1,16 @@
>> > +UIO for custom devices
>> > +
>> > +A device which will be mapped using the UIO subsystem.
>> > +
>> > +Properties:
>> > + - compatible : should contain the specific model used, followed by
>> > +                "generic-uio".
>> > + - reg : address range(s) of the device (up to MAX_UIO_MAPS)
>> > + - interrupts : interrupt of the device
>> > +
>> > +Example:
>> > +        c64fpga@0 {
>> > +                compatible = "ptx,c64fpga001", "generic-uio";
>> > +                reg = <0x0 0x10000>;
>> > +                interrupts = <0 0 3>;
>> > +        };
>>
>> Hmmm, I'm not happy about this.  The device tree describes the
>> hardware, not the way Linux uses the hardware.  UIO definitely falls
>> into the category of Linux implementation detail.
>
> Yes, I am aware of that. I just started with the mechanisms which are available
> today and hoped we could find some compatible-value which will suit all needs.

Trouble is a value that suits all needs today probably won't a year
from now.  :-)

>> This should be approached from the other way around.  Either the
>> generic-uio of_platform driver should contain an explicit list of
>> devices to be handled by UIO,
>
> Well, that could lead to a quite huge match_table over time.
>
>> or the OF infrastructure should be modified to allow things like force
>> binding of_devices to of_drivers at runtime.
>
> That is an interesting idea. I could imagine something like a 'new_compatible"
> entry in the sysfs-section of the driver similar to 'new_id' for PCI. After
> writing a new compatible-string into it, matching will triggered again with the
> new entry added. That could (should?) also be placed at the of-core-level. Or
> did you have something else in mind?

Yeah, that sounds appropriate.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.