Messages in this thread | | | From | Grant Likely <> | Date | Wed, 14 Dec 2011 14:39:26 -0700 | Subject | Re: [PATCH 8/9] gpio: pl061: enable interrupts with DT style binding |
| |
On Wed, Dec 14, 2011 at 8:28 AM, Rob Herring <robherring2@gmail.com> wrote: > From: Rob Herring <rob.herring@calxeda.com> > > Enable DT interrupt binding support for pl061 gpio lines. If the gpio > node has an interrupt-controller property, then it will be setup to > handle interrupts on gpio lines. > > Signed-off-by: Rob Herring <rob.herring@calxeda.com> > Cc: Grant Likely <grant.likely@secretlab.ca> > Cc: Linus Walleij <linus.ml.walleij@gmail.com> > --- > .../devicetree/bindings/gpio/pl061-gpio.txt | 15 ++++++++++ > drivers/gpio/gpio-pl061.c | 29 +++++++++++-------- > 2 files changed, 32 insertions(+), 12 deletions(-) > > diff --git a/Documentation/devicetree/bindings/gpio/pl061-gpio.txt b/Documentation/devicetree/bindings/gpio/pl061-gpio.txt > index a2c416b..9671d4e 100644 > --- a/Documentation/devicetree/bindings/gpio/pl061-gpio.txt > +++ b/Documentation/devicetree/bindings/gpio/pl061-gpio.txt > @@ -8,3 +8,18 @@ Required properties: > - gpio-controller : Marks the device node as a GPIO controller. > - interrupts : Interrupt mapping for GPIO IRQ. > > +Optional properties: > +- interrupt-controller : Identifies the node as an interrupt controller. Must > + be present if using gpios lines for interrupts. > +- #interrupt-cells : Specifies the number of cells needed to encode an > + interrupt source. The type shall be a <u32> and the value shall be 2. > + > + The 1st cell contains the interrupt number 0-7 corresponding to the gpio > + line. > + > + The 2nd cell is the flags, encoding trigger type and level flags. > + 1 = low-to-high edge triggered > + 2 = high-to-low edge triggered > + 4 = active high level-sensitive > + 8 = active low level-sensitive > + > diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c > index 4a1874f..0151798 100644 > --- a/drivers/gpio/gpio-pl061.c > +++ b/drivers/gpio/gpio-pl061.c > @@ -16,6 +16,8 @@ > #include <linux/io.h> > #include <linux/ioport.h> > #include <linux/irq.h> > +#include <linux/irqdomain.h> > +#include <linux/of.h> > #include <linux/bitops.h> > #include <linux/workqueue.h> > #include <linux/gpio.h> > @@ -52,7 +54,7 @@ struct pl061_gpio { > spinlock_t lock; /* GPIO registers */ > > void __iomem *base; > - int irq_base; > + struct irq_domain *irq_domain; > struct gpio_chip gc; > }; > > @@ -117,18 +119,16 @@ static void pl061_set_value(struct gpio_chip *gc, unsigned offset, int value) > static int pl061_to_irq(struct gpio_chip *gc, unsigned offset) > { > struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc); > - > - if (chip->irq_base <= 0) > - return -EINVAL; > - > - return chip->irq_base + offset; > + if (!chip->irq_domain) > + return -ENXIO; > + return irq_domain_to_irq(chip->irq_domain, offset); > } > > static int pl061_irq_type(struct irq_data *d, unsigned trigger) > { > struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > struct pl061_gpio *chip = gc->private; > - int offset = d->irq - chip->irq_base; > + int offset = d->hwirq; > unsigned long flags; > u8 gpiois, gpioibe, gpioiev; > > @@ -212,6 +212,7 @@ static void __init pl061_init_gc(struct pl061_gpio *chip, int irq_base) > > irq_setup_generic_chip(gc, IRQ_MSK(PL061_GPIO_NR), > IRQ_GC_INIT_NESTED_LOCK, IRQ_NOREQUEST, 0); > + chip->irq_domain = gc->domain;
I would expect the driver to store chip->gc = gc instead. Then the driver has access to both structures, and if ever needed, the gc could be freed on driver release.
> } > > static int pl061_probe(struct amba_device *dev, const struct amba_id *id) > @@ -219,7 +220,7 @@ static int pl061_probe(struct amba_device *dev, const struct amba_id *id) > struct pl061_platform_data *pdata; > struct pl061_gpio *chip; > struct list_head *chip_list; > - int ret, irq, i; > + int ret, irq, i, irq_base; > static DECLARE_BITMAP(init_irq, NR_IRQS); > > chip = kzalloc(sizeof(*chip), GFP_KERNEL); > @@ -229,10 +230,13 @@ static int pl061_probe(struct amba_device *dev, const struct amba_id *id) > pdata = dev->dev.platform_data; > if (pdata) { > chip->gc.base = pdata->gpio_base; > - chip->irq_base = pdata->irq_base; > + irq_base = pdata->irq_base; > } else if (dev->dev.of_node) { > chip->gc.base = -1; > - chip->irq_base = 0; > + if (of_get_property(dev->dev.of_node, "interrupt-controller", NULL)) > + irq_base = -1; > + else > + irq_base = 0; > } else { > ret = -ENODEV; > goto free_mem; > @@ -271,10 +275,11 @@ static int pl061_probe(struct amba_device *dev, const struct amba_id *id) > * irq_chip support > */ > > - if (chip->irq_base <= 0) > + if (!irq_base) > return 0;
A comment would be useful here as it took a bit for me to work out what the options are here. What are the possible values of irq_base at this point, and what do they mean?
Otherwise looks pretty good.
g. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |