Re: [PATCH 2/2] gpio: xlp: GPIO controller for Netlogic XLP SoCs

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Tue, Apr 28, 2015 at 4:40 PM,  <kamlakant.patel@xxxxxxxxxxxx> wrote:

> From: Kamlakant Patel <kamlakant.patel@xxxxxxxxxxxx>
>
> Add GPIO controller driver for Netlogic XLP MIPS64 SOCs.
>
> This driver is instantiated by device tree and supports interrupts
> for GPIOs.
>
> Signed-off-by: Kamlakant Patel <kamlakant.patel@xxxxxxxxxxxx>

Looking good, some stuff to go...

> +config GPIO_XLP
> +       tristate "Netlogic XLP GPIO support"
> +       depends on CPU_XLP
> +       select GPIOLIB_IRQCHIP

Don't you need:
depends on OF_GPIO

> +static irqreturn_t xlp_gpio_generic_handler(int irq, void *data)
> +{
> +       struct xlp_gpio_priv *priv = data;
> +       int gpio, regoff;
> +       u32 gpio_stat;
> +
> +       regoff = -1;
> +       gpio_stat = 0;

Usually we use a chained handler for this type of devices, meaning you
just get the IRQ and call gpiochip_set_chained_irqchip() in probe() and
do things like this in the handler:

struct irq_chip *host_chip = irq_get_chip(irq);

chained_irq_enter(host_chip, desc);

> +       for_each_set_bit(gpio, priv->gpio_enabled_mask, XLP_MAX_NR_GPIO) {
> +               if (regoff != gpio / XLP_GPIO_REGSZ) {
> +                       regoff = gpio / XLP_GPIO_REGSZ;
> +                       gpio_stat = readl(priv->gpio_intr_stat + regoff * 4);
> +               }
> +               if (gpio_stat & BIT(gpio % XLP_GPIO_REGSZ))
> +                       generic_handle_irq(irq_find_mapping(
> +                                               priv->chip.irqdomain, gpio));
> +       }

chained_irq_exit(host_chip, desc);

> +
> +       return IRQ_HANDLED;
> +}

Any place where the GPIO IRQ line is connected to a higher-order
IRQ controller this should be done like so...

(...)
> +static int xlp_gpio_probe(struct platform_device *pdev)
> +{
(...)
> +       irq_base = irq_alloc_descs(-1, XLP_GPIO_IRQ_BASE, gc->ngpio, 0);
> +       if (irq_base < 0) {
> +               dev_err(&pdev->dev, "Failed to allocate IRQ numbers\n");
> +               return err;
> +       }

Why are you doing this? GPIOLIB_IRQCHIP handles this for you.
Just drop this code and irq_base altogether.

> +       err = gpiochip_irqchip_add(gc, &xlp_gpio_irq_chip, irq_base,
> +                               handle_level_irq, IRQ_TYPE_NONE);

Pass 0 as irq_base and it will allocate IRQ descriptors.

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