diff mbox

pinctrl/nomadik: use irq_create_mapping()

Message ID 1350659375-7335-1-git-send-email-linus.walleij@stericsson.com (mailing list archive)
State New, archived
Headers show

Commit Message

Linus Walleij Oct. 19, 2012, 3:09 p.m. UTC 
From: Linus Walleij <linus.walleij@linaro.org>

Since in the DT case, the linear domain path will not allocate
descriptors for the IRQs, we need to use irq_create_mapping()
for mapping hwirqs to Linux IRQs, so these descriptors get
created on-the-fly in this case.

Cc: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/pinctrl/pinctrl-nomadik.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Stephen Warren Oct. 19, 2012, 4:22 p.m. UTC | #1 
On 10/19/2012 09:09 AM, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> Since in the DT case, the linear domain path will not allocate
> descriptors for the IRQs, we need to use irq_create_mapping()
> for mapping hwirqs to Linux IRQs, so these descriptors get
> created on-the-fly in this case.

> @@ -931,7 +931,7 @@ static void __nmk_gpio_irq_handler(unsigned int irq, struct irq_desc *desc,
>  	while (status) {
>  		int bit = __ffs(status);
>  
> -		generic_handle_irq(irq_find_mapping(nmk_chip->domain, bit));
> +		generic_handle_irq(irq_create_mapping(nmk_chip->domain, bit));

Surely this one can remain as irq_find_mapping() since isn't
nmk_gpio_to_irq() guaranteed to have been called first for this GPIO/IRQ?
Linus Walleij Oct. 22, 2012, 8:14 a.m. UTC | #2 
On Fri, Oct 19, 2012 at 6:22 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 10/19/2012 09:09 AM, Linus Walleij wrote:
>> From: Linus Walleij <linus.walleij@linaro.org>
>
>> @@ -931,7 +931,7 @@ static void __nmk_gpio_irq_handler(unsigned int irq, struct irq_desc *desc,
>>       while (status) {
>>               int bit = __ffs(status);
>>
>> -             generic_handle_irq(irq_find_mapping(nmk_chip->domain, bit));
>> +             generic_handle_irq(irq_create_mapping(nmk_chip->domain, bit));
>
> Surely this one can remain as irq_find_mapping() since isn't
> nmk_gpio_to_irq() guaranteed to have been called first for this GPIO/IRQ?

It's an IRQ handler so it should be robust to spurious IRQs due to
transient hardware states etc I believe.

So if there is a transient IRQ before gpio_to_irq() is called -> boom.

Yours,
Linus Walleij
Stephen Warren Oct. 22, 2012, 8:08 p.m. UTC | #3 
On 10/22/2012 02:14 AM, Linus Walleij wrote:
> On Fri, Oct 19, 2012 at 6:22 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 10/19/2012 09:09 AM, Linus Walleij wrote:
>>> From: Linus Walleij <linus.walleij@linaro.org>
>>
>>> @@ -931,7 +931,7 @@ static void __nmk_gpio_irq_handler(unsigned int irq, struct irq_desc *desc,
>>>       while (status) {
>>>               int bit = __ffs(status);
>>>
>>> -             generic_handle_irq(irq_find_mapping(nmk_chip->domain, bit));
>>> +             generic_handle_irq(irq_create_mapping(nmk_chip->domain, bit));
>>
>> Surely this one can remain as irq_find_mapping() since isn't
>> nmk_gpio_to_irq() guaranteed to have been called first for this GPIO/IRQ?
> 
> It's an IRQ handler so it should be robust to spurious IRQs due to
> transient hardware states etc I believe.
> 
> So if there is a transient IRQ before gpio_to_irq() is called -> boom.

I wonder though (a) why it would be unmasked in HW, and (b) why the
software would even look at the status bit if no handler were registered?
Linus Walleij Oct. 23, 2012, 8:31 a.m. UTC | #4 
On Mon, Oct 22, 2012 at 10:08 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 10/22/2012 02:14 AM, Linus Walleij wrote:

>> It's an IRQ handler so it should be robust to spurious IRQs due to
>> transient hardware states etc I believe.
>>
>> So if there is a transient IRQ before gpio_to_irq() is called -> boom.
>
> I wonder though (a) why it would be unmasked in HW, and (b) why the
> software would even look at the status bit if no handler were registered?

That's true of course ... OK I'll update the patch.

Still I'm not feeling good about the irq_create_mapping/irq_find_mapping
separation, I think a lot of drivers just get this wrong and it's causing
bugs... it'd be way better if there was just one of them and we could
count on descriptors being allocated after adding any kind of irqdomain
but I have no clue how hard it would be to achieve this.

Yours,
Linus Walleij
diff mbox

Patch

diff --git a/drivers/pinctrl/pinctrl-nomadik.c b/drivers/pinctrl/pinctrl-nomadik.c
index 01aea1c..d1d3cb9 100644
--- a/drivers/pinctrl/pinctrl-nomadik.c
+++ b/drivers/pinctrl/pinctrl-nomadik.c
@@ -931,7 +931,7 @@  static void __nmk_gpio_irq_handler(unsigned int irq, struct irq_desc *desc,
 	while (status) {
 		int bit = __ffs(status);
 
-		generic_handle_irq(irq_find_mapping(nmk_chip->domain, bit));
+		generic_handle_irq(irq_create_mapping(nmk_chip->domain, bit));
 		status &= ~BIT(bit);
 	}
 
@@ -1056,7 +1056,7 @@  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);
 
-	return irq_find_mapping(nmk_chip->domain, offset);
+	return irq_create_mapping(nmk_chip->domain, offset);
 }
 
 #ifdef CONFIG_DEBUG_FS