On 11/05/2013 05:55 PM, Alan Tull wrote:
From: Jamie Iles <[email protected]>
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 <[email protected]>
Cc: Linus Walleij <[email protected]>
Acked-by: Rob Herring <[email protected]>
Signed-off-by: Jamie Iles <[email protected]>
---
.../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 [email protected]
+ */
+#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");
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
