| Message ID | 1244765062-14144-3-git-send-email-w.sang@pengutronix.de (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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
On Thu, Jun 11, 2009 at 6:04 PM, Wolfram Sang<w.sang@pengutronix.de> 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@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>; > + }; 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.
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
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/ |
> 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
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/ |
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
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.
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.
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
> > 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
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/ |
> > + 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
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
> 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.)
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/ |
> 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
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.
> > 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
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.
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");
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@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