Message-ID: <CACRpkdZY0sZUK_9ZX8rBOExVRPQL=UeoAWT=O1NZX2wTJGQLsA@mail.gmail.com>
Date:	Wed, 27 Mar 2013 12:58:32 +0100
From:	Linus Walleij <linus.walleij@...aro.org>
To:	thloh@...era.com, Rob Herring <rob.herring@...xeda.com>
Cc:	Rob Landley <rob@...dley.net>, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, devicetree-discuss@...ts.ozlabs.org
Subject: Re: [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver

On Tue, Mar 12, 2013 at 6:37 AM,  <thloh@...era.com> wrote:

> From: Tien Hock Loh <thloh@...era.com>
>
> Adds a new driver for Altera soft GPIO IP. The driver is able to
> do read/write and allows GPIO to be a interrupt controller.
>
> Tested on Altera GHRD on interrupt handling and IO.
>
> Signed-off-by: Tien Hock Loh <thloh@...era.com>
> ---
>  .../devicetree/bindings/gpio/gpio-altera.txt       |  37 +++

Patches containing device tree bindings shall be CC:ed to devicetree-discuss
(added on CC here, remember at next posting).

(...)
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-altera.txt b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
> new file mode 100644
> index 0000000..3bdb8b6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
> @@ -0,0 +1,37 @@
> +Altera GPIO controller bindings
> +
> +Required properties:
> +- compatible:
> +  - "altr,pio-1.0"
> +- reg: Physical base address and length of the controller's registers.
> +- #gpio-cells : Should be two.
> +  - first cell is the pin number

What does that mean? The GPIO subsystem is not referring
to pins, it's about "gpios" which are just a handler or number.

It can be a local offset into the numberspace representiong
the GPIO lines at this block, I guess that is what is meant
here.

(...)
> +Altera GPIO specific properties:
> +- width: Width of the GPIO bank, range from 1-32
> +- level_trigger: Specifies whether the GPIO interrupt is level trigger.
> +  This field is required if the Altera GPIO controller used has IRQ enabled.

This is a configuration for the irc_chip portion of the driver
I guess, isn't the usual custom that you don't set up
things like this from the device tree, but instead use the
kernels facilities on request_irq() such as:

#define IRQF_TRIGGER_NONE       0x00000000
#define IRQF_TRIGGER_RISING     0x00000001
#define IRQF_TRIGGER_FALLING    0x00000002
#define IRQF_TRIGGER_HIGH       0x00000004
#define IRQF_TRIGGER_LOW        0x00000008

?

I.e. that this is something you do at runtime and not a static
config from the device tree?

Or shall this be percieved as some kind of default setting?

(...)
> +++ b/drivers/gpio/gpio-altera.c

> +struct altera_gpio_chip {
> +       struct of_mm_gpio_chip mmchip;
> +       struct irq_domain *irq; /* GPIO controller IRQ number */
> +       spinlock_t gpio_lock;   /* Lock used for synchronization */
> +       int level_trigger;

I doubt this member of the struct. The irq core shall keep track
of stuff like this.

> +       int hwirq;
> +};

(then follows some real nice code using irqdomain in a professional
way, thanks for doing this!)

> +static int altera_gpio_irq_set_type(struct irq_data *d,
> +                               unsigned int type)
> +{
> +       if (type == IRQ_TYPE_NONE)
> +               return 0;
> +       return -EINVAL;
> +}

So if the chip supports setting edge vs level trigging,
this is where you implement it, not as device tree
properties.

> +static void altera_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> +{
> +       struct altera_gpio_chip *altera_gc = irq_desc_get_handler_data(desc);
> +       struct irq_chip *chip = irq_desc_get_chip(desc);
> +       struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
> +       unsigned long status;
> +
> +       int base;
> +       chip->irq_mask(&desc->irq_data);
> +
> +       if (altera_gc->level_trigger)
> +               status = readl(mm_gc->regs + ALTERA_GPIO_DATA);

So on the level trigged variant you read the ALTERA_GPIO_DATA
and discard the result? Is it some kind of latch register?


> +       else {
> +               status = readl(mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
> +               writel(status, mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
> +       }

So you need to get the affected register depending on the type set
in the irq descriptor instead.

> +       status &= readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
> +       for (base = 0; base < mm_gc->gc.ngpio; base++) {
> +               if ((1 << base) & status) {
> +                       generic_handle_irq(
> +                               irq_linear_revmap(altera_gc->irq, base));
> +               }
> +       }

When we looked at this for some IRQ controllers we realized we
had to write the loop like so:

while ((stat = readl_relaxed(vic->base + VIC_IRQ_STATUS))) {
(...)
}

Only to avoid missing transient IRQs occuring while we were
processing another IRQ. (Like, you handle IRQ0, then IRQ4,
and while you're handling IRQ4, IRQ0 triggers again and
you miss it.

Make sure this does not apply to you...

> +       if (of_property_read_u32(node, "level_trigger", &reg)) {
> +               ret = -EINVAL;
> +               pr_err("%s: level_trigger value not set in device tree\n",
> +                       node->full_name);
> +               goto teardown;
> +       }
> +       altera_gc->level_trigger = reg;

So I'm suspicious about this.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists