[PATCH v4 1/3] ARM: bcm281xx: Add GPIO driver

[Linus Walleij]

On Mon, Aug 19, 2013 at 8:59 PM, Markus Mayer <markus.mayer at linaro.org> wrote:

> From: Markus Mayer <mmayer at broadcom.com>
>
> This patch adds the GPIO driver for the bcm281xx family of chips.
>
> Signed-off-by: Markus Mayer <markus.mayer at linaro.org>
> Reviewed-by: Christian Daudt <csd at broadcom.com>
> Reviewed-by: Tim Kryger <tim.kryger at linaro.org>
> Reviewed-by: Matt Porter <matt.porter at linaro.org>

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.

> +/* 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?

> +static void bcm_kona_gpio_set(struct gpio_chip *chip, unsigned gpio,
> +       int value)
> +{
(...)
> +       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);

(...)
> +static int bcm_kona_gpio_get(struct gpio_chip *chip, unsigned gpio)
> +{
(...)
> +       /* return the specified bit status */
> +       return (val >> bit) & 1;

Please do like this:

return !!(val & bit);

> +static int bcm_kona_gpio_direction_output(struct gpio_chip *chip,
> +       unsigned gpio, int value)
> +{
(...)
> +       val = readl(reg_base + reg_offset);
> +       val |= 1 << bit;

val |= BIT(bit);

> +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)

> +                               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.

> +static int bcm_kona_gpio_probe(struct platform_device *pdev)
> +{
(...)
> +       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.

Apart from this last thing, the rest is really nitpicking comments
and no blockers.

Yours,
Linus Walleij