[PATCH 1/2] gpio: zynq: Reduce level of indention in zynq_gpio_irqhandler()

[PATCH 1/2] gpio: zynq: Reduce level of indention in zynq_gpio_irqhandler()

[Lars-Peter Clausen]

zynq_gpio_irqhandler() uses up to 7 tabs of indention in some parts. Refactor
things to use a helper function for the inner loop to reduce the indention to a
sane level.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/gpio/gpio-zynq.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
index 31ad5df..f440317 100644
--- a/drivers/gpio/gpio-zynq.c
+++ b/drivers/gpio/gpio-zynq.c
@@ -457,6 +457,24 @@ static struct irq_chip zynq_gpio_edge_irqchip = {
 	.irq_set_wake	= zynq_gpio_set_wake,
 };
 
+static void zynq_gpio_handle_bank_irq(struct zynq_gpio *gpio,
+				      unsigned int bank_num,
+				      unsigned long pending)
+{
+	struct irq_domain *irqdomain = gpio->chip.irqdomain;
+	int offset;
+
+	if (!pending)
+		return;
+
+	for_each_set_bit(offset, &pending, 32) {
+		unsigned int gpio_irq;
+
+		gpio_irq = irq_find_mapping(irqdomain, offset);
+		generic_handle_irq(gpio_irq);
+	}
+}
+
 /**
  * zynq_gpio_irqhandler - IRQ handler for the gpio banks of a gpio device
  * @irq:	irq number of the gpio bank where interrupt has occurred
@@ -482,18 +500,7 @@ static void zynq_gpio_irqhandler(unsigned int irq, struct irq_desc *desc)
 					ZYNQ_GPIO_INTSTS_OFFSET(bank_num));
 		int_enb = readl_relaxed(gpio->base_addr +
 					ZYNQ_GPIO_INTMASK_OFFSET(bank_num));
-		int_sts &= ~int_enb;
-		if (int_sts) {
-			int offset;
-			unsigned long pending = int_sts;
-
-			for_each_set_bit(offset, &pending, 32) {
-				unsigned int gpio_irq =
-					irq_find_mapping(gpio->chip.irqdomain,
-							offset);
-				generic_handle_irq(gpio_irq);
-			}
-		}
+		zynq_gpio_handle_bank_irq(gpio, bank_num, int_sts & ~int_enb);
 	}
 
 	chained_irq_exit(irqchip, desc);
-- 
1.8.0

[PATCH v2 2/2] gpio: zynq: Take bank offset into account when reporting a IRQ

[Lars-Peter Clausen]

When looking up the IRQ the bank offset needs to be taken into account.
Otherwise interrupts for banks other than bank 0 get incorrectly reported as
interrupts for bank 0.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
Changes since v1:
	* Use lookup table instead of switch-case.
---
 drivers/gpio/gpio-zynq.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
index f440317..5db1af7 100644
--- a/drivers/gpio/gpio-zynq.c
+++ b/drivers/gpio/gpio-zynq.c
@@ -138,6 +138,13 @@ static inline void zynq_gpio_get_bank_pin(unsigned int pin_num,
 	}
 }
 
+static const unsigned int zynq_gpio_bank_offset[] = {
+	ZYNQ_GPIO_BANK0_PIN_MIN,
+	ZYNQ_GPIO_BANK1_PIN_MIN,
+	ZYNQ_GPIO_BANK2_PIN_MIN,
+	ZYNQ_GPIO_BANK3_PIN_MIN,
+};
+
 /**
  * zynq_gpio_get_value - Get the state of the specified pin of GPIO device
  * @chip:	gpio_chip instance to be worked on
@@ -461,6 +468,7 @@ static void zynq_gpio_handle_bank_irq(struct zynq_gpio *gpio,
 				      unsigned int bank_num,
 				      unsigned long pending)
 {
+	unsigned int bank_offset = zynq_gpio_bank_offset[bank_num];
 	struct irq_domain *irqdomain = gpio->chip.irqdomain;
 	int offset;
 
@@ -470,7 +478,7 @@ static void zynq_gpio_handle_bank_irq(struct zynq_gpio *gpio,
 	for_each_set_bit(offset, &pending, 32) {
 		unsigned int gpio_irq;
 
-		gpio_irq = irq_find_mapping(irqdomain, offset);
+		gpio_irq = irq_find_mapping(irqdomain, offset + bank_offset);
 		generic_handle_irq(gpio_irq);
 	}
 }
-- 
1.8.0

Re: [PATCH v2 2/2] gpio: zynq: Take bank offset into account when reporting a IRQ

[Alexandre Courbot]

On Mon, Aug 18, 2014 at 2:54 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> When looking up the IRQ the bank offset needs to be taken into account.
> Otherwise interrupts for banks other than bank 0 get incorrectly reported as
> interrupts for bank 0.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
> Changes since v1:
>         * Use lookup table instead of switch-case.
> ---
>  drivers/gpio/gpio-zynq.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)

Yep, you saved 16 lines compared to the first version of this patch. :)

I assume overflows over this array cannot happen (otherwise a quick
check against ARRAY_SIZE(zynq_gpio_bank_offset) would do the trick),
if so:

Acked-by: Alexandre Courbot <acourbot@nvidia.com>

>
> diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
> index f440317..5db1af7 100644
> --- a/drivers/gpio/gpio-zynq.c
> +++ b/drivers/gpio/gpio-zynq.c
> @@ -138,6 +138,13 @@ static inline void zynq_gpio_get_bank_pin(unsigned int pin_num,
>         }
>  }
>
> +static const unsigned int zynq_gpio_bank_offset[] = {
> +       ZYNQ_GPIO_BANK0_PIN_MIN,
> +       ZYNQ_GPIO_BANK1_PIN_MIN,
> +       ZYNQ_GPIO_BANK2_PIN_MIN,
> +       ZYNQ_GPIO_BANK3_PIN_MIN,
> +};
> +
>  /**
>   * zynq_gpio_get_value - Get the state of the specified pin of GPIO device
>   * @chip:      gpio_chip instance to be worked on
> @@ -461,6 +468,7 @@ static void zynq_gpio_handle_bank_irq(struct zynq_gpio *gpio,
>                                       unsigned int bank_num,
>                                       unsigned long pending)
>  {
> +       unsigned int bank_offset = zynq_gpio_bank_offset[bank_num];
>         struct irq_domain *irqdomain = gpio->chip.irqdomain;
>         int offset;
>
> @@ -470,7 +478,7 @@ static void zynq_gpio_handle_bank_irq(struct zynq_gpio *gpio,
>         for_each_set_bit(offset, &pending, 32) {
>                 unsigned int gpio_irq;
>
> -               gpio_irq = irq_find_mapping(irqdomain, offset);
> +               gpio_irq = irq_find_mapping(irqdomain, offset + bank_offset);
>                 generic_handle_irq(gpio_irq);
>         }
>  }
> --
> 1.8.0
>

Re: [PATCH v2 2/2] gpio: zynq: Take bank offset into account when reporting a IRQ

[Sören Brinkmann]

On Mon, 2014-08-18 at 09:28AM -0700, Alexandre Courbot wrote:
> On Mon, Aug 18, 2014 at 2:54 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> > When looking up the IRQ the bank offset needs to be taken into account.
> > Otherwise interrupts for banks other than bank 0 get incorrectly reported as
> > interrupts for bank 0.
> >
> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > ---
> > Changes since v1:
> >         * Use lookup table instead of switch-case.
> > ---
> >  drivers/gpio/gpio-zynq.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> Yep, you saved 16 lines compared to the first version of this patch. :)
> 
> I assume overflows over this array cannot happen (otherwise a quick
> check against ARRAY_SIZE(zynq_gpio_bank_offset) would do the trick),
> if so:
I think that overflow is impossible since we are in a for-loop that runs
from 0..(ZYNQ_GPIO_MAX_BANK - 1).

> 
> Acked-by: Alexandre Courbot <acourbot@nvidia.com>

For the series:
Acked-by: Soren Brinkmann <soren.brinkmann@xilinx.com>

	Thanks,
	Sören

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] gpio: zynq: Take bank offset into account when reporting a IRQ

[Harini Katakam]

On Tue, Aug 19, 2014 at 12:24 AM, Sören Brinkmann
<soren.brinkmann@xilinx.com> wrote:
> On Mon, 2014-08-18 at 09:28AM -0700, Alexandre Courbot wrote:
>> On Mon, Aug 18, 2014 at 2:54 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> > When looking up the IRQ the bank offset needs to be taken into account.
>> > Otherwise interrupts for banks other than bank 0 get incorrectly reported as
>> > interrupts for bank 0.
>> >
>> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> > ---
>> > Changes since v1:
>> >         * Use lookup table instead of switch-case.
>> > ---
>> >  drivers/gpio/gpio-zynq.c | 10 +++++++++-
>> >  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> Yep, you saved 16 lines compared to the first version of this patch. :)
>>
>> I assume overflows over this array cannot happen (otherwise a quick
>> check against ARRAY_SIZE(zynq_gpio_bank_offset) would do the trick),
>> if so:
> I think that overflow is impossible since we are in a for-loop that runs
> from 0..(ZYNQ_GPIO_MAX_BANK - 1).
>
>>
>> Acked-by: Alexandre Courbot <acourbot@nvidia.com>
>
> For the series:
> Acked-by: Soren Brinkmann <soren.brinkmann@xilinx.com>

Acked-by: Harini Katakam <harinik@xilinx.com>

Regards,
Harini
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] gpio: zynq: Take bank offset into account when reporting a IRQ

[Linus Walleij]

On Mon, Aug 18, 2014 at 4:54 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:

> When looking up the IRQ the bank offset needs to be taken into account.
> Otherwise interrupts for banks other than bank 0 get incorrectly reported as
> interrupts for bank 0.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
> Changes since v1:
>         * Use lookup table instead of switch-case.

I tried to apply this, but it doesn't apply on top of the other patch
"gpio: zynq: Fix IRQ handlers" that I have queued for fixes.

Can you please take a look in the GPIO tree or linux-next and
check that this applied on top of that patch?

Collect all ACKs when you're at it.

Yours,
Linus Walleij

Re: [PATCH v2 2/2] gpio: zynq: Take bank offset into account when reporting a IRQ

[Lars-Peter Clausen]

On 08/21/2014 02:40 PM, Linus Walleij wrote:
> On Mon, Aug 18, 2014 at 4:54 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>
>> When looking up the IRQ the bank offset needs to be taken into account.
>> Otherwise interrupts for banks other than bank 0 get incorrectly reported as
>> interrupts for bank 0.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> ---
>> Changes since v1:
>>          * Use lookup table instead of switch-case.
>
> I tried to apply this, but it doesn't apply on top of the other patch
> "gpio: zynq: Fix IRQ handlers" that I have queued for fixes.

Works fine for me. Did you try to apply both patch 1 and 2 of this series or 
just patch 2?

Thanks,
- Lars

Re: [PATCH 1/2] gpio: zynq: Reduce level of indention in zynq_gpio_irqhandler()

[Linus Walleij]

On Mon, Aug 18, 2014 at 11:54 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:

> zynq_gpio_irqhandler() uses up to 7 tabs of indention in some parts. Refactor
> things to use a helper function for the inner loop to reduce the indention to a
> sane level.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH v2 2/2] gpio: zynq: Take bank offset into account when reporting a IRQ

[Linus Walleij]

On Thu, Aug 21, 2014 at 3:30 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 08/21/2014 02:40 PM, Linus Walleij wrote:
>>
>> On Mon, Aug 18, 2014 at 4:54 AM, Lars-Peter Clausen <lars@metafoo.de>
>> wrote:
>>
>>> When looking up the IRQ the bank offset needs to be taken into account.
>>> Otherwise interrupts for banks other than bank 0 get incorrectly reported
>>> as
>>> interrupts for bank 0.
>>>
>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>> ---
>>> Changes since v1:
>>>          * Use lookup table instead of switch-case.
>>
>>
>> I tried to apply this, but it doesn't apply on top of the other patch
>> "gpio: zynq: Fix IRQ handlers" that I have queued for fixes.
>
>
> Works fine for me. Did you try to apply both patch 1 and 2 of this series or
> just patch 2?

Yes, and on my fixes branch, as well as on Torvald's master (which now
has patch 1/2) this happens:

]$ git am --signoff zynq2.patch
Applying: gpio: zynq: Take bank offset into account when reporting a IRQ
error: patch failed: drivers/gpio/gpio-zynq.c:461
error: drivers/gpio/gpio-zynq.c: patch does not apply
Patch failed at 0001 gpio: zynq: Take bank offset into account when
reporting a IRQ

Can you please test applying this patch only on upstream this and figure
out what is wrong?

Yours,
Linus Walleij

Re: [PATCH v2 2/2] gpio: zynq: Take bank offset into account when reporting a IRQ

[Lars-Peter Clausen]

On 08/29/2014 09:20 AM, Linus Walleij wrote:
> On Thu, Aug 21, 2014 at 3:30 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 08/21/2014 02:40 PM, Linus Walleij wrote:
>>>
>>> On Mon, Aug 18, 2014 at 4:54 AM, Lars-Peter Clausen <lars@metafoo.de>
>>> wrote:
>>>
>>>> When looking up the IRQ the bank offset needs to be taken into account.
>>>> Otherwise interrupts for banks other than bank 0 get incorrectly reported
>>>> as
>>>> interrupts for bank 0.
>>>>
>>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>>> ---
>>>> Changes since v1:
>>>>           * Use lookup table instead of switch-case.
>>>
>>>
>>> I tried to apply this, but it doesn't apply on top of the other patch
>>> "gpio: zynq: Fix IRQ handlers" that I have queued for fixes.
>>
>>
>> Works fine for me. Did you try to apply both patch 1 and 2 of this series or
>> just patch 2?
>
> Yes, and on my fixes branch, as well as on Torvald's master (which now
> has patch 1/2) this happens:
>
> ]$ git am --signoff zynq2.patch
> Applying: gpio: zynq: Take bank offset into account when reporting a IRQ
> error: patch failed: drivers/gpio/gpio-zynq.c:461
> error: drivers/gpio/gpio-zynq.c: patch does not apply
> Patch failed at 0001 gpio: zynq: Take bank offset into account when
> reporting a IRQ
>
> Can you please test applying this patch only on upstream this and figure
> out what is wrong?

I think there is some confusion here, maybe two different series got mixed 
up. Patch 1 of this series is "gpio: zynq: Reduce level of indention in 
zynq_gpio_irqhandler()", which you just applied, but which is not in Linus' 
tree or the public gpio tree yet.

- Lars

Re: [PATCH v2 2/2] gpio: zynq: Take bank offset into account when reporting a IRQ

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 2060 bytes --]

Hi Lars and Linus,

On 08/29/2014 09:34 AM, Lars-Peter Clausen wrote:
> On 08/29/2014 09:20 AM, Linus Walleij wrote:
>> On Thu, Aug 21, 2014 at 3:30 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>> On 08/21/2014 02:40 PM, Linus Walleij wrote:
>>>>
>>>> On Mon, Aug 18, 2014 at 4:54 AM, Lars-Peter Clausen <lars@metafoo.de>
>>>> wrote:
>>>>
>>>>> When looking up the IRQ the bank offset needs to be taken into account.
>>>>> Otherwise interrupts for banks other than bank 0 get incorrectly reported
>>>>> as
>>>>> interrupts for bank 0.
>>>>>
>>>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>>>> ---
>>>>> Changes since v1:
>>>>>           * Use lookup table instead of switch-case.
>>>>
>>>>
>>>> I tried to apply this, but it doesn't apply on top of the other patch
>>>> "gpio: zynq: Fix IRQ handlers" that I have queued for fixes.
>>>
>>>
>>> Works fine for me. Did you try to apply both patch 1 and 2 of this series or
>>> just patch 2?
>>
>> Yes, and on my fixes branch, as well as on Torvald's master (which now
>> has patch 1/2) this happens:
>>
>> ]$ git am --signoff zynq2.patch
>> Applying: gpio: zynq: Take bank offset into account when reporting a IRQ
>> error: patch failed: drivers/gpio/gpio-zynq.c:461
>> error: drivers/gpio/gpio-zynq.c: patch does not apply
>> Patch failed at 0001 gpio: zynq: Take bank offset into account when
>> reporting a IRQ
>>
>> Can you please test applying this patch only on upstream this and figure
>> out what is wrong?
> 
> I think there is some confusion here, maybe two different series got mixed up. Patch 1 of this series is "gpio: zynq: Reduce level of indention in zynq_gpio_irqhandler()", which you just applied, but which is not in Linus' tree or the public gpio tree yet.

I have no problem to apply that patches on the top of Linus tree.
Here is the branch with that two patches.
http://git.monstr.eu/?p=linux-2.6-microblaze.git;a=shortlog;h=refs/heads/xnext/zynq-gpio

Feel free to cherry pick them from this branch.

Thanks,
Michal




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2 2/2] gpio: zynq: Take bank offset into account when reporting a IRQ

[Linus Walleij]

On Fri, Aug 29, 2014 at 11:33 AM, Michal Simek <monstr@monstr.eu> wrote:
> Hi Lars and Linus,
>
> On 08/29/2014 09:34 AM, Lars-Peter Clausen wrote:
>> On 08/29/2014 09:20 AM, Linus Walleij wrote:
>>> On Thu, Aug 21, 2014 at 3:30 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>> On 08/21/2014 02:40 PM, Linus Walleij wrote:
>>>>>
>>>>> On Mon, Aug 18, 2014 at 4:54 AM, Lars-Peter Clausen <lars@metafoo.de>
>>>>> wrote:
>>>>>
>>>>>> When looking up the IRQ the bank offset needs to be taken into account.
>>>>>> Otherwise interrupts for banks other than bank 0 get incorrectly reported
>>>>>> as
>>>>>> interrupts for bank 0.
>>>>>>
>>>>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>>>>> ---
>>>>>> Changes since v1:
>>>>>>           * Use lookup table instead of switch-case.
>>>>>
>>>>>
>>>>> I tried to apply this, but it doesn't apply on top of the other patch
>>>>> "gpio: zynq: Fix IRQ handlers" that I have queued for fixes.
>>>>
>>>>
>>>> Works fine for me. Did you try to apply both patch 1 and 2 of this series or
>>>> just patch 2?
>>>
>>> Yes, and on my fixes branch, as well as on Torvald's master (which now
>>> has patch 1/2) this happens:
>>>
>>> ]$ git am --signoff zynq2.patch
>>> Applying: gpio: zynq: Take bank offset into account when reporting a IRQ
>>> error: patch failed: drivers/gpio/gpio-zynq.c:461
>>> error: drivers/gpio/gpio-zynq.c: patch does not apply
>>> Patch failed at 0001 gpio: zynq: Take bank offset into account when
>>> reporting a IRQ
>>>
>>> Can you please test applying this patch only on upstream this and figure
>>> out what is wrong?
>>
>> I think there is some confusion here, maybe two different series got mixed up. Patch 1 of this series is "gpio: zynq: Reduce level of indention in zynq_gpio_irqhandler()", which you just applied, but which is not in Linus' tree or the public gpio tree yet.
>
> I have no problem to apply that patches on the top of Linus tree.
> Here is the branch with that two patches.
> http://git.monstr.eu/?p=linux-2.6-microblaze.git;a=shortlog;h=refs/heads/xnext/zynq-gpio
>
> Feel free to cherry pick them from this branch.

I had all except the last one, which worked fine when picked from your
tree.

Applied now then, thanks!

Yours,
Linus Walleij