[PATCH 0/3] gpio-dwapb plus irq domain

[PATCH 0/3] gpio-dwapb plus irq domain

[Alan Tull]

From: Alan Tull <atull@altera.com>

Resending Jamie's gpio-dwapb driver patches.  Adding a patch to get 
them building with current irq domain stuff.

Alan Tull


Alan Tull (1):
  use linear irq domain in gpio-dwapb

Jamie Iles (2):
  gpio: add a driver for the Synopsys DesignWare APB GPIO block
  gpio: dwapb: add support for GPIO interrupts

 .../devicetree/bindings/gpio/snps-dwapb-gpio.txt   |   57 +++
 drivers/gpio/Kconfig                               |    9 +
 drivers/gpio/Makefile                              |    1 +
 drivers/gpio/gpio-dwapb.c                          |  436 ++++++++++++++++++++
 4 files changed, 503 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
 create mode 100644 drivers/gpio/gpio-dwapb.c

-- 
1.7.9.5

[PATCH 1/3] gpio: add a driver for the Synopsys DesignWare APB GPIO block

[Alan Tull]

From: Jamie Iles <jamie@jamieiles.com>

The Synopsys DesignWare block is used in some ARM devices (picoxcell)
and can be configured to provide multiple banks of GPIO pins.

v5:	- handle sparse bank population correctly
v3:	- depend on rather than select IRQ_DOMAIN
	- split IRQ support into a separate patch
v2:	- use Rob Herring's irqdomain in generic irq chip patches
	- use reg property to indicate bank index
	- support irqs on both edges based on LinusW's u300 driver

Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Linus Walleij <linus.walleij@stericsson.com>
Acked-by: Rob Herring <rob.herring@calxeda.com>
Signed-off-by: Jamie Iles <jamie@jamieiles.com>
---
 .../devicetree/bindings/gpio/snps-dwapb-gpio.txt   |   57 +++++++
 drivers/gpio/Kconfig                               |    9 +
 drivers/gpio/Makefile                              |    1 +
 drivers/gpio/gpio-dwapb.c                          |  172 ++++++++++++++++++++
 4 files changed, 239 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
 create mode 100644 drivers/gpio/gpio-dwapb.c

diff --git a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
new file mode 100644
index 0000000..73adf37
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
@@ -0,0 +1,57 @@
+* Synopsys DesignWare APB GPIO controller
+
+Required properties:
+- compatible : Should be "snps,dw-apb-gpio"
+- reg : Address and length of the register set for the device
+
+The GPIO controller has a configurable number of banks, each of which are
+represented as child nodes with the following properties:
+
+Required properties:
+- compatible : "snps,dw-apb-gpio-bank"
+- gpio-controller : Marks the device node as a gpio controller.
+- #gpio-cells : Should be two.  The first cell is the pin number and
+  the second cell is used to specify optional parameters (currently
+  unused).
+- reg : The integer bank index of the bank, a single cell.
+- nr-gpio : The number of pins in the bank, a single cell.
+
+Optional properties:
+- interrupt-controller : The first bank may be configured to be an interrupt
+controller.
+- #interrupt-cells : Specifies the number of cells needed to encode an
+interrupt.  Shall be set to 2.  The first cell defines the interrupt number,
+the second encodes the triger flags encoded as described in
+Documentation/devicetree/bindings/interrupts.txt
+- interrupt-parent : The parent interrupt controller.
+- interrupts : The interrupts to the parent controller raised when GPIOs
+generate the interrupts.
+
+Example:
+
+gpio: gpio@20000 {
+	compatible = "snps,dw-apb-gpio";
+	reg = <0x20000 0x1000>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	banka: gpio-controller@0 {
+		compatible = "snps,dw-apb-gpio-bank";
+		gpio-controller;
+		#gpio-cells = <2>;
+		nr-gpio = <8>;
+		reg = <0>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+		interrupt-parent = <&vic1>;
+		interrupts = <0 1 2 3 4 5 6 7>;
+	};
+
+	bankb: gpio-controller@1 {
+		compatible = "snps,dw-apb-gpio-bank";
+		gpio-controller;
+		#gpio-cells = <2>;
+		nr-gpio = <8>;
+		reg = <1>;
+	};
+};
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b6ed304..c5e7868 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -121,6 +121,15 @@ config GPIO_GENERIC_PLATFORM
 	help
 	  Say yes here to support basic platform_device memory-mapped GPIO controllers.
 
+config GPIO_DWAPB
+	bool "Synopsys DesignWare APB GPIO driver"
+	select GPIO_GENERIC
+	select GENERIC_IRQ_CHIP
+	depends on OF_GPIO
+	help
+	  Say Y or M here to build support for the Synopsys DesignWare APB
+	  GPIO block.  This requires device tree support.
+
 config GPIO_IT8761E
 	tristate "IT8761E GPIO support"
 	depends on X86  # unconditional access to IO space.
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 98e23eb..8de041a 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_GPIO_CS5535)	+= gpio-cs5535.o
 obj-$(CONFIG_GPIO_DA9052)	+= gpio-da9052.o
 obj-$(CONFIG_GPIO_DA9055)	+= gpio-da9055.o
 obj-$(CONFIG_ARCH_DAVINCI)	+= gpio-davinci.o
+obj-$(CONFIG_GPIO_DWAPB)	+= gpio-dwapb.o
 obj-$(CONFIG_GPIO_EM)		+= gpio-em.o
 obj-$(CONFIG_GPIO_EP93XX)	+= gpio-ep93xx.o
 obj-$(CONFIG_GPIO_F7188X)	+= gpio-f7188x.o
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
new file mode 100644
index 0000000..3a28697
--- /dev/null
+++ b/drivers/gpio/gpio-dwapb.c
@@ -0,0 +1,172 @@
+/*
+ * Copyright (c) 2011 Jamie Iles
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * All enquiries to support@picochip.com
+ */
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/module.h>
+#include <linux/basic_mmio_gpio.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+
+struct dwapb_gpio;
+
+struct dwapb_gpio_bank {
+	struct bgpio_chip	bgc;
+	unsigned int		bank_idx;
+	bool			is_registered;
+	struct dwapb_gpio	*gpio;
+};
+
+struct dwapb_gpio {
+	struct device_node	*of_node;
+	struct	device		*dev;
+	void __iomem		*regs;
+	struct dwapb_gpio_bank	*banks;
+	unsigned int		nr_banks;
+};
+
+static unsigned int dwapb_gpio_nr_banks(struct device_node *of_node)
+{
+	unsigned int nr_banks = 0;
+	struct device_node *np;
+
+	for_each_child_of_node(of_node, np)
+		++nr_banks;
+
+	return nr_banks;
+}
+
+static int dwapb_gpio_add_bank(struct dwapb_gpio *gpio,
+			       struct device_node *bank_np,
+			       unsigned int offs)
+{
+	struct dwapb_gpio_bank *bank;
+	u32 bank_idx, ngpio;
+	int err;
+
+	if (of_property_read_u32(bank_np, "reg", &bank_idx)) {
+		dev_err(gpio->dev, "invalid bank index for %s\n",
+			bank_np->full_name);
+		return -EINVAL;
+	}
+	bank = &gpio->banks[offs];
+	bank->gpio = gpio;
+
+	if (of_property_read_u32(bank_np, "nr-gpio", &ngpio)) {
+		dev_err(gpio->dev, "failed to get number of gpios for %s\n",
+			bank_np->full_name);
+		return -EINVAL;
+	}
+
+	bank->bank_idx = bank_idx;
+	err = bgpio_init(&bank->bgc, gpio->dev, 4,
+			 gpio->regs + 0x50 + (bank_idx * 0x4),
+			 gpio->regs + 0x00 + (bank_idx * 0xc),
+			 NULL, gpio->regs + 0x04 + (bank_idx * 0xc), NULL,
+			 false);
+	if (err) {
+		dev_err(gpio->dev, "failed to init gpio chip for %s\n",
+			bank_np->full_name);
+		return err;
+	}
+
+	bank->bgc.gc.ngpio = ngpio;
+	bank->bgc.gc.of_node = bank_np;
+
+	err = gpiochip_add(&bank->bgc.gc);
+	if (err)
+		dev_err(gpio->dev, "failed to register gpiochip for %s\n",
+			bank_np->full_name);
+	else
+		bank->is_registered = true;
+
+	return err;
+}
+
+static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
+{
+	unsigned int m;
+
+	for (m = 0; m < gpio->nr_banks; ++m)
+		if (gpio->banks[m].is_registered)
+			WARN_ON(gpiochip_remove(&gpio->banks[m].bgc.gc));
+	of_node_put(gpio->of_node);
+}
+
+static int __devinit dwapb_gpio_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct dwapb_gpio *gpio;
+	struct device_node *np;
+	int err;
+	unsigned int offs = 0;
+
+	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+	if (!gpio)
+		return -ENOMEM;
+	gpio->dev = &pdev->dev;
+
+	gpio->nr_banks = dwapb_gpio_nr_banks(pdev->dev.of_node);
+	if (!gpio->nr_banks)
+		return -EINVAL;
+	gpio->banks = devm_kzalloc(&pdev->dev, gpio->nr_banks *
+				   sizeof(*gpio->banks), GFP_KERNEL);
+	if (!gpio->banks)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "failed to get iomem\n");
+		return -ENXIO;
+	}
+	gpio->regs = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (!gpio->regs)
+		return -ENOMEM;
+
+	gpio->of_node = of_node_get(pdev->dev.of_node);
+	for_each_child_of_node(pdev->dev.of_node, np) {
+		err = dwapb_gpio_add_bank(gpio, np, offs++);
+		if (err)
+			goto out_unregister;
+	}
+	platform_set_drvdata(pdev, gpio);
+
+	return 0;
+
+out_unregister:
+	dwapb_gpio_unregister(gpio);
+
+	return err;
+}
+
+static const struct of_device_id dwapb_of_match_table[] = {
+	{ .compatible = "snps,dw-apb-gpio" },
+	{ /* Sentinel */ }
+};
+
+static struct platform_driver dwapb_gpio_driver = {
+	.driver		= {
+		.name	= "gpio-dwapb",
+		.owner	= THIS_MODULE,
+		.of_match_table = dwapb_of_match_table,
+	},
+	.probe		= dwapb_gpio_probe,
+};
+
+static int __init dwapb_gpio_init(void)
+{
+	return platform_driver_register(&dwapb_gpio_driver);
+}
+postcore_initcall(dwapb_gpio_init);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Jamie Iles");
+MODULE_DESCRIPTION("Synopsys DesignWare APB GPIO driver");
-- 
1.7.9.5

Re: [PATCH 1/3] gpio: add a driver for the Synopsys DesignWare APB GPIO block

[Sebastian Hesselbarth]

On 11/05/2013 05:55 PM, Alan Tull wrote:
> From: Jamie Iles <jamie@jamieiles.com>
>
> The Synopsys DesignWare block is used in some ARM devices (picoxcell)
> and can be configured to provide multiple banks of GPIO pins.
>
> v5:	- handle sparse bank population correctly
> v3:	- depend on rather than select IRQ_DOMAIN
> 	- split IRQ support into a separate patch
> v2:	- use Rob Herring's irqdomain in generic irq chip patches
> 	- use reg property to indicate bank index
> 	- support irqs on both edges based on LinusW's u300 driver
>

Alan,

thanks for reposting this. I haven't reviewed one of the earlier
versions, so some of my comments may be referring to already answered
questions.

> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Linus Walleij <linus.walleij@stericsson.com>
> Acked-by: Rob Herring <rob.herring@calxeda.com>
> Signed-off-by: Jamie Iles <jamie@jamieiles.com>
> ---
>   .../devicetree/bindings/gpio/snps-dwapb-gpio.txt   |   57 +++++++
>   drivers/gpio/Kconfig                               |    9 +
>   drivers/gpio/Makefile                              |    1 +
>   drivers/gpio/gpio-dwapb.c                          |  172 ++++++++++++++++++++
>   4 files changed, 239 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
>   create mode 100644 drivers/gpio/gpio-dwapb.c
>
> diff --git a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
> new file mode 100644
> index 0000000..73adf37
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
> @@ -0,0 +1,57 @@
> +* Synopsys DesignWare APB GPIO controller
> +
> +Required properties:
> +- compatible : Should be "snps,dw-apb-gpio"
> +- reg : Address and length of the register set for the device
> +
> +The GPIO controller has a configurable number of banks, each of which are
> +represented as child nodes with the following properties:
> +
> +Required properties:
> +- compatible : "snps,dw-apb-gpio-bank"

The register names refer to "bank" as "port", the above compatible
should also reflect this.

> +- gpio-controller : Marks the device node as a gpio controller.
> +- #gpio-cells : Should be two.  The first cell is the pin number and
> +  the second cell is used to specify optional parameters (currently
> +  unused).
> +- reg : The integer bank index of the bank, a single cell.
> +- nr-gpio : The number of pins in the bank, a single cell.

What about "nr-gpios" or "#gpios"? Also, this property should be
optional, as it can also be read from DW APB GPIOs CONFIG[12]
registers.

> +Optional properties:
> +- interrupt-controller : The first bank may be configured to be an interrupt
> +controller.
> +- #interrupt-cells : Specifies the number of cells needed to encode an
> +interrupt.  Shall be set to 2.  The first cell defines the interrupt number,
> +the second encodes the triger flags encoded as described in
> +Documentation/devicetree/bindings/interrupts.txt
> +- interrupt-parent : The parent interrupt controller.
> +- interrupts : The interrupts to the parent controller raised when GPIOs
> +generate the interrupts.
> +
> +Example:
> +
> +gpio: gpio@20000 {
> +	compatible = "snps,dw-apb-gpio";
> +	reg = <0x20000 0x1000>;
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	banka: gpio-controller@0 {
> +		compatible = "snps,dw-apb-gpio-bank";
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		nr-gpio = <8>;
> +		reg = <0>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +		interrupt-parent = <&vic1>;
> +		interrupts = <0 1 2 3 4 5 6 7>;
> +	};
> +
> +	bankb: gpio-controller@1 {
> +		compatible = "snps,dw-apb-gpio-bank";
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		nr-gpio = <8>;
> +		reg = <1>;
> +	};
> +};
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index b6ed304..c5e7868 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -121,6 +121,15 @@ config GPIO_GENERIC_PLATFORM
>   	help
>   	  Say yes here to support basic platform_device memory-mapped GPIO controllers.
>
> +config GPIO_DWAPB
> +	bool "Synopsys DesignWare APB GPIO driver"
> +	select GPIO_GENERIC
> +	select GENERIC_IRQ_CHIP
> +	depends on OF_GPIO
> +	help
> +	  Say Y or M here to build support for the Synopsys DesignWare APB
> +	  GPIO block.  This requires device tree support.

I'd remove the comment about DT requirement, but leave the OF_GPIO
dependency. If not now, we will remove DT dependency for sure later.

>   config GPIO_IT8761E
>   	tristate "IT8761E GPIO support"
>   	depends on X86  # unconditional access to IO space.
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 98e23eb..8de041a 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -22,6 +22,7 @@ obj-$(CONFIG_GPIO_CS5535)	+= gpio-cs5535.o
>   obj-$(CONFIG_GPIO_DA9052)	+= gpio-da9052.o
>   obj-$(CONFIG_GPIO_DA9055)	+= gpio-da9055.o
>   obj-$(CONFIG_ARCH_DAVINCI)	+= gpio-davinci.o
> +obj-$(CONFIG_GPIO_DWAPB)	+= gpio-dwapb.o
>   obj-$(CONFIG_GPIO_EM)		+= gpio-em.o
>   obj-$(CONFIG_GPIO_EP93XX)	+= gpio-ep93xx.o
>   obj-$(CONFIG_GPIO_F7188X)	+= gpio-f7188x.o
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> new file mode 100644
> index 0000000..3a28697
> --- /dev/null
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -0,0 +1,172 @@
> +/*
> + * Copyright (c) 2011 Jamie Iles
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * All enquiries to support@picochip.com
> + */
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/module.h>
> +#include <linux/basic_mmio_gpio.h>

alphabetical ordering

> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +
> +struct dwapb_gpio;
> +
> +struct dwapb_gpio_bank {

Again, "bank" is "port".

> +	struct bgpio_chip	bgc;
> +	unsigned int		bank_idx;
> +	bool			is_registered;
> +	struct dwapb_gpio	*gpio;
> +};
> +
> +struct dwapb_gpio {
> +	struct device_node	*of_node;
> +	struct	device		*dev;

What about taking dev.of_node instead of the an extra of_node ptr?

> +	void __iomem		*regs;
> +	struct dwapb_gpio_bank	*banks;
> +	unsigned int		nr_banks;

ditto and for the following "banks", too.

> +};
> +
> +static unsigned int dwapb_gpio_nr_banks(struct device_node *of_node)
> +{
> +	unsigned int nr_banks = 0;
> +	struct device_node *np;
> +
> +	for_each_child_of_node(of_node, np)
> +		++nr_banks;

In general, I'd prefer a DT agnostic driver based on platform_data
and DT parsing instead of using DT spread over the driver. Anyway,
I don't want to stall this patches even further, so it can be cleaned
up later.

> +	return nr_banks;
> +}
> +
> +static int dwapb_gpio_add_bank(struct dwapb_gpio *gpio,
> +			       struct device_node *bank_np,
> +			       unsigned int offs)
> +{
> +	struct dwapb_gpio_bank *bank;
> +	u32 bank_idx, ngpio;
> +	int err;
> +
> +	if (of_property_read_u32(bank_np, "reg", &bank_idx)) {
> +		dev_err(gpio->dev, "invalid bank index for %s\n",
> +			bank_np->full_name);

index > 3 are also "invalid", of_property_read_u32 fails if it
is "missing".

> +		return -EINVAL;
> +	}
> +	bank = &gpio->banks[offs];
> +	bank->gpio = gpio;
> +
> +	if (of_property_read_u32(bank_np, "nr-gpio", &ngpio)) {
> +		dev_err(gpio->dev, "failed to get number of gpios for %s\n",
> +			bank_np->full_name);
> +		return -EINVAL;
> +	}
> +
> +	bank->bank_idx = bank_idx;
> +	err = bgpio_init(&bank->bgc, gpio->dev, 4,
> +			 gpio->regs + 0x50 + (bank_idx * 0x4),
> +			 gpio->regs + 0x00 + (bank_idx * 0xc),
> +			 NULL, gpio->regs + 0x04 + (bank_idx * 0xc), NULL,

#defines for the magic numbers above?

> +			 false);
> +	if (err) {
> +		dev_err(gpio->dev, "failed to init gpio chip for %s\n",
> +			bank_np->full_name);
> +		return err;
> +	}
> +
> +	bank->bgc.gc.ngpio = ngpio;
> +	bank->bgc.gc.of_node = bank_np;
> +
> +	err = gpiochip_add(&bank->bgc.gc);
> +	if (err)
> +		dev_err(gpio->dev, "failed to register gpiochip for %s\n",
> +			bank_np->full_name);
> +	else
> +		bank->is_registered = true;
> +
> +	return err;
> +}
> +
> +static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
> +{
> +	unsigned int m;
> +
> +	for (m = 0; m < gpio->nr_banks; ++m)
> +		if (gpio->banks[m].is_registered)
> +			WARN_ON(gpiochip_remove(&gpio->banks[m].bgc.gc));
> +	of_node_put(gpio->of_node);

I may be wrong here, but platform_device_unregister will finally remove
pdev and pdev->dev anyway. No need to put the node right before it.

> +}
> +
> +static int __devinit dwapb_gpio_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct dwapb_gpio *gpio;
> +	struct device_node *np;
> +	int err;
> +	unsigned int offs = 0;
> +
> +	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> +	if (!gpio)
> +		return -ENOMEM;
> +	gpio->dev = &pdev->dev;
> +
> +	gpio->nr_banks = dwapb_gpio_nr_banks(pdev->dev.of_node);
> +	if (!gpio->nr_banks)
> +		return -EINVAL;
> +	gpio->banks = devm_kzalloc(&pdev->dev, gpio->nr_banks *
> +				   sizeof(*gpio->banks), GFP_KERNEL);
> +	if (!gpio->banks)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "failed to get iomem\n");
> +		return -ENXIO;
> +	}

devm_ioremap below can deal with NULL resources. I suggest to remove
the extra check and error message above, as devm_ioremap and friends
will also print error messages.

> +	gpio->regs = devm_ioremap(&pdev->dev, res->start, resource_size(res));

devm_ioremap_resource?

> +	if (!gpio->regs)
> +		return -ENOMEM;
> +
> +	gpio->of_node = of_node_get(pdev->dev.of_node);

no need to of_node_get here, and of_node_put above.

> +	for_each_child_of_node(pdev->dev.of_node, np) {
> +		err = dwapb_gpio_add_bank(gpio, np, offs++);
> +		if (err)
> +			goto out_unregister;
> +	}
> +	platform_set_drvdata(pdev, gpio);
> +
> +	return 0;
> +
> +out_unregister:
> +	dwapb_gpio_unregister(gpio);
> +
> +	return err;
> +}
> +
> +static const struct of_device_id dwapb_of_match_table[] = {
> +	{ .compatible = "snps,dw-apb-gpio" },
> +	{ /* Sentinel */ }
> +};
> +
> +static struct platform_driver dwapb_gpio_driver = {
> +	.driver		= {
> +		.name	= "gpio-dwapb",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = dwapb_of_match_table,

use of_match_ptr(dwapb_of_match_table) now, we will need it later.

Sebastian

> +	},
> +	.probe		= dwapb_gpio_probe,
> +};
> +
> +static int __init dwapb_gpio_init(void)
> +{
> +	return platform_driver_register(&dwapb_gpio_driver);
> +}
> +postcore_initcall(dwapb_gpio_init);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Jamie Iles");
> +MODULE_DESCRIPTION("Synopsys DesignWare APB GPIO driver");
>

[PATCH 2/3] gpio: dwapb: add support for GPIO interrupts

[Alan Tull]

From: Jamie Iles <jamie@jamieiles.com>

The controller supports interrupts on bank A (up to 8 interrupt
sources).  Use the generic IRQ chip to implement interrupt support for
this bank.

Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Linus Walleij <linus.walleij@stericsson.com>
Acked-by: Rob Herring <rob.herring@calxeda.com>
Signed-off-by: Jamie Iles <jamie@jamieiles.com>
---
 drivers/gpio/Kconfig      |    2 +-
 drivers/gpio/gpio-dwapb.c |  181 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 182 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index c5e7868..4cc26ed 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -125,7 +125,7 @@ config GPIO_DWAPB
 	bool "Synopsys DesignWare APB GPIO driver"
 	select GPIO_GENERIC
 	select GENERIC_IRQ_CHIP
-	depends on OF_GPIO
+	depends on OF_GPIO && IRQ_DOMAIN
 	help
 	  Say Y or M here to build support for the Synopsys DesignWare APB
 	  GPIO block.  This requires device tree support.
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 3a28697..64c7183 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -10,12 +10,22 @@
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
 #include <linux/module.h>
 #include <linux/basic_mmio_gpio.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_irq.h>
 #include <linux/platform_device.h>
 
+#define INT_EN_REG_OFFS		0x30
+#define INT_MASK_REG_OFFS	0x34
+#define INT_TYPE_REG_OFFS	0x38
+#define INT_POLARITY_REG_OFFS	0x3c
+#define INT_STATUS_REG_OFFS	0x40
+#define EOI_REG_OFFS		0x4c
+
 struct dwapb_gpio;
 
 struct dwapb_gpio_bank {
@@ -31,6 +41,8 @@ struct dwapb_gpio {
 	void __iomem		*regs;
 	struct dwapb_gpio_bank	*banks;
 	unsigned int		nr_banks;
+	struct irq_chip_generic	*irq_gc;
+	unsigned long		toggle_edge;
 };
 
 static unsigned int dwapb_gpio_nr_banks(struct device_node *of_node)
@@ -44,6 +56,168 @@ static unsigned int dwapb_gpio_nr_banks(struct device_node *of_node)
 	return nr_banks;
 }
 
+static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+	struct dwapb_gpio_bank *bank = container_of(bgc, struct
+						    dwapb_gpio_bank, bgc);
+	struct dwapb_gpio *gpio = bank->gpio;
+
+	return irq_domain_to_irq(&gpio->irq_gc->domain, offset);
+}
+
+static void dwapb_toggle_trigger(struct dwapb_gpio *gpio, unsigned int offs)
+{
+	u32 v = readl(gpio->regs + INT_TYPE_REG_OFFS);
+
+	if (gpio_get_value(gpio->banks[0].bgc.gc.base + offs))
+		v &= ~BIT(offs);
+	else
+		v |= BIT(offs);
+
+	writel(v, gpio->regs + INT_TYPE_REG_OFFS);
+}
+
+static void dwapb_irq_handler(u32 irq, struct irq_desc *desc)
+{
+	struct dwapb_gpio *gpio = irq_get_handler_data(irq);
+	u32 irq_status = readl(gpio->regs + INT_STATUS_REG_OFFS);
+
+	while (irq_status) {
+		int irqoffset = fls(irq_status) - 1;
+		int irq = irq_domain_to_irq(&gpio->irq_gc->domain, irqoffset);
+
+		generic_handle_irq(irq);
+		irq_status &= ~(1 << irqoffset);
+
+		if (gpio->toggle_edge & BIT(irqoffset))
+			dwapb_toggle_trigger(gpio, irqoffset);
+	}
+}
+
+static void dwapb_irq_enable(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct dwapb_gpio *gpio = gc->private;
+
+	u32 val = readl(gpio->regs + INT_EN_REG_OFFS);
+	val |= 1 << d->hwirq;
+	writel(val, gpio->regs + INT_EN_REG_OFFS);
+}
+
+static void dwapb_irq_disable(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct dwapb_gpio *gpio = gc->private;
+
+	u32 val = readl(gpio->regs + INT_EN_REG_OFFS);
+	val &= ~(1 << d->hwirq);
+	writel(val, gpio->regs + INT_EN_REG_OFFS);
+}
+
+static int dwapb_irq_set_type(struct irq_data *d, u32 type)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct dwapb_gpio *gpio = gc->private;
+	int bit = d->hwirq;
+	unsigned long level, polarity;
+
+	if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
+		     IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
+		return -EINVAL;
+
+	level = readl(gpio->regs + INT_TYPE_REG_OFFS);
+	polarity = readl(gpio->regs + INT_POLARITY_REG_OFFS);
+
+	gpio->toggle_edge &= ~BIT(bit);
+	if (type & IRQ_TYPE_EDGE_BOTH) {
+		gpio->toggle_edge |= BIT(bit);
+		level |= (1 << bit);
+		dwapb_toggle_trigger(gpio, bit);
+	} else if (type & IRQ_TYPE_EDGE_RISING) {
+		level |= (1 << bit);
+		polarity |= (1 << bit);
+	} else if (type & IRQ_TYPE_EDGE_FALLING) {
+		level |= (1 << bit);
+		polarity &= ~(1 << bit);
+	} else if (type & IRQ_TYPE_LEVEL_HIGH) {
+		level &= ~(1 << bit);
+		polarity |= (1 << bit);
+	} else if (type & IRQ_TYPE_LEVEL_LOW) {
+		level &= ~(1 << bit);
+		polarity &= ~(1 << bit);
+	}
+
+	writel(level, gpio->regs + INT_TYPE_REG_OFFS);
+	writel(polarity, gpio->regs + INT_POLARITY_REG_OFFS);
+
+	return 0;
+}
+
+static int dwapb_create_irqchip(struct dwapb_gpio *gpio,
+				struct dwapb_gpio_bank *bank,
+				unsigned int irq_base)
+{
+	struct irq_chip_type *ct;
+
+	gpio->irq_gc = irq_alloc_generic_chip("gpio-dwapb", 1, irq_base,
+					      gpio->regs, handle_level_irq);
+	if (!gpio->irq_gc)
+		return -EIO;
+
+	gpio->irq_gc->domain.of_node = of_node_get(bank->bgc.gc.of_node);
+	gpio->irq_gc->private = gpio;
+	ct = gpio->irq_gc->chip_types;
+	ct->chip.irq_ack = irq_gc_ack_set_bit;
+	ct->chip.irq_mask = irq_gc_mask_set_bit;
+	ct->chip.irq_unmask = irq_gc_mask_clr_bit;
+	ct->chip.irq_set_type = dwapb_irq_set_type;
+	ct->chip.irq_enable = dwapb_irq_enable;
+	ct->chip.irq_disable = dwapb_irq_disable;
+	ct->regs.ack = EOI_REG_OFFS;
+	ct->regs.mask = INT_MASK_REG_OFFS;
+	irq_setup_generic_chip(gpio->irq_gc, IRQ_MSK(bank->bgc.gc.ngpio),
+			       IRQ_GC_INIT_NESTED_LOCK, IRQ_NOREQUEST, 0);
+
+	return 0;
+}
+
+static int dwapb_configure_irqs(struct dwapb_gpio *gpio,
+				struct dwapb_gpio_bank *bank)
+{
+	unsigned int m, irq, ngpio = bank->bgc.gc.ngpio;
+	int irq_base;
+
+	for (m = 0; m < ngpio; ++m) {
+		irq = irq_of_parse_and_map(bank->bgc.gc.of_node, m);
+		if (!irq && m == 0) {
+			dev_warn(gpio->dev, "no irq for bank %s\n",
+				 bank->bgc.gc.of_node->full_name);
+			return -ENXIO;
+		} else if (!irq) {
+			break;
+		}
+
+		irq_set_chained_handler(irq, dwapb_irq_handler);
+		irq_set_handler_data(irq, gpio);
+	}
+	bank->bgc.gc.to_irq = dwapb_gpio_to_irq;
+
+	irq_base = irq_alloc_descs(-1, 0, ngpio, NUMA_NO_NODE);
+	if (irq_base < 0)
+		return irq_base;
+
+	if (dwapb_create_irqchip(gpio, bank, irq_base))
+		goto out_free_descs;
+
+	return 0;
+
+out_free_descs:
+	irq_free_descs(irq_base, ngpio);
+
+	return -EIO;
+}
+
 static int dwapb_gpio_add_bank(struct dwapb_gpio *gpio,
 			       struct device_node *bank_np,
 			       unsigned int offs)
@@ -81,6 +255,13 @@ static int dwapb_gpio_add_bank(struct dwapb_gpio *gpio,
 	bank->bgc.gc.ngpio = ngpio;
 	bank->bgc.gc.of_node = bank_np;
 
+	/*
+	 * Only bank A can provide interrupts in all configurations of the IP.
+	 */
+	if (bank_idx == 0 &&
+	    of_get_property(bank_np, "interrupt-controller", NULL))
+		dwapb_configure_irqs(gpio, bank);
+
 	err = gpiochip_add(&bank->bgc.gc);
 	if (err)
 		dev_err(gpio->dev, "failed to register gpiochip for %s\n",
-- 
1.7.9.5

Re: [PATCH 2/3] gpio: dwapb: add support for GPIO interrupts

[Sebastian Hesselbarth]

On 11/05/2013 05:55 PM, Alan Tull wrote:
> From: Jamie Iles <jamie@jamieiles.com>
>
> The controller supports interrupts on bank A (up to 8 interrupt

s/bank/port/g

> sources).  Use the generic IRQ chip to implement interrupt support for
> this bank.
>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Linus Walleij <linus.walleij@stericsson.com>
> Acked-by: Rob Herring <rob.herring@calxeda.com>
> Signed-off-by: Jamie Iles <jamie@jamieiles.com>
> ---
>   drivers/gpio/Kconfig      |    2 +-
>   drivers/gpio/gpio-dwapb.c |  181 +++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 182 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index c5e7868..4cc26ed 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -125,7 +125,7 @@ config GPIO_DWAPB
>   	bool "Synopsys DesignWare APB GPIO driver"
>   	select GPIO_GENERIC
>   	select GENERIC_IRQ_CHIP
> -	depends on OF_GPIO
> +	depends on OF_GPIO && IRQ_DOMAIN
>   	help
>   	  Say Y or M here to build support for the Synopsys DesignWare APB
>   	  GPIO block.  This requires device tree support.
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 3a28697..64c7183 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -10,12 +10,22 @@
>   #include <linux/init.h>
>   #include <linux/io.h>
>   #include <linux/ioport.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
>   #include <linux/module.h>
>   #include <linux/basic_mmio_gpio.h>
>   #include <linux/of.h>
>   #include <linux/of_address.h>
> +#include <linux/of_irq.h>
>   #include <linux/platform_device.h>
>
> +#define INT_EN_REG_OFFS		0x30
> +#define INT_MASK_REG_OFFS	0x34
> +#define INT_TYPE_REG_OFFS	0x38
> +#define INT_POLARITY_REG_OFFS	0x3c
> +#define INT_STATUS_REG_OFFS	0x40
> +#define EOI_REG_OFFS		0x4c

For sane register names, check [1] pp. 1271. I also suggest
to get rid of _REG_OFFS.

[1] http://www.altera.com/literature/hb/arria-v/hps.pdf

>   struct dwapb_gpio;
>
>   struct dwapb_gpio_bank {
> @@ -31,6 +41,8 @@ struct dwapb_gpio {
>   	void __iomem		*regs;
>   	struct dwapb_gpio_bank	*banks;
>   	unsigned int		nr_banks;
> +	struct irq_chip_generic	*irq_gc;
> +	unsigned long		toggle_edge;
>   };
>
>   static unsigned int dwapb_gpio_nr_banks(struct device_node *of_node)
> @@ -44,6 +56,168 @@ static unsigned int dwapb_gpio_nr_banks(struct device_node *of_node)
>   	return nr_banks;
>   }
>
> +static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
> +{
> +	struct bgpio_chip *bgc = to_bgpio_chip(gc);
> +	struct dwapb_gpio_bank *bank = container_of(bgc, struct
> +						    dwapb_gpio_bank, bgc);
> +	struct dwapb_gpio *gpio = bank->gpio;
> +
> +	return irq_domain_to_irq(&gpio->irq_gc->domain, offset);
> +}
> +
> +static void dwapb_toggle_trigger(struct dwapb_gpio *gpio, unsigned int offs)
> +{
> +	u32 v = readl(gpio->regs + INT_TYPE_REG_OFFS);

s/readl/readl_relaxed/g
s/writel/writel_relaxed/g ?

> +	if (gpio_get_value(gpio->banks[0].bgc.gc.base + offs))
> +		v &= ~BIT(offs);
> +	else
> +		v |= BIT(offs);
> +
> +	writel(v, gpio->regs + INT_TYPE_REG_OFFS);
> +}
> +
> +static void dwapb_irq_handler(u32 irq, struct irq_desc *desc)
> +{
> +	struct dwapb_gpio *gpio = irq_get_handler_data(irq);
> +	u32 irq_status = readl(gpio->regs + INT_STATUS_REG_OFFS);
> +
> +	while (irq_status) {
> +		int irqoffset = fls(irq_status) - 1;
> +		int irq = irq_domain_to_irq(&gpio->irq_gc->domain, irqoffset);
> +
> +		generic_handle_irq(irq);
> +		irq_status &= ~(1 << irqoffset);
> +
> +		if (gpio->toggle_edge & BIT(irqoffset))
> +			dwapb_toggle_trigger(gpio, irqoffset);

if ((irq_get_trigger_type(irq) & IRQ_TYPE_SENSE_MASK) == IRQ_TYPE_EDGE_BOTH)

and get rid of ->toggle_edge cache variable.

> +	}
> +}
> +
> +static void dwapb_irq_enable(struct irq_data *d)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct dwapb_gpio *gpio = gc->private;
> +
> +	u32 val = readl(gpio->regs + INT_EN_REG_OFFS);
> +	val |= 1 << d->hwirq;
> +	writel(val, gpio->regs + INT_EN_REG_OFFS);
> +}
> +
> +static void dwapb_irq_disable(struct irq_data *d)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct dwapb_gpio *gpio = gc->private;
> +
> +	u32 val = readl(gpio->regs + INT_EN_REG_OFFS);
> +	val &= ~(1 << d->hwirq);
> +	writel(val, gpio->regs + INT_EN_REG_OFFS);
> +}
> +
> +static int dwapb_irq_set_type(struct irq_data *d, u32 type)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct dwapb_gpio *gpio = gc->private;
> +	int bit = d->hwirq;
> +	unsigned long level, polarity;
> +
> +	if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
> +		     IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
> +		return -EINVAL;
> +
> +	level = readl(gpio->regs + INT_TYPE_REG_OFFS);
> +	polarity = readl(gpio->regs + INT_POLARITY_REG_OFFS);
> +
> +	gpio->toggle_edge &= ~BIT(bit);
> +	if (type & IRQ_TYPE_EDGE_BOTH) {
> +		gpio->toggle_edge |= BIT(bit);
> +		level |= (1 << bit);

If you remove ->toggle_edge, read current pin value here and
set next edge accordingly.

> +		dwapb_toggle_trigger(gpio, bit);
> +	} else if (type & IRQ_TYPE_EDGE_RISING) {
> +		level |= (1 << bit);
> +		polarity |= (1 << bit);
> +	} else if (type & IRQ_TYPE_EDGE_FALLING) {
> +		level |= (1 << bit);
> +		polarity &= ~(1 << bit);
> +	} else if (type & IRQ_TYPE_LEVEL_HIGH) {
> +		level &= ~(1 << bit);
> +		polarity |= (1 << bit);
> +	} else if (type & IRQ_TYPE_LEVEL_LOW) {
> +		level &= ~(1 << bit);
> +		polarity &= ~(1 << bit);
> +	}

switch (type) for the above if-else-if ?

> +	writel(level, gpio->regs + INT_TYPE_REG_OFFS);
> +	writel(polarity, gpio->regs + INT_POLARITY_REG_OFFS);
> +
> +	return 0;
> +}
> +
> +static int dwapb_create_irqchip(struct dwapb_gpio *gpio,
> +				struct dwapb_gpio_bank *bank,
> +				unsigned int irq_base)
> +{
> +	struct irq_chip_type *ct;
> +
> +	gpio->irq_gc = irq_alloc_generic_chip("gpio-dwapb", 1, irq_base,
> +					      gpio->regs, handle_level_irq);
> +	if (!gpio->irq_gc)
> +		return -EIO;
> +
> +	gpio->irq_gc->domain.of_node = of_node_get(bank->bgc.gc.of_node);
> +	gpio->irq_gc->private = gpio;
> +	ct = gpio->irq_gc->chip_types;
> +	ct->chip.irq_ack = irq_gc_ack_set_bit;
> +	ct->chip.irq_mask = irq_gc_mask_set_bit;
> +	ct->chip.irq_unmask = irq_gc_mask_clr_bit;
> +	ct->chip.irq_set_type = dwapb_irq_set_type;
> +	ct->chip.irq_enable = dwapb_irq_enable;
> +	ct->chip.irq_disable = dwapb_irq_disable;

I can see no functional difference between enable/mask and always
enable and mask initially. If there are no objections, remove
enable/disable callbacks and mask and enable in _probe().

> +	ct->regs.ack = EOI_REG_OFFS;
> +	ct->regs.mask = INT_MASK_REG_OFFS;
> +	irq_setup_generic_chip(gpio->irq_gc, IRQ_MSK(bank->bgc.gc.ngpio),
> +			       IRQ_GC_INIT_NESTED_LOCK, IRQ_NOREQUEST, 0);
> +
> +	return 0;
> +}
> +
> +static int dwapb_configure_irqs(struct dwapb_gpio *gpio,
> +				struct dwapb_gpio_bank *bank)
> +{
> +	unsigned int m, irq, ngpio = bank->bgc.gc.ngpio;
> +	int irq_base;
> +
> +	for (m = 0; m < ngpio; ++m) {
> +		irq = irq_of_parse_and_map(bank->bgc.gc.of_node, m);

Only port A can raise interrupts. Are you sure, that all IP has
one irq per gpio? As you always use the same handler, just map
all irqs passed and install the handler; no need to match against
individual gpios here.

Sebastian

> +		if (!irq && m == 0) {
> +			dev_warn(gpio->dev, "no irq for bank %s\n",
> +				 bank->bgc.gc.of_node->full_name);
> +			return -ENXIO;
> +		} else if (!irq) {
> +			break;
> +		}
> +
> +		irq_set_chained_handler(irq, dwapb_irq_handler);
> +		irq_set_handler_data(irq, gpio);
> +	}
> +	bank->bgc.gc.to_irq = dwapb_gpio_to_irq;
> +
> +	irq_base = irq_alloc_descs(-1, 0, ngpio, NUMA_NO_NODE);
> +	if (irq_base < 0)
> +		return irq_base;

I was going to mention irqdomain_linear here, but then I realized
that patch 3/3 reverts most of irq handling here and replaces it
with irqdomain. Alan, I think Jamie is fine with you mangling patches
2 and 3 into one.

> +	if (dwapb_create_irqchip(gpio, bank, irq_base))
> +		goto out_free_descs;
> +
> +	return 0;
> +
> +out_free_descs:
> +	irq_free_descs(irq_base, ngpio);
> +
> +	return -EIO;
> +}
> +
>   static int dwapb_gpio_add_bank(struct dwapb_gpio *gpio,
>   			       struct device_node *bank_np,
>   			       unsigned int offs)
> @@ -81,6 +255,13 @@ static int dwapb_gpio_add_bank(struct dwapb_gpio *gpio,
>   	bank->bgc.gc.ngpio = ngpio;
>   	bank->bgc.gc.of_node = bank_np;
>
> +	/*
> +	 * Only bank A can provide interrupts in all configurations of the IP.
> +	 */
> +	if (bank_idx == 0 &&
> +	    of_get_property(bank_np, "interrupt-controller", NULL))
> +		dwapb_configure_irqs(gpio, bank);
> +
>   	err = gpiochip_add(&bank->bgc.gc);
>   	if (err)
>   		dev_err(gpio->dev, "failed to register gpiochip for %s\n",
>

[PATCH 3/3] use linear irq domain in gpio-dwapb

[Alan Tull]

From: Alan Tull <atull@altera.com>

 * Changes to get gpio-dwapb (originally v3.2-rc7) driver building and
   working on current kernel.
 * Use linear irq domain.
 * Fix setting irq edge type for 'rising' and 'both'.
 * Support as a loadable module.
 * Use bgpio_chip's spinlock during register access.

Signed-off-by: Alan Tull <atull@altera.com>
---
 drivers/gpio/Kconfig      |    2 +-
 drivers/gpio/gpio-dwapb.c |  235 ++++++++++++++++++++++++++++++---------------
 2 files changed, 160 insertions(+), 77 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 4cc26ed..5fc674a 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -122,7 +122,7 @@ config GPIO_GENERIC_PLATFORM
 	  Say yes here to support basic platform_device memory-mapped GPIO controllers.
 
 config GPIO_DWAPB
-	bool "Synopsys DesignWare APB GPIO driver"
+	tristate "Synopsys DesignWare APB GPIO driver"
 	select GPIO_GENERIC
 	select GENERIC_IRQ_CHIP
 	depends on OF_GPIO && IRQ_DOMAIN
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 64c7183..cca97e3 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -18,6 +18,7 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/platform_device.h>
+#include <linux/spinlock.h>
 
 #define INT_EN_REG_OFFS		0x30
 #define INT_MASK_REG_OFFS	0x34
@@ -30,7 +31,6 @@ struct dwapb_gpio;
 
 struct dwapb_gpio_bank {
 	struct bgpio_chip	bgc;
-	unsigned int		bank_idx;
 	bool			is_registered;
 	struct dwapb_gpio	*gpio;
 };
@@ -41,8 +41,9 @@ struct dwapb_gpio {
 	void __iomem		*regs;
 	struct dwapb_gpio_bank	*banks;
 	unsigned int		nr_banks;
-	struct irq_chip_generic	*irq_gc;
 	unsigned long		toggle_edge;
+	struct irq_domain	*domain;
+	int			hwirq;
 };
 
 static unsigned int dwapb_gpio_nr_banks(struct device_node *of_node)
@@ -63,29 +64,30 @@ static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
 						    dwapb_gpio_bank, bgc);
 	struct dwapb_gpio *gpio = bank->gpio;
 
-	return irq_domain_to_irq(&gpio->irq_gc->domain, offset);
+	return irq_create_mapping(gpio->domain, offset);
 }
 
 static void dwapb_toggle_trigger(struct dwapb_gpio *gpio, unsigned int offs)
 {
-	u32 v = readl(gpio->regs + INT_TYPE_REG_OFFS);
+	u32 v = readl(gpio->regs + INT_POLARITY_REG_OFFS);
 
 	if (gpio_get_value(gpio->banks[0].bgc.gc.base + offs))
 		v &= ~BIT(offs);
 	else
 		v |= BIT(offs);
 
-	writel(v, gpio->regs + INT_TYPE_REG_OFFS);
+	writel(v, gpio->regs + INT_POLARITY_REG_OFFS);
 }
 
 static void dwapb_irq_handler(u32 irq, struct irq_desc *desc)
 {
 	struct dwapb_gpio *gpio = irq_get_handler_data(irq);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
 	u32 irq_status = readl(gpio->regs + INT_STATUS_REG_OFFS);
 
 	while (irq_status) {
 		int irqoffset = fls(irq_status) - 1;
-		int irq = irq_domain_to_irq(&gpio->irq_gc->domain, irqoffset);
+		int irq = irq_linear_revmap(gpio->domain, irqoffset);
 
 		generic_handle_irq(irq);
 		irq_status &= ~(1 << irqoffset);
@@ -93,44 +95,56 @@ static void dwapb_irq_handler(u32 irq, struct irq_desc *desc)
 		if (gpio->toggle_edge & BIT(irqoffset))
 			dwapb_toggle_trigger(gpio, irqoffset);
 	}
+
+	if (chip->irq_eoi)
+		chip->irq_eoi(irq_desc_get_irq_data(desc));
 }
 
 static void dwapb_irq_enable(struct irq_data *d)
 {
-	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	struct dwapb_gpio *gpio = gc->private;
+	struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d);
+	struct bgpio_chip *bgc = &gpio->banks[0].bgc;
+	unsigned long flags;
+	u32 val;
 
-	u32 val = readl(gpio->regs + INT_EN_REG_OFFS);
+	spin_lock_irqsave(&bgc->lock, flags);
+	val = readl(gpio->regs + INT_EN_REG_OFFS);
 	val |= 1 << d->hwirq;
 	writel(val, gpio->regs + INT_EN_REG_OFFS);
+	spin_unlock_irqrestore(&bgc->lock, flags);
 }
 
 static void dwapb_irq_disable(struct irq_data *d)
 {
-	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	struct dwapb_gpio *gpio = gc->private;
+	struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d);
+	struct bgpio_chip *bgc = &gpio->banks[0].bgc;
+	unsigned long flags;
+	u32 val;
 
-	u32 val = readl(gpio->regs + INT_EN_REG_OFFS);
+	spin_lock_irqsave(&bgc->lock, flags);
+	val = readl(gpio->regs + INT_EN_REG_OFFS);
 	val &= ~(1 << d->hwirq);
 	writel(val, gpio->regs + INT_EN_REG_OFFS);
+	spin_unlock_irqrestore(&bgc->lock, flags);
 }
 
 static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 {
-	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	struct dwapb_gpio *gpio = gc->private;
+	struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d);
+	struct bgpio_chip *bgc = &gpio->banks[0].bgc;
 	int bit = d->hwirq;
-	unsigned long level, polarity;
+	unsigned long level, polarity, flags;
 
 	if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
 		     IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
 		return -EINVAL;
 
+	spin_lock_irqsave(&bgc->lock, flags);
 	level = readl(gpio->regs + INT_TYPE_REG_OFFS);
 	polarity = readl(gpio->regs + INT_POLARITY_REG_OFFS);
 
 	gpio->toggle_edge &= ~BIT(bit);
-	if (type & IRQ_TYPE_EDGE_BOTH) {
+	if (type == IRQ_TYPE_EDGE_BOTH) {
 		gpio->toggle_edge |= BIT(bit);
 		level |= (1 << bit);
 		dwapb_toggle_trigger(gpio, bit);
@@ -150,72 +164,102 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 
 	writel(level, gpio->regs + INT_TYPE_REG_OFFS);
 	writel(polarity, gpio->regs + INT_POLARITY_REG_OFFS);
+	spin_unlock_irqrestore(&bgc->lock, flags);
 
 	return 0;
 }
 
-static int dwapb_create_irqchip(struct dwapb_gpio *gpio,
-				struct dwapb_gpio_bank *bank,
-				unsigned int irq_base)
+static void dwapb_irq_ack(struct irq_data *d)
 {
-	struct irq_chip_type *ct;
-
-	gpio->irq_gc = irq_alloc_generic_chip("gpio-dwapb", 1, irq_base,
-					      gpio->regs, handle_level_irq);
-	if (!gpio->irq_gc)
-		return -EIO;
-
-	gpio->irq_gc->domain.of_node = of_node_get(bank->bgc.gc.of_node);
-	gpio->irq_gc->private = gpio;
-	ct = gpio->irq_gc->chip_types;
-	ct->chip.irq_ack = irq_gc_ack_set_bit;
-	ct->chip.irq_mask = irq_gc_mask_set_bit;
-	ct->chip.irq_unmask = irq_gc_mask_clr_bit;
-	ct->chip.irq_set_type = dwapb_irq_set_type;
-	ct->chip.irq_enable = dwapb_irq_enable;
-	ct->chip.irq_disable = dwapb_irq_disable;
-	ct->regs.ack = EOI_REG_OFFS;
-	ct->regs.mask = INT_MASK_REG_OFFS;
-	irq_setup_generic_chip(gpio->irq_gc, IRQ_MSK(bank->bgc.gc.ngpio),
-			       IRQ_GC_INIT_NESTED_LOCK, IRQ_NOREQUEST, 0);
+	struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d);
+	struct bgpio_chip *bgc = &gpio->banks[0].bgc;
+	unsigned long flags;
 
-	return 0;
+	spin_lock_irqsave(&bgc->lock, flags);
+	writel(1 << d->hwirq, gpio->regs + EOI_REG_OFFS);
+	spin_unlock_irqrestore(&bgc->lock, flags);
 }
 
-static int dwapb_configure_irqs(struct dwapb_gpio *gpio,
-				struct dwapb_gpio_bank *bank)
+static void dwapb_irq_mask(struct irq_data *d)
 {
-	unsigned int m, irq, ngpio = bank->bgc.gc.ngpio;
-	int irq_base;
-
-	for (m = 0; m < ngpio; ++m) {
-		irq = irq_of_parse_and_map(bank->bgc.gc.of_node, m);
-		if (!irq && m == 0) {
-			dev_warn(gpio->dev, "no irq for bank %s\n",
-				 bank->bgc.gc.of_node->full_name);
-			return -ENXIO;
-		} else if (!irq) {
-			break;
-		}
-
-		irq_set_chained_handler(irq, dwapb_irq_handler);
-		irq_set_handler_data(irq, gpio);
-	}
-	bank->bgc.gc.to_irq = dwapb_gpio_to_irq;
+	struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d);
+	struct bgpio_chip *bgc = &gpio->banks[0].bgc;
+	unsigned long value, flags;
+
+	spin_lock_irqsave(&bgc->lock, flags);
+	value = readl(gpio->regs + INT_MASK_REG_OFFS);
+	value |= 1 << d->hwirq;
+	writel(value, gpio->regs + INT_MASK_REG_OFFS);
+	spin_unlock_irqrestore(&bgc->lock, flags);
+}
 
-	irq_base = irq_alloc_descs(-1, 0, ngpio, NUMA_NO_NODE);
-	if (irq_base < 0)
-		return irq_base;
+static void dwapb_irq_unmask(struct irq_data *d)
+{
+	struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d);
+	struct bgpio_chip *bgc = &gpio->banks[0].bgc;
+	unsigned long value, flags;
+
+	spin_lock_irqsave(&bgc->lock, flags);
+	value = readl(gpio->regs + INT_MASK_REG_OFFS);
+	value &= ~(1 << d->hwirq);
+	writel(value, gpio->regs + INT_MASK_REG_OFFS);
+	spin_unlock_irqrestore(&bgc->lock, flags);
+}
 
-	if (dwapb_create_irqchip(gpio, bank, irq_base))
-		goto out_free_descs;
+static struct irq_chip dwapb_irq_chip = {
+	.name = "gpio-dwapb",
+	.irq_ack = dwapb_irq_ack,
+	.irq_mask = dwapb_irq_mask,
+	.irq_unmask = dwapb_irq_unmask,
+	.irq_set_type = dwapb_irq_set_type,
+	.irq_enable = dwapb_irq_enable,
+	.irq_disable = dwapb_irq_disable,
+};
+
+static int dwapb_gpio_irq_map(struct irq_domain *domain,
+			      unsigned int irq,
+			      irq_hw_number_t hwirq)
+{
+	struct dwapb_gpio *gpio = domain->host_data;
+
+	irq_set_chip_data(irq, gpio);
+	irq_set_chip_and_handler(irq, &dwapb_irq_chip, handle_level_irq);
+	irq_set_irq_type(irq, IRQ_TYPE_NONE);
+	irq_set_handler_data(irq, gpio);
 
 	return 0;
+}
 
-out_free_descs:
-	irq_free_descs(irq_base, ngpio);
+static struct irq_domain_ops dwapb_gpio_irq_ops = {
+	.map = dwapb_gpio_irq_map,
+	.xlate = irq_domain_xlate_twocell,
+};
 
-	return -EIO;
+static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
+				 struct dwapb_gpio_bank *bank)
+{
+	struct gpio_chip *gc = &bank->bgc.gc;
+	struct device_node *node =  gc->of_node;
+	unsigned int ngpio = gc->ngpio;
+	int reg;
+
+	if (of_get_property(node, "interrupts", &reg) == NULL)
+		return;
+
+	gpio->hwirq = irq_of_parse_and_map(node, 0);
+	if (gpio->hwirq == NO_IRQ)
+		return;
+
+	gpio->domain = irq_domain_add_linear(node, ngpio, &dwapb_gpio_irq_ops,
+					     gpio);
+	if (!gpio->domain) {
+		irq_dispose_mapping(gpio->hwirq);
+		return;
+	}
+
+	irq_set_handler_data(gpio->hwirq, gpio);
+	irq_set_chained_handler(gpio->hwirq, dwapb_irq_handler);
+	bank->bgc.gc.to_irq = dwapb_gpio_to_irq;
 }
 
 static int dwapb_gpio_add_bank(struct dwapb_gpio *gpio,
@@ -240,7 +284,6 @@ static int dwapb_gpio_add_bank(struct dwapb_gpio *gpio,
 		return -EINVAL;
 	}
 
-	bank->bank_idx = bank_idx;
 	err = bgpio_init(&bank->bgc, gpio->dev, 4,
 			 gpio->regs + 0x50 + (bank_idx * 0x4),
 			 gpio->regs + 0x00 + (bank_idx * 0xc),
@@ -282,7 +325,7 @@ static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
 	of_node_put(gpio->of_node);
 }
 
-static int __devinit dwapb_gpio_probe(struct platform_device *pdev)
+static int dwapb_gpio_probe(struct platform_device *pdev)
 {
 	struct resource *res;
 	struct dwapb_gpio *gpio;
@@ -296,21 +339,29 @@ static int __devinit dwapb_gpio_probe(struct platform_device *pdev)
 	gpio->dev = &pdev->dev;
 
 	gpio->nr_banks = dwapb_gpio_nr_banks(pdev->dev.of_node);
-	if (!gpio->nr_banks)
-		return -EINVAL;
+	if (!gpio->nr_banks) {
+		err = -EINVAL;
+		goto out_err;
+	}
 	gpio->banks = devm_kzalloc(&pdev->dev, gpio->nr_banks *
 				   sizeof(*gpio->banks), GFP_KERNEL);
-	if (!gpio->banks)
-		return -ENOMEM;
+	if (!gpio->banks) {
+		err = -ENOMEM;
+		goto out_err;
+	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		dev_err(&pdev->dev, "failed to get iomem\n");
-		return -ENXIO;
+		err = -ENXIO;
+		goto out_free_banks;
 	}
+
 	gpio->regs = devm_ioremap(&pdev->dev, res->start, resource_size(res));
-	if (!gpio->regs)
-		return -ENOMEM;
+	if (!gpio->regs) {
+		err = -ENOMEM;
+		goto out_free_banks;
+	}
 
 	gpio->of_node = of_node_get(pdev->dev.of_node);
 	for_each_child_of_node(pdev->dev.of_node, np) {
@@ -325,9 +376,34 @@ static int __devinit dwapb_gpio_probe(struct platform_device *pdev)
 out_unregister:
 	dwapb_gpio_unregister(gpio);
 
+	if (gpio->domain) {
+		irq_dispose_mapping(gpio->hwirq);
+		irq_domain_remove(gpio->domain);
+	}
+
+out_free_banks:
+	devm_kfree(&pdev->dev, gpio->banks);
+
+out_err:
+	devm_kfree(&pdev->dev, gpio);
 	return err;
 }
 
+static int dwapb_gpio_remove(struct platform_device *pdev)
+{
+	struct dwapb_gpio *gpio = platform_get_drvdata(pdev);
+
+	dwapb_gpio_unregister(gpio);
+	if (gpio->domain) {
+		irq_dispose_mapping(gpio->hwirq);
+		irq_domain_remove(gpio->domain);
+	}
+	devm_kfree(&pdev->dev, gpio->banks);
+	devm_kfree(&pdev->dev, gpio);
+
+	return 0;
+}
+
 static const struct of_device_id dwapb_of_match_table[] = {
 	{ .compatible = "snps,dw-apb-gpio" },
 	{ /* Sentinel */ }
@@ -340,6 +416,7 @@ static struct platform_driver dwapb_gpio_driver = {
 		.of_match_table = dwapb_of_match_table,
 	},
 	.probe		= dwapb_gpio_probe,
+	.remove		= dwapb_gpio_remove,
 };
 
 static int __init dwapb_gpio_init(void)
@@ -348,6 +425,12 @@ static int __init dwapb_gpio_init(void)
 }
 postcore_initcall(dwapb_gpio_init);
 
+static void __exit dwapb_gpio_exit(void)
+{
+	platform_driver_unregister(&dwapb_gpio_driver);
+}
+module_exit(dwapb_gpio_exit);
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Jamie Iles");
 MODULE_DESCRIPTION("Synopsys DesignWare APB GPIO driver");
-- 
1.7.9.5

Re: [PATCH 3/3] use linear irq domain in gpio-dwapb

[Sebastian Hesselbarth]

On 11/05/2013 05:55 PM, Alan Tull wrote:
> From: Alan Tull <atull@altera.com>
>
>   * Changes to get gpio-dwapb (originally v3.2-rc7) driver building and
>     working on current kernel.
>   * Use linear irq domain.
>   * Fix setting irq edge type for 'rising' and 'both'.
>   * Support as a loadable module.
>   * Use bgpio_chip's spinlock during register access.

As this patch is basically messing what I have commented before,
I really lost interest in reviewing this again. Please just squash
them into one.

BTW, I do have a driver for dw-apb-gpio that almost looks feature
equivalent to this one. I am not ready for testing on Berlin SoCs,
so I haven't posted it. After the review, I somehow think I should
post it, especially because I see no point in port child nodes.

Anyway, some remarks below.

> Signed-off-by: Alan Tull <atull@altera.com>
> ---
>   drivers/gpio/Kconfig      |    2 +-
>   drivers/gpio/gpio-dwapb.c |  235 ++++++++++++++++++++++++++++++---------------
>   2 files changed, 160 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 4cc26ed..5fc674a 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -122,7 +122,7 @@ config GPIO_GENERIC_PLATFORM
>   	  Say yes here to support basic platform_device memory-mapped GPIO controllers.
>
>   config GPIO_DWAPB
> -	bool "Synopsys DesignWare APB GPIO driver"
> +	tristate "Synopsys DesignWare APB GPIO driver"
>   	select GPIO_GENERIC
>   	select GENERIC_IRQ_CHIP
>   	depends on OF_GPIO && IRQ_DOMAIN
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 64c7183..cca97e3 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -18,6 +18,7 @@
>   #include <linux/of_address.h>
>   #include <linux/of_irq.h>
>   #include <linux/platform_device.h>
> +#include <linux/spinlock.h>
>
>   #define INT_EN_REG_OFFS		0x30
>   #define INT_MASK_REG_OFFS	0x34
> @@ -30,7 +31,6 @@ struct dwapb_gpio;
>
>   struct dwapb_gpio_bank {
>   	struct bgpio_chip	bgc;
> -	unsigned int		bank_idx;
>   	bool			is_registered;
>   	struct dwapb_gpio	*gpio;
>   };
> @@ -41,8 +41,9 @@ struct dwapb_gpio {
>   	void __iomem		*regs;
>   	struct dwapb_gpio_bank	*banks;
>   	unsigned int		nr_banks;
> -	struct irq_chip_generic	*irq_gc;
>   	unsigned long		toggle_edge;
> +	struct irq_domain	*domain;
> +	int			hwirq;
>   };
>
>   static unsigned int dwapb_gpio_nr_banks(struct device_node *of_node)
> @@ -63,29 +64,30 @@ static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
>   						    dwapb_gpio_bank, bgc);
>   	struct dwapb_gpio *gpio = bank->gpio;
>
> -	return irq_domain_to_irq(&gpio->irq_gc->domain, offset);
> +	return irq_create_mapping(gpio->domain, offset);
>   }
>
>   static void dwapb_toggle_trigger(struct dwapb_gpio *gpio, unsigned int offs)
>   {
> -	u32 v = readl(gpio->regs + INT_TYPE_REG_OFFS);
> +	u32 v = readl(gpio->regs + INT_POLARITY_REG_OFFS);
>
>   	if (gpio_get_value(gpio->banks[0].bgc.gc.base + offs))
>   		v &= ~BIT(offs);
>   	else
>   		v |= BIT(offs);
>
> -	writel(v, gpio->regs + INT_TYPE_REG_OFFS);
> +	writel(v, gpio->regs + INT_POLARITY_REG_OFFS);
>   }
>
>   static void dwapb_irq_handler(u32 irq, struct irq_desc *desc)
>   {
>   	struct dwapb_gpio *gpio = irq_get_handler_data(irq);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
>   	u32 irq_status = readl(gpio->regs + INT_STATUS_REG_OFFS);
>
>   	while (irq_status) {
>   		int irqoffset = fls(irq_status) - 1;
> -		int irq = irq_domain_to_irq(&gpio->irq_gc->domain, irqoffset);
> +		int irq = irq_linear_revmap(gpio->domain, irqoffset);
>
>   		generic_handle_irq(irq);
>   		irq_status &= ~(1 << irqoffset);
> @@ -93,44 +95,56 @@ static void dwapb_irq_handler(u32 irq, struct irq_desc *desc)
>   		if (gpio->toggle_edge & BIT(irqoffset))
>   			dwapb_toggle_trigger(gpio, irqoffset);
>   	}
> +
> +	if (chip->irq_eoi)
> +		chip->irq_eoi(irq_desc_get_irq_data(desc));
>   }
>
>   static void dwapb_irq_enable(struct irq_data *d)
>   {
> -	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> -	struct dwapb_gpio *gpio = gc->private;
> +	struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d);
> +	struct bgpio_chip *bgc = &gpio->banks[0].bgc;
> +	unsigned long flags;
> +	u32 val;
>
> -	u32 val = readl(gpio->regs + INT_EN_REG_OFFS);
> +	spin_lock_irqsave(&bgc->lock, flags);
> +	val = readl(gpio->regs + INT_EN_REG_OFFS);
>   	val |= 1 << d->hwirq;
>   	writel(val, gpio->regs + INT_EN_REG_OFFS);
> +	spin_unlock_irqrestore(&bgc->lock, flags);
>   }
>
>   static void dwapb_irq_disable(struct irq_data *d)
>   {
> -	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> -	struct dwapb_gpio *gpio = gc->private;
> +	struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d);
> +	struct bgpio_chip *bgc = &gpio->banks[0].bgc;
> +	unsigned long flags;
> +	u32 val;
>
> -	u32 val = readl(gpio->regs + INT_EN_REG_OFFS);
> +	spin_lock_irqsave(&bgc->lock, flags);
> +	val = readl(gpio->regs + INT_EN_REG_OFFS);
>   	val &= ~(1 << d->hwirq);
>   	writel(val, gpio->regs + INT_EN_REG_OFFS);
> +	spin_unlock_irqrestore(&bgc->lock, flags);
>   }
>
>   static int dwapb_irq_set_type(struct irq_data *d, u32 type)
>   {
> -	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> -	struct dwapb_gpio *gpio = gc->private;
> +	struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d);
> +	struct bgpio_chip *bgc = &gpio->banks[0].bgc;
>   	int bit = d->hwirq;
> -	unsigned long level, polarity;
> +	unsigned long level, polarity, flags;
>
>   	if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
>   		     IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
>   		return -EINVAL;
>
> +	spin_lock_irqsave(&bgc->lock, flags);
>   	level = readl(gpio->regs + INT_TYPE_REG_OFFS);
>   	polarity = readl(gpio->regs + INT_POLARITY_REG_OFFS);
>
>   	gpio->toggle_edge &= ~BIT(bit);
> -	if (type & IRQ_TYPE_EDGE_BOTH) {
> +	if (type == IRQ_TYPE_EDGE_BOTH) {
>   		gpio->toggle_edge |= BIT(bit);
>   		level |= (1 << bit);
>   		dwapb_toggle_trigger(gpio, bit);
> @@ -150,72 +164,102 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
>
>   	writel(level, gpio->regs + INT_TYPE_REG_OFFS);
>   	writel(polarity, gpio->regs + INT_POLARITY_REG_OFFS);
> +	spin_unlock_irqrestore(&bgc->lock, flags);
>
>   	return 0;
>   }
>
> -static int dwapb_create_irqchip(struct dwapb_gpio *gpio,
> -				struct dwapb_gpio_bank *bank,
> -				unsigned int irq_base)
> +static void dwapb_irq_ack(struct irq_data *d)
>   {
> -	struct irq_chip_type *ct;
> -
> -	gpio->irq_gc = irq_alloc_generic_chip("gpio-dwapb", 1, irq_base,
> -					      gpio->regs, handle_level_irq);
> -	if (!gpio->irq_gc)
> -		return -EIO;
> -
> -	gpio->irq_gc->domain.of_node = of_node_get(bank->bgc.gc.of_node);
> -	gpio->irq_gc->private = gpio;
> -	ct = gpio->irq_gc->chip_types;
> -	ct->chip.irq_ack = irq_gc_ack_set_bit;
> -	ct->chip.irq_mask = irq_gc_mask_set_bit;
> -	ct->chip.irq_unmask = irq_gc_mask_clr_bit;
> -	ct->chip.irq_set_type = dwapb_irq_set_type;
> -	ct->chip.irq_enable = dwapb_irq_enable;
> -	ct->chip.irq_disable = dwapb_irq_disable;
> -	ct->regs.ack = EOI_REG_OFFS;
> -	ct->regs.mask = INT_MASK_REG_OFFS;
> -	irq_setup_generic_chip(gpio->irq_gc, IRQ_MSK(bank->bgc.gc.ngpio),
> -			       IRQ_GC_INIT_NESTED_LOCK, IRQ_NOREQUEST, 0);
> +	struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d);
> +	struct bgpio_chip *bgc = &gpio->banks[0].bgc;
> +	unsigned long flags;
>
> -	return 0;
> +	spin_lock_irqsave(&bgc->lock, flags);
> +	writel(1 << d->hwirq, gpio->regs + EOI_REG_OFFS);
> +	spin_unlock_irqrestore(&bgc->lock, flags);
>   }
>
> -static int dwapb_configure_irqs(struct dwapb_gpio *gpio,
> -				struct dwapb_gpio_bank *bank)
> +static void dwapb_irq_mask(struct irq_data *d)
>   {
> -	unsigned int m, irq, ngpio = bank->bgc.gc.ngpio;
> -	int irq_base;
> -
> -	for (m = 0; m < ngpio; ++m) {
> -		irq = irq_of_parse_and_map(bank->bgc.gc.of_node, m);
> -		if (!irq && m == 0) {
> -			dev_warn(gpio->dev, "no irq for bank %s\n",
> -				 bank->bgc.gc.of_node->full_name);
> -			return -ENXIO;
> -		} else if (!irq) {
> -			break;
> -		}
> -
> -		irq_set_chained_handler(irq, dwapb_irq_handler);
> -		irq_set_handler_data(irq, gpio);
> -	}
> -	bank->bgc.gc.to_irq = dwapb_gpio_to_irq;
> +	struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d);
> +	struct bgpio_chip *bgc = &gpio->banks[0].bgc;
> +	unsigned long value, flags;
> +
> +	spin_lock_irqsave(&bgc->lock, flags);
> +	value = readl(gpio->regs + INT_MASK_REG_OFFS);
> +	value |= 1 << d->hwirq;
> +	writel(value, gpio->regs + INT_MASK_REG_OFFS);
> +	spin_unlock_irqrestore(&bgc->lock, flags);
> +}
>
> -	irq_base = irq_alloc_descs(-1, 0, ngpio, NUMA_NO_NODE);
> -	if (irq_base < 0)
> -		return irq_base;
> +static void dwapb_irq_unmask(struct irq_data *d)
> +{
> +	struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d);
> +	struct bgpio_chip *bgc = &gpio->banks[0].bgc;
> +	unsigned long value, flags;
> +
> +	spin_lock_irqsave(&bgc->lock, flags);
> +	value = readl(gpio->regs + INT_MASK_REG_OFFS);
> +	value &= ~(1 << d->hwirq);
> +	writel(value, gpio->regs + INT_MASK_REG_OFFS);
> +	spin_unlock_irqrestore(&bgc->lock, flags);
> +}
>
> -	if (dwapb_create_irqchip(gpio, bank, irq_base))
> -		goto out_free_descs;
> +static struct irq_chip dwapb_irq_chip = {
> +	.name = "gpio-dwapb",
> +	.irq_ack = dwapb_irq_ack,
> +	.irq_mask = dwapb_irq_mask,
> +	.irq_unmask = dwapb_irq_unmask,
> +	.irq_set_type = dwapb_irq_set_type,
> +	.irq_enable = dwapb_irq_enable,
> +	.irq_disable = dwapb_irq_disable,
> +};
> +
> +static int dwapb_gpio_irq_map(struct irq_domain *domain,
> +			      unsigned int irq,
> +			      irq_hw_number_t hwirq)
> +{
> +	struct dwapb_gpio *gpio = domain->host_data;
> +
> +	irq_set_chip_data(irq, gpio);
> +	irq_set_chip_and_handler(irq, &dwapb_irq_chip, handle_level_irq);
> +	irq_set_irq_type(irq, IRQ_TYPE_NONE);
> +	irq_set_handler_data(irq, gpio);
>
>   	return 0;
> +}
>
> -out_free_descs:
> -	irq_free_descs(irq_base, ngpio);
> +static struct irq_domain_ops dwapb_gpio_irq_ops = {
> +	.map = dwapb_gpio_irq_map,
> +	.xlate = irq_domain_xlate_twocell,
> +};
>
> -	return -EIO;
> +static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
> +				 struct dwapb_gpio_bank *bank)
> +{
> +	struct gpio_chip *gc = &bank->bgc.gc;
> +	struct device_node *node =  gc->of_node;
> +	unsigned int ngpio = gc->ngpio;
> +	int reg;
> +
> +	if (of_get_property(node, "interrupts", &reg) == NULL)
> +		return;
> +
> +	gpio->hwirq = irq_of_parse_and_map(node, 0);
> +	if (gpio->hwirq == NO_IRQ)
> +		return;
> +
> +	gpio->domain = irq_domain_add_linear(node, ngpio, &dwapb_gpio_irq_ops,
> +					     gpio);
> +	if (!gpio->domain) {
> +		irq_dispose_mapping(gpio->hwirq);
> +		return;
> +	}
> +
> +	irq_set_handler_data(gpio->hwirq, gpio);
> +	irq_set_chained_handler(gpio->hwirq, dwapb_irq_handler);
> +	bank->bgc.gc.to_irq = dwapb_gpio_to_irq;
>   }
>
>   static int dwapb_gpio_add_bank(struct dwapb_gpio *gpio,
> @@ -240,7 +284,6 @@ static int dwapb_gpio_add_bank(struct dwapb_gpio *gpio,
>   		return -EINVAL;
>   	}
>
> -	bank->bank_idx = bank_idx;
>   	err = bgpio_init(&bank->bgc, gpio->dev, 4,
>   			 gpio->regs + 0x50 + (bank_idx * 0x4),
>   			 gpio->regs + 0x00 + (bank_idx * 0xc),
> @@ -282,7 +325,7 @@ static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
>   	of_node_put(gpio->of_node);
>   }
>
> -static int __devinit dwapb_gpio_probe(struct platform_device *pdev)
> +static int dwapb_gpio_probe(struct platform_device *pdev)
>   {
>   	struct resource *res;
>   	struct dwapb_gpio *gpio;
> @@ -296,21 +339,29 @@ static int __devinit dwapb_gpio_probe(struct platform_device *pdev)
>   	gpio->dev = &pdev->dev;
>
>   	gpio->nr_banks = dwapb_gpio_nr_banks(pdev->dev.of_node);
> -	if (!gpio->nr_banks)
> -		return -EINVAL;
> +	if (!gpio->nr_banks) {
> +		err = -EINVAL;
> +		goto out_err;
> +	}
>   	gpio->banks = devm_kzalloc(&pdev->dev, gpio->nr_banks *
>   				   sizeof(*gpio->banks), GFP_KERNEL);
> -	if (!gpio->banks)
> -		return -ENOMEM;
> +	if (!gpio->banks) {
> +		err = -ENOMEM;
> +		goto out_err;
> +	}
>
>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   	if (!res) {
>   		dev_err(&pdev->dev, "failed to get iomem\n");
> -		return -ENXIO;
> +		err = -ENXIO;
> +		goto out_free_banks;
>   	}
> +
>   	gpio->regs = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> -	if (!gpio->regs)
> -		return -ENOMEM;
> +	if (!gpio->regs) {
> +		err = -ENOMEM;
> +		goto out_free_banks;
> +	}
>
>   	gpio->of_node = of_node_get(pdev->dev.of_node);
>   	for_each_child_of_node(pdev->dev.of_node, np) {
> @@ -325,9 +376,34 @@ static int __devinit dwapb_gpio_probe(struct platform_device *pdev)
>   out_unregister:
>   	dwapb_gpio_unregister(gpio);
>
> +	if (gpio->domain) {
> +		irq_dispose_mapping(gpio->hwirq);
> +		irq_domain_remove(gpio->domain);
> +	}
> +
> +out_free_banks:
> +	devm_kfree(&pdev->dev, gpio->banks);
> +
> +out_err:
> +	devm_kfree(&pdev->dev, gpio);

devm_ will take care of kfree for you.

>   	return err;
>   }
>
> +static int dwapb_gpio_remove(struct platform_device *pdev)
> +{
> +	struct dwapb_gpio *gpio = platform_get_drvdata(pdev);
> +
> +	dwapb_gpio_unregister(gpio);
> +	if (gpio->domain) {
> +		irq_dispose_mapping(gpio->hwirq);
> +		irq_domain_remove(gpio->domain);
> +	}
> +	devm_kfree(&pdev->dev, gpio->banks);
> +	devm_kfree(&pdev->dev, gpio);

ditto.

> +
> +	return 0;
> +}
> +
>   static const struct of_device_id dwapb_of_match_table[] = {
>   	{ .compatible = "snps,dw-apb-gpio" },
>   	{ /* Sentinel */ }
> @@ -340,6 +416,7 @@ static struct platform_driver dwapb_gpio_driver = {
>   		.of_match_table = dwapb_of_match_table,
>   	},
>   	.probe		= dwapb_gpio_probe,
> +	.remove		= dwapb_gpio_remove,
>   };
>
>   static int __init dwapb_gpio_init(void)
> @@ -348,6 +425,12 @@ static int __init dwapb_gpio_init(void)
>   }
>   postcore_initcall(dwapb_gpio_init);
>
> +static void __exit dwapb_gpio_exit(void)
> +{
> +	platform_driver_unregister(&dwapb_gpio_driver);
> +}
> +module_exit(dwapb_gpio_exit);

module_platform_driver() for _init and _exit above and get rid
of postcore_initcall?

>   MODULE_LICENSE("GPL");
>   MODULE_AUTHOR("Jamie Iles");
>   MODULE_DESCRIPTION("Synopsys DesignWare APB GPIO driver");
>

Re: [PATCH 3/3] use linear irq domain in gpio-dwapb

[delicious quinoa]

On Tue, Nov 5, 2013 at 3:32 PM, Sebastian Hesselbarth
<sebastian.hesselbarth@gmail.com> wrote:
> On 11/05/2013 05:55 PM, Alan Tull wrote:
>>
>> From: Alan Tull <atull@altera.com>
>>
>>   * Changes to get gpio-dwapb (originally v3.2-rc7) driver building and
>>     working on current kernel.
>>   * Use linear irq domain.
>>   * Fix setting irq edge type for 'rising' and 'both'.
>>   * Support as a loadable module.
>>   * Use bgpio_chip's spinlock during register access.
>
>
> As this patch is basically messing what I have commented before,
> I really lost interest in reviewing this again. Please just squash
> them into one.

Hi Sebastian,

Thanks for the review.  I'm working on implementing these changes.

Do you want me to squash all 3 patches or just the last two?

In either case, I will be taking off all the Ack's and Signed-off-by's
as I will  be changing the patches post-ack/signoff.

Alan