Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Tue, Jun 11, 2013 at 11:25 PM, Grant Likely
grant.lik...@secretlab.ca wrote:
On Fri, 26 Apr 2013 16:31:24 -0500, Jon Hunter jon-hun...@ti.com wrote:
On 04/26/2013 02:31 AM, Linus Walleij wrote:
On Wed, Apr 17, 2013 at 2:41 AM, Javier Martinez Canillas
martinez.jav...@gmail.com wrote:
So:
+static int omap_gpio_irq_domain_xlate(struct irq_domain *d,
+ struct device_node *ctrlr,
+ const u32 *intspec, unsigned int
intsize,
+ irq_hw_number_t *out_hwirq,
+ unsigned int *out_type)
+{
+ int ret;
+ struct gpio_bank *bank = d-host_data;
+ int gpio = bank-chip.base + intspec[0];
+
+ if (WARN_ON(intsize 2))
+ return -EINVAL;
+
+ ret = gpio_request_one(gpio, GPIOF_IN, ctrlr-full_name);
+ if (ret)
+ return ret;
So how to figure out if a device is already requesting this GPIO
on some orthogonal axis?
I really don't think that is necessary. Hopefully, my other email [1]
elaborates on why. Let me know if this makes sense.
I would agree here. If the irq specified happens to be a GPIO; then the
onus is on the GPIO/IRQ controller driver to make sure that GPIO is
actually set up to work as an IRQ.
Hm, re-reading this I guess the gpio_request_one() will block
others from using the pin for anything else.
Isn't that the answer to my actual question...?
It seems more like I was asking a pretty stupid question
actually.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Fri, 26 Apr 2013 16:31:24 -0500, Jon Hunter jon-hun...@ti.com wrote:
On 04/26/2013 02:31 AM, Linus Walleij wrote:
On Wed, Apr 17, 2013 at 2:41 AM, Javier Martinez Canillas
martinez.jav...@gmail.com wrote:
So:
+static int omap_gpio_irq_domain_xlate(struct irq_domain *d,
+ struct device_node *ctrlr,
+ const u32 *intspec, unsigned int
intsize,
+ irq_hw_number_t *out_hwirq,
+ unsigned int *out_type)
+{
+ int ret;
+ struct gpio_bank *bank = d-host_data;
+ int gpio = bank-chip.base + intspec[0];
+
+ if (WARN_ON(intsize 2))
+ return -EINVAL;
+
+ ret = gpio_request_one(gpio, GPIOF_IN, ctrlr-full_name);
+ if (ret)
+ return ret;
So how to figure out if a device is already requesting this GPIO
on some orthogonal axis?
I really don't think that is necessary. Hopefully, my other email [1]
elaborates on why. Let me know if this makes sense.
I would agree here. If the irq specified happens to be a GPIO; then the
onus is on the GPIO/IRQ controller driver to make sure that GPIO is
actually set up to work as an IRQ.
g.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Fri, Apr 26, 2013 at 11:25 PM, Jon Hunter jon-hun...@ti.com wrote:
On 04/26/2013 02:27 AM, Linus Walleij wrote:
You will just have to give the xlate function information about which
GPIOs are used as IRQs only, and only request the GPIO on these.
That should not be necessary. The xlate is only called in the case where
the gpio controller is declared as the interrupt controller in the
device tree source ...
Sorry I don't understand this. Irqdomains are not device tree
concepts. Why would it only be called if it was declared such
or such in the device tree?
eth@0 {
compatible = ks8851;
...
interrupt-parent = gpio2;
interrupts = 2 8; /* gpio line 34, low triggered */
...
};
So in the case where you have a driver call gpio_request() and then
request_irq(), in the device tree source, the gpio controller will not
be declared as an interrupt controller. It will just list the gpios ...
And how do you get from there to the conclusion that
gpio_to_irq() will not ever be called, maybe by
some other driver than the ethernet?
sdhci@c8000200 {
...
cd-gpios = gpio 69 0; /* gpio PI5 */
wp-gpios = gpio 57 0; /* gpio PH1 */
...
};
Like if this driver wants to set up an IRQ?
It all comes back to this: keep all applicable knowledge in the
system, so it can make proper decisions, don't try to create
mechanisms that will half-guess things.
Agreed. However the xlate looks like a good place to request the gpio
without needing to add these input-hogs or any other platform code. So
if this is not considered abuse of the xlate, then it seems like an
ideal solution. I have been also implementing a generic xlate so that we
don't need to have a xlate for each platform as well.
I'm still not following. Maybe I'm just too stupid...
How do you *avoid* requesting the GPIO in the translation
for a certain IRQ if the driver first issues gpio_request() and
then goes on to perform gpio_to_irq() itself?
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Mon, Apr 15, 2013 at 6:58 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 04/14/2013 02:53 PM, Linus Walleij wrote: This is a pretty hard design pattern to properly replicate in every such driver is it not? Well, instead of adding .request_irq() to the irqchip, and then making each driver call gpio_request() from the implementation, perhaps you could add a .irq_to_gpio() to the irqchip, have the IRQ core call that, and if it gets back a non-error response, the IRQ core could call gpio_request(). That means that the change to each GPIO+IRQ driver is simply to implement a standalone data transformation function .irq_to_gpio(). Now, this does re-introduce irq_to_gpio() in some way, but with the following advantages: 1) The implementation is per-controller, not a single global function, so isn't introducing any kind of centralized mapping scheme again. 2) This irq-chip-specific .irq_to_gpio() would only be implemented for IRQ+GPIO chips that actually have a 1:1 mapping between GPIOs and IRQs. Its potential existence doesn't imply that all IRQ chips need implement this; it would be very specifically be for this one particular case. I sort of like the idea. It'd have to go past Grant and Thomas G though. A problem is that this will result in the same kind of dilemma that the pinctrl subsystem has been facing with mapping the global GPIO numbers back into local numbers (as is the nature of IRQ numbers). Such as is done by the GPIO ranges in pinctrl. The good part is that we now know how to actually contain that, even how to do it in DT. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Mon, Apr 15, 2013 at 11:40 PM, Jon Hunter jon-hun...@ti.com wrote: I am still concerned about the case where a driver may have already called gpio_request() and then calls request_irq(). I think that the solution needs to handle cases where the driver may or may not call gpio_request() to allocate the gpio. So take a step back and you see that this is my actual fear and why I'm so hesitant about this patch to begin with. I wanted drivers to follow the sequence: gpio_request(), gpio_to_irq(), request_irq() Now if request_irq() can implicitly perform gpio_request(), we end up with a semantic confusing mess like this where we are uncertain as to whether we should use the irq we obtain elsewhere (like from a resource on the platform device)) or maybe use gpio_to_irq() to convert it into an IRQ or not or ... It is certainly true that for some devices we don't want to know if the IRQ comes from a hard-wired IRQ line or from a GPIO line playing the rôle of IRQ controller, so we need to handle that. But then the drivers need to be consistent, and relying solely on code review to stop people from doing mixtures like the described is not working, the subsystem must enforce a proper policy. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Wed, Apr 17, 2013 at 5:41 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 04/16/2013 05:14 PM, Jon Hunter wrote: c) I have the feeling that hooking the of_xlate function for this is a bit of an abuse of the function. I was wondering about that. So I was grep'ing through the various xlate implementations and found this [1]. Also you may recall that in the of_dma_simple_xlate() we call the dma_request_channel() to allocate the channel, which is very similar. However, I don't wish to get a reputation as abusing APIs so would be good to know if this really is abuse or not ;-) Cheers Jon [1] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/195124 Interesting. This is really something that the core DT and GPIO and IRQ maintainers should weigh in on. Hence, changing them from Cc: to To: in this message and/or adding them. So I guess it is OK for a driver or DT node to use a GPIO as IRQ *only*, and then have the GPIO requested implicitly through the xlate function in the irqdomain. It's the use cases where one and the same driver tries to use the same GPIO line *also* by requesting it using gpio_request() that madness starts. In such cases the driver deserves to have an error thrown back at it, as it definately knows the GPIO pin and can be refactored to use gpio_to_irq() to translate it into an IRQ rather than having it implicitly done in the xlate function. You will just have to give the xlate function information about which GPIOs are used as IRQs only, and only request the GPIO on these. And I recently suggested a mechanism to do that, and that is what I called GPIO input-hogs, which means that you mark (e.g. from the device tree) at probe() of the gpiochip which GPIOs will only be used as inputs, so as to generate IRQs. This is perfectly OS-neutral information about the how the hardware is set up in the system. If you only allow these hogges inputs to be translated and requested in the xlate function, everything goes smooth. It all comes back to this: keep all applicable knowledge in the system, so it can make proper decisions, don't try to create mechanisms that will half-guess things. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Wed, Apr 17, 2013 at 2:41 AM, Javier Martinez Canillas
martinez.jav...@gmail.com wrote:
So:
+static int omap_gpio_irq_domain_xlate(struct irq_domain *d,
+ struct device_node *ctrlr,
+ const u32 *intspec, unsigned int
intsize,
+ irq_hw_number_t *out_hwirq,
+ unsigned int *out_type)
+{
+ int ret;
+ struct gpio_bank *bank = d-host_data;
+ int gpio = bank-chip.base + intspec[0];
+
+ if (WARN_ON(intsize 2))
+ return -EINVAL;
+
+ ret = gpio_request_one(gpio, GPIOF_IN, ctrlr-full_name);
+ if (ret)
+ return ret;
So how to figure out if a device is already requesting this GPIO
on some orthogonal axis?
if (this_gpio_was_hogged_as_input_in_gpiolib) {
ret = gpio_request_on()...
}
Is my suggestion, then you can explicitly mark which GPIOs
shall have their IRQs implicitly translated to GPIOs and
requested.
Alas, this requires the DTS:es to have this hogging added, but
it is fully backwards-compatible.
Then the applicabl OMAP drivers (like MMC) can be fixed to
do gpio_to_irq() on their pins instead of using the IRQ
directly.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 04/26/2013 02:27 AM, Linus Walleij wrote:
On Wed, Apr 17, 2013 at 5:41 PM, Stephen Warren swar...@wwwdotorg.org wrote:
On 04/16/2013 05:14 PM, Jon Hunter wrote:
c) I have the feeling that hooking the of_xlate function for this is a
bit of an abuse of the function.
I was wondering about that. So I was grep'ing through the various xlate
implementations and found this [1]. Also you may recall that in the
of_dma_simple_xlate() we call the dma_request_channel() to allocate the
channel, which is very similar. However, I don't wish to get a
reputation as abusing APIs so would be good to know if this really is
abuse or not ;-)
Cheers
Jon
[1] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/195124
Interesting.
This is really something that the core DT and GPIO and IRQ maintainers
should weigh in on. Hence, changing them from Cc: to To: in this message
and/or adding them.
So I guess it is OK for a driver or DT node to use a GPIO as IRQ
*only*, and then have the GPIO requested implicitly through the
xlate function in the irqdomain.
It's the use cases where one and the same driver tries to use the
same GPIO line *also* by requesting it using gpio_request()
that madness starts. In such cases the driver deserves to have
an error thrown back at it, as it definately knows the GPIO pin
and can be refactored to use gpio_to_irq() to translate it into
an IRQ rather than having it implicitly done in the xlate function.
You will just have to give the xlate function information about which
GPIOs are used as IRQs only, and only request the GPIO on these.
That should not be necessary. The xlate is only called in the case where
the gpio controller is declared as the interrupt controller in the
device tree source ...
eth@0 {
compatible = ks8851;
...
interrupt-parent = gpio2;
interrupts = 2 8; /* gpio line 34, low triggered */
...
};
So in the case where you have a driver call gpio_request() and then
request_irq(), in the device tree source, the gpio controller will not
be declared as an interrupt controller. It will just list the gpios ...
sdhci@c8000200 {
...
cd-gpios = gpio 69 0; /* gpio PI5 */
wp-gpios = gpio 57 0; /* gpio PH1 */
...
};
And I recently suggested a mechanism to do that, and that is
what I called GPIO input-hogs, which means that you mark
(e.g. from the device tree) at probe() of the gpiochip which GPIOs
will only be used as inputs, so as to generate IRQs.
This is perfectly OS-neutral information about the how the
hardware is set up in the system.
If you only allow these hogges inputs to be translated and
requested in the xlate function, everything goes smooth.
It all comes back to this: keep all applicable knowledge in the
system, so it can make proper decisions, don't try to create
mechanisms that will half-guess things.
Agreed. However the xlate looks like a good place to request the gpio
without needing to add these input-hogs or any other platform code. So
if this is not considered abuse of the xlate, then it seems like an
ideal solution. I have been also implementing a generic xlate so that we
don't need to have a xlate for each platform as well.
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 04/26/2013 02:31 AM, Linus Walleij wrote:
On Wed, Apr 17, 2013 at 2:41 AM, Javier Martinez Canillas
martinez.jav...@gmail.com wrote:
So:
+static int omap_gpio_irq_domain_xlate(struct irq_domain *d,
+ struct device_node *ctrlr,
+ const u32 *intspec, unsigned int
intsize,
+ irq_hw_number_t *out_hwirq,
+ unsigned int *out_type)
+{
+ int ret;
+ struct gpio_bank *bank = d-host_data;
+ int gpio = bank-chip.base + intspec[0];
+
+ if (WARN_ON(intsize 2))
+ return -EINVAL;
+
+ ret = gpio_request_one(gpio, GPIOF_IN, ctrlr-full_name);
+ if (ret)
+ return ret;
So how to figure out if a device is already requesting this GPIO
on some orthogonal axis?
I really don't think that is necessary. Hopefully, my other email [1]
elaborates on why. Let me know if this makes sense.
Cheers
Jon
[1] http://marc.info/?l=linux-arm-kernelm=136701158117966w=1
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Wed, Apr 17, 2013 at 4:00 AM, Jon Hunter jon-hun...@ti.com wrote:
On 04/16/2013 07:41 PM, Javier Martinez Canillas wrote:
On Wed, Apr 17, 2013 at 1:14 AM, Jon Hunter jon-hun...@ti.com wrote:
On 04/16/2013 05:11 PM, Stephen Warren wrote:
On 04/16/2013 01:27 PM, Jon Hunter wrote:
On 04/16/2013 01:40 PM, Stephen Warren wrote:
On 04/15/2013 05:04 PM, Jon Hunter wrote:
...
If some driver is calling gpio_request() directly, then they will most
likely just call gpio_to_irq() when requesting the interrupt and so the
xlate function would not be called in this case (mmc drivers are a good
example). So I don't see that as being a problem. In fact that's the
benefit of this approach as AFAICT it solves this problem.
Oh. That assumption seems very fragile. What about drivers that actually
do have platform data (or DT bindings) that provide both the IRQ and
GPIO IDs, and hence don't use gpio_to_irq()? That's entirely possible.
Right. In the DT case though, if someone does provide the IRQ and GPIO
IDs then at least they would use a different xlate function. Another
option to consider would be defining the #interrupt-cells = 3 where we
would have ...
cell-#1 -- IRQ domain ID
cell-#2 -- Trigger type
cell-#3 -- GPIO ID
Then we could have a generic xlate for 3 cells that would also request
the GPIO. Again not sure if people are against a gpio being requested in
the xlate but just an idea. Or given that irq_of_parse_and_map() calls
the xlate, we could have this function call gpio_request() if the
interrupt controller is a gpio and there are 3 cells.
I rather dislike this approach since:
a) It requires changes to the DT bindings, which are already defined.
Admittedly it's backwards-compatible, but still.
b) There isn't really any need for the DT to represent this; the
GPIO+IRQ driver itself already knows which IRQ ID is which GPIO ID and
vice-versa (if the HW has such a concept), so there's no need for the DT
to contain this information. This seems like pushing Linux's internal
requirements into the design of the DT binding.
Yes, so the only alternative is to use irq_to_gpio to avoid this.
c) I have the feeling that hooking the of_xlate function for this is a
bit of an abuse of the function.
I was wondering about that. So I was grep'ing through the various xlate
implementations and found this [1]. Also you may recall that in the
of_dma_simple_xlate() we call the dma_request_channel() to allocate the
channel, which is very similar. However, I don't wish to get a
reputation as abusing APIs so would be good to know if this really is
abuse or not ;-)
Cheers
Jon
[1] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/195124
I was looking at [1] shared by Jon and come up with the following
patch that does something similar for OMAP GPIO. This has the
advantage that is local to gpio-omap instead changing gpiolib-of and
also doesn't require DT changes
But I don't want to get a reputation for abusing APIs neither :-)
Best regards,
Javier
From 23368eb72b125227fcf4b22be19ea70b4ab94556 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas javier.marti...@collabora.co.uk
Date: Wed, 17 Apr 2013 02:03:08 +0200
Subject: [PATCH 1/1] gpio/omap: add custom xlate function handler
Signed-off-by: Javier Martinez Canillas javier.marti...@collabora.co.uk
---
drivers/gpio/gpio-omap.c | 29 -
1 files changed, 28 insertions(+), 1 deletions(-)
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 8524ce5..77216f9 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1097,6 +1097,33 @@ static void omap_gpio_chip_init(struct gpio_bank
*bank)
static const struct of_device_id omap_gpio_match[];
static void omap_gpio_init_context(struct gpio_bank *p);
+static int omap_gpio_irq_domain_xlate(struct irq_domain *d,
+ struct device_node *ctrlr,
+ const u32 *intspec, unsigned int intsize,
+ irq_hw_number_t *out_hwirq,
+ unsigned int *out_type)
+{
+ int ret;
+ struct gpio_bank *bank = d-host_data;
+ int gpio = bank-chip.base + intspec[0];
+
+ if (WARN_ON(intsize 2))
+ return -EINVAL;
+
+ ret = gpio_request_one(gpio, GPIOF_IN, ctrlr-full_name);
+ if (ret)
+ return ret;
+
+ *out_hwirq = intspec[0];
+ *out_type = (intsize 1) ? intspec[1] : IRQ_TYPE_NONE;
+
+ return 0;
+}
+
+static struct irq_domain_ops omap_gpio_irq_ops = {
+ .xlate = omap_gpio_irq_domain_xlate,
+};
+
static int omap_gpio_probe(struct platform_device *pdev)
{
struct device *dev = pdev-dev;
@@ -1144,7 +1171,7 @@ static int omap_gpio_probe(struct platform_device
*pdev)
bank-domain = irq_domain_add_linear(node, bank-width,
- irq_domain_simple_ops,
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 04/17/2013 02:55 AM, Javier Martinez Canillas wrote: ... There are so many patches flying around in this thread that I missed it :-) Sorry about that... No problem. I was trying to see if we could find a common solution that everyone could use as it seems that ideally we should all be requesting the gpio. Cheers Jon [1] http://marc.info/?l=linux-arm-kernelm=136606204823845w=1 btw, I shared the latest patch with only build testing it, but today I gave a try and I found a problem with this approach. The .xlate function is being called twice for each GPIO-IRQ so the first time gpio_request_one() succeeds but the second time it fails returning -EBUSY. I tried it and I did not see that. I don't see the below warning either. This raises a warning on drivers/of/platform.c (WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq)): 0.285308] [ cut here ] [0.285369] WARNING: at drivers/of/platform.c:171 of_device_alloc+0x154/0x168() [0.285430] Modules linked in: [0.285491] [c001a944] (unwind_backtrace+0x0/0xf0) from [c0041edc] (warn_slowpath_common+0x4c/0x68) [0.285552] [c0041edc] (warn_slowpath_common+0x4c/0x68) from [c0041f14] (warn_slowpath_null+0x1c/0x24) [0.285614] [c0041f14] (warn_slowpath_null+0x1c/0x24) from [c041ac3c] (of_device_alloc+0x154/0x168) [0.285675] [c041ac3c] (of_device_alloc+0x154/0x168) from [c041ac84] (of_platform_device_create_pdata+0x34/0x80) [0.285736] [c041ac84] (of_platform_device_create_pdata+0x34/0x80) from [c0027364] (gpmc_probe_generic_child+0x180/0x240) [0.285827] [c0027364] (gpmc_probe_generic_child+0x180/0x240) from [c00278d8] (gpmc_probe+0x4b4/0x614) [0.285888] [c00278d8] (gpmc_probe+0x4b4/0x614) from [c0325514] (platform_drv_probe+0x18/0x1c) [0.285949] [c0325514] (platform_drv_probe+0x18/0x1c) from [c0324354] (driver_probe_device+0x108/0x21c) Any chance you have still have some additional code in your dts to request the gpio? I recall you made some hacks to make this work before. I probably won't have time to dig further on this until later this week but I wanted to share with you in case you know why is being calling twice and if you thought about a solution. Care to post your dts file? It works if I don't check the return gpio_request_one() (or better if we don't return on omap_gpio_irq_domain_xlate) but of course is not the right solution. Yes we need to check the return value. Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Wed, Apr 17, 2013 at 3:25 PM, Jon Hunter jon-hun...@ti.com wrote:
On 04/17/2013 02:55 AM, Javier Martinez Canillas wrote:
...
There are so many patches flying around in this thread that I missed it :-)
Sorry about that...
No problem.
I was trying to see if we could find a common solution that everyone
could use as it seems that ideally we should all be requesting the gpio.
Cheers
Jon
[1] http://marc.info/?l=linux-arm-kernelm=136606204823845w=1
btw, I shared the latest patch with only build testing it, but today I
gave a try and I found a problem with this approach. The .xlate
function is being called twice for each GPIO-IRQ so the first time
gpio_request_one() succeeds but the second time it fails returning
-EBUSY.
I tried it and I did not see that. I don't see the below warning either.
weird, I wonder what's different here.
I'll try tonight to test using another branch based on your
omap-daily-testing branch.
This raises a warning on drivers/of/platform.c
(WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq)):
0.285308] [ cut here ]
[0.285369] WARNING: at drivers/of/platform.c:171
of_device_alloc+0x154/0x168()
[0.285430] Modules linked in:
[0.285491] [c001a944] (unwind_backtrace+0x0/0xf0) from
[c0041edc] (warn_slowpath_common+0x4c/0x68)
[0.285552] [c0041edc] (warn_slowpath_common+0x4c/0x68) from
[c0041f14] (warn_slowpath_null+0x1c/0x24)
[0.285614] [c0041f14] (warn_slowpath_null+0x1c/0x24) from
[c041ac3c] (of_device_alloc+0x154/0x168)
[0.285675] [c041ac3c] (of_device_alloc+0x154/0x168) from
[c041ac84] (of_platform_device_create_pdata+0x34/0x80)
[0.285736] [c041ac84]
(of_platform_device_create_pdata+0x34/0x80) from [c0027364]
(gpmc_probe_generic_child+0x180/0x240)
[0.285827] [c0027364] (gpmc_probe_generic_child+0x180/0x240)
from [c00278d8] (gpmc_probe+0x4b4/0x614)
[0.285888] [c00278d8] (gpmc_probe+0x4b4/0x614) from [c0325514]
(platform_drv_probe+0x18/0x1c)
[0.285949] [c0325514] (platform_drv_probe+0x18/0x1c) from
[c0324354] (driver_probe_device+0x108/0x21c)
Any chance you have still have some additional code in your dts to
request the gpio? I recall you made some hacks to make this work before.
Yes, but I remove all those hacks from my DT and gpmc driver. Is the
first thing I thought and I already doble checked that.
I probably won't have time to dig further on this until later this
week but I wanted to share with you in case you know why is being
calling twice and if you thought about a solution.
Care to post your dts file?
I'm using the following patch to add smsc ethernet support to my board
+ adding 'ranges = 5 0 0x2c00 0x100;' to the gpmc device
node on omap3.dtsi:
From 4fe26a40229e6e97c2ab3b80865c9f24e8ff3424 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas javier.marti...@collabora.co.uk
Date: Wed, 27 Feb 2013 02:59:29 +0100
Subject: [PATCH 2/2] ARM: dts: omap3-igep0020: Add SMSC911x LAN chip support
The IGEPv2 board has an SMSC LAN9221i ethernet chip connected to
the OMAP3 processor though the General-Purpose Memory Controller.
This patch adds a device node for the ethernet chip as a GPMC child
and all its requirements (regulators, GPIO and pin muxs).
Signed-off-by: Javier Martinez Canillas javier.marti...@collabora.co.uk
---
arch/arm/boot/dts/omap3-igep.dtsi|6
arch/arm/boot/dts/omap3-igep0020.dts | 52 ++
2 files changed, 58 insertions(+), 0 deletions(-)
diff --git a/arch/arm/boot/dts/omap3-igep.dtsi
b/arch/arm/boot/dts/omap3-igep.dtsi
index f8fe3b7..d5cd504 100644
--- a/arch/arm/boot/dts/omap3-igep.dtsi
+++ b/arch/arm/boot/dts/omap3-igep.dtsi
@@ -62,6 +62,12 @@
0x126 0x0100/* sdmmc1_dat7.sdmmc1_dat7 INPUT | MODE
0 */
;
};
+
+ smsc911x_pins: pinmux_smsc911x_pins {
+ pinctrl-single,pins =
+0x1a2 0x0104/* mcspi1_cs2.gpio_176 INPUT | MODE4 */
+ ;
+ };
};
i2c1 {
diff --git a/arch/arm/boot/dts/omap3-igep0020.dts
b/arch/arm/boot/dts/omap3-igep0020.dts
index e2b9849..32a59df 100644
--- a/arch/arm/boot/dts/omap3-igep0020.dts
+++ b/arch/arm/boot/dts/omap3-igep0020.dts
@@ -40,6 +40,18 @@
gpios = twl_gpio 19 1;
};
};
+
+ vddvario: regulator-vddvario {
+ compatible = regulator-fixed;
+ regulator-name = vddvario;
+ regulator-always-on;
+ };
+
+ vdd33a: regulator-vdd33a {
+ compatible = regulator-fixed;
+ regulator-name = vdd33a;
+ regulator-always-on;
+ };
};
i2c3 {
@@ -54,3 +66,43 @@
reg = 0x50;
};
};
+
+gpmc {
+ ethernet@5,0 {
+ pinctrl-names = default;
+ pinctrl-0 = smsc911x_pins;
+ compatible = smsc,lan9221, smsc,lan9115;
+
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 04/17/2013 08:42 AM, Javier Martinez Canillas wrote: On Wed, Apr 17, 2013 at 3:25 PM, Jon Hunter jon-hun...@ti.com wrote: On 04/17/2013 02:55 AM, Javier Martinez Canillas wrote: ... There are so many patches flying around in this thread that I missed it :-) Sorry about that... No problem. I was trying to see if we could find a common solution that everyone could use as it seems that ideally we should all be requesting the gpio. Cheers Jon [1] http://marc.info/?l=linux-arm-kernelm=136606204823845w=1 btw, I shared the latest patch with only build testing it, but today I gave a try and I found a problem with this approach. The .xlate function is being called twice for each GPIO-IRQ so the first time gpio_request_one() succeeds but the second time it fails returning -EBUSY. I tried it and I did not see that. I don't see the below warning either. weird, I wonder what's different here. I am testing on an omap4-sdp which uses a spi based ethernet device. However, I could try with the omap3430-sdp which uses gpmc. Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Wed, Apr 17, 2013 at 3:52 PM, Jon Hunter jon-hun...@ti.com wrote: On 04/17/2013 08:42 AM, Javier Martinez Canillas wrote: On Wed, Apr 17, 2013 at 3:25 PM, Jon Hunter jon-hun...@ti.com wrote: On 04/17/2013 02:55 AM, Javier Martinez Canillas wrote: ... There are so many patches flying around in this thread that I missed it :-) Sorry about that... No problem. I was trying to see if we could find a common solution that everyone could use as it seems that ideally we should all be requesting the gpio. Cheers Jon [1] http://marc.info/?l=linux-arm-kernelm=136606204823845w=1 btw, I shared the latest patch with only build testing it, but today I gave a try and I found a problem with this approach. The .xlate function is being called twice for each GPIO-IRQ so the first time gpio_request_one() succeeds but the second time it fails returning -EBUSY. I tried it and I did not see that. I don't see the below warning either. weird, I wonder what's different here. I am testing on an omap4-sdp which uses a spi based ethernet device. However, I could try with the omap3430-sdp which uses gpmc. Thanks, that would be great and it could be a difference. But don't worry I'll to test it more extensively when I have some free time. I just shared here in case you had the same issue. Cheers Jon Thanks a lot for your help, Jaiver -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 04/16/2013 05:14 PM, Jon Hunter wrote: On 04/16/2013 05:11 PM, Stephen Warren wrote: On 04/16/2013 01:27 PM, Jon Hunter wrote: On 04/16/2013 01:40 PM, Stephen Warren wrote: On 04/15/2013 05:04 PM, Jon Hunter wrote: ... If some driver is calling gpio_request() directly, then they will most likely just call gpio_to_irq() when requesting the interrupt and so the xlate function would not be called in this case (mmc drivers are a good example). So I don't see that as being a problem. In fact that's the benefit of this approach as AFAICT it solves this problem. Oh. That assumption seems very fragile. What about drivers that actually do have platform data (or DT bindings) that provide both the IRQ and GPIO IDs, and hence don't use gpio_to_irq()? That's entirely possible. Right. In the DT case though, if someone does provide the IRQ and GPIO IDs then at least they would use a different xlate function. Another option to consider would be defining the #interrupt-cells = 3 where we would have ... cell-#1 -- IRQ domain ID cell-#2 -- Trigger type cell-#3 -- GPIO ID Then we could have a generic xlate for 3 cells that would also request the GPIO. Again not sure if people are against a gpio being requested in the xlate but just an idea. Or given that irq_of_parse_and_map() calls the xlate, we could have this function call gpio_request() if the interrupt controller is a gpio and there are 3 cells. I rather dislike this approach since: a) It requires changes to the DT bindings, which are already defined. Admittedly it's backwards-compatible, but still. b) There isn't really any need for the DT to represent this; the GPIO+IRQ driver itself already knows which IRQ ID is which GPIO ID and vice-versa (if the HW has such a concept), so there's no need for the DT to contain this information. This seems like pushing Linux's internal requirements into the design of the DT binding. Yes, so the only alternative is to use irq_to_gpio to avoid this. c) I have the feeling that hooking the of_xlate function for this is a bit of an abuse of the function. I was wondering about that. So I was grep'ing through the various xlate implementations and found this [1]. Also you may recall that in the of_dma_simple_xlate() we call the dma_request_channel() to allocate the channel, which is very similar. However, I don't wish to get a reputation as abusing APIs so would be good to know if this really is abuse or not ;-) Cheers Jon [1] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/195124 Interesting. This is really something that the core DT and GPIO and IRQ maintainers should weigh in on. Hence, changing them from Cc: to To: in this message and/or adding them. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Wed, Apr 17, 2013 at 3:52 PM, Jon Hunter jon-hun...@ti.com wrote: On 04/17/2013 08:42 AM, Javier Martinez Canillas wrote: On Wed, Apr 17, 2013 at 3:25 PM, Jon Hunter jon-hun...@ti.com wrote: On 04/17/2013 02:55 AM, Javier Martinez Canillas wrote: ... There are so many patches flying around in this thread that I missed it :-) Sorry about that... No problem. I was trying to see if we could find a common solution that everyone could use as it seems that ideally we should all be requesting the gpio. Cheers Jon [1] http://marc.info/?l=linux-arm-kernelm=136606204823845w=1 btw, I shared the latest patch with only build testing it, but today I gave a try and I found a problem with this approach. The .xlate function is being called twice for each GPIO-IRQ so the first time gpio_request_one() succeeds but the second time it fails returning -EBUSY. I tried it and I did not see that. I don't see the below warning either. weird, I wonder what's different here. I am testing on an omap4-sdp which uses a spi based ethernet device. However, I could try with the omap3430-sdp which uses gpmc. Cheers Jon Hi Jon, I just created a new branch using your omap-daily-testing as a base and cherry-picked all the required patches and the .xlate function is working correctly. I don't see the WARN anymore neither is called twice. Sorry for the noise.. Best regards, Javier -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 04/15/2013 05:04 PM, Jon Hunter wrote:
On 04/15/2013 05:16 PM, Stephen Warren wrote:
On 04/15/2013 03:40 PM, Jon Hunter wrote:
...
mmc {
label = pandaboard::status2;
gpios = gpio1 8 0;
...
};
But that's a gpio-leds instance, not an MMC controller... I really
really hope there's no DT node using gpios to mean interrupts... And
it wouldn't make any sense for an on-SoC device anyway.
Oops yes, I overlooked that. However, the omap mmc driver
(drivers/mmc/host/omap_hsmmc.c) does call gpio_request() and
request_threaded_irq() for the mmc card-detect interrupt. I believe
tegra is doing the same ...
sdhci@7800 {
...
cd-gpios = gpio 69 0; /* gpio PI5 */
...
};
Ah true. I guess at least all MMC drivers are likely hooking cd-gpios as
an interrupt, /and/ requesting it as a GPIO so that they can read the
current state when the GPIO goes off.
That tends to imply that no core code can possibly call gpio_request()
in response to request_irq(), since doing so likely will conflict with
quite a few drivers...
Both these devices are using a gpio as an interrupt source, but the mmc
driver is requesting the gpio directly. In the first case the xlate
function for the gpio irq domain will be called where as it is not used
in the 2nd case. Therefore, we could add a custom xlate function. For
example ...
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
+int omap_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr,
...
+ gpio_request_one(irq_to_gpio(bank, intspec[0]), GPIOF_IN,
ctrlr-name);
I guess that could work, but:
a) It still means doing the gpio_request() in each driver instead of
centrally.
Right this is device specific, but it avoids exposing irq_to_gpio for a
device. However, we could make this generic if we did expose irq_to_gpio
for each device.
b) This approach doesn't solve the issue where some client driver has
already requested the GPIO. This code would simply prevent that call
from succeeding, which would probably make the driver probe() error out,
which isn't any different to the equivalent proposed centralized
gpio_request() inside some request_irq() failing, and causing the
device's probe() to error out.
If some driver is calling gpio_request() directly, then they will most
likely just call gpio_to_irq() when requesting the interrupt and so the
xlate function would not be called in this case (mmc drivers are a good
example). So I don't see that as being a problem. In fact that's the
benefit of this approach as AFAICT it solves this problem.
Oh. That assumption seems very fragile. What about drivers that actually
do have platform data (or DT bindings) that provide both the IRQ and
GPIO IDs, and hence don't use gpio_to_irq()? That's entirely possible.
Given all this, I guess simply having each GPIO+IRQ driver's set_type
function simply do whatever is required in HW to set up that GPIO
actually does seem like the best idea. Admittedly that isn't
centralized, but I'm not sure now that any centralized implementation is
possible, without significant rework of a bunch of drivers. This is what
the Tegra GPIO driver already does, and I think one of the earlier
patches in this thread did exactly that for OMAP IRQs too right?
BTW, just so you know, I'm on vacation for 2 weeks starting Wed
afternoon, so replies will be non-existent or spotty during that time.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 04/16/2013 01:40 PM, Stephen Warren wrote:
On 04/15/2013 05:04 PM, Jon Hunter wrote:
On 04/15/2013 05:16 PM, Stephen Warren wrote:
On 04/15/2013 03:40 PM, Jon Hunter wrote:
...
mmc {
label = pandaboard::status2;
gpios = gpio1 8 0;
...
};
But that's a gpio-leds instance, not an MMC controller... I really
really hope there's no DT node using gpios to mean interrupts... And
it wouldn't make any sense for an on-SoC device anyway.
Oops yes, I overlooked that. However, the omap mmc driver
(drivers/mmc/host/omap_hsmmc.c) does call gpio_request() and
request_threaded_irq() for the mmc card-detect interrupt. I believe
tegra is doing the same ...
sdhci@7800 {
...
cd-gpios = gpio 69 0; /* gpio PI5 */
...
};
Ah true. I guess at least all MMC drivers are likely hooking cd-gpios as
an interrupt, /and/ requesting it as a GPIO so that they can read the
current state when the GPIO goes off.
That tends to imply that no core code can possibly call gpio_request()
in response to request_irq(), since doing so likely will conflict with
quite a few drivers...
Yes that was my concern.
Both these devices are using a gpio as an interrupt source, but the mmc
driver is requesting the gpio directly. In the first case the xlate
function for the gpio irq domain will be called where as it is not used
in the 2nd case. Therefore, we could add a custom xlate function. For
example ...
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
+int omap_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr,
...
+ gpio_request_one(irq_to_gpio(bank, intspec[0]), GPIOF_IN,
ctrlr-name);
I guess that could work, but:
a) It still means doing the gpio_request() in each driver instead of
centrally.
Right this is device specific, but it avoids exposing irq_to_gpio for a
device. However, we could make this generic if we did expose irq_to_gpio
for each device.
b) This approach doesn't solve the issue where some client driver has
already requested the GPIO. This code would simply prevent that call
from succeeding, which would probably make the driver probe() error out,
which isn't any different to the equivalent proposed centralized
gpio_request() inside some request_irq() failing, and causing the
device's probe() to error out.
If some driver is calling gpio_request() directly, then they will most
likely just call gpio_to_irq() when requesting the interrupt and so the
xlate function would not be called in this case (mmc drivers are a good
example). So I don't see that as being a problem. In fact that's the
benefit of this approach as AFAICT it solves this problem.
Oh. That assumption seems very fragile. What about drivers that actually
do have platform data (or DT bindings) that provide both the IRQ and
GPIO IDs, and hence don't use gpio_to_irq()? That's entirely possible.
Right. In the DT case though, if someone does provide the IRQ and GPIO
IDs then at least they would use a different xlate function. Another
option to consider would be defining the #interrupt-cells = 3 where we
would have ...
cell-#1 -- IRQ domain ID
cell-#2 -- Trigger type
cell-#3 -- GPIO ID
Then we could have a generic xlate for 3 cells that would also request
the GPIO. Again not sure if people are against a gpio being requested in
the xlate but just an idea. Or given that irq_of_parse_and_map() calls
the xlate, we could have this function call gpio_request() if the
interrupt controller is a gpio and there are 3 cells.
Given all this, I guess simply having each GPIO+IRQ driver's set_type
function simply do whatever is required in HW to set up that GPIO
actually does seem like the best idea. Admittedly that isn't
centralized, but I'm not sure now that any centralized implementation is
possible, without significant rework of a bunch of drivers. This is what
the Tegra GPIO driver already does, and I think one of the earlier
patches in this thread did exactly that for OMAP IRQs too right?
Yes, however, Linus wanted us to make sure the gpio is requested which
is why we have not taken that patch. However, if we cannot find a better
alternative may be we have to do that for now.
BTW, just so you know, I'm on vacation for 2 weeks starting Wed
afternoon, so replies will be non-existent or spotty during that time.
Thanks!
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 04/16/2013 02:27 PM, Jon Hunter wrote:
...
Right. In the DT case though, if someone does provide the IRQ and GPIO
IDs then at least they would use a different xlate function. Another
option to consider would be defining the #interrupt-cells = 3 where we
would have ...
cell-#1 -- IRQ domain ID
cell-#2 -- Trigger type
cell-#3 -- GPIO ID
Then we could have a generic xlate for 3 cells that would also request
the GPIO. Again not sure if people are against a gpio being requested in
the xlate but just an idea. Or given that irq_of_parse_and_map() calls
the xlate, we could have this function call gpio_request() if the
interrupt controller is a gpio and there are 3 cells.
So basically some like the following (note I have only included the omap
portion in here for an example). The other benefit being no need to
re-introduce irq_to_gpio.
Jon
From f01ce047075a922969fcbe904b339fe8d03a997b Mon Sep 17 00:00:00 2001
From: Jon Hunter jon-hun...@ti.com
Date: Tue, 16 Apr 2013 09:54:28 -0500
Subject: [PATCH] gpio: add custom xlate for gpio irqs
---
drivers/gpio/gpio-omap.c |3 ++-
drivers/gpio/gpiolib-of.c | 24
include/linux/of_gpio.h |1 +
3 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index d337318..b3d7d43 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -24,6 +24,7 @@
#include linux/pm.h
#include linux/of.h
#include linux/of_device.h
+#include linux/of_gpio.h
#include linux/irqdomain.h
#include linux/gpio.h
#include linux/platform_data/gpio-omap.h
@@ -1135,7 +1136,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
bank-domain = irq_domain_add_linear(node, bank-width,
-irq_domain_simple_ops, NULL);
+of_gpio_irq_domain_ops, NULL);
if (!bank-domain)
return -ENODEV;
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 5150df6..fc01ec1 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -15,6 +15,8 @@
#include linux/errno.h
#include linux/module.h
#include linux/io.h
+#include linux/irq.h
+#include linux/irqdomain.h
#include linux/gpio.h
#include linux/of.h
#include linux/of_address.h
@@ -253,3 +255,25 @@ void of_gpiochip_remove(struct gpio_chip *chip)
if (chip-of_node)
of_node_put(chip-of_node);
}
+
+int of_gpio_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr,
+const u32 *intspec, unsigned int intsize,
+unsigned long *out_hwirq, unsigned int *out_type)
+{
+ int ret;
+
+ if (WARN_ON(intsize 3))
+ return -EINVAL;
+
+ ret = gpio_request_one(intspec[2], GPIOF_IN, NULL);
+ if (ret)
+ return ret;
+
+ *out_hwirq = intspec[0];
+ *out_type = (intsize 1) ? intspec[1] : IRQ_TYPE_NONE;
+ return 0;
+}
+
+const struct irq_domain_ops of_gpio_irq_domain_ops = {
+ .xlate = of_gpio_irq_domain_xlate,
+};
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index a83dc6f..ad2b962 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -58,6 +58,7 @@ extern void of_gpiochip_remove(struct gpio_chip *gc);
extern int of_gpio_simple_xlate(struct gpio_chip *gc,
const struct of_phandle_args *gpiospec,
u32 *flags);
+extern const struct irq_domain_ops of_gpio_irq_domain_ops;
#else /* CONFIG_OF_GPIO */
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 04/16/2013 01:27 PM, Jon Hunter wrote: On 04/16/2013 01:40 PM, Stephen Warren wrote: On 04/15/2013 05:04 PM, Jon Hunter wrote: ... If some driver is calling gpio_request() directly, then they will most likely just call gpio_to_irq() when requesting the interrupt and so the xlate function would not be called in this case (mmc drivers are a good example). So I don't see that as being a problem. In fact that's the benefit of this approach as AFAICT it solves this problem. Oh. That assumption seems very fragile. What about drivers that actually do have platform data (or DT bindings) that provide both the IRQ and GPIO IDs, and hence don't use gpio_to_irq()? That's entirely possible. Right. In the DT case though, if someone does provide the IRQ and GPIO IDs then at least they would use a different xlate function. Another option to consider would be defining the #interrupt-cells = 3 where we would have ... cell-#1 -- IRQ domain ID cell-#2 -- Trigger type cell-#3 -- GPIO ID Then we could have a generic xlate for 3 cells that would also request the GPIO. Again not sure if people are against a gpio being requested in the xlate but just an idea. Or given that irq_of_parse_and_map() calls the xlate, we could have this function call gpio_request() if the interrupt controller is a gpio and there are 3 cells. I rather dislike this approach since: a) It requires changes to the DT bindings, which are already defined. Admittedly it's backwards-compatible, but still. b) There isn't really any need for the DT to represent this; the GPIO+IRQ driver itself already knows which IRQ ID is which GPIO ID and vice-versa (if the HW has such a concept), so there's no need for the DT to contain this information. This seems like pushing Linux's internal requirements into the design of the DT binding. c) I have the feeling that hooking the of_xlate function for this is a bit of an abuse of the function. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 04/16/2013 05:11 PM, Stephen Warren wrote: On 04/16/2013 01:27 PM, Jon Hunter wrote: On 04/16/2013 01:40 PM, Stephen Warren wrote: On 04/15/2013 05:04 PM, Jon Hunter wrote: ... If some driver is calling gpio_request() directly, then they will most likely just call gpio_to_irq() when requesting the interrupt and so the xlate function would not be called in this case (mmc drivers are a good example). So I don't see that as being a problem. In fact that's the benefit of this approach as AFAICT it solves this problem. Oh. That assumption seems very fragile. What about drivers that actually do have platform data (or DT bindings) that provide both the IRQ and GPIO IDs, and hence don't use gpio_to_irq()? That's entirely possible. Right. In the DT case though, if someone does provide the IRQ and GPIO IDs then at least they would use a different xlate function. Another option to consider would be defining the #interrupt-cells = 3 where we would have ... cell-#1 -- IRQ domain ID cell-#2 -- Trigger type cell-#3 -- GPIO ID Then we could have a generic xlate for 3 cells that would also request the GPIO. Again not sure if people are against a gpio being requested in the xlate but just an idea. Or given that irq_of_parse_and_map() calls the xlate, we could have this function call gpio_request() if the interrupt controller is a gpio and there are 3 cells. I rather dislike this approach since: a) It requires changes to the DT bindings, which are already defined. Admittedly it's backwards-compatible, but still. b) There isn't really any need for the DT to represent this; the GPIO+IRQ driver itself already knows which IRQ ID is which GPIO ID and vice-versa (if the HW has such a concept), so there's no need for the DT to contain this information. This seems like pushing Linux's internal requirements into the design of the DT binding. Yes, so the only alternative is to use irq_to_gpio to avoid this. c) I have the feeling that hooking the of_xlate function for this is a bit of an abuse of the function. I was wondering about that. So I was grep'ing through the various xlate implementations and found this [1]. Also you may recall that in the of_dma_simple_xlate() we call the dma_request_channel() to allocate the channel, which is very similar. However, I don't wish to get a reputation as abusing APIs so would be good to know if this really is abuse or not ;-) Cheers Jon [1] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/195124 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Wed, Apr 17, 2013 at 1:14 AM, Jon Hunter jon-hun...@ti.com wrote:
On 04/16/2013 05:11 PM, Stephen Warren wrote:
On 04/16/2013 01:27 PM, Jon Hunter wrote:
On 04/16/2013 01:40 PM, Stephen Warren wrote:
On 04/15/2013 05:04 PM, Jon Hunter wrote:
...
If some driver is calling gpio_request() directly, then they will most
likely just call gpio_to_irq() when requesting the interrupt and so the
xlate function would not be called in this case (mmc drivers are a good
example). So I don't see that as being a problem. In fact that's the
benefit of this approach as AFAICT it solves this problem.
Oh. That assumption seems very fragile. What about drivers that actually
do have platform data (or DT bindings) that provide both the IRQ and
GPIO IDs, and hence don't use gpio_to_irq()? That's entirely possible.
Right. In the DT case though, if someone does provide the IRQ and GPIO
IDs then at least they would use a different xlate function. Another
option to consider would be defining the #interrupt-cells = 3 where we
would have ...
cell-#1 -- IRQ domain ID
cell-#2 -- Trigger type
cell-#3 -- GPIO ID
Then we could have a generic xlate for 3 cells that would also request
the GPIO. Again not sure if people are against a gpio being requested in
the xlate but just an idea. Or given that irq_of_parse_and_map() calls
the xlate, we could have this function call gpio_request() if the
interrupt controller is a gpio and there are 3 cells.
I rather dislike this approach since:
a) It requires changes to the DT bindings, which are already defined.
Admittedly it's backwards-compatible, but still.
b) There isn't really any need for the DT to represent this; the
GPIO+IRQ driver itself already knows which IRQ ID is which GPIO ID and
vice-versa (if the HW has such a concept), so there's no need for the DT
to contain this information. This seems like pushing Linux's internal
requirements into the design of the DT binding.
Yes, so the only alternative is to use irq_to_gpio to avoid this.
c) I have the feeling that hooking the of_xlate function for this is a
bit of an abuse of the function.
I was wondering about that. So I was grep'ing through the various xlate
implementations and found this [1]. Also you may recall that in the
of_dma_simple_xlate() we call the dma_request_channel() to allocate the
channel, which is very similar. However, I don't wish to get a
reputation as abusing APIs so would be good to know if this really is
abuse or not ;-)
Cheers
Jon
[1] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/195124
I was looking at [1] shared by Jon and come up with the following
patch that does something similar for OMAP GPIO. This has the
advantage that is local to gpio-omap instead changing gpiolib-of and
also doesn't require DT changes
But I don't want to get a reputation for abusing APIs neither :-)
Best regards,
Javier
From 23368eb72b125227fcf4b22be19ea70b4ab94556 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas javier.marti...@collabora.co.uk
Date: Wed, 17 Apr 2013 02:03:08 +0200
Subject: [PATCH 1/1] gpio/omap: add custom xlate function handler
Signed-off-by: Javier Martinez Canillas javier.marti...@collabora.co.uk
---
drivers/gpio/gpio-omap.c | 29 -
1 files changed, 28 insertions(+), 1 deletions(-)
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 8524ce5..77216f9 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1097,6 +1097,33 @@ static void omap_gpio_chip_init(struct gpio_bank *bank)
static const struct of_device_id omap_gpio_match[];
static void omap_gpio_init_context(struct gpio_bank *p);
+static int omap_gpio_irq_domain_xlate(struct irq_domain *d,
+ struct device_node *ctrlr,
+ const u32 *intspec, unsigned int intsize,
+ irq_hw_number_t *out_hwirq,
+ unsigned int *out_type)
+{
+ int ret;
+ struct gpio_bank *bank = d-host_data;
+ int gpio = bank-chip.base + intspec[0];
+
+ if (WARN_ON(intsize 2))
+ return -EINVAL;
+
+ ret = gpio_request_one(gpio, GPIOF_IN, ctrlr-full_name);
+ if (ret)
+ return ret;
+
+ *out_hwirq = intspec[0];
+ *out_type = (intsize 1) ? intspec[1] : IRQ_TYPE_NONE;
+
+ return 0;
+}
+
+static struct irq_domain_ops omap_gpio_irq_ops = {
+ .xlate = omap_gpio_irq_domain_xlate,
+};
+
static int omap_gpio_probe(struct platform_device *pdev)
{
struct device *dev = pdev-dev;
@@ -1144,7 +1171,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
bank-domain = irq_domain_add_linear(node, bank-width,
-irq_domain_simple_ops, NULL);
+omap_gpio_irq_ops, bank);
if (!bank-domain)
return -ENODEV;
--
1.7.7.6
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 04/16/2013 07:41 PM, Javier Martinez Canillas wrote:
On Wed, Apr 17, 2013 at 1:14 AM, Jon Hunter jon-hun...@ti.com wrote:
On 04/16/2013 05:11 PM, Stephen Warren wrote:
On 04/16/2013 01:27 PM, Jon Hunter wrote:
On 04/16/2013 01:40 PM, Stephen Warren wrote:
On 04/15/2013 05:04 PM, Jon Hunter wrote:
...
If some driver is calling gpio_request() directly, then they will most
likely just call gpio_to_irq() when requesting the interrupt and so the
xlate function would not be called in this case (mmc drivers are a good
example). So I don't see that as being a problem. In fact that's the
benefit of this approach as AFAICT it solves this problem.
Oh. That assumption seems very fragile. What about drivers that actually
do have platform data (or DT bindings) that provide both the IRQ and
GPIO IDs, and hence don't use gpio_to_irq()? That's entirely possible.
Right. In the DT case though, if someone does provide the IRQ and GPIO
IDs then at least they would use a different xlate function. Another
option to consider would be defining the #interrupt-cells = 3 where we
would have ...
cell-#1 -- IRQ domain ID
cell-#2 -- Trigger type
cell-#3 -- GPIO ID
Then we could have a generic xlate for 3 cells that would also request
the GPIO. Again not sure if people are against a gpio being requested in
the xlate but just an idea. Or given that irq_of_parse_and_map() calls
the xlate, we could have this function call gpio_request() if the
interrupt controller is a gpio and there are 3 cells.
I rather dislike this approach since:
a) It requires changes to the DT bindings, which are already defined.
Admittedly it's backwards-compatible, but still.
b) There isn't really any need for the DT to represent this; the
GPIO+IRQ driver itself already knows which IRQ ID is which GPIO ID and
vice-versa (if the HW has such a concept), so there's no need for the DT
to contain this information. This seems like pushing Linux's internal
requirements into the design of the DT binding.
Yes, so the only alternative is to use irq_to_gpio to avoid this.
c) I have the feeling that hooking the of_xlate function for this is a
bit of an abuse of the function.
I was wondering about that. So I was grep'ing through the various xlate
implementations and found this [1]. Also you may recall that in the
of_dma_simple_xlate() we call the dma_request_channel() to allocate the
channel, which is very similar. However, I don't wish to get a
reputation as abusing APIs so would be good to know if this really is
abuse or not ;-)
Cheers
Jon
[1] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/195124
I was looking at [1] shared by Jon and come up with the following
patch that does something similar for OMAP GPIO. This has the
advantage that is local to gpio-omap instead changing gpiolib-of and
also doesn't require DT changes
But I don't want to get a reputation for abusing APIs neither :-)
Best regards,
Javier
From 23368eb72b125227fcf4b22be19ea70b4ab94556 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas javier.marti...@collabora.co.uk
Date: Wed, 17 Apr 2013 02:03:08 +0200
Subject: [PATCH 1/1] gpio/omap: add custom xlate function handler
Signed-off-by: Javier Martinez Canillas javier.marti...@collabora.co.uk
---
drivers/gpio/gpio-omap.c | 29 -
1 files changed, 28 insertions(+), 1 deletions(-)
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 8524ce5..77216f9 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1097,6 +1097,33 @@ static void omap_gpio_chip_init(struct gpio_bank *bank)
static const struct of_device_id omap_gpio_match[];
static void omap_gpio_init_context(struct gpio_bank *p);
+static int omap_gpio_irq_domain_xlate(struct irq_domain *d,
+ struct device_node *ctrlr,
+ const u32 *intspec, unsigned int intsize,
+ irq_hw_number_t *out_hwirq,
+ unsigned int *out_type)
+{
+ int ret;
+ struct gpio_bank *bank = d-host_data;
+ int gpio = bank-chip.base + intspec[0];
+
+ if (WARN_ON(intsize 2))
+ return -EINVAL;
+
+ ret = gpio_request_one(gpio, GPIOF_IN, ctrlr-full_name);
+ if (ret)
+ return ret;
+
+ *out_hwirq = intspec[0];
+ *out_type = (intsize 1) ? intspec[1] : IRQ_TYPE_NONE;
+
+ return 0;
+}
+
+static struct irq_domain_ops omap_gpio_irq_ops = {
+ .xlate = omap_gpio_irq_domain_xlate,
+};
+
static int omap_gpio_probe(struct platform_device *pdev)
{
struct device *dev = pdev-dev;
@@ -1144,7 +1171,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
bank-domain = irq_domain_add_linear(node, bank-width,
- irq_domain_simple_ops, NULL);
+
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Sun, Apr 14, 2013 at 10:53 PM, Linus Walleij
linus.wall...@linaro.org wrote:
On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas
martinez.jav...@gmail.com wrote:
Is the following inlined patch [1] what you were thinking that would
be the right approach?
Hi Linus, thanks a lot for your feedback.
This looks sort of OK, but I'm still struggling with the question of
what we could do to help other implementations facing the same issue.
Yes, I don't know how we can make it easier to other implementations
(besides adding the irq_request hook to irq_chip) since the logic is
GPIO controller specific.
For example I took a look to drivers/gpio/gpio-tegra.c and if I
understood correctly, the implementation for this driver should be
something like this:
diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
index 414ad91..a2d5c3d 100644
--- a/drivers/gpio/gpio-tegra.c
+++ b/drivers/gpio/gpio-tegra.c
@@ -346,6 +346,11 @@ static int tegra_gpio_wake_enable(struct irq_data
*d, unsigned int enable)
}
#endif
+static int tegra_gpio_irq_request(struct irq_data *d)
+{
+ tegra_gpio_request(NULL, d-hwirq);
+}
+
static struct irq_chip tegra_gpio_irq_chip = {
.name = GPIO,
.irq_ack= tegra_gpio_irq_ack,
@@ -355,6 +360,7 @@ static struct irq_chip tegra_gpio_irq_chip = {
#ifdef CONFIG_PM_SLEEP
.irq_set_wake = tegra_gpio_wake_enable,
#endif
+ .irq_request= tegra_gpio_irq_request,
};
static const struct dev_pm_ops tegra_gpio_pm_ops = {
This is definitely quite similar to the omap-gpio.c change but not the same.
This is a pretty hard design pattern to properly replicate in every such
driver is it not?
H
Is indeed a hard design pattern but I couldn't figure out a more
generic way to handle this.
Maybe we could use [1] for now to solve the issue that we have with
OMAP GPIO and later when the same issue is found on other GPIO
controllers and a similar change is made on these drivers we will see
some form of pattern that emerge allowing us to make a later refactor
to reduce the code duplication.
Or do you have a better idea on how to solve this?
Yours,
Linus Walleij
Best regards,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 04/13/2013 07:35 PM, Javier Martinez Canillas wrote:
...
Is the following inlined patch [1] what you were thinking that would
be the right approach?
...
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
...
+static int omap_gpio_irq_request(struct irq_data *d)
+{
+ struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+
+ return omap_gpio_request(bank-chip, d-hwirq);
If you want the GPIO usage to be known to the GPIO subsystem, then
wouldn't you call gpio_request() here rather than omap_gpio_request()?
The above code will certainly do enough so that the OMAP GPIO HW is
fully enabled as you need, but I thought the idea was to also prevent
some other code successfully running gpio_request() on that same GPIO?
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 04/14/2013 02:53 PM, Linus Walleij wrote: On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas martinez.jav...@gmail.com wrote: Is the following inlined patch [1] what you were thinking that would be the right approach? This looks sort of OK, but I'm still struggling with the question of what we could do to help other implementations facing the same issue. This is a pretty hard design pattern to properly replicate in every such driver is it not? Well, instead of adding .request_irq() to the irqchip, and then making each driver call gpio_request() from the implementation, perhaps you could add a .irq_to_gpio() to the irqchip, have the IRQ core call that, and if it gets back a non-error response, the IRQ core could call gpio_request(). That means that the change to each GPIO+IRQ driver is simply to implement a standalone data transformation function .irq_to_gpio(). Now, this does re-introduce irq_to_gpio() in some way, but with the following advantages: 1) The implementation is per-controller, not a single global function, so isn't introducing any kind of centralized mapping scheme again. 2) This irq-chip-specific .irq_to_gpio() would only be implemented for IRQ+GPIO chips that actually have a 1:1 mapping between GPIOs and IRQs. Its potential existence doesn't imply that all IRQ chips need implement this; it would be very specifically be for this one particular case. So, I think it's reasonable to introduce this. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 04/15/2013 11:53 AM, Stephen Warren wrote:
On 04/13/2013 07:35 PM, Javier Martinez Canillas wrote:
...
Is the following inlined patch [1] what you were thinking that would
be the right approach?
...
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
...
+static int omap_gpio_irq_request(struct irq_data *d)
+{
+struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+
+return omap_gpio_request(bank-chip, d-hwirq);
If you want the GPIO usage to be known to the GPIO subsystem, then
wouldn't you call gpio_request() here rather than omap_gpio_request()?
The above code will certainly do enough so that the OMAP GPIO HW is
fully enabled as you need, but I thought the idea was to also prevent
some other code successfully running gpio_request() on that same GPIO?
Also, although omap gpios default to being inputs, we should not assume
that. So may be you should call gpio_request_one() here passing as flags
GPIOF_IN to configure as an input.
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 04/15/2013 11:58 AM, Stephen Warren wrote:
On 04/14/2013 02:53 PM, Linus Walleij wrote:
On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas
martinez.jav...@gmail.com wrote:
Is the following inlined patch [1] what you were thinking that would
be the right approach?
This looks sort of OK, but I'm still struggling with the question of
what we could do to help other implementations facing the same issue.
This is a pretty hard design pattern to properly replicate in every such
driver is it not?
Well, instead of adding .request_irq() to the irqchip, and then making
each driver call gpio_request() from the implementation, perhaps you
could add a .irq_to_gpio() to the irqchip, have the IRQ core call that,
and if it gets back a non-error response, the IRQ core could call
gpio_request(). That means that the change to each GPIO+IRQ driver is
simply to implement a standalone data transformation function
.irq_to_gpio().
I am still concerned about the case where a driver may have already
called gpio_request() and then calls request_irq(). I think that the
solution needs to handle cases where the driver may or may not call
gpio_request() to allocate the gpio.
Although it could be argued that this is problem is not DT specific,
it does become a bigger problem to handle in the case of DT. Therefore,
I am wondering if we should just focus on the DT case for now.
Now, this does re-introduce irq_to_gpio() in some way, but with the
following advantages:
1) The implementation is per-controller, not a single global function,
so isn't introducing any kind of centralized mapping scheme again.
2) This irq-chip-specific .irq_to_gpio() would only be implemented for
IRQ+GPIO chips that actually have a 1:1 mapping between GPIOs and IRQs.
Its potential existence doesn't imply that all IRQ chips need implement
this; it would be very specifically be for this one particular case.
So, I think it's reasonable to introduce this.
How about using the gpio irq domain xlate function?
Typically, in DT land a device using a gpio as an interrupt source
will have something like the following ...
eth@0 {
compatible = ks8851;
...
interrupt-parent = gpio2;
interrupts = 2 8; /* gpio line 34, low triggered */
};
... or ...
mmc {
label = pandaboard::status2;
gpios = gpio1 8 0;
...
};
Both these devices are using a gpio as an interrupt source, but the mmc
driver is requesting the gpio directly. In the first case the xlate
function for the gpio irq domain will be called where as it is not used
in the 2nd case. Therefore, we could add a custom xlate function. For
example ...
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 53bb8d5..caaeab2 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1085,6 +1085,33 @@ static void omap_gpio_chip_init(struct gpio_bank *bank)
irq_set_handler_data(bank-irq, bank);
}
+
+int omap_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr,
+ const u32 *intspec, unsigned int intsize,
+ unsigned long *out_hwirq, unsigned int *out_type)
+{
+ struct gpio_bank *bank;
+ int irq;
+
+ if (WARN_ON(intsize 1))
+ return -EINVAL;
+
+ irq = irq_find_mapping(d, intspec[0]);
+ bank = irq_get_chip_data(irq);
+ if (!bank)
+ return -ENODEV;
+
+ gpio_request_one(irq_to_gpio(bank, intspec[0]), GPIOF_IN, ctrlr-name);
+
+ *out_hwirq = intspec[0];
+ *out_type = (intsize 1) ? intspec[1] : IRQ_TYPE_NONE;
+ return 0;
+}
+
+static const struct irq_domain_ops omap_irq_domain_ops = {
+ .xlate = omap_irq_domain_xlate,
+};
+
static const struct of_device_id omap_gpio_match[];
static void omap_gpio_setup_context(struct gpio_bank *p);
@@ -1135,7 +1162,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
bank-domain = irq_domain_add_linear(node, bank-width,
-irq_domain_simple_ops, NULL);
+omap_irq_domain_ops, NULL);
if (!bank-domain)
return -ENODEV;
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 04/15/2013 04:40 PM, Jon Hunter wrote:
On 04/15/2013 11:58 AM, Stephen Warren wrote:
On 04/14/2013 02:53 PM, Linus Walleij wrote:
On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas
martinez.jav...@gmail.com wrote:
Is the following inlined patch [1] what you were thinking that would
be the right approach?
This looks sort of OK, but I'm still struggling with the question of
what we could do to help other implementations facing the same issue.
This is a pretty hard design pattern to properly replicate in every such
driver is it not?
Well, instead of adding .request_irq() to the irqchip, and then making
each driver call gpio_request() from the implementation, perhaps you
could add a .irq_to_gpio() to the irqchip, have the IRQ core call that,
and if it gets back a non-error response, the IRQ core could call
gpio_request(). That means that the change to each GPIO+IRQ driver is
simply to implement a standalone data transformation function
.irq_to_gpio().
I am still concerned about the case where a driver may have already
called gpio_request() and then calls request_irq(). I think that the
solution needs to handle cases where the driver may or may not call
gpio_request() to allocate the gpio.
Although it could be argued that this is problem is not DT specific,
it does become a bigger problem to handle in the case of DT. Therefore,
I am wondering if we should just focus on the DT case for now.
Now, this does re-introduce irq_to_gpio() in some way, but with the
following advantages:
1) The implementation is per-controller, not a single global function,
so isn't introducing any kind of centralized mapping scheme again.
2) This irq-chip-specific .irq_to_gpio() would only be implemented for
IRQ+GPIO chips that actually have a 1:1 mapping between GPIOs and IRQs.
Its potential existence doesn't imply that all IRQ chips need implement
this; it would be very specifically be for this one particular case.
So, I think it's reasonable to introduce this.
How about using the gpio irq domain xlate function?
Typically, in DT land a device using a gpio as an interrupt source
will have something like the following ...
eth@0 {
compatible = ks8851;
...
interrupt-parent = gpio2;
interrupts = 2 8; /* gpio line 34, low triggered */
};
... or ...
mmc {
label = pandaboard::status2;
gpios = gpio1 8 0;
...
};
Both these devices are using a gpio as an interrupt source, but the mmc
driver is requesting the gpio directly. In the first case the xlate
function for the gpio irq domain will be called where as it is not used
in the 2nd case. Therefore, we could add a custom xlate function. For
example ...
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 53bb8d5..caaeab2 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1085,6 +1085,33 @@ static void omap_gpio_chip_init(struct gpio_bank *bank)
irq_set_handler_data(bank-irq, bank);
}
+
+int omap_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr,
+ const u32 *intspec, unsigned int intsize,
+ unsigned long *out_hwirq, unsigned int *out_type)
+{
+ struct gpio_bank *bank;
+ int irq;
+
+ if (WARN_ON(intsize 1))
+ return -EINVAL;
+
+ irq = irq_find_mapping(d, intspec[0]);
+ bank = irq_get_chip_data(irq);
+ if (!bank)
+ return -ENODEV;
+
+ gpio_request_one(irq_to_gpio(bank, intspec[0]), GPIOF_IN,
ctrlr-name);
By the way, I know that I should check the return code here, but this
was just an example. Also I don't think using ctrlr-name here works
either as this is just gpio.
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 04/15/2013 03:40 PM, Jon Hunter wrote:
On 04/15/2013 11:58 AM, Stephen Warren wrote:
On 04/14/2013 02:53 PM, Linus Walleij wrote:
On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas
martinez.jav...@gmail.com wrote:
Is the following inlined patch [1] what you were thinking that would
be the right approach?
This looks sort of OK, but I'm still struggling with the question of
what we could do to help other implementations facing the same issue.
This is a pretty hard design pattern to properly replicate in every such
driver is it not?
Well, instead of adding .request_irq() to the irqchip, and then making
each driver call gpio_request() from the implementation, perhaps you
could add a .irq_to_gpio() to the irqchip, have the IRQ core call that,
and if it gets back a non-error response, the IRQ core could call
gpio_request(). That means that the change to each GPIO+IRQ driver is
simply to implement a standalone data transformation function
.irq_to_gpio().
I am still concerned about the case where a driver may have already
called gpio_request() and then calls request_irq(). I think that the
solution needs to handle cases where the driver may or may not call
gpio_request() to allocate the gpio.
Are there actually drivers that do this? Perhaps they could just be
fixed not to.
Although it could be argued that this is problem is not DT specific,
it does become a bigger problem to handle in the case of DT. Therefore,
I am wondering if we should just focus on the DT case for now.
That doesn't sound like a good idea; this issue is entirely orthogonal
to DT.
Now, this does re-introduce irq_to_gpio() in some way, but with the
following advantages:
1) The implementation is per-controller, not a single global function,
so isn't introducing any kind of centralized mapping scheme again.
2) This irq-chip-specific .irq_to_gpio() would only be implemented for
IRQ+GPIO chips that actually have a 1:1 mapping between GPIOs and IRQs.
Its potential existence doesn't imply that all IRQ chips need implement
this; it would be very specifically be for this one particular case.
So, I think it's reasonable to introduce this.
How about using the gpio irq domain xlate function?
That translates DT IRQ-specifiers to Linux IRQ numbers. There's no
reason to believe that, as an absolute rule, it would work for anything
GPIO-related. The fact that in practice most GPIO+IRQ controllers happen
to use the same numbering for GPIOs and IRQs is just co-incidence.
Typically, in DT land a device using a gpio as an interrupt source
will have something like the following ...
eth@0 {
compatible = ks8851;
...
interrupt-parent = gpio2;
interrupts = 2 8; /* gpio line 34, low triggered */
};
OK, that really is an interrupt...
... or ...
mmc {
label = pandaboard::status2;
gpios = gpio1 8 0;
...
};
But that's a gpio-leds instance, not an MMC controller... I really
really hope there's no DT node using gpios to mean interrupts... And
it wouldn't make any sense for an on-SoC device anyway.
Both these devices are using a gpio as an interrupt source, but the mmc
driver is requesting the gpio directly. In the first case the xlate
function for the gpio irq domain will be called where as it is not used
in the 2nd case. Therefore, we could add a custom xlate function. For
example ...
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
+int omap_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr,
...
+ gpio_request_one(irq_to_gpio(bank, intspec[0]), GPIOF_IN,
ctrlr-name);
I guess that could work, but:
a) It still means doing the gpio_request() in each driver instead of
centrally.
b) This approach doesn't solve the issue where some client driver has
already requested the GPIO. This code would simply prevent that call
from succeeding, which would probably make the driver probe() error out,
which isn't any different to the equivalent proposed centralized
gpio_request() inside some request_irq() failing, and causing the
device's probe() to error out.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 04/15/2013 05:16 PM, Stephen Warren wrote:
On 04/15/2013 03:40 PM, Jon Hunter wrote:
On 04/15/2013 11:58 AM, Stephen Warren wrote:
On 04/14/2013 02:53 PM, Linus Walleij wrote:
On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas
martinez.jav...@gmail.com wrote:
Is the following inlined patch [1] what you were thinking that would
be the right approach?
This looks sort of OK, but I'm still struggling with the question of
what we could do to help other implementations facing the same issue.
This is a pretty hard design pattern to properly replicate in every such
driver is it not?
Well, instead of adding .request_irq() to the irqchip, and then making
each driver call gpio_request() from the implementation, perhaps you
could add a .irq_to_gpio() to the irqchip, have the IRQ core call that,
and if it gets back a non-error response, the IRQ core could call
gpio_request(). That means that the change to each GPIO+IRQ driver is
simply to implement a standalone data transformation function
.irq_to_gpio().
I am still concerned about the case where a driver may have already
called gpio_request() and then calls request_irq(). I think that the
solution needs to handle cases where the driver may or may not call
gpio_request() to allocate the gpio.
Are there actually drivers that do this? Perhaps they could just be
fixed not to.
Yes ideally, but my fear is that there are several. I know both omap
display and mmc drivers do this. There are many drivers calling
gpio_direction_input() but I have not looked to see which of those are
just reading state versus configuring an interrupt.
Although it could be argued that this is problem is not DT specific,
it does become a bigger problem to handle in the case of DT. Therefore,
I am wondering if we should just focus on the DT case for now.
That doesn't sound like a good idea; this issue is entirely orthogonal
to DT.
True, but it is proving to be difficult to find a solution that everyone
can agree on.
Now, this does re-introduce irq_to_gpio() in some way, but with the
following advantages:
1) The implementation is per-controller, not a single global function,
so isn't introducing any kind of centralized mapping scheme again.
2) This irq-chip-specific .irq_to_gpio() would only be implemented for
IRQ+GPIO chips that actually have a 1:1 mapping between GPIOs and IRQs.
Its potential existence doesn't imply that all IRQ chips need implement
this; it would be very specifically be for this one particular case.
So, I think it's reasonable to introduce this.
How about using the gpio irq domain xlate function?
That translates DT IRQ-specifiers to Linux IRQ numbers. There's no
reason to believe that, as an absolute rule, it would work for anything
GPIO-related. The fact that in practice most GPIO+IRQ controllers happen
to use the same numbering for GPIOs and IRQs is just co-incidence.
Yes but provides a hook where we could call gpio_request(). However, I
am not sure if this would be considered abuse :-p
Typically, in DT land a device using a gpio as an interrupt source
will have something like the following ...
eth@0 {
compatible = ks8851;
...
interrupt-parent = gpio2;
interrupts = 2 8; /* gpio line 34, low triggered */
};
OK, that really is an interrupt...
... or ...
mmc {
label = pandaboard::status2;
gpios = gpio1 8 0;
...
};
But that's a gpio-leds instance, not an MMC controller... I really
really hope there's no DT node using gpios to mean interrupts... And
it wouldn't make any sense for an on-SoC device anyway.
Oops yes, I overlooked that. However, the omap mmc driver
(drivers/mmc/host/omap_hsmmc.c) does call gpio_request() and
request_threaded_irq() for the mmc card-detect interrupt. I believe
tegra is doing the same ...
sdhci@7800 {
...
cd-gpios = gpio 69 0; /* gpio PI5 */
...
};
Both these devices are using a gpio as an interrupt source, but the mmc
driver is requesting the gpio directly. In the first case the xlate
function for the gpio irq domain will be called where as it is not used
in the 2nd case. Therefore, we could add a custom xlate function. For
example ...
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
+int omap_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr,
...
+ gpio_request_one(irq_to_gpio(bank, intspec[0]), GPIOF_IN,
ctrlr-name);
I guess that could work, but:
a) It still means doing the gpio_request() in each driver instead of
centrally.
Right this is device specific, but it avoids exposing irq_to_gpio for a
device. However, we could make this generic if we did expose irq_to_gpio
for each device.
b) This approach doesn't solve the issue where some client driver has
already requested the GPIO. This code would simply prevent that call
from succeeding, which would probably make the driver probe() error out,
which isn't any different to
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas martinez.jav...@gmail.com wrote: Is the following inlined patch [1] what you were thinking that would be the right approach? This looks sort of OK, but I'm still struggling with the question of what we could do to help other implementations facing the same issue. This is a pretty hard design pattern to properly replicate in every such driver is it not? H Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Fri, Apr 12, 2013 at 12:47 AM, Stephen Warren swar...@wwwdotorg.org wrote:
On 04/11/2013 04:16 PM, Linus Walleij wrote:
On Thu, Apr 11, 2013 at 10:30 PM, Stephen Warren swar...@wwwdotorg.org
wrote:
On 04/10/2013 03:28 PM, Linus Walleij wrote:
So the only reason I'm rambing on about this is that it breaks the
I'm not sure I understand this paragraph; what is it in the line above.
If it is this patch, then should breaks be re-establishes?
No I'm replying to Javier Martinez Canillas mail in this thread:
http://marc.info/?l=linux-arm-kernelm=136334655902407w=2
which is stating a question to grand, and contains the below
hunk:
+static int gpio_irq_request(struct irq_data *d)
+{
+ struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+
+ return gpio_request(irq_to_gpio(bank, d-irq), gpio-irq);
+}
irq_to_gpio(). Notice that. not my_funny_driver_irq_to_gpio().
OK, right. sorry for being so confused/confusing.
Yes, that code should certainly call e.g. omap_gpio__irq_to_gpio() not
irq_to_gpio(). Probably gpio_irq_request() wants renaming to something
more like omap_gpio__irq_request() too, so the names don't look like
they'd clash with global functions. (__ added for clarity but probably
only one _ at a time)
Stephen, Linus,
Is the following inlined patch [1] what you were thinking that would
be the right approach?
With this patch an explicit call to call gpio_request() before a call
to chip-irq_set_type() is needed. I've tested both with DT and
without DT where a explicit call to gpio_request() is made and it
works in both cases. So it shouldn't have a functional change for
non-DT cases as far as I know.
If you agree with [1] then I'll split in two patches (one that adds
the irq_request function pointer to irq_chip and another one that adds
the implementation for gpio-omap) and send as a patch-set. I just
thought that it would be easier for you if I posted here an inlined
patch so you could have context.
Thanks a lot for your feedback and best regards,
Javier
[1]
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 685e850..e035e64 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -811,6 +811,13 @@ static void gpio_unmask_irq(struct irq_data *d)
spin_unlock_irqrestore(bank-lock, flags);
}
+static int omap_gpio_irq_request(struct irq_data *d)
+{
+ struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+
+ return omap_gpio_request(bank-chip, d-hwirq);
+}
+
static struct irq_chip gpio_irq_chip = {
.name = GPIO,
.irq_shutdown = gpio_irq_shutdown,
@@ -819,6 +826,7 @@ static struct irq_chip gpio_irq_chip = {
.irq_unmask = gpio_unmask_irq,
.irq_set_type = gpio_irq_type,
.irq_set_wake = gpio_wake_enable,
+ .irq_request= omap_gpio_irq_request,
};
/*-*/
@@ -1028,6 +1036,7 @@ omap_mpuio_alloc_gc(struct gpio_bank *bank,
unsigned int irq_start,
ct-chip.irq_mask = irq_gc_mask_set_bit;
ct-chip.irq_unmask = irq_gc_mask_clr_bit;
ct-chip.irq_set_type = gpio_irq_type;
+ ct-chip.irq_request = omap_gpio_irq_request;
if (bank-regs-wkup_en)
ct-chip.irq_set_wake = gpio_wake_enable,
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 0e8e3a6..85596cc 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -303,6 +303,7 @@ struct irq_chip {
void(*irq_shutdown)(struct irq_data *data);
void(*irq_enable)(struct irq_data *data);
void(*irq_disable)(struct irq_data *data);
+ int (*irq_request)(struct irq_data *data);
void(*irq_ack)(struct irq_data *data);
void(*irq_mask)(struct irq_data *data);
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index fa17855..a4bd4f7 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -588,6 +588,12 @@ int __irq_set_trigger(struct irq_desc *desc,
unsigned int irq,
unmask = 1;
}
+ if (chip-irq_request) {
+ ret = chip-irq_request(desc-irq_data);
+ if (ret)
+ return ret;
+ }
+
/* caller masked out all except trigger mode flags */
ret = chip-irq_set_type(desc-irq_data, flags);
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 04/10/2013 03:28 PM, Linus Walleij wrote: On Wed, Apr 10, 2013 at 10:29 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 04/10/2013 12:12 PM, Linus Walleij wrote: If the information is there, whether to convert from IRQ to GPIO or from GPIO to IRQ is a technicality and any order should be feasible in some way? There isn't always a unique 1:1 mapping between GPIOs and IRQs. Put another way, a single GPIO would likely only ever trigger a single IRQ, but a single IRQ might easily be triggered by any number of GPIOs. This is exactly why the function irq_to_gpio() isn't something one should use any more. I think there was an effort to outright remove the API, although it doesn't look like that's entirely complete yet. I guess you know this given your mentions of problem with gpio_to_irq() later on, so I'm not quite sure what your paragraph above was supposed to mean. So the only reason I'm rambing on about this is that it breaks the I'm not sure I understand this paragraph; what is it in the line above. If it is this patch, then should breaks be re-establishes? connection between GPIOs and their IRQs and at one point I percieved it as there was going to be some IRQ - GPIO lookup, and it would sneak back in. But now I realize that may have been supposed to be something local to the driver, in it's irqchip portions and then it's actually no problem for the kernel at large. Yes, I believe the intention was for their to be a correlation between GPIOs and IRQs only with the individual gpio_chip/irq_chip drivers for those GPIOs/IRQs, and not exposed anywhere outside it. Let's restate: I'm also after something fragile here. I assume you mean the opposite of that? IIUC, it will be possible for a GPIO to be set up as input and used as an IRQ without the GPIO subsystem knowing it. And it will be possible for the GPIO subsystem to go in and request the same pin and set it as output and e.g. bit-bang it. I wonder what happens then. Yes, I think that's possible now. If I recall the patch I'm replying to correctly, the idea was to add an IRQ request operation that would (internally to the GPIO/IRQ driver itself) map IRQ-GPIO, and call gpio_request(). That would then prevent exactly the situation you mention above. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Thu, Apr 11, 2013 at 10:30 PM, Stephen Warren swar...@wwwdotorg.org wrote:
On 04/10/2013 03:28 PM, Linus Walleij wrote:
So the only reason I'm rambing on about this is that it breaks the
I'm not sure I understand this paragraph; what is it in the line above.
If it is this patch, then should breaks be re-establishes?
No I'm replying to Javier Martinez Canillas mail in this thread:
http://marc.info/?l=linux-arm-kernelm=136334655902407w=2
which is stating a question to grand, and contains the below
hunk:
+static int gpio_irq_request(struct irq_data *d)
+{
+ struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+
+ return gpio_request(irq_to_gpio(bank, d-irq), gpio-irq);
+}
irq_to_gpio(). Notice that. not my_funny_driver_irq_to_gpio().
It's the same thing you mention below:
If I recall the patch I'm replying to correctly, the idea was to add an
IRQ request operation that would (internally to the GPIO/IRQ driver
itself) map IRQ-GPIO, and call gpio_request(). That would then prevent
exactly the situation you mention above.
So the above does not look like it's internal to the driver does it?
I think this is irq_to_gpio sneaking back in, and requiring that every
driver of this sort follow the same design pattern. And then maybe
you see my persistance ... are we talking about different things?
So I'd be happy with something local to the driver, foo_irq_to_gpio()
and that we also back out a bit and see what the subsystem can do
to avoid this kind of code having to be duplicated everywhere, that's
all.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Fri, Apr 12, 2013 at 12:16 AM, Linus Walleij
linus.wall...@linaro.org wrote:
On Thu, Apr 11, 2013 at 10:30 PM, Stephen Warren swar...@wwwdotorg.org
wrote:
On 04/10/2013 03:28 PM, Linus Walleij wrote:
So the only reason I'm rambing on about this is that it breaks the
I'm not sure I understand this paragraph; what is it in the line above.
If it is this patch, then should breaks be re-establishes?
No I'm replying to Javier Martinez Canillas mail in this thread:
http://marc.info/?l=linux-arm-kernelm=136334655902407w=2
which is stating a question to grand, and contains the below
hunk:
+static int gpio_irq_request(struct irq_data *d)
+{
+ struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+
+ return gpio_request(irq_to_gpio(bank, d-irq), gpio-irq);
+}
irq_to_gpio(). Notice that. not my_funny_driver_irq_to_gpio().
It's the same thing you mention below:
If I recall the patch I'm replying to correctly, the idea was to add an
IRQ request operation that would (internally to the GPIO/IRQ driver
itself) map IRQ-GPIO, and call gpio_request(). That would then prevent
exactly the situation you mention above.
So the above does not look like it's internal to the driver does it?
I think this is irq_to_gpio sneaking back in, and requiring that every
driver of this sort follow the same design pattern. And then maybe
you see my persistance ... are we talking about different things?
So I'd be happy with something local to the driver, foo_irq_to_gpio()
and that we also back out a bit and see what the subsystem can do
to avoid this kind of code having to be duplicated everywhere, that's
all.
Hi Linus,
Thanks a lot for your explanation. I didn't know that irq_to_gpio()
was being deprecated and we shouldn't use anymore. Now from this
thread discussion I understand the reasons for this decision.
I'll read the whole thread again and try to implement something that
is local to the gpio-omap driver instead using irq_to_gpio().
Besides using a controller specific mapping instead of irq_to_gpio(),
do you think that is a good idea to add a new irq_request function
pointer to struct irq_chip?
The idea is that GPIO controller drivers can call gpio_request() and
enabling the GPIO bank module before a call to request_irq() is made.
Or do you think that is better to implement the DT gpio hogs that you
were suggesting?
I really want to find a solution to this since currently we can't use
any device that uses an GPIO line as an IRQ on OMAP based boards.
Yours,
Linus Walleij
Best regards,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 04/11/2013 04:16 PM, Linus Walleij wrote:
On Thu, Apr 11, 2013 at 10:30 PM, Stephen Warren swar...@wwwdotorg.org
wrote:
On 04/10/2013 03:28 PM, Linus Walleij wrote:
So the only reason I'm rambing on about this is that it breaks the
I'm not sure I understand this paragraph; what is it in the line above.
If it is this patch, then should breaks be re-establishes?
No I'm replying to Javier Martinez Canillas mail in this thread:
http://marc.info/?l=linux-arm-kernelm=136334655902407w=2
which is stating a question to grand, and contains the below
hunk:
+static int gpio_irq_request(struct irq_data *d)
+{
+ struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+
+ return gpio_request(irq_to_gpio(bank, d-irq), gpio-irq);
+}
irq_to_gpio(). Notice that. not my_funny_driver_irq_to_gpio().
It's the same thing you mention below:
If I recall the patch I'm replying to correctly, the idea was to add an
IRQ request operation that would (internally to the GPIO/IRQ driver
itself) map IRQ-GPIO, and call gpio_request(). That would then prevent
exactly the situation you mention above.
OK, right. sorry for being so confused/confusing.
Yes, that code should certainly call e.g. omap_gpio__irq_to_gpio() not
irq_to_gpio(). Probably gpio_irq_request() wants renaming to something
more like omap_gpio__irq_request() too, so the names don't look like
they'd clash with global functions. (__ added for clarity but probably
only one _ at a time)
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 04/11/2013 04:16 PM, Linus Walleij wrote:
On Thu, Apr 11, 2013 at 10:30 PM, Stephen Warren swar...@wwwdotorg.org
wrote:
On 04/10/2013 03:28 PM, Linus Walleij wrote:
So the only reason I'm rambing on about this is that it breaks the
I'm not sure I understand this paragraph; what is it in the line above.
If it is this patch, then should breaks be re-establishes?
No I'm replying to Javier Martinez Canillas mail in this thread:
http://marc.info/?l=linux-arm-kernelm=136334655902407w=2
which is stating a question to grand, and contains the below
hunk:
+static int gpio_irq_request(struct irq_data *d)
+{
+ struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+
+ return gpio_request(irq_to_gpio(bank, d-irq), gpio-irq);
+}
irq_to_gpio(). Notice that. not my_funny_driver_irq_to_gpio().
OK, right. sorry for being so confused/confusing.
Yes, that code should certainly call e.g. omap_gpio__irq_to_gpio() not
irq_to_gpio(). Probably gpio_irq_request() wants renaming to something
more like omap_gpio__irq_request() too, so the names don't look like
they'd clash with global functions. (__ added for clarity but probably
only one _ at a time)
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Fri, Mar 29, 2013 at 6:01 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 03/27/2013 02:55 PM, Linus Walleij wrote: On Wed, Mar 27, 2013 at 5:09 PM, Stephen Warren swar...@wwwdotorg.org wrote: This is the case for some SMSC911x clients like the snowball routing it to a GPIO, whereas I think the RealView and Integrator/CP has a dedicated IRQ line for it on the FPGA so it's a good example. In such cases the right thing to do is for the platform code populating the platform data with the int irq field, or device tree core, or whatever piece of code that knows about the actual GPIO shall start from the GPIO, request it and then convert it to an IRQ. So board code could easily do that; plenty of opportunity to put whatever custom code is needed right there. For DT core code to do that, we'd need some alternative way of specifying interrupts. That would change the DT bindings for interrupts. I don't think we can do that... Sorry, I'm probably not knowledgeable about DT to understand this. The information for what GPIO corresponds to what IRQ is present in the device tree, is it not? If the information is there, whether to convert from IRQ to GPIO or from GPIO to IRQ is a technicality and any order should be feasible in some way? I just can't get the necessity to do it one way preferred over the other through my head, sorry... If it seems like identical boilerplate in every machine we can simply centralize it to gpiolib. Something like int gpio_add_single_irq_resource(struct platform_device *pdev, int gpio, unsigned flags) ... [code to convert GPIO to IRQ and create IORESOURCE_IRQ from it] int gpio_populate_irq_resource(struct platform_device *pdev, int gpio, unsigned flags, struct resource *r) ... [code to create IORESOURCE_IRQ from the IRQ] ... gpio_populate_irq_resource(ethernet, 17, IORESOURCE_IRQ_HIGHEDGE, ethernet_res[1]); platform_device_register(ethernet); That populates the device with the single GPIO IRQ from the GPIO number in a smooth way. Of course it has to be done after the GPIO driver is up and running so may require some other refactoring, but it should work. That latter issue also somewhat scuppers this approach; then you have to somehow run a bunch of your board file code inter-mixed with various driver probe() calls. That will quickly get nasy. No, just use a module_init() for the above code in the board file and it will defer just like any other initcall. And it doesn't address how the DT core will know when to call gpio_add_single_irq_resource() vs. gpio_populate_irq_resource() without changing the DT binding for interrupts. Is it not possible to do this in drivers/gpio/gpiolib-of.c: of_gpiochip_add(), if the DT *know* which GPIOs will be used as plain IRQs? That is the point with the gpio hogs I talk about ... GPIO hogs would hog input pins here and immediately call gpio_to_irq() on them, and from that point on they can only be used for input, and only for generating IRQs. And then, given that irq_to_gpio() isn't semantically possible, there's no way for the driver to be able to gpio_request() anything, since it doesn't, can't, and shouldn't know whether there is in fact any GPIO ID that it should request, let alone what that GPIO ID is. That's cool. The driver does not have to know the IRQ is backed by a GPIO, but somewhere in the system something should know this. Something already does; the irqchip driver for the IRQ in question would always know whether it's programming a plain IRQ controller, or an IRQ controller that's part of a GPIO controller. In the GPIO case, the irqchip driver also has enought HW-specific information to perform whatever GPIO-IRQ number conversion is needed. So I'm opposed to doing these things both ways because I think it's getting messy. We have already has irq_to_gpio() cause problems in the past and this reminds me heavily about that. Or, were you thinking of creating some kind of hogging system for gpiolib itself? Yes. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 04/10/2013 12:12 PM, Linus Walleij wrote: On Fri, Mar 29, 2013 at 6:01 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 03/27/2013 02:55 PM, Linus Walleij wrote: On Wed, Mar 27, 2013 at 5:09 PM, Stephen Warren swar...@wwwdotorg.org wrote: This is the case for some SMSC911x clients like the snowball routing it to a GPIO, whereas I think the RealView and Integrator/CP has a dedicated IRQ line for it on the FPGA so it's a good example. In such cases the right thing to do is for the platform code populating the platform data with the int irq field, or device tree core, or whatever piece of code that knows about the actual GPIO shall start from the GPIO, request it and then convert it to an IRQ. So board code could easily do that; plenty of opportunity to put whatever custom code is needed right there. For DT core code to do that, we'd need some alternative way of specifying interrupts. That would change the DT bindings for interrupts. I don't think we can do that... Sorry, I'm probably not knowledgeable about DT to understand this. The information for what GPIO corresponds to what IRQ is present in the device tree, is it not? No, I don't think so. DT treats the IRQ and GPIO numbering spaces as entirely unrelated things, just like the kernel APIs do. The fact that some DT nodes (device/drivers) happen to be both GPIO and IRQ providers (implement gpio_chip and irq_chip) and happen to use the same numbering scheme for both GPIO and IRQ IDs is purely an implementation detail within individual drivers (or rather, of specific DT binding definitions really), and not a general truism. If the information is there, whether to convert from IRQ to GPIO or from GPIO to IRQ is a technicality and any order should be feasible in some way? There isn't always a unique 1:1 mapping between GPIOs and IRQs. Put another way, a single GPIO would likely only ever trigger a single IRQ, but a single IRQ might easily be triggered by any number of GPIOs. This is exactly why the function irq_to_gpio() isn't something one should use any more. I think there was an effort to outright remove the API, although it doesn't look like that's entirely complete yet. I guess you know this given your mentions of problem with gpio_to_irq() later on, so I'm not quite sure what your paragraph above was supposed to mean. I just can't get the necessity to do it one way preferred over the other through my head, sorry... If it seems like identical boilerplate in every machine we can simply centralize it to gpiolib. Something like int gpio_add_single_irq_resource(struct platform_device *pdev, int gpio, unsigned flags) ... [code to convert GPIO to IRQ and create IORESOURCE_IRQ from it] int gpio_populate_irq_resource(struct platform_device *pdev, int gpio, unsigned flags, struct resource *r) ... [code to create IORESOURCE_IRQ from the IRQ] ... gpio_populate_irq_resource(ethernet, 17, IORESOURCE_IRQ_HIGHEDGE, ethernet_res[1]); platform_device_register(ethernet); That populates the device with the single GPIO IRQ from the GPIO number in a smooth way. Of course it has to be done after the GPIO driver is up and running so may require some other refactoring, but it should work. That latter issue also somewhat scuppers this approach; then you have to somehow run a bunch of your board file code inter-mixed with various driver probe() calls. That will quickly get nasy. No, just use a module_init() for the above code in the board file and it will defer just like any other initcall. Using initcalls to order things is rather fragile. Perhaps it could work out OK with board files, but it's certainly something that's frowned upon now for DT-based systems, since deferred probe solves it much more generally, and without any maintenance issues (e.g. continual re-shuffling of the initcall order, not having enough initcall levels if you end up with more dependencies being added, etc.). And it doesn't address how the DT core will know when to call gpio_add_single_irq_resource() vs. gpio_populate_irq_resource() without changing the DT binding for interrupts. Is it not possible to do this in drivers/gpio/gpiolib-of.c: of_gpiochip_add(), if the DT *know* which GPIOs will be used as plain IRQs? That is the point with the gpio hogs I talk about ... I suppose that if you modified the device tree bindings (schema definitions), you could have every GPIO provider DT node indicate which GPIOs were used as IRQs instead. However, a) This would be an incompatible change to the DT bindings, and they're supposed to be a stable ABI; old DTBs should work unmodified with new kernels. b) This would place GPIO usage information in multiple places. Right now, GPIO consumers list which GPIO(s) they use. The same goes for IRQs. If a GPIO provider started listing which GPIOs were really IRQs, then that'd mean configuring each GPIO-as-an-IRQ in two places; once in the client node to
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Wed, Apr 10, 2013 at 10:29 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 04/10/2013 12:12 PM, Linus Walleij wrote: If the information is there, whether to convert from IRQ to GPIO or from GPIO to IRQ is a technicality and any order should be feasible in some way? There isn't always a unique 1:1 mapping between GPIOs and IRQs. Put another way, a single GPIO would likely only ever trigger a single IRQ, but a single IRQ might easily be triggered by any number of GPIOs. This is exactly why the function irq_to_gpio() isn't something one should use any more. I think there was an effort to outright remove the API, although it doesn't look like that's entirely complete yet. I guess you know this given your mentions of problem with gpio_to_irq() later on, so I'm not quite sure what your paragraph above was supposed to mean. So the only reason I'm rambing on about this is that it breaks the connection between GPIOs and their IRQs and at one point I percieved it as there was going to be some IRQ - GPIO lookup, and it would sneak back in. But now I realize that may have been supposed to be something local to the driver, in it's irqchip portions and then it's actually no problem for the kernel at large. Let's restate: I'm also after something fragile here. IIUC, it will be possible for a GPIO to be set up as input and used as an IRQ without the GPIO subsystem knowing it. And it will be possible for the GPIO subsystem to go in and request the same pin and set it as output and e.g. bit-bang it. I wonder what happens then. If the drive code is written in such a way that this will be denied, I'll soften up, but I'd like to see the code first. Then the same mistake has to be avoided in all drivers. And that makes me want to probe for solutions tying them up in the core so no mistakes can be made. Is it not possible to do this in drivers/gpio/gpiolib-of.c: of_gpiochip_add(), if the DT *know* which GPIOs will be used as plain IRQs? That is the point with the gpio hogs I talk about ... I suppose that if you modified the device tree bindings (schema definitions), you could have every GPIO provider DT node indicate which GPIOs were used as IRQs instead. However, a) This would be an incompatible change to the DT bindings, and they're supposed to be a stable ABI; old DTBs should work unmodified with new kernels. b) This would place GPIO usage information in multiple places. Right now, GPIO consumers list which GPIO(s) they use. The same goes for IRQs. If a GPIO provider started listing which GPIOs were really IRQs, then that'd mean configuring each GPIO-as-an-IRQ in two places; once in the client node to indicate which IRQ it should hook/register, and once in the provider node to indicate that the GPIO is really an IRQ. Two places is more hassle for people to implement, and more opportunity for mistakes. Yeah but the current approach AFAICT isn't exactly safe either. So I need some more convincing... or code rather, I guess. If I can't point out the problems I see in a piece of code I guess I'm just plain wrong :-) Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Wednesday 10 April 2013 14:29:17 Stephen Warren wrote: If the information is there, whether to convert from IRQ to GPIO or from GPIO to IRQ is a technicality and any order should be feasible in some way? There isn't always a unique 1:1 mapping between GPIOs and IRQs. Put another way, a single GPIO would likely only ever trigger a single IRQ, but a single IRQ might easily be triggered by any number of GPIOs. This is exactly why the function irq_to_gpio() isn't something one should use any more. More importantly, most irqs don't have any GPIO associated with them at all. The interface made some sense for simple platforms that had a linear range of GPIO IRQs, but that also isn't true these days, with arbitrary IRQ domains. I think there was an effort to outright remove the API, although it doesn't look like that's entirely complete yet. It's essentially complete. There are a few remnants in areas of the kernel that we don't like to look at: * The blackfin architecture provides the interface and uses it in the pata_rb532_cf driver. * MIPS provides it on a few older platforms, and it's used on the alchemy pcmcia driver. * avr32, m68k, sh and unicore32 provide the interface but don't use it. * On ARM, the interface is provided on iop, gemini, ixp4xx, ks8695 and w90x900. None of these use it, and they rarely see any patches at all these days. * ARM/davinci provides a stub but always return -ENOSYS. * One driver for PXA (tosa_battery) still uses this interface, but PXA stopped providing it long ago. Arnd -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 03/27/2013 02:55 PM, Linus Walleij wrote: On Wed, Mar 27, 2013 at 5:09 PM, Stephen Warren swar...@wwwdotorg.org wrote: It's simply that if a device emits an IRQ, there's no reason to assume that the IRQ is in fact a GPIO. It might be a dedicated IRQ input pin and not something that gpiolib (or any other GPIO API) knows about. OK (following the first example, that's what I have in the Nomadik or Snowball already). Another possibility: (Device IRQ output wired directly into dedicated IRQ input pin, and that pin has no GPIO capabilities) ++ | SoC| ++ | IRQ 5 | DEV_IRQ | Device | | GIC --|-|-IRQ| || ++ ++ As such, the driver /must/ receive an int irq in platform data. Nah. Lets pass a struct resource * of type IORESOURCE_IRQ. But I understand the necessity to pass the IRQ number somehow. Oh sure. I tend to consider the resources part of platform data, although I agree they really aren't. If it received an int gpio, then there would be no way for the driver to support boards that routed that interrupt signal to a dedicated IRQ pin on the SoC, rather than to a GPIO pin that just happened to have interrupt generation capabilities. This is the case for some SMSC911x clients like the snowball routing it to a GPIO, whereas I think the RealView and Integrator/CP has a dedicated IRQ line for it on the FPGA so it's a good example. In such cases the right thing to do is for the platform code populating the platform data with the int irq field, or device tree core, or whatever piece of code that knows about the actual GPIO shall start from the GPIO, request it and then convert it to an IRQ. So board code could easily do that; plenty of opportunity to put whatever custom code is needed right there. For DT core code to do that, we'd need some alternative way of specifying interrupts. That would change the DT bindings for interrupts. I don't think we can do that... If it seems like identical boilerplate in every machine we can simply centralize it to gpiolib. Something like int gpio_add_single_irq_resource(struct platform_device *pdev, int gpio, unsigned flags) ... [code to convert GPIO to IRQ and create IORESOURCE_IRQ from it] int gpio_populate_irq_resource(struct platform_device *pdev, int gpio, unsigned flags, struct resource *r) ... [code to create IORESOURCE_IRQ from the IRQ] ... gpio_populate_irq_resource(ethernet, 17, IORESOURCE_IRQ_HIGHEDGE, ethernet_res[1]); platform_device_register(ethernet); That populates the device with the single GPIO IRQ from the GPIO number in a smooth way. Of course it has to be done after the GPIO driver is up and running so may require some other refactoring, but it should work. That latter issue also somewhat scuppers this approach; then you have to somehow run a bunch of your board file code inter-mixed with various driver probe() calls. That will quickly get nasy. And it doesn't address how the DT core will know when to call gpio_add_single_irq_resource() vs. gpio_populate_irq_resource() without changing the DT binding for interrupts. And then, given that irq_to_gpio() isn't semantically possible, there's no way for the driver to be able to gpio_request() anything, since it doesn't, can't, and shouldn't know whether there is in fact any GPIO ID that it should request, let alone what that GPIO ID is. That's cool. The driver does not have to know the IRQ is backed by a GPIO, but somewhere in the system something should know this. Something already does; the irqchip driver for the IRQ in question would always know whether it's programming a plain IRQ controller, or an IRQ controller that's part of a GPIO controller. In the GPIO case, the irqchip driver also has enought HW-specific information to perform whatever GPIO-IRQ number conversion is needed. Such as board code or DT core (I'm thinking drivers/of/platform.c code calling gpiolib-of.c to do this mapping for DT, for example). And by introducing the requirement that such GPIO lines be GPIO input hogs in the device tree else warn we can get a nice closure in the DT case: the GPIO subsystem knows the actual GPIO line as a hog that is request and sets as input at probe, the referencing device node only gets the IRQ but at runtime the device node instatiation for the IRQ consumer will ask the gpiolib to check its hog list to make sure one of these will match that IRQ, then request it. Hogs sounds like pinctrl. pinctrl doesn't support the concept of hogs on GPIOs, since it only knows about pins not GPIOs really; even with the integration with gpiolib, pinctrl gets informed when GPIOs are requested, not when setting up pinctrl states ahead of time, and also this notification is only so we can mark the pin related to
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Fri, Mar 22, 2013 at 11:52 PM, Jon Hunter jon-hun...@ti.com wrote:
On 03/22/2013 10:33 AM, Stephen Warren wrote:
On 03/22/2013 02:10 AM, Linus Walleij wrote:
This is just turning everything on it's head. The normal order of things
is this sequence for GPIO 14 something like:
gpio_request(14);
int irq = gpio_to_irq(14);
request_any_context_irq(irq);
That doesn't make any sense at all. Drivers don't want to care whether
the IRQ number they receive is a GPIO-that-can-act-like-an-IRQ or a pure
dedicated IRQ input.
To fully support the model you proprose, a driver would have to:
if (gpio_is_valid(pdata-gpio)) {
gpio_request_one(pdata-gpio, GPIOF_IN, foo irq);
irq = gpio_to_irq(pdata-gpio);
} else
irq = pdata-irq;
request_irq(...);
which means complex code in each driver, plus requiring some indication
in platform data and/or device tree re: whether the IRQ is hosted by a
GPIO or not.
I suspect the above is just referring to the DT or similar usecases
(such as ACPI)?
We already have a number of platforms passing IRQ numbers
for something that is actually a GPIO pin but we don't really
know and debugfs doesn't say because they never
issue gpio_request() of the pin. And that's something I
don't like.
Because that is loosing control over the GPIO numberspace
not quite knowing which pin is used and which one isn't
and making these things prone to bugs.
What worries me is changing kernel semantics to fit device tree,
I think it is better to try to confine this to the drivers/gpio/gpiolib-of.c
file.
I'd like OF GPIO code to hog the pin using gpio_request(),
then figure out the IRQ number using gpio_to_irq() and pass
that out when picking an IRQ right out of a GPIO controllers
DT node. And I'd like it to be compulsory exercise to list
that one GPIO line as an input hog when used for just
some IRQ line.
This is what I would want to achieve by the GPIO hog concept.
I tend to agree with Stephen here. When we had discussed this previously
you had mentioned about adding a platform function to request the gpio
[1], but I could see this adding a bunch more platform files when we are
trying to get rid of all the board-xxx.c files for OMAP. So if you don't
like the approach suggested by Stephen and implemented by Javier, then
may be we need to means to request/reserve gpios in the dts as you had
suggested before [1].
So it seems to me that there are two options at the moment ...
1. Add a irq_chip request as suggested by Stephen.
2. Reserve/request gpios in the dts when registering a gpio chip.
This seems like you need to bring in Grant for a second opinion I'm
getting confused by this now... Paging Alexandre who's doing
the GPIO descriptor refactoring as well.
[1] http://permalink.gmane.org/gmane.linux.ports.arm.omap/92327
Is there anything wrong with the GPIO hog concept as presented in the
mentioned reply?
+input-hog-gpios = 4, 5;
Would result in these GPIOs being requested, so we can keep
track of them in e.g. drivers/gpio/gpiolib-of.c, then supply
IRQs from these hogged inputs on demand from drivers.
But with the requirement that they be hogged, so we can keep
track of them, and they show up with some sensible
name in debugfs/gpio.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 03/27/2013 07:52 AM, Linus Walleij wrote:
On Fri, Mar 22, 2013 at 11:52 PM, Jon Hunter jon-hun...@ti.com wrote:
On 03/22/2013 10:33 AM, Stephen Warren wrote:
On 03/22/2013 02:10 AM, Linus Walleij wrote:
This is just turning everything on it's head. The normal order of things
is this sequence for GPIO 14 something like:
gpio_request(14);
int irq = gpio_to_irq(14);
request_any_context_irq(irq);
That doesn't make any sense at all. Drivers don't want to care whether
the IRQ number they receive is a GPIO-that-can-act-like-an-IRQ or a pure
dedicated IRQ input.
To fully support the model you proprose, a driver would have to:
if (gpio_is_valid(pdata-gpio)) {
gpio_request_one(pdata-gpio, GPIOF_IN, foo irq);
irq = gpio_to_irq(pdata-gpio);
} else
irq = pdata-irq;
request_irq(...);
which means complex code in each driver, plus requiring some indication
in platform data and/or device tree re: whether the IRQ is hosted by a
GPIO or not.
I suspect the above is just referring to the DT or similar usecases
(such as ACPI)?
...
What worries me is changing kernel semantics to fit device tree,
I think it is better to try to confine this to the drivers/gpio/gpiolib-of.c
file.
No, this has absolutely noting to do with device tree; the example I
gave above is purely based on platform data.
It's simply that if a device emits an IRQ, there's no reason to assume
that the IRQ is in fact a GPIO. It might be a dedicated IRQ input pin
and not something that gpiolib (or any other GPIO API) knows about.
One possibility:
(Device IRQ output wired into GPIO input on SoC which then generates an
interrupt)
++
| SoC| ++
| IRQ 5 +--+ GPIO 10 | DEV_IRQ | Device |
| GIC | GPIO | ---|-|-IRQ|
| +--+ | ++
++
Another possibility:
(Device IRQ output wired directly into dedicated IRQ input pin, and that
pin has no GPIO capabilities)
++
| SoC| ++
| IRQ 5 | DEV_IRQ | Device |
| GIC --|-|-IRQ|
|| ++
++
As such, the driver /must/ receive an int irq in platform data. If it
received an int gpio, then there would be no way for the driver to
support boards that routed that interrupt signal to a dedicated IRQ pin
on the SoC, rather than to a GPIO pin that just happened to have
interrupt generation capabilities.
And then, given that irq_to_gpio() isn't semantically possible, there's
no way for the driver to be able to gpio_request() anything, since it
doesn't, can't, and shouldn't know whether there is in fact any GPIO ID
that it should request, let alone what that GPIO ID is.
And finally, even if we ignore dedicated IRQ input pins, and assume that
every IRQ input is really a GPIO, why should the driver be forced to
call both request_irq() and gpio_request(); that's something that should
really be dealt with inside the IRQ subsystem or relevant IRQ driver.
Otherwise, every other driver that uses IRQs has to be burdened with the
code to do that.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Wed, Mar 27, 2013 at 5:09 PM, Stephen Warren swar...@wwwdotorg.org wrote:
It's simply that if a device emits an IRQ, there's no reason to assume
that the IRQ is in fact a GPIO. It might be a dedicated IRQ input pin
and not something that gpiolib (or any other GPIO API) knows about.
OK (following the first example, that's what I have in the Nomadik
or Snowball already).
Another possibility:
(Device IRQ output wired directly into dedicated IRQ input pin, and that
pin has no GPIO capabilities)
++
| SoC| ++
| IRQ 5 | DEV_IRQ | Device |
| GIC --|-|-IRQ|
|| ++
++
As such, the driver /must/ receive an int irq in platform data.
Nah. Lets pass a struct resource * of type IORESOURCE_IRQ.
But I understand the necessity to pass the IRQ number somehow.
If it
received an int gpio, then there would be no way for the driver to
support boards that routed that interrupt signal to a dedicated IRQ pin
on the SoC, rather than to a GPIO pin that just happened to have
interrupt generation capabilities.
This is the case for some SMSC911x clients like the snowball
routing it to a GPIO, whereas I think the RealView and Integrator/CP
has a dedicated IRQ line for it on the FPGA so it's a good example.
In such cases the right thing to do is for the platform
code populating the platform data with the int irq field, or device tree
core, or whatever piece of code that knows about
the actual GPIO shall start from the GPIO, request it and
then convert it to an IRQ.
If it seems like identical boilerplate in every machine we can
simply centralize it to gpiolib. Something like
int gpio_add_single_irq_resource(struct platform_device *pdev, int
gpio, unsigned flags)
{
struct resource *r;
int irq;
int err;
r = devm_kmalloc(pdev-dev, sizeof(*r), GFP_KERNEL);
if (!r)
return -ENOMEM;
memset(r, 0, sizeof(*r));
err = request_gpio(gpio);
if (err)
return err;
irq = gpio_to_irq(gpio);
if (irq 0)
return irq;
r-start = irq;
r-end = irq;
r-flags = IORESOURCE_IRQ | flags;
pdev-num_resources = 1;
pdev-resource = res;
};
int gpio_populate_irq_resource(struct platform_device *pdev, int gpio,
unsigned flags, struct resource *r)
{
int irq;
int err;
err = request_gpio(gpio);
if (err)
return err;
irq = gpio_to_irq(gpio);
if (irq 0)
return irq;
r-start = irq;
r-end = irq;
r-flags = IORESOURCE_IRQ | flags;
};
(Add permutations for either filling in a struct resource *r from
the board code, or krealloc() to extend an existing dynamic allocation
or whatever, surely these helpers can be written.)
So this is for a pure platform data case (based on
arch/arm/mach-ux500/board-mop500.c, which currently
relies on hard-coded GPIO and IRQ numbers):
static struct resource ethernet_res[] = {
{
.name = smsc911x-memory,
.start = (0x5000 16),
.end = (0x5000 16) + 0x,
.flags = IORESOURCE_MEM,
},
{ }, /* To be filled in with GPIO IRQ */
};
static struct platform_device ethernet = {
.name = smsc911x,
.dev= {
.platform_data = ,
},
};
gpio_populate_irq_resource(ethernet, 17,
IORESOURCE_IRQ_HIGHEDGE, ethernet_res[1]);
platform_device_register(ethernet);
That populates the device with the single GPIO IRQ from
the GPIO number in a smooth way.
Of course it has to be done after the GPIO driver is up
and running so may require some other refactoring,
but it should work.
I can implement this and convert the Snowball if it
helps.
And then, given that irq_to_gpio() isn't semantically possible, there's
no way for the driver to be able to gpio_request() anything, since it
doesn't, can't, and shouldn't know whether there is in fact any GPIO ID
that it should request, let alone what that GPIO ID is.
That's cool. The driver does not have to know the IRQ is backed by a
GPIO, but somewhere in the system something should know this.
Such as board code or DT core (I'm thinking drivers/of/platform.c code
calling gpiolib-of.c to do this mapping for DT, for example).
And by introducing the requirement that such GPIO lines be
GPIO input hogs in the device tree else warn we can get a nice
closure in the DT case: the GPIO subsystem knows the actual
GPIO line as a hog that is request and sets as input at probe,
the referencing device node only gets the IRQ but at runtime
the device node instatiation for the IRQ consumer will ask
the gpiolib to check its hog list to make sure one of these will
match that IRQ, then request it.
And finally, even if we ignore dedicated IRQ input pins, and assume that
every IRQ input is
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Fri, Mar 15, 2013 at 12:21 PM, Javier Martinez Canillas
martinez.jav...@gmail.com wrote:
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 159f5c5..f5feb43 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -807,6 +807,13 @@ static void gpio_unmask_irq(struct irq_data *d)
spin_unlock_irqrestore(bank-lock, flags);
}
+static int gpio_irq_request(struct irq_data *d)
+{
+ struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+
+ return gpio_request(irq_to_gpio(bank, d-irq), gpio-irq);
+}
+
static struct irq_chip gpio_irq_chip = {
.name = GPIO,
.irq_shutdown = gpio_irq_shutdown,
@@ -815,6 +822,7 @@ static struct irq_chip gpio_irq_chip = {
.irq_unmask = gpio_unmask_irq,
.irq_set_type = gpio_irq_type,
.irq_set_wake = gpio_wake_enable,
+ .irq_request= gpio_irq_request,
};
This is just turning everything on it's head. The normal order of things
is this sequence for GPIO 14 something like:
gpio_request(14);
int irq = gpio_to_irq(14);
request_any_context_irq(irq);
I understand that the approach take make things easier for device tree
but it screws up the semantics of the entire kernel (the lockdep etc
warnings are just a symptom), we are locked to the above semantic
sequence: you know the GPIO, *then* you get the IRQ, not the
other way around.
irq_to_gpio has been deemed a legacy thing that we want to get
rid of. The reason being that it is ambiguous on most every GPIO
expander out there. It is not supported by GPIOLIB. This is the
reason to why the implementation in the GPIOLIB in
linux/gpio.h looks like this:
static inline int irq_to_gpio(unsigned int irq)
{
return -EINVAL;
}
The only chance to have the function is in a non-gpiolib platform
that override it.
The irq_to_gpio() in gpio-omap.c overrides this and should
cause compile warnings, does it not?
There is a reason every other driver in drivers/gpio has renamed
this function to things like pxa_irq_to_gpio, msm_irq_to_gpio:
we just don't support it generically.
I know I'm not very helpful, I can just say this is not going to work. :-/
It seems you need a way to get the GPIO corresponding to a certain
IRQ from the device tree, not from the kernel as is done here, then
follow the mentioned sequence.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 03/22/2013 02:10 AM, Linus Walleij wrote:
On Fri, Mar 15, 2013 at 12:21 PM, Javier Martinez Canillas
martinez.jav...@gmail.com wrote:
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 159f5c5..f5feb43 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -807,6 +807,13 @@ static void gpio_unmask_irq(struct irq_data *d)
spin_unlock_irqrestore(bank-lock, flags);
}
+static int gpio_irq_request(struct irq_data *d)
+{
+ struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+
+ return gpio_request(irq_to_gpio(bank, d-irq), gpio-irq);
+}
+
static struct irq_chip gpio_irq_chip = {
.name = GPIO,
.irq_shutdown = gpio_irq_shutdown,
@@ -815,6 +822,7 @@ static struct irq_chip gpio_irq_chip = {
.irq_unmask = gpio_unmask_irq,
.irq_set_type = gpio_irq_type,
.irq_set_wake = gpio_wake_enable,
+ .irq_request= gpio_irq_request,
};
This is just turning everything on it's head. The normal order of things
is this sequence for GPIO 14 something like:
gpio_request(14);
int irq = gpio_to_irq(14);
request_any_context_irq(irq);
That doesn't make any sense at all. Drivers don't want to care whether
the IRQ number they receive is a GPIO-that-can-act-like-an-IRQ or a pure
dedicated IRQ input.
To fully support the model you proprose, a driver would have to:
if (gpio_is_valid(pdata-gpio)) {
gpio_request_one(pdata-gpio, GPIOF_IN, foo irq);
irq = gpio_to_irq(pdata-gpio);
} else
irq = pdata-irq;
request_irq(...);
which means complex code in each driver, plus requiring some indication
in platform data and/or device tree re: whether the IRQ is hosted by a
GPIO or not.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 03/22/2013 10:33 AM, Stephen Warren wrote:
On 03/22/2013 02:10 AM, Linus Walleij wrote:
On Fri, Mar 15, 2013 at 12:21 PM, Javier Martinez Canillas
martinez.jav...@gmail.com wrote:
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 159f5c5..f5feb43 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -807,6 +807,13 @@ static void gpio_unmask_irq(struct irq_data *d)
spin_unlock_irqrestore(bank-lock, flags);
}
+static int gpio_irq_request(struct irq_data *d)
+{
+ struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+
+ return gpio_request(irq_to_gpio(bank, d-irq), gpio-irq);
+}
+
static struct irq_chip gpio_irq_chip = {
.name = GPIO,
.irq_shutdown = gpio_irq_shutdown,
@@ -815,6 +822,7 @@ static struct irq_chip gpio_irq_chip = {
.irq_unmask = gpio_unmask_irq,
.irq_set_type = gpio_irq_type,
.irq_set_wake = gpio_wake_enable,
+ .irq_request= gpio_irq_request,
};
This is just turning everything on it's head. The normal order of things
is this sequence for GPIO 14 something like:
gpio_request(14);
int irq = gpio_to_irq(14);
request_any_context_irq(irq);
That doesn't make any sense at all. Drivers don't want to care whether
the IRQ number they receive is a GPIO-that-can-act-like-an-IRQ or a pure
dedicated IRQ input.
To fully support the model you proprose, a driver would have to:
if (gpio_is_valid(pdata-gpio)) {
gpio_request_one(pdata-gpio, GPIOF_IN, foo irq);
irq = gpio_to_irq(pdata-gpio);
} else
irq = pdata-irq;
request_irq(...);
which means complex code in each driver, plus requiring some indication
in platform data and/or device tree re: whether the IRQ is hosted by a
GPIO or not.
I tend to agree with Stephen here. When we had discussed this previously
you had mentioned about adding a platform function to request the gpio
[1], but I could see this adding a bunch more platform files when we are
trying to get rid of all the board-xxx.c files for OMAP. So if you don't
like the approach suggested by Stephen and implemented by Javier, then
may be we need to means to request/reserve gpios in the dts as you had
suggested before [1].
So it seems to me that there are two options at the moment ...
1. Add a irq_chip request as suggested by Stephen.
2. Reserve/request gpios in the dts when registering a gpio chip.
Cheers
Jon
[1] http://permalink.gmane.org/gmane.linux.ports.arm.omap/92327
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Fri, Mar 8, 2013 at 12:14 AM, Jon Hunter jon-hun...@ti.com wrote:
On 03/02/2013 02:05 PM, Grant Likely wrote:
On Tue, 26 Feb 2013 17:01:22 -0600, Jon Hunter jon-hun...@ti.com wrote:
On 02/26/2013 04:44 PM, Stephen Warren wrote:
On 02/26/2013 03:40 PM, Jon Hunter wrote:
On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
Are you requesting the gpio anywhere? If not then this is not going to
work as-is. This was discussed fairly recently [1] and the conclusion
was that the gpio needs to be requested before we can use as an interrupt.
That seems wrong; the GPIO/IRQ driver should handle this internally. The
Ethernet driver shouldn't know/care whether the interrupt it's given is
some form of dedicated interrupt or a GPIO-based interrupt, and even if
it somehow did, there's no irq_to_gpio() any more, so the driver can't
tell which GPIO ID it should request, unless it's given yet another
property to represent this.
I agree that ideally this should be handled internally. Did you read the
discussion on the thread that I referenced [1]? If you have any thoughts
we are open to ideas :-)
I'm on an airplane right now, but I agree 100% with Stephen. I'll try to
remember to go read that thread and respond, but this falls firmly in
the its-a-bug category for me. :-)
Grant, did you have chance to review the thread [1]?
I am trying to figure out if we should just take the original patch
proposed in the thread (although Linus had some objections) or look at
alternative solutions such as adding a irq_chip request as Stephen
suggested.
Cheers
Jon
[1] comments.gmane.org/gmane.linux.ports.arm.omap/92192
Hello Grant,
I was wondering if you have any opinions on this issue. As Jon said,
Stephen proposed [2] to add a request callback to irq_chip.
I hacked a very simple and naive patch (just to validate the idea) and
is working. The GPIO bank is requested before calling the gpio-omap
.irq_set_type function handler (gpio_irq_type) when using a GPIO as an
IRQ on a DT. So is not necessary to call it explicitly anymore.
But the patch is obviously wrong (to say the least) since the kernel
runtime locking validator complains that possible circular locking
dependency detected
I just wanted to know if I was on the right track or completely lost here.
Thanks a lot and best regards,
javier
[2]: http://www.mail-archive.com/linux-omap@vger.kernel.org/msg85592.html
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 159f5c5..f5feb43 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -807,6 +807,13 @@ static void gpio_unmask_irq(struct irq_data *d)
spin_unlock_irqrestore(bank-lock, flags);
}
+static int gpio_irq_request(struct irq_data *d)
+{
+ struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+
+ return gpio_request(irq_to_gpio(bank, d-irq), gpio-irq);
+}
+
static struct irq_chip gpio_irq_chip = {
.name = GPIO,
.irq_shutdown = gpio_irq_shutdown,
@@ -815,6 +822,7 @@ static struct irq_chip gpio_irq_chip = {
.irq_unmask = gpio_unmask_irq,
.irq_set_type = gpio_irq_type,
.irq_set_wake = gpio_wake_enable,
+ .irq_request= gpio_irq_request,
};
/*-*/
diff --git a/include/linux/irq.h b/include/linux/irq.h
index bc4e066..2aeaa24 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -303,6 +303,7 @@ struct irq_chip {
void(*irq_shutdown)(struct irq_data *data);
void(*irq_enable)(struct irq_data *data);
void(*irq_disable)(struct irq_data *data);
+ int (*irq_request)(struct irq_data *data);
void(*irq_ack)(struct irq_data *data);
void(*irq_mask)(struct irq_data *data);
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index fa17855..07c20f7 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1093,6 +1093,13 @@ __setup_irq(unsigned int irq, struct irq_desc
*desc, struct irqaction *new)
if (!shared) {
init_waitqueue_head(desc-wait_for_threads);
+ if (desc-irq_data.chip-irq_request) {
+ ret = desc-irq_data.chip-irq_request(desc-irq_data);
+
+ if (ret)
+ goto out_mask;
+ }
+
/* Setup the type (level, edge polarity) if configured: */
if (new-flags IRQF_TRIGGER_MASK) {
ret = __irq_set_trigger(desc, irq,
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 03/02/2013 02:05 PM, Grant Likely wrote: On Tue, 26 Feb 2013 17:01:22 -0600, Jon Hunter jon-hun...@ti.com wrote: On 02/26/2013 04:44 PM, Stephen Warren wrote: On 02/26/2013 03:40 PM, Jon Hunter wrote: On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote: Are you requesting the gpio anywhere? If not then this is not going to work as-is. This was discussed fairly recently [1] and the conclusion was that the gpio needs to be requested before we can use as an interrupt. That seems wrong; the GPIO/IRQ driver should handle this internally. The Ethernet driver shouldn't know/care whether the interrupt it's given is some form of dedicated interrupt or a GPIO-based interrupt, and even if it somehow did, there's no irq_to_gpio() any more, so the driver can't tell which GPIO ID it should request, unless it's given yet another property to represent this. I agree that ideally this should be handled internally. Did you read the discussion on the thread that I referenced [1]? If you have any thoughts we are open to ideas :-) I'm on an airplane right now, but I agree 100% with Stephen. I'll try to remember to go read that thread and respond, but this falls firmly in the its-a-bug category for me. :-) Grant, did you have chance to review the thread [1]? I am trying to figure out if we should just take the original patch proposed in the thread (although Linus had some objections) or look at alternative solutions such as adding a irq_chip request as Stephen suggested. Cheers Jon [1] comments.gmane.org/gmane.linux.ports.arm.omap/92192 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Tue, 26 Feb 2013 17:01:22 -0600, Jon Hunter jon-hun...@ti.com wrote: On 02/26/2013 04:44 PM, Stephen Warren wrote: On 02/26/2013 03:40 PM, Jon Hunter wrote: On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote: Are you requesting the gpio anywhere? If not then this is not going to work as-is. This was discussed fairly recently [1] and the conclusion was that the gpio needs to be requested before we can use as an interrupt. That seems wrong; the GPIO/IRQ driver should handle this internally. The Ethernet driver shouldn't know/care whether the interrupt it's given is some form of dedicated interrupt or a GPIO-based interrupt, and even if it somehow did, there's no irq_to_gpio() any more, so the driver can't tell which GPIO ID it should request, unless it's given yet another property to represent this. I agree that ideally this should be handled internally. Did you read the discussion on the thread that I referenced [1]? If you have any thoughts we are open to ideas :-) I'm on an airplane right now, but I agree 100% with Stephen. I'll try to remember to go read that thread and respond, but this falls firmly in the its-a-bug category for me. :-) g. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Thu, Feb 28, 2013 at 12:16 AM, Jon Hunter jon-hun...@ti.com wrote: On 02/26/2013 09:57 PM, Javier Martinez Canillas wrote: [snip] Something like that would definitely solve the GPIO request issue but we still have the issue that the current OMAP GPIO controller binding does not support #interrupt-cells = 2. So, we can't pass the trigger type and level flags for an IRQ-GPIO when using an GPIO controller as the interrupt-parent for a device node. Do you have any comments on that issue? Can you elaborate a bit more on why you say this is not supported? I have been playing with this today on an omap board and if I set the #interrupt-cells = 2, then I do see that irq_domain_xlate_onetwocell() is called and the irq number and flags read as expected. Following which I then see it will call the omap_irq_type() to set type. So AFAICT it works. Yes, it does. I (wrongly) assumed that it was not working since the GPIO OMAP binding documentation says that #interrupt-cells should be 2 but all OMAP2+ DTs use 1 instead. Also, when trying to change to 2 I had the kernel hang. But it was indeed that the GPIO bank was not enabled before calling gpio_irq_type() and this made the kernel to hang. Your patch fixed the issue and allowed me to find the cause. The problem was that when using the DT hack of defining the GPIO in the ethernet chip regulator, the calls to irq_domain_xlate_onetwocell() and gpio_irq_type() were made before the call to gpio_request_one() so the GPIO bank was not enabled. If gpio_request() is called in gpmc_probe_dt(), then the GPIO bank is enabled and gpio_irq_type() succeeds. So, it was just me being stupid and don't understanding the implementation. Please note I do see that when the SMC driver calls request_irq() in smc_drv_probe() it is also settings the trigger type to IRQ_TYPE_EDGE_RISING (default). So if you are setting to low-level sensitive in DT, then this is being overwritten. We could fix this by setting SMC_IRQ_FLAGS to -1 for OMAP. Please note that I'm using a SMSC 911x chip and not a SMSC 91x, so the driver is not smc91x but smc911x. It has the same behaviour though (IRQ flags overwritten somehow), just to be sure that we are on the same page. I don't know if just setting SMC_IRQ_FLAGS to -1 will be enough to fix the smc91x since request_irq() is just passing whatever value is in irq_flags. By looking at the two drivers (smc91x and smsc911x) it seems that the only difference on how they manage the flags is that smc91x does: unsigned long irq_flags = SMC_IRQ_FLAGS; ... if (irq_flags == -1 || ires-flags IRQF_TRIGGER_MASK) irq_flags = ires-flags IRQF_TRIGGER_MASK; while smsc911x driver's probe function uses the flags from the resource unconditionally: irq_flags = irq_res-flags IRQF_TRIGGER_MASK; So, at the end both will set irq_flags to whatever is on the IORESOURCE_IRQ struct resource flags member. The real problem is irq_flags to be 0 instead of the value defined on the second cell of the interrupts property. when irq_domain_xlate_onetwocell() is called for the ethernet GPIO-IRQ I see that both the cells size and the second cell with the flag values are set correctly (2 and IRQF_TRIGGER_LOW). But even when gpio_irq_type() succeeds it seems that the struct resource IRQ flags passed to the smsc911x driver is still overwritten or not set correctly since flags IRQF_TRIGGER_MASK is always 0. In general we do need to fix the gpio binding for omap to default to #interrupt-cell = 2, as this should work. However, before we can do that we need to fix the issue of ensuring the gpio module is enabled if being used as an interrupt source without having to call gpio_request() first. Indeed, although I still wonder why the flags are not set correctly for the smsc911x driver. I had only tested it with this device so I don't know if this is a general issue of IORESOURCE_IRQ structs not been initialized correctly when using GPIOs as IRQ on OMAP or if is only related to this driver. We should probably add the following patch as well to avoid any hangs if the bank is not enabled, when omap_irq_type is called. commit 5e298de564e09f5ca4148a9bc0ed5d16b4742f14 Author: Jon Hunter jon-hun...@ti.com Date: Wed Feb 27 17:14:11 2013 -0600 gpio/omap: warn if gpio bank is not enabled on setting irq type diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index f1fbedb2..cbdc796 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -421,6 +421,9 @@ static int gpio_irq_type(struct irq_data *d, unsigned type) int retval; unsigned long flags; + if (WARN_ON(!bank-mod_usage)) + return -EINVAL; + #ifdef CONFIG_ARCH_OMAP1 if (d-irq IH_MPUIO_BASE) gpio = OMAP_MPUIO(d-irq - IH_MPUIO_BASE); Cheers Jon Thanks a lot for your help and best regards, Javier -- To unsubscribe from this list: send the line unsubscribe
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
+ Jon who was brave enough to take over the OMAP GPIO driver
+ New email address for Kevin since he is no longer at TI :-(.
- Tarun that left TI but I don't have his new email
Hi Linus,
On 02/28/2013 12:41 AM, Linus Walleij wrote:
On Wed, Feb 15, 2012 at 5:04 PM, Benoit Cousson b-cous...@ti.com wrote:
Gosh! That's pretty old stuff :-)
@@ -52,7 +55,8 @@ struct gpio_bank {
struct list_head node;
void __iomem *base;
u16 irq;
- u16 virtual_irq_start;
+ int irq_base;
+ struct irq_domain *domain;
This seems wrong. IRQ domains are used to avoid keeping track of
irq base offsets. I would even say it's the whole point of irq domains.
Well at that time, it was not the only point. We were trying to boot
with DT, and for that irq_domain was mandatory. At the very same time,
Grant was cleaning and consolidating the whole irq_domain
infrastructure, and thus most of the following API were not really
stabilized or even available.
So, this idea was to avoid any regression while allowing DT boot... And
obviously a lot of stuff happened since that good old time.
@@ -669,7 +673,7 @@ static void gpio_irq_handler(unsigned int irq, struct
irq_desc *desc)
if (!isr)
break;
- gpio_irq = bank-virtual_irq_start;
+ gpio_irqkhil...@ti.com = bank-irq_base;
for (; isr != 0; isr = 1, gpio_irq++) {
gpio_index = GPIO_INDEX(bank, irq_to_gpio(gpio_irq));
Use irq_find_mapping(irqdomain, hwirq) in this function.
@@ -915,7 +919,7 @@ static int gpio_2irq(struct gpio_chip *chip, unsigned
offset)
struct gpio_bank *bank;
bank = container_of(chip, struct gpio_bank, chip);
- return bank-virtual_irq_start + offset;
+ return bank-irq_base + offset;
Use irq_create_mapping() in this function.
+ bank-irq_base = irq_alloc_descs(-1, 0, bank-width, 0);
+ if (bank-irq_base 0) {
+ dev_err(dev, Couldn't allocate IRQ numbers\n);
+ return -ENODEV;
+ }
+
+ bank-domain = irq_domain_add_legacy(node, bank-width,
bank-irq_base,
+0, irq_domain_simple_ops,
NULL);
Use irq_domain_add_simple() and the descs will be allocated for
you as part of the domain creation, when using a pre-fixed base.
Funny, that API was removed while I was writing this patch, and is now
back in town...
If you fix this I suspect the device tree discussion also will fix
itself :-)
Mmm, I hope it is the case, but I'm not sure it will be that easy :-)
Thanks,
Benoit
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 02/28/2013 06:17 AM, Javier Martinez Canillas wrote:
On Thu, Feb 28, 2013 at 12:16 AM, Jon Hunter jon-hun...@ti.com wrote:
On 02/26/2013 09:57 PM, Javier Martinez Canillas wrote:
[snip]
Something like that would definitely solve the GPIO request issue but
we still have the issue that the current OMAP GPIO controller binding
does not support #interrupt-cells = 2.
So, we can't pass the trigger type and level flags for an IRQ-GPIO
when using an GPIO controller as the interrupt-parent for a device
node.
Do you have any comments on that issue?
Can you elaborate a bit more on why you say this is not supported?
I have been playing with this today on an omap board and if I set the
#interrupt-cells = 2, then I do see that irq_domain_xlate_onetwocell()
is called and the irq number and flags read as expected. Following which
I then see it will call the omap_irq_type() to set type. So AFAICT it works.
Yes, it does.
I (wrongly) assumed that it was not working since the GPIO OMAP
binding documentation says that #interrupt-cells should be 2 but all
OMAP2+ DTs use 1 instead. Also, when trying to change to 2 I had
the kernel hang.
But it was indeed that the GPIO bank was not enabled before calling
gpio_irq_type() and this made the kernel to hang. Your patch fixed the
issue and allowed me to find the cause.
The problem was that when using the DT hack of defining the GPIO in
the ethernet chip regulator, the calls to
irq_domain_xlate_onetwocell() and gpio_irq_type() were made before the
call to gpio_request_one() so the GPIO bank was not enabled.
If gpio_request() is called in gpmc_probe_dt(), then the GPIO bank is
enabled and gpio_irq_type() succeeds.
So, it was just me being stupid and don't understanding the implementation.
No problem. Glad we are on the same page :-)
Please note I do see that when the SMC driver calls request_irq() in
smc_drv_probe() it is also settings the trigger type to
IRQ_TYPE_EDGE_RISING (default). So if you are setting to low-level
sensitive in DT, then this is being overwritten. We could fix this by
setting SMC_IRQ_FLAGS to -1 for OMAP.
Please note that I'm using a SMSC 911x chip and not a SMSC 91x, so the
driver is not smc91x but smc911x. It has the same behaviour though
(IRQ flags overwritten somehow), just to be sure that we are on the
same page.
I don't know if just setting SMC_IRQ_FLAGS to -1 will be enough to fix
the smc91x since request_irq() is just passing whatever value is in
irq_flags.
By looking at the two drivers (smc91x and smsc911x) it seems that the
only difference on how they manage the flags is that smc91x does:
unsigned long irq_flags = SMC_IRQ_FLAGS;
...
if (irq_flags == -1 || ires-flags IRQF_TRIGGER_MASK)
irq_flags = ires-flags IRQF_TRIGGER_MASK;
while smsc911x driver's probe function uses the flags from the
resource unconditionally:
irq_flags = irq_res-flags IRQF_TRIGGER_MASK;
So, at the end both will set irq_flags to whatever is on the
IORESOURCE_IRQ struct resource flags member.
Actually, that's not the case for smc91x. By default SMC_IRQ_FLAGS != -1
(for omap) and so it will not set irq_flags to ires-flags
IRQF_TRIGGER_MASK. However, if I force irq_flags to be -1, then I see
that irq_flags are to 0.
The real problem is irq_flags to be 0 instead of the value defined on
the second cell of the interrupts property.
So the resource flags for each irq is set in
of_irq_to_resource() which just does ...
r-start = r-end = irq;
r-flags = IORESOURCE_IRQ;
r-name = name ? name : dev-full_name;
of_irq_to_resource() calls irq_to_parse_and_map() which eventually just
calls irq_set_irq_type() but type/flags is not returned and not populated.
I am wondering if this is intentional. The irq_type is being correctly
configured by irq_set_irq_type() when of_irq_to_resource() is called. In
the smc driver, if irq_flags are 0, then when request_irq() is called
the trigger type will not be set again (which is ok). __setup_irq has
the following ...
/* Setup the type (level, edge polarity) if configured: */
if (new-flags IRQF_TRIGGER_MASK) {
ret = __irq_set_trigger(desc, irq,
new-flags IRQF_TRIGGER_MASK);
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Thu, Feb 28, 2013 at 2:04 PM, Benoit Cousson b-cous...@ti.com wrote: On 02/28/2013 12:41 AM, Linus Walleij wrote: On Wed, Feb 15, 2012 at 5:04 PM, Benoit Cousson b-cous...@ti.com wrote: Gosh! That's pretty old stuff :-) Hm yeah, haha :-) I didn't notice that someone was replying in the thread for some other reason (or maybe using the same subject and confusing my gmail reader?). Anyway, the basic idea is sound, so are you still working on the patch? I remember discussing this with someone in Denmark... Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 02/28/2013 06:09 PM, Linus Walleij wrote: On Thu, Feb 28, 2013 at 2:04 PM, Benoit Cousson b-cous...@ti.com wrote: On 02/28/2013 12:41 AM, Linus Walleij wrote: On Wed, Feb 15, 2012 at 5:04 PM, Benoit Cousson b-cous...@ti.com wrote: Gosh! That's pretty old stuff :-) Hm yeah, haha :-) I didn't notice that someone was replying in the thread for some other reason (or maybe using the same subject and confusing my gmail reader?). Anyway, the basic idea is sound, so are you still working on the patch? I remember discussing this with someone in Denmark... That was me :-) I have the patch, I am testing and will send tomorrow for review. Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 02/26/2013 08:33 PM, Javier Martinez Canillas wrote:
...
Yes, I realized that requesting the gpio was necessary so what I did
is to use the regulator-fixed optional property gpio and define
the GPIO used as an IRQ in a regulator used by the SMSC chip. So, I
have this on my board DT:
vddvario: regulator-vddvario {
compatible = regulator-fixed;
regulator-name = vddvario;
regulator-always-on;
gpio = gpio6 16 8; /* gpio line 176 */
enable-active-high;
gpio-open-drain;
While admittedly it's configured as open-drain, that will configure the
GPIO to be an output, whereas for usage as an interrupt, it really
should be configured as an input... Perhaps it makes no difference on
OMAP HW since the I/O paths are separate, but I can easily imagine HW
where this hack wouldn't work.
regulator-boot-on;
};
gpmc {
ethernet@5,0 {
pinctrl-names = default;
pinctrl-0 = smsc911x_pins;
compatible = smsc,lan9221, smsc,lan9115;
reg = 5 0 0xff;
interrupt-parent = gpio6;
interrupts = 16 8; /* gpio line 176 */
interrupt-names = smsc911x irq;
vmmc-supply = vddvario;
vmmc_aux-supply = vdd33a;
reg-io-width = 4;
smsc,save-mac-address;
};
};
That way a call to gpio_request_one() is made and the GPIO is requested.
This look a little hack-ish for me but I've seen this in other
DeviceTrees like omap4-sdp.dts so I thought it was a common DT
pattern.
Indeed; the GPIO is in no way a regulator, so while the above is fine
for testing, it's in no way a solution that can actually be used and
checked in.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 02/26/2013 08:57 PM, Javier Martinez Canillas wrote: On Wed, Feb 27, 2013 at 2:07 AM, Jon Hunter jon-hun...@ti.com wrote: On 02/26/2013 06:13 PM, Stephen Warren wrote: On 02/26/2013 04:45 PM, Jon Hunter wrote: ... One issue I see is that by not calling gpio_request, then potentially you could have someone request a gpio via gpio_request() and someone trying to use it as an interrupt source via request_irq(). Now obviously that represents a bug because there is only one physical gpio, but I gather it is something we need to protect against. I'm not sure it's really that much of an issue, but presumably the solution is for a combined GPIO+IRQ driver to simply call gpio_request internally from within some irq_chip function. It looks like struct irq_chip doesn't have a request/free, but perhaps they could be added to solve this? Yes I was wondering if we could do something like that. That would work, may be that's what we should propose. Something like that would definitely solve the GPIO request issue but we still have the issue that the current OMAP GPIO controller binding does not support #interrupt-cells = 2. The binding documentation in Documentation/devicetree/bindings/gpio/gpio-omap.txt indicates that it does. If this doesn't work in practice, it's a driver bug that can presumably be easily fixed. And no need to change any ABI definitions:-) BTW, I notice in that binding document that the description of the two cells for #interrupt-cells is actually part of the description of the interrupt-controller property; it should be moved up one line really. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Wed, Feb 27, 2013 at 6:47 PM, Stephen Warren swar...@wwwdotorg.org wrote:
On 02/26/2013 08:33 PM, Javier Martinez Canillas wrote:
...
Yes, I realized that requesting the gpio was necessary so what I did
is to use the regulator-fixed optional property gpio and define
the GPIO used as an IRQ in a regulator used by the SMSC chip. So, I
have this on my board DT:
vddvario: regulator-vddvario {
compatible = regulator-fixed;
regulator-name = vddvario;
regulator-always-on;
gpio = gpio6 16 8; /* gpio line 176 */
enable-active-high;
gpio-open-drain;
While admittedly it's configured as open-drain, that will configure the
GPIO to be an output, whereas for usage as an interrupt, it really
should be configured as an input... Perhaps it makes no difference on
OMAP HW since the I/O paths are separate, but I can easily imagine HW
where this hack wouldn't work.
regulator-boot-on;
};
gpmc {
ethernet@5,0 {
pinctrl-names = default;
pinctrl-0 = smsc911x_pins;
compatible = smsc,lan9221, smsc,lan9115;
reg = 5 0 0xff;
interrupt-parent = gpio6;
interrupts = 16 8; /* gpio line 176 */
interrupt-names = smsc911x irq;
vmmc-supply = vddvario;
vmmc_aux-supply = vdd33a;
reg-io-width = 4;
smsc,save-mac-address;
};
};
That way a call to gpio_request_one() is made and the GPIO is requested.
This look a little hack-ish for me but I've seen this in other
DeviceTrees like omap4-sdp.dts so I thought it was a common DT
pattern.
Indeed; the GPIO is in no way a regulator, so while the above is fine
for testing, it's in no way a solution that can actually be used and
checked in.
Yes, this is just a work-in-progress and is not meant to be taken as a
patch submission.
I'm just doing some testing with DT and figuring out how much effort
will take to have the same hardware support we currently with board
files for IGEP boards.
Thanks a lot for your feedback and best regards,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Wed, Feb 27, 2013 at 6:50 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 02/26/2013 08:57 PM, Javier Martinez Canillas wrote: On Wed, Feb 27, 2013 at 2:07 AM, Jon Hunter jon-hun...@ti.com wrote: On 02/26/2013 06:13 PM, Stephen Warren wrote: On 02/26/2013 04:45 PM, Jon Hunter wrote: ... One issue I see is that by not calling gpio_request, then potentially you could have someone request a gpio via gpio_request() and someone trying to use it as an interrupt source via request_irq(). Now obviously that represents a bug because there is only one physical gpio, but I gather it is something we need to protect against. I'm not sure it's really that much of an issue, but presumably the solution is for a combined GPIO+IRQ driver to simply call gpio_request internally from within some irq_chip function. It looks like struct irq_chip doesn't have a request/free, but perhaps they could be added to solve this? Yes I was wondering if we could do something like that. That would work, may be that's what we should propose. Something like that would definitely solve the GPIO request issue but we still have the issue that the current OMAP GPIO controller binding does not support #interrupt-cells = 2. The binding documentation in Documentation/devicetree/bindings/gpio/gpio-omap.txt indicates that it does. If this doesn't work in practice, it's a driver bug that can presumably be easily fixed. And no need to change any ABI definitions:-) indeed :-) In fact I think that some documentation bits were borrowed from the NVIDIA Tegra GPIO controller bindings but it was never implemented in the OMAP GPIO driver to parse the second interrupt-cell which should specify the flags. BTW, I notice in that binding document that the description of the two cells for #interrupt-cells is actually part of the description of the interrupt-controller property; it should be moved up one line really. Right, will submit a patch to fix this. Best regards, Javier -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 02/26/2013 09:47 PM, Javier Martinez Canillas wrote:
On Wed, Feb 27, 2013 at 12:08 AM, Jon Hunter jon-hun...@ti.com wrote:
On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
[snip]
I was wondering if the level/edge settings for gpios is working on OMAP.
I'm adding DT support for an SMSC911x ethernet chip connected to the
GPMC for an OMAP3 SoC based board.
In the smsc911x driver probe function (smsc911x_drv_probe() in
drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
Reading the gpio-omap.txt documentation it says that #interrupt-cells
should be 2 and that a value of 8 is active low level-sensitive.
So I tried this:
gpmc {
ethernet@5,0 {
pinctrl-names = default;
pinctrl-0 = smsc911x_pins;
compatible = smsc,lan9221, smsc,lan9115;
reg = 5 0 0xff; /* CS5 */
interrupt-parent = gpio6;
interrupts = 16 8; /* gpio line 176 */
interrupt-names = smsc911x irq;
vmmc-supply = vddvario;
vmmc_aux-supply = vdd33a;
reg-io-width = 4;
By the way, reg-io-width for omap does not look correct. The GPMC only
supports 8-bit or 16-bit devices IIRC. I believe all my omap boards use
16-bit.
I thought that even when the GPMC was a 16-bit external memory
controller it could do some access adaptation to support 32-bit
devices.
Right the GPMC itself may be able to translate 32-bit accesses into
16-bit. However, I have not looked at the data sheet for the smsc parts
to know what impact this would have on any register accesses in the smsc
device.
By looking at the board files for others OMAP3 based boards
(board-{omap3evm,overo,zoom-debugboard}.c) most of them set the struct
omap_smsc911x_platform_data .flags member to SMSC911X_USE_32BIT.
I see this too now. I was looking at the
arch/arm/mach-omap2/gpmc-smc91x.c which has SMC91X_USE_16BIT and
GPMC_CONFIG1_DEVICESIZE_16. So this is for a slightly different ethernet
chip but same concept. Hopefully, someone knew what they were doing for
those other boards!
And by looking at Documentation/devicetree/bindings/net/smsc911x.txt I
thought that the corresponding DT property for this flag was
reg-io-width
Anyway, I tried using both reg-io-width = 4 and reg-io-width = 2.
The ethernet chip works with both of them and I don't see too much
difference in performance:
16-bit
round-trip min/avg/max = 0.611/0.738/0.946 ms
32-bit
round-trip min/avg/max = 0.519/0.690/0.976 ms
So, is your call ;-)
While I can't argue with the fact it works, I would not be prepared to
make that call without reviewing the documentation for the smsc and the
driver.
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 02/26/2013 09:57 PM, Javier Martinez Canillas wrote: [snip] Something like that would definitely solve the GPIO request issue but we still have the issue that the current OMAP GPIO controller binding does not support #interrupt-cells = 2. So, we can't pass the trigger type and level flags for an IRQ-GPIO when using an GPIO controller as the interrupt-parent for a device node. Do you have any comments on that issue? Can you elaborate a bit more on why you say this is not supported? I have been playing with this today on an omap board and if I set the #interrupt-cells = 2, then I do see that irq_domain_xlate_onetwocell() is called and the irq number and flags read as expected. Following which I then see it will call the omap_irq_type() to set type. So AFAICT it works. Please note I do see that when the SMC driver calls request_irq() in smc_drv_probe() it is also settings the trigger type to IRQ_TYPE_EDGE_RISING (default). So if you are setting to low-level sensitive in DT, then this is being overwritten. We could fix this by setting SMC_IRQ_FLAGS to -1 for OMAP. In general we do need to fix the gpio binding for omap to default to #interrupt-cell = 2, as this should work. However, before we can do that we need to fix the issue of ensuring the gpio module is enabled if being used as an interrupt source without having to call gpio_request() first. We should probably add the following patch as well to avoid any hangs if the bank is not enabled, when omap_irq_type is called. commit 5e298de564e09f5ca4148a9bc0ed5d16b4742f14 Author: Jon Hunter jon-hun...@ti.com Date: Wed Feb 27 17:14:11 2013 -0600 gpio/omap: warn if gpio bank is not enabled on setting irq type diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index f1fbedb2..cbdc796 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -421,6 +421,9 @@ static int gpio_irq_type(struct irq_data *d, unsigned type) int retval; unsigned long flags; + if (WARN_ON(!bank-mod_usage)) + return -EINVAL; + #ifdef CONFIG_ARCH_OMAP1 if (d-irq IH_MPUIO_BASE) gpio = OMAP_MPUIO(d-irq - IH_MPUIO_BASE); Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Wed, Feb 15, 2012 at 5:04 PM, Benoit Cousson b-cous...@ti.com wrote:
@@ -52,7 +55,8 @@ struct gpio_bank {
struct list_head node;
void __iomem *base;
u16 irq;
- u16 virtual_irq_start;
+ int irq_base;
+ struct irq_domain *domain;
This seems wrong. IRQ domains are used to avoid keeping track of
irq base offsets. I would even say it's the whole point of irq domains.
@@ -669,7 +673,7 @@ static void gpio_irq_handler(unsigned int irq, struct
irq_desc *desc)
if (!isr)
break;
- gpio_irq = bank-virtual_irq_start;
+ gpio_irq = bank-irq_base;
for (; isr != 0; isr = 1, gpio_irq++) {
gpio_index = GPIO_INDEX(bank, irq_to_gpio(gpio_irq));
Use irq_find_mapping(irqdomain, hwirq) in this function.
@@ -915,7 +919,7 @@ static int gpio_2irq(struct gpio_chip *chip, unsigned
offset)
struct gpio_bank *bank;
bank = container_of(chip, struct gpio_bank, chip);
- return bank-virtual_irq_start + offset;
+ return bank-irq_base + offset;
Use irq_create_mapping() in this function.
+ bank-irq_base = irq_alloc_descs(-1, 0, bank-width, 0);
+ if (bank-irq_base 0) {
+ dev_err(dev, Couldn't allocate IRQ numbers\n);
+ return -ENODEV;
+ }
+
+ bank-domain = irq_domain_add_legacy(node, bank-width,
bank-irq_base,
+0, irq_domain_simple_ops, NULL);
Use irq_domain_add_simple() and the descs will be allocated for
you as part of the domain creation, when using a pre-fixed base.
If you fix this I suspect the device tree discussion also will fix
itself :-)
Documentation/IRQ-domain.txt
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Fri, Feb 24, 2012 at 4:30 PM, Cousson, Benoit b-cous...@ti.com wrote:
On 2/22/2012 7:29 PM, Stephen Warren wrote:
Rob Herring wrote at Wednesday, February 22, 2012 10:23 AM:
On 02/22/2012 08:31 AM, Cousson, Benoit wrote:
On 2/22/2012 3:23 PM, Rob Herring wrote:
On 02/15/2012 10:04 AM, Benoit Cousson wrote:
Adapt the GPIO driver to retrieve information from a DT file.
Allocate the irq_base dynamically and rename bank-virtual_irq_start
to bank-irq_base.
Change irq_base type to int instead of u16 to match irq_alloc_descs
output.
Add documentation for GPIO properties specific to OMAP.
Signed-off-by: Benoit Coussonb-cous...@ti.com
Cc: Tarun Kanti DebBarmatarun.ka...@ti.com
One comment below, but otherwise:
Acked-by: Rob Herringrob.herr...@calxeda.com
---
.../devicetree/bindings/gpio/gpio-omap.txt | 30 +
drivers/gpio/gpio-omap.c | 121
++--
2 files changed, 142 insertions(+), 9 deletions(-)
create mode 100644
Documentation/devicetree/bindings/gpio/gpio-omap.txt
diff --git a/Documentation/devicetree/bindings/gpio/gpio-omap.txt
b/Documentation/devicetree/bindings/gpio/gpio-omap.txt
new file mode 100644
index 000..c1b3100
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-omap.txt
@@ -0,0 +1,30 @@
+OMAP GPIO controller bindings
+
+Required properties:
+- compatible:
+ - ti,omap2-gpio for OMAP2 controllers
+ - ti,omap3-gpio for OMAP3 controllers
+ - ti,omap4-gpio for OMAP4 controllers
+- #gpio-cells : Should be two.
+ - first cell is the pin number
+ - second cell is used to specify optional parameters (unused)
+- gpio-controller : Marks the device node as a GPIO controller.
+- #interrupt-cells : Should be one
There's no level/edge settings for gpios?
That's a good question, because I was wondering as well :-)
I did no see how it was done in other GPIO implementation.
There's not really a good example that I've found. Many gpio nodes don't
even have interrupt-controller set.
So if you have an irq_set_type function for gpio's, then you should have
2 cells.
After checking the OMAP gpio code, I do have a gpio_irq_type and the GPIO do
support the 4 types. I'm not sure yet about the combinations.
Tegra's GPIO IRQ binding (gpio_nvidia.txt in linux-next at least) says:
- #interrupt-cells : Should be 2.
The first cell is the GPIO number.
The second cell is used to specify flags:
bits[3:0] trigger type and level flags:
1 = low-to-high edge triggered.
2 = high-to-low edge triggered.
4 = active high level-sensitive.
8 = active low level-sensitive.
Valid combinations are 1, 2, 3, 4, 8.
Indeed, so I'll just copy / paste Tegra's binding...
Thanks,
Benoit
Hello,
I was wondering if the level/edge settings for gpios is working on OMAP.
I'm adding DT support for an SMSC911x ethernet chip connected to the
GPMC for an OMAP3 SoC based board.
In the smsc911x driver probe function (smsc911x_drv_probe() in
drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
Reading the gpio-omap.txt documentation it says that #interrupt-cells
should be 2 and that a value of 8 is active low level-sensitive.
So I tried this:
gpmc {
ethernet@5,0 {
pinctrl-names = default;
pinctrl-0 = smsc911x_pins;
compatible = smsc,lan9221, smsc,lan9115;
reg = 5 0 0xff; /* CS5 */
interrupt-parent = gpio6;
interrupts = 16 8; /* gpio line 176 */
interrupt-names = smsc911x irq;
vmmc-supply = vddvario;
vmmc_aux-supply = vdd33a;
reg-io-width = 4;
smsc,save-mac-address;
};
};
But in the smsc911x probe function:
irq_res-flags IRQF_TRIGGER_MASK;
returns 0 which means that no trigger flags where set.
I took a look to the GPIOs device node definition on omap{3,4,5}.dtsi
and all look like this:
e.g from omap3.dtsi:
gpio6: gpio@49058000 {
compatible = ti,omap3-gpio;
ti,hwmods = gpio6;
gpio-controller;
#gpio-cells = 2;
interrupt-controller;
#interrupt-cells = 1;
};
So, even when the documentation says that all the GPIO device nodes in
OMAP2+ should use a #interrupt-cells property value of 2, they are
only using 1. Changing that value to 2 makes hangs the kernel and it
does not boot.
This is not part of my dayjob and I'm working on this in my free time
so it would be great if someone can give me some pointers or at least
confirm that this is a known issue so I can debug/fix/extend
drivers/gpio/gpio-omap.c to parse and pass the IRQF_TRIGGER_* flags
to the IORESOURCE_IRQ resource structure.
Thanks
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 02/26/2013 03:01 AM, Javier Martinez Canillas wrote:
...
I was wondering if the level/edge settings for gpios is working on OMAP.
...
Reading the gpio-omap.txt documentation it says that #interrupt-cells
should be 2 and that a value of 8 is active low level-sensitive.
So I tried this:
gpmc {
ethernet@5,0 {
...
interrupt-parent = gpio6;
interrupts = 16 8; /* gpio line 176 */
interrupt-names = smsc911x irq;
...
But in the smsc911x probe function:
irq_res-flags IRQF_TRIGGER_MASK;
returns 0 which means that no trigger flags where set.
I took a look to the GPIOs device node definition on omap{3,4,5}.dtsi
and all look like this:
e.g from omap3.dtsi:
gpio6: gpio@49058000 {
...
#interrupt-cells = 1;
...
So, even when the documentation says that all the GPIO device nodes in
OMAP2+ should use a #interrupt-cells property value of 2, they are
only using 1. Changing that value to 2 makes hangs the kernel and it
does not boot.
Take a look at what the OMAP GPIO driver is plugging in for the of_xlate
function; does it correctly handle the case where there are two cells?
Is there a default for the flags when no second cell is provided, such
that when you set #interrupt-cells to 2, you're changing away from the
default and mis-configuring things?
And, did you check for any other interrupts hooked through the GPIO
module, for which you'll need to add the second cell into their GPIO
specifier throughout the entire device tree?
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
[snip]
I was wondering if the level/edge settings for gpios is working on OMAP.
I'm adding DT support for an SMSC911x ethernet chip connected to the
GPMC for an OMAP3 SoC based board.
In the smsc911x driver probe function (smsc911x_drv_probe() in
drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
Reading the gpio-omap.txt documentation it says that #interrupt-cells
should be 2 and that a value of 8 is active low level-sensitive.
So I tried this:
gpmc {
ethernet@5,0 {
pinctrl-names = default;
pinctrl-0 = smsc911x_pins;
compatible = smsc,lan9221, smsc,lan9115;
reg = 5 0 0xff; /* CS5 */
interrupt-parent = gpio6;
interrupts = 16 8; /* gpio line 176 */
interrupt-names = smsc911x irq;
vmmc-supply = vddvario;
vmmc_aux-supply = vdd33a;
reg-io-width = 4;
smsc,save-mac-address;
};
};
Are you requesting the gpio anywhere? If not then this is not going to
work as-is. This was discussed fairly recently [1] and the conclusion
was that the gpio needs to be requested before we can use as an interrupt.
I have not seen your latest smsc code for omap, but when you are
requesting the gpmc chip-select you should also request the gpio.
But in the smsc911x probe function:
irq_res-flags IRQF_TRIGGER_MASK;
returns 0 which means that no trigger flags where set.
I took a look to the GPIOs device node definition on omap{3,4,5}.dtsi
and all look like this:
e.g from omap3.dtsi:
gpio6: gpio@49058000 {
compatible = ti,omap3-gpio;
ti,hwmods = gpio6;
gpio-controller;
#gpio-cells = 2;
interrupt-controller;
#interrupt-cells = 1;
};
So, even when the documentation says that all the GPIO device nodes in
OMAP2+ should use a #interrupt-cells property value of 2, they are
only using 1. Changing that value to 2 makes hangs the kernel and it
does not boot.
I will need to take a look at that. Is your code available anywhere so I
can test?
Cheers
Jon
[1] http://permalink.gmane.org/gmane.linux.ports.arm.omap/92192
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 02/26/2013 03:40 PM, Jon Hunter wrote:
On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
[snip]
I was wondering if the level/edge settings for gpios is working on OMAP.
I'm adding DT support for an SMSC911x ethernet chip connected to the
GPMC for an OMAP3 SoC based board.
In the smsc911x driver probe function (smsc911x_drv_probe() in
drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
Reading the gpio-omap.txt documentation it says that #interrupt-cells
should be 2 and that a value of 8 is active low level-sensitive.
So I tried this:
gpmc {
ethernet@5,0 {
pinctrl-names = default;
pinctrl-0 = smsc911x_pins;
compatible = smsc,lan9221, smsc,lan9115;
reg = 5 0 0xff; /* CS5 */
interrupt-parent = gpio6;
interrupts = 16 8; /* gpio line 176 */
interrupt-names = smsc911x irq;
vmmc-supply = vddvario;
vmmc_aux-supply = vdd33a;
reg-io-width = 4;
smsc,save-mac-address;
};
};
Are you requesting the gpio anywhere? If not then this is not going to
work as-is. This was discussed fairly recently [1] and the conclusion
was that the gpio needs to be requested before we can use as an interrupt.
That seems wrong; the GPIO/IRQ driver should handle this internally. The
Ethernet driver shouldn't know/care whether the interrupt it's given is
some form of dedicated interrupt or a GPIO-based interrupt, and even if
it somehow did, there's no irq_to_gpio() any more, so the driver can't
tell which GPIO ID it should request, unless it's given yet another
property to represent this.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 02/26/2013 04:44 PM, Stephen Warren wrote:
On 02/26/2013 03:40 PM, Jon Hunter wrote:
On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
[snip]
I was wondering if the level/edge settings for gpios is working on OMAP.
I'm adding DT support for an SMSC911x ethernet chip connected to the
GPMC for an OMAP3 SoC based board.
In the smsc911x driver probe function (smsc911x_drv_probe() in
drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
Reading the gpio-omap.txt documentation it says that #interrupt-cells
should be 2 and that a value of 8 is active low level-sensitive.
So I tried this:
gpmc {
ethernet@5,0 {
pinctrl-names = default;
pinctrl-0 = smsc911x_pins;
compatible = smsc,lan9221, smsc,lan9115;
reg = 5 0 0xff; /* CS5 */
interrupt-parent = gpio6;
interrupts = 16 8; /* gpio line 176 */
interrupt-names = smsc911x irq;
vmmc-supply = vddvario;
vmmc_aux-supply = vdd33a;
reg-io-width = 4;
smsc,save-mac-address;
};
};
Are you requesting the gpio anywhere? If not then this is not going to
work as-is. This was discussed fairly recently [1] and the conclusion
was that the gpio needs to be requested before we can use as an interrupt.
That seems wrong; the GPIO/IRQ driver should handle this internally. The
Ethernet driver shouldn't know/care whether the interrupt it's given is
some form of dedicated interrupt or a GPIO-based interrupt, and even if
it somehow did, there's no irq_to_gpio() any more, so the driver can't
tell which GPIO ID it should request, unless it's given yet another
property to represent this.
I agree that ideally this should be handled internally. Did you read the
discussion on the thread that I referenced [1]? If you have any thoughts
we are open to ideas :-)
Cheers
Jon
[1] http://comments.gmane.org/gmane.linux.ports.arm.omap/92192
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 02/26/2013 04:01 PM, Jon Hunter wrote:
On 02/26/2013 04:44 PM, Stephen Warren wrote:
On 02/26/2013 03:40 PM, Jon Hunter wrote:
On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
[snip]
I was wondering if the level/edge settings for gpios is working on OMAP.
I'm adding DT support for an SMSC911x ethernet chip connected to the
GPMC for an OMAP3 SoC based board.
In the smsc911x driver probe function (smsc911x_drv_probe() in
drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
Reading the gpio-omap.txt documentation it says that #interrupt-cells
should be 2 and that a value of 8 is active low level-sensitive.
So I tried this:
gpmc {
ethernet@5,0 {
pinctrl-names = default;
pinctrl-0 = smsc911x_pins;
compatible = smsc,lan9221, smsc,lan9115;
reg = 5 0 0xff; /* CS5 */
interrupt-parent = gpio6;
interrupts = 16 8; /* gpio line 176 */
interrupt-names = smsc911x irq;
vmmc-supply = vddvario;
vmmc_aux-supply = vdd33a;
reg-io-width = 4;
smsc,save-mac-address;
};
};
Are you requesting the gpio anywhere? If not then this is not going to
work as-is. This was discussed fairly recently [1] and the conclusion
was that the gpio needs to be requested before we can use as an interrupt.
That seems wrong; the GPIO/IRQ driver should handle this internally. The
Ethernet driver shouldn't know/care whether the interrupt it's given is
some form of dedicated interrupt or a GPIO-based interrupt, and even if
it somehow did, there's no irq_to_gpio() any more, so the driver can't
tell which GPIO ID it should request, unless it's given yet another
property to represent this.
I agree that ideally this should be handled internally. Did you read the
discussion on the thread that I referenced [1]? If you have any thoughts
we are open to ideas :-)
Cheers
Jon
[1] http://comments.gmane.org/gmane.linux.ports.arm.omap/92192
Oh, when I clicked that link the first time, all I saw was the patch,
not any discussion. I guess it must have timed out finding the other
emails or something.
I disagree that the GPIO needs to be requested, and that a custom DT
property and associated code are needed for that; simply requesting the
IRQ should be enough to make everything work.
In the Tegra GPIO IRQ driver for example, the irq_set_type irq_chip op
goes and configures the base GPIO HW to enable the pin as a GPIO, just
like gpio_request() would. I imagine the OMAP driver can do whatever
similar action it needs.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
[snip]
I was wondering if the level/edge settings for gpios is working on OMAP.
I'm adding DT support for an SMSC911x ethernet chip connected to the
GPMC for an OMAP3 SoC based board.
In the smsc911x driver probe function (smsc911x_drv_probe() in
drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
Reading the gpio-omap.txt documentation it says that #interrupt-cells
should be 2 and that a value of 8 is active low level-sensitive.
So I tried this:
gpmc {
ethernet@5,0 {
pinctrl-names = default;
pinctrl-0 = smsc911x_pins;
compatible = smsc,lan9221, smsc,lan9115;
reg = 5 0 0xff; /* CS5 */
interrupt-parent = gpio6;
interrupts = 16 8; /* gpio line 176 */
interrupt-names = smsc911x irq;
vmmc-supply = vddvario;
vmmc_aux-supply = vdd33a;
reg-io-width = 4;
By the way, reg-io-width for omap does not look correct. The GPMC only
supports 8-bit or 16-bit devices IIRC. I believe all my omap boards use
16-bit.
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 02/26/2013 05:06 PM, Stephen Warren wrote:
On 02/26/2013 04:01 PM, Jon Hunter wrote:
On 02/26/2013 04:44 PM, Stephen Warren wrote:
On 02/26/2013 03:40 PM, Jon Hunter wrote:
On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
[snip]
I was wondering if the level/edge settings for gpios is working on OMAP.
I'm adding DT support for an SMSC911x ethernet chip connected to the
GPMC for an OMAP3 SoC based board.
In the smsc911x driver probe function (smsc911x_drv_probe() in
drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
Reading the gpio-omap.txt documentation it says that #interrupt-cells
should be 2 and that a value of 8 is active low level-sensitive.
So I tried this:
gpmc {
ethernet@5,0 {
pinctrl-names = default;
pinctrl-0 = smsc911x_pins;
compatible = smsc,lan9221, smsc,lan9115;
reg = 5 0 0xff; /* CS5 */
interrupt-parent = gpio6;
interrupts = 16 8; /* gpio line 176 */
interrupt-names = smsc911x irq;
vmmc-supply = vddvario;
vmmc_aux-supply = vdd33a;
reg-io-width = 4;
smsc,save-mac-address;
};
};
Are you requesting the gpio anywhere? If not then this is not going to
work as-is. This was discussed fairly recently [1] and the conclusion
was that the gpio needs to be requested before we can use as an interrupt.
That seems wrong; the GPIO/IRQ driver should handle this internally. The
Ethernet driver shouldn't know/care whether the interrupt it's given is
some form of dedicated interrupt or a GPIO-based interrupt, and even if
it somehow did, there's no irq_to_gpio() any more, so the driver can't
tell which GPIO ID it should request, unless it's given yet another
property to represent this.
I agree that ideally this should be handled internally. Did you read the
discussion on the thread that I referenced [1]? If you have any thoughts
we are open to ideas :-)
Cheers
Jon
[1] http://comments.gmane.org/gmane.linux.ports.arm.omap/92192
Oh, when I clicked that link the first time, all I saw was the patch,
not any discussion. I guess it must have timed out finding the other
emails or something.
Actually, I sent a slightly different link the 2nd time to ensure you
saw the thread. So my fault ;-)
I disagree that the GPIO needs to be requested, and that a custom DT
property and associated code are needed for that; simply requesting the
IRQ should be enough to make everything work.
In the Tegra GPIO IRQ driver for example, the irq_set_type irq_chip op
goes and configures the base GPIO HW to enable the pin as a GPIO, just
like gpio_request() would. I imagine the OMAP driver can do whatever
similar action it needs.
Yes that is similar to what the patch in the thread was attempting to
do, but this got shot down.
One issue I see is that by not calling gpio_request, then potentially
you could have someone request a gpio via gpio_request() and someone
trying to use it as an interrupt source via request_irq(). Now obviously
that represents a bug because there is only one physical gpio, but I
gather it is something we need to protect against.
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 02/26/2013 04:45 PM, Jon Hunter wrote:
On 02/26/2013 05:06 PM, Stephen Warren wrote:
On 02/26/2013 04:01 PM, Jon Hunter wrote:
On 02/26/2013 04:44 PM, Stephen Warren wrote:
On 02/26/2013 03:40 PM, Jon Hunter wrote:
On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
[snip]
I was wondering if the level/edge settings for gpios is working on OMAP.
I'm adding DT support for an SMSC911x ethernet chip connected to the
GPMC for an OMAP3 SoC based board.
In the smsc911x driver probe function (smsc911x_drv_probe() in
drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
Reading the gpio-omap.txt documentation it says that #interrupt-cells
should be 2 and that a value of 8 is active low level-sensitive.
So I tried this:
gpmc {
ethernet@5,0 {
pinctrl-names = default;
pinctrl-0 = smsc911x_pins;
compatible = smsc,lan9221, smsc,lan9115;
reg = 5 0 0xff; /* CS5 */
interrupt-parent = gpio6;
interrupts = 16 8; /* gpio line 176 */
interrupt-names = smsc911x irq;
vmmc-supply = vddvario;
vmmc_aux-supply = vdd33a;
reg-io-width = 4;
smsc,save-mac-address;
};
};
Are you requesting the gpio anywhere? If not then this is not going to
work as-is. This was discussed fairly recently [1] and the conclusion
was that the gpio needs to be requested before we can use as an interrupt.
That seems wrong; the GPIO/IRQ driver should handle this internally. The
Ethernet driver shouldn't know/care whether the interrupt it's given is
some form of dedicated interrupt or a GPIO-based interrupt, and even if
it somehow did, there's no irq_to_gpio() any more, so the driver can't
tell which GPIO ID it should request, unless it's given yet another
property to represent this.
I agree that ideally this should be handled internally. Did you read the
discussion on the thread that I referenced [1]? If you have any thoughts
we are open to ideas :-)
Cheers
Jon
[1] http://comments.gmane.org/gmane.linux.ports.arm.omap/92192
Oh, when I clicked that link the first time, all I saw was the patch,
not any discussion. I guess it must have timed out finding the other
emails or something.
Actually, I sent a slightly different link the 2nd time to ensure you
saw the thread. So my fault ;-)
I disagree that the GPIO needs to be requested, and that a custom DT
property and associated code are needed for that; simply requesting the
IRQ should be enough to make everything work.
In the Tegra GPIO IRQ driver for example, the irq_set_type irq_chip op
goes and configures the base GPIO HW to enable the pin as a GPIO, just
like gpio_request() would. I imagine the OMAP driver can do whatever
similar action it needs.
Yes that is similar to what the patch in the thread was attempting to
do, but this got shot down.
One issue I see is that by not calling gpio_request, then potentially
you could have someone request a gpio via gpio_request() and someone
trying to use it as an interrupt source via request_irq(). Now obviously
that represents a bug because there is only one physical gpio, but I
gather it is something we need to protect against.
I'm not sure it's really that much of an issue, but presumably the
solution is for a combined GPIO+IRQ driver to simply call gpio_request
internally from within some irq_chip function. It looks like struct
irq_chip doesn't have a request/free, but perhaps they could be added to
solve this?
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 02/26/2013 06:13 PM, Stephen Warren wrote:
On 02/26/2013 04:45 PM, Jon Hunter wrote:
On 02/26/2013 05:06 PM, Stephen Warren wrote:
On 02/26/2013 04:01 PM, Jon Hunter wrote:
On 02/26/2013 04:44 PM, Stephen Warren wrote:
On 02/26/2013 03:40 PM, Jon Hunter wrote:
On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
[snip]
I was wondering if the level/edge settings for gpios is working on OMAP.
I'm adding DT support for an SMSC911x ethernet chip connected to the
GPMC for an OMAP3 SoC based board.
In the smsc911x driver probe function (smsc911x_drv_probe() in
drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
Reading the gpio-omap.txt documentation it says that #interrupt-cells
should be 2 and that a value of 8 is active low level-sensitive.
So I tried this:
gpmc {
ethernet@5,0 {
pinctrl-names = default;
pinctrl-0 = smsc911x_pins;
compatible = smsc,lan9221, smsc,lan9115;
reg = 5 0 0xff; /* CS5 */
interrupt-parent = gpio6;
interrupts = 16 8; /* gpio line 176 */
interrupt-names = smsc911x irq;
vmmc-supply = vddvario;
vmmc_aux-supply = vdd33a;
reg-io-width = 4;
smsc,save-mac-address;
};
};
Are you requesting the gpio anywhere? If not then this is not going to
work as-is. This was discussed fairly recently [1] and the conclusion
was that the gpio needs to be requested before we can use as an
interrupt.
That seems wrong; the GPIO/IRQ driver should handle this internally. The
Ethernet driver shouldn't know/care whether the interrupt it's given is
some form of dedicated interrupt or a GPIO-based interrupt, and even if
it somehow did, there's no irq_to_gpio() any more, so the driver can't
tell which GPIO ID it should request, unless it's given yet another
property to represent this.
I agree that ideally this should be handled internally. Did you read the
discussion on the thread that I referenced [1]? If you have any thoughts
we are open to ideas :-)
Cheers
Jon
[1] http://comments.gmane.org/gmane.linux.ports.arm.omap/92192
Oh, when I clicked that link the first time, all I saw was the patch,
not any discussion. I guess it must have timed out finding the other
emails or something.
Actually, I sent a slightly different link the 2nd time to ensure you
saw the thread. So my fault ;-)
I disagree that the GPIO needs to be requested, and that a custom DT
property and associated code are needed for that; simply requesting the
IRQ should be enough to make everything work.
In the Tegra GPIO IRQ driver for example, the irq_set_type irq_chip op
goes and configures the base GPIO HW to enable the pin as a GPIO, just
like gpio_request() would. I imagine the OMAP driver can do whatever
similar action it needs.
Yes that is similar to what the patch in the thread was attempting to
do, but this got shot down.
One issue I see is that by not calling gpio_request, then potentially
you could have someone request a gpio via gpio_request() and someone
trying to use it as an interrupt source via request_irq(). Now obviously
that represents a bug because there is only one physical gpio, but I
gather it is something we need to protect against.
I'm not sure it's really that much of an issue, but presumably the
solution is for a combined GPIO+IRQ driver to simply call gpio_request
internally from within some irq_chip function. It looks like struct
irq_chip doesn't have a request/free, but perhaps they could be added to
solve this?
Yes I was wondering if we could do something like that. That would work,
may be that's what we should propose.
Thanks
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Tue, Feb 26, 2013 at 11:40 PM, Jon Hunter jon-hun...@ti.com wrote:
On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
[snip]
I was wondering if the level/edge settings for gpios is working on OMAP.
I'm adding DT support for an SMSC911x ethernet chip connected to the
GPMC for an OMAP3 SoC based board.
In the smsc911x driver probe function (smsc911x_drv_probe() in
drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
Reading the gpio-omap.txt documentation it says that #interrupt-cells
should be 2 and that a value of 8 is active low level-sensitive.
So I tried this:
gpmc {
ethernet@5,0 {
pinctrl-names = default;
pinctrl-0 = smsc911x_pins;
compatible = smsc,lan9221, smsc,lan9115;
reg = 5 0 0xff; /* CS5 */
interrupt-parent = gpio6;
interrupts = 16 8; /* gpio line 176 */
interrupt-names = smsc911x irq;
vmmc-supply = vddvario;
vmmc_aux-supply = vdd33a;
reg-io-width = 4;
smsc,save-mac-address;
};
};
Are you requesting the gpio anywhere? If not then this is not going to
work as-is. This was discussed fairly recently [1] and the conclusion
was that the gpio needs to be requested before we can use as an interrupt.
I have not seen your latest smsc code for omap, but when you are
requesting the gpmc chip-select you should also request the gpio.
Yes, I realized that requesting the gpio was necessary so what I did
is to use the regulator-fixed optional property gpio and define
the GPIO used as an IRQ in a regulator used by the SMSC chip. So, I
have this on my board DT:
vddvario: regulator-vddvario {
compatible = regulator-fixed;
regulator-name = vddvario;
regulator-always-on;
gpio = gpio6 16 8; /* gpio line 176 */
enable-active-high;
gpio-open-drain;
regulator-boot-on;
};
gpmc {
ethernet@5,0 {
pinctrl-names = default;
pinctrl-0 = smsc911x_pins;
compatible = smsc,lan9221, smsc,lan9115;
reg = 5 0 0xff;
interrupt-parent = gpio6;
interrupts = 16 8; /* gpio line 176 */
interrupt-names = smsc911x irq;
vmmc-supply = vddvario;
vmmc_aux-supply = vdd33a;
reg-io-width = 4;
smsc,save-mac-address;
};
};
That way a call to gpio_request_one() is made and the GPIO is requested.
This look a little hack-ish for me but I've seen this in other
DeviceTrees like omap4-sdp.dts so I thought it was a common DT
pattern.
But in the smsc911x probe function:
irq_res-flags IRQF_TRIGGER_MASK;
returns 0 which means that no trigger flags where set.
I took a look to the GPIOs device node definition on omap{3,4,5}.dtsi
and all look like this:
e.g from omap3.dtsi:
gpio6: gpio@49058000 {
compatible = ti,omap3-gpio;
ti,hwmods = gpio6;
gpio-controller;
#gpio-cells = 2;
interrupt-controller;
#interrupt-cells = 1;
};
So, even when the documentation says that all the GPIO device nodes in
OMAP2+ should use a #interrupt-cells property value of 2, they are
only using 1. Changing that value to 2 makes hangs the kernel and it
does not boot.
I will need to take a look at that. Is your code available anywhere so I
can test?
Of course, I pushed a gpmc-smsc911x branch to my github linux repository [2].
The branch is latest Linus' master + Benoit's linux-omap-dt/for_3.9/dts +
ARM: OMAP2+: Prevent potential crash if GPMC probe fails [3] +
ARM: dts: OMAP3: Add GPMC controller [4] + my patches:
Javier Martinez Canillas (5):
ARM: dts: OMAP3: reduce GPMC mapped registers address space
ARM: dts: OMAP3: make GPMC node compatible with simple-bus
ARM: dts: OMAP3: add ranges property for GPMC chip-select 5
ARM: dts: omap3-igep0020: Add SMSC911x LAN chip support
smsc: smc911x: (HACK) force active low polarity for IRQ
The last patch is just an ugly hack that forces the IRQ flags to
active low level-sensitive so the SMSC911x IRQ is triggered on my
board (IGEPv2).
I just added for testing purposes since the omap3-gpio
interrupt-controller #interrupt-cells = 2 seems to not be working
and I can't pass this flag when defining the IRQ in the smsc911x
ethernet device node.
Cheers
Jon
Thanks a lot for your help and best regards,
Javier
[1]: http://permalink.gmane.org/gmane.linux.ports.arm.omap/92192
[2]: https://github.com/martinezjavier/linux.git
[3]: https://patchwork.kernel.org/patch/2118831/
[4]: https://patchwork.kernel.org/patch/2057111/
--
To unsubscribe from this list: send the line unsubscribe
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Wed, Feb 27, 2013 at 12:08 AM, Jon Hunter jon-hun...@ti.com wrote:
On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
[snip]
I was wondering if the level/edge settings for gpios is working on OMAP.
I'm adding DT support for an SMSC911x ethernet chip connected to the
GPMC for an OMAP3 SoC based board.
In the smsc911x driver probe function (smsc911x_drv_probe() in
drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
Reading the gpio-omap.txt documentation it says that #interrupt-cells
should be 2 and that a value of 8 is active low level-sensitive.
So I tried this:
gpmc {
ethernet@5,0 {
pinctrl-names = default;
pinctrl-0 = smsc911x_pins;
compatible = smsc,lan9221, smsc,lan9115;
reg = 5 0 0xff; /* CS5 */
interrupt-parent = gpio6;
interrupts = 16 8; /* gpio line 176 */
interrupt-names = smsc911x irq;
vmmc-supply = vddvario;
vmmc_aux-supply = vdd33a;
reg-io-width = 4;
By the way, reg-io-width for omap does not look correct. The GPMC only
supports 8-bit or 16-bit devices IIRC. I believe all my omap boards use
16-bit.
I thought that even when the GPMC was a 16-bit external memory
controller it could do some access adaptation to support 32-bit
devices.
By looking at the board files for others OMAP3 based boards
(board-{omap3evm,overo,zoom-debugboard}.c) most of them set the struct
omap_smsc911x_platform_data .flags member to SMSC911X_USE_32BIT.
And by looking at Documentation/devicetree/bindings/net/smsc911x.txt I
thought that the corresponding DT property for this flag was
reg-io-width
Anyway, I tried using both reg-io-width = 4 and reg-io-width = 2.
The ethernet chip works with both of them and I don't see too much
difference in performance:
16-bit
round-trip min/avg/max = 0.611/0.738/0.946 ms
32-bit
round-trip min/avg/max = 0.519/0.690/0.976 ms
So, is your call ;-)
Cheers
Jon
Thanks a lot for your help,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Wed, Feb 27, 2013 at 2:07 AM, Jon Hunter jon-hun...@ti.com wrote:
On 02/26/2013 06:13 PM, Stephen Warren wrote:
On 02/26/2013 04:45 PM, Jon Hunter wrote:
On 02/26/2013 05:06 PM, Stephen Warren wrote:
On 02/26/2013 04:01 PM, Jon Hunter wrote:
On 02/26/2013 04:44 PM, Stephen Warren wrote:
On 02/26/2013 03:40 PM, Jon Hunter wrote:
On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
[snip]
I was wondering if the level/edge settings for gpios is working on
OMAP.
I'm adding DT support for an SMSC911x ethernet chip connected to the
GPMC for an OMAP3 SoC based board.
In the smsc911x driver probe function (smsc911x_drv_probe() in
drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
Reading the gpio-omap.txt documentation it says that #interrupt-cells
should be 2 and that a value of 8 is active low level-sensitive.
So I tried this:
gpmc {
ethernet@5,0 {
pinctrl-names = default;
pinctrl-0 = smsc911x_pins;
compatible = smsc,lan9221, smsc,lan9115;
reg = 5 0 0xff; /* CS5 */
interrupt-parent = gpio6;
interrupts = 16 8; /* gpio line 176 */
interrupt-names = smsc911x irq;
vmmc-supply = vddvario;
vmmc_aux-supply = vdd33a;
reg-io-width = 4;
smsc,save-mac-address;
};
};
Are you requesting the gpio anywhere? If not then this is not going to
work as-is. This was discussed fairly recently [1] and the conclusion
was that the gpio needs to be requested before we can use as an
interrupt.
That seems wrong; the GPIO/IRQ driver should handle this internally. The
Ethernet driver shouldn't know/care whether the interrupt it's given is
some form of dedicated interrupt or a GPIO-based interrupt, and even if
it somehow did, there's no irq_to_gpio() any more, so the driver can't
tell which GPIO ID it should request, unless it's given yet another
property to represent this.
I agree that ideally this should be handled internally. Did you read the
discussion on the thread that I referenced [1]? If you have any thoughts
we are open to ideas :-)
Cheers
Jon
[1] http://comments.gmane.org/gmane.linux.ports.arm.omap/92192
Oh, when I clicked that link the first time, all I saw was the patch,
not any discussion. I guess it must have timed out finding the other
emails or something.
Actually, I sent a slightly different link the 2nd time to ensure you
saw the thread. So my fault ;-)
I disagree that the GPIO needs to be requested, and that a custom DT
property and associated code are needed for that; simply requesting the
IRQ should be enough to make everything work.
In the Tegra GPIO IRQ driver for example, the irq_set_type irq_chip op
goes and configures the base GPIO HW to enable the pin as a GPIO, just
like gpio_request() would. I imagine the OMAP driver can do whatever
similar action it needs.
Yes that is similar to what the patch in the thread was attempting to
do, but this got shot down.
One issue I see is that by not calling gpio_request, then potentially
you could have someone request a gpio via gpio_request() and someone
trying to use it as an interrupt source via request_irq(). Now obviously
that represents a bug because there is only one physical gpio, but I
gather it is something we need to protect against.
I'm not sure it's really that much of an issue, but presumably the
solution is for a combined GPIO+IRQ driver to simply call gpio_request
internally from within some irq_chip function. It looks like struct
irq_chip doesn't have a request/free, but perhaps they could be added to
solve this?
Yes I was wondering if we could do something like that. That would work,
may be that's what we should propose.
Thanks
Jon
Something like that would definitely solve the GPIO request issue but
we still have the issue that the current OMAP GPIO controller binding
does not support #interrupt-cells = 2.
So, we can't pass the trigger type and level flags for an IRQ-GPIO
when using an GPIO controller as the interrupt-parent for a device
node.
Do you have any comments on that issue?
I'll try to check Stephen's pointers but I'm not familiar with the
gpio-omap driver neither gpiolib.
Best regards,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 2/22/2012 7:29 PM, Stephen Warren wrote: Rob Herring wrote at Wednesday, February 22, 2012 10:23 AM: On 02/22/2012 08:31 AM, Cousson, Benoit wrote: On 2/22/2012 3:23 PM, Rob Herring wrote: On 02/15/2012 10:04 AM, Benoit Cousson wrote: Adapt the GPIO driver to retrieve information from a DT file. Allocate the irq_base dynamically and rename bank-virtual_irq_start to bank-irq_base. Change irq_base type to int instead of u16 to match irq_alloc_descs output. Add documentation for GPIO properties specific to OMAP. Signed-off-by: Benoit Coussonb-cous...@ti.com Cc: Tarun Kanti DebBarmatarun.ka...@ti.com One comment below, but otherwise: Acked-by: Rob Herringrob.herr...@calxeda.com --- .../devicetree/bindings/gpio/gpio-omap.txt | 30 + drivers/gpio/gpio-omap.c | 121 ++-- 2 files changed, 142 insertions(+), 9 deletions(-) create mode 100644 Documentation/devicetree/bindings/gpio/gpio-omap.txt diff --git a/Documentation/devicetree/bindings/gpio/gpio-omap.txt b/Documentation/devicetree/bindings/gpio/gpio-omap.txt new file mode 100644 index 000..c1b3100 --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/gpio-omap.txt @@ -0,0 +1,30 @@ +OMAP GPIO controller bindings + +Required properties: +- compatible: + - ti,omap2-gpio for OMAP2 controllers + - ti,omap3-gpio for OMAP3 controllers + - ti,omap4-gpio for OMAP4 controllers +- #gpio-cells : Should be two. + - first cell is the pin number + - second cell is used to specify optional parameters (unused) +- gpio-controller : Marks the device node as a GPIO controller. +- #interrupt-cells : Should be one There's no level/edge settings for gpios? That's a good question, because I was wondering as well :-) I did no see how it was done in other GPIO implementation. There's not really a good example that I've found. Many gpio nodes don't even have interrupt-controller set. So if you have an irq_set_type function for gpio's, then you should have 2 cells. After checking the OMAP gpio code, I do have a gpio_irq_type and the GPIO do support the 4 types. I'm not sure yet about the combinations. Tegra's GPIO IRQ binding (gpio_nvidia.txt in linux-next at least) says: - #interrupt-cells : Should be 2. The first cell is the GPIO number. The second cell is used to specify flags: bits[3:0] trigger type and level flags: 1 = low-to-high edge triggered. 2 = high-to-low edge triggered. 4 = active high level-sensitive. 8 = active low level-sensitive. Valid combinations are 1, 2, 3, 4, 8. Indeed, so I'll just copy / paste Tegra's binding... Thanks, Benoit -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 02/15/2012 10:04 AM, Benoit Cousson wrote:
Adapt the GPIO driver to retrieve information from a DT file.
Allocate the irq_base dynamically and rename bank-virtual_irq_start
to bank-irq_base.
Change irq_base type to int instead of u16 to match irq_alloc_descs
output.
Add documentation for GPIO properties specific to OMAP.
Signed-off-by: Benoit Cousson b-cous...@ti.com
Cc: Tarun Kanti DebBarma tarun.ka...@ti.com
One comment below, but otherwise:
Acked-by: Rob Herring rob.herr...@calxeda.com
---
.../devicetree/bindings/gpio/gpio-omap.txt | 30 +
drivers/gpio/gpio-omap.c | 121
++--
2 files changed, 142 insertions(+), 9 deletions(-)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-omap.txt
diff --git a/Documentation/devicetree/bindings/gpio/gpio-omap.txt
b/Documentation/devicetree/bindings/gpio/gpio-omap.txt
new file mode 100644
index 000..c1b3100
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-omap.txt
@@ -0,0 +1,30 @@
+OMAP GPIO controller bindings
+
+Required properties:
+- compatible:
+ - ti,omap2-gpio for OMAP2 controllers
+ - ti,omap3-gpio for OMAP3 controllers
+ - ti,omap4-gpio for OMAP4 controllers
+- #gpio-cells : Should be two.
+ - first cell is the pin number
+ - second cell is used to specify optional parameters (unused)
+- gpio-controller : Marks the device node as a GPIO controller.
+- #interrupt-cells : Should be one
There's no level/edge settings for gpios?
+- interrupt-controller: Mark the device node as an interrupt controller
+
+OMAP specific properties:
+- ti,hwmods: Name of the hwmod associated to the GPIO:
+ gpioX, X being the 1-based instance number from the HW spec
+
+
+Example:
+
+gpio4: gpio4 {
+compatible = ti,omap4-gpio;
+ti,hwmods = gpio4;
+#gpio-cells = 2;
+gpio-controller;
+#interrupt-cells = 1;
+interrupt-controller;
+};
+
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index c3a9dc8..bc2bd69 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -22,6 +22,9 @@
#include linux/device.h
#include linux/pm_runtime.h
#include linux/pm.h
+#include linux/of.h
+#include linux/of_device.h
+#include linux/irqdomain.h
#include mach/hardware.h
#include asm/irq.h
@@ -52,7 +55,8 @@ struct gpio_bank {
struct list_head node;
void __iomem *base;
u16 irq;
- u16 virtual_irq_start;
+ int irq_base;
+ struct irq_domain *domain;
u32 suspend_wakeup;
u32 saved_wakeup;
u32 non_wakeup_gpios;
@@ -669,7 +673,7 @@ static void gpio_irq_handler(unsigned int irq, struct
irq_desc *desc)
if (!isr)
break;
- gpio_irq = bank-virtual_irq_start;
+ gpio_irq = bank-irq_base;
for (; isr != 0; isr = 1, gpio_irq++) {
gpio_index = GPIO_INDEX(bank, irq_to_gpio(gpio_irq));
@@ -915,7 +919,7 @@ static int gpio_2irq(struct gpio_chip *chip, unsigned
offset)
struct gpio_bank *bank;
bank = container_of(chip, struct gpio_bank, chip);
- return bank-virtual_irq_start + offset;
+ return bank-irq_base + offset;
}
/*-*/
@@ -1028,8 +1032,7 @@ static void __devinit omap_gpio_chip_init(struct
gpio_bank *bank)
gpiochip_add(bank-chip);
- for (j = bank-virtual_irq_start;
- j bank-virtual_irq_start + bank-width; j++) {
+ for (j = bank-irq_base; j bank-irq_base + bank-width; j++) {
irq_set_lockdep_class(j, gpio_lock_class);
irq_set_chip_data(j, bank);
if (bank-is_mpuio) {
@@ -1044,15 +1047,22 @@ static void __devinit omap_gpio_chip_init(struct
gpio_bank *bank)
irq_set_handler_data(bank-irq, bank);
}
+static const struct of_device_id omap_gpio_match[];
+
static int __devinit omap_gpio_probe(struct platform_device *pdev)
{
struct device *dev = pdev-dev;
+ struct device_node *node = dev-of_node;
+ const struct of_device_id *match;
struct omap_gpio_platform_data *pdata;
struct resource *res;
struct gpio_bank *bank;
int ret = 0;
- if (!dev-platform_data)
+ match = of_match_device(of_match_ptr(omap_gpio_match), dev);
+
+ pdata = match ? match-data : dev-platform_data;
+ if (!pdata)
return -EINVAL;
bank = devm_kzalloc(pdev-dev, sizeof(struct gpio_bank), GFP_KERNEL);
@@ -1068,9 +1078,6 @@ static int __devinit omap_gpio_probe(struct
platform_device *pdev)
}
bank-irq = res-start;
-
- pdata = pdev-dev.platform_data;
- bank-virtual_irq_start = pdata-virtual_irq_start;
bank-dev = dev;
bank-dbck_flag = pdata-dbck_flag;
bank-stride = pdata-bank_stride;
@@ -1080,6 +1087,18 @@ static int
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 2/22/2012 3:23 PM, Rob Herring wrote: On 02/15/2012 10:04 AM, Benoit Cousson wrote: Adapt the GPIO driver to retrieve information from a DT file. Allocate the irq_base dynamically and rename bank-virtual_irq_start to bank-irq_base. Change irq_base type to int instead of u16 to match irq_alloc_descs output. Add documentation for GPIO properties specific to OMAP. Signed-off-by: Benoit Coussonb-cous...@ti.com Cc: Tarun Kanti DebBarmatarun.ka...@ti.com One comment below, but otherwise: Acked-by: Rob Herringrob.herr...@calxeda.com --- .../devicetree/bindings/gpio/gpio-omap.txt | 30 + drivers/gpio/gpio-omap.c | 121 ++-- 2 files changed, 142 insertions(+), 9 deletions(-) create mode 100644 Documentation/devicetree/bindings/gpio/gpio-omap.txt diff --git a/Documentation/devicetree/bindings/gpio/gpio-omap.txt b/Documentation/devicetree/bindings/gpio/gpio-omap.txt new file mode 100644 index 000..c1b3100 --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/gpio-omap.txt @@ -0,0 +1,30 @@ +OMAP GPIO controller bindings + +Required properties: +- compatible: + - ti,omap2-gpio for OMAP2 controllers + - ti,omap3-gpio for OMAP3 controllers + - ti,omap4-gpio for OMAP4 controllers +- #gpio-cells : Should be two. + - first cell is the pin number + - second cell is used to specify optional parameters (unused) +- gpio-controller : Marks the device node as a GPIO controller. +- #interrupt-cells : Should be one There's no level/edge settings for gpios? That's a good question, because I was wondering as well :-) I did no see how it was done in other GPIO implementation. Benoit -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On 02/22/2012 08:31 AM, Cousson, Benoit wrote: On 2/22/2012 3:23 PM, Rob Herring wrote: On 02/15/2012 10:04 AM, Benoit Cousson wrote: Adapt the GPIO driver to retrieve information from a DT file. Allocate the irq_base dynamically and rename bank-virtual_irq_start to bank-irq_base. Change irq_base type to int instead of u16 to match irq_alloc_descs output. Add documentation for GPIO properties specific to OMAP. Signed-off-by: Benoit Coussonb-cous...@ti.com Cc: Tarun Kanti DebBarmatarun.ka...@ti.com One comment below, but otherwise: Acked-by: Rob Herringrob.herr...@calxeda.com --- .../devicetree/bindings/gpio/gpio-omap.txt | 30 + drivers/gpio/gpio-omap.c | 121 ++-- 2 files changed, 142 insertions(+), 9 deletions(-) create mode 100644 Documentation/devicetree/bindings/gpio/gpio-omap.txt diff --git a/Documentation/devicetree/bindings/gpio/gpio-omap.txt b/Documentation/devicetree/bindings/gpio/gpio-omap.txt new file mode 100644 index 000..c1b3100 --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/gpio-omap.txt @@ -0,0 +1,30 @@ +OMAP GPIO controller bindings + +Required properties: +- compatible: + - ti,omap2-gpio for OMAP2 controllers + - ti,omap3-gpio for OMAP3 controllers + - ti,omap4-gpio for OMAP4 controllers +- #gpio-cells : Should be two. + - first cell is the pin number + - second cell is used to specify optional parameters (unused) +- gpio-controller : Marks the device node as a GPIO controller. +- #interrupt-cells : Should be one There's no level/edge settings for gpios? That's a good question, because I was wondering as well :-) I did no see how it was done in other GPIO implementation. There's not really a good example that I've found. Many gpio nodes don't even have interrupt-controller set. So if you have an irq_set_type function for gpio's, then you should have 2 cells. Rob -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
Rob Herring wrote at Wednesday, February 22, 2012 10:23 AM: On 02/22/2012 08:31 AM, Cousson, Benoit wrote: On 2/22/2012 3:23 PM, Rob Herring wrote: On 02/15/2012 10:04 AM, Benoit Cousson wrote: Adapt the GPIO driver to retrieve information from a DT file. Allocate the irq_base dynamically and rename bank-virtual_irq_start to bank-irq_base. Change irq_base type to int instead of u16 to match irq_alloc_descs output. Add documentation for GPIO properties specific to OMAP. Signed-off-by: Benoit Coussonb-cous...@ti.com Cc: Tarun Kanti DebBarmatarun.ka...@ti.com One comment below, but otherwise: Acked-by: Rob Herringrob.herr...@calxeda.com --- .../devicetree/bindings/gpio/gpio-omap.txt | 30 + drivers/gpio/gpio-omap.c | 121 ++-- 2 files changed, 142 insertions(+), 9 deletions(-) create mode 100644 Documentation/devicetree/bindings/gpio/gpio-omap.txt diff --git a/Documentation/devicetree/bindings/gpio/gpio-omap.txt b/Documentation/devicetree/bindings/gpio/gpio-omap.txt new file mode 100644 index 000..c1b3100 --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/gpio-omap.txt @@ -0,0 +1,30 @@ +OMAP GPIO controller bindings + +Required properties: +- compatible: + - ti,omap2-gpio for OMAP2 controllers + - ti,omap3-gpio for OMAP3 controllers + - ti,omap4-gpio for OMAP4 controllers +- #gpio-cells : Should be two. + - first cell is the pin number + - second cell is used to specify optional parameters (unused) +- gpio-controller : Marks the device node as a GPIO controller. +- #interrupt-cells : Should be one There's no level/edge settings for gpios? That's a good question, because I was wondering as well :-) I did no see how it was done in other GPIO implementation. There's not really a good example that I've found. Many gpio nodes don't even have interrupt-controller set. So if you have an irq_set_type function for gpio's, then you should have 2 cells. Tegra's GPIO IRQ binding (gpio_nvidia.txt in linux-next at least) says: - #interrupt-cells : Should be 2. The first cell is the GPIO number. The second cell is used to specify flags: bits[3:0] trigger type and level flags: 1 = low-to-high edge triggered. 2 = high-to-low edge triggered. 4 = active high level-sensitive. 8 = active low level-sensitive. Valid combinations are 1, 2, 3, 4, 8. Presumably, that's what you meant. -- nvpublic -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
