| Message ID | 1393842463-5206-1-git-send-email-thloh@altera.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Mon, Mar 3, 2014 at 6:27 PM, <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 using dipsw and LED using LED framework. > > Signed-off-by: Tien Hock Loh <thloh@altera.com> Can I get some ACKs on this? Like from Gerhard, DT folks? Or is there still more things to do? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 03, 2014 at 06:27:43PM +0800, 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 using dipsw and LED using LED framework. > > Signed-off-by: Tien Hock Loh <thloh@altera.com> > --- > .../devicetree/bindings/gpio/gpio-altera.txt | 44 +++ > drivers/gpio/Kconfig | 7 + > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-altera.c | 419 +++++++++++++++++++++ > 4 files changed, 471 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..1de1f9b > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt > @@ -0,0 +1,44 @@ > +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 > + - The first cell is the gpio offset number > +- gpio-controller : Marks the device node as a GPIO controller. > +- #interrupt-cells : Should be 1. > + - The first cell is the GPIO offset number within the GPIO controller. > +- interrupts: Specify the interrupt. > +- interrupt-controller: Mark the device node as an interrupt controller > + > +Altera GPIO specific required properties: > +- altr,interrupt_trigger: Specifies the interrupt trigger type the GPIO > + hardware is synthesized. This field is required if the Altera GPIO controller > + used has IRQ enabled as the interrupt type is not software controlled, > + but hardware synthesized. Required if GPIO is used as an interrupt > + controller. The value is defined in <dt-bindings/interrupt-controller/irq.h> > + Only the following flags are supported: > + IRQ_TYPE_EDGE_RISING > + IRQ_TYPE_EDGE_FALLING > + IRQ_TYPE_EDGE_BOTH > + IRQ_TYPE_LEVEL_HIGH I'd suggest "altr,interrupt-trigger" (with a hyphen). So, if I'm understanding properly, this controller doesn't support per-gpio trigger settings? Low-level triggered interrupts aren't supported? > + > +Altera GPIO specific optional properties: > +- altr,gpio-bank-width: Width of the GPIO bank. This defines how many pins the > + GPIO device has. Ranges between 1-32. Optional and defaults to 32 is not > + specified. Generally, this is called 'ngpio', I think. Might be nice to stay consistent with other bindings. > + > +Example: > + > +gpio_altr: gpio@40000 { > + compatible = "altr,pio-1.0"; > + reg = <0xff200000 0x10>; > + interrupts = <0 45 4>; > + altr,gpio-bank-width = <32>; > + altr,interrupt_trigger = <IRQ_TYPE_EDGE_RISING>; > + #gpio-cells = <1>; > + gpio-controller; > + #interrupt-cells = <1>; > + interrupt-controller; > +}; > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index 6973387..07bccdf 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -128,6 +128,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. nit: you should use tabs for indentation instead of spaces to stay consistent. > + > 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 5d50179..88374d2 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_BCM_KONA) += gpio-bcm-kona.o > diff --git a/drivers/gpio/gpio-altera.c b/drivers/gpio/gpio-altera.c > new file mode 100644 > index 0000000..ab0738f > --- /dev/null > +++ b/drivers/gpio/gpio-altera.c > @@ -0,0 +1,419 @@ > +/* > + * 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 2 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/init.h> > +#include <linux/irqdomain.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/irq.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of_irq.h> > +#include <linux/of_gpio.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > + > +#include <linux/irqchip/chained_irq.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 > +* @mmchip : memory mapped chip structure. > +* @irq : irq domain that this driver is registered to. s/@irq/@domain/ ? > +* @gpio_lock : synchronization lock so that new irq/set/get requests > + will be blocked until the current one completes. > +* @interrupt_trigger : specifies the hardware configured IRQ trigger type > + (rising, falling, both, high) @edge_type? > +* @mapped_irq : kernel mapped irq number. > +*/ > +struct altera_gpio_chip { > + struct of_mm_gpio_chip mmchip; I had never really looked into of_mm_gpio_chip before but...does this really get you anything? All it does is manage mapping your registers for you; as far as I can tell it's just another layer of indirection with no gain. I'd suggest lifting 'regs' and 'chip' directly into your altera_gpio_chip: struct altera_gpio_chip { struct gpio_chip chip; struct irq_domain *domain; void __iomem *regs; spinlock_t gpio_lock; int mapped_irq; }; [..] > +static unsigned int altera_gpio_irq_startup(struct irq_data *d) > +{ > + altera_gpio_irq_unmask(d); > + > + return 0; > +} > + > +static void altera_gpio_irq_shutdown(struct irq_data *d) > +{ > + altera_gpio_irq_unmask(d); > +} Should you really even be touching masks in startup/shutdown? > +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, > + .irq_startup = altera_gpio_irq_startup, > + .irq_shutdown = altera_gpio_irq_shutdown, > +}; > + [..] > +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->domain) > + return -ENXIO; How could this happen (hint, move your domain creation before gpiochip_add). > + if (offset < altera_gc->mmchip.gc.ngpio) > + return irq_find_mapping(altera_gc->domain, 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 i; > + > + chained_irq_enter(chip, desc); > + /* Handling for level trigger and edge trigger is different */ > + if (altera_gc->interrupt_trigger == IRQ_TYPE_LEVEL_HIGH) { Suggestion: split this into two handling functions, install one-or-the-other in your probe() based on the "altr,trigger-type" property. Bonus points: remove interrupt_trigger member from altera_gpio_chip entirely. > + status = readl_relaxed(mm_gc->regs + ALTERA_GPIO_DATA); > + status &= readl_relaxed(mm_gc->regs + ALTERA_GPIO_IRQ_MASK); > + > + for (i = 0; i < mm_gc->gc.ngpio; i++) { > + if (status & BIT(i)) { > + generic_handle_irq(irq_find_mapping( > + altera_gc->domain, i)); > + } > + } You may find for_each_set_bit() handy. > + } 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 (i = 0; i < mm_gc->gc.ngpio; i++) { > + if (status & BIT(i)) { > + generic_handle_irq(irq_find_mapping( > + altera_gc->domain, i)); > + } > + } > + } > + } > + > + chained_irq_exit(chip, desc); > +} > + > +static int altera_gpio_irq_map(struct irq_domain *h, unsigned int irq, > + irq_hw_number_t hw_irq_num) > +{ > + irq_set_chip_data(irq, h->host_data); > + irq_set_chip_and_handler(irq, &altera_irq_chip, handle_level_irq); > + irq_set_irq_type(irq, IRQ_TYPE_NONE); Does irq_set_irq_type(irq, IRQ_TYPE_NONE) even do anything meaningful here? > + > + return 0; > +} > + > +static struct irq_domain_ops altera_gpio_irq_ops = { const? > + .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 i, 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); This print is not necessary, as the allocator will complain loudly on your behalf. Also, I'd suggest you not define and initialize 'altera_gc' on the same line. > + return -ENOMEM; > + } > + altera_gc->domain = 0; > + > + spin_lock_init(&altera_gc->gpio_lock); > + > + id = pdev->id; > + > + if (of_property_read_u32(node, "altr,gpio-bank-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) { > + dev_warn(&pdev->dev, > + "ngpio is greater than 32, defaulting to 32\n"); > + 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) { > + dev_err(&pdev->dev, "Failed adding memory mapped gpiochip\n"); > + return ret; > + } > + > + platform_set_drvdata(pdev, altera_gc); > + > + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0); > platform_get_irq(pdev, 0); > + > + if (!altera_gc->mapped_irq) > + goto skip_irq; > + > + altera_gc->domain = irq_domain_add_linear(node, > + altera_gc->mmchip.gc.ngpio, &altera_gpio_irq_ops, altera_gc); > + > + if (!altera_gc->domain) { > + ret = -ENODEV; > + goto dispose_irq; > + } > + > + for (i = 0; i < altera_gc->mmchip.gc.ngpio; i++) > + irq_create_mapping(altera_gc->domain, i); > + > + if (of_property_read_u32(node, "altr,interrupt_type", ®)) { > + ret = -EINVAL; > + dev_err(&pdev->dev, > + "altr,interrupt_type value not set in device tree\n"); > + goto teardown; > + } > + altera_gc->interrupt_trigger = 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->domain); > +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->domain) { How could this happen? > + irq_dispose_mapping(altera_gc->mapped_irq); Does irq_domain_remove() not teardown mappings? > + > + for (i = 0; i < altera_gc->mmchip.gc.ngpio; i++) { > + irq = irq_find_mapping(altera_gc->domain, i); > + if (irq > 0) > + irq_dispose_mapping(irq); > + } > + > + irq_domain_remove(altera_gc->domain); > + } > + > + 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[] = { const? > + { .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_AUTHOR("Tien Hock Loh <thloh@altera.com>"); > +MODULE_DESCRIPTION("Altera GPIO driver"); > +MODULE_LICENSE("GPL");
On Mon, Mar 03, 2014 at 18:27 +0800, 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 using dipsw and LED using LED framework. > > Signed-off-by: Tien Hock Loh <thloh@altera.com> > --- > .../devicetree/bindings/gpio/gpio-altera.txt | 44 +++ > drivers/gpio/Kconfig | 7 + > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-altera.c | 419 +++++++++++++++++++++ > 4 files changed, 471 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpio/gpio-altera.txt > create mode 100644 drivers/gpio/gpio-altera.c Let's see. A v7, i.e. quite some iterations done, and still whitespace issues, and not a single line of changelogs. Not exactly an invitation for reviewers to spend their time on it. It's hard to tell which feedback was received before, and what of it has been addressed. Aren't binding patches to be separate (and first) in series these days, while driver adjustment or introduction then follows to implement what was specified? > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt > @@ -0,0 +1,44 @@ > +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 > + - The first cell is the gpio offset number Will there never be any use of flags, that usually are kept in the second cell? Can we tell for sure that one cell is enough? > +- gpio-controller : Marks the device node as a GPIO controller. > +- #interrupt-cells : Should be 1. > + - The first cell is the GPIO offset number within the GPIO controller. > +- interrupts: Specify the interrupt. > +- interrupt-controller: Mark the device node as an interrupt controller Is #interrupt-cells = <1> enough? Are triggers fixed in the hardware? A comment about it may prevent people asking questions. (The motivation might be mentioned below, see the comment on the two "required properties" sections.) I'd sort the last three items differently. 'interrupts' is where the GPIO block is "a client" to the IRQ controller which it is connected to. '#interrupt-cells' is the "server side" because the GPIO block is an IRQ controller and has the 'interrupt-controller' property. > + > +Altera GPIO specific required properties: This made me stop and wonder. This is the "Altera GPIO controller bindings" document, and it has a "Required properties" section as well as an "Altera GPIO specific required properties" section? Isn't this highly redundant, and confusing at the same time? > +- altr,interrupt_trigger: Specifies the interrupt trigger type the GPIO > + hardware is synthesized. This field is required if the Altera GPIO controller > + used has IRQ enabled as the interrupt type is not software controlled, > + but hardware synthesized. Required if GPIO is used as an interrupt > + controller. The value is defined in <dt-bindings/interrupt-controller/irq.h> > + Only the following flags are supported: > + IRQ_TYPE_EDGE_RISING > + IRQ_TYPE_EDGE_FALLING > + IRQ_TYPE_EDGE_BOTH > + IRQ_TYPE_LEVEL_HIGH > + > +Altera GPIO specific optional properties: > +- altr,gpio-bank-width: Width of the GPIO bank. This defines how many pins the > + GPIO device has. Ranges between 1-32. Optional and defaults to 32 is not > + specified. > + > +Example: > + > +gpio_altr: gpio@40000 { > + compatible = "altr,pio-1.0"; > + reg = <0xff200000 0x10>; > + interrupts = <0 45 4>; > + altr,gpio-bank-width = <32>; > + altr,interrupt_trigger = <IRQ_TYPE_EDGE_RISING>; > + #gpio-cells = <1>; > + gpio-controller; > + #interrupt-cells = <1>; > + interrupt-controller; > +}; The node's address does not match the 'reg' property. The interrupt trigger uses symbolic names, so could the 'interrupts' spec. Examples should not assume an external 'interrupt-parent' but should list them for completeness. > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -128,6 +128,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. > + I guess checkpatch nags about the rather short help text. Tristate options probably should mention the opportunity to build a module, and by convention should state what the module's name then would be (which in total gets you the minimum number of help text lines as a byproduct). > --- /dev/null > +++ b/drivers/gpio/gpio-altera.c > @@ -0,0 +1,419 @@ > [ ... ] > + > +#include <linux/gpio.h> > +#include <linux/init.h> > +#include <linux/irqdomain.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/irq.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of_irq.h> > +#include <linux/of_gpio.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > + > +#include <linux/irqchip/chained_irq.h> Includes should be alpha-sorted. To detect duplicates, and to avoid or reduce conflicts. > + > +#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 OUTSET and OUTCLEAR are not used in the driver source. You might as well drop their declarations (after all you are not declaring the register set layout here, but are just introducing magic offset numbers for some of the registers that the driver may access). > +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; I'd suggest <stdint.h> style fixed width data types (uint32_t) or their kernel equivalents (u32) for operations on fixed width peripheral registers. > + > + 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 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 (type == IRQ_TYPE_LEVEL_HIGH && > + altera_gc->interrupt_trigger == IRQ_TYPE_LEVEL_HIGH) { > + return 0; > + } else { > + if (type == IRQ_TYPE_EDGE_RISING && > + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_RISING) > + return 0; > + else if (type == IRQ_TYPE_EDGE_FALLING && > + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_FALLING) > + return 0; > + else if (type == IRQ_TYPE_EDGE_BOTH && > + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_BOTH) > + return 0; > + } > + > + return -EINVAL; > +} Whitespace issues here, obfuscating what's a condition and what what the instructions are. Given that the bodies do 'return', the 'elses' may be unnecessary and could save indentation levels. > +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 i; Several blocks of declarations? Remove the empty line. And I'm really not a fan of mixing assignment instructions "this complex, beyond trivial initialization" with declarations. But this style is rather popular. :( Since this is a new driver, there is not reason to "continue" what others did before. > + > + chained_irq_enter(chip, desc); > + /* Handling for level trigger and edge trigger is different */ > + if (altera_gc->interrupt_trigger == IRQ_TYPE_LEVEL_HIGH) { > + status = readl_relaxed(mm_gc->regs + ALTERA_GPIO_DATA); > + status &= readl_relaxed(mm_gc->regs + ALTERA_GPIO_IRQ_MASK); > + > + for (i = 0; i < mm_gc->gc.ngpio; i++) { > + if (status & BIT(i)) { > + generic_handle_irq(irq_find_mapping( > + altera_gc->domain, i)); > + } > + } > + } 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 (i = 0; i < mm_gc->gc.ngpio; i++) { > + if (status & BIT(i)) { > + generic_handle_irq(irq_find_mapping( > + altera_gc->domain, i)); > + } > + } > + } > + } for_each_set_bit() might be more descriptive, save indentation levels, and could use availalbe CPU instructions. There are more whitespace issues here. And I'm afraid that use of _relaxed() accessors makes the driver depend on specific architectures, which should then reflect in the Kconfig dependencies. Given that we are not talking about a GPIO block that is contained in SoCs which implement a specific CPU, but instead talk about an IP block that is supposed to get implemented in FPGAs connected to arbitrary CPUs, I'd suggest to use more portable instructions. > + > + chained_irq_exit(chip, desc); > +} > +int altera_gpio_probe(struct platform_device *pdev) > +{ > + struct device_node *node = pdev->dev.of_node; > + int i, 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->domain = 0; This really needs a whitespace cleanup. I do suggest to clearly separate declarations and instructions. This reduces diffs upon further maintenance, and really isn't that expensive in terms of typing characters (which should not be a criterion during development anyway). > + > + spin_lock_init(&altera_gc->gpio_lock); > + > + id = pdev->id; > + > + if (of_property_read_u32(node, "altr,gpio-bank-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) { > + dev_warn(&pdev->dev, > + "ngpio is greater than 32, defaulting to 32\n"); > + altera_gc->mmchip.gc.ngpio = 32; > + } Does this "maximum number of supported pins per bank" deserve a symbolic name? The "32' is repeated here several times within a few lines. > + > + 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) { > + dev_err(&pdev->dev, "Failed adding memory mapped gpiochip\n"); > + return ret; > + } > + > + platform_set_drvdata(pdev, altera_gc); > + > + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0); > + > + if (!altera_gc->mapped_irq) > + goto skip_irq; > + > + altera_gc->domain = irq_domain_add_linear(node, > + altera_gc->mmchip.gc.ngpio, &altera_gpio_irq_ops, altera_gc); > + > + if (!altera_gc->domain) { > + ret = -ENODEV; > + goto dispose_irq; > + } > + > + for (i = 0; i < altera_gc->mmchip.gc.ngpio; i++) > + irq_create_mapping(altera_gc->domain, i); > + > + if (of_property_read_u32(node, "altr,interrupt_type", ®)) { > + ret = -EINVAL; > + dev_err(&pdev->dev, > + "altr,interrupt_type value not set in device tree\n"); > + goto teardown; > + } > + altera_gc->interrupt_trigger = 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; The 'skip_irq' label should be here. It's really not an exceptional case or error path, it's just a shortcut for the absence of an optional feature. > + > +teardown: > + irq_domain_remove(altera_gc->domain); > +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; > +} virtually yours Gerhard Sittig
On Fri, Mar 07, 2014 at 11:31 +0800, Linus Walleij wrote: > > On Mon, Mar 3, 2014 at 6:27 PM, <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 using dipsw and LED using LED framework. > > > > Signed-off-by: Tien Hock Loh <thloh@altera.com> > > Can I get some ACKs on this? Like from Gerhard, DT folks? > > Or is there still more things to do? Sorry, had flagged the patch for review, but did not get around to respond earlier. No ACK from me for v7 as it is. Am I supposed to provide ACKs at all, given that I'm a reviewer and fellow developer. I always understood that Acked-By is something more formal, where one maintainer signals to another maintainer that it's OK to go though a different tree. virtually yours Gerhard Sittig
On Sat, Mar 8, 2014 at 12:50 PM, Gerhard Sittig <gsi@denx.de> wrote: > On Fri, Mar 07, 2014 at 11:31 +0800, Linus Walleij wrote: >> >> On Mon, Mar 3, 2014 at 6:27 PM, <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 using dipsw and LED using LED framework. >> > >> > Signed-off-by: Tien Hock Loh <thloh@altera.com> >> >> Can I get some ACKs on this? Like from Gerhard, DT folks? >> >> Or is there still more things to do? > > Sorry, had flagged the patch for review, but did not get around > to respond earlier. No ACK from me for v7 as it is. OK I'm holding this back. > Am I supposed to provide ACKs at all, given that I'm a reviewer > and fellow developer. I always understood that Acked-By is > something more formal, where one maintainer signals to another > maintainer that it's OK to go though a different tree. In this case I want an Acked-by/Reviewed-by tag (choose!) beacuse I trust your ability to review this specific code, as simple as that. :-) Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 7, 2014 at 11:14 PM, Josh Cartwright <joshc@codeaurora.org> wrote: > On Mon, Mar 03, 2014 at 06:27:43PM +0800, 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 using dipsw and LED using LED framework. >> >> Signed-off-by: Tien Hock Loh <thloh@altera.com> >> --- >> .../devicetree/bindings/gpio/gpio-altera.txt | 44 +++ >> drivers/gpio/Kconfig | 7 + >> drivers/gpio/Makefile | 1 + >> drivers/gpio/gpio-altera.c | 419 +++++++++++++++++++++ >> 4 files changed, 471 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..1de1f9b >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt >> @@ -0,0 +1,44 @@ >> +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 >> + - The first cell is the gpio offset number >> +- gpio-controller : Marks the device node as a GPIO controller. >> +- #interrupt-cells : Should be 1. >> + - The first cell is the GPIO offset number within the GPIO controller. >> +- interrupts: Specify the interrupt. >> +- interrupt-controller: Mark the device node as an interrupt controller >> + >> +Altera GPIO specific required properties: >> +- altr,interrupt_trigger: Specifies the interrupt trigger type the GPIO >> + hardware is synthesized. This field is required if the Altera GPIO controller >> + used has IRQ enabled as the interrupt type is not software controlled, >> + but hardware synthesized. Required if GPIO is used as an interrupt >> + controller. The value is defined in <dt-bindings/interrupt-controller/irq.h> >> + Only the following flags are supported: >> + IRQ_TYPE_EDGE_RISING >> + IRQ_TYPE_EDGE_FALLING >> + IRQ_TYPE_EDGE_BOTH >> + IRQ_TYPE_LEVEL_HIGH > > I'd suggest "altr,interrupt-trigger" (with a hyphen). So, if I'm > understanding properly, this controller doesn't support per-gpio trigger > settings? Okay, I'll change it to interrupt-trigger. Yes, the hardware doesn't support per-gpio trigger. > > Low-level triggered interrupts aren't supported? Yes, the hardware controller doesn't support it. > >> + >> +Altera GPIO specific optional properties: >> +- altr,gpio-bank-width: Width of the GPIO bank. This defines how many pins the >> + GPIO device has. Ranges between 1-32. Optional and defaults to 32 is not >> + specified. > > Generally, this is called 'ngpio', I think. Might be nice to stay > consistent with other bindings. Noted. Thanks. > >> + >> +Example: >> + >> +gpio_altr: gpio@40000 { >> + compatible = "altr,pio-1.0"; >> + reg = <0xff200000 0x10>; >> + interrupts = <0 45 4>; >> + altr,gpio-bank-width = <32>; >> + altr,interrupt_trigger = <IRQ_TYPE_EDGE_RISING>; >> + #gpio-cells = <1>; >> + gpio-controller; >> + #interrupt-cells = <1>; >> + interrupt-controller; >> +}; >> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig >> index 6973387..07bccdf 100644 >> --- a/drivers/gpio/Kconfig >> +++ b/drivers/gpio/Kconfig >> @@ -128,6 +128,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. > > nit: you should use tabs for indentation instead of spaces to stay > consistent. OK. > >> + >> 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 5d50179..88374d2 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_BCM_KONA) += gpio-bcm-kona.o >> diff --git a/drivers/gpio/gpio-altera.c b/drivers/gpio/gpio-altera.c >> new file mode 100644 >> index 0000000..ab0738f >> --- /dev/null >> +++ b/drivers/gpio/gpio-altera.c >> @@ -0,0 +1,419 @@ >> +/* >> + * 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 2 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/init.h> >> +#include <linux/irqdomain.h> >> +#include <linux/interrupt.h> >> +#include <linux/io.h> >> +#include <linux/irq.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/of_irq.h> >> +#include <linux/of_gpio.h> >> +#include <linux/platform_device.h> >> +#include <linux/slab.h> >> + >> +#include <linux/irqchip/chained_irq.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 >> +* @mmchip : memory mapped chip structure. >> +* @irq : irq domain that this driver is registered to. > > s/@irq/@domain/ ? > Yes missed this sorry. >> +* @gpio_lock : synchronization lock so that new irq/set/get requests >> + will be blocked until the current one completes. >> +* @interrupt_trigger : specifies the hardware configured IRQ trigger type >> + (rising, falling, both, high) > > @edge_type? > Oh, a stray. I'll remove this. >> +* @mapped_irq : kernel mapped irq number. >> +*/ >> +struct altera_gpio_chip { >> + struct of_mm_gpio_chip mmchip; > > I had never really looked into of_mm_gpio_chip before but...does this > really get you anything? All it does is manage mapping your registers > for you; as far as I can tell it's just another layer of indirection > with no gain. > > I'd suggest lifting 'regs' and 'chip' directly into your > altera_gpio_chip: > > struct altera_gpio_chip { > struct gpio_chip chip; > struct irq_domain *domain; > void __iomem *regs; > spinlock_t gpio_lock; > int mapped_irq; > }; > > [..] > The variable is used in the helper function of_mm_gpiochip_add() for general memory mapped gpio to reduce code duplications. A lot of the memory mapped GPIO driver uses this, I just refers to them. Please let me know if you strongly feel otherwise. >> +static unsigned int altera_gpio_irq_startup(struct irq_data *d) >> +{ >> + altera_gpio_irq_unmask(d); >> + >> + return 0; >> +} >> + >> +static void altera_gpio_irq_shutdown(struct irq_data *d) >> +{ >> + altera_gpio_irq_unmask(d); >> +} > > Should you really even be touching masks in startup/shutdown? > I'm referring from other GPIO drivers and that's what they do. I do make a mistake by unmasking the PIOs during shutdown. I'll fix this. >> +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, >> + .irq_startup = altera_gpio_irq_startup, >> + .irq_shutdown = altera_gpio_irq_shutdown, >> +}; >> + > [..] >> +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->domain) >> + return -ENXIO; > > How could this happen (hint, move your domain creation before > gpiochip_add). Noted. Thanks. > >> + if (offset < altera_gc->mmchip.gc.ngpio) >> + return irq_find_mapping(altera_gc->domain, 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 i; >> + >> + chained_irq_enter(chip, desc); >> + /* Handling for level trigger and edge trigger is different */ >> + if (altera_gc->interrupt_trigger == IRQ_TYPE_LEVEL_HIGH) { > > Suggestion: split this into two handling functions, install > one-or-the-other in your probe() based on the "altr,trigger-type" > property. Bonus points: remove interrupt_trigger member from > altera_gpio_chip entirely. > Good point. I'll implement this. However I don't think I can remove interrupt_trigger from altera_gpio_chip since when doing altera_gpio_irq_set_type we still need the variable to determine if we can set the type requested by the user. >> + status = readl_relaxed(mm_gc->regs + ALTERA_GPIO_DATA); >> + status &= readl_relaxed(mm_gc->regs + ALTERA_GPIO_IRQ_MASK); >> + >> + for (i = 0; i < mm_gc->gc.ngpio; i++) { >> + if (status & BIT(i)) { >> + generic_handle_irq(irq_find_mapping( >> + altera_gc->domain, i)); >> + } >> + } > > You may find for_each_set_bit() handy. > Good point. Will implement. Thanks. >> + } 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 (i = 0; i < mm_gc->gc.ngpio; i++) { >> + if (status & BIT(i)) { >> + generic_handle_irq(irq_find_mapping( >> + altera_gc->domain, i)); >> + } >> + } >> + } >> + } >> + >> + chained_irq_exit(chip, desc); >> +} >> + >> +static int altera_gpio_irq_map(struct irq_domain *h, unsigned int irq, >> + irq_hw_number_t hw_irq_num) >> +{ >> + irq_set_chip_data(irq, h->host_data); >> + irq_set_chip_and_handler(irq, &altera_irq_chip, handle_level_irq); >> + irq_set_irq_type(irq, IRQ_TYPE_NONE); > > Does irq_set_irq_type(irq, IRQ_TYPE_NONE) even do anything meaningful here? > I'll remove this. >> + >> + return 0; >> +} >> + >> +static struct irq_domain_ops altera_gpio_irq_ops = { > > const? > I'll add this. >> + .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 i, 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); > > This print is not necessary, as the allocator will complain loudly on > your behalf. Also, I'd suggest you not define and initialize > 'altera_gc' on the same line. Noted. > >> + return -ENOMEM; >> + } >> + altera_gc->domain = 0; >> + >> + spin_lock_init(&altera_gc->gpio_lock); >> + >> + id = pdev->id; >> + >> + if (of_property_read_u32(node, "altr,gpio-bank-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) { >> + dev_warn(&pdev->dev, >> + "ngpio is greater than 32, defaulting to 32\n"); >> + 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) { >> + dev_err(&pdev->dev, "Failed adding memory mapped gpiochip\n"); >> + return ret; >> + } >> + >> + platform_set_drvdata(pdev, altera_gc); >> + >> + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0); >> > > platform_get_irq(pdev, 0); > OK. >> + >> + if (!altera_gc->mapped_irq) >> + goto skip_irq; >> + >> + altera_gc->domain = irq_domain_add_linear(node, >> + altera_gc->mmchip.gc.ngpio, &altera_gpio_irq_ops, altera_gc); >> + >> + if (!altera_gc->domain) { >> + ret = -ENODEV; >> + goto dispose_irq; >> + } >> + >> + for (i = 0; i < altera_gc->mmchip.gc.ngpio; i++) >> + irq_create_mapping(altera_gc->domain, i); >> + >> + if (of_property_read_u32(node, "altr,interrupt_type", ®)) { >> + ret = -EINVAL; >> + dev_err(&pdev->dev, >> + "altr,interrupt_type value not set in device tree\n"); >> + goto teardown; >> + } >> + altera_gc->interrupt_trigger = 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->domain); >> +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->domain) { > > How could this happen? > Must be me getting overlay paranoid. I'll remove this. >> + irq_dispose_mapping(altera_gc->mapped_irq); > > Does irq_domain_remove() not teardown mappings? The irq_domain_remove states that caller has to ensure all mapping has been disposed prior to calling the function. >> + >> + for (i = 0; i < altera_gc->mmchip.gc.ngpio; i++) { >> + irq = irq_find_mapping(altera_gc->domain, i); >> + if (irq > 0) >> + irq_dispose_mapping(irq); >> + } >> + >> + irq_domain_remove(altera_gc->domain); >> + } >> + >> + 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[] = { > > const? > OK. >> + { .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_AUTHOR("Tien Hock Loh <thloh@altera.com>"); >> +MODULE_DESCRIPTION("Altera GPIO driver"); >> +MODULE_LICENSE("GPL"); > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > hosted by The Linux Foundation -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Mar 8, 2014 at 7:44 PM, Gerhard Sittig <gsi@denx.de> wrote: > On Mon, Mar 03, 2014 at 18:27 +0800, 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 using dipsw and LED using LED framework. >> >> Signed-off-by: Tien Hock Loh <thloh@altera.com> >> --- >> .../devicetree/bindings/gpio/gpio-altera.txt | 44 +++ >> drivers/gpio/Kconfig | 7 + >> drivers/gpio/Makefile | 1 + >> drivers/gpio/gpio-altera.c | 419 +++++++++++++++++++++ >> 4 files changed, 471 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-altera.txt >> create mode 100644 drivers/gpio/gpio-altera.c > > Let's see. A v7, i.e. quite some iterations done, and still > whitespace issues, and not a single line of changelogs. Not > exactly an invitation for reviewers to spend their time on it. > It's hard to tell which feedback was received before, and what of > it has been addressed. Noted. I'll be sure to put changelogs and fix all the whitespace issues. > > Aren't binding patches to be separate (and first) in series these > days, while driver adjustment or introduction then follows to > implement what was specified? OK. I'll split the patch to two with DT bindings as the first one. > >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt >> @@ -0,0 +1,44 @@ >> +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 >> + - The first cell is the gpio offset number > > Will there never be any use of flags, that usually are kept in > the second cell? Can we tell for sure that one cell is enough? OK. I'll add a second cell and mark it as currently unused. > >> +- gpio-controller : Marks the device node as a GPIO controller. >> +- #interrupt-cells : Should be 1. >> + - The first cell is the GPIO offset number within the GPIO controller. >> +- interrupts: Specify the interrupt. >> +- interrupt-controller: Mark the device node as an interrupt controller > > Is #interrupt-cells = <1> enough? Are triggers fixed in the > hardware? A comment about it may prevent people asking > questions. (The motivation might be mentioned below, see the > comment on the two "required properties" sections.) > Yes the triggers are fixed in the hardware. I'll add a comment on this. > I'd sort the last three items differently. 'interrupts' is where > the GPIO block is "a client" to the IRQ controller which it is > connected to. '#interrupt-cells' is the "server side" because > the GPIO block is an IRQ controller and has the > 'interrupt-controller' property. > I'm assuming the order should be: "interrupt-controller", "interrupt-cells", "interrupts". Am I correct here? >> + >> +Altera GPIO specific required properties: > > This made me stop and wonder. This is the "Altera GPIO > controller bindings" document, and it has a "Required properties" > section as well as an "Altera GPIO specific required properties" > section? Isn't this highly redundant, and confusing at the same > time? > Noted. I were referring to how other binding docs works and they have these "specific properties" header. It seems I should just specify required properties and optional properties. I'll lump them together. >> +- altr,interrupt_trigger: Specifies the interrupt trigger type the GPIO >> + hardware is synthesized. This field is required if the Altera GPIO controller >> + used has IRQ enabled as the interrupt type is not software controlled, >> + but hardware synthesized. Required if GPIO is used as an interrupt >> + controller. The value is defined in <dt-bindings/interrupt-controller/irq.h> >> + Only the following flags are supported: >> + IRQ_TYPE_EDGE_RISING >> + IRQ_TYPE_EDGE_FALLING >> + IRQ_TYPE_EDGE_BOTH >> + IRQ_TYPE_LEVEL_HIGH >> + >> +Altera GPIO specific optional properties: >> +- altr,gpio-bank-width: Width of the GPIO bank. This defines how many pins the >> + GPIO device has. Ranges between 1-32. Optional and defaults to 32 is not >> + specified. >> + >> +Example: >> + >> +gpio_altr: gpio@40000 { >> + compatible = "altr,pio-1.0"; >> + reg = <0xff200000 0x10>; >> + interrupts = <0 45 4>; >> + altr,gpio-bank-width = <32>; >> + altr,interrupt_trigger = <IRQ_TYPE_EDGE_RISING>; >> + #gpio-cells = <1>; >> + gpio-controller; >> + #interrupt-cells = <1>; >> + interrupt-controller; >> +}; > > The node's address does not match the 'reg' property. The > interrupt trigger uses symbolic names, so could the 'interrupts' > spec. Examples should not assume an external 'interrupt-parent' > but should list them for completeness. > Noted. I'll change the address. >> --- a/drivers/gpio/Kconfig >> +++ b/drivers/gpio/Kconfig >> @@ -128,6 +128,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. >> + > > I guess checkpatch nags about the rather short help text. > Tristate options probably should mention the opportunity to build > a module, and by convention should state what the module's name > then would be (which in total gets you the minimum number of help > text lines as a byproduct). > Noted. I'll update this. >> --- /dev/null >> +++ b/drivers/gpio/gpio-altera.c >> @@ -0,0 +1,419 @@ >> [ ... ] >> + >> +#include <linux/gpio.h> >> +#include <linux/init.h> >> +#include <linux/irqdomain.h> >> +#include <linux/interrupt.h> >> +#include <linux/io.h> >> +#include <linux/irq.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/of_irq.h> >> +#include <linux/of_gpio.h> >> +#include <linux/platform_device.h> >> +#include <linux/slab.h> >> + >> +#include <linux/irqchip/chained_irq.h> > > Includes should be alpha-sorted. To detect duplicates, and to > avoid or reduce conflicts. > 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 > > OUTSET and OUTCLEAR are not used in the driver source. You might > as well drop their declarations (after all you are not declaring > the register set layout here, but are just introducing magic > offset numbers for some of the registers that the driver may > access). > 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; > > I'd suggest <stdint.h> style fixed width data types (uint32_t) or > their kernel equivalents (u32) for operations on fixed width > peripheral registers. > OK. 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 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 (type == IRQ_TYPE_LEVEL_HIGH && >> + altera_gc->interrupt_trigger == IRQ_TYPE_LEVEL_HIGH) { >> + return 0; >> + } else { >> + if (type == IRQ_TYPE_EDGE_RISING && >> + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_RISING) >> + return 0; >> + else if (type == IRQ_TYPE_EDGE_FALLING && >> + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_FALLING) >> + return 0; >> + else if (type == IRQ_TYPE_EDGE_BOTH && >> + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_BOTH) >> + return 0; >> + } >> + >> + return -EINVAL; >> +} > > Whitespace issues here, obfuscating what's a condition and what > what the instructions are. Given that the bodies do 'return', > the 'elses' may be unnecessary and could save indentation levels. > Noted. I'll fix this. >> +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 i; > > Several blocks of declarations? Remove the empty line. > > And I'm really not a fan of mixing assignment instructions "this > complex, beyond trivial initialization" with declarations. But > this style is rather popular. :( Since this is a new driver, > there is not reason to "continue" what others did before. > OK noted. I'll implement this correctly. >> + >> + chained_irq_enter(chip, desc); >> + /* Handling for level trigger and edge trigger is different */ >> + if (altera_gc->interrupt_trigger == IRQ_TYPE_LEVEL_HIGH) { >> + status = readl_relaxed(mm_gc->regs + ALTERA_GPIO_DATA); >> + status &= readl_relaxed(mm_gc->regs + ALTERA_GPIO_IRQ_MASK); >> + >> + for (i = 0; i < mm_gc->gc.ngpio; i++) { >> + if (status & BIT(i)) { >> + generic_handle_irq(irq_find_mapping( >> + altera_gc->domain, i)); >> + } >> + } >> + } 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 (i = 0; i < mm_gc->gc.ngpio; i++) { >> + if (status & BIT(i)) { >> + generic_handle_irq(irq_find_mapping( >> + altera_gc->domain, i)); >> + } >> + } >> + } >> + } > > for_each_set_bit() might be more descriptive, save indentation > levels, and could use availalbe CPU instructions. > > There are more whitespace issues here. > > And I'm afraid that use of _relaxed() accessors makes the driver > depend on specific architectures, which should then reflect in > the Kconfig dependencies. > > Given that we are not talking about a GPIO block that is > contained in SoCs which implement a specific CPU, but instead > talk about an IP block that is supposed to get implemented in > FPGAs connected to arbitrary CPUs, I'd suggest to use more > portable instructions. > I'll use for_each_set_bit() function as the iterator. I'll fix the whitespace issue and run the check_patch before submitting again. Yes. There are complains about using _relaxed(). I'll remove the _relaxed() function and use just writel and readl. >> + >> + chained_irq_exit(chip, desc); >> +} > > >> +int altera_gpio_probe(struct platform_device *pdev) >> +{ >> + struct device_node *node = pdev->dev.of_node; >> + int i, 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->domain = 0; > > This really needs a whitespace cleanup. I do suggest to clearly > separate declarations and instructions. This reduces diffs upon > further maintenance, and really isn't that expensive in terms of > typing characters (which should not be a criterion during > development anyway). > Noted. >> + >> + spin_lock_init(&altera_gc->gpio_lock); >> + >> + id = pdev->id; >> + >> + if (of_property_read_u32(node, "altr,gpio-bank-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) { >> + dev_warn(&pdev->dev, >> + "ngpio is greater than 32, defaulting to 32\n"); >> + altera_gc->mmchip.gc.ngpio = 32; >> + } > > Does this "maximum number of supported pins per bank" deserve a > symbolic name? The "32' is repeated here several times within a > few lines. > OK I'll add a macro to the number. >> + >> + 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) { >> + dev_err(&pdev->dev, "Failed adding memory mapped gpiochip\n"); >> + return ret; >> + } >> + >> + platform_set_drvdata(pdev, altera_gc); >> + >> + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0); >> + >> + if (!altera_gc->mapped_irq) >> + goto skip_irq; >> + >> + altera_gc->domain = irq_domain_add_linear(node, >> + altera_gc->mmchip.gc.ngpio, &altera_gpio_irq_ops, altera_gc); >> + >> + if (!altera_gc->domain) { >> + ret = -ENODEV; >> + goto dispose_irq; >> + } >> + >> + for (i = 0; i < altera_gc->mmchip.gc.ngpio; i++) >> + irq_create_mapping(altera_gc->domain, i); >> + >> + if (of_property_read_u32(node, "altr,interrupt_type", ®)) { >> + ret = -EINVAL; >> + dev_err(&pdev->dev, >> + "altr,interrupt_type value not set in device tree\n"); >> + goto teardown; >> + } >> + altera_gc->interrupt_trigger = 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; > > The 'skip_irq' label should be here. It's really not an > exceptional case or error path, it's just a shortcut for the > absence of an optional feature. > OK noted. >> + >> +teardown: >> + irq_domain_remove(altera_gc->domain); >> +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; >> +} > > > virtually yours > Gerhard Sittig > -- > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 19, 2014 at 6:09 PM, Tien Hock Loh <thloh@altera.com> wrote: > On Fri, Mar 7, 2014 at 11:14 PM, Josh Cartwright <joshc@codeaurora.org> wrote: >> On Mon, Mar 03, 2014 at 06:27:43PM +0800, 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 using dipsw and LED using LED framework. >>> >>> Signed-off-by: Tien Hock Loh <thloh@altera.com> >>> --- >> >>> + return -ENOMEM; >>> + } >>> + altera_gc->domain = 0; >>> + >>> + spin_lock_init(&altera_gc->gpio_lock); >>> + >>> + id = pdev->id; >>> + >>> + if (of_property_read_u32(node, "altr,gpio-bank-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) { >>> + dev_warn(&pdev->dev, >>> + "ngpio is greater than 32, defaulting to 32\n"); >>> + 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) { >>> + dev_err(&pdev->dev, "Failed adding memory mapped gpiochip\n"); >>> + return ret; >>> + } >>> + >>> + platform_set_drvdata(pdev, altera_gc); >>> + >>> + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0); >>> >> >> platform_get_irq(pdev, 0); >> > OK. > platform_get_irq doesn't create the irq mapping which is needed by the driver. Since this driver is targeted at using of, should I be using irq_of_parse_and_map or should I still redo the codes with platform_get_irq and irq_create_mapping? I think the latter would be introducing code redundancy. Please advice. Thanks. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 07, 2014 at 04:00:43PM +0800, Tien Hock Loh wrote: > On Wed, Mar 19, 2014 at 6:09 PM, Tien Hock Loh <thloh@altera.com> wrote: > > On Fri, Mar 7, 2014 at 11:14 PM, Josh Cartwright <joshc@codeaurora.org> wrote: > >> On Mon, Mar 03, 2014 at 06:27:43PM +0800, thloh@altera.com wrote: > >>> From: Tien Hock Loh <thloh@altera.com> [..] > >>> + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0); > >>> > >> > >> platform_get_irq(pdev, 0); > >> > > OK. > > > > platform_get_irq doesn't create the irq mapping which is needed by the > driver. Since this driver is targeted at using of, should I be using > irq_of_parse_and_map or should I still redo the codes with > platform_get_irq and irq_create_mapping? I think the latter would be > introducing code redundancy. Please advice. Yes, it is technically true that platform_get_irq() doesn't do the mapping directly, but that's because the mapping is setup earlier, when of_device_alloc() (drivers/of/platform.c) allocates resources for your platform device. Calling irq_of_parse_and_map() should be unnecessary.
>>>>> 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> >>>>> --- >>>> >>>>> + return -ENOMEM; >>>>> + } >>>>> + altera_gc->domain = 0; >>>>> + >>>>> + spin_lock_init(&altera_gc->gpio_lock); >>>>> + >>>>> + id = pdev->id; >>>>> + >>>>> + if (of_property_read_u32(node, "altr,gpio-bank-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) { >>>>> + dev_warn(&pdev->dev, >>>>> + "ngpio is greater than 32, defaulting to 32\n"); >>>>> + 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) { >>>>> + dev_err(&pdev->dev, "Failed adding memory mapped gpiochip\n"); >>>>> + return ret; >>>>> + } >>>>> + >>>>> + platform_set_drvdata(pdev, altera_gc); >>>>> + >>>>> + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0); >>>>> >>>> >>>> platform_get_irq(pdev, 0); >>>> >>> OK. >>> >> >> platform_get_irq doesn't create the irq mapping which is needed by the >> driver. Since this driver is targeted at using of, should I be using >> irq_of_parse_and_map or should I still redo the codes with >> platform_get_irq and irq_create_mapping? I think the latter would be >> introducing code redundancy. Please advice. > > Yes, it is technically true that platform_get_irq() doesn't do the mapping directly, but that's > because the mapping is setup earlier, when > of_device_alloc() (drivers/of/platform.c) allocates resources for your platform device. > > Calling irq_of_parse_and_map() should be unnecessary. I checked and tried running the without irq_create_mapping but it seems the mapping is not done. What I've seen other GPIO driver is doing is to create the mapping during the gpio_to_irq call. However Linus suggested we are avoiding that route, thus the use of irq_of_parse_and_map. Do you agree with my findings? >> >> Thanks. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 3, 2014 at 11:27 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 using dipsw and LED using LED framework. > > Signed-off-by: Tien Hock Loh <thloh@altera.com> Some time has passed since this was posted and we now have GPIO irqchip helpers in the gpiolib core. If you repost this, please reform this driver to use the gpiochip irqchip helpers in the manner of e.g. drivers/gpio/gpio-pl061.c. More examples and documentation is available on the "devel" branch in the gpio git tree (also in linux-next). Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 23, 2014 at 9:32 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, Mar 3, 2014 at 11:27 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 using dipsw and LED using LED framework. >> >> Signed-off-by: Tien Hock Loh <thloh@altera.com> > > Some time has passed since this was posted and we now have > GPIO irqchip helpers in the gpiolib core. > > If you repost this, please reform this driver to use the gpiochip > irqchip helpers in the manner of e.g. drivers/gpio/gpio-pl061.c. > > More examples and documentation is available on the "devel" > branch in the gpio git tree (also in linux-next). OK noted. I'll do that. Thanks for the info. > > Yours, > Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 7 Apr 2014 12:11:15 -0500, Josh Cartwright <joshc@codeaurora.org> wrote: > On Mon, Apr 07, 2014 at 04:00:43PM +0800, Tien Hock Loh wrote: > > On Wed, Mar 19, 2014 at 6:09 PM, Tien Hock Loh <thloh@altera.com> wrote: > > > On Fri, Mar 7, 2014 at 11:14 PM, Josh Cartwright <joshc@codeaurora.org> wrote: > > >> On Mon, Mar 03, 2014 at 06:27:43PM +0800, thloh@altera.com wrote: > > >>> From: Tien Hock Loh <thloh@altera.com> > [..] > > >>> + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0); > > >>> > > >> > > >> platform_get_irq(pdev, 0); > > >> > > > OK. > > > > > > > platform_get_irq doesn't create the irq mapping which is needed by the > > driver. Since this driver is targeted at using of, should I be using > > irq_of_parse_and_map or should I still redo the codes with > > platform_get_irq and irq_create_mapping? I think the latter would be > > introducing code redundancy. Please advice. > > Yes, it is technically true that platform_get_irq() doesn't do the > mapping directly, but that's because the mapping is setup earlier, when > of_device_alloc() (drivers/of/platform.c) allocates resources for your > platform device. > > Calling irq_of_parse_and_map() should be unnecessary. In the latest mainline, platform_get_irq() now does the mapping when using an IRQ specified in the device tree. g. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/gpio/gpio-altera.txt b/Documentation/devicetree/bindings/gpio/gpio-altera.txt new file mode 100644 index 0000000..1de1f9b --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt @@ -0,0 +1,44 @@ +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 + - The first cell is the gpio offset number +- gpio-controller : Marks the device node as a GPIO controller. +- #interrupt-cells : Should be 1. + - The first cell is the GPIO offset number within the GPIO controller. +- interrupts: Specify the interrupt. +- interrupt-controller: Mark the device node as an interrupt controller + +Altera GPIO specific required properties: +- altr,interrupt_trigger: Specifies the interrupt trigger type the GPIO + hardware is synthesized. This field is required if the Altera GPIO controller + used has IRQ enabled as the interrupt type is not software controlled, + but hardware synthesized. Required if GPIO is used as an interrupt + controller. The value is defined in <dt-bindings/interrupt-controller/irq.h> + Only the following flags are supported: + IRQ_TYPE_EDGE_RISING + IRQ_TYPE_EDGE_FALLING + IRQ_TYPE_EDGE_BOTH + IRQ_TYPE_LEVEL_HIGH + +Altera GPIO specific optional properties: +- altr,gpio-bank-width: Width of the GPIO bank. This defines how many pins the + GPIO device has. Ranges between 1-32. Optional and defaults to 32 is not + specified. + +Example: + +gpio_altr: gpio@40000 { + compatible = "altr,pio-1.0"; + reg = <0xff200000 0x10>; + interrupts = <0 45 4>; + altr,gpio-bank-width = <32>; + altr,interrupt_trigger = <IRQ_TYPE_EDGE_RISING>; + #gpio-cells = <1>; + gpio-controller; + #interrupt-cells = <1>; + interrupt-controller; +}; diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 6973387..07bccdf 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -128,6 +128,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 5d50179..88374d2 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_BCM_KONA) += gpio-bcm-kona.o diff --git a/drivers/gpio/gpio-altera.c b/drivers/gpio/gpio-altera.c new file mode 100644 index 0000000..ab0738f --- /dev/null +++ b/drivers/gpio/gpio-altera.c @@ -0,0 +1,419 @@ +/* + * 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 2 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/init.h> +#include <linux/irqdomain.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/irq.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of_irq.h> +#include <linux/of_gpio.h> +#include <linux/platform_device.h> +#include <linux/slab.h> + +#include <linux/irqchip/chained_irq.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 +* @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. +* @interrupt_trigger : specifies the hardware configured IRQ trigger type + (rising, falling, both, high) +* @mapped_irq : kernel mapped irq number. +*/ +struct altera_gpio_chip { + struct of_mm_gpio_chip mmchip; + struct irq_domain *domain; + spinlock_t gpio_lock; + int interrupt_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 (type == IRQ_TYPE_LEVEL_HIGH && + altera_gc->interrupt_trigger == IRQ_TYPE_LEVEL_HIGH) { + return 0; + } else { + if (type == IRQ_TYPE_EDGE_RISING && + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_RISING) + return 0; + else if (type == IRQ_TYPE_EDGE_FALLING && + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_FALLING) + return 0; + else if (type == IRQ_TYPE_EDGE_BOTH && + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_BOTH) + return 0; + } + + return -EINVAL; +} + +static unsigned int altera_gpio_irq_startup(struct irq_data *d) +{ + altera_gpio_irq_unmask(d); + + return 0; +} + +static void altera_gpio_irq_shutdown(struct irq_data *d) +{ + altera_gpio_irq_unmask(d); +} + +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, + .irq_startup = altera_gpio_irq_startup, + .irq_shutdown = altera_gpio_irq_shutdown, +}; + +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); +} + +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); + if (value) + 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; + + 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); + if (value) + data_reg |= BIT(offset); + else + data_reg &= ~BIT(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->domain) + return -ENXIO; + if (offset < altera_gc->mmchip.gc.ngpio) + return irq_find_mapping(altera_gc->domain, 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 i; + + chained_irq_enter(chip, desc); + /* Handling for level trigger and edge trigger is different */ + if (altera_gc->interrupt_trigger == IRQ_TYPE_LEVEL_HIGH) { + status = readl_relaxed(mm_gc->regs + ALTERA_GPIO_DATA); + status &= readl_relaxed(mm_gc->regs + ALTERA_GPIO_IRQ_MASK); + + for (i = 0; i < mm_gc->gc.ngpio; i++) { + if (status & BIT(i)) { + generic_handle_irq(irq_find_mapping( + altera_gc->domain, i)); + } + } + } 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 (i = 0; i < mm_gc->gc.ngpio; i++) { + if (status & BIT(i)) { + generic_handle_irq(irq_find_mapping( + altera_gc->domain, i)); + } + } + } + } + + chained_irq_exit(chip, desc); +} + +static int altera_gpio_irq_map(struct irq_domain *h, unsigned int irq, + irq_hw_number_t hw_irq_num) +{ + irq_set_chip_data(irq, h->host_data); + irq_set_chip_and_handler(irq, &altera_irq_chip, handle_level_irq); + irq_set_irq_type(irq, 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 i, 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->domain = 0; + + spin_lock_init(&altera_gc->gpio_lock); + + id = pdev->id; + + if (of_property_read_u32(node, "altr,gpio-bank-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) { + dev_warn(&pdev->dev, + "ngpio is greater than 32, defaulting to 32\n"); + 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) { + dev_err(&pdev->dev, "Failed adding memory mapped gpiochip\n"); + return ret; + } + + platform_set_drvdata(pdev, altera_gc); + + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0); + + if (!altera_gc->mapped_irq) + goto skip_irq; + + altera_gc->domain = irq_domain_add_linear(node, + altera_gc->mmchip.gc.ngpio, &altera_gpio_irq_ops, altera_gc); + + if (!altera_gc->domain) { + ret = -ENODEV; + goto dispose_irq; + } + + for (i = 0; i < altera_gc->mmchip.gc.ngpio; i++) + irq_create_mapping(altera_gc->domain, i); + + if (of_property_read_u32(node, "altr,interrupt_type", ®)) { + ret = -EINVAL; + dev_err(&pdev->dev, + "altr,interrupt_type value not set in device tree\n"); + goto teardown; + } + altera_gc->interrupt_trigger = 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->domain); +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->domain) { + irq_dispose_mapping(altera_gc->mapped_irq); + + for (i = 0; i < altera_gc->mmchip.gc.ngpio; i++) { + irq = irq_find_mapping(altera_gc->domain, i); + if (irq > 0) + irq_dispose_mapping(irq); + } + + irq_domain_remove(altera_gc->domain); + } + + 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_AUTHOR("Tien Hock Loh <thloh@altera.com>"); +MODULE_DESCRIPTION("Altera GPIO driver"); +MODULE_LICENSE("GPL");