On 10/11/2013 02:39 AM, Linus Walleij wrote:
> On Tue, Sep 24, 2013 at 7:51 PM, Stephen Warren <[email protected]> wrote:
>> On 09/24/2013 05:33 AM, Linus Walleij wrote:
>>> 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.
>
> Yeah if we formalize the criterion that interrupts out of any GPIO
> chips should be possible to request without first getting it from the
> <linux/gpio.h> interface, then this holds.
>
> However that is not the whole story, is it? We have a gazillion
> drivers calling irq_create_mapping() in this function, so I would
> say that things are already a mess here.
I expect things are a mess indeed:-)
I believe that if a driver is only calling irq_create_mapping() inside
gpio_to_irq(), it's a bug. I think things can operate correctly in one
of two cases, at least with DT:
1) irq_create_mapping() is called from both gpio_to_irq() and the
of_xlate callback for IRQs.
(I don't think this method would work in a board-file-based system where
of_xlate isn't called for IRQs...)
or:
2) irq_create_mapping() is called for all IRQs when registering the IRQ
controller/domain.
To me, (2) is much simpler, and avoids the issue (1) probably has with
only supporting direct IRQ usage (without something calling gpio_to_irq()).
> One alternative is to do what gpio-tegra.c does and call
> irq_create_mapping() on every GPIO line that can do IRQ in
> probe(). However that is a bit sloppy is it not? Or is this what
> we always want drivers to do?
I tend to think it's a nice simple approach that should support any
higher-level usage-model (direct IRQ usage, or "mapped" via gpio_to_irq()).
> This has the side effect of showing
> all these IRQs in /proc/interrupts but maybe that is not such
> a big deal?
I think that's actually a benefit; you can see all the possible IRQ
sources in the system, and whether each is handled, or not.
>> 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.
>
> Yeah well you're right it's just supposed to be a translation function.
>
> Part of me want to add an optional irqdomain to struct gpio_chip
> and have gpio_to_irq() just call irq_find_mapping() by default
> unless the driver specifies its own translation callback, because
> I think this is what (modern) drivers should generally do.
>
> What do you think about this refactoring idea?
That sounds reasonable.
--
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
