On 09/16/2013 07:09 PM, Stephen Warren wrote: > On 09/16/2013 10:03 AM, Lars Poeschel wrote: >> On Monday 16 September 2013 13:43:50, Stephen Warren wrote: >>> On 09/10/2013 06:52 PM, Javier Martinez Canillas wrote: >>>> On 09/11/2013 12:34 AM, Stephen Warren wrote: >>>>> On 09/10/2013 03:37 PM, Mark Brown wrote: >>>>>> On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote: >>>>>>> Doesn't this patch call gpio_request() on the GPIO first, and >>>>>>> hence prevent the driver's own gpio_request() from succeeding, >>>>>>> since the GPIO is already requested? If this is not a problem, it >>>>>>> sounds like a bug in gpio_request() not ensuring mutual exclusion >>>>>>> for the GPIO. >>>>>> >>>>>> Or at the very least something that's likely to break in the >>>>>> future. >>>>> >>>>> Looking at the GPIO code, it already prevents double-requests: >>>>>> if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) { >>>>>> >>>>>> desc_set_label(desc, label ? : "?"); >>>>>> status = 0; >>>>>> >>>>>> } else { >>>>>> >>>>>> status = -EBUSY; >>>>>> module_put(chip->owner); >>>>>> goto done; >>>>>> >>>>>> } >>>>> >>>>> And I tested it in practice, and it really does fail. >>>> >>>> I'm a bit confused now. Doesn't the fact that gpio_request() prevents >>>> double-requests mean that the use-case that you say that have not been >>>> covered by this patch can't actually happen? >>>> >>>> I mean, if when using board files an explicit call to gpio_request() is >>>> made by platform code then a driver can't call gpio_request() for the >>>> same gpio. So this patch shouldn't cause any regression since is just >>>> auto-requesting a GPIO when is mapped as an IRQ in a DT which basically >>>> will be the same that was made by board files before. >>> >>> I'm not familiar with the board file path; Linus describe this. >> >> It seems Linus is busy, I'll try to help out. >> >>> It sounds like that path is for the case where a driver /only/ cares >>> about using a pin as an IRQ, and hence the driver only calls >>> request_irq(). The board file is (earlier) calling gpio_request() in >>> order to set up that input pin to work correctly as an IRQ. Hence, there >>> is no double-call to gpio_request(). >> >> No, a board file is not a path or something. A board file describes the wirings >> and specifics of an (embedded) computer in C code. The complete knowledge of >> how things are connected on a board and which drivers to use is in this piece >> of code. Devicetree replaces legacy board files. These two do pretty much the >> same, but board files have more power, because they are executed and can >> contain whatever code is needed to setup a board. >> But you are right, the driver only calls request_irq(), the board file set up >> the pin before and told the driver which irq to use. > > path == code path, or execution path. I'm well aware of what board files > are in general. > > I'm just not familiar with board files that employ this particular hack. > >>> The case I said wouldn't work is: >>> >>> * This patch calls gpio_request() in order to make the pin work as an IRQ. >>> >>> * Driver uses the pin as both a GPIO and an IRQ, and hence calls >>> gpio_request() and request_irq(). >>> >>> So, there's a double-call to gpio_request(), which fails, and the driver >>> fails to probe. >> >> Again, no. In that case you don't define your pin as irq in the device tree, >> but only as gpio. The driver knows how to handle gpios and turn them into irqs >> so you have to present it a gpio not an irq. In that case the patch will not >> call gpio_request() and there is no double-call to gpio_request(). > > That is a way to make this patch work, yes. However, there's no > guarantee that every driver or DT binding works this way. Forcing > bindings to work that way is forcing Linux-internal details upon > bindings, which should not be done. Put another way, I don't believe > there's any rule when writing DT bindings that states that bindings must > not describe the same pin as both a GPIO and an IRQ, although admittedly > that may be unusual. > > ... >> I agree with you that it would be the best if the only call would be >> request_irq and the chip driver programs the HW appropriately. It would be a >> dream, but unfortunately this is not possible at the moment. This is something >> that Linus pointed out very very early in this whole discussion. The gpio and >> irq frameworks don't share any information. The irq framework has no chance to >> program the HW, because it will never find the related gpio. >> For this to work the frameworks have to change (and possibly all drivers/board >> files/whatever using request_irq() and/or request_gpio()) have to change. >> That is something that I do not dare to do alone. > > This is a controller-specific issue, and has nothing to do with the GPIO > or IRQ frameworks. The driver for the combined irq/gpio_chip simply > needs to program the HW when the IRQ is requested or set up. The Tegra > driver already works this way, so there's actual proof that it is > possible to do this in practice. > Hi Stephen, I finally had some time to look at this and tried what you suggested, that is programming the hardware directly to do the setup when a IRQ is requested. I tested booting my OMAP3 board with DT and legacy booting and it both cases it works as expected. I sent a RFC patch "[RFC] gpio/omap: auto-setup a GPIO when used as an IRQ" [1]. It would be great if I can get some feedback from you to see if that is what you meant. Thanks a lot and best regards, Javier [1]: https://lkml.org/lkml/2013/9/22/78 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html