On 09/24/2013 05:33 AM, Linus Walleij wrote:
> It is currently often possible in many GPIO drivers to request
> a GPIO line to be used as IRQ after calling gpio_to_irq() and,
> as the gpiolib is not aware of this, set the same line to
> output and start driving it, with undesired side effects.
>
> As it is a bogus usage scenario to request a line flagged as
> output to used as IRQ, we introduce APIs to let gpiolib track
> the use of a line as IRQ, and also set this flag from the
> userspace ABI.
>
> In this RFC patch we also augment the Nomadik pinctrl driver
> to use this API to give an idea of how it is to be used.
> It is probably not possible to lock line as IRQ in the gpiolib
> .to_irq() callback, as the line may have different use cases
> over time in a system. For a certain system or driver it may
> however be possible to lock the line as IRQ in the .to_irq()
> path. An alternative approach is to let the irq_chip
> .irq_enable() callback do this.
>
> The API is symmetric so that lines can also be unflagged from
> IRQ use by e.g. .irq_disable(). The debugfs file is altered
> so that we see if a line is reserved for IRQ.
OK, overall this seems like a reasonable general approach. I have a
couple comments on the implementation though:
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> @@ -466,6 +478,13 @@ static int gpio_setup_irq(struct gpio_desc *desc, struct
> device *dev,
> if (ret < 0)
> goto free_id;
>
> + ret = gpiod_lock_as_irq(desc);
> + if (ret < 0) {
> + gpiod_warn(desc,
> + "failed to flag the GPIO for IRQ\n");
> + goto free_id;
> + }
I believe gpio_setup_irq() can be called with flags==0 (disable IRQ
usage) or flags!=0 (set up IRQ usage). As such, I believe that block of
code needs to check flags, and either call gpiod_lock_as_irq() or
gpio_unlock_as_irq() as appropriate.
> diff --git a/drivers/pinctrl/pinctrl-nomadik.c
> b/drivers/pinctrl/pinctrl-nomadik.c
> @@ -795,6 +795,14 @@ static int nmk_gpio_to_irq(struct gpio_chip *chip,
> unsigned offset)
> {
> struct nmk_gpio_chip *nmk_chip =
> container_of(chip, struct nmk_gpio_chip, chip);
> + int ret;
> +
> + ret = gpio_lock_as_irq(chip, offset);
I don't think that gpio_to_irq() is the correct place to call the new
function, for two reasons:
1)
Not all paths that use interrupts call gpio_to_irq(). It's perfectly
valid for a driver to receive an IRQ number, request it, and be done.
The is commmon when a driver only cares about IRQ functionality and not
GPIO functionality, and hence did not receive a GPIO and convert it to
the IRQ.
To solve this, I think irq_chip drivers should call the new gpiolib
functions when the IRQ is actually requested or set up.
Related, where does gpio_unlock_as_irq() get called in the Nomadik
driver? It should happen when free_irq() is called.
2)
(Nit):
The fact that gpio_to_irq() was called doesn't actually guarantee that
the IRQ will be requested. Admittedly it's silly to call gpio_to_irq()
if you're not going to request the IRQ, adn this can be considered a
bug, but it can be done. This might happen (at least temporarily) due to
deferred probe.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
