* [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver
@ 2013-03-12 5:37 thloh
2013-03-19 6:01 ` Tien Hock Loh
2013-03-27 11:58 ` Linus Walleij
0 siblings, 2 replies; 20+ messages in thread
From: thloh @ 2013-03-12 5:37 UTC (permalink / raw)
To: Linus Walleij, Rob Landley; +Cc: linux-doc, linux-kernel, Tien Hock Loh
From: Tien Hock Loh <thloh@altera.com>
Adds a new driver for Altera soft GPIO IP. The driver is able to
do read/write and allows GPIO to be a interrupt controller.
Tested on Altera GHRD on interrupt handling and IO.
Signed-off-by: Tien Hock Loh <thloh@altera.com>
---
.../devicetree/bindings/gpio/gpio-altera.txt | 37 +++
drivers/gpio/Kconfig | 7 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-altera.c | 363 +++++++++++++++++++++
4 files changed, 408 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-altera.txt
create mode 100644 drivers/gpio/gpio-altera.c
diff --git a/Documentation/devicetree/bindings/gpio/gpio-altera.txt b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
new file mode 100644
index 0000000..3bdb8b6
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
@@ -0,0 +1,37 @@
+Altera GPIO controller bindings
+
+Required properties:
+- compatible:
+ - "altr,pio-1.0"
+- reg: Physical base address and length of the controller's registers.
+- #gpio-cells : Should be two.
+ - first cell is the pin number
+ - second cell is used to specify optional parameters (unused)
+- gpio-controller : Marks the device node as a GPIO controller.
+- #interrupt-cells : Should be 1.
+- interrupts: Specify the interrupt.
+- interrupt-controller: Mark the device node as an interrupt controller
+ The first cell is the GPIO number.
+
+Altera GPIO specific properties:
+- width: Width of the GPIO bank, range from 1-32
+- level_trigger: Specifies whether the GPIO interrupt is level trigger.
+ This field is required if the Altera GPIO controller used has IRQ enabled.
+
+Note: If the Altera GPIO is set to be built as a module, peripherals that uses
+Altera GPIO as interrupt-parent should be a module so that the peripheral
+doesn't get initialized before Altera GPIO is initialized.
+
+Example:
+
+gpio_altr: gpio_altr {
+ compatible = "altr,pio-1.0";
+ reg = <0xff200000 0x10>;
+ interrupts = <0 45 4>;
+ width = <32>;
+ level_trigger = <0>;
+ #gpio-cells = <2>;
+ gpio-controller;
+ #interrupt-cells = <1>;
+ interrupt-controller;
+};
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 93aaadf..953e9f2 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -119,6 +119,13 @@ config GPIO_GENERIC_PLATFORM
help
Say yes here to support basic platform_device memory-mapped GPIO controllers.
+config GPIO_ALTERA
+ tristate "Altera GPIO"
+ select IRQ_DOMAIN
+ depends on OF_GPIO
+ help
+ Say yes here to support the Altera PIO device.
+
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 22e07bc..c57449c 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_GPIO_74X164) += gpio-74x164.o
obj-$(CONFIG_GPIO_ADNP) += gpio-adnp.o
obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o
obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o
+obj-$(CONFIG_GPIO_ALTERA) += gpio-altera.o
obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o
obj-$(CONFIG_GPIO_ARIZONA) += gpio-arizona.o
obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
diff --git a/drivers/gpio/gpio-altera.c b/drivers/gpio/gpio-altera.c
new file mode 100644
index 0000000..b832a3a
--- /dev/null
+++ b/drivers/gpio/gpio-altera.c
@@ -0,0 +1,363 @@
+/*
+ * Copyright (C) 2013 Altera Corporation
+ * Based on gpio-mpc8xxx.c
+ *
+ * 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 3 of the License, or
+ * (at your option) any later version.
+ *
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>
+ */
+
+#include <linux/gpio.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/of_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/interrupt.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+
+#define ALTERA_GPIO_DATA 0x0
+#define ALTERA_GPIO_DIR 0x4
+#define ALTERA_GPIO_IRQ_MASK 0x8
+#define ALTERA_GPIO_EDGE_CAP 0xc
+#define ALTERA_GPIO_OUTSET 0x10
+#define ALTERA_GPIO_OUTCLEAR 0x14
+
+struct altera_gpio_chip {
+ struct of_mm_gpio_chip mmchip;
+ struct irq_domain *irq; /* GPIO controller IRQ number */
+ spinlock_t gpio_lock; /* Lock used for synchronization */
+ int level_trigger;
+ int hwirq;
+};
+
+static void altera_gpio_irq_unmask(struct irq_data *d)
+{
+ struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
+ struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
+ unsigned long flags;
+ unsigned int intmask;
+
+ spin_lock_irqsave(&altera_gc->gpio_lock, flags);
+ intmask = readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+ /* Set ALTERA_GPIO_IRQ_MASK bit to unmask */
+ intmask |= (1 << irqd_to_hwirq(d));
+ writel(intmask, mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+ spin_unlock_irqrestore(&altera_gc->gpio_lock, flags);
+}
+
+static void altera_gpio_irq_mask(struct irq_data *d)
+{
+ struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
+ struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
+ unsigned long flags;
+ unsigned int intmask;
+
+ spin_lock_irqsave(&altera_gc->gpio_lock, flags);
+ intmask = readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+ /* Clear ALTERA_GPIO_IRQ_MASK bit to mask */
+ intmask &= ~(1 << irqd_to_hwirq(d));
+ writel(intmask, mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+ spin_unlock_irqrestore(&altera_gc->gpio_lock, flags);
+}
+
+static int altera_gpio_irq_set_type(struct irq_data *d,
+ unsigned int type)
+{
+ if (type == IRQ_TYPE_NONE)
+ return 0;
+ return -EINVAL;
+}
+
+static struct irq_chip altera_irq_chip = {
+ .name = "altera-gpio",
+ .irq_mask = altera_gpio_irq_mask,
+ .irq_unmask = altera_gpio_irq_unmask,
+ .irq_set_type = altera_gpio_irq_set_type,
+};
+
+static int altera_gpio_get(struct gpio_chip *gc, unsigned offset)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+
+ return (readl(mm_gc->regs + ALTERA_GPIO_DATA) >> offset) & 1;
+}
+
+static void altera_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct altera_gpio_chip *chip = container_of(mm_gc,
+ struct altera_gpio_chip, mmchip);
+ unsigned long flags;
+ unsigned int data_reg;
+
+ spin_lock_irqsave(&chip->gpio_lock, flags);
+ data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA);
+ data_reg = (data_reg & ~(1 << offset)) | (value << offset);
+ writel(data_reg, mm_gc->regs + ALTERA_GPIO_DATA);
+ spin_unlock_irqrestore(&chip->gpio_lock, flags);
+}
+
+static int altera_gpio_direction_input(struct gpio_chip *gc, unsigned offset)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct altera_gpio_chip *chip = container_of(mm_gc,
+ struct altera_gpio_chip, mmchip);
+ unsigned long flags;
+ unsigned int gpio_ddr;
+
+ spin_lock_irqsave(&chip->gpio_lock, flags);
+ /* Set pin as input, assumes software controlled IP */
+ gpio_ddr = readl(mm_gc->regs + ALTERA_GPIO_DIR);
+ gpio_ddr &= ~(1 << offset);
+ writel(gpio_ddr, mm_gc->regs + ALTERA_GPIO_DIR);
+ spin_unlock_irqrestore(&chip->gpio_lock, flags);
+
+ return 0;
+}
+
+static int altera_gpio_direction_output(struct gpio_chip *gc,
+ unsigned offset, int value)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct altera_gpio_chip *chip = container_of(mm_gc,
+ struct altera_gpio_chip, mmchip);
+ unsigned long flags;
+ unsigned int data_reg, gpio_ddr;
+
+ spin_lock_irqsave(&chip->gpio_lock, flags);
+ /* Sets the GPIO value */
+ data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA);
+ data_reg = (data_reg & ~(1 << offset)) | (value << offset);
+ writel(data_reg, mm_gc->regs + ALTERA_GPIO_DATA);
+
+ /* Set pin as output, assumes software controlled IP */
+ gpio_ddr = readl(mm_gc->regs + ALTERA_GPIO_DIR);
+ gpio_ddr |= (1 << offset);
+ writel(gpio_ddr, mm_gc->regs + ALTERA_GPIO_DIR);
+ spin_unlock_irqrestore(&chip->gpio_lock, flags);
+
+ return 0;
+}
+
+static int altera_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct altera_gpio_chip *altera_gc = container_of(mm_gc,
+ struct altera_gpio_chip, mmchip);
+
+ if (altera_gc->irq == 0)
+ return -ENXIO;
+ if ((altera_gc->irq && offset) < altera_gc->mmchip.gc.ngpio)
+ return irq_create_mapping(altera_gc->irq, offset);
+ else
+ return -ENXIO;
+}
+
+static void altera_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
+{
+ struct altera_gpio_chip *altera_gc = irq_desc_get_handler_data(desc);
+ struct irq_chip *chip = irq_desc_get_chip(desc);
+ struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
+ unsigned long status;
+
+ int base;
+ chip->irq_mask(&desc->irq_data);
+
+ if (altera_gc->level_trigger)
+ status = readl(mm_gc->regs + ALTERA_GPIO_DATA);
+ else {
+ status = readl(mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
+ writel(status, mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
+ }
+
+ status &= readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+
+ for (base = 0; base < mm_gc->gc.ngpio; base++) {
+ if ((1 << base) & status) {
+ generic_handle_irq(
+ irq_linear_revmap(altera_gc->irq, base));
+ }
+ }
+ chip->irq_eoi(irq_desc_get_irq_data(desc));
+ chip->irq_unmask(&desc->irq_data);
+}
+
+static int altera_gpio_irq_map(struct irq_domain *h, unsigned int virq,
+ irq_hw_number_t hw_irq_num)
+{
+ irq_set_chip_data(virq, h->host_data);
+ irq_set_chip_and_handler(virq, &altera_irq_chip, handle_level_irq);
+ irq_set_irq_type(virq, IRQ_TYPE_NONE);
+
+ return 0;
+}
+
+static struct irq_domain_ops altera_gpio_irq_ops = {
+ .map = altera_gpio_irq_map,
+ .xlate = irq_domain_xlate_twocell,
+};
+
+int altera_gpio_probe(struct platform_device *pdev)
+{
+ struct device_node *node = pdev->dev.of_node;
+ int id, reg, ret;
+ struct altera_gpio_chip *altera_gc = devm_kzalloc(&pdev->dev,
+ sizeof(*altera_gc), GFP_KERNEL);
+ if (altera_gc == NULL) {
+ ret = -ENOMEM;
+ pr_err("%s: registration failed with status %d\n",
+ node->full_name, ret);
+ return ret;
+ }
+ altera_gc->irq = 0;
+
+ spin_lock_init(&altera_gc->gpio_lock);
+
+ id = pdev->id;
+
+ if (of_property_read_u32(node, "width", ®))
+ /*By default assume full GPIO controller*/
+ altera_gc->mmchip.gc.ngpio = 32;
+ else
+ altera_gc->mmchip.gc.ngpio = reg;
+
+ if (altera_gc->mmchip.gc.ngpio > 32) {
+ pr_warn("%s: ngpio is greater than 32, defaulting to 32\n",
+ node->full_name);
+ altera_gc->mmchip.gc.ngpio = 32;
+ }
+
+ altera_gc->mmchip.gc.direction_input = altera_gpio_direction_input;
+ altera_gc->mmchip.gc.direction_output = altera_gpio_direction_output;
+ altera_gc->mmchip.gc.get = altera_gpio_get;
+ altera_gc->mmchip.gc.set = altera_gpio_set;
+ altera_gc->mmchip.gc.to_irq = altera_gpio_to_irq;
+ altera_gc->mmchip.gc.owner = THIS_MODULE;
+
+ ret = of_mm_gpiochip_add(node, &altera_gc->mmchip);
+ if (ret)
+ goto err;
+
+ platform_set_drvdata(pdev, altera_gc);
+
+ if (of_get_property(node, "interrupts", ®) == NULL)
+ goto skip_irq;
+ altera_gc->hwirq = irq_of_parse_and_map(node, 0);
+
+ if (altera_gc->hwirq == NO_IRQ)
+ goto skip_irq;
+
+ altera_gc->irq = irq_domain_add_linear(node, altera_gc->mmchip.gc.ngpio,
+ &altera_gpio_irq_ops, altera_gc);
+
+ if (!altera_gc->irq) {
+ ret = -ENODEV;
+ goto dispose_irq;
+ }
+
+ if (of_property_read_u32(node, "level_trigger", ®)) {
+ ret = -EINVAL;
+ pr_err("%s: level_trigger value not set in device tree\n",
+ node->full_name);
+ goto teardown;
+ }
+ altera_gc->level_trigger = reg;
+
+ irq_set_handler_data(altera_gc->hwirq, altera_gc);
+ irq_set_chained_handler(altera_gc->hwirq, altera_gpio_irq_handler);
+
+ return 0;
+
+teardown:
+ irq_domain_remove(altera_gc->irq);
+dispose_irq:
+ irq_dispose_mapping(altera_gc->hwirq);
+ WARN_ON(gpiochip_remove(&altera_gc->mmchip.gc) < 0);
+
+err:
+ pr_err("%s: registration failed with status %d\n",
+ node->full_name, ret);
+ devm_kfree(&pdev->dev, altera_gc);
+
+ return ret;
+skip_irq:
+ return 0;
+}
+
+static int altera_gpio_remove(struct platform_device *pdev)
+{
+ unsigned int irq, i;
+ int status;
+ struct altera_gpio_chip *altera_gc = platform_get_drvdata(pdev);
+
+ status = gpiochip_remove(&altera_gc->mmchip.gc);
+
+ if (status < 0)
+ return status;
+
+ if (altera_gc->irq) {
+ irq_dispose_mapping(altera_gc->hwirq);
+
+ for (i = 0; i < altera_gc->mmchip.gc.ngpio; i++) {
+ irq = irq_find_mapping(altera_gc->irq, i);
+ if (irq > 0)
+ irq_dispose_mapping(irq);
+ }
+
+ irq_domain_remove(altera_gc->irq);
+ }
+
+ irq_set_handler_data(altera_gc->hwirq, NULL);
+ irq_set_chained_handler(altera_gc->hwirq, NULL);
+ devm_kfree(&pdev->dev, altera_gc);
+ return -EIO;
+}
+
+#ifdef CONFIG_OF
+static struct of_device_id altera_gpio_of_match[] = {
+ { .compatible = "altr,pio-1.0", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, altera_gpio_of_match);
+#else
+#define altera_gpio_of_match NULL
+#endif
+
+static struct platform_driver altera_gpio_driver = {
+ .driver = {
+ .name = "altera_gpio",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(altera_gpio_of_match),
+ },
+ .probe = altera_gpio_probe,
+ .remove = altera_gpio_remove,
+};
+
+static int __init altera_gpio_init(void)
+{
+ return platform_driver_register(&altera_gpio_driver);
+}
+subsys_initcall(altera_gpio_init);
+
+static void __exit altera_gpio_exit(void)
+{
+ platform_driver_unregister(&altera_gpio_driver);
+}
+module_exit(altera_gpio_exit);
+
+MODULE_DESCRIPTION("Altera GPIO driver");
+MODULE_LICENSE("GPL");
--
1.7.11.GIT
^ permalink raw reply related [flat|nested] 20+ messages in thread
* RE: [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver
2013-03-12 5:37 [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver thloh
@ 2013-03-19 6:01 ` Tien Hock Loh
2013-03-27 11:58 ` Linus Walleij
1 sibling, 0 replies; 20+ messages in thread
From: Tien Hock Loh @ 2013-03-19 6:01 UTC (permalink / raw)
To: 'Linus Walleij', 'Rob Landley'
Cc: 'linux-doc@vger.kernel.org',
'linux-kernel@vger.kernel.org'
Hi,
Are the codes good for upstream?
Thanks.
-----Original Message-----
From: Tien Hock Loh
Sent: Tuesday, March 12, 2013 1:38 PM
To: Linus Walleij; Rob Landley
Cc: linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org; Tien Hock Loh
Subject: [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver
From: Tien Hock Loh <thloh@altera.com>
Adds a new driver for Altera soft GPIO IP. The driver is able to do read/write and allows GPIO to be a interrupt controller.
Tested on Altera GHRD on interrupt handling and IO.
Signed-off-by: Tien Hock Loh <thloh@altera.com>
---
.../devicetree/bindings/gpio/gpio-altera.txt | 37 +++
drivers/gpio/Kconfig | 7 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-altera.c | 363 +++++++++++++++++++++
4 files changed, 408 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-altera.txt
create mode 100644 drivers/gpio/gpio-altera.c
diff --git a/Documentation/devicetree/bindings/gpio/gpio-altera.txt b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
new file mode 100644
index 0000000..3bdb8b6
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
@@ -0,0 +1,37 @@
+Altera GPIO controller bindings
+
+Required properties:
+- compatible:
+ - "altr,pio-1.0"
+- reg: Physical base address and length of the controller's registers.
+- #gpio-cells : Should be two.
+ - first cell is the pin number
+ - second cell is used to specify optional parameters (unused)
+- gpio-controller : Marks the device node as a GPIO controller.
+- #interrupt-cells : Should be 1.
+- interrupts: Specify the interrupt.
+- interrupt-controller: Mark the device node as an interrupt controller
+ The first cell is the GPIO number.
+
+Altera GPIO specific properties:
+- width: Width of the GPIO bank, range from 1-32
+- level_trigger: Specifies whether the GPIO interrupt is level trigger.
+ This field is required if the Altera GPIO controller used has IRQ enabled.
+
+Note: If the Altera GPIO is set to be built as a module, peripherals
+that uses Altera GPIO as interrupt-parent should be a module so that
+the peripheral doesn't get initialized before Altera GPIO is initialized.
+
+Example:
+
+gpio_altr: gpio_altr {
+ compatible = "altr,pio-1.0";
+ reg = <0xff200000 0x10>;
+ interrupts = <0 45 4>;
+ width = <32>;
+ level_trigger = <0>;
+ #gpio-cells = <2>;
+ gpio-controller;
+ #interrupt-cells = <1>;
+ interrupt-controller;
+};
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 93aaadf..953e9f2 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -119,6 +119,13 @@ config GPIO_GENERIC_PLATFORM
help
Say yes here to support basic platform_device memory-mapped GPIO controllers.
+config GPIO_ALTERA
+ tristate "Altera GPIO"
+ select IRQ_DOMAIN
+ depends on OF_GPIO
+ help
+ Say yes here to support the Altera PIO device.
+
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 22e07bc..c57449c 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_GPIO_74X164) += gpio-74x164.o
obj-$(CONFIG_GPIO_ADNP) += gpio-adnp.o
obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o
obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o
+obj-$(CONFIG_GPIO_ALTERA) += gpio-altera.o
obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o
obj-$(CONFIG_GPIO_ARIZONA) += gpio-arizona.o
obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
diff --git a/drivers/gpio/gpio-altera.c b/drivers/gpio/gpio-altera.c new file mode 100644 index 0000000..b832a3a
--- /dev/null
+++ b/drivers/gpio/gpio-altera.c
@@ -0,0 +1,363 @@
+/*
+ * Copyright (C) 2013 Altera Corporation
+ * Based on gpio-mpc8xxx.c
+ *
+ * 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 3 of the License, or
+ * (at your option) any later version.
+ *
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>
+*/
+
+#include <linux/gpio.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/of_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/interrupt.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+
+#define ALTERA_GPIO_DATA 0x0
+#define ALTERA_GPIO_DIR 0x4
+#define ALTERA_GPIO_IRQ_MASK 0x8
+#define ALTERA_GPIO_EDGE_CAP 0xc
+#define ALTERA_GPIO_OUTSET 0x10
+#define ALTERA_GPIO_OUTCLEAR 0x14
+
+struct altera_gpio_chip {
+ struct of_mm_gpio_chip mmchip;
+ struct irq_domain *irq; /* GPIO controller IRQ number */
+ spinlock_t gpio_lock; /* Lock used for synchronization */
+ int level_trigger;
+ int hwirq;
+};
+
+static void altera_gpio_irq_unmask(struct irq_data *d) {
+ struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
+ struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
+ unsigned long flags;
+ unsigned int intmask;
+
+ spin_lock_irqsave(&altera_gc->gpio_lock, flags);
+ intmask = readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+ /* Set ALTERA_GPIO_IRQ_MASK bit to unmask */
+ intmask |= (1 << irqd_to_hwirq(d));
+ writel(intmask, mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+ spin_unlock_irqrestore(&altera_gc->gpio_lock, flags); }
+
+static void altera_gpio_irq_mask(struct irq_data *d) {
+ struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
+ struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
+ unsigned long flags;
+ unsigned int intmask;
+
+ spin_lock_irqsave(&altera_gc->gpio_lock, flags);
+ intmask = readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+ /* Clear ALTERA_GPIO_IRQ_MASK bit to mask */
+ intmask &= ~(1 << irqd_to_hwirq(d));
+ writel(intmask, mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+ spin_unlock_irqrestore(&altera_gc->gpio_lock, flags); }
+
+static int altera_gpio_irq_set_type(struct irq_data *d,
+ unsigned int type)
+{
+ if (type == IRQ_TYPE_NONE)
+ return 0;
+ return -EINVAL;
+}
+
+static struct irq_chip altera_irq_chip = {
+ .name = "altera-gpio",
+ .irq_mask = altera_gpio_irq_mask,
+ .irq_unmask = altera_gpio_irq_unmask,
+ .irq_set_type = altera_gpio_irq_set_type,
+};
+
+static int altera_gpio_get(struct gpio_chip *gc, unsigned offset) {
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+
+ return (readl(mm_gc->regs + ALTERA_GPIO_DATA) >> offset) & 1; }
+
+static void altera_gpio_set(struct gpio_chip *gc, unsigned offset, int
+value) {
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct altera_gpio_chip *chip = container_of(mm_gc,
+ struct altera_gpio_chip, mmchip);
+ unsigned long flags;
+ unsigned int data_reg;
+
+ spin_lock_irqsave(&chip->gpio_lock, flags);
+ data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA);
+ data_reg = (data_reg & ~(1 << offset)) | (value << offset);
+ writel(data_reg, mm_gc->regs + ALTERA_GPIO_DATA);
+ spin_unlock_irqrestore(&chip->gpio_lock, flags); }
+
+static int altera_gpio_direction_input(struct gpio_chip *gc, unsigned
+offset) {
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct altera_gpio_chip *chip = container_of(mm_gc,
+ struct altera_gpio_chip, mmchip);
+ unsigned long flags;
+ unsigned int gpio_ddr;
+
+ spin_lock_irqsave(&chip->gpio_lock, flags);
+ /* Set pin as input, assumes software controlled IP */
+ gpio_ddr = readl(mm_gc->regs + ALTERA_GPIO_DIR);
+ gpio_ddr &= ~(1 << offset);
+ writel(gpio_ddr, mm_gc->regs + ALTERA_GPIO_DIR);
+ spin_unlock_irqrestore(&chip->gpio_lock, flags);
+
+ return 0;
+}
+
+static int altera_gpio_direction_output(struct gpio_chip *gc,
+ unsigned offset, int value)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct altera_gpio_chip *chip = container_of(mm_gc,
+ struct altera_gpio_chip, mmchip);
+ unsigned long flags;
+ unsigned int data_reg, gpio_ddr;
+
+ spin_lock_irqsave(&chip->gpio_lock, flags);
+ /* Sets the GPIO value */
+ data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA);
+ data_reg = (data_reg & ~(1 << offset)) | (value << offset);
+ writel(data_reg, mm_gc->regs + ALTERA_GPIO_DATA);
+
+ /* Set pin as output, assumes software controlled IP */
+ gpio_ddr = readl(mm_gc->regs + ALTERA_GPIO_DIR);
+ gpio_ddr |= (1 << offset);
+ writel(gpio_ddr, mm_gc->regs + ALTERA_GPIO_DIR);
+ spin_unlock_irqrestore(&chip->gpio_lock, flags);
+
+ return 0;
+}
+
+static int altera_gpio_to_irq(struct gpio_chip *gc, unsigned offset) {
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct altera_gpio_chip *altera_gc = container_of(mm_gc,
+ struct altera_gpio_chip, mmchip);
+
+ if (altera_gc->irq == 0)
+ return -ENXIO;
+ if ((altera_gc->irq && offset) < altera_gc->mmchip.gc.ngpio)
+ return irq_create_mapping(altera_gc->irq, offset);
+ else
+ return -ENXIO;
+}
+
+static void altera_gpio_irq_handler(unsigned int irq, struct irq_desc
+*desc) {
+ struct altera_gpio_chip *altera_gc = irq_desc_get_handler_data(desc);
+ struct irq_chip *chip = irq_desc_get_chip(desc);
+ struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
+ unsigned long status;
+
+ int base;
+ chip->irq_mask(&desc->irq_data);
+
+ if (altera_gc->level_trigger)
+ status = readl(mm_gc->regs + ALTERA_GPIO_DATA);
+ else {
+ status = readl(mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
+ writel(status, mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
+ }
+
+ status &= readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+
+ for (base = 0; base < mm_gc->gc.ngpio; base++) {
+ if ((1 << base) & status) {
+ generic_handle_irq(
+ irq_linear_revmap(altera_gc->irq, base));
+ }
+ }
+ chip->irq_eoi(irq_desc_get_irq_data(desc));
+ chip->irq_unmask(&desc->irq_data);
+}
+
+static int altera_gpio_irq_map(struct irq_domain *h, unsigned int virq,
+ irq_hw_number_t hw_irq_num)
+{
+ irq_set_chip_data(virq, h->host_data);
+ irq_set_chip_and_handler(virq, &altera_irq_chip, handle_level_irq);
+ irq_set_irq_type(virq, IRQ_TYPE_NONE);
+
+ return 0;
+}
+
+static struct irq_domain_ops altera_gpio_irq_ops = {
+ .map = altera_gpio_irq_map,
+ .xlate = irq_domain_xlate_twocell,
+};
+
+int altera_gpio_probe(struct platform_device *pdev) {
+ struct device_node *node = pdev->dev.of_node;
+ int id, reg, ret;
+ struct altera_gpio_chip *altera_gc = devm_kzalloc(&pdev->dev,
+ sizeof(*altera_gc), GFP_KERNEL);
+ if (altera_gc == NULL) {
+ ret = -ENOMEM;
+ pr_err("%s: registration failed with status %d\n",
+ node->full_name, ret);
+ return ret;
+ }
+ altera_gc->irq = 0;
+
+ spin_lock_init(&altera_gc->gpio_lock);
+
+ id = pdev->id;
+
+ if (of_property_read_u32(node, "width", ®))
+ /*By default assume full GPIO controller*/
+ altera_gc->mmchip.gc.ngpio = 32;
+ else
+ altera_gc->mmchip.gc.ngpio = reg;
+
+ if (altera_gc->mmchip.gc.ngpio > 32) {
+ pr_warn("%s: ngpio is greater than 32, defaulting to 32\n",
+ node->full_name);
+ altera_gc->mmchip.gc.ngpio = 32;
+ }
+
+ altera_gc->mmchip.gc.direction_input = altera_gpio_direction_input;
+ altera_gc->mmchip.gc.direction_output = altera_gpio_direction_output;
+ altera_gc->mmchip.gc.get = altera_gpio_get;
+ altera_gc->mmchip.gc.set = altera_gpio_set;
+ altera_gc->mmchip.gc.to_irq = altera_gpio_to_irq;
+ altera_gc->mmchip.gc.owner = THIS_MODULE;
+
+ ret = of_mm_gpiochip_add(node, &altera_gc->mmchip);
+ if (ret)
+ goto err;
+
+ platform_set_drvdata(pdev, altera_gc);
+
+ if (of_get_property(node, "interrupts", ®) == NULL)
+ goto skip_irq;
+ altera_gc->hwirq = irq_of_parse_and_map(node, 0);
+
+ if (altera_gc->hwirq == NO_IRQ)
+ goto skip_irq;
+
+ altera_gc->irq = irq_domain_add_linear(node, altera_gc->mmchip.gc.ngpio,
+ &altera_gpio_irq_ops, altera_gc);
+
+ if (!altera_gc->irq) {
+ ret = -ENODEV;
+ goto dispose_irq;
+ }
+
+ if (of_property_read_u32(node, "level_trigger", ®)) {
+ ret = -EINVAL;
+ pr_err("%s: level_trigger value not set in device tree\n",
+ node->full_name);
+ goto teardown;
+ }
+ altera_gc->level_trigger = reg;
+
+ irq_set_handler_data(altera_gc->hwirq, altera_gc);
+ irq_set_chained_handler(altera_gc->hwirq, altera_gpio_irq_handler);
+
+ return 0;
+
+teardown:
+ irq_domain_remove(altera_gc->irq);
+dispose_irq:
+ irq_dispose_mapping(altera_gc->hwirq);
+ WARN_ON(gpiochip_remove(&altera_gc->mmchip.gc) < 0);
+
+err:
+ pr_err("%s: registration failed with status %d\n",
+ node->full_name, ret);
+ devm_kfree(&pdev->dev, altera_gc);
+
+ return ret;
+skip_irq:
+ return 0;
+}
+
+static int altera_gpio_remove(struct platform_device *pdev) {
+ unsigned int irq, i;
+ int status;
+ struct altera_gpio_chip *altera_gc = platform_get_drvdata(pdev);
+
+ status = gpiochip_remove(&altera_gc->mmchip.gc);
+
+ if (status < 0)
+ return status;
+
+ if (altera_gc->irq) {
+ irq_dispose_mapping(altera_gc->hwirq);
+
+ for (i = 0; i < altera_gc->mmchip.gc.ngpio; i++) {
+ irq = irq_find_mapping(altera_gc->irq, i);
+ if (irq > 0)
+ irq_dispose_mapping(irq);
+ }
+
+ irq_domain_remove(altera_gc->irq);
+ }
+
+ irq_set_handler_data(altera_gc->hwirq, NULL);
+ irq_set_chained_handler(altera_gc->hwirq, NULL);
+ devm_kfree(&pdev->dev, altera_gc);
+ return -EIO;
+}
+
+#ifdef CONFIG_OF
+static struct of_device_id altera_gpio_of_match[] = {
+ { .compatible = "altr,pio-1.0", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, altera_gpio_of_match); #else #define
+altera_gpio_of_match NULL #endif
+
+static struct platform_driver altera_gpio_driver = {
+ .driver = {
+ .name = "altera_gpio",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(altera_gpio_of_match),
+ },
+ .probe = altera_gpio_probe,
+ .remove = altera_gpio_remove,
+};
+
+static int __init altera_gpio_init(void) {
+ return platform_driver_register(&altera_gpio_driver);
+}
+subsys_initcall(altera_gpio_init);
+
+static void __exit altera_gpio_exit(void) {
+ platform_driver_unregister(&altera_gpio_driver);
+}
+module_exit(altera_gpio_exit);
+
+MODULE_DESCRIPTION("Altera GPIO driver"); MODULE_LICENSE("GPL");
--
1.7.11.GIT
Confidentiality Notice.
This message may contain information that is confidential or otherwise protected from disclosure. If you are not the intended recipient, you are hereby notified that any use, disclosure, dissemination, distribution, or copying of this message, or any attachments, is strictly prohibited. If you have received this message in error, please advise the sender by reply e-mail, and delete the message and any attachments. Thank you.
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver
2013-03-12 5:37 [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver thloh
2013-03-19 6:01 ` Tien Hock Loh
@ 2013-03-27 11:58 ` Linus Walleij
2013-03-28 11:40 ` Tien Hock Loh
1 sibling, 1 reply; 20+ messages in thread
From: Linus Walleij @ 2013-03-27 11:58 UTC (permalink / raw)
To: thloh, Rob Herring
Cc: Rob Landley, linux-doc, linux-kernel, devicetree-discuss
On Tue, Mar 12, 2013 at 6:37 AM, <thloh@altera.com> wrote:
> From: Tien Hock Loh <thloh@altera.com>
>
> Adds a new driver for Altera soft GPIO IP. The driver is able to
> do read/write and allows GPIO to be a interrupt controller.
>
> Tested on Altera GHRD on interrupt handling and IO.
>
> Signed-off-by: Tien Hock Loh <thloh@altera.com>
> ---
> .../devicetree/bindings/gpio/gpio-altera.txt | 37 +++
Patches containing device tree bindings shall be CC:ed to devicetree-discuss
(added on CC here, remember at next posting).
(...)
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-altera.txt b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
> new file mode 100644
> index 0000000..3bdb8b6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
> @@ -0,0 +1,37 @@
> +Altera GPIO controller bindings
> +
> +Required properties:
> +- compatible:
> + - "altr,pio-1.0"
> +- reg: Physical base address and length of the controller's registers.
> +- #gpio-cells : Should be two.
> + - first cell is the pin number
What does that mean? The GPIO subsystem is not referring
to pins, it's about "gpios" which are just a handler or number.
It can be a local offset into the numberspace representiong
the GPIO lines at this block, I guess that is what is meant
here.
(...)
> +Altera GPIO specific properties:
> +- width: Width of the GPIO bank, range from 1-32
> +- level_trigger: Specifies whether the GPIO interrupt is level trigger.
> + This field is required if the Altera GPIO controller used has IRQ enabled.
This is a configuration for the irc_chip portion of the driver
I guess, isn't the usual custom that you don't set up
things like this from the device tree, but instead use the
kernels facilities on request_irq() such as:
#define IRQF_TRIGGER_NONE 0x00000000
#define IRQF_TRIGGER_RISING 0x00000001
#define IRQF_TRIGGER_FALLING 0x00000002
#define IRQF_TRIGGER_HIGH 0x00000004
#define IRQF_TRIGGER_LOW 0x00000008
?
I.e. that this is something you do at runtime and not a static
config from the device tree?
Or shall this be percieved as some kind of default setting?
(...)
> +++ b/drivers/gpio/gpio-altera.c
> +struct altera_gpio_chip {
> + struct of_mm_gpio_chip mmchip;
> + struct irq_domain *irq; /* GPIO controller IRQ number */
> + spinlock_t gpio_lock; /* Lock used for synchronization */
> + int level_trigger;
I doubt this member of the struct. The irq core shall keep track
of stuff like this.
> + int hwirq;
> +};
(then follows some real nice code using irqdomain in a professional
way, thanks for doing this!)
> +static int altera_gpio_irq_set_type(struct irq_data *d,
> + unsigned int type)
> +{
> + if (type == IRQ_TYPE_NONE)
> + return 0;
> + return -EINVAL;
> +}
So if the chip supports setting edge vs level trigging,
this is where you implement it, not as device tree
properties.
> +static void altera_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> +{
> + struct altera_gpio_chip *altera_gc = irq_desc_get_handler_data(desc);
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
> + unsigned long status;
> +
> + int base;
> + chip->irq_mask(&desc->irq_data);
> +
> + if (altera_gc->level_trigger)
> + status = readl(mm_gc->regs + ALTERA_GPIO_DATA);
So on the level trigged variant you read the ALTERA_GPIO_DATA
and discard the result? Is it some kind of latch register?
> + else {
> + status = readl(mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
> + writel(status, mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
> + }
So you need to get the affected register depending on the type set
in the irq descriptor instead.
> + status &= readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
> + for (base = 0; base < mm_gc->gc.ngpio; base++) {
> + if ((1 << base) & status) {
> + generic_handle_irq(
> + irq_linear_revmap(altera_gc->irq, base));
> + }
> + }
When we looked at this for some IRQ controllers we realized we
had to write the loop like so:
while ((stat = readl_relaxed(vic->base + VIC_IRQ_STATUS))) {
(...)
}
Only to avoid missing transient IRQs occuring while we were
processing another IRQ. (Like, you handle IRQ0, then IRQ4,
and while you're handling IRQ4, IRQ0 triggers again and
you miss it.
Make sure this does not apply to you...
> + if (of_property_read_u32(node, "level_trigger", ®)) {
> + ret = -EINVAL;
> + pr_err("%s: level_trigger value not set in device tree\n",
> + node->full_name);
> + goto teardown;
> + }
> + altera_gc->level_trigger = reg;
So I'm suspicious about this.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver
2013-03-27 11:58 ` Linus Walleij
@ 2013-03-28 11:40 ` Tien Hock Loh
0 siblings, 0 replies; 20+ messages in thread
From: Tien Hock Loh @ 2013-03-28 11:40 UTC (permalink / raw)
To: Linus Walleij
Cc: Rob Herring, Rob Landley, linux-doc, linux-kernel, devicetree-discuss
On Wed, 2013-03-27 at 12:58 +0100, Linus Walleij wrote:
> On Tue, Mar 12, 2013 at 6:37 AM, <thloh@altera.com> wrote:
>
> > From: Tien Hock Loh <thloh@altera.com>
> >
> > Adds a new driver for Altera soft GPIO IP. The driver is able to
> > do read/write and allows GPIO to be a interrupt controller.
> >
> > Tested on Altera GHRD on interrupt handling and IO.
> >
> > Signed-off-by: Tien Hock Loh <thloh@altera.com>
> > ---
> > .../devicetree/bindings/gpio/gpio-altera.txt | 37 +++
>
> Patches containing device tree bindings shall be CC:ed to devicetree-discuss
> (added on CC here, remember at next posting).
Noted. Thanks for the info.
>
> (...)
> > diff --git a/Documentation/devicetree/bindings/gpio/gpio-altera.txt b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
> > new file mode 100644
> > index 0000000..3bdb8b6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
> > @@ -0,0 +1,37 @@
> > +Altera GPIO controller bindings
> > +
> > +Required properties:
> > +- compatible:
> > + - "altr,pio-1.0"
> > +- reg: Physical base address and length of the controller's registers.
> > +- #gpio-cells : Should be two.
> > + - first cell is the pin number
>
> What does that mean? The GPIO subsystem is not referring
> to pins, it's about "gpios" which are just a handler or number.
>
> It can be a local offset into the numberspace representiong
> the GPIO lines at this block, I guess that is what is meant
> here.
Yes that's what I was trying to describe. I'll fix this in the next
patch.
>
> (...)
> > +Altera GPIO specific properties:
> > +- width: Width of the GPIO bank, range from 1-32
> > +- level_trigger: Specifies whether the GPIO interrupt is level trigger.
> > + This field is required if the Altera GPIO controller used has IRQ enabled.
>
> This is a configuration for the irc_chip portion of the driver
> I guess, isn't the usual custom that you don't set up
> things like this from the device tree, but instead use the
> kernels facilities on request_irq() such as:
>
> #define IRQF_TRIGGER_NONE 0x00000000
> #define IRQF_TRIGGER_RISING 0x00000001
> #define IRQF_TRIGGER_FALLING 0x00000002
> #define IRQF_TRIGGER_HIGH 0x00000004
> #define IRQF_TRIGGER_LOW 0x00000008
>
> ?
>
> I.e. that this is something you do at runtime and not a static
> config from the device tree?
>
> Or shall this be percieved as some kind of default setting?
The Altera GPIO IP cannot be software configurable. It is determined by
the tool that generates the hardware, and thus I've put this as device
tree parameter. If I understand correctly, if we implement using
irq_set_type, it should be software configurable.
Technically it can still be implemented in irq_set_type, I'm just not
sure if it is the correct way, because the trigger type cannot be "set"
in Altera GPIO. Please let me know if you think this should still be
implemented as irq_set_type.
>
> (...)
> > +++ b/drivers/gpio/gpio-altera.c
>
> > +struct altera_gpio_chip {
> > + struct of_mm_gpio_chip mmchip;
> > + struct irq_domain *irq; /* GPIO controller IRQ number */
> > + spinlock_t gpio_lock; /* Lock used for synchronization */
> > + int level_trigger;
>
> I doubt this member of the struct. The irq core shall keep track
> of stuff like this.
>
We shouldn't be adding anything to the gpio chip struct and has to
follow exactly how the irq core defines it?
> > + int hwirq;
> > +};
>
> (then follows some real nice code using irqdomain in a professional
> way, thanks for doing this!)
>
> > +static int altera_gpio_irq_set_type(struct irq_data *d,
> > + unsigned int type)
> > +{
> > + if (type == IRQ_TYPE_NONE)
> > + return 0;
> > + return -EINVAL;
> > +}
>
> So if the chip supports setting edge vs level trigging,
> this is where you implement it, not as device tree
> properties.
Discussed above
>
> > +static void altera_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> > +{
> > + struct altera_gpio_chip *altera_gc = irq_desc_get_handler_data(desc);
> > + struct irq_chip *chip = irq_desc_get_chip(desc);
> > + struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
> > + unsigned long status;
> > +
> > + int base;
> > + chip->irq_mask(&desc->irq_data);
> > +
> > + if (altera_gc->level_trigger)
> > + status = readl(mm_gc->regs + ALTERA_GPIO_DATA);
>
> So on the level trigged variant you read the ALTERA_GPIO_DATA
> and discard the result? Is it some kind of latch register?
No, the status isn't discarded. It will be & with the peripheral's
interrupt mask to get the interrupts that needs to be handled.
>
>
> > + else {
> > + status = readl(mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
> > + writel(status, mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
> > + }
>
> So you need to get the affected register depending on the type set
> in the irq descriptor instead.
Yes you are correct.
>
> > + status &= readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
> > + for (base = 0; base < mm_gc->gc.ngpio; base++) {
> > + if ((1 << base) & status) {
> > + generic_handle_irq(
> > + irq_linear_revmap(altera_gc->irq, base));
> > + }
> > + }
>
> When we looked at this for some IRQ controllers we realized we
> had to write the loop like so:
>
> while ((stat = readl_relaxed(vic->base + VIC_IRQ_STATUS))) {
> (...)
> }
>
> Only to avoid missing transient IRQs occuring while we were
> processing another IRQ. (Like, you handle IRQ0, then IRQ4,
> and while you're handling IRQ4, IRQ0 triggers again and
> you miss it.
>
> Make sure this does not apply to you...
Noted. Thanks for the info. I'll look into this.
>
> > + if (of_property_read_u32(node, "level_trigger", ®)) {
> > + ret = -EINVAL;
> > + pr_err("%s: level_trigger value not set in device tree\n",
> > + node->full_name);
> > + goto teardown;
> > + }
> > + altera_gc->level_trigger = reg;
>
> So I'm suspicious about this.
Discussed above.
>
> Yours,
> Linus Walleij
>
Thanks.
Tien Hock Loh
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver
@ 2013-03-28 11:40 ` Tien Hock Loh
0 siblings, 0 replies; 20+ messages in thread
From: Tien Hock Loh @ 2013-03-28 11:40 UTC (permalink / raw)
To: Linus Walleij
Cc: Rob Herring, Rob Landley, linux-doc, linux-kernel, devicetree-discuss
On Wed, 2013-03-27 at 12:58 +0100, Linus Walleij wrote:
> On Tue, Mar 12, 2013 at 6:37 AM, <thloh@altera.com> wrote:
>
> > From: Tien Hock Loh <thloh@altera.com>
> >
> > Adds a new driver for Altera soft GPIO IP. The driver is able to
> > do read/write and allows GPIO to be a interrupt controller.
> >
> > Tested on Altera GHRD on interrupt handling and IO.
> >
> > Signed-off-by: Tien Hock Loh <thloh@altera.com>
> > ---
> > .../devicetree/bindings/gpio/gpio-altera.txt | 37 +++
>
> Patches containing device tree bindings shall be CC:ed to devicetree-discuss
> (added on CC here, remember at next posting).
Noted. Thanks for the info.
>
> (...)
> > diff --git a/Documentation/devicetree/bindings/gpio/gpio-altera.txt b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
> > new file mode 100644
> > index 0000000..3bdb8b6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
> > @@ -0,0 +1,37 @@
> > +Altera GPIO controller bindings
> > +
> > +Required properties:
> > +- compatible:
> > + - "altr,pio-1.0"
> > +- reg: Physical base address and length of the controller's registers.
> > +- #gpio-cells : Should be two.
> > + - first cell is the pin number
>
> What does that mean? The GPIO subsystem is not referring
> to pins, it's about "gpios" which are just a handler or number.
>
> It can be a local offset into the numberspace representiong
> the GPIO lines at this block, I guess that is what is meant
> here.
Yes that's what I was trying to describe. I'll fix this in the next
patch.
>
> (...)
> > +Altera GPIO specific properties:
> > +- width: Width of the GPIO bank, range from 1-32
> > +- level_trigger: Specifies whether the GPIO interrupt is level trigger.
> > + This field is required if the Altera GPIO controller used has IRQ enabled.
>
> This is a configuration for the irc_chip portion of the driver
> I guess, isn't the usual custom that you don't set up
> things like this from the device tree, but instead use the
> kernels facilities on request_irq() such as:
>
> #define IRQF_TRIGGER_NONE 0x00000000
> #define IRQF_TRIGGER_RISING 0x00000001
> #define IRQF_TRIGGER_FALLING 0x00000002
> #define IRQF_TRIGGER_HIGH 0x00000004
> #define IRQF_TRIGGER_LOW 0x00000008
>
> ?
>
> I.e. that this is something you do at runtime and not a static
> config from the device tree?
>
> Or shall this be percieved as some kind of default setting?
The Altera GPIO IP cannot be software configurable. It is determined by
the tool that generates the hardware, and thus I've put this as device
tree parameter. If I understand correctly, if we implement using
irq_set_type, it should be software configurable.
Technically it can still be implemented in irq_set_type, I'm just not
sure if it is the correct way, because the trigger type cannot be "set"
in Altera GPIO. Please let me know if you think this should still be
implemented as irq_set_type.
>
> (...)
> > +++ b/drivers/gpio/gpio-altera.c
>
> > +struct altera_gpio_chip {
> > + struct of_mm_gpio_chip mmchip;
> > + struct irq_domain *irq; /* GPIO controller IRQ number */
> > + spinlock_t gpio_lock; /* Lock used for synchronization */
> > + int level_trigger;
>
> I doubt this member of the struct. The irq core shall keep track
> of stuff like this.
>
We shouldn't be adding anything to the gpio chip struct and has to
follow exactly how the irq core defines it?
> > + int hwirq;
> > +};
>
> (then follows some real nice code using irqdomain in a professional
> way, thanks for doing this!)
>
> > +static int altera_gpio_irq_set_type(struct irq_data *d,
> > + unsigned int type)
> > +{
> > + if (type == IRQ_TYPE_NONE)
> > + return 0;
> > + return -EINVAL;
> > +}
>
> So if the chip supports setting edge vs level trigging,
> this is where you implement it, not as device tree
> properties.
Discussed above
>
> > +static void altera_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> > +{
> > + struct altera_gpio_chip *altera_gc = irq_desc_get_handler_data(desc);
> > + struct irq_chip *chip = irq_desc_get_chip(desc);
> > + struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
> > + unsigned long status;
> > +
> > + int base;
> > + chip->irq_mask(&desc->irq_data);
> > +
> > + if (altera_gc->level_trigger)
> > + status = readl(mm_gc->regs + ALTERA_GPIO_DATA);
>
> So on the level trigged variant you read the ALTERA_GPIO_DATA
> and discard the result? Is it some kind of latch register?
No, the status isn't discarded. It will be & with the peripheral's
interrupt mask to get the interrupts that needs to be handled.
>
>
> > + else {
> > + status = readl(mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
> > + writel(status, mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
> > + }
>
> So you need to get the affected register depending on the type set
> in the irq descriptor instead.
Yes you are correct.
>
> > + status &= readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
> > + for (base = 0; base < mm_gc->gc.ngpio; base++) {
> > + if ((1 << base) & status) {
> > + generic_handle_irq(
> > + irq_linear_revmap(altera_gc->irq, base));
> > + }
> > + }
>
> When we looked at this for some IRQ controllers we realized we
> had to write the loop like so:
>
> while ((stat = readl_relaxed(vic->base + VIC_IRQ_STATUS))) {
> (...)
> }
>
> Only to avoid missing transient IRQs occuring while we were
> processing another IRQ. (Like, you handle IRQ0, then IRQ4,
> and while you're handling IRQ4, IRQ0 triggers again and
> you miss it.
>
> Make sure this does not apply to you...
Noted. Thanks for the info. I'll look into this.
>
> > + if (of_property_read_u32(node, "level_trigger", ®)) {
> > + ret = -EINVAL;
> > + pr_err("%s: level_trigger value not set in device tree\n",
> > + node->full_name);
> > + goto teardown;
> > + }
> > + altera_gc->level_trigger = reg;
>
> So I'm suspicious about this.
Discussed above.
>
> Yours,
> Linus Walleij
>
Thanks.
Tien Hock Loh
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver
2013-03-28 11:40 ` Tien Hock Loh
(?)
@ 2013-04-02 8:55 ` Linus Walleij
-1 siblings, 0 replies; 20+ messages in thread
From: Linus Walleij @ 2013-04-02 8:55 UTC (permalink / raw)
To: Tien Hock Loh
Cc: Rob Herring, Rob Landley, linux-doc, linux-kernel, devicetree-discuss
On Thu, Mar 28, 2013 at 12:40 PM, Tien Hock Loh <thloh@altera.com> wrote:
>> (...)
>> > +Altera GPIO specific properties:
>> > +- width: Width of the GPIO bank, range from 1-32
>> > +- level_trigger: Specifies whether the GPIO interrupt is level trigger.
>> > + This field is required if the Altera GPIO controller used has IRQ enabled.
>>
>> This is a configuration for the irc_chip portion of the driver
>> I guess, isn't the usual custom that you don't set up
>> things like this from the device tree, but instead use the
>> kernels facilities on request_irq() such as:
>>
>> #define IRQF_TRIGGER_NONE 0x00000000
>> #define IRQF_TRIGGER_RISING 0x00000001
>> #define IRQF_TRIGGER_FALLING 0x00000002
>> #define IRQF_TRIGGER_HIGH 0x00000004
>> #define IRQF_TRIGGER_LOW 0x00000008
>>
>> ?
>>
>> I.e. that this is something you do at runtime and not a static
>> config from the device tree?
>>
>> Or shall this be percieved as some kind of default setting?
>
> The Altera GPIO IP cannot be software configurable. It is determined by
> the tool that generates the hardware, and thus I've put this as device
> tree parameter. If I understand correctly, if we implement using
> irq_set_type, it should be software configurable.
>
> Technically it can still be implemented in irq_set_type, I'm just not
> sure if it is the correct way, because the trigger type cannot be "set"
> in Altera GPIO. Please let me know if you think this should still be
> implemented as irq_set_type.
Hm I must be getting things wrong. It's correct that the driver
needs to keep track of if a certain IRQ line has a certain
trigger characteristic. So the member in this struct:
+struct altera_gpio_chip {
+ struct of_mm_gpio_chip mmchip;
+ struct irq_domain *irq; /* GPIO controller IRQ number */
+ spinlock_t gpio_lock; /* Lock used for synchronization */
+ int level_trigger;
Is OK. But it should definately be a bool instead.
Then the irq_set_type() should check if the requested type is available.
You state that some interrupts are edge triggered, so this is
clearly wrong:
+static int altera_gpio_irq_set_type(struct irq_data *d,
+ unsigned int type)
+{
+ if (type == IRQ_TYPE_NONE)
+ return 0;
+ return -EINVAL;
+}
I think it should rather be something like:
static int altera_gpio_irq_set_type(struct irq_data *d,
unsigned int type)
{
struct altera_gpio_chip *a = irq_data_get_irq_chip_data(d);
if (a->level_trigger) {
if (type == IRQ_TYPE_LEVEL_HIGH)
/* Configure the IRQ for high level */
else if (type == IRQ_TYPE_LEVEL_LOW)
/* Configure the IRQ for low level */
else
return -EINVAL;
} else {
/* assume it's edge triggered? */
if (type == IRQ_TYPE_EDGE_RISING)
/* Configure for rising edge */
else if (type == IRQ_TYPE_EDGE_FALLING)
/* Configure for falling edge */
else
return -EINVAL;
}
}
As you can see from above a few new questions arise.
When you say an IRQ is "level trigged" does it actually mean
it is high or low level triggered? Or is this somehow configurable?
If it is *not* edge triggered, is it then implicitly edge
triggered?
If it is then implicitly edge triggered, does it trigger on
rising or falling edges or is this configurable?
As you see, the .irq_set_type needs to reflect what the
hardware can actually react to, and return errors on the
rest.
The current implementation which states that the hardware
cannot react to any IRQ is clearly wrong.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver
2013-07-29 16:24 ` Loh Tien Hock
@ 2013-08-16 13:12 ` Linus Walleij
0 siblings, 0 replies; 20+ messages in thread
From: Linus Walleij @ 2013-08-16 13:12 UTC (permalink / raw)
To: Loh Tien Hock
Cc: Tien Hock Loh, devicetree, Rob Herring, Rob Landley, linux-doc,
devicetree-discuss
On Mon, Jul 29, 2013 at 6:24 PM, Loh Tien Hock <thloh@altera.com> wrote:
> On Tue, Jul 30, 2013 at 12:11 AM, Linus Walleij
>>>>> +static int altera_gpio_irq_set_type(struct irq_data *d,
>>>>> + unsigned int type)
>>>>> +{
>>>>> + struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
>>>>> +
>>>>> + if (type == IRQ_TYPE_NONE)
>>>>> + return 0;
>>>>> +
>>>>> + if (altera_gc->level_trigger) {
>>>>> + if (type == IRQ_TYPE_LEVEL_HIGH)
>>>>> + return 0;
>>>>> + } else {
>>>>> + if (type == IRQ_TYPE_EDGE_RISING &&
>>>>> + altera_gc->edge_type == ALTERA_IRQ_RISING)
>>>>> + return 0;
>>>>> + else if (type == IRQ_TYPE_EDGE_FALLING &&
>>>>> + altera_gc->edge_type == ALTERA_IRQ_FALLING)
>>>>> + return 0;
>>>>> + else if (type == IRQ_TYPE_EDGE_BOTH &&
>>>>> + altera_gc->edge_type == ALTERA_IRQ_BOTH)
>>>>> + return 0;
>>>>
>>>> I don't quite realize why you need the local version of
>>>> the IRQflag at all. Just store the standard edge indicator
>>>> in an unsigned long?
>>>
>>> I don't understand the comment. Can you elaborate further?
>>
>> I misunderstood it for the above reason. But get rid of the
>> custom ALTERA_IRQ_* defines and just use the standard
>> definitions for IRQ_TYPE_*, the local defines does not add
>> anything useful.
>
> We still need to compare the altera_gc->edge_type to the expected ones
> (so that we don't allow incorrect configuration, eg. the GPIO is
> configured as rising, but set_type tries to set falling).
> So we should compare it with magic numbers, instead of custom defines
> like the example below?
> if (type == IRQ_TYPE_EDGE_RISING &&
> altera_gc->edge_type == 0)
> return 0;
And why can't altera_gc->edge_type use exactly the same enumerators
so it becomes:
(type == IRQ_TYPE_EDGE_RISING &&
altera_gc->edge_type == IRQ_TYPE_EDGE_RISING)
?
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver
2013-07-29 16:11 ` Linus Walleij
@ 2013-07-29 16:24 ` Loh Tien Hock
2013-08-16 13:12 ` Linus Walleij
0 siblings, 1 reply; 20+ messages in thread
From: Loh Tien Hock @ 2013-07-29 16:24 UTC (permalink / raw)
To: Linus Walleij
Cc: Tien Hock Loh, devicetree, Rob Herring, Rob Landley, linux-doc,
devicetree-discuss
On Tue, Jul 30, 2013 at 12:11 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Tue, Jul 23, 2013 at 4:51 AM, Loh Tien Hock <thloh85@gmail.com> wrote:
>> On Tue, Jul 23, 2013 at 3:07 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>
>>> On Mon, Jul 8, 2013 at 9:04 AM, <thloh.linux@gmail.com> wrote:
>>>> +- level_trigger: Specifies whether the GPIO interrupt is level trigger (high).
>>>> + This field is required if the Altera GPIO controller used has IRQ enabled.
>>>> +- edge_type: Specifies edge type if the GPIO interrupt is edge trigger:
>>>> + 0 = Rising edge
>>>> + 1 = Falling edge
>>>> + 2 = Both edge
>>>
>>> No thanks.
>>
>> I don't understand the comment here. Could you elaborate further?
>> The edge_type is required as the soft IP's interrupt type isn't
>> software configurable, and thus is a hardware parameter passed in from
>> device tree blob.
>
> Aha I see. Explain this in the binding document, i.e. state that
> this is a synthesis parameter in the hardware block and not
> software controlled.
>
>>>> +static int altera_gpio_irq_set_type(struct irq_data *d,
>>>> + unsigned int type)
>>>> +{
>>>> + struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
>>>> +
>>>> + if (type == IRQ_TYPE_NONE)
>>>> + return 0;
>>>> +
>>>> + if (altera_gc->level_trigger) {
>>>> + if (type == IRQ_TYPE_LEVEL_HIGH)
>>>> + return 0;
>>>> + } else {
>>>> + if (type == IRQ_TYPE_EDGE_RISING &&
>>>> + altera_gc->edge_type == ALTERA_IRQ_RISING)
>>>> + return 0;
>>>> + else if (type == IRQ_TYPE_EDGE_FALLING &&
>>>> + altera_gc->edge_type == ALTERA_IRQ_FALLING)
>>>> + return 0;
>>>> + else if (type == IRQ_TYPE_EDGE_BOTH &&
>>>> + altera_gc->edge_type == ALTERA_IRQ_BOTH)
>>>> + return 0;
>>>
>>> I don't quite realize why you need the local version of
>>> the IRQflag at all. Just store the standard edge indicator
>>> in an unsigned long?
>>
>> I don't understand the comment. Can you elaborate further?
>
> I misunderstood it for the above reason. But get rid of the
> custom ALTERA_IRQ_* defines and just use the standard
> definitions for IRQ_TYPE_*, the local defines does not add
> anything useful.
>
We still need to compare the altera_gc->edge_type to the expected ones
(so that we don't allow incorrect configuration, eg. the GPIO is
configured as rising, but set_type tries to set falling).
So we should compare it with magic numbers, instead of custom defines
like the example below?
if (type == IRQ_TYPE_EDGE_RISING &&
altera_gc->edge_type == 0)
return 0;
>>>> + chip->irq_mask(&desc->irq_data);
>>>> +
>>>> + if (altera_gc->level_trigger) {
>>>> + status = readl_relaxed(mm_gc->regs + ALTERA_GPIO_DATA);
>>>> + status &= readl_relaxed(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
>>>> +
>>>> + for (base = 0; base < mm_gc->gc.ngpio; base++) {
>>>> + if (BIT(base) & status) {
>>>> + generic_handle_irq(irq_linear_revmap(
>>>> + altera_gc->irq, base));
>>>> + }
>>>> + }
>>>> + } else {
>>>> + while ((status =
>>>> + (readl_relaxed(mm_gc->regs + ALTERA_GPIO_EDGE_CAP) &
>>>> + readl_relaxed(mm_gc->regs + ALTERA_GPIO_IRQ_MASK)))) {
>>>> + writel_relaxed(status,
>>>> + mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
>>>
>>> Nice use if relaxed accessors in the irq handler!
>>
>> I don't understand the comment. Could you elaborate further?
>
> I am just saying that you are doing the right thing, it is a positive
> comment :-)
Oh. Thanks.
>
> Yours,
> Linus Walleij
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver
2013-07-23 2:51 ` Loh Tien Hock
@ 2013-07-29 16:11 ` Linus Walleij
2013-07-29 16:24 ` Loh Tien Hock
0 siblings, 1 reply; 20+ messages in thread
From: Linus Walleij @ 2013-07-29 16:11 UTC (permalink / raw)
To: Loh Tien Hock
Cc: Tien Hock Loh, devicetree, Rob Herring, Rob Landley, linux-doc,
devicetree-discuss
On Tue, Jul 23, 2013 at 4:51 AM, Loh Tien Hock <thloh85@gmail.com> wrote:
> On Tue, Jul 23, 2013 at 3:07 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Mon, Jul 8, 2013 at 9:04 AM, <thloh.linux@gmail.com> wrote:
>>> +- level_trigger: Specifies whether the GPIO interrupt is level trigger (high).
>>> + This field is required if the Altera GPIO controller used has IRQ enabled.
>>> +- edge_type: Specifies edge type if the GPIO interrupt is edge trigger:
>>> + 0 = Rising edge
>>> + 1 = Falling edge
>>> + 2 = Both edge
>>
>> No thanks.
>
> I don't understand the comment here. Could you elaborate further?
> The edge_type is required as the soft IP's interrupt type isn't
> software configurable, and thus is a hardware parameter passed in from
> device tree blob.
Aha I see. Explain this in the binding document, i.e. state that
this is a synthesis parameter in the hardware block and not
software controlled.
>>> +static int altera_gpio_irq_set_type(struct irq_data *d,
>>> + unsigned int type)
>>> +{
>>> + struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
>>> +
>>> + if (type == IRQ_TYPE_NONE)
>>> + return 0;
>>> +
>>> + if (altera_gc->level_trigger) {
>>> + if (type == IRQ_TYPE_LEVEL_HIGH)
>>> + return 0;
>>> + } else {
>>> + if (type == IRQ_TYPE_EDGE_RISING &&
>>> + altera_gc->edge_type == ALTERA_IRQ_RISING)
>>> + return 0;
>>> + else if (type == IRQ_TYPE_EDGE_FALLING &&
>>> + altera_gc->edge_type == ALTERA_IRQ_FALLING)
>>> + return 0;
>>> + else if (type == IRQ_TYPE_EDGE_BOTH &&
>>> + altera_gc->edge_type == ALTERA_IRQ_BOTH)
>>> + return 0;
>>
>> I don't quite realize why you need the local version of
>> the IRQflag at all. Just store the standard edge indicator
>> in an unsigned long?
>
> I don't understand the comment. Can you elaborate further?
I misunderstood it for the above reason. But get rid of the
custom ALTERA_IRQ_* defines and just use the standard
definitions for IRQ_TYPE_*, the local defines does not add
anything useful.
>>> + chip->irq_mask(&desc->irq_data);
>>> +
>>> + if (altera_gc->level_trigger) {
>>> + status = readl_relaxed(mm_gc->regs + ALTERA_GPIO_DATA);
>>> + status &= readl_relaxed(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
>>> +
>>> + for (base = 0; base < mm_gc->gc.ngpio; base++) {
>>> + if (BIT(base) & status) {
>>> + generic_handle_irq(irq_linear_revmap(
>>> + altera_gc->irq, base));
>>> + }
>>> + }
>>> + } else {
>>> + while ((status =
>>> + (readl_relaxed(mm_gc->regs + ALTERA_GPIO_EDGE_CAP) &
>>> + readl_relaxed(mm_gc->regs + ALTERA_GPIO_IRQ_MASK)))) {
>>> + writel_relaxed(status,
>>> + mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
>>
>> Nice use if relaxed accessors in the irq handler!
>
> I don't understand the comment. Could you elaborate further?
I am just saying that you are doing the right thing, it is a positive
comment :-)
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver
2013-07-22 19:07 ` Linus Walleij
@ 2013-07-23 2:51 ` Loh Tien Hock
2013-07-29 16:11 ` Linus Walleij
0 siblings, 1 reply; 20+ messages in thread
From: Loh Tien Hock @ 2013-07-23 2:51 UTC (permalink / raw)
To: Linus Walleij
Cc: thloh.linux, devicetree, Rob Herring, Rob Landley, linux-doc,
devicetree-discuss
On Tue, Jul 23, 2013 at 3:07 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Jul 8, 2013 at 9:04 AM, <thloh.linux@gmail.com> wrote:
>
>> From: Tien Hock Loh <thloh@altera.com>
>>
>> Add driver support for Altera GPIO soft IP, including interrupts and I/O.
>> Tested on Altera CV SoC board using dipsw and LED using LED framework.
>>
>> Signed-off-by: Tien Hock Loh <thloh@altera.com>
>
> Note that the new mailing list for devicetree patches is
> devicetree@vger.kernel.org nowadays.
>
>> +Altera GPIO specific properties:
>> +- width: Width of the GPIO bank, range from 1-32
>
> Can it really vary like that? 1, 2, 3, ...
Yes, any number within the range of 32.
>
> Can there be "holes" in this range then, so you might prefer
> a bitmask over a numeral?
No, there is no "holes" in the Altera GPIO soft IP.
>
>> +- level_trigger: Specifies whether the GPIO interrupt is level trigger (high).
>> + This field is required if the Altera GPIO controller used has IRQ enabled.
>> +- edge_type: Specifies edge type if the GPIO interrupt is edge trigger:
>> + 0 = Rising edge
>> + 1 = Falling edge
>> + 2 = Both edge
>
> No thanks.
I don't understand the comment here. Could you elaborate further?
The edge_type is required as the soft IP's interrupt type isn't
software configurable, and thus is a hardware parameter passed in from
device tree blob.
>
> #include <dt-bindings/interrupt-controller/irq.h>
> and define these things per-irq by using more interrupt-cells.
>
>> +Example:
>> +
>> +gpio_altr: gpio_altr {
>> + compatible = "altr,pio-1.0";
>> + reg = <0xff200000 0x10>;
>> + interrupts = <0 45 4>;
>> + width = <32>;
>> + level_trigger = <0>;
>> + #gpio-cells = <1>;
>> + gpio-controller;
>> + #interrupt-cells = <1>;
>
> This needs to be 2 or something so you can pass the edge specifics
> for each line.
OK noted.
>
>> +config GPIO_ALTERA
>> + tristate "Altera GPIO"
>
> Do you *really* load this as a module sometimes?
Yes, as this peripheral is a soft IP resides in FPGA, and the FPGA may
not be up and running by the time the system boots, and thus in most
cases they should be loaded as module instead of a built in driver.
>
>> +++ b/drivers/gpio/gpio-altera.c
>
>> +#include <linux/gpio.h>
>> +#include <linux/kernel.h>
>> +#include <linux/slab.h>
>> +#include <linux/module.h>
>> +#include <linux/io.h>
>> +#include <linux/irq.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/platform_device.h>
>
> #include <linux/bitops.h>
>
> BIT() comes from this file.
Noted.
>
>> +
>> +#define ALTERA_GPIO_DATA 0x0
>> +#define ALTERA_GPIO_DIR 0x4
>> +#define ALTERA_GPIO_IRQ_MASK 0x8
>> +#define ALTERA_GPIO_EDGE_CAP 0xc
>> +#define ALTERA_GPIO_OUTSET 0x10
>> +#define ALTERA_GPIO_OUTCLEAR 0x14
>> +#define ALTERA_IRQ_RISING 0
>> +#define ALTERA_IRQ_FALLING 1
>> +#define ALTERA_IRQ_BOTH 2
>
> Why do you need these local redefinitions of rising/falling/both?
> Use the defines from the subsystem.
The Altera tool that generates the device tree uses the number that is
not the same as the subsystem ones (notice ALTERA_IRQ_RISING is 0, but
IRQ_TYPE_EDGE_RISING is 1), thus these defines are required. Changes
to the tool will break existing compatibility.
>
>> +
>> +/**
>> +* struct altera_gpio_chip
>> +* @mmchip : memory mapped chip structure.
>> +* @irq : irq domain that this driver is registered to.
>
> This is confusing. Rename this to "irqdomain" instead.
Noted.
>
>> +* @gpio_lock : synchronization lock so that new irq/set/get requests
>> + will be blocked until the current one completes.
>> +* @level_trigger : specifies if the controller is level triggered
>> + (high).
>> +* @edge_type : specify the edge type of the controller (rising,
>> + falling, both). Only used for !level_trigger.
>> +* @mapped_irq : kernel mapped irq number.
>> +*/
>> +struct altera_gpio_chip {
>> + struct of_mm_gpio_chip mmchip;
>> + struct irq_domain *irq;
>> + spinlock_t gpio_lock;
>> + int level_trigger;
>> + int edge_type;
>> + int mapped_irq;
>
> And name this "irq".
OK noted.
>
>> +};
>> +
>> +static void altera_gpio_irq_unmask(struct irq_data *d)
>> +{
>> + struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
>> + struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
>> + unsigned long flags;
>> + unsigned int intmask;
>
> Use u32 for this variable.
Noted.
>
>> +
>> + spin_lock_irqsave(&altera_gc->gpio_lock, flags);
>> + intmask = readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
>> + /* Set ALTERA_GPIO_IRQ_MASK bit to unmask */
>> + intmask |= BIT(irqd_to_hwirq(d));
>> + writel(intmask, mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
>> + spin_unlock_irqrestore(&altera_gc->gpio_lock, flags);
>> +}
>> +
>> +static void altera_gpio_irq_mask(struct irq_data *d)
>> +{
>> + struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
>> + struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
>> + unsigned long flags;
>> + unsigned int intmask;
>
> u32
Noted.
>
>> +
>> + spin_lock_irqsave(&altera_gc->gpio_lock, flags);
>> + intmask = readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
>> + /* Clear ALTERA_GPIO_IRQ_MASK bit to mask */
>> + intmask &= BIT(irqd_to_hwirq(d));
>> + writel(intmask, mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
>> + spin_unlock_irqrestore(&altera_gc->gpio_lock, flags);
>> +}
>> +
>> +static int altera_gpio_irq_set_type(struct irq_data *d,
>> + unsigned int type)
>> +{
>> + struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
>> +
>> + if (type == IRQ_TYPE_NONE)
>> + return 0;
>> +
>> + if (altera_gc->level_trigger) {
>> + if (type == IRQ_TYPE_LEVEL_HIGH)
>> + return 0;
>> + } else {
>> + if (type == IRQ_TYPE_EDGE_RISING &&
>> + altera_gc->edge_type == ALTERA_IRQ_RISING)
>> + return 0;
>> + else if (type == IRQ_TYPE_EDGE_FALLING &&
>> + altera_gc->edge_type == ALTERA_IRQ_FALLING)
>> + return 0;
>> + else if (type == IRQ_TYPE_EDGE_BOTH &&
>> + altera_gc->edge_type == ALTERA_IRQ_BOTH)
>> + return 0;
>
> I don't quite realize why you need the local version of
> the IRQflag at all. Just store the standard edge indicator
> in an unsigned long?
I don't understand the comment. Can you elaborate further?
>
> Also: if you have a property in the device tree setting all IRQs
> to one or the other type (as the weird binding is now) why
> are you still supporting all these variants on each and every
> IRQ?
>
This is actually to handle scenarios like when drivers (like gpio-led)
wishes to "set" the software configurable parameters (usually exist in
general GPIO), it needs to check if the IP is configured the exact
same configuration requested and return the status correctly (ie. if
the soft IP is set as edge falling interrupt, and the gpio-led
requests edge rising, this will return non zero and thus the gpio-led
will know this IP failed to be configured with the requested interrupt
type).
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static struct irq_chip altera_irq_chip = {
>> + .name = "altera-gpio",
>> + .irq_mask = altera_gpio_irq_mask,
>> + .irq_unmask = altera_gpio_irq_unmask,
>> + .irq_set_type = altera_gpio_irq_set_type,
>> +};
>> +
>> +static int altera_gpio_get(struct gpio_chip *gc, unsigned offset)
>> +{
>> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
>> +
>> + return (readl(mm_gc->regs + ALTERA_GPIO_DATA) >> offset) & 1;
>
> Pls write it like this:
> return !!(readl(mm_gc->regs + ALTERA_GPIO_DATA) & BIT(offset)));
OK noted.
>
>> +}
>> +
>> +static void altera_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
>> +{
>> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
>> + struct altera_gpio_chip *chip = container_of(mm_gc,
>> + struct altera_gpio_chip, mmchip);
>> + unsigned long flags;
>> + unsigned int data_reg;
>
> u32
Noted.
>
>> +
>> + spin_lock_irqsave(&chip->gpio_lock, flags);
>> + data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA);
>> + data_reg = (data_reg & ~BIT(offset)) | (value << offset);
>
> Value is always 0 or 1.
>
> I would prefer this idom even though its more lines:
>
> if (val)
> data_reg |= BIT(offset);
> else
> data_reg &= ~BIT(offset);
>
Noted.
>> + writel(data_reg, mm_gc->regs + ALTERA_GPIO_DATA);
>> + spin_unlock_irqrestore(&chip->gpio_lock, flags);
>> +}
>> +
>> +static int altera_gpio_direction_input(struct gpio_chip *gc, unsigned offset)
>> +{
>> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
>> + struct altera_gpio_chip *chip = container_of(mm_gc,
>> + struct altera_gpio_chip, mmchip);
>> + unsigned long flags;
>> + unsigned int gpio_ddr;
>
> u32
Noted.
>
>> +
>> + spin_lock_irqsave(&chip->gpio_lock, flags);
>> + /* Set pin as input, assumes software controlled IP */
>> + gpio_ddr = readl(mm_gc->regs + ALTERA_GPIO_DIR);
>> + gpio_ddr &= ~BIT(offset);
>> + writel(gpio_ddr, mm_gc->regs + ALTERA_GPIO_DIR);
>> + spin_unlock_irqrestore(&chip->gpio_lock, flags);
>> +
>> + return 0;
>> +}
>> +
>> +static int altera_gpio_direction_output(struct gpio_chip *gc,
>> + unsigned offset, int value)
>> +{
>> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
>> + struct altera_gpio_chip *chip = container_of(mm_gc,
>> + struct altera_gpio_chip, mmchip);
>> + unsigned long flags;
>> + unsigned int data_reg, gpio_ddr;
>
> u32
Noted
>
>> +
>> + spin_lock_irqsave(&chip->gpio_lock, flags);
>> + /* Sets the GPIO value */
>> + data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA);
>> + data_reg = (data_reg & ~BIT(offset)) | (value << offset);
>
> Use same construct as indicated above.
Noted.
>
>> + writel(data_reg, mm_gc->regs + ALTERA_GPIO_DATA);
>> +
>> + /* Set pin as output, assumes software controlled IP */
>> + gpio_ddr = readl(mm_gc->regs + ALTERA_GPIO_DIR);
>> + gpio_ddr |= BIT(offset);
>> + writel(gpio_ddr, mm_gc->regs + ALTERA_GPIO_DIR);
>> + spin_unlock_irqrestore(&chip->gpio_lock, flags);
>> +
>> + return 0;
>> +}
>> +
>> +static int altera_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
>> +{
>> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
>> + struct altera_gpio_chip *altera_gc = container_of(mm_gc,
>> + struct altera_gpio_chip, mmchip);
>> +
>> + if (altera_gc->irq == 0)
>> + return -ENXIO;
>> + if ((altera_gc->irq && offset) < altera_gc->mmchip.gc.ngpio)
>
> I don't understand this. Why are you making a binary and &&
> with the offset and then checking if that is less that the number
> of GPIO?
I'll look into this. Thanks for catching this.
>
>> + return irq_create_mapping(altera_gc->irq, offset);
>> + else
>> + return -ENXIO;
>> +}
>> +
>> +static void altera_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>> +{
>> + struct altera_gpio_chip *altera_gc = irq_desc_get_handler_data(desc);
>> + struct irq_chip *chip = irq_desc_get_chip(desc);
>> + struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
>> + unsigned long status;
>
> u32
Noted.
>
>> +
>> + int base;
>
> Is this an int really? It looks like u8 would suffice.
> Why is this named "base"? Can you give it a more self-explanatory
> name? Else just use "i".
OK noted.
>
>> + chip->irq_mask(&desc->irq_data);
>> +
>> + if (altera_gc->level_trigger) {
>> + status = readl_relaxed(mm_gc->regs + ALTERA_GPIO_DATA);
>> + status &= readl_relaxed(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
>> +
>> + for (base = 0; base < mm_gc->gc.ngpio; base++) {
>> + if (BIT(base) & status) {
>> + generic_handle_irq(irq_linear_revmap(
>> + altera_gc->irq, base));
>> + }
>> + }
>> + } else {
>> + while ((status =
>> + (readl_relaxed(mm_gc->regs + ALTERA_GPIO_EDGE_CAP) &
>> + readl_relaxed(mm_gc->regs + ALTERA_GPIO_IRQ_MASK)))) {
>> + writel_relaxed(status,
>> + mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
>
> Nice use if relaxed accessors in the irq handler!
I don't understand the comment. Could you elaborate further?
>
>> + for (base = 0; base < mm_gc->gc.ngpio; base++) {
>> + if (BIT(base) & status) {
>> + generic_handle_irq(irq_linear_revmap(
>> + altera_gc->irq, base));
>> + }
>> + }
>> + }
>> + }
>> +
>> + chip->irq_eoi(irq_desc_get_irq_data(desc));
>> + chip->irq_unmask(&desc->irq_data);
>> +}
>> +
>> +static int altera_gpio_irq_map(struct irq_domain *h, unsigned int virq,
>> + irq_hw_number_t hw_irq_num)
>> +{
>> + irq_set_chip_data(virq, h->host_data);
>> + irq_set_chip_and_handler(virq, &altera_irq_chip, handle_level_irq);
>> + irq_set_irq_type(virq, IRQ_TYPE_NONE);
>> +
>> + return 0;
>> +}
>> +
>> +static struct irq_domain_ops altera_gpio_irq_ops = {
>> + .map = altera_gpio_irq_map,
>> + .xlate = irq_domain_xlate_onecell,
>> +};
>> +
>> +int altera_gpio_probe(struct platform_device *pdev)
>> +{
>> + struct device_node *node = pdev->dev.of_node;
>> + int id, reg, ret;
>> + struct altera_gpio_chip *altera_gc = devm_kzalloc(&pdev->dev,
>> + sizeof(*altera_gc), GFP_KERNEL);
>> + if (altera_gc == NULL) {
>> + pr_err("%s: out of memory\n", node->full_name);
>> + return -ENOMEM;
>> + }
>> + altera_gc->irq = 0;
>> +
>> + spin_lock_init(&altera_gc->gpio_lock);
>> +
>> + id = pdev->id;
>> +
>> + if (of_property_read_u32(node, "width", ®))
>> + /*By default assume full GPIO controller*/
>> + altera_gc->mmchip.gc.ngpio = 32;
>> + else
>> + altera_gc->mmchip.gc.ngpio = reg;
>
> I question this property above...
>
>> +
>> + if (altera_gc->mmchip.gc.ngpio > 32) {
>> + pr_warn("%s: ngpio is greater than 32, defaulting to 32\n",
>> + node->full_name);
>> + altera_gc->mmchip.gc.ngpio = 32;
>> + }
>> +
>> + altera_gc->mmchip.gc.direction_input = altera_gpio_direction_input;
>> + altera_gc->mmchip.gc.direction_output = altera_gpio_direction_output;
>> + altera_gc->mmchip.gc.get = altera_gpio_get;
>> + altera_gc->mmchip.gc.set = altera_gpio_set;
>> + altera_gc->mmchip.gc.to_irq = altera_gpio_to_irq;
>> + altera_gc->mmchip.gc.owner = THIS_MODULE;
>> +
>> + ret = of_mm_gpiochip_add(node, &altera_gc->mmchip);
>> + if (ret) {
>> + pr_err("%s: Failed adding memory mapped gpiochip\n",
>> + node->full_name);
>> + return ret;
>> + }
>> +
>> + platform_set_drvdata(pdev, altera_gc);
>> +
>> + if (of_get_property(node, "interrupts", ®) == NULL)
>> + goto skip_irq;
>> + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0);
>> +
>> + if (altera_gc->mapped_irq == NO_IRQ)
>> + goto skip_irq;
>
> Just return 0; here.
>
>> +
>> + altera_gc->irq = irq_domain_add_linear(node, altera_gc->mmchip.gc.ngpio,
>> + &altera_gpio_irq_ops, altera_gc);
>> +
>> + if (!altera_gc->irq) {
>> + ret = -ENODEV;
>> + goto dispose_irq;
>> + }
>> +
>> + if (of_property_read_u32(node, "level_trigger", ®)) {
>> + ret = -EINVAL;
>> + pr_err("%s: level_trigger value not set in device tree\n",
>> + node->full_name);
>> + goto teardown;
>> + }
>> + altera_gc->level_trigger = reg;
>> +
>> + /*
>> + * If it is not level triggered PIO
>> + * Check what edge type it is
>> + */
>> + if (!altera_gc->level_trigger) {
>> + if (of_property_read_u32(node, "edge_type", ®)) {
>> + ret = -EINVAL;
>> + pr_err("%s: edge_type value not set in device tree\n"
>> + , node->full_name);
>> + goto teardown;
>> + }
>> + }
>> + altera_gc->edge_type = reg;
>> +
>> + irq_set_handler_data(altera_gc->mapped_irq, altera_gc);
>> + irq_set_chained_handler(altera_gc->mapped_irq, altera_gpio_irq_handler);
>> +
>> + return 0;
>> +
>> +teardown:
>> + irq_domain_remove(altera_gc->irq);
>> +dispose_irq:
>> + irq_dispose_mapping(altera_gc->mapped_irq);
>> + WARN_ON(gpiochip_remove(&altera_gc->mmchip.gc) < 0);
>> +
>> + pr_err("%s: registration failed with status %d\n",
>> + node->full_name, ret);
>> +
>> + return ret;
>> +skip_irq:
>> + return 0;
>> +}
>> +
>> +static int altera_gpio_remove(struct platform_device *pdev)
>> +{
>> + unsigned int irq, i;
>> + int status;
>> + struct altera_gpio_chip *altera_gc = platform_get_drvdata(pdev);
>> +
>> + status = gpiochip_remove(&altera_gc->mmchip.gc);
>> +
>> + if (status < 0)
>> + return status;
>> +
>> + if (altera_gc->irq) {
>> + irq_dispose_mapping(altera_gc->mapped_irq);
>> +
>> + for (i = 0; i < altera_gc->mmchip.gc.ngpio; i++) {
>> + irq = irq_find_mapping(altera_gc->irq, i);
>> + if (irq > 0)
>> + irq_dispose_mapping(irq);
>> + }
>
> This loop, isn't thad done inside irq_domain_remove() already?
Noted. I'll remove this.
>
> That's all review comments so far. As you see it needs some
> rough edges fixed up...
>
> Yours,
> Linus Walleij
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver
2013-07-08 7:04 thloh.linux
@ 2013-07-22 19:07 ` Linus Walleij
2013-07-23 2:51 ` Loh Tien Hock
0 siblings, 1 reply; 20+ messages in thread
From: Linus Walleij @ 2013-07-22 19:07 UTC (permalink / raw)
To: thloh.linux, devicetree
Cc: Rob Herring, Rob Landley, linux-doc, devicetree-discuss,
Loh Tien Hock, Tien Hock Loh
On Mon, Jul 8, 2013 at 9:04 AM, <thloh.linux@gmail.com> wrote:
> From: Tien Hock Loh <thloh@altera.com>
>
> Add driver support for Altera GPIO soft IP, including interrupts and I/O.
> Tested on Altera CV SoC board using dipsw and LED using LED framework.
>
> Signed-off-by: Tien Hock Loh <thloh@altera.com>
Note that the new mailing list for devicetree patches is
devicetree@vger.kernel.org nowadays.
> +Altera GPIO specific properties:
> +- width: Width of the GPIO bank, range from 1-32
Can it really vary like that? 1, 2, 3, ...
Can there be "holes" in this range then, so you might prefer
a bitmask over a numeral?
> +- level_trigger: Specifies whether the GPIO interrupt is level trigger (high).
> + This field is required if the Altera GPIO controller used has IRQ enabled.
> +- edge_type: Specifies edge type if the GPIO interrupt is edge trigger:
> + 0 = Rising edge
> + 1 = Falling edge
> + 2 = Both edge
No thanks.
#include <dt-bindings/interrupt-controller/irq.h>
and define these things per-irq by using more interrupt-cells.
> +Example:
> +
> +gpio_altr: gpio_altr {
> + compatible = "altr,pio-1.0";
> + reg = <0xff200000 0x10>;
> + interrupts = <0 45 4>;
> + width = <32>;
> + level_trigger = <0>;
> + #gpio-cells = <1>;
> + gpio-controller;
> + #interrupt-cells = <1>;
This needs to be 2 or something so you can pass the edge specifics
for each line.
> +config GPIO_ALTERA
> + tristate "Altera GPIO"
Do you *really* load this as a module sometimes?
> +++ b/drivers/gpio/gpio-altera.c
> +#include <linux/gpio.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/of_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/interrupt.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
#include <linux/bitops.h>
BIT() comes from this file.
> +
> +#define ALTERA_GPIO_DATA 0x0
> +#define ALTERA_GPIO_DIR 0x4
> +#define ALTERA_GPIO_IRQ_MASK 0x8
> +#define ALTERA_GPIO_EDGE_CAP 0xc
> +#define ALTERA_GPIO_OUTSET 0x10
> +#define ALTERA_GPIO_OUTCLEAR 0x14
> +#define ALTERA_IRQ_RISING 0
> +#define ALTERA_IRQ_FALLING 1
> +#define ALTERA_IRQ_BOTH 2
Why do you need these local redefinitions of rising/falling/both?
Use the defines from the subsystem.
> +
> +/**
> +* struct altera_gpio_chip
> +* @mmchip : memory mapped chip structure.
> +* @irq : irq domain that this driver is registered to.
This is confusing. Rename this to "irqdomain" instead.
> +* @gpio_lock : synchronization lock so that new irq/set/get requests
> + will be blocked until the current one completes.
> +* @level_trigger : specifies if the controller is level triggered
> + (high).
> +* @edge_type : specify the edge type of the controller (rising,
> + falling, both). Only used for !level_trigger.
> +* @mapped_irq : kernel mapped irq number.
> +*/
> +struct altera_gpio_chip {
> + struct of_mm_gpio_chip mmchip;
> + struct irq_domain *irq;
> + spinlock_t gpio_lock;
> + int level_trigger;
> + int edge_type;
> + int mapped_irq;
And name this "irq".
> +};
> +
> +static void altera_gpio_irq_unmask(struct irq_data *d)
> +{
> + struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
> + struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
> + unsigned long flags;
> + unsigned int intmask;
Use u32 for this variable.
> +
> + spin_lock_irqsave(&altera_gc->gpio_lock, flags);
> + intmask = readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
> + /* Set ALTERA_GPIO_IRQ_MASK bit to unmask */
> + intmask |= BIT(irqd_to_hwirq(d));
> + writel(intmask, mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
> + spin_unlock_irqrestore(&altera_gc->gpio_lock, flags);
> +}
> +
> +static void altera_gpio_irq_mask(struct irq_data *d)
> +{
> + struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
> + struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
> + unsigned long flags;
> + unsigned int intmask;
u32
> +
> + spin_lock_irqsave(&altera_gc->gpio_lock, flags);
> + intmask = readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
> + /* Clear ALTERA_GPIO_IRQ_MASK bit to mask */
> + intmask &= BIT(irqd_to_hwirq(d));
> + writel(intmask, mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
> + spin_unlock_irqrestore(&altera_gc->gpio_lock, flags);
> +}
> +
> +static int altera_gpio_irq_set_type(struct irq_data *d,
> + unsigned int type)
> +{
> + struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
> +
> + if (type == IRQ_TYPE_NONE)
> + return 0;
> +
> + if (altera_gc->level_trigger) {
> + if (type == IRQ_TYPE_LEVEL_HIGH)
> + return 0;
> + } else {
> + if (type == IRQ_TYPE_EDGE_RISING &&
> + altera_gc->edge_type == ALTERA_IRQ_RISING)
> + return 0;
> + else if (type == IRQ_TYPE_EDGE_FALLING &&
> + altera_gc->edge_type == ALTERA_IRQ_FALLING)
> + return 0;
> + else if (type == IRQ_TYPE_EDGE_BOTH &&
> + altera_gc->edge_type == ALTERA_IRQ_BOTH)
> + return 0;
I don't quite realize why you need the local version of
the IRQflag at all. Just store the standard edge indicator
in an unsigned long?
Also: if you have a property in the device tree setting all IRQs
to one or the other type (as the weird binding is now) why
are you still supporting all these variants on each and every
IRQ?
> + }
> +
> + return -EINVAL;
> +}
> +
> +static struct irq_chip altera_irq_chip = {
> + .name = "altera-gpio",
> + .irq_mask = altera_gpio_irq_mask,
> + .irq_unmask = altera_gpio_irq_unmask,
> + .irq_set_type = altera_gpio_irq_set_type,
> +};
> +
> +static int altera_gpio_get(struct gpio_chip *gc, unsigned offset)
> +{
> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> +
> + return (readl(mm_gc->regs + ALTERA_GPIO_DATA) >> offset) & 1;
Pls write it like this:
return !!(readl(mm_gc->regs + ALTERA_GPIO_DATA) & BIT(offset)));
> +}
> +
> +static void altera_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
> +{
> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> + struct altera_gpio_chip *chip = container_of(mm_gc,
> + struct altera_gpio_chip, mmchip);
> + unsigned long flags;
> + unsigned int data_reg;
u32
> +
> + spin_lock_irqsave(&chip->gpio_lock, flags);
> + data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA);
> + data_reg = (data_reg & ~BIT(offset)) | (value << offset);
Value is always 0 or 1.
I would prefer this idom even though its more lines:
if (val)
data_reg |= BIT(offset);
else
data_reg &= ~BIT(offset);
> + writel(data_reg, mm_gc->regs + ALTERA_GPIO_DATA);
> + spin_unlock_irqrestore(&chip->gpio_lock, flags);
> +}
> +
> +static int altera_gpio_direction_input(struct gpio_chip *gc, unsigned offset)
> +{
> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> + struct altera_gpio_chip *chip = container_of(mm_gc,
> + struct altera_gpio_chip, mmchip);
> + unsigned long flags;
> + unsigned int gpio_ddr;
u32
> +
> + spin_lock_irqsave(&chip->gpio_lock, flags);
> + /* Set pin as input, assumes software controlled IP */
> + gpio_ddr = readl(mm_gc->regs + ALTERA_GPIO_DIR);
> + gpio_ddr &= ~BIT(offset);
> + writel(gpio_ddr, mm_gc->regs + ALTERA_GPIO_DIR);
> + spin_unlock_irqrestore(&chip->gpio_lock, flags);
> +
> + return 0;
> +}
> +
> +static int altera_gpio_direction_output(struct gpio_chip *gc,
> + unsigned offset, int value)
> +{
> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> + struct altera_gpio_chip *chip = container_of(mm_gc,
> + struct altera_gpio_chip, mmchip);
> + unsigned long flags;
> + unsigned int data_reg, gpio_ddr;
u32
> +
> + spin_lock_irqsave(&chip->gpio_lock, flags);
> + /* Sets the GPIO value */
> + data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA);
> + data_reg = (data_reg & ~BIT(offset)) | (value << offset);
Use same construct as indicated above.
> + writel(data_reg, mm_gc->regs + ALTERA_GPIO_DATA);
> +
> + /* Set pin as output, assumes software controlled IP */
> + gpio_ddr = readl(mm_gc->regs + ALTERA_GPIO_DIR);
> + gpio_ddr |= BIT(offset);
> + writel(gpio_ddr, mm_gc->regs + ALTERA_GPIO_DIR);
> + spin_unlock_irqrestore(&chip->gpio_lock, flags);
> +
> + return 0;
> +}
> +
> +static int altera_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
> +{
> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> + struct altera_gpio_chip *altera_gc = container_of(mm_gc,
> + struct altera_gpio_chip, mmchip);
> +
> + if (altera_gc->irq == 0)
> + return -ENXIO;
> + if ((altera_gc->irq && offset) < altera_gc->mmchip.gc.ngpio)
I don't understand this. Why are you making a binary and &&
with the offset and then checking if that is less that the number
of GPIO?
> + return irq_create_mapping(altera_gc->irq, offset);
> + else
> + return -ENXIO;
> +}
> +
> +static void altera_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> +{
> + struct altera_gpio_chip *altera_gc = irq_desc_get_handler_data(desc);
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
> + unsigned long status;
u32
> +
> + int base;
Is this an int really? It looks like u8 would suffice.
Why is this named "base"? Can you give it a more self-explanatory
name? Else just use "i".
> + chip->irq_mask(&desc->irq_data);
> +
> + if (altera_gc->level_trigger) {
> + status = readl_relaxed(mm_gc->regs + ALTERA_GPIO_DATA);
> + status &= readl_relaxed(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
> +
> + for (base = 0; base < mm_gc->gc.ngpio; base++) {
> + if (BIT(base) & status) {
> + generic_handle_irq(irq_linear_revmap(
> + altera_gc->irq, base));
> + }
> + }
> + } else {
> + while ((status =
> + (readl_relaxed(mm_gc->regs + ALTERA_GPIO_EDGE_CAP) &
> + readl_relaxed(mm_gc->regs + ALTERA_GPIO_IRQ_MASK)))) {
> + writel_relaxed(status,
> + mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
Nice use if relaxed accessors in the irq handler!
> + for (base = 0; base < mm_gc->gc.ngpio; base++) {
> + if (BIT(base) & status) {
> + generic_handle_irq(irq_linear_revmap(
> + altera_gc->irq, base));
> + }
> + }
> + }
> + }
> +
> + chip->irq_eoi(irq_desc_get_irq_data(desc));
> + chip->irq_unmask(&desc->irq_data);
> +}
> +
> +static int altera_gpio_irq_map(struct irq_domain *h, unsigned int virq,
> + irq_hw_number_t hw_irq_num)
> +{
> + irq_set_chip_data(virq, h->host_data);
> + irq_set_chip_and_handler(virq, &altera_irq_chip, handle_level_irq);
> + irq_set_irq_type(virq, IRQ_TYPE_NONE);
> +
> + return 0;
> +}
> +
> +static struct irq_domain_ops altera_gpio_irq_ops = {
> + .map = altera_gpio_irq_map,
> + .xlate = irq_domain_xlate_onecell,
> +};
> +
> +int altera_gpio_probe(struct platform_device *pdev)
> +{
> + struct device_node *node = pdev->dev.of_node;
> + int id, reg, ret;
> + struct altera_gpio_chip *altera_gc = devm_kzalloc(&pdev->dev,
> + sizeof(*altera_gc), GFP_KERNEL);
> + if (altera_gc == NULL) {
> + pr_err("%s: out of memory\n", node->full_name);
> + return -ENOMEM;
> + }
> + altera_gc->irq = 0;
> +
> + spin_lock_init(&altera_gc->gpio_lock);
> +
> + id = pdev->id;
> +
> + if (of_property_read_u32(node, "width", ®))
> + /*By default assume full GPIO controller*/
> + altera_gc->mmchip.gc.ngpio = 32;
> + else
> + altera_gc->mmchip.gc.ngpio = reg;
I question this property above...
> +
> + if (altera_gc->mmchip.gc.ngpio > 32) {
> + pr_warn("%s: ngpio is greater than 32, defaulting to 32\n",
> + node->full_name);
> + altera_gc->mmchip.gc.ngpio = 32;
> + }
> +
> + altera_gc->mmchip.gc.direction_input = altera_gpio_direction_input;
> + altera_gc->mmchip.gc.direction_output = altera_gpio_direction_output;
> + altera_gc->mmchip.gc.get = altera_gpio_get;
> + altera_gc->mmchip.gc.set = altera_gpio_set;
> + altera_gc->mmchip.gc.to_irq = altera_gpio_to_irq;
> + altera_gc->mmchip.gc.owner = THIS_MODULE;
> +
> + ret = of_mm_gpiochip_add(node, &altera_gc->mmchip);
> + if (ret) {
> + pr_err("%s: Failed adding memory mapped gpiochip\n",
> + node->full_name);
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, altera_gc);
> +
> + if (of_get_property(node, "interrupts", ®) == NULL)
> + goto skip_irq;
> + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0);
> +
> + if (altera_gc->mapped_irq == NO_IRQ)
> + goto skip_irq;
Just return 0; here.
> +
> + altera_gc->irq = irq_domain_add_linear(node, altera_gc->mmchip.gc.ngpio,
> + &altera_gpio_irq_ops, altera_gc);
> +
> + if (!altera_gc->irq) {
> + ret = -ENODEV;
> + goto dispose_irq;
> + }
> +
> + if (of_property_read_u32(node, "level_trigger", ®)) {
> + ret = -EINVAL;
> + pr_err("%s: level_trigger value not set in device tree\n",
> + node->full_name);
> + goto teardown;
> + }
> + altera_gc->level_trigger = reg;
> +
> + /*
> + * If it is not level triggered PIO
> + * Check what edge type it is
> + */
> + if (!altera_gc->level_trigger) {
> + if (of_property_read_u32(node, "edge_type", ®)) {
> + ret = -EINVAL;
> + pr_err("%s: edge_type value not set in device tree\n"
> + , node->full_name);
> + goto teardown;
> + }
> + }
> + altera_gc->edge_type = reg;
> +
> + irq_set_handler_data(altera_gc->mapped_irq, altera_gc);
> + irq_set_chained_handler(altera_gc->mapped_irq, altera_gpio_irq_handler);
> +
> + return 0;
> +
> +teardown:
> + irq_domain_remove(altera_gc->irq);
> +dispose_irq:
> + irq_dispose_mapping(altera_gc->mapped_irq);
> + WARN_ON(gpiochip_remove(&altera_gc->mmchip.gc) < 0);
> +
> + pr_err("%s: registration failed with status %d\n",
> + node->full_name, ret);
> +
> + return ret;
> +skip_irq:
> + return 0;
> +}
> +
> +static int altera_gpio_remove(struct platform_device *pdev)
> +{
> + unsigned int irq, i;
> + int status;
> + struct altera_gpio_chip *altera_gc = platform_get_drvdata(pdev);
> +
> + status = gpiochip_remove(&altera_gc->mmchip.gc);
> +
> + if (status < 0)
> + return status;
> +
> + if (altera_gc->irq) {
> + irq_dispose_mapping(altera_gc->mapped_irq);
> +
> + for (i = 0; i < altera_gc->mmchip.gc.ngpio; i++) {
> + irq = irq_find_mapping(altera_gc->irq, i);
> + if (irq > 0)
> + irq_dispose_mapping(irq);
> + }
This loop, isn't thad done inside irq_domain_remove() already?
That's all review comments so far. As you see it needs some
rough edges fixed up...
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver
@ 2013-07-08 7:04 thloh.linux
2013-07-22 19:07 ` Linus Walleij
0 siblings, 1 reply; 20+ messages in thread
From: thloh.linux @ 2013-07-08 7:04 UTC (permalink / raw)
To: linus.walleij, rob.herring, rob
Cc: linux-doc, devicetree-discuss, thloh85, thloh
From: Tien Hock Loh <thloh@altera.com>
Add driver support for Altera GPIO soft IP, including interrupts and I/O.
Tested on Altera CV SoC board using dipsw and LED using LED framework.
Signed-off-by: Tien Hock Loh <thloh@altera.com>
---
.../devicetree/bindings/gpio/gpio-altera.txt | 36 ++
.../devicetree/bindings/vendor-prefixes.txt | 1 +
drivers/gpio/Kconfig | 7 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-altera.c | 405 +++++++++++++++++++++
5 files changed, 450 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-altera.txt
create mode 100644 drivers/gpio/gpio-altera.c
diff --git a/Documentation/devicetree/bindings/gpio/gpio-altera.txt b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
new file mode 100644
index 0000000..402c99e
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
@@ -0,0 +1,36 @@
+Altera GPIO controller bindings
+
+Required properties:
+- compatible:
+ - "altr,pio-1.0"
+- reg: Physical base address and length of the controller's registers.
+- #gpio-cells : Should be 1
+ - first cell is the gpio offset number
+- gpio-controller : Marks the device node as a GPIO controller.
+- #interrupt-cells : Should be 1.
+- interrupts: Specify the interrupt.
+- interrupt-controller: Mark the device node as an interrupt controller
+ The first cell is the local interrupt offset number to the GPIO controller.
+
+Altera GPIO specific properties:
+- width: Width of the GPIO bank, range from 1-32
+- level_trigger: Specifies whether the GPIO interrupt is level trigger (high).
+ This field is required if the Altera GPIO controller used has IRQ enabled.
+- edge_type: Specifies edge type if the GPIO interrupt is edge trigger:
+ 0 = Rising edge
+ 1 = Falling edge
+ 2 = Both edge
+
+Example:
+
+gpio_altr: gpio_altr {
+ compatible = "altr,pio-1.0";
+ reg = <0xff200000 0x10>;
+ interrupts = <0 45 4>;
+ width = <32>;
+ level_trigger = <0>;
+ #gpio-cells = <1>;
+ gpio-controller;
+ #interrupt-cells = <1>;
+ interrupt-controller;
+};
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index db0457d..21a4dc3 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -7,6 +7,7 @@ ad Avionic Design GmbH
adi Analog Devices, Inc.
aeroflexgaisler Aeroflex Gaisler AB
ak Asahi Kasei Corp.
+altr Altera Corp.
amcc Applied Micro Circuits Corporation (APM, formally AMCC)
apm Applied Micro Circuits Corporation (APM)
arm ARM Ltd.
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b2450ba..3d46d10 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -121,6 +121,13 @@ config GPIO_GENERIC_PLATFORM
help
Say yes here to support basic platform_device memory-mapped GPIO controllers.
+config GPIO_ALTERA
+ tristate "Altera GPIO"
+ select IRQ_DOMAIN
+ depends on OF_GPIO
+ help
+ Say yes here to support the Altera PIO device.
+
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 ef3e983..bb96ce5 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_GPIO_74X164) += gpio-74x164.o
obj-$(CONFIG_GPIO_ADNP) += gpio-adnp.o
obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o
obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o
+obj-$(CONFIG_GPIO_ALTERA) += gpio-altera.o
obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o
obj-$(CONFIG_GPIO_ARIZONA) += gpio-arizona.o
obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
diff --git a/drivers/gpio/gpio-altera.c b/drivers/gpio/gpio-altera.c
new file mode 100644
index 0000000..75eebed
--- /dev/null
+++ b/drivers/gpio/gpio-altera.c
@@ -0,0 +1,405 @@
+/*
+ * Copyright (C) 2013 Altera Corporation
+ * Based on gpio-mpc8xxx.c
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2. This program is licensed "as is" without any warranty of any
+ * kind, whether express or implied.
+ */
+
+#include <linux/gpio.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/of_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/interrupt.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+
+#define ALTERA_GPIO_DATA 0x0
+#define ALTERA_GPIO_DIR 0x4
+#define ALTERA_GPIO_IRQ_MASK 0x8
+#define ALTERA_GPIO_EDGE_CAP 0xc
+#define ALTERA_GPIO_OUTSET 0x10
+#define ALTERA_GPIO_OUTCLEAR 0x14
+#define ALTERA_IRQ_RISING 0
+#define ALTERA_IRQ_FALLING 1
+#define ALTERA_IRQ_BOTH 2
+
+/**
+* struct altera_gpio_chip
+* @mmchip : memory mapped chip structure.
+* @irq : irq domain that this driver is registered to.
+* @gpio_lock : synchronization lock so that new irq/set/get requests
+ will be blocked until the current one completes.
+* @level_trigger : specifies if the controller is level triggered
+ (high).
+* @edge_type : specify the edge type of the controller (rising,
+ falling, both). Only used for !level_trigger.
+* @mapped_irq : kernel mapped irq number.
+*/
+struct altera_gpio_chip {
+ struct of_mm_gpio_chip mmchip;
+ struct irq_domain *irq;
+ spinlock_t gpio_lock;
+ int level_trigger;
+ int edge_type;
+ int mapped_irq;
+};
+
+static void altera_gpio_irq_unmask(struct irq_data *d)
+{
+ struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
+ struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
+ unsigned long flags;
+ unsigned int intmask;
+
+ spin_lock_irqsave(&altera_gc->gpio_lock, flags);
+ intmask = readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+ /* Set ALTERA_GPIO_IRQ_MASK bit to unmask */
+ intmask |= BIT(irqd_to_hwirq(d));
+ writel(intmask, mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+ spin_unlock_irqrestore(&altera_gc->gpio_lock, flags);
+}
+
+static void altera_gpio_irq_mask(struct irq_data *d)
+{
+ struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
+ struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
+ unsigned long flags;
+ unsigned int intmask;
+
+ spin_lock_irqsave(&altera_gc->gpio_lock, flags);
+ intmask = readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+ /* Clear ALTERA_GPIO_IRQ_MASK bit to mask */
+ intmask &= BIT(irqd_to_hwirq(d));
+ writel(intmask, mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+ spin_unlock_irqrestore(&altera_gc->gpio_lock, flags);
+}
+
+static int altera_gpio_irq_set_type(struct irq_data *d,
+ unsigned int type)
+{
+ struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
+
+ if (type == IRQ_TYPE_NONE)
+ return 0;
+
+ if (altera_gc->level_trigger) {
+ if (type == IRQ_TYPE_LEVEL_HIGH)
+ return 0;
+ } else {
+ if (type == IRQ_TYPE_EDGE_RISING &&
+ altera_gc->edge_type == ALTERA_IRQ_RISING)
+ return 0;
+ else if (type == IRQ_TYPE_EDGE_FALLING &&
+ altera_gc->edge_type == ALTERA_IRQ_FALLING)
+ return 0;
+ else if (type == IRQ_TYPE_EDGE_BOTH &&
+ altera_gc->edge_type == ALTERA_IRQ_BOTH)
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
+static struct irq_chip altera_irq_chip = {
+ .name = "altera-gpio",
+ .irq_mask = altera_gpio_irq_mask,
+ .irq_unmask = altera_gpio_irq_unmask,
+ .irq_set_type = altera_gpio_irq_set_type,
+};
+
+static int altera_gpio_get(struct gpio_chip *gc, unsigned offset)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+
+ return (readl(mm_gc->regs + ALTERA_GPIO_DATA) >> offset) & 1;
+}
+
+static void altera_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct altera_gpio_chip *chip = container_of(mm_gc,
+ struct altera_gpio_chip, mmchip);
+ unsigned long flags;
+ unsigned int data_reg;
+
+ spin_lock_irqsave(&chip->gpio_lock, flags);
+ data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA);
+ data_reg = (data_reg & ~BIT(offset)) | (value << offset);
+ writel(data_reg, mm_gc->regs + ALTERA_GPIO_DATA);
+ spin_unlock_irqrestore(&chip->gpio_lock, flags);
+}
+
+static int altera_gpio_direction_input(struct gpio_chip *gc, unsigned offset)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct altera_gpio_chip *chip = container_of(mm_gc,
+ struct altera_gpio_chip, mmchip);
+ unsigned long flags;
+ unsigned int gpio_ddr;
+
+ spin_lock_irqsave(&chip->gpio_lock, flags);
+ /* Set pin as input, assumes software controlled IP */
+ gpio_ddr = readl(mm_gc->regs + ALTERA_GPIO_DIR);
+ gpio_ddr &= ~BIT(offset);
+ writel(gpio_ddr, mm_gc->regs + ALTERA_GPIO_DIR);
+ spin_unlock_irqrestore(&chip->gpio_lock, flags);
+
+ return 0;
+}
+
+static int altera_gpio_direction_output(struct gpio_chip *gc,
+ unsigned offset, int value)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct altera_gpio_chip *chip = container_of(mm_gc,
+ struct altera_gpio_chip, mmchip);
+ unsigned long flags;
+ unsigned int data_reg, gpio_ddr;
+
+ spin_lock_irqsave(&chip->gpio_lock, flags);
+ /* Sets the GPIO value */
+ data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA);
+ data_reg = (data_reg & ~BIT(offset)) | (value << offset);
+ writel(data_reg, mm_gc->regs + ALTERA_GPIO_DATA);
+
+ /* Set pin as output, assumes software controlled IP */
+ gpio_ddr = readl(mm_gc->regs + ALTERA_GPIO_DIR);
+ gpio_ddr |= BIT(offset);
+ writel(gpio_ddr, mm_gc->regs + ALTERA_GPIO_DIR);
+ spin_unlock_irqrestore(&chip->gpio_lock, flags);
+
+ return 0;
+}
+
+static int altera_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct altera_gpio_chip *altera_gc = container_of(mm_gc,
+ struct altera_gpio_chip, mmchip);
+
+ if (altera_gc->irq == 0)
+ return -ENXIO;
+ if ((altera_gc->irq && offset) < altera_gc->mmchip.gc.ngpio)
+ return irq_create_mapping(altera_gc->irq, offset);
+ else
+ return -ENXIO;
+}
+
+static void altera_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
+{
+ struct altera_gpio_chip *altera_gc = irq_desc_get_handler_data(desc);
+ struct irq_chip *chip = irq_desc_get_chip(desc);
+ struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
+ unsigned long status;
+
+ int base;
+ chip->irq_mask(&desc->irq_data);
+
+ if (altera_gc->level_trigger) {
+ status = readl_relaxed(mm_gc->regs + ALTERA_GPIO_DATA);
+ status &= readl_relaxed(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+
+ for (base = 0; base < mm_gc->gc.ngpio; base++) {
+ if (BIT(base) & status) {
+ generic_handle_irq(irq_linear_revmap(
+ altera_gc->irq, base));
+ }
+ }
+ } else {
+ while ((status =
+ (readl_relaxed(mm_gc->regs + ALTERA_GPIO_EDGE_CAP) &
+ readl_relaxed(mm_gc->regs + ALTERA_GPIO_IRQ_MASK)))) {
+ writel_relaxed(status,
+ mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
+ for (base = 0; base < mm_gc->gc.ngpio; base++) {
+ if (BIT(base) & status) {
+ generic_handle_irq(irq_linear_revmap(
+ altera_gc->irq, base));
+ }
+ }
+ }
+ }
+
+ chip->irq_eoi(irq_desc_get_irq_data(desc));
+ chip->irq_unmask(&desc->irq_data);
+}
+
+static int altera_gpio_irq_map(struct irq_domain *h, unsigned int virq,
+ irq_hw_number_t hw_irq_num)
+{
+ irq_set_chip_data(virq, h->host_data);
+ irq_set_chip_and_handler(virq, &altera_irq_chip, handle_level_irq);
+ irq_set_irq_type(virq, IRQ_TYPE_NONE);
+
+ return 0;
+}
+
+static struct irq_domain_ops altera_gpio_irq_ops = {
+ .map = altera_gpio_irq_map,
+ .xlate = irq_domain_xlate_onecell,
+};
+
+int altera_gpio_probe(struct platform_device *pdev)
+{
+ struct device_node *node = pdev->dev.of_node;
+ int id, reg, ret;
+ struct altera_gpio_chip *altera_gc = devm_kzalloc(&pdev->dev,
+ sizeof(*altera_gc), GFP_KERNEL);
+ if (altera_gc == NULL) {
+ pr_err("%s: out of memory\n", node->full_name);
+ return -ENOMEM;
+ }
+ altera_gc->irq = 0;
+
+ spin_lock_init(&altera_gc->gpio_lock);
+
+ id = pdev->id;
+
+ if (of_property_read_u32(node, "width", ®))
+ /*By default assume full GPIO controller*/
+ altera_gc->mmchip.gc.ngpio = 32;
+ else
+ altera_gc->mmchip.gc.ngpio = reg;
+
+ if (altera_gc->mmchip.gc.ngpio > 32) {
+ pr_warn("%s: ngpio is greater than 32, defaulting to 32\n",
+ node->full_name);
+ altera_gc->mmchip.gc.ngpio = 32;
+ }
+
+ altera_gc->mmchip.gc.direction_input = altera_gpio_direction_input;
+ altera_gc->mmchip.gc.direction_output = altera_gpio_direction_output;
+ altera_gc->mmchip.gc.get = altera_gpio_get;
+ altera_gc->mmchip.gc.set = altera_gpio_set;
+ altera_gc->mmchip.gc.to_irq = altera_gpio_to_irq;
+ altera_gc->mmchip.gc.owner = THIS_MODULE;
+
+ ret = of_mm_gpiochip_add(node, &altera_gc->mmchip);
+ if (ret) {
+ pr_err("%s: Failed adding memory mapped gpiochip\n",
+ node->full_name);
+ return ret;
+ }
+
+ platform_set_drvdata(pdev, altera_gc);
+
+ if (of_get_property(node, "interrupts", ®) == NULL)
+ goto skip_irq;
+ altera_gc->mapped_irq = irq_of_parse_and_map(node, 0);
+
+ if (altera_gc->mapped_irq == NO_IRQ)
+ goto skip_irq;
+
+ altera_gc->irq = irq_domain_add_linear(node, altera_gc->mmchip.gc.ngpio,
+ &altera_gpio_irq_ops, altera_gc);
+
+ if (!altera_gc->irq) {
+ ret = -ENODEV;
+ goto dispose_irq;
+ }
+
+ if (of_property_read_u32(node, "level_trigger", ®)) {
+ ret = -EINVAL;
+ pr_err("%s: level_trigger value not set in device tree\n",
+ node->full_name);
+ goto teardown;
+ }
+ altera_gc->level_trigger = reg;
+
+ /*
+ * If it is not level triggered PIO
+ * Check what edge type it is
+ */
+ if (!altera_gc->level_trigger) {
+ if (of_property_read_u32(node, "edge_type", ®)) {
+ ret = -EINVAL;
+ pr_err("%s: edge_type value not set in device tree\n"
+ , node->full_name);
+ goto teardown;
+ }
+ }
+ altera_gc->edge_type = reg;
+
+ irq_set_handler_data(altera_gc->mapped_irq, altera_gc);
+ irq_set_chained_handler(altera_gc->mapped_irq, altera_gpio_irq_handler);
+
+ return 0;
+
+teardown:
+ irq_domain_remove(altera_gc->irq);
+dispose_irq:
+ irq_dispose_mapping(altera_gc->mapped_irq);
+ WARN_ON(gpiochip_remove(&altera_gc->mmchip.gc) < 0);
+
+ pr_err("%s: registration failed with status %d\n",
+ node->full_name, ret);
+
+ return ret;
+skip_irq:
+ return 0;
+}
+
+static int altera_gpio_remove(struct platform_device *pdev)
+{
+ unsigned int irq, i;
+ int status;
+ struct altera_gpio_chip *altera_gc = platform_get_drvdata(pdev);
+
+ status = gpiochip_remove(&altera_gc->mmchip.gc);
+
+ if (status < 0)
+ return status;
+
+ if (altera_gc->irq) {
+ irq_dispose_mapping(altera_gc->mapped_irq);
+
+ for (i = 0; i < altera_gc->mmchip.gc.ngpio; i++) {
+ irq = irq_find_mapping(altera_gc->irq, i);
+ if (irq > 0)
+ irq_dispose_mapping(irq);
+ }
+
+ irq_domain_remove(altera_gc->irq);
+ }
+
+ irq_set_handler_data(altera_gc->mapped_irq, NULL);
+ irq_set_chained_handler(altera_gc->mapped_irq, NULL);
+ return -EIO;
+}
+
+static struct of_device_id altera_gpio_of_match[] = {
+ { .compatible = "altr,pio-1.0", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, altera_gpio_of_match);
+
+static struct platform_driver altera_gpio_driver = {
+ .driver = {
+ .name = "altera_gpio",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(altera_gpio_of_match),
+ },
+ .probe = altera_gpio_probe,
+ .remove = altera_gpio_remove,
+};
+
+static int __init altera_gpio_init(void)
+{
+ return platform_driver_register(&altera_gpio_driver);
+}
+subsys_initcall(altera_gpio_init);
+
+static void __exit altera_gpio_exit(void)
+{
+ platform_driver_unregister(&altera_gpio_driver);
+}
+module_exit(altera_gpio_exit);
+
+MODULE_DESCRIPTION("Altera GPIO driver");
+MODULE_LICENSE("GPL");
--
1.7.11.GIT
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver
2013-06-17 7:29 ` Loh Tien Hock
@ 2013-06-17 9:12 ` Linus Walleij
0 siblings, 0 replies; 20+ messages in thread
From: Linus Walleij @ 2013-06-17 9:12 UTC (permalink / raw)
To: Loh Tien Hock; +Cc: Rob Herring, Rob Landley, linux-doc, devicetree-discuss
On Mon, Jun 17, 2013 at 9:29 AM, Loh Tien Hock <thloh@altera.com> wrote:
> On Mon, Jun 17, 2013 at 2:38 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Thu, Jun 6, 2013 at 10:05 AM, <thloh@altera.com> wrote:
>>> +static void altera_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>>> +{
>>> + struct altera_gpio_chip *altera_gc = irq_desc_get_handler_data(desc);
>>> + struct irq_chip *chip = irq_desc_get_chip(desc);
>>> + struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
>>> + unsigned long status;
>>> +
>>> + int base;
>>> + chip->irq_mask(&desc->irq_data);
>>> +
>>> + if (altera_gc->level_trigger) {
>>> + status = __raw_readl(mm_gc->regs + ALTERA_GPIO_DATA);
>>> + status &= __raw_readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
>>> +
>>> + for (base = 0; base < mm_gc->gc.ngpio; base++) {
>>> + if ((1 << base) & status) {
>>> + generic_handle_irq(irq_linear_revmap(
>>> + altera_gc->irq, base));
>>> + }
>>> + }
>>> + } else {
>>> + while ((status =
>>> + (__raw_readl(mm_gc->regs + ALTERA_GPIO_EDGE_CAP) &
>>> + __raw_readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK)))) {
>>> + __raw_writel(status,
>>> + mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
>>> + for (base = 0; base < mm_gc->gc.ngpio; base++) {
>>> + if ((1 << base) & status) {
>>> + generic_handle_irq(irq_linear_revmap(
>>> + altera_gc->irq, base));
>>> + }
>>> + }
>>> + }
>>> + }
>>> +
>>> + chip->irq_eoi(irq_desc_get_irq_data(desc));
>>> + chip->irq_unmask(&desc->irq_data);
>>> +}
>>
>> Why is the above code using __raw_* accessors?
>>
>> Atleast use readl_relaxed() instead of __raw.
>
> Noted.
>
> A question regarding readl_relaxed(), is that because the endianess
> swapping? If that's the case, should codes in other functions use
> readl_relaxed()/writel_relaxed() as well?
Basically I think of it like this:
__raw_* = will just poke the registers without really taking anything
into account, in native endianness.
readl/writel_relaxed() = same thing but always little-endian.
readl()/write() = always little-endian, and will go to lengths to also
drain buffers etc so the write hits the target registers after the PC
passed this instruction. To the extent possible, and may cause a
performance problem due to all buffer draining.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver
2013-06-17 7:10 ` Loh Tien Hock
@ 2013-06-17 7:29 ` Loh Tien Hock
2013-06-17 9:12 ` Linus Walleij
0 siblings, 1 reply; 20+ messages in thread
From: Loh Tien Hock @ 2013-06-17 7:29 UTC (permalink / raw)
To: Linus Walleij; +Cc: Rob Herring, Rob Landley, linux-doc, devicetree-discuss
Sorry for the spam, had a bad copy and paste in previous email.
On Mon, Jun 17, 2013 at 2:38 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Jun 6, 2013 at 10:05 AM, <thloh@altera.com> wrote:
>
>> From: Tien Hock Loh <thloh@altera.com>
>>
>> Add driver support for Altera GPIO soft IP, including interrupts and I/O.
>> Tested on Altera CV SoC board.
>>
>> Signed-off-by: Tien Hock Loh <thloh@altera.com>
> (...)
>
> This is looking much better, some remaining comments.
>
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
>> @@ -0,0 +1,36 @@
>> +Altera GPIO controller bindings
>> +
>> +Required properties:
>> +- compatible:
>> + - "altr,pio-1.0"
>
> I asked you to add this prefix to
> Documentation/devicetree/bindings/vendor-prefixes.txt
>
Noted, missed that out.
>> +- reg: Physical base address and length of the controller's registers.
>> +- #gpio-cells : Should be 1
>> + - first cell is the gpio offset number
>> +- gpio-controller : Marks the device node as a GPIO controller.
>> +- #interrupt-cells : Should be 1.
>> +- interrupts: Specify the interrupt.
>> +- interrupt-controller: Mark the device node as an interrupt controller
>> + The first cell is the GPIO number.
>
> How can that cell be the "GPIO number"? Surely it is the local
> interrupt offset number on the GPIO controller?
>
Yup, bad documentation. I'll update this.
> (...)
>> +++ b/drivers/gpio/gpio-altera.c
>
>> +static int altera_gpio_get(struct gpio_chip *gc, unsigned offset)
>> +{
>> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
>> +
>> + return (readl(mm_gc->regs + ALTERA_GPIO_DATA) >> offset) & 1;
>
> I usually would write that like this:
>
> #include <linux/bitops.h>
>
> return !!(readl(mm_gc->regs + ALTERA_GPIO_DATA) & BIT(offset));
>
> But no big deal.
>
>> + gpio_ddr |= (1 << offset);
>
> And with <linux/bitops.h> I would just:
>
> gpio_ddr |= BIT(offset);
Noted, that looks cleaner.
>
>> +static void altera_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>> +{
>> + struct altera_gpio_chip *altera_gc = irq_desc_get_handler_data(desc);
>> + struct irq_chip *chip = irq_desc_get_chip(desc);
>> + struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
>> + unsigned long status;
>> +
>> + int base;
>> + chip->irq_mask(&desc->irq_data);
>> +
>> + if (altera_gc->level_trigger) {
>> + status = __raw_readl(mm_gc->regs + ALTERA_GPIO_DATA);
>> + status &= __raw_readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
>> +
>> + for (base = 0; base < mm_gc->gc.ngpio; base++) {
>> + if ((1 << base) & status) {
>> + generic_handle_irq(irq_linear_revmap(
>> + altera_gc->irq, base));
>> + }
>> + }
>> + } else {
>> + while ((status =
>> + (__raw_readl(mm_gc->regs + ALTERA_GPIO_EDGE_CAP) &
>> + __raw_readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK)))) {
>> + __raw_writel(status,
>> + mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
>> + for (base = 0; base < mm_gc->gc.ngpio; base++) {
>> + if ((1 << base) & status) {
>> + generic_handle_irq(irq_linear_revmap(
>> + altera_gc->irq, base));
>> + }
>> + }
>> + }
>> + }
>> +
>> + chip->irq_eoi(irq_desc_get_irq_data(desc));
>> + chip->irq_unmask(&desc->irq_data);
>> +}
>
> Why is the above code using __raw_* accessors?
>
> Atleast use readl_relaxed() instead of __raw.
Noted.
A question regarding readl_relaxed(), is that because the endianess
swapping? If that's the case, should codes in other functions use
readl_relaxed()/writel_relaxed() as well?
>
>> + if (of_get_property(node, "interrupts", ®) == NULL)
>> + goto skip_irq;
>> + altera_gc->hwirq = irq_of_parse_and_map(node, 0);
>
> But wait. How can a hardware IRQ be something coming out
> of irq_of_parse_and_map()???
>
> The entire point of mapping an IRQ is to move it into some
> free Linux IRQ slot, it is as far away from a HW IRQ you
> ever get.
>
> Either rename the variable or rethink what you're doing here.
The variable naming came from gpio-mpc8xxx.c, I'll update this to a better name.
>
> Yours,
> Linus Walleij
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver
2013-06-17 6:38 ` Linus Walleij
@ 2013-06-17 7:10 ` Loh Tien Hock
2013-06-17 7:29 ` Loh Tien Hock
0 siblings, 1 reply; 20+ messages in thread
From: Loh Tien Hock @ 2013-06-17 7:10 UTC (permalink / raw)
To: Linus Walleij; +Cc: Rob Herring, Rob Landley, linux-doc, devicetree-discuss
On Mon, Jun 17, 2013 at 2:38 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Jun 6, 2013 at 10:05 AM, <thloh@altera.com> wrote:
>
>> From: Tien Hock Loh <thloh@altera.com>
>>
>> Add driver support for Altera GPIO soft IP, including interrupts and I/O.
>> Tested on Altera CV SoC board.
>>
>> Signed-off-by: Tien Hock Loh <thloh@altera.com>
> (...)
>
> This is looking much better, some remaining comments.
>
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
>> @@ -0,0 +1,36 @@
>> +Altera GPIO controller bindings
>> +
>> +Required properties:
>> +- compatible:
>> + - "altr,pio-1.0"
>
> I asked you to add this prefix to
> Documentation/devicetree/bindings/vendor-prefixes.txt
>
Noted, missed that out.
>> +- reg: Physical base address and length of the controller's registers.
>> +- #gpio-cells : Should be 1
>> + - first cell is the gpio offset number
>> +- gpio-controller : Marks the device node as a GPIO controller.
>> +- #interrupt-cells : Should be 1.
>> +- interrupts: Specify the interrupt.
>> +- interrupt-controller: Mark the device node as an interrupt controller
>> + The first cell is the GPIO number.
>
> How can that cell be the "GPIO number"? Surely it is the local
> interrupt offset number on the GPIO controller?
>
Yup, bad documentation. I'll update this.
> (...)
>> +++ b/drivers/gpio/gpio-altera.c
>
>> +static int altera_gpio_get(struct gpio_chip *gc, unsigned offset)
>> +{
>> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
>> +
>> + return (readl(mm_gc->regs + ALTERA_GPIO_DATA) >> offset) & 1;
>
> I usually would write that like this:
>
> #include <linux/bitops.h>
>
> return !!(readl(mm_gc->regs + ALTERA_GPIO_DATA) & BIT(offset));
>
> But no big deal.
>
>> + gpio_ddr |= (1 << offset);
>
> And with <linux/bitops.h> I would just:
>
> gpio_ddr |= BIT(offset);
Noted, that looks cleaner.
Noted.
A question regarding readl_relaxed(), is that because the endianess
swapping? If that's the case, should codes in other functions use
readl_relaxed()/writel_
relaxed() as well?
>
>> + if (of_get_property(node, "interrupts", ®) == NULL)
>> + goto skip_irq;
>> + altera_gc->hwirq = irq_of_parse_and_map(node, 0);
>
> But wait. How can a hardware IRQ be something coming out
> of irq_of_parse_and_map()???
>
> The entire point of mapping an IRQ is to move it into some
> free Linux IRQ slot, it is as far away from a HW IRQ you
> ever get.
>
> Either rename the variable or rethink what you're doing here.
The variable naming came from gpio-mpc8xxx.c, I'll update this to a better name.
>
> Yours,
> Linus Walleij
On Mon, Jun 17, 2013 at 2:38 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Jun 6, 2013 at 10:05 AM, <thloh@altera.com> wrote:
>
>> From: Tien Hock Loh <thloh@altera.com>
>>
>> Add driver support for Altera GPIO soft IP, including interrupts and I/O.
>> Tested on Altera CV SoC board.
>>
>> Signed-off-by: Tien Hock Loh <thloh@altera.com>
> (...)
>
> This is looking much better, some remaining comments.
>
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
>> @@ -0,0 +1,36 @@
>> +Altera GPIO controller bindings
>> +
>> +Required properties:
>> +- compatible:
>> + - "altr,pio-1.0"
>
> I asked you to add this prefix to
> Documentation/devicetree/bindings/vendor-prefixes.txt
>
>> +- reg: Physical base address and length of the controller's registers.
>> +- #gpio-cells : Should be 1
>> + - first cell is the gpio offset number
>> +- gpio-controller : Marks the device node as a GPIO controller.
>> +- #interrupt-cells : Should be 1.
>> +- interrupts: Specify the interrupt.
>> +- interrupt-controller: Mark the device node as an interrupt controller
>> + The first cell is the GPIO number.
>
> How can that cell be the "GPIO number"? Surely it is the local
> interrupt offset number on the GPIO controller?
>
> (...)
>> +++ b/drivers/gpio/gpio-altera.c
>
>> +static int altera_gpio_get(struct gpio_chip *gc, unsigned offset)
>> +{
>> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
>> +
>> + return (readl(mm_gc->regs + ALTERA_GPIO_DATA) >> offset) & 1;
>
> I usually would write that like this:
>
> #include <linux/bitops.h>
>
> return !!(readl(mm_gc->regs + ALTERA_GPIO_DATA) & BIT(offset));
>
> But no big deal.
>
>> + gpio_ddr |= (1 << offset);
>
> And with <linux/bitops.h> I would just:
>
> gpio_ddr |= BIT(offset);
>
>> +static void altera_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>> +{
>> + struct altera_gpio_chip *altera_gc = irq_desc_get_handler_data(desc);
>> + struct irq_chip *chip = irq_desc_get_chip(desc);
>> + struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
>> + unsigned long status;
>> +
>> + int base;
>> + chip->irq_mask(&desc->irq_data);
>> +
>> + if (altera_gc->level_trigger) {
>> + status = __raw_readl(mm_gc->regs + ALTERA_GPIO_DATA);
>> + status &= __raw_readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
>> +
>> + for (base = 0; base < mm_gc->gc.ngpio; base++) {
>> + if ((1 << base) & status) {
>> + generic_handle_irq(irq_linear_revmap(
>> + altera_gc->irq, base));
>> + }
>> + }
>> + } else {
>> + while ((status =
>> + (__raw_readl(mm_gc->regs + ALTERA_GPIO_EDGE_CAP) &
>> + __raw_readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK)))) {
>> + __raw_writel(status,
>> + mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
>> + for (base = 0; base < mm_gc->gc.ngpio; base++) {
>> + if ((1 << base) & status) {
>> + generic_handle_irq(irq_linear_revmap(
>> + altera_gc->irq, base));
>> + }
>> + }
>> + }
>> + }
>> +
>> + chip->irq_eoi(irq_desc_get_irq_data(desc));
>> + chip->irq_unmask(&desc->irq_data);
>> +}
>
> Why is the above code using __raw_* accessors?
>
> Atleast use readl_relaxed() instead of __raw.
>
>> + if (of_get_property(node, "interrupts", ®) == NULL)
>> + goto skip_irq;
>> + altera_gc->hwirq = irq_of_parse_and_map(node, 0);
>
> But wait. How can a hardware IRQ be something coming out
> of irq_of_parse_and_map()???
>
> The entire point of mapping an IRQ is to move it into some
> free Linux IRQ slot, it is as far away from a HW IRQ you
> ever get.
>
> Either rename the variable or rethink what you're doing here.
>
> Yours,
> Linus Walleij
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver
2013-06-06 8:05 thloh
@ 2013-06-17 6:38 ` Linus Walleij
2013-06-17 7:10 ` Loh Tien Hock
0 siblings, 1 reply; 20+ messages in thread
From: Linus Walleij @ 2013-06-17 6:38 UTC (permalink / raw)
To: Tien Hock Loh
Cc: Rob Herring, Rob Landley, linux-doc, devicetree-discuss, Loh Tien Hock
On Thu, Jun 6, 2013 at 10:05 AM, <thloh@altera.com> wrote:
> From: Tien Hock Loh <thloh@altera.com>
>
> Add driver support for Altera GPIO soft IP, including interrupts and I/O.
> Tested on Altera CV SoC board.
>
> Signed-off-by: Tien Hock Loh <thloh@altera.com>
(...)
This is looking much better, some remaining comments.
> +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
> @@ -0,0 +1,36 @@
> +Altera GPIO controller bindings
> +
> +Required properties:
> +- compatible:
> + - "altr,pio-1.0"
I asked you to add this prefix to
Documentation/devicetree/bindings/vendor-prefixes.txt
> +- reg: Physical base address and length of the controller's registers.
> +- #gpio-cells : Should be 1
> + - first cell is the gpio offset number
> +- gpio-controller : Marks the device node as a GPIO controller.
> +- #interrupt-cells : Should be 1.
> +- interrupts: Specify the interrupt.
> +- interrupt-controller: Mark the device node as an interrupt controller
> + The first cell is the GPIO number.
How can that cell be the "GPIO number"? Surely it is the local
interrupt offset number on the GPIO controller?
(...)
> +++ b/drivers/gpio/gpio-altera.c
> +static int altera_gpio_get(struct gpio_chip *gc, unsigned offset)
> +{
> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> +
> + return (readl(mm_gc->regs + ALTERA_GPIO_DATA) >> offset) & 1;
I usually would write that like this:
#include <linux/bitops.h>
return !!(readl(mm_gc->regs + ALTERA_GPIO_DATA) & BIT(offset));
But no big deal.
> + gpio_ddr |= (1 << offset);
And with <linux/bitops.h> I would just:
gpio_ddr |= BIT(offset);
> +static void altera_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> +{
> + struct altera_gpio_chip *altera_gc = irq_desc_get_handler_data(desc);
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
> + unsigned long status;
> +
> + int base;
> + chip->irq_mask(&desc->irq_data);
> +
> + if (altera_gc->level_trigger) {
> + status = __raw_readl(mm_gc->regs + ALTERA_GPIO_DATA);
> + status &= __raw_readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
> +
> + for (base = 0; base < mm_gc->gc.ngpio; base++) {
> + if ((1 << base) & status) {
> + generic_handle_irq(irq_linear_revmap(
> + altera_gc->irq, base));
> + }
> + }
> + } else {
> + while ((status =
> + (__raw_readl(mm_gc->regs + ALTERA_GPIO_EDGE_CAP) &
> + __raw_readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK)))) {
> + __raw_writel(status,
> + mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
> + for (base = 0; base < mm_gc->gc.ngpio; base++) {
> + if ((1 << base) & status) {
> + generic_handle_irq(irq_linear_revmap(
> + altera_gc->irq, base));
> + }
> + }
> + }
> + }
> +
> + chip->irq_eoi(irq_desc_get_irq_data(desc));
> + chip->irq_unmask(&desc->irq_data);
> +}
Why is the above code using __raw_* accessors?
Atleast use readl_relaxed() instead of __raw.
> + if (of_get_property(node, "interrupts", ®) == NULL)
> + goto skip_irq;
> + altera_gc->hwirq = irq_of_parse_and_map(node, 0);
But wait. How can a hardware IRQ be something coming out
of irq_of_parse_and_map()???
The entire point of mapping an IRQ is to move it into some
free Linux IRQ slot, it is as far away from a HW IRQ you
ever get.
Either rename the variable or rethink what you're doing here.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver
@ 2013-06-06 8:05 thloh
2013-06-17 6:38 ` Linus Walleij
0 siblings, 1 reply; 20+ messages in thread
From: thloh @ 2013-06-06 8:05 UTC (permalink / raw)
To: linus.walleij, rob.herring, rob
Cc: linux-doc, devicetree-discuss, thloh85, thloh
From: Tien Hock Loh <thloh@altera.com>
Add driver support for Altera GPIO soft IP, including interrupts and I/O.
Tested on Altera CV SoC board.
Signed-off-by: Tien Hock Loh <thloh@altera.com>
---
.../devicetree/bindings/gpio/gpio-altera.txt | 36 ++
drivers/gpio/Kconfig | 7 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-altera.c | 405 +++++++++++++++++++++
4 files changed, 449 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-altera.txt
create mode 100644 drivers/gpio/gpio-altera.c
diff --git a/Documentation/devicetree/bindings/gpio/gpio-altera.txt b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
new file mode 100644
index 0000000..dc85a04
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
@@ -0,0 +1,36 @@
+Altera GPIO controller bindings
+
+Required properties:
+- compatible:
+ - "altr,pio-1.0"
+- reg: Physical base address and length of the controller's registers.
+- #gpio-cells : Should be 1
+ - first cell is the gpio offset number
+- gpio-controller : Marks the device node as a GPIO controller.
+- #interrupt-cells : Should be 1.
+- interrupts: Specify the interrupt.
+- interrupt-controller: Mark the device node as an interrupt controller
+ The first cell is the GPIO number.
+
+Altera GPIO specific properties:
+- width: Width of the GPIO bank, range from 1-32
+- level_trigger: Specifies whether the GPIO interrupt is level trigger (high).
+ This field is required if the Altera GPIO controller used has IRQ enabled.
+- edge_type: Specifies edge type if the GPIO interrupt is edge trigger:
+ 0 = Rising edge
+ 1 = Falling edge
+ 2 = Both edge
+
+Example:
+
+gpio_altr: gpio_altr {
+ compatible = "altr,pio-1.0";
+ reg = <0xff200000 0x10>;
+ interrupts = <0 45 4>;
+ width = <32>;
+ level_trigger = <0>;
+ #gpio-cells = <1>;
+ gpio-controller;
+ #interrupt-cells = <1>;
+ interrupt-controller;
+};
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 573c449..2be1b45 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -118,6 +118,13 @@ config GPIO_GENERIC_PLATFORM
help
Say yes here to support basic platform_device memory-mapped GPIO controllers.
+config GPIO_ALTERA
+ tristate "Altera GPIO"
+ select IRQ_DOMAIN
+ depends on OF_GPIO
+ help
+ Say yes here to support the Altera PIO device.
+
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 0cb2d65..695b954 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_GPIO_74X164) += gpio-74x164.o
obj-$(CONFIG_GPIO_ADNP) += gpio-adnp.o
obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o
obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o
+obj-$(CONFIG_GPIO_ALTERA) += gpio-altera.o
obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o
obj-$(CONFIG_GPIO_ARIZONA) += gpio-arizona.o
obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
diff --git a/drivers/gpio/gpio-altera.c b/drivers/gpio/gpio-altera.c
new file mode 100644
index 0000000..38eb6cf
--- /dev/null
+++ b/drivers/gpio/gpio-altera.c
@@ -0,0 +1,405 @@
+/*
+ * Copyright (C) 2013 Altera Corporation
+ * Based on gpio-mpc8xxx.c
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2. This program is licensed "as is" without any warranty of any
+ * kind, whether express or implied.
+ */
+
+#include <linux/gpio.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/of_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/interrupt.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+
+#define ALTERA_GPIO_DATA 0x0
+#define ALTERA_GPIO_DIR 0x4
+#define ALTERA_GPIO_IRQ_MASK 0x8
+#define ALTERA_GPIO_EDGE_CAP 0xc
+#define ALTERA_GPIO_OUTSET 0x10
+#define ALTERA_GPIO_OUTCLEAR 0x14
+#define ALTERA_IRQ_RISING 0
+#define ALTERA_IRQ_FALLING 1
+#define ALTERA_IRQ_BOTH 2
+
+/**
+* struct altera_gpio_chip
+* @mmchip : memory mapped chip structure.
+* @irq : irq domain that this driver is registered to.
+* @gpio_lock : synchronization lock so that new irq/set/get requests
+ will be blocked until the current one completes.
+* @level_trigger : specifies if the controller is level triggered
+ (high).
+* @edge_type : specify the edge type of the controller (rising,
+ falling, both). Only used for !level_trigger.
+* @hwirq : kernel mapped hwirq.
+*/
+struct altera_gpio_chip {
+ struct of_mm_gpio_chip mmchip;
+ struct irq_domain *irq;
+ spinlock_t gpio_lock;
+ int level_trigger;
+ int edge_type;
+ int hwirq;
+};
+
+static void altera_gpio_irq_unmask(struct irq_data *d)
+{
+ struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
+ struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
+ unsigned long flags;
+ unsigned int intmask;
+
+ spin_lock_irqsave(&altera_gc->gpio_lock, flags);
+ intmask = readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+ /* Set ALTERA_GPIO_IRQ_MASK bit to unmask */
+ intmask |= (1 << irqd_to_hwirq(d));
+ writel(intmask, mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+ spin_unlock_irqrestore(&altera_gc->gpio_lock, flags);
+}
+
+static void altera_gpio_irq_mask(struct irq_data *d)
+{
+ struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
+ struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
+ unsigned long flags;
+ unsigned int intmask;
+
+ spin_lock_irqsave(&altera_gc->gpio_lock, flags);
+ intmask = readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+ /* Clear ALTERA_GPIO_IRQ_MASK bit to mask */
+ intmask &= ~(1 << irqd_to_hwirq(d));
+ writel(intmask, mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+ spin_unlock_irqrestore(&altera_gc->gpio_lock, flags);
+}
+
+static int altera_gpio_irq_set_type(struct irq_data *d,
+ unsigned int type)
+{
+ struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
+
+ if (type == IRQ_TYPE_NONE)
+ return 0;
+
+ if (altera_gc->level_trigger) {
+ if (type == IRQ_TYPE_LEVEL_HIGH)
+ return 0;
+ } else {
+ if (type == IRQ_TYPE_EDGE_RISING &&
+ altera_gc->edge_type == ALTERA_IRQ_RISING)
+ return 0;
+ else if (type == IRQ_TYPE_EDGE_FALLING &&
+ altera_gc->edge_type == ALTERA_IRQ_FALLING)
+ return 0;
+ else if (type == IRQ_TYPE_EDGE_BOTH &&
+ altera_gc->edge_type == ALTERA_IRQ_BOTH)
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
+static struct irq_chip altera_irq_chip = {
+ .name = "altera-gpio",
+ .irq_mask = altera_gpio_irq_mask,
+ .irq_unmask = altera_gpio_irq_unmask,
+ .irq_set_type = altera_gpio_irq_set_type,
+};
+
+static int altera_gpio_get(struct gpio_chip *gc, unsigned offset)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+
+ return (readl(mm_gc->regs + ALTERA_GPIO_DATA) >> offset) & 1;
+}
+
+static void altera_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct altera_gpio_chip *chip = container_of(mm_gc,
+ struct altera_gpio_chip, mmchip);
+ unsigned long flags;
+ unsigned int data_reg;
+
+ spin_lock_irqsave(&chip->gpio_lock, flags);
+ data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA);
+ data_reg = (data_reg & ~(1 << offset)) | (value << offset);
+ writel(data_reg, mm_gc->regs + ALTERA_GPIO_DATA);
+ spin_unlock_irqrestore(&chip->gpio_lock, flags);
+}
+
+static int altera_gpio_direction_input(struct gpio_chip *gc, unsigned offset)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct altera_gpio_chip *chip = container_of(mm_gc,
+ struct altera_gpio_chip, mmchip);
+ unsigned long flags;
+ unsigned int gpio_ddr;
+
+ spin_lock_irqsave(&chip->gpio_lock, flags);
+ /* Set pin as input, assumes software controlled IP */
+ gpio_ddr = readl(mm_gc->regs + ALTERA_GPIO_DIR);
+ gpio_ddr &= ~(1 << offset);
+ writel(gpio_ddr, mm_gc->regs + ALTERA_GPIO_DIR);
+ spin_unlock_irqrestore(&chip->gpio_lock, flags);
+
+ return 0;
+}
+
+static int altera_gpio_direction_output(struct gpio_chip *gc,
+ unsigned offset, int value)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct altera_gpio_chip *chip = container_of(mm_gc,
+ struct altera_gpio_chip, mmchip);
+ unsigned long flags;
+ unsigned int data_reg, gpio_ddr;
+
+ spin_lock_irqsave(&chip->gpio_lock, flags);
+ /* Sets the GPIO value */
+ data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA);
+ data_reg = (data_reg & ~(1 << offset)) | (value << offset);
+ writel(data_reg, mm_gc->regs + ALTERA_GPIO_DATA);
+
+ /* Set pin as output, assumes software controlled IP */
+ gpio_ddr = readl(mm_gc->regs + ALTERA_GPIO_DIR);
+ gpio_ddr |= (1 << offset);
+ writel(gpio_ddr, mm_gc->regs + ALTERA_GPIO_DIR);
+ spin_unlock_irqrestore(&chip->gpio_lock, flags);
+
+ return 0;
+}
+
+static int altera_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct altera_gpio_chip *altera_gc = container_of(mm_gc,
+ struct altera_gpio_chip, mmchip);
+
+ if (altera_gc->irq == 0)
+ return -ENXIO;
+ if ((altera_gc->irq && offset) < altera_gc->mmchip.gc.ngpio)
+ return irq_create_mapping(altera_gc->irq, offset);
+ else
+ return -ENXIO;
+}
+
+static void altera_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
+{
+ struct altera_gpio_chip *altera_gc = irq_desc_get_handler_data(desc);
+ struct irq_chip *chip = irq_desc_get_chip(desc);
+ struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
+ unsigned long status;
+
+ int base;
+ chip->irq_mask(&desc->irq_data);
+
+ if (altera_gc->level_trigger) {
+ status = __raw_readl(mm_gc->regs + ALTERA_GPIO_DATA);
+ status &= __raw_readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+
+ for (base = 0; base < mm_gc->gc.ngpio; base++) {
+ if ((1 << base) & status) {
+ generic_handle_irq(irq_linear_revmap(
+ altera_gc->irq, base));
+ }
+ }
+ } else {
+ while ((status =
+ (__raw_readl(mm_gc->regs + ALTERA_GPIO_EDGE_CAP) &
+ __raw_readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK)))) {
+ __raw_writel(status,
+ mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
+ for (base = 0; base < mm_gc->gc.ngpio; base++) {
+ if ((1 << base) & status) {
+ generic_handle_irq(irq_linear_revmap(
+ altera_gc->irq, base));
+ }
+ }
+ }
+ }
+
+ chip->irq_eoi(irq_desc_get_irq_data(desc));
+ chip->irq_unmask(&desc->irq_data);
+}
+
+static int altera_gpio_irq_map(struct irq_domain *h, unsigned int virq,
+ irq_hw_number_t hw_irq_num)
+{
+ irq_set_chip_data(virq, h->host_data);
+ irq_set_chip_and_handler(virq, &altera_irq_chip, handle_level_irq);
+ irq_set_irq_type(virq, IRQ_TYPE_NONE);
+
+ return 0;
+}
+
+static struct irq_domain_ops altera_gpio_irq_ops = {
+ .map = altera_gpio_irq_map,
+ .xlate = irq_domain_xlate_onecell,
+};
+
+int altera_gpio_probe(struct platform_device *pdev)
+{
+ struct device_node *node = pdev->dev.of_node;
+ int id, reg, ret;
+ struct altera_gpio_chip *altera_gc = devm_kzalloc(&pdev->dev,
+ sizeof(*altera_gc), GFP_KERNEL);
+ if (altera_gc == NULL) {
+ pr_err("%s: out of memory\n", node->full_name);
+ return -ENOMEM;
+ }
+ altera_gc->irq = 0;
+
+ spin_lock_init(&altera_gc->gpio_lock);
+
+ id = pdev->id;
+
+ if (of_property_read_u32(node, "width", ®))
+ /*By default assume full GPIO controller*/
+ altera_gc->mmchip.gc.ngpio = 32;
+ else
+ altera_gc->mmchip.gc.ngpio = reg;
+
+ if (altera_gc->mmchip.gc.ngpio > 32) {
+ pr_warn("%s: ngpio is greater than 32, defaulting to 32\n",
+ node->full_name);
+ altera_gc->mmchip.gc.ngpio = 32;
+ }
+
+ altera_gc->mmchip.gc.direction_input = altera_gpio_direction_input;
+ altera_gc->mmchip.gc.direction_output = altera_gpio_direction_output;
+ altera_gc->mmchip.gc.get = altera_gpio_get;
+ altera_gc->mmchip.gc.set = altera_gpio_set;
+ altera_gc->mmchip.gc.to_irq = altera_gpio_to_irq;
+ altera_gc->mmchip.gc.owner = THIS_MODULE;
+
+ ret = of_mm_gpiochip_add(node, &altera_gc->mmchip);
+ if (ret) {
+ pr_err("%s: Failed adding memory mapped gpiochip\n",
+ node->full_name);
+ return ret;
+ }
+
+ platform_set_drvdata(pdev, altera_gc);
+
+ if (of_get_property(node, "interrupts", ®) == NULL)
+ goto skip_irq;
+ altera_gc->hwirq = irq_of_parse_and_map(node, 0);
+
+ if (altera_gc->hwirq == NO_IRQ)
+ goto skip_irq;
+
+ altera_gc->irq = irq_domain_add_linear(node, altera_gc->mmchip.gc.ngpio,
+ &altera_gpio_irq_ops, altera_gc);
+
+ if (!altera_gc->irq) {
+ ret = -ENODEV;
+ goto dispose_irq;
+ }
+
+ if (of_property_read_u32(node, "level_trigger", ®)) {
+ ret = -EINVAL;
+ pr_err("%s: level_trigger value not set in device tree\n",
+ node->full_name);
+ goto teardown;
+ }
+ altera_gc->level_trigger = reg;
+
+ /*
+ * If it is not level triggered PIO
+ * Check what edge type it is
+ */
+ if (!altera_gc->level_trigger) {
+ if (of_property_read_u32(node, "edge_type", ®)) {
+ ret = -EINVAL;
+ pr_err("%s: edge_type value not set in device tree\n"
+ , node->full_name);
+ goto teardown;
+ }
+ }
+ altera_gc->edge_type = reg;
+
+ irq_set_handler_data(altera_gc->hwirq, altera_gc);
+ irq_set_chained_handler(altera_gc->hwirq, altera_gpio_irq_handler);
+
+ return 0;
+
+teardown:
+ irq_domain_remove(altera_gc->irq);
+dispose_irq:
+ irq_dispose_mapping(altera_gc->hwirq);
+ WARN_ON(gpiochip_remove(&altera_gc->mmchip.gc) < 0);
+
+ pr_err("%s: registration failed with status %d\n",
+ node->full_name, ret);
+
+ return ret;
+skip_irq:
+ return 0;
+}
+
+static int altera_gpio_remove(struct platform_device *pdev)
+{
+ unsigned int irq, i;
+ int status;
+ struct altera_gpio_chip *altera_gc = platform_get_drvdata(pdev);
+
+ status = gpiochip_remove(&altera_gc->mmchip.gc);
+
+ if (status < 0)
+ return status;
+
+ if (altera_gc->irq) {
+ irq_dispose_mapping(altera_gc->hwirq);
+
+ for (i = 0; i < altera_gc->mmchip.gc.ngpio; i++) {
+ irq = irq_find_mapping(altera_gc->irq, i);
+ if (irq > 0)
+ irq_dispose_mapping(irq);
+ }
+
+ irq_domain_remove(altera_gc->irq);
+ }
+
+ irq_set_handler_data(altera_gc->hwirq, NULL);
+ irq_set_chained_handler(altera_gc->hwirq, NULL);
+ return -EIO;
+}
+
+static struct of_device_id altera_gpio_of_match[] = {
+ { .compatible = "altr,pio-1.0", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, altera_gpio_of_match);
+
+static struct platform_driver altera_gpio_driver = {
+ .driver = {
+ .name = "altera_gpio",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(altera_gpio_of_match),
+ },
+ .probe = altera_gpio_probe,
+ .remove = altera_gpio_remove,
+};
+
+static int __init altera_gpio_init(void)
+{
+ return platform_driver_register(&altera_gpio_driver);
+}
+subsys_initcall(altera_gpio_init);
+
+static void __exit altera_gpio_exit(void)
+{
+ platform_driver_unregister(&altera_gpio_driver);
+}
+module_exit(altera_gpio_exit);
+
+MODULE_DESCRIPTION("Altera GPIO driver");
+MODULE_LICENSE("GPL");
--
1.7.11.GIT
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver
2013-05-20 18:02 ` Linus Walleij
@ 2013-06-05 8:05 ` Loh Tien Hock
0 siblings, 0 replies; 20+ messages in thread
From: Loh Tien Hock @ 2013-06-05 8:05 UTC (permalink / raw)
To: Linus Walleij
Cc: Tien Hock Loh, Rob Herring, Rob Landley, linux-doc, devicetree-discuss
On Tue, May 21, 2013 at 2:02 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, May 8, 2013 at 4:39 AM, Tien Hock Loh <thloh@altera.com> wrote:
>
> > Add driver support for Altera GPIO soft IP, including interrupts and I/O.
> > Tested on Altera CV SoC board.
> >
> > Signed-off-by: Tien Hock Loh <thloh@altera.com>
> (...)
>
> Note: I have come to realize that there is apparently almost zero
> review on the devicetree-discuss list, is there anyone in the world
> reviewing new device tree bindings for generality, or is it all up to
> Linux subsystem maintainers like myself to take care of this?
>
> > +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
> > @@ -0,0 +1,40 @@
> > +Altera GPIO controller bindings
> > +
> > +Required properties:
> > +- compatible:
> > + - "altr,pio-1.0"
>
> This vendor prefix does not exist in
> Documentation/devicetree/bindings/vendor-prefixes.txt
> so atleast add it.
>
> I don't see the point in abbreviating "altera" to "altr" to
> save two letters, use the full name of the company!
"altr" is used for all other Altera soft IP driver currently in
kernel.org (such as drivers/tty/serial/altera_jtaguart.c and
altera_uart.c), thus for consistency, we are using "altr" instead of
"altera".
>
> > +- reg: Physical base address and length of the controller's registers.
> > +- #gpio-cells : Should be two.
> > + - first cell is the gpio offset number
> > + - second cell is used to specify optional parameters (unused)
>
> So why do you add it? Because all other GPIO controllers
> use it? Or because you will use it for something later?
>
My bad, I copied over from other GPIO controller and assumed that is
how things works. I'll update this.
> > +- gpio-controller : Marks the device node as a GPIO controller.
> > +- #interrupt-cells : Should be 1.
> > +- interrupts: Specify the interrupt.
> > +- interrupt-controller: Mark the device node as an interrupt controller
> > + The first cell is the GPIO number.
> > +Altera GPIO specific properties:
> > +- width: Width of the GPIO bank, range from 1-32
> > +- level_trigger: Specifies whether the GPIO interrupt is level trigger.
> > + This field is required if the Altera GPIO controller used has IRQ enabled.
>
> Is this how other GPIO controllers do it?
Altera GPIO is configurable during hardware generation time, and
cannot be changed in software. Other GPIOs are different because they
have a software configurable trigger, thus we need this.
>
> > + Note: This field is does not specify whether Altera GPIO controller is
> > + level or edge trigger. It is used to detect internally how the trigger is
> > + handled.
>
> I don't get it, but I guess reading the code will. I suspect this
> needs editing since DT bindings shall be *reusable* without
> having to read the code to understand them.
>
> Explain exactly what you mean when you say it is "used to
> detect internally how the trigger is handled". Used how?
> Handled how? Internally to what?
Sorry that the note is a little ambiguous. I'll update this (as what
you replied below as well on the trigger handling).
>
> > +Note: If the Altera GPIO is set to be built as a module, peripherals that uses
> > +Altera GPIO as interrupt-parent should be a module so that the peripheral
> > +doesn't get initialized before Altera GPIO is initialized.
>
> Delete this. We do not put Linux-specific information into the
> device tree bindings.
>
Noted.
> (...)
> > +++ b/drivers/gpio/gpio-altera.c
> (...)
> > +#include <linux/platform_device.h>
>
> Suggest
> #include <linux/basic_mmio_gpio.h>
>
> > +#define ALTERA_GPIO_DATA 0x0
> > +#define ALTERA_GPIO_DIR 0x4
> > +#define ALTERA_GPIO_IRQ_MASK 0x8
> > +#define ALTERA_GPIO_EDGE_CAP 0xc
> > +#define ALTERA_GPIO_OUTSET 0x10
> > +#define ALTERA_GPIO_OUTCLEAR 0x14
>
> Looking at this is is pretty apparent that you shall use
> the generic basic MMIO GPIO helper library.
>
> In Kconfig:
> select GPIO_GENERIC
>
> Include #include <linux/basic_mmio_gpio.h>
>
> See other GPIO drivers to figure out how to reuse the
> generic MMIO GPIO helpers. For example the new
> GRGPIO driver, drivers/gpio/gpio-grgpio.c.
>
> > +struct altera_gpio_chip {
> > + struct of_mm_gpio_chip mmchip;
> > + struct irq_domain *irq; /* GPIO controller IRQ number */
> > + spinlock_t gpio_lock; /* Lock used for synchronization */
> > + bool level_trigger;
> > + int hwirq;
> > +};
>
> Convert inline documentation to kerneldoc.
> Document the rest of the members as well because
> I don't understand them, and the documentation for the
> irqdomain is plain wrong, the other comment is
> just stating the obvious: tell us *what* is it locking
> and *why*.
Okay.
>
> (...)
> > +static int altera_gpio_irq_set_type(struct irq_data *d,
> > + unsigned int type)
> > +{
> > + /* There is no way to know the soft IP configuration,
> > + so we accept any interrupt types that are supported by the soft
> > + IP. Nothing to do here since it is hardware defined. Just return
> > + OK status to the caller. */
>
> Eh what? Why don't you say pass in the capabilities from
> the device tree then, if the hardware cannot say what it is
> capable of, something else has to.
I've updated Altera's tool to export this parameter to device tree
generated so that we can handle this better.
I'll update this in the next patch.
>
> /*
> * Please use this multiline comment
> * style, it is easier to read.
> */
Understood.
>
> > + if (type == IRQ_TYPE_EDGE_RISING ||
> > + type == IRQ_TYPE_EDGE_FALLING ||
> > + type == IRQ_TYPE_EDGE_BOTH ||
> > + type == IRQ_TYPE_NONE) {
> > + return 0;
> > + }
>
> This is not OK if it is unclear whether this is all supported.
>
> Get that info from somewhere.
>
> Apparently this is exactly what the level trigger field in
> the device tree defines.
>
> Especially handling both edges is quite tricky, have you
> actually tested this?
As above, I'll update this in the next patch. Yes, the level trigger
tells if it is a level triggered. I'll add an edge_type field to
identify when it is not level triggered, what is the edge type
(rising, falling, both).
I've tested "both edges" on a dev kit to control a LED blinking and it
worked correctly.
>
> (...)
> > +static int altera_gpio_get(struct gpio_chip *gc, unsigned offset)
> > +static void altera_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
> > +static int altera_gpio_direction_input(struct gpio_chip *gc, unsigned offset)
> > +static int altera_gpio_direction_output(struct gpio_chip *gc,
> > + unsigned offset, int value)
>
> These look like they could all use basic_mmio_gpio instead
> of being verbatim copies of generic code.
I couldn't update this because the bgpio_init doesn't really support
ngpio that is other than 8, 16, 32, and 64. In Altera GPIO IP, the
ngpio can be anywhere between 1-32.
Please advise if you disagree.
>
> > +static int altera_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
> > +static void altera_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> > +{
> > + struct altera_gpio_chip *altera_gc = irq_desc_get_handler_data(desc);
> > + struct irq_chip *chip = irq_desc_get_chip(desc);
> > + struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
> > + unsigned long status;
> > +
> > + int base;
> > + chip->irq_mask(&desc->irq_data);
> > +
> > + if (altera_gc->level_trigger == ALTERA_TRIGGER_LEVEL_HIGH) {
> > +{
> > + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> > + struct altera_gpio_chip *altera_gc = container_of(mm_gc,
> > + struct altera_gpio_chip, mmchip);
> > +
> > + if (altera_gc->irq == 0)
> > + return -ENXIO;
> > + if ((altera_gc->irq && offset) < altera_gc->mmchip.gc.ngpio)
> > + return irq_create_mapping(altera_gc->irq, offset);
> > + else
> > + return -ENXIO;
> > +}
> > +
>
> So here it is pretty clear what this thing in the device tree
> means. It means "I am a level triggered chip".
Yes, will do.
>
> > + status = readl(mm_gc->regs + ALTERA_GPIO_DATA);
> > + status &= readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
> > +
> > + for (base = 0; base < mm_gc->gc.ngpio; base++) {
> > + if ((1 << base) & status) {
> > + generic_handle_irq(irq_linear_revmap(
> > + altera_gc->irq, base));
> > + }
> > + }
> > + } else {
>
> And if I'm not level triggered I'm edge triggered, right?
Yup, will update this.
>
> > + while ((status = (readl(mm_gc->regs + ALTERA_GPIO_EDGE_CAP) &
> > + readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK)))) {
> > + writel(status, mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
> > + for (base = 0; base < mm_gc->gc.ngpio; base++) {
> > + if ((1 << base) & status) {
> > + generic_handle_irq(irq_linear_revmap(
> > + altera_gc->irq, base));
> > + }
> > + }
> > + }
> > + }
> > +
> > + chip->irq_eoi(irq_desc_get_irq_data(desc));
> > + chip->irq_unmask(&desc->irq_data);
> > +}
>
> (...)
> > +int altera_gpio_probe(struct platform_device *pdev)
> > +{
> > + struct device_node *node = pdev->dev.of_node;
> > + int id, reg, ret;
> > + struct altera_gpio_chip *altera_gc = devm_kzalloc(&pdev->dev,
> > + sizeof(*altera_gc), GFP_KERNEL);
> > + if (altera_gc == NULL) {
> > + ret = -ENOMEM;
> > + pr_err("%s: registration failed with status %d\n",
> > + node->full_name, ret);
> > + return ret;
>
> dev_err(&pdev->dev, "%s out of memory\n");
> return -ENOMEM;
>
> So:
>
> - Use dev_* print primitives
>
> - Print the actual error, or we go back to the 1980ies
> habit of having to look up an abstract error code in a
> binder.
>
> - Just return the error.
Understood.
>
>
> > + }
> > + altera_gc->irq = 0;
> > +
> > + spin_lock_init(&altera_gc->gpio_lock);
> > +
> > + id = pdev->id;
> > +
> > + if (of_property_read_u32(node, "width", ®))
> > + /*By default assume full GPIO controller*/
> > + altera_gc->mmchip.gc.ngpio = 32;
> > + else
> > + altera_gc->mmchip.gc.ngpio = reg;
> > +
> > + if (altera_gc->mmchip.gc.ngpio > 32) {
> > + pr_warn("%s: ngpio is greater than 32, defaulting to 32\n",
> > + node->full_name);
> > + altera_gc->mmchip.gc.ngpio = 32;
> > + }
> > +
> > + altera_gc->mmchip.gc.direction_input = altera_gpio_direction_input;
> > + altera_gc->mmchip.gc.direction_output = altera_gpio_direction_output;
> > + altera_gc->mmchip.gc.get = altera_gpio_get;
> > + altera_gc->mmchip.gc.set = altera_gpio_set;
> > + altera_gc->mmchip.gc.to_irq = altera_gpio_to_irq;
> > + altera_gc->mmchip.gc.owner = THIS_MODULE;
> > +
> > + ret = of_mm_gpiochip_add(node, &altera_gc->mmchip);
> > + if (ret)
> > + goto err;
>
> Just print the error and return directly.
>
> There is nothing to free or clean up at the err: label.
Okay
>
> > + platform_set_drvdata(pdev, altera_gc);
> > +
> > + if (of_get_property(node, "interrupts", ®) == NULL)
> > + goto skip_irq;
> > + altera_gc->hwirq = irq_of_parse_and_map(node, 0);
> > +
> > + if (altera_gc->hwirq == NO_IRQ)
> > + goto skip_irq;
> > +
> > + altera_gc->irq = irq_domain_add_linear(node, altera_gc->mmchip.gc.ngpio,
> > + &altera_gpio_irq_ops, altera_gc);
> > +
> > + if (!altera_gc->irq) {
> > + ret = -ENODEV;
> > + goto dispose_irq;
> > + }
> > +
> > + if (of_property_read_u32(node, "level_trigger", ®)) {
> > + ret = -EINVAL;
> > + pr_err("%s: level_trigger value not set in device tree\n",
> > + node->full_name);
> > + goto teardown;
> > + }
> > + altera_gc->level_trigger = reg;
> > +
> > + irq_set_handler_data(altera_gc->hwirq, altera_gc);
> > + irq_set_chained_handler(altera_gc->hwirq, altera_gpio_irq_handler);
> > +
> > + return 0;
> > +
> > +teardown:
> > + irq_domain_remove(altera_gc->irq);
> > +dispose_irq:
> > + irq_dispose_mapping(altera_gc->hwirq);
> > + WARN_ON(gpiochip_remove(&altera_gc->mmchip.gc) < 0);
> > +
> > +err:
> > + pr_err("%s: registration failed with status %d\n",
> > + node->full_name, ret);
> > + devm_kfree(&pdev->dev, altera_gc);
>
> No. The devm_* managed resources take care of freeing when probe
> fails, that is the major idea with it.
>
Understood
> > +
> > + return ret;
> > +skip_irq:
> > + return 0;
> > +}
> > +
> > +static int altera_gpio_remove(struct platform_device *pdev)
> > +{
> > + unsigned int irq, i;
> > + int status;
> > + struct altera_gpio_chip *altera_gc = platform_get_drvdata(pdev);
> > +
> > + status = gpiochip_remove(&altera_gc->mmchip.gc);
> > +
> > + if (status < 0)
> > + return status;
> > +
> > + if (altera_gc->irq) {
> > + irq_dispose_mapping(altera_gc->hwirq);
> > +
> > + for (i = 0; i < altera_gc->mmchip.gc.ngpio; i++) {
> > + irq = irq_find_mapping(altera_gc->irq, i);
> > + if (irq > 0)
> > + irq_dispose_mapping(irq);
> > + }
> > +
> > + irq_domain_remove(altera_gc->irq);
> > + }
> > +
> > + irq_set_handler_data(altera_gc->hwirq, NULL);
> > + irq_set_chained_handler(altera_gc->hwirq, NULL);
> > + devm_kfree(&pdev->dev, altera_gc);
>
> Again no. You're missing the point with devm*.
Understood.
>
> > +#ifdef CONFIG_OF
> > +static struct of_device_id altera_gpio_of_match[] = {
> > + { .compatible = "altr,pio-1.0", },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, altera_gpio_of_match);
> > +#else
> > +#define altera_gpio_of_match NULL
> > +#endif
>
> Your driver depends on OF_GPIO, which depends on OF.
>
> Skip the #ifdefs.
Will update this.
>
> Yours,
> Linus Walleij
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver
2013-05-08 2:39 Tien Hock Loh
@ 2013-05-20 18:02 ` Linus Walleij
2013-06-05 8:05 ` Loh Tien Hock
0 siblings, 1 reply; 20+ messages in thread
From: Linus Walleij @ 2013-05-20 18:02 UTC (permalink / raw)
To: Tien Hock Loh
Cc: Rob Herring, Rob Landley, linux-doc, devicetree-discuss, thloh85
On Wed, May 8, 2013 at 4:39 AM, Tien Hock Loh <thloh@altera.com> wrote:
> Add driver support for Altera GPIO soft IP, including interrupts and I/O.
> Tested on Altera CV SoC board.
>
> Signed-off-by: Tien Hock Loh <thloh@altera.com>
(...)
Note: I have come to realize that there is apparently almost zero
review on the devicetree-discuss list, is there anyone in the world
reviewing new device tree bindings for generality, or is it all up to
Linux subsystem maintainers like myself to take care of this?
> +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
> @@ -0,0 +1,40 @@
> +Altera GPIO controller bindings
> +
> +Required properties:
> +- compatible:
> + - "altr,pio-1.0"
This vendor prefix does not exist in
Documentation/devicetree/bindings/vendor-prefixes.txt
so atleast add it.
I don't see the point in abbreviating "altera" to "altr" to
save two letters, use the full name of the company!
> +- reg: Physical base address and length of the controller's registers.
> +- #gpio-cells : Should be two.
> + - first cell is the gpio offset number
> + - second cell is used to specify optional parameters (unused)
So why do you add it? Because all other GPIO controllers
use it? Or because you will use it for something later?
> +- gpio-controller : Marks the device node as a GPIO controller.
> +- #interrupt-cells : Should be 1.
> +- interrupts: Specify the interrupt.
> +- interrupt-controller: Mark the device node as an interrupt controller
> + The first cell is the GPIO number.
> +Altera GPIO specific properties:
> +- width: Width of the GPIO bank, range from 1-32
> +- level_trigger: Specifies whether the GPIO interrupt is level trigger.
> + This field is required if the Altera GPIO controller used has IRQ enabled.
Is this how other GPIO controllers do it?
> + Note: This field is does not specify whether Altera GPIO controller is
> + level or edge trigger. It is used to detect internally how the trigger is
> + handled.
I don't get it, but I guess reading the code will. I suspect this
needs editing since DT bindings shall be *reusable* without
having to read the code to understand them.
Explain exactly what you mean when you say it is "used to
detect internally how the trigger is handled". Used how?
Handled how? Internally to what?
> +Note: If the Altera GPIO is set to be built as a module, peripherals that uses
> +Altera GPIO as interrupt-parent should be a module so that the peripheral
> +doesn't get initialized before Altera GPIO is initialized.
Delete this. We do not put Linux-specific information into the
device tree bindings.
(...)
> +++ b/drivers/gpio/gpio-altera.c
(...)
> +#include <linux/platform_device.h>
Suggest
#include <linux/basic_mmio_gpio.h>
> +#define ALTERA_GPIO_DATA 0x0
> +#define ALTERA_GPIO_DIR 0x4
> +#define ALTERA_GPIO_IRQ_MASK 0x8
> +#define ALTERA_GPIO_EDGE_CAP 0xc
> +#define ALTERA_GPIO_OUTSET 0x10
> +#define ALTERA_GPIO_OUTCLEAR 0x14
Looking at this is is pretty apparent that you shall use
the generic basic MMIO GPIO helper library.
In Kconfig:
select GPIO_GENERIC
Include #include <linux/basic_mmio_gpio.h>
See other GPIO drivers to figure out how to reuse the
generic MMIO GPIO helpers. For example the new
GRGPIO driver, drivers/gpio/gpio-grgpio.c.
> +struct altera_gpio_chip {
> + struct of_mm_gpio_chip mmchip;
> + struct irq_domain *irq; /* GPIO controller IRQ number */
> + spinlock_t gpio_lock; /* Lock used for synchronization */
> + bool level_trigger;
> + int hwirq;
> +};
Convert inline documentation to kerneldoc.
Document the rest of the members as well because
I don't understand them, and the documentation for the
irqdomain is plain wrong, the other comment is
just stating the obvious: tell us *what* is it locking
and *why*.
(...)
> +static int altera_gpio_irq_set_type(struct irq_data *d,
> + unsigned int type)
> +{
> + /* There is no way to know the soft IP configuration,
> + so we accept any interrupt types that are supported by the soft
> + IP. Nothing to do here since it is hardware defined. Just return
> + OK status to the caller. */
Eh what? Why don't you say pass in the capabilities from
the device tree then, if the hardware cannot say what it is
capable of, something else has to.
/*
* Please use this multiline comment
* style, it is easier to read.
*/
> + if (type == IRQ_TYPE_EDGE_RISING ||
> + type == IRQ_TYPE_EDGE_FALLING ||
> + type == IRQ_TYPE_EDGE_BOTH ||
> + type == IRQ_TYPE_NONE) {
> + return 0;
> + }
This is not OK if it is unclear whether this is all supported.
Get that info from somewhere.
Apparently this is exactly what the level trigger field in
the device tree defines.
Especially handling both edges is quite tricky, have you
actually tested this?
(...)
> +static int altera_gpio_get(struct gpio_chip *gc, unsigned offset)
> +static void altera_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
> +static int altera_gpio_direction_input(struct gpio_chip *gc, unsigned offset)
> +static int altera_gpio_direction_output(struct gpio_chip *gc,
> + unsigned offset, int value)
These look like they could all use basic_mmio_gpio instead
of being verbatim copies of generic code.
> +static int altera_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
> +static void altera_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> +{
> + struct altera_gpio_chip *altera_gc = irq_desc_get_handler_data(desc);
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
> + unsigned long status;
> +
> + int base;
> + chip->irq_mask(&desc->irq_data);
> +
> + if (altera_gc->level_trigger == ALTERA_TRIGGER_LEVEL_HIGH) {
> +{
> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> + struct altera_gpio_chip *altera_gc = container_of(mm_gc,
> + struct altera_gpio_chip, mmchip);
> +
> + if (altera_gc->irq == 0)
> + return -ENXIO;
> + if ((altera_gc->irq && offset) < altera_gc->mmchip.gc.ngpio)
> + return irq_create_mapping(altera_gc->irq, offset);
> + else
> + return -ENXIO;
> +}
> +
So here it is pretty clear what this thing in the device tree
means. It means "I am a level triggered chip".
> + status = readl(mm_gc->regs + ALTERA_GPIO_DATA);
> + status &= readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
> +
> + for (base = 0; base < mm_gc->gc.ngpio; base++) {
> + if ((1 << base) & status) {
> + generic_handle_irq(irq_linear_revmap(
> + altera_gc->irq, base));
> + }
> + }
> + } else {
And if I'm not level triggered I'm edge triggered, right?
> + while ((status = (readl(mm_gc->regs + ALTERA_GPIO_EDGE_CAP) &
> + readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK)))) {
> + writel(status, mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
> + for (base = 0; base < mm_gc->gc.ngpio; base++) {
> + if ((1 << base) & status) {
> + generic_handle_irq(irq_linear_revmap(
> + altera_gc->irq, base));
> + }
> + }
> + }
> + }
> +
> + chip->irq_eoi(irq_desc_get_irq_data(desc));
> + chip->irq_unmask(&desc->irq_data);
> +}
(...)
> +int altera_gpio_probe(struct platform_device *pdev)
> +{
> + struct device_node *node = pdev->dev.of_node;
> + int id, reg, ret;
> + struct altera_gpio_chip *altera_gc = devm_kzalloc(&pdev->dev,
> + sizeof(*altera_gc), GFP_KERNEL);
> + if (altera_gc == NULL) {
> + ret = -ENOMEM;
> + pr_err("%s: registration failed with status %d\n",
> + node->full_name, ret);
> + return ret;
dev_err(&pdev->dev, "%s out of memory\n");
return -ENOMEM;
So:
- Use dev_* print primitives
- Print the actual error, or we go back to the 1980ies
habit of having to look up an abstract error code in a
binder.
- Just return the error.
> + }
> + altera_gc->irq = 0;
> +
> + spin_lock_init(&altera_gc->gpio_lock);
> +
> + id = pdev->id;
> +
> + if (of_property_read_u32(node, "width", ®))
> + /*By default assume full GPIO controller*/
> + altera_gc->mmchip.gc.ngpio = 32;
> + else
> + altera_gc->mmchip.gc.ngpio = reg;
> +
> + if (altera_gc->mmchip.gc.ngpio > 32) {
> + pr_warn("%s: ngpio is greater than 32, defaulting to 32\n",
> + node->full_name);
> + altera_gc->mmchip.gc.ngpio = 32;
> + }
> +
> + altera_gc->mmchip.gc.direction_input = altera_gpio_direction_input;
> + altera_gc->mmchip.gc.direction_output = altera_gpio_direction_output;
> + altera_gc->mmchip.gc.get = altera_gpio_get;
> + altera_gc->mmchip.gc.set = altera_gpio_set;
> + altera_gc->mmchip.gc.to_irq = altera_gpio_to_irq;
> + altera_gc->mmchip.gc.owner = THIS_MODULE;
> +
> + ret = of_mm_gpiochip_add(node, &altera_gc->mmchip);
> + if (ret)
> + goto err;
Just print the error and return directly.
There is nothing to free or clean up at the err: label.
> + platform_set_drvdata(pdev, altera_gc);
> +
> + if (of_get_property(node, "interrupts", ®) == NULL)
> + goto skip_irq;
> + altera_gc->hwirq = irq_of_parse_and_map(node, 0);
> +
> + if (altera_gc->hwirq == NO_IRQ)
> + goto skip_irq;
> +
> + altera_gc->irq = irq_domain_add_linear(node, altera_gc->mmchip.gc.ngpio,
> + &altera_gpio_irq_ops, altera_gc);
> +
> + if (!altera_gc->irq) {
> + ret = -ENODEV;
> + goto dispose_irq;
> + }
> +
> + if (of_property_read_u32(node, "level_trigger", ®)) {
> + ret = -EINVAL;
> + pr_err("%s: level_trigger value not set in device tree\n",
> + node->full_name);
> + goto teardown;
> + }
> + altera_gc->level_trigger = reg;
> +
> + irq_set_handler_data(altera_gc->hwirq, altera_gc);
> + irq_set_chained_handler(altera_gc->hwirq, altera_gpio_irq_handler);
> +
> + return 0;
> +
> +teardown:
> + irq_domain_remove(altera_gc->irq);
> +dispose_irq:
> + irq_dispose_mapping(altera_gc->hwirq);
> + WARN_ON(gpiochip_remove(&altera_gc->mmchip.gc) < 0);
> +
> +err:
> + pr_err("%s: registration failed with status %d\n",
> + node->full_name, ret);
> + devm_kfree(&pdev->dev, altera_gc);
No. The devm_* managed resources take care of freeing when probe
fails, that is the major idea with it.
> +
> + return ret;
> +skip_irq:
> + return 0;
> +}
> +
> +static int altera_gpio_remove(struct platform_device *pdev)
> +{
> + unsigned int irq, i;
> + int status;
> + struct altera_gpio_chip *altera_gc = platform_get_drvdata(pdev);
> +
> + status = gpiochip_remove(&altera_gc->mmchip.gc);
> +
> + if (status < 0)
> + return status;
> +
> + if (altera_gc->irq) {
> + irq_dispose_mapping(altera_gc->hwirq);
> +
> + for (i = 0; i < altera_gc->mmchip.gc.ngpio; i++) {
> + irq = irq_find_mapping(altera_gc->irq, i);
> + if (irq > 0)
> + irq_dispose_mapping(irq);
> + }
> +
> + irq_domain_remove(altera_gc->irq);
> + }
> +
> + irq_set_handler_data(altera_gc->hwirq, NULL);
> + irq_set_chained_handler(altera_gc->hwirq, NULL);
> + devm_kfree(&pdev->dev, altera_gc);
Again no. You're missing the point with devm*.
> +#ifdef CONFIG_OF
> +static struct of_device_id altera_gpio_of_match[] = {
> + { .compatible = "altr,pio-1.0", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, altera_gpio_of_match);
> +#else
> +#define altera_gpio_of_match NULL
> +#endif
Your driver depends on OF_GPIO, which depends on OF.
Skip the #ifdefs.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver
@ 2013-05-08 2:39 Tien Hock Loh
2013-05-20 18:02 ` Linus Walleij
0 siblings, 1 reply; 20+ messages in thread
From: Tien Hock Loh @ 2013-05-08 2:39 UTC (permalink / raw)
To: linus.walleij, rob.herring, rob
Cc: linux-doc, devicetree-discuss, thloh85, thloh
Add driver support for Altera GPIO soft IP, including interrupts and I/O.
Tested on Altera CV SoC board.
Signed-off-by: Tien Hock Loh <thloh@altera.com>
---
.../devicetree/bindings/gpio/gpio-altera.txt | 40 +++
drivers/gpio/Kconfig | 7 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-altera.c | 375 +++++++++++++++++++++
4 files changed, 423 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-altera.txt
create mode 100644 drivers/gpio/gpio-altera.c
diff --git a/Documentation/devicetree/bindings/gpio/gpio-altera.txt b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
new file mode 100644
index 0000000..b4a5c24
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
@@ -0,0 +1,40 @@
+Altera GPIO controller bindings
+
+Required properties:
+- compatible:
+ - "altr,pio-1.0"
+- reg: Physical base address and length of the controller's registers.
+- #gpio-cells : Should be two.
+ - first cell is the gpio offset number
+ - second cell is used to specify optional parameters (unused)
+- gpio-controller : Marks the device node as a GPIO controller.
+- #interrupt-cells : Should be 1.
+- interrupts: Specify the interrupt.
+- interrupt-controller: Mark the device node as an interrupt controller
+ The first cell is the GPIO number.
+
+Altera GPIO specific properties:
+- width: Width of the GPIO bank, range from 1-32
+- level_trigger: Specifies whether the GPIO interrupt is level trigger.
+ This field is required if the Altera GPIO controller used has IRQ enabled.
+ Note: This field is does not specify whether Altera GPIO controller is
+ level or edge trigger. It is used to detect internally how the trigger is
+ handled.
+
+Note: If the Altera GPIO is set to be built as a module, peripherals that uses
+Altera GPIO as interrupt-parent should be a module so that the peripheral
+doesn't get initialized before Altera GPIO is initialized.
+
+Example:
+
+gpio_altr: gpio_altr {
+ compatible = "altr,pio-1.0";
+ reg = <0xff200000 0x10>;
+ interrupts = <0 45 4>;
+ width = <32>;
+ level_trigger = <0>;
+ #gpio-cells = <2>;
+ gpio-controller;
+ #interrupt-cells = <1>;
+ interrupt-controller;
+};
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index c22eed9..8861c65 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -119,6 +119,13 @@ config GPIO_GENERIC_PLATFORM
help
Say yes here to support basic platform_device memory-mapped GPIO controllers.
+config GPIO_ALTERA
+ tristate "Altera GPIO"
+ select IRQ_DOMAIN
+ depends on OF_GPIO
+ help
+ Say yes here to support the Altera PIO device.
+
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 0cb2d65..695b954 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_GPIO_74X164) += gpio-74x164.o
obj-$(CONFIG_GPIO_ADNP) += gpio-adnp.o
obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o
obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o
+obj-$(CONFIG_GPIO_ALTERA) += gpio-altera.o
obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o
obj-$(CONFIG_GPIO_ARIZONA) += gpio-arizona.o
obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
diff --git a/drivers/gpio/gpio-altera.c b/drivers/gpio/gpio-altera.c
new file mode 100644
index 0000000..0cb0049
--- /dev/null
+++ b/drivers/gpio/gpio-altera.c
@@ -0,0 +1,375 @@
+/*
+ * Copyright (C) 2013 Altera Corporation
+ * Based on gpio-mpc8xxx.c
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2. This program is licensed "as is" without any warranty of any
+ * kind, whether express or implied.
+ */
+
+#include <linux/gpio.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/of_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/interrupt.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+
+#define ALTERA_GPIO_DATA 0x0
+#define ALTERA_GPIO_DIR 0x4
+#define ALTERA_GPIO_IRQ_MASK 0x8
+#define ALTERA_GPIO_EDGE_CAP 0xc
+#define ALTERA_GPIO_OUTSET 0x10
+#define ALTERA_GPIO_OUTCLEAR 0x14
+
+#define ALTERA_TRIGGER_EDGE_RISING 0
+#define ALTERA_TRIGGER_LEVEL_HIGH 1
+#define ALTERA_TRIGGER_EDGE_FALLING 2
+#define ALTERA_TRIGGER_EDGE_BOTH 3
+
+struct altera_gpio_chip {
+ struct of_mm_gpio_chip mmchip;
+ struct irq_domain *irq; /* GPIO controller IRQ number */
+ spinlock_t gpio_lock; /* Lock used for synchronization */
+ bool level_trigger;
+ int hwirq;
+};
+
+static void altera_gpio_irq_unmask(struct irq_data *d)
+{
+ struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
+ struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
+ unsigned long flags;
+ unsigned int intmask;
+
+ spin_lock_irqsave(&altera_gc->gpio_lock, flags);
+ intmask = readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+ /* Set ALTERA_GPIO_IRQ_MASK bit to unmask */
+ intmask |= (1 << irqd_to_hwirq(d));
+ writel(intmask, mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+ spin_unlock_irqrestore(&altera_gc->gpio_lock, flags);
+}
+
+static void altera_gpio_irq_mask(struct irq_data *d)
+{
+ struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
+ struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
+ unsigned long flags;
+ unsigned int intmask;
+
+ spin_lock_irqsave(&altera_gc->gpio_lock, flags);
+ intmask = readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+ /* Clear ALTERA_GPIO_IRQ_MASK bit to mask */
+ intmask &= ~(1 << irqd_to_hwirq(d));
+ writel(intmask, mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+ spin_unlock_irqrestore(&altera_gc->gpio_lock, flags);
+}
+
+static int altera_gpio_irq_set_type(struct irq_data *d,
+ unsigned int type)
+{
+ /* There is no way to know the soft IP configuration,
+ so we accept any interrupt types that are supported by the soft
+ IP. Nothing to do here since it is hardware defined. Just return
+ OK status to the caller. */
+ if (type == IRQ_TYPE_EDGE_RISING ||
+ type == IRQ_TYPE_EDGE_FALLING ||
+ type == IRQ_TYPE_EDGE_BOTH ||
+ type == IRQ_TYPE_NONE) {
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
+static struct irq_chip altera_irq_chip = {
+ .name = "altera-gpio",
+ .irq_mask = altera_gpio_irq_mask,
+ .irq_unmask = altera_gpio_irq_unmask,
+ .irq_set_type = altera_gpio_irq_set_type,
+};
+
+static int altera_gpio_get(struct gpio_chip *gc, unsigned offset)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+
+ return (readl(mm_gc->regs + ALTERA_GPIO_DATA) >> offset) & 1;
+}
+
+static void altera_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct altera_gpio_chip *chip = container_of(mm_gc,
+ struct altera_gpio_chip, mmchip);
+ unsigned long flags;
+ unsigned int data_reg;
+
+ spin_lock_irqsave(&chip->gpio_lock, flags);
+ data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA);
+ data_reg = (data_reg & ~(1 << offset)) | (value << offset);
+ writel(data_reg, mm_gc->regs + ALTERA_GPIO_DATA);
+ spin_unlock_irqrestore(&chip->gpio_lock, flags);
+}
+
+static int altera_gpio_direction_input(struct gpio_chip *gc, unsigned offset)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct altera_gpio_chip *chip = container_of(mm_gc,
+ struct altera_gpio_chip, mmchip);
+ unsigned long flags;
+ unsigned int gpio_ddr;
+
+ spin_lock_irqsave(&chip->gpio_lock, flags);
+ /* Set pin as input, assumes software controlled IP */
+ gpio_ddr = readl(mm_gc->regs + ALTERA_GPIO_DIR);
+ gpio_ddr &= ~(1 << offset);
+ writel(gpio_ddr, mm_gc->regs + ALTERA_GPIO_DIR);
+ spin_unlock_irqrestore(&chip->gpio_lock, flags);
+
+ return 0;
+}
+
+static int altera_gpio_direction_output(struct gpio_chip *gc,
+ unsigned offset, int value)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct altera_gpio_chip *chip = container_of(mm_gc,
+ struct altera_gpio_chip, mmchip);
+ unsigned long flags;
+ unsigned int data_reg, gpio_ddr;
+
+ spin_lock_irqsave(&chip->gpio_lock, flags);
+ /* Sets the GPIO value */
+ data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA);
+ data_reg = (data_reg & ~(1 << offset)) | (value << offset);
+ writel(data_reg, mm_gc->regs + ALTERA_GPIO_DATA);
+
+ /* Set pin as output, assumes software controlled IP */
+ gpio_ddr = readl(mm_gc->regs + ALTERA_GPIO_DIR);
+ gpio_ddr |= (1 << offset);
+ writel(gpio_ddr, mm_gc->regs + ALTERA_GPIO_DIR);
+ spin_unlock_irqrestore(&chip->gpio_lock, flags);
+
+ return 0;
+}
+
+static int altera_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct altera_gpio_chip *altera_gc = container_of(mm_gc,
+ struct altera_gpio_chip, mmchip);
+
+ if (altera_gc->irq == 0)
+ return -ENXIO;
+ if ((altera_gc->irq && offset) < altera_gc->mmchip.gc.ngpio)
+ return irq_create_mapping(altera_gc->irq, offset);
+ else
+ return -ENXIO;
+}
+
+static void altera_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
+{
+ struct altera_gpio_chip *altera_gc = irq_desc_get_handler_data(desc);
+ struct irq_chip *chip = irq_desc_get_chip(desc);
+ struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
+ unsigned long status;
+
+ int base;
+ chip->irq_mask(&desc->irq_data);
+
+ if (altera_gc->level_trigger == ALTERA_TRIGGER_LEVEL_HIGH) {
+ status = readl(mm_gc->regs + ALTERA_GPIO_DATA);
+ status &= readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
+
+ for (base = 0; base < mm_gc->gc.ngpio; base++) {
+ if ((1 << base) & status) {
+ generic_handle_irq(irq_linear_revmap(
+ altera_gc->irq, base));
+ }
+ }
+ } else {
+ while ((status = (readl(mm_gc->regs + ALTERA_GPIO_EDGE_CAP) &
+ readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK)))) {
+ writel(status, mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
+ for (base = 0; base < mm_gc->gc.ngpio; base++) {
+ if ((1 << base) & status) {
+ generic_handle_irq(irq_linear_revmap(
+ altera_gc->irq, base));
+ }
+ }
+ }
+ }
+
+ chip->irq_eoi(irq_desc_get_irq_data(desc));
+ chip->irq_unmask(&desc->irq_data);
+}
+
+static int altera_gpio_irq_map(struct irq_domain *h, unsigned int virq,
+ irq_hw_number_t hw_irq_num)
+{
+ irq_set_chip_data(virq, h->host_data);
+ irq_set_chip_and_handler(virq, &altera_irq_chip, handle_level_irq);
+ irq_set_irq_type(virq, IRQ_TYPE_NONE);
+
+ return 0;
+}
+
+static struct irq_domain_ops altera_gpio_irq_ops = {
+ .map = altera_gpio_irq_map,
+ .xlate = irq_domain_xlate_twocell,
+};
+
+int altera_gpio_probe(struct platform_device *pdev)
+{
+ struct device_node *node = pdev->dev.of_node;
+ int id, reg, ret;
+ struct altera_gpio_chip *altera_gc = devm_kzalloc(&pdev->dev,
+ sizeof(*altera_gc), GFP_KERNEL);
+ if (altera_gc == NULL) {
+ ret = -ENOMEM;
+ pr_err("%s: registration failed with status %d\n",
+ node->full_name, ret);
+ return ret;
+ }
+ altera_gc->irq = 0;
+
+ spin_lock_init(&altera_gc->gpio_lock);
+
+ id = pdev->id;
+
+ if (of_property_read_u32(node, "width", ®))
+ /*By default assume full GPIO controller*/
+ altera_gc->mmchip.gc.ngpio = 32;
+ else
+ altera_gc->mmchip.gc.ngpio = reg;
+
+ if (altera_gc->mmchip.gc.ngpio > 32) {
+ pr_warn("%s: ngpio is greater than 32, defaulting to 32\n",
+ node->full_name);
+ altera_gc->mmchip.gc.ngpio = 32;
+ }
+
+ altera_gc->mmchip.gc.direction_input = altera_gpio_direction_input;
+ altera_gc->mmchip.gc.direction_output = altera_gpio_direction_output;
+ altera_gc->mmchip.gc.get = altera_gpio_get;
+ altera_gc->mmchip.gc.set = altera_gpio_set;
+ altera_gc->mmchip.gc.to_irq = altera_gpio_to_irq;
+ altera_gc->mmchip.gc.owner = THIS_MODULE;
+
+ ret = of_mm_gpiochip_add(node, &altera_gc->mmchip);
+ if (ret)
+ goto err;
+
+ platform_set_drvdata(pdev, altera_gc);
+
+ if (of_get_property(node, "interrupts", ®) == NULL)
+ goto skip_irq;
+ altera_gc->hwirq = irq_of_parse_and_map(node, 0);
+
+ if (altera_gc->hwirq == NO_IRQ)
+ goto skip_irq;
+
+ altera_gc->irq = irq_domain_add_linear(node, altera_gc->mmchip.gc.ngpio,
+ &altera_gpio_irq_ops, altera_gc);
+
+ if (!altera_gc->irq) {
+ ret = -ENODEV;
+ goto dispose_irq;
+ }
+
+ if (of_property_read_u32(node, "level_trigger", ®)) {
+ ret = -EINVAL;
+ pr_err("%s: level_trigger value not set in device tree\n",
+ node->full_name);
+ goto teardown;
+ }
+ altera_gc->level_trigger = reg;
+
+ irq_set_handler_data(altera_gc->hwirq, altera_gc);
+ irq_set_chained_handler(altera_gc->hwirq, altera_gpio_irq_handler);
+
+ return 0;
+
+teardown:
+ irq_domain_remove(altera_gc->irq);
+dispose_irq:
+ irq_dispose_mapping(altera_gc->hwirq);
+ WARN_ON(gpiochip_remove(&altera_gc->mmchip.gc) < 0);
+
+err:
+ pr_err("%s: registration failed with status %d\n",
+ node->full_name, ret);
+ devm_kfree(&pdev->dev, altera_gc);
+
+ return ret;
+skip_irq:
+ return 0;
+}
+
+static int altera_gpio_remove(struct platform_device *pdev)
+{
+ unsigned int irq, i;
+ int status;
+ struct altera_gpio_chip *altera_gc = platform_get_drvdata(pdev);
+
+ status = gpiochip_remove(&altera_gc->mmchip.gc);
+
+ if (status < 0)
+ return status;
+
+ if (altera_gc->irq) {
+ irq_dispose_mapping(altera_gc->hwirq);
+
+ for (i = 0; i < altera_gc->mmchip.gc.ngpio; i++) {
+ irq = irq_find_mapping(altera_gc->irq, i);
+ if (irq > 0)
+ irq_dispose_mapping(irq);
+ }
+
+ irq_domain_remove(altera_gc->irq);
+ }
+
+ irq_set_handler_data(altera_gc->hwirq, NULL);
+ irq_set_chained_handler(altera_gc->hwirq, NULL);
+ devm_kfree(&pdev->dev, altera_gc);
+ return -EIO;
+}
+
+#ifdef CONFIG_OF
+static struct of_device_id altera_gpio_of_match[] = {
+ { .compatible = "altr,pio-1.0", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, altera_gpio_of_match);
+#else
+#define altera_gpio_of_match NULL
+#endif
+
+static struct platform_driver altera_gpio_driver = {
+ .driver = {
+ .name = "altera_gpio",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(altera_gpio_of_match),
+ },
+ .probe = altera_gpio_probe,
+ .remove = altera_gpio_remove,
+};
+
+static int __init altera_gpio_init(void)
+{
+ return platform_driver_register(&altera_gpio_driver);
+}
+subsys_initcall(altera_gpio_init);
+
+static void __exit altera_gpio_exit(void)
+{
+ platform_driver_unregister(&altera_gpio_driver);
+}
+module_exit(altera_gpio_exit);
+
+MODULE_DESCRIPTION("Altera GPIO driver");
+MODULE_LICENSE("GPL");
--
1.7.11.GIT
^ permalink raw reply related [flat|nested] 20+ messages in thread
end of thread, other threads:[~2013-08-16 13:12 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-12 5:37 [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver thloh
2013-03-19 6:01 ` Tien Hock Loh
2013-03-27 11:58 ` Linus Walleij
2013-03-28 11:40 ` Tien Hock Loh
2013-03-28 11:40 ` Tien Hock Loh
2013-04-02 8:55 ` Linus Walleij
2013-05-08 2:39 Tien Hock Loh
2013-05-20 18:02 ` Linus Walleij
2013-06-05 8:05 ` Loh Tien Hock
2013-06-06 8:05 thloh
2013-06-17 6:38 ` Linus Walleij
2013-06-17 7:10 ` Loh Tien Hock
2013-06-17 7:29 ` Loh Tien Hock
2013-06-17 9:12 ` Linus Walleij
2013-07-08 7:04 thloh.linux
2013-07-22 19:07 ` Linus Walleij
2013-07-23 2:51 ` Loh Tien Hock
2013-07-29 16:11 ` Linus Walleij
2013-07-29 16:24 ` Loh Tien Hock
2013-08-16 13:12 ` Linus Walleij
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.