diff mbox

[v8,03/12] gpio: pl061: allocate irq dynamically

Message ID 1360602659-4774-4-git-send-email-haojian.zhuang@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Haojian Zhuang Feb. 11, 2013, 5:10 p.m. UTC
In original implementation, irq base is always specified in platform
data. If it's not specified, pl061 gpio driver can't pass the probe()
function since irq base is missing. While moving to device tree, everything
should be parsed from DTS file. So allocate irq dynamically for irq
base.

Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
---
 drivers/gpio/gpio-pl061.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

Comments

Linus Walleij Feb. 14, 2013, 2:04 p.m. UTC | #1
On Mon, Feb 11, 2013 at 6:10 PM, Haojian Zhuang
<haojian.zhuang@linaro.org> wrote:

We don't convert drivers to irqdomain without actually *using*
the irqdomain. I don't know where this pattern comes from,
but I'm suspicious about patch sets that just focus on enabling
DT functionality without considering irqdomain stuff as a concept
of its own.

The biggest problem with this patch is what is missing from it:

- irq_base shall be *deleted* from struct pl061_gpio
- instead a struct irqdomain  * shall be stored for offsetting IRQs
- everywhere chip->irq_base is referenced, use irq_find_mapping()
- In pl061_to_irq, irq_create_mapping() shall be used, so it will
  allocate a descriptor even if we're using the driver with
  SPARSE_IRQ and a linear domain.

Look at e.g. gpio-langwell.c for guidance.

> +       chip->irq_base = irq_alloc_descs(chip->irq_base, 0, PL061_GPIO_NR, 0);
> +       if (chip->irq_base < 0)
> +               return chip->irq_base;
> +       if (!irq_domain_add_legacy(adev->dev.of_node, PL061_GPIO_NR,
> +                                  chip->irq_base, 0, &pl061_domain_ops, chip))
>                 return -ENODEV;

Instead of the above, please use:

chip->irqdomain = irq_domain_add_simple(adev->dev.of_node,
                                         PL061_GPIO_NR,
                                         chip->irq_base,
                                         &pl061_domain_ops,
                                         chip);

Notice that I don't throw the domain away after creation either...

This call will allocate descriptors for you as long as the
irq_base > 0, which it should be, since 0 is NO_IRQ.

It make things easier the day you start using a purely
dynamic approach.

Yours,
Linus Walleij
Haojian Zhuang Feb. 14, 2013, 5:10 p.m. UTC | #2
On 14 February 2013 22:04, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Feb 11, 2013 at 6:10 PM, Haojian Zhuang
> <haojian.zhuang@linaro.org> wrote:
>
> We don't convert drivers to irqdomain without actually *using*
> the irqdomain. I don't know where this pattern comes from,
> but I'm suspicious about patch sets that just focus on enabling
> DT functionality without considering irqdomain stuff as a concept
> of its own.
>
> The biggest problem with this patch is what is missing from it:
>
> - irq_base shall be *deleted* from struct pl061_gpio
> - instead a struct irqdomain  * shall be stored for offsetting IRQs
> - everywhere chip->irq_base is referenced, use irq_find_mapping()
> - In pl061_to_irq, irq_create_mapping() shall be used, so it will
>   allocate a descriptor even if we're using the driver with
>   SPARSE_IRQ and a linear domain.
>
> Look at e.g. gpio-langwell.c for guidance.
>
>> +       chip->irq_base = irq_alloc_descs(chip->irq_base, 0, PL061_GPIO_NR, 0);
>> +       if (chip->irq_base < 0)
>> +               return chip->irq_base;
>> +       if (!irq_domain_add_legacy(adev->dev.of_node, PL061_GPIO_NR,
>> +                                  chip->irq_base, 0, &pl061_domain_ops, chip))
>>                 return -ENODEV;
>
> Instead of the above, please use:
>
> chip->irqdomain = irq_domain_add_simple(adev->dev.of_node,
>                                          PL061_GPIO_NR,
>                                          chip->irq_base,
>                                          &pl061_domain_ops,
>                                          chip);
>
> Notice that I don't throw the domain away after creation either...
>
> This call will allocate descriptors for you as long as the
> irq_base > 0, which it should be, since 0 is NO_IRQ.
>
> It make things easier the day you start using a purely
> dynamic approach.
>
> Yours,
> Linus Walleij

Thank you. I'll update it.

Regards
Haojian
diff mbox

Patch

diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index b820869..1a6d05c 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -15,6 +15,7 @@ 
 #include <linux/io.h>
 #include <linux/ioport.h>
 #include <linux/irq.h>
+#include <linux/irqdomain.h>
 #include <linux/bitops.h>
 #include <linux/workqueue.h>
 #include <linux/gpio.h>
@@ -211,6 +212,10 @@  static void __init pl061_init_gc(struct pl061_gpio *chip, int irq_base)
 			       IRQ_GC_INIT_NESTED_LOCK, IRQ_NOREQUEST, 0);
 }
 
+static const struct irq_domain_ops pl061_domain_ops = {
+	.xlate	= irq_domain_xlate_twocell,
+};
+
 static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 {
 	struct device *dev = &adev->dev;
@@ -225,10 +230,15 @@  static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 	if (pdata) {
 		chip->gc.base = pdata->gpio_base;
 		chip->irq_base = pdata->irq_base;
-	} else if (adev->dev.of_node) {
+	} else {
 		chip->gc.base = -1;
-		chip->irq_base = 0;
-	} else
+		chip->irq_base = -1;
+	}
+	chip->irq_base = irq_alloc_descs(chip->irq_base, 0, PL061_GPIO_NR, 0);
+	if (chip->irq_base < 0)
+		return chip->irq_base;
+	if (!irq_domain_add_legacy(adev->dev.of_node, PL061_GPIO_NR,
+				   chip->irq_base, 0, &pl061_domain_ops, chip))
 		return -ENODEV;
 
 	if (!devm_request_mem_region(dev, adev->res.start,