[PATCH v5 0/3] gpio: Add device driver for GRGPIO cores and support custom accessors with gpio-generic

[PATCH v5 0/3] gpio: Add device driver for GRGPIO cores and support custom accessors with gpio-generic

[Andreas Larsson]

Differences since v4:
- Split out changes to gpio-generic into patch 1
- Make the basic driver without any irq support into patch 2, so that
  things can be applied so far if more revisions needs to be done for
  the irq support parts.
- Change irq support to use irq domain and put it in patch 3

Signed-off-by: Andreas Larsson <andreas@gaisler.com>

Andreas Larsson (3):
  gpio: gpio-generic: Add 16 and 32 bit big endian byte order support
  gpio: grgpio: Add device driver for GRGPIO cores
  gpio: grgpio: Add irq support

 .../devicetree/bindings/gpio/gpio-grgpio.txt       |   29 ++
 drivers/gpio/Kconfig                               |    9 +
 drivers/gpio/Makefile                              |    1 +
 drivers/gpio/gpio-generic.c                        |   56 ++-
 drivers/gpio/gpio-grgpio.c                         |  489 ++++++++++++++++++++
 include/linux/basic_mmio_gpio.h                    |    1 +
 6 files changed, 576 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-grgpio.txt
 create mode 100644 drivers/gpio/gpio-grgpio.c

-- 
1.7.10.4

[PATCH v5 1/3] gpio: gpio-generic: Add 16 and 32 bit big endian byte order support

[Andreas Larsson]

There is no general support for 64-bit big endian accesses, so that is
left unsupported.

Signed-off-by: Andreas Larsson <andreas@gaisler.com>
---
 drivers/gpio/gpio-generic.c     |   56 ++++++++++++++++++++++++++++++++-------
 include/linux/basic_mmio_gpio.h |    1 +
 2 files changed, 48 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
index 05fcc0f..42d4706 100644
--- a/drivers/gpio/gpio-generic.c
+++ b/drivers/gpio/gpio-generic.c
@@ -104,6 +104,26 @@ static unsigned long bgpio_read64(void __iomem *reg)
 }
 #endif /* BITS_PER_LONG >= 64 */
 
+static void bgpio_write16be(void __iomem *reg, unsigned long data)
+{
+	iowrite16be(data, reg);
+}
+
+static unsigned long bgpio_read16be(void __iomem *reg)
+{
+	return ioread16be(reg);
+}
+
+static void bgpio_write32be(void __iomem *reg, unsigned long data)
+{
+	iowrite32be(data, reg);
+}
+
+static unsigned long bgpio_read32be(void __iomem *reg)
+{
+	return ioread32be(reg);
+}
+
 static unsigned long bgpio_pin2mask(struct bgpio_chip *bgc, unsigned int pin)
 {
 	return 1 << pin;
@@ -249,7 +269,8 @@ static int bgpio_dir_out_inv(struct gpio_chip *gc, unsigned int gpio, int val)
 
 static int bgpio_setup_accessors(struct device *dev,
 				 struct bgpio_chip *bgc,
-				 bool be)
+				 bool bit_be,
+				 bool byte_be)
 {
 
 	switch (bgc->bits) {
@@ -258,17 +279,33 @@ static int bgpio_setup_accessors(struct device *dev,
 		bgc->write_reg	= bgpio_write8;
 		break;
 	case 16:
-		bgc->read_reg	= bgpio_read16;
-		bgc->write_reg	= bgpio_write16;
+		if (byte_be) {
+			bgc->read_reg	= bgpio_read16be;
+			bgc->write_reg	= bgpio_write16be;
+		} else {
+			bgc->read_reg	= bgpio_read16;
+			bgc->write_reg	= bgpio_write16;
+		}
 		break;
 	case 32:
-		bgc->read_reg	= bgpio_read32;
-		bgc->write_reg	= bgpio_write32;
+		if (byte_be) {
+			bgc->read_reg	= bgpio_read32be;
+			bgc->write_reg	= bgpio_write32be;
+		} else {
+			bgc->read_reg	= bgpio_read32;
+			bgc->write_reg	= bgpio_write32;
+		}
 		break;
 #if BITS_PER_LONG >= 64
 	case 64:
-		bgc->read_reg	= bgpio_read64;
-		bgc->write_reg	= bgpio_write64;
+		if (byte_be) {
+			dev_err(dev,
+				"64 bit big endian byte order unsupported\n");
+			return -EINVAL;
+		} else {
+			bgc->read_reg	= bgpio_read64;
+			bgc->write_reg	= bgpio_write64;
+		}
 		break;
 #endif /* BITS_PER_LONG >= 64 */
 	default:
@@ -276,7 +313,7 @@ static int bgpio_setup_accessors(struct device *dev,
 		return -EINVAL;
 	}
 
-	bgc->pin2mask = be ? bgpio_pin2mask_be : bgpio_pin2mask;
+	bgc->pin2mask = bit_be ? bgpio_pin2mask_be : bgpio_pin2mask;
 
 	return 0;
 }
@@ -385,7 +422,8 @@ int bgpio_init(struct bgpio_chip *bgc, struct device *dev,
 	if (ret)
 		return ret;
 
-	ret = bgpio_setup_accessors(dev, bgc, flags & BGPIOF_BIG_ENDIAN);
+	ret = bgpio_setup_accessors(dev, bgc, flags & BGPIOF_BIG_ENDIAN,
+				    flags & BGPIOF_BIG_ENDIAN_BYTE_ORDER);
 	if (ret)
 		return ret;
 
diff --git a/include/linux/basic_mmio_gpio.h b/include/linux/basic_mmio_gpio.h
index 1c504ca..d8a97ec 100644
--- a/include/linux/basic_mmio_gpio.h
+++ b/include/linux/basic_mmio_gpio.h
@@ -72,5 +72,6 @@ int bgpio_init(struct bgpio_chip *bgc, struct device *dev,
 #define BGPIOF_BIG_ENDIAN		BIT(0)
 #define BGPIOF_UNREADABLE_REG_SET	BIT(1) /* reg_set is unreadable */
 #define BGPIOF_UNREADABLE_REG_DIR	BIT(2) /* reg_dir is unreadable */
+#define BGPIOF_BIG_ENDIAN_BYTE_ORDER	BIT(3)
 
 #endif /* __BASIC_MMIO_GPIO_H */
-- 
1.7.10.4

Re: [PATCH v5 1/3] gpio: gpio-generic: Add 16 and 32 bit big endian byte order support

[Anton Vorontsov]

On Fri, Mar 15, 2013 at 02:45:38PM +0100, Andreas Larsson wrote:
> There is no general support for 64-bit big endian accesses, so that is
> left unsupported.
> 
> Signed-off-by: Andreas Larsson <andreas@gaisler.com>

This looks perfect, thanks a lot!

Acked-by: Anton Vorontsov <anton@enomsg.org>

Re: [PATCH v5 1/3] gpio: gpio-generic: Add 16 and 32 bit big endian byte order support

[Linus Walleij]

On Tue, Mar 19, 2013 at 1:40 AM, Anton Vorontsov <anton@enomsg.org> wrote:
> On Fri, Mar 15, 2013 at 02:45:38PM +0100, Andreas Larsson wrote:
>> There is no general support for 64-bit big endian accesses, so that is
>> left unsupported.
>>
>> Signed-off-by: Andreas Larsson <andreas@gaisler.com>
>
> This looks perfect, thanks a lot!
>
> Acked-by: Anton Vorontsov <anton@enomsg.org>

Looks good to me as well, but since this was discussed with Grant
I want his review on this before I merge it.

Yours,
Linus Walleij

Re: [PATCH v5 1/3] gpio: gpio-generic: Add 16 and 32 bit big endian byte order support

[Andreas Larson]

On 2013-03-27 09:58, Linus Walleij wrote:
> On Tue, Mar 19, 2013 at 1:40 AM, Anton Vorontsov <anton@enomsg.org> wrote:
>> On Fri, Mar 15, 2013 at 02:45:38PM +0100, Andreas Larsson wrote:
>>> There is no general support for 64-bit big endian accesses, so that is
>>> left unsupported.
>>>
>>> Signed-off-by: Andreas Larsson <andreas@gaisler.com>
>>
>> This looks perfect, thanks a lot!
>>
>> Acked-by: Anton Vorontsov <anton@enomsg.org>
>
> Looks good to me as well, but since this was discussed with Grant
> I want his review on this before I merge it.

I take it that you are happy with the other two patches as well?

Grant, any chance of getting this into the 3.10 merge window?

Cheers,
Andreas Larsson

Re: [PATCH v5 1/3] gpio: gpio-generic: Add 16 and 32 bit big endian byte order support

[Linus Walleij]

On Wed, Apr 3, 2013 at 9:49 AM, Andreas Larson <andreas@gaisler.com> wrote:
> On 2013-03-27 09:58, Linus Walleij wrote:
>>
>> On Tue, Mar 19, 2013 at 1:40 AM, Anton Vorontsov <anton@enomsg.org> wrote:
>>>
>>> On Fri, Mar 15, 2013 at 02:45:38PM +0100, Andreas Larsson wrote:
>>>>
>>>> There is no general support for 64-bit big endian accesses, so that is
>>>> left unsupported.
>>>>
>>>> Signed-off-by: Andreas Larsson <andreas@gaisler.com>
>>>
>>>
>>> This looks perfect, thanks a lot!
>>>
>>> Acked-by: Anton Vorontsov <anton@enomsg.org>
>>
>>
>> Looks good to me as well, but since this was discussed with Grant
>> I want his review on this before I merge it.
>
>
> I take it that you are happy with the other two patches as well?
>
> Grant, any chance of getting this into the 3.10 merge window?

Apparently Grant is busy these days, so I took a review of the
patch and I like it. With Anton's ACK as well I'm certain it's good,
so patch applied!

Yours,
Linus Walleij

[PATCH v5 2/3] gpio: grgpio: Add device driver for GRGPIO cores

[Andreas Larsson]

This driver supports GRGPIO gpio cores available in the GRLIB VHDL IP
core library from Aeroflex Gaisler.

Signed-off-by: Andreas Larsson <andreas@gaisler.com>
---
 .../devicetree/bindings/gpio/gpio-grgpio.txt       |   24 +++
 drivers/gpio/Kconfig                               |    9 ++
 drivers/gpio/Makefile                              |    1 +
 drivers/gpio/gpio-grgpio.c                         |  164 ++++++++++++++++++++
 4 files changed, 198 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-grgpio.txt
 create mode 100644 drivers/gpio/gpio-grgpio.c

diff --git a/Documentation/devicetree/bindings/gpio/gpio-grgpio.txt b/Documentation/devicetree/bindings/gpio/gpio-grgpio.txt
new file mode 100644
index 0000000..1050dc8
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-grgpio.txt
@@ -0,0 +1,24 @@
+Aeroflex Gaisler GRGPIO General Purpose I/O cores.
+
+The GRGPIO GPIO core is available in the GRLIB VHDL IP core library.
+
+Note: In the ordinary environment for the GRGPIO core, a Leon SPARC system,
+these properties are built from information in the AMBA plug&play.
+
+Required properties:
+
+- name : Should be "GAISLER_GPIO" or "01_01a"
+
+- reg : Address and length of the register set for the device
+
+- interrupts : Interrupt numbers for this device
+
+Optional properties:
+
+- base : The base gpio number for the core. A dynamic base is used if not
+	present
+
+- nbits : The number of gpio lines. If not present driver assumes 32 lines.
+
+For further information look in the documentation for the GLIB IP core library:
+http://www.gaisler.com/products/grlib/grip.pdf
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 93aaadf..32f068d 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -309,6 +309,15 @@ config GPIO_LYNXPOINT
 	  driver for GPIO functionality on Intel Lynxpoint PCH chipset
 	  Requires ACPI device enumeration code to set up a platform device.
 
+config GPIO_GRGPIO
+	tristate "Aeroflex Gaisler GRGPIO support"
+	depends on OF
+	select GPIO_GENERIC
+	select IRQ_DOMAIN
+	help
+	  Select this to support Aeroflex Gaisler GRGPIO cores from the GRLIB
+	  VHDL IP core library.
+
 comment "I2C GPIO expanders:"
 
 config GPIO_ARIZONA
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 22e07bc..3e6be51 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_ARCH_DAVINCI)	+= gpio-davinci.o
 obj-$(CONFIG_GPIO_EM)		+= gpio-em.o
 obj-$(CONFIG_GPIO_EP93XX)	+= gpio-ep93xx.o
 obj-$(CONFIG_GPIO_GE_FPGA)	+= gpio-ge.o
+obj-$(CONFIG_GPIO_GRGPIO)	+= gpio-grgpio.o
 obj-$(CONFIG_GPIO_ICH)		+= gpio-ich.o
 obj-$(CONFIG_GPIO_IT8761E)	+= gpio-it8761e.o
 obj-$(CONFIG_GPIO_JANZ_TTL)	+= gpio-janz-ttl.o
diff --git a/drivers/gpio/gpio-grgpio.c b/drivers/gpio/gpio-grgpio.c
new file mode 100644
index 0000000..f8902da
--- /dev/null
+++ b/drivers/gpio/gpio-grgpio.c
@@ -0,0 +1,164 @@
+/*
+ * Driver for Aeroflex Gaisler GRGPIO General Purpose I/O cores.
+ *
+ * 2013 (c) Aeroflex Gaisler AB
+ *
+ * This driver supports the GRGPIO GPIO core available in the GRLIB VHDL
+ * IP core library.
+ *
+ * Full documentation of the GRGPIO core can be found here:
+ * http://www.gaisler.com/products/grlib/grip.pdf
+ *
+ * See "Documentation/devicetree/bindings/gpio/gpio-grgpio.txt" for
+ * information on open firmware properties.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ * Contributors: Andreas Larsson <andreas@gaisler.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/spinlock.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/of_platform.h>
+#include <linux/gpio.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/basic_mmio_gpio.h>
+
+#define GRGPIO_MAX_NGPIO 32
+
+struct grgpio_regs {
+	u32 data;	/* 0x00 */
+	u32 output;	/* 0x04 */
+	u32 dir;	/* 0x08 */
+	u32 imask;	/* 0x0c */
+	u32 ipol;	/* 0x10  */
+	u32 iedge;	/* 0x14 */
+	u32 bypass;	/* 0x18 */
+	u32 __reserved;	/* 0x1c */
+	u32 imap[8];	/* 0x20-0x3c */
+};
+
+struct grgpio_priv {
+	struct bgpio_chip bgc;
+	struct grgpio_regs __iomem *regs;
+	struct device *dev;
+};
+
+static inline struct grgpio_priv *grgpio_gc_to_priv(struct gpio_chip *gc)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+
+	return container_of(bgc, struct grgpio_priv, bgc);
+}
+
+static int grgpio_to_irq(struct gpio_chip *gc, unsigned offset)
+{
+	return -ENXIO;
+}
+
+static int grgpio_probe(struct platform_device *ofdev)
+{
+	struct device_node *np = ofdev->dev.of_node;
+	struct grgpio_regs __iomem *regs;
+	struct gpio_chip *gc;
+	struct bgpio_chip *bgc;
+	struct grgpio_priv *priv;
+	struct resource *res;
+	int err;
+	u32 prop;
+
+	priv = devm_kzalloc(&ofdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(ofdev, IORESOURCE_MEM, 0);
+	regs = devm_ioremap_resource(&ofdev->dev, res);
+	if (IS_ERR(regs))
+		return PTR_ERR(regs);
+
+	bgc = &priv->bgc;
+	err = bgpio_init(bgc, &ofdev->dev, 4, &regs->data, &regs->output, NULL,
+			 &regs->dir, NULL, BGPIOF_BIG_ENDIAN_BYTE_ORDER);
+	if (err) {
+		dev_err(&ofdev->dev, "bgpio_init() failed\n");
+		return err;
+	}
+
+	priv->regs = regs;
+	priv->dev = &ofdev->dev;
+
+	gc = &bgc->gc;
+	gc->of_node = np;
+	gc->owner = THIS_MODULE;
+	gc->to_irq = grgpio_to_irq;
+	gc->label = np->full_name;
+
+	err = of_property_read_u32(np, "base", &prop);
+	if (err) {
+		dev_dbg(&ofdev->dev, "No base property: use dynamic base\n");
+		gc->base = -1;
+	} else {
+		gc->base = prop;
+	}
+
+	err = of_property_read_u32(np, "nbits", &prop);
+	if (err || prop <= 0 || prop > GRGPIO_MAX_NGPIO) {
+		gc->ngpio = GRGPIO_MAX_NGPIO;
+		dev_dbg(&ofdev->dev,
+			"No or invalid nbits property: assume %d\n", gc->ngpio);
+	} else {
+		gc->ngpio = prop;
+	}
+
+	platform_set_drvdata(ofdev, priv);
+
+	err = gpiochip_add(gc);
+	if (err) {
+		dev_err(&ofdev->dev, "Could not add gpiochip\n");
+		return err;
+	}
+
+	dev_info(&ofdev->dev, "regs=0x%p, base=%d, ngpio=%d\n",
+		 priv->regs, gc->base, gc->ngpio);
+
+	return 0;
+}
+
+static int grgpio_remove(struct platform_device *ofdev)
+{
+	struct grgpio_priv *priv = platform_get_drvdata(ofdev);
+
+	return gpiochip_remove(&priv->bgc.gc);
+}
+
+static struct of_device_id grgpio_match[] = {
+	{.name = "GAISLER_GPIO"},
+	{.name = "01_01a"},
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, grgpio_match);
+
+static struct platform_driver grgpio_driver = {
+	.driver = {
+		.name = "grgpio",
+		.owner = THIS_MODULE,
+		.of_match_table = grgpio_match,
+	},
+	.probe = grgpio_probe,
+	.remove = grgpio_remove,
+};
+module_platform_driver(grgpio_driver);
+
+MODULE_AUTHOR("Aeroflex Gaisler AB.");
+MODULE_DESCRIPTION("Driver for Aeroflex Gaisler GRGPIO");
+MODULE_LICENSE("GPL");
-- 
1.7.10.4

Re: [PATCH v5 2/3] gpio: grgpio: Add device driver for GRGPIO cores

[Linus Walleij]

On Fri, Mar 15, 2013 at 2:45 PM, Andreas Larsson <andreas@gaisler.com> wrote:

> This driver supports GRGPIO gpio cores available in the GRLIB VHDL IP
> core library from Aeroflex Gaisler.
>
> Signed-off-by: Andreas Larsson <andreas@gaisler.com>
> ---
>  .../devicetree/bindings/gpio/gpio-grgpio.txt       |   24 +++
>  drivers/gpio/Kconfig                               |    9 ++
>  drivers/gpio/Makefile                              |    1 +
>  drivers/gpio/gpio-grgpio.c                         |  164 ++++++++++++++++++++
>  4 files changed, 198 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-grgpio.txt
>  create mode 100644 drivers/gpio/gpio-grgpio.c
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-grgpio.txt b/Documentation/devicetree/bindings/gpio/gpio-grgpio.txt
> new file mode 100644
> index 0000000..1050dc8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-grgpio.txt
> @@ -0,0 +1,24 @@
> +Aeroflex Gaisler GRGPIO General Purpose I/O cores.
> +
> +The GRGPIO GPIO core is available in the GRLIB VHDL IP core library.
> +
> +Note: In the ordinary environment for the GRGPIO core, a Leon SPARC system,
> +these properties are built from information in the AMBA plug&play.
> +
> +Required properties:
> +
> +- name : Should be "GAISLER_GPIO" or "01_01a"

What is this? Don't we usually use a .compatible string for this?
Name? Que? Is that something legacy?

I would prefer:

- Add you vendor prefix to Documentation/devicetree/bindings/vendor-prefixes.txt
- Use a compatible string like this "gaisler,gpio"

> +- reg : Address and length of the register set for the device
> +
> +- interrupts : Interrupt numbers for this device
> +
> +Optional properties:
> +
> +- base : The base gpio number for the core. A dynamic base is used if not
> +       present

This has to go. We don't hardcode numbers from the global GPIO
space into the device tree, beacause as you soon realize this is
Linux-specific and the device tree shall be OS agnostic.

The discussion has come up a number of times, review the mailing
lists for suggestions on how to get around this.

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

> +struct grgpio_regs {
> +       u32 data;       /* 0x00 */
> +       u32 output;     /* 0x04 */
> +       u32 dir;        /* 0x08 */
> +       u32 imask;      /* 0x0c */
> +       u32 ipol;       /* 0x10  */
> +       u32 iedge;      /* 0x14 */
> +       u32 bypass;     /* 0x18 */
> +       u32 __reserved; /* 0x1c */
> +       u32 imap[8];    /* 0x20-0x3c */
> +};

Um... Why are you doing this?

> +struct grgpio_priv {
> +       struct bgpio_chip bgc;
> +       struct grgpio_regs __iomem *regs;

And that's tagged as __iomem * as well, that is very unorthodox.
The usual practice is to have a base pointer void __iomem *base
and offset from that.

> +       struct device *dev;
> +};
> +
> +static inline struct grgpio_priv *grgpio_gc_to_priv(struct gpio_chip *gc)
> +{
> +       struct bgpio_chip *bgc = to_bgpio_chip(gc);
> +
> +       return container_of(bgc, struct grgpio_priv, bgc);
> +}
> +
> +static int grgpio_to_irq(struct gpio_chip *gc, unsigned offset)
> +{
> +       return -ENXIO;
> +}

The gpiolib core already returns -ENXIO if this function is
not assigned so just delete this function and leave that
function pointer as NULL.

> +static int grgpio_probe(struct platform_device *ofdev)
> +{
> +       struct device_node *np = ofdev->dev.of_node;
> +       struct grgpio_regs __iomem *regs;

Prefer void __iomem *base;

> +       struct gpio_chip *gc;
> +       struct bgpio_chip *bgc;
> +       struct grgpio_priv *priv;
> +       struct resource *res;
> +       int err;
> +       u32 prop;
> +
> +       priv = devm_kzalloc(&ofdev->dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       res = platform_get_resource(ofdev, IORESOURCE_MEM, 0);
> +       regs = devm_ioremap_resource(&ofdev->dev, res);
> +       if (IS_ERR(regs))
> +               return PTR_ERR(regs);
> +
> +       bgc = &priv->bgc;
> +       err = bgpio_init(bgc, &ofdev->dev, 4, &regs->data, &regs->output, NULL,
> +                        &regs->dir, NULL, BGPIOF_BIG_ENDIAN_BYTE_ORDER);

So I would prefer if you did:

#define GRGPIO_DATA 0x00
#define GRGPIO_OUTPUT 0x04
#define GRGPIO_DIR 0x08
(...)

err = bgpio_init(bgc, &ofdev->dev, 4, base + GRGPIO_DATA, base +
GRGPIO_OUTPUT, NULL,
                       base + GRGPIO_DIR, NULL, BGPIOF_BIG_ENDIAN_BYTE_ORDER);

> +       if (err) {
> +               dev_err(&ofdev->dev, "bgpio_init() failed\n");
> +               return err;
> +       }
> +
> +       priv->regs = regs;
> +       priv->dev = &ofdev->dev;
> +
> +       gc = &bgc->gc;
> +       gc->of_node = np;
> +       gc->owner = THIS_MODULE;
> +       gc->to_irq = grgpio_to_irq;
> +       gc->label = np->full_name;
> +
> +       err = of_property_read_u32(np, "base", &prop);
> +       if (err) {
> +               dev_dbg(&ofdev->dev, "No base property: use dynamic base\n");
> +               gc->base = -1;
> +       } else {
> +               gc->base = prop;
> +       }

Over my dead body ;-)

(...)
> +static struct of_device_id grgpio_match[] = {
> +       {.name = "GAISLER_GPIO"},
> +       {.name = "01_01a"},
> +       {},
> +};

This is very weird. Especially "01_01a" needs a real good explanation
if it is to be kept.

Alas, I don't really know what the .name field in the of_device_id is for...

Yours,
Linus Walleij

Re: [PATCH v5 2/3] gpio: grgpio: Add device driver for GRGPIO cores

[Andreas Larson]

On 2013-04-10 20:50, Linus Walleij wrote:
> On Fri, Mar 15, 2013 at 2:45 PM, Andreas Larsson <andreas@gaisler.com> wrote:
>
>> This driver supports GRGPIO gpio cores available in the GRLIB VHDL IP
>> core library from Aeroflex Gaisler.
>>
>> Signed-off-by: Andreas Larsson <andreas@gaisler.com>
>> ---
>>   .../devicetree/bindings/gpio/gpio-grgpio.txt       |   24 +++
>>   drivers/gpio/Kconfig                               |    9 ++
>>   drivers/gpio/Makefile                              |    1 +
>>   drivers/gpio/gpio-grgpio.c                         |  164 ++++++++++++++++++++
>>   4 files changed, 198 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/gpio/gpio-grgpio.txt
>>   create mode 100644 drivers/gpio/gpio-grgpio.c
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-grgpio.txt b/Documentation/devicetree/bindings/gpio/gpio-grgpio.txt
>> new file mode 100644
>> index 0000000..1050dc8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-grgpio.txt
>> @@ -0,0 +1,24 @@
>> +Aeroflex Gaisler GRGPIO General Purpose I/O cores.
>> +
>> +The GRGPIO GPIO core is available in the GRLIB VHDL IP core library.
>> +
>> +Note: In the ordinary environment for the GRGPIO core, a Leon SPARC system,
>> +these properties are built from information in the AMBA plug&play.
>> +
>> +Required properties:
>> +
>> +- name : Should be "GAISLER_GPIO" or "01_01a"
>
> What is this? Don't we usually use a .compatible string for this?
> Name? Que? Is that something legacy?

Regarding using name, this is standard for SPARC. The names in the
device tree originates from the PROM.

The name field is the actually the first field checked for a match in
of_match_node, followed by type then compatible. See
http://lxr.free-electrons.com/source/drivers/of/base.c#L572

Examples can be found among many others in:
- drivers/watchdog/riowd.c
- drivers/watchdog/cpwd.c
- drivers/mtd/maps/sun_uflash.c
- drivers/input/misc/sparcspkr.c
- drivers/input/serio/i8042-sparcio.h
- drivers/hwmon/ultra45_env.c

As for the "01_01a", the LEON SPARC systems uses a plug&play to identify
different IP cores in the system. When the PROM is unaware of the name
of a certain core, the name field presented from the prom will be on
this form. This is standard handling for LEON SPARC drivers.

Examples of this can be found in:
- drivers/gpio/gpio-grgpio.c
- drivers/usb/host/uhci-grlib.c
- drivers/usb/host/ehci-grlib.c
- drivers/video/grvga.c
- drivers/net/can/grcan.c
- drivers/net/ethernet/aeroflex/greth.c
- drivers/tty/serial/apbuart.c


> I would prefer:
>
> - Add you vendor prefix to Documentation/devicetree/bindings/vendor-prefixes.txt
> - Use a compatible string like this "gaisler,gpio"
>
>> +- reg : Address and length of the register set for the device
>> +
>> +- interrupts : Interrupt numbers for this device
>> +
>> +Optional properties:
>> +
>> +- base : The base gpio number for the core. A dynamic base is used if not
>> +       present
>
> This has to go. We don't hardcode numbers from the global GPIO
> space into the device tree, beacause as you soon realize this is
> Linux-specific and the device tree shall be OS agnostic.
>
> The discussion has come up a number of times, review the mailing
> lists for suggestions on how to get around this.
>
> (...)
>> +++ b/drivers/gpio/gpio-grgpio.c
>
>> +struct grgpio_regs {
>> +       u32 data;       /* 0x00 */
>> +       u32 output;     /* 0x04 */
>> +       u32 dir;        /* 0x08 */
>> +       u32 imask;      /* 0x0c */
>> +       u32 ipol;       /* 0x10  */
>> +       u32 iedge;      /* 0x14 */
>> +       u32 bypass;     /* 0x18 */
>> +       u32 __reserved; /* 0x1c */
>> +       u32 imap[8];    /* 0x20-0x3c */
>> +};
>
> Um... Why are you doing this?
>
>> +struct grgpio_priv {
>> +       struct bgpio_chip bgc;
>> +       struct grgpio_regs __iomem *regs;
>
> And that's tagged as __iomem * as well, that is very unorthodox.
> The usual practice is to have a base pointer void __iomem *base
> and offset from that.
>
>> +       struct device *dev;
>> +};
>> +
>> +static inline struct grgpio_priv *grgpio_gc_to_priv(struct gpio_chip *gc)
>> +{
>> +       struct bgpio_chip *bgc = to_bgpio_chip(gc);
>> +
>> +       return container_of(bgc, struct grgpio_priv, bgc);
>> +}
>> +
>> +static int grgpio_to_irq(struct gpio_chip *gc, unsigned offset)
>> +{
>> +       return -ENXIO;
>> +}
>
> The gpiolib core already returns -ENXIO if this function is
> not assigned so just delete this function and leave that
> function pointer as NULL.

Sure, the second patch should add it from scratch.

>> +static int grgpio_probe(struct platform_device *ofdev)
>> +{
>> +       struct device_node *np = ofdev->dev.of_node;
>> +       struct grgpio_regs __iomem *regs;
>
> Prefer void __iomem *base;
>
>> +       struct gpio_chip *gc;
>> +       struct bgpio_chip *bgc;
>> +       struct grgpio_priv *priv;
>> +       struct resource *res;
>> +       int err;
>> +       u32 prop;
>> +
>> +       priv = devm_kzalloc(&ofdev->dev, sizeof(*priv), GFP_KERNEL);
>> +       if (!priv)
>> +               return -ENOMEM;
>> +
>> +       res = platform_get_resource(ofdev, IORESOURCE_MEM, 0);
>> +       regs = devm_ioremap_resource(&ofdev->dev, res);
>> +       if (IS_ERR(regs))
>> +               return PTR_ERR(regs);
>> +
>> +       bgc = &priv->bgc;
>> +       err = bgpio_init(bgc, &ofdev->dev, 4, &regs->data, &regs->output, NULL,
>> +                        &regs->dir, NULL, BGPIOF_BIG_ENDIAN_BYTE_ORDER);
>
> So I would prefer if you did:
>
> #define GRGPIO_DATA 0x00
> #define GRGPIO_OUTPUT 0x04
> #define GRGPIO_DIR 0x08
> (...)

Sure, I'll change it, either way works for me, although I don't see a 
problem with the original approach.


> err = bgpio_init(bgc, &ofdev->dev, 4, base + GRGPIO_DATA, base +
> GRGPIO_OUTPUT, NULL,
>                         base + GRGPIO_DIR, NULL, BGPIOF_BIG_ENDIAN_BYTE_ORDER);
>
>> +       if (err) {
>> +               dev_err(&ofdev->dev, "bgpio_init() failed\n");
>> +               return err;
>> +       }
>> +
>> +       priv->regs = regs;
>> +       priv->dev = &ofdev->dev;
>> +
>> +       gc = &bgc->gc;
>> +       gc->of_node = np;
>> +       gc->owner = THIS_MODULE;
>> +       gc->to_irq = grgpio_to_irq;
>> +       gc->label = np->full_name;
>> +
>> +       err = of_property_read_u32(np, "base", &prop);
>> +       if (err) {
>> +               dev_dbg(&ofdev->dev, "No base property: use dynamic base\n");
>> +               gc->base = -1;
>> +       } else {
>> +               gc->base = prop;
>> +       }
>
> Over my dead body ;-)

Removed

> (...)
>> +static struct of_device_id grgpio_match[] = {
>> +       {.name = "GAISLER_GPIO"},
>> +       {.name = "01_01a"},
>> +       {},
>> +};
>
> This is very weird. Especially "01_01a" needs a real good explanation
> if it is to be kept.
>
> Alas, I don't really know what the .name field in the of_device_id is for...

Treated above.


Thanks for the feedback!

Cheers,
Andreas Larsson

Re: [PATCH v5 2/3] gpio: grgpio: Add device driver for GRGPIO cores

[Linus Walleij]

On Mon, Apr 15, 2013 at 2:38 PM, Andreas Larson <andreas@gaisler.com> wrote:
> On 2013-04-10 20:50, Linus Walleij wrote:

>>> +Required properties:
>>> +
>>> +- name : Should be "GAISLER_GPIO" or "01_01a"
>>
>> What is this? Don't we usually use a .compatible string for this?
>> Name? Que? Is that something legacy?
>
> Regarding using name, this is standard for SPARC. The names in the
> device tree originates from the PROM.
>
> The name field is the actually the first field checked for a match in
> of_match_node, followed by type then compatible. See
> http://lxr.free-electrons.com/source/drivers/of/base.c#L572
>
(...)
> As for the "01_01a", the LEON SPARC systems uses a plug&play to identify
> different IP cores in the system. When the PROM is unaware of the name
> of a certain core, the name field presented from the prom will be on
> this form. This is standard handling for LEON SPARC drivers.

Thanks for detailing this! There is such a big world out there sometimes.

I'm sort of wondering why we do things the way we do in the ARM DT
world sometimes. Specifically I suspect that we should be using
name or type rather than compatible for the ARM PrimeCells, which
are a sort of plug-n-play type devices, but I guess it's too late
to change that now...

Yours,
Linus Walleij

[PATCH v5 3/3] gpio: grgpio: Add irq support

[Andreas Larsson]

The drivers sets up an irq domain and hands out unique virqs to irq
capable gpio lines regardless of which underlying irqs maps to which gpio
line.

Signed-off-by: Andreas Larsson <andreas@gaisler.com>
---
 .../devicetree/bindings/gpio/gpio-grgpio.txt       |    5 +
 drivers/gpio/gpio-grgpio.c                         |  333 +++++++++++++++++++-
 2 files changed, 334 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-grgpio.txt b/Documentation/devicetree/bindings/gpio/gpio-grgpio.txt
index 1050dc8..e28bc3c 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-grgpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-grgpio.txt
@@ -20,5 +20,10 @@ Optional properties:
 
 - nbits : The number of gpio lines. If not present driver assumes 32 lines.
 
+- irqmap : An array with an index for each gpio line. An index is either a valid
+	index into the interrupts property array, or 0xffffffff that indicates
+	no irq for that line. Driver provides no interrupt support if not
+	present.
+
 For further information look in the documentation for the GLIB IP core library:
 http://www.gaisler.com/products/grlib/grip.pdf
diff --git a/drivers/gpio/gpio-grgpio.c b/drivers/gpio/gpio-grgpio.c
index f8902da..3acff77 100644
--- a/drivers/gpio/gpio-grgpio.c
+++ b/drivers/gpio/gpio-grgpio.c
@@ -32,6 +32,9 @@
 #include <linux/slab.h>
 #include <linux/err.h>
 #include <linux/basic_mmio_gpio.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
 
 #define GRGPIO_MAX_NGPIO 32
 
@@ -47,10 +50,41 @@ struct grgpio_regs {
 	u32 imap[8];	/* 0x20-0x3c */
 };
 
+struct grgpio_uirq {
+	u8 refcnt; /* Reference counter to manage requesting/freeing of uirq */
+	u8 uirq; /* Underlying virq of the gpio driver */
+};
+
+struct grgpio_lirq {
+	s8 index; /* Index into struct grgpio_priv's uirqs, or -1 */
+	u8 virq; /* virq for the gpio line */
+};
+
 struct grgpio_priv {
 	struct bgpio_chip bgc;
 	struct grgpio_regs __iomem *regs;
 	struct device *dev;
+
+	u32 imask; /* irq mask shadow register */
+
+	/* The grgpio core can have multiple irqs. Severeal gpio lines can
+	 * potentially be mapped to the same irq. This driver sets up an
+	 * irq domain and hands out separate virqs to each gpio line
+	 */
+	struct irq_domain *domain;
+
+	/* This array contains information on each underlying irq, each
+	 * irq of the grgpio core itself.
+	 */
+	struct grgpio_uirq uirqs[GRGPIO_MAX_NGPIO];
+
+	/* This array contains information for each gpio line on the
+	 * virtual irqs obtains from this driver. An index value of -1 for
+	 * a certain gpio line indicates that the line has no
+	 * irq. Otherwise the index connects the virq to the underlying
+	 * irq by pointing into the uirqs array.
+	 */
+	struct grgpio_lirq lirqs[GRGPIO_MAX_NGPIO];
 };
 
 static inline struct grgpio_priv *grgpio_gc_to_priv(struct gpio_chip *gc)
@@ -60,11 +94,231 @@ static inline struct grgpio_priv *grgpio_gc_to_priv(struct gpio_chip *gc)
 	return container_of(bgc, struct grgpio_priv, bgc);
 }
 
+static void grgpio_set_imask(struct grgpio_priv *priv, unsigned int offset,
+			     int val)
+{
+	struct bgpio_chip *bgc = &priv->bgc;
+	unsigned long mask = bgc->pin2mask(bgc, offset);
+	unsigned long flags;
+
+	spin_lock_irqsave(&bgc->lock, flags);
+
+	if (val)
+		priv->imask |= mask;
+	else
+		priv->imask &= ~mask;
+	bgc->write_reg(&priv->regs->imask, priv->imask);
+
+	spin_unlock_irqrestore(&bgc->lock, flags);
+}
+
 static int grgpio_to_irq(struct gpio_chip *gc, unsigned offset)
 {
-	return -ENXIO;
+	struct grgpio_priv *priv = grgpio_gc_to_priv(gc);
+
+	if (offset > gc->ngpio)
+		return -ENXIO;
+
+	if (priv->lirqs[offset].index < 0)
+		return -ENXIO;
+
+	return irq_create_mapping(priv->domain, offset);
+}
+
+/* -------------------- IRQ chip functions -------------------- */
+
+static int grgpio_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	struct grgpio_priv *priv = irq_data_get_irq_chip_data(d);
+	unsigned long flags;
+	u32 mask = BIT(d->hwirq);
+	u32 ipol;
+	u32 iedge;
+	u32 pol;
+	u32 edge;
+
+	switch (type) {
+	case IRQ_TYPE_LEVEL_LOW:
+		pol = 0;
+		edge = 0;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		pol = mask;
+		edge = 0;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		pol = 0;
+		edge = mask;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		pol = mask;
+		edge = mask;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	spin_lock_irqsave(&priv->bgc.lock, flags);
+
+	ipol = priv->bgc.read_reg(&priv->regs->ipol) & ~mask;
+	iedge = priv->bgc.read_reg(&priv->regs->iedge) & ~mask;
+
+	priv->bgc.write_reg(&priv->regs->ipol, ipol | pol);
+	priv->bgc.write_reg(&priv->regs->iedge, iedge | edge);
+
+	spin_unlock_irqrestore(&priv->bgc.lock, flags);
+
+	return 0;
+}
+
+static void grgpio_irq_mask(struct irq_data *d)
+{
+	struct grgpio_priv *priv = irq_data_get_irq_chip_data(d);
+	int offset = d->hwirq;
+
+	grgpio_set_imask(priv, offset, 0);
+}
+
+static void grgpio_irq_unmask(struct irq_data *d)
+{
+	struct grgpio_priv *priv = irq_data_get_irq_chip_data(d);
+	int offset = d->hwirq;
+
+	grgpio_set_imask(priv, offset, 1);
+}
+
+static struct irq_chip grgpio_irq_chip = {
+	.name			= "grgpio",
+	.irq_mask		= grgpio_irq_mask,
+	.irq_unmask		= grgpio_irq_unmask,
+	.irq_set_type		= grgpio_irq_set_type,
+};
+
+static irqreturn_t grgpio_irq_handler(int irq, void *dev)
+{
+	struct grgpio_priv *priv = dev;
+	int ngpio = priv->bgc.gc.ngpio;
+	int i;
+
+	for (i = 0; i < ngpio; i++) {
+		struct grgpio_lirq *lirq = &priv->lirqs[i];
+
+		if (priv->imask & BIT(i) && lirq->index >= 0 &&
+		    priv->uirqs[lirq->index].uirq == irq) {
+			generic_handle_irq(lirq->virq);
+		}
+	}
+
+	return IRQ_HANDLED;
+}
+
+/* This function will be called as a consequence of the call to
+ * irq_create_mapping in grgpio_to_irq
+ */
+int grgpio_irq_map(struct irq_domain *d, unsigned int virq,
+		   irq_hw_number_t hwirq)
+{
+	struct grgpio_priv *priv = d->host_data;
+	struct grgpio_lirq *lirq;
+	struct grgpio_uirq *uirq;
+	unsigned long flags;
+	int offset = hwirq;
+	int ret = 0;
+
+	if (!priv)
+		return -EINVAL;
+
+	lirq = &priv->lirqs[offset];
+	if (lirq->index < 0)
+		return -EINVAL;
+
+	dev_dbg(priv->dev, "Mapping virq %d for gpio line %d\n",
+		virq, offset);
+
+	spin_lock_irqsave(&priv->bgc.lock, flags);
+
+	/* Request underlying irq if not already requested */
+	lirq->virq = virq;
+	uirq = &priv->uirqs[lirq->index];
+	if (uirq->refcnt == 0) {
+		ret = request_irq(uirq->uirq, grgpio_irq_handler, 0,
+				  dev_name(priv->dev), priv);
+		if (ret) {
+			dev_err(priv->dev,
+				"Could not request underlying irq %d\n",
+				uirq->uirq);
+
+			spin_unlock_irqrestore(&priv->bgc.lock, flags);
+
+			return ret;
+		}
+	}
+	uirq->refcnt++;
+
+	spin_unlock_irqrestore(&priv->bgc.lock, flags);
+
+	/* Setup virq  */
+	irq_set_chip_data(virq, priv);
+	irq_set_chip_and_handler(virq, &grgpio_irq_chip,
+				 handle_simple_irq);
+	irq_clear_status_flags(virq, IRQ_NOREQUEST);
+#ifdef CONFIG_ARM
+	set_irq_flags(virq, IRQF_VALID);
+#else
+	irq_set_noprobe(virq);
+#endif
+
+	return ret;
+}
+
+void grgpio_irq_unmap(struct irq_domain *d, unsigned int virq)
+{
+	struct grgpio_priv *priv = d->host_data;
+	int index;
+	struct grgpio_lirq *lirq;
+	struct grgpio_uirq *uirq;
+	unsigned long flags;
+	int ngpio = priv->bgc.gc.ngpio;
+	int i;
+
+#ifdef CONFIG_ARM
+	set_irq_flags(virq, 0);
+#endif
+	irq_set_chip_and_handler(virq, NULL, NULL);
+	irq_set_chip_data(virq, NULL);
+
+	spin_lock_irqsave(&priv->bgc.lock, flags);
+
+	/* Free underlying irq if last user unmapped */
+	index = -1;
+	for (i = 0; i < ngpio; i++) {
+		lirq = &priv->lirqs[i];
+		if (lirq->virq == virq) {
+			grgpio_set_imask(priv, i, 0);
+			lirq->virq = 0;
+			index = lirq->index;
+			break;
+		}
+	}
+	WARN_ON(index < 0);
+
+	if (index >= 0) {
+		uirq = &priv->uirqs[lirq->index];
+		uirq->refcnt--;
+		if (uirq->refcnt == 0)
+			free_irq(uirq->uirq, priv);
+	}
+
+	spin_unlock_irqrestore(&priv->bgc.lock, flags);
 }
 
+static struct irq_domain_ops grgpio_irq_domain_ops = {
+	.map	= grgpio_irq_map,
+	.unmap	= grgpio_irq_unmap,
+};
+
+/* ------------------------------------------------------------ */
+
 static int grgpio_probe(struct platform_device *ofdev)
 {
 	struct device_node *np = ofdev->dev.of_node;
@@ -75,6 +329,9 @@ static int grgpio_probe(struct platform_device *ofdev)
 	struct resource *res;
 	int err;
 	u32 prop;
+	s32 *irqmap;
+	int size;
+	int i;
 
 	priv = devm_kzalloc(&ofdev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -94,6 +351,7 @@ static int grgpio_probe(struct platform_device *ofdev)
 	}
 
 	priv->regs = regs;
+	priv->imask = bgc->read_reg(&regs->imask);
 	priv->dev = &ofdev->dev;
 
 	gc = &bgc->gc;
@@ -119,6 +377,49 @@ static int grgpio_probe(struct platform_device *ofdev)
 		gc->ngpio = prop;
 	}
 
+	/* The irqmap contains the index values indicating which underlying irq,
+	 * if anyone, is connected to that line
+	 */
+	irqmap = (s32 *)of_get_property(np, "irqmap", &size);
+	if (irqmap) {
+		if (size < gc->ngpio) {
+			dev_err(&ofdev->dev,
+				"irqmap shorter than ngpio (%d < %d)\n",
+				size, gc->ngpio);
+			return -EINVAL;
+		}
+
+		priv->domain = irq_domain_add_linear(np, gc->ngpio,
+						     &grgpio_irq_domain_ops,
+						     priv);
+		if (!priv->domain) {
+			dev_err(&ofdev->dev, "Could not add irq domain\n");
+			return -EINVAL;
+		}
+
+		for (i = 0; i < gc->ngpio; i++) {
+			struct grgpio_lirq *lirq;
+			int ret;
+
+			lirq = &priv->lirqs[i];
+			lirq->index = irqmap[i];
+
+			if (lirq->index < 0)
+				continue;
+
+			ret = platform_get_irq(ofdev, lirq->index);
+			if (ret <= 0) {
+				/* Continue without irq functionality for that
+				 * gpio line
+				 */
+				dev_err(priv->dev,
+					"Failed to get irq for offset %d\n", i);
+				continue;
+			}
+			priv->uirqs[lirq->index].uirq = ret;
+		}
+	}
+
 	platform_set_drvdata(ofdev, priv);
 
 	err = gpiochip_add(gc);
@@ -127,8 +428,8 @@ static int grgpio_probe(struct platform_device *ofdev)
 		return err;
 	}
 
-	dev_info(&ofdev->dev, "regs=0x%p, base=%d, ngpio=%d\n",
-		 priv->regs, gc->base, gc->ngpio);
+	dev_info(&ofdev->dev, "regs=0x%p, base=%d, ngpio=%d, irqs=%s\n",
+		 priv->regs, gc->base, gc->ngpio, priv->domain ? "on" : "off");
 
 	return 0;
 }
@@ -136,8 +437,32 @@ static int grgpio_probe(struct platform_device *ofdev)
 static int grgpio_remove(struct platform_device *ofdev)
 {
 	struct grgpio_priv *priv = platform_get_drvdata(ofdev);
+	unsigned long flags;
+	int i;
+	int ret = 0;
+
+	spin_lock_irqsave(&priv->bgc.lock, flags);
+
+	if (priv->domain) {
+		for (i = 0; i < GRGPIO_MAX_NGPIO; i++) {
+			if (priv->uirqs[i].refcnt != 0) {
+				ret = -EBUSY;
+				goto out;
+			}
+		}
+	}
+
+	ret = gpiochip_remove(&priv->bgc.gc);
+	if (ret)
+		goto out;
+
+	if (priv->domain)
+		irq_domain_remove(priv->domain);
+
+out:
+	spin_unlock_irqrestore(&priv->bgc.lock, flags);
 
-	return gpiochip_remove(&priv->bgc.gc);
+	return ret;
 }
 
 static struct of_device_id grgpio_match[] = {
-- 
1.7.10.4

Re: [PATCH v5 3/3] gpio: grgpio: Add irq support

[Linus Walleij]

On Fri, Mar 15, 2013 at 2:45 PM, Andreas Larsson <andreas@gaisler.com> wrote:

> The drivers sets up an irq domain and hands out unique virqs to irq
> capable gpio lines regardless of which underlying irqs maps to which gpio
> line.
>
> Signed-off-by: Andreas Larsson <andreas@gaisler.com>
(...)
> +- irqmap : An array with an index for each gpio line. An index is either a valid
> +       index into the interrupts property array, or 0xffffffff that indicates
> +       no irq for that line. Driver provides no interrupt support if not
> +       present.

This looks really complicated.

Can't you just do this with a flag instead?

irq-enabled-mask = <0xf0f0f0f0>;

Like 0xf0f0f0f0 would indicate that lines 0-3, 8-11, 16-19 and 24-27
have no IRQ, lines 4-7, 12-15, 20-23 and 28-31 does have IRQ.

Then supply the resulting 16 IRQs in the interrupts property.

But you should double-check this with some more DT-aware person
I guess, there may be precedents. Did you get this idea from
some other binding?

> +struct grgpio_uirq {
> +       u8 refcnt; /* Reference counter to manage requesting/freeing of uirq */
> +       u8 uirq; /* Underlying virq of the gpio driver */

This looks very complicated. But well, maybe there are no
other ways...

If you do this you will need some spinlock to protect the refcount,
or make it an atomic type. Another way is to use <linux/kref.h>
if you refactor around that.

> +};
> +
> +struct grgpio_lirq {
> +       s8 index; /* Index into struct grgpio_priv's uirqs, or -1 */
> +       u8 virq; /* virq for the gpio line */

Just call it irq. These are Linux irqs, they are not "virtual".

> +       u32 imask; /* irq mask shadow register */
> +
> +       /* The grgpio core can have multiple irqs. Severeal gpio lines can
> +        * potentially be mapped to the same irq. This driver sets up an
> +        * irq domain and hands out separate virqs to each gpio line
> +        */

This is very common. Most GPIO controller have something like
one IRQ line per 32 GPIO lines. It's a bit odd that it can have
an arbitrary number of non-irq capable GPIOs though.

Note:

/*
 * I really prefer this comment style, where ther is no text on the
 * first line of the comment.
 */

> +       struct irq_domain *domain;
> +
> +       /* This array contains information on each underlying irq, each
> +        * irq of the grgpio core itself.
> +        */
> +       struct grgpio_uirq uirqs[GRGPIO_MAX_NGPIO];

So "u" is for "underlying"?

> +
> +       /* This array contains information for each gpio line on the
> +        * virtual irqs obtains from this driver. An index value of -1 for
> +        * a certain gpio line indicates that the line has no
> +        * irq. Otherwise the index connects the virq to the underlying
> +        * irq by pointing into the uirqs array.
> +        */
> +       struct grgpio_lirq lirqs[GRGPIO_MAX_NGPIO];

Hm, very complex...

> +static void grgpio_set_imask(struct grgpio_priv *priv, unsigned int offset,
> +                            int val)
> +{
> +       struct bgpio_chip *bgc = &priv->bgc;
> +       unsigned long mask = bgc->pin2mask(bgc, offset);
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&bgc->lock, flags);
> +
> +       if (val)
> +               priv->imask |= mask;
> +       else
> +               priv->imask &= ~mask;
> +       bgc->write_reg(&priv->regs->imask, priv->imask);

I prefer:

#define GRGPIO_IMASK 0xnn

bgc->write_reg(priv->base + GRGPIO_IMASK, priv->imask);

(...)

> +       ipol = priv->bgc.read_reg(&priv->regs->ipol) & ~mask;
> +       iedge = priv->bgc.read_reg(&priv->regs->iedge) & ~mask;
> +
> +       priv->bgc.write_reg(&priv->regs->ipol, ipol | pol);
> +       priv->bgc.write_reg(&priv->regs->iedge, iedge | edge);

Dito on all these.

> +static irqreturn_t grgpio_irq_handler(int irq, void *dev)
> +{
> +       struct grgpio_priv *priv = dev;
> +       int ngpio = priv->bgc.gc.ngpio;
> +       int i;
> +
> +       for (i = 0; i < ngpio; i++) {
> +               struct grgpio_lirq *lirq = &priv->lirqs[i];
> +
> +               if (priv->imask & BIT(i) && lirq->index >= 0 &&
> +                   priv->uirqs[lirq->index].uirq == irq) {
> +                       generic_handle_irq(lirq->virq);
> +               }

Should there not be an else clause here handling invalid
interrupts or printing an error or something?

> +       }
> +
> +       return IRQ_HANDLED;
> +}


(...)
> +
> +/* This function will be called as a consequence of the call to
> + * irq_create_mapping in grgpio_to_irq
> + */
> +int grgpio_irq_map(struct irq_domain *d, unsigned int virq,
> +                  irq_hw_number_t hwirq)
> +{
> +       struct grgpio_priv *priv = d->host_data;
> +       struct grgpio_lirq *lirq;
> +       struct grgpio_uirq *uirq;
> +       unsigned long flags;
> +       int offset = hwirq;
> +       int ret = 0;
> +
> +       if (!priv)
> +               return -EINVAL;
> +
> +       lirq = &priv->lirqs[offset];
> +       if (lirq->index < 0)
> +               return -EINVAL;
> +
> +       dev_dbg(priv->dev, "Mapping virq %d for gpio line %d\n",
> +               virq, offset);
> +
> +       spin_lock_irqsave(&priv->bgc.lock, flags);
> +
> +       /* Request underlying irq if not already requested */
> +       lirq->virq = virq;
> +       uirq = &priv->uirqs[lirq->index];
> +       if (uirq->refcnt == 0) {
> +               ret = request_irq(uirq->uirq, grgpio_irq_handler, 0,
> +                                 dev_name(priv->dev), priv);


No, please request all present uirqs in probe() before creating the
irqdomain.

And use devm_request_irq() for that.

> +               if (ret) {
> +                       dev_err(priv->dev,
> +                               "Could not request underlying irq %d\n",
> +                               uirq->uirq);
> +
> +                       spin_unlock_irqrestore(&priv->bgc.lock, flags);
> +
> +                       return ret;
> +               }
> +       }
> +       uirq->refcnt++;
> +
> +       spin_unlock_irqrestore(&priv->bgc.lock, flags);
> +
> +       /* Setup virq  */
> +       irq_set_chip_data(virq, priv);
> +       irq_set_chip_and_handler(virq, &grgpio_irq_chip,
> +                                handle_simple_irq);
> +       irq_clear_status_flags(virq, IRQ_NOREQUEST);
> +#ifdef CONFIG_ARM
> +       set_irq_flags(virq, IRQF_VALID);
> +#else
> +       irq_set_noprobe(virq);
> +#endif
> +
> +       return ret;
> +}
> +
> +void grgpio_irq_unmap(struct irq_domain *d, unsigned int virq)
> +{
> +       struct grgpio_priv *priv = d->host_data;
> +       int index;
> +       struct grgpio_lirq *lirq;
> +       struct grgpio_uirq *uirq;
> +       unsigned long flags;
> +       int ngpio = priv->bgc.gc.ngpio;
> +       int i;
> +
> +#ifdef CONFIG_ARM
> +       set_irq_flags(virq, 0);
> +#endif
> +       irq_set_chip_and_handler(virq, NULL, NULL);
> +       irq_set_chip_data(virq, NULL);
> +
> +       spin_lock_irqsave(&priv->bgc.lock, flags);
> +
> +       /* Free underlying irq if last user unmapped */
> +       index = -1;
> +       for (i = 0; i < ngpio; i++) {
> +               lirq = &priv->lirqs[i];
> +               if (lirq->virq == virq) {
> +                       grgpio_set_imask(priv, i, 0);
> +                       lirq->virq = 0;
> +                       index = lirq->index;
> +                       break;
> +               }
> +       }
> +       WARN_ON(index < 0);
> +
> +       if (index >= 0) {
> +               uirq = &priv->uirqs[lirq->index];
> +               uirq->refcnt--;
> +               if (uirq->refcnt == 0)
> +                       free_irq(uirq->uirq, priv);
> +       }


Don't do that either. devm_request_irq() will free the
IRQ when the driver unloads. No need for messy code
trying to cover it up.

> +       spin_unlock_irqrestore(&priv->bgc.lock, flags);
>  }
>
> +static struct irq_domain_ops grgpio_irq_domain_ops = {
> +       .map    = grgpio_irq_map,
> +       .unmap  = grgpio_irq_unmap,
> +};
> +
> +/* ------------------------------------------------------------ */
> +
>  static int grgpio_probe(struct platform_device *ofdev)
>  {
>         struct device_node *np = ofdev->dev.of_node;
> @@ -75,6 +329,9 @@ static int grgpio_probe(struct platform_device *ofdev)
>         struct resource *res;
>         int err;
>         u32 prop;
> +       s32 *irqmap;
> +       int size;
> +       int i;
>
>         priv = devm_kzalloc(&ofdev->dev, sizeof(*priv), GFP_KERNEL);
>         if (!priv)
> @@ -94,6 +351,7 @@ static int grgpio_probe(struct platform_device *ofdev)
>         }
>
>         priv->regs = regs;
> +       priv->imask = bgc->read_reg(&regs->imask);
>         priv->dev = &ofdev->dev;
>
>         gc = &bgc->gc;
> @@ -119,6 +377,49 @@ static int grgpio_probe(struct platform_device *ofdev)
>                 gc->ngpio = prop;
>         }
>
> +       /* The irqmap contains the index values indicating which underlying irq,
> +        * if anyone, is connected to that line
> +        */
> +       irqmap = (s32 *)of_get_property(np, "irqmap", &size);
> +       if (irqmap) {
> +               if (size < gc->ngpio) {
> +                       dev_err(&ofdev->dev,
> +                               "irqmap shorter than ngpio (%d < %d)\n",
> +                               size, gc->ngpio);
> +                       return -EINVAL;
> +               }


So devm_request all uirqs somehere like here.

> +               priv->domain = irq_domain_add_linear(np, gc->ngpio,
> +                                                    &grgpio_irq_domain_ops,
> +                                                    priv);

Then move this below the loop, so you know you have all uirqs
when you create the domain.

> +               if (!priv->domain) {
> +                       dev_err(&ofdev->dev, "Could not add irq domain\n");
> +                       return -EINVAL;
> +               }
> +
> +               for (i = 0; i < gc->ngpio; i++) {
> +                       struct grgpio_lirq *lirq;
> +                       int ret;
> +
> +                       lirq = &priv->lirqs[i];
> +                       lirq->index = irqmap[i];
> +
> +                       if (lirq->index < 0)
> +                               continue;
> +
> +                       ret = platform_get_irq(ofdev, lirq->index);
> +                       if (ret <= 0) {
> +                               /* Continue without irq functionality for that
> +                                * gpio line
> +                                */
> +                               dev_err(priv->dev,
> +                                       "Failed to get irq for offset %d\n", i);
> +                               continue;
> +                       }
> +                       priv->uirqs[lirq->index].uirq = ret;
> +               }
> +       }
> +
>         platform_set_drvdata(ofdev, priv);
>
>         err = gpiochip_add(gc);
> @@ -127,8 +428,8 @@ static int grgpio_probe(struct platform_device *ofdev)
>                 return err;
>         }
>
> -       dev_info(&ofdev->dev, "regs=0x%p, base=%d, ngpio=%d\n",
> -                priv->regs, gc->base, gc->ngpio);
> +       dev_info(&ofdev->dev, "regs=0x%p, base=%d, ngpio=%d, irqs=%s\n",
> +                priv->regs, gc->base, gc->ngpio, priv->domain ? "on" : "off");
>
>         return 0;
>  }
(...)

Yours,
Linus Walleij

Re: [PATCH v5 3/3] gpio: grgpio: Add irq support

[Andreas Larson]

On 2013-04-10 21:25, Linus Walleij wrote:
> On Fri, Mar 15, 2013 at 2:45 PM, Andreas Larsson <andreas@gaisler.com> wrote:
>
>> The drivers sets up an irq domain and hands out unique virqs to irq
>> capable gpio lines regardless of which underlying irqs maps to which gpio
>> line.
>>
>> Signed-off-by: Andreas Larsson <andreas@gaisler.com>
> (...)
>> +- irqmap : An array with an index for each gpio line. An index is either a valid
>> +       index into the interrupts property array, or 0xffffffff that indicates
>> +       no irq for that line. Driver provides no interrupt support if not
>> +       present.
>
> This looks really complicated.
>
> Can't you just do this with a flag instead?
>
> irq-enabled-mask = <0xf0f0f0f0>;
>
> Like 0xf0f0f0f0 would indicate that lines 0-3, 8-11, 16-19 and 24-27
> have no IRQ, lines 4-7, 12-15, 20-23 and 28-31 does have IRQ.
>
> Then supply the resulting 16 IRQs in the interrupts property.
>
> But you should double-check this with some more DT-aware person
> I guess, there may be precedents. Did you get this idea from
> some other binding?
>
>> +struct grgpio_uirq {
>> +       u8 refcnt; /* Reference counter to manage requesting/freeing of uirq */
>> +       u8 uirq; /* Underlying virq of the gpio driver */
>
> This looks very complicated. But well, maybe there are no
> other ways...

The reason for the complex handling is that any line can any one irq of
the irqs of the gpio core or no irq at all - completely independenly of
each other.

E.g. we can have a a system where:
- line 0 has no irq
- line 1 has irq 7
- line 2 has irq 9
- line 3 has irq 7
- line 4 has no irq
- line 5 has irq 4
- etc.

That is the reason why the "imap" of property (originating from the
PROM) contains an index that maps a line to one of the irqs of the core
or indicates no irq at all.


> If you do this you will need some spinlock to protect the refcount,
> or make it an atomic type. Another way is to use <linux/kref.h>
> if you refactor around that.

Yes, I use the bgpio-lock for that but I missed using it in the irq 
handler - added.


>
>> +};
>> +
>> +struct grgpio_lirq {
>> +       s8 index; /* Index into struct grgpio_priv's uirqs, or -1 */
>> +       u8 virq; /* virq for the gpio line */
>
> Just call it irq. These are Linux irqs, they are not "virtual".

Sure

>
>> +       u32 imask; /* irq mask shadow register */
>> +
>> +       /* The grgpio core can have multiple irqs. Severeal gpio lines can
>> +        * potentially be mapped to the same irq. This driver sets up an
>> +        * irq domain and hands out separate virqs to each gpio line
>> +        */
>
> This is very common. Most GPIO controller have something like
> one IRQ line per 32 GPIO lines. It's a bit odd that it can have
> an arbitrary number of non-irq capable GPIOs though.
>
> Note:
>
> /*
>   * I really prefer this comment style, where ther is no text on the
>   * first line of the comment.
>   */

Sure

>> +       struct irq_domain *domain;
>> +
>> +       /* This array contains information on each underlying irq, each
>> +        * irq of the grgpio core itself.
>> +        */
>> +       struct grgpio_uirq uirqs[GRGPIO_MAX_NGPIO];
>
> So "u" is for "underlying"?

Yes

>> +
>> +       /* This array contains information for each gpio line on the
>> +        * virtual irqs obtains from this driver. An index value of -1 for
>> +        * a certain gpio line indicates that the line has no
>> +        * irq. Otherwise the index connects the virq to the underlying
>> +        * irq by pointing into the uirqs array.
>> +        */
>> +       struct grgpio_lirq lirqs[GRGPIO_MAX_NGPIO];
>
> Hm, very complex...
>
>> +static void grgpio_set_imask(struct grgpio_priv *priv, unsigned int offset,
>> +                            int val)
>> +{
>> +       struct bgpio_chip *bgc = &priv->bgc;
>> +       unsigned long mask = bgc->pin2mask(bgc, offset);
>> +       unsigned long flags;
>> +
>> +       spin_lock_irqsave(&bgc->lock, flags);
>> +
>> +       if (val)
>> +               priv->imask |= mask;
>> +       else
>> +               priv->imask &= ~mask;
>> +       bgc->write_reg(&priv->regs->imask, priv->imask);
>
> I prefer:
>
> #define GRGPIO_IMASK 0xnn
>
> bgc->write_reg(priv->base + GRGPIO_IMASK, priv->imask);
>
> (...)
>
>> +       ipol = priv->bgc.read_reg(&priv->regs->ipol) & ~mask;
>> +       iedge = priv->bgc.read_reg(&priv->regs->iedge) & ~mask;
>> +
>> +       priv->bgc.write_reg(&priv->regs->ipol, ipol | pol);
>> +       priv->bgc.write_reg(&priv->regs->iedge, iedge | edge);
>
> Dito on all these.
>
>> +static irqreturn_t grgpio_irq_handler(int irq, void *dev)
>> +{
>> +       struct grgpio_priv *priv = dev;
>> +       int ngpio = priv->bgc.gc.ngpio;
>> +       int i;
>> +
>> +       for (i = 0; i < ngpio; i++) {
>> +               struct grgpio_lirq *lirq = &priv->lirqs[i];
>> +
>> +               if (priv->imask & BIT(i) && lirq->index >= 0 &&
>> +                   priv->uirqs[lirq->index].uirq == irq) {
>> +                       generic_handle_irq(lirq->virq);
>> +               }
>
> Should there not be an else clause here handling invalid
> interrupts or printing an error or something?

Not an else clause per se, as we need to loop through all the lines and
only treat the ones that should be triggered by the given underlying
irq. I'll add a comment on what it is all about.

On the other hand, if no line whatsoever matches it might be good to add
a warning about it. I am not sure if it could legitimately happen on an
SMP system if the irq is unmapped at one CPU at the same time as another
after another CPU has entered this interrupt handler but before taking
the lock protecting the uirq structure.

>
>> +       }
>> +
>> +       return IRQ_HANDLED;
>> +}
>
>
> (...)
>> +
>> +/* This function will be called as a consequence of the call to
>> + * irq_create_mapping in grgpio_to_irq
>> + */
>> +int grgpio_irq_map(struct irq_domain *d, unsigned int virq,
>> +                  irq_hw_number_t hwirq)
>> +{
>> +       struct grgpio_priv *priv = d->host_data;
>> +       struct grgpio_lirq *lirq;
>> +       struct grgpio_uirq *uirq;
>> +       unsigned long flags;
>> +       int offset = hwirq;
>> +       int ret = 0;
>> +
>> +       if (!priv)
>> +               return -EINVAL;
>> +
>> +       lirq = &priv->lirqs[offset];
>> +       if (lirq->index < 0)
>> +               return -EINVAL;
>> +
>> +       dev_dbg(priv->dev, "Mapping virq %d for gpio line %d\n",
>> +               virq, offset);
>> +
>> +       spin_lock_irqsave(&priv->bgc.lock, flags);
>> +
>> +       /* Request underlying irq if not already requested */
>> +       lirq->virq = virq;
>> +       uirq = &priv->uirqs[lirq->index];
>> +       if (uirq->refcnt == 0) {
>> +               ret = request_irq(uirq->uirq, grgpio_irq_handler, 0,
>> +                                 dev_name(priv->dev), priv);
>
>
> No, please request all present uirqs in probe() before creating the
> irqdomain.


The reason for doing it at irq_map and not in probe is that one
typical hardware setup for this core is that it has irqs that are shared
with pretty much all other cores in the system. Therefore, if this
drivers requests all its irqs in the probe function, pretty much all the
drivers for the other cores in the system that were not lucky enough to
request their irq before this driver will fail their probes in such a
hardware setup. This driver can not share irq:s so even if the other
drivers can handle shared irqs they are out of luck if this driver is
probed first.

That is why only the irqs that are actually used should be requested and
thus why the driver does it on demand instead of in the probe.


> And use devm_request_irq() for that.
>
>> +               if (ret) {
>> +                       dev_err(priv->dev,
>> +                               "Could not request underlying irq %d\n",
>> +                               uirq->uirq);
>> +
>> +                       spin_unlock_irqrestore(&priv->bgc.lock, flags);
>> +
>> +                       return ret;
>> +               }
>> +       }
>> +       uirq->refcnt++;
>> +
>> +       spin_unlock_irqrestore(&priv->bgc.lock, flags);
>> +
>> +       /* Setup virq  */
>> +       irq_set_chip_data(virq, priv);
>> +       irq_set_chip_and_handler(virq, &grgpio_irq_chip,
>> +                                handle_simple_irq);
>> +       irq_clear_status_flags(virq, IRQ_NOREQUEST);
>> +#ifdef CONFIG_ARM
>> +       set_irq_flags(virq, IRQF_VALID);
>> +#else
>> +       irq_set_noprobe(virq);
>> +#endif
>> +
>> +       return ret;
>> +}
>> +
>> +void grgpio_irq_unmap(struct irq_domain *d, unsigned int virq)
>> +{
>> +       struct grgpio_priv *priv = d->host_data;
>> +       int index;
>> +       struct grgpio_lirq *lirq;
>> +       struct grgpio_uirq *uirq;
>> +       unsigned long flags;
>> +       int ngpio = priv->bgc.gc.ngpio;
>> +       int i;
>> +
>> +#ifdef CONFIG_ARM
>> +       set_irq_flags(virq, 0);
>> +#endif
>> +       irq_set_chip_and_handler(virq, NULL, NULL);
>> +       irq_set_chip_data(virq, NULL);
>> +
>> +       spin_lock_irqsave(&priv->bgc.lock, flags);
>> +
>> +       /* Free underlying irq if last user unmapped */
>> +       index = -1;
>> +       for (i = 0; i < ngpio; i++) {
>> +               lirq = &priv->lirqs[i];
>> +               if (lirq->virq == virq) {
>> +                       grgpio_set_imask(priv, i, 0);
>> +                       lirq->virq = 0;
>> +                       index = lirq->index;
>> +                       break;
>> +               }
>> +       }
>> +       WARN_ON(index < 0);
>> +
>> +       if (index >= 0) {
>> +               uirq = &priv->uirqs[lirq->index];
>> +               uirq->refcnt--;
>> +               if (uirq->refcnt == 0)
>> +                       free_irq(uirq->uirq, priv);
>> +       }
>
>
> Don't do that either. devm_request_irq() will free the
> IRQ when the driver unloads. No need for messy code
> trying to cover it up.
>
>> +       spin_unlock_irqrestore(&priv->bgc.lock, flags);
>>   }
>>
>> +static struct irq_domain_ops grgpio_irq_domain_ops = {
>> +       .map    = grgpio_irq_map,
>> +       .unmap  = grgpio_irq_unmap,
>> +};
>> +
>> +/* ------------------------------------------------------------ */
>> +
>>   static int grgpio_probe(struct platform_device *ofdev)
>>   {
>>          struct device_node *np = ofdev->dev.of_node;
>> @@ -75,6 +329,9 @@ static int grgpio_probe(struct platform_device *ofdev)
>>          struct resource *res;
>>          int err;
>>          u32 prop;
>> +       s32 *irqmap;
>> +       int size;
>> +       int i;
>>
>>          priv = devm_kzalloc(&ofdev->dev, sizeof(*priv), GFP_KERNEL);
>>          if (!priv)
>> @@ -94,6 +351,7 @@ static int grgpio_probe(struct platform_device *ofdev)
>>          }
>>
>>          priv->regs = regs;
>> +       priv->imask = bgc->read_reg(&regs->imask);
>>          priv->dev = &ofdev->dev;
>>
>>          gc = &bgc->gc;
>> @@ -119,6 +377,49 @@ static int grgpio_probe(struct platform_device *ofdev)
>>                  gc->ngpio = prop;
>>          }
>>
>> +       /* The irqmap contains the index values indicating which underlying irq,
>> +        * if anyone, is connected to that line
>> +        */
>> +       irqmap = (s32 *)of_get_property(np, "irqmap", &size);
>> +       if (irqmap) {
>> +               if (size < gc->ngpio) {
>> +                       dev_err(&ofdev->dev,
>> +                               "irqmap shorter than ngpio (%d < %d)\n",
>> +                               size, gc->ngpio);
>> +                       return -EINVAL;
>> +               }
>
>
> So devm_request all uirqs somehere like here.
>
>> +               priv->domain = irq_domain_add_linear(np, gc->ngpio,
>> +                                                    &grgpio_irq_domain_ops,
>> +                                                    priv);
>
> Then move this below the loop, so you know you have all uirqs
> when you create the domain.
>
>> +               if (!priv->domain) {
>> +                       dev_err(&ofdev->dev, "Could not add irq domain\n");
>> +                       return -EINVAL;
>> +               }
>> +
>> +               for (i = 0; i < gc->ngpio; i++) {
>> +                       struct grgpio_lirq *lirq;
>> +                       int ret;
>> +
>> +                       lirq = &priv->lirqs[i];
>> +                       lirq->index = irqmap[i];
>> +
>> +                       if (lirq->index < 0)
>> +                               continue;
>> +
>> +                       ret = platform_get_irq(ofdev, lirq->index);
>> +                       if (ret <= 0) {
>> +                               /* Continue without irq functionality for that
>> +                                * gpio line
>> +                                */
>> +                               dev_err(priv->dev,
>> +                                       "Failed to get irq for offset %d\n", i);
>> +                               continue;
>> +                       }
>> +                       priv->uirqs[lirq->index].uirq = ret;
>> +               }
>> +       }
>> +
>>          platform_set_drvdata(ofdev, priv);
>>
>>          err = gpiochip_add(gc);
>> @@ -127,8 +428,8 @@ static int grgpio_probe(struct platform_device *ofdev)
>>                  return err;
>>          }
>>
>> -       dev_info(&ofdev->dev, "regs=0x%p, base=%d, ngpio=%d\n",
>> -                priv->regs, gc->base, gc->ngpio);
>> +       dev_info(&ofdev->dev, "regs=0x%p, base=%d, ngpio=%d, irqs=%s\n",
>> +                priv->regs, gc->base, gc->ngpio, priv->domain ? "on" : "off");
>>
>>          return 0;
>>   }
> (...)

Thanks for the feedback!

Cheers,
Andreas Larsson

Re: [PATCH v5 3/3] gpio: grgpio: Add irq support

[Linus Walleij]

On Mon, Apr 15, 2013 at 2:38 PM, Andreas Larson <andreas@gaisler.com> wrote:

> On 2013-04-10 21:25, Linus Walleij wrote:
>>> +int grgpio_irq_map(struct irq_domain *d, unsigned int virq,
(...)
>>> +       /* Request underlying irq if not already requested */
>>> +       lirq->virq = virq;
>>> +       uirq = &priv->uirqs[lirq->index];
>>> +       if (uirq->refcnt == 0) {
>>> +               ret = request_irq(uirq->uirq, grgpio_irq_handler, 0,
>>> +                                 dev_name(priv->dev), priv);
>>
>>
>> No, please request all present uirqs in probe() before creating the
>> irqdomain.
>
>
> The reason for doing it at irq_map and not in probe is that one
> typical hardware setup for this core is that it has irqs that are shared
> with pretty much all other cores in the system. Therefore, if this
> drivers requests all its irqs in the probe function, pretty much all the
> drivers for the other cores in the system that were not lucky enough to
> request their irq before this driver will fail their probes in such a
> hardware setup. This driver can not share irq:s so even if the other
> drivers can handle shared irqs they are out of luck if this driver is
> probed first.
>
> That is why only the irqs that are actually used should be requested and
> thus why the driver does it on demand instead of in the probe.

OK I buy that explanation...

Thanks,
Linus Walleij