On 23 August 2013 10:34, Linus Walleij [off-list ref] wrote:

On Mon, Aug 19, 2013 at 8:59 PM, Markus Mayer [off-list ref] wrote:

quoted

From: Markus Mayer <mmayer@broadcom.com>

This patch adds the GPIO driver for the bcm281xx family of chips.

Signed-off-by: Markus Mayer <redacted>
Reviewed-by: Christian Daudt <redacted>
Reviewed-by: Tim Kryger <redacted>
Reviewed-by: Matt Porter <redacted>

This is getting into nice shape quickly, but at this point the driver
will still miss the v3.12 merge window as it seems, so let's
aim for v3.13.

Yes, it is starting to look that way.

quoted

+/* This function counts IRQ entries in the device tree */
+static int bcm_kona_count_irq_resources(struct platform_device *pdev)
+{
+       int i, ret;
+
+       for (i = 0;; i++) {
+               ret = platform_get_irq(pdev, i);
+               if (ret < 0)
+                       break;
+       }
+       return i;
+}

This looks weird, and it is not operating on a device tree but
on the platform resources created by the device tree core,
so atleast remove that comment. What it does is just count
the number of IRQs for this device, no matter where it came
from, right?

That function is actually gone now in my current working version. I
have taken Thomesz's advice and am using of_count_irq().

You are right that we only care about the number of IRQs, no matter
where they came from. In our case, they'd always come from device
tree, though.

quoted

+static void bcm_kona_gpio_set(struct gpio_chip *chip, unsigned gpio,
+       int value)
+{

(...)

quoted

+       val = readl(reg_base + reg_offset);
+       val |= 1 << bit;

I would really like you to do like this:

#include <linux/bitops.h>

val |= BIT(bit);

I'll incorporate the bit operations here and below.

[...]

quoted

+static void bcm_kona_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
+{
+       void __iomem *reg_base;
+       int bit, bank_id;
+       unsigned long sta;
+       struct bcm_kona_gpio_bank *bank = irq_get_handler_data(irq);
+       struct irq_chip *chip = irq_desc_get_chip(desc);
+
+       chained_irq_enter(chip, desc);
+
+       /*
+        * For bank interrupts, we can't use chip_data to store the kona_gpio
+        * pointer, since GIC needs it for its own purposes. Therefore, we get
+        * our pointer from the bank structure.
+        */
+       reg_base = bank->reg_base;
+       bank_id = bank->id;
+       bcm_kona_gpio_unlock_bank(reg_base, bank_id);
+
+       sta = readl(reg_base + GPIO_INT_STATUS(bank_id)) &
+           (~(readl(reg_base + GPIO_INT_MASK(bank_id))));
+
+       for_each_set_bit(bit, &sta, 32) {
+               /*
+                * Clear interrupt before handler is called so we don't
+                * miss any interrupt occurred during executing them.
+                */
+               writel(readl(reg_base + GPIO_INT_STATUS(bank_id)) |
+                               (1 << bit),

Use BIT(bit)

quoted

+                               reg_base + GPIO_INT_STATUS(bank_id));
+               /* Invoke interrupt handler */
+               generic_handle_irq(gpio_to_irq(GPIO_PER_BANK * bank_id + bit));
+       }

Usually you may want to re-read thet status for each iteration
of this loop, if IRQs appear as you are processing.

Will do.

quoted

+static int bcm_kona_gpio_probe(struct platform_device *pdev)
+{

(...)

quoted

+       kona_gpio->irq_base = irq_alloc_descs(-1, 0, chip->ngpio, 0);
+       if (kona_gpio->irq_base < 0) {
+               dev_err(dev, "Couldn't allocate IRQ numbers\n");
+               return -ENXIO;
+       }

As mentioned by Tomasz this looks strange.

Yep, it does, and it's gone now. It was a forgotten and overlooked
piece of code from an old version of the driver that used legacy
mapping. I took it out.

Thanks,
-Markus

-- 
Markus Mayer
Broadcom Landing Team