| Message ID | 1385524192-21501-1-git-send-email-thloh@altera.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi, On Wed, Nov 27, 2013 at 03:49:52AM +0000, 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 | 35 ++ > drivers/gpio/Kconfig | 7 + > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-altera.c | 395 +++++++++++++++++++++ > 4 files changed, 438 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..6c57d84 > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt > @@ -0,0 +1,35 @@ > +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. Huh? This line doesn't make much sense immediately after the description of interrupt-controller. Presumably this is associated iwth #interrupt-cells? > + > +Altera GPIO specific properties: > +- altr,gpio-bank-width: Width of the GPIO bank. This defines how many pins the > + GPIO device has. Ranges between 1-32. The code appears to handle this as an optional property, such that this proeprty is only required if the GPIO controller does not have 32 GPIOs. It would be worth notign that this is optional, and describing when this is expected in the binding. > +- 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. s/_/-/ What type is this, and what values are expected? The description implies that this is an optional property, but the code seems to require it. > + > +Example: > + > +gpio_altr: gpio_altr { > + compatible = "altr,pio-1.0"; > + reg = <0xff200000 0x10>; > + interrupts = <0 45 4>; > + altr,gpio-bank-width = <32>; > + altr,interrupt_trigger = <0>; The description doesn't tell me what this porperty in the example means... > + #gpio-cells = <1>; > + gpio-controller; > + #interrupt-cells = <1>; > + interrupt-controller; > +}; [...] > +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, "altr,gpio-bank-width", ®)) > + /*By default assume full GPIO controller*/ > + altera_gc->mmchip.gc.ngpio = 32; > + else > + altera_gc->mmchip.gc.ngpio = reg; As mentioned above, the binding doesn't describe this as optional, yet it's treated as such. > + > + 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); > + > + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0); > + > + if (!altera_gc->mapped_irq) > + goto skip_irq; It seems odd to jump directly to a return 0. Why not just return 0 here (or initialise ret to 0 and combine the two return cases)? Thanks, Mark. -- 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
Hi Mark On Wed, Nov 27, 2013 at 10:40 PM, Mark Rutland <mark.rutland@arm.com> wrote: > Hi, > > On Wed, Nov 27, 2013 at 03:49:52AM +0000, 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 | 35 ++ >> drivers/gpio/Kconfig | 7 + >> drivers/gpio/Makefile | 1 + >> drivers/gpio/gpio-altera.c | 395 +++++++++++++++++++++ >> 4 files changed, 438 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..6c57d84 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt >> @@ -0,0 +1,35 @@ >> +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. > > Huh? This line doesn't make much sense immediately after the description > of interrupt-controller. Presumably this is associated iwth > #interrupt-cells? Yes, it must have got displaced when I was editing the text. I'll get this fixed. > >> + >> +Altera GPIO specific properties: >> +- altr,gpio-bank-width: Width of the GPIO bank. This defines how many pins the >> + GPIO device has. Ranges between 1-32. > > The code appears to handle this as an optional property, such that this > proeprty is only required if the GPIO controller does not have 32 GPIOs. > It would be worth notign that this is optional, and describing when this > is expected in the binding. > Noted. Will add a note on required and optional parameters. >> +- 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. > > s/_/-/ > > What type is this, and what values are expected? > > The description implies that this is an optional property, but the code > seems to require it. This isn't always required, but only required when the controller is used as an interrupt controller. I'll specify that this is a required field if GPIO is used as an interrupt controller. I'll add in the expected values as well. > >> + >> +Example: >> + >> +gpio_altr: gpio_altr { >> + compatible = "altr,pio-1.0"; >> + reg = <0xff200000 0x10>; >> + interrupts = <0 45 4>; >> + altr,gpio-bank-width = <32>; >> + altr,interrupt_trigger = <0>; > > The description doesn't tell me what this porperty in the example > means... Noted, I'll put the supported interrupt trigger to the description above. This is the value of interrupt trigger as defined in <dt-bindings/interrupt-controller/irq.h>. > >> + #gpio-cells = <1>; >> + gpio-controller; >> + #interrupt-cells = <1>; >> + interrupt-controller; >> +}; > > [...] > >> +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, "altr,gpio-bank-width", ®)) >> + /*By default assume full GPIO controller*/ >> + altera_gc->mmchip.gc.ngpio = 32; >> + else >> + altera_gc->mmchip.gc.ngpio = reg; > > As mentioned above, the binding doesn't describe this as optional, yet > it's treated as such. I'll get this fixed in the documentation. > >> + >> + 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); >> + >> + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0); >> + >> + if (!altera_gc->mapped_irq) >> + goto skip_irq; > > It seems odd to jump directly to a return 0. Why not just return 0 here > (or initialise ret to 0 and combine the two return cases)? Noted. I'll return 0 here. > > Thanks, > Mark. Thanks for the comments. I'll post a patch v5. -- 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, Nov 27, 2013 at 11:49 +0800, thloh@altera.com wrote: > > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt > @@ -0,0 +1,35 @@ > +[ ... ] > + > +Example: > + > +gpio_altr: gpio_altr { > + compatible = "altr,pio-1.0"; > + reg = <0xff200000 0x10>; This length appears to be less than what the code defines (the latter has offsets beyond 0x10). > + interrupts = <0 45 4>; > + altr,gpio-bank-width = <32>; > + altr,interrupt_trigger = <0>; The numerical value of zero is not one of the valid options. Strictly speaking, "none" is zero -- but if the GPIO chip is an IRQ controller, the value should be _some_ trigger type, I guess. Is it useful to drop numerical specs and use symbolic names, when you already refer to dt-bindings header files in your reply? And/or reference common GPIO and IRQ related documentation. > + #gpio-cells = <1>; > + gpio-controller; > + #interrupt-cells = <1>; > + interrupt-controller; > +}; > --- /dev/null > +++ b/drivers/gpio/gpio-altera.c > @@ -0,0 +1,395 @@ > +[ ... ] > + > +#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 See above. These aren't used in the code, but they suggest that the register window is larger than 0x10. > +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; > + chip->irq_mask(&desc->irq_data); > + > + /* 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 (BIT(i) & status) { > + generic_handle_irq(irq_linear_revmap( > + altera_gc->irq, i)); > + } > + } for_each_set_bit() > + } 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 (BIT(i) & status) { > + generic_handle_irq(irq_linear_revmap( > + altera_gc->irq, i)); > + } > + } > + } > + } Is the "level low" trigger missing? Or is it not supported (in the hardware) and is this worth documenting? OTOH later versions of the IP block may support all types of triggers, so the main concern is that the driver's setup and handling are consistent, which they are from a quick glance. > + > + chip->irq_eoi(irq_desc_get_irq_data(desc)); > + chip->irq_unmask(&desc->irq_data); > +} > +int altera_gpio_probe(struct platform_device *pdev) > +{ > +[ ... ] > + > + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0); > + > + if (!altera_gc->mapped_irq) > + goto skip_irq; Personally I would not eliminate this goto instruction, but would put the skip_irq label into the regular probe() path. After all it's a shortcut when an option does not apply, it's not an error path. > + > + 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, "altr,interrupt_trigger", ®)) { > + ret = -EINVAL; > + pr_err("%s: interrupt_trigger value not set in device tree\n", > + node->full_name); > + 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; I'd put skip_irq here, right before the return (as other GPIO drivers do). The remaining lines of this routine all can be considered "exceptions" and "bail out" paths. > + > +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; > +} virtually yours Gerhard Sittig
On Fri, Nov 29, 2013 at 4:24 AM, Gerhard Sittig <gsi@denx.de> wrote: > On Wed, Nov 27, 2013 at 11:49 +0800, thloh@altera.com wrote: >> >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt >> @@ -0,0 +1,35 @@ >> +[ ... ] >> + >> +Example: >> + >> +gpio_altr: gpio_altr { >> + compatible = "altr,pio-1.0"; >> + reg = <0xff200000 0x10>; > > This length appears to be less than what the code defines (the > latter has offsets beyond 0x10). The higher registers (0x10 and 0x14) are only available if the controller is configured as GPIO output. Thus, there are two configuration - one that has the length 0x10 (GPIO without output ports), and one with the length 0x20 (GPIO with output ports). Is there anything I need to handle for cases like this? > >> + interrupts = <0 45 4>; >> + altr,gpio-bank-width = <32>; >> + altr,interrupt_trigger = <0>; > > The numerical value of zero is not one of the valid options. > Strictly speaking, "none" is zero -- but if the GPIO chip is an > IRQ controller, the value should be _some_ trigger type, I guess. > Yes, you're right, I'll get it fixed. > Is it useful to drop numerical specs and use symbolic names, when > you already refer to dt-bindings header files in your reply? > And/or reference common GPIO and IRQ related documentation. > Noted. I'll update the documentation on using dt-binding macros instead of values. >> + #gpio-cells = <1>; >> + gpio-controller; >> + #interrupt-cells = <1>; >> + interrupt-controller; >> +}; > > >> --- /dev/null >> +++ b/drivers/gpio/gpio-altera.c >> @@ -0,0 +1,395 @@ >> +[ ... ] >> + >> +#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 > > See above. These aren't used in the code, but they suggest that > the register window is larger than 0x10. See my explanation 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 i; >> + chip->irq_mask(&desc->irq_data); >> + >> + /* 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 (BIT(i) & status) { >> + generic_handle_irq(irq_linear_revmap( >> + altera_gc->irq, i)); >> + } >> + } > > for_each_set_bit() > >> + } 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 (BIT(i) & status) { >> + generic_handle_irq(irq_linear_revmap( >> + altera_gc->irq, i)); >> + } >> + } >> + } >> + } > > Is the "level low" trigger missing? Or is it not supported (in > the hardware) and is this worth documenting? OTOH later versions > of the IP block may support all types of triggers, so the main > concern is that the driver's setup and handling are consistent, > which they are from a quick glance. Yes, the Altera GPIO IP doesn't support level low trigger. Patch V5 has the supported interrupt triggers in the documentation. > >> + >> + chip->irq_eoi(irq_desc_get_irq_data(desc)); >> + chip->irq_unmask(&desc->irq_data); >> +} > > >> +int altera_gpio_probe(struct platform_device *pdev) >> +{ >> +[ ... ] >> + >> + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0); >> + >> + if (!altera_gc->mapped_irq) >> + goto skip_irq; > > Personally I would not eliminate this goto instruction, but would > put the skip_irq label into the regular probe() path. After all > it's a shortcut when an option does not apply, it's not an error > path. Right. I'll get this fixed. > >> + >> + 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, "altr,interrupt_trigger", ®)) { >> + ret = -EINVAL; >> + pr_err("%s: interrupt_trigger value not set in device tree\n", >> + node->full_name); >> + 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; > > I'd put skip_irq here, right before the return (as other GPIO > drivers do). The remaining lines of this routine all can be > considered "exceptions" and "bail out" paths. Noted. Thanks. > >> + >> +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; >> +} > > > 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 Thanks for the comments. I'll get these fixed. -- 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, Nov 29, 2013 at 09:59 +0800, Tien Hock Loh wrote: > > On Fri, Nov 29, 2013 at 4:24 AM, Gerhard Sittig <gsi@denx.de> wrote: > > On Wed, Nov 27, 2013 at 11:49 +0800, thloh@altera.com wrote: > >> > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt > >> @@ -0,0 +1,35 @@ > >> +[ ... ] > >> + > >> +Example: > >> + > >> +gpio_altr: gpio_altr { > >> + compatible = "altr,pio-1.0"; > >> + reg = <0xff200000 0x10>; > > > > This length appears to be less than what the code defines (the > > latter has offsets beyond 0x10). > > The higher registers (0x10 and 0x14) are only available if the > controller is configured as GPIO output. Thus, there are two > configuration - one that has the length 0x10 (GPIO without output > ports), and one with the length 0x20 (GPIO with output ports). Is > there anything I need to handle for cases like this? Hmm. I cannot tell whether there is a preference in mainline or whether there is prior art of IP blocks having different sizes depending on their "being compatible" to something or their configuration and feature set. Others may know more. With your explanation in mind, writing "specify an address and size that fits your component" may be considered "stating the obvious". So I don't have strong feelings about it, I was just wondering. This need not mean that something was wrong. :) virtually yours Gerhard Sittig
On Fri, Nov 29, 2013 at 5:41 PM, Gerhard Sittig <gsi@denx.de> wrote: > On Fri, Nov 29, 2013 at 09:59 +0800, Tien Hock Loh wrote: >> >> On Fri, Nov 29, 2013 at 4:24 AM, Gerhard Sittig <gsi@denx.de> wrote: >> > On Wed, Nov 27, 2013 at 11:49 +0800, thloh@altera.com wrote: >> >> >> >> --- /dev/null >> >> +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt >> >> @@ -0,0 +1,35 @@ >> >> +[ ... ] >> >> + >> >> +Example: >> >> + >> >> +gpio_altr: gpio_altr { >> >> + compatible = "altr,pio-1.0"; >> >> + reg = <0xff200000 0x10>; >> > >> > This length appears to be less than what the code defines (the >> > latter has offsets beyond 0x10). >> >> The higher registers (0x10 and 0x14) are only available if the >> controller is configured as GPIO output. Thus, there are two >> configuration - one that has the length 0x10 (GPIO without output >> ports), and one with the length 0x20 (GPIO with output ports). Is >> there anything I need to handle for cases like this? > > Hmm. I cannot tell whether there is a preference in mainline or > whether there is prior art of IP blocks having different sizes > depending on their "being compatible" to something or their > configuration and feature set. Others may know more. More of a question to the DT people but having the actual size of this very variant seems to fit the DT paradigm of "describing hardware" precisely. In worst case, what happens? Code dealing with an 0x10 sized controller writes into register 0x14, and instead of silently ignoring a write to the unused memory (unless it causes a bus stall!) you get a nice crash, which in this case is a feature as it helps you debug the code. So I'd say, stick the actual size in the DTS. 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
diff --git a/Documentation/devicetree/bindings/gpio/gpio-altera.txt b/Documentation/devicetree/bindings/gpio/gpio-altera.txt new file mode 100644 index 0000000..6c57d84 --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt @@ -0,0 +1,35 @@ +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: +- altr,gpio-bank-width: Width of the GPIO bank. This defines how many pins the + GPIO device has. Ranges between 1-32. +- 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. + +Example: + +gpio_altr: gpio_altr { + compatible = "altr,pio-1.0"; + reg = <0xff200000 0x10>; + interrupts = <0 45 4>; + altr,gpio-bank-width = <32>; + altr,interrupt_trigger = <0>; + #gpio-cells = <1>; + gpio-controller; + #interrupt-cells = <1>; + interrupt-controller; +}; diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 0f04444..acd67de 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 7971e36..a326fd2 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..9f4de82 --- /dev/null +++ b/drivers/gpio/gpio-altera.c @@ -0,0 +1,395 @@ +/* + * 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/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 +* @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 *irq; + 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 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) + return -ENXIO; + if (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 i; + chip->irq_mask(&desc->irq_data); + + /* 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 (BIT(i) & status) { + generic_handle_irq(irq_linear_revmap( + altera_gc->irq, 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 (BIT(i) & status) { + generic_handle_irq(irq_linear_revmap( + altera_gc->irq, i)); + } + } + } + } + + 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, "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) { + 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); + + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0); + + if (!altera_gc->mapped_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, "altr,interrupt_trigger", ®)) { + ret = -EINVAL; + pr_err("%s: interrupt_trigger value not set in device tree\n", + node->full_name); + 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->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_AUTHOR("Tien Hock Loh <thloh@altera.com>"); +MODULE_DESCRIPTION("Altera GPIO driver"); +MODULE_LICENSE("GPL");