On Thu, Oct 3, 2013 at 12:44 PM, <thloh.linux@xxxxxxxxx> wrote:
> From: Tien Hock Loh <thloh@xxxxxxxxxx>
>
> Add driver support for Altera GPIO soft IP, including interrupts and I/O.
> Tested on Altera CV SoC board using dipsw and LED using LED framework.
>
> Signed-off-by: Tien Hock Loh <thloh@xxxxxxxxxx>
> ---
> .../devicetree/bindings/gpio/gpio-altera.txt | 34 ++
This driver adds device tree bindings and need to be CC:
devicetree@xxxxxxxxxxxxxxx
on subsequent postings.
It will not be merged until a DT person ACKs it.
> +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
> @@ -0,0 +1,34 @@
> +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 1
> + - first cell is the gpio offset number
> +- gpio-controller : Marks the device node as a GPIO controller.
> +- #interrupt-cells : Should be 1.
> +- interrupts: Specify the interrupt.
> +- interrupt-controller: Mark the device node as an interrupt controller
> + The first cell is the local interrupt offset number to the GPIO controller.
> +
> +Altera GPIO specific properties:
> +- width: Width of the GPIO bank, range from 1-32
> +- interrupt_trigger: Specifies the interrupt trigger type the GPIO hardware is
> + synthesized. This field is required if the Altera GPIO controller used has
> + IRQ enabled as the interrupt type is not software controlled, but hardware
> + synthesized.
Ah yeah I remember this thing! :-)
(...)
> +static int altera_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
> +{
> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> + struct altera_gpio_chip *altera_gc = container_of(mm_gc,
> + struct altera_gpio_chip, mmchip);
> +
> + if (altera_gc->irq == 0)
> + return -ENXIO;
Just write it like this:
if (!altera_gc->irq)
> + if ((altera_gc->irq && offset) < altera_gc->mmchip.gc.ngpio)
This makes no sense. && is boolean logical AND. The expression
to the left of "<" will be 0 or 1.
> + return irq_create_mapping(altera_gc->irq, offset);
> + else
> + return -ENXIO;
> +}
(...)
> +
> +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;
Do you have to call this variable "base"? It's quite confusing for me,
can't you just call it "i"?
> +int altera_gpio_probe(struct platform_device *pdev)
> +{
(...)
> + struct device_node *node = pdev->dev.of_node;
> + if (of_get_property(node, "interrupts", ®) == NULL)
> + goto skip_irq;
> + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0);
I think this IRQ has already been added to the pdev as a resource
by the OF core code, and you'r redoing the work of the core here.
Doesn't this work:
irq = platform_get_irq(dev, 0);
?
> + if (altera_gc->mapped_irq == NO_IRQ)
> + goto skip_irq;
Do not reference NO_IRQ, it is always 0 these days.
Just do this:
if (!altera_gc->mapped_irq)
Apart from this it is starting to look real good!
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
