[PATCH v2] uio/pdrv_genirq: Add OF support

[PATCH v2] uio/pdrv_genirq: Add OF support

[Michal Simek]

Support OF support. "generic-uio" compatible property is used.

Signed-off-by: Michal Simek <monstr@monstr.eu>

---
v2: Remove additional resource binding
    Setup correct version string
    Clear compatible string
---
 drivers/uio/uio_pdrv_genirq.c |   45 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index 7174d51..fd84a93 100644
--- a/drivers/uio/uio_pdrv_genirq.c
+++ b/drivers/uio/uio_pdrv_genirq.c
@@ -23,6 +23,10 @@
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
 
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+
 #define DRIVER_NAME "uio_pdrv_genirq"
 
 struct uio_pdrv_genirq_platdata {
@@ -97,6 +101,28 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
 	int ret = -EINVAL;
 	int i;
 
+	if (!uioinfo) {
+		/* alloc uioinfo for one device */
+		uioinfo = kzalloc(sizeof(*uioinfo), GFP_KERNEL);
+		if (!uioinfo) {
+			ret = -ENOMEM;
+			dev_err(&pdev->dev, "unable to kmalloc\n");
+			goto bad2;
+		}
+		uioinfo->name = pdev->dev.of_node->name;
+		uioinfo->version = "dt";
+
+		/* Multiple IRQs are not supported */
+		if (pdev->num_resources > 1) {
+			struct resource *r = &pdev->resource[1];
+			uioinfo->irq = r->start;
+			dev_info(&pdev->dev, "irq %d\n", (u32)uioinfo->irq);
+		} else {
+			uioinfo->irq = UIO_IRQ_NONE;
+			dev_info(&pdev->dev, "no IRQ found\n");
+		}
+	}
+
 	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
 		dev_err(&pdev->dev, "missing platform_data\n");
 		goto bad0;
@@ -180,6 +206,10 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
 	kfree(priv);
 	pm_runtime_disable(&pdev->dev);
  bad0:
+	/* kfree uioinfo for CONFIG_OF */
+	if (!pdev->dev.platform_data)
+		kfree(uioinfo);
+ bad2:
 	return ret;
 }
 
@@ -215,6 +245,20 @@ static const struct dev_pm_ops uio_pdrv_genirq_dev_pm_ops = {
 	.runtime_resume = uio_pdrv_genirq_runtime_nop,
 };
 
+#ifdef CONFIG_OF
+/*
+ * Empty match table for of_platform binding
+ * Select your custom compatible string in format
+ * { .compatible = "<compatible string>", },
+ */
+static const struct of_device_id __devinitconst uio_of_genirq_match[] = {
+	{ /* end of list */ },
+};
+MODULE_DEVICE_TABLE(of, uio_of_genirq_match);
+#else
+# define uio_of_genirq_match NULL
+#endif
+
 static struct platform_driver uio_pdrv_genirq = {
 	.probe = uio_pdrv_genirq_probe,
 	.remove = uio_pdrv_genirq_remove,
@@ -222,6 +266,7 @@ static struct platform_driver uio_pdrv_genirq = {
 		.name = DRIVER_NAME,
 		.owner = THIS_MODULE,
 		.pm = &uio_pdrv_genirq_dev_pm_ops,
+		.of_match_table = uio_of_genirq_match,
 	},
 };
 
-- 
1.5.5.6

Re: [PATCH v2] uio/pdrv_genirq: Add OF support

[Wolfram Sang]

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

Hi Michal,

On Thu, Apr 14, 2011 at 11:41:46AM +0200, Michal Simek wrote:
> Support OF support. "generic-uio" compatible property is used.

This description is not true anymore. Please also add a short paragrpah how it
is intended to be used now.

> Signed-off-by: Michal Simek <monstr@monstr.eu>

> +		uioinfo->version = "dt";

Minor nit: "devicetree" is probably more descriptive.

> +
> +		/* Multiple IRQs are not supported */
> +		if (pdev->num_resources > 1) {
> +			struct resource *r = &pdev->resource[1];

Are you sure the irq-ressource is always [1]? (Similar question for the
if()-block above). Try platform_get_irq().

> +#ifdef CONFIG_OF
> +/*
> + * Empty match table for of_platform binding

While it probably doesn't make change to put every supported device in
upstream, it still deosn't technically have to be empty. So, mabye drop this
comment and add something like "/* empty for now */" to the table?

Thanks,

   Wolfram

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

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

Re: [PATCH v2] uio/pdrv_genirq: Add OF support

[Michal Simek]

Hi Wolfram,

Wolfram Sang wrote:
> Hi Michal,
> 
> On Thu, Apr 14, 2011 at 11:41:46AM +0200, Michal Simek wrote:
>> Support OF support. "generic-uio" compatible property is used.
> 
> This description is not true anymore. Please also add a short paragrpah how it
> is intended to be used now.

ok.

> 
>> Signed-off-by: Michal Simek <monstr@monstr.eu>
> 
>> +		uioinfo->version = "dt";
> 
> Minor nit: "devicetree" is probably more descriptive.

no problem to change it

> 
>> +
>> +		/* Multiple IRQs are not supported */
>> +		if (pdev->num_resources > 1) {
>> +			struct resource *r = &pdev->resource[1];
> 
> Are you sure the irq-ressource is always [1]?  (Similar question for the
> if()-block above). Try platform_get_irq().

I wasn't aware about platform_get_irq. You are right.

What "if()-block above" are you talking about?
Above is kzalloc uioinfo allocation.
I am going to send v3 that's why please comment this there.

> 
>> +#ifdef CONFIG_OF
>> +/*
>> + * Empty match table for of_platform binding
> 
> While it probably doesn't make change to put every supported device in
> upstream, it still deosn't technically have to be empty. So, mabye drop this
> comment and add something like "/* empty for now */" to the table?

ok, done.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian

Re: [PATCH v2] uio/pdrv_genirq: Add OF support

[Wolfram Sang]

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

> >>+
> >>+		/* Multiple IRQs are not supported */
> >>+		if (pdev->num_resources > 1) {
> >>+			struct resource *r = &pdev->resource[1];
> >
> >Are you sure the irq-ressource is always [1]?  (Similar question for the
> >if()-block above). Try platform_get_irq().
> 
> I wasn't aware about platform_get_irq. You are right.
> 
> What "if()-block above" are you talking about?

The one I quoted: if (pdev->num_resources > 1)

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2] uio/pdrv_genirq: Add OF support

[Hans J. Koch]

On Thu, Apr 14, 2011 at 11:41:46AM +0200, Michal Simek wrote:
> Support OF support. "generic-uio" compatible property is used.
> 
> Signed-off-by: Michal Simek <monstr@monstr.eu>
> 
> ---
> v2: Remove additional resource binding
>     Setup correct version string
>     Clear compatible string
> ---
>  drivers/uio/uio_pdrv_genirq.c |   45 +++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 45 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
> index 7174d51..fd84a93 100644
> --- a/drivers/uio/uio_pdrv_genirq.c
> +++ b/drivers/uio/uio_pdrv_genirq.c
> @@ -23,6 +23,10 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/slab.h>
>  
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +
>  #define DRIVER_NAME "uio_pdrv_genirq"
>  
>  struct uio_pdrv_genirq_platdata {
> @@ -97,6 +101,28 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
>  	int ret = -EINVAL;
>  	int i;
>  
> +	if (!uioinfo) {
> +		/* alloc uioinfo for one device */
> +		uioinfo = kzalloc(sizeof(*uioinfo), GFP_KERNEL);
> +		if (!uioinfo) {
> +			ret = -ENOMEM;
> +			dev_err(&pdev->dev, "unable to kmalloc\n");
> +			goto bad2;
> +		}
> +		uioinfo->name = pdev->dev.of_node->name;
> +		uioinfo->version = "dt";

Can that be something more descriptive?

> +
> +		/* Multiple IRQs are not supported */

But multiple mappings are, and also no mappings.

> +		if (pdev->num_resources > 1) {
> +			struct resource *r = &pdev->resource[1];
> +			uioinfo->irq = r->start;

Why has the irq be resource[1] ?

> +			dev_info(&pdev->dev, "irq %d\n", (u32)uioinfo->irq);
> +		} else {
> +			uioinfo->irq = UIO_IRQ_NONE;
> +			dev_info(&pdev->dev, "no IRQ found\n");
> +		}
> +	}
> +
>  	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
>  		dev_err(&pdev->dev, "missing platform_data\n");
>  		goto bad0;
> @@ -180,6 +206,10 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
>  	kfree(priv);
>  	pm_runtime_disable(&pdev->dev);
>   bad0:
> +	/* kfree uioinfo for CONFIG_OF */
> +	if (!pdev->dev.platform_data)
> +		kfree(uioinfo);
> + bad2:
>  	return ret;
>  }
>  
> @@ -215,6 +245,20 @@ static const struct dev_pm_ops uio_pdrv_genirq_dev_pm_ops = {
>  	.runtime_resume = uio_pdrv_genirq_runtime_nop,
>  };
>  
> +#ifdef CONFIG_OF
> +/*
> + * Empty match table for of_platform binding
> + * Select your custom compatible string in format
> + * { .compatible = "<compatible string>", },
> + */
> +static const struct of_device_id __devinitconst uio_of_genirq_match[] = {
> +	{ /* end of list */ },

doesn't that want a NULL termination?

> +};
> +MODULE_DEVICE_TABLE(of, uio_of_genirq_match);
> +#else
> +# define uio_of_genirq_match NULL
> +#endif
> +
>  static struct platform_driver uio_pdrv_genirq = {
>  	.probe = uio_pdrv_genirq_probe,
>  	.remove = uio_pdrv_genirq_remove,
> @@ -222,6 +266,7 @@ static struct platform_driver uio_pdrv_genirq = {
>  		.name = DRIVER_NAME,
>  		.owner = THIS_MODULE,
>  		.pm = &uio_pdrv_genirq_dev_pm_ops,
> +		.of_match_table = uio_of_genirq_match,
>  	},
>  };
>  
> -- 
> 1.5.5.6
> 
> 

Re: [PATCH v2] uio/pdrv_genirq: Add OF support

[Arnd Bergmann]

On Friday 15 April 2011, Hans J. Koch wrote:
> > +#ifdef CONFIG_OF
> > +/*
> > + * Empty match table for of_platform binding
> > + * Select your custom compatible string in format
> > + * { .compatible = "<compatible string>", },
> > + */
> > +static const struct of_device_id __devinitconst uio_of_genirq_match[] = {
> > +     { /* end of list */ },
> 
> doesn't that want a NULL termination?
> 

It is NULL terminated, that is the /* end of list */.

	Arnd

Re: [PATCH v2] uio/pdrv_genirq: Add OF support

[Hans J. Koch]

On Sun, Apr 17, 2011 at 07:53:42PM +0200, Arnd Bergmann wrote:
> On Friday 15 April 2011, Hans J. Koch wrote:
> > > +#ifdef CONFIG_OF
> > > +/*
> > > + * Empty match table for of_platform binding
> > > + * Select your custom compatible string in format
> > > + * { .compatible = "<compatible string>", },
> > > + */
> > > +static const struct of_device_id __devinitconst uio_of_genirq_match[] = {
> > > +     { /* end of list */ },
> > 
> > doesn't that want a NULL termination?
> > 
> 
> It is NULL terminated, that is the /* end of list */.

Right.

Thanks,
Hans

Re: [PATCH v2] uio/pdrv_genirq: Add OF support

[Michal Simek]

Hans J. Koch wrote:
> On Thu, Apr 14, 2011 at 11:41:46AM +0200, Michal Simek wrote:
>> Support OF support. "generic-uio" compatible property is used.
>>
>> Signed-off-by: Michal Simek <monstr@monstr.eu>
>>
>> ---
>> v2: Remove additional resource binding
>>     Setup correct version string
>>     Clear compatible string
>> ---
>>  drivers/uio/uio_pdrv_genirq.c |   45 +++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 45 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
>> index 7174d51..fd84a93 100644
>> --- a/drivers/uio/uio_pdrv_genirq.c
>> +++ b/drivers/uio/uio_pdrv_genirq.c
>> @@ -23,6 +23,10 @@
>>  #include <linux/pm_runtime.h>
>>  #include <linux/slab.h>
>>  
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/of_address.h>
>> +
>>  #define DRIVER_NAME "uio_pdrv_genirq"
>>  
>>  struct uio_pdrv_genirq_platdata {
>> @@ -97,6 +101,28 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
>>  	int ret = -EINVAL;
>>  	int i;
>>  
>> +	if (!uioinfo) {
>> +		/* alloc uioinfo for one device */
>> +		uioinfo = kzalloc(sizeof(*uioinfo), GFP_KERNEL);
>> +		if (!uioinfo) {
>> +			ret = -ENOMEM;
>> +			dev_err(&pdev->dev, "unable to kmalloc\n");
>> +			goto bad2;
>> +		}
>> +		uioinfo->name = pdev->dev.of_node->name;
>> +		uioinfo->version = "dt";
> 
> Can that be something more descriptive?

look at my previous post.

> 
>> +
>> +		/* Multiple IRQs are not supported */
> 
> But multiple mappings are, and also no mappings.

I have tested multiple mappings and it is no problem to use it.
No mapping is also fine.

> 
>> +		if (pdev->num_resources > 1) {
>> +			struct resource *r = &pdev->resource[1];
>> +			uioinfo->irq = r->start;
> 
> Why has the irq be resource[1] ?

look at my previous post.

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian