Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Tue, Jun 11, 2013 at 11:25 PM, Grant Likely wrote: > On Fri, 26 Apr 2013 16:31:24 -0500, Jon Hunter wrote: >> >> On 04/26/2013 02:31 AM, Linus Walleij wrote: >> > On Wed, Apr 17, 2013 at 2:41 AM, Javier Martinez Canillas >> > 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 wrote: > > On 04/26/2013 02:31 AM, Linus Walleij wrote: > > On Wed, Apr 17, 2013 at 2:41 AM, Javier Martinez Canillas > > 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 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 04/26/2013 02:31 AM, Linus Walleij wrote: > On Wed, Apr 17, 2013 at 2:41 AM, Javier Martinez Canillas > 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-kernel&m=136701158117966&w=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 04/26/2013 02:27 AM, Linus Walleij wrote: > On Wed, Apr 17, 2013 at 5:41 PM, Stephen Warren 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 Wed, Apr 17, 2013 at 2:41 AM, Javier Martinez Canillas 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 Wed, Apr 17, 2013 at 5:41 PM, Stephen Warren 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 Mon, Apr 15, 2013 at 11:40 PM, Jon Hunter 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 Mon, Apr 15, 2013 at 6:58 PM, Stephen Warren 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 Wed, Apr 17, 2013 at 3:52 PM, Jon Hunter wrote: > > On 04/17/2013 08:42 AM, Javier Martinez Canillas wrote: >> On Wed, Apr 17, 2013 at 3:25 PM, Jon Hunter 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-kernel&m=136606204823845&w=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/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 wrote: > > On 04/17/2013 08:42 AM, Javier Martinez Canillas wrote: >> On Wed, Apr 17, 2013 at 3:25 PM, Jon Hunter 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-kernel&m=136606204823845&w=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/17/2013 08:42 AM, Javier Martinez Canillas wrote: > On Wed, Apr 17, 2013 at 3:25 PM, Jon Hunter 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-kernel&m=136606204823845&w=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:25 PM, Jon Hunter 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-kernel&m=136606204823845&w=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] [] (unwind_backtrace+0x0/0xf0) from >> [] (warn_slowpath_common+0x4c/0x68) >> [0.285552] [] (warn_slowpath_common+0x4c/0x68) from >> [] (warn_slowpath_null+0x1c/0x24) >> [0.285614] [] (warn_slowpath_null+0x1c/0x24) from >> [] (of_device_alloc+0x154/0x168) >> [0.285675] [] (of_device_alloc+0x154/0x168) from >> [] (of_platform_device_create_pdata+0x34/0x80) >> [0.285736] [] >> (of_platform_device_create_pdata+0x34/0x80) from [] >> (gpmc_probe_generic_child+0x180/0x240) >> [0.285827] [] (gpmc_probe_generic_child+0x180/0x240) >> from [] (gpmc_probe+0x4b4/0x614) >> [0.285888] [] (gpmc_probe+0x4b4/0x614) from [] >> (platform_drv_probe+0x18/0x1c) >> [0.285949] [] (platform_drv_probe+0x18/0x1c) from >> [] (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 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 --- 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"; + reg = <5 0 0xff>; + bank-width = <2>; + +
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-kernel&m=136606204823845&w=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] [] (unwind_backtrace+0x0/0xf0) from > [] (warn_slowpath_common+0x4c/0x68) > [0.285552] [] (warn_slowpath_common+0x4c/0x68) from > [] (warn_slowpath_null+0x1c/0x24) > [0.285614] [] (warn_slowpath_null+0x1c/0x24) from > [] (of_device_alloc+0x154/0x168) > [0.285675] [] (of_device_alloc+0x154/0x168) from > [] (of_platform_device_create_pdata+0x34/0x80) > [0.285736] [] > (of_platform_device_create_pdata+0x34/0x80) from [] > (gpmc_probe_generic_child+0x180/0x240) > [0.285827] [] (gpmc_probe_generic_child+0x180/0x240) > from [] (gpmc_probe+0x4b4/0x614) > [0.285888] [] (gpmc_probe+0x4b4/0x614) from [] > (platform_drv_probe+0x18/0x1c) > [0.285949] [] (platform_drv_probe+0x18/0x1c) from > [] (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 4:00 AM, Jon Hunter wrote: > > On 04/16/2013 07:41 PM, Javier Martinez Canillas wrote: >> On Wed, Apr 17, 2013 at 1:14 AM, 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 >> >> 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 >> 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 >> --- >> 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_
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 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 > 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 > --- > 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) >
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Wed, Apr 17, 2013 at 1:14 AM, 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 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 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 --- 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_op
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 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 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 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 #include #include +#include #include #include #include @@ -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 #include #include +#include +#include #include #include #include @@ -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: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/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/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 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 is
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 >>> 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 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 >>> 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 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 >> 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 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/14/2013 02:53 PM, Linus Walleij wrote: > On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas > 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/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 Sun, Apr 14, 2013 at 10:53 PM, Linus Walleij wrote: > On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas > 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 Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas 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 wrote: > On 04/11/2013 04:16 PM, Linus Walleij wrote: >> On Thu, Apr 11, 2013 at 10:30 PM, Stephen Warren >> 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-kernel&m=136334655902407&w=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/11/2013 04:16 PM, Linus Walleij wrote: > On Thu, Apr 11, 2013 at 10:30 PM, Stephen Warren > 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-kernel&m=136334655902407&w=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 04/11/2013 04:16 PM, Linus Walleij wrote: > On Thu, Apr 11, 2013 at 10:30 PM, Stephen Warren > 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-kernel&m=136334655902407&w=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 Fri, Apr 12, 2013 at 12:16 AM, Linus Walleij wrote: > On Thu, Apr 11, 2013 at 10:30 PM, Stephen Warren > 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-kernel&m=136334655902407&w=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 Thu, Apr 11, 2013 at 10:30 PM, Stephen Warren 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-kernel&m=136334655902407&w=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 04/10/2013 03:28 PM, Linus Walleij wrote: > On Wed, Apr 10, 2013 at 10:29 PM, Stephen Warren > 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 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 Wed, Apr 10, 2013 at 10:29 PM, Stephen Warren 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 04/10/2013 12:12 PM, Linus Walleij wrote: > On Fri, Mar 29, 2013 at 6:01 PM, Stephen Warren wrote: >> On 03/27/2013 02:55 PM, Linus Walleij wrote: >>> On Wed, Mar 27, 2013 at 5:09 PM, Stephen Warren >>> 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(Ă°ernet, 17, >>> IORESOURCE_IRQ_HIGHEDGE, Ă°ernet_res[1]); >>> platform_device_register(Ă°ernet); >>> >>> 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
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Fri, Mar 29, 2013 at 6:01 PM, Stephen Warren wrote: > On 03/27/2013 02:55 PM, Linus Walleij wrote: >> On Wed, Mar 27, 2013 at 5:09 PM, Stephen Warren >> 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(Ă°ernet, 17, >> IORESOURCE_IRQ_HIGHEDGE, Ă°ernet_res[1]); >> platform_device_register(Ă°ernet); >> >> 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 03/27/2013 02:55 PM, Linus Walleij wrote: > On Wed, Mar 27, 2013 at 5:09 PM, Stephen Warren 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(Ă°ernet, 17, > IORESOURCE_IRQ_HIGHEDGE, Ă°ernet_res[1]); > platform_device_register(Ă°ernet); > > 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 ahe
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Wed, Mar 27, 2013 at 5:09 PM, Stephen Warren 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(Ă°ernet, 17, IORESOURCE_IRQ_HIGHEDGE, Ă°ernet_res[1]); platform_device_register(Ă°ernet); 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
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 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 Fri, Mar 22, 2013 at 11:52 PM, Jon Hunter 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 /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/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 >> 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 03/22/2013 02:10 AM, Linus Walleij wrote: > On Fri, Mar 15, 2013 at 12:21 PM, Javier Martinez Canillas > 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 Fri, Mar 15, 2013 at 12:21 PM, Javier Martinez Canillas 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 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 Fri, Mar 8, 2013 at 12:14 AM, Jon Hunter wrote: > > On 03/02/2013 02:05 PM, Grant Likely wrote: >> On Tue, 26 Feb 2013 17:01:22 -0600, 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: > 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 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 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 02/28/2013 06:09 PM, Linus Walleij wrote: > On Thu, Feb 28, 2013 at 2:04 PM, Benoit Cousson wrote: > >> On 02/28/2013 12:41 AM, Linus Walleij wrote: >>> On Wed, Feb 15, 2012 at 5:04 PM, Benoit Cousson 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 Thu, Feb 28, 2013 at 2:04 PM, Benoit Cousson wrote: > On 02/28/2013 12:41 AM, Linus Walleij wrote: >> On Wed, Feb 15, 2012 at 5:04 PM, Benoit Cousson 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:17 AM, Javier Martinez Canillas wrote: > On Thu, Feb 28, 2013 at 12:16 AM, Jon Hunter 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
+ 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 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 Thu, Feb 28, 2013 at 12:16 AM, Jon Hunter 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 > 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
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Wed, Feb 15, 2012 at 5:04 PM, Benoit Cousson 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 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 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 02/26/2013 09:47 PM, Javier Martinez Canillas wrote: > On Wed, Feb 27, 2013 at 12:08 AM, 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>; >> >> 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 Wed, Feb 27, 2013 at 6:50 PM, Stephen Warren wrote: > On 02/26/2013 08:57 PM, Javier Martinez Canillas wrote: >> On Wed, Feb 27, 2013 at 2:07 AM, Jon Hunter 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 Wed, Feb 27, 2013 at 6:47 PM, Stephen Warren 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 02/26/2013 08:57 PM, Javier Martinez Canillas wrote: > On Wed, Feb 27, 2013 at 2:07 AM, Jon Hunter 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 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 Wed, Feb 27, 2013 at 2:07 AM, Jon Hunter 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
Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver
On Wed, Feb 27, 2013 at 12:08 AM, 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>; > > 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 Tue, Feb 26, 2013 at 11: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. > > 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]: h
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 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 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: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 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: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 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: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: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 Fri, Feb 24, 2012 at 4:30 PM, Cousson, Benoit 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 Cousson >> Cc: Tarun Kanti DebBarma > > > One comment below, but otherwise: > > Acked-by: Rob Herring > >> --- >>.../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
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 Cousson Cc: Tarun Kanti DebBarma One comment below, but otherwise: Acked-by: Rob Herring --- .../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
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 Cousson > >>> Cc: Tarun Kanti DebBarma > >> > >> One comment below, but otherwise: > >> > >> Acked-by: Rob Herring > >> > >>> --- > >>> .../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
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 Cousson >>> Cc: Tarun Kanti DebBarma >> >> One comment below, but otherwise: >> >> Acked-by: Rob Herring >> >>> --- >>> .../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
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 Cousson Cc: Tarun Kanti DebBarma One comment below, but otherwise: Acked-by: Rob Herring --- .../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/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 > Cc: Tarun Kanti DebBarma One comment below, but otherwise: Acked-by: Rob Herring > --- > .../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: > + "gpio", 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 > #include > #include > +#include > +#include > +#include > > #include > #include > @@ -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
[PATCH 3/5] gpio/omap: Add DT support to GPIO driver
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 Cc: Tarun Kanti DebBarma --- .../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 +- interrupt-controller: Mark the device node as an interrupt controller + +OMAP specific properties: +- ti,hwmods: Name of the hwmod associated to the GPIO: + "gpio", 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 #include #include +#include +#include +#include #include #include @@ -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 __devinit omap_gpio_probe(struct platform_device *pdev) bank->loses_context = pdata->loses_context; bank->get_context_loss_count = pdata->get_context_loss_count; bank->regs = pdata->regs; +#ifdef CONFIG_OF_GPIO + bank->chip.of_node = of_node_get(node); +#endif + + bank->irq_base = irq_allo