UIO OF support

UIO OF support

[Michal Simek]

Hi,

here is OF support for UIO. I tried to do minimal changes.
John Williams has warned me that there was any previous attempt to add this OF support 
to UIO.

Grant: Can you please review it? I wasn't sure about one part. I think you will see it 
and will comment it if there is something stupid.

I look forward for your comments and discuss about it.

Thanks,
Michal

[PATCH] uio/pdrv_genirq: Add OF support

[Michal Simek]

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

Signed-off-by: Michal Simek <monstr-pSz03upnqPeHXe+LvDLADg@public.gmane.org>
---
 drivers/uio/uio_pdrv_genirq.c |   60 ++++++++++++++++++++++++++++++++++++++--
 1 files changed, 57 insertions(+), 3 deletions(-)

diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index 7174d51..9e89806 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 {
@@ -92,11 +96,44 @@ static int uio_pdrv_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on)
 static int uio_pdrv_genirq_probe(struct platform_device *pdev)
 {
 	struct uio_info *uioinfo = pdev->dev.platform_data;
-	struct uio_pdrv_genirq_platdata *priv;
+	struct uio_pdrv_genirq_platdata *priv = NULL;
 	struct uio_mem *uiomem;
 	int ret = -EINVAL;
 	int i;
 
+	if (!uioinfo) {
+		struct resource r_irq; /* Interrupt resources */
+		int rc = 0;
+
+		rc = of_address_to_resource(pdev->dev.of_node, 0,
+							&pdev->resource[0]);
+		if (rc) {
+			dev_err(&pdev->dev, "invalid address\n");
+			goto bad2;
+		}
+		pdev->num_resources = 1;
+
+		/* 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;
+		/* Use version for storing full IP name for identification */
+		uioinfo->version = pdev->dev.of_node->full_name;
+
+		/* Get IRQ for the device */
+		rc = of_irq_to_resource(pdev->dev.of_node, 0, &r_irq);
+		if (rc == NO_IRQ)
+			dev_err(&pdev->dev, "no IRQ found\n");
+		else {
+			uioinfo->irq = r_irq.start;
+			dev_info(&pdev->dev, "irq %d\n", (u32)uioinfo->irq);
+		}
+	}
+
 	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
 		dev_err(&pdev->dev, "missing platform_data\n");
 		goto bad0;
@@ -176,10 +213,15 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, priv);
 	return 0;
- bad1:
+
+bad1:
 	kfree(priv);
 	pm_runtime_disable(&pdev->dev);
- bad0:
+bad0:
+	/* kfree uioinfo for CONFIG_OF */
+	if (!pdev->dev.platform_data)
+		kfree(uioinfo);
+bad2:
 	return ret;
 }
 
@@ -215,6 +257,17 @@ static const struct dev_pm_ops uio_pdrv_genirq_dev_pm_ops = {
 	.runtime_resume = uio_pdrv_genirq_runtime_nop,
 };
 
+#ifdef CONFIG_OF
+/* Match table for of_platform binding */
+static const struct of_device_id __devinitconst uio_of_genirq_match[] = {
+	{ .compatible = "generic-uio", },
+	{ /* 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 +275,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] uio/pdrv_genirq: Add OF support

[Wolfram Sang]

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

On Thu, Mar 31, 2011 at 02:30:00PM +0200, Michal Simek wrote:
> Support OF support. "generic-uio" compatible property is used.

And exactly this was the issue last time (when I tried). This is a
generic property, which is linux-specific and not describing HW. The
agreement back then was to we probably need to add compatible-entries at
runtime (something like new_id for USB). So the uio-of-driver could be
matched against any device. Otherwise, we would collect a lot of
potential entries like "vendor,special-card1". Although I wonder
meanwhile if it is really going to be that bad; we don't have so much
UIO-driver in tree as well. Maybe worth a try?

> Signed-off-by: Michal Simek <monstr-pSz03upnqPeHXe+LvDLADg@public.gmane.org>
> ---
>  drivers/uio/uio_pdrv_genirq.c |   60 ++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 57 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
> index 7174d51..9e89806 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 {
> @@ -92,11 +96,44 @@ static int uio_pdrv_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on)
>  static int uio_pdrv_genirq_probe(struct platform_device *pdev)
>  {
>  	struct uio_info *uioinfo = pdev->dev.platform_data;
> -	struct uio_pdrv_genirq_platdata *priv;
> +	struct uio_pdrv_genirq_platdata *priv = NULL;

unrelated?

>  	struct uio_mem *uiomem;
>  	int ret = -EINVAL;
>  	int i;
>  
> +	if (!uioinfo) {
> +		struct resource r_irq; /* Interrupt resources */
> +		int rc = 0;
> +
> +		rc = of_address_to_resource(pdev->dev.of_node, 0,
> +							&pdev->resource[0]);
> +		if (rc) {
> +			dev_err(&pdev->dev, "invalid address\n");
> +			goto bad2;
> +		}
> +		pdev->num_resources = 1;
> +
> +		/* 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;
> +		/* Use version for storing full IP name for identification */
> +		uioinfo->version = pdev->dev.of_node->full_name;

I don't think this is apropriate, but will leave that to Hans.

> +		/* Get IRQ for the device */
> +		rc = of_irq_to_resource(pdev->dev.of_node, 0, &r_irq);
> +		if (rc == NO_IRQ)
> +			dev_err(&pdev->dev, "no IRQ found\n");

No error, I think. Sometimes just mmaping the registers is enough.

> +		else {
> +			uioinfo->irq = r_irq.start;
> +			dev_info(&pdev->dev, "irq %d\n", (u32)uioinfo->irq);
> +		}
> +	}
> +
>  	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
>  		dev_err(&pdev->dev, "missing platform_data\n");
>  		goto bad0;
> @@ -176,10 +213,15 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, priv);
>  	return 0;
> - bad1:
> +
> +bad1:

The spaces before labels are intentional, better keep them.

>  	kfree(priv);
>  	pm_runtime_disable(&pdev->dev);
> - bad0:
> +bad0:
> +	/* kfree uioinfo for CONFIG_OF */
> +	if (!pdev->dev.platform_data)
> +		kfree(uioinfo);
> +bad2:
>  	return ret;
>  }
>  
> @@ -215,6 +257,17 @@ static const struct dev_pm_ops uio_pdrv_genirq_dev_pm_ops = {
>  	.runtime_resume = uio_pdrv_genirq_runtime_nop,
>  };
>  
> +#ifdef CONFIG_OF
> +/* Match table for of_platform binding */
> +static const struct of_device_id __devinitconst uio_of_genirq_match[] = {
> +	{ .compatible = "generic-uio", },
> +	{ /* 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 +275,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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

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

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

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

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

[John Williams]

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

Hi Wolfram,

We discuss this again :)

On Thu, Mar 31, 2011 at 10:49 PM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>wrote:

> On Thu, Mar 31, 2011 at 02:30:00PM +0200, Michal Simek wrote:
> > Support OF support. "generic-uio" compatible property is used.
>
> And exactly this was the issue last time (when I tried). This is a
> generic property, which is linux-specific and not describing HW. The
> agreement back then was to we probably need to add compatible-entries at
> runtime (something like new_id for USB). So the uio-of-driver could be
> matched against any device. Otherwise, we would collect a lot of
> potential entries like "vendor,special-card1". Although I wonder
> meanwhile if it is really going to be that bad; we don't have so much
> UIO-driver in tree as well. Maybe worth a try?
>

Maybe I misunderstand you, in my view it is the responsibility of <vendor>
to create their DTS files to indicate they want <special-card1> to bind to
generic-uio.

So, no great list of compat strings should grow in the driver, but rather
the user of the driver must make it happen.

Am I missing something?

Our use-case is pretty clear, in FPGA-based systems it is common to create
arbitrary devices that developers just want to control from userspace, with
simple IRQ and IO capabilities (DMA can come later :).  They don't need to
bind to other kernel APIs or subsystems, and don't want to invest in one-off
kernel drivers that simply will never go upstream.

UIO is perfect, and simply tagging the device as generic-uio in the DTS is
so simple, clean, and elegant.  Traditional embedded developers really light
up when you tell them you can write simple IRQ handlers in Linux userspace.
 They love it even more if they don't have to write a line of kernel code,
which generic-uio enables.

John
-- 
John Williams, PhD, B. Eng, B. IT
PetaLogix - Linux Solutions for a Reconfigurable World
w: www.petalogix.com  p: +61-7-30090663  f: +61-7-30090663

[-- Attachment #1.2: Type: text/html, Size: 2594 bytes --]

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

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

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

[Wolfram Sang]

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


>    Maybe I misunderstand you, in my view it is the responsibility of <vendor>
>    to create their DTS files to indicate they want <special-card1> to bind to
>    generic-uio.

Device tree is a OS-neutral hardware description language. "generic-uio"
is neither OS-neutral nor a hardware description. devicetree.org has
more information about this.

>    Our use-case is pretty clear, in FPGA-based systems it is common to create
>    arbitrary devices that developers just want to control from userspace,
>    with simple IRQ and IO capabilities (DMA can come later :). �They don't
>    need to bind to other kernel APIs or subsystems, and don't want to invest
>    in one-off kernel drivers that simply will never go upstream.

For that, the new_compatible-file would be suitable, I think.

>    UIO is perfect, and simply tagging the device as generic-uio in the DTS is
>    so simple, clean, and elegant.

Simple, yes (I do understand I wrote the first approach ;)) . Elegant,
not really, because it breaks core conventions of the device tree. For
your case it is a very conveniant hack, but it is still a hack.

Regards,

   Wolfram

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

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

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

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

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

[Michal Simek]

Wolfram Sang wrote:
>>    Maybe I misunderstand you, in my view it is the responsibility of <vendor>
>>    to create their DTS files to indicate they want <special-card1> to bind to
>>    generic-uio.
> 
> Device tree is a OS-neutral hardware description language. "generic-uio"
> is neither OS-neutral nor a hardware description. devicetree.org has
> more information about this.

If you look at dts for ppc you should be able to find out linux,stdout-path in 
chosen node. The same is for bootargs which are linux specific option.
If chosem node can be used for OS specific description then this could be a way.

M



-- 
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] uio/pdrv_genirq: Add OF support

[John Williams]

On Thu, Mar 31, 2011 at 11:23 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
>
>>    Maybe I misunderstand you, in my view it is the responsibility of <vendor>
>>    to create their DTS files to indicate they want <special-card1> to bind to
>>    generic-uio.
>
> Device tree is a OS-neutral hardware description language. "generic-uio"
> is neither OS-neutral nor a hardware description. devicetree.org has
> more information about this.

If we are trying to be pure, I might argue we are not changing the DTS
language, but rather just add support in Linux for a particular
use-case.  There is no violation of DTS syntax.

It might be *recommended* that device trees describe only hardware,
although as Michal points out there is already precedent in the
'chosen' node where this is clearly violated, but in a way that is
compatible with DTS syntax.

Is it forbidden to have DTS descriptions of purely virtual devices, as
would be present if you boot a DTS-driven kernel inside a VM
environment, which provides only virtual implementations of various
devices (ethernet etc)?

'vmware,virt-enet' or whatever?

>>    Our use-case is pretty clear, in FPGA-based systems it is common to create
>>    arbitrary devices that developers just want to control from userspace,
>>    with simple IRQ and IO capabilities (DMA can come later :). �They don't
>>    need to bind to other kernel APIs or subsystems, and don't want to invest
>>    in one-off kernel drivers that simply will never go upstream.
>
> For that, the new_compatible-file would be suitable, I think.

# echo "generic-uio" > /sys/class/uio/<something>

?

>>    UIO is perfect, and simply tagging the device as generic-uio in the DTS is
>>    so simple, clean, and elegant.
>
> Simple, yes (I do understand I wrote the first approach ;)) . Elegant,
> not really, because it breaks core conventions of the device tree. For
> your case it is a very conveniant hack, but it is still a hack.

Being useful seems like a high priority in the kernel, I'm not ashamed of it! :)

Regards,

John
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

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

[Grant Likely]

On Thu, Mar 31, 2011 at 11:47:04PM +1000, John Williams wrote:
> On Thu, Mar 31, 2011 at 11:23 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> >
> >>    Maybe I misunderstand you, in my view it is the responsibility of <vendor>
> >>    to create their DTS files to indicate they want <special-card1> to bind to
> >>    generic-uio.
> >
> > Device tree is a OS-neutral hardware description language. "generic-uio"
> > is neither OS-neutral nor a hardware description. devicetree.org has
> > more information about this.
> 
> If we are trying to be pure, I might argue we are not changing the DTS
> language, but rather just add support in Linux for a particular
> use-case.  There is no violation of DTS syntax.
> 
> It might be *recommended* that device trees describe only hardware,
> although as Michal points out there is already precedent in the
> 'chosen' node where this is clearly violated, but in a way that is
> compatible with DTS syntax.

There are of course exceptions, particularly for passing boot
information that is OS specific.  There is strong pressure to avoid it
however.

> 
> Is it forbidden to have DTS descriptions of purely virtual devices, as
> would be present if you boot a DTS-driven kernel inside a VM
> environment, which provides only virtual implementations of various
> devices (ethernet etc)?
> 
> 'vmware,virt-enet' or whatever?

No, it is not at all forbidden.  However it needs to be anchored on a
real implementation of the virtual device.  The difference with uio is
that 'uio' is a very specific description of /how/ linux interacts
with the device.  It doesn't describe /what/ the interface is.

The virtio stuff is a good example because the interface is defined
indepenently of how Linux actually drives it.

So you could modify the earlier statement to say that device trees
describe only interfaces; not internal OS implementation details.

A really big problem with 'generic-uio' is that it casts a very large
net.  If you add 'generic-uio' to a nodes compatible list, then it
immediately precludes any possibility of it being driven by an
in-kernel driver.

However, as already raised there is another way to skin this cat....

> 
> >>    Our use-case is pretty clear, in FPGA-based systems it is common to create
> >>    arbitrary devices that developers just want to control from userspace,
> >>    with simple IRQ and IO capabilities (DMA can come later :). �They don't
> >>    need to bind to other kernel APIs or subsystems, and don't want to invest
> >>    in one-off kernel drivers that simply will never go upstream.
> >
> > For that, the new_compatible-file would be suitable, I think.
> 
> # echo "generic-uio" > /sys/class/uio/<something>
> 
> ?

Yeah, something like that.  I'd prefer something like:
"<vendor>,<hardware-name>" > /sys/bus/platform/drivers/generic-uio/compatible-hardware

That makes it the model to supplement the driver with additional
information about what devices it binds against.

> 
> >>    UIO is perfect, and simply tagging the device as generic-uio in the DTS is
> >>    so simple, clean, and elegant.
> >
> > Simple, yes (I do understand I wrote the first approach ;)) . Elegant,
> > not really, because it breaks core conventions of the device tree. For
> > your case it is a very conveniant hack, but it is still a hack.
> 
> Being useful seems like a high priority in the kernel, I'm not ashamed of it! :)

:-)

> 
> Regards,
> 
> John
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

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

[John Williams]

Hi Wolfram,

Sorry for the resend, my gmail session was in rich formatting mode and
the reply was rejected from the list servers.

On Thu, Mar 31, 2011 at 10:49 PM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
>
> On Thu, Mar 31, 2011 at 02:30:00PM +0200, Michal Simek wrote:
> > Support OF support. "generic-uio" compatible property is used.
>
> And exactly this was the issue last time (when I tried). This is a
> generic property, which is linux-specific and not describing HW. The
> agreement back then was to we probably need to add compatible-entries at
> runtime (something like new_id for USB). So the uio-of-driver could be
> matched against any device. Otherwise, we would collect a lot of
> potential entries like "vendor,special-card1". Although I wonder
> meanwhile if it is really going to be that bad; we don't have so much
> UIO-driver in tree as well. Maybe worth a try?


Maybe I misunderstand you, in my view it is the responsibility of
<vendor> to create their DTS files to indicate they want
<special-card1> to bind to generic-uio.

So, no great list of compat strings should grow in the driver, but
rather the user of the driver must make it happen.

Am I missing something?

Our use-case is pretty clear, in FPGA-based systems it is common to
create arbitrary devices that developers just want to control from
userspace, with simple IRQ and IO capabilities (DMA can come later :).
 They don't need to bind to other kernel APIs or subsystems, and don't
want to invest in one-off kernel drivers that simply will never go
upstream.

UIO is perfect, and simply tagging the device as generic-uio in the
DTS is so simple, clean, and elegant.  Traditional embedded developers
really light up when you tell them you can write simple IRQ handlers
in Linux userspace.  They love it even more if they don't have to
write a line of kernel code, which generic-uio enables.

John
--
John Williams, PhD, B. Eng, B. IT
PetaLogix - Linux Solutions for a Reconfigurable World
w: www.petalogix.com  p: +61-7-30090663  f: +61-7-30090663

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

[Arnd Bergmann]

On Thursday 31 March 2011, John Williams wrote:
> On Thu, Mar 31, 2011 at 10:49 PM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> >
> > On Thu, Mar 31, 2011 at 02:30:00PM +0200, Michal Simek wrote:
> > > Support OF support. "generic-uio" compatible property is used.
> >
> > And exactly this was the issue last time (when I tried). This is a
> > generic property, which is linux-specific and not describing HW. The
> > agreement back then was to we probably need to add compatible-entries at
> > runtime (something like new_id for USB). So the uio-of-driver could be
> > matched against any device. Otherwise, we would collect a lot of
> > potential entries like "vendor,special-card1". Although I wonder
> > meanwhile if it is really going to be that bad; we don't have so much
> > UIO-driver in tree as well. Maybe worth a try?
> 
> 
> Maybe I misunderstand you, in my view it is the responsibility of
> <vendor> to create their DTS files to indicate they want
> <special-card1> to bind to generic-uio.
> 
> So, no great list of compat strings should grow in the driver, but
> rather the user of the driver must make it happen.
> 
> Am I missing something?

We try to make the device tree on describe the present hardware,
but not relate to how it is used.

There are certainly cases where a specific piece of hardware can
be used either by a kernel-only driver or the UIO driver with a
user backend. I would argue that you should be able to use an
identical device tree for both cases, because the hardware is
the same. Chosing which driver to use can be either in the realm
of the kernel, or even user policy.

	Arnd

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

[Michal Simek]

Arnd Bergmann wrote:
> On Thursday 31 March 2011, John Williams wrote:
>> On Thu, Mar 31, 2011 at 10:49 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
>>> On Thu, Mar 31, 2011 at 02:30:00PM +0200, Michal Simek wrote:
>>>> Support OF support. "generic-uio" compatible property is used.
>>> And exactly this was the issue last time (when I tried). This is a
>>> generic property, which is linux-specific and not describing HW. The
>>> agreement back then was to we probably need to add compatible-entries at
>>> runtime (something like new_id for USB). So the uio-of-driver could be
>>> matched against any device. Otherwise, we would collect a lot of
>>> potential entries like "vendor,special-card1". Although I wonder
>>> meanwhile if it is really going to be that bad; we don't have so much
>>> UIO-driver in tree as well. Maybe worth a try?
>>
>> Maybe I misunderstand you, in my view it is the responsibility of
>> <vendor> to create their DTS files to indicate they want
>> <special-card1> to bind to generic-uio.
>>
>> So, no great list of compat strings should grow in the driver, but
>> rather the user of the driver must make it happen.
>>
>> Am I missing something?
> 
> We try to make the device tree on describe the present hardware,
> but not relate to how it is used.
> 
> There are certainly cases where a specific piece of hardware can
> be used either by a kernel-only driver or the UIO driver with a
> user backend. I would argue that you should be able to use an
> identical device tree for both cases, because the hardware is
> the same. Chosing which driver to use can be either in the realm
> of the kernel, or even user policy.

ok. What about to keep of_device_id empty?  Then there is compatible property 
string and everybody can choose what wants.
OF is just a different driver initialization method but it is in the same 
category which is supported right now which is initialization through 
platform_device structure.

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] uio/pdrv_genirq: Add OF support

[Grant Likely]

On Thu, Mar 31, 2011 at 03:51:32PM +0200, Michal Simek wrote:
> Arnd Bergmann wrote:
> >On Thursday 31 March 2011, John Williams wrote:
> >>On Thu, Mar 31, 2011 at 10:49 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> >>>On Thu, Mar 31, 2011 at 02:30:00PM +0200, Michal Simek wrote:
> >>>>Support OF support. "generic-uio" compatible property is used.
> >>>And exactly this was the issue last time (when I tried). This is a
> >>>generic property, which is linux-specific and not describing HW. The
> >>>agreement back then was to we probably need to add compatible-entries at
> >>>runtime (something like new_id for USB). So the uio-of-driver could be
> >>>matched against any device. Otherwise, we would collect a lot of
> >>>potential entries like "vendor,special-card1". Although I wonder
> >>>meanwhile if it is really going to be that bad; we don't have so much
> >>>UIO-driver in tree as well. Maybe worth a try?
> >>
> >>Maybe I misunderstand you, in my view it is the responsibility of
> >><vendor> to create their DTS files to indicate they want
> >><special-card1> to bind to generic-uio.
> >>
> >>So, no great list of compat strings should grow in the driver, but
> >>rather the user of the driver must make it happen.
> >>
> >>Am I missing something?
> >
> >We try to make the device tree on describe the present hardware,
> >but not relate to how it is used.
> >
> >There are certainly cases where a specific piece of hardware can
> >be used either by a kernel-only driver or the UIO driver with a
> >user backend. I would argue that you should be able to use an
> >identical device tree for both cases, because the hardware is
> >the same. Chosing which driver to use can be either in the realm
> >of the kernel, or even user policy.
> 
> ok. What about to keep of_device_id empty?  Then there is compatible
> property string and everybody can choose what wants.
> OF is just a different driver initialization method but it is in the
> same category which is supported right now which is initialization
> through platform_device structure.

I'm not completely sure I understand what you're suggesting here.
Yes, of_device_id can be left unpopulated, but then you need to make
sure another method is available for binding the driver.

hmmmm....

You could see if the manual 'bind/unbind' platform_bus sysfs
attributes would do the job for you (see drivers/base/bus.c).  You'd
need some mechanism to force the generic-uio driver to accept the
device.

g.

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

[Michal Simek]

Wolfram Sang wrote:
> On Thu, Mar 31, 2011 at 02:30:00PM +0200, Michal Simek wrote:
>> Support OF support. "generic-uio" compatible property is used.
> 
> And exactly this was the issue last time (when I tried). This is a
> generic property, which is linux-specific and not describing HW. The
> agreement back then was to we probably need to add compatible-entries at
> runtime (something like new_id for USB). So the uio-of-driver could be
> matched against any device. Otherwise, we would collect a lot of
> potential entries like "vendor,special-card1". Although I wonder
> meanwhile if it is really going to be that bad; we don't have so much
> UIO-driver in tree as well. Maybe worth a try?

I will read reactions to get better picture to be able to argue. :-)

> 
>> Signed-off-by: Michal Simek <monstr@monstr.eu>
>> ---
>>  drivers/uio/uio_pdrv_genirq.c |   60 ++++++++++++++++++++++++++++++++++++++--
>>  1 files changed, 57 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
>> index 7174d51..9e89806 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 {
>> @@ -92,11 +96,44 @@ static int uio_pdrv_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on)
>>  static int uio_pdrv_genirq_probe(struct platform_device *pdev)
>>  {
>>  	struct uio_info *uioinfo = pdev->dev.platform_data;
>> -	struct uio_pdrv_genirq_platdata *priv;
>> +	struct uio_pdrv_genirq_platdata *priv = NULL;
> 
> unrelated?

you are right here. I changed order and this is not necessary.


> 
>>  	struct uio_mem *uiomem;
>>  	int ret = -EINVAL;
>>  	int i;
>>  
>> +	if (!uioinfo) {
>> +		struct resource r_irq; /* Interrupt resources */
>> +		int rc = 0;
>> +
>> +		rc = of_address_to_resource(pdev->dev.of_node, 0,
>> +							&pdev->resource[0]);
>> +		if (rc) {
>> +			dev_err(&pdev->dev, "invalid address\n");
>> +			goto bad2;
>> +		}
>> +		pdev->num_resources = 1;
>> +
>> +		/* 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;
>> +		/* Use version for storing full IP name for identification */
>> +		uioinfo->version = pdev->dev.of_node->full_name;
> 
> I don't think this is apropriate, but will leave that to Hans.

I was thinking what to add and I choose full_name because I can read this value 
and identify which UIO is this device.
I know that there should be version but there is no version string in DTS.

> 
>> +		/* Get IRQ for the device */
>> +		rc = of_irq_to_resource(pdev->dev.of_node, 0, &r_irq);
>> +		if (rc == NO_IRQ)
>> +			dev_err(&pdev->dev, "no IRQ found\n");
> 
> No error, I think. Sometimes just mmaping the registers is enough.

OK. Let's changed it to dev_info if you like.


> 
>> +		else {
>> +			uioinfo->irq = r_irq.start;
>> +			dev_info(&pdev->dev, "irq %d\n", (u32)uioinfo->irq);
>> +		}
>> +	}
>> +
>>  	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
>>  		dev_err(&pdev->dev, "missing platform_data\n");
>>  		goto bad0;
>> @@ -176,10 +213,15 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
>>  
>>  	platform_set_drvdata(pdev, priv);
>>  	return 0;
>> - bad1:
>> +
>> +bad1:
> 
> The spaces before labels are intentional, better keep them.

I found both cases. checkpatch doesn't show any problem for both cases that's 
why if you like space before label, I am fine with this.

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] uio/pdrv_genirq: Add OF support

[Hans J. Koch]

On Thu, Mar 31, 2011 at 03:28:41PM +0200, Michal Simek wrote:
> >>+		uioinfo->name = pdev->dev.of_node->name;
> >>+		/* Use version for storing full IP name for identification */
> >>+		uioinfo->version = pdev->dev.of_node->full_name;
> >
> >I don't think this is apropriate, but will leave that to Hans.
> 
> I was thinking what to add and I choose full_name because I can read
> this value and identify which UIO is this device.
> I know that there should be version but there is no version string in DTS.

The purpose of uio_info->version is to give the userspace part of the driver
additional information. Kernel part and userspace part might be developed
independently, and there should be a chance for the userspace part to find
out if a certain feature is already supported by the kernel part without
having to do dirty kernel version checks.

So, uio_info->version is an information about the driver, not the hardware.

Example: You write a UIO driver for a chip you use in a project. You don't
need all the functionality of that chip. One year later you need additional
chip functionality, and it turns out that you have to do certain
initializations in the kernel part. Your new userspace will need the new
kernel driver, but there are lots of older kernels around in your customers
devices. In that case, your userspace part can simply check the version
string in sysfs and require at least your new version.

> 
> >
> >>+		/* Get IRQ for the device */
> >>+		rc = of_irq_to_resource(pdev->dev.of_node, 0, &r_irq);
> >>+		if (rc == NO_IRQ)
> >>+			dev_err(&pdev->dev, "no IRQ found\n");
> >
> >No error, I think. Sometimes just mmaping the registers is enough.
> 
> OK. Let's changed it to dev_info if you like.

The correct thing is to set uio_info->irq to UIO_IRQ_NONE.

Thanks,
Hans

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

[Michal Simek]

Hans J. Koch wrote:
> On Thu, Mar 31, 2011 at 03:28:41PM +0200, Michal Simek wrote:
>>>> +		uioinfo->name = pdev->dev.of_node->name;
>>>> +		/* Use version for storing full IP name for identification */
>>>> +		uioinfo->version = pdev->dev.of_node->full_name;
>>> I don't think this is apropriate, but will leave that to Hans.
>> I was thinking what to add and I choose full_name because I can read
>> this value and identify which UIO is this device.
>> I know that there should be version but there is no version string in DTS.
> 
> The purpose of uio_info->version is to give the userspace part of the driver
> additional information. Kernel part and userspace part might be developed
> independently, and there should be a chance for the userspace part to find
> out if a certain feature is already supported by the kernel part without
> having to do dirty kernel version checks.
> 
> So, uio_info->version is an information about the driver, not the hardware.
> 
> Example: You write a UIO driver for a chip you use in a project. You don't
> need all the functionality of that chip. One year later you need additional
> chip functionality, and it turns out that you have to do certain
> initializations in the kernel part. Your new userspace will need the new
> kernel driver, but there are lots of older kernels around in your customers
> devices. In that case, your userspace part can simply check the version
> string in sysfs and require at least your new version.

I understand reasons but this information is not in device tree and it must be 
setup.
Grant suggested compatible string but it is not the best option too.


> 
>>>> +		/* Get IRQ for the device */
>>>> +		rc = of_irq_to_resource(pdev->dev.of_node, 0, &r_irq);
>>>> +		if (rc == NO_IRQ)
>>>> +			dev_err(&pdev->dev, "no IRQ found\n");
>>> No error, I think. Sometimes just mmaping the registers is enough.
>> OK. Let's changed it to dev_info if you like.
> 
> The correct thing is to set uio_info->irq to UIO_IRQ_NONE.

ok, done.


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] uio/pdrv_genirq: Add OF support

[Hans J. Koch]

On Thu, Mar 31, 2011 at 07:57:47PM +0200, Michal Simek wrote:
> Hans J. Koch wrote:
> >On Thu, Mar 31, 2011 at 03:28:41PM +0200, Michal Simek wrote:
> >>>>+		uioinfo->name = pdev->dev.of_node->name;
> >>>>+		/* Use version for storing full IP name for identification */
> >>>>+		uioinfo->version = pdev->dev.of_node->full_name;
> >>>I don't think this is apropriate, but will leave that to Hans.
> >>I was thinking what to add and I choose full_name because I can read
> >>this value and identify which UIO is this device.
> >>I know that there should be version but there is no version string in DTS.
> >
> >The purpose of uio_info->version is to give the userspace part of the driver
> >additional information. Kernel part and userspace part might be developed
> >independently, and there should be a chance for the userspace part to find
> >out if a certain feature is already supported by the kernel part without
> >having to do dirty kernel version checks.
> >
> >So, uio_info->version is an information about the driver, not the hardware.
> >
> >Example: You write a UIO driver for a chip you use in a project. You don't
> >need all the functionality of that chip. One year later you need additional
> >chip functionality, and it turns out that you have to do certain
> >initializations in the kernel part. Your new userspace will need the new
> >kernel driver, but there are lots of older kernels around in your customers
> >devices. In that case, your userspace part can simply check the version
> >string in sysfs and require at least your new version.
> 
> I understand reasons but this information is not in device tree and
> it must be setup.
> Grant suggested compatible string but it is not the best option too.

In uio_pdrv_genirq, uio_info->version is hardcoded in platform data. Hardware
initialization can also take place in the same platform specific file, which
is common practice on archs like ARM. Therefore, a driver specific versioning
can make sense for UIO, even if the driver code itself doesn't change.

If you have no equivalent for that in device tree, you should create a new
generic driver (uio_of_genirq?) that simply doesn't support this kind of
versioning.

Seems like sometimes it's not enough to just describe hardware...

Thanks,
Hans

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

[Grant Likely]

On Thu, Mar 31, 2011 at 1:23 PM, Hans J. Koch <hjk@hansjkoch.de> wrote:
> On Thu, Mar 31, 2011 at 07:57:47PM +0200, Michal Simek wrote:
>> Hans J. Koch wrote:
>> >On Thu, Mar 31, 2011 at 03:28:41PM +0200, Michal Simek wrote:
>> >>>>+         uioinfo->name = pdev->dev.of_node->name;
>> >>>>+         /* Use version for storing full IP name for identification */
>> >>>>+         uioinfo->version = pdev->dev.of_node->full_name;
>> >>>I don't think this is apropriate, but will leave that to Hans.
>> >>I was thinking what to add and I choose full_name because I can read
>> >>this value and identify which UIO is this device.
>> >>I know that there should be version but there is no version string in DTS.
>> >
>> >The purpose of uio_info->version is to give the userspace part of the driver
>> >additional information. Kernel part and userspace part might be developed
>> >independently, and there should be a chance for the userspace part to find
>> >out if a certain feature is already supported by the kernel part without
>> >having to do dirty kernel version checks.
>> >
>> >So, uio_info->version is an information about the driver, not the hardware.
>> >
>> >Example: You write a UIO driver for a chip you use in a project. You don't
>> >need all the functionality of that chip. One year later you need additional
>> >chip functionality, and it turns out that you have to do certain
>> >initializations in the kernel part. Your new userspace will need the new
>> >kernel driver, but there are lots of older kernels around in your customers
>> >devices. In that case, your userspace part can simply check the version
>> >string in sysfs and require at least your new version.
>>
>> I understand reasons but this information is not in device tree and
>> it must be setup.
>> Grant suggested compatible string but it is not the best option too.
>
> In uio_pdrv_genirq, uio_info->version is hardcoded in platform data. Hardware
> initialization can also take place in the same platform specific file, which
> is common practice on archs like ARM. Therefore, a driver specific versioning
> can make sense for UIO, even if the driver code itself doesn't change.
>
> If you have no equivalent for that in device tree, you should create a new
> generic driver (uio_of_genirq?) that simply doesn't support this kind of
> versioning.

I'd avoid that.  Current trend is to move away from separate
of-specific data because the driver is essentially identical other
than the data source being different (platform_data vs. a device node
pointer).

g.

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

[Hans J. Koch]

On Thu, Mar 31, 2011 at 01:48:32PM -0600, Grant Likely wrote:
> On Thu, Mar 31, 2011 at 1:23 PM, Hans J. Koch <hjk@hansjkoch.de> wrote:
> > On Thu, Mar 31, 2011 at 07:57:47PM +0200, Michal Simek wrote:
> >> Hans J. Koch wrote:
> >> >On Thu, Mar 31, 2011 at 03:28:41PM +0200, Michal Simek wrote:
> >> >>>>+         uioinfo->name = pdev->dev.of_node->name;
> >> >>>>+         /* Use version for storing full IP name for identification */
> >> >>>>+         uioinfo->version = pdev->dev.of_node->full_name;
> >> >>>I don't think this is apropriate, but will leave that to Hans.
> >> >>I was thinking what to add and I choose full_name because I can read
> >> >>this value and identify which UIO is this device.
> >> >>I know that there should be version but there is no version string in DTS.
> >> >
> >> >The purpose of uio_info->version is to give the userspace part of the driver
> >> >additional information. Kernel part and userspace part might be developed
> >> >independently, and there should be a chance for the userspace part to find
> >> >out if a certain feature is already supported by the kernel part without
> >> >having to do dirty kernel version checks.
> >> >
> >> >So, uio_info->version is an information about the driver, not the hardware.
> >> >
> >> >Example: You write a UIO driver for a chip you use in a project. You don't
> >> >need all the functionality of that chip. One year later you need additional
> >> >chip functionality, and it turns out that you have to do certain
> >> >initializations in the kernel part. Your new userspace will need the new
> >> >kernel driver, but there are lots of older kernels around in your customers
> >> >devices. In that case, your userspace part can simply check the version
> >> >string in sysfs and require at least your new version.
> >>
> >> I understand reasons but this information is not in device tree and
> >> it must be setup.
> >> Grant suggested compatible string but it is not the best option too.
> >
> > In uio_pdrv_genirq, uio_info->version is hardcoded in platform data. Hardware
> > initialization can also take place in the same platform specific file, which
> > is common practice on archs like ARM. Therefore, a driver specific versioning
> > can make sense for UIO, even if the driver code itself doesn't change.
> >
> > If you have no equivalent for that in device tree, you should create a new
> > generic driver (uio_of_genirq?) that simply doesn't support this kind of
> > versioning.
> 
> I'd avoid that.  Current trend is to move away from separate
> of-specific data because the driver is essentially identical other
> than the data source being different (platform_data vs. a device node
> pointer).

The point here his that UIO drivers are _not_ like normal drivers, because
they're split into a kernel and a userspace part. If there are changes in the
hardware initialization the kernel performs, and the userspace part of the
driver needs to know about it, we use the "version" attribute in sysfs to
communicate that.

OK, userspace could check the kernel version. But UIO drivers are mostly used
in environments where custom non-mainline kernels reporting arbitrary version
numbers are running (less than 1% of the UIO drivers in use are ever posted
on LKML, I guess). That makes it at least difficult for these people to write
a clean UIO driver that can deal with different kernels.

Killing the "version" attribute means breaking some out-of-tree UIO drivers.
Personally, I'm fine with that. But completely throwing away the possibility
of that kind of versioning is one step too far IMHO.

For UIO, it is not enough to just know "we have this chip using that driver",
at least not for a generic driver like the one proposed in this patch.

Thanks,
Hans

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

[Wolfram Sang]

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


> For UIO, it is not enough to just know "we have this chip using that driver",
> at least not for a generic driver like the one proposed in this patch.

So, what about setting this string to a default (e.g. '0' or 'generic' or
whatever). Then, userspace knows this is the unmodified upstream version of the
generic driver. If the generic driver is not sufficent for a user and he
patches it, he can simply patch the version string, too?

Or can we use the notifiers to set up an individual version? That could also be
the place to do special board-setup connected to the selected version.

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: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

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

[Hans J. Koch]

On Sat, Apr 02, 2011 at 12:35:50PM +0200, Wolfram Sang wrote:
> 
> > For UIO, it is not enough to just know "we have this chip using that driver",
> > at least not for a generic driver like the one proposed in this patch.
> 
> So, what about setting this string to a default (e.g. '0' or 'generic' or
> whatever). Then, userspace knows this is the unmodified upstream version of the
> generic driver.

A "generic driver" should always be unmodified since you don't know who else
might be using it. With uio_pdrv_genirq it is different when used with
platform devices (which is its original purpose), since in the same file where
you setup your struct uio_info you can also have code to initialize the device.

Therefore, platform devices need the ability to adjust the version attribute.
Device tree code should probably don't touch it and leave it at a default value
as you suggested.

> If the generic driver is not sufficent for a user and he
> patches it, he can simply patch the version string, too?

Or better write a dedicated driver. Patching a generic thing like uio_pdrv_genirq
can only be done in specialized drivers in a project. That's hackery that won't
make it into mainline anyway.

> 
> Or can we use the notifiers to set up an individual version? That could also be
> the place to do special board-setup connected to the selected version.

How could that look like? If you have an idea for a generic solution, you're
welcome.

Thanks,
Hans

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

[Wolfram Sang]

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

Hi Hans,

> > If the generic driver is not sufficent for a user and he
> > patches it, he can simply patch the version string, too?
> 
> Or better write a dedicated driver. Patching a generic thing like uio_pdrv_genirq
> can only be done in specialized drivers in a project. That's hackery that won't
> make it into mainline anyway.

I exactly meant that, maybe didn't make it clear enough. Of course, a seperate
driver would be better, but guess what will happen in most projects.

> > Or can we use the notifiers to set up an individual version? That could also be
> > the place to do special board-setup connected to the selected version.
> 
> How could that look like? If you have an idea for a generic solution, you're
> welcome.

That was more a brainstorming question; there are notifiers which help
populating function-pointers which also can't be expressed in a device-tree.
I just wanted to make sure this approach doesn't get overlooked.

So, the solution for now is to simply add a default value and we can check
later if it can be modified at runtime?

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: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

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

[Hans J. Koch]

On Mon, Apr 04, 2011 at 07:31:49PM +0200, Wolfram Sang wrote:
> 
> So, the solution for now is to simply add a default value and we can check
> later if it can be modified at runtime?

Yes.

Thanks,
Hans

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

[Michal Simek]

Hans J. Koch wrote:
> On Mon, Apr 04, 2011 at 07:31:49PM +0200, Wolfram Sang wrote:
>> So, the solution for now is to simply add a default value and we can check
>> later if it can be modified at runtime?
> 
> Yes.

ok. Nice. What default value would you like to see as the version string?
'0', 'generic', 'generic-uio', 'dt'

Ok the last thing is how to handle compatible property.

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] uio/pdrv_genirq: Add OF support

[Hans J. Koch]

On Tue, Apr 05, 2011 at 08:25:44AM +0200, Michal Simek wrote:
> Hans J. Koch wrote:
> >On Mon, Apr 04, 2011 at 07:31:49PM +0200, Wolfram Sang wrote:
> >>So, the solution for now is to simply add a default value and we can check
> >>later if it can be modified at runtime?
> >
> >Yes.
> 
> ok. Nice. What default value would you like to see as the version string?
> '0', 'generic', 'generic-uio', 'dt'

Actually, I don't care too much. '0' or 'generic' would be OK. I don't
like 'generic-uio' because it only applies to one driver, not UIO as
a whole.

> 
> Ok the last thing is how to handle compatible property.

if 'dt' means 'device tree' then I'd actually like that one since this
default is only used in the device tree case. Obviously you must not
change the version attributes set by platform devices.

Thanks,
Hans

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

[Grant Likely]

On Thu, Mar 31, 2011 at 02:30:00PM +0200, Michal Simek wrote:
> Support OF support. "generic-uio" compatible property is used.
> 
> Signed-off-by: Michal Simek <monstr-pSz03upnqPeHXe+LvDLADg@public.gmane.org>
> ---
>  drivers/uio/uio_pdrv_genirq.c |   60 ++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 57 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
> index 7174d51..9e89806 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 {
> @@ -92,11 +96,44 @@ static int uio_pdrv_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on)
>  static int uio_pdrv_genirq_probe(struct platform_device *pdev)
>  {
>  	struct uio_info *uioinfo = pdev->dev.platform_data;
> -	struct uio_pdrv_genirq_platdata *priv;
> +	struct uio_pdrv_genirq_platdata *priv = NULL;
>  	struct uio_mem *uiomem;
>  	int ret = -EINVAL;
>  	int i;
>  
> +	if (!uioinfo) {
> +		struct resource r_irq; /* Interrupt resources */
> +		int rc = 0;
> +
> +		rc = of_address_to_resource(pdev->dev.of_node, 0,
> +							&pdev->resource[0]);
> +		if (rc) {
> +			dev_err(&pdev->dev, "invalid address\n");
> +			goto bad2;
> +		}
> +		pdev->num_resources = 1;

You shouldn't need this anymore.  Device tree sourced platform_devices
get their resource table populated automatically.  Also, drivers
should /never/ modify the resource values set in the device because it
messes up driver rebinding.

> +
> +		/* 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;
> +		/* Use version for storing full IP name for identification */
> +		uioinfo->version = pdev->dev.of_node->full_name;

Comment on the binding: You should probably use the first entry in the
compatible list for the name of the device.  Node names should be
generic and usually they will say what a device does, but not what a
device actually /is/ (this is the Generic Names recommended practice).

Modern convention is to rely on the first compatible entry for
describing what ip block it is.


> +
> +		/* Get IRQ for the device */
> +		rc = of_irq_to_resource(pdev->dev.of_node, 0, &r_irq);

Ditto here, the pdev should already have the irq resource populated.

> +		if (rc == NO_IRQ)
> +			dev_err(&pdev->dev, "no IRQ found\n");
> +		else {
> +			uioinfo->irq = r_irq.start;
> +			dev_info(&pdev->dev, "irq %d\n", (u32)uioinfo->irq);
> +		}
> +	}
> +
>  	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
>  		dev_err(&pdev->dev, "missing platform_data\n");
>  		goto bad0;
> @@ -176,10 +213,15 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, priv);
>  	return 0;
> - bad1:
> +
> +bad1:
>  	kfree(priv);
>  	pm_runtime_disable(&pdev->dev);
> - bad0:
> +bad0:
> +	/* kfree uioinfo for CONFIG_OF */
> +	if (!pdev->dev.platform_data)
> +		kfree(uioinfo);
> +bad2:
>  	return ret;
>  }
>  
> @@ -215,6 +257,17 @@ static const struct dev_pm_ops uio_pdrv_genirq_dev_pm_ops = {
>  	.runtime_resume = uio_pdrv_genirq_runtime_nop,
>  };
>  
> +#ifdef CONFIG_OF
> +/* Match table for of_platform binding */
> +static const struct of_device_id __devinitconst uio_of_genirq_match[] = {
> +	{ .compatible = "generic-uio", },
> +	{ /* 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 +275,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] uio/pdrv_genirq: Add OF support

[Michal Simek]

Grant Likely wrote:
> On Thu, Mar 31, 2011 at 02:30:00PM +0200, Michal Simek wrote:
>> Support OF support. "generic-uio" compatible property is used.
>>
>> Signed-off-by: Michal Simek <monstr-pSz03upnqPeHXe+LvDLADg@public.gmane.org>
>> ---
>>  drivers/uio/uio_pdrv_genirq.c |   60 ++++++++++++++++++++++++++++++++++++++--
>>  1 files changed, 57 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
>> index 7174d51..9e89806 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 {
>> @@ -92,11 +96,44 @@ static int uio_pdrv_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on)
>>  static int uio_pdrv_genirq_probe(struct platform_device *pdev)
>>  {
>>  	struct uio_info *uioinfo = pdev->dev.platform_data;
>> -	struct uio_pdrv_genirq_platdata *priv;
>> +	struct uio_pdrv_genirq_platdata *priv = NULL;
>>  	struct uio_mem *uiomem;
>>  	int ret = -EINVAL;
>>  	int i;
>>  
>> +	if (!uioinfo) {
>> +		struct resource r_irq; /* Interrupt resources */
>> +		int rc = 0;
>> +
>> +		rc = of_address_to_resource(pdev->dev.of_node, 0,
>> +							&pdev->resource[0]);
>> +		if (rc) {
>> +			dev_err(&pdev->dev, "invalid address\n");
>> +			goto bad2;
>> +		}
>> +		pdev->num_resources = 1;
> 
> You shouldn't need this anymore.  Device tree sourced platform_devices
> get their resource table populated automatically.  Also, drivers
> should /never/ modify the resource values set in the device because it
> messes up driver rebinding.

done.

> 
>> +
>> +		/* 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;
>> +		/* Use version for storing full IP name for identification */
>> +		uioinfo->version = pdev->dev.of_node->full_name;
> 
> Comment on the binding: You should probably use the first entry in the
> compatible list for the name of the device.  Node names should be
> generic and usually they will say what a device does, but not what a
> device actually /is/ (this is the Generic Names recommended practice).
> 
> Modern convention is to rely on the first compatible entry for
> describing what ip block it is.

Is it easy to way to find it out?

M


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